-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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 minicore
test auxiliary and support //@ use-minicore
directive in ui/assembly/codegen tests
#130693
base: master
Are you sure you want to change the base?
Conversation
@bors try |
[PROTOTYPE] Add minicore test auxiliary and support `//@ use-minicore` directive in ui/assembly/codegen tests **TODO: work in progress prototype implementation, opened early for MCP to reference** Context: [Real cross-compiling tests instead of `#![no_core]` silliness rust-lang#130375](rust-lang#130375) This PR introduces a prototype implementation of `minicore` auxiliary test helper. `minicore` contains stub definitions of std/core prelude items intended for consumption by tests that want the typical prelude items like `Copy` or `Result` in cross-compilation scenarios, but don't want nor need full `-Z build-std` (e.g. `tests/ui/abi/compatibility.rs`). The `minicore` auxiliary is a single source file `tests/auxiliary/minicore.rs`. The path to this auxiliary is made avaiable from bootstrap to compiletest via the `--minicore-path` compiletest flag. The `minicore` auxiliary is then built, on demand via `//@ use-minicore` compiletest directives, for each test revision for the given target (this distinction is important for when host != target in cross-compilation scenario). ### Implementation steps - [ ] 1. File an MCP to describe `tests/auxiliary/minicore.rs`, `--minicore-path` compiletest flag, `//@ use-minicore` compiletest directive and the behavior. - [ ] 2. And some self-tests to sanity check the behavior. - [ ] 3. Update rustc-dev-guide to describe the new `use-minicore` directive and provide an example, noting that `use-minicore` both requires `no_std` + `no_core` and implies `-C panic=abort` for the test file. r? `@ghost` (not yet ready for full review, still needs some self-tests and some try-jobs) (TODO: cc interested people once this passes initial try jobs in cross-compilation scenarios) cc `@/workingjubilee` `@/RalfJung` `@/nikic` `@/chrisnc` (if this makes sense to you in terms of functionality and UX) try-job: aarch64-apple try-job: armhf-gnu try-job: x86_64-msvc try-job: test-various try-job: dist-various-1
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
pub struct ManuallyDrop<T: ?Sized> { | ||
value: T, | ||
} | ||
impl<T: Copy + ?Sized> Copy for ManuallyDrop<T> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can derive Copy
if you add
#[rustc_builtin_macro]
pub macro Copy($item:item) {
/* compiler built-in */
}
and the same applies to Clone
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not make this change because I couldn't figure out how to make use of this, just kept manual impl Copy for ...
for now.
34e9887
to
df03437
Compare
Changes since last review:
|
f4a7c38
to
8bd9644
Compare
@@ -1129,6 +1135,37 @@ impl<'test> TestCx<'test> { | |||
) | |||
} | |||
|
|||
/// Builds `minicore`. Returns the path to the minicore rlib within the base test output | |||
/// directory. | |||
fn build_minicore(&self) -> PathBuf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remark: I don't like how convoluted the top-level runtest logic is, but intended for future compiletest cleanup PRs instead of this PR.
src/tools/compiletest/src/runtest.rs
Outdated
// `use-minicore` requires `#![no_std]` and `#![no_core]`, which means no unwinding panics. | ||
if self.props.use_minicore { | ||
rustc.arg("-Cpanic=abort"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I need to document //@ use-minicore
implies and mandates -C panic=abort
for the test in rustc-dev-guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/tools/compiletest/src/runtest.rs
Outdated
// `use-minicore` requires `#![no_std]` and `#![no_core]`, which means no unwinding panics. | ||
if self.props.use_minicore { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remark: technically, this also needs to check and ensure that the test writer does not manually override -C panic=xxx
where xxx != abort
, but I would prefer that happen after untangling how directives are handled. The flag is now set in the final position, which will override custom user compile-flags
.
//@ use-minicore
directive in ui/assembly/codegen testsminicore
test auxiliary and support //@ use-minicore
directive in ui/assembly/codegen tests
I might temporarily shelve this unless we just use the |
Given that the alternatives have been considered, a new directive seems fine for me. But let's see what t-compiler says. :) |
It's unfortunate, but given that I would propose that we go with this: // usage of `minicore.rs` in every test that want to use it
#[rustc_builtin_macro]
macro_rules! include { ($file:expr $(,)?) => {{ /* compiler built-in */ }}; }
include!("../../auxiliary/minicore.rs"); and we ensure |
You forgot several lines: #![no_core]
#![crate_type = "lib"]
#![feature(rustc_attrs)]
#![feature(no_core)]
#![feature(lang_items)]
#![allow(internal_features)] |
Remember: #66920 |
Sure, but they also need to be included with all the other proposal I think, so using |
If minicore is a separate crate, those lines will not be needed in the test.
|
I still see (some) of those lines in the current (but there would probably be fewer of them) (and while I don't like |
Summary for T-compiler triage
Context: Real cross-compiling tests instead of Candidate ApproachesApproach 1: special
|
…n-test, r=jieyouxu Regression test for AVR `rjmp` offset This adds a regression test for rust-lang#129301 by minimizing the code in the linked issue and putting it into a `#![no_core]`-compatible format, so that it can easily be used within an `rmake`-test. This needs to be a `rmake` test (opposed to a `tests/assembly` one), since the linked issue describes, that the problem only occurs if the code is directly compiled. Note, that `lld` is used instead of `avr-gcc`; see the [comments](rust-lang#131755 (comment)) [below](rust-lang#131755 (comment)). Closes rust-lang#129301. To show, that the test actually catches the wrong behavior, this can be tested with a faulty rustc: ```bash $ rustup install nightly-2024-08-19 $ rustc +nightly-2024-08-19 -C opt-level=s -C panic=abort --target avr-unknown-gnu-atmega328 -Clinker=build/x86_64-unknown-linux-gnu/ci-llvm/bin/lld -Clink-arg='--entry=main' -o compiled tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs $ llvm-objdump -d compiled | grep '<main>' -A 6 000110b4 <main>: 110b4: 81 e0 ldi r24, 0x1 110b6: 92 e0 ldi r25, 0x2 110b8: 85 b9 out 0x5, r24 110ba: 95 b9 out 0x5, r25 110bc: fe cf rjmp .-4 ``` One can see, that the wrong label offset (`4` instead of `6`) is produced, which would trigger an assertion in the test case. This would be a good candidate for using the `minicore` proposed in rust-lang#130693. Since this is not yet merged, there is a FIXME. r? Patryk27 I think, you are the yet-to-be official target maintainer, hence I'll assign to you. `@rustbot` label +O-AVR
Rollup merge of rust-lang#131755 - jfrimmel:avr-rjmp-offset-regression-test, r=jieyouxu Regression test for AVR `rjmp` offset This adds a regression test for rust-lang#129301 by minimizing the code in the linked issue and putting it into a `#![no_core]`-compatible format, so that it can easily be used within an `rmake`-test. This needs to be a `rmake` test (opposed to a `tests/assembly` one), since the linked issue describes, that the problem only occurs if the code is directly compiled. Note, that `lld` is used instead of `avr-gcc`; see the [comments](rust-lang#131755 (comment)) [below](rust-lang#131755 (comment)). Closes rust-lang#129301. To show, that the test actually catches the wrong behavior, this can be tested with a faulty rustc: ```bash $ rustup install nightly-2024-08-19 $ rustc +nightly-2024-08-19 -C opt-level=s -C panic=abort --target avr-unknown-gnu-atmega328 -Clinker=build/x86_64-unknown-linux-gnu/ci-llvm/bin/lld -Clink-arg='--entry=main' -o compiled tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs $ llvm-objdump -d compiled | grep '<main>' -A 6 000110b4 <main>: 110b4: 81 e0 ldi r24, 0x1 110b6: 92 e0 ldi r25, 0x2 110b8: 85 b9 out 0x5, r24 110ba: 95 b9 out 0x5, r25 110bc: fe cf rjmp .-4 ``` One can see, that the wrong label offset (`4` instead of `6`) is produced, which would trigger an assertion in the test case. This would be a good candidate for using the `minicore` proposed in rust-lang#130693. Since this is not yet merged, there is a FIXME. r? Patryk27 I think, you are the yet-to-be official target maintainer, hence I'll assign to you. `@rustbot` label +O-AVR
Based on https://rust-lang.zulipchat.com/#narrow/channel/233931-t-compiler.2Fmajor-changes/topic/Support.20tests.20to.20use.20.60minicore.60.20std.2Fcore.20.E2.80.A6.20compiler-team.23786/near/477576328 having 7 votes in favor of the special directive approach, I'm going to proceed with that. @rustbot author |
Context: Real cross-compiling tests instead of
#![no_core]
silliness #130375MCP: rust-lang/compiler-team#786
Tracking issue: #131485
This prototype PR is subject to further changes based on feedback.
New
minicore
test auxiliary and//@ use-minicore
compiletest directiveThis PR introduces a prototype implementation of a
minicore
auxiliary test helper that providescore
prelude stubs for#![no_core]
ui/assembly/codegen tests that need to build but not run on both the host and the cross-compiled target.Key summary:
minicore
contains stub definitions ofcore
items intended for consumption bycheck-pass
/build-pass
tests that want the typical prelude items likeCopy
to be stubbed out under#![no_core]
scenarios, so that the test can be built (not run) for cross-compiled target. Such tests don't want nor need full-Z build-std
(e.g.tests/ui/abi/compatibility.rs
).minicore
is intended forcore
items only, notstd
-only oralloc
items. If stubs foralloc
orstd
are wanted, they should be provided by an additional directive and test auxiliary, and not be conflated withminicore
orcore
stubs. This is because a wider range of tests can benefit fromcore
-only stubs.Implementation
minicore
auxiliary is a single source filetests/auxiliary/minicore.rs
.minicore
is made avaiable from bootstrap to compiletest via the--minicore-path
compiletest flag.minicore
is then built on-demand via the//@ use-minicore
compiletest directive, for each test revision for the given target (this distinction is important for when host != target in cross-compilation scenario).minicore
is then made available to the test as an extern prelude.Example usage
Implementation steps
minicore
test auxiliary.minicore
in bootstrap.--minicore-path
compiletest cli flag and passminicore
build artifact path from bootstrap to compiletest.use-minicore
is mutually incompatible with tests that require to berun
, as the stubs are only good for tests that only need to be built (i.e. norun-{pass,fail}
).tests/auxiliary/minicore.rs
is input stamped, i.e. modifyingtests/auxiliary/minicore.rs
should invalidate test cache and force the test to be rerun.try-job: aarch64-apple
try-job: armhf-gnu
try-job: x86_64-msvc
try-job: test-various
try-job: dist-various-1