[llvm] [PDB] Refactor cache strategy for function symbol lookups (PR #188927)

[via llvm-commits]

================
@@ -355,29 +366,67 @@ SymbolCache::findFunctionSymbolBySectOffset(uint32_t Sect, uint32_t Offset) {
     consumeError(ExpectedModS.takeError());
     return nullptr;
   }
-  CVSymbolArray Syms = ExpectedModS->getSymbolArray();
 
-  // Search for the symbol in this module.
+  // Return empty intervals in AddressToSymbolId from Start to Stop.
+  auto getInsertRanges = [this](uint64_t Start, uint64_t Stop) {
+    SmallVector<std::pair<uint64_t, uint64_t>> Ranges;
+    auto Iter = AddressToSymbolId.find(Start);
+    while (Iter.valid() && IMapTy::KeyTraits::nonEmpty(Start, Stop)) {
+      if (IMapTy::KeyTraits::startLess(Start, Iter.start()))
+        Ranges.push_back({Start, std::min(Iter.start(), Stop)});
+
+      // Same result as Start = std::min(Stop, Iter.stop()).
+      Start = Iter.stop();
+      ++Iter;
+    }
+    if (IMapTy::KeyTraits::nonEmpty(Start, Stop))
+      Ranges.push_back({Start, Stop});
+
+    return Ranges;
+  };
+
+  // Decode symbols in this module.
+  CVSymbolArray Syms = ExpectedModS->getSymbolArray();
   for (auto I = Syms.begin(), E = Syms.end(); I != E; ++I) {
     if (I->kind() != S_LPROC32 && I->kind() != S_GPROC32)
       continue;
+
     auto PS = cantFail(SymbolDeserializer::deserializeAs<ProcSym>(*I));
-    if (Sect == PS.Segment && Offset >= PS.CodeOffset &&
-        Offset < PS.CodeOffset + PS.CodeSize) {
-      // Check if the symbol is already cached.
-      auto Found = AddressToSymbolId.find({PS.Segment, PS.CodeOffset});
-      if (Found != AddressToSymbolId.end())
-        return getSymbolById(Found->second);
-
-      // Otherwise, create a new symbol.
+    uint64_t SymStart = Session.getVAFromSectOffset(PS.Segment, PS.CodeOffset);
+    uint64_t SymStop = SymStart + PS.CodeSize;
+    if (LLVM_UNLIKELY(!IMapTy::KeyTraits::nonEmpty(SymStart, SymStop))) {
+      I = Syms.at(PS.End);
+      continue;
+    }
+#ifndef NDEBUG
+    const auto &AddrToModuleIndex = Session.getAddrToModuleIndex();
+    auto ModIter = AddrToModuleIndex.find(SymStart);
+    using ModKeyTraits =
+        std::remove_reference_t<decltype(AddrToModuleIndex)>::KeyTraits;
+    assert(ModIter.valid() &&
+           !ModKeyTraits::startLess(SymStart, ModIter.start()) &&
+           !ModKeyTraits::stopLess(ModIter.stop(), SymStop - 1) &&
+           "Symbol spans modules");
----------------
Nerixyz wrote:

> If a module have non-contiguous range, then the current implementation can check it.

If a module has gaps, it would have two entries. For example, module 1 might have the ranges `[10, 14), [16, 20)`. So in the map, we'd have the entries `[10, 14) => 1, [16, 20) => 1`. If we'd have an entry with start=12, size=6, this would fail. I don't think this can happen, though.

This is checking that the function is contained in a contiguous span from one module.

My issue with the check is that it needs `getAddrToModuleIndex` from the other class. As an alternative, you could add a method that performs this lookup. Not sure how useful that would be.


https://github.com/llvm/llvm-project/pull/188927