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

Cleanup and document -C relocation-model #71490

Merged
merged 5 commits into from
Apr 26, 2020
Merged

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Apr 23, 2020

As the title says, this is mostly a refactoring and documentation.

One potentially observable change here is that -C relocation-model=default now takes the default from the Rust target, rather than from the underlying LLVM target. In other words, -C relocation-model=default is now equivalent to not specifying the relocation model on command line at all.
Apparently no one used that option because it has other bugs as well, e.g. PIC default wasn't treated as PIC in some places.

@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 Apr 23, 2020
@Amanieu
Copy link
Member

Amanieu commented Apr 23, 2020

Could we add some validation to check that a relocation model is valid for a particular target? For example ropi, rwpi, ropi-rwpi are only supported on ARM targets (clang actually validates this before passing it on to LLVM). Also dynamic-no-pic is only support on Darwin targets.

Question: should the choice of relocation model have an impact on the TLS model? For example, if a crate is only going to be used in an executable and not in a shared library then we can use the local-exec TLS model.

@rust-highfive

This comment has been minimized.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Apr 25, 2020

@Amanieu

Could we add some validation to check that a relocation model is valid for a particular target? For example ropi, rwpi, ropi-rwpi are only supported on ARM targets (clang actually validates this before passing it on to LLVM). Also dynamic-no-pic is only support on Darwin targets.

Yeah, it would be nice to warn about this, if not error.
Right now rustc behaves more like gcc which silently accepts conflicting options and produces something unspecified, than like clang which tries to produce warnings.
(E.g. in gcc -shared -pie -pie silently wins, but in clang -shared -pie -shared wins with a warning.)

I'm not sure we can emit errors here due to compatibility.
Apparently there's some folklore about dynamic-no-pic being the way to disable -pie, for example.
The special models usually turn into static when enabled for inappropriate targets, so nothing fatal happens in that case.

I've made an issue about this - #71552.

(EDIT: I'll look at the TLS model question a bit later.)

@Amanieu
Copy link
Member

Amanieu commented Apr 25, 2020

I'm more concerned that we are passing relocation models to LLVM on targets that do not support them. These code paths are untested since clang doesn't do this, which is likely to lead to LLVM assert failures down the line.

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Implementation LGTM, r=me after discussion about validation is resolved.

@petrochenkov
Copy link
Contributor Author

@Amanieu

Question: should the choice of relocation model have an impact on the TLS model? For example, if a crate is only going to be used in an executable and not in a shared library then we can use the local-exec TLS model.

Looks like LLVM does that automatically, see TargetMachine::getTLSModel, it's enough for us to set the PIE level (which we do).
User (e.g. rustc with -Z tls-model) can only choose a more relaxed model than LLVM does, e.g. local-exec in a shared library, or something like that.

https://github.com/rust-lang/llvm-project/blob/rustc/10.0-2020-02-05/llvm/lib/Target/TargetMachine.cpp#L210-L235

It's not yet clear to me what we need to do for LLVM to set IsLocal to true and relax global-dynamic/initial-exec to local-dynamic/local-exec.

@petrochenkov
Copy link
Contributor Author

I'm more concerned that we are passing relocation models to LLVM on targets that do not support them. These code paths are untested since clang doesn't do this, which is likely to lead to LLVM assert failures down the line.

I don't disagree, I just think it's better done in a separate PR.

The referenced `sanitizer-address/Makefile` no longer exists, so perhaps these options are no longer necessary as well.
Even if they are still necessary, they should use `-C relocation-model=static` instead.
@petrochenkov
Copy link
Contributor Author

@bors r=davidtwco

@bors
Copy link
Contributor

bors commented Apr 26, 2020

📌 Commit 45fbe8f 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 Apr 26, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 26, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#71490 (Cleanup and document `-C relocation-model`)
 - rust-lang#71562 (fix more clippy warnings)
 - rust-lang#71571 (Fix since attribute for nonzero_bitor impl's)
 - rust-lang#71574 (proc_macro: Fix since attributes for new Span methods)
 - rust-lang#71575 (Fix stable(since) attribute for BTreeMap::remove_entry)

Failed merges:

r? @ghost
@bors bors merged commit 96c1bb5 into rust-lang:master Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants