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

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

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 to initialize the 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101247