Skip to content

Commit

Permalink
Unrolled build for rust-lang#131523
Browse files Browse the repository at this point in the history
Rollup merge of rust-lang#131523 - nbdd0121:asm, r=compiler-errors

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

Tracking issue: rust-lang#119364

This PR addresses 3 aspects of asm goto with outputs:
* Codegen is fixed. My initial implementation has an oversight which cause the output to be only stored in fallthrough path, but not in label blocks.
* Outputs can now be used with `options(noreturn)` if a label block is given.
* All of this is moved to a new feature gate, because we likely want to stabilise `asm_goto` before asm goto with outputs.

`@rustbot` labels: +A-inline-assembly +F-asm
  • Loading branch information
rust-timer authored Nov 25, 2024
2 parents 67a8c64 + 0178ba2 commit 27a6052
Show file tree
Hide file tree
Showing 11 changed files with 178 additions and 50 deletions.
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`.

0 comments on commit 27a6052

Please sign in to comment.