-
Notifications
You must be signed in to change notification settings - Fork 13.7k
rustc_expand: ensure stack in InvocationCollector::visit_expr
#145410
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
base: master
Are you sure you want to change the base?
Conversation
In Fedora, when we built rustc with PGO on ppc64le, we started failing the test `issue-74564-if-expr-stack-overflow.rs`. This could also be reproduced on other arches by setting a smaller `RUST_MIN_STACK`, so it's probably just unlucky that ppc64le PGO created a large stack frame somewhere in this recursion path. Adding an `ensure_sufficient_stack` solves the stack overflow. Historically, that test and its fix were added in rust-lang#74708, which was also an `ensure_sufficient_stack` in this area of code at the time. However, the refactor in rust-lang#92573 basically left that to the general `MutVisitor`, and then rust-lang#142240 removed even that ensure call. It may be luck that our tier-1 tested targets did not regress the original issue across those refactors.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustc_expand: ensure stack in `InvocationCollector::visit_expr`
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (573f999): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.4%, secondary -4.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 5.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 470.516s -> 469.329s (-0.25%) |
r? lqd @bors r+ |
In Fedora, when we built rustc with PGO on ppc64le, we started failing
the test
issue-74564-if-expr-stack-overflow.rs
. This could also bereproduced on other arches by setting a smaller
RUST_MIN_STACK
, soit's probably just unlucky that ppc64le PGO created a large stack frame
somewhere in this recursion path. Adding an
ensure_sufficient_stack
solves the stack overflow.
Historically, that test and its fix were added in #74708,
which was also an
ensure_sufficient_stack
in this area of code at thetime. However, the refactor in #92573 basically left that
to the general
MutVisitor
, and then #142240 removed eventhat ensure call. It may be luck that our tier-1 tested targets did not
regress the original issue across those refactors.