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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: grimar.
jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:42
+FileHeader:
+  Version:         0x00000001
+Sections:
----------------
sbc100 wrote:
> jhenderson wrote:
> > 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.
> I feel like for yaml inputs like this should be the verbatim result of compiling some simple input file (either from .ll or .s) and as such should not be modified in place.  Instead we should include instructions on how to regenerate it periodically if/when the yaml format changes.
> 
> For yaml output checking I agree there could be argument made for trimming since we often only check for a tiny subset.
This sounds like a philosophical difference to me. I'd like to keep our test inputs to the bare minimum that actually test what we care about. This is the approach we take in the ELF objcopy testing, and indeed in most other newer tests on other tools like llvm-readobj (@grimar in particular has gone to a lot of effort to help improve those tests recently along these sort of lines).

If we start going down the approach of just blindly taking whatever obj2yaml produces for an input, many of the tests will be testing things that aren't really anything to do with their purpose.

That all being said, there may be a place for additional tests that do the round-trip obj2yaml, llvm-objcopy, yaml2obj testing, but I don't think these should be the general rule. In this case, I'd actually start from a .ll or .s file and use llvm-mc or whatever to turn them into an object. That way, there is no risk of the YAML and original source drifting apart.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930