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

C-variadic functions may not be extern "C-unwind" #126836

Closed
asomers opened this issue Jun 22, 2024 · 3 comments · Fixed by #126843
Closed

C-variadic functions may not be extern "C-unwind" #126836

asomers opened this issue Jun 22, 2024 · 3 comments · Fixed by #126843
Labels
C-bug Category: This is a bug. F-c_unwind `#![feature(c_unwind)]` F-c_variadic `#![feature(c_variadic)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@asomers
Copy link
Contributor

asomers commented Jun 22, 2024

Overview

Using the unstable c_variadic feature, functions may be variadic as long as they have an extern "C" ABI. That used to be a reasonable restriction. However, after #74990 , extern "C" isn't as useful as it used to be. Now, any function that might panic needs to be extern "C-unwind". This may not technically be a compiler regression. But it's certainly a regression in functionality. Previously, Mockall was able to mock C variadic functions. Now, it cannot. Would it be possible to relax that restriction?

Example

I tried this code:

#![feature(c_variadic)]

pub extern "C-unwind" fn foo(x: i32, y: i32, ...) -> i32 {
    unimplemented!()
}

I expected to see this happen: It should compile ok.

Instead, this happened: It fails with this error:

error: only foreign or `unsafe extern "C"` functions may be C-variadic

Meta

rustc --version --verbose:

rustc 1.81.0-nightly (c1b336cb6 2024-06-21)
binary: rustc
commit-hash: c1b336cb6b491b3be02cd821774f03af4992f413
commit-date: 2024-06-21
host: x86_64-unknown-freebsd
release: 1.81.0-nightly
LLVM version: 18.1.7
@asomers asomers added the C-bug Category: This is a bug. label Jun 22, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 22, 2024
asomers added a commit to asomers/mockall that referenced this issue Jun 22, 2024
Begining within Rust 1.81
(rust-lang/rust#74990) Rust will abort when
attempting to unwind a panic across an "extern C" boundary.  Previously
it was technically UB, but it worked and Mockall relied on it.  Now,
unwinding across such a boundary requires the "extern C-unwind" ABI.
Use that ABI in the generated code.

However, don't use that ABI when mocking a C variadic function.  That
doesn't work, due to rust-lang/rust#126836 .

Fixes #584
asomers added a commit to asomers/mockall that referenced this issue Jun 22, 2024
Begining within Rust 1.81
(rust-lang/rust#74990) Rust will abort when
attempting to unwind a panic across an "extern C" boundary.  Previously
it was technically UB, but it worked and Mockall relied on it.  Now,
unwinding across such a boundary requires the "extern C-unwind" ABI.
Use that ABI in the generated code.

However, don't use that ABI when mocking a C variadic function.  That
doesn't work, due to rust-lang/rust#126836 .

Fixes #584
@workingjubilee workingjubilee added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 22, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Jun 22, 2024

Thanks for reporting this. It seems in several places, we just check for the literal "C" ABI, and those places are not checked for the "C-unwind" ABI as a possibility.

@workingjubilee
Copy link
Member

workingjubilee commented Jun 22, 2024

Now, any function that might panic needs to be extern "C-unwind". This may not technically be a compiler regression. But it's certainly a regression in functionality. Previously, Mockall was able to mock C variadic functions. Now, it cannot. Would it be possible to relax that restriction?

Just for note: You were not supposed to be allowing panics to unwind into C. Unless I am misunderstanding something, if this is a regression for you in functionality, your code was UB. I don't mean to quibble much, though, as I literally already fixed this.

@workingjubilee workingjubilee added F-c_variadic `#![feature(c_variadic)]` F-c_unwind `#![feature(c_unwind)]` labels Jun 22, 2024
@asomers
Copy link
Contributor Author

asomers commented Jun 22, 2024

Just for note: You were not supposed to be allowing panics to unwind into C.

Yeah, I realize that now. But since it worked before, I never bothered to read any of the fine details about external C functions. And thanks for the amazingly fast PR!

@bors bors closed this as completed in 9459fc2 Jun 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 23, 2024
Rollup merge of rust-lang#126843 - workingjubilee:allow-variadics-in-c-unwind, r=nnethercote

Allow "C-unwind" fn to have C variadics

Fixes rust-lang#126836
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 23, 2024
asomers added a commit to asomers/mockall that referenced this issue Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. F-c_unwind `#![feature(c_unwind)]` F-c_variadic `#![feature(c_variadic)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants