| Message ID | 20170421213931.155210-3-mka@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Fri, Apr 21, 2017 at 2:39 PM, Matthias Kaehlcke <mka@chromium.org> wrote: > clang generates plenty of these warnings in different parts of the code. > They are mostly caused by container_of() and other macros which declare > a "const <type> *" variable for their internal use which triggers a > "duplicate 'const' specifier" warning if the <type> is already const > qualified. > > Wording-mostly-from: Michael Davidson <md@google.com> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Makefile b/Makefile > index df5abf346354..6cd6d428db43 100644 > --- a/Makefile > +++ b/Makefile > @@ -704,6 +704,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable) > KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier) > KBUILD_CFLAGS += $(call cc-disable-warning, gnu) > KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) > +KBUILD_CFLAGS += $(call cc-disable-warning, duplicate-decl-specifier) > # Quiet clang warning: comparison of unsigned expression < 0 is always false > KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) > # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the It seems like gcc 7 may have the same warning. Specifically I saw a patch fly by from Arnd, which you can find in Mark Brown's tree now: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=topic/rt5614&id=03ba791df98d15d07ea74075122af71e35c7611c +Arnd since he may be trying to solve the same issues? -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote: > Hi > > On Fri, Apr 21, 2017 at 2:39 PM, Matthias Kaehlcke <mka@chromium.org> wrote: >> clang generates plenty of these warnings in different parts of the code. >> They are mostly caused by container_of() and other macros which declare >> a "const <type> *" variable for their internal use which triggers a >> "duplicate 'const' specifier" warning if the <type> is already const >> qualified. >> >> Wording-mostly-from: Michael Davidson <md@google.com> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> >> --- >> Makefile | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/Makefile b/Makefile >> index df5abf346354..6cd6d428db43 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -704,6 +704,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable) >> KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier) >> KBUILD_CFLAGS += $(call cc-disable-warning, gnu) >> KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) >> +KBUILD_CFLAGS += $(call cc-disable-warning, duplicate-decl-specifier) >> # Quiet clang warning: comparison of unsigned expression < 0 is always false >> KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) >> # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the > > It seems like gcc 7 may have the same warning. Specifically I saw a > patch fly by from Arnd, which you can find in Mark Brown's tree now: > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=topic/rt5614&id=03ba791df98d15d07ea74075122af71e35c7611c > > > +Arnd since he may be trying to solve the same issues? gcc-7 has a warning option with the same name, and I think I have fixed all the occurrences we got in mainline (some patches my still be in flight). However, it seems that only clang warns about 'const typeof(type)' with 'type' being already const. I have not looked at clang warnings in a while, how many of these do we get overall (aside from container_of)? We might be able to turn off this particular warning by sprinkling in '_Pragma("clang diagnostic push") _Pragma("clang diagnostic ignored \"-Wduplicate-decl-specifier\"")' inside of the macro (not sure if clang allows it there, gcc-4.4 and earlier I think did not). It might also be useful to open a bug against clang so they can change it in future releases, as the gcc behavior seems more sensible in this instance. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit: > On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote: > > Hi > > > > On Fri, Apr 21, 2017 at 2:39 PM, Matthias Kaehlcke <mka@chromium.org> wrote: > >> clang generates plenty of these warnings in different parts of the code. > >> They are mostly caused by container_of() and other macros which declare > >> a "const <type> *" variable for their internal use which triggers a > >> "duplicate 'const' specifier" warning if the <type> is already const > >> qualified. > >> > >> Wording-mostly-from: Michael Davidson <md@google.com> > >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > >> --- > >> Makefile | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/Makefile b/Makefile > >> index df5abf346354..6cd6d428db43 100644 > >> --- a/Makefile > >> +++ b/Makefile > >> @@ -704,6 +704,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable) > >> KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier) > >> KBUILD_CFLAGS += $(call cc-disable-warning, gnu) > >> KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) > >> +KBUILD_CFLAGS += $(call cc-disable-warning, duplicate-decl-specifier) > >> # Quiet clang warning: comparison of unsigned expression < 0 is always false > >> KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) > >> # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the > > > > It seems like gcc 7 may have the same warning. Specifically I saw a > > patch fly by from Arnd, which you can find in Mark Brown's tree now: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=topic/rt5614&id=03ba791df98d15d07ea74075122af71e35c7611c > > > > > > +Arnd since he may be trying to solve the same issues? > > gcc-7 has a warning option with the same name, and I think I have > fixed all the occurrences we got in mainline (some patches my still > be in flight). However, it seems that only clang warns about > 'const typeof(type)' with 'type' being already const. > > I have not looked at clang warnings in a while, how many of these do > we get overall (aside from container_of)? We might be able > to turn off this particular warning by sprinkling in > '_Pragma("clang diagnostic push") _Pragma("clang diagnostic > ignored \"-Wduplicate-decl-specifier\"")' inside of the macro > (not sure if clang allows it there, gcc-4.4 and earlier I think did > not). > > It might also be useful to open a bug against clang so they can > change it in future releases, as the gcc behavior seems more > sensible in this instance. I asked our toolchain folks to follow up with the clang devs. They asked me for a simple test case, to my suprise clang didn't raise a warning when building this: static const int x; static const typeof(x) y; It turns out that the warning is only raised when -std=gnu89 (and potentially others) is set, which is the case of the kernel. Definitely looks like this should be fixed in clang. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
El Wed, May 17, 2017 at 11:45:29AM -0700 Matthias Kaehlcke ha dit: > El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit: > > > On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote: > > > Hi > > > > > > On Fri, Apr 21, 2017 at 2:39 PM, Matthias Kaehlcke <mka@chromium.org> wrote: > > >> clang generates plenty of these warnings in different parts of the code. > > >> They are mostly caused by container_of() and other macros which declare > > >> a "const <type> *" variable for their internal use which triggers a > > >> "duplicate 'const' specifier" warning if the <type> is already const > > >> qualified. > > >> > > >> Wording-mostly-from: Michael Davidson <md@google.com> > > >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > >> --- > > >> Makefile | 1 + > > >> 1 file changed, 1 insertion(+) > > >> > > >> diff --git a/Makefile b/Makefile > > >> index df5abf346354..6cd6d428db43 100644 > > >> --- a/Makefile > > >> +++ b/Makefile > > >> @@ -704,6 +704,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable) > > >> KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier) > > >> KBUILD_CFLAGS += $(call cc-disable-warning, gnu) > > >> KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) > > >> +KBUILD_CFLAGS += $(call cc-disable-warning, duplicate-decl-specifier) > > >> # Quiet clang warning: comparison of unsigned expression < 0 is always false > > >> KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) > > >> # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the > > > > > > It seems like gcc 7 may have the same warning. Specifically I saw a > > > patch fly by from Arnd, which you can find in Mark Brown's tree now: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=topic/rt5614&id=03ba791df98d15d07ea74075122af71e35c7611c > > > > > > > > > +Arnd since he may be trying to solve the same issues? > > > > gcc-7 has a warning option with the same name, and I think I have > > fixed all the occurrences we got in mainline (some patches my still > > be in flight). However, it seems that only clang warns about > > 'const typeof(type)' with 'type' being already const. > > > > I have not looked at clang warnings in a while, how many of these do > > we get overall (aside from container_of)? We might be able > > to turn off this particular warning by sprinkling in > > '_Pragma("clang diagnostic push") _Pragma("clang diagnostic > > ignored \"-Wduplicate-decl-specifier\"")' inside of the macro > > (not sure if clang allows it there, gcc-4.4 and earlier I think did > > not). > > > > It might also be useful to open a bug against clang so they can > > change it in future releases, as the gcc behavior seems more > > sensible in this instance. > > I asked our toolchain folks to follow up with the clang devs. They > asked me for a simple test case, to my suprise clang didn't raise a > warning when building this: > > static const int x; > static const typeof(x) y; > > It turns out that the warning is only raised when -std=gnu89 (and > potentially others) is set, which is the case of the > kernel. Definitely looks like this should be fixed in clang. It seems the duplicate-decl-specifier warning targets specifically C89: "The same type qualifier shall not appear more than once in the same specifier list or qualifier list, either directly or via one or more typedefs." C89 (6.5.3) gcc also raises a warning when '-pedantic' is specified and -std=gnu89/c89 (or unspecified), but not with -std=gnu99/c99. This bug might help to shed more light on this: https://bugs.llvm.org/show_bug.cgi?id=32985 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 24, 2017 at 2:04 AM, Matthias Kaehlcke <mka@chromium.org> wrote: > El Wed, May 17, 2017 at 11:45:29AM -0700 Matthias Kaehlcke ha dit: >> El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit: >> > On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote: > It seems the duplicate-decl-specifier warning targets specifically C89: > > "The same type qualifier shall not appear more than once in the same > specifier list or qualifier list, either directly or via one or more > typedefs." > > C89 (6.5.3) > > gcc also raises a warning when '-pedantic' is specified and > -std=gnu89/c89 (or unspecified), but not with -std=gnu99/c99. > > This bug might help to shed more light on this: > https://bugs.llvm.org/show_bug.cgi?id=32985 I also notice that neither compiler differentiates between a) typedef const int cint; const cint i; and b) const int i; const typeof(a) j; I would have expected a warning for a) but not b), but both 'clang --std=gnu89' and 'gcc --pedantic --std=gnu89' warn about both of b as well, and don't warn for newer standards. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi. 2017-05-24 17:21 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > On Wed, May 24, 2017 at 2:04 AM, Matthias Kaehlcke <mka@chromium.org> wrote: >> El Wed, May 17, 2017 at 11:45:29AM -0700 Matthias Kaehlcke ha dit: >>> El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit: >>> > On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote: >> It seems the duplicate-decl-specifier warning targets specifically C89: >> >> "The same type qualifier shall not appear more than once in the same >> specifier list or qualifier list, either directly or via one or more >> typedefs." >> >> C89 (6.5.3) >> >> gcc also raises a warning when '-pedantic' is specified and >> -std=gnu89/c89 (or unspecified), but not with -std=gnu99/c99. >> >> This bug might help to shed more light on this: >> https://bugs.llvm.org/show_bug.cgi?id=32985 > > I also notice that neither compiler differentiates between a) > > typedef const int cint; > const cint i; > > and b) > const int i; > const typeof(a) j; > > I would have expected a warning for a) but not b), but both 'clang --std=gnu89' > and 'gcc --pedantic --std=gnu89' warn about both of b as well, and don't warn > for newer standards. > > Arnd I think we agreed to apply 1/2. How about 2/2? I think we mostly discussed preferable behavior of -Wduplicate-decl-specifier, but we did not come up with an idea to solve the problem for already shipped clang versions. (BTW, we have not defined the minimal supported version of clang yet.)
On Wed, Jun 21, 2017 at 11:11 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2017-05-24 17:21 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: >> On Wed, May 24, 2017 at 2:04 AM, Matthias Kaehlcke <mka@chromium.org> wrote: >>> El Wed, May 17, 2017 at 11:45:29AM -0700 Matthias Kaehlcke ha dit: >>>> El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit: >>>> > On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote: >>> It seems the duplicate-decl-specifier warning targets specifically C89: >>> >>> "The same type qualifier shall not appear more than once in the same >>> specifier list or qualifier list, either directly or via one or more >>> typedefs." >>> >>> C89 (6.5.3) >>> >>> gcc also raises a warning when '-pedantic' is specified and >>> -std=gnu89/c89 (or unspecified), but not with -std=gnu99/c99. >>> >>> This bug might help to shed more light on this: >>> https://bugs.llvm.org/show_bug.cgi?id=32985 >> >> I also notice that neither compiler differentiates between a) >> >> typedef const int cint; >> const cint i; >> >> and b) >> const int i; >> const typeof(a) j; >> >> I would have expected a warning for a) but not b), but both 'clang --std=gnu89' >> and 'gcc --pedantic --std=gnu89' warn about both of b as well, and don't warn >> for newer standards. >> >> Arnd > > > > > I think we agreed to apply 1/2. > > How about 2/2? > > I think we mostly discussed preferable behavior of -Wduplicate-decl-specifier, > but we did not come up with an idea to solve the problem for > already shipped clang versions. > (BTW, we have not defined the minimal supported version of clang yet.) I see that container_of() has been modified in linux-next and no longer adds the 'const' keyword, do we actually still need the patch? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
El Wed, Jun 21, 2017 at 12:11:55PM +0200 Arnd Bergmann ha dit: > On Wed, Jun 21, 2017 at 11:11 AM, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: > > 2017-05-24 17:21 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > >> On Wed, May 24, 2017 at 2:04 AM, Matthias Kaehlcke <mka@chromium.org> wrote: > >>> El Wed, May 17, 2017 at 11:45:29AM -0700 Matthias Kaehlcke ha dit: > >>>> El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit: > >>>> > On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote: > >>> It seems the duplicate-decl-specifier warning targets specifically C89: > >>> > >>> "The same type qualifier shall not appear more than once in the same > >>> specifier list or qualifier list, either directly or via one or more > >>> typedefs." > >>> > >>> C89 (6.5.3) > >>> > >>> gcc also raises a warning when '-pedantic' is specified and > >>> -std=gnu89/c89 (or unspecified), but not with -std=gnu99/c99. > >>> > >>> This bug might help to shed more light on this: > >>> https://bugs.llvm.org/show_bug.cgi?id=32985 > >> > >> I also notice that neither compiler differentiates between a) > >> > >> typedef const int cint; > >> const cint i; > >> > >> and b) > >> const int i; > >> const typeof(a) j; > >> > >> I would have expected a warning for a) but not b), but both 'clang --std=gnu89' > >> and 'gcc --pedantic --std=gnu89' warn about both of b as well, and don't warn > >> for newer standards. > >> > >> Arnd > > > > > > > > > > I think we agreed to apply 1/2. > > > > How about 2/2? > > > > I think we mostly discussed preferable behavior of -Wduplicate-decl-specifier, > > but we did not come up with an idea to solve the problem for > > already shipped clang versions. > > (BTW, we have not defined the minimal supported version of clang yet.) I think it will have to be a future version. There is still an issue affecting at least llist_for_each_entry_safe(), where clang optimizes away a check for check for a NULL pointer. For gcc this optimization is switched off with -fno-delete-null-pointer-check, clang currently does not have this flag or an equivalent. For now a workaround like this is needed for newer kernels: https://android-git.linaro.org/kernel/hikey-clang.git/commit/?h=android-hikey-linaro-4.9-clang&id=4f3c3c1e7b153e333603be74d786d79bb872e8ff For arm64 at least one other clang fix is missing, to make -mgeneral-regs-only consistent with gcc (https://bugs.llvm.org/show_bug.cgi?id=30792) > I see that container_of() has been modified in linux-next and no longer adds > the 'const' keyword, do we actually still need the patch? There is still (at least) the case of const arrays passed to MODULE_DEVICE_TABLE. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 21, 2017 at 6:58 PM, Matthias Kaehlcke <mka@chromium.org> wrote: > El Wed, Jun 21, 2017 at 12:11:55PM +0200 Arnd Bergmann ha dit: >> I see that container_of() has been modified in linux-next and no longer adds >> the 'const' keyword, do we actually still need the patch? > > There is still (at least) the case of const arrays passed to > MODULE_DEVICE_TABLE. Does the 'const' have any effect there? As it's just an alias, it should at least not impact the placement of the symbol in the object file, right? Maybe we can just remove that 'const' too. Do you see any other instances? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
El Wed, Jun 21, 2017 at 07:59:42PM +0200 Arnd Bergmann ha dit: > On Wed, Jun 21, 2017 at 6:58 PM, Matthias Kaehlcke <mka@chromium.org> wrote: > > El Wed, Jun 21, 2017 at 12:11:55PM +0200 Arnd Bergmann ha dit: > >> I see that container_of() has been modified in linux-next and no longer adds > >> the 'const' keyword, do we actually still need the patch? > > > > There is still (at least) the case of const arrays passed to > > MODULE_DEVICE_TABLE. > > Does the 'const' have any effect there? As it's just an alias, it > should at least > not impact the placement of the symbol in the object file, right? I agree, it shouldn't make a difference. > Maybe we can just remove that 'const' too. Seems worth a try. Do you want to send a patch for the removal? > Do you see any other instances? For both x86 and arm64 defconfig the instances are all from container_of() or MODULE_DEVICE_TABLE. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
El Wed, Jun 21, 2017 at 07:59:42PM +0200 Arnd Bergmann ha dit: > On Wed, Jun 21, 2017 at 6:58 PM, Matthias Kaehlcke <mka@chromium.org> wrote: > > El Wed, Jun 21, 2017 at 12:11:55PM +0200 Arnd Bergmann ha dit: > >> I see that container_of() has been modified in linux-next and no longer adds > >> the 'const' keyword, do we actually still need the patch? > > > > There is still (at least) the case of const arrays passed to > > MODULE_DEVICE_TABLE. > > Does the 'const' have any effect there? As it's just an alias, it > should at least > not impact the placement of the symbol in the object file, right? Maybe we can > just remove that 'const' too. Do you see any other instances? For the record: https://git.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git/commit/?h=modules-next&id=0bf8bf50eddc7511b52461bae798cbfaa0157a34 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Makefile b/Makefile index df5abf346354..6cd6d428db43 100644 --- a/Makefile +++ b/Makefile @@ -704,6 +704,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable) KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier) KBUILD_CFLAGS += $(call cc-disable-warning, gnu) KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) +KBUILD_CFLAGS += $(call cc-disable-warning, duplicate-decl-specifier) # Quiet clang warning: comparison of unsigned expression < 0 is always false KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
clang generates plenty of these warnings in different parts of the code. They are mostly caused by container_of() and other macros which declare a "const <type> *" variable for their internal use which triggers a "duplicate 'const' specifier" warning if the <type> is already const qualified. Wording-mostly-from: Michael Davidson <md@google.com> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- Makefile | 1 + 1 file changed, 1 insertion(+)