JDevlieghere marked 5 inline comments as done.
JDevlieghere added inline comments.
================
Comment at: lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp:37
+ offset_t file_offset, offset_t length) {
+ if (!data_sp) {
+ data_sp = MapFileData(*file, length, file_offset);
----------------
mib wrote:
>
See my comment bellow why this is split up.
================
Comment at: lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp:47-52
+ if (data_sp->GetByteSize() < length) {
+ data_sp = MapFileData(*file, length, file_offset);
+ if (!data_sp)
+ return nullptr;
+ data_offset = 0;
+ }
----------------
mib wrote:
> Why do we do this twice ?
When we create an object file instance, we usually read the first 512 bytes to
read the magic. If we haven't read anything at all, the code above reads in the
file. If we've only read the first 512 bytes, we read the rest of the file
here.
================
Comment at: lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp:92-111
+ if (!MagicBytesMatch(data_sp, data_offset, data_sp->GetByteSize()))
+ return 0;
+
+ auto text =
+ llvm::StringRef(reinterpret_cast<const char *>(data_sp->GetBytes()));
+
+ Expected<json::Value> json = json::parse(text);
----------------
mib wrote:
> I don't see the point of re-parsing the file here, we could just save the
> parsed object as a class member.
That doesn't work because `GetModuleSpecifications` is a static method.
================
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,
----------------
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.
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
