Fix i32 sign extension in MemoryPacking pass#8368
Fix i32 sign extension in MemoryPacking pass#8368sumleo wants to merge 2 commits intoWebAssembly:mainfrom
Conversation
src/passes/MemoryPacking.cpp
Outdated
| if (offset && size) { | ||
| uint64_t offsetVal(offset->value.geti32()); | ||
| uint64_t sizeVal(size->value.geti32()); | ||
| uint64_t offsetVal(uint32_t(offset->value.geti32())); |
There was a problem hiding this comment.
These can all be in the form auto x = value->getUnsigned(), which would be simpler.
There was a problem hiding this comment.
Done — switched to getUnsigned() for all four occurrences. Also cleaned up the existing uint32_t() casts on lines 458/461 to use getUnsigned() consistently.
Use getUnsigned() to properly zero-extend i32 values when computing memory.init offset and size. Previously, geti32() returned a signed int32_t that sign-extended when stored as uint64_t, causing values >= 0x80000000 to produce incorrect overflow detection and range calculations. Also clean up existing uint32_t() casts in the same function to use getUnsigned() consistently.
sumleo
force-pushed
the
fix-memory-packing-sign-extension
branch
from
0a19127 to
a3eb99c
Compare
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
lgtm, thanks!
Though before landing, I have a question for @tlively - how did we not see crashes with running this pass on memory64? All the places that assume a segment offset is 32-bit were wrong, weren't they?
And we do run this pass on memory64, each time e.g. emscripten compiles memory64 code?
|
My guess would be that most programs do not have static data with addresses at or above 0x8000000, so there is no difference between the incorrect sign extension and the correct zero extension. @sumleo, would it be possible to add a regression test for this fix? |
Add a test covering memory64 data segment offsets with the high bit set (>= 0x80000000) to verify they are treated as unsigned values and not sign-extended to 64 bits.
|
Added a regression test in |
3 participants
Summary
geti32()results when assigned touint64_t/size_tin MemoryPacking passvisitMemoryInit(line 465-466):offsetValandsizeValsign-extend for values >=0x80000000, causing validmemory.initinstructions to be incorrectly replaced with trapscreateSplitSegments(line 713-714): same pattern corrupts start/end range calculation for segment splittingThe fix casts through
uint32_tbefore widening, consistent with the pattern already used at lines 458 and 461 in the same function.Test plan