Skip to content

MemoryPacking changes d8 results #2528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
aheejin opened this issue Dec 14, 2019 · 3 comments · Fixed by #2529
Closed

MemoryPacking changes d8 results #2528

aheejin opened this issue Dec 14, 2019 · 3 comments · Fixed by #2529

Comments

@aheejin
Copy link
Member

aheejin commented Dec 14, 2019 •
edited
Loading

The original file, a.wasm, when run with d8,

d8 --experimental-wasm-eh --experimental-wasm-mv --experimental-wasm-sat-f2i-conversions --experimental-wasm-se --experimental-wasm-threads --experimental-wasm-simd --experimental-wasm-anyref --experimental-wasm-bulk-memory --experimental-wasm-return-call a.js -- a.wasm

prints this:

[fuzz-exec] calling $hashMemory
[fuzz-exec] note result: $hashMemory => i32.const -873354128
[fuzz-exec] calling $memory
exception!
[fuzz-exec] calling $func_5_invoker
[LoggingExternalInterface logging i32.const -873354128]
[fuzz-exec] calling $func_8
[fuzz-exec] note result: $func_8 => i32.const -56
[fuzz-exec] calling $func_9_invoker
[fuzz-exec] calling $func_11
[LoggingExternalInterface logging f64.const -1.1754943508222875e-38]
[LoggingExternalInterface logging i32.const -1073741824]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[fuzz-exec] note result: $func_11 => i32.const -32768
[fuzz-exec] calling $func_11_invoker
[LoggingExternalInterface logging f64.const -1.1754943508222875e-38]
[LoggingExternalInterface logging i32.const -1073741824]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[LoggingExternalInterface logging i32.const -873354128]
[fuzz-exec] calling $hangLimitInitializer

But if we run --memory-packing on this,

wasm-opt -all --memory-packing a.wasm -o b.wasm

and run d8 on it:

d8 --experimental-wasm-eh --experimental-wasm-mv --experimental-wasm-sat-f2i-conversions --experimental-wasm-se --experimental-wasm-threads --experimental-wasm-simd --experimental-wasm-anyref --experimental-wasm-bulk-memory --experimental-wasm-return-call a.js -- b.wasm

The result changes:

[fuzz-exec] calling $hashMemory
[fuzz-exec] note result: $hashMemory => i32.const -873354128
[fuzz-exec] calling $memory
exception!
[fuzz-exec] calling $func_5_invoker
[LoggingExternalInterface logging i32.const -873354128]
[fuzz-exec] calling $func_8
[fuzz-exec] note result: $func_8 => i32.const -56
[fuzz-exec] calling $func_9_invoker
[fuzz-exec] calling $func_11
[LoggingExternalInterface logging f64.const -1.1754943508222875e-38]
exception!
[fuzz-exec] calling $func_11_invoker
[LoggingExternalInterface logging f64.const -1.1754943508222875e-38]
exception!
[fuzz-exec] calling $hangLimitInitializer

I attached a.wasm and a.js here.
a.wasm.tar.gz
a.js.tar.gz
@aheejin aheejin assigned aheejin and unassigned aheejin Dec 14, 2019
@aheejin
Copy link
Member Author

aheejin commented Dec 15, 2019

Turned out this is not a Binaryen bug but a V8 bug. V8 does not trap when dropping an active segment, when it is supposed to. It also doesn't trap when it drops a segment twice. I filed a bug in V8.

@conrad-watt
Copy link

conrad-watt commented Dec 15, 2019 •
edited
Loading

I believe this is an intended behaviour on V8's part. The drop operation was recently changed to behave as you describe (repeated dropping is a no-op). (WebAssembly/bulk-memory-operations#126)

Maybe @tlively has insight, since that spec change PR as a whole was intended to facilitate memory-packing optimisations. (WebAssembly/bulk-memory-operations#124)

@aheejin
Copy link
Member Author

aheejin commented Dec 15, 2019

Thanks! I didn't know about the recent spec changes. I'll fix Binaryen interpreter to match the V8's behavior.

aheejin added a commit to aheejin/binaryen that referenced this issue Dec 15, 2019
@aheejin
Implement bulk memory semantic changes
d0ed83f 
This implements recent bulk memory spec changes
(WebAssembly/bulk-memory-operations#126) in Binaryen. Now `data.drop` is
equivalent to shrinking a segment size to 0, and dropping already
dropped segments or active segments (which are thought to be dropped in
the beginning) is treated as a no-op. And all bounds checking is
performed in advance, so partial copying/filling/initializing does not
occur.

I tried to implement `visitDataDrop` in the interpreter as
`segment.data.clear();`, which is exactly what the revised spec says. I
didn't end up doing that because this also deletes all contents from
active segments, and there are cases we shouldn't do that:
- `wasm-ctor-eval` shouldn't delete active segments, because it will
  store the changed contents back into segments
- When `--fuzz-exec` is given to `wasm-opt`, it runs the module and
  compare the execution call results before and after transformations.
  But if running a module will nullify all active segments, applying
  any transformation to the module or re-running it does not make any
  sense.

Fixes WebAssembly#2528.
aheejin added a commit to aheejin/binaryen that referenced this issue Dec 15, 2019
@aheejin
Implement 0-len/drop spec changes in bulk memory
bd41a46 
This implements recent bulk memory spec changes
(WebAssembly/bulk-memory-operations#126) in Binaryen. Now `data.drop` is
equivalent to shrinking a segment size to 0, and dropping already
dropped segments or active segments (which are thought to be dropped in
the beginning) is treated as a no-op. And all bounds checking is
performed in advance, so partial copying/filling/initializing does not
occur.

I tried to implement `visitDataDrop` in the interpreter as
`segment.data.clear();`, which is exactly what the revised spec says. I
didn't end up doing that because this also deletes all contents from
active segments, and there are cases we shouldn't do that:
- `wasm-ctor-eval` shouldn't delete active segments, because it will
  store the changed contents back into segments
- When `--fuzz-exec` is given to `wasm-opt`, it runs the module and
  compare the execution call results before and after transformations.
  But if running a module will nullify all active segments, applying
  any transformation to the module or re-running it does not make any
  sense.

Fixes WebAssembly#2528.
@aheejin aheejin mentioned this issue Dec 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement 0-len/drop spec changes in bulk memory aheejin/binaryen
2 participants
@conrad-watt @aheejin