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

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
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)) {}
+
----------------
amccarth wrote:
> 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>`.)
This is actually the recommended way to pass objects if your function "consumes" them and you don't care about every last drop of performance. The assertion that this will always incur a copy is not true. In fact, `const T &` guarantees a copy. This puts the choice into the hands of the caller. If the caller calls the constructor with an lvalue, there will be 1 copy (+ 1 move). If the caller calls this with an xvalue, there will be no copies (only two moves).


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:427
+      uint64_t End = VA + C.Size;
+      IMap.insert(VA, End - 1, C.Imod);
+    }
----------------
akhuang wrote:
> amccarth wrote:
> > 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.
> Would it be reasonable to just check for overlap before adding to the map and ignore those?
> 
> Also, I hadn't run this on larger pdbs before but it seems like the IntervalMap sometimes segfaults on destruction (in map.visitNodes()). 
explodes == assert if you have asserts enabled, UB otherwise

You're right that this only happens if the mapped-to value differs. I didn't mention that part, but I assume this is going to be hold if we actually end up with overlapping contribution (i.e., `C.Imod` is some sort of a contribution identifier). IntervalMap does have a method for testing overlaps (it's called `overlaps`).

As for checking for overlap, I think that is definitely better than crashing. Whether that is the "reasonable" thing to do depends on the use case, which I don't know anything about.

BTW: Instead passing `End-1` you could consider instantiating the IntervalMap to use half-open intervals.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79269