Do not promote extern statics

[sjwang05]

View all comments

We already reject thread-local statics during promotion, so extend that check to extern statics as well.

fixes #143174

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 20 candidates

[rustbot]

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to constck

cc @fee1-dead

[oli-obk]

So... This is technically a breaking change, as we now have a new const check, which previously only errored if actually evaluated. So an associated const could now error, if it previously was unused in the current crate.

A different oddity is that we don't check this in const fns, as those can be fine at runtime, as long as the extern static is never read at compile time.

Thoughts @rust-lang/wg-const-eval?

[lcnr]

crater than FCP merge in that case?

[RalfJung]

Even though we're allowed to reference extern statics, it's impossible to know the value of an extern static during CTFE when it's used by-value, so we shouldn't try promoting them.

The PR changes const-checking, not promotion. Something seems wrong, what is this actually trying to fix?

This looks like an ad-hoc special case in const checking; I am not sure what this buys us since it can be trivially bypassed (by creating a pointer and then deref'ing it).

To fix #143174, I would have expected this to change promotion (compiler/rustc_mir_transform/src/promote_consts.rs), not const checking.

[RalfJung]

We already avoid promotion in this:

fn main() { unsafe {
let f: &Foo = &Foo(BAR);
} }

Why do we promote in the other case?

[oli-obk]

This PR originally had a promotion time check. Preventing it in const check covers exactly the problematic use case, as everything else doesn't get promoted.

[RalfJung]

I don't think we should fix a promotion bug by changing const-checking. That's a very non-local invariant relying on the subtle interplay of which exact code gets rejected where.

Promotion on its own is supposed to only accept code that cannot fail to evaluate. It apparently does not do this job properly.

[sjwang05]

To fix #143174, I would have expected this to change promotion (compiler/rustc_mir_transform/src/promote_consts.rs), not const checking.

The original version of this PR actually did modify promotion; the diff is available here: 8877f4c. We decided to change it because the original logic felt too messy: #157641 (comment), and I just forgot to change the PR description after making the change.

Why do we promote in the other case?

Promotion is gated behind let Some(hir::ConstContext::Static(..)) = self.const_kind:

rust/compiler/rustc_mir_transform/src/promote_consts.rs

Line 318 in d2f2412

&& let Some(hir::ConstContext::Static(..)) = self.const_kind

so the fn main case never gets promoted, even though both desugar to "deref of a static addr."

[RalfJung]

That check already special-cases thread-local statics here:

rust/compiler/rustc_mir_transform/src/promote_consts.rs

Line 319 in d2f2412

&& !self.tcx.is_thread_local_static(did)

It is very common in the interpreter to treat thread-local statics and extern statics the same, so what I would have expected is that we add && !self.tcx.is_foreign_item(did) there. Does that not work? This is just promotion, so I think excluding any use of extern statics is fine (we'll have to check with crater though).

[sjwang05]

I actually did try that at first, just adding !self.tcx.is_foreign_item(did) at the end of the if-statement; the problem was that this ended up rejecting creating refs/ptrs to extern statics as well, like this line in tests/mir-opt/const_promotion_extern_static.rs:

rust/tests/mir-opt/const_promotion_extern_static.rs

Line 14 in 6368fd5

static mut FOO: *const &i32 = [unsafe { &X }].as_ptr();

since &X still desugars to &(*X_REF), which is the same "deref of a static ref" pattern that's problematic for by-value reads of extern statics, which the original check wrongly failed to compile.

For instance, in the non-problematic case,

extern "C" {
    static X: i32;
}

static mut FOO: *const &i32 = [unsafe { &X }].as_ptr();

gets lowered to

const FOO::promoted[0]: &[&i32; 1] = {
    bb0: {
        _3 = const {alloc1: *const i32}; // addr of X
        _2 = &(*_3);
        _1 = [move _2];
        _0 = &_1;
        return;
    }
}

alloc1 (extern static: X)

after promotion, and since (*_3) is only ever reborrowed and never read by value, CTFE never tries to evaluate X — the promoted just bakes the address in. On the other hand, in the problematic case,

extern "C" {
    static BAR: i32;
}

static FOO: &(i32,) = unsafe { &(BAR,) };

gets lowered to

const FOO::promoted[0]: &(i32,) = {
    bb0: {
        _3 = const {alloc1: *const i32}; // addr of BAR
        _2 = copy (*_3);
        _1 = (move _2,);
        _0 = &_1;
        return;
    }
}

alloc1 (extern static: BAR)

where the copy (*_3) forces CTFE to try to find the value of BAR, causing the ICE. The !self.tcx.is_foreign_item(did) check rejects both instances of *_3, though, regardless of whether or not the value is ever actually read or if we're just taking a ref to it.

[RalfJung]

Yeah I think we should reject such promotion (unless crater disagrees). People can use const blocks if they need this.

[oli-obk]

Cool, let's try that then and crater it. Seems very niche

[oli-obk]

Please also add a test for the snippet that used to compile:

unsafe extern "C" {
    static X: i32;
}

static mut FOO: *const &i32 = [unsafe { &X }].as_ptr();

I think this is a fairly straight forward "make promotion simpler" change.

@rfcbot fcp merge lang

"Stabilization" report

This PR, intended as an ICE fix for

type Fun = unsafe extern "C" fn();

struct Foo(Fun);

static FOO: &Foo = &Foo(BAR); // ICE, reading from an extern static

unsafe extern "C" {
    static BAR: Fun;
}

also breaks the more benign

unsafe extern "C" {
    static X: i32;
}

static mut FOO: *const &i32 = [unsafe { &X }].as_ptr();

instead requiring it to be written as

unsafe extern "C" {
    static X: i32;
}

static HELPER: &i32 = unsafe { &X };
static mut FOO: *const &i32 = [HELPER].as_ptr();

We encountered a single regression on crater (#157641 (comment)), which only exists in an old version of a crate, all newer ones have fixed it. The regression always turns into a compilation failure, unfortunately not in a way that we can make any suggestion for improving it. The diagnostic itself is a generic diagnostic we already emit today and which can get some improvements, but that's orthogonal to this change.

This also claws back one of the accidental capabilities we gave promotion.

While it may be fixable in a way that doesn't break such code, all other ways would just add hacks to the compiler to avoid this really rare case that is easily worked around in code

[rust-rfcbot]

@oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

Makes sense to me. The rule is that we want to reject promotion of code that might fail evaluation. Here, we're doing that by rejecting derefs of extern statics. We could reject less and allow reborrows, but not doing that helps keep promotion simple, which it also should be.

Thanks @sjwang05 for your efforts on this (and to @oli-obk and @RalfJung for the reviews and support).

It's my understanding that these more-local rewrites would also be valid:

unsafe extern "C" { static X: i32; }
static mut FOO: *const &i32 = [const { unsafe { &X } }].as_ptr();

unsafe extern "C" { static X: i32; }
static mut FOO: *const &i32 = const { [unsafe { &X }] }.as_ptr();

@rfcbot reviewed

[scottmcm]

I certainly like moving the tricky cases to requiring const { … } where we can get away with it 👍 Happy to follow the const folks' recommendation here.

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[nikomatsakis]

@rfcbot reviewed

[sjwang05]

Please also add a test for the snippet that used to compile

Done, added a new uitest for it.

Thanks @oli-obk @RalfJung for helping me through this.

[sjwang05]

Reference PR is here: rust-lang/reference#2302