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

Tracking Issue for const_refs_to_static #119618

Open
3 of 6 tasks
RalfJung opened this issue Jan 5, 2024 · 17 comments
Open
3 of 6 tasks

Tracking Issue for const_refs_to_static #119618

RalfJung opened this issue Jan 5, 2024 · 17 comments
Assignees
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-const_refs_to_static `#![feature(const_refs_to_static)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jan 5, 2024

This is a tracking issue for consts referencing statics, a @rust-lang/wg-const-eval experiment.
The feature gate for the issue is #![feature(const_refs_to_static)].

This feature allow constants to refer to statics. However, reading from a static that is mutable (static mut, or static with interior mutability) leads to an evaluator error. This is done to ensure that it doesn't matter when the constant is evaluated, it will always produce the same result.

Having a reference to a mutable static in the final value leads to a validation error. This is necessary to ensure that converting these references to a valtree (e.g. for pattern matching) will not error. (The conversion would error because valtree conversion must never read from anything mutable.)

The same goes for reading from or having a reference to extern static; those obviously can't be read as we can't know their value at compile time. Mutating any static is also not possible -- we can't have global mutable state shared between const-eval queries.

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

  • When a constant refers to or reads from mutable memory, this is currently rejected at type-check time. With this feature it is instead rejected at const-eval time, which can be during monomorphization. However, that's not fundamentally different from all the other ways in which const-eval can fail post-monomorphization.
  • What about the interaction with reference-typed const generics?

Implementation history

This issue has been assigned to @dingxiangfei2009 via this comment.

@RalfJung RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. A-const-eval Area: Constant evaluation (MIR interpretation) labels Jan 5, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Feb 5, 2024

@ojeda I saw you added this to the Rust-for-Linux wishlist. Could you say a bit more about the intended usecase? Last time this came up, it turned out that you actually need #118447, not consts that point to statics. So I wonder if maybe this time there also is a possible alternative.

@Darksonn
Copy link
Contributor

Darksonn commented Feb 8, 2024

My understanding is that many of the vtables in the kernel have the following shape:

use core::ptr::addr_of_mut;

struct ThisModule;

trait Module {
    const THIS_MODULE: *mut ThisModule;
}

struct MyModule;

// Generated by a macro.
extern "C" {
    static mut THIS_MODULE: ThisModule;
}

// Generated by a macro.
impl Module for MyModule {
    const THIS_MODULE: *mut ThisModule = unsafe { addr_of_mut!(THIS_MODULE) };
}

struct Vtable {
    module: *mut ThisModule,
    foo_fn: fn(*mut ()) -> i32,
}

pub trait Foo {
    type Mod: Module;

    fn foo(&mut self) -> i32;
}

fn generate_vtable<T: Foo>() -> &'static Vtable {
    &Vtable {
        module: T::Mod::THIS_MODULE,
        foo_fn: |ptr| unsafe { &mut *ptr.cast::<T>() }.foo(),
    }
}
error[E0013]: constants cannot refer to statics
  --> src/lib.rs:18:64
   |
18 |     const THIS_MODULE: *mut ThisModule = unsafe { addr_of_mut!(THIS_MODULE) };
   |                                                                ^^^^^^^^^^^
   |
   = help: consider extracting the value of the `static` to a `const`, and referring to that

We do not need to read or write to THIS_MODULE at const evaluation time. In fact, since it's an extern static, it doesn't really make sense to even talk about doing that.

@RalfJung
Copy link
Member Author

Your code literally reads the value of THIS_MODULE (an extern static) to create the vtable. That's impossible to do in a const (or static) initializer, and I don't know any way to allow this.

Or did you mean addr_of_mut!(T::Mod::THIS_MODULE)?

@RalfJung
Copy link
Member Author

Oh wait, you have multiple items with the same name in different modules. That's very confusing code. I'll have to de-obfuscate it first.^^

@RalfJung
Copy link
Member Author

I added your testcase in #120932, it then passes with feature(const_mut_refs, const_refs_to_static). :)

@dtolnay
Copy link
Member

dtolnay commented Apr 2, 2024

Potential use case in the standard library:

pub const fn noop() -> &'static Waker {
const WAKER: &Waker = &Waker { waker: RawWaker::NOOP };
WAKER
}

By using const_refs_to_static in the standard library and changing WAKER to a static, one can use waker.will_wake(Waker::noop()) or Waker::noop().will_wake(&waker) to determine whether a particular waker is noop. will_wake is based on pointer identity for the waker's data ptr and vtable ptr.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 2, 2024

There's no mutability here that I can see, so yeah that would work.

Basically it's a trade-off between a lower risk of monomorphization-time errors and letting const to more things. With const_refs_to_static you get a mono-time error if you ever read from or (transitively) have a reference to mutable statics (this includes interior mutability) or extern statics.

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/[email protected]/ [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/[email protected]/ [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 <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
[ Added a few more details and links I mentioned in the list. - Miguel ]
Signed-off-by: Miguel Ojeda <[email protected]>
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jun 8, 2024

What is the current status here -- in particular, the Rust For Linux project makes use of this feature to support creation of vtables (example code) and I am hoping that in the next 6 months we can move their subset to stabilization. @RalfJung or @oli-obk -- can you enlighten me as to what the key blockers and/or questions are?

EDIT: Oh, that example appears earlier in the thread -- right, I forgot, we have already added a testcase to ensure that it continues to compile.

EDIT: Created a Zulip thread to discuss.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 8, 2024

Oh, that example appears earlier in the thread -- right, I forgot, we have already added a testcase to ensure that it continues to compile.

Yeah, the feature was added specifically to support that example. ;)

Main concerns/blockers I can think of:

  • This trades some "early" compile failures for "late" / monomorphization-time failures: when a const has a reference to a static with (interior) mutability, this is now considered an "invalid" const and will fail const validation (similar to e.g. violations of basic type validity invariants). If this is an associated const or an inline const, const validation happens only during monomorphization. Similarly, having a raw pointer to a static with (interior) mutability is allowed, but reading from that raw pointer during const-eval will lead to an error -- and again if this is in an associated/inline const, that error occurs only during monomorphization.
  • Potential open question: instead of considering a constant "invalid" when it has a reference to a static with (interior) mutability, we could allow this and only complain when the constant is used as a pattern, const generic value, or const-eval reads through that reference. However, that would make these errors show up "even later", so I think for now it is better to check this as early as possible.
  • There are subtle interactions with using references in const generics, Cc How should const generics with references work around pointer identity and padding? #120961 @rust-lang/types
  • Also FTR there was never an RFC for this feature, it just got implemented as an experiment.

@nikomatsakis
Copy link
Contributor

There are subtle interactions with using references in const generics, Cc #120961 @rust-lang/types

My general feeling is that we are overrotating on this. It is already observable that generics don't always have unique addresses (e.g., with function pointers). Certainly for types that lack interior mutability this is much less likely to be an issue.

However, reading the issue, I remembered that I still feel a bit uneasy about when two constants are equal, and can never remember if we've landed somewhere or what.

@RalfJung
Copy link
Member Author

While consts, vtables, and function pointers do not have a unique address, so far it has been very clear that static have a unique address. I would say that is one of their defining features. That's why I find this concerning.

IMO we should still allow const_refs_to_static, and do something about this on the side of const generics instead, like reject const generics that point to statics or actually preserve their identity in the valtree somehow or so.

@Ddystopia
Copy link
Contributor

Hello, is it correct that this feature is required for this code?

static A: usize = 0;
static B1: [u8; A] = [0; A];
error[E0658]: referencing statics in constants is unstable
 --> src/main.rs:2:17
  |
2 | static B1: [u8; A] = [0; A];
  |                 ^
  |
  = note: see issue #119618 <https://github.com/rust-lang/rust/issues/119618> for more information
  = help: add `#![feature(const_refs_to_static)]` to the crate attributes to enable
  = note: this compiler was built on 2024-06-13; consider upgrading it if it is out of date
  = note: `static` and `const` variables can refer to other `const` variables. A `const` variable, however, cannot refer to a `static` variable.
  = help: to fix this, the value can be extracted to a `const` and then used.

error[E0658]: referencing statics in constants is unstable
 --> src/main.rs:2:26
  |
2 | static B1: [u8; A] = [0; A];
  |                          ^
  |
  = note: see issue #119618 <https://github.com/rust-lang/rust/issues/119618> for more information
  = help: add `#![feature(const_refs_to_static)]` to the crate attributes to enable
  = note: this compiler was built on 2024-06-13; consider upgrading it if it is out of date
  = note: `static` and `const` variables can refer to other `const` variables. A `const` variable, however, cannot refer to a `static` variable.
  = help: to fix this, the value can be extracted to a `const` and then used.

For more information about this error, try `rustc --explain E0658`.
error: could not compile `play` (bin "play") due to 2 previous errors```

@oli-obk
Copy link
Contributor

oli-obk commented Jun 21, 2024

Yes, array lengths are constants, so the feature is required for that

@Pzixel
Copy link

Pzixel commented Jun 21, 2024

Note that it is currently possible to declare a const fn which mutates static variable. Which leads to a function which is declared as const fn but actually cannot be called in const context. E.g.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=de9d10f90ca407933fd2c10ae85556b6

If we now replace

if let Some(y) = nonzero(X) {

with

if let Some(y) = const { nonzero(X) } {

It won't compile. Which means that const modifier on this function does nothing and shouldn't be allowed.

@RalfJung
Copy link
Member Author

We could detect some simple cases but in general it is your responsibility, as part of writing unsafe code, to ensure this does not happen. Consider that one can take a reference or raw pointer to a static mut and then read through that. It's fine to create the reference, and by the time you read through it we can't know whether it points to a static mut or not.

The const does something, but it can't check everything. Some things are only checked at "runtime" (i.e. during const-eval), at least with const_refs_to_static.

bartlomieju pushed a commit to denoland/rusty_v8 that referenced this issue Jul 20, 2024
#1532)

A subtle unsoundness / undefined behaviour made its way into the fairly recently added ExternalOneByteStringResource object: The as_str API is not sound as the data inside may be be Latin-1, not ASCII.

As the API was not used anywhere in deno or deno_core, I opted to simply remove it and replace it with an as_bytes API. I also modified the test to showcase the Latin-1 string case and added copious notes and explanations around the code to make sure this doesn't accidentally happen again. The likely reason why the API originally slipped in is because the OneByteConst has this API where it is safe because the OneByteConst creation checks the data for ASCII-ness.

I also tried to add an API to extract an Option<&'static OneByteConst> from an &ExternalOneByteStringResource but run into rust-lang/rust#119618 ie. OneByteConst is actually duplicating the vtables... which is not great.
@nikomatsakis
Copy link
Contributor

Stabilization proposal:

#128183

@nikomatsakis
Copy link
Contributor

Seeing as #128183 is in FCP, I'm assigning @dingxiangfei2009 to prepare the stabilization PR:

@rustbot assign @dingxiangfei2009

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-const_refs_to_static `#![feature(const_refs_to_static)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants