Why does const-prop refuse to read from statics that contain pointers?

[RalfJung]

const-prop checks statics for whether they contain relocations, and if they do, it refuses to read from them. It is not clear why that happens.

As discussed in #70241 (comment), it turns out that const-prop will sometimes permit reading from mutable global allocations. That could be a soundness issue in case those allocations are actually mutated at run-time; const-prop would then be propagating non-constant data.

Currently it is not entirely clear what those mutable allocations are that const-prop is reading from, all we have is an ICE because an unexpected error ("const-prop does not support reading from mutable global allocation") arose during validation. It is also unclear (to me, at least) why const-prop is doing validation at all.

Cc @oli-obk @wesleywiser

[RalfJung]

I just realized the condition is allocation.mutability == Mutability::Mut || allocation.relocations().len() > 0, so maybe the allocation in question was not even mutable but had relocations in it. I am testing that now.

I do not understand the argument given in the code for why relocations are a problem:

        // If the static allocation is mutable or if it has relocations (it may be legal to mutate
        // the memory behind that in the future), then we can't const prop it.

If the memory behind that pointer is mutable, we will notice that when that allocation is being accessed. Why do we have to bail out so early? Is it because the hook used to only be called for statics, not all global allocations?

[RalfJung]

Also the mystery about why validation is even happening is answered here:

rust/src/librustc_mir/transform/const_prop.rs

Line 643 in 1add455

if let Err(e) = self.ecx.const_validate_operand(

I think I understand why, conservatively, this does validation... can this ever fail? Validation is quite picky about the kinds of errors in expects can happen, and with const-prop having a machine that bails out early on many operations, that could be a problem. The reason validation is so picky is to make sure we get good errors, which const-prop does not care about (the errors are not shown to the user), but it re-uses the same machinery.

[RalfJung]

Okay so the actual problem turned out to be relocations. There is no bug here, we can reject all reads from mutable globals (phew).

What remains open is why we check for the presence of relocations at all. @wesleywiser you added that comment that I mentioned above, would be great if you could explain it a bit more. :)

[wesleywiser]

@oli-obk wanted it:

for now also bail out if the allocation has relocations. If a static contains a *mut i32 it may be legal to mutate the memory behind that (if not now, maybe in the future).

But I'm not quite sure why...

[oli-obk]

Basically I want to avoid the following (assume it compiles with more boilerplate) from optimizing foo to unsafe fn foo() -> u32 { 42 }

static A: UnsafeCell<u32> = UnsafeCell::new(42);
static B: &'static UnsafeCell<u32> = &A;
unsafe fn foo() -> u32 {
	*B.get()
}

Since we can't ever know what users are up to with their code, the presence of relocations was the easiest way to avoid any kind of troubles there.

Furthermore, assuming we just forbid raw pointers from being derefed in const prop, we'd still have problems with the following getting const propped::

let x = Cell::new(42);
let y = &x;

I want to prevent const prop from creating any kind of mutable global memory that we may end up pointing to. While we can probably devise a smart scheme, right now it seems better to just forbid any relocations.

[RalfJung]

@oli-obk that sounds to me like an indirect way to do the check that we can now do much more reliably and directly:

rust/src/librustc_mir/transform/const_prop.rs

Lines 285 to 287 in cdb50c6

	if allocation.mutability == Mutability::Mut {
	throw_machine_stop_str!("can't eval mutable globals in ConstProp");
	}

This is done for all accesses to globals (things stored in tcx). So if a constant global points to a mutable one, the check will fire when the mutable global actually gets accessed.

[oli-obk]

that's true. Makes more sense to handle the first case like that.

[oli-obk]

But how does that help with the second example I gave?

[RalfJung]

The second example does not involve any global though? So nothing we do in that hook will have any effect? I am confused.^^

[oli-obk]

The problem is that it would magically create a global if we used const prop here. Basically the same problems apply that we have with promotion (except that it's not a problem if we regress optimizations as they aren't "stable")

[RalfJung]

But again, what does the before_access_global hook have to do with that? It won't prevent creation of that global, just access to it.

I also cannot imagine how const-prop could do anything in your example... x doesn't have a known value. And in a case like

let x = Cell::new(42);
let y = &x;
let z = x.get();

it would be entirely okay to replace z by 42.

Of course with

let x = Cell::new(42);
let y = &x;
some_function(y);
let z = x.get();

it would not be okay, but then that is exactly the same as using mutable references -- and it has nothing to do with globals. So I fail to see the relevance of any of this for the discussion at hand.

[oli-obk]

I'm not worried about z. I'm worried about const propping into y, so x is eliminated entirely.

let x = Cell::new(42);
let y = &x;
let z = y.get();

optimized to

static X: &'static Cell<i32> = &Cell::new(42);
let y = X;
let z = y.get();

I'm not sure how we're supposed to prevent this without banning relocations in propagated constants.

[RalfJung]

I don't see why we would even try to do that, that seems like a really odd transformation? And certainly, if any code is responsible for getting things like that right, it's the code that constructs new constants, not the code that reads from existing constants?

I'm not sure how we're supposed to prevent this without banning relocations in propagated constants.

You still didn't explain how the before_access_global prevents anything like that. It prevents reading from constants that have relocation, but you keep talking about constructing new constants that have relocations.

My confusion remains: what does anything you say have to do with what the issue is about, namely this check?

[oli-obk]

Ah, that's where the confusion comes from. Let's start with a code example again:

use std::sync::atomic::{AtomicUsize, Ordering};

fn main() {
    static A: AtomicUsize = AtomicUsize::new(42);
    static B: &'static AtomicUsize = &A;
    let x = B;
	A.store(0, Ordering::Relaxed);
    let y = x.load(Ordering::Relaxed);
}

If we const prop the load at some point (or construct your own lockfree datastructure that doesn't use any intrinsics), we have now changed behaviour because we never read the runtime value of A (0) but instead the initializer value (42).

[RalfJung]

(Sorry for the double-post, GH keeps submitting comments for me way before I am done. Probably some silly trigger-happy shortcut.)

If we const prop the load at some point (or construct your own lockfree datastructure that doesn't use any intrinsics), we have now changed behaviour because we never read the runtime value of A (0) but instead the initializer value (42).

Indeed, that would be a problem. And that problem is caught by

rust/src/librustc_mir/transform/const_prop.rs

Lines 285 to 287 in 6c19a10

	if allocation.mutability == Mutability::Mut {
	throw_machine_stop_str!("can't eval mutable globals in ConstProp");
	}

which would reject any read from a mutable global allocation such as A.

Which additional problem necessitates the additional check that follows next?

rust/src/librustc_mir/transform/const_prop.rs

Lines 288 to 290 in 6c19a10

	if static_def_id.is_some() && allocation.relocations().len() > 0 {
	throw_machine_stop_str!("can't eval statics with pointers in ConstProp");
	}

[oli-obk]

Oh. I see now. Yea, we don't need the second check, any later const prop will do the mutability check and bail out, no matter the number and kind of indirections.

We can remove the if static_def_id.is_some() && allocation.relocations().len() > 0 { check

[RalfJung]

That's what I thought, thanks. :)