Skip to content

use #[align] attribute for fn_align #142507

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 1 commit 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
3 changes: 3 additions & 0 deletions compiler/rustc_attr_data_structures/src/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ impl Deprecation {
#[derive(Clone, Debug, HashStable_Generic, Encodable, Decodable, PrintAttribute)]
pub enum AttributeKind {
// tidy-alphabetical-start
/// Represents `#[align(N)]`.
Align { align: Align, span: Span },

/// Represents `#[rustc_allow_const_fn_unstable]`.
AllowConstFnUnstable(ThinVec<Symbol>),

Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_attr_parsing/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ attr_parsing_incorrect_repr_format_packed_expect_integer =
attr_parsing_incorrect_repr_format_packed_one_or_zero_arg =
incorrect `repr(packed)` attribute format: `packed` takes exactly one parenthesized argument, or no parentheses at all

attr_parsing_invalid_alignment_value =
invalid alignment value: {$error_part}

attr_parsing_invalid_issue_string =
`issue` must be a non-zero numeric string or "none"
.must_not_be_zero = `issue` must not be "0", use "none" instead
Expand Down
54 changes: 52 additions & 2 deletions compiler/rustc_attr_parsing/src/attributes/repr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use rustc_ast::{IntTy, LitIntType, LitKind, UintTy};
use rustc_attr_data_structures::{AttributeKind, IntType, ReprAttr};
use rustc_span::{DUMMY_SP, Span, Symbol, sym};

use super::{CombineAttributeParser, ConvertFn};
use super::{AcceptMapping, AttributeParser, CombineAttributeParser, ConvertFn, FinalizeContext};
use crate::context::{AcceptContext, Stage};
use crate::parser::{ArgParser, MetaItemListParser, MetaItemParser};
use crate::session_diagnostics;
Expand Down Expand Up @@ -199,7 +199,7 @@ fn parse_repr_align<S: Stage>(
});
}
Align => {
cx.dcx().emit_err(session_diagnostics::IncorrectReprFormatAlignOneArg {
cx.emit_err(session_diagnostics::IncorrectReprFormatAlignOneArg {
span: param_span,
});
}
Expand Down Expand Up @@ -262,3 +262,53 @@ fn parse_alignment(node: &LitKind) -> Result<Align, &'static str> {
Err("not an unsuffixed integer")
}
}

/// Parse #[align(N)].
#[derive(Default)]
pub(crate) struct AlignParser(Option<(Align, Span)>);

impl AlignParser {
const PATH: &'static [Symbol] = &[sym::align];

fn parse<'c, S: Stage>(
&mut self,
cx: &'c mut AcceptContext<'_, '_, S>,
args: &'c ArgParser<'_>,
) {
// The builtin attributes parser already emits an error in this case.
Copy link
Contributor

Choose a reason for hiding this comment

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

by the template logic? I'd quite like to move those checks here at some point, including the templates. For converted attributes I usually have added an exception there to ignore attributes with new-style parsers so that these checks do do something, and all checks happen in one place. If you wouldn't mind, could you do the same for align?

Copy link
Contributor

Choose a reason for hiding this comment

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

With #138165 you don't need to make a new diagnostic for this either which should make your life easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by the template logic?

Exactly

If you wouldn't mind, could you do the same for align?

Do you have an example here? E.g ConfusablesParser also seems to ignore this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea! I've been making exceptions to new attributes here:


and then add proper validation for them here:

(n.b. I exhaustively match and error on all cases)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think those links are to #138165? Should I rebase on that now? (happy to, but could also do a follow-up later)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think rebasing would be nice, then this parser is in a much nicer state. Otherwise I'll go back to fix it next week again. That #138165 seems to make good progress.

let Some(list) = args.list() else { return };

let Some(align) = list.single() else {
cx.emit_err(session_diagnostics::IncorrectReprFormatAlignOneArg { span: list.span });

return;
};

let Some(lit) = align.lit() else {
cx.emit_err(session_diagnostics::IncorrectReprFormatExpectInteger {
span: align.span(),
});

return;
};

match parse_alignment(&lit.kind) {
Ok(literal) => self.0 = Ord::max(self.0, Some((literal, cx.attr_span))),
Err(message) => {
cx.emit_err(session_diagnostics::InvalidAlignmentValue {
span: lit.span,
error_part: message,
});
}
}
}
}

impl<S: Stage> AttributeParser<S> for AlignParser {
const ATTRIBUTES: AcceptMapping<Self, S> = &[(Self::PATH, Self::parse)];

fn finalize(self, _cx: &FinalizeContext<'_, '_, S>) -> Option<AttributeKind> {
let (align, span) = self.0?;
Some(AttributeKind::Align { align, span })
}
}
3 changes: 2 additions & 1 deletion compiler/rustc_attr_parsing/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use rustc_span::{DUMMY_SP, ErrorGuaranteed, Span, Symbol, sym};
use crate::attributes::allow_unstable::{AllowConstFnUnstableParser, AllowInternalUnstableParser};
use crate::attributes::confusables::ConfusablesParser;
use crate::attributes::deprecation::DeprecationParser;
use crate::attributes::repr::ReprParser;
use crate::attributes::repr::{AlignParser, ReprParser};
use crate::attributes::stability::{
BodyStabilityParser, ConstStabilityIndirectParser, ConstStabilityParser, StabilityParser,
};
Expand Down Expand Up @@ -89,6 +89,7 @@ macro_rules! attribute_parsers {
attribute_parsers!(
pub(crate) static ATTRIBUTE_PARSERS = [
// tidy-alphabetical-start
AlignParser,
BodyStabilityParser,
ConfusablesParser,
ConstStabilityParser,
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_attr_parsing/src/session_diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -490,3 +490,11 @@ pub(crate) struct UnrecognizedReprHint {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(attr_parsing_invalid_alignment_value, code = E0589)]
pub(crate) struct InvalidAlignmentValue {
#[primary_span]
pub span: Span,
pub error_part: &'static str,
}
14 changes: 2 additions & 12 deletions compiler/rustc_codegen_ssa/src/codegen_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use std::str::FromStr;
use rustc_abi::ExternAbi;
use rustc_ast::expand::autodiff_attrs::{AutoDiffAttrs, DiffActivity, DiffMode};
use rustc_ast::{LitKind, MetaItem, MetaItemInner, attr};
use rustc_attr_data_structures::ReprAttr::ReprAlign;
use rustc_attr_data_structures::{AttributeKind, InlineAttr, InstructionSetAttr, OptimizeAttr};
use rustc_data_structures::fx::FxHashMap;
use rustc_hir::def::DefKind;
Expand Down Expand Up @@ -110,17 +109,8 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
}
};

if let hir::Attribute::Parsed(p) = attr {
match p {
AttributeKind::Repr(reprs) => {
codegen_fn_attrs.alignment = reprs
.iter()
.filter_map(|(r, _)| if let ReprAlign(x) = r { Some(*x) } else { None })
.max();
}

_ => {}
}
if let hir::Attribute::Parsed(AttributeKind::Align { align, .. }) = attr {
codegen_fn_attrs.alignment = Some(*align);
}

let Some(Ident { name, .. }) = attr.ident() else {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,7 @@ pub static BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
),
ungated!(no_link, Normal, template!(Word), WarnFollowing, EncodeCrossCrate::No),
ungated!(repr, Normal, template!(List: "C"), DuplicatesOk, EncodeCrossCrate::No),
gated!(align, Normal, template!(List: "alignment"), DuplicatesOk, EncodeCrossCrate::No, fn_align, experimental!(align)),
ungated!(unsafe(Edition2024) export_name, Normal, template!(NameValueStr: "name"), FutureWarnPreceding, EncodeCrossCrate::No),
ungated!(unsafe(Edition2024) link_section, Normal, template!(NameValueStr: "name"), FutureWarnPreceding, EncodeCrossCrate::No),
ungated!(unsafe(Edition2024) no_mangle, Normal, template!(Word), WarnFollowing, EncodeCrossCrate::No),
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_middle/src/middle/codegen_fn_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ pub struct CodegenFnAttrs {
/// be generated against a specific instruction set. Only usable on architectures which allow
/// switching between multiple instruction sets.
pub instruction_set: Option<InstructionSetAttr>,
/// The `#[repr(align(...))]` attribute. Indicates the value of which the function should be
/// aligned to.
/// The `#[align(...)]` attribute. Determines the alignment of the function body.
pub alignment: Option<Align>,
/// The `#[patchable_function_entry(...)]` attribute. Indicates how many nops should be around
/// the function entry.
Expand Down
15 changes: 8 additions & 7 deletions compiler/rustc_passes/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ passes_abi_ne =
passes_abi_of =
fn_abi_of({$fn_name}) = {$fn_abi}

passes_align_should_be_repr_align =
`#[align(...)]` is not supported on {$item} items
.suggestion = use `#[repr(align(...))]` instead

passes_allow_incoherent_impl =
`rustc_allow_incoherent_impl` attribute should be applied to impl items
.label = the only currently supported targets are inherent methods
Expand All @@ -29,10 +33,6 @@ passes_attr_application_struct =
attribute should be applied to a struct
.label = not a struct

passes_attr_application_struct_enum_function_method_union =
attribute should be applied to a struct, enum, function, associated function, or union
.label = not a struct, enum, function, associated function, or union

passes_attr_application_struct_enum_union =
attribute should be applied to a struct, enum, or union
.label = not a struct, enum, or union
Expand Down Expand Up @@ -583,13 +583,14 @@ passes_remove_fields =
*[other] fields
}

passes_repr_align_function =
`repr(align)` attributes on functions are unstable

passes_repr_align_greater_than_target_max =
alignment must not be greater than `isize::MAX` bytes
.note = `isize::MAX` is {$size} for the current target

passes_repr_align_should_be_align =
`#[repr(align(...))]` is not supported on {$item} items
.help = use `#[align(...)]` instead

passes_repr_conflicting =
conflicting representation hints

Expand Down
75 changes: 46 additions & 29 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
}
Attribute::Parsed(AttributeKind::Repr(_)) => { /* handled below this loop and elsewhere */
}
Attribute::Parsed(AttributeKind::Align { align, span: repr_span }) => {
self.check_align(span, target, *align, *repr_span)
}

Attribute::Parsed(
AttributeKind::BodyStability { .. }
| AttributeKind::ConstStabilityIndirect
Expand Down Expand Up @@ -636,6 +640,7 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
sym::naked,
sym::instruction_set,
sym::repr,
sym::align,
sym::rustc_std_internal_symbol,
// code generation
sym::cold,
Expand Down Expand Up @@ -672,7 +677,9 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
// this check can be part of the parser and be removed here
match other_attr {
Attribute::Parsed(
AttributeKind::Deprecation { .. } | AttributeKind::Repr { .. },
AttributeKind::Deprecation { .. }
| AttributeKind::Repr { .. }
| AttributeKind::Align { .. },
) => {
continue;
}
Expand Down Expand Up @@ -1947,6 +1954,28 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
}
}

/// Checks if the `#[align]` attributes on `item` are valid.
fn check_align(&self, span: Span, target: Target, align: Align, repr_span: Span) {
match target {
Target::Fn | Target::Method(_) => {}
Target::Struct | Target::Union | Target::Enum => {
self.dcx().emit_err(errors::AlignShouldBeReprAlign {
span: repr_span,
item: target.name(),
align_bytes: align.bytes(),
});
}
_ => {
self.dcx().emit_err(errors::AttrApplication::StructEnumUnion {
hint_span: repr_span,
span,
});
}
}

self.check_align_value(align, repr_span);
}

/// Checks if the `#[repr]` attributes on `item` are valid.
fn check_repr(
&self,
Expand Down Expand Up @@ -1999,23 +2028,16 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
match target {
Target::Struct | Target::Union | Target::Enum => {}
Target::Fn | Target::Method(_) => {
if !self.tcx.features().fn_align() {
feature_err(
&self.tcx.sess,
sym::fn_align,
*repr_span,
fluent::passes_repr_align_function,
)
.emit();
}
self.dcx().emit_err(errors::ReprAlignShouldBeAlign {
span: *repr_span,
item: target.name(),
});
}
_ => {
self.dcx().emit_err(
errors::AttrApplication::StructEnumFunctionMethodUnion {
hint_span: *repr_span,
span,
},
);
self.dcx().emit_err(errors::AttrApplication::StructEnumUnion {
hint_span: *repr_span,
span,
});
}
}

Expand Down Expand Up @@ -2073,21 +2095,16 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
match target {
Target::Struct | Target::Union | Target::Enum => continue,
Target::Fn | Target::Method(_) => {
feature_err(
&self.tcx.sess,
sym::fn_align,
*repr_span,
fluent::passes_repr_align_function,
)
.emit();
self.dcx().emit_err(errors::ReprAlignShouldBeAlign {
Copy link
Contributor

Choose a reason for hiding this comment

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

love these helpful diagnostics :)

span: *repr_span,
item: target.name(),
});
}
_ => {
self.dcx().emit_err(
errors::AttrApplication::StructEnumFunctionMethodUnion {
hint_span: *repr_span,
span,
},
);
self.dcx().emit_err(errors::AttrApplication::StructEnumUnion {
hint_span: *repr_span,
span,
});
}
}
}
Expand Down
30 changes: 23 additions & 7 deletions compiler/rustc_passes/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1308,13 +1308,6 @@ pub(crate) enum AttrApplication {
#[label]
span: Span,
},
#[diag(passes_attr_application_struct_enum_function_method_union, code = E0517)]
StructEnumFunctionMethodUnion {
#[primary_span]
hint_span: Span,
#[label]
span: Span,
},
}

#[derive(Diagnostic)]
Expand Down Expand Up @@ -1816,3 +1809,26 @@ pub(crate) enum UnexportableItem<'a> {
field_name: &'a str,
},
}

#[derive(Diagnostic)]
#[diag(passes_repr_align_should_be_align)]
pub(crate) struct ReprAlignShouldBeAlign {
#[primary_span]
#[help]
pub span: Span,
pub item: &'static str,
}

#[derive(Diagnostic)]
#[diag(passes_align_should_be_repr_align)]
pub(crate) struct AlignShouldBeReprAlign {
#[primary_span]
#[suggestion(
style = "verbose",
applicability = "machine-applicable",
code = "#[repr(align({align_bytes}))]"
)]
pub span: Span,
pub item: &'static str,
pub align_bytes: u64,
}
10 changes: 5 additions & 5 deletions src/tools/miri/tests/pass/fn_align.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
//@compile-flags: -Zmin-function-alignment=8
#![feature(fn_align)]

// When a function uses `repr(align(N))`, the function address should be a multiple of `N`.
// When a function uses `align(N)`, the function address should be a multiple of `N`.

#[repr(align(256))]
#[align(256)]
fn foo() {}

#[repr(align(16))]
#[align(16)]
fn bar() {}

#[repr(align(4))]
#[align(4)]
fn baz() {}

fn main() {
assert!((foo as usize).is_multiple_of(256));
assert!((bar as usize).is_multiple_of(16));

// The maximum of `repr(align(N))` and `-Zmin-function-alignment=N` is used.
// The maximum of `align(N)` and `-Zmin-function-alignment=N` is used.
assert!((baz as usize).is_multiple_of(8));
}
Loading
Loading