Skip to content

Commit

Permalink
Fix the side effects of data.drop (WebAssembly#2996)
Browse files Browse the repository at this point in the history
We marked it as readsMemory so that it could be reordered with various
things, except for memory.init. However, the fuzzer found that's not quite
right, as it has a global side effect - memory.inits that run later can notice
that. So it can't be reordered with anything that might affect global side
effects from happening, as in the testcase added here (an instruction that
may trap cannot be reordered with a data.drop, as it may prevent the
data.drop from happening and changing global state).

There may be a way to optimize this more carefully that would allow more
optimizations, but as this is a rare instruction I'm not sure it's worth more
work.
  • Loading branch information
kripken committed Jul 28, 2020
1 parent 26f240c commit 63d60fe
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 8 deletions.
6 changes: 4 additions & 2 deletions src/ir/effects.h
Expand Up @@ -392,8 +392,10 @@ struct EffectAnalyzer
}
}
void visitDataDrop(DataDrop* curr) {
// prevent reordering with memory.init
readsMemory = true;
// data.drop does not actually write memory, but it does alter the size of
// a segment, which can be noticeable later by memory.init, so we need to
// mark it as having a global side effect of some kind.
writesMemory = true;
if (!ignoreImplicitTraps) {
implicitTrap = true;
}
Expand Down
27 changes: 24 additions & 3 deletions test/passes/simplify-locals_all-features.txt
Expand Up @@ -1837,13 +1837,15 @@
)
(func $data-drop-load
(local $x i32)
(nop)
(data.drop 0)
(drop
(local.set $x
(i32.load
(i32.const 0)
)
)
(data.drop 0)
(drop
(local.get $x)
)
)
(func $data-drop-store
(local $x i32)
Expand Down Expand Up @@ -2019,3 +2021,22 @@
)
)
)
(module
(type $none_=>_i32 (func (result i32)))
(memory $0 (shared 1 1))
(data passive "data")
(export "foo" (func $0))
(func $0 (result i32)
(local $0 i32)
(block $block (result i32)
(local.set $0
(i32.rem_u
(i32.const 0)
(i32.const 0)
)
)
(data.drop 0)
(local.get $0)
)
)
)
18 changes: 18 additions & 0 deletions test/passes/simplify-locals_all-features.wast
Expand Up @@ -1782,3 +1782,21 @@
)
)
)
;; data.drop has global side effects
(module
(memory $0 (shared 1 1))
(data passive "data")
(func "foo" (result i32)
(local $0 i32)
(block (result i32)
(local.set $0
(i32.rem_u ;; will trap, so cannot be reordered to the end
(i32.const 0)
(i32.const 0)
)
)
(data.drop 0) ;; has global side effects that may be noticed later
(local.get $0)
)
)
)
Expand Up @@ -1831,13 +1831,15 @@
)
(func $data-drop-load
(local $x i32)
(nop)
(data.drop 0)
(drop
(local.set $x
(i32.load
(i32.const 0)
)
)
(data.drop 0)
(drop
(local.get $x)
)
)
(func $data-drop-store
(local $x i32)
Expand Down

0 comments on commit 63d60fe

Please sign in to comment.