sess: -Zbranch-protection is a target modifier#152909
sess: -Zbranch-protection is a target modifier#152909rust-bors[bot] merged 1 commit intorust-lang:mainfrom
-Zbranch-protection is a target modifier#152909Conversation
rustbot
added
S-waiting-on-review
|
r? @jackh726 rustbot has assigned @jackh726. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
I don't see a reason why we shouldn't compile the standard library with branch protection enabled to give users the ability to enable branch protection for their own executable without recompiling the standard library. There is no ABI constraint that prevents mixing. In fact hardware cfi extensions tend to use reuse instructions that were nops when cfi is disabled. An option would be to deny depending on libraries compiled without branch protection from a crate with it enabled, but not the other way around. Just like we do for mixing panic=abort and panic=unwind. |
`-Zbranch-protection` only makes sense if the entire crate graph has the option set, otherwise the security properties that branch protection provides won't be effective. This flag is unstable so I don't think this warrants an MCP.
I'm not convinced, I think think this flag is better as a target modifier because you don't get the security properties of branch protection unless the entire crate graph has it, so turning it on for some crates gives a false sense of security. I think it's better to have consistency with how target modifiers behave and have only the "flags that must match across crate graph" and "flags that can be different across crate graph" behaviours that users learn than expand the additional third category that As a target modifier, it's hard to compile the standard library with branch protection unless we also change the default to enable branch protection, which is another discussion. |
davidtwco
force-pushed
the
branch-protection-target-modifier
branch
from
e76f989 to
cca2865
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
My 2 cents, I think it makes sense to have this be something to some crates in a tree can set without other crates setting it, and in this sense, setting this as a target modifier doesn't quite make sense. That being said, I also buy the reasoning that you most often likely want to enable it for the whole crate graph. Unfortunately, we don't have something like with lints, where we can say "enable this for my crate only" vs "enable this for the entire crate graph" (and you would want the "obvious"/"easy" option to be enable for the entire crate graph). This is probably something we should think about more generally. I'm going to r+ this, as it is unstable. But @davidtwco can you update the tracking issue to reflect this unresolved question? @bors r+ |
rust-bors
bot
added
S-waiting-on-bors
…-modifier, r=jackh726 sess: `-Zbranch-protection` is a target modifier `-Zbranch-protection` only makes sense if the entire crate graph has the option set, otherwise the security properties that branch protection provides won't be effective - hence a target modifier. This flag is unstable so I don't think this warrants an MCP.
…-modifier, r=jackh726 sess: `-Zbranch-protection` is a target modifier `-Zbranch-protection` only makes sense if the entire crate graph has the option set, otherwise the security properties that branch protection provides won't be effective - hence a target modifier. This flag is unstable so I don't think this warrants an MCP.
…-modifier, r=jackh726 sess: `-Zbranch-protection` is a target modifier `-Zbranch-protection` only makes sense if the entire crate graph has the option set, otherwise the security properties that branch protection provides won't be effective - hence a target modifier. This flag is unstable so I don't think this warrants an MCP.
…-modifier, r=jackh726 sess: `-Zbranch-protection` is a target modifier `-Zbranch-protection` only makes sense if the entire crate graph has the option set, otherwise the security properties that branch protection provides won't be effective - hence a target modifier. This flag is unstable so I don't think this warrants an MCP.
…uwer Rollup of 12 pull requests Successful merges: - #152909 (sess: `-Zbranch-protection` is a target modifier) - #153556 (`impl` restriction lowering) - #154048 (Don't emit rustdoc `missing_doc_code_examples` lint on impl items) - #153992 (bootstrap: Optionally print a backtrace if a command fails) - #154019 (two smaller feature cleanups) - #154059 (tests: Activate `must_not_suspend` test for `MutexGuard` dropped before `await`) - #154075 (Rewrite `query_ensure_result`.) - #154082 (Updates derive_where and removes workaround) - #154084 (Preserve braces around `self` in use tree pretty printing) - #154086 (Insert space after float literal ending with `.` in pretty printer) - #154087 (Fix whitespace after fragment specifiers in macro pretty printing) - #154109 (tests: Add regression test for async closures involving HRTBs)
Rollup of 15 pull requests Successful merges: - #152909 (sess: `-Zbranch-protection` is a target modifier) - #153556 (`impl` restriction lowering) - #154048 (Don't emit rustdoc `missing_doc_code_examples` lint on impl items) - #150935 (Introduce #[diagnostic::on_move(message)]) - #152973 (remove -Csoft-float) - #153862 (Rename `cycle_check` to `find_cycle`) - #153992 (bootstrap: Optionally print a backtrace if a command fails) - #154019 (two smaller feature cleanups) - #154059 (tests: Activate `must_not_suspend` test for `MutexGuard` dropped before `await`) - #154075 (Rewrite `query_ensure_result`.) - #154082 (Updates derive_where and removes workaround) - #154084 (Preserve braces around `self` in use tree pretty printing) - #154086 (Insert space after float literal ending with `.` in pretty printer) - #154087 (Fix whitespace after fragment specifiers in macro pretty printing) - #154109 (tests: Add regression test for async closures involving HRTBs)
5 participants
-Zbranch-protectiononly makes sense if the entire crate graph has the option set, otherwise the security properties that branch protection provides won't be effective - hence a target modifier. This flag is unstable so I don't think this warrants an MCP.