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

x86-retpoline flag (target modifier) to enable retpoline-related target features #135927

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

azhogin
Copy link
Contributor

@azhogin azhogin commented Jan 23, 2025

-Zx86-retpoline flag is target modifier (tracked to be equal in linked crates). And so, this PR is dependent from #133138.
Enables target features:
+retpoline-external-thunk, +retpoline-indirect-branches, +retpoline-indirect-calls.

Also this PR forbids to specify those target features manually (warning).

@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2025

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 23, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2025

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

r? compiler

@bors
Copy link
Contributor

bors commented Jan 26, 2025

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

@azhogin azhogin marked this pull request as draft January 30, 2025 08:48
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@chenyukang
Copy link
Member

r? compiler

@rustbot rustbot assigned fee1-dead and unassigned chenyukang Feb 5, 2025
@rust-log-analyzer

This comment has been minimized.

@fee1-dead
Copy link
Member

r? compiler

@rustbot rustbot assigned lcnr and unassigned fee1-dead Feb 8, 2025
@lcnr
Copy link
Contributor

lcnr commented Feb 9, 2025

r? @davidtwco maybe 😅

@Darksonn
Copy link
Contributor

I don't think retpoline actually changes the ABI, so it might not need to be a target modifier. On the other hand, as an exploit mitigation, we do still want to avoid cases where you don't enable the mitigation in every compilation unit.

cc @andyhhp do I understand things correctly?

@azhogin
Copy link
Contributor Author

azhogin commented Feb 13, 2025

I don't think retpoline actually changes the ABI, so it might not need to be a target modifier. On the other hand, as an exploit mitigation, we do still want to avoid cases where you don't enable the mitigation in every compilation unit.

I read your post in #116852:

I've investigated this issue in more details. It turns out that the retpoline mitigations do not affect ABI and can be mixed.

Mixing them gives up their advantage, though. Linking together two compilation units that disagree on whether to apply the mitigation may make both vulnerable to what they attempt to mitigate.

That's why I thought it should be a target modifier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants