FuzzAgainstJS: Do not refine exact to inexact defined types#8855
Conversation
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
I think this could all be much simpler (and more general) if options were a vector of std::pair<HeapType, Exactness>. Then the loop where we populate options could take exactness into account and we wouldn't have to deal with it afterward.
|
I tried that a little but I don't think it avoids complexity? We still need to handle the case of the old type beginning exact, and only allowing exact for the exact heap type in the new type, make sure not to make a basic type exact - i.e. all the same stuff? |
|
Here's what I have in mind: diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp
index 2e1e9281b..00075e05b 100644
--- a/src/tools/fuzzing/fuzzing.cpp
+++ b/src/tools/fuzzing/fuzzing.cpp
@@ -2487,32 +2487,41 @@ void TranslateToFuzzReader::mutateJSBoundary() {
if (new_ == Type::unreachable) {
new_ = Type(old.getHeapType().getBottom(), NonNullable);
}
+ assert(Type::isSubType(new_, old));
// Find all heap types between the old and new, starting from new.
auto oldHeapType = old.getHeapType();
+ auto oldExactness = old.getExactness();
auto newHeapType = new_.getHeapType();
- assert(HeapType::isSubType(newHeapType, oldHeapType));
- std::vector<HeapType> options;
+ auto newExactness = new_.getExactness();
+ std::vector<std::pair<HeapType, Exactness>> options;
while (1) {
- options.push_back(newHeapType);
+ options.push_back({newHeapType, newExactness});
// We cannot look at a bottom type's supers (there can be many, and the
// getSuperType() API doesn't return them), but can use
// interestingHeapSubTypes: any subtype of old is valid.
if (newHeapType.isBottom()) {
for (auto type : interestingHeapSubTypes[oldHeapType]) {
- options.push_back(type);
+ if (type != oldHeapType || oldExactness == Inexact) {
+ options.push_back({type, Inexact});
+ }
+ options.push_back({type, Exact});
}
break;
}
// Continue until we reach the old type.
- if (newHeapType == oldHeapType) {
+ if (newHeapType == oldHeapType && newExactness == oldExactness) {
break;
}
+ if (newExactness == Exact) {
+ newExactness = Inexact;
+ continue;
+ }
auto next = newHeapType.getSuperType();
assert(next);
newHeapType = *next;
}
- newHeapType = pick(options);
+ auto [heapType, exactness] = pick(options);
// Pick the nullability.
auto oldNullability = old.getNullability();
@@ -2521,23 +2530,7 @@ void TranslateToFuzzReader::mutateJSBoundary() {
newNullability = getNullability();
}
- // Pick the exactness.
- auto oldExactness = old.getExactness();
- auto newExactness = new_.getExactness();
- // We can only be exact if we are using the new heap type: that type is
- // exactly what is sent here, and no intermediate heap type would be valid.
- // For example, given $A :> $B :> $C, then maybeRefine($A, exact $C) can
- // return exact $C, but cannot return exact $B.
- //
- // Also, basic heap types cannot be exact.
- if (newHeapType != new_.getHeapType() || newHeapType.isBasic()) {
- newExactness = Inexact;
- } else if (newExactness != oldExactness) {
- // TODO: once getExactness() is fixed (see there), use that
- newExactness = oneIn(2) ? Exact : Inexact;
- }
-
- return Type(newHeapType, newNullability, newExactness);
+ return Type(heapType, newNullability, exactness);
};
// Given a set of types (all params or all results), and an index among them,
This is a net reduction in code. |
|
That is shorter, but iianm, given old type |
|
Sure, I guess I misplaced the new |
|
Ok, there was one more bug - in the exact case, that code didn't add the old exact type - but after fixing that, I think it is still simpler to do it in the loop as you suggested, and I checked and all other cases seem to be handled. Updated. |
|
Actually there was yet one more bug, see last commit. |
| // Regardless of the old exactness, it is valid to add the old type as | ||
| // exact. | ||
| options.push_back({oldHeapType, Exact}); |
There was a problem hiding this comment.
What about the case where new_ == old and they are inexact?
There was a problem hiding this comment.
Good point - yet another if is needed here 😄
Fixed.
There was a problem hiding this comment.
I don't think the fix addresses the problem. We could have new_ == old where they are inexact and not basic and it would still be invalid to add the exact version of the old type.
There was a problem hiding this comment.
But all this is inside if (newHeapType.isBottom()) {
2 participants
It is ok to refine
exact $Atonullref, but not to inexact$B.