[PATCH] D101247: [Debug-Info] strict dwarf for DW_OP_stack_value

[David Blaikie via Phabricator via llvm-commits]

dblaikie added subscribers: jhenderson, JDevlieghere.
dblaikie added a comment.

In D101247#2750145 <https://reviews.llvm.org/D101247#2750145>, @shchenz wrote:

> In D101247#2748905 <https://reviews.llvm.org/D101247#2748905>, @dblaikie wrote:
>
>> In D101247#2747318 <https://reviews.llvm.org/D101247#2747318>, @shchenz wrote:
>>
>>> In D101247#2717218 <https://reviews.llvm.org/D101247#2717218>, @dblaikie wrote:
>>>
>>>> I think this fix is too narrow/special-cased. Other places where expressions are emitted might use DW_OP_stack_value (check out all the test cases in llvm/test that mention DW_OP_stack_value - I think they're not only from global variables) - so probably the solution is to have some abstraction for emitting a DwarfExpression into a block for an attribute (if we don't have one already) and have that abstraction scan the operations to see if there's a DW_OP_stack_value, and if there is, the attribute would be skipped. Also refactoring any emission of expressions to ensure they all use such an abstraction so they go through a common codepath.
>>>
>>> Hi David @dblaikie , I did more investigation and still got the same conclusion in the description: it is hard to distinguish op `DW_OP_stack_value` from other unsigned values in location expression like offsets in the low-level/abstract API.
>>>
>>> and also since the users of `DW_OP_stack_value` are very scattered, do you think it is acceptable to just fix this special narrow case we met on AIX, current llvm code base already guards some usage of `DW_OP_stack_value` like in file `DwarfExpression.cpp`? Thanks.
>>
>> Have you tried walking through an expression with the DIExpressionCursor? Like the code in `DwarfExpression::addExpression` which iterates a DIExpressionCursor  and is able to visit each operation without muddling up the operations from their operands. Maybe that or maybe some nearby/related APIs might be useful for this task.
>
> `DIExpressionCursor`seems be able to iterate the expressions defined in IR Metadata/Debug instructions. We always initialize the `DIExpressionCursor` with a return value of function `getXXXExpr`.
>
> For now, there are two kinds of location info:
> 1: from `DIELoc` class directly
>
>   DIELoc *VBaseLocationDie = new (DIEValueAllocator) DIELoc;
>     addUInt(*VBaseLocationDie, dwarf::DW_FORM_data1, dwarf::DW_OP_dup);
>     addUInt(*VBaseLocationDie, dwarf::DW_FORM_data1, dwarf::DW_OP_deref);
>     addUInt(*VBaseLocationDie, dwarf::DW_FORM_data1, dwarf::DW_OP_constu);
>     addUInt(*VBaseLocationDie, dwarf::DW_FORM_udata, DT->getOffsetInBits());
>     addUInt(*VBaseLocationDie, dwarf::DW_FORM_data1, dwarf::DW_OP_minus);
>     addUInt(*VBaseLocationDie, dwarf::DW_FORM_data1, dwarf::DW_OP_deref);
>     addUInt(*VBaseLocationDie, dwarf::DW_FORM_data1, dwarf::DW_OP_plus);
>   
>     addBlock(MemberDie, dwarf::DW_AT_data_member_location, VBaseLocationDie);
>
> 2: from `DIEDwarfExpression DwarfExpr`. This kind can interact with `DIExpressionCursor`.
>
>   DIELoc *Loc = new (DIEValueAllocator) DIELoc;
>   DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
>   // If there is an expression, emit raw unsigned bytes.
>   DwarfExpr.addFragmentOffset(Expr);
>   DwarfExpr.addUnsignedConstant(Entry->getInt());
>   DwarfExpr.addExpression(Expr);
>   addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
>
> refactor 1: The `DIELoc` kind initialization lost the DW_OP_ info as all operations are just unsigned integers, so we need to refactor all the places which use kind 1 to kind 2, which means always passing `DwarfExpr.finalize()` to `addBlock()`.
>
> refactor 2: After using `DwarfExpr.finalize()`, we also need to use `DwarfExpr.addExpression(DIExpression)` to add a single operation, which means the only way to add an operations is to call `DwarfExpr.addExpression()`. And then maybe we can check strict DWARF in `DwarfExpr.addExpression()`
>
> refactor 3: and at last, we need to use `addBlock()` to add location-related attributes for all places.
>
> Two remaining issues for the above proposal:
> 1: Is it possible to convert `addUInt(LocDIE, form, DW_OP);` to `DwarfExpr.addExpression(Expr);`?
> 2: How to make sure location-related attributes can only be added by calling ` addBlock(DwarfExpr.finalize())`?
>
> This seems like really a big effort if the above solution makes sense.

I'm not fully understanding the specifics of the refactoring you're suggesting - but, broadly speaking, yes, that's one direction we could go.

@aprantl @probinson @JDevlieghere @jhenderson - you folks have any opinions on this? I think it might be worth pushing expression emission through some common interface - the hand-crafted expressions we emit in a few places could probably be refactored to some common structure (either a DwarfExpression, or some other thing that could adapt from both DwarfExpression and these hand crafted expressions)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101247