[PATCH v1 5/5] rust: Add warn_on and warn_on_once

[FUJITA Tomonori]

On Wed, 11 Dec 2024 10:48:27 +0100
Alice Ryhl <aliceryhl at google.com> wrote:

>> >> +#[macro_export]
>> >> +#[doc(hidden)]
>> >> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))]
>> >> +#[cfg(any(target_arch = "x86_64", target_arch = "riscv64"))]
>> >
>> >> +#[macro_export]
>> >> +#[doc(hidden)]
>> >> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))]
>> >> +#[cfg(any(target_arch = "aarch64", target_arch = "loongarch64"))]
>> >
>> > What's the reason for this arch-specific code? The file!()/line!()
>> > invocations? Could they be passed as an argument to the asm instead so
>> > that we don't need target_arch cfgs? I understand that they don't work
>> > exactly the same way, but maybe it could still work?
>>
>> Because of "error: named argument never used" in Rust inline assembly:
>>
>> All the archs define ARCH_WARN_ASM macro in the same way:
>>
>> #define ARCH_WARN_ASM(file, line, flags, size)
>>
>> However, only x86 and risc-v asm code use the size argument. Without
>> the cfgs, I'll get the following on arm64/loongarch:
>>
>> error: named argument never used
>>   --> /home/fujita/git/linux-rust/drivers/block/rnull.rs:54:9
>>    |
>> 54 |         warn_on!(true);
>>    |         ^^^^^^^^^^^^^^ named argument never used
>>    |
>>    = help: if this argument is intentionally unused, consider using it in an asm comment: `"/* {size} */"`
>>    = note: this error originates in the macro `$crate::warn_flags` which comes from the expansion of the macro `warn_on` (in Nightly builds, run with -Z macro-backtrace for more info)
>>
>>
>> Any way to make the compiler to ignore this?
> 
> The error message suggests adding an asm comment. Does that not work?

Ah, works! I didn't understand what "an asm comment" is..

> You could even add the comment unconditionally on the Rust side. It's
> not like the comment hurts on the platforms that *do* use the size
> parameter.

Yeah, I confirmed that this also works for the architectures that
do use the size.

I'll remove the target_arch cfg in v2. Thanks!

>> >> +#[macro_export]
>> >> +#[doc(hidden)]
>> >> +#[cfg(all(CONFIG_BUG, CONFIG_UML))]
>> >> +macro_rules! warn_flags {
>> >> +    ($flags:expr) => {
>> >> +        // SAFETY: Just an FFI call.
>> >> +        unsafe {
>> >> +            $crate::bindings::warn_slowpath_fmt(
>> >> +                $crate::c_str!(::core::file!()).as_ptr() as *const ::core::ffi::c_char,
>> >> +                line!() as i32,
>> >> +                $flags as u32,
>> >> +                ::core::ptr::null() as *const ::core::ffi::c_char,
>> >
>> > I wonder if this could be written to utilize Location::caller()
>> > instead so that `#[track_caller]` works?
>>
>> You meant that we could make warn_flags() function instead of macro
>> with Location::caller()?
>>
>> If so, we need to add cfgs to warn_on and warn_on_once because both macro
>> and function of warn_flags are necessary?
> 
> Well, I'm not sure! I don't know if it's feasible at all, since using
> Location::caller() would mean that the file/line is not a compile-time
> constant. But if we can, then I think #[track_caller] support would be
> nice.

I think that using Location::caller() makes the code complicated
(needs more cfgs for UML). All the callers (warn_* and bug_*) of
warn_flags and warn_flags itself must be macro for all the archs
execpt for UML. We don't want to have the UML version of all the
callers.