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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a comment.

Thanks for the detailed review. I just uploaded a diff to address the comments (other than splitting the patch apart).



================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/add-section.test:1
+# This actually tests dumping, removal, and addition
+# RUN: yaml2obj %s > %t
----------------
jhenderson wrote:
> Nit: missing full stop.
> 
> In general with newer tests, we use '##' to indicate comments, so that they stand out from test lines.
> 
> I'd also be more expansive in your top-level comment with what the test is doing, i.e. it tests adding a section to a WASM object.
> 
> Same sort of comments apply in the other tests.
> 
> Why are you testing multiple pieces of functionality in one test?
This started out as a copy of `test/tools/llvm-objcopy/ELF/add-section.test` which has a similar structure. Since you've asked to start without this functionality anyway, I can back this test out entirely for now.


================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/basic-copy.test:7-8
+# RUN: diff %t.yaml %t2.yaml
+# Currently the copied object is not bit-identical to the yaml2obj-generated
+# one, as yaml2obj uses 5-byte LEBs for section sizes (unlike objcopy/clang).
+# NORUN : cmp %t.o %t2.o
----------------
jhenderson wrote:
> What's stopping llvm-objcopy using the 5-byte LEBs?
> 
> FWIW, in ELF, we don't require a basic copy to produce identical output to the input, because the input might be written in an order style (e.g. the section header table might appear in a different place to "normal", there might be gaps in the object which objcopy compresses etc). Assuming this applies for WASM, you shouldn't try to do a binary comparison, but rather test the key things are all identical.
Nothing is stopping it; I just decided to match clang's behavior instead of yaml2obj's (as the comment mentions, they are different). I'll just clarify the comment, since as you say, there's not really any reason to change it.


================
Comment at: llvm/tools/llvm-objcopy/Wasm/Object.h:60
+public:
+  ::llvm::wasm::WasmObjectHeader Header;
+  // TODO: Support synbols, relocations, debug info, etc
----------------
jhenderson wrote:
> You're already in the llvm namespace. Can't you just do `wasm::WasmObjectHeader`?
No:
`/s/llvm-upstream/llvm-project/llvm/tools/llvm-objcopy/Wasm/Object.h:60:3: error: no type named 'WasmObjectHeader' in namespace 'llvm::objcopy::wasm'; did you mean '::llvm::wasm::WasmObjectHeader'?`


================
Comment at: llvm/tools/llvm-objcopy/Wasm/WasmObjcopy.cpp:39-41
+        return createStringError(object_error::parse_failed,
+                                 "cannot dump section '%s': it has no contents",
+                                 SecName.str().c_str());
----------------
jhenderson wrote:
> Should this be an error? In ELF, an empty section is legitimate, and there's no reason it couldn't generate an empty file in this case.
Actually you're right; I believe custom sections can be empty (Wasm has a distinction between "known" sections whose structure is specified by the standard and "custom" sections which are not interpreted by the VM). Since we only try to support custom sections for now, I'll remove this check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930