Stream: t-compiler/const-eval

Topic: perf stress test broken


simulacrum (Mar 24 2020 at 18:15, on Zulip):
error[E0080]: could not evaluate static initializer
  --> src/lib.rs:53:35
   |
25 |         e(0); e(0); e(0); e(0);
   |                     ---- inside call to `CONST_FN::e::e::e::e` at src/lib.rs:25:21
26 |         e(0); e(0); e(0); e(0)
   |               ----  ---- inside call to `CONST_FN::e::e::e` at src/lib.rs:26:21
   |               |
   |               inside call to `CONST_FN::e::e` at src/lib.rs:26:15
...
32 |         e(0); e(0); e(0); e(0)
   |         ---- inside call to `CONST_FN::e` at src/lib.rs:32:9
...
53 | const fn inc(i: i32) -> i32 { i + 1 }
   |                                   ^ exceeded interpreter step limit (see `#[const_eval_limit]`)
...
58 | expensive_static!(CONST_FN: i32 = inc(42); [8 16 16 16 16]);
   |                                   ------- inside call to `inc` at src/lib.rs:58:35

on the perf ctfe-stress-4 benchmark

simulacrum (Mar 24 2020 at 18:15, on Zulip):

Should we bump up the step limit? cc @oli @Wesley Wiser (?)

Wesley Wiser (Mar 24 2020 at 18:16, on Zulip):

Did this go over the limit recently?

simulacrum (Mar 24 2020 at 18:17, on Zulip):

uh, yeah, in the past day or two

simulacrum (Mar 24 2020 at 18:17, on Zulip):

I can get exact commit in a moment

Wesley Wiser (Mar 24 2020 at 18:17, on Zulip):

Was it https://github.com/rust-lang/rust/pull/70087?

simulacrum (Mar 24 2020 at 18:18, on Zulip):

broke in https://github.com/rust-lang/rust/commit/342c5f33d097b2dc07a2dbc0ca45a37379d2ff60

simulacrum (Mar 24 2020 at 18:18, on Zulip):

probably that PR?

Wesley Wiser (Mar 24 2020 at 18:18, on Zulip):

Yeah looks that way

Wesley Wiser (Mar 24 2020 at 18:18, on Zulip):

cc @ecstatic-morse

Wesley Wiser (Mar 24 2020 at 18:19, on Zulip):

To answer your question, probably yeah. If ctfe-stress-4 is breaking, there's probably other crazy CTFE code out there that's going to break too

simulacrum (Mar 24 2020 at 18:20, on Zulip):

oh, I meant that as in "bump the step limit in perf"

simulacrum (Mar 24 2020 at 18:20, on Zulip):

but bumping it globally seems good

Wesley Wiser (Mar 24 2020 at 18:20, on Zulip):

Ah

simulacrum (Mar 24 2020 at 18:21, on Zulip):

(can also do both, fwiw)

Wesley Wiser (Mar 24 2020 at 18:21, on Zulip):

I didn't realize we had that option.

Wesley Wiser (Mar 24 2020 at 18:21, on Zulip):

Bumping it for ctfe-stress-4 locally seems like a good first step regardless

simulacrum (Mar 24 2020 at 18:21, on Zulip):

sure, we can just edit the perf benchmark to include the attribute

simulacrum (Mar 24 2020 at 18:21, on Zulip):

suprisingly the error message does not suggest a limit

simulacrum (Mar 24 2020 at 18:25, on Zulip):

hm, so the limit is unstable

simulacrum (Mar 24 2020 at 18:25, on Zulip):

that seems bad

simulacrum (Mar 24 2020 at 18:25, on Zulip):
error[E0658]: the `#[const_eval_limit]` attribute is an experimental feature
 --> src/lib.rs:2:1
  |
2 | #![const_eval_limit = "1000000"]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: see issue #67217 <https://github.com/rust-lang/rust/issues/67217> for more information
  = help: add `#![feature(const_eval_limit)]` to the crate attributes to enable
simulacrum (Mar 24 2020 at 18:26, on Zulip):

then we should definitely bump the default up because that suggests that previously working code on stable could regress?

Wesley Wiser (Mar 24 2020 at 18:26, on Zulip):

Ah yeah

Wesley Wiser (Mar 24 2020 at 18:26, on Zulip):

I think we should get ctfe-stress-4 running again asap so adding the feature attribute seems fine to me

simulacrum (Mar 24 2020 at 18:26, on Zulip):

yeah pushing a fix now

Wesley Wiser (Mar 24 2020 at 18:27, on Zulip):

But we should think about the fallout of this more

simulacrum (Mar 24 2020 at 18:27, on Zulip):

could you open up an issue or comment somewhere?

Wesley Wiser (Mar 24 2020 at 18:28, on Zulip):

Would re-opening https://github.com/rust-lang/rust/issues/54384 be appropriate or should I create a new issue?

simulacrum (Mar 24 2020 at 18:44, on Zulip):

I would open a new issue

ecstatic-morse (Mar 24 2020 at 19:15, on Zulip):

I addressed backwards compatibility issues in the summary of #70087, which also had a crater run with 0 regressions. The fact that ctfe-stress-4 broke doesn't change my thinking here, since I believe that the likelihood of real world code going over the limit without loops or conditional recursion (both of which require nightly features) is basically zero.

RalfJ (Mar 25 2020 at 15:32, on Zulip):

so the default limit is 1_000_000 (10^6) and this stress test required 10_000_000 (10^7)? That seems reasonable, the test is extremely silly. I wouldn't increase the default limit for that, I'd say this shows the limit was picked quite reasonably.^^

simulacrum (Mar 25 2020 at 15:38, on Zulip):

the risk of increasing the limit is just that we run for longer before emitting a warning though right?

RalfJ (Mar 25 2020 at 15:38, on Zulip):

I dont think there's a warning any more? we just hard error and abort evaluation

RalfJ (Mar 25 2020 at 15:38, on Zulip):

similar to the recursion_limit rustc uses elsewhere

simulacrum (Mar 25 2020 at 15:38, on Zulip):

er, yeah, I meant an error

simulacrum (Mar 25 2020 at 15:38, on Zulip):

so realistically it feels like we should set it such that no program we know of encounters it

RalfJ (Mar 25 2020 at 15:39, on Zulip):

I'd qualify that with "no reasonable program", or at least "no realistic program"

simulacrum (Mar 25 2020 at 15:39, on Zulip):

maybe that's not the right policy though, not sure

simulacrum (Mar 25 2020 at 15:39, on Zulip):

hm, okay. given the zero regression crater run we're probably fine

RalfJ (Mar 25 2020 at 15:40, on Zulip):

if we set it too high, people will see the compiler hang and not know what's going on... not sure how the team decided about the recursion_limit default value, or about requests to increase it

RalfJ (Mar 25 2020 at 15:40, on Zulip):

I've certainly seen that value increased in the wild

simulacrum (Mar 25 2020 at 15:40, on Zulip):

yeah, me too

simulacrum (Mar 25 2020 at 15:40, on Zulip):

ideally we'd be able to "know" apriori that the limit will be reached, but that's obviously not really possible

RalfJ (Mar 25 2020 at 15:40, on Zulip):

;)

simulacrum (Mar 25 2020 at 15:41, on Zulip):

anyway I'm happy with leaving it as is I guess

RalfJ (Mar 25 2020 at 15:41, on Zulip):

kk. and thanks for bringing this up, it's a good data point.

RalfJ (Mar 25 2020 at 15:41, on Zulip):

we'll only really get user data "in the wild" once loops are allowed in stable CTFE, I think

simulacrum (Mar 25 2020 at 15:43, on Zulip):

makes sense

simulacrum (Mar 25 2020 at 15:44, on Zulip):

I wonder if we could borrow from what bpf(?) in the kernel did for those loops etc -- IIRC, they managed to ensure that things terminate

simulacrum (Mar 25 2020 at 15:45, on Zulip):

otoh, even if you will eventually terminate but will run for years we probably want to error out

eddyb (Mar 25 2020 at 15:46, on Zulip):

(e)BPF is... really weird :P

eddyb (Mar 25 2020 at 15:46, on Zulip):

and I don't think it's anything we can replicate anyway

simulacrum (Mar 25 2020 at 15:46, on Zulip):

yeah I don't know the details. wouldn't be surprised

eddyb (Mar 25 2020 at 15:47, on Zulip):

also I think it has arbitrary limits too? so it's not like a principled solution or anything

eddyb (Mar 25 2020 at 15:49, on Zulip):

(dependently typed aka "proof assistant" languages are the main example I know of that does enforce termination, but it's nothing we can do for miri, as the problem is the turing-complete unsafe-internals Rust code and we can't realistically enforce it will terminate)

eddyb (Mar 25 2020 at 15:50, on Zulip):

the way you make an interpreter for some TC imperative thing in one of those languages is with "fuel", i.e. an instruction limit of sorts

eddyb (Mar 25 2020 at 15:51, on Zulip):

it works because natural numbers are inductive and you can think of it like uhhh 5000 is a container that has 4999 inside which has 4998 all the way down to 2 containing 1 containing 0 and then it bottoms out

eddyb (Mar 25 2020 at 15:52, on Zulip):

so if each step is guaranteed to terminate then doing n: Nat steps is like a list fold, it's n that determines the shape of your execution/termination, not the interpreted code

RalfJ (Mar 26 2020 at 08:34, on Zulip):

eddyb said:

(dependently typed aka "proof assistant" languages are the main example I know of that does enforce termination, but it's nothing we can do for miri, as the problem is the turing-complete unsafe-internals Rust code and we can't realistically enforce it will terminate)

it's also not really better than an arbitrary limit, in practice. I can easily write a Coq program that terminates, but not in your lifetime.

Last update: Apr 03 2020 at 17:45UTC