Skip to content

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Sep 13, 2025

These were previously disabled, in part for performance reasons, in part due to needing availability symbols __isPlatformVersionAtLeast and __isOSVersionAtLeast that compiler-builtins did not provide, see #62592 (comment) and #134275 (comment) for failed checks.

Since #138944 though, std now provides these symbols, so we should be able to re-enable LLVM assertions, debug assertions and overflow checks.

Fixes #59637.

try-job: *apple*

@madsmtm madsmtm added O-macos Operating system: macOS T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. A-CI Area: Our Github Actions CI labels Sep 13, 2025
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 13, 2025
@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 13, 2025

@bors try

rust-bors bot added a commit that referenced this pull request Sep 13, 2025
Re-enable assertions on macOS

try-job: `*apple*`
@rust-bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Sep 13, 2025

💔 Test for beeae75 failed: CI. Failed jobs:

@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 13, 2025

Hmm, might need a stage0 bump, waiting for that:
@rustbot blocked

EDIT: #146636

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 16, 2025
@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 27, 2025

@rustbot author
@bors try

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Sep 27, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 27, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Sep 27, 2025
Re-enable assertions on macOS

try-job: `*apple*`
@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 27, 2025

@bors try cancel
I need to rebase first probably

@rust-bors
Copy link

rust-bors bot commented Sep 27, 2025

Try build cancelled. Cancelled workflows:

@madsmtm madsmtm force-pushed the apple-reenable-assertions branch from 2bdfca2 to b2c7b6b Compare September 27, 2025 09:09
@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 27, 2025

@bors try

rust-bors bot added a commit that referenced this pull request Sep 27, 2025
Re-enable assertions on macOS

try-job: `*apple*`
@rust-bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Sep 27, 2025

💔 Test for 56e5725 failed: CI. Failed jobs:

This wasn't caught by CI, because debug assertions aren't enabled there.
@madsmtm madsmtm force-pushed the apple-reenable-assertions branch from b2c7b6b to 25bedcd Compare September 27, 2025 12:41
@madsmtm madsmtm removed A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 27, 2025
@rust-bors
Copy link

rust-bors bot commented Sep 27, 2025

☀️ Try build successful (CI)
Build commit: 2ec5766 (2ec5766d21d2e578d3ba452d7d12814b21e8140b, parent: ade84871f718ea20a6460d28e82290353b4bf3d2)

@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 27, 2025

So, I think this works!
@rustbot ready

r? @Mark-Simulacrum since you're on t-infra and were assigned to #59637 in the past.

Also CC @jieyouxu, since you've tried to do this in the past.

@rustbot rustbot 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. labels Sep 27, 2025
@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 27, 2025

I'm unsure if it has a performance impact, comparing it against the auto build on 959b450 gives:

Previously This PR
CI Run Link Link
dist-x86_64-apple 1h 40m 1h 56m
dist-apple-various 1h 5m 1h 5m
dist-aarch64-apple 2h 2m 1h 37m
aarch64-apple 1h 49m 2h 3m

Which feels like wayy too high a variance to really be able to say anything conclusive?

@madsmtm madsmtm marked this pull request as ready for review September 27, 2025 18:31
@rustbot
Copy link
Collaborator

rustbot commented Sep 27, 2025

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

I'm a bit confused. This appears to enable assertions on a bunch of dist targets, but distributed toolchains are not supposed to enable assertions.

Should this change be limited to only aarch64-apple, which is a test target and as such should enable assertions?

View changes since this review

@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 27, 2025

I'm a bit confused. This appears to enable assertions on a bunch of dist targets, but distributed toolchains are not supposed to enable assertions.

Right, the dist- builds set DEPLOY. Only the -alt builds get debug assertions, overflow checks and LLVM assertions in here. So this PR as-is only actually affects aarch64-apple, the rest still have all the assertions disabled.

That said, it this will allow others to enable -alt if they find they need to test with debug assertions. I tested an -alt build in this CI run, it currently passes.

@Mark-Simulacrum Mark-Simulacrum changed the title Re-enable assertions on macOS Re-enable assertions on macOS alt builds Sep 28, 2025
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

This seems reasonable to me. Thanks! I agree that the CI times don't seem affected (at least within noise reasonable to achieve from a single PR's measurements).

@bors
Copy link
Collaborator

bors commented Sep 28, 2025

📌 Commit 9878be7 has been approved by Mark-Simulacrum

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 Sep 28, 2025
@bors
Copy link
Collaborator

bors commented Sep 28, 2025

⌛ Testing commit 9878be7 with merge f957826...

@bors
Copy link
Collaborator

bors commented Sep 29, 2025

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing f957826 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 29, 2025
@bors bors merged commit f957826 into rust-lang:master Sep 29, 2025
12 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Sep 29, 2025
Copy link
Contributor

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 c8905ea (parent) -> f957826 (this PR)

Test differences

Show 35 test diffs

Stage 2

  • [codegen] tests/codegen-llvm/binary-heap-peek-mut-pop-no-panic.rs: pass -> ignore (ignored when std is built with debug assertions) (J0)
  • [codegen] tests/codegen-llvm/mem-replace-big-type.rs: pass -> ignore (ignored when std is built with debug assertions) (J0)
  • [codegen] tests/codegen-llvm/mem-replace-simple-type.rs: ignore (only executed when the architecture is x86_64 ((to not worry about usize differing))) -> ignore (ignored when std is built with debug assertions) (J0)
  • [codegen] tests/codegen-llvm/range-loop.rs: pass -> ignore (ignored when std is built with debug assertions) (J0)
  • [codegen] tests/codegen-llvm/slice-reverse.rs: ignore (only executed when the architecture is x86_64) -> ignore (ignored when std is built with debug assertions ((debug assertions prevent generating shufflevector))) (J0)
  • [codegen] tests/codegen-llvm/swap-small-types.rs: ignore (only executed when the architecture is x86_64) -> ignore (ignored when std is built with debug assertions ((ptr::swap_nonoverlapping has one which blocks some optimizations))) (J0)
  • [codegen] tests/codegen-llvm/vec-in-place.rs: pass -> ignore (ignored when std is built with debug assertions ((FIXME: checks for call detect scoped noalias metadata))) (J0)
  • [codegen] tests/codegen-llvm/vec-shrink-panik.rs: pass -> ignore (ignored when std is built with debug assertions ((plain old debug assertions))) (J0)
  • [codegen] tests/codegen-llvm/vec-with-capacity.rs: pass -> ignore (ignored when std is built with debug assertions) (J0)
  • [codegen] tests/codegen-llvm/vecdeque-drain.rs: pass -> ignore (ignored when std is built with debug assertions ((FIXME: checks for call detect scoped noalias metadata))) (J0)
  • [codegen] tests/codegen-llvm/vecdeque_no_panic.rs: pass -> ignore (ignored when std is built with debug assertions ((plain old debug assertions))) (J0)
  • [crashes] tests/crashes/108428.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [crashes] tests/crashes/118778.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [crashes] tests/crashes/118784.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [crashes] tests/crashes/120175.rs: ignore (ignored if rustc wasn't built with debug assertions) -> ignore (ignored when the target vendor is Apple ((raw-dylib doesn't work on Apple targets yet))) (J0)
  • [crashes] tests/crashes/123862.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [crashes] tests/crashes/130395.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [crashes] tests/crashes/133965.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [crashes] tests/crashes/134061.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [crashes] tests/crashes/137514.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [crashes] tests/crashes/139381.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [crashes] tests/crashes/139387.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [mir-opt] tests/mir-opt/pre-codegen/mem_replace.rs: pass -> ignore (ignored when std is built with debug assertions) (J0)
  • [mir-opt] tests/mir-opt/pre-codegen/ptr_offset.rs: pass -> ignore (ignored when std is built with debug assertions ((precondition checks are under cfg(debug_assertions)))) (J0)
  • [mir-opt] tests/mir-opt/pre-codegen/slice_iter.rs: pass -> ignore (ignored when std is built with debug assertions ((precondition checks on ptr::add are under cfg(debug_assertions)))) (J0)
  • [ui] tests/ui/associated-consts/issue-93775.rs#current: pass -> ignore (ignored when rustc is built with debug assertions) (J0)
  • [ui] tests/ui/associated-consts/issue-93775.rs#next: pass -> ignore (ignored when rustc is built with debug assertions) (J0)
  • [ui] tests/ui/errors/emitter-overflow-bad-whitespace.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [ui] tests/ui/mir/static-by-value-dyn.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [ui] tests/ui/mir/unreachable-loop-jump-threading.rs: ignore (ignored if rustc wasn't built with debug assertions) -> pass (J0)
  • [ui] tests/ui/print_type_sizes/niche-filling.rs: pass -> ignore (ignored when std is built with debug assertions ((debug assertions will print more types))) (J0)
  • registry::reach_max_unpack_size: [missing] -> pass (J0)

Additionally, 3 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard f957826bff7a68b267ce75b1ea56352aed0cca0a --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. aarch64-apple: 5057.5s -> 9296.2s (83.8%)
  2. dist-armhf-linux: 4979.7s -> 7862.3s (57.9%)
  3. aarch64-gnu-llvm-20-2: 2690.6s -> 2166.5s (-19.5%)
  4. pr-check-1: 1757.8s -> 1434.7s (-18.4%)
  5. x86_64-gnu-llvm-20: 2792.4s -> 2384.4s (-14.6%)
  6. x86_64-rust-for-linux: 3103.8s -> 2723.7s (-12.2%)
  7. i686-gnu-2: 6382.0s -> 5607.5s (-12.1%)
  8. x86_64-gnu-llvm-20-2: 5503.0s -> 6158.6s (11.9%)
  9. pr-check-2: 2633.3s -> 2334.5s (-11.3%)
  10. i686-gnu-1: 8167.3s -> 7354.5s (-10.0%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f957826): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.0% [0.0%, 0.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -1.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.6% [-3.6%, -3.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.6% [-3.6%, 0.5%] 2

Cycles

Results (primary 2.0%, secondary 2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
2.0% [2.0%, 2.0%] 1
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.0% [2.0%, 2.0%] 1

Binary size

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

Bootstrap: 470.874s -> 470.189s (-0.15%)
Artifact size: 387.66 MiB -> 387.63 MiB (-0.01%)

@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 29, 2025

Okay, so this did change the timings fairly significantly:

aarch64-apple: 5057.5s -> 9296.2s (83.8%)

Is this within an acceptable threshold? Or should we revert this? Or maybe split the aarch64-apple CI step into two?

@madsmtm madsmtm deleted the apple-reenable-assertions branch September 29, 2025 10:35
@nikic
Copy link
Contributor

nikic commented Sep 29, 2025

That result is probably not representative, as this change will have triggered an uncached LLVM rebuild.

@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 29, 2025

Re perf regression, I think that's spurious, this is only affecting the macOS runners, which aren't perf-tested, and it shouldn't affect the dist- builds (which are the ones that perf would test, and those are still built without assertions).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: Our Github Actions CI merged-by-bors This PR was explicitly merged by bors. O-macos Operating system: macOS S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-enable LLVM and debug assertions for slow builders
7 participants