[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 ↗

(On Diff #90016)

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 ↗

(On Diff #90016)

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 (Form == dwarf::DW_FORM_implicit_const)
+    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 (Data[i].getForm() == dwarf::DW_FORM_implicit_const)
+      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();
+  const dwarf::Attribute Attr = DW_AT_lo_user;
+  const int64_t Val1 = 42;
+  const int64_t Val2 = 43;
+
+  auto FirstVal1DIE = CUDie.addChild(DW_TAG_class_type);
+  FirstVal1DIE.addAttribute(Attr, DW_FORM_implicit_const, Val1);
+
+  auto SecondVal1DIE = CUDie.addChild(DW_TAG_class_type);
+  SecondVal1DIE.addAttribute(Attr, DW_FORM_implicit_const, Val1);
+
+  auto Val2DIE = CUDie.addChild(DW_TAG_class_type);
+  Val2DIE.addAttribute(Attr, DW_FORM_implicit_const, Val2);
+
+  MemoryBufferRef FileBuffer(DG->generate(), "dwarf");
+  auto Obj = object::ObjectFile::createObjectFile(FileBuffer);
+  EXPECT_TRUE((bool)Obj);
+  DWARFContextInMemory DwarfContext(*Obj.get());
+  DWARFCompileUnit *U = DwarfContext.getCompileUnitAtIndex(0);
+  EXPECT_TRUE((bool)U);
+
+  const auto *Abbrevs = U->getAbbreviations();
+  EXPECT_TRUE((bool)Abbrevs);
+
+  // Let's find implicit_const abbrevs and verify,
+  // that there are exactly two of them and both of them
+  // can be dumped correctly.
+  typedef decltype(Abbrevs->begin()) AbbrevIt;
+  AbbrevIt Val1Abbrev = Abbrevs->end();
+  AbbrevIt Val2Abbrev = Abbrevs->end();
+  for(auto it = Abbrevs->begin(); it != Abbrevs->end(); ++it) {
+    if (it->getNumAttributes() == 0)
+      continue; // root abbrev for DW_TAG_compile_unit
+
+    auto A = it->getAttrByIndex(0);
+    EXPECT_EQ(A, Attr);
+
+    auto FormValue = it->getAttributeValue(/* offset */ 0, A, *U);
+    EXPECT_TRUE((bool)FormValue);
+    EXPECT_EQ(FormValue->getForm(), dwarf::DW_FORM_implicit_const);
+
+    const auto V = FormValue->getAsSignedConstant();
+    EXPECT_TRUE((bool)V);
+
+    auto VerifyAbbrevDump = [&V](AbbrevIt it) {
+      std::string S;
+      llvm::raw_string_ostream OS(S);
+      it->dump(OS);
+      auto FormPos = OS.str().find("DW_FORM_implicit_const");
+      EXPECT_NE(FormPos, std::string::npos);
+      auto ValPos = S.find_first_of("-0123456789", FormPos);
+      EXPECT_NE(ValPos, std::string::npos);
+      int64_t Val = std::atoll(S.substr(ValPos).c_str());
+      EXPECT_EQ(Val, *V);
+    };
+
+    switch(*V) {
+    case Val1:
+      EXPECT_EQ(Val1Abbrev, Abbrevs->end());
+      Val1Abbrev = it;
+      VerifyAbbrevDump(it);
+      break;
+    case Val2:
+      EXPECT_EQ(Val2Abbrev, Abbrevs->end());
+      Val2Abbrev = it;
+      VerifyAbbrevDump(it);
+      break;
+    default:
+      FAIL() << "Unexpected attribute value: " << *V;
+    }
+  }
+
+  // Now let's make sure that two Val1-DIEs refer to the same abbrev,
+  // and Val2-DIE refers to another one.
+  auto DieDG = U->getUnitDIE(false);
+  auto it = DieDG.begin();
+  std::multimap<int64_t, decltype(it->getAbbreviationDeclarationPtr())> DIEs;
+  const DWARFAbbreviationDeclaration *AbbrevPtrVal1 = nullptr;
+  const DWARFAbbreviationDeclaration *AbbrevPtrVal2 = nullptr;
+  for (; it != DieDG.end(); ++it) {
+    const auto *AbbrevPtr = it->getAbbreviationDeclarationPtr();
+    EXPECT_TRUE((bool)AbbrevPtr);
+    auto FormValue = it->find(Attr);
+    EXPECT_TRUE((bool)FormValue);
+    const auto V = FormValue->getAsSignedConstant();
+    EXPECT_TRUE((bool)V);
+    switch(*V) {
+    case Val1:
+      AbbrevPtrVal1 = AbbrevPtr;
+      break;
+    case Val2:
+      AbbrevPtrVal2 = AbbrevPtr;
+      break;
+    default:
+      FAIL() << "Unexpected attribute value: " << *V;
+    }
+    DIEs.insert(std::make_pair(*V, AbbrevPtr));
+  }
+  EXPECT_EQ(DIEs.count(Val1), 2u);
+  EXPECT_EQ(DIEs.count(Val2), 1u);
+  auto Val1Range = DIEs.equal_range(Val1);
+  for (auto it = Val1Range.first; it != Val1Range.second; ++it)
+    EXPECT_EQ(it->second, AbbrevPtrVal1);
+  EXPECT_EQ(DIEs.find(Val2)->second, AbbrevPtrVal2);
+}
+
 } // end anonymous namespace