| Message ID | 20240215171740.14550-5-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 188942f05ce45f80c06f7242ad7478bd204c3387 |
| Headers | show |
| Series | MIPS: Fix missing proto and passing arg warnings | expand |
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 ? > 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@gmail.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,
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 = ≻ 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 = ≻ 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@gmail.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 >
On Thu, Feb 15, 2024 at 08:17:29PM +0300, 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, > ^~~~~~~~~~~~~~~~~~~~~ > > 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@gmail.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]; applied to mips-next. Thomas.
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];
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, ^~~~~~~~~~~~~~~~~~~~~ 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@gmail.com> --- drivers/tty/mips_ejtag_fdc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)