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


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


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/add-section.test:69-73
+## Check that the producers section has been added back unchanged.
+# CHECK:    Name:            producers
+# CHECK-NEXT:    Tools:
+# CHECK-NEXT:      - Name:            clang
+# CHECK-NEXT:        Version:         9.0.0
----------------
I generally find it easier to follow the test if CHECKs are before the YAML, especially where the YAML is large.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/dump-section.test:1
+## Test the contents of a custom section dumped from a binary.
+# RUN: yaml2obj %s -o %t
----------------
You need a test showing what happens when you attempt to dump a non-existent section.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/dump-section.test:24
+
+## The contents of the producers section
+# CHECK: 0000000 01 0c 70 72 6f 63 65 73 73 65 64 2d 62 79 01 05
----------------
Missing trailing full stop.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/dump-section.test:25-26
+## The contents of the producers section
+# CHECK: 0000000 01 0c 70 72 6f 63 65 73 73 65 64 2d 62 79 01 05
+# CHECK: 0000020 63 6c 61 6e 67 05 39 2e 30 2e 30
----------------
Same comment as add-section.test


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/remove-section.test:1
+## Test the --remove-section flag
+# RUN: yaml2obj %s -o %t
----------------
Missing trailing full stop.


================
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
----------------
You need a test showing what happens when you attempt to remove a non-existent section.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/remove-section.test:23-25
+## Check that the producers section has been removed, but not the type section.
+# CHECK: TYPE
+# CHECK-NOT: producers
----------------
Same comment as in other test (move earlier).

Also, this is insufficient to show that the `producers` section doesn't exist. This only shows that it doesn't exist AFTER the `TYPE` section. You either want CHECK-NOT lines before and after the `CHECK: TYPE` line, or you want to add `--implicit-check-not=producers` to your FileCheck line.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Object.cpp:28
+void Object::removeSections(function_ref<bool(const Section &)> ToRemove) {
+  // TODO: remove reloc sections for the removed section, handle symbols, etc
+  Sections.erase(
----------------
Nit: missing trailing full stop.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:37
+                               Object &Obj) {
+  for (auto &Sec : Obj.getSections()) {
+    if (Sec.Name == SecName) {
----------------
What is the type of `Sec` here (please don't use `auto` unless the type is obvious)? Also, should it be a `const &`?


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:42-43
+          FileOutputBuffer::create(Filename, Contents.size());
+      if (!BufferOrErr)
+        return BufferOrErr.takeError();
+      std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
----------------
If there's an easy way, this should be tested. Example is that the output file is a directory.


================
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;
----------------
If there's an easy way, this should be tested too.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:63
+
+  for (const auto &Flag : Config.AddSection) {
+    StringRef SecName, FileName;
----------------
I'd prefer this not to be `auto`.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:66
+    std::tie(SecName, FileName) = Flag.split("=");
+    auto BufOrErr = MemoryBuffer::getFile(FileName);
+    if (!BufOrErr)
----------------
Ditto.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:76
+
+  for (const auto &Flag : Config.DumpSection) {
+    StringRef SecName;
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70970