- 
                Notifications
    You must be signed in to change notification settings 
- Fork 15k
[DebugInfo] Support to get TU for hash from .debug_types.dwo section in DWARF4. #161067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…section in Dwarf4
| @llvm/pr-subscribers-bolt @llvm/pr-subscribers-debuginfo Author: Liu Ke (Sockke) ChangesUsing the DWP's cu_index/tu_index only loads the DWO units from the .debug_info.dwo section for hash, which works fine in DWARF5. However, tu_index points to .debug_types.dwo section in DWARF4, which can cause the type unit to be lost due to the incorrect loading target. (Related discussion in 811b60f) This patch supports to get the type unit for hash from .debug_types.dwo section in DWARF4. Full diff: https://github.com/llvm/llvm-project/pull/161067.diff 3 Files Affected: 
 diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
index 964ff8e396660..3e8d7e8b32fc8 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
@@ -143,7 +143,9 @@ class DWARFUnitVector final : public SmallVector<std::unique_ptr<DWARFUnit>, 1>
       decltype(make_filter_range(std::declval<iterator_range>(), isCompileUnit));
 
   LLVM_ABI DWARFUnit *getUnitForOffset(uint64_t Offset) const;
-  LLVM_ABI DWARFUnit *getUnitForIndexEntry(const DWARFUnitIndex::Entry &E);
+  LLVM_ABI DWARFUnit *
+  getUnitForIndexEntry(const DWARFUnitIndex::Entry &E, DWARFSectionKind Sec,
+                       const DWARFSection *Section = nullptr);
 
   /// Read units from a .debug_info or .debug_types section.  Calls made
   /// before finishedInfoUnits() are assumed to be for .debug_info sections,
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFContext.cpp b/llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
index 73df62abaf023..c40042c2958e4 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
@@ -1344,9 +1344,20 @@ void DWARFContext::dump(
 DWARFTypeUnit *DWARFContext::getTypeUnitForHash(uint64_t Hash, bool IsDWO) {
   DWARFUnitVector &DWOUnits = State->getDWOUnits();
   if (const auto &TUI = getTUIndex()) {
-    if (const auto *R = TUI.getFromHash(Hash))
-      return dyn_cast_or_null<DWARFTypeUnit>(
-          DWOUnits.getUnitForIndexEntry(*R));
+    if (const auto *R = TUI.getFromHash(Hash)) {
+      if (TUI.getVersion() >= 5)
+        return dyn_cast_or_null<DWARFTypeUnit>(
+            DWOUnits.getUnitForIndexEntry(*R, DW_SECT_INFO));
+      else {
+        DWARFUnit *TypesUnit = nullptr;
+        getDWARFObj().forEachTypesDWOSections([&](const DWARFSection &S) {
+          if (!TypesUnit)
+            TypesUnit =
+                DWOUnits.getUnitForIndexEntry(*R, DW_SECT_EXT_TYPES, &S);
+        });
+        return dyn_cast_or_null<DWARFTypeUnit>(TypesUnit);
+      }
+    }
     return nullptr;
   }
   return State->getTypeUnitMap(IsDWO).lookup(Hash);
@@ -1358,7 +1369,7 @@ DWARFCompileUnit *DWARFContext::getDWOCompileUnitForHash(uint64_t Hash) {
   if (const auto &CUI = getCUIndex()) {
     if (const auto *R = CUI.getFromHash(Hash))
       return dyn_cast_or_null<DWARFCompileUnit>(
-          DWOUnits.getUnitForIndexEntry(*R));
+          DWOUnits.getUnitForIndexEntry(*R, DW_SECT_INFO));
     return nullptr;
   }
 
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index ef59c82fc6a01..da0bf03e1ac57 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -161,17 +161,24 @@ DWARFUnit *DWARFUnitVector::getUnitForOffset(uint64_t Offset) const {
   return nullptr;
 }
 
-DWARFUnit *
-DWARFUnitVector::getUnitForIndexEntry(const DWARFUnitIndex::Entry &E) {
-  const auto *CUOff = E.getContribution(DW_SECT_INFO);
+DWARFUnit *DWARFUnitVector::getUnitForIndexEntry(const DWARFUnitIndex::Entry &E,
+                                                 DWARFSectionKind Sec,
+                                                 const DWARFSection *Section) {
+  const auto *CUOff = E.getContribution(Sec);
   if (!CUOff)
     return nullptr;
 
   uint64_t Offset = CUOff->getOffset();
-  auto end = begin() + getNumInfoUnits();
+  auto begin = this->begin();
+  auto end = begin + getNumInfoUnits();
+
+  if (Sec == DW_SECT_EXT_TYPES) {
+    begin = end;
+    end = this->end();
+  }
 
   auto *CU =
-      std::upper_bound(begin(), end, CUOff->getOffset(),
+      std::upper_bound(begin, end, CUOff->getOffset(),
                        [](uint64_t LHS, const std::unique_ptr<DWARFUnit> &RHS) {
                          return LHS < RHS->getNextUnitOffset();
                        });
@@ -181,13 +188,14 @@ DWARFUnitVector::getUnitForIndexEntry(const DWARFUnitIndex::Entry &E) {
   if (!Parser)
     return nullptr;
 
-  auto U = Parser(Offset, DW_SECT_INFO, nullptr, &E);
+  auto U = Parser(Offset, Sec, Section, &E);
   if (!U)
     return nullptr;
 
   auto *NewCU = U.get();
   this->insert(CU, std::move(U));
-  ++NumInfoUnits;
+  if (Sec == DW_SECT_INFO)
+    ++NumInfoUnits;
   return NewCU;
 }
 
 | 
| Can you add a test case? My rudimentary testing seems to show this working correctly already - but perhaps by accident or something? Specifically, when dumping the CU it shows the name of the type at offset 0x30, which indicates that it's successfully looking through the  | 
| 
 Thanks for your review, I have added a test. 
 Using the TU index for type parsing in dwp may cause problems, like: After applying this patch: Btw, this bug also cause llvm-bolt to lose the .debug_types.dwo information in its output when parsing a dwp file. | 
| auto end = begin + getNumInfoUnits(); | ||
|  | ||
| if (Sec == DW_SECT_EXT_TYPES) { | ||
| begin = end; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite follow this logic.
For DWARF4 .debug_types only has TUs.
For DWARF5 TUs can be anywhere in the .debug_info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For DWARF4 TUs are stored after CUs in DWOUnits vector:
  /// Get units from .debug_types.dwo in the DWO context.
  unit_iterator_range dwo_types_section_units() {
    DWARFUnitVector &DWOUnits = State->getDWOUnits();
    return unit_iterator_range(DWOUnits.begin() + DWOUnits.getNumInfoUnits(),
                               DWOUnits.end());
  }
DW_SECT_EXT_TYPES indicates that DWARF4 .debug_types is being processed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that part.
I was wondering about this:
begin = end;
I guess getNumInfoUnits() will return 0 since types are in a separate section for DWARF4. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it will return the number of CUs from the DWGUnits vector. For DWARF4, since TUs are stored after the CUs in the DWGUnits vector, we need to traverse the TUs starting from the end of the CUs.
| ✅ With the latest revision this PR passed the C/C++ code formatter. | 
f835b03    to
    ba88da1      
    Compare
  
    ba88da1    to
    3d5ec57      
    Compare
  
    | Ah OK. Looks reasonable to me. | 
| LLVM Buildbot has detected a new failure on builder  Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/24607 Here is the relevant piece of the build log for the reference | 
| LLVM Buildbot has detected a new failure on builder  Full details are available at: https://lab.llvm.org/buildbot/#/builders/94/builds/12140 Here is the relevant piece of the build log for the reference | 
| LLVM Buildbot has detected a new failure on builder  Full details are available at: https://lab.llvm.org/buildbot/#/builders/38/builds/6073 Here is the relevant piece of the build log for the reference | 
The test added in #161067 writes artifacts to the current dir, i.e. `test.o` / `test.dwo` / `test.dwp`, which might not be writeable. Tests should use `%t` for test artifact location, i.e. `%t.o` / `%t.dwo` / `%t.dwp` However, since `"test.dwo"` is part of the assembly source file used as a test input, and that's not something lit will substitute, that typical approach doesn't work. We can instead ensure the output is in a good location by running `cd %t` (after setting it up).
…in DWARF4. (llvm#161067) Using the DWP's cu_index/tu_index only loads the DWO units from the .debug_info.dwo section for hash, which works fine in DWARF5. However, tu_index points to .debug_types.dwo section in DWARF4, which can cause the type unit to be lost due to the incorrect loading target. (Related discussion in [811b60f](llvm@811b60f)) This patch supports to get the type unit for hash from .debug_types.dwo section in DWARF4.
The test added in llvm#161067 writes artifacts to the current dir, i.e. `test.o` / `test.dwo` / `test.dwp`, which might not be writeable. Tests should use `%t` for test artifact location, i.e. `%t.o` / `%t.dwo` / `%t.dwp` However, since `"test.dwo"` is part of the assembly source file used as a test input, and that's not something lit will substitute, that typical approach doesn't work. We can instead ensure the output is in a good location by running `cd %t` (after setting it up).
…in DWARF4. (llvm#161067) Using the DWP's cu_index/tu_index only loads the DWO units from the .debug_info.dwo section for hash, which works fine in DWARF5. However, tu_index points to .debug_types.dwo section in DWARF4, which can cause the type unit to be lost due to the incorrect loading target. (Related discussion in [811b60f](llvm@811b60f)) This patch supports to get the type unit for hash from .debug_types.dwo section in DWARF4.
The test added in llvm#161067 writes artifacts to the current dir, i.e. `test.o` / `test.dwo` / `test.dwp`, which might not be writeable. Tests should use `%t` for test artifact location, i.e. `%t.o` / `%t.dwo` / `%t.dwp` However, since `"test.dwo"` is part of the assembly source file used as a test input, and that's not something lit will substitute, that typical approach doesn't work. We can instead ensure the output is in a good location by running `cd %t` (after setting it up).
Using the DWP's cu_index/tu_index only loads the DWO units from the .debug_info.dwo section for hash, which works fine in DWARF5. However, tu_index points to .debug_types.dwo section in DWARF4, which can cause the type unit to be lost due to the incorrect loading target. (Related discussion in 811b60f)
This patch supports to get the type unit for hash from .debug_types.dwo section in DWARF4.