Skip to content
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

Revert "miri: make sure we can find link_section statics even for the local crate" #127099

Merged
merged 2 commits into from
Jun 29, 2024

Conversation

lqd
Copy link
Member

@lqd lqd commented Jun 28, 2024

This PR reverts #126938 as requested by its author, to fix the #127052 regression.

Fixes #127052

We should probably improve the used rmake test(s) in the future, but this should do for now.

@rustbot rustbot added 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 Jun 28, 2024
@lqd
Copy link
Member Author

lqd commented Jun 28, 2024

@bors try

@bors
Copy link
Contributor

bors commented Jun 28, 2024

⌛ Trying commit 57931e5 with merge 6ee8465...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 28, 2024
Revert "miri: make sure we can find link_section statics even for the local crate"

This PR reverts rust-lang#126938 as [requested by its author](rust-lang#127052 (comment)), to fix the rust-lang#127052 regression.

r? ghost

fixes rust-lang#127052

We should probably improve the [`used` rmake test(s)](https://github.com/rust-lang/rust/blob/57931e50409f9365791f7923a68f9ae1ec301c4c/tests/run-make/used/rmake.rs#L7) in the future, though this should do for now.

(Opening as draft to run tests on a windows env)

try-job: x86_64-msvc
@workingjubilee
Copy link
Member

Thank you for filing this revert + regression test!

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

thanks @lqd, r=me when this is done bors-trying so we can fix this

@lqd
Copy link
Member Author

lqd commented Jun 28, 2024

The test technically looks good on the try build already.

test [ui] tests\ui\linkage-attr\unreferenced-used-static-issue-127052.rs ... ok

(and by now an old windows box I have has finished rebuilding LLVM -- to workaround the recent linking issues that other people currently also experience when building rustc -- so I also checked that the test does fail on master without the revert, no need for another try build 🚀).

I'm not sure if //@ only-msvc is restrictive enough in this case (but probably?), and I guess the test comment could be more explicit about what happens but nbd -- so I wanted to ask @ChrisDenton but it seems fine for a revert :)

And if this is indeed what caused CI to fail recently maybe we can bump the prio.

@lqd lqd marked this pull request as ready for review June 28, 2024 22:20
@rustbot
Copy link
Collaborator

rustbot commented Jun 28, 2024

The Miri subtree was changed

cc @rust-lang/miri

@matthiaskrgr
Copy link
Member

@bors p=25 because this problem broke a bunch of ci runs already

@bors
Copy link
Contributor

bors commented Jun 28, 2024

☀️ Try build successful - checks-actions
Build commit: 6ee8465 (6ee8465a2ca6b4af09e56482223dcfacd262ffa4)

@lqd
Copy link
Member Author

lqd commented Jun 28, 2024

finallyyyyyyy 😴

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Jun 28, 2024

📌 Commit 57931e5 has been approved by compiler-errors

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 Jun 28, 2024
@bors
Copy link
Contributor

bors commented Jun 28, 2024

⌛ Testing commit 57931e5 with merge 9ed2ab3...

@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #127099!

Tested on commit 9ed2ab3.
Direct link to PR: #127099

🎉 reference on windows: test-fail → test-pass (cc @Havvy @ehuss @matthewjasper).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jun 29, 2024
Tested on commit rust-lang/rust@9ed2ab3.
Direct link to PR: <rust-lang/rust#127099>

🎉 reference on windows: test-fail → test-pass (cc @Havvy @ehuss @matthewjasper).
@bors
Copy link
Contributor

bors commented Jun 29, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 9ed2ab3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 29, 2024
@bors bors merged commit 9ed2ab3 into rust-lang:master Jun 29, 2024
7 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 29, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9ed2ab3): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results (primary -1.7%, secondary 4.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.7% [0.7%, 2.7%] 2
Regressions ❌
(secondary)
4.8% [4.8%, 4.8%] 1
Improvements ✅
(primary)
-8.6% [-8.6%, -8.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.7% [-8.6%, 2.7%] 3

Cycles

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

Binary size

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

Bootstrap: 694.621s -> 695.135s (0.07%)
Artifact size: 324.60 MiB -> 324.56 MiB (-0.01%)

@lqd lqd deleted the revert-126938 branch June 29, 2024 05:37
// removed by the MSVC linker, causing linking errors.

//@ build-pass: needs linking
//@ only-msvc
Copy link
Member

Choose a reason for hiding this comment

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

Why shouldn't we run this test on all targets?

Copy link
Member Author

Choose a reason for hiding this comment

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

The specific issue doesn't exist elsewhere so it's less useful there: the existing tests already cover #[used] features and targets more than this test. Of course, it wouldn't hurt. Feel free to change the directive to something you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Well, clearly the existing tests do not cover #[used] in the binary, or my PR would not have landed.

@apiraino
Copy link
Contributor

apiraino commented Jul 2, 2024

thank you @lqd 🙇‍♂️

(sometimes I have to spell it out where a emoji is not enough)

jhpratt added a commit to jhpratt/rust that referenced this pull request Jul 3, 2024
… r=lqd,ChrisDenton

unreferenced-used-static: run test everywhere

Follow-up to rust-lang#127099.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 3, 2024
Rollup merge of rust-lang#127115 - RalfJung:unreferenced-used-static, r=lqd,ChrisDenton

unreferenced-used-static: run test everywhere

Follow-up to rust-lang#127099.
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. 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.

Regression in #[used] attribute on Windows msvc
10 participants