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

[Amy Huang via Phabricator via llvm-commits]

akhuang marked 2 inline comments as done.
akhuang 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);
+
----------------
amccarth wrote:
> 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?
Yep, it can definitely be -BySectOffset here.


================
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)) {}
+
----------------
labath wrote:
> amccarth wrote:
> > labath wrote:
> > > 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).
> > Two moves would require that `Sym` has a move constructor, right?  It looks like `Sym` is a `codeview::ProcSym`.  I suppose it might get a compiler-generated move constructor, but that class appears to be a collection of scalar types (enums, integers, and a non-owning raw pointer).
> > 
> > 
> Ah, right, sorry. In that case a const T&would be more suitable.
thanks; I think the std::move was just there because it was in the class I copied this from, but I've changed the constructor to take a `const &`


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:343
+    auto PS = cantFail(SymbolDeserializer::deserializeAs<ProcSym>(Sym));
+    if (PS.Segment == Sect && PS.CodeOffset == Offset) {
+      SymIndexId Id = createSymbol<NativeFunctionSymbol>(std::move(PS));
----------------
rnk wrote:
> We discussed adjusting this condition to be a range check.
I think I had just updated the patch to include this. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79269