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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/Wasm.h:113
   std::vector<wasm::WasmRelocation> Relocations; // Relocations for this section
+  std::optional<uint8_t> HeaderSecSizeEncodingLen;
 };
----------------
aheejin wrote:
> One-liner comment on what this is (in line with other members above) would be nice
FWIW, I think the comments for the other variables are redundant for the most part (e.g. the ones for type, offset and relocations provide no additional info beyond what the variable name provides).


================
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,
----------------
dschuff wrote:
> dschuff wrote:
> > 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?
> (or if not option 2, then 4, 1, or 3 in that order. Really any option other than 3 is fine with me).
Option 4 would be my suggestion, mostly because that's what ELF yaml2obj does for many of its fields. It means that we can have minimal YAML whilst also being able to go yaml2obj -> obj2yaml -> yaml2obj -> ... ad infinitum without the file ever changing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155535