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.

Sign up for GitHub

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

Jump to bottom

Report unsafe for overriding link sections #97086

Merged
merged 1 commit into from
Jun 6, 2022
Merged

Report unsafe for overriding link sections #97086

merged 1 commit into from
Jun 6, 2022

Conversation

5225225
Copy link
Contributor

@5225225 5225225 commented May 16, 2022

I'm not too sure about the lint wording here, but I couldn't think of anything better.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 16, 2022
@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2022
@5225225 5225225 mentioned this pull request May 16, 2022
Closed
10 tasks
@5225225
Copy link
Contributor Author

5225225 commented May 16, 2022

This presumably will require a crater run to see how many things this breaks. I'd be surprised if there were that many crates that both forbid unsafe_code explicitly and set a link_section.

@davidtwco
Copy link
Member

A check-only crater run is sufficient for this, right?

@5225225
Copy link
Contributor Author

5225225 commented May 17, 2022

Yep, just running cargo check causes compilation to fail if this lint is triggered.

@davidtwco

This comment was marked as outdated.

Sign in to view
@craterbot

This comment was marked as outdated.

Sign in to view
@davidtwco
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented May 18, 2022

⌛ Trying commit a42a7a3 with merge 89681135fd92d7dee32c3e562b052ee82f97008c...

@bors
Copy link
Contributor

bors commented May 18, 2022

☀️ Try build successful - checks-actions
Build commit: 89681135fd92d7dee32c3e562b052ee82f97008c (89681135fd92d7dee32c3e562b052ee82f97008c)

@davidtwco
Copy link
Member

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-97086 created and queued.
🤖 Automatically detected try build 89681135fd92d7dee32c3e562b052ee82f97008c
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2022
@craterbot
Copy link
Collaborator

🚧 Experiment pr-97086 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-97086 is completed!
📊 10 regressed and 4 fixed (234770 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 26, 2022
@5225225
Copy link
Contributor Author

5225225 commented Jun 1, 2022

Got back home and got a chance to look at these in detail, and yeah, they're all unrelated random failures, no crate actually overrides a link section and forbids unsafe code.

No idea what is up with https://crater-reports.s3.amazonaws.com/pr-97086/try%2389681135fd92d7dee32c3e562b052ee82f97008c/gh/jackson1992121.solana-messengerapp/log.txt failing to link, but it looks unrelated? Not sure why that'd be getting killed with a -9.

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 6, 2022

📌 Commit a42a7a3 has been approved by davidtwco

@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 6, 2022
@bors
Copy link
Contributor

bors commented Jun 6, 2022

⌛ Testing commit a42a7a3 with merge 79b6bad...

@bors
Copy link
Contributor

bors commented Jun 6, 2022

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 79b6bad to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 6, 2022
@bors bors merged commit 79b6bad into rust-lang:master Jun 6, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 6, 2022
@5225225 5225225 deleted the link-section-is-unsafe branch June 6, 2022 13:33
@therealprof
Copy link
Contributor

Overriding the link section is very typical for embedded code (which I can't imagine being part of the crate run). Not sure how common it is to forbid unsafe code though...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (79b6bad): comparison url.

Instruction count

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

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -2.3% -2.3% 1
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -2.8% -2.8% 1
All 😿🎉 (primary) N/A N/A 0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2

  2. number of relevant changes ↩ ↩2

@Lokathor
Copy link
Contributor

@therealprof I've got embedded code which overrides the link section on things and which is in public repos (so i assume crater sees it and builds it). I'm definitely not the only one.

@therealprof
Copy link
Contributor

@therealprof I've got embedded code which overrides the link section on things and which is in public repos (so i assume crater sees it and builds it). I'm definitely not the only one.

I thought crater would only pick up crates which can be built natively and tested on the builders...

@Lokathor
Copy link
Contributor

In my case (GBA) a cargo check will work with just Nightly and build-std and no special non-rust tools. And if I were using a newer target even a full build will would work (GBA is too old to work with llvm's default linker).

I don't really know how crater works, but I hope it's able to try a cargo check on its own.

@melvyn2 melvyn2 mentioned this pull request May 18, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

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
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.
9 participants
@5225225 @rust-highfive @davidtwco @craterbot @bors @therealprof @rust-timer @Lokathor @rustbot