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

Wed Dec 4 01:58:38 PST 2019

jhenderson added a comment.

> I looks like objcopy might be the exception here but normally when writing tests in a given format such as this we give them the file extension of the format. So this file should be a .yaml maybe? One of the advantages of this is that you get syntax highlighting by default.
> 
> Maybe we should land a rename of the existing tests to follow this convention first?

objcopy isn't the exception. It's common practice for YAML tests throughout the binutils to have .test as an extension, because .yaml isn't in the default set of extensions that lit recognises as a test. I don't object to a mass renaming, but it should probably be done for each of the tools and probably needs to include a change in the default suffixes too (which might cause a problem in some places where a separate YAML file is used as an input...).

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:1
+## Test that a basic copy of an object works from an archive.
+
----------------
Maybe rephrase this slightly:

"Test a basic copy of an archive containing a WASM object."

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:15-16
+
+## Check the copied object has the correct sections
+# RUN: llvm-readobj --sections %t2 | FileCheck %s
+
----------------
This is probably not needed: you already have basic-copy.test, which shows that objects are copied correctly, so the comparison of the extracted object is probably sufficient.

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:42
+FileHeader:
+  Version:         0x00000001
+Sections:
----------------
As mentioned in the now-deleted test, please remove the excessive whitespace, so that the Values in any given block line up neatly, but as close as possible to their tags, i.e. 
```
--- !WASM
FileHeader:
  Version: 0x00000001
Sections:
  - Type: TYPE
    Signatures:
      - Index: 0
        ParamTypes:
          - I32
        ReturnTypes:
          - F32
      - Index: 1
        ParamTypes:
          - I32
          - I64
        ReturnTypes: []
# etc
      - Type:   R_WASM_TABLE_INDEX_SLEB
        Index:  0
        Offset: 0x00000006
```

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:73
+           Count:           3
+        Body:            418080808000210020002101200111808080800000210220020F0B
+      - Index:              1
----------------
Not knowing anything really about WASM, how much of the stuff in this output is really necessary for the test? Can, e.g. the `Body` value here be reduced down to a minimal content?

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:93-94
+        Function:        1
+...
+
+
----------------
You don't need these two lines.

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-copy.test:4
+## one, as yaml2obj uses 5-byte LEBs for section sizes (unlike objcopy/clang).
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-objcopy %t.o %t2.o
----------------
Prefer `-o` to shell redirection.

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-copy.test:10
+
+--- !WASM
+FileHeader:
----------------
Same comments apply to this YAML as in the other test.

================
Comment at: llvm/tools/llvm-objcopy/Wasm/Object.cpp:57
+    auto &Header = SectionHeaders.back();
+    raw_svector_ostream OS(Header);
+    encodeULEB128(S.SectionType, OS);
----------------
dschuff wrote:
> sbc100 wrote:
> > Seems like kind of a shame we have so many different places where we write the object format.  This existing ones that I know of are:
> > 
> > 1. `llvm/lib/MC/WasmObjectWriter.cpp`
> > 2. `llvm/lib/ObjectYAML/WasmEmitter.cpp`
> > 3. `lld/wasm/Writer.cpp`
> > 
> > So this is added a 4th.   But maybe its the same for ELF.   I wonder if it would make sense to add a writing capability to `lib/Object` to unify these.  I guess they are all subtly different so maybe it doesn't make sense?
> It is a bit odd that there is a completely independent data model of object files here (the ELF one is particularly complex but I don't have a particular intuition about how much duplication there is for ELF). For wasm (assuming we want to model symbols and relocations against custom sections), my hope was that we could continue to treat the known sections and most custom sections as opaque blobs and hopefully reuse the MC logic for symbols, which would mean that this code wouldn't get too much more complex. Not sure about how much work/duplication symbols would be though.
I can't comment about WASM, but to some degree, ELF llvm-objcopy needs to have a different internal representation of an Object to that of the library, because the existing Object library object file there doesn't really support mutation. That's largely why things are written the way they were. @jakehehrlich might have more comments on that though.

================
Comment at: llvm/tools/llvm-objcopy/wasm/Object.h:65
+  void addSections(ArrayRef<Section> NewSections);
+  void removeSections(function_ref<bool(const Section &)> ToRemove);
+
----------------
Can we get rid of this for this version (i.e. add it later when they are needed)?

================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:38
+      !Config.SetSectionFlags.empty() || !Config.SymbolsToRename.empty() ||
+      !Config.ToRemove.empty() || !Config.DumpSection.empty() || !Config.AddSection.empty()) {
+    return createStringError(
----------------
clang-format?

================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:41
+        llvm::errc::invalid_argument,
+        "No flags are supported yet other than basic copying");
+  }
----------------
I'd probably just change this comment so that it is future proof "unsupported flag specified", or something like that. Also, we usually use lower-case first letter for error/warning messages.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930