Skip to content

Commit 5ed1123

Browse files
ChayimFriedman2ChayimFriedman2
committed
Improve asm support
Including: - Infer `label {}` and `const` operands. - Correctly handle unsafe check inside `label {}`. - Fix an embarrassing parser typo that cause labels to never be part of the AST
1 parent f8e7843 commit 5ed1123

File tree

10 files changed

+186
-40
lines changed

10 files changed

+186
-40
lines changed

‎crates/hir-def/src/expr_store.rs‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -298,17 +298,16 @@ impl ExpressionStore {
298298
Expr::InlineAsm(it) => it.operands.iter().for_each(|(_, op)| match op {
299299
AsmOperand::In { expr, .. }
300300
| AsmOperand::Out { expr: Some(expr), .. }
301-
| AsmOperand::InOut { expr, .. } => f(*expr),
301+
| AsmOperand::InOut { expr, .. }
302+
| AsmOperand::Const(expr)
303+
| AsmOperand::Label(expr) => f(*expr),
302304
AsmOperand::SplitInOut { in_expr, out_expr, .. } => {
303305
f(*in_expr);
304306
if let Some(out_expr) = out_expr {
305307
f(*out_expr);
306308
}
307309
}
308-
AsmOperand::Out { expr: None, .. }
309-
| AsmOperand::Const(_)
310-
| AsmOperand::Label(_)
311-
| AsmOperand::Sym(_) => (),
310+
AsmOperand::Out { expr: None, .. } | AsmOperand::Sym(_) => (),
312311
}),
313312
Expr::If { condition, then_branch, else_branch } => {
314313
f(*condition);
@@ -435,17 +434,16 @@ impl ExpressionStore {
435434
Expr::InlineAsm(it) => it.operands.iter().for_each(|(_, op)| match op {
436435
AsmOperand::In { expr, .. }
437436
| AsmOperand::Out { expr: Some(expr), .. }
438-
| AsmOperand::InOut { expr, .. } => f(*expr),
437+
| AsmOperand::InOut { expr, .. }
438+
| AsmOperand::Const(expr)
439+
| AsmOperand::Label(expr) => f(*expr),
439440
AsmOperand::SplitInOut { in_expr, out_expr, .. } => {
440441
f(*in_expr);
441442
if let Some(out_expr) = out_expr {
442443
f(*out_expr);
443444
}
444445
}
445-
AsmOperand::Out { expr: None, .. }
446-
| AsmOperand::Const(_)
447-
| AsmOperand::Label(_)
448-
| AsmOperand::Sym(_) => (),
446+
AsmOperand::Out { expr: None, .. } | AsmOperand::Sym(_) => (),
449447
}),
450448
Expr::If { condition, then_branch, else_branch } => {
451449
f(*condition);

‎crates/hir-ty/src/diagnostics/unsafe_check.rs‎

Lines changed: 45 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use either::Either;
77
use hir_def::{
88
AdtId, DefWithBodyId, FieldId, FunctionId, VariantId,
99
expr_store::{Body, path::Path},
10-
hir::{Expr, ExprId, ExprOrPatId, Pat, PatId, Statement, UnaryOp},
10+
hir::{AsmOperand, Expr, ExprId, ExprOrPatId, Pat, PatId, Statement, UnaryOp},
1111
resolver::{HasResolver, ResolveValueResult, Resolver, ValueNs},
1212
signatures::StaticFlags,
1313
type_ref::Rawness,
@@ -199,6 +199,17 @@ impl<'db> UnsafeVisitor<'db> {
199199
}
200200
}
201201

202+
fn with_inside_unsafe_block<R>(
203+
&mut self,
204+
inside_unsafe_block: InsideUnsafeBlock,
205+
f: impl FnOnce(&mut Self) -> R,
206+
) -> R {
207+
let old = mem::replace(&mut self.inside_unsafe_block, inside_unsafe_block);
208+
let result = f(self);
209+
self.inside_unsafe_block = old;
210+
result
211+
}
212+
202213
fn walk_pats_top(&mut self, pats: impl Iterator<Item = PatId>, parent_expr: ExprId) {
203214
let guard = self.resolver.update_to_inner_scope(self.db, self.def, parent_expr);
204215
pats.for_each(|pat| self.walk_pat(pat));
@@ -303,7 +314,29 @@ impl<'db> UnsafeVisitor<'db> {
303314
self.walk_pats_top(std::iter::once(target), current);
304315
self.inside_assignment = old_inside_assignment;
305316
}
306-
Expr::InlineAsm(_) => self.on_unsafe_op(current.into(), UnsafetyReason::InlineAsm),
317+
Expr::InlineAsm(asm) => {
318+
self.on_unsafe_op(current.into(), UnsafetyReason::InlineAsm);
319+
asm.operands.iter().for_each(|(_, op)| match op {
320+
AsmOperand::In { expr, .. }
321+
| AsmOperand::Out { expr: Some(expr), .. }
322+
| AsmOperand::InOut { expr, .. }
323+
| AsmOperand::Const(expr) => self.walk_expr(*expr),
324+
AsmOperand::SplitInOut { in_expr, out_expr, .. } => {
325+
self.walk_expr(*in_expr);
326+
if let Some(out_expr) = out_expr {
327+
self.walk_expr(*out_expr);
328+
}
329+
}
330+
AsmOperand::Out { expr: None, .. } | AsmOperand::Sym(_) => (),
331+
AsmOperand::Label(expr) => {
332+
// Inline asm labels are considered safe even when inside unsafe blocks.
333+
self.with_inside_unsafe_block(InsideUnsafeBlock::No, |this| {
334+
this.walk_expr(*expr)
335+
});
336+
}
337+
});
338+
return;
339+
}
307340
// rustc allows union assignment to propagate through field accesses and casts.
308341
Expr::Cast { .. } => self.inside_assignment = inside_assignment,
309342
Expr::Field { .. } => {
@@ -317,17 +350,16 @@ impl<'db> UnsafeVisitor<'db> {
317350
}
318351
}
319352
Expr::Unsafe { statements, .. } => {
320-
let old_inside_unsafe_block =
321-
mem::replace(&mut self.inside_unsafe_block, InsideUnsafeBlock::Yes);
322-
self.walk_pats_top(
323-
statements.iter().filter_map(|statement| match statement {
324-
&Statement::Let { pat, .. } => Some(pat),
325-
_ => None,
326-
}),
327-
current,
328-
);
329-
self.body.walk_child_exprs_without_pats(current, |child| self.walk_expr(child));
330-
self.inside_unsafe_block = old_inside_unsafe_block;
353+
self.with_inside_unsafe_block(InsideUnsafeBlock::Yes, |this| {
354+
this.walk_pats_top(
355+
statements.iter().filter_map(|statement| match statement {
356+
&Statement::Let { pat, .. } => Some(pat),
357+
_ => None,
358+
}),
359+
current,
360+
);
361+
this.body.walk_child_exprs_without_pats(current, |child| this.walk_expr(child));
362+
});
331363
return;
332364
}
333365
Expr::Block { statements, .. } | Expr::Async { statements, .. } => {

‎crates/hir-ty/src/infer/expr.rs‎

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -959,8 +959,8 @@ impl InferenceContext<'_> {
959959
}
960960
Expr::OffsetOf(_) => TyKind::Scalar(Scalar::Uint(UintTy::Usize)).intern(Interner),
961961
Expr::InlineAsm(asm) => {
962-
let mut check_expr_asm_operand = |expr, is_input: bool| {
963-
let ty = self.infer_expr_no_expect(expr, ExprIsRead::Yes);
962+
let check_expr_asm_operand = |this: &mut Self, expr, is_input: bool| {
963+
let ty = this.infer_expr_no_expect(expr, ExprIsRead::Yes);
964964

965965
// If this is an input value, we require its type to be fully resolved
966966
// at this point. This allows us to provide helpful coercions which help
@@ -970,18 +970,18 @@ impl InferenceContext<'_> {
970970
// allows them to be inferred based on how they are used later in the
971971
// function.
972972
if is_input {
973-
let ty = self.resolve_ty_shallow(&ty);
973+
let ty = this.resolve_ty_shallow(&ty);
974974
match ty.kind(Interner) {
975975
TyKind::FnDef(def, parameters) => {
976976
let fnptr_ty = TyKind::Function(
977-
CallableSig::from_def(self.db, *def, parameters).to_fn_ptr(),
977+
CallableSig::from_def(this.db, *def, parameters).to_fn_ptr(),
978978
)
979979
.intern(Interner);
980-
_ = self.coerce(Some(expr), &ty, &fnptr_ty, CoerceNever::Yes);
980+
_ = this.coerce(Some(expr), &ty, &fnptr_ty, CoerceNever::Yes);
981981
}
982982
TyKind::Ref(mutbl, _, base_ty) => {
983983
let ptr_ty = TyKind::Raw(*mutbl, base_ty.clone()).intern(Interner);
984-
_ = self.coerce(Some(expr), &ty, &ptr_ty, CoerceNever::Yes);
984+
_ = this.coerce(Some(expr), &ty, &ptr_ty, CoerceNever::Yes);
985985
}
986986
_ => {}
987987
}
@@ -990,22 +990,28 @@ impl InferenceContext<'_> {
990990

991991
let diverge = asm.options.contains(AsmOptions::NORETURN);
992992
asm.operands.iter().for_each(|(_, operand)| match *operand {
993-
AsmOperand::In { expr, .. } => check_expr_asm_operand(expr, true),
993+
AsmOperand::In { expr, .. } => check_expr_asm_operand(self, expr, true),
994994
AsmOperand::Out { expr: Some(expr), .. } | AsmOperand::InOut { expr, .. } => {
995-
check_expr_asm_operand(expr, false)
995+
check_expr_asm_operand(self, expr, false)
996996
}
997997
AsmOperand::Out { expr: None, .. } => (),
998998
AsmOperand::SplitInOut { in_expr, out_expr, .. } => {
999-
check_expr_asm_operand(in_expr, true);
999+
check_expr_asm_operand(self, in_expr, true);
10001000
if let Some(out_expr) = out_expr {
1001-
check_expr_asm_operand(out_expr, false);
1001+
check_expr_asm_operand(self, out_expr, false);
10021002
}
10031003
}
1004-
// FIXME
1005-
AsmOperand::Label(_) => (),
1006-
// FIXME
1007-
AsmOperand::Const(_) => (),
1008-
// FIXME
1004+
AsmOperand::Label(expr) => {
1005+
self.infer_expr(
1006+
expr,
1007+
&Expectation::HasType(self.result.standard_types.unit.clone()),
1008+
ExprIsRead::No,
1009+
);
1010+
}
1011+
AsmOperand::Const(expr) => {
1012+
self.infer_expr(expr, &Expectation::None, ExprIsRead::No);
1013+
}
1014+
// FIXME: `sym` should report for things that are not functions or statics.
10091015
AsmOperand::Sym(_) => (),
10101016
});
10111017
if diverge {

‎crates/hir-ty/src/tests/macros.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1505,6 +1505,10 @@ fn main() {
15051505
!119..120 'o': i32
15061506
293..294 'o': i32
15071507
308..317 'thread_id': usize
1508+
!314..320 'OffPtr': usize
1509+
!333..338 'OffFn': usize
1510+
!354..355 '0': i32
1511+
!371..382 'MEM_RELEASE': usize
15081512
"#]],
15091513
)
15101514
}

‎crates/hir-ty/src/tests/simple.rs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3926,3 +3926,42 @@ fn foo<T: Bar>() {
39263926
"#]],
39273927
);
39283928
}
3929+
3930+
#[test]
3931+
fn asm_const_label() {
3932+
check_infer(
3933+
r#"
3934+
//- minicore: asm
3935+
const fn bar() -> i32 { 123 }
3936+
fn baz(s: &str) {}
3937+
3938+
fn foo() {
3939+
unsafe {
3940+
core::arch::asm!(
3941+
"mov eax, {}",
3942+
"jmp {}",
3943+
const bar(),
3944+
label {
3945+
baz("hello");
3946+
},
3947+
);
3948+
}
3949+
}
3950+
"#,
3951+
expect![[r#"
3952+
22..29 '{ 123 }': i32
3953+
24..27 '123': i32
3954+
37..38 's': &'? str
3955+
46..48 '{}': ()
3956+
!0..68 'builti...");},)': ()
3957+
!40..43 'bar': fn bar() -> i32
3958+
!40..45 'bar()': i32
3959+
!51..66 '{baz("hello");}': ()
3960+
!52..55 'baz': fn baz(&'? str)
3961+
!52..64 'baz("hello")': ()
3962+
!56..63 '"hello"': &'static str
3963+
59..257 '{ ... } }': ()
3964+
65..255 'unsafe... }': ()
3965+
"#]],
3966+
);
3967+
}

‎crates/ide-diagnostics/src/handlers/missing_unsafe.rs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -894,4 +894,25 @@ fn main() {
894894
"#,
895895
);
896896
}
897+
898+
#[test]
899+
fn asm_label() {
900+
check_diagnostics(
901+
r#"
902+
//- minicore: asm
903+
fn foo() {
904+
unsafe {
905+
core::arch::asm!(
906+
"jmp {}",
907+
label {
908+
let p = 0xDEADBEAF as *mut u8;
909+
*p = 3;
910+
// ^^ error: dereference of raw pointer is unsafe and requires an unsafe function or block
911+
},
912+
);
913+
}
914+
}
915+
"#,
916+
);
917+
}
897918
}

‎crates/parser/src/grammar/expressions/atom.rs‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -381,10 +381,14 @@ fn parse_asm_expr(p: &mut Parser<'_>, m: Marker) -> Option<CompletedMarker> {
381381
op.complete(p, ASM_REG_OPERAND);
382382
op_n.complete(p, ASM_OPERAND_NAMED);
383383
} else if p.eat_contextual_kw(T![label]) {
384+
// test asm_label
385+
// fn foo() {
386+
// builtin#asm("", label {});
387+
// }
384388
dir_spec.abandon(p);
385389
block_expr(p);
386-
op.complete(p, ASM_OPERAND_NAMED);
387-
op_n.complete(p, ASM_LABEL);
390+
op.complete(p, ASM_LABEL);
391+
op_n.complete(p, ASM_OPERAND_NAMED);
388392
} else if p.eat(T![const]) {
389393
dir_spec.abandon(p);
390394
expr(p);

‎crates/parser/test_data/generated/runner.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ mod ok {
2121
#[test]
2222
fn asm_expr() { run_and_expect_no_errors("test_data/parser/inline/ok/asm_expr.rs"); }
2323
#[test]
24+
fn asm_label() { run_and_expect_no_errors("test_data/parser/inline/ok/asm_label.rs"); }
25+
#[test]
2426
fn assoc_const_eq() {
2527
run_and_expect_no_errors("test_data/parser/inline/ok/assoc_const_eq.rs");
2628
}

‎crates/parser/test_data/parser/inline/ok/asm_label.rast‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
SOURCE_FILE
2+
FN
3+
FN_KW "fn"
4+
WHITESPACE " "
5+
NAME
6+
IDENT "foo"
7+
PARAM_LIST
8+
L_PAREN "("
9+
R_PAREN ")"
10+
WHITESPACE " "
11+
BLOCK_EXPR
12+
STMT_LIST
13+
L_CURLY "{"
14+
WHITESPACE "\n "
15+
EXPR_STMT
16+
ASM_EXPR
17+
BUILTIN_KW "builtin"
18+
POUND "#"
19+
ASM_KW "asm"
20+
L_PAREN "("
21+
LITERAL
22+
STRING "\"\""
23+
COMMA ","
24+
WHITESPACE " "
25+
ASM_OPERAND_NAMED
26+
ASM_LABEL
27+
LABEL_KW "label"
28+
WHITESPACE " "
29+
BLOCK_EXPR
30+
STMT_LIST
31+
L_CURLY "{"
32+
R_CURLY "}"
33+
R_PAREN ")"
34+
SEMICOLON ";"
35+
WHITESPACE "\n"
36+
R_CURLY "}"
37+
WHITESPACE "\n"

‎crates/parser/test_data/parser/inline/ok/asm_label.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
fn foo() {
2+
builtin#asm("", label {});
3+
}

0 commit comments

Comments
 (0)