Skip to content

Restore the extern heap type #4898

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Aug 18, 2022
Merged

Restore the extern heap type #4898

merged 8 commits into from
Aug 18, 2022

Conversation

tlively
Copy link
Member

@tlively tlively commented Aug 11, 2022

The GC proposal has split any and extern back into two separate types, so
reintroduce HeapType::ext to represent extern. Before it was originally
removed in #4633, externref was a subtype of anyref, but now it is not. Now that
we have separate heaptype type hierarchies, make HeapType::getLeastUpperBound
fallible as well.

@tlively tlively requested a review from kripken August 11, 2022 01:38
@tlively
Copy link
Member Author

tlively commented Aug 11, 2022 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@tlively tlively requested a review from aheejin August 11, 2022 01:39
kripken
kripken reviewed Aug 11, 2022
View reviewed changes
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Why are the tests changed here?

src/ir/possible-constant.h Outdated
@@ -114,8 +114,9 @@ struct PossibleConstantValues {
auto type = getConstantLiteral().type.getHeapType();
auto otherType = other.getConstantLiteral().type.getHeapType();
auto lub = HeapType::getLeastUpperBound(type, otherType);
if (lub != type) {
value = Literal::makeNull(lub);
assert(lub && "TODO: Handle case without LUB");
Copy link
Member

Choose a reason for hiding this comment

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

I think we should handle this in this PR, or else we'll get fuzzer failures. To handle it, if there is no LUB let's turn the result into Many().

Copy link
Member Author

@tlively tlively Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't we turn it into a None, since the types are incompatible? Although once we implement bottom types, we'll be able to make it a Literal of that bottom type instead.

Edit: I guess the conservative thing to do here is use Many for now and Literal in the future.

Copy link
Member

Choose a reason for hiding this comment

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

None means "no contents". Many means "many things are possible here", which can include many incompatible things.

src/ir/possible-contents.cpp
@@ -56,7 +56,8 @@ void PossibleContents::combine(const PossibleContents& other) {
assert(other.isNull());
auto lub = HeapType::getLeastUpperBound(type.getHeapType(),
otherType.getHeapType());
value = Literal::makeNull(lub);
assert(lub && "TODO: handle case where there is no LUB");
value = Literal::makeNull(*lub);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, this can make a Many().

@tlively
Copy link
Member Author

tlively commented Aug 11, 2022

The tests changed because their input used "externref" or "(ref extern)", which previously was an alias for "anyref" and "(ref any)" but now is not.

@kripken
Copy link
Member

kripken commented Aug 11, 2022

I see about the tests, thanks.

kripken
kripken approved these changes Aug 11, 2022
View reviewed changes
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

lgtm if this passes fuzzing.

aheejin
aheejin reviewed Aug 11, 2022
View reviewed changes
Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

Some places this didn't just add externref but replaced anyref with it. We don't need anyref in those places anymore?

src/binaryen-c.cpp
case HeapType::eq:
assert(x.isNull() && "unexpected non-null reference type literal");
break;
case HeapType::any:
Copy link
Member

Choose a reason for hiding this comment

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

Why is any's behavior here different from that of ext? Is any not nullable?

Copy link
Member Author

Choose a reason for hiding this comment

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

We only support the standardized reference types extern and func in this API and all the GC types are meant to hit that WASM_UNREACHABLE. Previously HeapType::any represented the standardized extern, but now it only represents the sepate any GC type.

src/passes/InstrumentLocals.cpp Outdated
@@ -57,15 +57,15 @@ Name get_f32("get_f32");
Name get_f64("get_f64");
Name get_v128("get_v128");
Name get_funcref("get_funcref");
Name get_anyref("get_anyref");
Name get_extref("get_extref");
Copy link
Member

Choose a reason for hiding this comment

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

We don't abbreviate externref to extref in other places, so how about just get_externref/set_externref here as well?

@tlively
Copy link
Member Author

tlively commented Aug 12, 2022

Note to self: I should also update the changelog.

@tlively
Copy link
Member Author

tlively commented Aug 12, 2022

Some places this didn't just add externref but replaced anyref with it. We don't need anyref in those places anymore?

Right, these were places that were logically using externref all along, but because we had externref == anyref, they had to use anyref in the code.

tlively added 8 commits August 17, 2022 15:37
@tlively
Restore the extern heap type
7797d59 
The GC proposal has split `any` and `extern` back into two separate types, so
reintroduce `HeapType::ext` to represent `extern`. Before it was originally
removed in #4633, externref was a subtype of anyref, but now it is not. Now that
we have separate heaptype type hierarchies, make `HeapType::getLeastUpperBound`
fallible as well.
@tlively
fuzzer fixes
e01f50b
@tlively
make compiler happy
155c1de
@tlively
Address possible-con{stant,tents} comments
e2efbf2
@tlively
update test
515f9de
@tlively
extref => externref
67278a7
@tlively
update changelog
ac6d404
@tlively
update test
1761ae9
@tlively
Copy link
Member Author

tlively commented Aug 18, 2022

Fuzzer looks good when I hack it to use v8-10.5.117, which still has func <: any. The next PR will make it work with up-to-date v8.

@tlively tlively merged commit 2d86d1f into main Aug 18, 2022
@tlively tlively deleted the heaptype-extern branch August 18, 2022 05:51
@kripken
Copy link
Member

kripken commented Aug 18, 2022

./scripts/fuzz_opt.py 5710490634504070365 fails on main now, which I am guessing is related to this as the error is:

binaryen/src/tools/fuzzing/fuzzing.cpp:1972: wasm::Expression* wasm::TranslateToFuzzReader::makeConstBasicRef(wasm::Type): Assertion `type.isNullable() && "Cannot handle non-nullable externref"' failed.

@kripken
Copy link
Member

kripken commented Aug 18, 2022

Bisected to confirm, the breakage indeed starts here.

This type of error seems to happen every 1,000 or so fuzzer iterations now, but sometimes it's 100 or 10,000, so this type of rare issue is easy to miss...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aheejin aheejin aheejin left review comments

@kripken kripken kripken approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tlively @kripken @aheejin