| Message ID | 20250227-export-macro-v1-4-948775fc37aa@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Check Rust signatures at compile time | expand |
On Thu, Feb 27, 2025 at 05:02:02PM +0000, Alice Ryhl wrote: > This validates at compile time that the signatures match what is in the > header file. It highlights one annoyance with the compile-time check, > which is that it can only be used with functions marked unsafe. > > If the function is not unsafe, then this error is emitted: > > error[E0308]: `if` and `else` have incompatible types Is there a way to improve this error message? I vaguely remember there are ways to do customized error message. Regards, Boqun > --> <linux>/drivers/gpu/drm/drm_panic_qr.rs:987:19 > | > 986 | #[export] > | --------- expected because of this > 987 | pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize { > | ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected unsafe fn, found safe fn > | > = note: expected fn item `unsafe extern "C" fn(_, _) -> _ {kernel::bindings::drm_panic_qr_max_data_size}` > found fn item `extern "C" fn(_, _) -> _ {drm_panic_qr_max_data_size}` > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > drivers/gpu/drm/drm_panic.c | 5 ----- > drivers/gpu/drm/drm_panic_qr.rs | 15 +++++++++++---- > include/drm/drm_panic.h | 7 +++++++ > rust/bindings/bindings_helper.h | 4 ++++ > 4 files changed, 22 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c > index f128d345b16d..dee5301dd729 100644 > --- a/drivers/gpu/drm/drm_panic.c > +++ b/drivers/gpu/drm/drm_panic.c > @@ -486,11 +486,6 @@ static void drm_panic_qr_exit(void) > stream.workspace = NULL; > } > > -extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len); > - > -extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size, > - u8 *tmp, size_t tmp_size); > - > static int drm_panic_get_qr_code_url(u8 **qr_image) > { > struct kmsg_dump_iter iter; > diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs > index bcf248f69252..d055655aa0cd 100644 > --- a/drivers/gpu/drm/drm_panic_qr.rs > +++ b/drivers/gpu/drm/drm_panic_qr.rs > @@ -27,7 +27,10 @@ > //! * <https://github.com/bjguillot/qr> > > use core::cmp; > -use kernel::str::CStr; > +use kernel::{ > + prelude::*, > + str::CStr, > +}; > > #[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)] > struct Version(usize); > @@ -929,7 +932,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) { > /// * `tmp` must be valid for reading and writing for `tmp_size` bytes. > /// > /// They must remain valid for the duration of the function call. > -#[no_mangle] > +#[export] > pub unsafe extern "C" fn drm_panic_qr_generate( > url: *const kernel::ffi::c_char, > data: *mut u8, > @@ -980,8 +983,12 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) { > /// * If `url_len` > 0, remove the 2 segments header/length and also count the > /// conversion to numeric segments. > /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment. > -#[no_mangle] > -pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize { > +/// > +/// # Safety > +/// > +/// Always safe to call. > +#[export] > +pub unsafe extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize { > #[expect(clippy::manual_range_contains)] > if version < 1 || version > 40 { > return 0; > diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h > index f4e1fa9ae607..2a1536e0229a 100644 > --- a/include/drm/drm_panic.h > +++ b/include/drm/drm_panic.h > @@ -163,4 +163,11 @@ static inline void drm_panic_unlock(struct drm_device *dev, unsigned long flags) > > #endif > > +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE) > +extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len); > + > +extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size, > + u8 *tmp, size_t tmp_size); > +#endif > + > #endif /* __DRM_PANIC_H__ */ > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index 55354e4dec14..5345aa93fb8a 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -36,6 +36,10 @@ > #include <linux/workqueue.h> > #include <trace/events/rust_sample.h> > > +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE) > +#include <drm/drm_panic.h> > +#endif > + > /* `bindgen` gets confused at certain things. */ > const size_t RUST_CONST_HELPER_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN; > const size_t RUST_CONST_HELPER_PAGE_SIZE = PAGE_SIZE; > > -- > 2.48.1.658.g4767266eb4-goog >
On Thu, Feb 27, 2025 at 10:29 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Thu, Feb 27, 2025 at 05:02:02PM +0000, Alice Ryhl wrote: > > This validates at compile time that the signatures match what is in the > > header file. It highlights one annoyance with the compile-time check, > > which is that it can only be used with functions marked unsafe. > > > > If the function is not unsafe, then this error is emitted: > > > > error[E0308]: `if` and `else` have incompatible types > > Is there a way to improve this error message? I vaguely remember there > are ways to do customized error message. The if/else error message is super nice because it shows the two types next to each other where it's very easy to read the signatures and spot the difference. But if you want to investigate other constructs that potentially have better error messages, feel free. I'm happy to update if you find a better construct. Alice
"Alice Ryhl" <aliceryhl@google.com> writes: > This validates at compile time that the signatures match what is in the > header file. It highlights one annoyance with the compile-time check, > which is that it can only be used with functions marked unsafe. It would indeed be nice if there was a way to mark some functions to be emitted as safe by bindgen. > > If the function is not unsafe, then this error is emitted: > > error[E0308]: `if` and `else` have incompatible types > --> <linux>/drivers/gpu/drm/drm_panic_qr.rs:987:19 > | > 986 | #[export] > | --------- expected because of this > 987 | pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize { > | ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected unsafe fn, found safe fn > | > = note: expected fn item `unsafe extern "C" fn(_, _) -> _ {kernel::bindings::drm_panic_qr_max_data_size}` > found fn item `extern "C" fn(_, _) -> _ {drm_panic_qr_max_data_size}` > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org> Best regards, Andreas Hindborg
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index f128d345b16d..dee5301dd729 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -486,11 +486,6 @@ static void drm_panic_qr_exit(void) stream.workspace = NULL; } -extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len); - -extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size, - u8 *tmp, size_t tmp_size); - static int drm_panic_get_qr_code_url(u8 **qr_image) { struct kmsg_dump_iter iter; diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs index bcf248f69252..d055655aa0cd 100644 --- a/drivers/gpu/drm/drm_panic_qr.rs +++ b/drivers/gpu/drm/drm_panic_qr.rs @@ -27,7 +27,10 @@ //! * <https://github.com/bjguillot/qr> use core::cmp; -use kernel::str::CStr; +use kernel::{ + prelude::*, + str::CStr, +}; #[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)] struct Version(usize); @@ -929,7 +932,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) { /// * `tmp` must be valid for reading and writing for `tmp_size` bytes. /// /// They must remain valid for the duration of the function call. -#[no_mangle] +#[export] pub unsafe extern "C" fn drm_panic_qr_generate( url: *const kernel::ffi::c_char, data: *mut u8, @@ -980,8 +983,12 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) { /// * If `url_len` > 0, remove the 2 segments header/length and also count the /// conversion to numeric segments. /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment. -#[no_mangle] -pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize { +/// +/// # Safety +/// +/// Always safe to call. +#[export] +pub unsafe extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize { #[expect(clippy::manual_range_contains)] if version < 1 || version > 40 { return 0; diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h index f4e1fa9ae607..2a1536e0229a 100644 --- a/include/drm/drm_panic.h +++ b/include/drm/drm_panic.h @@ -163,4 +163,11 @@ static inline void drm_panic_unlock(struct drm_device *dev, unsigned long flags) #endif +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE) +extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len); + +extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size, + u8 *tmp, size_t tmp_size); +#endif + #endif /* __DRM_PANIC_H__ */ diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 55354e4dec14..5345aa93fb8a 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -36,6 +36,10 @@ #include <linux/workqueue.h> #include <trace/events/rust_sample.h> +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE) +#include <drm/drm_panic.h> +#endif + /* `bindgen` gets confused at certain things. */ const size_t RUST_CONST_HELPER_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN; const size_t RUST_CONST_HELPER_PAGE_SIZE = PAGE_SIZE;
This validates at compile time that the signatures match what is in the header file. It highlights one annoyance with the compile-time check, which is that it can only be used with functions marked unsafe. If the function is not unsafe, then this error is emitted: error[E0308]: `if` and `else` have incompatible types --> <linux>/drivers/gpu/drm/drm_panic_qr.rs:987:19 | 986 | #[export] | --------- expected because of this 987 | pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize { | ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected unsafe fn, found safe fn | = note: expected fn item `unsafe extern "C" fn(_, _) -> _ {kernel::bindings::drm_panic_qr_max_data_size}` found fn item `extern "C" fn(_, _) -> _ {drm_panic_qr_max_data_size}` Signed-off-by: Alice Ryhl <aliceryhl@google.com> --- drivers/gpu/drm/drm_panic.c | 5 ----- drivers/gpu/drm/drm_panic_qr.rs | 15 +++++++++++---- include/drm/drm_panic.h | 7 +++++++ rust/bindings/bindings_helper.h | 4 ++++ 4 files changed, 22 insertions(+), 9 deletions(-)