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

[Paolo Severini via Phabricator via lldb-commits]

paolosev added inline comments.


================
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);
----------------
labath wrote:
> Maybe merge these and make the maximum width a function argument?
I'd keep the functions separate, it's better if they return different sized integers.


================
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 &
----------------
labath wrote:
> Are the debug info sections actually loaded into memory for wasm? Should these be zero (that's what they are for elf)?
Yes, I was thinking that the debug sections should be loaded into memory; not sure how this works for ELF, how does the debugger find the debug info in that case?


================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:382
+
+  DecodeSections(load_address);
+
----------------
labath wrote:
> 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?
There is a reason for this: DecodeNextSection() calls:
```ReadImageData(offset, size)```
and when the debug info sections are embedded in the wasm module (not loaded from a separated symbol file), ReadImageData() calls Process::ReadMemory() that, for a GDB remote connection, goes to ProcessGDBRemote::DoReadMemory(). Here I pass the offset because it also represents the specific id of the module loaded in the target debuggee process, as explained in the comment above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71575/new/

https://reviews.llvm.org/D71575