[DebugInfo] Unique abbrevs for DIEs with different implicit_const values

[vleschuk]

Summary

Take DW_FORM_implicit_const attribute value into account when profiling DIEAbbrevData.

Currently if we have two similar types with implicit_const attributes and different values we end up with only one abbrev in .debug_abbrev section. For example consider two structures: S1 with implicit_const attribute ATTR and value VAL1 and S2 with implicit_const ATTR and value VAL2. The .debug_abbrev section will contain only 1 related record:

[N] DW_TAG_structure_type       DW_CHILDREN_yes
        DW_AT_ATTR        DW_FORM_implicit_const  VAL1
        // ....

This is incorrect as struct S2 (with VAL2) will use abbrev record with VAL1.

With this patch we will have two different abbreviations here:

[N] DW_TAG_structure_type       DW_CHILDREN_yes
        DW_AT_ATTR        DW_FORM_implicit_const  VAL1
        // ....

[M] DW_TAG_structure_type       DW_CHILDREN_yes
        DW_AT_ATTR        DW_FORM_implicit_const  VAL2
        // ....

[dblaikie]

lib/CodeGen/AsmPrinter/DIE.cpp

45

Probably switch this around (don't usually see this kind of reverse comparison in the LLVM codebase):

if (Form == dwarf::DW_FORM_implicit_const)

46

Does this need an unsigned cast like the other calls in this function?

114–117

This should be tested (probably by modifying the existing test that covers implicit_const to verify the value in the abbrev printed here)

unittests/DebugInfo/DWARF/DwarfGenerator.h

135–137

This seems like a significant API change (exposing this implementation detail) & not really using this API for its original purpose (which was to generate DWARF bytes to test the DWARF parsing APIs, rather than for testing the DWARF generation APIs)

But it's not a bad test, as such... not sure what the right answer is here. Usually the DWARF generation APIs are tested in-situ (in LLVM test cases that exercise them, rather than in unit tests). It might be best to continue to stick with that for now, as much as I do like unit tests.

Well, I guess the parsing APIs need testing too, though - so if you actually make this a round-trip test (then you won't need this new getDIE API) that would be good/acceptable. You might be able to test the dumping support in the unit test too?

[vleschuk]

lib/CodeGen/AsmPrinter/DIE.cpp

45

Will do.

46

Nope, Value is signed integer (implicit_const values are always signed according to DWARF standard). And there is proper overloaded AddInteger method for signed ints.

114–117

Actually this is dump method for debugging purposes. If I am not mistaken we do not test debug helpers.

unittests/DebugInfo/DWARF/DwarfGenerator.h

135–137

Maybe it would be better to proxy AbbrevSet.uniqueAbbreviation() through dwarfgen? In this case we will not expose actual DIE object to caller. What do you think?

Also I am afraid I don't understand how to test dumping from such unit test... Could you please point me in right direction?

[vleschuk]

• Discarded changes in dwarfgen API
• Modified test: verify generated DWARF abbrevs using MemoryBuffer and verify dump format using raw_string_ostream
• Small style changes:

change

DW_FORM_implicit_const == getForm()

to

getForm() == DW_FORM_implicit_const

[dblaikie]

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1581–1628

Would it be simpler to visit the DIEs themselves, and verify their attributes represent the correct values?

(I suppose it'd be nice to verify that the two DIEs using the same implicit_const use the same abbreviation, as well)

[vleschuk]

Extended test to verify that two DIEs with equal values refer to the same abbrev, and third DIE with another value refers to another abbrev.

[vleschuk]

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1581–1628

Extended test. Left abbrev checking code just to verify that we have exact set of abbrevs that we expect.

[dblaikie]

I think the tests are probably a bit complicated/long, but should suffice.

Thanks!

[vleschuk]

r296691

lib/CodeGen/AsmPrinter/DIE.cpp

 
 /// Profile - Used to gather unique data for the abbreviation folding set.
 ///
 void DIEAbbrevData::Profile(FoldingSetNodeID &ID) const {
   // Explicitly cast to an integer type for which FoldingSetNodeID has
   // overloads.  Otherwise MSVC 2010 thinks this call is ambiguous.
   ID.AddInteger(unsigned(Attribute));
   ID.AddInteger(unsigned(Form));
+  if (dwarf::DW_FORM_implicit_const == Form)
+    ID.AddInteger(Value);
 }
 
 //===----------------------------------------------------------------------===//
 // DIEAbbrev Implementation
 //===----------------------------------------------------------------------===//
 
 /// Profile - Used to gather unique data for the abbreviation folding set.
 ///
O << "Abbreviation @"
     << " "
     << dwarf::ChildrenString(Children)
     << '\n';
 
   for (unsigned i = 0, N = Data.size(); i < N; ++i) {
     O << "  "
       << dwarf::AttributeString(Data[i].getAttribute())
       << "  "
-      << dwarf::FormEncodingString(Data[i].getForm())
-      << '\n';
+      << dwarf::FormEncodingString(Data[i].getForm());
+
+    if (dwarf::DW_FORM_implicit_const == Data[i].getForm())
+      O << " " << Data[i].getValue();
+
+    O << '\n';
   }
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 LLVM_DUMP_METHOD void DIEAbbrev::dump() {
   print(dbgs());
 }
 #endif

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

TEST(DWARFDebugInfo, TestFindAttrs) {
   // Make sure we can extract the name from the specification die when using
   // DWARFDie::findRecursively() since it should recurse through the
   // DW_AT_specification DIE.
   auto NameOpt = FuncDie.findRecursively(Attrs);
   EXPECT_TRUE(NameOpt.hasValue());
   EXPECT_EQ(DieMangled, toString(NameOpt, ""));
 }
 
+TEST(DWARFDebugInfo, TestImplicitConstAbbrevs) {
+  uint16_t Version = 5;
+
+  const uint8_t AddrSize = sizeof(void *);
+  initLLVMIfNeeded();
+  Triple Triple = getHostTripleForAddrSize(AddrSize);
+  auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
+  if (HandleExpectedError(ExpectedDG))
+    return;
+  dwarfgen::Generator *DG = ExpectedDG.get().get();
+  dwarfgen::CompileUnit &CU = DG->addCompileUnit();
+  dwarfgen::DIE CUDie = CU.getUnitDIE();
+  uint16_t Attr = DW_AT_lo_user;
+  int64_t Val1 = 42;
+  int64_t Val2 = 43;
+
+  auto &AbbrevSet = DG->getAbbrevSet();
+  auto FirstVal1DIE = CUDie.addChild(DW_TAG_class_type);
+  FirstVal1DIE.addAttribute(Attr, DW_FORM_implicit_const, Val1);
+  auto &FirstVal1Abbrev = AbbrevSet.uniqueAbbreviation(FirstVal1DIE.getDIE());
+
+  auto SecondVal1DIE = CUDie.addChild(DW_TAG_class_type);
+  SecondVal1DIE.addAttribute(Attr, DW_FORM_implicit_const, Val1);
+  auto &SecondVal1Abbrev = AbbrevSet.uniqueAbbreviation(SecondVal1DIE.getDIE());
+
+  EXPECT_EQ(FirstVal1Abbrev.getNumber(), SecondVal1Abbrev.getNumber());
+
+  auto Val2DIE = CUDie.addChild(DW_TAG_class_type);
+  Val2DIE.addAttribute(Attr, DW_FORM_implicit_const, Val2);
+  auto &Val2Abbrev = AbbrevSet.uniqueAbbreviation(Val2DIE.getDIE());
+
+  EXPECT_NE(FirstVal1Abbrev.getNumber(), Val2Abbrev.getNumber());
+}
+
 } // end anonymous namespace

unittests/DebugInfo/DWARF/DwarfGenerator.h

public:
   ///
   /// \param Tag the dwarf::Tag to assing to the llvm::DIE object.
   /// \returns the newly created DIE object that is now a child owned by this
   /// object.
   dwarfgen::DIE addChild(dwarf::Tag Tag);
   
   /// Force a DIE to say it has children even when it doesn't.
   void setForceChildren();
+
+  /// Get actual DIE object
+  llvm::DIE &getDIE() { return *Die; }
 };
 
 /// A DWARF compile unit used to generate DWARF compile/type units.
 ///
 /// Instances of these classes are created by instances of the Generator
 /// class. All information required to generate a DWARF compile unit is
 /// contained inside this class.
 class CompileUnit {