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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:42
+FileHeader:
+  Version:         0x00000001
+Sections:
----------------
dschuff wrote:
> 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.
Excessive whitespace for me at least makes it harder to read.

An obj2yaml-generated document is not much better than a canned-object file in most test cases. The vast majority (all?) of our ELF tests use YAML that has been hand-written to make sure it covers the things we care about and nothing more. obj2yaml may well generate significantly more output than is needed to test the different bits that are really being implemented, so whilst it may be useful to provide a basis, the output really should be cut down.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:41
+        llvm::errc::invalid_argument,
+        "No flags are supported yet other than basic copying");
+  }
----------------
dschuff wrote:
> 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.
Yeah, I'm not particularly happy with how this is implemented in COFF or Mach-O either. Note that when you do add support for a single switch, you'll need to message, and that will just get unwieldy as more options are added. Perhaps we need to figure out a better method of resolving this, probably with some sort of command-line validation layer somewhere that goes over all switches and sees if they're supported.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930