rustc_session: default to -Z plt=yes on non-x86_64 #109982

durin42 · 2023-04-05T19:55:08Z

Per the discussion in #106380 plt=no isn't a great default, and rust-lang/compiler-team#581 decided that the default should be PLT=yes for everything except x86_64. Not everyone agrees about the x86_64 part of this change, but this at least is an improvement in the state of things without changing the x86_64 situation, so I've attempted making this change in the name of not letting the perfect be the enemy of the good.

Please let me know if I've messed this up somehow - I'm not wholly confident I got this right.

r? @nikic

rustbot · 2023-04-05T19:55:16Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikic (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

nikic · 2023-04-05T20:14:08Z

compiler/rustc_session/src/session.rs

@@ -978,7 +978,7 @@ impl Session {
    pub fn needs_plt(&self) -> bool {
        // Check if the current target usually needs PLT to be enabled.
        // The user can use the command line flag to override it.
-        let needs_plt = self.target.needs_plt;
+        let needs_plt = self.target.arch != "x86_64";


This should still be based on a target property. Just inverted from needs_plt to no_plt_by_default or something.

I did that, but I realized as I did it that this might invalidate existing target json files. Is that something I need to worry about?

(I'm also only about 70% sure I did this right, so please give it careful scrutiny)

apiraino · 2023-05-03T16:27:41Z

Switching to waiting on author to incorporate changes. @durin42 Feel free to request a review with @rustbot ready, thanks!

@rustbot author

Dylan-DPC · 2023-05-22T06:05:26Z

@durin42 any updates on this? thanks

durin42 · 2023-05-22T13:32:43Z

I'll get back to it - I've been out of the office for most of May, but should get back to this in early June.

If someone else wants to take this over, I won't be offended, just have had a busy month. :)

rustbot · 2023-06-08T16:25:32Z

These commits modify compiler targets.
(See the Target Tier Policy.)

durin42 · 2023-06-08T16:35:36Z

@rustbot review

nikic · 2023-06-08T18:45:37Z

@bors r+ rollup=never

bors · 2023-06-08T18:45:39Z

📌 Commit 0d21728597d2f6bbda7d56cbb55e0848917c04b8 has been approved by nikic

It is now in the queue for this repository.

bors · 2023-06-08T20:23:26Z

⌛ Testing commit 0d21728597d2f6bbda7d56cbb55e0848917c04b8 with merge c7b76ef7e61b11a84a7eff0556fe84e383763eaa...

bors · 2023-06-08T21:06:35Z

💔 Test failed - checks-actions

Per the discussion in rust-lang#106380 plt=no isn't a great default, and rust-lang/compiler-team#581 decided that the default should be PLT=yes for everything except x86_64. Not everyone agrees about the x86_64 part of this change, but this at least is an improvement in the state of things without changing the x86_64 situation, so I've attempted making this change in the name of not letting the perfect be the enemy of the good.

tests/codegen/stack-protector.rs

nikic · 2023-06-23T15:21:29Z

@bors r+

bors · 2023-06-23T15:21:31Z

📌 Commit 52d50fb has been approved by nikic

It is now in the queue for this repository.

bors · 2023-06-23T16:50:39Z

⌛ Testing commit 52d50fb with merge c79d6be...

bors · 2023-06-23T19:43:38Z

☀️ Test successful - checks-actions
Approved by: nikic
Pushing c79d6be to master...

rust-timer · 2023-06-23T20:58:32Z

Finished benchmarking commit (c79d6be): 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.3%	[-0.3%, -0.2%]	5
Improvements ✅ (secondary)	-0.5%	[-0.6%, -0.3%]	3
All ❌✅ (primary)	-0.3%	[-0.3%, -0.2%]	5

Max RSS (memory usage)

Results

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.7%, -0.7%]	4
All ❌✅ (primary)	-	-	0

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: 661.369s -> 661.671s (0.05%)

rustbot assigned nikic Apr 5, 2023

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 Apr 5, 2023

nikic reviewed Apr 5, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 3, 2023

durin42 force-pushed the plt-no-x86_64-only branch from d4834e8 to 0d21728 Compare June 8, 2023 16:25

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 8, 2023

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 8, 2023

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 8, 2023

durin42 added 2 commits June 22, 2023 14:29

switch to using a target property to control plt default

34d0cff

durin42 force-pushed the plt-no-x86_64-only branch from 0d21728 to 34d0cff Compare June 22, 2023 19:01

tests: be more permissive on attributes in one test

d94d17c

nikic reviewed Jun 23, 2023

View reviewed changes

tests/codegen/stack-protector.rs Show resolved Hide resolved

tests: be even more permissive on attributes in one test

52d50fb

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 23, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 23, 2023

bors merged commit c79d6be into rust-lang:master Jun 23, 2023

rustbot added this to the 1.72.0 milestone Jun 23, 2023

nikic mentioned this pull request Jun 27, 2023

Default to -Z plt=yes #106380

Closed

durin42 deleted the plt-no-x86_64-only branch July 27, 2023 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc_session: default to -Z plt=yes on non-x86_64 #109982

rustc_session: default to -Z plt=yes on non-x86_64 #109982

durin42 commented Apr 5, 2023

rustbot commented Apr 5, 2023

nikic Apr 5, 2023

durin42 Jun 8, 2023

This comment has been minimized.

apiraino commented May 3, 2023

Dylan-DPC commented May 22, 2023

durin42 commented May 22, 2023

rustbot commented Jun 8, 2023

durin42 commented Jun 8, 2023

nikic commented Jun 8, 2023

bors commented Jun 8, 2023

bors commented Jun 8, 2023

This comment has been minimized.

bors commented Jun 8, 2023

nikic commented Jun 23, 2023

bors commented Jun 23, 2023

bors commented Jun 23, 2023

bors commented Jun 23, 2023

rust-timer commented Jun 23, 2023

rustc_session: default to -Z plt=yes on non-x86_64 #109982

rustc_session: default to -Z plt=yes on non-x86_64 #109982

Conversation

durin42 commented Apr 5, 2023

rustbot commented Apr 5, 2023

nikic Apr 5, 2023

Choose a reason for hiding this comment

durin42 Jun 8, 2023

Choose a reason for hiding this comment

This comment has been minimized.

apiraino commented May 3, 2023

Dylan-DPC commented May 22, 2023

durin42 commented May 22, 2023

rustbot commented Jun 8, 2023

durin42 commented Jun 8, 2023

nikic commented Jun 8, 2023

bors commented Jun 8, 2023

bors commented Jun 8, 2023

This comment has been minimized.

bors commented Jun 8, 2023

nikic commented Jun 23, 2023

bors commented Jun 23, 2023

bors commented Jun 23, 2023

bors commented Jun 23, 2023

rust-timer commented Jun 23, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size