Skip to content

Commit

Permalink
Rollup merge of rust-lang#83437 - Amanieu:asm_syntax, r=petrochenkov
Browse files Browse the repository at this point in the history 
Refactor rust-lang#82270 as lint instead of an error

This PR fixes several issues with rust-lang#82270 which generated an error when `.intel_syntax` or `.att_syntax` was used in inline assembly:
- It is now a warn-by-default lint instead of an error.
- The lint only triggers on x86. `.intel_syntax` and `.att_syntax` are only valid on x86.
- The lint no longer provides machine-applicable suggestions for two reasons:
	- These changes should not be made automatically since changes to assembly code can be very subtle.
	- The template string is not always just a string: it can contain macro invocation (`concat!`), raw strings, escape characters, etc.

cc `@asquared31415`
  • Loading branch information
@Dylan-DPC
Dylan-DPC authored Mar 25, 2021
2 parents d20b8a2 + 5dabc80 commit 632ec71
Show file tree
Hide file tree
Showing 6 changed files with 179 additions and 118 deletions.
95 changes: 30 additions & 65 deletions compiler/rustc_builtin_macros/src/asm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +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_span::{
symbol::{kw, sym, Symbol},
BytePos,
};
use rustc_session::lint;
use rustc_span::symbol::{kw, sym, Symbol};
use rustc_span::{InnerSpan, Span};
use rustc_target::asm::InlineAsmArch;

struct AsmArgs {
templates: Vec<P<ast::Expr>>,
Expand Down Expand Up @@ -402,8 +401,6 @@ fn expand_preparsed_asm(ecx: &mut ExtCtxt<'_>, sp: Span, args: AsmArgs) -> P<ast
let mut line_spans = Vec::with_capacity(args.templates.len());
let mut curarg = 0;

let default_dialect = ecx.sess.inline_asm_dialect();

for template_expr in args.templates.into_iter() {
if !template.is_empty() {
template.push(ast::InlineAsmTemplatePiece::String("\n".to_string()));
Expand All @@ -430,56 +427,36 @@ fn expand_preparsed_asm(ecx: &mut ExtCtxt<'_>, sp: Span, args: AsmArgs) -> P<ast
let template_str = &template_str.as_str();
let template_snippet = ecx.source_map().span_to_snippet(template_sp).ok();

if let Some(snippet) = &template_snippet {
let snippet = snippet.trim_matches('"');
match default_dialect {
ast::LlvmAsmDialect::Intel => {
if let Some(span) = check_syntax_directive(snippet, ".intel_syntax") {
let span = template_span.from_inner(span);
let mut err = ecx.struct_span_err(span, "intel syntax is the default syntax on this target, and trying to use this directive may cause issues");
err.span_suggestion(
span,
"remove this assembler directive",
"".to_string(),
Applicability::MachineApplicable,
);
err.emit();
}

if let Some(span) = check_syntax_directive(snippet, ".att_syntax") {
let span = template_span.from_inner(span);
let mut err = ecx.struct_span_err(span, "using the .att_syntax directive may cause issues, use the att_syntax option instead");
let asm_end = sp.hi() - BytePos(2);
let suggestions = vec![
(span, "".to_string()),
(
Span::new(asm_end, asm_end, sp.ctxt()),
", options(att_syntax)".to_string(),
),
];
err.multipart_suggestion(
"remove the assembler directive and replace it with options(att_syntax)",
suggestions,
Applicability::MachineApplicable,
);
err.emit();
if let Some(InlineAsmArch::X86 | InlineAsmArch::X86_64) = ecx.sess.asm_arch {
let find_span = |needle: &str| -> Span {
if let Some(snippet) = &template_snippet {
if let Some(pos) = snippet.find(needle) {
let end = pos
+ &snippet[pos..]
.find(|c| matches!(c, '\n' | ';' | '\\' | '"'))
.unwrap_or(snippet[pos..].len() - 1);
let inner = InnerSpan::new(pos, end);
return template_sp.from_inner(inner);
}
}
ast::LlvmAsmDialect::Att => {
if let Some(span) = check_syntax_directive(snippet, ".att_syntax") {
let span = template_span.from_inner(span);
let mut err = ecx.struct_span_err(span, "att syntax is the default syntax on this target, and trying to use this directive may cause issues");
err.span_suggestion(
span,
"remove this assembler directive",
"".to_string(),
Applicability::MachineApplicable,
);
err.emit();
}
template_sp
};

// Use of .intel_syntax is ignored
}
if template_str.contains(".intel_syntax") {
ecx.parse_sess().buffer_lint(
lint::builtin::BAD_ASM_STYLE,
find_span(".intel_syntax"),
ecx.resolver.lint_node_id(ecx.current_expansion.id),
"avoid using `.intel_syntax`, Intel syntax is the default",
);
}
if template_str.contains(".att_syntax") {
ecx.parse_sess().buffer_lint(
lint::builtin::BAD_ASM_STYLE,
find_span(".att_syntax"),
ecx.resolver.lint_node_id(ecx.current_expansion.id),
"avoid using `.att_syntax`, prefer using `options(att_syntax)` instead",
);
}
}

Expand Down Expand Up @@ -690,15 +667,3 @@ pub fn expand_asm<'cx>(
}
}
}

fn check_syntax_directive<S: AsRef<str>>(piece: S, syntax: &str) -> Option<InnerSpan> {
let piece = piece.as_ref();
if let Some(idx) = piece.find(syntax) {
let end =
idx + &piece[idx..].find(|c| matches!(c, '\n' | ';')).unwrap_or(piece[idx..].len());
// Offset by one because these represent the span with the " removed
Some(InnerSpan::new(idx + 1, end + 1))
} else {
None
}
}
46 changes: 46 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2486,6 +2486,52 @@ declare_lint! {
"using only a subset of a register for inline asm inputs",
}

declare_lint! {
/// The `bad_asm_style` lint detects the use of the `.intel_syntax` and
/// `.att_syntax` directives.
///
/// ### Example
///
/// ```rust,ignore (fails on system llvm)
/// #![feature(asm)]
///
/// fn main() {
/// #[cfg(target_arch="x86_64")]
/// unsafe {
/// asm!(
/// ".att_syntax",
/// "movl {0}, {0}", in(reg) 0usize
/// );
/// }
/// }
/// ```
///
/// This will produce:
///
/// ```text
/// warning: avoid using `.att_syntax`, prefer using `options(att_syntax)` instead
/// --> test.rs:7:14
/// |
/// 7 | ".att_syntax",
/// | ^^^^^^^^^^^
/// 8 | "movq {0}, {0}", out(reg) _,
/// 9 | );
/// | - help: add option: `, options(att_syntax)`
/// |
/// = note: `#[warn(bad_asm_style)]` on by default
/// ```
///
/// ### Explanation
///
/// On x86, `asm!` uses the intel assembly syntax by default. While this
/// can be switched using assembler directives like `.att_syntax`, using the
/// `att_syntax` option is recomended instead because it will also properly
/// prefix register placeholders with `%` as required by AT&T syntax.
pub BAD_ASM_STYLE,
Warn,
"incorrect use of inline assembly",
}

declare_lint! {
/// The `unsafe_op_in_unsafe_fn` lint detects unsafe operations in unsafe
/// functions without an explicit unsafe block.
Expand Down
7 changes: 0 additions & 7 deletions compiler/rustc_session/src/session.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -793,13 +793,6 @@ impl Session {
}
}

pub fn inline_asm_dialect(&self) -> rustc_ast::LlvmAsmDialect {
match self.asm_arch {
Some(InlineAsmArch::X86 | InlineAsmArch::X86_64) => rustc_ast::LlvmAsmDialect::Intel,
_ => rustc_ast::LlvmAsmDialect::Att,
}
}

pub fn relocation_model(&self) -> RelocModel {
self.opts.cg.relocation_model.unwrap_or(self.target.relocation_model)
}
Expand Down
74 changes: 67 additions & 7 deletions src/test/ui/asm/inline-syntax.arm.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,74 @@
error: att syntax is the default syntax on this target, and trying to use this directive may cause issues
--> $DIR/inline-syntax.rs:23:15
error: unknown directive
--> $DIR/inline-syntax.rs:22:15
|
LL | asm!(".intel_syntax noprefix", "nop");
| ^
|
note: instantiated into assembly here
--> <inline asm>:1:2
|
LL | .intel_syntax noprefix
| ^

error: unknown directive
--> $DIR/inline-syntax.rs:25:15
|
LL | asm!(".intel_syntax aaa noprefix", "nop");
| ^
|
note: instantiated into assembly here
--> <inline asm>:1:2
|
LL | .intel_syntax aaa noprefix
| ^

error: unknown directive
--> $DIR/inline-syntax.rs:28:15
|
LL | asm!(".att_syntax noprefix", "nop");
| ^^^^^^^^^^^^^^^^^^^^ help: remove this assembler directive
| ^
|
note: instantiated into assembly here
--> <inline asm>:1:2
|
LL | .att_syntax noprefix
| ^

error: att syntax is the default syntax on this target, and trying to use this directive may cause issues
--> $DIR/inline-syntax.rs:26:15
error: unknown directive
--> $DIR/inline-syntax.rs:31:15
|
LL | asm!(".att_syntax bbb noprefix", "nop");
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: remove this assembler directive
| ^
|
note: instantiated into assembly here
--> <inline asm>:1:2
|
LL | .att_syntax bbb noprefix
| ^

error: unknown directive
--> $DIR/inline-syntax.rs:34:15
|
LL | asm!(".intel_syntax noprefix; nop");
| ^
|
note: instantiated into assembly here
--> <inline asm>:1:2
|
LL | .intel_syntax noprefix; nop
| ^

error: unknown directive
--> $DIR/inline-syntax.rs:40:13
|
LL | .intel_syntax noprefix
| ^
|
note: instantiated into assembly here
--> <inline asm>:2:13
|
LL | .intel_syntax noprefix
| ^

error: aborting due to 2 previous errors
error: aborting due to 6 previous errors

25 changes: 16 additions & 9 deletions src/test/ui/asm/inline-syntax.rs
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
// needs-llvm-components: arm
// revisions: x86_64 arm
//[x86_64] compile-flags: --target x86_64-unknown-linux-gnu
//[x86_64] check-pass
//[arm] compile-flags: --target armv7-unknown-linux-gnueabihf
//[arm] build-fail

#![feature(no_core, lang_items, rustc_attrs)]
#![crate_type = "rlib"]
#![no_core]

#[rustc_builtin_macro]
Expand All @@ -14,26 +17,30 @@ macro_rules! asm {
#[lang = "sized"]
trait Sized {}

fn main() {
pub fn main() {
unsafe {
asm!(".intel_syntax noprefix", "nop");
//[x86_64]~^ ERROR intel syntax is the default syntax on this target
//[x86_64]~^ WARN avoid using `.intel_syntax`
//[arm]~^^ ERROR unknown directive
asm!(".intel_syntax aaa noprefix", "nop");
//[x86_64]~^ ERROR intel syntax is the default syntax on this target
//[x86_64]~^ WARN avoid using `.intel_syntax`
//[arm]~^^ ERROR unknown directive
asm!(".att_syntax noprefix", "nop");
//[x86_64]~^ ERROR using the .att_syntax directive may cause issues
//[arm]~^^ att syntax is the default syntax on this target
//[x86_64]~^ WARN avoid using `.att_syntax`
//[arm]~^^ ERROR unknown directive
asm!(".att_syntax bbb noprefix", "nop");
//[x86_64]~^ ERROR using the .att_syntax directive may cause issues
//[arm]~^^ att syntax is the default syntax on this target
//[x86_64]~^ WARN avoid using `.att_syntax`
//[arm]~^^ ERROR unknown directive
asm!(".intel_syntax noprefix; nop");
//[x86_64]~^ ERROR intel syntax is the default syntax on this target
//[x86_64]~^ WARN avoid using `.intel_syntax`
//[arm]~^^ ERROR unknown directive

asm!(
r"
.intel_syntax noprefix
nop"
);
//[x86_64]~^^^ ERROR intel syntax is the default syntax on this target
//[x86_64]~^^^ WARN avoid using `.intel_syntax`
//[arm]~^^^^ ERROR unknown directive
}
}
50 changes: 20 additions & 30 deletions src/test/ui/asm/inline-syntax.x86_64.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,50 +1,40 @@
error: intel syntax is the default syntax on this target, and trying to use this directive may cause issues
--> $DIR/inline-syntax.rs:19:15
warning: avoid using `.intel_syntax`, Intel syntax is the default
--> $DIR/inline-syntax.rs:22:15
|
LL | asm!(".intel_syntax noprefix", "nop");
| ^^^^^^^^^^^^^^^^^^^^^^ help: remove this assembler directive
| ^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(bad_asm_style)]` on by default

error: intel syntax is the default syntax on this target, and trying to use this directive may cause issues
--> $DIR/inline-syntax.rs:21:15
warning: avoid using `.intel_syntax`, Intel syntax is the default
--> $DIR/inline-syntax.rs:25:15
|
LL | asm!(".intel_syntax aaa noprefix", "nop");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove this assembler directive
| ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: using the .att_syntax directive may cause issues, use the att_syntax option instead
--> $DIR/inline-syntax.rs:23:15
warning: avoid using `.att_syntax`, prefer using `options(att_syntax)` instead
--> $DIR/inline-syntax.rs:28:15
|
LL | asm!(".att_syntax noprefix", "nop");
| ^^^^^^^^^^^^^^^^^^^^
|
help: remove the assembler directive and replace it with options(att_syntax)
|
LL | asm!("", "nop", options(att_syntax));
| -- ^^^^^^^^^^^^^^^^^^^^^

error: using the .att_syntax directive may cause issues, use the att_syntax option instead
--> $DIR/inline-syntax.rs:26:15
warning: avoid using `.att_syntax`, prefer using `options(att_syntax)` instead
--> $DIR/inline-syntax.rs:31:15
|
LL | asm!(".att_syntax bbb noprefix", "nop");
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
help: remove the assembler directive and replace it with options(att_syntax)
|
LL | asm!("", "nop", options(att_syntax));
| -- ^^^^^^^^^^^^^^^^^^^^^

error: intel syntax is the default syntax on this target, and trying to use this directive may cause issues
--> $DIR/inline-syntax.rs:29:15
warning: avoid using `.intel_syntax`, Intel syntax is the default
--> $DIR/inline-syntax.rs:34:15
|
LL | asm!(".intel_syntax noprefix; nop");
| ^^^^^^^^^^^^^^^^^^^^^^ help: remove this assembler directive
| ^^^^^^^^^^^^^^^^^^^^^^

error: intel syntax is the default syntax on this target, and trying to use this directive may cause issues
--> $DIR/inline-syntax.rs:34:14
warning: avoid using `.intel_syntax`, Intel syntax is the default
--> $DIR/inline-syntax.rs:40:13
|
LL | .intel_syntax noprefix
| ______________^
LL | | nop"
| |_ help: remove this assembler directive
LL | .intel_syntax noprefix
| ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 6 previous errors
warning: 6 warnings emitted

0 comments on commit 632ec71

Please sign in to comment.