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

[Derek Schuff via Phabricator via llvm-commits]

dschuff marked 18 inline comments as done.
dschuff 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
----------------
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.


================
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
----------------
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?


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:46
+      std::copy(Contents.begin(), Contents.end(), Buf->getBufferStart());
+      if (Error E = Buf->commit())
+        return E;
----------------
jhenderson wrote:
> If there's an easy way, this should be tested too.
I couldn't find any way to cause error this without trying to do racy things like changing things about the filesystem while llvm-objcopy is running.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70970