Skip to content

Warn when passing pointers to asm! with nomem/readonly options #127063

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Warn when passing pointers to asm! with nomem/readonly options #127063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 30 additions & 24 deletions compiler/rustc_hir_analysis/src/check/intrinsicck.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use rustc_ast::InlineAsmTemplatePiece;
use rustc_ast::{InlineAsmOptions, InlineAsmTemplatePiece};
use rustc_data_structures::fx::FxIndexSet;
use rustc_hir::{self as hir, LangItem};
use rustc_middle::bug;
Expand Down Expand Up @@ -124,7 +124,7 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> {
idx: usize,
reg: InlineAsmRegOrRegClass,
expr: &'tcx hir::Expr<'tcx>,
template: &[InlineAsmTemplatePiece],
asm: &hir::InlineAsm<'tcx>,
is_input: bool,
tied_input: Option<(&'tcx hir::Expr<'tcx>, Option<InlineAsmType>)>,
target_features: &FxIndexSet<Symbol>,
Expand Down Expand Up @@ -267,7 +267,7 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> {
// Search for any use of this operand without a modifier and emit
// the suggestion for them.
let mut spans = vec![];
for piece in template {
for piece in asm.template {
if let &InlineAsmTemplatePiece::Placeholder { operand_idx, modifier, span } = piece
{
if operand_idx == idx && modifier.is_none() {
Expand Down Expand Up @@ -299,6 +299,28 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> {
}
}

match *ty.kind() {
ty::RawPtr(_, hir::Mutability::Mut) if asm.options.contains(InlineAsmOptions::READONLY) =>
self
.tcx
.dcx()
.struct_span_warn(expr.span, "passing a mutable pointer to asm! block with 'readonly' option.")
.with_note("`readonly` means that no memory write happens inside the asm! block.")
.with_note("This is not limited to global variables, it also includes passed pointers.")
.with_note("If passing this mutable pointer is intentional, remove the `readonly` attribute.")
.emit(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this message is appropriate: if passing a mutable pointer is intentional, we should only suggest something which suppresses the warning, not something that changes the semantics of the asm!.

The problem here is that I don't see an obvious way to suppress the warning. Perhaps this lint would be better in clippy instead rather than something that generally applies to all Rust code?

Copy link
Contributor Author

@Soveu Soveu Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, now that I'm thinking about it, maybe you're right - *mut pointer should anyway implicitly cast to *const one. What do you think about the other case with *const/mut and nomem?

ty::RawPtr(_, _) if asm.options.contains(InlineAsmOptions::NOMEM) =>
self
.tcx
.dcx()
.struct_span_warn(expr.span, "passing a pointer to asm! block with 'nomem' option.")
.with_note("`nomem` means that no memory write or read happens inside the asm! block.")
.with_note("This is not limited to global variables, it also includes passed pointers.")
.with_note("If passing this pointer is intentional, replace the `nomem` attribute with `readonly` or remove it completely.")
.emit(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, if passing the pointer is intentional then our recommendation should not involve changing the semantics of asm!.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#127063 (comment)

The problem here is that I don't see an obvious way to suppress the warning. Perhaps this lint would be better in clippy instead rather than something that generally applies to all Rust code?

At first, I wanted to make it an error, but probably some code would break and asm! is stable (could we jump on the edition2024 bandwagon?). Yeah, this is unsafe, and we can do a lot of stuff inside the asm! block, including freestyle transmute, however transmute does one job and it is dangerous, we can't do much with it besides "being smart" and looking for surrounding context, which is what clippy does with eager_transmute for example.

Here we could impose impose a simple rule that would help avoiding bugs with unintentional nomem. I can agree that the case with *mut T and readonly could be allowed, because we could pretend that the pointer is implicitly cast to *const T (like when calling functions that take *const T) and also the difference between *mut and *const is mostly cosmetic, but I don't see the same applying to nomem + *const/mut T.

Why would someone pass a pointer and do nothing with the data? If they need just the address, maybe they also need to consider if the provenance should be exposed or not. Idk, I think you know better than me how asm! is used, so if the final decision will be to not implement it here, then I won't insist. 🤔

Copy link
Member

@workingjubilee workingjubilee Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just noting that there's like two, maybe three versions of "intended/intention" floating around in this discussion. let me see if I can name all of them:

  • "I intended to pass *mut T + nomem: I will basically ptr.addr() it or something."
  • "I intended to pass *mut T but I meant readonly: just .cast_const() it."
  • "I intended to pass *mut T, and I'm totally gonna read it, lol, GIMME DAT UB!!! (no plz don't, let me remove nomem...)"

_ => {} // we're only interested in pointers when asm! has `nomem` or `readonly`
}

Some(asm_ty)
}

Expand Down Expand Up @@ -399,46 +421,30 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> {

match *op {
hir::InlineAsmOperand::In { reg, expr } => {
self.check_asm_operand_type(
idx,
reg,
expr,
asm.template,
true,
None,
target_features,
);
self.check_asm_operand_type(idx, reg, expr, asm, true, None, target_features);
}
hir::InlineAsmOperand::Out { reg, late: _, expr } => {
if let Some(expr) = expr {
self.check_asm_operand_type(
idx,
reg,
expr,
asm.template,
asm,
false,
None,
target_features,
);
}
}
hir::InlineAsmOperand::InOut { reg, late: _, expr } => {
self.check_asm_operand_type(
idx,
reg,
expr,
asm.template,
false,
None,
target_features,
);
self.check_asm_operand_type(idx, reg, expr, asm, false, None, target_features);
}
hir::InlineAsmOperand::SplitInOut { reg, late: _, in_expr, out_expr } => {
let in_ty = self.check_asm_operand_type(
idx,
reg,
in_expr,
asm.template,
asm,
true,
None,
target_features,
Expand All @@ -448,7 +454,7 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> {
idx,
reg,
out_expr,
asm.template,
asm,
false,
Some((in_expr, in_ty)),
target_features,
Expand Down
34 changes: 34 additions & 0 deletions tests/ui/asm/x86_64/passing-pointer-nomem-readonly.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//@ only-x86_64
//@ needs-asm-support
//@ build-pass

#![crate_type = "lib"]
#![no_std]

unsafe fn nomem_bad(p: &i32) {
core::arch::asm!("mov {p}, {p}", p = in(reg) p, options(nomem, nostack, preserves_flags));
//~^ WARNING passing a pointer to asm! block with 'nomem' option.
}

unsafe fn readonly_bad(p: &mut i32) {
core::arch::asm!("mov {p}, {p}", p = in(reg) p, options(readonly, nostack, preserves_flags));
//~^ WARNING passing a mutable pointer to asm! block with 'readonly' option.
}

unsafe fn nomem_good(p: &i32) {
core::arch::asm!("mov {p}, {p}", p = in(reg) p, options(readonly, nostack, preserves_flags));
let p = p as *const i32 as usize;
core::arch::asm!("mov {p}, {p}", p = in(reg) p, options(nomem, nostack, preserves_flags));
}

unsafe fn readonly_good(p: &mut i32) {
core::arch::asm!("mov {p}, {p}", p = in(reg) p, options(nostack, preserves_flags));
core::arch::asm!("mov {p}, {p}", p = in(reg) &*p, options(readonly, nostack, preserves_flags));
let p = p as *const i32;
core::arch::asm!("mov {p}, {p}", p = in(reg) p, options(readonly, nostack, preserves_flags));
}

unsafe fn nomem_bad2(p: &mut i32) {
core::arch::asm!("mov {p}, {p}", p = in(reg) p, options(nomem, nostack, preserves_flags));
//~^ WARNING passing a pointer to asm! block with 'nomem' option.
}
32 changes: 32 additions & 0 deletions tests/ui/asm/x86_64/passing-pointer-nomem-readonly.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
warning: passing a pointer to asm! block with 'nomem' option.
--> $DIR/passing-pointer-nomem-readonly.rs:9:50
|
LL | core::arch::asm!("mov {p}, {p}", p = in(reg) p, options(nomem, nostack, preserves_flags));
| ^
|
= note: `nomem` means that no memory write or read happens inside the asm! block.
= note: This is not limited to global variables, it also includes passed pointers.
= note: If passing this pointer is intentional, replace the `nomem` attribute with `readonly` or remove it completely.

warning: passing a mutable pointer to asm! block with 'readonly' option.
--> $DIR/passing-pointer-nomem-readonly.rs:14:50
|
LL | core::arch::asm!("mov {p}, {p}", p = in(reg) p, options(readonly, nostack, preserves_flags));
| ^
|
= note: `readonly` means that no memory write happens inside the asm! block.
= note: This is not limited to global variables, it also includes passed pointers.
= note: If passing this mutable pointer is intentional, remove the `readonly` attribute.

warning: passing a pointer to asm! block with 'nomem' option.
--> $DIR/passing-pointer-nomem-readonly.rs:32:50
|
LL | core::arch::asm!("mov {p}, {p}", p = in(reg) p, options(nomem, nostack, preserves_flags));
| ^
|
= note: `nomem` means that no memory write or read happens inside the asm! block.
= note: This is not limited to global variables, it also includes passed pointers.
= note: If passing this pointer is intentional, replace the `nomem` attribute with `readonly` or remove it completely.

warning: 3 warnings emitted

Loading