[Debug-Info] strict dwarf for DW_OP_stack_value

[shchenz]

Summary

This is the last patch for strict dwarf we found on AIX.

This is for the location expression DW_OP_stack_value, we found this operation is emitted in DWARF 3.

I tried to check this in a low level interface, for example, if we found an attribute is DW_AT_location, then we go through all its location values to find unmatched location expressions. But seems it can not distinguish the operations with other unsigned consts like offsets for some operations.

Now DW_OP_stack_value is added to location attribute as an unsigned value. See:

addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_stack_value);

void DwarfUnit::addUInt(DIEValueList &Block, dwarf::Form Form,
                        uint64_t Integer) {
  addUInt(Block, (dwarf::Attribute)0, Form, Integer);
}

There may be some other const unsigned value for the location attribute, like:

void DwarfExpression::addOpPiece(unsigned SizeInBits, unsigned OffsetInBits) {
    emitOp(dwarf::DW_OP_bit_piece);
    emitUnsigned(SizeInBits);
    emitUnsigned(OffsetInBits);
}

void DIEDwarfExpression::emitUnsigned(uint64_t Value) {
  CU.addUInt(getActiveDIE(), dwarf::DW_FORM_udata, Value);
}

So it is hard to tell in the low-level API if the unsigned integer is from a DW_OP_ or from an unsigned const like above offsets.

I also went through the places where we generate DW_OP_stack_value in llc, it includes 3 places:
1: in DwarfCompileUnit.cpp, it is generated for Wasm target, so I don't change it.
2: in DwarfExpression.cpp, all the places in this file are guarded by DWARF 4. I saw the same strict DWARF check in this file. See https://github.com/llvm/llvm-project/blob/c3f95e9197643b699b891ca416ce7d72cf89f5fc/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp#L314-L322
3: in DwarfUnit.cpp, this is what I fixed in this patch.

[dblaikie]

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.

[shchenz]

In 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.

Thanks, David, just back from a vacation. I will have a check if we can abstract some logic here.

[shchenz]

In 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.

[dblaikie]

In D101247#2747318, @shchenz wrote:

In 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.

[shchenz]

In D101247#2748905, @dblaikie wrote:

In D101247#2747318, @shchenz wrote:

In 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.

DIExpressionCursorseems 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.

[dblaikie]

In D101247#2750145, @shchenz wrote:

In D101247#2748905, @dblaikie wrote:

In D101247#2747318, @shchenz wrote:

In 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.

DIExpressionCursorseems 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)

[jhenderson]

In D101247#2752337, @dblaikie wrote:

@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)

I'm afraid the DwarfExpression and related code is not something I'm really familiar with, so can't provide a useful opinion.

[jmorse]

Hi @shchenz, is it feasible to use DIExpression::isImplicit at a higher level to filter out these kinds of expressions? I believe it's possible to parse a DIExpression before it's lowered to a DWARF expression, and there are places where we rely on the isImplicit method for correctness already. Also paging @StephenTozer who's touched this area recently.

I believe this would work for DW_AT_location attributes of DW_TAG_variable and DW_TAG_formal_parameter's, I'm less certain about other parts of DWARF that use expressions.

[shchenz]

In D101247#2763927, @jmorse wrote:

Hi @shchenz, is it feasible to use DIExpression::isImplicit at a higher level to filter out these kinds of expressions? I believe it's possible to parse a DIExpression before it's lowered to a DWARF expression, and there are places where we rely on the isImplicit method for correctness already. Also paging @StephenTozer who's touched this area recently.

I believe this would work for DW_AT_location attributes of DW_TAG_variable and DW_TAG_formal_parameter's, I'm less certain about other parts of DWARF that use expressions.

Hi @jmorse , first of all, thanks very much for your comment. Yes, DIExpression::isImplicit() should work to check if a DW_AT_location attribute has a DW_OP_stack_value operation. But this will only work if the location-related attributes are added by DIEDwarfExpression.addExpression(), right? There are also some other places that add location-related attributes not involving DIEDwarfExpression. See the below example:

DIELoc *Loc = new (DIEValueAllocator) DIELoc;
addOpAddress(*Loc, Asm->getSymbol(GV));
// Emit DW_OP_stack_value to use the address as the immediate value of
// the parameter, rather than a pointer to it.
addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_stack_value);
addBlock(ParamDIE, dwarf::DW_AT_location, Loc);

We need a refactor here to change all places that call addBlock(, location_attr, ) to use DIEDwarfExpression, so that we have a common interface to add location-related attributes.
To achieve this goal, we also need some other refactors, see comment https://reviews.llvm.org/D101247#2750145

[jmorse]

We need a refactor here to change all places that call addBlock(, location_attr, ) to use DIEDwarfExpression

Ah, I get it, it's the irregular parts of the DWARF emitting code that require maintenance. Putting everything through a common interface makes sense to me; although I'm no expert on DwarfDebug.cpp and nearby regions of code.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

void DwarfUnit::constructTemplateValueParameterDIE(
   if (!VP->getName().empty())
     addString(ParamDIE, dwarf::DW_AT_name, VP->getName());
   if (VP->isDefault() && (DD->getDwarfVersion() >= 5))
     addFlag(ParamDIE, dwarf::DW_AT_default_value);
   if (Metadata *Val = VP->getValue()) {
     if (ConstantInt *CI = mdconst::dyn_extract<ConstantInt>(Val))
       addConstantValue(ParamDIE, CI, VP->getType());
     else if (GlobalValue *GV = mdconst::dyn_extract<GlobalValue>(Val)) {
+      // We need DW_OP_stack_value to represent the value of the global value.
+      // DW_OP_stack_value was introduced in DWARF 4, in strict dwarf mode, we
+      // should not use this expression if DWARF version is smaller than 4.
+      if (Asm->TM.Options.DebugStrictDwarf && DD->getDwarfVersion() < 4)
+        return;
       // We cannot describe the location of dllimport'd entities: the
       // computation of their address requires loads from the IAT.
       if (!GV->hasDLLImportStorageClass()) {
         // For declaration non-type template parameters (such as global values
         // and functions)
         DIELoc *Loc = new (DIEValueAllocator) DIELoc;
         addOpAddress(*Loc, Asm->getSymbol(GV));
         // Emit DW_OP_stack_value to use the address as the immediate value of
         // the parameter, rather than a pointer to it.
         addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_stack_value);
         addBlock(ParamDIE, dwarf::DW_AT_location, Loc);

llvm/test/DebugInfo/PowerPC/strict-dwarf-op-stack-value.ll

+; RUN: llc -filetype=obj -mtriple=powerpc64le-unknown-linux-gnu < %s | \
+; RUN:   llvm-dwarfdump -debug-info - | FileCheck %s
+; RUN: llc -filetype=obj -mtriple=powerpc64le-unknown-linux-gnu \
+; RUN:   -strict-dwarf=true < %s | llvm-dwarfdump -debug-info - | \
+; RUN:   FileCheck %s -check-prefix=STRICT
+
+; check that DWARF 4 location expression DW_OP_stack_value is not emitted in
+; DWARF 3 at strict dwarf mode.
+
+; CHECK: DW_AT_location (DW_OP_addr 0x0, DW_OP_stack_value)
+; STRICT-NOT: DW_OP_stack_value
+
+; $ cat 1.cpp
+; extern bool cmp();
+;
+; typedef bool comparator_function();
+; template<class Iterator, comparator_function compare>
+; void quicksort(Iterator begin, Iterator end)
+; {
+; }
+; void foo(void)
+; {
+;   double a[100];
+;   quicksort<double *, cmp>(a, a + 100);
+; }
+
+
+$_Z9quicksortIPdXadL_Z3cmpvEEEvT_S1_ = comdat any
+
+; Function Attrs: noinline optnone uwtable mustprogress
+define dso_local void @_Z3foov() #0 !dbg !8 {
+entry:
+  %a = alloca [100 x double], align 8
+  call void @llvm.dbg.declare(metadata [100 x double]* %a, metadata !11, metadata !DIExpression()), !dbg !16
+  %arraydecay = getelementptr inbounds [100 x double], [100 x double]* %a, i64 0, i64 0, !dbg !17
+  %arraydecay1 = getelementptr inbounds [100 x double], [100 x double]* %a, i64 0, i64 0, !dbg !18
+  %add.ptr = getelementptr inbounds double, double* %arraydecay1, i64 100, !dbg !19
+  call void @_Z9quicksortIPdXadL_Z3cmpvEEEvT_S1_(double* %arraydecay, double* %add.ptr), !dbg !20
+  ret void, !dbg !21
+}
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: noinline nounwind optnone uwtable mustprogress
+define linkonce_odr dso_local void @_Z9quicksortIPdXadL_Z3cmpvEEEvT_S1_(double* %begin, double* %end) #2 comdat !dbg !22 {
+entry:
+  %begin.addr = alloca double*, align 8
+  %end.addr = alloca double*, align 8
+  store double* %begin, double** %begin.addr, align 8
+  call void @llvm.dbg.declare(metadata double** %begin.addr, metadata !33, metadata !DIExpression()), !dbg !34
+  store double* %end, double** %end.addr, align 8
+  call void @llvm.dbg.declare(metadata double** %end.addr, metadata !35, metadata !DIExpression()), !dbg !36
+  ret void, !dbg !37
+}
+
+declare zeroext i1 @_Z3cmpv() #3
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 13.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "1.cpp", directory: "./")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 3}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{i32 7, !"uwtable", i32 1}
+!7 = !{!"clang version 13.0.0"}
+!8 = distinct !DISubprogram(name: "foo", linkageName: "_Z3foov", scope: !1, file: !1, line: 9, type: !9, scopeLine: 10, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!9 = !DISubroutineType(types: !10)
+!10 = !{null}
+!11 = !DILocalVariable(name: "a", scope: !8, file: !1, line: 11, type: !12)
+!12 = !DICompositeType(tag: DW_TAG_array_type, baseType: !13, size: 6400, elements: !14)
+!13 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
+!14 = !{!15}
+!15 = !DISubrange(count: 100)
+!16 = !DILocation(line: 11, column: 10, scope: !8)
+!17 = !DILocation(line: 12, column: 28, scope: !8)
+!18 = !DILocation(line: 12, column: 31, scope: !8)
+!19 = !DILocation(line: 12, column: 33, scope: !8)
+!20 = !DILocation(line: 12, column: 3, scope: !8)
+!21 = !DILocation(line: 13, column: 1, scope: !8)
+!22 = distinct !DISubprogram(name: "quicksort<double *, &cmp>", linkageName: "_Z9quicksortIPdXadL_Z3cmpvEEEvT_S1_", scope: !1, file: !1, line: 6, type: !23, scopeLine: 7, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, templateParams: !26, retainedNodes: !2)
+!23 = !DISubroutineType(types: !24)
+!24 = !{null, !25, !25}
+!25 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 64)
+!26 = !{!27, !28}
+!27 = !DITemplateTypeParameter(name: "Iterator", type: !25)
+!28 = !DITemplateValueParameter(name: "compare", type: !29, value: i1 ()* @_Z3cmpv)
+!29 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !30, size: 64)
+!30 = !DISubroutineType(types: !31)
+!31 = !{!32}
+!32 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!33 = !DILocalVariable(name: "begin", arg: 1, scope: !22, file: !1, line: 6, type: !25)
+!34 = !DILocation(line: 6, column: 25, scope: !22)
+!35 = !DILocalVariable(name: "end", arg: 2, scope: !22, file: !1, line: 6, type: !25)
+!36 = !DILocation(line: 6, column: 41, scope: !22)
+!37 = !DILocation(line: 8, column: 1, scope: !22)