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

Disabling particular unsafe precondition checks under -Cdebug-assertions=y #120969

Open
ojeda opened this issue Feb 12, 2024 · 24 comments
Open

Disabling particular unsafe precondition checks under -Cdebug-assertions=y #120969

ojeda opened this issue Feb 12, 2024 · 24 comments
Labels
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

@ojeda
Copy link
Contributor

ojeda commented Feb 12, 2024

From #85122 (comment) and #85122 (comment).

After #120594 (1.78), unsafe precondition checks always apply under -Cdebug-assertions=y, but there is currently no way to disable particular cases in debug builds:

They will make things too slow for somebody sooner or later -- it is not uncommon in projects to have a "fast debug" build where a few checks need to be explicitly bypassed in hot paths. And it is not just the branch/call itself, it can also break other optimizations like vectorization: https://godbolt.org/z/MMTordabn.

I really like the unsafe precondition checks, but I would like to make sure there is an escape hatch for -Cdebug-assertions=y builds. Perhaps we need a core::hint::unreachable_unchecked_even_in_debug-like (even if it does not solve the "there is no checked API" case, but I don't know if that is an issue). For me, core::hint::unreachable_unchecked was that in the past, and things like unwrap_unchecked() were the "checked in debug" ones.

I also just noticed that using the intrinsic for slice indexing is not enough to remove the check in a debug build and get vectorization back (included in the link).

Cc #120848.
Cc @saethlin.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 12, 2024
@ojeda
Copy link
Contributor Author

ojeda commented Feb 12, 2024

I started typing out a long reply, but I'll wait for a new issue. For now, please check #120848, I've already thought of quite a few follow-up tasks on these checks and wouldn't mind adding to the list.

Thanks Ben -- I see you want things like get_unchecked to use intrinsics, which would solve (I guess) the slice indexing bit I mention above, right?

@clarfonthey
Copy link
Contributor

clarfonthey commented Feb 12, 2024

I think that it's very fair to separate out unsafe preconditions from standard debug assertions, since these are operations where the developer is stating they've already met the preconditions and the extra checks are only there for, well, cases where they missed up.

Perhaps a separate unsafe_preconditions flag could be added and the assertions could be exposed to end users who wish to add them to their own crates? The default would be to enable them alongside debug assertions, but they could be enabled separately. Perhaps they could even be enabled with debug assertions off, since in some ways, the two checks are usually redundant.

Another important thing to consider here is that methods on top of the intrinsics using these checks are done intentionally so that less logic actually has to be included in the intrinsics themselves. From this perspective, it's actually less desirable to push people to use the intrinsics directly, since those are unstable whereas the other methods are intended to eventually become stable.

@the8472
Copy link
Member

the8472 commented Feb 12, 2024

Afaik debug-asserts works on a per-crate level. So one option is to move the hot paths into a separate crate and applying package overrides to it when building the binary.

Generally I wouldn't expect vectorization to work with debug asserts. Practically no effort is made to make this work. E.g. none of the codegen tests run when std debug asserts are enabled.

@saethlin
Copy link
Member

Afaik debug-asserts works on a per-crate level. So one option is to move the hot paths into a separate crate and applying package overrides to it when building the binary.

@the8472 These checks specifically do not work that way. Please see my PR that implemented the system they use: #120594

@the8472
Copy link
Member

the8472 commented Feb 12, 2024

Hrrm, that seems problematic because it renders the usual advice to do that kind of splitting ineffective.

@saethlin
Copy link
Member

Thanks Ben -- I see you want things like get_unchecked to use intrinsics, which would solve (I guess) the slice indexing bit I mention above, right?

No. My wish/goal is that if a user of the standard library calls any function in a manner which is locally invalid (i.e. the kinds of things ubsan finds, not asan/msan/tsan), that erroneous call is detected at runtime, by the default cargo run/cargo test. I mentioned using an intrinsic so that we can have a single check, and a helpful error message from get_unchecked instead of catching the problem in a callee, which does prevent the UB but has a generic and less-helpful message.

and get vectorization back

Since you were getting vectorization before, are you compiling with -Copt-level=1 in your debug builds? If you are, I think your use case is better satisfied by one of these items in the issue I linked:

The actual checks are hidden behind #[inline(never)] to prevent monomorphizing them many times, but that attribute also means LLVM cannot see what is being checked and optimize on it. Try changing the monomorphic check function from #[inline(never)] to some new attribute that makes them inlinable by LLVM, but not by the MIR inliner. Perhaps we call this #[inline(only_post_mono)]?

Try to deduplicate checks in a MIR pass or codegen. It's possible that after GVN we end up with MIR where a call dominates another call with the same argument(s).

Or we could also toggle some of the checks off at -Copt-level=1.

@ojeda
Copy link
Contributor Author

ojeda commented Feb 12, 2024

My wish/goal is that if a user of the standard library calls any function in a manner which is locally invalid (...) that erroneous call is detected at runtime, (...) I mentioned using an intrinsic so that we can have a single check

I was not talking about avoiding the check completely. What I meant is that, from the quick test I did (in the CE link) with get + intrinsics, at least one check is not getting removed, which is what I guessed broke vectorization. I was hoping it was related to the duplicated checks somehow, and therefore that by removing them (i.e. simplifying) the compiler would be able to vectorize again. But from what are saying, the checks are done in a way that they are not currently inlinable, which sounds like a more likely reason. :)

Since you were getting vectorization before, are you compiling with -Copt-level=1 in your debug builds? If you are, I think your use case is better satisfied by one of these items in the issue I linked:

Please see the CE link I mentioned in OP -- it uses -O. It was just an example of projects with "fast debug" builds (or "release with asserts"), which may be compiled with -Copt-level=2 or even -Copt-level=3, and where a decrease of performance may make the debug build unusable for certain use cases because there may be some real-time constraints. In those projects, you can typically keep the vast majority of checks enabled, but in the hot parts of the code you may need to avoid a few of them.

That is why keeping the ability to selectively remove the checks in certain cases for -Cdebug-assertions=y builds is important (i.e. you don't want to remove all the checks either), and thus the core::hint::unreachable_unchecked_even_in_debug-like ideas.

@ojeda
Copy link
Contributor Author

ojeda commented Feb 12, 2024

The actual checks are hidden behind #[inline(never)] to prevent monomorphizing them many times, but that attribute also means LLVM cannot see what is being checked and optimize on it. Try changing the monomorphic check function from #[inline(never)] to some new attribute that makes them inlinable by LLVM, but not by the MIR inliner. Perhaps we call this #[inline(only_post_mono)]?

Try to deduplicate checks in a MIR pass or codegen. It's possible that after GVN we end up with MIR where a call dominates another call with the same argument(s).

From what I can tell, you would still do the checks even with those items done, right? i.e. you are just removing the duplicated checks and making them inlinable etc., but the check would still need to be emitted in cases it cannot be proven to not happen. So since the checks would still be there in those cases, it is likely not enough, unless combined with a core::hint::unreachable_unchecked_even_in_debug or similar.

Or we could also toggle some of the checks off at -Copt-level=1.

Ideally, the users would pick what they need to disable, ideally per-call (i.e. not even per check, but per instance of the check, e.g. I may want 99% of the get_unchecked calls to be checked, but not particular ones in the hot parts). That is why I mentioned the core::hint::unreachable_unchecked_even_in_debug (which would be like the intrinsic) or similar.

@saethlin
Copy link
Member

From what I can tell, you would still do the checks even with those items done, right?

Currently, all of the checks regardless of complexity are hidden behind a #[inline(never)] function call. I'm going to move the slice bounds check condition out of the function very soon because my benchmarking indicates that this doesn't have a detrimental effect on compile time.

Of course that doesn't get vectorization in your example code, but it should help on larger programs.

Ideally, the users would pick what they need to disable, ideally per-call

I don't currently know how to implement this. I am very wary of increasing the library maintenance burden or adding language features in the form of in-source annotations to support this. Maybe someone else who knows the library or compiler better sees how this could be done.

@saethlin saethlin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 12, 2024
@the8472
Copy link
Member

the8472 commented Feb 12, 2024

I think in-source annotations would be of limited use since those checks can be buried behind several layers of abstraction in a library. Iterators are an obvious example. Unless they applied to the call-tree, but we don't have many of those.

@RalfJung
Copy link
Member

I think Miri is running into this. We are running CI with debug assertions + optimizations (since I want to catch e.g. integer overflows). When #120594 landed, our overall CI times increased by 50%.

@saethlin
Copy link
Member

Some quick profiling indicates that ./miri test currently spends ~31% of the time in the interpreter executing the slice::from_raw_parts precondition check (and no time in others). I suspect most of that is from Vec's Deref impl.

@RalfJung
Copy link
Member

RalfJung commented Feb 13, 2024

That seems to align pretty well with the 50% slowdown -- the old execution time is around 66% of the current execution time, so if you can shave off 31% that brings it back very close to where we started.

@RalfJung
Copy link
Member

Given the heavy perf impact, I am going to mark this as a regression.

@RalfJung RalfJung added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Feb 17, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Feb 17, 2024
@RalfJung RalfJung added I-slow Issue: Problems and improvements with respect to performance of generated code. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Feb 17, 2024
@Noratrieb
Copy link
Member

I think it's reasonable to make less of the standard library (like vec::deref) use the checked from_raw_parts

@RalfJung
Copy link
Member

RalfJung commented Feb 17, 2024 via email

@Noratrieb
Copy link
Member

I guess most of the current negative perf impact should go away once the check is able to be inlined. But using the extra-ub-checks flag or however we wanted to expose it for this makes sense.

@RalfJung
Copy link
Member

I was more thinking of cfg(ub_checks), defaults to the same value as cfg(debug_assertions) but can be set separately if desired.

@saethlin
Copy link
Member

This issue is about being able to disable a specific instantiation of a specific check, so I do not think globally-applied flags or globally-set cfgs are relevant.

@RalfJung RalfJung removed I-slow Issue: Problems and improvements with respect to performance of generated code. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 17, 2024
@RalfJung
Copy link
Member

RalfJung commented Feb 17, 2024

I was interpreting this as a more general issue for how to deal with the perf issues around these precondition checks. Giving the author of the check more control is just one way to achieve that. But we can make that a separate issue 🤷

Opened #121245.

@RalfJung
Copy link
Member

I've sometimes wanted a way to enable/disable arithmetic overflow checks in a particular region of the code. This issue (the original description) sounds similar. But such a mechanism seems pretty tricky to design so I don't think we are anywhere close to having something like that.

@asquared31415
Copy link
Contributor

data point: C# has checked and unchecked blocks (that look syntactically like Rust's unsafe blocks): https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/checked-and-unchecked

These control the behavior of integer overflow at a block-scoped level. I don't think that such a design is particularly useful for configuring extra UB checks, but it may be something to think about.

@saethlin
Copy link
Member

The obstacle here is not coming up with speculative designs for such a thing, but implementing any of them. I'm sure there will be an incredible amount of bikeshedding, but maybe we can at least postone that.

The nearest design to this that I can think of is #[rustc_inherit_overflow_checks], but crucially that is handled in MIR building, where "which function did this Rvalue come from" is obvious. But Rvalue::DebugAssertions is lowered after MIR inlining and other such optimizations have happened.

ojeda added a commit to ojeda/linux that referenced this issue Apr 1, 2024
This is the next upgrade to the Rust toolchain, from 1.77.1 to 1.78.0
(i.e. the latest) [1].

See the upgrade policy [2] and the comments on the first upgrade in
commit 3ed03f4 ("rust: upgrade to Rust 1.68.2").

# Unstable features

There have been no changes to the set of unstable features used in
our own code. Therefore, the only unstable features allowed to be used
outside the `kernel` crate is still `new_uninit`.

However, since we are finally dropping our `alloc` fork [3], all the
unstable features used by `alloc` (~30 language ones, ~60 library ones)
are not a concern anymore. This reduces the maintanance burden, increases
the chances of new compiler versions working without changes and gets
us closer to the goal of supporting several compiler versions.

It also means that, ignoring non-language/library features, we are
currently left with just the few language features needed to implement the
kernel `Arc`, the `new_uninit` library feature, the `compiler_builtins`
marker and the few `no_*` `cfg`s we pass when compiling `core`/`alloc`.

Please see [4] for details.

# Required changes

## LLVM's data layout

Rust 1.77.0 (i.e. the previous upgrade) introduced a check for matching
LLVM data layouts [5]. Then, Rust 1.78.0 upgraded LLVM's bundled major
version from 17 to 18 [6], which changed the data layout in x86 [7]. Thus
update the data layout in our custom target specification for x86 so
that the compiler does not complain about the mismatch:

    error: data-layout for target `target-5559158138856098584`,
    `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128`,
    differs from LLVM target's `x86_64-linux-gnu` default layout,
    `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128`

In the future, the goal is to drop the custom target specification
files. Meanwhile, if we want to support other LLVM versions used
in `rustc` (e.g. for LTO), we will need to add some extra logic
(e.g. conditional on LLVM's version, or extracting the data layout from
an existing built-in target specification).

## `unused_imports`

Rust's `unused_imports` lint covers both unused and redudant imports.
Now, in 1.78.0, the lint detects more cases of redundant imports [8].
Thus the previous commit cleaned them up.

## Clippy's `new_without_default`

Clippy now suggests to implement `Default` even when `new()` is `const`,
since `Default::default()` may call `const` functions even if it is not
`const` itself [9]. Thus the previous commit added the implementation.

# Other changes in Rust

Rust 1.78.0 introduces `feature(asm_goto)` [10] [11]. This feature was
discussed in the past [12].

Rust 1.78.0 introduced support for mutable pointers to Rust statics,
including a test case for the Linux kernel's `VTABLE` use case [13].

Rust 1.78.0 with debug assertions enabled (i.e. `-Cdebug-assertions=y`,
kernel's `CONFIG_RUST_DEBUG_ASSERTIONS=y`) will now always check all
unsafe preconditions, without a way to opt-out for particular cases [14].

Rust 1.78.0 also improved a couple issues we reported when giving feedback
for the new `--check-cfg` feature [15] [16].

# `alloc` upgrade and reviewing

As mentioned above, compiler upgrades will not update `alloc` anymore,
since we are dropping our `alloc` fork [3].

As a bonus, even if that series is not applied, the new compiler release
happens to build cleanly the existing `alloc` too (i.e. the previous
version's).

Link: https://github.com/rust-lang/rust/blob/stable/RELEASES.md#version-1770-2024-03-21 [1]
Link: https://rust-for-linux.com/rust-version-policy [2]
Link: https://lore.kernel.org/rust-for-linux/20240328013603.206764-1-wedsonaf@gmail.com/ [3]
Link: Rust-for-Linux#2 [4]
Link: rust-lang/rust#120062 [5]
Link: rust-lang/rust#120055 [6]
Link: https://reviews.llvm.org/D86310 [7]
Link: rust-lang/rust#117772 [8]
Link: rust-lang/rust-clippy#10903 [9]
Link: rust-lang/rust#119365 [10]
Link: rust-lang/rust#119364 [11]
Link: https://lore.kernel.org/rust-for-linux/ZWipTZysC2YL7qsq@Boquns-Mac-mini.home/ [12]
Link: rust-lang/rust#120932 [13]
Link: rust-lang/rust#120969 [14]
Link: rust-lang/rust#121202 [15]
Link: rust-lang/rust#121237 [16]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda added a commit to ojeda/linux that referenced this issue Apr 1, 2024
This is the next upgrade to the Rust toolchain, from 1.77.1 to 1.78.0
(i.e. the latest) [1].

See the upgrade policy [2] and the comments on the first upgrade in
commit 3ed03f4 ("rust: upgrade to Rust 1.68.2").

# Unstable features

There have been no changes to the set of unstable features used in
our own code. Therefore, the only unstable features allowed to be used
outside the `kernel` crate is still `new_uninit`.

However, since we are finally dropping our `alloc` fork [3], all the
unstable features used by `alloc` (~30 language ones, ~60 library ones)
are not a concern anymore. This reduces the maintenance burden, increases
the chances of new compiler versions working without changes and gets
us closer to the goal of supporting several compiler versions.

It also means that, ignoring non-language/library features, we are
currently left with just the few language features needed to implement the
kernel `Arc`, the `new_uninit` library feature, the `compiler_builtins`
marker and the few `no_*` `cfg`s we pass when compiling `core`/`alloc`.

Please see [4] for details.

# Required changes

## LLVM's data layout

Rust 1.77.0 (i.e. the previous upgrade) introduced a check for matching
LLVM data layouts [5]. Then, Rust 1.78.0 upgraded LLVM's bundled major
version from 17 to 18 [6], which changed the data layout in x86 [7]. Thus
update the data layout in our custom target specification for x86 so
that the compiler does not complain about the mismatch:

    error: data-layout for target `target-5559158138856098584`,
    `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128`,
    differs from LLVM target's `x86_64-linux-gnu` default layout,
    `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128`

In the future, the goal is to drop the custom target specification
files. Meanwhile, if we want to support other LLVM versions used
in `rustc` (e.g. for LTO), we will need to add some extra logic
(e.g. conditional on LLVM's version, or extracting the data layout from
an existing built-in target specification).

## `unused_imports`

Rust's `unused_imports` lint covers both unused and redundant imports.
Now, in 1.78.0, the lint detects more cases of redundant imports [8].
Thus one of the previous patches cleaned them up.

## Clippy's `new_without_default`

Clippy now suggests to implement `Default` even when `new()` is `const`,
since `Default::default()` may call `const` functions even if it is not
`const` itself [9]. Thus one of the previous patches implemented it.

# Other changes in Rust

Rust 1.78.0 introduced `feature(asm_goto)` [10] [11]. This feature was
discussed in the past [12].

Rust 1.78.0 introduced support for mutable pointers to Rust statics,
including a test case for the Linux kernel's `VTABLE` use case [13].

Rust 1.78.0 with debug assertions enabled (i.e. `-Cdebug-assertions=y`,
kernel's `CONFIG_RUST_DEBUG_ASSERTIONS=y`) now always checks all unsafe
preconditions, without a way to opt-out for particular cases [14].

Rust 1.78.0 also improved a couple issues we reported when giving feedback
for the new `--check-cfg` feature [15] [16].

# `alloc` upgrade and reviewing

As mentioned above, compiler upgrades will not update `alloc` anymore,
since we are dropping our `alloc` fork [3].

Link: https://github.com/rust-lang/rust/blob/stable/RELEASES.md#version-1780-2024-05-02 [1]
Link: https://rust-for-linux.com/rust-version-policy [2]
Link: https://lore.kernel.org/rust-for-linux/20240328013603.206764-1-wedsonaf@gmail.com/ [3]
Link: Rust-for-Linux#2 [4]
Link: rust-lang/rust#120062 [5]
Link: rust-lang/rust#120055 [6]
Link: https://reviews.llvm.org/D86310 [7]
Link: rust-lang/rust#117772 [8]
Link: rust-lang/rust-clippy#10903 [9]
Link: rust-lang/rust#119365 [10]
Link: rust-lang/rust#119364 [11]
Link: https://lore.kernel.org/rust-for-linux/ZWipTZysC2YL7qsq@Boquns-Mac-mini.home/ [12]
Link: rust-lang/rust#120932 [13]
Link: rust-lang/rust#120969 [14]
Link: rust-lang/rust#121202 [15]
Link: rust-lang/rust#121237 [16]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda added a commit to ojeda/linux that referenced this issue Apr 1, 2024
This is the next upgrade to the Rust toolchain, from 1.77.1 to 1.78.0
(i.e. the latest) [1].

See the upgrade policy [2] and the comments on the first upgrade in
commit 3ed03f4 ("rust: upgrade to Rust 1.68.2").

# Unstable features

There have been no changes to the set of unstable features used in
our own code. Therefore, the only unstable features allowed to be used
outside the `kernel` crate is still `new_uninit`.

However, since we are finally dropping our `alloc` fork [3], all the
unstable features used by `alloc` (~30 language ones, ~60 library ones)
are not a concern anymore. This reduces the maintenance burden, increases
the chances of new compiler versions working without changes and gets
us closer to the goal of supporting several compiler versions.

It also means that, ignoring non-language/library features, we are
currently left with just the few language features needed to implement the
kernel `Arc`, the `new_uninit` library feature, the `compiler_builtins`
marker and the few `no_*` `cfg`s we pass when compiling `core`/`alloc`.

Please see [4] for details.

# Required changes

## LLVM's data layout

Rust 1.77.0 (i.e. the previous upgrade) introduced a check for matching
LLVM data layouts [5]. Then, Rust 1.78.0 upgraded LLVM's bundled major
version from 17 to 18 [6], which changed the data layout in x86 [7]. Thus
update the data layout in our custom target specification for x86 so
that the compiler does not complain about the mismatch:

    error: data-layout for target `target-5559158138856098584`,
    `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128`,
    differs from LLVM target's `x86_64-linux-gnu` default layout,
    `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128`

In the future, the goal is to drop the custom target specifications.
Meanwhile, if we want to support other LLVM versions used in `rustc`
(e.g. for LTO), we will need to add some extra logic (e.g. conditional on
LLVM's version, or extracting the data layout from an existing built-in
target specification).

## `unused_imports`

Rust's `unused_imports` lint covers both unused and redundant imports.
Now, in 1.78.0, the lint detects more cases of redundant imports [8].
Thus one of the previous patches cleaned them up.

## Clippy's `new_without_default`

Clippy now suggests to implement `Default` even when `new()` is `const`,
since `Default::default()` may call `const` functions even if it is not
`const` itself [9]. Thus one of the previous patches implemented it.

# Other changes in Rust

Rust 1.78.0 introduced `feature(asm_goto)` [10] [11]. This feature was
discussed in the past [12].

Rust 1.78.0 introduced support for mutable pointers to Rust statics,
including a test case for the Linux kernel's `VTABLE` use case [13].

Rust 1.78.0 with debug assertions enabled (i.e. `-Cdebug-assertions=y`,
kernel's `CONFIG_RUST_DEBUG_ASSERTIONS=y`) now always checks all unsafe
preconditions, without a way to opt-out for particular cases [14].

Rust 1.78.0 also improved a couple issues we reported when giving feedback
for the new `--check-cfg` feature [15] [16].

# `alloc` upgrade and reviewing

As mentioned above, compiler upgrades will not update `alloc` anymore,
since we are dropping our `alloc` fork [3].

Link: https://github.com/rust-lang/rust/blob/stable/RELEASES.md#version-1780-2024-05-02 [1]
Link: https://rust-for-linux.com/rust-version-policy [2]
Link: https://lore.kernel.org/rust-for-linux/20240328013603.206764-1-wedsonaf@gmail.com/ [3]
Link: Rust-for-Linux#2 [4]
Link: rust-lang/rust#120062 [5]
Link: rust-lang/rust#120055 [6]
Link: https://reviews.llvm.org/D86310 [7]
Link: rust-lang/rust#117772 [8]
Link: rust-lang/rust-clippy#10903 [9]
Link: rust-lang/rust#119365 [10]
Link: rust-lang/rust#119364 [11]
Link: https://lore.kernel.org/rust-for-linux/ZWipTZysC2YL7qsq@Boquns-Mac-mini.home/ [12]
Link: rust-lang/rust#120932 [13]
Link: rust-lang/rust#120969 [14]
Link: rust-lang/rust#121202 [15]
Link: rust-lang/rust#121237 [16]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda added a commit to ojeda/linux that referenced this issue May 5, 2024
This is the next upgrade to the Rust toolchain, from 1.77.1 to 1.78.0
(i.e. the latest) [1].

See the upgrade policy [2] and the comments on the first upgrade in
commit 3ed03f4 ("rust: upgrade to Rust 1.68.2").

It is much smaller than previous upgrades, since the `alloc` fork was
dropped in commit 9d0441b ("rust: alloc: remove our fork of the
`alloc` crate") [3].

# Unstable features

There have been no changes to the set of unstable features used in
our own code. Therefore, the only unstable features allowed to be used
outside the `kernel` crate is still `new_uninit`.

However, since we finally dropped our `alloc` fork [3], all the unstable
features used by `alloc` (~30 language ones, ~60 library ones) are not
a concern anymore. This reduces the maintenance burden, increases the
chances of new compiler versions working without changes and gets us
closer to the goal of supporting several compiler versions.

It also means that, ignoring non-language/library features, we are
currently left with just the few language features needed to implement the
kernel `Arc`, the `new_uninit` library feature, the `compiler_builtins`
marker and the few `no_*` `cfg`s we pass when compiling `core`/`alloc`.

Please see [4] for details.

# Required changes

## LLVM's data layout

Rust 1.77.0 (i.e. the previous upgrade) introduced a check for matching
LLVM data layouts [5]. Then, Rust 1.78.0 upgraded LLVM's bundled major
version from 17 to 18 [6], which changed the data layout in x86 [7]. Thus
update the data layout in our custom target specification for x86 so
that the compiler does not complain about the mismatch:

    error: data-layout for target `target-5559158138856098584`,
    `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128`,
    differs from LLVM target's `x86_64-linux-gnu` default layout,
    `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128`

In the future, the goal is to drop the custom target specifications.
Meanwhile, if we want to support other LLVM versions used in `rustc`
(e.g. for LTO), we will need to add some extra logic (e.g. conditional on
LLVM's version, or extracting the data layout from an existing built-in
target specification).

## `unused_imports`

Rust's `unused_imports` lint covers both unused and redundant imports.
Now, in 1.78.0, the lint detects more cases of redundant imports [8].
Thus one of the previous patches cleaned them up.

## Clippy's `new_without_default`

Clippy now suggests to implement `Default` even when `new()` is `const`,
since `Default::default()` may call `const` functions even if it is not
`const` itself [9]. Thus one of the previous patches implemented it.

# Other changes in Rust

Rust 1.78.0 introduced `feature(asm_goto)` [10] [11]. This feature was
discussed in the past [12].

Rust 1.78.0 introduced `feature(const_refs_to_static)` [13] to allow
referencing statics in constants and extended `feature(const_mut_refs)`
to allow raw mutable pointers in constants. Together, this should cover
the kernel's `VTABLE` use case. In fact, the implementation [14] in
upstream Rust added a test case for it [15].

Rust 1.78.0 with debug assertions enabled (i.e. `-Cdebug-assertions=y`,
kernel's `CONFIG_RUST_DEBUG_ASSERTIONS=y`) now always checks all unsafe
preconditions, though without a way to opt-out for particular cases [16].
It would be ideal to have a way to selectively disable certain checks
per-call site for this one (i.e. not just per check but for particular
instances of a check), even if the vast majority of the checks remain
in place [17].

Rust 1.78.0 also improved a couple issues we reported when giving feedback
for the new `--check-cfg` feature [18] [19].

# `alloc` upgrade and reviewing

As mentioned above, compiler upgrades will not update `alloc` anymore,
since we dropped our `alloc` fork [3].

Link: https://github.com/rust-lang/rust/blob/stable/RELEASES.md#version-1780-2024-05-02 [1]
Link: https://rust-for-linux.com/rust-version-policy [2]
Link: https://lore.kernel.org/rust-for-linux/20240328013603.206764-1-wedsonaf@gmail.com/ [3]
Link: Rust-for-Linux#2 [4]
Link: rust-lang/rust#120062 [5]
Link: rust-lang/rust#120055 [6]
Link: https://reviews.llvm.org/D86310 [7]
Link: rust-lang/rust#117772 [8]
Link: rust-lang/rust-clippy#10903 [9]
Link: rust-lang/rust#119365 [10]
Link: rust-lang/rust#119364 [11]
Link: https://lore.kernel.org/rust-for-linux/ZWipTZysC2YL7qsq@Boquns-Mac-mini.home/ [12]
Link: rust-lang/rust#119618 [13]
Link: rust-lang/rust#120932 [14]
Link: https://github.com/rust-lang/rust/pull/120932/files#diff-e6fc1622c46054cd46b1d225c5386c5554564b3b0fa8a03c2dc2d8627a1079d9 [15]
Link: rust-lang/rust#120969 [16]
Link: Rust-for-Linux#354 [17]
Link: rust-lang/rust#121202 [18]
Link: rust-lang/rust#121237 [19]
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/r/20240401212303.537355-4-ojeda@kernel.org
[ Added a few more details and links I mentioned in the list. - Miguel ]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@saethlin
Copy link
Member

saethlin commented Aug 28, 2024

In #129498 I'm experimenting with ways to get the precondition checks for ptr::{read,write} enabled. One of the things I'm trying is inventing an attribute currently called #[rustc_no_ubchecks] which makes the MIR optimization pipeline delete checks that got inlined into the body of the indicated function. I don't think this idea is particularly good or possible to stabilize, but it is easy to implement. If it lands, it would be a thing for RFL to play around with, just in case you happen to find a use for it.

This could probably be easily extended to (again) a crude general mechanism by putting the attribute on a closure to disable checks in a few lines of code instead of "a whole function". If the inliner cooperates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

8 participants