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