Message-ID: <qefx2cny4g3v6gvafw2gqx6bqncoxbo37vtbutvgofezvof3uh@zu7vthspxs3u>
Date: Fri, 16 Feb 2024 17:12:31 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Jiri Slaby <jirislaby@...nel.org>
Cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Arnd Bergmann <arnd@...db.de>, 
	Jiaxun Yang <jiaxun.yang@...goat.com>, Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>, 
	Stephen Rothwell <sfr@...hwell.id.au>, Andrew Morton <akpm@...ux-foundation.org>, 
	linux-mips@...r.kernel.org, linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] tty: mips_ejtag_fdc: Fix passing incompatible
 pointer type warning

On Fri, Feb 16, 2024 at 06:51:01AM +0100, Jiri Slaby wrote:
> On 15. 02. 24, 18:17, Serge Semin wrote:
> > mips_ejtag_fdc_encode() method expects having a first argument passed of
> > the "u8 **" type, meanwhile the driver passes the "const char **" type.
> > That causes the next build-warning:
> > 
> > drivers/tty/mips_ejtag_fdc.c: In function ‘mips_ejtag_fdc_console_write’:
> > drivers/tty/mips_ejtag_fdc.c:343:32: error: passing argument 1 of ‘mips_ejtag_fdc_encode’ from incompatible pointer type [-Werror=incompatible-pointer-types]
> >     word = mips_ejtag_fdc_encode(&buf_ptr, &buf_len, 1);
> >                                  ^
> > drivers/tty/mips_ejtag_fdc.c:216:24: note: expected ‘const u8 ** {aka const unsigned char **}’ but argument is of type ‘const char **’
> >   static struct fdc_word mips_ejtag_fdc_encode(const u8 **ptrs,
> >                          ^~~~~~~~~~~~~~~~~~~~~
> 
> It's a correct change. But why the compiler complains, given
> KBUILD_CFLAGS += -funsigned-char
> ?

What a tricky question you asked.) It cost me some new gray hairs, but
I guess I figured the correct answer out.

First of all it turns out that "char", "signed char" and "unsigned
char" are three _distant_ types. The "-funsigned-char/-fsigned-char"
flag changes the signedness of the plain "char", but it doesn't make
it matching to any of "signed char" or "unsigned char". Thus the flag
you mentioned couldn't suppress the warning I discovered (a bit later
you'll see that it is unrelated to that flag, but to the fact that the
types are different). Here is a simple test-code illustrating what I
said above:

1: int main(int argc, char *argv[], char *env[])
2: {
3:         char c;
4:         char *cp = &c;
5:         signed char sc;
6:         signed char *scp1 = &c;
7:         signed char *scp2 = &sc;
8:         unsigned char uc;
9:         unsigned char *ucp1 = &c;
10:        unsigned char *ucp2 = &uc;
11:        return 0;
12: }

$ gcc -Wall -Wno-unused-variable -funsigned-char tmp_char.c -o tmp_char
tmp_char.c: In function ‘main’:
tmp_char.c:6:29: warning: pointer targets in initialization of ‘signed char *’ from ‘char *’ differ in signedness [-Wpointer-sign]
    6 |         signed char *scp1 = &c;
      |                             ^
tmp_char.c:9:31: warning: pointer targets in initialization of ‘unsigned char *’ from ‘char *’ differ in signedness [-Wpointer-sign]
    9 |         unsigned char *ucp1 = &c;
      |                               ^
$

See, a new warning (not as the one in the patch log) is printed
despite of having the "-funsigned-char" flag specified. A more
detailed discussion around that you can find here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=28912
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=23087
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71202

The next question would be: "Why don't we see such warning printed all
over the kernel then?" That's because the "-Wno-pointer-sign" warning
suppressor is enabled in the kernel. It shuts up the sign-mismatching
pointers cast warnings. So no warning will be printed if the test-code
above is compiled as:

$ gcc -Wall -Wno-unused-variable -funsigned-char -Wno-pointer-sign tmp_char.c -o tmp_char
$

But why do we still see a warning as mentioned in the patch log? It
turns out that the "-Wno-pointer-sign" flag only works for the
_simple_ pointer signedness mismatch, but not for the multi-level
pointers. So as long as there is a pointer-to-pointer or
pointer-to-pointer-to-pointer, etc involved, another warning will be
printed. Here is the test-code modified to re-produce the compile
warning cited in the patch log:

1: int main(int argc, char *argv[], char *env[])
2: {
3:         char c;
4:         char *cp = &c;
5:         signed char sc;
6:         signed char *scp1 = &c;
7:         signed char *scp2 = &sc;
8:         signed char **scpp = &cp;
9:         unsigned char uc;
10:        unsigned char *ucp1 = &c;
11:        unsigned char *ucp2 = &uc;
12:        unsigned char **ucpp = &cp;
13:        return 0;
14: }

$ gcc -Wall -Wno-unused-variable -funsigned-char -Wno-pointer-sign tmp_char.c -o tmp_char
tmp_char.c: In function ‘main’:
tmp_char.c:8:30: warning: initialization of ‘signed char **’ from incompatible pointer type ‘char **’ [-Wincompatible-pointer-types]
    8 |         signed char **scpp = &cp;
      |                              ^
tmp_char.c:12:32: warning: initialization of ‘unsigned char **’ from incompatible pointer type ‘char **’ [-Wincompatible-pointer-types]
   12 |         unsigned char **ucpp = &cp;
      |                                ^
$

See, the lines 6 and 10 don't cause any warning printed (due to the
"-Wno-pointer-sign" flag), but the new lines 8 and 12 do. This is the
case that simulates what was discovered in the
drivers/tty/mips_ejtag_fdc.c driver and what was fixed in this patch.
I don't know for sure but I guess the compiler considers the
high-level pointers a bit differently than the single-level ones. So
the pointers to different pointer types are considered as
incompatible, which is also relevant for the char-family types since
these are three distant types. Thus that's what the warning about.

> 
> > Fix it by altering the type of the pointer which is passed to the
> > mips_ejtag_fdc_encode() method.
> > 
> > Fixes: ce7cbd9a6c81 ("tty: mips_ejtag_fdc: use u8 for character pointers")
> > Signed-off-by: Serge Semin <fancer.lancer@...il.com>
> > ---
> >   drivers/tty/mips_ejtag_fdc.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/mips_ejtag_fdc.c b/drivers/tty/mips_ejtag_fdc.c
> > index aac80b69a069..afbf7738c7c4 100644
> > --- a/drivers/tty/mips_ejtag_fdc.c
> > +++ b/drivers/tty/mips_ejtag_fdc.c
> > @@ -309,7 +309,7 @@ static void mips_ejtag_fdc_console_write(struct console *c, const char *s,
> >   	unsigned int i, buf_len, cpu;
> >   	bool done_cr = false;
> >   	char buf[4];
> > -	const char *buf_ptr = buf;
> > +	const u8 *buf_ptr = buf;
> >   	/* Number of bytes of input data encoded up to each byte in buf */
> >   	u8 inc[4];
> 
> thanks,

Please explicitly add your tag if you are ok with the fix.

-Serge(y)

> -- 
> js
> suse labs
> 

Powered by blists - more mailing lists