[Debug-Info] make DIE attributes generating under control of strict dwarf

[shchenz]

Summary

We added strict dwarf support in D100826.

This is a suggestion from @probinson and @dblaikie

Make use of this flag for attribute generating.

We should also guard other DWARF info(tags, forms, loc expressions) generating under strict dwarf flag, but currently, there is no obvious way. Leave for later improvement.

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

1801–1805 ↗

(On Diff #339495)

This patch is adding this boilerplate in several places.

Perhaps it'd be cleaner to refactor the underlying "Die.addValue" call to a helper in DwarfUnit in a preparatory patch, then the boilerplate can be added to that one place?

[probinson]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

1801–1805 ↗

(On Diff #339495)

That was my first thought too, but it looks like class DIE does not know the DwarfUnit (only the DIEUnit) so it doesn't know the version or have access to the strictness flag.
But, there could be a predicate in DwarfUnit that takes the Attribute and hides the details, instead of repeating them in each of these places.

[shchenz]

1: use new wrapper function addAttribute()

[shchenz]

@dblaikie @probinson thanks for your review. I added a new wrapper function in DwarfUnit class in NFC patch D101125 and update this patch according to that NFC changes. Could you please help to have another look?

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

82

When is the Attribute 0?

[shchenz]

1: update due to parent change

[shchenz]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

82

one example:

/// Add a Dwarf expression attribute data and value.
void DwarfCompileUnit::addExpr(DIELoc &Die, dwarf::Form Form,
                               const MCExpr *Expr) {
  Die.addValue(DIEValueAllocator, (dwarf::Attribute)0, Form, DIEExpr(Expr));
}

This is not for adding an attribute but for adding an expression for an attribute. Even dwarf::AttributeVersion(0) returns 0 and we will still go to return branch, but I think it is meaningless to get a attribute version for attribute 0?

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

82

Ah, I think that's used for putting things in blocks, etc.

I suspect that not adding the attribute if the attribute is 0 is probably the wrong thing to do - perhaps you could check some test cases/try some small pieces of code to see what this breaks/omits? I think it'll end up omitting DWARF location expressions, for instance - so any non-trivial variable location would be incorrect/missing?

[shchenz]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

82

sorry, I made a mistake here. We should always call the Die.addValue()(will not go to return branch) even there is no Attribute != 0 when Attribute == 0 So I think for not a real attribute(Attribute == 0), we will always emit them.

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

82

Could you add a test case for some situation that has attribute 0 to exercise this test/show why it's the right thing to do?

[shchenz]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

82

Do you mean adding a case without changes before and after this patch? If so, I think there should be many cases in LIT already. See DwarfCompileUnit::addLocationAttribute(), in this function there are many calls to addUInt which also contains calling to 0 attribute.

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

What do you think? @dblaikie

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

82

I mean a test that specifies strict dwarf, but yes - produces the same output as without it. (or a test that specifies strict-dwarf, does have some differences, but verifies this particular thing doesn't have differences)

eg: the PowerPC/strict-dwarf.ll test could include a variable with a location, and showing the location has the correct location expression (when it would not have had that if the Attribute != 0 test was inverted or not present

[shchenz]

1: add tests for attribute 0

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

82

Ah, OK. Got your means. I add one more check point in the strict-dwarf test, does it make sense to you? Thanks. @dblaikie

[dblaikie]

Looks good, thanks!

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

85

Might be worth expanding the comment to describe the Attribute 0 case - "Attribute 0 is used when emitting form-encoded values in blocks, which don't have attributes (only forms) so we cannot detect their DWARF version compatibility here and assume they are compatible"

llvm/test/DebugInfo/PowerPC/strict-dwarf.ll

11

Rather than using a comment (or in addition to the comment, you can use a CHECK for DW_AT_name of "var" to make it clear that the location checking is for that variable. (with a CHECK-NOT: DW_TAG between the attributes to ensure the test didn't skip from one tag to another between the two attributes)

[shchenz]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

85

Thanks, will update in the commit

llvm/test/DebugInfo/PowerPC/strict-dwarf.ll

11

Thanks for the good idea. Will update this in the commit.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

 #ifndef LLVM_LIB_CODEGEN_ASMPRINTER_DWARFUNIT_H
 #define LLVM_LIB_CODEGEN_ASMPRINTER_DWARFUNIT_H
 
 #include "DwarfDebug.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "llvm/CodeGen/DIE.h"
+#include "llvm/Target/TargetMachine.h"
 #include <string>
 
 namespace llvm {
 
 class ConstantFP;
 class ConstantInt;
 class DbgVariable;
 class DwarfCompileUnit;
DwarfUnit(dwarf::Tag, const DICompileUnit *Node, AsmPrinter *A, DwarfDebug *DW,
             DwarfFile *DWU);
 
   bool applySubprogramDefinitionAttributes(const DISubprogram *SP, DIE &SPDie);
 
   bool isShareableAcrossCUs(const DINode *D) const;
 
   template <typename T>
   void addAttribute(DIEValueList &Die, dwarf::Attribute Attribute,
                     dwarf::Form Form, T &&Value) {
+    // For strict DWARF mode, only generate attributes available to current
+    // DWARF version.
+    if (Attribute != 0 && Asm->TM.Options.DebugStrictDwarf &&
+        DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+      return;
+
     Die.addValue(DIEValueAllocator,
                  DIEValue(Attribute, Form, std::forward<T>(Value)));
   }
 
 public:
   // Accessors.
   AsmPrinter* getAsmPrinter() const { return Asm; }
   MCSymbol *getEndLabel() const { return EndLabel; }

llvm/test/DebugInfo/PowerPC/strict-dwarf.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
+; RUN:   FileCheck %s -check-prefix=STRICT
 
-; FIXME: when -strict-dwarf=true is specified, we should check "STRICT" to tell
-; that with DWARF 4, we should not generate DWARF 5 attribute DW_AT_noreturn and
-; DW_AT_alignment.
+; We also check that with/without -strict-dwarf=true, the location attribute
+; is not changed. The location attribute adding will call DwarfUnit::addUInt()
+; which contains a attribute 0, we want to make sure the strict-dwarf handling
+; is also right for attribute 0.
+; For this case, the location attribute adding is for global variable @_ZL3var
+; and the call chain to addUInt() is:
+; 1: DwarfCompileUnit::addLocationAttribute()
+; 2: DwarfUnit::addOpAddress()
+; 3: DwarfUnit::addUInt()
+; 4: addUInt(Block, (dwarf::Attribute)0, Form, Integer);
 
 ; CHECK: DW_AT_alignment
+; CHECK: DW_AT_location  (DW_OP_addr 0x0)
 ; CHECK: DW_AT_noreturn
 ; STRICT-NOT: DW_AT_noreturn
 ; STRICT-NOT: DW_AT_alignment
+; STRICT: DW_AT_location  (DW_OP_addr 0x0)
 
 @_ZL3var = internal global i32 0, align 16, !dbg !0
 
 ; Function Attrs: noinline noreturn optnone uwtable mustprogress
 define dso_local void @_Z1fv() #0 !dbg !12 {
 entry:
   call void @_Z4exitv(), !dbg !15
   ret void, !dbg !16