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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin marked an inline comment as done.
aheejin added inline comments.


================
Comment at: llvm/lib/ObjectYAML/WasmEmitter.cpp:650
+    unsigned HeaderSecSizeEncodingLen =
+        Sec->HeaderSecSizeEncodingLen ? *Sec->HeaderSecSizeEncodingLen : 0;
+    if (HeaderSecSizeEncodingLen < getULEB128Size(OutString.size())) {
----------------
jhenderson wrote:
> 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.
Changed to this so we assert the required length should not be greater than 5 bytes:
```
    unsigned RequiredLen = getULEB128Size(OutString.size());                   
    // Wasm spec does not allow LEBs larger than 5 bytes
    assert(RequiredLen <= 5);                                                   
    if (HeaderSecSizeEncodingLen < RequiredLen) {                     
      reportError("section length can't be encoded in a LEB of size " +
                  Twine(HeaderSecSizeEncodingLen));                             
      return false;                                                             
    }  
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155535