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 relro-level tests #48388

Merged
merged 3 commits into from
Mar 10, 2018
Merged

Add relro-level tests #48388

merged 3 commits into from
Mar 10, 2018

Conversation

kyrias
Copy link
Contributor

@kyrias kyrias commented Feb 20, 2018

The relro-level debugging flag was added in #43170 which was merged in July 2017. This PR moves this flag to be a proper codegen flag.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(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 Feb 20, 2018
@nikomatsakis
Copy link
Contributor

This is basically a stabilization. Is there a tracking issue for this feature?

@nikomatsakis nikomatsakis added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 21, 2018
@nikomatsakis
Copy link
Contributor

I know nothing about this, so it's hard for me to judge if we are happy enough with its implementation to be ready to stabilize. =)

cc @rust-lang/compiler

@kyrias
Copy link
Contributor Author

kyrias commented Feb 21, 2018 •
edited
Loading

No tracking issue. This is essentially just a flag to be able to disable RELRO, or use only partial RELRO. Full RELRO has been on by default as of #43170 being merged, but @alexcrichton wanted the flag to disable it to stay unstable for a while at the time.

@nikomatsakis
Copy link
Contributor

Ok. Usually for a stabilization I like to see a little report that shows the test cases that show that this feature is indeed working as expected -- based on the the diffs in this PR, I take it that there are no tests at all for this (i.e., nothing had to be changed from -Z to -C). Do you think you'd be up for adding one?

@kyrias
Copy link
Contributor Author

kyrias commented Feb 21, 2018

Not really sure how to test this from rust honestly. To actually check that it is functioning properly we'd essentially need to actually do a compile and then parse the ELF binary and check for the GNU_RELRO header and BIND_NOW sections. Not sure if there's any nice way to do that though?

@nikomatsakis
Copy link
Contributor

@kyrias usually we would make a run-make test for that sort of thing

@nikomatsakis
Copy link
Contributor

Maybe it's not worth the trouble, I don't know, but I am a big believer in "if it ain't tested, it's broken".

@kyrias
Copy link
Contributor Author

kyrias commented Feb 22, 2018

Interesting, didn't know about run-make tests, I'll give it a shot. (Is there any good docs for getting more details about the different parts of the compiler tests, and how to write the different types of tests.)

Is there any specific list of dependencies that are guaranteed to exist when they're run?

@kyrias
Copy link
Contributor Author

kyrias commented Feb 22, 2018

So I figured that when you specify relro-level=off you probably want to tell the linker to explicitly not use RELRO rather than just have the compiler not pass on the flag at all, so I added that as well, and then a test for this case.

Since this is a very small change I just added it in this PR, but if you would prefer I can split it out and file a PR for it separately.

@bors
Copy link
Contributor

bors commented Feb 25, 2018

☔ The latest upstream changes (presumably #48531) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors 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 Feb 25, 2018
@alexcrichton
Copy link
Member

Thanks @kyrias! For stabilizatin purposes could you also detail a bit what your plans are with the stable flag?

I'm realizing now that we don't have a ton of specific paltform-specific options stabilized in the sense that -C relro doesn't mean much on Windows, but that's not necessarily a problem.

@pietroalbini
Copy link
Member

Hi @kyrias, ping from the release team! @alexcrichton asked you more details about the stabilization in #48388 (comment), can you please reply so we can move forward with this PR?

@kyrias
Copy link
Contributor Author

kyrias commented Mar 6, 2018 •
edited
Loading

Hey, sorry for the delay, been away for a week. Now that I think about it I don't really have any specific plans for it, I just thought that it can be useful to be able to disable it in some rare circumstances. Since the executables end up being slightly bigger, disabling RELRO can be useful for extremely low memory targets. (And BIND_NOW additionally causes some performance impact as it has to resolve all symbols at load time, even if there is a large number of symbols that end up not being used.) But other than that I guess there aren't anything in specific I can think off.

@alexcrichton
Copy link
Member

Ah ok thanks for the info @kyrias! In that case I wonder if it's best left unstable for now? That way it's there to tinker with if necessary but otherwise we may want to hold out as it may inform how we stabilize this.

@kyrias
Copy link
Contributor Author

kyrias commented Mar 6, 2018

Yeah, might make sense until the problems become more real. I think I'm mostly just bothered by the fact that -Z is documented as debug options everywhere, which bothers me since it's not actually a debug option, heh.

Anyway, I could drop the move of the option and just keep the tests and the relro-level=off fix.

@alexcrichton
Copy link
Member

Sounds great to me!

@kyrias kyrias changed the title Move relro-level flag from debugging to codegen Add relro-level tests Mar 6, 2018
kyrias added 2 commits March 6, 2018 23:39
@kyrias
Make relro-level=off explicitly disable RELRO
f0c459e 
Previously relro-level=off would just not tell the linker to use RELRO,
but when you want to disable RELRO you most likely want to entirely
prevent.

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
@kyrias
Add RELRO run-make tests
54b68b6 
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
@kyrias
Make the default relro level be doing nothing at all
1dbce4b 
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 9, 2018

📌 Commit 1dbce4b has been approved by alexcrichton

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 9, 2018
@bors
Copy link
Contributor

bors commented Mar 9, 2018

⌛ Testing commit 1dbce4b with merge 7f6687d357f136197ea8285137bfddb7cd99c231...

@bors
Copy link
Contributor

bors commented Mar 9, 2018

💔 Test failed - status-travis

@bors 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 Mar 9, 2018
@pietroalbini
Copy link
Member

@bors retry #48866

@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 Mar 9, 2018
@pietroalbini pietroalbini mentioned this pull request Mar 9, 2018
Closed
@bors
Copy link
Contributor

bors commented Mar 10, 2018

⌛ Testing commit 1dbce4b with merge 211c155e8eb32ad9e147edf35471e6a2c73c30b5...

@bors
Copy link
Contributor

bors commented Mar 10, 2018

💔 Test failed - status-travis

@bors 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 Mar 10, 2018
@kennytm
Copy link
Member

kennytm commented Mar 10, 2018

@bors retry

Some OpenSSL installation error, should be already fixed by #48901.

@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 Mar 10, 2018
@bors
Copy link
Contributor

bors commented Mar 10, 2018

⌛ Testing commit 1dbce4b with merge 2f0e6a3...

bors added a commit that referenced this pull request Mar 10, 2018
@bors
Auto merge of #48388 - kyrias:relro-level-cg, r=alexcrichton
2f0e6a3 
Add relro-level tests

The `relro-level` debugging flag was added in #43170 which was merged in July 2017.  This PR moves this flag to be a proper codegen flag.
@bors
Copy link
Contributor

bors commented Mar 10, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 2f0e6a3 to master...

@bors bors merged commit 1dbce4b into rust-lang:master Mar 10, 2018
@kyrias kyrias deleted the relro-level-cg branch March 13, 2018 18:37
@davidtwco davidtwco mentioned this pull request Feb 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
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
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kyrias @rust-highfive @nikomatsakis @bors @alexcrichton @pietroalbini @kennytm