-
Notifications
You must be signed in to change notification settings - Fork 827
[EH][GC] Send a non-nullable exnref from TryTable #7013
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
Conversation
We confirmed in WebAssembly/exception-handling#336 that the sent type can be non-nullable when GC is enabled, but should it be non-nullable? |
|
@aheejin In general, I think yes: We try to refine as much as possible as we go, as it opens up more opportunities (e.g. a more refined target of a TryTable might have a RefAsNonNull outside of it, which could be removed). There are situations where less-refined types are better, and I think we have plans for such a pass to run at the very end, but I can't think of a specific downside to always refining TryTable. |
|
|
||
| const char* invalidSentTypeMsg = "invalid catch sent type information"; | ||
| Type exnref = Type(HeapType::exn, Nullable); | ||
| Type exnref = Type(HeapType::exn, NonNullable); |
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 understand it's better to make types as refined as possible for optimization, but shouldn't we not error error out on nullable exnref in the validator? I think we also generate nullable exnrefs in the fuzzer. I think we should leave it as is and check for not the equality but the subtype below (in line 2696)
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 think we should be sending a non-nullable exnref. The target might be less refined (until ReFinalize fixes that) but we should still be sending the refined type, as the spec says?
Unless I missed something in the code here and it is checking something other than the type being sent?
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.
If I'm misunderstanding you, can you give an example of code that should validate but this code will not accept?
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 think I might have misunderstood what this PR does. So basically, from now on we assume exnref == (ref exn) and not (ref null exn) and treat it as such in the IR and all passes, and make it (ref null exn) only when we write it back to a binary. Right? (By the way don't we need to do the same thing when we write it back as a text?)
If that's the case, if you programatically create a TryTable object with exnref in sentType field, it will not validate. Of course, we don't usually manually set the fields in the IR classes; we usually use constructors and finalize methods, so I guess practically it's fine, but it was a little confusing to me. (Maybe giving TryTable::sentType a private visibility might help a little.)
I think it'd help if we have little more comments in wasm.cpp that why we are doing this. Also, can't we also validate (ref null exn) in sentType as valid? (I can't give you an example wast program that does not validate, because we basically read exnref as (ref exn) when reading.)
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.
exnref should be equivalent to (ref null exn), not (ref exn). Since try_table never produces a null exn value, the most precise type for the exn value it produces is always (ref exn). We can therefore take it as an invariant that the IR models sending a (ref exn). However, non-nullable references like that do not exist without GC enabled, so without GC, we fix it up by transforming the (ref exn) into exnref (i.e. (ref null exn)) in the binary writer.
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 added a comment now. I also noticed we were missing docs on this in the readme and added those.
When EH+GC are enabled then wasm has non-nullable types, and the
sent exnref should be non-nullable. In BinaryenIR we use the non-
nullable type all the time, which we also do for function references
and other things; we lower it if GC is not enabled to a nullable type
for the binary format (see
WasmBinaryWriter::writeType, to whichcomments were added in this PR). That is, this PR makes us handle
exnref the same as those other types.
A new test verifies that behavior. Various existing tests are updated
because ReFinalize will now use the more refined type, so this is
an optimization. It is also a bugfix as in #6987 we started to emit
the refined form in the fuzzer, and this PR makes us handle it
properly in validation and ReFinalization.