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

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,

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

labath added inline comments. 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, clayborg wrote: >

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGcf3524a5746f: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON (authored by JDevlieghere). Changed prior to commit: https://reviews.llvm.org/D145180?vs=503575=503611#toc Repository: rG LLVM

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

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Thanks for the changes! LGTM. Just one missed EXPECT_THAT_EXPECTED, but accepted. Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:144-145 + Expected symbol =

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

JDevlieghere updated this revision to Diff 503575. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145180/new/ https://reviews.llvm.org/D145180 Files: lldb/include/lldb/Symbol/Symbol.h lldb/source/Plugins/ObjectFile/CMakeLists.txt lldb/source/Plugins/ObjectFile/JSON/CMakeLists.txt

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

JDevlieghere added inline comments. Comment at: lldb/include/lldb/Core/Debugger.h:594 lldb::TargetSP m_dummy_target_sp; + Diagnostics::CallbackID m_diagnostics_callback_id; clayborg wrote: > I am guessing these changes are in Debugger.h and Debugger.cpp

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

JDevlieghere updated this revision to Diff 503543. JDevlieghere marked 21 inline comments as done. JDevlieghere added a comment. - Remove 9d311dd6a71b from patch - Use `EXPECT_THAT_EXPECTED` CHANGES SINCE LAST ACTION

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

clayborg added inline comments. Comment at: lldb/include/lldb/Core/Debugger.h:594 lldb::TargetSP m_dummy_target_sp; + Diagnostics::CallbackID m_diagnostics_callback_id; I am guessing these changes are in Debugger.h and Debugger.cpp are not related to this

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

JDevlieghere updated this revision to Diff 503432. JDevlieghere added a comment. - Add unit tests for JSON deserialization - Add unit tests for converting JSONSymbol to Symbol CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145180/new/ https://reviews.llvm.org/D145180 Files:

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

clayborg added a comment. Looks good. Only questions is if we can add a C++ unit test for this file and test the new Symbol::FromJSON() and test all error conditions. Comment at: lldb/source/Symbol/Symbol.cpp:99-100 +llvm::Expected Symbol::FromJSON(const JSONSymbol , +

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

JDevlieghere updated this revision to Diff 503166. JDevlieghere added a comment. - Remove section field - Support raw value symbols - Update test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145180/new/ https://reviews.llvm.org/D145180 Files: lldb/include/lldb/Symbol/Symbol.h

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

JDevlieghere marked an inline comment as done. JDevlieghere added inline comments. Comment at: lldb/source/Symbol/Symbol.cpp:44-49 + if (!section_list) +return; + + // This should've been caught during deserialization. + if (!symbol.value && !symbol.address) +return;

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

JDevlieghere marked an inline comment as done. JDevlieghere added inline comments. Comment at: lldb/source/Symbol/Symbol.cpp:44-49 + if (!section_list) +return; + + // This should've been caught during deserialization. + if (!symbol.value && !symbol.address) +return;

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

clayborg added inline comments. Comment at: lldb/include/lldb/Symbol/Symbol.h:27 + std::optional id; + std::optional section; + std::optional type; Come to think of it, we might not need the section as a name as it adds no real value unless we want to add a

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

JDevlieghere updated this revision to Diff 502854. JDevlieghere added a comment. Implement Greg's features. I'll add a unit test for the deserialization later today. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145180/new/ https://reviews.llvm.org/D145180 Files:

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

clayborg added inline comments. Comment at: lldb/include/lldb/Symbol/Symbol.h:23 +struct JSONSymbol { + uint64_t value; + std::optional size; JDevlieghere wrote: > clayborg wrote: > > clayborg wrote: > > > JDevlieghere wrote: > > > > clayborg wrote: > > > > >

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

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 size; clayborg wrote: > clayborg wrote: > > JDevlieghere wrote: > > >

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

clayborg added inline comments. Comment at: lldb/include/lldb/Symbol/Symbol.h:23 +struct JSONSymbol { + uint64_t value; + std::optional size; JDevlieghere wrote: > clayborg wrote: > > Do we something that says "value is an address"? Or are we inferring that >

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

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 size; clayborg wrote: > Do we something that says "value is an address"?

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

clayborg added inline comments. Comment at: lldb/include/lldb/Symbol/Symbol.h:351-356 +bool fromJSON(const llvm::json::Value , lldb_private::JSONSymbol , + llvm::json::Path path); + +bool fromJSON(const llvm::json::Value , lldb::SymbolType , +

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

clayborg added a comment. If these files can be used as the only source of information (without a stripped executable), we really should include a serialized SectionList in the JSON that can be loaded into ObjectFileJSON. This would be very useful for easily creating unit tests.

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

JDevlieghere updated this revision to Diff 502330. JDevlieghere marked 3 inline comments as done. JDevlieghere added a comment. Address Greg's feedback CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145180/new/ https://reviews.llvm.org/D145180 Files: lldb/include/lldb/Symbol/Symbol.h

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

JDevlieghere marked 4 inline comments as done. JDevlieghere added a comment. Thanks for the feedback Greg, they're all great suggestions. In D145180#4166302 , @clayborg wrote: > From reading this it looks like your main use case is to supply additional

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

clayborg added a comment. I like this idea of this, but I would like to see this be a bit more complete. One idea is to remove the ObjectFileJSON::Symbol definition and just use lldb_private::Symbol objects and allow these objects to construct encode and decode from JSON. Then we have the

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

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,

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

kastiglione added a comment. Given that json numbers are read into double, should the addresses be stored as strings to avoid any issues with address integer values that can't be represented as double (>=2**53)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

mib added a comment. Left some comments but overall looks good to me :) Thanks for taking this ! Comment at: lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp:37 + offset_t file_offset, offset_t length) { + if (!data_sp) { +data_sp =

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

JDevlieghere created this revision. JDevlieghere added reviewers: jingham, clayborg, labath, DavidSpickett, mib. Herald added a subscriber: kristof.beyls. Herald added a project: All. JDevlieghere requested review of this revision. Herald added a reviewer: jdoerfert. Herald added a subscriber: