const_mut_refs: allow mutable pointers to statics by RalfJung · Pull Request #120932 · rust-lang/rust (original) (raw)

Fixes #118447

Writing this PR became a bit messy, see Zulip for some of my journey.^^ Turns out there was a long-standing bug in our qualif logic that led to us incorrectly classifying certain places as "no interior mut" when they actually had interior mut. Due to that the const_refs_to_cell feature gate was required far less often than it otherwise would, e.g. in this code. Fixing this however would be a massive breaking change all over libcore and likely the wider ecosystem. So I also changed the const-checking logic to just not require the feature gate for the affected cases. While doing so I discovered a bunch of logic that is not explained and that I could not explain. However I think stabilizing some const-eval feature will make most of that logic inconsequential so I just added some FIXMEs and left it at that.

r? @oli-obk

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Feb 11, 2024

// `StorageDead` in every control flow path leading to a `return` terminator.
// The good news is that interning will detect if any unexpected mutable
// pointer slips through.
if place.is_indirect_first_projection() \|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this runs after the deref'er? Debug assertions would fire if the Deref was later in the projection chain.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I get a bunch of debug assertion failures.^^

Hm, but... the "reborrow" checks only check the first projection. How does that make sense then?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

derefer is still after borrowck. We haven't managed to move it earlier yet

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means &mut (*ptr).field will not hit the reborrow code path in const-checking. I wonder if there's an example here where this PR changes behavior because of that...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah no, not hitting that codepath is largely inconsequential for shared references since the qualif will say "there is no interior mutability" for any path that involves a Deref, and thus just allow it.

So there's just the mutable reborrow path that could act strangely. I'm surprised we allow mutable reborrows at all given that we don't usually want to allow anything with mutable borrows.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think for mutable references that reborrow logic largely serves to avoid duplicate errors, and to paper over particular desugarings (like how _ as *mut _ becomes a reborrow).

I can't wait to rip out all this half-effective logic when we stabilize const_mut_refs.^^

This comment has been minimized.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this file does not trigger error code 0017. Nor does the other file trigger error code 0388.

Cc #120935

ojeda mentioned this pull request

Feb 11, 2024

41 tasks

This comment has been minimized.

RalfJung changed the title~~const_mut_refs: allow mutable refs to statics~~ const_mut_refs: allow mutable pointers to statics

Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before I try to understand the full logic, here are some things I def don't understand

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Feb 12, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Feb 12, 2024

check_consts: fix duplicate errors, make importance consistent

This is stuff I noticed while working on rust-lang#120932, but it's orthogonal to that PR.

r? @oli-obk

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Feb 12, 2024

check_consts: fix duplicate errors, make importance consistent

This is stuff I noticed while working on rust-lang#120932, but it's orthogonal to that PR.

r? @oli-obk

oli-obk added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Feb 12, 2024

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Feb 12, 2024

Rollup merge of rust-lang#120933 - RalfJung:const-check-misc, r=oli-obk

check_consts: fix duplicate errors, make importance consistent

This is stuff I noticed while working on rust-lang#120932, but it's orthogonal to that PR.

r? @oli-obk

📌 Commit 340f8aa has been approved by oli-obk

It is now in the queue for this repository.

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Feb 17, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Feb 17, 2024

Rollup of 8 pull requests

Successful merges:

rust-lang#119032 (Use a hardcoded constant instead of calling OpenProcessToken.)
rust-lang#120932 (const_mut_refs: allow mutable pointers to statics)
rust-lang#121059 (Add and use a simple extension trait derive macro in the compiler)
rust-lang#121135 (coverage: Discard spans that fill the entire function body)
rust-lang#121187 (Add examples to document the return type of quickselect functions)
rust-lang#121191 (Add myself to review rotation (and a rustbot ping))
rust-lang#121192 (Give some intrinsics fallback bodies)
rust-lang#121197 (Ensure ./configure works when configure.py path contains spaces)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Feb 17, 2024

ojeda mentioned this pull request

Feb 17, 2024

95 tasks

This turned out to be a breaking change: #121250.

Let's see if we can crater this after the fact...

@craterbot run start=dfdbe30004a095ad63951c4cd6e10a9a971a7399 end=ba824a2e25948a86596ccf7594afe548020e86e6 mode=check-only

Since this crater run is on the critical path for what to do with #121250...
@craterbot p=1

📝 Configuration of the pr-120932 experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot start=master#dfdbe30004a095ad63951c4cd6e10a9a971a7399 end=master#ba824a2e25948a86596ccf7594afe548020e86e6

📝 Configuration of the pr-120932 experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

🚧 Experiment pr-120932 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

ojeda added a commit to ojeda/linux that referenced this pull request

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_* cfgs 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).

ojeda added a commit to ojeda/linux that referenced this pull request

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.

Please see [4] for details.

Required changes

LLVM's data layout

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`

`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].

ojeda added a commit to ojeda/linux that referenced this pull request

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.

Please see [4] for details.

Required changes

LLVM's data layout

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`

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 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].

lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request

Apr 7, 2024

check_consts: fix duplicate errors, make importance consistent

This is stuff I noticed while working on rust-lang/rust#120932, but it's orthogonal to that PR.

r? @oli-obk

RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request

Apr 27, 2024

check_consts: fix duplicate errors, make importance consistent

This is stuff I noticed while working on rust-lang/rust#120932, but it's orthogonal to that PR.

r? @oli-obk

ojeda added a commit to ojeda/linux that referenced this pull request

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.

Please see [4] for details.

Required changes

LLVM's data layout

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`

`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`

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].

const_mut_refs: allow mutable pointers to statics by RalfJung · Pull Request #120932 · rust-lang/rust (original) (raw)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Unstable features

Required changes

LLVM's data layout

unused_imports

Clippy's new_without_default

Other changes in Rust

alloc upgrade and reviewing

Unstable features

Required changes

LLVM's data layout

unused_imports

Clippy's new_without_default

Other changes in Rust

alloc upgrade and reviewing

Unstable features

Required changes

LLVM's data layout

unused_imports

Clippy's new_without_default

Other changes in Rust

alloc upgrade and reviewing

Unstable features

Required changes

LLVM's data layout

unused_imports

Clippy's new_without_default

Other changes in Rust

alloc upgrade and reviewing

`unused_imports`

Clippy's `new_without_default`

`alloc` upgrade and reviewing

`unused_imports`

Clippy's `new_without_default`

`alloc` upgrade and reviewing

`unused_imports`

Clippy's `new_without_default`

`alloc` upgrade and reviewing

`unused_imports`

Clippy's `new_without_default`

`alloc` upgrade and reviewing