Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_cranelift/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ fn build_isa(sess: &Session, jit: bool) -> Arc<dyn TargetIsa + 'static> {

flags_builder.set("enable_llvm_abi_extensions", "true").unwrap();

if let Some(align) = sess.opts.unstable_opts.min_function_alignment {
if let Some(align) = sess.opts.cg.min_function_alignment {
flags_builder
.set("log2_min_function_alignment", &align.bytes().ilog2().to_string())
.unwrap();
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_codegen_llvm/src/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -491,10 +491,10 @@ pub(crate) fn llfn_attrs_from_instance<'ll, 'tcx>(
let allocated_pointer = AttributeKind::AllocatedPointer.create_attr(cx.llcx);
attributes::apply_to_llfn(llfn, AttributePlace::Argument(0), &[allocated_pointer]);
}
// function alignment can be set globally with the `-Zmin-function-alignment=<n>` flag;
// the alignment from a `#[repr(align(<n>))]` is used if it specifies a higher alignment.
// function alignment can be set globally with the `-Cmin-function-alignment=<n>` flag;
// the alignment from a `#[align(<n>)]` is used if it specifies a higher alignment.
if let Some(align) =
Ord::max(cx.tcx.sess.opts.unstable_opts.min_function_alignment, codegen_fn_attrs.alignment)
Ord::max(cx.tcx.sess.opts.cg.min_function_alignment, codegen_fn_attrs.alignment)
{
llvm::set_alignment(llfn, align);
}
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_codegen_ssa/src/mir/naked_asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,10 @@ fn prefix_and_suffix<'tcx>(
let attrs = tcx.codegen_fn_attrs(instance.def_id());
let link_section = attrs.link_section.map(|symbol| symbol.as_str().to_string());

// function alignment can be set globally with the `-Zmin-function-alignment=<n>` flag;
// the alignment from a `#[repr(align(<n>))]` is used if it specifies a higher alignment.
// Function alignment can be set globally with the `-Cmin-function-alignment=<n>` flag;
// the alignment from a `#[align(<n>)]` is used if it specifies a higher alignment.
// if no alignment is specified, an alignment of 4 bytes is used.
let min_function_alignment = tcx.sess.opts.unstable_opts.min_function_alignment;
let min_function_alignment = tcx.sess.opts.cg.min_function_alignment;
let align_bytes =
Ord::max(min_function_alignment, attrs.alignment).map(|a| a.bytes()).unwrap_or(4);

Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_const_eval/src/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -877,10 +877,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
if let Some(fn_val) = self.get_fn_alloc(id) {
let align = match fn_val {
FnVal::Instance(instance) => {
// Function alignment can be set globally with the `-Zmin-function-alignment=<n>` flag;
// the alignment from a `#[repr(align(<n>))]` is used if it specifies a higher alignment.
// Function alignment can be set globally with the `-Cmin-function-alignment=<n>` flag;
// the alignment from a `#[align(<n>)]` is used if it specifies a higher alignment.
let fn_align = self.tcx.codegen_fn_attrs(instance.def_id()).alignment;
let global_align = self.tcx.sess.opts.unstable_opts.min_function_alignment;
let global_align = self.tcx.sess.opts.cg.min_function_alignment;
Copy link
Member

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 take min_function_alignment into account?

Copy link
Member

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...


Ord::max(global_align, fn_align).unwrap_or(Align::ONE)
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_interface/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,7 @@ fn test_codegen_options_tracking_hash() {
tracked!(llvm_args, vec![String::from("1"), String::from("2")]);
tracked!(lto, LtoCli::Fat);
tracked!(metadata, vec![String::from("A"), String::from("B")]);
tracked!(min_function_alignment, Some(Align::EIGHT));
tracked!(no_prepopulate_passes, true);
tracked!(no_redzone, Some(true));
tracked!(no_vectorize_loops, true);
Expand Down Expand Up @@ -818,7 +819,6 @@ fn test_unstable_options_tracking_hash() {
tracked!(location_detail, LocationDetail { file: true, line: false, column: false });
tracked!(maximal_hir_to_mir_coverage, true);
tracked!(merge_functions, Some(MergeFunctions::Disabled));
tracked!(min_function_alignment, Some(Align::EIGHT));
tracked!(mir_emit_retag, true);
tracked!(mir_enable_passes, vec![("DestProp".to_string(), false)]);
tracked!(mir_opt_level, Some(4));
Expand Down
12 changes: 9 additions & 3 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 -falign-function only goes up to 1 << 16, although you can manually align a function to a much higher alignment.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I think the options are:

  1. Limit to 8192 on all platforms, it's consistent and unlikely to cause issues. The limit can be safely raised in the future if a need arises
  2. Follow clang and allow 1 << 16, except when the target object format is COFF, then the limit is 8192.
  3. Accept what llvm accepts: allow 1 << 29, except when the target object format is COFF, then the limit is 8192.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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;
};
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -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)"),
Expand Down
22 changes: 22 additions & 0 deletions src/doc/rustc/src/codegen-options/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,28 @@ opt-level=0`](#opt-level)). That is:

See also [linker-plugin-lto](#linker-plugin-lto) for cross-language LTO.

## min-function-alignment

The `-Cmin-function-alignment=<align>` flag specifies the minimum alignment of functions for which code is generated.
The `align` value must be a power of 2, other values are rejected.

Note that `-Zbuild-std` (or similar) is required to apply this minimum alignment to standard library functions.
By default, these functions come precompiled and their alignments won't respect the `min-function-alignment` flag.

This flag is equivalent to:

- `-fmin-function-alignment` for [GCC](https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-fmin-function-alignment_003dn)
- `-falign-functions` for [Clang](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang1-falign-functions)

The specified alignment is a minimum. A higher alignment can be specified for specific functions by annotating the function with a `#[align(<align>)]` attribute.
The attribute's value is ignored when it is lower than the value passed to `min-function-alignment`.

There are two additional edge cases for this flag:

- targets have a minimum alignment for functions (e.g. on x86_64 the lowest that LLVM generates is 16 bytes).
A `min-function-alignment` value lower than the target's minimum has no effect.
- the maximum alignment supported by this flag is `8192`. Trying to set a higher value results in an error.

## metadata

This option allows you to control the metadata used for symbol mangling. This
Expand Down
24 changes: 0 additions & 24 deletions src/doc/unstable-book/src/compiler-flags/min-function-alignment.md

This file was deleted.

2 changes: 1 addition & 1 deletion src/tools/miri/tests/pass/fn_align.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//@compile-flags: -Zmin-function-alignment=8
//@compile-flags: -Cmin-function-alignment=8
#![feature(fn_align)]

// When a function uses `align(N)`, the function address should be a multiple of `N`.
Expand Down
6 changes: 3 additions & 3 deletions tests/codegen/min-function-alignment.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//@ revisions: align16 align1024
//@ compile-flags: -C no-prepopulate-passes -Z mir-opt-level=0
//@ [align16] compile-flags: -Zmin-function-alignment=16
//@ [align1024] compile-flags: -Zmin-function-alignment=1024
//@ [align16] compile-flags: -Cmin-function-alignment=16
//@ [align1024] compile-flags: -Cmin-function-alignment=1024

#![crate_type = "lib"]
#![feature(fn_align)]
Expand Down Expand Up @@ -33,7 +33,7 @@ pub fn higher_align() {}
// cold functions follow the same rules as other functions
//
// in GCC, the `-falign-functions` does not apply to cold functions, but
// `-Zmin-function-alignment` applies to all functions.
// `-Cmin-function-alignment` applies to all functions.
//
// CHECK-LABEL: @no_explicit_align_cold
// align16: align 16
Expand Down
4 changes: 2 additions & 2 deletions tests/codegen/naked-fn/min-function-alignment.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//@ compile-flags: -C no-prepopulate-passes -Copt-level=0 -Zmin-function-alignment=16
//@ compile-flags: -C no-prepopulate-passes -Copt-level=0 -Cmin-function-alignment=16
//@ needs-asm-support
//@ ignore-arm no "ret" mnemonic

Expand Down Expand Up @@ -33,7 +33,7 @@ pub extern "C" fn naked_higher_align() {
// cold functions follow the same rules as other functions
//
// in GCC, the `-falign-functions` does not apply to cold functions, but
// `-Zmin-function-alignment` applies to all functions.
// `-Cmin-function-alignment` applies to all functions.
//
// CHECK: .balign 16
#[no_mangle]
Expand Down
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

6 changes: 6 additions & 0 deletions tests/ui/invalid-compile-flags/min-function-alignment.rs
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

Loading