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:
> > 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.
================
Comment at: lldb/include/lldb/Symbol/Symbol.h:23
+struct JSONSymbol {
+ uint64_t value;
+ std::optional<uint64_t> size;
----------------
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
================
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);
+
----------------
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.
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
