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

Remove support for extern "rust-intrinsic" blocks #132735

Open
RalfJung opened this issue Nov 7, 2024 · 25 comments
Open

Remove support for extern "rust-intrinsic" blocks #132735

RalfJung opened this issue Nov 7, 2024 · 25 comments
Assignees
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Nov 7, 2024

We currently have two ways to declare symbols that are invoked as intrinsics. The old way:

extern "rust-intrinsic" {
    fn unreachable() -> !;
}

The new way, which supports giving a "fallback body" that will be used for backends that do not have the intrinsic implemented:

#[rustc_intrinsic]
#[rustc_intrinsic_must_be_overridden]
unsafe fn unreachable() -> ! { unreachable!() }

The goal of this issue is to remove support for the old style, and consistently use the new style.

  1. Port the remaining extern "rust-intrinsic" intrinsics in library to the new style, updating them using the pattern described above. This can be a PR on its own.
  2. Port the uses of extern "rust-intrinsic" in src/tools/miri and tests/ui/simd to the new style. In fact, these can use the even newer style (which can't be used in library yet because of bootstraping):
#[rustc_intrinsic]
unsafe fn unreachable();
  1. Remove support for extern "rust-intrinsic" blocks from the compiler -- in particular, remove this. AFAIK these are also the only extern blocks that support generics, so there might be more things that can be cleaned up here. (@compiler-errors or @oli-obk might know more about that.) A bunch of tests will still need updating; you can grep for rust-intrinsic to find them all. They should all be ported to the new style, similar to the PR in step 2.
@RalfJung RalfJung added C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Nov 7, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 7, 2024
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 7, 2024
@compiler-errors
Copy link
Member

I do wonder if there's some way we could have body-less free function items when users write #[rustc_intrinsic_must_be_overridden]. Having all these dummy bodies like loop {} and unreachable {} seem kinda meh.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 7, 2024 via email

@compiler-errors
Copy link
Member

We actually reject them after expansion but before ast lowering.

We could perhaps intercept these bodyless functions during ast lowering and fill them with a dummy body like loop {} or something that diverges rather than having it be manually written on each function.

@compiler-errors
Copy link
Member

Obviously only intercepting them if they're marked with rustc_intrinsic_must_be_overridden; if it's a regular function we'd issue the regular error for a bodyless free function, and if it's rustc_intrinsic we could tell the user that it needs to be marked with rustc_intrinsic_must_be_overridden.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 7, 2024

That's way beyond my knowledge of those parts of the compiler. ;) So sure, sounds great. :D

Cc @petrochenkov

Obviously only intercepting them if they're marked with rustc_intrinsic_must_be_overridden

I was actually going to suggest that a #[rustc_intrinsic] not having a body should replace the rustc_intrinsic_must_be_overridden attribute.

@compiler-errors
Copy link
Member

Oh, that works too.

@workingjubilee
Copy link
Member

hmm do we have the notion of a builtin attribute macro...?

@compiler-errors
Copy link
Member

@workingjubilee: Yes

@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 8, 2024
@BLANKatGITHUB
Copy link
Contributor

@rustbot claim

@BLANKatGITHUB
Copy link
Contributor

BLANKatGITHUB commented Nov 10, 2024

Ok so basically I tried porting the first block of extern "rust-intrinsic" in library/core/src/intrinsics/mod.rs
and it gave the errors

`error[E0308]: intrinsic has wrong type
core/src/intrinsics/mod.rs:1277:26

pub fn prefetch_read_data<T>(data: *const T, locality: i32) {
                                                ^^^ expected unsafe fn, found safe fn
 
 note: expected signature `unsafe fn(_, _)`
            found signature `fn(_, _)` 

and

``
error: intrinsic safety mismatch between list of intrinsics within the compiler and core library intrinsics for intrinsic prefetch_write_data
core/src/intrinsics/mod.rs:1292:1

pub fn prefetch_write_data(data: *const T, locality: i32) {

``

@RalfJung
Copy link
Member Author

Yes, the intrinsics need to be marked as unsafe fn. Sorry I forgot that when writing up the issue.

workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 14, 2024
Change intrinsic declarations to new style

Pr is for issue rust-lang#132735
This changes the first `extern "rust-intrinsic"` block to the new style.
r? `@RalfJung`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 14, 2024
Rollup merge of rust-lang#132907 - BLANKatGITHUB:intrinsic, r=saethlin

Change intrinsic declarations to new style

Pr is for issue rust-lang#132735
This changes the first `extern "rust-intrinsic"` block to the new style.
r? `@RalfJung`
jieyouxu added a commit to jieyouxu/rust that referenced this issue Nov 23, 2024
changes old intrinsic declaration to new declaration

This pr is for issue rust-lang#132735

It changes old `extern "intrinsic"` code block with new declaration.

There are other blocks that use old declaration but as the changes needed in single block is quite large I do them in parts
jieyouxu added a commit to jieyouxu/rust that referenced this issue Nov 30, 2024
changes old intrinsic declaration to new declaration

This pr is for issue rust-lang#132735

It changes old `extern "intrinsic"` code block with new declaration.

There are other blocks that use old declaration but as the changes needed in single block is quite large I do them in parts
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 30, 2024
Rollup merge of rust-lang#133106 - BLANKatGITHUB:intrinsic, r=RalfJung

changes old intrinsic declaration to new declaration

This pr is for issue rust-lang#132735

It changes old `extern "intrinsic"` code block with new declaration.

There are other blocks that use old declaration but as the changes needed in single block is quite large I do them in parts
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 8, 2024
Adds new intrinsic declaration

This pr is for rust-lang#132735 removes removes `extern "intrinsic"`

I think its the last block of this file and was kind of asking for advice how to handle other files as mentioned in the issue .
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 9, 2024
Rollup merge of rust-lang#134013 - BLANKatGITHUB:intrinsic, r=saethlin

Adds new intrinsic declaration

This pr is for rust-lang#132735 removes removes `extern "intrinsic"`

I think its the last block of this file and was kind of asking for advice how to handle other files as mentioned in the issue .
@RalfJung
Copy link
Member Author

RalfJung commented Jan 2, 2025

I do wonder if there's some way we could have body-less free function items when users write #[rustc_intrinsic_must_be_overridden]. Having all these dummy bodies like loop {} and unreachable {} seem kinda meh.

#135031 implements that.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 2, 2025

The current status for this issue is that many but not all of the intrinsics in library have been ported to the new style.

@BLANKatGITHUB do you plan to continue this, porting the remaining intrinsics in library?

@BLANKatGITHUB
Copy link
Contributor

Sorry I am currently busy with my exams, but I will get to it as soon I get the time

@RalfJung
Copy link
Member Author

RalfJung commented Jan 2, 2025

No worries. :) Focus on your exams first!

@RalfJung
Copy link
Member Author

RalfJung commented Jan 4, 2025

Note that uses outside library can use an even newer style now, but it will take 6 weeks before we can use that style in library/:

#[rustc_intrinsic]
unsafe fn unreachable();

rustc_intrinsic_must_be_overridden should not be used outside library.

@BLANKatGITHUB
Copy link
Contributor

Thanks for understanding I will be free again around 20 January, I will contact you then for more information.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 20, 2025
…lfJung

Partial progress on rust-lang#132735: Replace extern "rust-intrinsic" with #[rustc_intrinsic] across the codebase

Part of rust-lang#132735: Replace `extern "rust-intrinsic"` with `#[rustc_intrinsic]` macro

- Updated all instances of `extern "rust-intrinsic"` to use the `#[rustc_intrinsic]` macro.
- Skipped `.md` files and test files to avoid unnecessary changes.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 20, 2025
…lfJung

Partial progress on rust-lang#132735: Replace extern "rust-intrinsic" with #[rustc_intrinsic] across the codebase

Part of rust-lang#132735: Replace `extern "rust-intrinsic"` with `#[rustc_intrinsic]` macro

- Updated all instances of `extern "rust-intrinsic"` to use the `#[rustc_intrinsic]` macro.
- Skipped `.md` files and test files to avoid unnecessary changes.
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 20, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#133695 (Reexport likely/unlikely in std::hint)
 - rust-lang#135330 (Respect --sysroot for rustc -vV and -Cpasses=list)
 - rust-lang#135333 (Partial progress on rust-lang#132735: Replace extern "rust-intrinsic" with #[rustc_intrinsic] across the codebase)
 - rust-lang#135741 (Recognise new IPv6 documentation range from IETF RFC 9637)
 - rust-lang#135770 (Update contributing docs for submodule/subtree changes)
 - rust-lang#135775 (Subtree update of `rust-analyzer`)
 - rust-lang#135776 (Subtree sync for rustc_codegen_cranelift)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 20, 2025
Rollup merge of rust-lang#135333 - vayunbiyani:test-environment, r=RalfJung

Partial progress on rust-lang#132735: Replace extern "rust-intrinsic" with #[rustc_intrinsic] across the codebase

Part of rust-lang#132735: Replace `extern "rust-intrinsic"` with `#[rustc_intrinsic]` macro

- Updated all instances of `extern "rust-intrinsic"` to use the `#[rustc_intrinsic]` macro.
- Skipped `.md` files and test files to avoid unnecessary changes.
@vayunbiyani
Copy link
Contributor

Thanks for understanding I will be free again around 20 January, I will contact you then for more information.

Hi @BLANKatGITHUB , I hope your exams went well! While you were away, I made some progress on the issue. Let me know if there's anything else I can assist you with regarding its resolution.

@BLANKatGITHUB
Copy link
Contributor

@vayunbiyani thanks I saw you change quite a few files in one pr , I am not quite sure if every file in compiler and all the tests are ported , I hope you can provide more info on that as I was busy with exams currently I am working on /home/blanky/projects/opensource/rust/library/core/src/intrinsics/mod.rs .

@BLANKatGITHUB
Copy link
Contributor

@RalfJung I don't see ./library/core/src/ffi/va_list.rs are they ported? and can you provide more info on new style we can't use it inside the library though, right?

@RalfJung
Copy link
Member Author

RalfJung commented Jan 21, 2025

What do you mean you don't see it? It's right there.

And yes the "even newer" style without a body cannot be used in the library. The issue documentation says that.

@vayunbiyani
Copy link
Contributor

I am not quite sure if every file in compiler and all the tests are ported

@BLANKatGITHUB You’re correct, not every file has been updated. I’ve only ported some of the more obvious ones. I’m a bit uncertain about how to handle porting the test cases while adhering to best practices. What do you think about jumping on Zulip to figure this out?

@RalfJung
Copy link
Member Author

tests/ui/simd has a bunch of test cases that need porting, I would suggest those be ported in a separate PR before we do the final big PR that removes support for this ABI from the compiler entirely.

@BLANKatGITHUB
Copy link
Contributor

@vayunbiyani sure drop the link

@vayunbiyani
Copy link
Contributor

@vayunbiyani sure drop the link

You can find me here as Vayun Biyani

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 25, 2025
ports last few library files to new intrinsic style

This pr ports the last 2 library files to new intrinsic style this pr is part of issue rust-lang#132735
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 26, 2025
Rollup merge of rust-lang#136005 - BLANKatGITHUB:library, r=RalfJung

ports last few library files to new intrinsic style

This pr ports the last 2 library files to new intrinsic style this pr is part of issue rust-lang#132735
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants