-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Stabilize -Cmin-function-alignment
#142824
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -811,7 +811,7 @@ mod desc { | |
pub(crate) const parse_wasm_c_abi: &str = "`spec`"; | ||
pub(crate) const parse_mir_include_spans: &str = | ||
"either a boolean (`yes`, `no`, `on`, `off`, etc), or `nll` (default: `nll`)"; | ||
pub(crate) const parse_align: &str = "a number that is a power of 2 between 1 and 2^29"; | ||
pub(crate) const parse_align: &str = "a number that is a power of 2 between 1 and 8192"; | ||
} | ||
|
||
pub mod parse { | ||
|
@@ -1925,6 +1925,12 @@ pub mod parse { | |
return false; | ||
} | ||
|
||
// Limit the alignment to 8192 (i.e. 0x2000, or 1 << 13) bytes. It is the highest function | ||
// alignment that works on all target platforms. COFF does not support higher alignments. | ||
if bytes > 8192 { | ||
return false; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why should it be limited on all platforms? Can't we error when the alignment exceeds the maximum that the actual target we are compiling for supports? Maybe someone genuinely needs to align to 16k on ELF for whatever reason? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, so as we've discovered, alignment is just a mess between clang and llvm. For instance For 8192, we know it'll work everywhere, and the logic for when to accept/reject an alignment value is clear. This flag aligns all functions to the minimum, so I have a hard time seeing a realistic scenario where aligning all functions to 16k is a reasonable thing to do (the limit on individual functions will be handled separately). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I think the options are:
I've picked the most conservative one (again, with the option to relax the limits if the need ever arises), but if there is consensus now on one of the other options that's also fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is my understanding that on Linux kernels we still nominally support, overaligning beyond the page size, typically 4096, is not going to work properly, so it is arguable that even 8192 is too high. |
||
|
||
let Ok(align) = Align::from_bytes(bytes) else { | ||
return false; | ||
}; | ||
|
@@ -2014,6 +2020,8 @@ options! { | |
"perform LLVM link-time optimizations"), | ||
metadata: Vec<String> = (Vec::new(), parse_list, [TRACKED], | ||
"metadata to mangle symbol names with"), | ||
min_function_alignment: Option<Align> = (None, parse_align, [TRACKED], | ||
"align all functions to at least this many bytes. Must be a power of 2"), | ||
no_prepopulate_passes: bool = (false, parse_no_value, [TRACKED], | ||
"give an empty list of passes to the pass manager"), | ||
no_redzone: Option<bool> = (None, parse_opt_bool, [TRACKED], | ||
|
@@ -2327,8 +2335,6 @@ options! { | |
"gather metadata statistics (default: no)"), | ||
metrics_dir: Option<PathBuf> = (None, parse_opt_pathbuf, [UNTRACKED], | ||
"the directory metrics emitted by rustc are dumped into (implicitly enables default set of metrics)"), | ||
min_function_alignment: Option<Align> = (None, parse_align, [TRACKED], | ||
"align all functions to at least this many bytes. Must be a power of 2"), | ||
mir_emit_retag: bool = (false, parse_bool, [TRACKED], | ||
"emit Retagging MIR statements, interpreted e.g., by miri; implies -Zmir-opt-level=0 \ | ||
(default: no)"), | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
error: incorrect value `3` for codegen option `min-function-alignment` - a number that is a power of 2 between 1 and 8192 was expected | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
//@ revisions: too-high not-power-of-2 | ||
// | ||
//@ [too-high] compile-flags: -Cmin-function-alignment=16384 | ||
//@ [not-power-of-2] compile-flags: -Cmin-function-alignment=3 | ||
|
||
//~? ERROR a number that is a power of 2 between 1 and 8192 was expected |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
error: incorrect value `16384` for codegen option `min-function-alignment` - a number that is a power of 2 between 1 and 8192 was expected | ||
|
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.
(orthogonal to this PR)
it is not great that the logic for merging the per-fn alignment and the global alignment needs to be repeated in each backend.
Maybe
codegen_fn_attrs
should just takemin_function_alignment
into account?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.
In fact, using
sess.opts
in the interpreter is at best iffy since it means the interpreter can behave differently in different crates in the same crate graph which can cause unsoundness...