GUFA: Emit RefAsNonNull instead of RefCast where possible #6434
Conversation
test/lit/passes/gufa-cast-all.wast
Outdated
| ;; These params are only called with non-null values, so we can cast them | ||
| ;; to non-null. We must not use ref.cast for that, which is disallowed on | ||
| ;; strings and views. |
There was a problem hiding this comment.
Make these params anyref and we still have a problem, though, right? This PR is a good optimization improvement (although I think OptimizeInstructions makes the same improvement), but it doesn't really solve the underlying problem.
There was a problem hiding this comment.
I don't follow - what problem do you mean?
If you mean a problem with a string/view that flows into anyref, then that isn't the issue I observed in the fuzz bug, it was just this specific thing: that we emitted a ref.cast in order to force a string/view to be non-nullable. string/view flowing into an anyref is a separate issue though, if a codebase does that, of course.
There was a problem hiding this comment.
Wouldn't we still emit a ref.cast targeting string views if the params were anyref?
There was a problem hiding this comment.
Are you saying if we passed string views into anyref params? Yes, that can be a problem, as I agreed, but it doesn't occur in practice (yet?) unlike the problem this PR fixes.
Or, if I've misunderstood you, can you explain what you mean, maybe with a testcase?
There was a problem hiding this comment.
I'm not saying it happens in practice, but the fundamental bug here seems to be that we consider the string views to be subtypes of any while V8 does not. GUFA emitting ref.cast for them follows from that bug, and this PR works around that in a particular case, but not in general.
This PR seems fine, but I see it as partially addressing a symptom of the underlying bug, so we should still fix the underlying bug as well.
Another fix for the particular symptom would be to run OptimizeInstructions after GUFA.
There was a problem hiding this comment.
Yeah, I agree this works around the issue. But it does it in a useful way - even though this overlaps with OptimizeInstructions, it's a small addition, and it makes the output here more idiomatic (and, actually valid even without running OptimizeInstructions). I added a comment for that.
There was a problem hiding this comment.
(And I don't know how to fix the underlying issue - we were discussing it might need various internalize/externalize operations... but as it doesn't happen in practice, I hope we don't need it.)
| if (oracleType.getHeapType() == curr->type.getHeapType()) { | ||
| // These only differ in nullability: use RefAsNonNull. (We do not | ||
| // rely on OptimizeInstructions to turn a RefCast into a | ||
| // RefAsNonNull, as there are cases where RefCast is actually not |
There was a problem hiding this comment.
| // RefAsNonNull, as there are cases where RefCast is actually not | |
| // RefAsNonNull, as there are unfortunately cases where RefCast is actually not |
|
Why is #6440 needed in addition to this - what does this miss? |
|
But GUFA would only emit a cast from anyref to a stringview if a stringview was actually written to an anyref slot? But we don't do that in the fuzzer in practice, which is why I wasn't worried about it. Regardless, yeah, good point that fixing the type system makes sense as you do there. Meanwhile I wonder if another option instead of this PR is to fix the issue once in the binary writer: anytime we see |
|
Yeah, if we had the binary writer optimize no-op casts out or to ref.as_non_null, then combined with #6440 that would entirely eliminate the possibility that we would ever emit a cast to a stringview type. |
|
Closing in favor of #6549 which replaces |
…uzzing (#6549) As suggested in #6434 (comment) , lower ref.cast of string views to ref.as_non_null in binary writing. It is a simple hack that avoids the problem of V8 not allowing them to be cast. Add fuzzing support for the last three core string operations, after which that problem becomes very frequent. Also add yet another makeTrappingRefUse that was missing in that fuzzer code.
2 participants
No description provided.