| Message ID | 20190812023214.107817-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: Avoid clang warnings around setjmp and longjmp | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (da206bd46848568e1aaf35f00e2d78bf9bc94f95) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 22 lines checked |
Le 12/08/2019 à 04:32, Nathan Chancellor a écrit : > Commit aea447141c7e ("powerpc: Disable -Wbuiltin-requires-header when > setjmp is used") disabled -Wbuiltin-requires-header because of a warning > about the setjmp and longjmp declarations. > > r367387 in clang added another diagnostic around this, complaining that > there is no jmp_buf declaration. > [...] > > Cc: stable@vger.kernel.org # 4.19+ > Link: https://github.com/ClangBuiltLinux/linux/issues/625 > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > [...] > > arch/powerpc/kernel/Makefile | 5 +++-- > arch/powerpc/xmon/Makefile | 5 +++-- What about scripts/recordmcount.c and scripts/sortextable.c which contains calls to setjmp() and longjmp() ? And arch/um/ ? Christophe > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile > index ea0c69236789..44e340ed4722 100644 > --- a/arch/powerpc/kernel/Makefile > +++ b/arch/powerpc/kernel/Makefile > @@ -5,8 +5,9 @@ > > CFLAGS_ptrace.o += -DUTS_MACHINE='"$(UTS_MACHINE)"' > > -# Disable clang warning for using setjmp without setjmp.h header > -CFLAGS_crash.o += $(call cc-disable-warning, builtin-requires-header) > +# Avoid clang warnings about longjmp and setjmp built-ins (inclusion of setjmp.h and declaration of jmp_buf type) > +CFLAGS_crash.o += $(call cc-disable-warning, builtin-requires-header) \ > + $(call cc-disable-warning, incomplete-setjmp-declaration) > > ifdef CONFIG_PPC64 > CFLAGS_prom_init.o += $(NO_MINIMAL_TOC) > diff --git a/arch/powerpc/xmon/Makefile b/arch/powerpc/xmon/Makefile > index f142570ad860..53f341391210 100644 > --- a/arch/powerpc/xmon/Makefile > +++ b/arch/powerpc/xmon/Makefile > @@ -1,8 +1,9 @@ > # SPDX-License-Identifier: GPL-2.0 > # Makefile for xmon > > -# Disable clang warning for using setjmp without setjmp.h header > -subdir-ccflags-y := $(call cc-disable-warning, builtin-requires-header) > +# Avoid clang warnings about longjmp and setjmp built-ins (inclusion of setjmp.h and declaration of jmp_buf type) > +subdir-ccflags-y := $(call cc-disable-warning, builtin-requires-header) \ > + $(call cc-disable-warning, incomplete-setjmp-declaration) > > GCOV_PROFILE := n > KCOV_INSTRUMENT := n >
On Mon, Aug 12, 2019 at 07:37:51AM +0200, Christophe Leroy wrote: > > > Le 12/08/2019 à 04:32, Nathan Chancellor a écrit : > > Commit aea447141c7e ("powerpc: Disable -Wbuiltin-requires-header when > > setjmp is used") disabled -Wbuiltin-requires-header because of a warning > > about the setjmp and longjmp declarations. > > > > r367387 in clang added another diagnostic around this, complaining that > > there is no jmp_buf declaration. > > > [...] > > > > > Cc: stable@vger.kernel.org # 4.19+ > > Link: https://github.com/ClangBuiltLinux/linux/issues/625 > > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07 > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > > [...] > > > > > arch/powerpc/kernel/Makefile | 5 +++-- > > arch/powerpc/xmon/Makefile | 5 +++-- > > What about scripts/recordmcount.c and scripts/sortextable.c which contains > calls to setjmp() and longjmp() ? > > And arch/um/ ? > > Christophe Hi Christophe, It looks like all of those will be using the system's setjmp header, which won't cause these warnings. Cheers, Nathan
On Sun, Aug 11, 2019 at 7:42 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > Commit aea447141c7e ("powerpc: Disable -Wbuiltin-requires-header when > setjmp is used") disabled -Wbuiltin-requires-header because of a warning > about the setjmp and longjmp declarations. > > r367387 in clang added another diagnostic around this, complaining that > there is no jmp_buf declaration. > > In file included from ../arch/powerpc/xmon/xmon.c:47: > ../arch/powerpc/include/asm/setjmp.h:10:13: error: declaration of > built-in function 'setjmp' requires the declaration of the 'jmp_buf' > type, commonly provided in the header <setjmp.h>. > [-Werror,-Wincomplete-setjmp-declaration] > extern long setjmp(long *); > ^ > ../arch/powerpc/include/asm/setjmp.h:11:13: error: declaration of > built-in function 'longjmp' requires the declaration of the 'jmp_buf' > type, commonly provided in the header <setjmp.h>. > [-Werror,-Wincomplete-setjmp-declaration] > extern void longjmp(long *, long); > ^ > 2 errors generated. > > Take the same approach as the above commit by disabling the warning for > the same reason, we provide our own longjmp/setjmp function. > > Cc: stable@vger.kernel.org # 4.19+ > Link: https://github.com/ClangBuiltLinux/linux/issues/625 > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> Thanks for the patch, Nathan. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > --- > > It may be worth using -fno-builtin-setjmp and -fno-builtin-longjmp > instead as it makes it clear to clang that we are not using the builtin > longjmp and setjmp functions, which I think is why these warnings are > appearing (at least according to the commit that introduced this waring). > > Sample patch: > https://github.com/ClangBuiltLinux/linux/issues/625#issuecomment-519251372 > > However, this is the most conservative approach, as I have already had > someone notice this error when building LLVM with PGO on tip of tree > LLVM. > > arch/powerpc/kernel/Makefile | 5 +++-- > arch/powerpc/xmon/Makefile | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile > index ea0c69236789..44e340ed4722 100644 > --- a/arch/powerpc/kernel/Makefile > +++ b/arch/powerpc/kernel/Makefile > @@ -5,8 +5,9 @@ > > CFLAGS_ptrace.o += -DUTS_MACHINE='"$(UTS_MACHINE)"' > > -# Disable clang warning for using setjmp without setjmp.h header > -CFLAGS_crash.o += $(call cc-disable-warning, builtin-requires-header) > +# Avoid clang warnings about longjmp and setjmp built-ins (inclusion of setjmp.h and declaration of jmp_buf type) > +CFLAGS_crash.o += $(call cc-disable-warning, builtin-requires-header) \ > + $(call cc-disable-warning, incomplete-setjmp-declaration) > > ifdef CONFIG_PPC64 > CFLAGS_prom_init.o += $(NO_MINIMAL_TOC) > diff --git a/arch/powerpc/xmon/Makefile b/arch/powerpc/xmon/Makefile > index f142570ad860..53f341391210 100644 > --- a/arch/powerpc/xmon/Makefile > +++ b/arch/powerpc/xmon/Makefile > @@ -1,8 +1,9 @@ > # SPDX-License-Identifier: GPL-2.0 > # Makefile for xmon > > -# Disable clang warning for using setjmp without setjmp.h header > -subdir-ccflags-y := $(call cc-disable-warning, builtin-requires-header) > +# Avoid clang warnings about longjmp and setjmp built-ins (inclusion of setjmp.h and declaration of jmp_buf type) > +subdir-ccflags-y := $(call cc-disable-warning, builtin-requires-header) \ > + $(call cc-disable-warning, incomplete-setjmp-declaration) > > GCOV_PROFILE := n > KCOV_INSTRUMENT := n > -- > 2.23.0.rc2 >
On Sun, Aug 11, 2019 at 07:32:15PM -0700, Nathan Chancellor wrote: > Commit aea447141c7e ("powerpc: Disable -Wbuiltin-requires-header when > setjmp is used") disabled -Wbuiltin-requires-header because of a warning > about the setjmp and longjmp declarations. > > r367387 in clang added another diagnostic around this, complaining that > there is no jmp_buf declaration. > > In file included from ../arch/powerpc/xmon/xmon.c:47: > ../arch/powerpc/include/asm/setjmp.h:10:13: error: declaration of > built-in function 'setjmp' requires the declaration of the 'jmp_buf' > type, commonly provided in the header <setjmp.h>. > [-Werror,-Wincomplete-setjmp-declaration] > extern long setjmp(long *); > ^ > ../arch/powerpc/include/asm/setjmp.h:11:13: error: declaration of > built-in function 'longjmp' requires the declaration of the 'jmp_buf' > type, commonly provided in the header <setjmp.h>. > [-Werror,-Wincomplete-setjmp-declaration] > extern void longjmp(long *, long); > ^ > 2 errors generated. > > Take the same approach as the above commit by disabling the warning for > the same reason, we provide our own longjmp/setjmp function. > > Cc: stable@vger.kernel.org # 4.19+ > Link: https://github.com/ClangBuiltLinux/linux/issues/625 > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > > It may be worth using -fno-builtin-setjmp and -fno-builtin-longjmp > instead as it makes it clear to clang that we are not using the builtin > longjmp and setjmp functions, which I think is why these warnings are > appearing (at least according to the commit that introduced this waring). > > Sample patch: > https://github.com/ClangBuiltLinux/linux/issues/625#issuecomment-519251372 > > However, this is the most conservative approach, as I have already had > someone notice this error when building LLVM with PGO on tip of tree > LLVM. > > arch/powerpc/kernel/Makefile | 5 +++-- > arch/powerpc/xmon/Makefile | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile > index ea0c69236789..44e340ed4722 100644 > --- a/arch/powerpc/kernel/Makefile > +++ b/arch/powerpc/kernel/Makefile > @@ -5,8 +5,9 @@ > > CFLAGS_ptrace.o += -DUTS_MACHINE='"$(UTS_MACHINE)"' > > -# Disable clang warning for using setjmp without setjmp.h header > -CFLAGS_crash.o += $(call cc-disable-warning, builtin-requires-header) > +# Avoid clang warnings about longjmp and setjmp built-ins (inclusion of setjmp.h and declaration of jmp_buf type) > +CFLAGS_crash.o += $(call cc-disable-warning, builtin-requires-header) \ > + $(call cc-disable-warning, incomplete-setjmp-declaration) > > ifdef CONFIG_PPC64 > CFLAGS_prom_init.o += $(NO_MINIMAL_TOC) > diff --git a/arch/powerpc/xmon/Makefile b/arch/powerpc/xmon/Makefile > index f142570ad860..53f341391210 100644 > --- a/arch/powerpc/xmon/Makefile > +++ b/arch/powerpc/xmon/Makefile > @@ -1,8 +1,9 @@ > # SPDX-License-Identifier: GPL-2.0 > # Makefile for xmon > > -# Disable clang warning for using setjmp without setjmp.h header > -subdir-ccflags-y := $(call cc-disable-warning, builtin-requires-header) > +# Avoid clang warnings about longjmp and setjmp built-ins (inclusion of setjmp.h and declaration of jmp_buf type) > +subdir-ccflags-y := $(call cc-disable-warning, builtin-requires-header) \ > + $(call cc-disable-warning, incomplete-setjmp-declaration) > > GCOV_PROFILE := n > KCOV_INSTRUMENT := n > -- > 2.23.0.rc2 > Did any of the maintainers have any comment on this patch? Cheers, Nathan
Nathan Chancellor <natechancellor@gmail.com> writes: > Commit aea447141c7e ("powerpc: Disable -Wbuiltin-requires-header when > setjmp is used") disabled -Wbuiltin-requires-header because of a warning > about the setjmp and longjmp declarations. > > r367387 in clang added another diagnostic around this, complaining that > there is no jmp_buf declaration. > > In file included from ../arch/powerpc/xmon/xmon.c:47: > ../arch/powerpc/include/asm/setjmp.h:10:13: error: declaration of > built-in function 'setjmp' requires the declaration of the 'jmp_buf' > type, commonly provided in the header <setjmp.h>. > [-Werror,-Wincomplete-setjmp-declaration] > extern long setjmp(long *); > ^ > ../arch/powerpc/include/asm/setjmp.h:11:13: error: declaration of > built-in function 'longjmp' requires the declaration of the 'jmp_buf' > type, commonly provided in the header <setjmp.h>. > [-Werror,-Wincomplete-setjmp-declaration] > extern void longjmp(long *, long); > ^ > 2 errors generated. > > Take the same approach as the above commit by disabling the warning for > the same reason, we provide our own longjmp/setjmp function. > > Cc: stable@vger.kernel.org # 4.19+ > Link: https://github.com/ClangBuiltLinux/linux/issues/625 > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > > It may be worth using -fno-builtin-setjmp and -fno-builtin-longjmp > instead as it makes it clear to clang that we are not using the builtin > longjmp and setjmp functions, which I think is why these warnings are > appearing (at least according to the commit that introduced this waring). > > Sample patch: > https://github.com/ClangBuiltLinux/linux/issues/625#issuecomment-519251372 Couldn't we just add those flags to CFLAGS for the whole kernel? Rather than making them per-file. I mean there's no kernel code that wants to use clang's builtin setjmp/longjmp implementation at all right? cheers
On Wed, Aug 28, 2019 at 11:43:53PM +1000, Michael Ellerman wrote: > Nathan Chancellor <natechancellor@gmail.com> writes: > > > Commit aea447141c7e ("powerpc: Disable -Wbuiltin-requires-header when > > setjmp is used") disabled -Wbuiltin-requires-header because of a warning > > about the setjmp and longjmp declarations. > > > > r367387 in clang added another diagnostic around this, complaining that > > there is no jmp_buf declaration. > > > > In file included from ../arch/powerpc/xmon/xmon.c:47: > > ../arch/powerpc/include/asm/setjmp.h:10:13: error: declaration of > > built-in function 'setjmp' requires the declaration of the 'jmp_buf' > > type, commonly provided in the header <setjmp.h>. > > [-Werror,-Wincomplete-setjmp-declaration] > > extern long setjmp(long *); > > ^ > > ../arch/powerpc/include/asm/setjmp.h:11:13: error: declaration of > > built-in function 'longjmp' requires the declaration of the 'jmp_buf' > > type, commonly provided in the header <setjmp.h>. > > [-Werror,-Wincomplete-setjmp-declaration] > > extern void longjmp(long *, long); > > ^ > > 2 errors generated. > > > > Take the same approach as the above commit by disabling the warning for > > the same reason, we provide our own longjmp/setjmp function. > > > > Cc: stable@vger.kernel.org # 4.19+ > > Link: https://github.com/ClangBuiltLinux/linux/issues/625 > > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07 > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > > > It may be worth using -fno-builtin-setjmp and -fno-builtin-longjmp > > instead as it makes it clear to clang that we are not using the builtin > > longjmp and setjmp functions, which I think is why these warnings are > > appearing (at least according to the commit that introduced this waring). > > > > Sample patch: > > https://github.com/ClangBuiltLinux/linux/issues/625#issuecomment-519251372 > > Couldn't we just add those flags to CFLAGS for the whole kernel? Rather > than making them per-file. Yes, I don't think this would be unreasonable. Are you referring to the cc-disable-warning flags or the -fno-builtin flags? I personally think the -fno-builtin flags convey to clang what the kernel is intending to do better than disabling the warnings outright. > I mean there's no kernel code that wants to use clang's builtin > setjmp/longjmp implementation at all right? > > cheers I did a quick search of the tree and it looks like powerpc and x86/um are the only architectures that do anything with setjmp/longjmp. x86/um avoids this by using a define flag to change setjmp to kernel_setjmp: arch/um/Makefile: -Dlongjmp=kernel_longjmp -Dsetjmp=kernel_setjmp \ Seems like adding those flags should be safe. Cheers, Nathan
On Wed, Aug 28, 2019 at 10:53 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Wed, Aug 28, 2019 at 11:43:53PM +1000, Michael Ellerman wrote: > > Nathan Chancellor <natechancellor@gmail.com> writes: > > > > > Commit aea447141c7e ("powerpc: Disable -Wbuiltin-requires-header when > > > setjmp is used") disabled -Wbuiltin-requires-header because of a warning > > > about the setjmp and longjmp declarations. > > > > > > r367387 in clang added another diagnostic around this, complaining that > > > there is no jmp_buf declaration. > > > > > > In file included from ../arch/powerpc/xmon/xmon.c:47: > > > ../arch/powerpc/include/asm/setjmp.h:10:13: error: declaration of > > > built-in function 'setjmp' requires the declaration of the 'jmp_buf' > > > type, commonly provided in the header <setjmp.h>. > > > [-Werror,-Wincomplete-setjmp-declaration] > > > extern long setjmp(long *); > > > ^ > > > ../arch/powerpc/include/asm/setjmp.h:11:13: error: declaration of > > > built-in function 'longjmp' requires the declaration of the 'jmp_buf' > > > type, commonly provided in the header <setjmp.h>. > > > [-Werror,-Wincomplete-setjmp-declaration] > > > extern void longjmp(long *, long); > > > ^ > > > 2 errors generated. > > > > > > Take the same approach as the above commit by disabling the warning for > > > the same reason, we provide our own longjmp/setjmp function. > > > > > > Cc: stable@vger.kernel.org # 4.19+ > > > Link: https://github.com/ClangBuiltLinux/linux/issues/625 > > > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07 > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > --- > > > > > > It may be worth using -fno-builtin-setjmp and -fno-builtin-longjmp > > > instead as it makes it clear to clang that we are not using the builtin > > > longjmp and setjmp functions, which I think is why these warnings are > > > appearing (at least according to the commit that introduced this waring). > > > > > > Sample patch: > > > https://github.com/ClangBuiltLinux/linux/issues/625#issuecomment-519251372 > > > > Couldn't we just add those flags to CFLAGS for the whole kernel? Rather > > than making them per-file. > > Yes, I don't think this would be unreasonable. Are you referring to the > cc-disable-warning flags or the -fno-builtin flags? I personally think > the -fno-builtin flags convey to clang what the kernel is intending to > do better than disabling the warnings outright. The `-f` family of flags have dire implications for codegen, I'd really prefer we think long and hard before adding/removing them to suppress warnings. I don't think it's a solution for this particular problem. > > > I mean there's no kernel code that wants to use clang's builtin > > setjmp/longjmp implementation at all right? > > > > cheers > > I did a quick search of the tree and it looks like powerpc and x86/um > are the only architectures that do anything with setjmp/longjmp. x86/um > avoids this by using a define flag to change setjmp to kernel_setjmp: > > arch/um/Makefile: -Dlongjmp=kernel_longjmp -Dsetjmp=kernel_setjmp \ > > Seems like adding those flags should be safe. > > Cheers, > Nathan
On Wed, Aug 28, 2019 at 11:01:14AM -0700, Nick Desaulniers wrote: > On Wed, Aug 28, 2019 at 10:53 AM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Wed, Aug 28, 2019 at 11:43:53PM +1000, Michael Ellerman wrote: > > > Nathan Chancellor <natechancellor@gmail.com> writes: > > > > > > > Commit aea447141c7e ("powerpc: Disable -Wbuiltin-requires-header when > > > > setjmp is used") disabled -Wbuiltin-requires-header because of a warning > > > > about the setjmp and longjmp declarations. > > > > > > > > r367387 in clang added another diagnostic around this, complaining that > > > > there is no jmp_buf declaration. > > > > > > > > In file included from ../arch/powerpc/xmon/xmon.c:47: > > > > ../arch/powerpc/include/asm/setjmp.h:10:13: error: declaration of > > > > built-in function 'setjmp' requires the declaration of the 'jmp_buf' > > > > type, commonly provided in the header <setjmp.h>. > > > > [-Werror,-Wincomplete-setjmp-declaration] > > > > extern long setjmp(long *); > > > > ^ > > > > ../arch/powerpc/include/asm/setjmp.h:11:13: error: declaration of > > > > built-in function 'longjmp' requires the declaration of the 'jmp_buf' > > > > type, commonly provided in the header <setjmp.h>. > > > > [-Werror,-Wincomplete-setjmp-declaration] > > > > extern void longjmp(long *, long); > > > > ^ > > > > 2 errors generated. > > > > > > > > Take the same approach as the above commit by disabling the warning for > > > > the same reason, we provide our own longjmp/setjmp function. > > > > > > > > Cc: stable@vger.kernel.org # 4.19+ > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/625 > > > > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07 > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > --- > > > > > > > > It may be worth using -fno-builtin-setjmp and -fno-builtin-longjmp > > > > instead as it makes it clear to clang that we are not using the builtin > > > > longjmp and setjmp functions, which I think is why these warnings are > > > > appearing (at least according to the commit that introduced this waring). > > > > > > > > Sample patch: > > > > https://github.com/ClangBuiltLinux/linux/issues/625#issuecomment-519251372 > > > > > > Couldn't we just add those flags to CFLAGS for the whole kernel? Rather > > > than making them per-file. > > > > Yes, I don't think this would be unreasonable. Are you referring to the > > cc-disable-warning flags or the -fno-builtin flags? I personally think > > the -fno-builtin flags convey to clang what the kernel is intending to > > do better than disabling the warnings outright. > > The `-f` family of flags have dire implications for codegen, I'd > really prefer we think long and hard before adding/removing them to > suppress warnings. I don't think it's a solution for this particular > problem. I am fine with whatever approach gets this warning fixed to the maintainer's satisfaction... However, I think that -fno-builtin-* would be appropriate here because we are providing our own setjmp implementation, meaning clang should not be trying to do anything with the builtin implementation like building a declaration for it. Cheers, Nathan
On Wed, Aug 28, 2019 at 11:45 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Wed, Aug 28, 2019 at 11:01:14AM -0700, Nick Desaulniers wrote: > > On Wed, Aug 28, 2019 at 10:53 AM Nathan Chancellor > > <natechancellor@gmail.com> wrote: > > > > > > Yes, I don't think this would be unreasonable. Are you referring to the > > > cc-disable-warning flags or the -fno-builtin flags? I personally think > > > the -fno-builtin flags convey to clang what the kernel is intending to > > > do better than disabling the warnings outright. > > > > The `-f` family of flags have dire implications for codegen, I'd > > really prefer we think long and hard before adding/removing them to > > suppress warnings. I don't think it's a solution for this particular > > problem. > > I am fine with whatever approach gets this warning fixed to the > maintainer's satisfaction... > > However, I think that -fno-builtin-* would be appropriate here because > we are providing our own setjmp implementation, meaning clang should not > be trying to do anything with the builtin implementation like building a > declaration for it. That's a good reason IMO. IIRC, the -fno-builtin-* flags don't warn if * is some unrecognized value, so -fno-builtin-setjmp may not actually do anything, and you may need to scan the source (of clang or llvm).
On Wed, Aug 28, 2019 at 03:16:19PM -0700, Nick Desaulniers wrote: > That's a good reason IMO. IIRC, the -fno-builtin-* flags don't warn > if * is some unrecognized value, so -fno-builtin-setjmp may not > actually do anything, and you may need to scan the source (of clang or > llvm). -fno-builtin-foo makes the compiler not handle "foo" the same as it handles "__builtin_foo". If the compiler has no idea about "foo", well, that is exactly what it does then anyhow, so why would it warn :-) Segher
From: Nathan Chancellor > Sent: 28 August 2019 19:45 ... > However, I think that -fno-builtin-* would be appropriate here because > we are providing our own setjmp implementation, meaning clang should not > be trying to do anything with the builtin implementation like building a > declaration for it. Isn't implementing setjmp impossible unless you tell the compiler that you function is 'setjmp-like' ? For instance I think it all goes horribly wrong if the generated code looks like: push local_variable // setup arguments to setjmp call setjmp pop local_variable // check return value of setjmp With a naive compiler and simple ABI setjmp just has to save the return address, stack pointer and caller saved registers. With modern compilers and ABI I doubt you can implement setjmp without some help from the compiler. It is probably best to avoid setjmp/longjmp completely. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Aug 29, 2019 at 09:59:48AM +0000, David Laight wrote: > From: Nathan Chancellor > > Sent: 28 August 2019 19:45 > ... > > However, I think that -fno-builtin-* would be appropriate here because > > we are providing our own setjmp implementation, meaning clang should not > > be trying to do anything with the builtin implementation like building a > > declaration for it. > > Isn't implementing setjmp impossible unless you tell the compiler that > you function is 'setjmp-like' ? No idea, PowerPC is the only architecture that does such a thing. https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/tree/arch/powerpc/kernel/misc.S#n43 Goes back all the way to before git history (all the way to ppc64's addition actually): https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=61542216fa90397a2e70c46583edf26bc81994df https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/arch/ppc64/xmon/setjmp.c?id=5f12b0bff93831620218e8ed3970903ecb7861ce I would just like this warning fixed given that PowerPC builds with -Werror by default so it is causing a build failure in our CI. Cheers, Nathan
On Mon, Sep 02, 2019 at 10:55:53PM -0700, Nathan Chancellor wrote: > On Thu, Aug 29, 2019 at 09:59:48AM +0000, David Laight wrote: > > From: Nathan Chancellor > > > Sent: 28 August 2019 19:45 > > ... > > > However, I think that -fno-builtin-* would be appropriate here because > > > we are providing our own setjmp implementation, meaning clang should not > > > be trying to do anything with the builtin implementation like building a > > > declaration for it. > > > > Isn't implementing setjmp impossible unless you tell the compiler that > > you function is 'setjmp-like' ? > > No idea, PowerPC is the only architecture that does such a thing. Since setjmp can return more than once, yes, exciting things can happen if you do not tell the compiler about this. Segher
On Tue, Sep 03, 2019 at 02:31:28PM -0500, Segher Boessenkool wrote: > On Mon, Sep 02, 2019 at 10:55:53PM -0700, Nathan Chancellor wrote: > > On Thu, Aug 29, 2019 at 09:59:48AM +0000, David Laight wrote: > > > From: Nathan Chancellor > > > > Sent: 28 August 2019 19:45 > > > ... > > > > However, I think that -fno-builtin-* would be appropriate here because > > > > we are providing our own setjmp implementation, meaning clang should not > > > > be trying to do anything with the builtin implementation like building a > > > > declaration for it. > > > > > > Isn't implementing setjmp impossible unless you tell the compiler that > > > you function is 'setjmp-like' ? > > > > No idea, PowerPC is the only architecture that does such a thing. > > Since setjmp can return more than once, yes, exciting things can happen > if you do not tell the compiler about this. > > > Segher > Fair enough so I guess we are back to just outright disabling the warning. Cheers, Nathan
From: Nathan Chancellor [mailto:natechancellor@gmail.com] > Sent: 04 September 2019 01:24 > On Tue, Sep 03, 2019 at 02:31:28PM -0500, Segher Boessenkool wrote: > > On Mon, Sep 02, 2019 at 10:55:53PM -0700, Nathan Chancellor wrote: > > > On Thu, Aug 29, 2019 at 09:59:48AM +0000, David Laight wrote: > > > > From: Nathan Chancellor > > > > > Sent: 28 August 2019 19:45 > > > > ... > > > > > However, I think that -fno-builtin-* would be appropriate here because > > > > > we are providing our own setjmp implementation, meaning clang should not > > > > > be trying to do anything with the builtin implementation like building a > > > > > declaration for it. > > > > > > > > Isn't implementing setjmp impossible unless you tell the compiler that > > > > you function is 'setjmp-like' ? > > > > > > No idea, PowerPC is the only architecture that does such a thing. > > > > Since setjmp can return more than once, yes, exciting things can happen > > if you do not tell the compiler about this. > > > > > > Segher > > > > Fair enough so I guess we are back to just outright disabling the > warning. Just disabling the warning won't stop the compiler generating code that breaks a 'user' implementation of setjmp(). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Sep 04, 2019 at 08:16:45AM +0000, David Laight wrote: > From: Nathan Chancellor [mailto:natechancellor@gmail.com] > > Fair enough so I guess we are back to just outright disabling the > > warning. > > Just disabling the warning won't stop the compiler generating code > that breaks a 'user' implementation of setjmp(). Yeah. I have a patch (will send in an hour or so) that enables the "returns_twice" attribute for setjmp (in <asm/setjmp.h>). In testing (with GCC trunk) it showed no difference in code generation, but better save than sorry. It also sets "noreturn" on longjmp, and that *does* help, it saves a hundred insns or so (all in xmon, no surprise there). I don't think this will make LLVM shut up about this though. And technically it is right: the C standard does say that in hosted mode setjmp is a reserved name and you need to include <setjmp.h> to access it (not <asm/setjmp.h>). So why is the kernel compiled as hosted? Does adding -ffreestanding hurt anything? Is that actually supported on LLVM, on all relevant versions of it? Does it shut up the warning there (if not, that would be an LLVM bug)? Segher
On Wed, Sep 04, 2019 at 08:01:35AM -0500, Segher Boessenkool wrote: > On Wed, Sep 04, 2019 at 08:16:45AM +0000, David Laight wrote: > > From: Nathan Chancellor [mailto:natechancellor@gmail.com] > > > Fair enough so I guess we are back to just outright disabling the > > > warning. > > > > Just disabling the warning won't stop the compiler generating code > > that breaks a 'user' implementation of setjmp(). > > Yeah. I have a patch (will send in an hour or so) that enables the > "returns_twice" attribute for setjmp (in <asm/setjmp.h>). In testing > (with GCC trunk) it showed no difference in code generation, but > better save than sorry. > > It also sets "noreturn" on longjmp, and that *does* help, it saves a > hundred insns or so (all in xmon, no surprise there). > > I don't think this will make LLVM shut up about this though. And > technically it is right: the C standard does say that in hosted mode > setjmp is a reserved name and you need to include <setjmp.h> to access > it (not <asm/setjmp.h>). It does not fix the warning, I tested your patch. > So why is the kernel compiled as hosted? Does adding -ffreestanding > hurt anything? Is that actually supported on LLVM, on all relevant > versions of it? Does it shut up the warning there (if not, that would > be an LLVM bug)? It does fix this warning because -ffreestanding implies -fno-builtin, which also solves the warning. LLVM has supported -ffreestanding since at least 3.0.0. There are some parts of the kernel that are compiled with this and it probably should be used in more places but it sounds like there might be some good codegen improvements that are disabled with it: https://lore.kernel.org/lkml/CAHk-=wi-epJZfBHDbKKDZ64us7WkF=LpUfhvYBmZSteO8Q0RAg@mail.gmail.com/ Cheers, Nathan
Nathan Chancellor <natechancellor@gmail.com> writes: > On Wed, Sep 04, 2019 at 08:01:35AM -0500, Segher Boessenkool wrote: >> On Wed, Sep 04, 2019 at 08:16:45AM +0000, David Laight wrote: >> > From: Nathan Chancellor [mailto:natechancellor@gmail.com] >> > > Fair enough so I guess we are back to just outright disabling the >> > > warning. >> > >> > Just disabling the warning won't stop the compiler generating code >> > that breaks a 'user' implementation of setjmp(). >> >> Yeah. I have a patch (will send in an hour or so) that enables the >> "returns_twice" attribute for setjmp (in <asm/setjmp.h>). In testing >> (with GCC trunk) it showed no difference in code generation, but >> better save than sorry. >> >> It also sets "noreturn" on longjmp, and that *does* help, it saves a >> hundred insns or so (all in xmon, no surprise there). >> >> I don't think this will make LLVM shut up about this though. And >> technically it is right: the C standard does say that in hosted mode >> setjmp is a reserved name and you need to include <setjmp.h> to access >> it (not <asm/setjmp.h>). > > It does not fix the warning, I tested your patch. > >> So why is the kernel compiled as hosted? Does adding -ffreestanding >> hurt anything? Is that actually supported on LLVM, on all relevant >> versions of it? Does it shut up the warning there (if not, that would >> be an LLVM bug)? > > It does fix this warning because -ffreestanding implies -fno-builtin, > which also solves the warning. LLVM has supported -ffreestanding since > at least 3.0.0. There are some parts of the kernel that are compiled > with this and it probably should be used in more places but it sounds > like there might be some good codegen improvements that are disabled > with it: > > https://lore.kernel.org/lkml/CAHk-=wi-epJZfBHDbKKDZ64us7WkF=LpUfhvYBmZSteO8Q0RAg@mail.gmail.com/ For xmon.c and crash.c I think using -ffreestanding would be fine. They're both crash/debug code, so we don't care about minor optimisation differences. If anything we don't want the compiler being too clever when generating that code. cheers
On Wed, Sep 11, 2019 at 04:30:38AM +1000, Michael Ellerman wrote: > Nathan Chancellor <natechancellor@gmail.com> writes: > > On Wed, Sep 04, 2019 at 08:01:35AM -0500, Segher Boessenkool wrote: > >> On Wed, Sep 04, 2019 at 08:16:45AM +0000, David Laight wrote: > >> > From: Nathan Chancellor [mailto:natechancellor@gmail.com] > >> > > Fair enough so I guess we are back to just outright disabling the > >> > > warning. > >> > > >> > Just disabling the warning won't stop the compiler generating code > >> > that breaks a 'user' implementation of setjmp(). > >> > >> Yeah. I have a patch (will send in an hour or so) that enables the > >> "returns_twice" attribute for setjmp (in <asm/setjmp.h>). In testing > >> (with GCC trunk) it showed no difference in code generation, but > >> better save than sorry. > >> > >> It also sets "noreturn" on longjmp, and that *does* help, it saves a > >> hundred insns or so (all in xmon, no surprise there). > >> > >> I don't think this will make LLVM shut up about this though. And > >> technically it is right: the C standard does say that in hosted mode > >> setjmp is a reserved name and you need to include <setjmp.h> to access > >> it (not <asm/setjmp.h>). > > > > It does not fix the warning, I tested your patch. > > > >> So why is the kernel compiled as hosted? Does adding -ffreestanding > >> hurt anything? Is that actually supported on LLVM, on all relevant > >> versions of it? Does it shut up the warning there (if not, that would > >> be an LLVM bug)? > > > > It does fix this warning because -ffreestanding implies -fno-builtin, > > which also solves the warning. LLVM has supported -ffreestanding since > > at least 3.0.0. There are some parts of the kernel that are compiled > > with this and it probably should be used in more places but it sounds > > like there might be some good codegen improvements that are disabled > > with it: > > > > https://lore.kernel.org/lkml/CAHk-=wi-epJZfBHDbKKDZ64us7WkF=LpUfhvYBmZSteO8Q0RAg@mail.gmail.com/ > > For xmon.c and crash.c I think using -ffreestanding would be fine. > They're both crash/debug code, so we don't care about minor optimisation > differences. If anything we don't want the compiler being too clever > when generating that code. > > cheers I will send a v2 later today along with another patch to fix this warning and another build error. Cheers, Nathan
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile index ea0c69236789..44e340ed4722 100644 --- a/arch/powerpc/kernel/Makefile +++ b/arch/powerpc/kernel/Makefile @@ -5,8 +5,9 @@ CFLAGS_ptrace.o += -DUTS_MACHINE='"$(UTS_MACHINE)"' -# Disable clang warning for using setjmp without setjmp.h header -CFLAGS_crash.o += $(call cc-disable-warning, builtin-requires-header) +# Avoid clang warnings about longjmp and setjmp built-ins (inclusion of setjmp.h and declaration of jmp_buf type) +CFLAGS_crash.o += $(call cc-disable-warning, builtin-requires-header) \ + $(call cc-disable-warning, incomplete-setjmp-declaration) ifdef CONFIG_PPC64 CFLAGS_prom_init.o += $(NO_MINIMAL_TOC) diff --git a/arch/powerpc/xmon/Makefile b/arch/powerpc/xmon/Makefile index f142570ad860..53f341391210 100644 --- a/arch/powerpc/xmon/Makefile +++ b/arch/powerpc/xmon/Makefile @@ -1,8 +1,9 @@ # SPDX-License-Identifier: GPL-2.0 # Makefile for xmon -# Disable clang warning for using setjmp without setjmp.h header -subdir-ccflags-y := $(call cc-disable-warning, builtin-requires-header) +# Avoid clang warnings about longjmp and setjmp built-ins (inclusion of setjmp.h and declaration of jmp_buf type) +subdir-ccflags-y := $(call cc-disable-warning, builtin-requires-header) \ + $(call cc-disable-warning, incomplete-setjmp-declaration) GCOV_PROFILE := n KCOV_INSTRUMENT := n
Commit aea447141c7e ("powerpc: Disable -Wbuiltin-requires-header when setjmp is used") disabled -Wbuiltin-requires-header because of a warning about the setjmp and longjmp declarations. r367387 in clang added another diagnostic around this, complaining that there is no jmp_buf declaration. In file included from ../arch/powerpc/xmon/xmon.c:47: ../arch/powerpc/include/asm/setjmp.h:10:13: error: declaration of built-in function 'setjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header <setjmp.h>. [-Werror,-Wincomplete-setjmp-declaration] extern long setjmp(long *); ^ ../arch/powerpc/include/asm/setjmp.h:11:13: error: declaration of built-in function 'longjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header <setjmp.h>. [-Werror,-Wincomplete-setjmp-declaration] extern void longjmp(long *, long); ^ 2 errors generated. Take the same approach as the above commit by disabling the warning for the same reason, we provide our own longjmp/setjmp function. Cc: stable@vger.kernel.org # 4.19+ Link: https://github.com/ClangBuiltLinux/linux/issues/625 Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07 Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- It may be worth using -fno-builtin-setjmp and -fno-builtin-longjmp instead as it makes it clear to clang that we are not using the builtin longjmp and setjmp functions, which I think is why these warnings are appearing (at least according to the commit that introduced this waring). Sample patch: https://github.com/ClangBuiltLinux/linux/issues/625#issuecomment-519251372 However, this is the most conservative approach, as I have already had someone notice this error when building LLVM with PGO on tip of tree LLVM. arch/powerpc/kernel/Makefile | 5 +++-- arch/powerpc/xmon/Makefile | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-)