Exposes interface to free up caching data structure in DWARFDebugLine and DWARFUnit for memory management

[florinpapa]

Summary

This is minimum changes extracted from https://reviews.llvm.org/D78950. The old patch tried to add LRU eviction of caching data structure. Due to multiple layers of interfaces that users could be using, it was not clear where to put the functionality. While we work out on where to put that functionality, it'll be great to add this minimum interface change so that the user could implement their own memory management. More specifically:

• Add a clearLineTable method for DWARFDebugLine which erases the given offset from the LineTableMap.
• DWARFDebugContext adds the clearLineTableForUnit method that leverages clearLineTable to remove the object corresponding to a given compile unit, for memory management purposes. When it is referred to again, the line table object will be repopulated.

[dblaikie]

The main motivation for going down the D78950 route was to ensure the code was testable (and preferably in a live codepath that other folks are using, etc) - so having some test coverage for this would still be necessary/highly preferred, though perhaps unit test coverage could be achieved/would suffice?

[dblaikie]

(looks like the patch description is incomplete? Seems there's a partial sentence at the end?)

[labath]

One thing to note is that a line table can be shared by multiple dwarf units. This regularly happens with type units. Theoretically, compile units can share a line table too, though that would be a pretty unusual setup...

[dblaikie]

In D90006#2367734, @labath wrote:

One thing to note is that a line table can be shared by multiple dwarf units. This regularly happens with type units. Theoretically, compile units can share a line table too, though that would be a pretty unusual setup...

FWIW, that does come up for LLVM when doing any kind of LTO + non-integrated assembler. (the assembly syntax doesn't provide a way to describe two distinct line tables, so both CUs end up having to refer to one line table)

Though in that case, using this API wouldn't break things, right? (unless they were being used concurrently, which would be problematic from the start - since the lazy-query/line table parsing API would race to begin with, I think?) It would mean that two CUs sharing a line table, if you invalidate one CU's line table (which is shared) then both/either CU, on its next line table query, would see a performance hit?

I guess the more involved alternative would be a reference counting scheme so that one could clear the line table from one CU and it would be a no-op unless all CUs sharing that line table had cleared it. I don't think that use case is necessary to support at the moment, so I'm fine with the more aggressive version that's implemented here.

[labath]

In D90006#2368823, @dblaikie wrote:

In D90006#2367734, @labath wrote:

One thing to note is that a line table can be shared by multiple dwarf units. This regularly happens with type units. Theoretically, compile units can share a line table too, though that would be a pretty unusual setup...

FWIW, that does come up for LLVM when doing any kind of LTO + non-integrated assembler. (the assembly syntax doesn't provide a way to describe two distinct line tables, so both CUs end up having to refer to one line table)

Interesting. I am moderately surprised that this does not cause any problems in the consumers. I don't know if this would actually break anything, but I believe it will cause lldb to (re)parse the line table for each CU that refers to it.

Though in that case, using this API wouldn't break things, right? (unless they were being used concurrently, which would be problematic from the start - since the lazy-query/line table parsing API would race to begin with, I think?) It would mean that two CUs sharing a line table, if you invalidate one CU's line table (which is shared) then both/either CU, on its next line table query, would see a performance hit?

Yeah, this should break anything. I don't think this needs to be a show-stopper, but I wanted to make sure you are aware of that and are fine with that kind of a performance hit.

I guess the more involved alternative would be a reference counting scheme so that one could clear the line table from one CU and it would be a no-op unless all CUs sharing that line table had cleared it. I don't think that use case is necessary to support at the moment, so I'm fine with the more aggressive version that's implemented here.

Another option would be to decouple line table and die clearing and expose separate interfaces for clearing each one -- thereby pushing the reference counting problem to the user.

[dblaikie]

In D90006#2370774, @labath wrote:

In D90006#2368823, @dblaikie wrote:

In D90006#2367734, @labath wrote:

One thing to note is that a line table can be shared by multiple dwarf units. This regularly happens with type units. Theoretically, compile units can share a line table too, though that would be a pretty unusual setup...

FWIW, that does come up for LLVM when doing any kind of LTO + non-integrated assembler. (the assembly syntax doesn't provide a way to describe two distinct line tables, so both CUs end up having to refer to one line table)

Interesting. I am moderately surprised that this does not cause any problems in the consumers. I don't know if this would actually break anything, but I believe it will cause lldb to (re)parse the line table for each CU that refers to it.

Yeah, could totally believe that. Might be unfortunate for a large full LTO binary built without the integrated assembler, but otherwise wouldn't come up.

Though in that case, using this API wouldn't break things, right? (unless they were being used concurrently, which would be problematic from the start - since the lazy-query/line table parsing API would race to begin with, I think?) It would mean that two CUs sharing a line table, if you invalidate one CU's line table (which is shared) then both/either CU, on its next line table query, would see a performance hit?

Yeah, this should break anything. I don't think this needs to be a show-stopper, but I wanted to make sure you are aware of that and are fine with that kind of a performance hit.

Fair, for sure - good to be aware/explicit about that.

I guess the more involved alternative would be a reference counting scheme so that one could clear the line table from one CU and it would be a no-op unless all CUs sharing that line table had cleared it. I don't think that use case is necessary to support at the moment, so I'm fine with the more aggressive version that's implemented here.

Another option would be to decouple line table and die clearing and expose separate interfaces for clearing each one -- thereby pushing the reference counting problem to the user.

*nod* I don't think this /probably/ merits that nuance, but certainly something to keep in mind.

[dblaikie]

@netforce is this still of interest? To you/anyone else? (just curious/not important to me personally)

[florinpapa]

Taking over this change, as it is still useful and I'd like to get it through.

[florinpapa]

Rebase against origin/master

[dblaikie]

Based on offline discussion: It'd be nice to at least have unit testing for this functionality (if/since it won't be used by llvm-symbolizer or other llvm tools) & some comments in the unit test about why it exists.

[florinpapa]

Address comments

[florinpapa]

Update test comment.

[dblaikie]

Looks OK otherwise

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

402

Could this be written as:

ASSERT_TRUE(ExpectedLineTable4);

? (similarly for other bool tests - in general, explicitly calling operator overloads is a bit "weird" and best avoided if possible - if there's an issue with gtest only testing implicit conversion, then maybe it'd be suitable to explicitly cast to bool: ASSERT_true((bool)ExpectedLineTable4))

[florinpapa]

Avoid calling operator overloads in unit test.

[florinpapa]

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

402

if there's an issue with gtest only testing implicit conversion, then maybe it'd be suitable to explicitly cast to bool: ASSERT_true((bool)ExpectedLineTable4))

How do I test that? I have been using ninja check-llvm-unit for testing so far, is that enough?

402

I had to go with the latter, as the build failed without the explicit cast.

[florinpapa]

Perform rebase.

[florinpapa]

Hello, this change was approved. Could one of the reviewers help commit this, since I don't have permissions? The failing tests look like unrelated flakes, as they are timeouts and seem to fail differently after each rebase.

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

public:
   const DWARFDebugLine::LineTable *getLineTableForUnit(DWARFUnit *U);
 
   /// Get a pointer to a parsed line table corresponding to a compile unit.
   /// Report any recoverable parsing problems using the handler.
   Expected<const DWARFDebugLine::LineTable *>
   getLineTableForUnit(DWARFUnit *U,
                       function_ref<void(Error)> RecoverableErrorHandler);
 
+  // Clear the line table object corresponding to a compile unit for memory
+  // management purpose. When it's referred to again, it'll be re-populated.
+  void clearLineTableForUnit(DWARFUnit *U);
+
   DataExtractor getStringExtractor() const {
     return DataExtractor(DObj->getStrSection(), false, 0);
   }
   DataExtractor getStringDWOExtractor() const {
     return DataExtractor(DObj->getStrDWOSection(), false, 0);
   }
   DataExtractor getLineStringExtractor() const {
     return DataExtractor(DObj->getLineStrSection(), false, 0);

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

bool lookupAddressRangeImpl(object::SectionedAddress Address, uint64_t Size,
                                 std::vector<uint32_t> &Result) const;
   };
 
   const LineTable *getLineTable(uint64_t Offset) const;
   Expected<const LineTable *>
   getOrParseLineTable(DWARFDataExtractor &DebugLineData, uint64_t Offset,
                       const DWARFContext &Ctx, const DWARFUnit *U,
                       function_ref<void(Error)> RecoverableErrorHandler);
+  void clearLineTable(uint64_t Offset);
 
   /// Helper to allow for parsing of an entire .debug_line section in sequence.
   class SectionParser {
   public:
     using LineToUnitMap = std::map<uint64_t, DWARFUnit *>;
 
     SectionParser(DWARFDataExtractor &Data, const DWARFContext &C,
                   DWARFUnitVector::iterator_range Units);

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

Expected<const DWARFDebugLine::LineTable *> DWARFContext::getLineTableForUnit(
 
   // We have to parse it first.
   DWARFDataExtractor lineData(*DObj, U->getLineSection(), isLittleEndian(),
                               U->getAddressByteSize());
   return Line->getOrParseLineTable(lineData, stmtOffset, *this, U,
                                    RecoverableErrorHandler);
 }
 
+void DWARFContext::clearLineTableForUnit(DWARFUnit *U) {
+  if (!Line)
+    return;
+
+  auto UnitDIE = U->getUnitDIE();
+  if (!UnitDIE)
+    return;
+
+  auto Offset = toSectionOffset(UnitDIE.find(DW_AT_stmt_list));
+  if (!Offset)
+    return;
+
+  uint64_t stmtOffset = *Offset + U->getLineTableOffset();
+  Line->clearLineTable(stmtOffset);
+}
+
 void DWARFContext::parseNormalUnits() {
   if (!NormalUnits.empty())
     return;
   DObj->forEachInfoSections([&](const DWARFSection &S) {
     NormalUnits.addUnitsForSection(*this, S, DW_SECT_INFO);
   });
   NormalUnits.finishedInfoUnits();
   DObj->forEachTypesSections([&](const DWARFSection &S) {

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

if (Pos.second) {
     if (Error Err =
             LT->parse(DebugLineData, &Offset, Ctx, U, RecoverableErrorHandler))
       return std::move(Err);
     return LT;
   }
   return LT;
 }
 
+void DWARFDebugLine::clearLineTable(uint64_t Offset) {
+  LineTableMap.erase(Offset);
+}
+
 static StringRef getOpcodeName(uint8_t Opcode, uint8_t OpcodeBase) {
   assert(Opcode != 0);
   if (Opcode < OpcodeBase)
     return LNStandardString(Opcode);
   return "special";
 }
 
 uint64_t DWARFDebugLine::ParsingState::advanceAddr(uint64_t OperationAdvance,

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp

 void DWARFUnit::clear() {
   Abbrevs = nullptr;
   BaseAddr.reset();
   RangeSectionBase = 0;
   LocSectionBase = 0;
   AddrOffsetSectionBase = None;
   SU = nullptr;
   clearDIEs(false);
+  AddrDieMap.clear();
+  if (DWO)
+    DWO->clear();
   DWO.reset();
 }
 
 const char *DWARFUnit::getCompilationDir() {
   return dwarf::toString(getUnitDIE().find(DW_AT_comp_dir), nullptr);
 }
 
 void DWARFUnit::extractDIEsToVector(

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

#endif
   EXPECT_FALSE(Recoverable);
   EXPECT_EQ(Expected2, *ExpectedLineTable4);
 
   // TODO: Add tests that show that the body of the programs have been read
   // correctly.
 }
 
 #ifdef _AIX
+TEST_P(DebugLineParameterisedFixture, DISABLED_ClearLineValidTable) {
+#else
+TEST_P(DebugLineParameterisedFixture, ClearLineValidTable) {
+#endif
+  if (!setupGenerator(Version))
+    GTEST_SKIP();
+
+  SCOPED_TRACE("Checking Version " + std::to_string(Version) + ", Format " +
+               (Format == DWARF64 ? "DWARF64" : "DWARF32"));
+
+  LineTable &LT = Gen->addLineTable(Format);
+  LT.addExtendedOpcode(9, DW_LNE_set_address, {{0xadd4e55, LineTable::Quad}});
+  LT.addStandardOpcode(DW_LNS_copy, {});
+  LT.addByte(0xaa);
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+  LineTable &LT2 = Gen->addLineTable(Format);
+  LT2.addExtendedOpcode(9, DW_LNE_set_address, {{0x11223344, LineTable::Quad}});
+  LT2.addStandardOpcode(DW_LNS_copy, {});
+  LT2.addByte(0xbb);
+  LT2.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+  LT2.addExtendedOpcode(9, DW_LNE_set_address, {{0x55667788, LineTable::Quad}});
+  LT2.addStandardOpcode(DW_LNS_copy, {});
+  LT2.addByte(0xcc);
+  LT2.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+  generate();
+
+  // Check that we have what we expect before calling clearLineTable().
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+                                                    nullptr, RecordRecoverable);
+  ASSERT_TRUE((bool)ExpectedLineTable);
+  EXPECT_FALSE(Recoverable);
+  const DWARFDebugLine::LineTable *Expected = *ExpectedLineTable;
+  checkDefaultPrologue(Version, Format, Expected->Prologue, 16);
+  EXPECT_EQ(Expected->Sequences.size(), 1u);
+
+  uint64_t SecondOffset =
+      Expected->Prologue.sizeofTotalLength() + Expected->Prologue.TotalLength;
+  Recoverable = Error::success();
+  auto ExpectedLineTable2 = Line.getOrParseLineTable(
+      LineData, SecondOffset, *Context, nullptr, RecordRecoverable);
+  ASSERT_TRUE((bool)ExpectedLineTable2);
+  EXPECT_FALSE(Recoverable);
+  const DWARFDebugLine::LineTable *Expected2 = *ExpectedLineTable2;
+  checkDefaultPrologue(Version, Format, Expected2->Prologue, 32);
+  EXPECT_EQ(Expected2->Sequences.size(), 2u);
+
+  // Check that we no longer get the line tables after clearLineTable().
+  Line.clearLineTable(0);
+  Line.clearLineTable(SecondOffset);
+  EXPECT_EQ(Line.getLineTable(0), nullptr);
+  EXPECT_EQ(Line.getLineTable(SecondOffset), nullptr);
+
+  // Check that if the same offset is requested, the contents match what we
+  // had before.
+  Recoverable = Error::success();
+  auto ExpectedLineTable3 = Line.getOrParseLineTable(
+      LineData, 0, *Context, nullptr, RecordRecoverable);
+  ASSERT_TRUE((bool)ExpectedLineTable3);
+  EXPECT_FALSE(Recoverable);
+  const DWARFDebugLine::LineTable *Expected3 = *ExpectedLineTable3;
+  checkDefaultPrologue(Version, Format, Expected3->Prologue, 16);
+  EXPECT_EQ(Expected3->Sequences.size(), 1u);
+
+  Recoverable = Error::success();
+  auto ExpectedLineTable4 = Line.getOrParseLineTable(
+      LineData, SecondOffset, *Context, nullptr, RecordRecoverable);
+  ASSERT_TRUE((bool)ExpectedLineTable4);
+  EXPECT_FALSE(Recoverable);
+  const DWARFDebugLine::LineTable *Expected4 = *ExpectedLineTable4;
+  checkDefaultPrologue(Version, Format, Expected4->Prologue, 32);
+  EXPECT_EQ(Expected4->Sequences.size(), 2u);
+}
+
+#ifdef _AIX
 TEST_F(DebugLineBasicFixture, DISABLED_ErrorForReservedLength) {
 #else
 TEST_F(DebugLineBasicFixture, ErrorForReservedLength) {
 #endif
   if (!setupGenerator())
     GTEST_SKIP();
 
   LineTable &LT = Gen->addLineTable();