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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:42
+FileHeader:
+  Version:         0x00000001
+Sections:
----------------
jhenderson wrote:
> 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.
> 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

+1. If we use YAML it makes a lot of sense to keep it minimal. It is much easier to mantain, to extend and add new test cases.

> 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.

Yes, and we saw that many times. For example, having an excessive "AddressAlign" flag might hide the possibility to produce a section that triggers a data type misalignment exception on some platforms for a few sections. It was fixed in llvm-readobj for versioning sections recently. If we did not try to use the minimal YAML descriptions, we would never caught such bugs.
The less YAML instructions we have - the more chances we test exactly what we want to test and not something else.






Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930