[Wasm GC] Fuzz BrOn #7006
[Wasm GC] Fuzz BrOn #7006
Conversation
| AtomicFence : 1 | ||
| Binary : 77 | ||
| Block : 79 | ||
| BrOn : 1 |
There was a problem hiding this comment.
(easy come, easy go... update lost it)
src/tools/fuzzing/fuzzing.cpp
Outdated
| &Self::makeRefCast); | ||
| } | ||
| if (wasm.features.hasGC()) { | ||
| options.add(FeatureSet::ReferenceTypes | FeatureSet::GC, &Self::makeBrOn); |
There was a problem hiding this comment.
This should probably go up in the if (canMakeControlFlow) block rather than here. No need to explicitly guard on wasm.features.hasGC(), since that guard is already built into the option.
| .add(FeatureSet::GC | FeatureSet::ReferenceTypes, &Self::makeCallRef) | ||
| .add(FeatureSet::GC | FeatureSet::ReferenceTypes, &Self::makeStructSet) | ||
| .add(FeatureSet::GC | FeatureSet::ReferenceTypes, &Self::makeArraySet) | ||
| .add(FeatureSet::ReferenceTypes | FeatureSet::GC, &Self::makeBrOn) |
There was a problem hiding this comment.
We should make the order of the features consistent with those in the surrounding lines.
There was a problem hiding this comment.
Good point. I fixed up a few more as well, all going with "references | GC", based on the logic of using the same order as the order of the proposals.
| case BrOnNonNull: { | ||
| // The sent type is the non-nullable version of the reference, so any ref | ||
| // of that type is ok, nullable or not. | ||
| refType = Type(targetType.getHeapType(), getNullability()); |
There was a problem hiding this comment.
Might as well hard-code Nullable here and let make() choose to make a non-nullable subtype later.
There was a problem hiding this comment.
Not sure I follow. That might conflict with the behavior of the other variants, as they all share the same make at the bottom?
It seems simpler to pick random nullability here, which lets refType be equal to the type that make will produce later.
There was a problem hiding this comment.
The first thing make does is choose a random subtype of the requested type, though. Choosing a random subtype of the allowed type before calling make is therefore redundant. (And I guess actually this applies to where we now randomly pick a subtype when setting refType in the BrOnCast and BrOnCastFail cases as well.)
There was a problem hiding this comment.
Fair point. Though I'm not sure I'd like to depend on the behavior of make always picking a subtype. E.g. maybe in the future we'll make it do that only rarely, or something like that.
src/tools/fuzzing/fuzzing.cpp
Outdated
| castType = getSubType(targetType); | ||
| // The ref's type must be castable to the target, or we'd not validate. | ||
| refType = getSuperType(castType); |
There was a problem hiding this comment.
Maybe with some small probability we should have the refType be a subtype of the castType, which is valid and just makes the cast trivially succeed.
src/tools/fuzzing/fuzzing.cpp
Outdated
| if (targetType.isNonNullable()) { | ||
| // And it must have the right nullability for the target, as mentioned | ||
| // above. | ||
| refType = Type(refType.getHeapType(), NonNullable); |
There was a problem hiding this comment.
If the target type is non-nullable, either the refType or the castType must be non-nullable, but it's not necessary for them both to be non-nullable. We should be able to generate it all three ways.
src/tools/fuzzing/fuzzing.cpp
Outdated
| castType = getSubType(refType); | ||
| // There is no nullability to adjust: if targetType is non-nullable then | ||
| // both refType and castType are as well, as subtypes of it. |
There was a problem hiding this comment.
If the targetType is non-nullable, then castType should be allowed to be nullable.
2 participants
This found #6995