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