Skip to content
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.

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

MemoryPacking: Removing empty segments can remove a trap #6230

Closed
kripken opened this issue Jan 19, 2024 · 1 comment · Fixed by #6243
Closed

MemoryPacking: Removing empty segments can remove a trap #6230

kripken opened this issue Jan 19, 2024 · 1 comment · Fixed by #6243

Comments

@kripken
Copy link
Member

kripken commented Jan 19, 2024

(module
 (memory $0 16 17)
 (data $0 (i32.const -1) "")
)
$ bin/wasm-opt a.wat --memory-packing --print
(module
 (memory $0 16 17)
)

Before the optimization this module traps during startup, as the segment offset is out of bounds.

We should probably make MemoryPacking not remove such segments unless TrapsNeverHappen is set, but perhaps there is a better idea?

@kripken
Copy link
Member Author

kripken commented Jan 22, 2024

This is also an issue in remove-unused-module-elements:

(module
 (import "fuzzing-support" "log-i32" (func $fimport$0 (param i32)))

 (memory $0 16 17 shared)
 (data $1 (i32.const -1) "")

 (export "func_13_invoker" (func $0))

 (func $0
  (call $fimport$0
   (i32.const 0)
  )
 )
)
$ bin/wasm-opt a.wat -all --fuzz-exec --remove-unused-module-elements
[trap out of bounds memory access in memory.init]
[fuzz-exec] calling func_13_invoker
[LoggingExternalInterface logging 0]
logging counts not identical!
[fuzz-exec] optimization passes changed results

kripken added a commit that referenced this issue Jan 25, 2024
#6242)

An out of bounds active segment traps during startup, which is an effect we must
preserve.

To avoid a regression here, ignore this in TNH mode (where the user assures us
nothing will trap), and also check if a segment will trivially be in bounds and not
trap (if so, it can be removed).

Fixes the remove-unused-module-elements part of #6230

The small change to an existing testcase made a segment there be in bounds,
to avoid this affecting it. Tests for this are in a new file.
kripken added a commit that referenced this issue Jan 25, 2024
They might trap. Leave that for RemoveUnusedModuleElements.

Fixes #6230
radekdoulik pushed a commit to dotnet/binaryen that referenced this issue Jul 12, 2024
WebAssembly#6242)

An out of bounds active segment traps during startup, which is an effect we must
preserve.

To avoid a regression here, ignore this in TNH mode (where the user assures us
nothing will trap), and also check if a segment will trivially be in bounds and not
trap (if so, it can be removed).

Fixes the remove-unused-module-elements part of WebAssembly#6230

The small change to an existing testcase made a segment there be in bounds,
to avoid this affecting it. Tests for this are in a new file.
radekdoulik pushed a commit to dotnet/binaryen that referenced this issue Jul 12, 2024
They might trap. Leave that for RemoveUnusedModuleElements.

Fixes WebAssembly#6230
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant