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

#[target_feature] is allowed on #[panic_handler] with target_feature 1.1 #109411

Closed
est31 opened this issue Mar 20, 2023 · 6 comments · Fixed by #115910
Closed

#[target_feature] is allowed on #[panic_handler] with target_feature 1.1 #109411

est31 opened this issue Mar 20, 2023 · 6 comments · Fixed by #115910
Labels
C-bug Category: This is a bug. F-target_feature_11 target feature 1.1 RFC I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@est31
Copy link
Member

est31 commented Mar 20, 2023

This works (but it shouldn't):

#![feature(target_feature_11)]
#![no_std]

use core::panic::PanicInfo;

#[panic_handler]
#[target_feature(enable = "avx2")]
fn panic(_info: &PanicInfo) -> ! {
    loop {}
}

similar to #108645 , cc #69098 (tracking issue)

@rustbot label T-lang T-compiler C-bug I-unsound F-target_feature_11

@est31 est31 added the C-bug Category: This is a bug. label Mar 20, 2023
@rustbot rustbot added F-target_feature_11 target feature 1.1 RFC I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 20, 2023
@est31
Copy link
Member Author

est31 commented Mar 20, 2023

cc @LeSeulArtichaut , author of #108651

Note that this is about the no_std panic handler, not about the std::panic::set_hook function. This fails as it should:

#![feature(target_feature_11)]

use core::panic::PanicInfo;

#[target_feature(enable = "avx2")]
fn panic(_info: &PanicInfo) {
    loop {}
}

fn main() {
    std::panic::set_hook(Box::new(panic));
}

gives

error[E0277]: expected a `Fn<(&PanicInfo<'_>,)>` closure, found `for<'a, 'b> fn(&'a PanicInfo<'b>) {panic}`
  --> src/main.rs:11:26
   |
11 |     std::panic::set_hook(Box::new(panic));
   |                          ^^^^^^^^^^^^^^^ expected an `Fn<(&PanicInfo<'_>,)>` closure, found `for<'a, 'b> fn(&'a PanicInfo<'b>) {panic}`
   |
   = help: the trait `for<'a, 'b> Fn<(&'a PanicInfo<'b>,)>` is not implemented for fn item `for<'a, 'b> fn(&'a PanicInfo<'b>) {panic}`
   = note: `#[target_feature]` functions do not implement the `Fn` traits
   = note: required for the cast from `for<'a, 'b> fn(&'a PanicInfo<'b>) {panic}` to the object type `dyn for<'a, 'b> Fn(&'a PanicInfo<'b>) + Send + Sync`

@Noratrieb
Copy link
Member

ugh we really need a way to make sure that every single "this is called by rust" function gets a check...
I assume that these functions can't be unsafe, so I guess those checks are a food way to start looking and make sure to deduplicate the logic there?

@est31
Copy link
Member Author

est31 commented Mar 20, 2023

Yeah sadly we do need to make sure, not just for the constructs that we have now but also it needs to be kept in mind for the future constructs we add or stabilize.

A lot thankfully goes through either the trait system (operators or the GlobalAlloc trait used by #[global_alloc]) or function pointers (alloc error hook, std panic hook). Functions that you can make callable solely by setting an attribute, those are rare.

The best list of lang items I could find was in the unstable book, so that would be a great start for such an exhaustive search. Thankfully most of them are unstable, but still it needs to be addressed for each of them before stabilization (if that is a target).

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 21, 2023
@LeSeulArtichaut
Copy link
Contributor

@rustbot label +requires-nightly

@rustbot rustbot added the requires-nightly This issue requires a nightly compiler in some way. label Mar 21, 2023
@est31
Copy link
Member Author

est31 commented Apr 10, 2023

There is also another table of lang items in the source code, here.

@bors bors closed this as completed in aace2df Sep 22, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 23, 2023
Prevent using `#[target_feature]` on lang item functions

Fixes rust-lang/rust#109411 and also prevents from using `#[target_feature]` on other `fn` lang items to mitigate the concerns from rust-lang/rust#109411 (comment).
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
Prevent using `#[target_feature]` on lang item functions

Fixes rust-lang/rust#109411 and also prevents from using `#[target_feature]` on other `fn` lang items to mitigate the concerns from rust-lang/rust#109411 (comment).
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Prevent using `#[target_feature]` on lang item functions

Fixes rust-lang/rust#109411 and also prevents from using `#[target_feature]` on other `fn` lang items to mitigate the concerns from rust-lang/rust#109411 (comment).
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-target_feature_11 target feature 1.1 RFC I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants