-
Notifications
You must be signed in to change notification settings - Fork 837
Fix Unsubtyping and GTO for configureAll #8267
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
Take into account the implicit casts and conversions that happen on the boundary with JS as well as the fact that JS can essentially read the first field on descriptors with configured prototypes.
3d31e7e to
224c6c9
Compare
|
|
||
| // Whether this is a struct type whose first field is immutable and a subtype | ||
| // of externref. | ||
| bool hasPossibleJSPrototypeField() const; |
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.
This feels like a somewhat specific property beyond the core type system - perhaps it can go in struct-utils.h?
| if (!curr->value->type.isRef()) { | ||
| return; | ||
| } | ||
| auto exact = curr->value->type.getExactness(); |
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.
can move this into the if.
| return; | ||
| } | ||
| for (auto func : Intrinsics(wasm).getConfigureAllFunctions()) { | ||
| for (auto type : wasm.getFunction(func)->getResults()) { |
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.
Perhaps add a comment here as to why only the results matter?
| // This descriptor will expose a prototype to JS, so we must keep | ||
| // it. | ||
| noteDescriptor(heapType, *desc); | ||
| } |
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.
Why is the cast symmetric in params and results, but the prototype only appears for the results?
| collectedInfo.casts.insert({src, dst}); | ||
| } | ||
| dsts.clear(); | ||
| } |
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 an unrelated fix?
Take into account the implicit casts and conversions that happen on the
boundary with JS as well as the fact that JS can essentially read the
first field on descriptors with configured prototypes.