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