Page MenuHomePhabricator

Fix an assertion failure in DwarfExpression's subregister composition

Authored by aprantl on Jan 17 2020, 10:45 AM.



This patch fixes an assertion failure in DwarfExpression that is triggered when a complex fragment has exactly
the size of a subregister of the register the DBG_VALUE points to *and* there is no DWARF encoding for the super-register.

I took the opportunity to replace/document some magic values with static constructor functions to make this code less confusing to read.


Diff Detail

Event Timeline

aprantl created this revision.Jan 17 2020, 10:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2020, 10:45 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
vsk added inline comments.Jan 17 2020, 2:21 PM

Is CurSubReg the intersection between the bits already emitted & the bits covered by the subreg, or just the bits covered by this subreg?


So, here: DwarfRegs.push_back(Register::createSubRegister(-1, Offset - CurPos, "no DWARF register encoding")

Are we saying: bits [0, Offset) of the value aren't going to be described?


This (DwarfRegs.push_back(Register::createRegister(Reg, "sub-register")) is the only functional change, right? IIUC this means that we're describing bits [0, MaxSize) of the value. Can we break out of the MCSubRegIterator loop here?




Why is the DW_OP_plus_uconst needed?

bjope added inline comments.Jan 18 2020, 4:20 AM

If you add a second test case with for example DW_OP_LLVM_fragment, 0, 56, then I guess that you still will hit the assert (you'll go into the else below due to Size>MaxSize and then you'll end up with a single sub-register. Well, I haven't tested it myself so I might be wrong.

So maybe the condition should be Size>=MaxSize. But then we end up without adding any DW_OP_piece/DW_OP_bit_piece, so we'd rely on that the DW_OP_piece is implicit if the described type is smaller than the register location. A caveat with that might be that afaik it is ABI specific which bits in the register that should be used when describing a DW_OP_piece using a register location that is larger in size (so is that true also for an implicit piece?).

On the other hand, aren't there some other troubles here (not neccessarily to be solved by this patch). We can't just generally adjust the size of the register to read based on the fragment size, right? Specially considering the addition of typed info for the DW ops.

Assume we got:

DBG_VALUE $q8, 0, !139, !DIExpression(DW_OP_shr, 32, DW_OP_LLVM_fragment, 0, 64)

It would be easy to assume that it means, read q8, shift right 32 steps, and then use the 64 least significant bits to describe the 64 bit fragment of !139. If we do DW_OP_bregx D16 we won't shift down bits from D17. Or is it clear that we only take n-bits from the register when having an n-bit fragment?

Another example could be:

DBG_VALUE $reg16bit, 0, !139, !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 8, ,DW_ATE_signed, DW_OP_stack_value, DW_OP_LLVM_fragment, 8, 8)

Here we got a 8-bit fragment being described using a 16-bit register. Not sure if we generate this kind of expressions, but my point is that the number of referenced bits on the left of the expression does not neccessarily match the size of the fragment on the right of the expression nowadays. That might be a problem.


You'd still need to update CurPos if bailing out. But we could probably bail out from the loop, in general, after the CurPos increment when CurPos >= MaxSize.

aprantl marked 4 inline comments as done.Jan 21 2020, 1:03 PM
aprantl added inline comments.

This isn't strictly necessary. It was there in the testcase I reduced this from, and I thought it was needed to go down the HasComplexExpression() path, but the test also works without it.

aprantl updated this revision to Diff 239418.Jan 21 2020, 1:18 PM
aprantl marked an inline comment as done.

Address review feedback.

Bjorn made an excellent point about operations that need access to more bits, such as DW_OP_shr. I will address that in a follow-up soon!

@aprantl Thanks for this! I think something went wrong with the latest patch you updated, I guess you posted a wrong one. :)

It might be reasonable to split this into an NFC and functional change, since we will try to address the example @bjope described as well.

aprantl updated this revision to Diff 239945.Jan 23 2020, 10:52 AM

Updated with the full patch again. Sorry!

This revision is now accepted and ready to land.Jan 27 2020, 10:06 AM
This revision was automatically updated to reflect the committed changes.