Fuzz waitqueue instructions - #9012
Conversation
e7f2a7d to
ca0f641
Compare
a046452 to
3460108
Compare
6001030 to
853b2cc
Compare
40e088c to
d51648f
Compare
96f5fe4 to
218c831
Compare
3860cae to
0624377
Compare
f1c1052 to
25b4a0d
Compare
9352c74 to
84702dd
Compare
stevenfontanella
changed the title
84702dd to
a98e6c0
Compare
|
It looks parts of this PR have been already landed in #9139? Are you planning to land this too? Then can you rebase this PR onto the current main? |
a98e6c0 to
80e9dd5
Compare
Thanks, I missed this. Done. |
|
(Reassign since Heejin is out) |
| if (fieldType == Type::i32 || fieldType == Type::i64 || | ||
| Type::isSubType( | ||
| fieldType, Type(HeapTypes::eq.getBasic(Shared), Nullable))) { |
There was a problem hiding this comment.
It might be worth pulling this condition out into a wasm-type.h helper. I assume it shows up in at least a few places in the code base.
There was a problem hiding this comment.
Done, added Field::isValidControlWord. I considered putting it on Type but we need to check if the field is packed, otherwise a packed field will look valid while it should not be.
| if (type.isRef() && type.getHeapType() == HeapTypes::sharedWaitqueue) { | ||
| return makeWaitqueueNew(); | ||
| } | ||
| TODO_SINGLE_COMPOUND(type); |
There was a problem hiding this comment.
It's not clear to me that it makes sense to use waitqueue.new here. Waitqueues have (indirectly) observable identity, and it might be the case that callers of makeConstantExpression expect that not to be the case. I would follow the lead of struct and array types and just not support waitqueues in this function.
There was a problem hiding this comment.
Sounds good, and looks like this isn't used in the fuzzer anyway. The only call to makeConstantExpression is creating a func ref link.
| if (!ATOMIC_WAITS) { | ||
| for (auto* wait : FindAll<StructWait>(func->body).list) { | ||
| if (auto* c = wait->timeout->dynCast<Const>()) { | ||
| c->value = Literal(int64_t(0)); | ||
| } else if (wait->timeout->type == Type::i64) { | ||
| wait->timeout = builder.makeSequence(builder.makeDrop(wait->timeout), | ||
| builder.makeConst(int64_t(0))); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
If we don't do this, does the fuzzer easily run into lengthy hangs? I don't believe we have a similar fixup for linear memory hangs, but it's only rarely been an issue.
There was a problem hiding this comment.
memory.atomic.wait works a little differently. memory.atomic.wait is only generated if ATOMIC_WAITS is true to begin with (FWIW this looks like a hard-coded flag that's always false), then it generates a random i64 for the timeout and has no fixup.
OTOH for struct.wait we may always generate the instruction, but ATOMIC_WAITS only determines whether the timeout can be non-0. I think this is the better way to do it since we can at least exercise the non-blocking code paths. We probably don't get hangs from memory.atomic.wait often because it would have to take an existing test that contains the instruction and mutate it to make it block (either via the timeout or the control word); it's never generated from scratch by the fuzzer while struct.wait is.
I could update memory.atomic.wait in a future PR if we want more uniformity here.
There was a problem hiding this comment.
It would be nice to make them consistent in a follow-up, thanks.
959a435 to
33c8794
Compare
|
Ran 62k fuzzer iterations with no issues. |
3 participants
Part of #8315. Generate
waitqueue.new,waitqueue.notify, andstruct.waitin the fuzzer.When
!ATOMIC_WAITS, the timeout argument on allstruct.waits is forced to be equal to 0 so that programs can't block. In this case it will return either 1 (not equal) or 2 (equal and timed out waiting), but never 0 (blocked and got notified).Ran for 57k iterations with no issues. For fuzzing against V8:
shared-everythingis already disabled when fuzzing against V8: link. Waitqueues are mostly implemented in V8 but currently only support i32 control words and not i64 or subtypes of eqref: https://chromium-review.googlesource.com/c/v8/v8/+/8449412/2.