Re: [PATCH V5 2/2] rust: kernel: Add KUnit tests for powerpc ARCH_WARN_ASM bug table emission

[Mukesh Kumar Chaurasiya]

[...]
> This works for me under QEMU, but I have a couple of small suggestions.
> 
> 1. You should use the kernel CStr and kernel::ffi::c_* types, not the ones
> from core. This properly matches the way the kernel uses -funsigned-char.
> For any architectures where char is normally signed, this would fail to
> build. Which brings me to…
> 
Thanks for pointing it out, i missed this.
> 2. Why is this specific to powerpc? Shouldn't it be possible to do this in a
> way that works across more architectures? (Indeed, the current
> implementation happens to pass on x86_64 with the above fix, though more
> would be needed for other architectures, and there's probably a better
> solution overall which doesn't hardcode a special bug address.)
> 
> So this still seems like it's something worth having, but I'd much prefer it
> to not be architecture-specific if we can avoid it.
> 
> Cheers,
> -- David
> 
The dependency to support these on all archs as it is would be to probe
the implementation and figure out which will again cause it to have some
arch specific stuff. I couldn't find a clean way of having all that at
the same place without a lot of #cfg's so i just made it powerpc
specific.

Tomonori sent out a patch with a single test that's arch agnostic, but
for ppc I would still like to keep these if the maintainer agrees.

[...]
> 
> It'd be really nice to use the warn_flags!() macro here (or even all of
> warn_on!() instead of hardcoding a separate implementation here. But even if
> we can't (due, e.g., to the need for global_asm! vs asm!), it'd be nice if
> this were more similar to the warn_flags!() implementation. Not only would
> that reduce the likelihood of these deviating needlessly, but it'd better
> support more architectures, too.
> 
> For example, the commented "{size}" would help on architectures where size
> isn't needed.
> 
Let me try this.
[...]
> > +    /// With `CONFIG_DEBUG_BUGVERBOSE` the entry must record a non-null file
> > +    /// pointer pointing back into this source file.
> > +    #[test]
> > +    #[cfg(CONFIG_DEBUG_BUGVERBOSE)]
> > +    fn bug_entry_file() {
> > +        use core::ffi::CStr;
> 
> Please don't use core::ffi::* in the kernel. The kernel provides its own
> CStr and c_char implementations. Otherwise, this can lead to issues as the
> kernel builds with -funsigned-char, but core::ffi will use the architecture
> default.
> 
Sure.

> > +
> > +        assert!(trap_addr() != 0);
> > +        let entry = unsafe { bindings::find_bug(trap_addr()) };
> > +        assert!(!entry.is_null());
> > +
> > +        let mut file_ptr: *const core::ffi::c_char = core::ptr::null();
> 
> Again, core::ffi::c_char should be avoided in kernel code. Use
> [kernel::]ffi:c_char
> 
Sure.
[...]
> > +    /// With `CONFIG_DEBUG_BUGVERBOSE` the recorded line number must be
> > +    /// non-zero (a zero line would mean the asm operand was not substituted).
> > +    #[test]
> > +    #[cfg(CONFIG_DEBUG_BUGVERBOSE)]
> > +    fn bug_entry_line() {
> > +        assert!(trap_addr() != 0);
> > +        let entry = unsafe { bindings::find_bug(trap_addr()) };
> > +        assert!(!entry.is_null());
> > +
> > +        let mut file_ptr: *const core::ffi::c_char = core::ptr::null();
> 
> And again.
> 
Sure 

Regards,
Mukesh
[...]