[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:1
+## Test --add-section. This test dumps the section first and checks that adding
+## it back doesn't change the result.
----------------
dumps -> dumps and removes


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/add-section.test:9-12
+# CHECK:    Name:            producers
+# CHECK-NEXT:    Tools:
+# CHECK-NEXT:      - Name:            clang
+# CHECK-NEXT:        Version:         9.0.0
----------------
The whitespace doesn't need to exactly match what obj2yaml produces, so it might look a little nicer and easier to read by reformatting this slightly:
```
# CHECK:      Name: producers
# CHECK-NEXT: Tools:
# CHECK-NEXT:   - Name:    clang
# CHECK-NEXT:     Version: 9.0.0
```


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/add-section.test:72-74
+
+...
+
----------------
Nit: I think these last three lines (two blank) can be deleted.


================
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
+
----------------
dschuff wrote:
> 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.
Seems reasonable. I think the same approach for ELF should work, if you don't mind fixing it up in a separate patch.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/dump-section.test:32-33
+        Version: 9.0.0
+...
+
----------------
You can delete these two lines.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/remove-section.test:9
+## Requests to remove nonexistent sections are silently ignored.
+# RUN: llvm-objcopy --remove-section=nonexistent=%t.sec %t
+
----------------
> It's probably a good idea to check that there are no warnings produced as part of this operation.
I think you missed this part. You can check for warnings by doing something like:
`# RUN: llvm-objcopy --remove-section=nonexistent=%t.sec %t 2&>1 | count 0`


================
Comment at: llvm/tools/llvm-objcopy/wasm/Object.h:32
   llvm::wasm::WasmObjectHeader Header;
+  // Add a section
+  void addSectionWithOwnedContents(Section NewSection,
----------------
I don't think this comment is useful. It's obvious from the function name that this adds a section.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:44
 static Error handleArgs(const CopyConfig &Config, Object &Obj) {
+  for (StringRef Flag : Config.AddSection) {
+    StringRef SecName, FileName;
----------------
Check against GNU objcopy for ELF, but I suspect --add-section is applied after removal and dumping. This would allow replacing a section to be done by `--remove-section foo --add-section foo=foo.bin` or something along those lines.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:70
+  // Only support AddSection, DumpSection, RemoveSection.
+  // Don't support OnlySection yet because we only operate on custom sections.
+  Obj.removeSections([&Config](const Section &Sec) {
----------------
Either make this a TODO, or just remove the line, since the check below shows we don't support OnlySection.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70970