| Message ID | CAK8P3a3qrkZLkL=PuUHkaq9p021-Y+odTj5UrdM=dZw8L=oM8g@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18 July 2017 at 20:53, Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Jul 14, 2017 at 2:28 PM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote: >>> gcc warns when MODULES_VADDR/END is defined to the same value as >>> VMALLOC_START/VMALLOC_END, e.g. on x86-32: >>> >>> fs/proc/kcore.c: In function ‘add_modules_range’: >>> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare] >>> if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) { >>> >> >> Does it occur for subtraction as well? Or only for comparison? > > This replacement patch would also address the warning: > > diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c > index 45629f4b5402..35824e986c2c 100644 > --- a/fs/proc/kcore.c > +++ b/fs/proc/kcore.c > @@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void) > struct kcore_list kcore_modules; > static void __init add_modules_range(void) > { > - if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) { > + if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) { > kclist_add(&kcore_modules, (void *)MODULES_VADDR, > MODULES_END - MODULES_VADDR, KCORE_VMALLOC); > } > > I have also verified that four of the 14 patches are not needed when building > without ccache, this is one of them: > > acpi: thermal: fix gcc-6/ccache warning > proc/kcore: hide a harmless warning > SFI: fix tautological-compare warning > [media] fix warning on v4l2_subdev_call() result interpreted as bool > > Not sure what to do with those, we could either ignore them all and > not care about ccache, or we try to address them all in some way. > Any idea why ccache makes a difference here? It is not obvious (not to me at least)
On Tue, Jul 18, 2017 at 9:55 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 18 July 2017 at 20:53, Arnd Bergmann <arnd@arndb.de> wrote: >> On Fri, Jul 14, 2017 at 2:28 PM, Ard Biesheuvel >> <ard.biesheuvel@linaro.org> wrote: >>> On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote: >>>> gcc warns when MODULES_VADDR/END is defined to the same value as >>>> VMALLOC_START/VMALLOC_END, e.g. on x86-32: >>>> >>>> fs/proc/kcore.c: In function ‘add_modules_range’: >>>> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare] >>>> if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) { >>>> >>> >>> Does it occur for subtraction as well? Or only for comparison? >> >> This replacement patch would also address the warning: >> >> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c >> index 45629f4b5402..35824e986c2c 100644 >> --- a/fs/proc/kcore.c >> +++ b/fs/proc/kcore.c >> @@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void) >> struct kcore_list kcore_modules; >> static void __init add_modules_range(void) >> { >> - if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) { >> + if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) { >> kclist_add(&kcore_modules, (void *)MODULES_VADDR, >> MODULES_END - MODULES_VADDR, KCORE_VMALLOC); >> } >> >> I have also verified that four of the 14 patches are not needed when building >> without ccache, this is one of them: >> >> acpi: thermal: fix gcc-6/ccache warning >> proc/kcore: hide a harmless warning >> SFI: fix tautological-compare warning >> [media] fix warning on v4l2_subdev_call() result interpreted as bool >> >> Not sure what to do with those, we could either ignore them all and >> not care about ccache, or we try to address them all in some way. >> > > Any idea why ccache makes a difference here? It is not obvious (not to > me at least) When ccache is used, the compilation stage is apparently always done on the preprocessed source file. So instead of parsing (with the integrated preprocessor) if (MODULES_VADDR != VMALLOC_START ...) the compiler sees if (((unsigned long)high_memory + (8 * 1024 * 1024)) != ((unsigned long)high_memory + (8 * 1024 * 1024)) ...) and it correctly considers the first expression something that one would write in source code, while -Wtautological-compare is intended to warn about the second version being always true, which makes the 'if()' pointless. Arnd
On 18 July 2017 at 21:01, Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, Jul 18, 2017 at 9:55 PM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> On 18 July 2017 at 20:53, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Fri, Jul 14, 2017 at 2:28 PM, Ard Biesheuvel >>> <ard.biesheuvel@linaro.org> wrote: >>>> On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote: >>>>> gcc warns when MODULES_VADDR/END is defined to the same value as >>>>> VMALLOC_START/VMALLOC_END, e.g. on x86-32: >>>>> >>>>> fs/proc/kcore.c: In function ‘add_modules_range’: >>>>> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare] >>>>> if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) { >>>>> >>>> >>>> Does it occur for subtraction as well? Or only for comparison? >>> >>> This replacement patch would also address the warning: >>> >>> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c >>> index 45629f4b5402..35824e986c2c 100644 >>> --- a/fs/proc/kcore.c >>> +++ b/fs/proc/kcore.c >>> @@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void) >>> struct kcore_list kcore_modules; >>> static void __init add_modules_range(void) >>> { >>> - if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) { >>> + if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) { >>> kclist_add(&kcore_modules, (void *)MODULES_VADDR, >>> MODULES_END - MODULES_VADDR, KCORE_VMALLOC); >>> } >>> >>> I have also verified that four of the 14 patches are not needed when building >>> without ccache, this is one of them: >>> >>> acpi: thermal: fix gcc-6/ccache warning >>> proc/kcore: hide a harmless warning >>> SFI: fix tautological-compare warning >>> [media] fix warning on v4l2_subdev_call() result interpreted as bool >>> >>> Not sure what to do with those, we could either ignore them all and >>> not care about ccache, or we try to address them all in some way. >>> >> >> Any idea why ccache makes a difference here? It is not obvious (not to >> me at least) > > When ccache is used, the compilation stage is apparently always done on > the preprocessed source file. So instead of parsing (with the integrated > preprocessor) > > if (MODULES_VADDR != VMALLOC_START ...) > > the compiler sees > > if (((unsigned long)high_memory + (8 * 1024 * 1024)) != > ((unsigned long)high_memory + (8 * 1024 * 1024)) ...) > > and it correctly considers the first expression something that one > would write in source code, while -Wtautological-compare > is intended to warn about the second version being always true, > which makes the 'if()' pointless. > Ah, now it makes sense. I was a bit surprised that -Wtautological-compare complains about symbolic constants that resolve to the same expression, but apparently it doesn't. I see how ccache needs to preprocess first: that is how it notices changes, by hashing the preprocessed input and comparing it to the stored hash. I'd still expect it to go back to letting the compiler preprocess for the actual build, but apparently it doesn't. A quick google search didn't produce anything useful, but I'd expect other ccache users to run into the same issue.
On Tue, Jul 18, 2017 at 10:07 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 18 July 2017 at 21:01, Arnd Bergmann <arnd@arndb.de> wrote: >> On Tue, Jul 18, 2017 at 9:55 PM, Ard Biesheuvel > > Ah, now it makes sense. I was a bit surprised that > -Wtautological-compare complains about symbolic constants that resolve > to the same expression, but apparently it doesn't. > > I see how ccache needs to preprocess first: that is how it notices > changes, by hashing the preprocessed input and comparing it to the > stored hash. I'd still expect it to go back to letting the compiler > preprocess for the actual build, but apparently it doesn't. When I tried to figure this out, I saw that ccache has two modes, "direct" and "preprocessed". It usually tries to use direct mode unless something prevents that. In "direct" mode, it hashes the source file and the included headers instead of the preprocessed source file, however it still calls the compiler for the preprocessed source file, I guess since it has to preprocess the file the first time it is seen so it can record which headers are included. > A quick google search didn't produce anything useful, but I'd expect > other ccache users to run into the same issue. I suspect gcc-7 is still too new for most people to have noticed this. The kernel is a very large codebase, and we only got a handful of -Wtautological-compare warnings at all, most of them happen wtihout ccache, too. Among the four patches, three are for -Wtautological-compare, and one is for -Wint-in-bool-context: if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced)) v4l2_subdev_call() in this case is a function-like macro that may return -ENODEV if its first argument is NULL. The other -Wint-in-bool-context I found all happen with or without ccache, most commonly there is an constant integer expression passed into a macro and then checked like #define macro(arg) \ do { \ if (arg) \ do_something(arg); \ } while (0) Arnd
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c index 45629f4b5402..35824e986c2c 100644 --- a/fs/proc/kcore.c +++ b/fs/proc/kcore.c @@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void) struct kcore_list kcore_modules; static void __init add_modules_range(void) { - if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) { + if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) { kclist_add(&kcore_modules, (void *)MODULES_VADDR, MODULES_END - MODULES_VADDR, KCORE_VMALLOC); }