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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
