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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with nits addressed.



================
Comment at: llvm/lib/ObjectYAML/WasmEmitter.cpp:650
+    unsigned HeaderSecSizeEncodingLen =
+        Sec->HeaderSecSizeEncodingLen ? *Sec->HeaderSecSizeEncodingLen : 0;
+    if (HeaderSecSizeEncodingLen < getULEB128Size(OutString.size())) {
----------------
tlively wrote:
> jhenderson wrote:
> > aheejin wrote:
> > > I think this is why the tests are failing. Will fix that.
> > Thanks, I agree the previous version looks suspicious. This number controls the default encoding length, so 5 seems appropriate.
> > 
> > Looking at this again, I think there is one technical bug here still though, in that if OutString happened to be so long that the LEB had to be 6 bytes or longer, then the user would be forced to specify the YAML field, even if it was just to the same size as the expected LEB. Perhaps worth changing 5 to something like `std::max(5, getULEB128Size(OutString.size())`? (Probably factor out the size calculation into a local variable)
> It's not possible for the size to require greater than 5 bytes because it is an unsigned 32-bit value. Each byte of a LEB has 7 significant bits and ceil(32/7) = 5. Indeed the Wasm spec does not allow using LEBs larger than 5 bytes to encode 32-bit values.
Fair enough. FWIW, a ULEB has no technical upper limit on its size, so the constraint is in the wasm spec and what we implement rather than the theoretical what the user could write here, I guess.


================
Comment at: llvm/test/ObjectYAML/wasm/section_header_size.yaml:82
+  - Type:            TYPE
+  # INVALID: yaml2obj: error: section length can't be encoded in a LEB of size 0
+    HeaderSecSizeEncodingLen:   0
----------------
Two nits:

1) I personally wouldn't mix the check line in with the yaml. I think most readers would expect it to appear immediately after the RUN line, or after the end of the YAML doc (I personally prefer the former). However, I don't feel strongly about this.

2) Should this be "section header length" rather than "section length"?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155535