| Message ID | 20180225172236.29650-7-malat@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/gamecube: make W=1 compilation errors free | expand |
Le 25/02/2018 à 18:22, Mathieu Malaterre a écrit : > Rewrite check size - 1 <= Y as size < Y since `size` is unsigned value. > Fix warning (treated as error in W=1): > > CC arch/powerpc/kernel/signal_32.o > In file included from ./include/linux/uaccess.h:14:0, > from ./include/asm-generic/termios-base.h:8, > from ./arch/powerpc/include/asm/termios.h:20, > from ./include/uapi/linux/termios.h:6, > from ./include/linux/tty.h:7, > from arch/powerpc/kernel/signal_32.c:36: > ./include/asm-generic/termios-base.h: In function ‘user_termio_to_kernel_termios’: > ./arch/powerpc/include/asm/uaccess.h:52:35: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits] > (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) > ^ > ./arch/powerpc/include/asm/uaccess.h:58:3: note: in expansion of macro ‘__access_ok’ > __access_ok((__force unsigned long)(addr), (size), get_fs())) > ^~~~~~~~~~~ > ./arch/powerpc/include/asm/uaccess.h:262:6: note: in expansion of macro ‘access_ok’ > if (access_ok(VERIFY_READ, __gu_addr, (size))) \ > ^~~~~~~~~ > ./arch/powerpc/include/asm/uaccess.h:80:2: note: in expansion of macro ‘__get_user_check’ > __get_user_check((x), (ptr), sizeof(*(ptr))) > ^~~~~~~~~~~~~~~~ > ./include/asm-generic/termios-base.h:36:6: note: in expansion of macro ‘get_user’ > if (get_user(termios->c_line, &termio->c_line) < 0) > ^~~~~~~~ > [...] > cc1: all warnings being treated as errors > > Signed-off-by: Mathieu Malaterre <malat@debian.org> > --- > arch/powerpc/include/asm/uaccess.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h > index 51bfeb8777f0..fadc406bd39d 100644 > --- a/arch/powerpc/include/asm/uaccess.h > +++ b/arch/powerpc/include/asm/uaccess.h > @@ -49,7 +49,7 @@ > > #define __access_ok(addr, size, segment) \ > (((addr) <= (segment).seg) && \ > - (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) > + (((size) == 0) || ((size) < ((segment).seg - (addr))))) IIUC, ((2 - 1) <= 1) is the same as (2 < 1) ????? Christophe > > #endif > >
Le 26/02/2018 à 07:34, Christophe LEROY a écrit : > > > Le 25/02/2018 à 18:22, Mathieu Malaterre a écrit : >> Rewrite check size - 1 <= Y as size < Y since `size` is unsigned value. >> Fix warning (treated as error in W=1): >> >> CC arch/powerpc/kernel/signal_32.o >> In file included from ./include/linux/uaccess.h:14:0, >> from ./include/asm-generic/termios-base.h:8, >> from ./arch/powerpc/include/asm/termios.h:20, >> from ./include/uapi/linux/termios.h:6, >> from ./include/linux/tty.h:7, >> from arch/powerpc/kernel/signal_32.c:36: >> ./include/asm-generic/termios-base.h: In function >> ‘user_termio_to_kernel_termios’: >> ./arch/powerpc/include/asm/uaccess.h:52:35: error: comparison of >> unsigned expression >= 0 is always true [-Werror=type-limits] >> (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) >> ^ >> ./arch/powerpc/include/asm/uaccess.h:58:3: note: in expansion of macro >> ‘__access_ok’ >> __access_ok((__force unsigned long)(addr), (size), get_fs())) >> ^~~~~~~~~~~ >> ./arch/powerpc/include/asm/uaccess.h:262:6: note: in expansion of >> macro ‘access_ok’ >> if (access_ok(VERIFY_READ, __gu_addr, (size))) \ >> ^~~~~~~~~ >> ./arch/powerpc/include/asm/uaccess.h:80:2: note: in expansion of macro >> ‘__get_user_check’ >> __get_user_check((x), (ptr), sizeof(*(ptr))) >> ^~~~~~~~~~~~~~~~ >> ./include/asm-generic/termios-base.h:36:6: note: in expansion of macro >> ‘get_user’ >> if (get_user(termios->c_line, &termio->c_line) < 0) >> ^~~~~~~~ >> [...] >> cc1: all warnings being treated as errors >> >> Signed-off-by: Mathieu Malaterre <malat@debian.org> >> --- >> arch/powerpc/include/asm/uaccess.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/include/asm/uaccess.h >> b/arch/powerpc/include/asm/uaccess.h >> index 51bfeb8777f0..fadc406bd39d 100644 >> --- a/arch/powerpc/include/asm/uaccess.h >> +++ b/arch/powerpc/include/asm/uaccess.h >> @@ -49,7 +49,7 @@ >> #define __access_ok(addr, size, segment) \ >> (((addr) <= (segment).seg) && \ >> - (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) >> + (((size) == 0) || ((size) < ((segment).seg - (addr))))) > > IIUC, ((2 - 1) <= 1) is the same as (2 < 1) ????? Note that I already try to submit a fix for this warning 3 years ago (https://patchwork.ozlabs.org/patch/418075/) and it was rejected with the following comment: Again, I don't think Linux enables this warning. What did you do to produce this? In any case, it's a bad warning that doesn't take macros into account, and the answer is not to make the code less clear by hiding the fact that zero is a special case. Christophe > > Christophe > >> #endif >>
On Mon, Feb 26, 2018 at 7:50 AM, Christophe LEROY <christophe.leroy@c-s.fr> wrote: > > > Le 26/02/2018 à 07:34, Christophe LEROY a écrit : >> >> >> >> Le 25/02/2018 à 18:22, Mathieu Malaterre a écrit : >>> >>> Rewrite check size - 1 <= Y as size < Y since `size` is unsigned value. >>> Fix warning (treated as error in W=1): >>> >>> CC arch/powerpc/kernel/signal_32.o >>> In file included from ./include/linux/uaccess.h:14:0, >>> from ./include/asm-generic/termios-base.h:8, >>> from ./arch/powerpc/include/asm/termios.h:20, >>> from ./include/uapi/linux/termios.h:6, >>> from ./include/linux/tty.h:7, >>> from arch/powerpc/kernel/signal_32.c:36: >>> ./include/asm-generic/termios-base.h: In function >>> ‘user_termio_to_kernel_termios’: >>> ./arch/powerpc/include/asm/uaccess.h:52:35: error: comparison of unsigned >>> expression >= 0 is always true [-Werror=type-limits] >>> (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) >>> ^ >>> ./arch/powerpc/include/asm/uaccess.h:58:3: note: in expansion of macro >>> ‘__access_ok’ >>> __access_ok((__force unsigned long)(addr), (size), get_fs())) >>> ^~~~~~~~~~~ >>> ./arch/powerpc/include/asm/uaccess.h:262:6: note: in expansion of macro >>> ‘access_ok’ >>> if (access_ok(VERIFY_READ, __gu_addr, (size))) \ >>> ^~~~~~~~~ >>> ./arch/powerpc/include/asm/uaccess.h:80:2: note: in expansion of macro >>> ‘__get_user_check’ >>> __get_user_check((x), (ptr), sizeof(*(ptr))) >>> ^~~~~~~~~~~~~~~~ >>> ./include/asm-generic/termios-base.h:36:6: note: in expansion of macro >>> ‘get_user’ >>> if (get_user(termios->c_line, &termio->c_line) < 0) >>> ^~~~~~~~ >>> [...] >>> cc1: all warnings being treated as errors >>> >>> Signed-off-by: Mathieu Malaterre <malat@debian.org> >>> --- >>> arch/powerpc/include/asm/uaccess.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/powerpc/include/asm/uaccess.h >>> b/arch/powerpc/include/asm/uaccess.h >>> index 51bfeb8777f0..fadc406bd39d 100644 >>> --- a/arch/powerpc/include/asm/uaccess.h >>> +++ b/arch/powerpc/include/asm/uaccess.h >>> @@ -49,7 +49,7 @@ >>> #define __access_ok(addr, size, segment) \ >>> (((addr) <= (segment).seg) && \ >>> - (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) >>> + (((size) == 0) || ((size) < ((segment).seg - (addr))))) >> >> >> IIUC, ((2 - 1) <= 1) is the same as (2 < 1) ????? > The whole series was pretty mediocre, but this one was actually pretty destructive. Thanks for catching this. > > Note that I already try to submit a fix for this warning 3 years ago > (https://patchwork.ozlabs.org/patch/418075/) and it was rejected with the > following comment: > > Again, I don't think Linux enables this warning. What did you do to > produce this? In any case, it's a bad warning that doesn't take macros > into account, and the answer is not to make the code less clear by hiding > the fact that zero is a special case. Right. I'll try to see how to make W=1 run without error with an alternate solution. > Christophe > > >> >> Christophe >> >>> #endif >>> >
On Mon, Feb 26, 2018 at 8:44 AM, Mathieu Malaterre <malat@debian.org> wrote: > On Mon, Feb 26, 2018 at 7:50 AM, Christophe LEROY > <christophe.leroy@c-s.fr> wrote: >> >> >> Le 26/02/2018 à 07:34, Christophe LEROY a écrit : >>> >>> >>> >>> Le 25/02/2018 à 18:22, Mathieu Malaterre a écrit : >>>> >>>> Rewrite check size - 1 <= Y as size < Y since `size` is unsigned value. >>>> Fix warning (treated as error in W=1): >>>> >>>> CC arch/powerpc/kernel/signal_32.o >>>> In file included from ./include/linux/uaccess.h:14:0, >>>> from ./include/asm-generic/termios-base.h:8, >>>> from ./arch/powerpc/include/asm/termios.h:20, >>>> from ./include/uapi/linux/termios.h:6, >>>> from ./include/linux/tty.h:7, >>>> from arch/powerpc/kernel/signal_32.c:36: >>>> ./include/asm-generic/termios-base.h: In function >>>> ‘user_termio_to_kernel_termios’: >>>> ./arch/powerpc/include/asm/uaccess.h:52:35: error: comparison of unsigned >>>> expression >= 0 is always true [-Werror=type-limits] >>>> (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) >>>> ^ >>>> ./arch/powerpc/include/asm/uaccess.h:58:3: note: in expansion of macro >>>> ‘__access_ok’ >>>> __access_ok((__force unsigned long)(addr), (size), get_fs())) >>>> ^~~~~~~~~~~ >>>> ./arch/powerpc/include/asm/uaccess.h:262:6: note: in expansion of macro >>>> ‘access_ok’ >>>> if (access_ok(VERIFY_READ, __gu_addr, (size))) \ >>>> ^~~~~~~~~ >>>> ./arch/powerpc/include/asm/uaccess.h:80:2: note: in expansion of macro >>>> ‘__get_user_check’ >>>> __get_user_check((x), (ptr), sizeof(*(ptr))) >>>> ^~~~~~~~~~~~~~~~ >>>> ./include/asm-generic/termios-base.h:36:6: note: in expansion of macro >>>> ‘get_user’ >>>> if (get_user(termios->c_line, &termio->c_line) < 0) >>>> ^~~~~~~~ >>>> [...] >>>> cc1: all warnings being treated as errors >>>> >>>> Signed-off-by: Mathieu Malaterre <malat@debian.org> >>>> --- >>>> arch/powerpc/include/asm/uaccess.h | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/arch/powerpc/include/asm/uaccess.h >>>> b/arch/powerpc/include/asm/uaccess.h >>>> index 51bfeb8777f0..fadc406bd39d 100644 >>>> --- a/arch/powerpc/include/asm/uaccess.h >>>> +++ b/arch/powerpc/include/asm/uaccess.h >>>> @@ -49,7 +49,7 @@ >>>> #define __access_ok(addr, size, segment) \ >>>> (((addr) <= (segment).seg) && \ >>>> - (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) >>>> + (((size) == 0) || ((size) < ((segment).seg - (addr))))) >>> >>> >>> IIUC, ((2 - 1) <= 1) is the same as (2 < 1) ????? >> > > The whole series was pretty mediocre, but this one was actually pretty > destructive. Thanks for catching this. > >> >> Note that I already try to submit a fix for this warning 3 years ago >> (https://patchwork.ozlabs.org/patch/418075/) and it was rejected with the >> following comment: Tested again today with gcc 6.3.0 and gcc is still producing the original warning (treated as error). >> Again, I don't think Linux enables this warning. What did you do to >> produce this? In any case, it's a bad warning that doesn't take macros >> into account, and the answer is not to make the code less clear by hiding >> the fact that zero is a special case. > > Right. I'll try to see how to make W=1 run without error with an > alternate solution. So the other alternative is to update a bunch of ppc32 defconfig(s) with: CONFIG_PPC_DISABLE_WERROR=y. Would that be preferable ? >> Christophe >> >> >>> >>> Christophe >>> >>>> #endif >>>> >>
Le 26/02/2018 à 18:50, Mathieu Malaterre a écrit : > On Mon, Feb 26, 2018 at 8:44 AM, Mathieu Malaterre <malat@debian.org> wrote: >> On Mon, Feb 26, 2018 at 7:50 AM, Christophe LEROY >> <christophe.leroy@c-s.fr> wrote: >>> >>> >>> Le 26/02/2018 à 07:34, Christophe LEROY a écrit : >>>> >>>> >>>> >>>> Le 25/02/2018 à 18:22, Mathieu Malaterre a écrit : >>>>> >>>>> Rewrite check size - 1 <= Y as size < Y since `size` is unsigned value. >>>>> Fix warning (treated as error in W=1): >>>>> >>>>> CC arch/powerpc/kernel/signal_32.o >>>>> In file included from ./include/linux/uaccess.h:14:0, >>>>> from ./include/asm-generic/termios-base.h:8, >>>>> from ./arch/powerpc/include/asm/termios.h:20, >>>>> from ./include/uapi/linux/termios.h:6, >>>>> from ./include/linux/tty.h:7, >>>>> from arch/powerpc/kernel/signal_32.c:36: >>>>> ./include/asm-generic/termios-base.h: In function >>>>> ‘user_termio_to_kernel_termios’: >>>>> ./arch/powerpc/include/asm/uaccess.h:52:35: error: comparison of unsigned >>>>> expression >= 0 is always true [-Werror=type-limits] >>>>> (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) >>>>> ^ >>>>> ./arch/powerpc/include/asm/uaccess.h:58:3: note: in expansion of macro >>>>> ‘__access_ok’ >>>>> __access_ok((__force unsigned long)(addr), (size), get_fs())) >>>>> ^~~~~~~~~~~ >>>>> ./arch/powerpc/include/asm/uaccess.h:262:6: note: in expansion of macro >>>>> ‘access_ok’ >>>>> if (access_ok(VERIFY_READ, __gu_addr, (size))) \ >>>>> ^~~~~~~~~ >>>>> ./arch/powerpc/include/asm/uaccess.h:80:2: note: in expansion of macro >>>>> ‘__get_user_check’ >>>>> __get_user_check((x), (ptr), sizeof(*(ptr))) >>>>> ^~~~~~~~~~~~~~~~ >>>>> ./include/asm-generic/termios-base.h:36:6: note: in expansion of macro >>>>> ‘get_user’ >>>>> if (get_user(termios->c_line, &termio->c_line) < 0) >>>>> ^~~~~~~~ >>>>> [...] >>>>> cc1: all warnings being treated as errors >>>>> >>>>> Signed-off-by: Mathieu Malaterre <malat@debian.org> >>>>> --- >>>>> arch/powerpc/include/asm/uaccess.h | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/powerpc/include/asm/uaccess.h >>>>> b/arch/powerpc/include/asm/uaccess.h >>>>> index 51bfeb8777f0..fadc406bd39d 100644 >>>>> --- a/arch/powerpc/include/asm/uaccess.h >>>>> +++ b/arch/powerpc/include/asm/uaccess.h >>>>> @@ -49,7 +49,7 @@ >>>>> #define __access_ok(addr, size, segment) \ >>>>> (((addr) <= (segment).seg) && \ >>>>> - (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) >>>>> + (((size) == 0) || ((size) < ((segment).seg - (addr))))) >>>> >>>> >>>> IIUC, ((2 - 1) <= 1) is the same as (2 < 1) ????? >>> >> >> The whole series was pretty mediocre, but this one was actually pretty >> destructive. Thanks for catching this. >> >>> >>> Note that I already try to submit a fix for this warning 3 years ago >>> (https://patchwork.ozlabs.org/patch/418075/) and it was rejected with the >>> following comment: > > Tested again today with gcc 6.3.0 and gcc is still producing the > original warning (treated as error). That's right, it seems that recent versions of gcc are not happy anymore with that change. Maybe Segher has a suggestion for that one ? > >>> Again, I don't think Linux enables this warning. What did you do to >>> produce this? In any case, it's a bad warning that doesn't take macros >>> into account, and the answer is not to make the code less clear by hiding >>> the fact that zero is a special case. >> >> Right. I'll try to see how to make W=1 run without error with an >> alternate solution. > > So the other alternative is to update a bunch of ppc32 defconfig(s) > with: CONFIG_PPC_DISABLE_WERROR=y. > > Would that be preferable ? No I don't think it is the solution. PPC is built with WERROR in order to catch warnings on real problems. You could disable it selectively when you want to run 'make W=1' and see all possible warnings, then select by yourself which warnings are worth fixing up. Christophe > >>> Christophe >>> >>> >>>> >>>> Christophe >>>> >>>>> #endif >>>>> >>> --- L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast. https://www.avast.com/antivirus
On Mon, Feb 26, 2018 at 09:00:09PM +0100, christophe leroy wrote: > Le 26/02/2018 à 18:50, Mathieu Malaterre a écrit : > >On Mon, Feb 26, 2018 at 8:44 AM, Mathieu Malaterre <malat@debian.org> > >wrote: > >>On Mon, Feb 26, 2018 at 7:50 AM, Christophe LEROY > >><christophe.leroy@c-s.fr> wrote: > >>>Note that I already try to submit a fix for this warning 3 years ago > >>>(https://patchwork.ozlabs.org/patch/418075/) and it was rejected with the > >>>following comment: > > > >Tested again today with gcc 6.3.0 and gcc is still producing the > >original warning (treated as error). > > That's right, it seems that recent versions of gcc are not happy anymore > with that change. > > Maybe Segher has a suggestion for that one ? Your patch: #define __access_ok(addr, size, segment) \ (((addr) <= (segment).seg) && \ - (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) + (((size) <= 1) || (((size) - 1) <= ((segment).seg - (addr))))) Is there any reason to write this as a macro? Let's make this more readable: static inline int __access_ok(unsigned long addr, unsigned long size, mm_segment_t seg) { if (addr > seg.seg) return 0; return (size == 0 || size - 1 <= seg.seg - addr); } and I think we are done already, or will this warn for any input? Segher
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h index 51bfeb8777f0..fadc406bd39d 100644 --- a/arch/powerpc/include/asm/uaccess.h +++ b/arch/powerpc/include/asm/uaccess.h @@ -49,7 +49,7 @@ #define __access_ok(addr, size, segment) \ (((addr) <= (segment).seg) && \ - (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) + (((size) == 0) || ((size) < ((segment).seg - (addr))))) #endif
Rewrite check size - 1 <= Y as size < Y since `size` is unsigned value. Fix warning (treated as error in W=1): CC arch/powerpc/kernel/signal_32.o In file included from ./include/linux/uaccess.h:14:0, from ./include/asm-generic/termios-base.h:8, from ./arch/powerpc/include/asm/termios.h:20, from ./include/uapi/linux/termios.h:6, from ./include/linux/tty.h:7, from arch/powerpc/kernel/signal_32.c:36: ./include/asm-generic/termios-base.h: In function ‘user_termio_to_kernel_termios’: ./arch/powerpc/include/asm/uaccess.h:52:35: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits] (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr))))) ^ ./arch/powerpc/include/asm/uaccess.h:58:3: note: in expansion of macro ‘__access_ok’ __access_ok((__force unsigned long)(addr), (size), get_fs())) ^~~~~~~~~~~ ./arch/powerpc/include/asm/uaccess.h:262:6: note: in expansion of macro ‘access_ok’ if (access_ok(VERIFY_READ, __gu_addr, (size))) \ ^~~~~~~~~ ./arch/powerpc/include/asm/uaccess.h:80:2: note: in expansion of macro ‘__get_user_check’ __get_user_check((x), (ptr), sizeof(*(ptr))) ^~~~~~~~~~~~~~~~ ./include/asm-generic/termios-base.h:36:6: note: in expansion of macro ‘get_user’ if (get_user(termios->c_line, &termio->c_line) < 0) ^~~~~~~~ [...] cc1: all warnings being treated as errors Signed-off-by: Mathieu Malaterre <malat@debian.org> --- arch/powerpc/include/asm/uaccess.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)