clayborg added inline comments.
================
Comment at: lldb/include/lldb/Symbol/Symbol.h:27
+ std::optional<uint64_t> id;
+ std::optional<std::string> section;
+ std::optional<lldb::SymbolType> 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 "std::optional<uint64_t> sect_offset;" field that
could be specified. We could switch to saying "if we have a valid address
member, we must be able to look it up in the section lists by address".
================
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;
----------------
We probably should make this function into a static factory function that
returns a llvm::Expected<Symbol> so we can return an error if the symbol has
not value or address, and if we can't resolve the address in a section:
```
llvm::Expected<Symbol> Symbol::CreateSymbol(const JSONSymbol &symbol,
SectionList *section_list);
```
Then we can return errors for the above cases and also for below.
================
Comment at: lldb/source/Symbol/Symbol.cpp:55-74
+ if (symbol.section) {
+ if (SectionSP section_sp =
+ section_list->FindSectionByName(ConstString(*symbol.section))) {
+ const uint64_t offset =
+ symbol.address ? *symbol.address - section_sp->GetFileAddress()
+ : *symbol.value;
+ m_addr_range = AddressRange(section_sp, offset, size);
----------------
This code should probably check if we have a "symbol.address" first, then
always fill out the address range correctly and expect a value section_sp like
the code above is doing. If we have.a symbol.value, then we don't expect a
symbol to have an address, and to do this, we will in the AddressRange with no
section and the "symbol.value" is the offset.
I am not sure if we need a "JSONSymbol::section" member anymore since we know
if this is an address now, we will expect the section lookup to succeed and can
return an error if this fails, so we can probably remove "JSONSymbol::section".
WE could leave this in if we want to specify an offset directly.
================
Comment at: lldb/test/API/macosx/symbols/TestSymbolFileJSON.py:118-119
+ "size": main_symbol.GetSize(),
+ "section": main_symbol.addr.GetSection().GetName(),
+ "value": main_symbol.addr.GetOffset(),
+ })
----------------
We should never have "section" and "value" as a combination. See above
discussion about possibly removing the "section" field as we might not need it
if we can assume:
- if "address" is valid, we must be able to look it up by address in the
section list
- if "value" is valid, it just gets encoded as the suggested code edit
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145180/new/
https://reviews.llvm.org/D145180
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
