-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Stabilize -Zno-jump-tables into -Cjump-tables=bool #145974
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
base: master
Are you sure you want to change the base?
Conversation
Both gcc and llvm accept -fjump-tables as well as -fno-jump-tables. For consistency, allow rustc to accept -Zjump-tables=yes too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question [GUARANTEE 1/3]: is this intended to be a hint, or a guarantee?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question:
This option enables the -fno-jump-tables flag for LLVM, which makes the codegen backend avoid generating jump tables when lowering switches.
This option adds the LLVM no-jump-tables=true attribute to every function.
The option can be used to help provide protection against jump-oriented-programming (JOP) attacks, such as with the linux kernel's IBT.
How does this interact with pre-compiled std? I.e. can you mix downstream user crates compiled with -Cjump-tables=no
versus a pre-compiled std compiled and distributed with -Cjump-tables=yes
?
Disabling jump tables can be used to help provide protection against | ||
jump-oriented-programming (JOP) attacks, such as with the linux kernel's [IBT]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion [GUARANTEE 3/3]: If the flag is intended to be a hint, then this sentence can be a bit misleading, because we may not always guarantee it. We may want to slightly caveat this wording to not convey a "false promise" so to speak.
Or, if a user do want such protection, then do they need to enforce it over the whole crate graph?
This option is used to allow or prevent the codegen backend from creating | ||
jump tables when lowering switches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion: Hm, what happens if a different cg backend is selected?
This option is used to allow or prevent the codegen backend from creating | ||
jump tables when lowering switches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question [GUARANTEE 2/3]: This wording reads like a guarantee -- but is it? Can we make that guarantee?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: I'm assuming that if you inspect the assembly of an actual hello world binary that uses std in some way, then you might see jump stables still? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there objections renaming -Zno-jump-tables to -Zjump-tables=?
Not having double-negatives is very nice 👍
Is it desirable to keep
-Zno-jump-tables
for a period of time?
No, I rather not have to keep around an unstable flag as an alias to a stable flag, that's very weird.
Thanks for this!
Yeah, it may get used for other things. Apart from what I mentioned in the original PR, I see LoongArch also uses it since a year ago (so I assume it should be passed for Rust too there, Cc @chenhuacai in case there is a reason not to).
I assume you mean As for the name change, it seems fine -- the usual argument for using the current name is to keep it close to GCC's and Clang's flags, which always helps, but here it is obvious, i.e. we are not changing other parts of the name or grouping different flags into a new one or things like that.
I think it is fine either way for us. If the old flag isn't there, we may get a kernel build error here in this PR, in which case I can give you a commit to fix it. |
I propose stabilizing the -Zno-jump-tables option into -Cjump-tables=.
-Zno-jump-tables
stabilization reportWhat is the RFC for this feature and what changes have occurred to the user-facing design since the RFC was finalized?
No RFC was created for this option. This was a narrowly scoped option introduced in #105812 to support code generation requirements of the x86-64 linux kernel, and eventually other targets as Rust For Linux grows.
The tracking is #116592.
What behavior are we committing to that has been controversial? Summarize the major arguments pro/con.
The behavior of this flag is well defined, and mimics the existing
-fno-jump-tables
option currently available with LLVM and GCC.As introduced, this option was named
-Zno-jump-tables
. However, other major toolchains allow both positive and negative variants of this option to toggle this feature. Renaming the option to-Cjump-tables=<bool>
makes this option consistent, and if for some reason, expandable to other arguments in the future. Notably, many LLVM targets have a configurable and different thresholds for when to lower into a jump table.Are there extensions to this feature that remain unstable? How do we know that we are not accidentally committing to those.
No. This option is used exclusively to gate a very specific class of optimization.
Summarize the major parts of the implementation and provide links into the code (or to PRs)
-Zno-jump-tables
#105812 by @ojedahttps://github.com/pmur/rust/blob/68bfda9025ccb2778e2606e12e8021b9918f40d3/compiler/rustc_session/src/options.rs#L2025-L2026
https://github.com/pmur/rust/blob/68bfda9025ccb2778e2606e12e8021b9918f40d3/compiler/rustc_codegen_llvm/src/attributes.rs#L210-L215
https://github.com/pmur/rust/blob/68bfda9025ccb2778e2606e12e8021b9918f40d3/src/doc/rustc/src/codegen-options/index.md?plain=1#L212-L223
Has a call-for-testing period been conducted? If so, what feedback was received?
No. The option has originally created is being used by Rust For Linux to build the x86-64 kernel without issue.
What outstanding bugs in the issue tracker involve this feature? Are they stabilization-blocking?
There are no outstanding issues.
Summarize contributors to the feature by name for recognition and assuredness that people involved in the feature agree with stabilization
-Zno-jump-tables
.no-jump-tables
#116592, and provided feedback about the naming of the cli option.What FIXMEs are still in the code for that feature and why is it ok to leave them there?
There are none.
What static checks are done that are needed to prevent undefined behavior?
This option cannot cause undefined behavior. It is a boolean option with well defined behavior in both cases.
In what way does this feature interact with the reference/specification, and are those edits prepared?
This adds a new cli option to
rustc
. The documentation is updated, and the unstable documentation cleaned up in this PR.Does this feature introduce new expressions and can they produce temporaries? What are the lifetimes of those temporaries?
No.
What other unstable features may be exposed by this feature?
None.
What is tooling support like for this feature, w.r.t rustdoc, clippy, rust-analzyer, rustfmt, etc.?
No support is required from other rust tooling.
Open Items
-Zno-jump-tables
to-Zjump-tables=<bool>
?-Zno-jump-tables
for a period of time?Closes #116592