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

[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

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[oli-obk]

@bors r+ rollup

View changes since this review

[rust-bors]

📋 This PR cannot be approved because it currently has the following labels: proposed-final-comment-period, needs-reference-pr.

[traviscross]

Thanks @sjwang05.

@bors r=oli-obk,traviscross rollup

[rust-bors]

📌 Commit 9ac2606 has been approved by oli-obk,traviscross

It is now in the queue for this repository.