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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:42
+FileHeader:
+  Version:         0x00000001
+Sections:
----------------
jhenderson wrote:
> 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
> ```
OK, but is it really worthwhile to deviate from the default output format of obj2yaml and hand-edit everything just so there's less whitespace? It doesn't look any more readable to me.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:73
+           Count:           3
+        Body:            418080808000210020002101200111808080800000210220020F0B
+      - Index:              1
----------------
jhenderson wrote:
> 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?
I guess for the purposes of this test it doesn't actually matter whether the function body is valid wasm code or not. I do want to have some known (CODE, TYPE, etc) and linking/reloc sections in the test so that we can ensure that both code that is and is not understood by the object file layer doesn't get modified. That also means that the body has to be long enough that 5-byte relocations against it make sense.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:93-94
+        Function:        1
+...
+
+
----------------
jhenderson wrote:
> You don't need these two lines.
Removing the readobj test above means removing all of this anyway.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:41
+        llvm::errc::invalid_argument,
+        "No flags are supported yet other than basic copying");
+  }
----------------
jhenderson wrote:
> 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.
That's more future-proof for me, but much less useful for a user. In order to figure out what *is* supported if someone gets this error, they'd have to keep removing flags until it works. I did update the case though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930