rustc_expand: ensure stack in InvocationCollector::visit_expr#145410
rustc_expand: ensure stack in InvocationCollector::visit_expr#145410bors merged 1 commit intorust-lang:masterfrom
InvocationCollector::visit_expr#145410Conversation
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+ |
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 8e3710e (parent) -> d127901 (this PR) Test differencesNo test diffs found Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard d127901d940d96209fd2ae8ff6769ad2788099fb --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (d127901): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary 4.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.8%, secondary -3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 471.382s -> 470.79s (-0.13%) |
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_stacksolves the stack overflow.
Historically, that test and its fix were added in #74708,
which was also an
ensure_sufficient_stackin 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.