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

Print a backtrace in const eval if interrupted #111769

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented May 19, 2023

Demo:

#![feature(const_eval_limit)]
#![const_eval_limit = "0"]

const OW: u64 = {
    let mut res: u64 = 0;
    let mut i = 0;
    while i < u64::MAX {
        res = res.wrapping_add(i);
        i += 1;
    }
    res
};

fn main() {
    println!("{}", OW);
}
╭ ➜ ben@archlinux:~/rust
╰ ➤ rustc +stage1 spin.rs 
^Cerror[E0080]: evaluation of constant value failed
 --> spin.rs:8:33
  |
8 |         res = res.wrapping_add(i);
  |                                 ^ Compilation was interrupted

note: erroneous constant used
  --> spin.rs:15:20
   |
15 |     println!("{}", OW);
   |                    ^^

note: erroneous constant used
  --> spin.rs:15:20
   |
15 |     println!("{}", OW);
   |                    ^^
   |
   = note: this note originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0080`.

@rustbot
Copy link
Collaborator

rustbot commented May 19, 2023

r? @Mark-Simulacrum

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 19, 2023
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented May 22, 2023

Yea, this is exactly what I wanted. Do any of the new deps have problematic licenses? Otherwise seems fine to just add them to the list of allowed crates.

@saethlin
Copy link
Member Author

ctrlc is MIT or Apache-2.0, nix is MIT. I'll try to spruce up the implementation then get this reviewed.

Jynn said Mark has previously been uncomfortable with signal handlers or atexit code, so I'd prefer Mark approve this before it goes in (got lucky with the random selection I suppose).

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems broadly fine to me, my main concern here is that we don't write custom code where we need to be worried about signal safety etc to a great degree. But it seems like that's not being done here, which is good.

compiler/rustc_const_eval/src/const_eval/eval_queries.rs Outdated Show resolved Hide resolved
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 22, 2023
@saethlin saethlin marked this pull request as ready for review May 22, 2023 20:37
@rustbot
Copy link
Collaborator

rustbot commented May 22, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@saethlin
Copy link
Member Author

saethlin commented May 22, 2023

Just so everyone follows along, we recently added a mechanism like this to Miri: rust-lang/miri#2899

Since we ctrlc::set_hook inside rustc_driver::main, Miri should probably keep installing its own hook, but it might be nice to use the static inside rustc_*. cc @RalfJung (ah duh the bot has pinged you already, well now you know I really care about you knowing :p )

@oli-obk
Copy link
Contributor

oli-obk commented May 23, 2023

Miri should probably keep installing its own hook, but it might be nice to use the static inside rustc_*.

You could do this change in this PR, subtrees ftw

@RalfJung
Copy link
Member

You could do this change in this PR, subtrees ftw

We should first push Miri changes to rustc though or else we'll get conflicts.

@bors
Copy link
Contributor

bors commented May 24, 2023

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

@rustbot
Copy link
Collaborator

rustbot commented May 24, 2023

The Miri subtree was changed

cc @rust-lang/miri

@RalfJung
Copy link
Member

r=me on the interpreter changes. Do we still need to get some approval for the new rustc dependencies?

@bors
Copy link
Contributor

bors commented May 25, 2023

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

@saethlin
Copy link
Member Author

I'm happy to wait for @Mark-Simulacrum to swing by again and offer an opinion :)

@Mark-Simulacrum
Copy link
Member

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented May 29, 2023

📌 Commit 38a5fee has been approved by RalfJung

It is now in the queue for this repository.

@RalfJung
Copy link
Member

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 10, 2024
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with some last nits, but we'll have to figure out this approval for the new dependency

compiler/rustc_driver_impl/src/lib.rs Outdated Show resolved Hide resolved
src/tools/miri/src/bin/miri.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/deps.rs Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Mar 11, 2024 via email

@bors
Copy link
Contributor

bors commented Mar 17, 2024

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

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2024

Some changes occurred in run-make tests.

cc @jieyouxu

@saethlin saethlin mentioned this pull request Mar 18, 2024
@saethlin saethlin added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 25, 2024
@saethlin
Copy link
Member Author

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Mar 25, 2024

📌 Commit 9e0d1a3 has been approved by RalfJung

It is now in the queue for this repository.

@bors 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 25, 2024
@bors
Copy link
Contributor

bors commented Mar 26, 2024

⌛ Testing commit 9e0d1a3 with merge c98ea0d...

@bors
Copy link
Contributor

bors commented Mar 26, 2024

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing c98ea0d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 26, 2024
@bors bors merged commit c98ea0d into rust-lang:master Mar 26, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 26, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c98ea0d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.3% [3.4%, 5.1%] 2
Improvements ✅
(primary)
-1.6% [-3.2%, -0.9%] 12
Improvements ✅
(secondary)
-2.3% [-3.1%, -1.2%] 16
All ❌✅ (primary) -1.6% [-3.2%, -0.9%] 12

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.7% [-1.5%, -0.4%] 6
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.7% [-1.5%, -0.4%] 6

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.315s -> 675.795s (0.67%)
Artifact size: 315.65 MiB -> 315.67 MiB (0.00%)

@saethlin saethlin deleted the ctfe-backtrace-ctrlc branch March 26, 2024 04:04
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  PR_MESSAGE: Automation to keep dependencies in `Cargo.lock` current.
following is the output from `cargo update`:
  COMMIT_MESSAGE: cargo update 
##[endgroup]
Starting download for Cargo-lock
##[error]Unable to find any artifacts for the associated workflow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.