Segfault or wrong values from const FOO: u32 = 0 - 1; println!("{}", FOO);

[ollie27]

The following programs produce a binary that will either segfault or produce a nonsense value seemingly depending on whether optimizations are enabled or not.

fn main() {
    const FOO: u32 = 0 - 1;
    println!("{}", FOO);
}

fn main() {
    println!("{}", 0u32 - 1);
}

On 1.25.0 they behave as expected (the first doesn't even compile) but 1.26.0 and newer are broken.

[varkor]

The constant evaluation error is being flagged, but only as a warning, so the code is still executed.

cc @oli-obk

[oli-obk]

@eddyb the way promotion works we can't back out of promoting in case of const eval failure, can we?

The named const case can be fixed. But I'm not sure what do do on the promotion case

[eddyb]

@oli-obk Ah, this is where we disagree - overflow is not an evaluation error.
That is, because this is compiled in release mode, there's no Assert MIR terminator, which means that wrapping should happen.

But there is a more general issue, where if a const fn is called, and evaluation fails in it (for whatever reason, e.g. wrong unsafe code), a promoted call to it doesn't inject a runtime panic.
So I think we have to add that panic to the runtime codegen, and keep compile-time warnings.

[oli-obk]

Ok so this is super easy to fix. Just remove the check in https://github.com/rust-lang/rust/blob/master/src/librustc_mir/interpret/eval_context.rs#L533

The const fn part is "fine" right now, because users can't write any const fns and the const fns that libstd offers are safe to use.

[eddyb]

@oli-obk I would not rely on assumptions about libstd, IMO, it's too fragile. We have code to emit an Assert failure, just need to reuse it for errors while evaluating a promoted.

[oli-obk]

Ok. some irc discussion has shown that this is a horrible situation we're in. Summary:

Let's use a strawman union for demonstration purposes, but it's not necessary, there are other ways to fail the const evaluator, they just aren't short examples.

/// Helper for casting `&'static u32` into `usize`
union Union {
    a: &'static u32,
    b: usize,
}

1. unsafe { Union { a: &1 }.b == Union { a: &2 }.b } is perfectly sane at runtime (but not doable at compile-time right now. This is just a placeholder example. Take any piece of code that works at runtime but should be const evaluable but isn't at the moment.
   • This means that perfectly safe and working code that in the future could become promoted, would, after promotion, not be evaluable by miri. We can't report an error, because that would be a breaking change, we also can't turn this into a runtime panic, because that's also a breaking change. So we'd need to keep doing the original runtime code before promotion. We can't do that, because we need the result to be in static memory because promotion already promised the 'static lifetime.
2. We can't make promotion depend on the const evaluation succeeding, because typeck needs to know whether promotion will actually succeed. At typeck time we don't know the generic arguments to non-monomorphized function.
   • We could do a breaking change and make promoteds like regular statics, so you can't depend on the surrounding function's generics. That would allow typeck to simply evaluate the promoted during the creation of the promoted and skip promoting it in the case of errors. I hate suggesting this, but it seems like the sanest option.
3. rustc 1.25 just hard errored at monomorphization time. We can reintroduce that error, but it'll mean we can never improve promotion to accept more code.

[oli-obk]

Here's a problematic case with depending on a generic function's args:

trait C {
    const BOO: usize;
}

trait Foo<T> {
    const BAR: usize;
}

struct A<T>(T);

impl<T: C> Foo<T> for A<T> {
    const BAR: usize = [5, 6, 7][T::BOO];
}

fn foo<T: C>() -> &'static usize {
    &<A<T> as Foo<T>>::BAR
}

impl C for () {
    const BOO: usize = 42;
}

impl C for u32 {
    const BOO: usize = 1;
}

fn main() {
    println!("{:x}", foo::<()>() as *const usize as usize);
    println!("{:x}", foo::<u32>() as *const usize as usize);
    println!("{:x}", foo::<()>());
    println!("{:x}", foo::<u32>());
}

You cannot implement this with static, just with promotion

[petertodd]

Looks like I ran into a variant of this as well:

trait Unsigned {
    const MAX: u8;
}

struct U8(u8);
impl Unsigned for U8 {
    const MAX: u8 = 0xff;
}

struct Sum<A,B>(A,B);

impl<A: Unsigned, B: Unsigned> Unsigned for Sum<A,B> {
    const MAX: u8 = A::MAX + B::MAX;
}

fn foo<T>(_: T) -> &'static u8 {
    &Sum::<U8,U8>::MAX
}

fn main() {
    foo(0);
}

Unlike the OP's examples, this still fails on rustc 1.28.0-nightly (cb20f68 2018-05-21), and doesn't produce any compile time warnings or errors (it also fails on 1.26.0).

[nikomatsakis]

For future reference: we're discussing this issue on Discord with the conversation GUID: 5fd923d1-f8ea-4a3a-a85b-67dbe3557544 (you can search for it =)

[nikomatsakis]

Notes from that discussion are available in this gist

https://gist.github.com/nikomatsakis/eee212d7c4a49ae75e19765c1e06bf49

[hanna-kruppe]

What would such "strict limitations" on const fns in promotion look like?

Also: Could we simply not allow any const fn's in promoted expressions? This would mean some loss of expressivity in promotion, but seems like a simpler mental model.

[RalfJung]

I'm still trying to understand what the problem is here, so let me try to rephrase and please tell me if that makes any sense:

What happens with promotion is that the compiler identifies some expression to be a promotion candidate. Such expressions than are evaluated at compile time using miri's CTFE. If that succeeds, all is well and the &... is turned into the address of the new static. However, if CTFE fails then we have to either (a) abort compilation, or (b) compile the &... into an abort/panic. Currently, something goes awfully wrong and the &... turns into "undef", which is just bad and leads to the SEGFAULT here.

Either way, there is the problem that the check if something should be promoted has to be very conservative because if it claims something is promotable but, but then that code errors at CTFE time, we broke compilation for no good reason (as this code would have compiled just fine if we didn't decide to promote).

Does that summarize the problem?

---

I see two separate things to do/discuss here, though they are obviously related. One is whether we pick (a) or (b). (The current situation of emitting "undef" is clearly a bug, right?) I don't see any good reason to to (b) -- silently (or with a warning?) emit code that will definitely panic at run-time seems just bad. Also, @oli-obk mentioned that it's not even easy to do because panic is a block terminator and the place where this code is emitted does not expect a block terminator. So, I think we should do (a).

The notes @nikomatsakis linked above mention (a), but only as an alternative to "forbid unions in promotion", which I don't understand how that's an alternative. We still have to do something if miri errors, right?

Secondly, how can we avoid the problem of trying to promote something that's not promoteable? This ties into the discussions around CTFE soundess/correctness at #24111 (comment). @oli-obk was convinced that we should have a "const mode typesystem" that makes things like raw pointer comparisons or ptr-to-int-casts unsafe. Based on that, shouldn't it be enough to check that the thing to be promoted is entirely "const well-typed" ignoring unsafe blocks, i.e., everything in there would have to be called in safe const code. That would rule out union field access.

There is still some possible issue if a const fn claims to be safe but actually does things that make miri error. However, I think that's okay: Such a const fn is wrong, it's as wrong as a safe fn that triggers UB for a valid input! If someone writes const fn, and they use unsafe code in there, ten they are fully responsible for making sure this code will not ever fail in CTFE. If they don't follow this rule, they screw up their user -- such is the fate of unsafe code. I think it's fine to also do that in this case.#24111 (comment)

[eddyb]

The current situation of emitting "undef" is clearly a bug, right?

It was intended for errors from things like out-of-bounds indexing and division by zero, where the original Assert terminator stays in the runtime code and gets tripped before the undef is observed. In retrospective, what I should've done is replace the undef with a llvm.trap call.

[oli-obk]

I have addressed the union issue in #51328

[oli-obk]

There is a full fix in #51570, but it's overzealous and will error on completely fine code. The correct solution is outlined in #51570 (comment) but that requires some refactorings that @TheDarkula is currently working on. After their refactorings are done I'll update the PR to implement the fix properly.

[pnkfelix]

visiting for triage.

@oli-obk had a fix in PR #51570, but @RalfJung found flaws in the checks implemented there. So there is further work that needs to be done to actually define the correct checking that we want to do.

[RalfJung]

Thanks to #52571 (which got backported to beta and hence will be in the next stable), at least similar bugs can no longer create UB. Instead, the program will abort. Not good but better.^^

That means this is no longer I-unsound.

[pnkfelix]

closing at the actual soundness holes have been plugged (by codegening abort code instead, as @RalfJung described.)

A new issue will hopefully be opened somewhere soon to track the follow-on work to do better static checking in such cases rather than falling back on dynamic aborts.

[oli-obk]

I did a (short) writeup with a possible solution in rust-lang/const-eval#2