[PATCH] D72938: Fix an assertion failure in DwarfExpression's subregister composition

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:164
+            -1, Offset - CurPos, "no DWARF register encoding"));
+      if (Offset == 0 && Size == MaxSize)
+        DwarfRegs.push_back(Register::createRegister(Reg, "sub-register"));
----------------
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.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:165
+      if (Offset == 0 && Size == MaxSize)
+        DwarfRegs.push_back(Register::createRegister(Reg, "sub-register"));
+      else
----------------
vsk wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72938/new/

https://reviews.llvm.org/D72938