Skip to content

Commit

Permalink
Rollup merge of rust-lang#88196 - asquared31415:named-asm-labels-refa…
Browse files Browse the repository at this point in the history 
…ctor, r=Amanieu

Refactor `named_asm_labels` to a HIR lint

As discussed on rust-lang#88169, the `named_asm_labels` lint could be moved to a HIR lint.  That allows future lints or custom plugins or clippy lints to more easily access the `asm!` macro's data and create better error messages with the lints.
  • Loading branch information
@LeSeulArtichaut
LeSeulArtichaut authored Aug 25, 2021
2 parents 2512fb0 + 0b81c2e commit 891fa3c
Show file tree
Hide file tree
Showing 10 changed files with 304 additions and 147 deletions.
1 change: 1 addition & 0 deletions compiler/rustc_ast/src/ast.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2028,6 +2028,7 @@ pub enum InlineAsmOperand {
#[derive(Clone, Encodable, Decodable, Debug)]
pub struct InlineAsm {
pub template: Vec<InlineAsmTemplatePiece>,
pub template_strs: Box<[(Symbol, Option<Symbol>, Span)]>,
pub operands: Vec<(InlineAsmOperand, Span)>,
pub clobber_abi: Option<(Symbol, Span)>,
pub options: InlineAsmOptions,
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_ast/src/visit.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,20 @@ use crate::token;
use rustc_span::symbol::{Ident, Symbol};
use rustc_span::Span;

#[derive(Copy, Clone, PartialEq)]
#[derive(Copy, Clone, Debug, PartialEq)]
pub enum AssocCtxt {
Trait,
Impl,
}

#[derive(Copy, Clone, PartialEq)]
#[derive(Copy, Clone, Debug, PartialEq)]
pub enum FnCtxt {
Free,
Foreign,
Assoc(AssocCtxt),
}

#[derive(Copy, Clone)]
#[derive(Copy, Clone, Debug)]
pub enum FnKind<'a> {
/// E.g., `fn foo()`, `fn foo(&self)`, or `extern "Abi" fn foo()`.
Fn(FnCtxt, Ident, &'a FnSig, &'a Visibility, Option<&'a Block>),
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_ast_lowering/src/asm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,10 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {

let operands = self.arena.alloc_from_iter(operands);
let template = self.arena.alloc_from_iter(asm.template.iter().cloned());
let template_strs = self.arena.alloc_from_iter(asm.template_strs.iter().cloned());
let line_spans = self.arena.alloc_slice(&asm.line_spans[..]);
let hir_asm = hir::InlineAsm { template, operands, options: asm.options, line_spans };
let hir_asm =
hir::InlineAsm { template, template_strs, operands, options: asm.options, line_spans };
self.arena.alloc(hir_asm)
}
}
90 changes: 14 additions & 76 deletions compiler/rustc_builtin_macros/src/asm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ use rustc_errors::{Applicability, DiagnosticBuilder};
use rustc_expand::base::{self, *};
use rustc_parse::parser::Parser;
use rustc_parse_format as parse;
use rustc_session::lint::{self, BuiltinLintDiagnostics};
use rustc_session::lint;
use rustc_span::symbol::Ident;
use rustc_span::symbol::{kw, sym, Symbol};
use rustc_span::{InnerSpan, MultiSpan, Span};
use rustc_span::{InnerSpan, Span};
use rustc_target::asm::InlineAsmArch;
use smallvec::smallvec;

Expand Down Expand Up @@ -484,11 +484,7 @@ fn parse_reg<'a>(
Ok(result)
}

fn expand_preparsed_asm(
ecx: &mut ExtCtxt<'_>,
args: AsmArgs,
is_local_asm: bool,
) -> Option<ast::InlineAsm> {
fn expand_preparsed_asm(ecx: &mut ExtCtxt<'_>, args: AsmArgs) -> Option<ast::InlineAsm> {
let mut template = vec![];
// Register operands are implicitly used since they are not allowed to be
// referenced in the template string.
Expand All @@ -501,6 +497,8 @@ fn expand_preparsed_asm(
let mut line_spans = Vec::with_capacity(args.templates.len());
let mut curarg = 0;

let mut template_strs = Vec::with_capacity(args.templates.len());

for template_expr in args.templates.into_iter() {
if !template.is_empty() {
template.push(ast::InlineAsmTemplatePiece::String("\n".to_string()));
Expand All @@ -524,8 +522,13 @@ fn expand_preparsed_asm(
ast::StrStyle::Raw(raw) => Some(raw as usize),
};

let template_str = &template_str.as_str();
let template_snippet = ecx.source_map().span_to_snippet(template_sp).ok();
template_strs.push((
template_str,
template_snippet.as_ref().map(|s| Symbol::intern(s)),
template_sp,
));
let template_str = &template_str.as_str();

if let Some(InlineAsmArch::X86 | InlineAsmArch::X86_64) = ecx.sess.asm_arch {
let find_span = |needle: &str| -> Span {
Expand Down Expand Up @@ -560,72 +563,6 @@ fn expand_preparsed_asm(
}
}

// Lint against the use of named labels in inline `asm!` but not `global_asm!`
if is_local_asm {
let find_label_span = |needle: &str| -> Option<Span> {
if let Some(snippet) = &template_snippet {
if let Some(pos) = snippet.find(needle) {
let end = pos
+ &snippet[pos..]
.find(|c| c == ':')
.unwrap_or(snippet[pos..].len() - 1);
let inner = InnerSpan::new(pos, end);
return Some(template_sp.from_inner(inner));
}
}

None
};

let mut found_labels = Vec::new();

// A semicolon might not actually be specified as a separator for all targets, but it seems like LLVM accepts it always
let statements = template_str.split(|c| matches!(c, '\n' | ';'));
for statement in statements {
// If there's a comment, trim it from the statement
let statement = statement.find("//").map_or(statement, |idx| &statement[..idx]);
let mut start_idx = 0;
for (idx, _) in statement.match_indices(':') {
let possible_label = statement[start_idx..idx].trim();
let mut chars = possible_label.chars();
if let Some(c) = chars.next() {
// A label starts with an alphabetic character or . or _ and continues with alphanumeric characters, _, or $
if (c.is_alphabetic() || matches!(c, '.' | '_'))
&& chars.all(|c| c.is_alphanumeric() || matches!(c, '_' | '$'))
{
found_labels.push(possible_label);
} else {
// If we encounter a non-label, there cannot be any further labels, so stop checking
break;
}
} else {
// Empty string means a leading ':' in this section, which is not a label
break;
}

start_idx = idx + 1;
}
}

if found_labels.len() > 0 {
let spans =
found_labels.into_iter().filter_map(find_label_span).collect::<Vec<Span>>();
// If there were labels but we couldn't find a span, combine the warnings and use the template span
let target_spans: MultiSpan =
if spans.len() > 0 { spans.into() } else { template_sp.into() };
ecx.parse_sess().buffer_lint_with_diagnostic(
lint::builtin::NAMED_ASM_LABELS,
target_spans,
ecx.current_expansion.lint_node_id,
"avoid using named labels in inline assembly",
BuiltinLintDiagnostics::NamedAsmLabel(
"only local labels of the form `<number>:` should be used in inline asm"
.to_string(),
),
);
}
}

// Don't treat raw asm as a format string.
if args.options.contains(ast::InlineAsmOptions::RAW) {
template.push(ast::InlineAsmTemplatePiece::String(template_str.to_string()));
Expand Down Expand Up @@ -819,6 +756,7 @@ fn expand_preparsed_asm(

Some(ast::InlineAsm {
template,
template_strs: template_strs.into_boxed_slice(),
operands: args.operands,
clobber_abi: args.clobber_abi,
options: args.options,
Expand All @@ -833,7 +771,7 @@ pub fn expand_asm<'cx>(
) -> Box<dyn base::MacResult + 'cx> {
match parse_args(ecx, sp, tts, false) {
Ok(args) => {
let expr = if let Some(inline_asm) = expand_preparsed_asm(ecx, args, true) {
let expr = if let Some(inline_asm) = expand_preparsed_asm(ecx, args) {
P(ast::Expr {
id: ast::DUMMY_NODE_ID,
kind: ast::ExprKind::InlineAsm(P(inline_asm)),
Expand All @@ -860,7 +798,7 @@ pub fn expand_global_asm<'cx>(
) -> Box<dyn base::MacResult + 'cx> {
match parse_args(ecx, sp, tts, true) {
Ok(args) => {
if let Some(inline_asm) = expand_preparsed_asm(ecx, args, false) {
if let Some(inline_asm) = expand_preparsed_asm(ecx, args) {
MacEager::items(smallvec![P(ast::Item {
ident: Ident::invalid(),
attrs: Vec::new(),
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir/src/hir.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2386,6 +2386,7 @@ impl<'hir> InlineAsmOperand<'hir> {
#[derive(Debug, HashStable_Generic)]
pub struct InlineAsm<'hir> {
pub template: &'hir [InlineAsmTemplatePiece],
pub template_strs: &'hir [(Symbol, Option<Symbol>, Span)],
pub operands: &'hir [(InlineAsmOperand<'hir>, Span)],
pub options: InlineAsmOptions,
pub line_spans: &'hir [Span],
Expand Down
124 changes: 122 additions & 2 deletions compiler/rustc_lint/src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_middle::ty::subst::{GenericArgKind, Subst};
use rustc_middle::ty::Instance;
use rustc_middle::ty::{self, layout::LayoutError, Ty, TyCtxt};
use rustc_session::lint::FutureIncompatibilityReason;
use rustc_session::lint::{BuiltinLintDiagnostics, FutureIncompatibilityReason};
use rustc_span::edition::Edition;
use rustc_span::source_map::Spanned;
use rustc_span::symbol::{kw, sym, Ident, Symbol};
use rustc_span::{BytePos, Span};
use rustc_span::{BytePos, InnerSpan, MultiSpan, Span};
use rustc_target::abi::{LayoutOf, VariantIdx};
use rustc_trait_selection::traits::misc::can_type_implement_copy;

Expand Down Expand Up @@ -3140,3 +3140,123 @@ impl<'tcx> LateLintPass<'tcx> for DerefNullPtr {
}
}
}

declare_lint! {
/// The `named_asm_labels` lint detects the use of named labels in the
/// inline `asm!` macro.
///
/// ### Example
///
/// ```rust,compile_fail
/// #![feature(asm)]
/// fn main() {
/// unsafe {
/// asm!("foo: bar");
/// }
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// LLVM is allowed to duplicate inline assembly blocks for any
/// reason, for example when it is in a function that gets inlined. Because
/// of this, GNU assembler [local labels] *must* be used instead of labels
/// with a name. Using named labels might cause assembler or linker errors.
///
/// See the [unstable book] for more details.
///
/// [local labels]: https://sourceware.org/binutils/docs/as/Symbol-Names.html#Local-Labels
/// [unstable book]: https://doc.rust-lang.org/nightly/unstable-book/library-features/asm.html#labels
pub NAMED_ASM_LABELS,
Deny,
"named labels in inline assembly",
}

declare_lint_pass!(NamedAsmLabels => [NAMED_ASM_LABELS]);

impl<'tcx> LateLintPass<'tcx> for NamedAsmLabels {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) {
if let hir::Expr {
kind: hir::ExprKind::InlineAsm(hir::InlineAsm { template_strs, .. }),
..
} = expr
{
for (template_sym, template_snippet, template_span) in template_strs.iter() {
let template_str = &template_sym.as_str();
let find_label_span = |needle: &str| -> Option<Span> {
if let Some(template_snippet) = template_snippet {
let snippet = template_snippet.as_str();
if let Some(pos) = snippet.find(needle) {
let end = pos
+ &snippet[pos..]
.find(|c| c == ':')
.unwrap_or(snippet[pos..].len() - 1);
let inner = InnerSpan::new(pos, end);
return Some(template_span.from_inner(inner));
}
}

None
};

let mut found_labels = Vec::new();

// A semicolon might not actually be specified as a separator for all targets, but it seems like LLVM accepts it always
let statements = template_str.split(|c| matches!(c, '\n' | ';'));
for statement in statements {
// If there's a comment, trim it from the statement
let statement = statement.find("//").map_or(statement, |idx| &statement[..idx]);
let mut start_idx = 0;
for (idx, _) in statement.match_indices(':') {
let possible_label = statement[start_idx..idx].trim();
let mut chars = possible_label.chars();
if let Some(c) = chars.next() {
// A label starts with an alphabetic character or . or _ and continues with alphanumeric characters, _, or $
if (c.is_alphabetic() || matches!(c, '.' | '_'))
&& chars.all(|c| c.is_alphanumeric() || matches!(c, '_' | '$'))
{
found_labels.push(possible_label);
} else {
// If we encounter a non-label, there cannot be any further labels, so stop checking
break;
}
} else {
// Empty string means a leading ':' in this section, which is not a label
break;
}

start_idx = idx + 1;
}
}

debug!("NamedAsmLabels::check_expr(): found_labels: {:#?}", &found_labels);

if found_labels.len() > 0 {
let spans = found_labels
.into_iter()
.filter_map(|label| find_label_span(label))
.collect::<Vec<Span>>();
// If there were labels but we couldn't find a span, combine the warnings and use the template span
let target_spans: MultiSpan =
if spans.len() > 0 { spans.into() } else { (*template_span).into() };

cx.lookup_with_diagnostics(
NAMED_ASM_LABELS,
Some(target_spans),
|diag| {
let mut err =
diag.build("avoid using named labels in inline assembly");
err.emit();
},
BuiltinLintDiagnostics::NamedAsmLabel(
"only local labels of the form `<number>:` should be used in inline asm"
.to_string(),
),
);
}
}
}
}
}
1 change: 1 addition & 0 deletions compiler/rustc_lint/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ macro_rules! late_lint_passes {
NonPanicFmt: NonPanicFmt,
NoopMethodCall: NoopMethodCall,
InvalidAtomicOrdering: InvalidAtomicOrdering,
NamedAsmLabels: NamedAsmLabels,
]
);
};
Expand Down
33 changes: 0 additions & 33 deletions compiler/rustc_lint_defs/src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2468,38 +2468,6 @@ declare_lint! {
"incorrect use of inline assembly",
}

declare_lint! {
/// The `named_asm_labels` lint detects the use of named labels in the
/// inline `asm!` macro.
///
/// ### Example
///
/// ```rust,compile_fail
/// fn main() {
/// unsafe {
/// asm!("foo: bar");
/// }
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// LLVM is allowed to duplicate inline assembly blocks for any
/// reason, for example when it is in a function that gets inlined. Because
/// of this, GNU assembler [local labels] *must* be used instead of labels
/// with a name. Using named labels might cause assembler or linker errors.
///
/// See the [unstable book] for more details.
///
/// [local labels]: https://sourceware.org/binutils/docs/as/Symbol-Names.html#Local-Labels
/// [unstable book]: https://doc.rust-lang.org/nightly/unstable-book/library-features/asm.html#labels
pub NAMED_ASM_LABELS,
Deny,
"named labels in inline assembly",
}

declare_lint! {
/// The `unsafe_op_in_unsafe_fn` lint detects unsafe operations in unsafe
/// functions without an explicit unsafe block.
Expand Down Expand Up @@ -3020,7 +2988,6 @@ declare_lint_pass! {
INLINE_NO_SANITIZE,
BAD_ASM_STYLE,
ASM_SUB_REGISTER,
NAMED_ASM_LABELS,
UNSAFE_OP_IN_UNSAFE_FN,
INCOMPLETE_INCLUDE,
CENUM_IMPL_DROP_CAST,
Expand Down
Loading

0 comments on commit 891fa3c

Please sign in to comment.