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

[Paolo Severini via Phabricator via lldb-commits]

paolosev marked 10 inline comments as done.
paolosev added a comment.

In D71575#1793791 <https://reviews.llvm.org/D71575#1793791>, @labath wrote:

> 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.


I have considered this option, there is indeed some code duplication because there is already a Wasm parser in class WasmObjectFile (llvm/include/llvm/Object/Wasm.h).
However class WasmObjectFile is initialized with a memory buffer that contains the whole module, and this is not ideal. LLDB might create an ObjectFileWasm either from a file or by retrieving specific module chunks from a remote, but it doesn't often need to load the whole module in memory.
I think this is the reason why other kind of object files (like ObjectfileELF) are re-implemented in LLDB rather than reusing existing code in LLVM (like ELFFile in llvm/include/llvm/Object/ELF.h).



================
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:
> 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`.
Good point, modified to use read32le.


================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:252
+
+  ModuleSpec spec(file, ArchSpec("wasm32-unknown-unknown-wasm"));
+  specs.Append(spec);
----------------
labath wrote:
> I take it that wasm files don't have anything like a build id, uuid or something similar?
Not yet. There is a proposal to add a uuid to wasm modules in a custom section, but it's not part of the standard yet.


================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.h:116
+
+  typedef struct section_info {
+    lldb::offset_t offset;
----------------
labath wrote:
> We don't use this typedef style (except possibly in some old code which we shouldn't emulate).
Yes, this was copied from old code :). Removed. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71575