[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Aleksandr Urakov via Phabricator via lldb-commits]

aleksandr.urakov marked 5 inline comments as done.
aleksandr.urakov added a comment.

Ah, yes, sure! It's my mistake. I didn't pay attention to the fact that a symtab owns symbols. I'll update the patch, thanks!



================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1344-1346
+    auto symbol_ptr = GetPublicSymbol(*pub_symbol);
+    if (!symbol_ptr)
+      continue;
----------------
clayborg wrote:
> Maybe get the file address from "pub_symbol" and avoid converting to a SymbolSP that we will never use? See more comments below.
Unfortunately there is no method of `PDBSymbolPublicSymbol` which allows to retrieve the file address directly. I'll calculate it as section + offset instead.


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1353
+
+    symtab.AddSymbol(*symbol_ptr);
+  }
----------------
clayborg wrote:
> Just make  a local symbol and convert it from PDB to lldb_private::Symbol here? Something like:
> ```
> symtab.AddSymbol(ConvertPDBSymbol(pdb_symbol));
> ```
> 
> So it seems we shouldn't need the m_public_symbols storage in this class anymore since "symtab" will now own the symbol we just created. 
The problem here is that `ConvertPDBSymbol` can fail e.g. if somehow `pub_symbol` will contain an invalid section number. We can:
- change the interface of the function to signal about errors;
- just assume that the section number is always valid (because we already retrieved it before during the file address calculation).
In both cases we will retrieve the section twice. We also can:
- just pass already calculated section and offset to `ConvertPDBSymbol`. But it leads to a blurred responsibility and a weird interface.
So I think that it would be better just create a symbol right here - it seems that it doesn't make the code more complex. What do you think about it?


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1968-1969
+
+lldb_private::Symbol *SymbolFilePDB::GetPublicSymbol(
+    const llvm::pdb::PDBSymbolPublicSymbol &pub_symbol) {
+  auto it = m_public_symbols.find(pub_symbol.getSymIndexId());
----------------
clayborg wrote:
> Maybe convert this to:
> ```
> lldb_private::Symbol ConvertPDBSymbol(const llvm::pdb::PDBSymbolPublicSymbol &pub_symbol)
> ```
> And we only call this if we need to create a symbol we will add to the "symtab" in SymbolFilePDB::AddSymbols(...)
See the comment above


https://reviews.llvm.org/D53368