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: [email protected] # 4.19+
Link: https://github.com/ClangBuiltLinux/linux/issues/625
Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07
Signed-off-by: Nathan Chancellor <[email protected]>
---
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
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: [email protected] # 4.19+
> Link: https://github.com/ClangBuiltLinux/linux/issues/625
> Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
>
[...]
>
> 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: [email protected] # 4.19+
> > Link: https://github.com/ClangBuiltLinux/linux/issues/625
> > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07
> > Signed-off-by: Nathan Chancellor <[email protected]>
> > ---
> >
> [...]
>
> >
> > 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
<[email protected]> 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: [email protected] # 4.19+
> Link: https://github.com/ClangBuiltLinux/linux/issues/625
> Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07
> Signed-off-by: Nathan Chancellor <[email protected]>
Thanks for the patch, Nathan.
Reviewed-by: Nick Desaulniers <[email protected]>
> ---
>
> 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
>
--
Thanks,
~Nick Desaulniers
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: [email protected] # 4.19+
> Link: https://github.com/ClangBuiltLinux/linux/issues/625
> Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
>
> 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 <[email protected]> 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: [email protected] # 4.19+
> Link: https://github.com/ClangBuiltLinux/linux/issues/625
> Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
>
> 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 <[email protected]> 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: [email protected] # 4.19+
> > Link: https://github.com/ClangBuiltLinux/linux/issues/625
> > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07
> > Signed-off-by: Nathan Chancellor <[email protected]>
> > ---
> >
> > 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
<[email protected]> wrote:
>
> On Wed, Aug 28, 2019 at 11:43:53PM +1000, Michael Ellerman wrote:
> > Nathan Chancellor <[email protected]> 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: [email protected] # 4.19+
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/625
> > > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07
> > > Signed-off-by: Nathan Chancellor <[email protected]>
> > > ---
> > >
> > > 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
--
Thanks,
~Nick Desaulniers
On Wed, Aug 28, 2019 at 11:01:14AM -0700, Nick Desaulniers wrote:
> On Wed, Aug 28, 2019 at 10:53 AM Nathan Chancellor
> <[email protected]> wrote:
> >
> > On Wed, Aug 28, 2019 at 11:43:53PM +1000, Michael Ellerman wrote:
> > > Nathan Chancellor <[email protected]> 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: [email protected] # 4.19+
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/625
> > > > Link: https://github.com/llvm/llvm-project/commit/3be25e79477db2d31ac46493d97eca8c20592b07
> > > > Signed-off-by: Nathan Chancellor <[email protected]>
> > > > ---
> > > >
> > > > 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
<[email protected]> 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
> > <[email protected]> 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).
--
Thanks,
~Nick Desaulniers
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
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
On Wed, Sep 04, 2019 at 08:16:45AM +0000, David Laight wrote:
> From: Nathan Chancellor [mailto:[email protected]]
> > 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:[email protected]]
> > > 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 <[email protected]> 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:[email protected]]
>> > > 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 <[email protected]> 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:[email protected]]
> >> > > 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