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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 3 inline comments as done.
JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Symbol/Symbol.h:23
+struct JSONSymbol {
+  uint64_t value;
+  std::optional<uint64_t> size;
----------------
clayborg wrote:
> Do we something that says "value is an address"? Or are we inferring that from the lldb::SymbolType?
In the Symbolc constructor that takes a JSONSymbol, I assume the value is an address if there's no section_id and an offset otherwise. We can definitely tweak that or add a sanity check based on the symbol type. 


================
Comment at: lldb/include/lldb/Symbol/Symbol.h:351-356
+bool fromJSON(const llvm::json::Value &value, lldb_private::JSONSymbol &symbol,
+              llvm::json::Path path);
+
+bool fromJSON(const llvm::json::Value &value, lldb::SymbolType &type,
+              llvm::json::Path path);
+
----------------
clayborg wrote:
> Do we want to stick with JSONSymbol or just teach lldb_private::Symbol to serialize and deserialize from JSON? If we so the latter, we can create any type of symbol we need and it would be useful for easily making unit tests that could use ObjectFileJSON as the basis. 
I think we really want the `JSONSymbol` class. That also matches what we do for the tracing objects as well, where there's a JSON object and a "real" object. For the crashlog use case where I don't have sections in the JSON, I want to be able to reuse the existing section list from the module so I need a way to pass that in. To (de)serialize the Symbol object directly, we'd need a way to pass that in, which I don't think exists today. 


================
Comment at: lldb/test/API/macosx/symbols/TestSymbolFileJSON.py:39
+            "size": main_symbol.GetSize(),
+            "value": main_symbol.addr.GetFileAddress(),
+        })
----------------
clayborg wrote:
> we probably should test that the "id" and "section_id" fields work correctly. We should also test that we are able to make symbols with only an address and name, then add tests for symbols that each add a new optional value that can be specified to ensure we can correctly make symbols. 
Yep, I saw that working manually but I can extend the test case to include that. 


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

https://reviews.llvm.org/D145180