Re: [greybus-dev] [PATCH] Kbuild: remove -std=gnu89 from compiler arguments
On 2/27/22 5:11 PM, Linus Torvalds wrote: On Sun, Feb 27, 2022 at 3:04 PM Alex Elder wrote: Glancing at the Greybus code, I don't believe there's any reason it needs to shift a negative value. Such warnings could be fixed by making certain variables unsigned, for example. As mentioned in the original thread, making things unsigned actually is likely to introduce bugs and make things worse. Understood. What I meant is that the shifts were producing single-bit masks from plain int values that range from 0 to 10 or something (in a for loop). Looking again though, that it's not so simple. Regardless, your point about the warning is good and I won't plan to "fix" this. Thanks. -Alex The warning is simply bogus, and the fact that it was enabled by -Wextra in gcc for std=gnu99 and up was a mistake that looks likely to be fixed in gcc. So don't try to "fix" the code to make any possible warnings go away. You may just make things worse. (That is often true in general for the more esoteric warnings, but in this case it's just painfully more obvious). Linus
Re: [greybus-dev] [PATCH] Kbuild: remove -std=gnu89 from compiler arguments
On 2/27/22 3:52 PM, Arnd Bergmann wrote: From: Arnd Bergmann During a patch discussion, Linus brought up the option of changing the C standard version from gnu89 to gnu99, which allows using variable declaration inside of a for() loop. While the C99, C11 and later standards introduce many other features, most of these are already available in gnu89 as GNU extensions as well. An earlier attempt to do this when gcc-5 started defaulting to -std=gnu11 failed because at the time that caused warnings about designated initializers with older compilers. Now that gcc-5.1 is the minimum compiler version used for building kernels, that is no longer a concern. Similarly, the behavior of 'inline' functions changes between gnu89 and gnu89, but this was taken care of by defining 'inline' to include __attribute__((gnu_inline)) in order to allow building with clang a while ago. One minor issue that remains is an added gcc warning for shifts of negative integers when building with -Werror, which happens with the 'make W=1' option, as well as for three drivers in the kernel that always enable -Werror, but it was only observed with the i915 driver so far. Nathan Chancellor reported an additional -Wdeclaration-after-statement warning that appears in a system header on arm, this still needs a workaround. Since the differences between gnu99, gnu11 and gnu17 are fairly minimal and mainly impact warnings at the -Wpedantic level that the kernel never enables, the easiest way is to just leave out the -std=gnu89 argument entirely, and rely on the compiler default language setting, which is gnu11 for gcc-5, and gnu1x/gnu17 for all other supported versions of gcc or clang. Link: https://lore.kernel.org/lkml/CAHk-=wiych7xehcmifj-ygxuy2jaj7pnkdkpcovt8fybvfw...@mail.gmail.com/ Link: https://github.com/ClangBuiltLinux/linux/issues/1603 Suggested-by: Linus Torvalds Cc: Masahiro Yamada Cc: linux-kbu...@vger.kernel.org Cc: l...@lists.linux.dev Signed-off-by: Arnd Bergmann --- I put the suggestion into patch form, based on what we discussed in the thread. I only gave it minimal testing, but it would be good to have it in linux-next if we want to do this in the merge window. Did you determine what needed the new compiler flag based on compilation results? Glancing at the Greybus code, I don't believe there's any reason it needs to shift a negative value. Such warnings could be fixed by making certain variables unsigned, for example. I have no objection, I'll just make a note of it. -Alex --- Documentation/process/programming-language.rst | 4 ++-- .../translations/it_IT/process/programming-language.rst| 4 ++-- .../translations/zh_CN/process/programming-language.rst| 4 ++-- .../translations/zh_TW/process/programming-language.rst| 4 ++-- Makefile | 7 +++ arch/arm64/kernel/vdso32/Makefile | 3 +-- drivers/gpu/drm/i915/Makefile | 1 + drivers/staging/greybus/tools/Makefile | 3 ++- fs/btrfs/Makefile | 1 + scripts/Makefile.extrawarn | 1 + 10 files changed, 17 insertions(+), 15 deletions(-) . . . diff --git a/drivers/staging/greybus/tools/Makefile b/drivers/staging/greybus/tools/Makefile index ad0ae8053b79..a3bbd73171f2 100644 --- a/drivers/staging/greybus/tools/Makefile +++ b/drivers/staging/greybus/tools/Makefile @@ -12,7 +12,8 @@ CFLAGS+= -std=gnu99 -Wall -Wextra -g \ -Wredundant-decls \ -Wcast-align \ -Wsign-compare \ - -Wno-missing-field-initializers + -Wno-missing-field-initializers \ + -Wno-shift-negative-value CC := $(CROSS_COMPILE)gcc . . .
Re: [PATCH] Kbuild: remove -std=gnu89 from compiler arguments
On Sun, Feb 27, 2022 at 10:52:43PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > During a patch discussion, Linus brought up the option of changing > the C standard version from gnu89 to gnu99, which allows using variable > declaration inside of a for() loop. While the C99, C11 and later standards > introduce many other features, most of these are already available in > gnu89 as GNU extensions as well. > > An earlier attempt to do this when gcc-5 started defaulting to > -std=gnu11 failed because at the time that caused warnings about > designated initializers with older compilers. Now that gcc-5.1 is the > minimum compiler version used for building kernels, that is no longer a > concern. Similarly, the behavior of 'inline' functions changes between > gnu89 and gnu89, but this was taken care of by defining 'inline' to Typo here? Second one should be gnu99 right? > include __attribute__((gnu_inline)) in order to allow building with > clang a while ago. > > One minor issue that remains is an added gcc warning for shifts of > negative integers when building with -Werror, which happens with the > 'make W=1' option, as well as for three drivers in the kernel that always > enable -Werror, but it was only observed with the i915 driver so far. > > Nathan Chancellor reported an additional -Wdeclaration-after-statement > warning that appears in a system header on arm, this still needs a > workaround. > > Since the differences between gnu99, gnu11 and gnu17 are fairly minimal > and mainly impact warnings at the -Wpedantic level that the kernel > never enables, the easiest way is to just leave out the -std=gnu89 > argument entirely, and rely on the compiler default language setting, > which is gnu11 for gcc-5, and gnu1x/gnu17 for all other supported > versions of gcc or clang. > > Link: > https://lore.kernel.org/lkml/CAHk-=wiych7xehcmifj-ygxuy2jaj7pnkdkpcovt8fybvfw...@mail.gmail.com/ > Link: https://github.com/ClangBuiltLinux/linux/issues/1603 > Suggested-by: Linus Torvalds > Cc: Masahiro Yamada > Cc: linux-kbu...@vger.kernel.org > Cc: l...@lists.linux.dev > Signed-off-by: Arnd Bergmann > --- > I put the suggestion into patch form, based on what we discussed > in the thread. I only gave it minimal testing, but it would > be good to have it in linux-next if we want to do this in the > merge window. > --- > Documentation/process/programming-language.rst | 4 ++-- > .../translations/it_IT/process/programming-language.rst| 4 ++-- > .../translations/zh_CN/process/programming-language.rst| 4 ++-- > .../translations/zh_TW/process/programming-language.rst| 4 ++-- > Makefile | 7 +++ > arch/arm64/kernel/vdso32/Makefile | 3 +-- > drivers/gpu/drm/i915/Makefile | 1 + > drivers/staging/greybus/tools/Makefile | 3 ++- > fs/btrfs/Makefile | 1 + > scripts/Makefile.extrawarn | 1 + > 10 files changed, 17 insertions(+), 15 deletions(-) > > diff --git a/Documentation/process/programming-language.rst > b/Documentation/process/programming-language.rst > index ec474a70a02f..894f2a6eb9db 100644 > --- a/Documentation/process/programming-language.rst > +++ b/Documentation/process/programming-language.rst > @@ -5,8 +5,8 @@ Programming Language > > The kernel is written in the C programming language [c-language]_. > More precisely, the kernel is typically compiled with ``gcc`` [gcc]_ > -under ``-std=gnu89`` [gcc-c-dialect-options]_: the GNU dialect of ISO C90 > -(including some C99 features). ``clang`` [clang]_ is also supported, see > +under ``-std=gnu11`` [gcc-c-dialect-options]_: the GNU dialect of ISO C11 > +(including some C17 features). ``clang`` [clang]_ is also supported, see > docs on :ref:`Building Linux with Clang/LLVM `. > > This dialect contains many extensions to the language [gnu-extensions]_, > diff --git > a/Documentation/translations/it_IT/process/programming-language.rst > b/Documentation/translations/it_IT/process/programming-language.rst > index 41db2598ce11..aa21097737ae 100644 > --- a/Documentation/translations/it_IT/process/programming-language.rst > +++ b/Documentation/translations/it_IT/process/programming-language.rst > @@ -10,8 +10,8 @@ Linguaggio di programmazione > > Il kernel è scritto nel linguaggio di programmazione C [it-c-language]_. > Più precisamente, il kernel viene compilato con ``gcc`` [it-gcc]_ usando > -l'opzione ``-std=gnu89`` [it-gcc-c-dialect-options]_: il dialetto GNU > -dello standard ISO C90 (con l'aggiunta di alcune funzionalità da C99). > +l'opzione ``-std=gnu11`` [it-gcc-c-dialect-options]_: il dialetto GNU > +dello standard ISO C11 (con l'aggiunta di alcune funzionalità da C17). > Linux supporta anche ``clang`` [it-clang]_, leggete la documentazione > :ref:`Building Linux with Clang/LLVM `. > > diff --git >
Re: [PATCH] Kbuild: remove -std=gnu89 from compiler arguments
On Mon, Feb 28, 2022 at 1:14 AM John Stoffel wrote: > > On Sun, Feb 27, 2022 at 10:52:43PM +0100, Arnd Bergmann wrote: > > From: Arnd Bergmann > > > > During a patch discussion, Linus brought up the option of changing > > the C standard version from gnu89 to gnu99, which allows using variable > > declaration inside of a for() loop. While the C99, C11 and later standards > > introduce many other features, most of these are already available in > > gnu89 as GNU extensions as well. > > > > An earlier attempt to do this when gcc-5 started defaulting to > > -std=gnu11 failed because at the time that caused warnings about > > designated initializers with older compilers. Now that gcc-5.1 is the > > minimum compiler version used for building kernels, that is no longer a > > concern. Similarly, the behavior of 'inline' functions changes between > > gnu89 and gnu89, but this was taken care of by defining 'inline' to > > Typo here? Second one should be gnu99 right? Fixed, thanks! Arnd
Re: [greybus-dev] [PATCH] Kbuild: remove -std=gnu89 from compiler arguments
On Mon, Feb 28, 2022 at 12:04 AM Alex Elder wrote: > On 2/27/22 3:52 PM, Arnd Bergmann wrote: From: Arnd Bergmann > > I put the suggestion into patch form, based on what we discussed > > in the thread. I only gave it minimal testing, but it would > > be good to have it in linux-next if we want to do this in the > > merge window. > > Did you determine what needed the new compiler flag based on > compilation results? > > Glancing at the Greybus code, I don't believe there's any > reason it needs to shift a negative value. Such warnings > could be fixed by making certain variables unsigned, for > example. > > I have no objection, I'll just make a note of it. I've clarified in the changelog that I'm adding the -Wno-shift-negative-value everywhere that -Wextra is used, not because I saw actual warnings for greybus and btrfs. The -Wextra is copied from scripts/Makefile.extrawarn, so this keeps it in sync. Ideally we should have an easier way for a subdirectory to get the W=1 flags without copying the list, but the patch I started to do this never got close to getting integrated. Arnd
Re: [PATCH] Kbuild: remove -std=gnu89 from compiler arguments
On Sun, Feb 27, 2022 at 11:36 PM Linus Torvalds wrote: > > And I don't want somebody with a newer compiler version to not notice > that he or she ended up using a c17 feature, just because _that_ > compiler supported it, and then other people get build errors because > their compilers use gnu11 instead by default. > > Put another way: I see absolutely no upside to allowing different > users using higher/lower versions of the standard. There are only > downsides. > > If gnu11 is supported by gcc-5.1 and up, and all the relevant clang > versions, then let's just pick that. Ok, changed my patch to -gnu11 now. > And if there are any possible future advantages to gnu17 (or eventual > gnu2x versions), let's document those, so that we can say "once our > compiler version requirements go up sufficiently, we'll move to gnuXX > because we want to take advantage of YY". > > Please? I think all of the options here are equally bad: picking gnu11 means we use a non-standard default for anything other than gcc-5 and may get surprised again in the future when we want to change to a newer version; -std=gnu1x would work as an alias for gnu17 in all versions including gcc-5 but is already marked as 'deprecated' in the gcc documentation; and using -std=gnu17 for modern compilers requires a workaround for gcc-7 and earlier. Regarding new features from gcc-2x, I think we already use most of what is listed in https://en.wikipedia.org/wiki/C2x, as those are all GNU extensions that are valid in modern gnu89 as well. Newly added features seem to only depend on the compiler version, e.g. #elifdef works in both clang-13 and gcc-12 with any -std=gnu?? argument, so picking an earlier standard won't stop people from breaking the build with older compilers. Arnd
Re: [greybus-dev] [PATCH] Kbuild: remove -std=gnu89 from compiler arguments
On Sun, Feb 27, 2022 at 3:04 PM Alex Elder wrote: > > Glancing at the Greybus code, I don't believe there's any > reason it needs to shift a negative value. Such warnings > could be fixed by making certain variables unsigned, for > example. As mentioned in the original thread, making things unsigned actually is likely to introduce bugs and make things worse. The warning is simply bogus, and the fact that it was enabled by -Wextra in gcc for std=gnu99 and up was a mistake that looks likely to be fixed in gcc. So don't try to "fix" the code to make any possible warnings go away. You may just make things worse. (That is often true in general for the more esoteric warnings, but in this case it's just painfully more obvious). Linus
Re: [PATCH] Kbuild: remove -std=gnu89 from compiler arguments
On Sun, Feb 27, 2022 at 1:54 PM Arnd Bergmann wrote: > > Since the differences between gnu99, gnu11 and gnu17 are fairly minimal > and mainly impact warnings at the -Wpedantic level that the kernel > never enables, the easiest way is to just leave out the -std=gnu89 > argument entirely, and rely on the compiler default language setting, > which is gnu11 for gcc-5, and gnu1x/gnu17 for all other supported > versions of gcc or clang. Honestly, I'd rather keep the C version we support as some explicit thing, instead of "whatever the installed compiler is". Not only do I suspect that you can set it in gcc spec files (so the standard version might actually be site-specific, not compiler version specific), but particularly with clang, I'd like that "GNU extensions enabled" to be explicit. Yes, maybe it's the default, but let's make sure. The C version level has traditionally had a lot of odd semantic meaning details - you mention "inline", others have existed. So it's not just the actual new features that some C version implements, it's those kind of "same syntax, different meaning" issues. I really don't think that's something we want in the kernel any more. Been there, done that, and we did the explicit standards level for a reason. It may be true that c99/c11/c17 are all very similar, and don't have those issues. Or maybe they do. And I don't want somebody with a newer compiler version to not notice that he or she ended up using a c17 feature, just because _that_ compiler supported it, and then other people get build errors because their compilers use gnu11 instead by default. Put another way: I see absolutely no upside to allowing different users using higher/lower versions of the standard. There are only downsides. If gnu11 is supported by gcc-5.1 and up, and all the relevant clang versions, then let's just pick that. And if there are any possible future advantages to gnu17 (or eventual gnu2x versions), let's document those, so that we can say "once our compiler version requirements go up sufficiently, we'll move to gnuXX because we want to take advantage of YY". Please? Linus Linus
[PATCH] Kbuild: remove -std=gnu89 from compiler arguments
From: Arnd Bergmann During a patch discussion, Linus brought up the option of changing the C standard version from gnu89 to gnu99, which allows using variable declaration inside of a for() loop. While the C99, C11 and later standards introduce many other features, most of these are already available in gnu89 as GNU extensions as well. An earlier attempt to do this when gcc-5 started defaulting to -std=gnu11 failed because at the time that caused warnings about designated initializers with older compilers. Now that gcc-5.1 is the minimum compiler version used for building kernels, that is no longer a concern. Similarly, the behavior of 'inline' functions changes between gnu89 and gnu89, but this was taken care of by defining 'inline' to include __attribute__((gnu_inline)) in order to allow building with clang a while ago. One minor issue that remains is an added gcc warning for shifts of negative integers when building with -Werror, which happens with the 'make W=1' option, as well as for three drivers in the kernel that always enable -Werror, but it was only observed with the i915 driver so far. Nathan Chancellor reported an additional -Wdeclaration-after-statement warning that appears in a system header on arm, this still needs a workaround. Since the differences between gnu99, gnu11 and gnu17 are fairly minimal and mainly impact warnings at the -Wpedantic level that the kernel never enables, the easiest way is to just leave out the -std=gnu89 argument entirely, and rely on the compiler default language setting, which is gnu11 for gcc-5, and gnu1x/gnu17 for all other supported versions of gcc or clang. Link: https://lore.kernel.org/lkml/CAHk-=wiych7xehcmifj-ygxuy2jaj7pnkdkpcovt8fybvfw...@mail.gmail.com/ Link: https://github.com/ClangBuiltLinux/linux/issues/1603 Suggested-by: Linus Torvalds Cc: Masahiro Yamada Cc: linux-kbu...@vger.kernel.org Cc: l...@lists.linux.dev Signed-off-by: Arnd Bergmann --- I put the suggestion into patch form, based on what we discussed in the thread. I only gave it minimal testing, but it would be good to have it in linux-next if we want to do this in the merge window. --- Documentation/process/programming-language.rst | 4 ++-- .../translations/it_IT/process/programming-language.rst| 4 ++-- .../translations/zh_CN/process/programming-language.rst| 4 ++-- .../translations/zh_TW/process/programming-language.rst| 4 ++-- Makefile | 7 +++ arch/arm64/kernel/vdso32/Makefile | 3 +-- drivers/gpu/drm/i915/Makefile | 1 + drivers/staging/greybus/tools/Makefile | 3 ++- fs/btrfs/Makefile | 1 + scripts/Makefile.extrawarn | 1 + 10 files changed, 17 insertions(+), 15 deletions(-) diff --git a/Documentation/process/programming-language.rst b/Documentation/process/programming-language.rst index ec474a70a02f..894f2a6eb9db 100644 --- a/Documentation/process/programming-language.rst +++ b/Documentation/process/programming-language.rst @@ -5,8 +5,8 @@ Programming Language The kernel is written in the C programming language [c-language]_. More precisely, the kernel is typically compiled with ``gcc`` [gcc]_ -under ``-std=gnu89`` [gcc-c-dialect-options]_: the GNU dialect of ISO C90 -(including some C99 features). ``clang`` [clang]_ is also supported, see +under ``-std=gnu11`` [gcc-c-dialect-options]_: the GNU dialect of ISO C11 +(including some C17 features). ``clang`` [clang]_ is also supported, see docs on :ref:`Building Linux with Clang/LLVM `. This dialect contains many extensions to the language [gnu-extensions]_, diff --git a/Documentation/translations/it_IT/process/programming-language.rst b/Documentation/translations/it_IT/process/programming-language.rst index 41db2598ce11..aa21097737ae 100644 --- a/Documentation/translations/it_IT/process/programming-language.rst +++ b/Documentation/translations/it_IT/process/programming-language.rst @@ -10,8 +10,8 @@ Linguaggio di programmazione Il kernel è scritto nel linguaggio di programmazione C [it-c-language]_. Più precisamente, il kernel viene compilato con ``gcc`` [it-gcc]_ usando -l'opzione ``-std=gnu89`` [it-gcc-c-dialect-options]_: il dialetto GNU -dello standard ISO C90 (con l'aggiunta di alcune funzionalità da C99). +l'opzione ``-std=gnu11`` [it-gcc-c-dialect-options]_: il dialetto GNU +dello standard ISO C11 (con l'aggiunta di alcune funzionalità da C17). Linux supporta anche ``clang`` [it-clang]_, leggete la documentazione :ref:`Building Linux with Clang/LLVM `. diff --git a/Documentation/translations/zh_CN/process/programming-language.rst b/Documentation/translations/zh_CN/process/programming-language.rst index 2a47a1d2ec20..58d2b3bd2d85 100644 --- a/Documentation/translations/zh_CN/process/programming-language.rst +++
