[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/lib/ObjectYAML/WasmEmitter.cpp:652
     // Write the section size followed by the content
-    encodeULEB128(OutString.size(), OS);
+    encodeULEB128(OutString.size(), OS, HeaderSecSizeEncodingLen);
     OS << OutString;
----------------
jhenderson wrote:
> What happens if the encoding length is smaller than what is actually required to encode the size? I assume it either throws some kind of error somehow (no idea how), or silently writes size in full without truncation. If the latter, I think it might be worth reporting an error from yaml2obj, as otherwise the data won't reflect what was explicitly requested.
The length field is "padTo", meaning that extra padding will be added if necessary, but otherwise you'll get whatever the minimum length is. I added an error report here if the length can't be encoded in the specified size.


================
Comment at: llvm/tools/obj2yaml/wasm2yaml.cpp:402-403
+    if (WasmSec.HeaderSecSizeEncodingLen &&
+        WasmSec.HeaderSecSizeEncodingLen !=
+            getULEB128Size(WasmSec.Content.size()) &&
+        WasmSec.HeaderSecSizeEncodingLen != 5)
----------------
jhenderson wrote:
> Maybe I'm missing something, but doesn't this mean that there's ambiguity between whether a length of the LEB size or 5 is appropriate when the emitted YAML is converted back to an object?
No, you're correct. This does mean that a binary could fail to roundtrip from obj -> YAML -> obj. However doing it this way meant that we don't have to rewrite all of the YAML test expectations (since some components e.g. MCAssembler emit patchable 5-byte encodings, while others emit minimal-sized encodings). This potential failure to round-trip is maybe suboptimal in theory but I think it doesn't matter that much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155535