Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

const limit for CTFE #67260

Merged
merged 5 commits into from Mar 5, 2020
Merged

const limit for CTFE #67260

merged 5 commits into from Mar 5, 2020

Conversation

@TheSamsa
Copy link
Contributor

TheSamsa commented Dec 12, 2019

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 12, 2019

r? @zackmdavis

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

}

fn update_limit(krate: &ast::Crate, limit: &Once<usize>, name: Symbol, default: usize) {
for attr in &krate.attrs {

This comment has been minimized.

Copy link
@Centril

Centril Dec 12, 2019

Member

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);

This comment has been minimized.

Copy link
@TheSamsa

TheSamsa Dec 12, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@Centril

Centril Dec 12, 2019

Member

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

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 12, 2019

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 12, 2019

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)

@@ -0,0 +1,7 @@
# `const_limit`

This comment has been minimized.

Copy link
@ecstatic-morse

ecstatic-morse Dec 12, 2019

Contributor

Bikeshed: I would prefer const_eval_limit. cc @Centril

This comment has been minimized.

Copy link
@TheSamsa

TheSamsa Dec 12, 2019

Author Contributor

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 comment has been minimized.

Copy link
@Centril

Centril Dec 12, 2019

Member

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

This comment has been minimized.

Copy link
Contributor

ecstatic-morse commented Dec 12, 2019

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.

/// 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.

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[..],
)
}

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 12, 2019

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. :)

@TheSamsa TheSamsa force-pushed the TheSamsa:const-limit branch from 93a97f1 to 73425ca Dec 12, 2019
src/librustc/middle/limits.rs Outdated Show resolved Hide resolved
src/librustc/middle/limits.rs Outdated Show resolved Hide resolved
src/librustc_mir/const_eval.rs Outdated Show resolved Hide resolved
@TheSamsa TheSamsa force-pushed the TheSamsa:const-limit branch 2 times, most recently from 3f4bc34 to ccbe418 Dec 13, 2019
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 15, 2019

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

@TheSamsa TheSamsa force-pushed the TheSamsa:const-limit branch 2 times, most recently from 0605ff2 to 7f00a07 Dec 16, 2019
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 21, 2019

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

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 21, 2019

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

@TheSamsa

This comment has been minimized.

Copy link
Contributor Author

TheSamsa commented Mar 3, 2020

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

This comment has been minimized.

Copy link
Contributor

oli-obk commented Mar 3, 2020

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

@TheSamsa TheSamsa force-pushed the TheSamsa:const-limit branch 2 times, most recently from 3492b49 to ad670e1 Mar 4, 2020
@TheSamsa

This comment has been minimized.

Copy link
Contributor Author

TheSamsa commented Mar 4, 2020

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 4, 2020

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)

TheSamsa added 5 commits Dec 12, 2019
@TheSamsa TheSamsa force-pushed the TheSamsa:const-limit branch from ad670e1 to 527456e Mar 5, 2020
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Mar 5, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 5, 2020

📌 Commit 527456e has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 5, 2020

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

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 5, 2020

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

@bors bors added the merged-by-bors label Mar 5, 2020
@bors bors merged commit 96bb8b3 into rust-lang:master Mar 5, 2020
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20200305.10 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Mar 9, 2020

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?

bors added a commit that referenced this pull request Mar 18, 2020
…, r=<try>

Remove const eval loop detector

Now that there is a configurable instruction limit for CTFE (see #67260), we can replace the loop detector with something much simpler. See #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 #54384 cc #49980
r? @oli-obk cc @RalfJung
Centril added a commit to Centril/rust that referenced this pull request Mar 23, 2020
…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
Centril added a commit to Centril/rust that referenced this pull request Mar 23, 2020
…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
Centril added a commit to Centril/rust that referenced this pull request Mar 23, 2020
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.