-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Port #[link]
to the new attribute parsing infrastructure
#143193
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
Conversation
47c45bb
to
aaf7d53
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #143233) made this pull request unmergeable. Please resolve the merge conflicts. |
aaf7d53
to
d43b4b5
Compare
This comment has been minimized.
This comment has been minimized.
29b3648
to
6b10a20
Compare
PrintAttribute | ||
)] | ||
#[derive(HashStable_Generic)] | ||
pub enum NativeLibKind { |
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.
This type was moved here because it is needed for the link attribute and the original location caused a dependency cycle
|
||
#[link(name = "bar", import_name_type = "decorated", kind = "raw-dylib")] | ||
#[link(name = "bar", kind = "raw-dylib")] |
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.
This test is not testing for the invalid attribute, but instead for the link attribute on an extern type. Because attributes are parsed earlier and this attribute was invalid the error blocked the crash from happening. This is true for quite a few other of the test changes as well
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 noted when testing today that this test as of 7db1840 is now fixed on aarch64-apple-darwin
. It continues to be fixed on latest master
.
Testing stage2 compiletest suite=crashes mode=crashes (stage1 -> stage2, aarch64-apple-darwin)
running 1 tests
2025-09-03T19:52:06.078181Z ERROR compiletest::runtest: fatal error, panic: "crashtest no longer crashes/triggers ICE, hooray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge. If you want to see verbose output, set `COMPILETEST_VERBOSE_CRASHES=1`."
[crashes] tests/crashes/120175.rs ... F
failures:
---- [crashes] tests/crashes/120175.rs stdout ----
------rustc stdout------------------------------
------rustc stderr------------------------------
warning: the feature `raw_dylib_elf` is incomplete and may not be safe to use and/or cause compiler crashes
--> /Users/ana/git/rust-lang/rust/tests/crashes/120175.rs:5:12
|
5 | #![feature(raw_dylib_elf)]
| ^^^^^^^^^^^^^
|
= note: see issue #135694 <https://github.com/rust-lang/rust/issues/135694> for more information
= note: `#[warn(incomplete_features)]` on by default
error[E0455]: link kind `raw-dylib` is only supported on Windows targets
--> /Users/ana/git/rust-lang/rust/tests/crashes/120175.rs:7:29
|
7 | #[link(name = "bar", kind = "raw-dylib")]
| ^^^^^^^^^^^
error: aborting due to 1 previous error; 1 warning emitted
For more information about this error, try `rustc --explain E0455`.
------------------------------------------
error: crashtest no longer crashes/triggers ICE, hooray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge. If you want to see verbose output, set `COMPILETEST_VERBOSE_CRASHES=1`.
thread '[crashes] tests/crashes/120175.rs' panicked at src/tools/compiletest/src/runtest/crashes.rs:17:18:
fatal error
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
[crashes] tests/crashes/120175.rs
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 225 filtered out; finished in 149.92ms
Some tests failed in compiletest suite=crashes mode=crashes host=aarch64-apple-darwin target=aarch64-apple-darwin
Build completed unsuccessfully in 0:04:28
Took 4min 28sec 739ms
Exit status 1
@@ -81,8 +81,7 @@ | |||
#[export_stable = 1] | |||
//~^ ERROR malformed | |||
#[link] | |||
//~^ ERROR attribute must be of the form | |||
//~| WARN this was previously accepted by the compiler | |||
//~^ ERROR malformed |
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.
BREAKING CHANGE 1: A #[link]
attribute which is not a list #[link(...)]
used to be a warning future error, is now an error. This needs a crater run.
@@ -61,7 +61,7 @@ | |||
#![doc = "2400"] | |||
#![cold] //~ WARN attribute should be applied to a function | |||
//~^ WARN this was previously accepted | |||
#![link()] //~ WARN attribute should be applied to an `extern` block | |||
#![link(name = "x")] //~ WARN attribute should be applied to an `extern` block |
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.
BREAKING CHANGE 2: A link attribute applied to an invalid target was not checked for syntactic correctness, it now is. The invalid target remains a warning, but the syntactic corectness is now checked
@rustbot ready Needs a crater run |
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_codegen_ssa These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in compiler/rustc_attr_data_structures This PR changes a file inside |
@bors try @craterbot queue |
Port `#[link]` to the new attribute parsing infrastructure Ports `link` to the new attribute parsing infrastructure for #131229 (comment) This rework is not yet finished, it is blocked on the `cfg` attribute being finished, since `link` takes a `cfg` attribute as argument `@rustbot` blocked
☀️ Try build successful - checks-actions |
@craterbot queue |
🚨 Error: failed to parse the command 🆘 If you have any trouble with Crater please ask in t-infra on Zulip |
:/ |
Would it be possible to change the test to a check-pass and set the target instead of only running on x86 windows host? |
Hmmm that seems reasonable for at least this specific test. There's actually a bunch of windows only tests for link, I'll see how many I can convert |
I quickly tried this. Here's what I came up with: //@ add-core-stubs
//@ check-pass
//@ compile-flags: --target i686-pc-windows-msvc
// maybe also test i686-pc-windows-gnu?
#![feature(no_core, rustc_attrs, lang_items)]
#![no_core]
#![crate_type = "lib"]
#[link(name = "foo", kind = "raw-dylib", import_name_type = 6)]
//~^ ERROR import name type must be of the form `import_name_type = "string"`
extern "C" {} Unfortunately |
ff053ba
to
9cbd0c7
Compare
@ChrisDenton I followed your trick (thanks a lot!) and I think I have all tests working now, but I didn't verify on a windows VM since after an hour of trying to get it working I still can't build the rust project using it, no clue why, getting weird errors and everything is super slow for some reason @jdonszelmann Could you do a try run of |
@bors try jobs=i686-msvc-1,i686-msvc-2,aarch64-msvc-1,aarch64-msvc-2 |
This comment has been minimized.
This comment has been minimized.
Port `#[link]` to the new attribute parsing infrastructure try-job: i686-msvc-1 try-job: i686-msvc-2 try-job: aarch64-msvc-1 try-job: aarch64-msvc-2
☔ The latest upstream changes (presumably #145906) made this pull request unmergeable. Please resolve the merge conflicts. |
Signed-off-by: Jonathan Brouwer <[email protected]>
9cbd0c7
to
7db1840
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
@bors r+ |
Rollup of 9 pull requests Successful merges: - #142727 (wasm: rm static mut) - #143193 (Port `#[link]` to the new attribute parsing infrastructure ) - #144864 (No source fixes) - #145913 (Add spin_loop hint for LoongArch) - #145926 (compiletest: Remove several remnants of the old libtest-based executor) - #145928 (Rename `Location::file_with_nul` to `file_as_c_str`) - #145930 (`const`ify (the unstable) `str::as_str`) - #145941 (Disable `integer_to_ptr_transmutes` suggestion for unsized types) - #145953 (Update `icu_list` to 2.0) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #143193 - JonathanBrouwer:link_rework, r=jdonszelmann Port `#[link]` to the new attribute parsing infrastructure Ports `link` to the new attribute parsing infrastructure for #131229 (comment)
…=jdonszelmann Improve error messages around invalid literals in attribute arguments r? `@jdonszelmann` This previously created two errors, which is a bit ugly and the second one didn't add any value Blocked on rust-lang#143193
Ports
link
to the new attribute parsing infrastructure for #131229 (comment)