[PATCH] D70930: [llvm-objcopy][WebAssembly] Initial support for wasm in llvm-objcopy

[Derek Schuff via Phabricator via llvm-commits]

dschuff marked 2 inline comments as done.
dschuff added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/Wasm/Object.cpp:57
+    auto &Header = SectionHeaders.back();
+    raw_svector_ostream OS(Header);
+    encodeULEB128(S.SectionType, OS);
----------------
sbc100 wrote:
> Seems like kind of a shame we have so many different places where we write the object format.  This existing ones that I know of are:
> 
> 1. `llvm/lib/MC/WasmObjectWriter.cpp`
> 2. `llvm/lib/ObjectYAML/WasmEmitter.cpp`
> 3. `lld/wasm/Writer.cpp`
> 
> So this is added a 4th.   But maybe its the same for ELF.   I wonder if it would make sense to add a writing capability to `lib/Object` to unify these.  I guess they are all subtly different so maybe it doesn't make sense?
It is a bit odd that there is a completely independent data model of object files here (the ELF one is particularly complex but I don't have a particular intuition about how much duplication there is for ELF). For wasm (assuming we want to model symbols and relocations against custom sections), my hope was that we could continue to treat the known sections and most custom sections as opaque blobs and hopefully reuse the MC logic for symbols, which would mean that this code wouldn't get too much more complex. Not sure about how much work/duplication symbols would be though.


================
Comment at: llvm/tools/llvm-objcopy/Wasm/WasmObjcopy.cpp:53
+  Object *Obj = ObjOrErr->get();
+  assert(Obj && "Unable to deserialize Wasm object");
+  if (Error E = handleArgs(Config, *Obj))
----------------
sbc100 wrote:
> Is this assert useful?   The contract with the Expected is that that the object pointer is valid if its not an error right?   Also "deserialize" is a strange word here.. read or load would make more sense to me).
I think if Expected holds a pointer/smart pointer, it can still be null?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930