[PATCH] D79269: [NativeSession] Implement NativeSession::findSymbolByAddress.

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/SymbolCache.h:93
+  std::unique_ptr<PDBSymbol> findPublicSymbolByAddress(uint32_t Sect,
+                                                       uint32_t Offset);
+
----------------
In the NativeSession, we have findSymbolByAddress, -ByRVA, and -BySectOffset. Here we find -ByAddress, but is seems like it should be -BySectOffset.

I realized you inherited a bunch of the API definition, but is there a technical reason for the naming inconsistency?  Can we make it more consistent without breaking things?


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeFunctionSymbol.cpp:21
+                                           SymIndexId Id, codeview::ProcSym Sym)
+    : NativeRawSymbol(Session, PDB_SymType::Data, Id), Sym(std::move(Sym)) {}
+
----------------
The reason for the `std::move` is not obvious (to me).  The constructor takes the Sym parameter by value (so it's a copy) and then stashes it in member Sym.  Is the goal to avoid a second copy from the argument to the member?

If it's expensive to copy, should the constructor take a `const &` and then copy the value (once) into the data member?

(And if you must use `std::move`, I think this file should include `<utility>`.)


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:310
+    return nullptr;
+  }
+  }
----------------
this `}` would make more sense just above `default:`


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:427
+      uint64_t End = VA + C.Size;
+      IMap.insert(VA, End - 1, C.Imod);
+    }
----------------
labath wrote:
> IntervalMap explodes if you try to insert overlapping intervals into it. How sure are you that no input will contain overlapping addresses?
> 
> I'm not sure what's the pdb position on asserting on invalid inputs, but it certainly seems like this could happen with a hand-crafted/corrupted pdbs. However, I wouldn't be surprised if that happens for "valid" pdbs too...
Can you clarify "IntervalMap explodes"?  Crashes, hangs, uses crazy amounts of memory or time, corrupts itself?

>From the IntervalMap comments:

```
/// insert - Add a mapping of [a;b] to y, coalesce with adjacent intervals.
/// It is assumed that no key in the interval is mapped to another value, but
/// overlapping intervals already mapped to y will be coalesced.
```

If I understand that correctly, it's not overlapping intervals in general, but ones that give a different value for at least part of the overlap.  Perhaps IntervalMap needs a way to test for a conflicting overlap.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79269/new/

https://reviews.llvm.org/D79269