-
Notifications
You must be signed in to change notification settings - Fork 14k
Error on invalid signatures for interrupt ABIs #142633
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
Error on invalid signatures for interrupt ABIs #142633
Conversation
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
This comment has been minimized.
This comment has been minimized.
6c041ae to
6086486
Compare
workingjubilee
left a comment
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.
pass of style nits, will doublecheck more closely a bit later, you can apply them now or wait until second pass
6086486 to
411fce5
Compare
|
☔ The latest upstream changes (presumably #142817) made this pull request unmergeable. Please resolve the merge conflicts. |
411fce5 to
aec1f72
Compare
aec1f72 to
32fbbc2
Compare
| ast_passes_abi_must_not_have_return_type= | ||
| invalid signature for `extern {$abi}` function | ||
| .note = functions with the `"custom"` ABI cannot have a return 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.
leftover
| .note = functions with the `"custom"` ABI cannot have a return type | |
| .note = functions with the "custom" ABI cannot have a return 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.
right, I left those unchanged easier but we can fold that into this PR. Fixed.
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.
Oh, was this not new? GH UI may have fooled me.
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.
hmm maybe this one was? but some others in that file were older. Anyway, it's all fixed now.
| // An `extern "custom"` function cannot be `async` and/or `gen`. | ||
| self.check_abi_is_not_coroutine(abi, sig); | ||
|
|
||
| // An `extern "custom"` function must have type `fn()`. |
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.
pedantic note: its type is unsafe extern "custom" fn(). and yes, you can have different implementations on unsafe fn() and fn().
don't actually change anything, I just couldn't resist noting.
| /// type. | ||
| fn check_custom_abi(&self, ctxt: FnCtxt, ident: &Ident, sig: &FnSig) { | ||
| /// Check that this function does not violate the constraints of its ABI. | ||
| fn check_abi(&self, abi: ExternAbi, ctxt: FnCtxt, ident: &Ident, sig: &FnSig) { |
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 find these check_this names to be slightly nondescript.
- sometimes they warn. sometimes they error.
- we specifically check only the signature.
| fn check_abi(&self, abi: ExternAbi, ctxt: FnCtxt, ident: &Ident, sig: &FnSig) { | |
| fn reject_invalid_abi_sig(&self, abi: ExternAbi, ctxt: FnCtxt, ident: &Ident, sig: &FnSig) { |
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.
Could also be, at least, check_extern_fn_sig
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 some renaming. Most functions in that module do start with check_ so that is what I followed.
32fbbc2 to
7d6e5aa
Compare
|
☔ The latest upstream changes (presumably #142929) made this pull request unmergeable. Please resolve the merge conflicts. |
7d6e5aa to
943d379
Compare
|
hell yeah. @bors r+ |
| ast_passes_abi_must_not_have_return_type= | ||
| invalid signature for `extern {$abi}` function | ||
| .note = functions with the "custom" ABI cannot have a return 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.
oops, missed this.
We recently added
extern "custom", which must have typefn(). The variousextern "interrupt"ABIs impose similar constraints on the signature of functions with that ABI:x86-interruptshould not have a return type (linting on the exact argument types is left as future work), and the other interrupt ABIs cannot have any parameters or a return type.r? @workingjubilee
Closes #132841