[ELF] .gdb_index: fix CuOff when a .debug_info section contains more than 1 CU

[MaskRay]

Summary

Idx passed to readPubNamesAndTypes was an index into Chunks, not an
index into the CU list. This would be incorrect if some .debug_info
section contained more than 1 CU.

In real world, glibc Scrt1.o is a partial link of start.os abi-note.o init.o and contains 2 CUs in debug builds.
Without this patch, any application linking such Scrt1.o would have invalid .gdb_index
The issue could be demonstrated by:

(gdb) py print(gdb.lookup_global_symbol('main'))
None

[MaskRay]

.

[MaskRay]

.

[dblaikie]

I think this would fail at least in the following example:

• Compile one file with gnu-pubnames and one withou
• ld -r the two together (non-pubnames first, then pubnames one)
• link those

I believe that'll produce a different (& incorrect) gdb-index compared to skipping the -r step and linking the two objects directly, maybe? (I haven't looked at the code carefully/completely enough to understand the CU counting aspect - but suffice to say, the linker shouldn't assume there are as many gnu_pubnames contributions/sets as there are CUs - or that they're in the same order, for that matter)

Probably the right solution is to store the Offset (DWARFDebugPubTable::Set::Offset) that describes the CU from the pubnames - then lookup that offset to determine the CU index - possibly by a search within a sorted list of CU offsets, built by a walk of the CUs in the input)

[MaskRay]

Use debug_info_offset to index into CUs

[MaskRay]

Description

[grimar]

Testcase?

[MaskRay]

Add test gdb-index-multiple-cu.s

[MaskRay]

Rebase

[ruiu]

ELF/SyntheticSections.cpp

2426–2427

Not sure if I understand this comment, because this comment explains only the tricky part of the code but doesn't explain the normal condition. I'd just explain what I represents and don't focus too much on the detail.

2429

Can you use a regular for loop instead of std::lower_bound? I think std::lower_bound with a lambda is not very easy to read because because a lambda of the third argument takes two arguments, even though Off is actually just Set.Offset in this case.

[MaskRay]

Address comments

[MaskRay]

The thing we've done in readPubNamesAndTypes (try getting the correct CuIndexInObject) is a plus. The mechanism is not clearly specified and I think gold only handles the first .debug_gnu_pubnames set and ignores the rest.

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;hb=160fc977b6f33e5ef51f6c1f3cdcb57965c522c8;f=gold/gdb-index.cc#l864

[ruiu]

LGTM

[dblaikie]

(ah, I see I hadn't posted my first feedback earlier (written but unsubmitted) - oops, sorry about that)

ELF/SyntheticSections.cpp

2432

CUs[*].CuOff is necessarily increasing, because it's the offset as the CUs are read from the input, but there's no reason to believe that Set.Offset is necessarily increasing - pubnames could be in any order relative to the CUs.

lld/trunk/ELF/SyntheticSections.cpp

2432 ↗

(On Diff #173818)

Could you fix this? This assumption seems pretty subtle/likely to break valid DWARF in surprising/hard to diagnose ways.

Moving the declaration of I into the for(Set) loop (ie: putting it just before this while loop) should suffice - and a test case with 2 pubnames in the reverse order to CUs would demonstrate the fix?

(would be nice to do a binary search rather than a linear one on the CU offsets, since they are naturally sorted by offset)

[MaskRay]

lld/trunk/ELF/SyntheticSections.cpp

2432 ↗

(On Diff #173818)

I used std::lower_bound(CUs.begin(), CUs.end(), ...) - CUs.begin()) in a former version but @ruiu thought it was too complicated...

[dblaikie]

lld/trunk/ELF/SyntheticSections.cpp

2432 ↗

(On Diff #173818)

*nod* std::lower_bound is a somewhat awkward API, but the general goal of using a binary search still seems achievable with readable/not-so-awkward code.

Having a convenient binary search API that takes a lambda/functor without also taking the comparison object, instead relying on the functor to have that parameter hardcoded/captured, seems useful. Not sure what to name it (naming it llvm::binary_search would be likely to get mixed up with the interface to std::binary_search, which has the similar API quirks to std::lower_bound, unfortunately) though.

ELF/SyntheticSections.cpp

for (std::unique_ptr<DWARFUnit> &Cu : Dwarf.compile_units()) {
     }
     ++CuIdx;
   }
   return Ret;
 }
 
 static std::vector<GdbIndexSection::NameTypeEntry>
 readPubNamesAndTypes(DWARFContext &Dwarf, uint32_t Idx) {
+  // Idx is chunk/section offset which should be converted to CU offset later.
+
   StringRef Sec1 = Dwarf.getDWARFObj().getGnuPubNamesSection();
   StringRef Sec2 = Dwarf.getDWARFObj().getGnuPubTypesSection();
 
   std::vector<GdbIndexSection::NameTypeEntry> Ret;
   for (StringRef Sec : {Sec1, Sec2}) {
     DWARFDebugPubTable Table(Sec, Config->IsLE, true);
     for (const DWARFDebugPubTable::Set &Set : Table.getData())
       for (const DWARFDebugPubTable::Entry &Ent : Set.Entries)
         Ret.push_back({{Ent.Name, computeGdbHash(Ent.Name)},
                        (Ent.Descriptor.toBits() << 24) | Idx});
   }
   return Ret;
 }
 
 // Create a list of symbols from a given list of symbol names and types
 // by uniquifying them by name.
 static std::vector<GdbIndexSection::GdbSymbol>
 createSymbols(ArrayRef<std::vector<GdbIndexSection::NameTypeEntry>> NameTypes) {
   typedef GdbIndexSection::GdbSymbol GdbSymbol;
   typedef GdbIndexSection::NameTypeEntry NameTypeEntry;
 
   // The number of symbols we will handle in this function is of the order
for (CuEntry &Cu : Chunk.CompilationUnits) {
       Buf += 16;
     }
   }
 
   // Write the address area.
   Hdr->CuTypesOff = Buf - Start;
   Hdr->AddressAreaOff = Buf - Start;
   uint32_t CuOff = 0;
+  std::vector<uint32_t> ChunkOffToCuOff;
+  ChunkOffToCuOff.reserve(Chunks.size());
   for (GdbChunk &Chunk : Chunks) {
     for (AddressEntry &E : Chunk.AddressAreas) {
       uint64_t BaseAddr = E.Section->getVA(0);
       write64le(Buf, BaseAddr + E.LowAddress);
       write64le(Buf + 8, BaseAddr + E.HighAddress);
       write32le(Buf + 16, E.CuIndex + CuOff);
       Buf += 20;
     }
+    ChunkOffToCuOff.push_back(CuOff);
     CuOff += Chunk.CompilationUnits.size();
   }
 
   // Write the on-disk open-addressing hash table containing symbols.
   Hdr->SymtabOff = Buf - Start;
   size_t SymtabSize = computeSymtabSize();
   uint32_t Mask = SymtabSize - 1;
 
void GdbIndexSection::writeTo(uint8_t *Buf) {
 
   // Write the string pool.
   Hdr->ConstantPoolOff = Buf - Start;
   parallelForEach(Symbols, [&](GdbSymbol &Sym) {
     memcpy(Buf + Sym.NameOff, Sym.Name.data(), Sym.Name.size());
   });
 
   // Write the CU vectors.
+  constexpr uint32_t CuOffMask = (1 << 24) - 1;
   for (GdbSymbol &Sym : Symbols) {
     write32le(Buf, Sym.CuVector.size());
     Buf += 4;
     for (uint32_t Val : Sym.CuVector) {
-      write32le(Buf, Val);
+      write32le(Buf, (Val & ~CuOffMask) | ChunkOffToCuOff[Val & CuOffMask]);
       Buf += 4;
     }
   }
 }
 
 bool GdbIndexSection::empty() const { return Chunks.empty(); }
 
 EhFrameHeader::EhFrameHeader()