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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Symbol/Symbol.h:23
+struct JSONSymbol {
+  uint64_t value;
+  std::optional<uint64_t> size;
----------------
JDevlieghere wrote:
> clayborg wrote:
> > clayborg wrote:
> > > JDevlieghere wrote:
> > > > 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. 
> > > Remember there are some symbols that are not addresses. Think of trying to create a N_OSO symbol where the value isn't an address, but it is a time stamp integer. 
> > > 
> > > Symbols in the symbol table for mach-o files can have a section ID that is valid and the value is not an offset, it is still an address. Not sure if that matters. 
> > > 
> > > One way to make this clear is if we have a "section_id" then "value" is an address, and if not, then "value" is just an integer? It does require that we always specify section IDs though. Also, section IDs are not human readable, we could change "std::optional<uint64_t> section_id;" to be "std::optional<std::string> section;" and thid becomes the fully qualified section name like "__TEXT.__text" or "PT_LOAD[0]..text". Much more readable. And easy to dig up the section from the Module's section list.
> > Another way to do this would be to have JSONSymbol know if it has an address or just an integer value by defining this as:
> > ```
> > std::optional<uint64_t> address;
> > std::optional<uint64_t> value;
> > ```
> > The deserializer would verify that only one of these is valid and fail if both or neither were specified
> Sure, that sounds reasonable. 
And we can add tests that verify we get an error back if neither or both are specified.


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

https://reviews.llvm.org/D145180