[PATCH] D71575: [LLDB] Add ObjectFileWasm plugin for WebAssembly debugging

[Pavel Labath via Phabricator via llvm-commits]

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 &section_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