Lint against #[no_mangle] with the rust ABI #10347

bjorn3 · 2023-02-14T16:44:44Z

What it does

The lint should lint against #[no_mangle] on functions with the rust abi. See Rust-for-Linux/linux#967 for an example of this kind of issue.

Lint Name

no_mangle_with_rust_abi

Advantage

The Rust ABI is not stable, but in many simple cases matches enough with the C ABI that it is possible to forget to add extern "C" to a function called from C. Changes to the Rust ABI can break this at any point.

Drawbacks

No response

Example

#[no_mangle]
fn foo() {}

Should instead be written as:

#[no_mangle]
extern "C" fn foo() {}

The text was updated successfully, but these errors were encountered:

nindalf · 2023-02-17T15:42:17Z

@rustbot claim

nindalf · 2023-02-20T09:30:49Z

@bjorn3 I've implemented this is in a PR that's waiting on review. This is what the lint does

iff the function has no_mangle AND Abi == Abi::Rust, suggest adding extern "C" to it.
It will not run for any of the other variants defined rustc_target::spec::abi::Abi, nor suggest any conversion other than conversion to the C ABI.

My question to you is - is it possible to have false positives on this lint, to your knowledge? Is there some possible use case for adding #[no_mangle] to a Rust ABI function?

One discussion I found on internals.rust-lang.org suggests there aren't many use cases.

bjorn3 · 2023-02-20T10:46:40Z

I don't think there are many use cases. Maybe you could make it such that using an explicit extern "Rust" doesn't give a warning, but omitting extern entirely does? This way you can suppress the lint using extern "Rust" and at the same time indicate that this was what you actually meant.

nindalf · 2023-02-20T13:08:14Z

I've added an exception for extern "Rust". Suppressing this lint #[allow(no_mangle_with_rust_abi)] on an item or the whole crate is another option users have.

If we can't think of any use cases for using #[no_mangle] on a Rust ABI function, I'm leaning towards this lint being opt-out rather than opt-in.

Probably a call for the clippy maintainers to take. @llogiq does that seem fine?

llogiq · 2023-02-20T13:12:15Z

I'm not completely clear on the semantics here: Doesn't the ABI default to Rust, thus making no ABI specification equal to extern "Rust"? If that's right, I don't see the benefit of this lint in the general case, and it should probably be allow by default.

bjorn3 · 2023-02-20T13:16:18Z

When using #[no_mangle] you almost always intended it to be called from C and thus must not use the Rust abi. If you did actually mean to use the Rust abi, writing an explicit extern "Rust" helps clarify this.

ojeda · 2023-02-20T13:16:31Z

If we can't think of any use cases for using #[no_mangle] on a Rust ABI function

The __rust_* functions are one such case in Rust for Linux, but we had to declare them due to the fact that some crate types did not generate them (i.e. maybe that changes, and therefore we could remove them and so we may not have such a case anymore) -- Rust-for-Linux/linux#967 (comment).

bjorn3 · 2023-02-20T13:19:44Z

Note the real versions of __rg_*, __rdl_* and __rust_* are actually #[rustc_std_internal_symbol] rather than #[no_mangle]. Both attributes are identical except that #[rustc_std_internal_symbol] isn't exported from cdylib's unlike #[no_mangle]. Also having to define those __rust_* functions will hopefully go away soon.

nindalf · 2023-02-20T13:25:55Z

Copy pasting what the current impl of the lint does. Let me know if this works for everyone

This PR adds a new lint no_mangle_with_rust_abi that suggests converting a function to the C ABI to if the function has the #[no_mangle] attribute and Abi == Abi::Rust. It will not run for any of the other variants defined in rustc_target::spec::abi::Abi, nor suggest any conversion other than conversion to the C ABI.

Functions that explicitly opt into the Rust ABI with extern "Rust" are ignored by this lint.

ojeda · 2023-02-20T13:39:06Z

If that's right, I don't see the benefit of this lint in the general case, and it should probably be allow by default.

The problem is forgetting extern "C" when having written #[no_mangle].

Perhaps somebody has a common enough use case for such combination, but it would seem that the lint should be warn or deny by default (not sure what is the threshold/policy for those in Clippy), especially if the explicit extern "Rust" escape hatch is provided.

That is, projects that really use it that way can either use the escape hatch or simply allow it as needed (which are good things, because they inform the reader that was intentional), but everyone else should be informed about the (very likely) mistake they made (i.e. either assuming #[no_mangle] implied C ABI or simply forgetting to write it even if they knew).

ojeda · 2023-02-20T13:49:51Z

Is is possible to get a Crater run with the lint enabled as deny, by the way? It would be nice to know how common the combination is, how many things would break if it was deny by default, and to report/spot bugs in the wild if any.

nindalf · 2023-02-20T14:41:40Z

Not a crater run, but here are a couple of Github search queries

without extern "C" matches ~~4.5k~~ 14.8k files.
with extern "C" matches ~~16.9k~~ 35.8k files.

@ojeda and @bjorn3 - are most of the examples from the first link making a mistake?

And follow up question - if they are making a mistake, @llogiq - is this an acceptable number of people impacted?

ojeda · 2023-02-20T15:19:02Z

The "without" search appears to hit cases where extern "C" was written for me -- the regex is not taken properly into account. I guess it requires the new GitHub Search.

By the way, I see some #[naked] cases there, though rustc already warns about extern "Rust" there, explicitly written or not.

nindalf · 2023-02-20T15:20:27Z

Sorry about that, I think I've fixed the query now.

bjorn3 · 2023-02-20T17:03:27Z

@ojeda and @bjorn3 - are most of the examples from the first link making a mistake?

I saw some cases of #[no_mangle] inside extern "C" {} blocks where it doesn't have any effect. Others indeed look like they are missing extern "C".

nindalf · 2023-02-20T18:33:40Z

@llogiq what do you think? It seems like the lint is doing the right thing, but it will fire on a lot of code.

nindalf · 2023-02-22T16:13:52Z

One thing I want to add about the exemption for extern "Rust" - rustc will compile it correctly but rustfmt overwrites it. The rule force_explicit_abi (on by default) will replace the extern declaration with an empty string (src). Therefore, we won't see this code unless the author skipped running rustfmt.

No change to the existing PR - the exemption is still there. Might be useful for those who use clippy but not rustfmt.

bjorn3 · 2023-02-22T18:16:13Z

but rustfmt overwrites it.

Didn't know that. That is somewhat annoying. At least #[allow] would still work.

nindalf · 2023-02-23T10:40:47Z

@bjorn3 and @ojeda - I'm trying to think of a way for kernel developers to use this lint without impacting other users. Would you be open to explicitly opting in with #![warn(clippy:: no_mangle_with_rust_abi)] in the kernel prelude? In that case we could change this lint to pedantic and merge it.

ojeda · 2023-02-23T10:57:51Z

Yeah, that is completely fine for us -- we do so already for other lints, via the command-line, see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Makefile#n479.

Still, it would be nice to see how to fix other projects, or at least them make them aware a new lint (even if pedantic) is available, so that they can start fixing things, and eventually enable it by default somehow.

Thanks a lot, by the way!

Add new lint no_mangle_with_rust_abi Fixes issue #10347 This PR adds a new lint `no_mangle_with_rust_abi` that suggests converting a function to the C ABI to if the function has the `#[no_mangle]` attribute and `Abi == Abi::Rust`. It will not run for any of the other variants defined in [rustc_target::spec::abi::Abi](https://doc.rust-lang.org/beta/nightly-rustc/rustc_target/spec/abi/enum.Abi.html), nor suggest any conversion other than conversion to the C ABI. Functions that explicitly opt into the Rust ABI with `extern "Rust"` are ignored by this lint. --- changelog: [`no_mangle_with_rust_abi`]: add lint that converts Rust ABI functions with the `#[no_mangle]` attribute to C ABI

nindalf · 2023-02-23T20:35:16Z

The PR is merged.

It should be available in stable with Rust 1.69.0, expected April 20th. 🚀

bjorn3 · 2023-02-23T20:37:54Z

Thanks for working on this!

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). Thus enable it. In some cases, we need to use the Rust ABI even with `#[no_mangle]`, and for that, one may use `extern "Rust"` explicitly, but `rustfmt` overwrites it (and there is no option to prevent that, not even `force_explicit_abi` [3]). Therefore, use an `allow` instead for the few cases we had. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux#967 [2] Link: https://rust-lang.github.io/rustfmt/?version=v1.5.2&search=force_explicit_abi#force_explicit_abi [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: #967 [2] Link: rust-lang/rustfmt#5701 [3] Reviewed-by: Trevor Gross <tmgross@umich.edu> Reviewed-by: Gary Guo <gary@garyguo.net> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Link: https://lore.kernel.org/r/20230729220317.416771-2-ojeda@kernel.org Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux/linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Reviewed-by: Trevor Gross <tmgross@umich.edu> Reviewed-by: Gary Guo <gary@garyguo.net> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Link: https://lore.kernel.org/r/20230729220317.416771-2-ojeda@kernel.org Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux/linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

bjorn3 added the A-lint Area: New lints label Feb 14, 2023

bjorn3 mentioned this issue Feb 14, 2023

rust: kernel: Mark rust_fmt_argument as extern "C" Rust-for-Linux/linux#967

Closed

ojeda mentioned this issue Feb 14, 2023

Clippy wanted features & bugfixes Rust-for-Linux/linux#349

Open

33 tasks

rustbot assigned nindalf Feb 17, 2023

nindalf mentioned this issue Feb 18, 2023

Add new lint no_mangle_with_rust_abi #10369

Merged

bjorn3 closed this as completed Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint against #[no_mangle] with the rust ABI #10347

Lint against #[no_mangle] with the rust ABI #10347

bjorn3 commented Feb 14, 2023 •

edited by rustbot

Loading

nindalf commented Feb 17, 2023

nindalf commented Feb 20, 2023 •

edited

Loading

bjorn3 commented Feb 20, 2023

nindalf commented Feb 20, 2023

llogiq commented Feb 20, 2023

bjorn3 commented Feb 20, 2023 •

edited

Loading

ojeda commented Feb 20, 2023 •

edited

Loading

bjorn3 commented Feb 20, 2023

nindalf commented Feb 20, 2023

ojeda commented Feb 20, 2023 •

edited

Loading

ojeda commented Feb 20, 2023

nindalf commented Feb 20, 2023 •

edited

Loading

ojeda commented Feb 20, 2023

nindalf commented Feb 20, 2023 •

edited

Loading

bjorn3 commented Feb 20, 2023

nindalf commented Feb 20, 2023

nindalf commented Feb 22, 2023 •

edited

Loading

bjorn3 commented Feb 22, 2023

nindalf commented Feb 23, 2023

ojeda commented Feb 23, 2023 •

edited

Loading

nindalf commented Feb 23, 2023

bjorn3 commented Feb 23, 2023

Lint against #[no_mangle] with the rust ABI #10347

Lint against #[no_mangle] with the rust ABI #10347

Comments

bjorn3 commented Feb 14, 2023 • edited by rustbot Loading

What it does

Lint Name

Category

Advantage

Drawbacks

Example

nindalf commented Feb 17, 2023

nindalf commented Feb 20, 2023 • edited Loading

bjorn3 commented Feb 20, 2023

nindalf commented Feb 20, 2023

llogiq commented Feb 20, 2023

bjorn3 commented Feb 20, 2023 • edited Loading

ojeda commented Feb 20, 2023 • edited Loading

bjorn3 commented Feb 20, 2023

nindalf commented Feb 20, 2023

ojeda commented Feb 20, 2023 • edited Loading

ojeda commented Feb 20, 2023

nindalf commented Feb 20, 2023 • edited Loading

ojeda commented Feb 20, 2023

nindalf commented Feb 20, 2023 • edited Loading

bjorn3 commented Feb 20, 2023

nindalf commented Feb 20, 2023

nindalf commented Feb 22, 2023 • edited Loading

bjorn3 commented Feb 22, 2023

nindalf commented Feb 23, 2023

ojeda commented Feb 23, 2023 • edited Loading

nindalf commented Feb 23, 2023

bjorn3 commented Feb 23, 2023

bjorn3 commented Feb 14, 2023 •

edited by rustbot

Loading

nindalf commented Feb 20, 2023 •

edited

Loading

bjorn3 commented Feb 20, 2023 •

edited

Loading

ojeda commented Feb 20, 2023 •

edited

Loading

ojeda commented Feb 20, 2023 •

edited

Loading

nindalf commented Feb 20, 2023 •

edited

Loading

nindalf commented Feb 20, 2023 •

edited

Loading

nindalf commented Feb 22, 2023 •

edited

Loading

ojeda commented Feb 23, 2023 •

edited

Loading