-
Notifications
You must be signed in to change notification settings - Fork 13.4k
add extern "custom"
functions
#140770
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: master
Are you sure you want to change the base?
add extern "custom"
functions
#140770
Conversation
These commits modify compiler targets. |
This comment has been minimized.
This comment has been minimized.
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in compiler/rustc_codegen_gcc |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tests/ui/abi/custom.rs
Outdated
#[unsafe(naked)] | ||
unsafe extern "custom" fn double(a: u64) -> u64 { | ||
naked_asm!("add rax, rax", "ret"); | ||
} |
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.
For now, I think I'd suggest just not allowing parameters and a return type -- or putting this under a separate feature gate.
We could always allow this later. It seems like one of the few question marks here, so it'd be better to do the minimal thing to start.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Did you forget to press escape before quitting vim? |
ah, yeah. Well it'll probably get squashed before this is ready |
I've made a proper tracking issue at #140829 |
☔ The latest upstream changes (presumably #141002) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #141984) made this pull request unmergeable. Please resolve the merge conflicts. |
I've rebased and changed the implementation to match #141774, which is both shorter and apparently faster too. |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #142099) made this pull request unmergeable. Please resolve the merge conflicts. |
@traviscross are we waiting for T-lang for something here? Otherwise we could get a T-compiler review to move this forward right? |
☔ The latest upstream changes (presumably #141774) made this pull request unmergeable. Please resolve the merge conflicts. |
I believe T-lang has already basically okayed this experiment per TC's comment here and I think any concerns that TC winds up having, in addition to the review TC already gave, can be addressed in followup. It's not like lang's gonna FCP this tomorrow, is it? Rolling a compiler review. r? compiler |
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'm sorry but I have a huge and tedious nit that should be a tidy rule but isn't because Reasons.
Yes, this is OK as a lang matter. I'd probably suggest... r? tgross35 |
This comment was marked as resolved.
This comment was marked as resolved.
Happy to review on Monday, but @workingjubilee if you've already taken a look at the rest then feel free to pick it up as well |
☔ The latest upstream changes (presumably #142181) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #141435) made this pull request unmergeable. Please resolve the merge conflicts. |
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 looks pretty clean to me, and well tested. Left a handful of small comments but nothing major.
// Functions with this calling convention can only be called from assembly, but it is | ||
// possible to declare an `extern "custom"` block, so the backend still needs a calling | ||
// convention for declaring foreign functions. | ||
CanonAbi::Custom => default_call_conv, |
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 seems reasonable but I wonder if down the line clif (or the other backends) might want to have some kind of calling convention representation that can be bug!
ed on if it gets directly called somewhere. (cc @bjorn3)
let abi = match callee_ty.kind() { | ||
ty::FnDef(def_id, _) => self.tcx.fn_sig(def_id).skip_binder().skip_binder().abi, | ||
ty::FnPtr(_, header) => header.abi, | ||
_ => return, |
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.
Are there other valid callees? If so, it may be good to return
from them specifically then bug!
on the fallback _
.
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, that raises the question of what async extern "custom" fn foo() {}
should do. It always returns impl Future
, so I think we should just disallow this.
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.
Agreed to disallowing async 👍 that's likely pretty tied to the Rust ABI too.
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.
@folkertdev could you make the fallback bug so this doesn't miss any future call types?
ty::FnDef(def_id, _) => self.tcx.fn_sig(def_id).skip_binder().skip_binder().abi,
ty::FnPtr(_, header) => header.abi,
// Closures cannot have the "custom" ABI
ty::Closure(..) => return,
// Ignore errors
ty::Error(_) => return,
_ => span_bug!(span, "unexpected callee type {callee_ty:?}"),
(skipping errors based on
rust/compiler/rustc_type_ir/src/ty_kind.rs
Line 276 in 8ce2287
pub fn fn_sig(self, interner: I) -> ty::Binder<I, ty::FnSig<I>> { |
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.
Ah, I did look into that but it turns out that we do get (at least) Adt
and Alias
in that final branch. So that'll need some investigation.
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 believe that what can happen here is that the scrutinee is some impl Fn()
(or similar) value: it is callable, but not a function definition or function pointer.
@oli-obk you've been working in this area a bunch: do you have advice on getting the ABI here (anything that implicitly uses the extern "Rust"
(closures, Fn()
) is irrelevant, we're just looking for callable items with the "custom"
ABI. I suspect &unsafe extern "custom" fn()
in particular is going to be tricky and is missed by the current logic. Is there some standard way to get at the "real" 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.
Based on #140770 (comment) could you add a test for an async
function and a const
function? Const is rejected for calling asm (will probably always be that way) and async
seems to be rejected by checks on the form it desugars to, but it would be good to make sure these don't regress.
☔ The latest upstream changes (presumably #141883) made this pull request unmergeable. Please resolve the merge conflicts. |
tracking issue: #140829
previous discussion: #140566
In short, an
extern "custom"
function is a function with a custom ABI, that rust does not know about. Therefore, such functions can only be defined with#[unsafe(naked)]
andnaked_asm!
, or via anextern "C" { /* ... */ }
block. These functions cannot be called using normal rust syntax: calling them can only be done from inline assembly.The motivation is low-level scenarios where a custom calling convention is used. Currently, we often pick
extern "C"
, but that is a lie because the function does not actually respect the C calling convention.At the moment
"custom"
seems to be the name with the most support. That name is not final, but we need to pick something to actually implement this.r? @traviscross
cc @tgross35