-
Notifications
You must be signed in to change notification settings - Fork 820
Make imported functions inexact #7993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Update the Literal constructors for funcrefs to take Type instead of HeapType to allow them to be given inexact function references types when the referenced function is an import. Use the new capability to give references to imported functions inexact types in GUFA. Add a test where this change fixes a misoptimization as well as tests where this change simply changes the nature of the misoptimization. Future PRs will fix these tests.
|
Fuzzer noticed that we didn't type imported ref.funcs correctly. Last two (non-merge) commits fix and test that. |
| // This is imported, so it might be anything of the proper type. | ||
| addRoot(curr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we still want to track that this is a reference to the imported function? It would just have an inexact type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But without knowing the identity, I think we can misoptimize? Imagine we have equality for a second, then ref.func a == ref.func a is definitely 1, but ref.func a == ref.func b is not necessarily 0 (can have duplicate imports).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But functions references cannot be compared for equality, so there is nothing to misoptimize, unless I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did say "imagine" 😆
But, while we don't have ref.eq on functions in wasm userspace, we do have optimizations that compare functions in other ways. E.g. folding an if with ref.eq arms, or GUFA inferences. I admit I don't see an actual bug atm in our optimizer, but a future one is conceivable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how optimizations might see that two references are the same and e.g. merge two equivalent if arms or something like that, but I still don't see how we could ever have an optimization that does something unsafe when reasoning that two different function references are different. Can we at least consider changes here in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agreed. I'm not opposed to being conservative here to be on the safe side. But we should keep the less-conservative status quo in this PR to make sure we're not unexpectedly regressing optimizations due to just the introduction of inexact imported functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the status quo does mean keeping the known cases of invalid optimization we have today, including the new gufa.wast tests here, like
(module
(type $func (sub (func)))
(type $sub (sub $func (func)))
(import "" "" (func $f (type $func)))
(func $test (export "test") (result i32)
(ref.test (ref $sub)
(ref.func $f)
)
)
)We misoptimize that to 0 before the fix, because we think imported function literals are actual concrete functions. Given such an actual function, we don't need exactness to know that it will fail that test.
The fuzzer can find this after this PR - perhaps because of the new testcases? Or perhaps because of the companion fuzzing PR #7963, which should really land as it increases coverage enough to find those recent vulnerabilities. So while I see your point, our options seem to be
- Land this PR as is, fixing the misoptimization but potentially regressing optimizations on imported function references.
- Land this without fixing the misoptimization, which will not regress any opts, and work around it in the fuzzer, maybe not landing Fuzzer: Merge and optimize even with closed world in Two() #7963, maybe marking new tests as non-fuzzable, maybe both.
- Fix the misoptimization otherwise, e.g., GUFA/possible-contents could special case function literals in various places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if GUFA has a function literal, but its type isn't exact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still a literal. The code assumes that a literal is an actual identifiable thing, like 42 or the function "foo", and unlike a global "bar" (whose value we don't know).
We could special-case the code to make it treat an inexact funcref as "a literal, but not really; more like a global." But that won't work once we have exact function imports - the same problem would happen with exact ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the problem. Once we have exact imports, then the Literal for the imported function would have an exact type iff the import is exact. GUFA would then look at the literal type to see whether casts would succeed or fail, for example. The changes to support inexact function literals are already in this PR.
| @@ -0,0 +1,36 @@ | |||
| ;; Import a function of type $C as type $A, cast to $A, $B, $C. All those casts | |||
| ;; should succeed. Exact casts, however TODO | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, just some old text. These casts become exact. Cleaned up now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add some exact casts to the test to show that they work as expected. I don't think the test currently demonstrates that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, added.
| ;; CHECK-NEXT: (ref.test (ref (exact $func)) | ||
| ;; CHECK-NEXT: (ref.func $f) | ||
| ;; CHECK-NEXT: ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment suggests we should have been able to optimize this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I think we need to look at finality in GUFA somehow. I added a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this will be fixed by using a non-exact Literal for references to function imports.
Co-authored-by: Thomas Lively <[email protected]>
Co-authored-by: Thomas Lively <[email protected]>
This reverts commit 0a93101.
Defined functions remain exact, but imported ones are inexact.
This is a step along the recent Custom Descriptors spec changes.
RefFunc::finalizeandLiteral::makeFuncvariants get the module, and look upthe type there.
Builder::makeRefFuncvariant gets a Type and applies it. The HeapTypevariant does a lookup on the module (so the Type one is more efficient/applicable
if the IR is not fully built yet).
look up the type of the import; see changelog, this seems the least-annoying way to
update here, avoiding new APIs, and less breakage for users - hopefully none, all our
tests here pass as is).