[PATCH] D155535: [WebAssembly][Objcopy] Write output section headers identically to inputs

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/section-header-size.test:3
+## the input binary, including the encoded size of the LEB that represents the
+## section size. sec-header-3bytelebs.wasm has LEB encodings padded to 3 bytes, which
+## is unlikely to happen accidentally (since the smallest size is 1 byte,
----------------
jhenderson wrote:
> Rather than adding a canned binary, it seems to me like it would be fairly straightforward to add some additional functionality to yaml2obj to customise the LEB size? Probably an additional, optional field called "HeaderSizeLength" or something to that effect as a section member?
Yes, it's straightforward to add YAML support for this. The reason I didn't was that it would mean a new field in all of the sections of every YAML output, which would be a lot of extra output that we don't care about in every section in every test (which would make the output harder to read and require rewriting the expectations for basically every wasm YAML test). There are a couple of options here.
1. Just use the checked-in binary in this one case
2. Implement reading the value from YAML but not print it when outputting YAML. This allows writing a test without the checked-in binary and avoids polluting all the output. The test would read YAML with this field and diff the output of llvm-objcopy against the one generated by yaml2obj. That's nice, although it's a slightly odd that the support is asymmetric.
3. Just bite the bullet and rewrite all the YAML tests. Then we could write this test with yaml2obj and obj2yaml.
4. Same as option 2 but only print the value in YAML if it's a "non-default" value, i.e. neither the minimum size for the section in question, nor 5 (the default "padded" value). This would complicate the wasm2yaml logic slightly but not require rewriting the tests.

I think I favor option 2, WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155535