Skip to content

Conversation

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Oct 17, 2025

r? @jieyouxu

Unfortunately this doesn't work for all tests; minicore sometimes fails to build with errors like

rustc-LLVM ERROR: ILP32E cannot be used with the D ISA extension

and

error: the target features paca, pacg must all be either enabled or disabled together

These errors are meant to be triggered in the tests, but not in minicore.

It seems like all @compile-flags are forwarded to minicore. Maybe we should exclude -Ctarget-feature from that? Or provide some way to set flags only for the current file, not minicore?

@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2025

Some changes occurred in tests/ui/sanitizer

cc @rcvalle

@rustbot rustbot added PG-exploit-mitigations Project group: Exploit mitigations 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 Oct 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2025

jieyouxu is not on the review rotation at the moment.
They may take a while to respond.

@RalfJung RalfJung changed the title use minicore for target-feature tests use minicore for more tests Oct 17, 2025
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, test changes themselves look good

View changes since this review

@RalfJung
Copy link
Member Author

So do you have an idea what we could do about those minicore build failures?

@jieyouxu
Copy link
Member

(Sorry, it's not a full review yet, I'm still looking, I just got distracted by something else IRL 😆)

@jieyouxu
Copy link
Member

jieyouxu commented Oct 19, 2025

It seems like all @compile-flags are forwarded to minicore. Maybe we should exclude -Ctarget-feature from that? Or provide some way to set flags only for the current file, not minicore?

Hm, forwarding seems a bit strange, I would've expected //@ compile-flags to be current-test-crate only. Saethlin added a dedicated //@ core-stubs-compile-flags for configuring compile flags of minicore specifically. I'll need to double-check this.

@RalfJung
Copy link
Member Author

I would've expected //@ compile-flags to be current-test-crate only

The make_compile_args call here adds all compile-flags when building minicore.

Saethlin added a dedicated //@ core-stubs-compile-flags for configuring compile flags of minicore specifically. I'll need to double-check this.

Yeah I just found that, it's not exactly very documented. The result looks a bit strange but it works. :)

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

jieyouxu commented Oct 19, 2025

I took a closer look -- as is tradition, this is a bit of a mess in compiletest. The gist though is that make_compile_args is needed for the preset compiler flags that ui (or other test modes) should respectively get, except we don't properly or explicitly tell apart:

  1. Compiler flags that should be set for all ui tests (unless overriden), which includes auxiliaries or minicore when built as an auxiliary under the test mode
  2. Compiler flags that should be set for the current test crate itself, versus auxiliaries, versus minicore

I would say that we probably should not be forwarding compiler flags in the general case (... or provide an opt-out to stop that behavior), because yeah with something like target features, it's not exactly obvious or easy to disable...

Comment on lines 9 to 10
// We need to *disable* the feature again for minicore as otherwise that will fail to build.
//@[riscv] core-stubs-compile-flags: -Ctarget-feature=-d
Copy link
Member

Choose a reason for hiding this comment

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

Remark: but yeah, this is a sensible workaround in the mean time

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to // FIXME(#147881): ... for these cases, the workarounds are fine for this PR tho

@RalfJung
Copy link
Member Author

I would say that we probably should not be forwarding compiler flags in the general case (... or provide an opt-out to stop that behavior), because yeah with something like target features, it's not exactly obvious or easy to disable...

We need --target to be forwarded though. So a lot of tests are already relying on this forwarding.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 19, 2025 via email

@bors
Copy link
Collaborator

bors commented Oct 19, 2025

📌 Commit eb1c62b has been approved by jieyouxu

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 Oct 19, 2025
@bors
Copy link
Collaborator

bors commented Oct 19, 2025

⌛ Testing commit eb1c62b with merge f04e3df...

@bors
Copy link
Collaborator

bors commented Oct 19, 2025

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing f04e3df to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 19, 2025
@bors bors merged commit f04e3df into rust-lang:master Oct 19, 2025
12 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Oct 19, 2025
@github-actions
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 4ddbb60 (parent) -> f04e3df (this PR)

Test differences

Show 2 test diffs

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

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard f04e3dfc87d7e2b6ad53e7a52253812cd62eba50 --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. dist-apple-various: 3322.7s -> 4268.0s (28.5%)
  2. aarch64-apple: 7085.0s -> 7897.9s (11.5%)
  3. x86_64-gnu-llvm-20-1: 3059.2s -> 3378.7s (10.4%)
  4. x86_64-gnu-distcheck: 6263.6s -> 6858.7s (9.5%)
  5. x86_64-gnu: 6803.4s -> 7403.1s (8.8%)
  6. dist-aarch64-apple: 5659.3s -> 6105.4s (7.9%)
  7. dist-x86_64-mingw: 8385.1s -> 8980.2s (7.1%)
  8. dist-loongarch64-linux: 5669.4s -> 5271.1s (-7.0%)
  9. aarch64-gnu-llvm-20-2: 2132.5s -> 2278.8s (6.9%)
  10. dist-ohos-x86_64: 4222.2s -> 4508.8s (6.8%)
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 (f04e3df): 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.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

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

Cycles

Results (secondary -2.2%)

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

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

Binary size

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

Bootstrap: 471.841s -> 473.871s (0.43%)
Artifact size: 388.67 MiB -> 388.68 MiB (0.00%)

@RalfJung RalfJung deleted the more-minicore branch October 20, 2025 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. PG-exploit-mitigations Project group: Exploit mitigations 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.

6 participants