[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

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

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

85 ↗

(On Diff #339844)

When is the Attribute 0?

[shchenz]

1: update due to parent change

[shchenz]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

85 ↗

(On Diff #339844)

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

85 ↗

(On Diff #339844)

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

85 ↗

(On Diff #339844)

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

85 ↗

(On Diff #339844)

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

85 ↗

(On Diff #339844)

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

85 ↗

(On Diff #339844)

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

85 ↗

(On Diff #339844)

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 ↗

(On Diff #340320)

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 ↗

(On Diff #340320)

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 ↗

(On Diff #340320)

Thanks, will update in the commit

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

11 ↗

(On Diff #340320)

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

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDwarf.h"
 #include "llvm/MC/MCSection.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MachineLocation.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Target/TargetLoweringObjectFile.h"
+#include "llvm/Target/TargetMachine.h"
 #include <cassert>
 #include <cstdint>
 #include <string>
 #include <utility>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "dwarfdebug"
void DwarfUnit::insertDIE(const DINode *Desc, DIE *D) {
   MDNodeToDieMap.insert(std::make_pair(Desc, D));
 }
 
 void DwarfUnit::insertDIE(DIE *D) {
   MDNodeToDieMap.insert(std::make_pair(nullptr, D));
 }
 
 void DwarfUnit::addFlag(DIE &Die, dwarf::Attribute Attribute) {
+  // For strict DWARF mode, only generate attributes available to current DWARF
+  // version.
+  if (Asm->TM.Options.DebugStrictDwarf &&
+      DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+    return;
   if (DD->getDwarfVersion() >= 4)
     Die.addValue(DIEValueAllocator, Attribute, dwarf::DW_FORM_flag_present,
                  DIEInteger(1));
   else
     Die.addValue(DIEValueAllocator, Attribute, dwarf::DW_FORM_flag,
                  DIEInteger(1));
 }
 
 void DwarfUnit::addUInt(DIEValueList &Die, dwarf::Attribute Attribute,
                         Optional<dwarf::Form> Form, uint64_t Integer) {
+  // For strict DWARF mode, only generate attributes available to current DWARF
+  // version.
+  if (Asm->TM.Options.DebugStrictDwarf &&
+      DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+    return;
   if (!Form)
     Form = DIEInteger::BestForm(false, Integer);
   assert(Form != dwarf::DW_FORM_implicit_const &&
          "DW_FORM_implicit_const is used only for signed integers");
   Die.addValue(DIEValueAllocator, Attribute, *Form, DIEInteger(Integer));
 }
 
 void DwarfUnit::addUInt(DIEValueList &Block, dwarf::Form Form,
                         uint64_t Integer) {
   addUInt(Block, (dwarf::Attribute)0, Form, Integer);
 }
 
 void DwarfUnit::addSInt(DIEValueList &Die, dwarf::Attribute Attribute,
                         Optional<dwarf::Form> Form, int64_t Integer) {
+  // For strict DWARF mode, only generate attributes available to current DWARF
+  // version.
+  if (Asm->TM.Options.DebugStrictDwarf &&
+      DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+    return;
   if (!Form)
     Form = DIEInteger::BestForm(true, Integer);
   Die.addValue(DIEValueAllocator, Attribute, *Form, DIEInteger(Integer));
 }
 
 void DwarfUnit::addSInt(DIELoc &Die, Optional<dwarf::Form> Form,
                         int64_t Integer) {
   addSInt(Die, (dwarf::Attribute)0, Form, Integer);
 }
 
 void DwarfUnit::addString(DIE &Die, dwarf::Attribute Attribute,
                           StringRef String) {
   if (CUNode->isDebugDirectivesOnly())
     return;
 
+  // For strict DWARF mode, only generate attributes available to current DWARF
+  // version.
+  if (Asm->TM.Options.DebugStrictDwarf &&
+      DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+    return;
+
   if (DD->useInlineStrings()) {
     Die.addValue(DIEValueAllocator, Attribute, dwarf::DW_FORM_string,
                  new (DIEValueAllocator)
                      DIEInlineString(String, DIEValueAllocator));
     return;
   }
   dwarf::Form IxForm =
       isDwoUnit() ? dwarf::DW_FORM_GNU_str_index : dwarf::DW_FORM_strp;
else if (Index > 0xff)
       IxForm = dwarf::DW_FORM_strx2;
   }
   Die.addValue(DIEValueAllocator, Attribute, IxForm,
                DIEString(StringPoolEntry));
 }
 
 void DwarfUnit::addLabel(DIEValueList &Die, dwarf::Attribute Attribute,
                          dwarf::Form Form, const MCSymbol *Label) {
+  // For strict DWARF mode, only generate attributes available to current DWARF
+  // version.
+  if (Asm->TM.Options.DebugStrictDwarf &&
+      DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+    return;
   Die.addValue(DIEValueAllocator, Attribute, Form, DIELabel(Label));
 }
 
 void DwarfUnit::addLabel(DIELoc &Die, dwarf::Form Form, const MCSymbol *Label) {
   addLabel(Die, (dwarf::Attribute)0, Form, Label);
 }
 
 void DwarfUnit::addSectionOffset(DIE &Die, dwarf::Attribute Attribute,
void DwarfUnit::addOpAddress(DIELoc &Die, const MCSymbol *Sym) {
   }
 
   addUInt(Die, dwarf::DW_FORM_data1, dwarf::DW_OP_addr);
   addLabel(Die, dwarf::DW_FORM_addr, Sym);
 }
 
 void DwarfUnit::addLabelDelta(DIEValueList &Die, dwarf::Attribute Attribute,
                               const MCSymbol *Hi, const MCSymbol *Lo) {
+  // For strict DWARF mode, only generate attributes available to current DWARF
+  // version.
+  if (Asm->TM.Options.DebugStrictDwarf &&
+      DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+    return;
   Die.addValue(DIEValueAllocator, Attribute, dwarf::DW_FORM_data4,
                new (DIEValueAllocator) DIEDelta(Hi, Lo));
 }
 
 void DwarfUnit::addDIEEntry(DIE &Die, dwarf::Attribute Attribute, DIE &Entry) {
   addDIEEntry(Die, Attribute, DIEEntry(Entry));
 }
 
 void DwarfUnit::addDIETypeSignature(DIE &Die, uint64_t Signature) {
   // Flag the type unit reference as a declaration so that if it contains
   // members (implicit special members, static data member definitions, member
   // declarations for definitions in this CU, etc) consumers don't get confused
   // and think this is a full definition.
   addFlag(Die, dwarf::DW_AT_declaration);
 
   Die.addValue(DIEValueAllocator, dwarf::DW_AT_signature,
                dwarf::DW_FORM_ref_sig8, DIEInteger(Signature));
 }
 
 void DwarfUnit::addDIEEntry(DIE &Die, dwarf::Attribute Attribute,
                             DIEEntry Entry) {
+  // For strict DWARF mode, only generate attributes available to current DWARF
+  // version.
+  if (Asm->TM.Options.DebugStrictDwarf &&
+      DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+    return;
   const DIEUnit *CU = Die.getUnit();
   const DIEUnit *EntryCU = Entry.getEntry().getUnit();
   if (!CU)
     // We assume that Die belongs to this CU, if it is not linked to any CU yet.
     CU = getUnitDie().getUnit();
   if (!EntryCU)
     EntryCU = getUnitDie().getUnit();
   Die.addValue(DIEValueAllocator, Attribute,
                EntryCU == CU ? dwarf::DW_FORM_ref4 : dwarf::DW_FORM_ref_addr,
                Entry);
 }
 
 DIE &DwarfUnit::createAndAddDIE(unsigned Tag, DIE &Parent, const DINode *N) {
   DIE &Die = Parent.addChild(DIE::get(DIEValueAllocator, (dwarf::Tag)Tag));
   if (N)
     insertDIE(N, &Die);
   return Die;
 }
 
 void DwarfUnit::addBlock(DIE &Die, dwarf::Attribute Attribute, DIELoc *Loc) {
+  // For strict DWARF mode, only generate attributes available to current DWARF
+  // version.
+  if (Asm->TM.Options.DebugStrictDwarf &&
+      DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+    return;
   Loc->ComputeSize(Asm);
   DIELocs.push_back(Loc); // Memoize so we can call the destructor later on.
   Die.addValue(DIEValueAllocator, Attribute,
                Loc->BestForm(DD->getDwarfVersion()), Loc);
 }
 
 void DwarfUnit::addBlock(DIE &Die, dwarf::Attribute Attribute, dwarf::Form Form,
                          DIEBlock *Block) {
+  // For strict DWARF mode, only generate attributes available to current DWARF
+  // version.
+  if (Asm->TM.Options.DebugStrictDwarf &&
+      DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+    return;
   Block->ComputeSize(Asm);
   DIEBlocks.push_back(Block); // Memoize so we can call the destructor later on.
   Die.addValue(DIEValueAllocator, Attribute, Form, Block);
 }
 
 void DwarfUnit::addBlock(DIE &Die, dwarf::Attribute Attribute,
                          DIEBlock *Block) {
   addBlock(Die, Attribute, Block->BestForm(), Block);
if (!Ty->getName().empty() && !Ty->isForwardDecl()) {
     if (!Context || isa<DICompileUnit>(Context) || isa<DIFile>(Context) ||
         isa<DINamespace>(Context) || isa<DICommonBlock>(Context))
       addGlobalType(Ty, TyDIE, Context);
   }
 }
 
 void DwarfUnit::addType(DIE &Entity, const DIType *Ty,
                         dwarf::Attribute Attribute) {
+  // For strict DWARF mode, only generate attributes available to current DWARF
+  // version.
+  if (Asm->TM.Options.DebugStrictDwarf &&
+      DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+    return;
   assert(Ty && "Trying to add a type that doesn't exist?");
   addDIEEntry(Entity, Attribute, DIEEntry(*getOrCreateTypeDIE(Ty)));
 }
 
 std::string DwarfUnit::getParentContextString(const DIScope *Context) const {
   if (!Context)
     return "";
 
void DwarfTypeUnit::emitHeader(bool UseOffsets) {
   Asm->OutStreamer->emitIntValue(TypeSignature, sizeof(TypeSignature));
   Asm->OutStreamer->AddComment("Type DIE Offset");
   // In a skeleton type unit there is no type DIE so emit a zero offset.
   Asm->emitDwarfLengthOrOffset(Ty ? Ty->getOffset() : 0);
 }
 
 void DwarfUnit::addSectionDelta(DIE &Die, dwarf::Attribute Attribute,
                                 const MCSymbol *Hi, const MCSymbol *Lo) {
+  // For strict DWARF mode, only generate attributes available to current DWARF
+  // version.
+  if (Asm->TM.Options.DebugStrictDwarf &&
+      DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
+    return;
   Die.addValue(DIEValueAllocator, Attribute, DD->getDwarfSectionOffsetForm(),
                new (DIEValueAllocator) DIEDelta(Hi, Lo));
 }
 
 void DwarfUnit::addSectionLabel(DIE &Die, dwarf::Attribute Attribute,
                                 const MCSymbol *Label, const MCSymbol *Sec) {
   if (Asm->MAI->doesDwarfUseRelocationsAcrossSections())
     addLabel(Die, Attribute, DD->getDwarfSectionOffsetForm(), Label);

llvm/test/DebugInfo/PowerPC/alignment.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
-
-; 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_alignment.
+; RUN:   FileCheck %s --check-prefix=STRICT
 
 ; CHECK: DW_AT_alignment
 ; STRICT-NOT: DW_AT_alignment
 
 @_ZL3var = internal global i32 0, align 16, !dbg !0
 
 define dso_local signext i32 @_Z3foov() #0 !dbg !11 {
   %1 = load i32, i32* @_ZL3var, align 16, !dbg !15

llvm/test/DebugInfo/PowerPC/noreturn.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
-
-; 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.
+; RUN:   FileCheck %s --check-prefix=STRICT
 
 ; CHECK: DW_AT_noreturn
 ; STRICT-NOT: DW_AT_noreturn
 
 ; Function Attrs: noreturn
 define dso_local void @_Z1fv() #0 !dbg !7 {
   call void @_Z4exitv() #2, !dbg !10
   unreachable, !dbg !10