rust: kernel: introduce `unsafe_precondition_assert!` macro
PATCH 1/1 rust: kernel: introduce `unsafe_precondition_assert!` macro
Introduce a new safety module containing unsafe_precondition_assert! macro. It is a wrapper around debug_assert!, intended for validating pre-conditions of unsafe code blocks and functions.
When CONFIG_RUST_DEBUG_ASSERTIONS flag is enabled, this macro performs runtime checks to ensure that the pre-conditions for unsafe blocks hold. In release builds, the macro is a no-op.
--- rust/kernel/lib.rs | 1 + rust/kernel/safety.rs | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 rust/kernel/safety.rs
rust/kernel/lib.rs +1 -0
@@ -101,6 +101,7 @@pub mod print;pub mod rbtree;pub mod revocable; +pub mod safety;pub mod security;pub mod seq_file;pub mod sizes;
rust/kernel/safety.rs new file +42 -0
42 lines not on this page · raw message
base-commit: 8ecb65b7b68ea48350833ba59c1257718e859768
Signature · 1 line
2.50.1
Quoted 3 lines · Ritvik Gupta
macro. It is a wrapper around
debug_assert!, intended for validating pre-conditions of unsafe code blocks and functions.
It isn't really a wrapper, since it doesn't call it, no?
rust/kernel/safety.rs line 4
Quoted 2 lines
rust/kernel/safety.rs new file +2
-0+//! This module contains the kernel APIs for verifying invariants +//! required by the unsafe code.
We normally call these preconditions, and normally I think "unsafe code" is associated with unsafe blocks, rather than e.g. unsafe fn.
In addition, we may want to use this module for more things, so I would probably just say something more general, e.g. "Safety-related APIs" or similar. Probably Benno can chime in here.
Quoted 1 line
+/// Checks that preconditions of an unsafe code are followed.
Same as above -- I would say unsafe function (if we really want to use this only for that).
Quoted 1 line
+/// are enabled. In release builds, this macro is no-op.
I would say "Otherwise, ...". Same in the commit message.
Quoted 1 line
+/// // SAFETY: The caller ensures the size and alignment
This should be a # Safety section, not a SAFETY comment. Please see how we usually write those in other files.
Quoted 1 line
macro_rules!unsafe_precondition_assert{
I wonder if we should call it safety_precondition_assert -- although I like the current name since it matches better the (unstable) Rust standard library one.
rust/kernel/safety.rs line 40
Quoted 4 lines
rust/kernel/safety.rs new file +4
-0+if cfg! (debug_assertions) { +crate ::pr_err!("unsafe precondition(s) violated" ); + ::core::assert!($($arg)*); + }
Doesn't this print an error every time debug assertions are enabled, whether or not the expression is true?
Also, shouldn't that be $crate?
This leads me to believe the patch wasn't tested -- please test patches before submitting them! (Or, if it couldn't be tested for some reason, please say so in the message)
Finally, could we actually wrap/forward the call to debug_assert!? If we want to add a custom message, could we match the parameters so that we can then prefix it?
Thanks for the patch!
Cheers,
Miguel
Quoted 6 lines · Miguel Ojeda
Quoted 3 lines · Ritvik Gupta
macro. It is a wrapper around
debug_assert!, intended for validating pre-conditions of unsafe code blocks and functions.It isn't really a wrapper, since it doesn't call it, no?
It was intended to work the same way as debug_assert! [1] with 'unsafe precondition(s) violated' message, if assertion fails.
Quoted 15 lines
rust/kernel/safety.rs line 4
Quoted 2 lines
rust/kernel/safety.rs new file +2
-0+//! This module contains the kernel APIs for verifying invariants +//! required by the unsafe code. We normally call these preconditions, and normally I think "unsafe code" is associated with unsafe blocks, rather than e.g.
unsafe fn.In addition, we may want to use this module for more things, so I would probably just say something more general, e.g. "Safety-related APIs" or similar. Probably Benno can chime in here.
Quoted 1 line
+/// Checks that preconditions of an unsafe code are followed.
Same as above -- I would say unsafe function (if we really want to use this only for that).
Quoted 8 lines
Quoted 1 line
+/// are enabled. In release builds, this macro is no-op.
I would say "Otherwise, ...". Same in the commit message.
Quoted 1 line
+/// // SAFETY: The caller ensures the size and alignment
This should be a
# Safetysection, not aSAFETYcomment. Please see how we usually write those in other files.
I'll fix the docs.
Yes, @Benno's input would be super helpful as well :)
Quoted 7 lines
rust/kernel/safety.rs line 40
Quoted 4 lines
rust/kernel/safety.rs new file +4
-0+if cfg! (debug_assertions) { +crate ::pr_err!("unsafe precondition(s) violated" ); + ::core::assert!($($arg)*); + }Doesn't this print an error every time debug assertions are enabled, whether or not the expression is true?
You're right! I messed up the testing part (explained later).
Quoted 1 line
Also, shouldn't that be
$crate?
I think, in this case, the compiler automatically resolved it to correct path. Nevertheless, I will use $crate.
Quoted 3 lines
This leads me to believe the patch wasn't tested -- please test patches before submitting them! (Or, if it couldn't be tested for some reason, please say so in the message)
The code did compile, but during testing I only verified the failing case and overlooked the passing one.
Quoted 3 lines
Finally, could we actually wrap/forward the call to
debug_assert!? If we want to add a custom message, could we match the parameters so that we can then prefix it?
Yes, I'll do this way in v2.
Thanks for the feedback :)
[1]: https://doc.rust-lang.org/src/core/macros/mod.rs.html#306-312
Just to be clear for v2,
the caller should be able to call this way?
```
// Allowed
unsafe_precondition_assert!(condition, "message");
unsafe_precondition_assert!(condition, "formatted message: {} {} {}", arg1, arg2, argn);
// Not allowed?
unsafe_precondition_assert!(condition)
```When the assertion fails, this will be the output:
``` precondition(s) violated: message passed by caller ```
Quoted 11 lines · Ritvik Gupta
the caller should be able to call this way?
``` // Allowed unsafe_precondition_assert!(condition, "message"); unsafe_precondition_assert!(condition, "formatted message: {} {} {}", arg1, arg2, argn); // Not allowed? unsafe_precondition_assert!(condition) ```
I think allowing the second one would align it with other macros, which is an advantage, and makes it a bit simpler to use for obvious cases.
On the other hand, assert_unsafe_precondition! upstream doesn't allow it (but it has different syntax anyway), and we may want to always have a proper message in these cases anyway.
So I think either way is OK. I would perhaps lean to being consistent with our other assertion macros, especially if it makes it simpler.
Thanks!
Cheers,
Miguel