const limit for CTFE by TheSamsa · Pull Request #67260 · rust-lang/rust

TheSamsa · 2019-12-12T16:35:06Z

I tried to tackle the first steps for this issue.
The active feature flag does link to the issue below, I think this has to change, because there should be a tracking issue?
https://github.com/TheSamsa/rust/blob/1679a7647da0de672bac26b716db82d16f3896a8/src/librustc_feature/active.rs#L530

Also, I only put up the storage of the limit like "recursion_limit" but created a seperate file in the same place. Since I guess the invocation happens seperately.
https://github.com/TheSamsa/rust/blob/const-limit/src/librustc/middle/const_limit.rs

If this does not hold up for the issue and since there is a time pressure, just reject it.

hopefully this does not put more load on you than I expected...

rust-highfive · 2019-12-12T16:35:11Z

r? @zackmdavis

(rust_highfive has picked a reviewer for you, use r? to override)

Centril · 2019-12-12T16:48:47Z

src/librustc/middle/const_limit.rs

I think this would be cleaner as:

let value = krate .attrs .iter() .filter(|attr| attr.check_name(name)) .find(|attr| attr.value_str().as_str().parse().ok()) .unwrap_or(default); limit.set(value);

discussion regarding the possibility to merge const_limit.rs and the existing recursion_limit.rs is below.

Sure; If the identical logic exists in recursion_limit.rs I would rewrite that one as well, and try to unify the parsing.

src/librustc_session/session.rs

Centril · 2019-12-12T16:50:27Z

r? @oli-obk cc @ecstatic-morse @RalfJung

rust-highfive · 2019-12-12T16:52:57Z

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-12-12T16:35:49.4448577Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-12T16:35:49.4635588Z ##[command]git config gc.auto 0
2019-12-12T16:35:49.4704658Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-12T16:35:49.4764391Z ##[command]git config --get-all http.proxy
2019-12-12T16:35:49.4917029Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67260/merge:refs/remotes/pull/67260/merge
---
2019-12-12T16:41:02.2882385Z    Compiling serde_json v1.0.40
2019-12-12T16:41:03.8089940Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-12-12T16:41:13.5348451Z     Finished release [optimized] target(s) in 1m 15s
2019-12-12T16:41:13.5450997Z tidy check
2019-12-12T16:41:14.1398020Z tidy error: /checkout/src/test/ui/consts/const_limit/const_limit_is_feature_gated.rs: missing trailing newline
2019-12-12T16:41:14.2706204Z tidy error: /checkout/src/librustc/middle/const_limit.rs: leading newline
2019-12-12T16:41:15.0916805Z tidy error: Found 1 features without a gate test.
2019-12-12T16:41:15.0918780Z Expected a gate test for the feature 'const_limit'.
2019-12-12T16:41:15.0919339Z Hint: create a failing test file named 'feature-gate-const_limit.rs'
2019-12-12T16:41:15.0920088Z       in the 'ui' test suite, with its failures due to
2019-12-12T16:41:15.0920203Z       missing usage of `#![feature(const_limit)]`.
2019-12-12T16:41:15.0920444Z Hint: If you already have such a test and don't want to rename it,
2019-12-12T16:41:15.0920695Z       you can also add a // gate-test-const_limit line to the test file.
2019-12-12T16:41:16.6984285Z some tidy checks failed
2019-12-12T16:41:16.6988282Z Found 485 error codes
2019-12-12T16:41:16.6988332Z Found 0 error codes with no tests
2019-12-12T16:41:16.6988370Z Done!
2019-12-12T16:41:16.6988370Z Done!
2019-12-12T16:41:16.6988395Z 
2019-12-12T16:41:16.6988418Z 
2019-12-12T16:41:16.6989628Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-12-12T16:41:16.6989752Z 
2019-12-12T16:41:16.6989794Z 
2019-12-12T16:41:16.6989998Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-12-12T16:41:16.6990039Z Build completed unsuccessfully in 0:01:18
2019-12-12T16:41:16.6990039Z Build completed unsuccessfully in 0:01:18
2019-12-12T16:41:16.6990091Z == clock drift check ==
2019-12-12T16:41:16.6990127Z   local time: Thu Dec 12 16:41:15 UTC 2019
2019-12-12T16:41:16.6990165Z   network time: Thu, 12 Dec 2019 16:41:16 GMT
2019-12-12T16:41:16.6990217Z == end clock drift check ==
2019-12-12T16:41:17.4729565Z 
2019-12-12T16:41:17.4830950Z ##[error]Bash exited with code '1'.
2019-12-12T16:41:17.4855810Z ##[section]Starting: Checkout
2019-12-12T16:41:17.4857233Z ==============================================================================
2019-12-12T16:41:17.4857294Z Task         : Get sources
2019-12-12T16:41:17.4857348Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

src/librustc/middle/const_limit.rs

ecstatic-morse · 2019-12-12T19:45:54Z

src/doc/unstable-book/src/language-features/const-limit.md

Bikeshed: I would prefer const_eval_limit. cc @Centril

I was leaning towards const_eval_limit too, since it is more expressive.
But decided against it, since it was a request to keep it more arbitrary

This is easily changeable based on community & language team discussion so go with whatever makes the PR easiest to land for now. ^^

I prefer const_limit since we don't have to care about e.g. "interpretation vs. evaluation vs. reduction vs. ..." and because it's shorter... but this is probably for an RFC to discuss & lay out the pros & cons of various alternatives.

ecstatic-morse · 2019-12-12T19:53:35Z

Good work so far @TheSamsa! In retrospect, I was overly concerned about this landing concurrently with #67216. No need to stress.

The next step would be to use the newly added const_limit field from tcx.sess in place of STEPS_UNTIL_DETECTOR_ENABLED.

rust/src/librustc_mir/const_eval.rs

Lines 30 to 32 in e9469a6

    
           /// Number of steps until the detector even starts doing anything. 
        
           /// Also, a warning is shown to the user when this number is reached. 
        
           const STEPS_UNTIL_DETECTOR_ENABLED: isize = 1_000_000;

Note that STEPS_UNTIL_DETECTOR_ENABLED is an isize, not a usize, so you probably want a saturating cast. Finally, I think you should use 0 as a sentinel value that disables the loop detector. You'll want to add an is_detector_enabled flag to CompileTimeInterpreter, and return early from the before_terminator callback if it is not set.

rust/src/librustc_mir/const_eval.rs

Lines 502 to 524 in e9469a6

    
           fn before_terminator(ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> { 
        
               { 
        
                   let steps = &mut ecx.machine.steps_since_detector_enabled; 
        
                   *steps += 1; 
        
                   if *steps < 0 { 
        
                       return Ok(()); 
        
                   } 
        
                   *steps %= DETECTOR_SNAPSHOT_PERIOD; 
        
                   if *steps != 0 { 
        
                       return Ok(()); 
        
                   } 
        
               } 
        
               let span = ecx.frame().span; 
        
               ecx.machine.loop_detector.observe_and_analyze( 
        
                   *ecx.tcx, 
        
                   span, 
        
                   &ecx.memory, 
        
                   &ecx.stack[..], 
        
               ) 
        
           }

src/test/ui/consts/const_limit/const_limit_is_feature_gated.rs

Centril · 2019-12-12T21:22:40Z

Also a note re. merge commits:

Merge branch 'master' of github.com:rust-lang/rust

We would prefer to not have those in our history, so please try to remove them by squashing & rebasing as appropriate. :)

src/librustc/middle/limits.rs

src/librustc_mir/const_eval.rs

src/test/ui/consts/const_limit/feature-gate-const_limit.rs

src/test/ui/consts/const_limit/const_limit_not_reached.rs

bors · 2019-12-15T04:48:21Z

☔ The latest upstream changes (presumably #67216) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2019-12-21T09:52:21Z

Oh, I didn't see your force push, sorry. Please rebase again.

You also have a not yet resolved review comment (#67260 (comment))

oli-obk · 2019-12-21T09:53:09Z

The reference submodule change was unintended I guess? please remove it

TheSamsa · 2020-03-03T12:56:48Z

I have a weird issue which i don't know how to resolve.
If I execute
x.py test src/test/ui/consts/const_limit
the stderr shows one WARNING

but if I executer
rustc +stage1 src/test/ui/consts/const_limit
it emmits two WARNINGS, like in the pipeline. So it either fails with my tests or the pipeline.

Do someone know how to fix this?

oli-obk · 2020-03-03T16:43:28Z

Stage 1 and stage 2 should not differ. I don't see how this is happening. I'll give it another review tomorrow

TheSamsa · 2020-03-04T15:09:04Z

Stage 1 and stage 2 should not differ. I don't see how this is happening. I'll give it another review tomorrow

I found the issue, on my machine it used "opt-level=2" and in the pipeline "opt-level=0" so I specified the test to use "opt-level=0" as in the pipeline. Now I have the correct stderr outputs even on my machine.

rust-highfive · 2020-03-04T15:32:56Z

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-03-04T15:09:45.7327329Z ========================== Starting Command Output ===========================
2020-03-04T15:09:45.7330326Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/82e0e40f-17dd-4dbf-b84a-711c4c3c9b31.sh
2020-03-04T15:09:45.7330760Z 
2020-03-04T15:09:45.7335654Z ##[section]Finishing: Disable git automatic line ending conversion
2020-03-04T15:09:45.7350207Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/67260/merge to s
2020-03-04T15:09:45.7353468Z Task         : Get sources
2020-03-04T15:09:45.7353774Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-03-04T15:09:45.7354053Z Version      : 1.0.0
2020-03-04T15:09:45.7354261Z Author       : Microsoft
---
2020-03-04T15:09:46.7286628Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-03-04T15:09:46.7290945Z ##[command]git config gc.auto 0
2020-03-04T15:09:46.7293759Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-03-04T15:09:46.7296381Z ##[command]git config --get-all http.proxy
2020-03-04T15:09:46.7301238Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67260/merge:refs/remotes/pull/67260/merge
---
2020-03-04T15:16:00.8172914Z    Compiling serde_json v1.0.40
2020-03-04T15:16:02.3143678Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2020-03-04T15:16:09.9443880Z     Finished release [optimized] target(s) in 1m 12s
2020-03-04T15:16:09.9559589Z tidy check
2020-03-04T15:16:10.1123568Z tidy error: /checkout/src/test/ui/consts/const_limit/const_eval_limit_reached.rs:16: line longer than 100 chars
2020-03-04T15:16:12.0136731Z some tidy checks failed
2020-03-04T15:16:12.0138017Z Found 489 error codes
2020-03-04T15:16:12.0139908Z Found 0 error codes with no tests
2020-03-04T15:16:12.0140534Z Done!
2020-03-04T15:16:12.0140534Z Done!
2020-03-04T15:16:12.0150064Z 
2020-03-04T15:16:12.0150608Z 
2020-03-04T15:16:12.0152957Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2020-03-04T15:16:12.0154804Z 
2020-03-04T15:16:12.0155212Z 
2020-03-04T15:16:12.0156236Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2020-03-04T15:16:12.0157124Z Build completed unsuccessfully in 0:01:20
2020-03-04T15:16:12.0157124Z Build completed unsuccessfully in 0:01:20
2020-03-04T15:16:12.0189173Z == clock drift check ==
2020-03-04T15:16:12.0200355Z   local time: Wed Mar  4 15:16:12 UTC 2020
2020-03-04T15:16:12.3094208Z   network time: Wed, 04 Mar 2020 15:16:12 GMT
2020-03-04T15:16:12.3100644Z == end clock drift check ==
2020-03-04T15:16:13.0427944Z 
2020-03-04T15:16:13.0472385Z ##[error]Bash exited with code '1'.
2020-03-04T15:16:13.0482866Z ##[section]Finishing: Run build
2020-03-04T15:16:13.0524793Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/67260/merge to s
2020-03-04T15:16:13.0529569Z Task         : Get sources
2020-03-04T15:16:13.0529939Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-03-04T15:16:13.0530264Z Version      : 1.0.0
2020-03-04T15:16:13.0530497Z Author       : Microsoft
2020-03-04T15:16:13.0530497Z Author       : Microsoft
2020-03-04T15:16:13.0530876Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-03-04T15:16:13.0531299Z ==============================================================================
2020-03-04T15:16:13.3105054Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-03-04T15:16:13.3159405Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/67260/merge to s
2020-03-04T15:16:13.3230893Z Cleaning up task key
2020-03-04T15:16:13.3232169Z Start cleaning up orphan processes.
2020-03-04T15:16:13.3409138Z Terminate orphan process: pid (3950) (python)
2020-03-04T15:16:13.3521923Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

…, which defaults to 1_000_000

…INGs yet rename feature to const_eval_limit

and renamed 'recursion_limit' in limits.rs to simple 'limit' because it does handle other limits too.

oli-obk · 2020-03-05T11:45:05Z

@bors r+

bors · 2020-03-05T11:45:07Z

📌 Commit 527456e has been approved by oli-obk

bors · 2020-03-05T17:47:03Z

⌛ Testing commit 527456e with merge 96bb8b3...

bors · 2020-03-05T21:38:54Z

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 96bb8b3 to master...

RalfJung · 2020-03-09T12:23:55Z

Now that this landed, is the intention to kill the snapshot / loop detector infrastructure? Is there an issue for that or is there someone working on it?

@oli-obk

…op-detector, r=RalfJung Remove const eval loop detector Now that there is a configurable instruction limit for CTFE (see rust-lang#67260), we can replace the loop detector with something much simpler. See rust-lang#66946 for more discussion about this. Although the instruction limit is nightly-only, the only practical way to reach the default limit uses nightly-only features as well (although CTFE will still execute code using such features inside an array initializer on stable). This will at the very least require a crater run, since it will result in an error wherever the "long running const eval" warning appeared before. We may need to increase the default for `const_eval_limit` to work around this. Resolves rust-lang#54384 cc rust-lang#49980 r? @oli-obk cc @RalfJung

@oli-obk

…op-detector, r=RalfJung Remove const eval loop detector Now that there is a configurable instruction limit for CTFE (see rust-lang#67260), we can replace the loop detector with something much simpler. See rust-lang#66946 for more discussion about this. Although the instruction limit is nightly-only, the only practical way to reach the default limit uses nightly-only features as well (although CTFE will still execute code using such features inside an array initializer on stable). This will at the very least require a crater run, since it will result in an error wherever the "long running const eval" warning appeared before. We may need to increase the default for `const_eval_limit` to work around this. Resolves rust-lang#54384 cc rust-lang#49980 r? @oli-obk cc @RalfJung

rust-highfive assigned zackmdavis Dec 12, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 12, 2019

Centril reviewed Dec 12, 2019

View reviewed changes

src/librustc_session/session.rs Outdated Show resolved Hide resolved

rust-highfive assigned oli-obk and unassigned zackmdavis Dec 12, 2019

ecstatic-morse reviewed Dec 12, 2019

View reviewed changes

src/librustc/middle/const_limit.rs Outdated Show resolved Hide resolved

ecstatic-morse reviewed Dec 12, 2019

View reviewed changes

src/test/ui/consts/const_limit/const_limit_is_feature_gated.rs Outdated Show resolved Hide resolved

TheSamsa force-pushed the const-limit branch from 93a97f1 to 73425ca Compare December 12, 2019 22:23

Centril reviewed Dec 12, 2019

View reviewed changes

src/librustc/middle/limits.rs Outdated Show resolved Hide resolved

oli-obk requested changes Dec 13, 2019

View reviewed changes

src/librustc/middle/limits.rs Outdated Show resolved Hide resolved

src/librustc_mir/const_eval.rs Outdated Show resolved Hide resolved

src/test/ui/consts/const_limit/feature-gate-const_limit.rs Outdated Show resolved Hide resolved

TheSamsa force-pushed the const-limit branch 2 times, most recently from 3f4bc34 to ccbe418 Compare December 14, 2019 10:43

oli-obk reviewed Dec 14, 2019

View reviewed changes

src/test/ui/consts/const_limit/const_limit_not_reached.rs Outdated Show resolved Hide resolved

oli-obk reviewed Dec 14, 2019

View reviewed changes

src/test/ui/consts/const_limit/const_limit_not_reached.rs Outdated Show resolved Hide resolved

TheSamsa force-pushed the const-limit branch 2 times, most recently from 0605ff2 to 7f00a07 Compare December 16, 2019 16:39

oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2019

oli-obk mentioned this pull request Dec 21, 2019

Use a DFS to compare CTFE snapshots #66946

Closed

TheSamsa force-pushed the const-limit branch from 7f00a07 to d654c22 Compare December 26, 2019 10:10

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 2, 2020

TheSamsa force-pushed the const-limit branch 2 times, most recently from 3492b49 to ad670e1 Compare March 4, 2020 15:07

TheSamsa added 5 commits March 5, 2020 08:09

Prepare const_limit feature gate and attribute

337af5e

Disable CTFE if const_limit was set to 0, otherwise use the value set…

ff38bab

…, which defaults to 1_000_000

Add a new test to reach const_limit setting, although with wrong WARN…

288e142

…INGs yet rename feature to const_eval_limit

Opt out of CTFE if the 'const_eval_limit' is set to 0

c94c74e

Bumped version number for const_eval_limit in active.rs

527456e

and renamed 'recursion_limit' in limits.rs to simple 'limit' because it does handle other limits too.

TheSamsa force-pushed the const-limit branch from ad670e1 to 527456e Compare March 5, 2020 07:10

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 5, 2020

bors merged commit 96bb8b3 into rust-lang:master Mar 5, 2020

bors mentioned this pull request Mar 5, 2020

Enable AVR as a Tier 3 target upstream #69478

Merged

RalfJung mentioned this pull request Mar 12, 2020

remove CTFE loop detector #54384

Closed

ecstatic-morse mentioned this pull request Mar 17, 2020

Remove const eval loop detector #70087

Merged

Uh oh!

Conversation

TheSamsa commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Dec 12, 2019

Uh oh!

Centril Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

TheSamsa Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

Centril Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Centril commented Dec 12, 2019

Uh oh!

rust-highfive commented Dec 12, 2019

Uh oh!

Uh oh!

ecstatic-morse Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

TheSamsa Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

Centril Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

ecstatic-morse commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Centril commented Dec 12, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented Dec 15, 2019

Uh oh!

oli-obk commented Dec 21, 2019

Uh oh!

oli-obk commented Dec 21, 2019

Uh oh!

TheSamsa commented Mar 3, 2020

Uh oh!

oli-obk commented Mar 3, 2020

Uh oh!

TheSamsa commented Mar 4, 2020

Uh oh!

rust-highfive commented Mar 4, 2020

Uh oh!

oli-obk commented Mar 5, 2020

Uh oh!

bors commented Mar 5, 2020

Uh oh!

bors commented Mar 5, 2020

Uh oh!

bors commented Mar 5, 2020

Uh oh!

RalfJung commented Mar 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

TheSamsa commented Dec 12, 2019 •

edited

Loading

ecstatic-morse commented Dec 12, 2019 •

edited

Loading