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

[Jonas Devlieghere via Phabricator via lldb-commits]

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