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

Fri Mar 17 10:31:52 PDT 2023

JDevlieghere marked an inline comment as done.
JDevlieghere added inline comments.
Herald added a subscriber: jplehr.

================
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,
----------------
labath wrote:
> 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...
I feel pretty dumb now: looking at breakpad in more detail, the `PUBLIC` records pretty much have everything I was looking for. For some reason I was under the impression that it was a binary format and I really wanted something human readable. I think I got it confused with minidumps. 

I wasn't planning on adding anything fancy to the JSON format so I agree that if that need arises for something more advanced we should use breakpad. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145180