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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/basic-archive-copy.test:1
+## Test that a basic copy of an object works from an archive.
+
----------------
I looks like objcopy might be the exception here but normally when writing tests in a given format such as this we give them the file extension of the format.  So this file should be a `.yaml` maybe?   One of the advantages of this is that you get syntax highlighting by default.

Maybe we should land a rename of the existing tests to follow this convention first?


================
Comment at: llvm/tools/llvm-objcopy/Wasm/Object.cpp:57
+    auto &Header = SectionHeaders.back();
+    raw_svector_ostream OS(Header);
+    encodeULEB128(S.SectionType, OS);
----------------
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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930