Page MenuHomePhabricator

Handle complex DWARF expressions in combination with "complex" registers
Needs ReviewPublic

Authored by aprantl on Jan 23 2020, 10:54 AM.

Details

Summary

Thanks to @bjope for pointing out this problem!

Here's my proposed solution:

If the register itself can only be described by combining multiple subregisters, we cannot use DW_OP_piece, since it doesn't safely compose with another complex expression, Since it is not possible to apply any DWARF operation to the combined DW_OP_pieces. By manualy shifting the subregisters into place, this can be worked around at the expense of a slightly larger encoding.

Diff Detail

Event Timeline

aprantl created this revision.Jan 23 2020, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2020, 10:54 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
aprantl updated this revision to Diff 239946.Jan 23 2020, 10:57 AM
aprantl marked an inline comment as done.Jan 23 2020, 1:40 PM
aprantl added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
313

One thing I don't like about this myself is that this is a special case of the code in addExpression().

aprantl marked an inline comment as done.Jan 23 2020, 1:42 PM
aprantl added inline comments.
llvm/test/DebugInfo/MIR/ARM/split-superreg-complex.mir
1

I'd appreciate someone double-checking my reasoning here; but I do think that in the case of a DBG_VALUE tied to a single register, which doesn't have a DWARF number of its own, this is okay.

aemerson added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
225

I think you can use llvm::scope_exit for this kind of thing?

aprantl updated this revision to Diff 240705.Jan 27 2020, 3:43 PM

Use llvm::scope_exit as suggested by Amara. Thanks, I was looking for something like that!

aprantl marked an inline comment as done.Jan 27 2020, 3:43 PM
dstenb added a subscriber: dstenb.Jan 28 2020, 8:28 AM
dstenb added inline comments.
llvm/test/DebugInfo/MIR/ARM/subregister-complex-expression.mir
12

I don't have much knowledge about AArch64 so I might be overlooking something here, but I wonder if this expression really is valid, given the size of an element on the DWARF expression stack. The size of the elements on the stack is the size of an address on the target machine, which I assume is 64 bits on AArch64? DWARFv4 (and DWARFv5 for the generic type) specifies that the result of DW_OP_shl is truncated to fit in an element (modulo one plus the largest representable address), so it seems that the result of (D17 << 64) should be 0, and we'll just end up with the value of (D16 >> 32)?

Is this a case where we need to use DWARFv5's type operations (e.g. using DW_OP_regval_type to push the registers) to allow for the operations to be performed on a 128-bit base type?

I think there in general are quite a lot of cases where LLVM needs to consider the size of the elements on the expression stack in which it currently doesn't.

aprantl updated this revision to Diff 241034.Jan 28 2020, 5:30 PM
dstenb added inline comments.Jan 29 2020, 2:22 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
241

Nit: s/manualy/manually/

246

Nit: s/in are/in the registers are/

250

Shouldn't there be a version check so that we don't emit DW_OP_regval_type when targeting earlier DWARF standards?

if (Large && DwarfVersion < 5)
  return false;

(Should we perhaps consider using the GNU extension, DW_OP_GNU_regval_type, for earlier versions?)

406

Can this change be moved to a preceding NFC commit to reduce this patch's size?

djtodoro added inline comments.Jan 29 2020, 2:43 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
250

(Should we perhaps consider using the GNU extension, DW_OP_GNU_regval_type, for earlier versions?)

+1
I don't see any obstacle for that. We can extend the DwarfCompileUnit::getDwarf5OrGNULocationAtom() supporting the location atom as well.

dstenb added inline comments.Jan 29 2020, 7:19 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
260–261

The DWARFv5 standard specifies that all operands of logical and arithmetic operations must have the same type, so I think that the shift amount operand must use the same base type as the registers here.

The same issue also exists for the complex expressions that are applied to this function's result, for example the DW_OP_shr in the attached test case.

There already exists similar issues for DW_OP_LLVM_convert. I recently discovered a case for our downstream target where the following was emitted:

DW_OP_bregx 0xf0 +0, DW_OP_convert (0x0000017b) "DW_ATE_unsigned_32", DW_OP_convert (0x00000191) "DW_ATE_unsigned_16", DW_OP_lit31, DW_OP_and, DW_OP_stack_value

The DW_OP_and is done with one unsigned 16-bit base type operand, and one operand of the generic type. GDB throws an Incompatible types on DWARF stack error when trying to print that variable. I'll see if I can create a downstream reproducer for that and write a Bugzilla ticket.

I guess DwarfExpression in some way must keep track of the element types.

I'd prefer if you didn't mix refactoring and functional changes (assuming that the changes to the DwarfExpression::Register struct could be done as an NFC regardless of what happens to this patch). Currently I think this is both a little bit hard to review, and it will become a mess to integrate downstream (even if that is hard for you to predict).

We got a couple of special cases that we currently need to handle downstream in addMachineRegExpression involving types that aren't a multiple of the byte size (we need to pad with extra pieces to satisfy the debugger). And for some reason we also need to reverse the order of all pieces since they should describe the variable in increasing memory order, and at least for our big-endian target (together with the iteration order of MCSubRegIterator) we get things in the wrong order unless we reverse the DwarfReg vector at the end of addMachineReg. Well, this is hopefully nothing that you need to care about, but right now it is hard for me to see how things would work when this patch has landed. And temporarily maintaining two implemenations would also be hard if the framework is refactored/renamed as part of the patch.

Sorry if I misunderstood something (if the DwarfExpression::Register changes wouldn't make sense at all as a standalone patch).

aprantl updated this revision to Diff 241238.Jan 29 2020, 11:32 AM

Factored out the NFC bits. Have not yet addressed @dstenb 's comment yet.