Skip to content
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

Fix asm goto with outputs and move it to a separate feature gate #131523

Merged
merged 3 commits into from
Nov 25, 2024
Merged
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: 2 additions & 0 deletions compiler/rustc_ast_lowering/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,8 @@ ast_lowering_underscore_expr_lhs_assign =
.label = `_` not allowed here

ast_lowering_unstable_inline_assembly = inline assembly is not stable yet on this architecture
ast_lowering_unstable_inline_assembly_label_operand_with_outputs =
using both label and output operands for inline assembly is unstable
ast_lowering_unstable_inline_assembly_label_operands =
label operands for inline assembly are unstable
ast_lowering_unstable_may_unwind = the `may_unwind` option is unstable
Expand Down
44 changes: 35 additions & 9 deletions compiler/rustc_ast_lowering/src/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,15 +239,6 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
}
}
InlineAsmOperand::Label { block } => {
if !self.tcx.features().asm_goto() {
feature_err(
sess,
sym::asm_goto,
*op_sp,
fluent::ast_lowering_unstable_inline_assembly_label_operands,
)
.emit();
}
hir::InlineAsmOperand::Label { block: self.lower_block(block, false) }
}
};
Expand Down Expand Up @@ -466,6 +457,41 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
}
}

// Feature gate checking for asm goto.
if let Some((_, op_sp)) =
operands.iter().find(|(op, _)| matches!(op, hir::InlineAsmOperand::Label { .. }))
{
if !self.tcx.features().asm_goto() {
feature_err(
sess,
sym::asm_goto,
*op_sp,
fluent::ast_lowering_unstable_inline_assembly_label_operands,
)
.emit();
}

// In addition, check if an output operand is used.
// This is gated behind an additional feature.
let output_operand_used = operands.iter().any(|(op, _)| {
matches!(
op,
hir::InlineAsmOperand::Out { expr: Some(_), .. }
| hir::InlineAsmOperand::InOut { .. }
| hir::InlineAsmOperand::SplitInOut { out_expr: Some(_), .. }
)
});
if output_operand_used && !self.tcx.features().asm_goto_with_outputs() {
feature_err(
sess,
sym::asm_goto_with_outputs,
*op_sp,
fluent::ast_lowering_unstable_inline_assembly_label_operand_with_outputs,
)
.emit();
}
}

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(
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_builtin_macros/src/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,10 @@ pub fn parse_asm_args<'a>(
if args.options.contains(ast::InlineAsmOptions::PURE) && !have_real_output {
dcx.emit_err(errors::AsmPureNoOutput { spans: args.options_spans.clone() });
}
if args.options.contains(ast::InlineAsmOptions::NORETURN) && !outputs_sp.is_empty() {
if args.options.contains(ast::InlineAsmOptions::NORETURN)
&& !outputs_sp.is_empty()
&& labels_sp.is_empty()
{
let err = dcx.create_err(errors::AsmNoReturn { outputs_sp });
// Bail out now since this is likely to confuse MIR
return Err(err);
Expand Down
42 changes: 25 additions & 17 deletions compiler/rustc_codegen_llvm/src/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,24 +342,32 @@ impl<'ll, 'tcx> AsmBuilderMethods<'tcx> for Builder<'_, 'll, 'tcx> {
}
attributes::apply_to_callsite(result, llvm::AttributePlace::Function, &{ attrs });

// Switch to the 'normal' basic block if we did an `invoke` instead of a `call`
if let Some(dest) = dest {
self.switch_to_block(dest);
}
// Write results to outputs. We need to do this for all possible control flow.
//
// Note that `dest` maybe populated with unreachable_block when asm goto with outputs
// is used (because we need to codegen callbr which always needs a destination), so
// here we use the NORETURN option to determine if `dest` should be used.
for block in (if options.contains(InlineAsmOptions::NORETURN) { None } else { Some(dest) })
.into_iter()
.chain(labels.iter().copied().map(Some))
{
if let Some(block) = block {
self.switch_to_block(block);
}

// Write results to outputs
for (idx, op) in operands.iter().enumerate() {
if let InlineAsmOperandRef::Out { reg, place: Some(place), .. }
| InlineAsmOperandRef::InOut { reg, out_place: Some(place), .. } = *op
{
let value = if output_types.len() == 1 {
result
} else {
self.extract_value(result, op_idx[&idx] as u64)
};
let value =
llvm_fixup_output(self, value, reg.reg_class(), &place.layout, instance);
OperandValue::Immediate(value).store(self, place);
for (idx, op) in operands.iter().enumerate() {
if let InlineAsmOperandRef::Out { reg, place: Some(place), .. }
| InlineAsmOperandRef::InOut { reg, out_place: Some(place), .. } = *op
{
let value = if output_types.len() == 1 {
result
} else {
self.extract_value(result, op_idx[&idx] as u64)
};
let value =
llvm_fixup_output(self, value, reg.reg_class(), &place.layout, instance);
OperandValue::Immediate(value).store(self, place);
}
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_feature/src/unstable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,8 @@ declare_features! (
(unstable, asm_experimental_arch, "1.58.0", Some(93335)),
/// Allows using `label` operands in inline assembly.
(unstable, asm_goto, "1.78.0", Some(119364)),
/// Allows using `label` operands in inline assembly together with output operands.
(unstable, asm_goto_with_outputs, "CURRENT_RUSTC_VERSION", Some(119364)),
/// Allows the `may_unwind` option in inline assembly.
(unstable, asm_unwind, "1.58.0", Some(93334)),
/// Allows users to enforce equality of associated constants `TraitImpl<AssocConst=3>`.
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ symbols! {
asm_const,
asm_experimental_arch,
asm_goto,
asm_goto_with_outputs,
asm_sym,
asm_unwind,
assert,
Expand Down
38 changes: 25 additions & 13 deletions tests/codegen/asm/goto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,10 @@
//@ only-x86_64

#![crate_type = "rlib"]
#![feature(asm_goto)]
#![feature(asm_goto, asm_goto_with_outputs)]

use std::arch::asm;

#[no_mangle]
pub extern "C" fn panicky() {}

struct Foo;

impl Drop for Foo {
fn drop(&mut self) {
println!();
}
}

// CHECK-LABEL: @asm_goto
#[no_mangle]
pub unsafe fn asm_goto() {
Expand All @@ -38,14 +27,37 @@ pub unsafe fn asm_goto_with_outputs() -> u64 {
out
}

// CHECK-LABEL: @asm_goto_with_outputs_use_in_label
#[no_mangle]
pub unsafe fn asm_goto_with_outputs_use_in_label() -> u64 {
let out: u64;
// CHECK: [[RES:%[0-9]+]] = callbr i64 asm sideeffect alignstack inteldialect "
// CHECK-NEXT: to label %[[FALLTHROUGHBB:[a-b0-9]+]] [label %[[JUMPBB:[a-b0-9]+]]]
asm!("{} /* {} */", out(reg) out, label { return out; });
// CHECK: [[JUMPBB]]:
// CHECK-NEXT: [[RET:%.+]] = phi i64 [ 1, %[[FALLTHROUGHBB]] ], [ [[RES]], %start ]
// CHECK-NEXT: ret i64 [[RET]]
1
}

// CHECK-LABEL: @asm_goto_noreturn
#[no_mangle]
pub unsafe fn asm_goto_noreturn() -> u64 {
let out: u64;
// CHECK: callbr void asm sideeffect alignstack inteldialect "
// CHECK-NEXT: to label %unreachable [label %[[JUMPBB:[a-b0-9]+]]]
asm!("jmp {}", label { return 1; }, options(noreturn));
// CHECK: [[JUMPBB]]:
// CHECK-NEXT: ret i64 1
}

// CHECK-LABEL: @asm_goto_noreturn_with_outputs
#[no_mangle]
pub unsafe fn asm_goto_noreturn_with_outputs() -> u64 {
let out: u64;
// CHECK: [[RES:%[0-9]+]] = callbr i64 asm sideeffect alignstack inteldialect "
// CHECK-NEXT: to label %[[FALLTHROUGHBB:[a-b0-9]+]] [label %[[JUMPBB:[a-b0-9]+]]]
asm!("mov {}, 1", "jmp {}", out(reg) out, label { return out; });
// CHECK: [[JUMPBB]]:
// CHECK-NEXT: ret i64 [[RES]]
out
}
64 changes: 56 additions & 8 deletions tests/ui/asm/x86_64/goto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//@ needs-asm-support

#![deny(unreachable_code)]
#![feature(asm_goto)]
#![feature(asm_goto, asm_goto_with_outputs)]

use std::arch::asm;

Expand Down Expand Up @@ -31,10 +31,6 @@ fn goto_jump() {
}
}

// asm goto with outputs cause miscompilation in LLVM. UB can be triggered
// when outputs are used inside the label block when optimisation is enabled.
// See: https://github.com/llvm/llvm-project/issues/74483
/*
fn goto_out_fallthrough() {
unsafe {
let mut out: usize;
Expand Down Expand Up @@ -68,7 +64,57 @@ fn goto_out_jump() {
assert!(value);
}
}
*/

fn goto_out_jump_noreturn() {
unsafe {
let mut value = false;
let mut out: usize;
asm!(
"lea {}, [{} + 1]",
"jmp {}",
out(reg) out,
in(reg) 0x12345678usize,
label {
value = true;
assert_eq!(out, 0x12345679);
},
options(noreturn)
);
assert!(value);
}
}

// asm goto with outputs cause miscompilation in LLVM when multiple outputs are present.
// The code sample below is adapted from https://github.com/llvm/llvm-project/issues/74483
// and does not work with `-C opt-level=0`
#[expect(unused)]
fn goto_multi_out() {
#[inline(never)]
#[allow(unused)]
fn goto_multi_out(a: usize, b: usize) -> usize {
let mut x: usize;
let mut y: usize;
let mut z: usize;
unsafe {
core::arch::asm!(
"mov {x}, {a}",
"test {a}, {a}",
"jnz {label1}",
"/* {y} {z} {b} {label2} */",
x = out(reg) x,
y = out(reg) y,
z = out(reg) z,
a = in(reg) a,
b = in(reg) b,
label1 = label { return x },
label2 = label { return 1 },
);
0
}
}

assert_eq!(goto_multi_out(11, 22), 11);
}

fn goto_noreturn() {
unsafe {
Expand Down Expand Up @@ -102,8 +148,10 @@ fn goto_noreturn_diverge() {
fn main() {
goto_fallthough();
goto_jump();
// goto_out_fallthrough();
// goto_out_jump();
goto_out_fallthrough();
goto_out_jump();
goto_out_jump_noreturn();
// goto_multi_out();
goto_noreturn();
goto_noreturn_diverge();
}
4 changes: 2 additions & 2 deletions tests/ui/asm/x86_64/goto.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
warning: unreachable statement
--> $DIR/goto.rs:97:9
--> $DIR/goto.rs:143:9
|
LL | / asm!(
LL | | "jmp {}",
Expand All @@ -13,7 +13,7 @@ LL | unreachable!();
| ^^^^^^^^^^^^^^ unreachable statement
|
note: the lint level is defined here
--> $DIR/goto.rs:87:8
--> $DIR/goto.rs:133:8
|
LL | #[warn(unreachable_code)]
| ^^^^^^^^^^^^^^^^
Expand Down
13 changes: 13 additions & 0 deletions tests/ui/feature-gates/feature-gate-asm_goto_with_outputs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//@ only-x86_64

#![feature(asm_goto)]

use std::arch::asm;

fn main() {
let mut _out: u64;
unsafe {
asm!("mov {}, 1", "jmp {}", out(reg) _out, label {});
//~^ ERROR using both label and output operands for inline assembly is unstable
}
}
13 changes: 13 additions & 0 deletions tests/ui/feature-gates/feature-gate-asm_goto_with_outputs.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error[E0658]: using both label and output operands for inline assembly is unstable
--> $DIR/feature-gate-asm_goto_with_outputs.rs:10:52
|
LL | asm!("mov {}, 1", "jmp {}", out(reg) _out, label {});
| ^^^^^^^^
|
= note: see issue #119364 <https://github.com/rust-lang/rust/issues/119364> for more information
= help: add `#![feature(asm_goto_with_outputs)]` to the crate attributes to enable
= note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0658`.
Loading