[PATCH] drm/panic: fix compilation issue on ARM

[PATCH] drm/panic: fix compilation issue on ARM

[Emmanuel Gil Peyrot]

In C, the char type is specified with “The implementation shall define char to
have the same range, representation, and behavior as either signed char or
unsigned char.”

On x86 it defaults to signed char, and on ARM it defaults to unsigned char.
This carries over to Rust’s FFI, which aliases its c_char type to i8 on x86,
and to u8 on ARM.

We can fix this error by using the *const c_char type wherever it is needed,
rather than assuming we are on x86 by passing *const i8.

The only other place which uses the CStr::from_char_ptr() function is
errname(), which already uses c_char correctly thanks to bindgen.

Here is the error I encountered, building latest master:
```
error[E0308]: mismatched types
   --> drivers/gpu/drm/drm_panic_qr.rs:961:60
    |
961 |         let url_cstr: &CStr = unsafe { CStr::from_char_ptr(url) };
    |                                        ------------------- ^^^ expected `*const u8`, found `*const i8`
    |                                        |
    |                                        arguments to this function are incorrect
    |
    = note: expected raw pointer `*const u8`
               found raw pointer `*const i8`
note: associated function defined here
   --> rust/kernel/str.rs:187:19
    |
187 |     pub unsafe fn from_char_ptr<'a>(ptr: *const crate::ffi::c_char) -> &'a Self {
    |                   ^^^^^^^^^^^^^

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0308`.
```
---
 drivers/gpu/drm/drm_panic_qr.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
index ef2d490965ba..39ddb431b361 100644
--- a/drivers/gpu/drm/drm_panic_qr.rs
+++ b/drivers/gpu/drm/drm_panic_qr.rs
@@ -27,6 +27,7 @@
 //! * <https://github.com/bjguillot/qr>
 
 use core::cmp;
+use kernel::ffi::c_char;
 use kernel::str::CStr;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)]
@@ -931,7 +932,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
 /// They must remain valid for the duration of the function call.
 #[no_mangle]
 pub unsafe extern "C" fn drm_panic_qr_generate(
-    url: *const i8,
+    url: *const c_char,
     data: *mut u8,
     data_len: usize,
     data_size: usize,
-- 
2.48.1

Re: [PATCH] drm/panic: fix compilation issue on ARM

[Miguel Ojeda]

Hi Emmanuel,

On Mon, Jan 20, 2025 at 1:45 PM Emmanuel Gil Peyrot
<linkmauve@linkmauve.fr> wrote:
>
> In C, the char type is specified with “The implementation shall define char to
> have the same range, representation, and behavior as either signed char or
> unsigned char.”
>
> On x86 it defaults to signed char, and on ARM it defaults to unsigned char.
> This carries over to Rust’s FFI, which aliases its c_char type to i8 on x86,
> and to u8 on ARM.

In the kernel `-funsigned-char` is used, see commit 3bc753c06dd0
("kbuild: treat char as always unsigned").

In any case, the change is fine, because we want to use the proper
type, but this is already in the Rust PR for this cycle, which should
land in mainline in some days, see commit 27c7518e7f1c ("rust: finish
using custom FFI integer types") in rust-next.

However, I am nevertheless confused, because in mainline
`crate::ffi::c_char` is `core::ffi::c_char` which is `i8` in both
arm64 and x86_64 and thus there is no build issue there.

If by ARM you mean 32-bit, then we don't have support for it yet in
mainline, so you shouldn't be able to see it there either.

Could you please clarify? Are you using patches on top of mainline,
e.g. the 32-bit arm support one?

Thanks!

Cheers,
Miguel

Re: [PATCH] drm/panic: fix compilation issue on ARM

[Link Mauve]

On Mon, Jan 20, 2025 at 10:52:48PM +0100, Miguel Ojeda wrote:
> Hi Emmanuel,

Hi Miguel,

> 
> On Mon, Jan 20, 2025 at 1:45 PM Emmanuel Gil Peyrot
> <linkmauve@linkmauve.fr> wrote:
> >
> > In C, the char type is specified with “The implementation shall define char to
> > have the same range, representation, and behavior as either signed char or
> > unsigned char.”
> >
> > On x86 it defaults to signed char, and on ARM it defaults to unsigned char.
> > This carries over to Rust’s FFI, which aliases its c_char type to i8 on x86,
> > and to u8 on ARM.
> 
> In the kernel `-funsigned-char` is used, see commit 3bc753c06dd0
> ("kbuild: treat char as always unsigned").
> 
> In any case, the change is fine, because we want to use the proper
> type, but this is already in the Rust PR for this cycle, which should
> land in mainline in some days, see commit 27c7518e7f1c ("rust: finish
> using custom FFI integer types") in rust-next.

Oh I see, I hadn’t read that merge request before submitting this fix, I
just tried to build the current kernel and it failed and I identified it
as a common issue I’ve already experienced in userspace.

> 
> However, I am nevertheless confused, because in mainline
> `crate::ffi::c_char` is `core::ffi::c_char` which is `i8` in both
> arm64 and x86_64 and thus there is no build issue there.

That’s weird, I clearly see the error I mentioned in my first email,
when building with today’s nightly compiler (maybe that’s relevant?).

> 
> If by ARM you mean 32-bit, then we don't have support for it yet in
> mainline, so you shouldn't be able to see it there either.

I am building an AArch64 kernel from an amd64 build computer.

> 
> Could you please clarify? Are you using patches on top of mainline,
> e.g. the 32-bit arm support one?

Nope, I was building a 6.13.0 kernel straight from torvalds’s master
branch, so the issue will likely be present on the 6.13.y branch as
well.  Perhaps my patch can be applied to that branch only, if rust-next
will be merged soon.  Otherwise I guess we could also live without
drm_panic support on AArch64 for the 6.13 kernel.

> 
> Thanks!
> 
> Cheers,
> Miguel

-- 
Link Mauve

Re: [PATCH] drm/panic: fix compilation issue on ARM

[Miguel Ojeda]

On Mon, Jan 20, 2025 at 11:03 PM Link Mauve <linkmauve@linkmauve.fr> wrote:
>
> when building with today’s nightly compiler (maybe that’s relevant?).

Ah, that explains it! The type changes with beta (1.85.0), and I can
also reproduce it in CE.

We should probably try to backport the cleanups we did before the
remapping, i.e. the commit I mentioned in the previous reply, so I
will see if those apply cleanly. If that does not work, then a
targeted change can be made.

Cheers,
Miguel

Re: [PATCH] drm/panic: fix compilation issue on ARM

[Alice Ryhl]

On 1/20/25 10:52 PM, Miguel Ojeda wrote:
> Hi Emmanuel,
> 
> On Mon, Jan 20, 2025 at 1:45 PM Emmanuel Gil Peyrot
> <linkmauve@linkmauve.fr> wrote:
>>
>> In C, the char type is specified with “The implementation shall define char to
>> have the same range, representation, and behavior as either signed char or
>> unsigned char.”
>>
>> On x86 it defaults to signed char, and on ARM it defaults to unsigned char.
>> This carries over to Rust’s FFI, which aliases its c_char type to i8 on x86,
>> and to u8 on ARM.
> 
> In the kernel `-funsigned-char` is used, see commit 3bc753c06dd0
> ("kbuild: treat char as always unsigned").
> 
> In any case, the change is fine, because we want to use the proper
> type, but this is already in the Rust PR for this cycle, which should
> land in mainline in some days, see commit 27c7518e7f1c ("rust: finish
> using custom FFI integer types") in rust-next.
> 
> However, I am nevertheless confused, because in mainline
> `crate::ffi::c_char` is `core::ffi::c_char` which is `i8` in both
> arm64 and x86_64 and thus there is no build issue there.
Isn't core::ffi::c_char an u8 on arm64?

Alice