Implement array.fill, array.init_data, and array.init_elem #5637
Conversation
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
These complement array.copy, which we already supported, as an initial complete set of bulk array operations. Replace the WIP spec tests with the upstream spec tests, lightly edited for compatibility with Binaryen.
src/ir/possible-contents.cpp
Outdated
| @@ -988,7 +988,8 @@ struct InfoCollector | |||
| auto* set = builder.makeArraySet(curr->destRef, curr->destIndex, get); | |||
| visitArraySet(set); | |||
| } | |||
|
|
|||
| void visitArrayFill(ArrayFill* curr) { WASM_UNREACHABLE("unimplemented"); } | |||
| void visitArrayInit(ArrayInit* curr) { WASM_UNREACHABLE("uimplemented"); } | |||
There was a problem hiding this comment.
The fuzzer will crash on these (any testcase with the instructions + --gufa).
I think their implementation can be similar to ArraySet (edit: or ArrayCopy)
There was a problem hiding this comment.
What should I do to model reading from the data or element segment in ArrayInit?
There was a problem hiding this comment.
Looks like TableGet just models the value as a root (an unknown value that could be anything). I think we should do the same here. But that value would need to be written into the array's storage instead of just declaring this expression to be a root.
If it doesn't look obvious how to do that I can look into it. I'm not sure if it will be trivial or not.
There was a problem hiding this comment.
Thanks. I'm not sure how to hook up the root value with the synthetic write, so I'll leave that to you and look forward to seeing what's involved.
There was a problem hiding this comment.
My fuzzing did find this after 7591 iterations, so at least it's rare.
There was a problem hiding this comment.
Ah, I was hoping to fix this before it lands, as that frequency still means it'll be found many times a day. But no worries, I'll look into this right now.
There was a problem hiding this comment.
No problem, I think I found the solution already, PR is up (#5638)
| if (offsetVal + readSize > seg->data.size()) { | ||
| trap("out of bounds segment access in array.init_data"); | ||
| } | ||
| if (offsetVal + sizeVal > 0 && droppedSegments.count(curr->segment)) { |
There was a problem hiding this comment.
Should offsetVal + sizeVal be just sizeVal? If the size is 0, it seems like the offset should not matter.
There was a problem hiding this comment.
No, dropped segments semantically have their length truncated to zero, so if offset and size are both zero, there's no trap, but if either is nonzero there will be a trap. This could perhaps be more clearly expressed, but this code is copied from the implementation of MemoryInit.
There was a problem hiding this comment.
I see. Maybe my confusion is about the spec then. It seems odd to trap on trying to load 0 items from offset 17 but not to trap on trying to load 0 items from offset 0 (since both do no actual reads).
There was a problem hiding this comment.
It's about keeping the bounds checks uniform in the spec. For the spec, the only rule is "trap if offset + length is out of bounds." Since there are no special cases for zero sized accesses, you get the somewhat counterintuitive behavior of trapping on out-of-bounds but not in-bounds or at-bounds zero-sized accesses. Combine that with the bounds changing for dropped segments, and you get some pretty weird behavior for anything that doesn't model things exactly like the spec.
| local.get 2 | ||
| local.get 3 | ||
| array.fill $a2 | ||
| ) |
There was a problem hiding this comment.
just fill for now and not init*?
There was a problem hiding this comment.
We don't parse element segment references yet, so I left ArrayInit as future work.
|
Fuzzer found a bug: (module
(type $[mut:i32] (array (mut i32)))
(type $ref|[mut:i32]|_i32_i32_i32_=>_none (func (param (ref $[mut:i32]) i32 i32 i32)))
(global $global$0 (mut i32) (i32.const -69))
(global $global$1 (mut i32) (i32.const 100))
(memory $0 (shared 16 17))
(data $0 "")
(data $1 (i32.const 0) "")
(data $2 (i32.const 14) "")
(data $3 (i32.const 30) "")
(func $0 (param $0 (ref $[mut:i32])) (param $1 i32) (param $2 i32) (param $3 i32)
(array.init_data $[mut:i32] $0
(local.get $0)
(i32.const 0)
(i32.const 0)
(i32.const 1)
)
)
) |
These complement array.copy, which we already supported, as an initial complete
set of bulk array operations. Replace the WIP spec tests with the upstream spec
tests, lightly edited for compatibility with Binaryen.