Warn when passing pointers to asm! with nomem/readonly options #127063
Conversation
rustbot
added
S-waiting-on-review
|
r? @Amanieu |
| } | ||
| if asm.options.contains(InlineAsmOptions::NOMEM) { | ||
| let msg = "Passing a pointer to asm!() with 'nomem' option"; | ||
| let note = "Pointer suggest that this piece of assembly reads the underlying object, consider using usize or checking the options"; |
There was a problem hiding this comment.
why is a usize preferable?
There was a problem hiding this comment.
That was just what came to my mind when writing this at midnight. The warning and note could definitely be better, for example now I'm thinking of this:
warning: passing a pointer to asm! block with `nomem` option
--> $PATH:$LINE
|
| core::arch::asm!("mov {p}, {p}", p = in(reg) p, options(nomem, nostack, preserves_flags));
| ^
= note: `nomem` must be only used when no memory read or write happens inside the asm! block.
= note: This is not limited to global variables, it also includes passed pointers.
= note: If this is intentional, consider casting the pointer to an integer.
= note: If not, change the `nomem` attribute to `readonly`
Warning is concise and note explains in more detail what is going on and how to fix the issue. Of course, depending on the pointer mutability and asm options, they will be a bit different.
There was a problem hiding this comment.
I think this is probably underexplained and should be omitted:
= note: If this is intentional, consider casting the pointer to an integer.
Assembly is unsafe and we should be careful to eschew recommendations that aren't clearly applicable, because people will blindly take recommendations without thinking hard (no matter how much we would like to believe otherwise).
Further, people who work with assembly often have the interesting belief that pointers and integers are the same thing, so I can imagine someone misunderstanding this and the next line as somehow equivalent:
= note: If this is intentional, consider casting the pointer to an integer.
= note: If not, change the `nomem` attribute to `readonly`
So I think the suggestions should be more conservative, only including:
- remove nomem (as this is safest)
- replace nomem with readonly (less safe, but may be sound)
- allow this lint (would require moving this into the linting framework, but clearly has no soundness impacts)
There was a problem hiding this comment.
It is probably fine if there's a good way to explain why the cast to integer would be okay (so that people don't do it if it's not okay), but it seems sus, honestly.
| .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.") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
| .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.") |
There was a problem hiding this comment.
Same thing here, if passing the pointer is intentional then our recommendation should not involve changing the semantics of asm!.
There was a problem hiding this 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. 🤔
There was a problem hiding this comment.
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 basicallyptr.addr()it or something." - "I intended to pass
*mut Tbut 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...)"
|
Considering the fact that this suggestion isn't always applicable, I think this makes more sense as clippy lint than a built-in rustc lint. I would encourage you to submit it there. |
Add new check for passing pointers to an `asm!` block with `nomem` option changelog: Add new check for passing pointers to an `asm!` block with `nomem` option Continuing work from rust-lang/rust#127063
I think these warnings would help a lot finding weird bugs caused by improper
optionsinsideasm!, while allowing existing code to compile.