labath added a comment.
A bunch more comments from me. :)
A higher level question I have is whether there's something suitable within
llvm for parsing wasm object files that could be reused. I know this can be
tricky with files loaded from memory etc., so it's fine if it isn't possible to
do that, but I am wondering if you have considered this option...
================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:35-38
+ llvm::StringRef magic(reinterpret_cast<const char *>(data_sp->GetBytes()),
4);
+ if (magic != llvm::StringRef(llvm::wasm::WasmMagic,
+ sizeof(llvm::wasm::WasmMagic) / sizeof(char)))
+ return false;
----------------
`if (llvm::identify_magic(toStringRef(data_sp->GetData())) !=
llvm::file_magic::wasm_object)` maybe ?
================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:40
+
+ const uint32_t *version = reinterpret_cast<const uint32_t *>(
+ data_sp->GetBytes() + sizeof(llvm::wasm::WasmMagic));
----------------
labath wrote:
> This looks like it will cause problems on big endian hosts..
One way to get around that would be to use something like
`llvm::support::read32le`.
================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:46-63
+llvm::Optional<uint8_t> GetVaruint7(DataExtractor §ion_header_data,
+ lldb::offset_t *offset_ptr) {
+ lldb::offset_t initial_offset = *offset_ptr;
+ uint64_t value = section_header_data.GetULEB128(offset_ptr);
+ if (*offset_ptr == initial_offset || value > 127)
+ return llvm::None;
+ return static_cast<uint8_t>(value);
----------------
Maybe merge these and make the maximum width a function argument?
================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:192
+
+ ConstString sect_name(reinterpret_cast<const char *>(name_bytes),
+ *name_len);
----------------
I'd just use StringRef here -- there's no advantage in ConstStringifying this...
================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:215
+ *url_len);
+ GetModule()->SetSymbolFileFileSpec(FileSpec(symbols_url));
+
----------------
This seems odd -- I don't think any of our object file plugins work this way.
It's normally the symbol vendor who fiddles with the symbol file spec.
This is kind of similar to the gnu_debuglink section, and the way that works in
elf is that the object file exposes this via a separate method, which the
symbol vendor can then query and do the appropriate thing.
Maybe you could just drop this part and we can get back to it with the symbol
vendor patch?
================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:217-225
+ uint32_t section_length =
+ *payload_len - *name_len - name_len_uleb_size - url_len_uleb_size;
+ offset += section_length;
+ } else {
+ uint32_t section_length = *payload_len - *name_len - name_len_uleb_size;
+ m_sect_infos.push_back(section_info{*offset_ptr + offset, section_length,
+ *section_id, sect_name});
----------------
I wouldn't be afraid of presenting `external_debug_info` as an actual section,
if that's how it's treated byt the object file format. And it looks like that
could simplify this code a bit...
================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:342-343
+ section_type, // Section type.
+ sect_info.offset & 0xffffffff, // VM address.
+ sect_info.size, // VM size in bytes of this section.
+ sect_info.offset &
----------------
Are the debug info sections actually loaded into memory for wasm? Should these
be zero (that's what they are for elf)?
================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:382
+
+ DecodeSections(load_address);
+
----------------
This is strange.. I wouldn't expect that the section decoding logic should
depend on the actual address that the object is loaded in memory. Can you
explain the reasoning here?
================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:395
+ if (target.GetSectionLoadList().SetSectionLoadAddress(
+ section_sp, load_address | section_sp->GetFileOffset())) {
+ ++num_loaded_sections;
----------------
Normally I would expect to see `->GetFileAddress()` here, as that's the thing
which says how the sections are laid out in memory. The way you create these
sections, the two values are the same, but it still seems more correct to call
GetFileAddress()
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71575/new/
https://reviews.llvm.org/D71575
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
