[PATCH] D155535: [WebAssembly][Objcopy] Write output section headers identically to inputs
Heejin Ahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 27 15:28:00 PDT 2023
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
More information about the llvm-commits
mailing list