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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
