Skip to content
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

[EH] Add exnref type back #6149

Merged
merged 6 commits into from
Dec 8, 2023
Merged

[EH] Add exnref type back #6149

merged 6 commits into from
Dec 8, 2023

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Dec 7, 2023

At the Oct hybrid CG meeting, we decided to add back exnref, which was removed in 2020:
https://github.com/WebAssembly/meetings/blob/main/main/2023/CG-10.md

The new version of the proposal reflected in the explainer: https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md

While adding support for exnref in the current codebase which has all GC subtype hierarchies, I noticed we might need noexn heap type for the bottom type of exn. We don't have it now so I just set it to 0xff for the moment.

At the Oct hybrid CG meeting, we decided to add back `exnref`, which was
removed in 2020:
https://github.com/WebAssembly/meetings/blob/main/main/2023/CG-10.md

The new version of the proposal reflected in the explainer:
https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md

While adding support for `exnref` in the current codebase which has all
GC subtype hierarchies, I noticed we might need `noexn` heap type for
the bottom type of `exn`. We don't have it now so I just set it to 0x00
for the moment.
@aheejin aheejin requested a review from tlively December 7, 2023 00:44
@aheejin aheejin marked this pull request as draft December 7, 2023 02:38
@@ -917,6 +923,10 @@ FeatureSet Type::getFeatures() const {
// ref.null with just reference types.
feats |= FeatureSet::ReferenceTypes;
return;
case HeapType::exn:
case HeapType::noexn:
feats |= FeatureSet::ExceptionHandling;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we say that this requires reference types as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to port this test to lit, either as this PR or a separate PR? This test is an example where we did that port without losing any coverage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do in a follow-up!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aheejin aheejin marked this pull request as ready for review December 8, 2023 03:38
@aheejin aheejin merged commit 1d615b3 into WebAssembly:main Dec 8, 2023
14 checks passed
@aheejin aheejin deleted the new_exnref branch December 8, 2023 23:03
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
At the Oct hybrid CG meeting, we decided to add back `exnref`, which was
removed in 2020:
https://github.com/WebAssembly/meetings/blob/main/main/2023/CG-10.md

The new version of the proposal reflected in the explainer:
https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md

While adding support for `exnref` in the current codebase which has all
GC subtype hierarchies, I noticed we might need `noexn` heap type for
the bottom type of `exn`. We don't have it now so I just set it to 0xff
for the moment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants