[PATCH] D70970: [llvm-objcopy][WebAssembly] Add dump/add/remove-section support

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/add-section.test:4-5
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --dump-section=producers=%t.sec %t
+# RUN: llvm-objcopy --add-section=producers=%t.sec %t2 %t3
+# RUN: obj2yaml %t3 | FileCheck %s
----------------
dschuff wrote:
> jhenderson wrote:
> > Where does %t2 come from? I think this test is missing a line to remove the section after the dump?
> > 
> > I'd expect to see something like:
> > ```
> > # RUN: llvm-objcopy --dump-section=producers=%t.sec --remove-section=producers %t %t2
> > # RUN: llvm-objcopy --add-section=producers=%t.sec %t2 %t3
> > ```
> Yeah, my original version tried to avoid using dump/remove in the test, but this ended up a lot simpler this way.
> I updated the test as suggested. Your suggestion doesn't work as-written because section removal happens before section dumping (and I kept it that way for consistency with the other targets), but I just did it in a separate invocation.
> Your suggestion doesn't work as-written because section removal happens before section dumping (and I kept it that way for consistency with the other targets), but I just did it in a separate invocation.

It turns out that this is a bug compared with GNU objcopy for ELF (and I'm guessing other formats too): --dump-section should be runnable in the same operation as the section is removed. I've filed PR44283 for that.




================
Comment at: llvm/test/tools/llvm-objcopy/wasm/dump-section.test:7
+# RUN: not llvm-objcopy --dump-section=nonexistent=%t.sec %t 2>&1 | FileCheck --check-prefix=NONEXISTENT %s
+# RUN: not llvm-objcopy --dump-section=producers=foo/bar %t 2>&1 | FileCheck --check-prefix=DIROUT %s
+
----------------
I'd recommend changing this to "%t.dir/bar" or similar, since theoretically something else might create "foo" (by accident or intentionally, it doesn't matter).


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/dump-section.test:14
+# NONEXISTENT: section 'nonexistent' not found
+# DIROUT: error: {{.*}}: {{[nN]}}o such file or directory
+
----------------
I'd include the bit of the path that provides context in this check - I'm guessing something like `error: {{.*}}/bar: {{[nN]}}o such file or directory` would work.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/remove-section.test:7
+## Requests to remove nonexistent sections are silently ignored.
+# RUN: llvm-objcopy --remove-section=nonexistent=%t.sec %t
+
----------------
I think you mean here to write `llvm-objcopy --remove-section=nonexistent %t`?

It's probably a good idea to check that there are no warnings produced as part of this operation.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/remove-section.test:9-10
+
+## Check that the producers section has been removed, but not the type section.
+# CHECK: TYPE
+
----------------
You probably want to move this to immediately after the corresponding FileCheck run to make it a little clearer what is using it.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/remove-section.test:12
+
+# NONEXISTENT: section 'nonexistent' not found
+
----------------
Unnecessary?


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/remove-section.test:3
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy -R producers %t %t2
+# RUN: obj2yaml %t2 | FileCheck %s
----------------
dschuff wrote:
> jhenderson wrote:
> > You need a test showing what happens when you attempt to remove a non-existent section.
> This is actually a more interesting question than I realized. The current behavior is to error on dumping a nonexistent section but will silently ignore request to remove nonexistent sections, which matches ELF objcopy. I can see why one might want that behavior, but is that in your opinion a good thing to duplicate from ELF?
Most of the llvm-objcopy behaviour for ELF at least is intended to replicate what GNU objcopy does, for compatibility reasons. I'm guessing there isn't another version of objcopy for wasm yet? If there is, we should try to follow that. Otherwise, I think for consistency it probably makes sense to follow the general guidelines of what ELF does, as if wasm is support is eventually added to GNU objcopy, it's likely to follow its own behaviour for other formats.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:63
+
+  for (const StringRef &Flag : Config.AddSection) {
+    StringRef SecName, FileName;
----------------
No need for a `StringRef` to be a `const &`, since it is already implicitly.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:66
+    std::tie(SecName, FileName) = Flag.split("=");
+    ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr = MemoryBuffer::getFile(FileName);
+    if (!BufOrErr)
----------------
clang-format?


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:76
+
+  for (const StringRef &Flag : Config.DumpSection) {
+    StringRef SecName;
----------------
See my above comment re. `StringRef`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70970