Re: [PATCH v1] powerpc: Make setjmp/longjump signature standard
On Fri, Mar 27, 2020 at 06:45:21PM +0100, Christophe Leroy wrote: > Subject line, change longjump to longjmp > > Le 27/03/2020 à 11:07, Clement Courbet a écrit : > > Declaring setjmp()/longjmp() as taking longs makes the signature > > non-standard, and makes clang complain. In the past, this has been > > worked around by adding -ffreestanding to the compile flags. > > > > The implementation looks like it only ever propagates the value > > (in longjmp) or sets it to 1 (in setjmp), and we only call longjmp > > with integer parameters. > > > > This allows removing -ffreestanding from the compilation flags. > > > > Context: > > https://lore.kernel.org/patchwork/patch/1214060 > > https://lore.kernel.org/patchwork/patch/1216174 > > > > Signed-off-by: Clement Courbet > > --- > > arch/powerpc/include/asm/setjmp.h | 6 -- > > arch/powerpc/kexec/Makefile | 3 --- > > 2 files changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/setjmp.h > > b/arch/powerpc/include/asm/setjmp.h > > index e9f81bb3f83b..84bb0d140d59 100644 > > --- a/arch/powerpc/include/asm/setjmp.h > > +++ b/arch/powerpc/include/asm/setjmp.h > > @@ -7,7 +7,9 @@ > > #define JMP_BUF_LEN23 > > -extern long setjmp(long *) __attribute__((returns_twice)); > > -extern void longjmp(long *, long) __attribute__((noreturn)); > > +typedef long *jmp_buf; > > Do we need that new opaque typedef ? Why not just keep long * ? Yes, otherwise the warning comes back: In file included from arch/powerpc/kexec/crash.c:25: arch/powerpc/include/asm/setjmp.h:10:12: error: declaration of built-in function 'setjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header . [-Werror,-Wincomplete-setjmp-declaration] extern int setjmp(long *env) __attribute__((returns_twice)); ^ 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 . [-Werror,-Wincomplete-setjmp-declaration] extern void longjmp(long *env, int val) __attribute__((noreturn)); ^ 2 errors generated. > > + > > +extern int setjmp(jmp_buf env) __attribute__((returns_twice)); > > +extern void longjmp(jmp_buf env, int val) __attribute__((noreturn)); > > #endif /* _ASM_POWERPC_SETJMP_H */ > > diff --git a/arch/powerpc/kexec/Makefile b/arch/powerpc/kexec/Makefile > > index 378f6108a414..86380c69f5ce 100644 > > --- a/arch/powerpc/kexec/Makefile > > +++ b/arch/powerpc/kexec/Makefile > > @@ -3,9 +3,6 @@ > > # Makefile for the linux kernel. > > # > > -# Avoid clang warnings around longjmp/setjmp declarations > > -CFLAGS_crash.o += -ffreestanding > > - > > obj-y += core.o crash.o core_$(BITS).o > > obj-$(CONFIG_PPC32) += relocate_32.o > > > > Christophe
Re: [PATCH v1] powerpc: Make setjmp/longjump signature standard
Subject line, change longjump to longjmp Le 27/03/2020 à 11:07, Clement Courbet a écrit : Declaring setjmp()/longjmp() as taking longs makes the signature non-standard, and makes clang complain. In the past, this has been worked around by adding -ffreestanding to the compile flags. The implementation looks like it only ever propagates the value (in longjmp) or sets it to 1 (in setjmp), and we only call longjmp with integer parameters. This allows removing -ffreestanding from the compilation flags. Context: https://lore.kernel.org/patchwork/patch/1214060 https://lore.kernel.org/patchwork/patch/1216174 Signed-off-by: Clement Courbet --- arch/powerpc/include/asm/setjmp.h | 6 -- arch/powerpc/kexec/Makefile | 3 --- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/arch/powerpc/include/asm/setjmp.h b/arch/powerpc/include/asm/setjmp.h index e9f81bb3f83b..84bb0d140d59 100644 --- a/arch/powerpc/include/asm/setjmp.h +++ b/arch/powerpc/include/asm/setjmp.h @@ -7,7 +7,9 @@ #define JMP_BUF_LEN23 -extern long setjmp(long *) __attribute__((returns_twice)); -extern void longjmp(long *, long) __attribute__((noreturn)); +typedef long *jmp_buf; Do we need that new opaque typedef ? Why not just keep long * ? + +extern int setjmp(jmp_buf env) __attribute__((returns_twice)); +extern void longjmp(jmp_buf env, int val) __attribute__((noreturn)); #endif /* _ASM_POWERPC_SETJMP_H */ diff --git a/arch/powerpc/kexec/Makefile b/arch/powerpc/kexec/Makefile index 378f6108a414..86380c69f5ce 100644 --- a/arch/powerpc/kexec/Makefile +++ b/arch/powerpc/kexec/Makefile @@ -3,9 +3,6 @@ # Makefile for the linux kernel. # -# Avoid clang warnings around longjmp/setjmp declarations -CFLAGS_crash.o += -ffreestanding - obj-y += core.o crash.o core_$(BITS).o obj-$(CONFIG_PPC32) += relocate_32.o Christophe
Re: [PATCH v1] powerpc: Make setjmp/longjump signature standard
On Fri, Mar 27, 2020 at 10:10:44AM -0700, Nick Desaulniers wrote:
> On Fri, Mar 27, 2020 at 3:08 AM Clement Courbet wrote:
> >
> > Declaring setjmp()/longjmp() as taking longs makes the signature
> > non-standard, and makes clang complain. In the past, this has been
> > worked around by adding -ffreestanding to the compile flags.
> >
> > The implementation looks like it only ever propagates the value
> > (in longjmp) or sets it to 1 (in setjmp), and we only call longjmp
> > with integer parameters.
> >
> > This allows removing -ffreestanding from the compilation flags.
> >
> > Context:
> > https://lore.kernel.org/patchwork/patch/1214060
> > https://lore.kernel.org/patchwork/patch/1216174
> >
> > Signed-off-by: Clement Courbet
Thanks for fixing this properly, not really sure why I did not think of
this in the first place. I guess my thought was the warning makes it
seem like clang is going to ignore the kernel's implementation of
setjmp/longjmp but I can't truly remember.
> Hi Clement, thanks for the patch! Would you mind sending a V2 that
> included a similar fix to arch/powerpc/xmon/Makefile?
Agreed.
> For context, this was the original patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=aea447141c7e7824b81b49acd1bc78
> which was then modified to:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c9029ef9c95765e7b63c4d9aa780674447db1ec0
>
> So on your V2, if you include in the commit message, the line:
>
> Fixes c9029ef9c957 ("powerpc: Avoid clang warnings around setjmp and longjmp")
>
> then that will help our LTS branch maintainers back port it to the
> appropriate branches.
The tags should be:
Cc: sta...@vger.kernel.org # v4.14+
Fixes: c9029ef9c957 ("powerpc: Avoid clang warnings around setjmp and longjmp")
that way it explicitly gets picked up for stable, rather than Sasha's
AUTOSEL process, which could miss it.
With the xmon/Makefile -ffreestanding removed and the tags updated,
consider this:
Reviewed-by: Nathan Chancellor
Tested-by: Nathan Chancellor
Cheers,
Nathan
Re: [PATCH v1] powerpc: Make setjmp/longjump signature standard
On Fri, Mar 27, 2020 at 3:08 AM Clement Courbet wrote:
>
> Declaring setjmp()/longjmp() as taking longs makes the signature
> non-standard, and makes clang complain. In the past, this has been
> worked around by adding -ffreestanding to the compile flags.
>
> The implementation looks like it only ever propagates the value
> (in longjmp) or sets it to 1 (in setjmp), and we only call longjmp
> with integer parameters.
>
> This allows removing -ffreestanding from the compilation flags.
>
> Context:
> https://lore.kernel.org/patchwork/patch/1214060
> https://lore.kernel.org/patchwork/patch/1216174
>
> Signed-off-by: Clement Courbet
Hi Clement, thanks for the patch! Would you mind sending a V2 that
included a similar fix to arch/powerpc/xmon/Makefile?
For context, this was the original patch:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=aea447141c7e7824b81b49acd1bc78
which was then modified to:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c9029ef9c95765e7b63c4d9aa780674447db1ec0
So on your V2, if you include in the commit message, the line:
Fixes c9029ef9c957 ("powerpc: Avoid clang warnings around setjmp and longjmp")
then that will help our LTS branch maintainers back port it to the
appropriate branches.
>
> ---
> arch/powerpc/include/asm/setjmp.h | 6 --
> arch/powerpc/kexec/Makefile | 3 ---
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/setjmp.h
> b/arch/powerpc/include/asm/setjmp.h
> index e9f81bb3f83b..84bb0d140d59 100644
> --- a/arch/powerpc/include/asm/setjmp.h
> +++ b/arch/powerpc/include/asm/setjmp.h
> @@ -7,7 +7,9 @@
>
> #define JMP_BUF_LEN23
>
> -extern long setjmp(long *) __attribute__((returns_twice));
> -extern void longjmp(long *, long) __attribute__((noreturn));
> +typedef long *jmp_buf;
> +
> +extern int setjmp(jmp_buf env) __attribute__((returns_twice));
> +extern void longjmp(jmp_buf env, int val) __attribute__((noreturn));
>
> #endif /* _ASM_POWERPC_SETJMP_H */
> diff --git a/arch/powerpc/kexec/Makefile b/arch/powerpc/kexec/Makefile
> index 378f6108a414..86380c69f5ce 100644
> --- a/arch/powerpc/kexec/Makefile
> +++ b/arch/powerpc/kexec/Makefile
> @@ -3,9 +3,6 @@
> # Makefile for the linux kernel.
> #
>
> -# Avoid clang warnings around longjmp/setjmp declarations
> -CFLAGS_crash.o += -ffreestanding
> -
> obj-y += core.o crash.o core_$(BITS).o
>
> obj-$(CONFIG_PPC32)+= relocate_32.o
> --
> 2.25.1.696.g5e7596f4ac-goog
>
--
Thanks,
~Nick Desaulniers
