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

add safe compilation options #117966

Merged
merged 1 commit into from Dec 10, 2023
Merged

add safe compilation options #117966

merged 1 commit into from Dec 10, 2023

Conversation

lxy19980601
Copy link
Contributor

Add two options when building rustc : strip and stack protector.
If set strip = true, rustc will be stripped of symbols using -Cstrip=symbols.
Also can set stack-protector and then rustc will be compiled with stack protectors.

@rustbot
Copy link
Collaborator

rustbot commented Nov 16, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @onur-ozkan (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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Nov 16, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 16, 2023

This PR modifies src/bootstrap/src/core/config. If appropriate, please also update CONFIG_CHANGE_HISTORY in src/bootstrap/src/lib.rs and change-id in config.example.toml.

This PR changes config.example.toml. If appropriate, please also update CONFIG_CHANGE_HISTORY in src/bootstrap/src/lib.rs and change-id in config.example.toml.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@davidtwco
Copy link
Member

Some context: these options are useful to us at Huawei to imply with some internal requirements, if we can configure them via config.toml then that saves us having to maintain an (admittedly small) patch.

Mark-Simulacrum
Mark-Simulacrum reviewed Nov 17, 2023
View reviewed changes
config.example.toml Outdated Show resolved Hide resolved
config.example.toml
@@ -603,6 +603,13 @@ change-id = 116881
# desired in distributions, for example.
#rpath = true

# Indicates whether `rustc` should be stripped of symbols using `-Cstrip=symbols`.
#strip = false
Copy link
Member

Choose a reason for hiding this comment

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

When is this necessary, if debuginfo is not enabled in the first place? Do we really need another option here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-Cstrip=symbols will strip all symbols, not only the debuginfo. The artifacts will be smaller and have fewer symbols if we use strip = true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I compare the size of binary artifacts, it will be smaller if use -Cstrip=symbols compare to not using. For example, the size of librustc_driver.so is 194.5MB without using -Cstrip=symbols, but 141.3MB when using -Cstrip=symbols.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks!

config.example.toml Show resolved Hide resolved
src/bootstrap/src/core/builder.rs Outdated Show resolved Hide resolved
src/bootstrap/src/core/config/config.rs Outdated
"none" => Ok(StackProtector::None),
"basic" => Ok(StackProtector::Basic),
"strong" => Ok(StackProtector::Strong),
"all" => Ok(StackProtector::All),
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to avoid duplicating the names of the values for this unstable option here from rustc. Can we just pass this through as an opaque string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified according to comments.

@Mark-Simulacrum
Copy link
Member

@rustbot author

@rustbot 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 Nov 18, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lxy19980601
Copy link
Contributor Author

@Mark-Simulacrum

@Mark-Simulacrum Mark-Simulacrum 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 Nov 21, 2023
davidtwco
davidtwco reviewed Dec 4, 2023
View reviewed changes
config.example.toml Outdated
Comment on lines 606 to 611
# Indicates whether stack protectors should be used
# via the unstable option `-Zstack-protector`.
# Valid options are : `none`(default),`basic`,`strong`, or `all`.
# But `strong` and `basic` options may be buggy and are not suggested here.(rust-lang/rust #114903)
#stack-protector = "none"
Copy link
Member

@davidtwco davidtwco Dec 4, 2023 •
edited

Choose a reason for hiding this comment

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

Suggested change
# Indicates whether stack protectors should be used
# via the unstable option `-Zstack-protector`.
# Valid options are : `none`(default),`basic`,`strong`, or `all`.
# But `strong` and `basic` options may be buggy and are not suggested here.(rust-lang/rust #114903)
#stack-protector = "none"
# Indicates whether stack protectors should be used
# via the unstable option `-Zstack-protector`.
#
# Valid options are: `none`(default), `basic`, `strong`, or `all`.
# `strong` and `basic` may be buggy and are not recommended, see rust-lang/rust#114903.
#stack-protector = "none"

davidtwco
davidtwco reviewed Dec 4, 2023
View reviewed changes
src/bootstrap/src/core/builder.rs Outdated
Comment on lines 1674 to 1680
match self.config.rust_stack_protector.as_str() {
"none" => rustflags.arg("-Zstack-protector=none"),
"basic" => rustflags.arg("-Zstack-protector=basic"),
"strong" => rustflags.arg("-Zstack-protector=strong"),
"all" => rustflags.arg("-Zstack-protector=all"),
other => unreachable!("{:?} is not recognized as valid stack protectors", other),
};
Copy link
Member

Choose a reason for hiding this comment

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

Something like this might have been more what Mark had in mind, assuming that rust_stack_protector is made an Option.

Suggested change
match self.config.rust_stack_protector.as_str() {
"none" => rustflags.arg("-Zstack-protector=none"),
"basic" => rustflags.arg("-Zstack-protector=basic"),
"strong" => rustflags.arg("-Zstack-protector=strong"),
"all" => rustflags.arg("-Zstack-protector=all"),
other => unreachable!("{:?} is not recognized as valid stack protectors", other),
};
if let Some(stack_protector) = self.config.rust_stack_protector {
rustflags.arg(format!("-Zstack-protector={stack_protector}")));
}

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lxy19980601
Add safe compilation options
3f8487a 
Add two options when building rust: strip and stack protector.
If set `strip = true`, symbols will be stripped using `-Cstrip=symbols`.
Also can set `stack-protector` and stack protectors will be used.
@lxy19980601
Copy link
Contributor Author

@Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

This is in my queue to review, I just haven't had the chance to do so yet.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 9, 2023

📌 Commit 3f8487a has been approved by Mark-Simulacrum

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 Dec 9, 2023
@GuillaumeGomez GuillaumeGomez mentioned this pull request Dec 9, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2023
@bors
Auto merge of rust-lang#118787 - GuillaumeGomez:rollup-fj5wr3q, r=Gui…
e845469 
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#117966 (add safe compilation options)
 - rust-lang#118747 (Remove extra check cfg handled by libc directly)
 - rust-lang#118774 (add test for inductive cycle hangs)
 - rust-lang#118775 (chore: add test case for type with generic)
 - rust-lang#118782 (use `&` instead of start-process in x.ps1)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 10, 2023
@bors
Auto merge of rust-lang#118787 - GuillaumeGomez:rollup-fj5wr3q, r=Gui…
f7253f2 
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#117966 (add safe compilation options)
 - rust-lang#118747 (Remove extra check cfg handled by libc directly)
 - rust-lang#118774 (add test for inductive cycle hangs)
 - rust-lang#118775 (chore: add test case for type with generic)
 - rust-lang#118782 (use `&` instead of start-process in x.ps1)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 83e814f into rust-lang:master Dec 10, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 10, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 10, 2023
@rust-timer
Unrolled build for rust-lang#117966
2efa930 
Rollup merge of rust-lang#117966 - lxy19980601:safe_compilation_options, r=Mark-Simulacrum

add safe compilation options

Add two options when building rustc : strip and stack protector.
If set `strip = true`, `rustc` will be stripped of symbols using `-Cstrip=symbols`.
Also can set `stack-protector` and then `rustc` will be compiled with stack protectors.
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

@Mark-Simulacrum Mark-Simulacrum Awaiting requested review from Mark-Simulacrum

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@lxy19980601 @rustbot @rust-log-analyzer @davidtwco @Mark-Simulacrum @bors @onur-ozkan