[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:
> 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. 


================
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:
> JDevlieghere wrote:
> > 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. 
> Any ObjectFile can always get the section list from the Module, so if there is no serialized section list, ObjectFileJSON doesn't need to provide any sections, and it can just call:
> ```
> SectionList *sections = GetModule()->GetSectionList();
> ```
> Any object files within a module are asked to add their own sections if they have any extra sections. We do this with dSYM files where we add the __DWARF segment to the Module's main section list. So the executable adds "__PAGEZERO, __TEXT, DATA_, etc", then the dSYM object file just adds any extra sections it need with a call to:
> ```
> void ObjectFile::CreateSections(SectionList &unified_section_list);
> ```
> So if we have a section list already in ObjectFileJSON, we need to verify that they all match with any pre-existing sections that are already in the SectionList, and can add any extra sections if needed. If the ObjectFileJSON was the only object file, it could fully populate a Module's section list on its own.
I'm aware the data is present but I don't see how to pass that into the JSON deserialization function if we don't go through an intermediary object. As far as I can tell, there's no way to pass this additional data into `fromJSON`. That's why I have a ctor (`Symbol(const JSONSymbol &symbol, SectionList *section_list);`) that takes both the deserialized symbol and the section list. If there's a way to do that I'm happy to scrap the `JSONSymbol`. 


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

https://reviews.llvm.org/D145180