[PATCH] D70930: [llvm-objcopy][WebAssembly] Initial support for wasm in llvm-objcopy

Tue Dec 3 01:46:45 PST 2019

jhenderson added a comment.

In general, we've tended to only add support for a basic copy in the first patch, and add further functionality as needed for each switch. That helps keep the patch size down and focuses on getting the basic writing algorithm right before worrying about any manipulations that need to make. As such, please break this patch into several smaller patches.

I don't really know anything about WASM by the way, but I've taken a look at generic stuff.

================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/add-section.test:1
+# This actually tests dumping, removal, and addition
+# RUN: yaml2obj %s > %t
----------------
Nit: missing full stop.

In general with newer tests, we use '##' to indicate comments, so that they stand out from test lines.

I'd also be more expansive in your top-level comment with what the test is doing, i.e. it tests adding a section to a WASM object.

Same sort of comments apply in the other tests.

Why are you testing multiple pieces of functionality in one test?

================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/add-section.test:2
+# This actually tests dumping, removal, and addition
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --dump-section=producers=%t.sec %t
----------------
`yaml2obj %s -o %t` is more common in newer tests, to avoid a shell redirection.

================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/add-section.test:11
+FileHeader:
+  Version:         0x00000001
+Sections:
----------------
At least in ELF YAML, we tend to remove the excessive whitespace that yaml2obj currently produces. I'd recommend doing the same here. Keep only enough whitespace so that all values within a block are lined up.

================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/add-section.test:71
+# Check that the producers section has been removed.
+REMOVED-NOT: producers
+
----------------
Add '#' characters for these CHECK lines.

================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/add-section.test:78
+CHECK-NEXT:        Version:         9.0.0
\ No newline at end of file

----------------
Nit: no new line at EOF.

================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/basic-archive-copy.test:1
+# RUN: yaml2obj %s > %t
+
----------------
Same sort of comment here as in the above test:
1. Add a top-level comment explaining the purpose of the test.
2. Use -o for yaml2obj output.

================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/basic-archive-copy.test:11
+
+
+# RUN: llvm-readobj --sections %t2 | FileCheck %s
----------------
Nit: too many blank lines.

================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/basic-archive-copy.test:15
+# RUN: llvm-nm --print-armap %t2.a | FileCheck --check-prefix=INDEX-TABLE %s
+# Verify that llvm-objcopy has not modifed the input.
+# RUN: cmp %t.copy.a %t.a
----------------
'##'

================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/basic-archive-copy.test:89
+
+CHECK: Arch: wasm32
+CHECK: Sections [
----------------
Use '#' characters here.

================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/basic-copy.test:1
+# Test that the copied object has the same yaml conversion as the original.
+# RUN: yaml2obj %s > %t.o
----------------
Same as above.

================
Comment at: llvm/test/tools/llvm-objcopy/Wasm/basic-copy.test:7-8
+# RUN: diff %t.yaml %t2.yaml
+# Currently the copied object is not bit-identical to the yaml2obj-generated
+# one, as yaml2obj uses 5-byte LEBs for section sizes (unlike objcopy/clang).
+# NORUN : cmp %t.o %t2.o
----------------
What's stopping llvm-objcopy using the 5-byte LEBs?

FWIW, in ELF, we don't require a basic copy to produce identical output to the input, because the input might be written in an order style (e.g. the section header table might appear in a different place to "normal", there might be gaps in the object which objcopy compresses etc). Assuming this applies for WASM, you shouldn't try to do a binary comparison, but rather test the key things are all identical.

================
Comment at: llvm/tools/llvm-objcopy/Wasm/Object.cpp:49
+size_t Writer::finalize() {
+  size_t ObjectSize = 8; // Size of wasm header
+  SectionHeaders.reserve(Obj.getSections().size());
----------------
Is there any structure that you can do a sizeof operation on or similar to avoid a literal here (and the need of the comment)?

================
Comment at: llvm/tools/llvm-objcopy/Wasm/Object.cpp:75
+    return E;
+  // write header
+  uint8_t *Ptr = Buf.getBufferStart();
----------------
Put a blank line before each grouping here. Also, comments need leading capital letters and trailing full stops.

================
Comment at: llvm/tools/llvm-objcopy/Wasm/Object.h:60
+public:
+  ::llvm::wasm::WasmObjectHeader Header;
+  // TODO: Support synbols, relocations, debug info, etc
----------------
You're already in the llvm namespace. Can't you just do `wasm::WasmObjectHeader`?

================
Comment at: llvm/tools/llvm-objcopy/Wasm/Object.h:62
+  // TODO: Support synbols, relocations, debug info, etc
+  // Stuff for sections
+  ArrayRef<Section> getSections() { return Sections; };
----------------
This line of the comment isn't useful. The names of the functions show this. Please delete.

================
Comment at: llvm/tools/llvm-objcopy/Wasm/WasmObjcopy.cpp:39-41
+        return createStringError(object_error::parse_failed,
+                                 "cannot dump section '%s': it has no contents",
+                                 SecName.str().c_str());
----------------
Should this be an error? In ELF, an empty section is legitimate, and there's no reason it couldn't generate an empty file in this case.

================
Comment at: llvm/tools/llvm-objcopy/Wasm/WasmObjcopy.cpp:53
+  }
+  return createStringError(object_error::parse_failed, "section '%s' not found",
+                           SecName.str().c_str());
----------------
This may want to be `errc::invalid_argument`, since the process hasn't failed to parse the file.

================
Comment at: llvm/tools/llvm-objcopy/Wasm/WasmObjcopy.cpp:114
+  Object *Obj = ObjOrErr->get();
+  assert(Obj && "Unable to deserialize COFF object");
+  if (Error E = handleArgs(Config, *Obj))
----------------
Copy-paste error?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930