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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Nearly there. Just a couple of small points now from me.



================
Comment at: llvm/lib/ObjectYAML/WasmEmitter.cpp:650
+    unsigned HeaderSecSizeEncodingLen =
+        Sec->HeaderSecSizeEncodingLen ? *Sec->HeaderSecSizeEncodingLen : 0;
+    if (HeaderSecSizeEncodingLen < getULEB128Size(OutString.size())) {
----------------
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)


================
Comment at: llvm/lib/ObjectYAML/WasmEmitter.cpp:652
+    if (HeaderSecSizeEncodingLen < getULEB128Size(OutString.size())) {
+      reportError("section length can't be encoded in an LEB of size " +
+                  Twine(HeaderSecSizeEncodingLen));
----------------
Nit: I'd tend to say "leb" rather than "el-ee-bee" for "LEB", so "an" -> "a", but if you pronounce it differently, I'm fine with this staying as is.


================
Comment at: llvm/test/ObjectYAML/wasm/invalid_section_header_size.yaml:1
+## Test if we correctly error out if the provided section header size is less
+## than the size required.
----------------
Rather than spin this off into a separate test file, I think it should be part of the existing section_header_size.yaml test. You could use either --docnum to allow you to have multiple YAML docs in the same file, or you could use yaml2obj's -D option to parmaterise an existing section header length field to be the appropriate too-small value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155535