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

Add a couple of missing ensure_sufficient_stacks #136352

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

lqd
Copy link
Member

@lqd lqd commented Jan 31, 2025

r? @saethlin I hope you didn't spend time on this already.

(I couldn't sleep, opened check_tail_calls, there was a single call where it could happen, might as well fix it)

This PR adds a couple of missing ensure_sufficient_stacks:

  • one in check_tail_calls that prevented the Temporarily bring back Rvalue::Len #135709 backport on some targets.
  • after that was fixed, the test still didn't pass starting at 4MB, so I also added one in check_unsafety and that made it pass.

I didn't add an rmake test purposefully limiting the min stack size on issue-74564-if-expr-stack-overflow.rs, but we could if we wanted to.

On apple-aarch64-darwin, this is enough to make RUST_MIN_STACK=$((1024*1024*3)) ./x test tests/ui --test-args tests/ui/issues/issue-74564-if-expr-stack-overflow.rs pass for me locally, and it does stack overflow otherwise.

@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 Jan 31, 2025
@compiler-errors
Copy link
Member

r? compiler-errors @bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jan 31, 2025

📌 Commit 94562ee has been approved by compiler-errors

It is now in the queue for this repository.

@rustbot rustbot assigned compiler-errors and unassigned saethlin Jan 31, 2025
@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 Jan 31, 2025
@lqd
Copy link
Member Author

lqd commented Jan 31, 2025

Let’s also cc @cuviper for awareness, and we can nominate and discuss next week.

@rustbot label +beta-nominated

@rustbot rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 31, 2025
@cuviper
Copy link
Member

cuviper commented Jan 31, 2025

Thanks! I've confirmed on aarch64-linux that beta-backporting this with #135709 does pass the test was overflowing.

@jieyouxu
Copy link
Member

jieyouxu commented Feb 3, 2025

@bors p=10 (rollup-scheduling, this unblocks a beta backport)

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 3, 2025
Add a couple of missing `ensure_sufficient_stacks`

r? `@saethlin` I hope you didn't spend time on this already.

(I couldn't sleep, opened `check_tail_calls`, there was a single call where it could happen, might as well fix it)

This PR adds a couple of missing `ensure_sufficient_stack`s:
- one in `check_tail_calls` that prevented the rust-lang#135709 backport on some targets.
- after that was fixed, the test still didn't pass starting at 4MB, so I also added one in `check_unsafety` and that made it pass.

I didn't add an `rmake` test purposefully limiting the min stack size on `issue-74564-if-expr-stack-overflow.rs`, but we could if we wanted to.

On `apple-aarch64-darwin`, this is enough to make `RUST_MIN_STACK=$((1024*1024*3)) ./x test tests/ui --test-args tests/ui/issues/issue-74564-if-expr-stack-overflow.rs` pass for me locally, and it does stack overflow otherwise.
@bors
Copy link
Contributor

bors commented Feb 3, 2025

⌛ Testing commit 94562ee with merge b4b70d7...

@bors
Copy link
Contributor

bors commented Feb 3, 2025

💔 Test failed - checks-actions

@bors 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 Feb 3, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Feb 3, 2025

   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
 96 5495k   96 5285k    0     0  35.5M      0 --:--:-- --:--:-- --:--:-- 36.0M
curl: (56) OpenSSL SSL_read: SSL_ERROR_SYSCALL, errno 0
Error: Process completed with exit code 56.

@bors retry (network failure?)

@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 Feb 3, 2025
@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)

@jieyouxu jieyouxu closed this Feb 3, 2025
@jieyouxu jieyouxu reopened this Feb 3, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Feb 3, 2025

@bors retry (maybe network failure?)

@bors
Copy link
Contributor

bors commented Feb 3, 2025

⌛ Testing commit 94562ee with merge f2c4ccd...

@bors
Copy link
Contributor

bors commented Feb 3, 2025

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing f2c4ccd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 3, 2025
@bors bors merged commit f2c4ccd into rust-lang:master Feb 3, 2025
13 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 3, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f2c4ccd): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

  • If the regression was expected or you think it can be justified,
    please write a comment with sufficient written justification, and add
    @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
  • If you think that you know of a way to resolve the regression, try to create
    a new PR with a fix for the regression.
  • If you do not understand the regression or you think that it is just noise,
    you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
    were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.2%, 0.3%] 12
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 4.3%, secondary 3.9%)

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)
4.3% [4.3%, 4.3%] 1
Regressions ❌
(secondary)
3.9% [3.7%, 4.1%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 4.3% [4.3%, 4.3%] 1

Cycles

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

Binary size

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

Bootstrap: 777.808s -> 777.679s (-0.02%)
Artifact size: 328.83 MiB -> 328.83 MiB (0.00%)

@rustbot rustbot added the perf-regression Performance regression. label Feb 3, 2025
@lqd
Copy link
Member Author

lqd commented Feb 4, 2025

This doesn't look like noise. It is quite small however, is on secondary benchmarks only and has no cycles regressions. It's more of a bugfix than a feature, preventing a p-critical fix from being backported.

So it seems fine to me.

@lqd lqd deleted the ensure-stacks branch February 4, 2025 08:04
@rylev
Copy link
Member

rylev commented Feb 4, 2025

Marking as triaged for reasons specified in #136352 (comment)

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 4, 2025
@lqd
Copy link
Member Author

lqd commented Feb 6, 2025

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels handled by them.

@rustbot label +beta-accepted

cc @cuviper we should be finally good to go for this and #135709 :)

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 6, 2025
@cuviper cuviper mentioned this pull request Feb 6, 2025
@cuviper cuviper modified the milestones: 1.86.0, 1.85.0 Feb 6, 2025
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 6, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 6, 2025
[beta] backports

- Ensure that we don't try to access fields on a non-struct pattern type rust-lang#135222
- Do not include GCC source code in source tarballs rust-lang#135658
- Temporarily bring back `Rvalue::Len` rust-lang#135709
- Add a couple of missing `ensure_sufficient_stacks` rust-lang#136352
- Enable kernel sanitizers for aarch64-unknown-none-softfloat rust-lang#135905

r? cuviper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

regression: cannot borrow ... as immutable because it is also borrowed as mutable
10 participants