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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a comment.

Rebased against master, and addressed all the comments; please take a look.



================
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
----------------
jhenderson wrote:
> 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.
> 
> 
Fixed for wasm in this patchset, and updated the test as suggested.


================
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
+
----------------
jhenderson wrote:
> 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.
This actually exposed a bug; namely, that filename in the error message was that of the input file (%t) instead of the section dump output file (%t/bar). This same bug is seems to be present in the ELF objcopy; my fix is making `handleArgs` create the `FileError` itself (with either the input or output file) rather than having its caller `executeObjcopyOnBinary` always wrap the return value. ELF objcopy has a lot more functionality, I don't know if the same approach would work there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70970