[Lldb-commits] [PATCH] D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

Tue Mar 14 09:53:15 PDT 2023

labath added inline comments.

================
Comment at: lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.cpp:112
+    symtab.AddSymbol(Symbol(
+        /*symID*/ 0, Mangled(symbol.name), eSymbolTypeCode,
+        /*is_global*/ true, /*is_debug*/ false,
----------------
clayborg wrote:
> JDevlieghere wrote:
> > mib wrote:
> > > Should we increment the `symID` here ?
> > The IDs are arbitrary, and if we start at zero, we'll have conflicts with the ones already in the symbol table (e.g. the lldb_unnamed_symbols for stripped binaries). We could check the size of the symtab and continue counting from there. Or we can use 0 like we did here to indicate that these values are "special". I went with the latter approach mostly because that's what SymbolFileBreakpad does too. 
> regardless of where the code goes that converts ObjectFileJSON::Symbol to lldb_private::Symbol, I would vote that we include enough information in ObjectFileJSON::Symbol so that we don't have to make up symbols IDs, or set all symbol IDs to zero. LLDB relies on having unique symbol IDs in each lldb_private::Symbol instance. 
Speaking of breakpad, have you considered using SymbolFileBreakpad directly? I think it serves pretty much the same purpose, and it also supports other, more advanced functionality (e.g. line tables and unwind info). It sounds like you don't need that now, but if you ever did, you wouldn't need to reinvent that logic...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145180