bug.h: Work around GCC PR82365 in BUG()

Message ID 20171219114112.939391-1-arnd@arndb.de
State New
Headers show
Series
  • bug.h: Work around GCC PR82365 in BUG()
Related show

Commit Message

Arnd Bergmann Dec. 19, 2017, 11:39 a.m.
Looking at functions with large stack frames across all architectures
led me discovering that BUG() suffers from the same problem as
fortify_panic(), which I've added a workaround for already. In short,
variables that go out of scope by calling a noreturn function or
__builtin_unreachable() keep using stack space in functions afterwards.

A workaround that was identified is to insert an empty assembler statement
just before calling the function that doesn't return.  I'm adding a macro
"barrier_before_unreachable()" to document this, and insert calls to
that in all instances of BUG() that currently suffer from this problem.

The files that saw the largest change from this had these frame sizes
before, and much less with my patch:

fs/ext4/inode.c:82:1: warning: the frame size of 1672 bytes is larger than 800 bytes [-Wframe-larger-than=]
fs/ext4/namei.c:434:1: warning: the frame size of 904 bytes is larger than 800 bytes [-Wframe-larger-than=]
fs/ext4/super.c:2279:1: warning: the frame size of 1160 bytes is larger than 800 bytes [-Wframe-larger-than=]
fs/ext4/xattr.c:146:1: warning: the frame size of 1168 bytes is larger than 800 bytes [-Wframe-larger-than=]
fs/f2fs/inode.c:152:1: warning: the frame size of 1424 bytes is larger than 800 bytes [-Wframe-larger-than=]
net/netfilter/ipvs/ip_vs_core.c:1195:1: warning: the frame size of 1068 bytes is larger than 800 bytes [-Wframe-larger-than=]
net/netfilter/ipvs/ip_vs_core.c:395:1: warning: the frame size of 1084 bytes is larger than 800 bytes [-Wframe-larger-than=]
net/netfilter/ipvs/ip_vs_ftp.c:298:1: warning: the frame size of 928 bytes is larger than 800 bytes [-Wframe-larger-than=]
net/netfilter/ipvs/ip_vs_ftp.c:418:1: warning: the frame size of 908 bytes is larger than 800 bytes [-Wframe-larger-than=]
net/netfilter/ipvs/ip_vs_lblcr.c:718:1: warning: the frame size of 960 bytes is larger than 800 bytes [-Wframe-larger-than=]
drivers/net/xen-netback/netback.c:1500:1: warning: the frame size of 1088 bytes is larger than 800 bytes [-Wframe-larger-than=]

In case of ARC and CRIS, it turns out that the BUG() implementation
actually does return (or at least the compiler thinks it does), resulting
in lots of warnings about uninitialized variable use and leaving noreturn
functions, such as:

block/cfq-iosched.c: In function 'cfq_async_queue_prio':
block/cfq-iosched.c:3804:1: error: control reaches end of non-void function [-Werror=return-type]
include/linux/dmaengine.h: In function 'dma_maxpq':
include/linux/dmaengine.h:1123:1: error: control reaches end of non-void function [-Werror=return-type]

This makes them call __builtin_trap() instead, which should normally
dump the stack and kill the current process, like some of the other
architectures already do.

I tried adding barrier_before_unreachable() to panic() and fortify_panic()
as well, but that had very little effect, so I'm not submitting that
patch.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
The name barrier_before_unreachable() is a bit suboptimal here,
as it fails to describe the fact that it is needed for both
__builtin_unreachable() and for calling noreturn functions.  Any other
suggestions would be welcome here.
---
 arch/arc/include/asm/bug.h            |  3 ++-
 arch/cris/include/arch-v10/arch/bug.h | 11 +++++++++--
 arch/ia64/include/asm/bug.h           |  6 +++++-
 arch/m68k/include/asm/bug.h           |  3 +++
 arch/sparc/include/asm/bug.h          |  6 +++++-
 include/asm-generic/bug.h             |  1 +
 include/linux/compiler-gcc.h          | 15 ++++++++++++++-
 include/linux/compiler.h              |  5 +++++
 8 files changed, 44 insertions(+), 6 deletions(-)

Comments

Geert Uytterhoeven Dec. 19, 2017, 11:49 a.m. | #1
Hi Arnd,

On Tue, Dec 19, 2017 at 12:39 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> The name barrier_before_unreachable() is a bit suboptimal here,
> as it fails to describe the fact that it is needed for both
> __builtin_unreachable() and for calling noreturn functions.  Any other
> suggestions would be welcome here.

/me joins bikeshedding...

barrier_before_noreturn()?
barrier_before_dead_end()?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Vineet Gupta Dec. 19, 2017, 4:57 p.m. | #2
On 12/19/2017 03:41 AM, Arnd Bergmann wrote:
> Looking at functions with large stack frames across all architectures
> led me discovering that BUG() suffers from the same problem as
> fortify_panic(), which I've added a workaround for already. In short,
> variables that go out of scope by calling a noreturn function or
> __builtin_unreachable() keep using stack space in functions afterwards.
>
> A workaround that was identified is to insert an empty assembler statement
> just before calling the function that doesn't return.  I'm adding a macro
> "barrier_before_unreachable()" to document this, and insert calls to
> that in all instances of BUG() that currently suffer from this problem.
>
> The files that saw the largest change from this had these frame sizes
> before, and much less with my patch:
>
> fs/ext4/inode.c:82:1: warning: the frame size of 1672 bytes is larger than 800 bytes [-Wframe-larger-than=]
> fs/ext4/namei.c:434:1: warning: the frame size of 904 bytes is larger than 800 bytes [-Wframe-larger-than=]
> fs/ext4/super.c:2279:1: warning: the frame size of 1160 bytes is larger than 800 bytes [-Wframe-larger-than=]
> fs/ext4/xattr.c:146:1: warning: the frame size of 1168 bytes is larger than 800 bytes [-Wframe-larger-than=]
> fs/f2fs/inode.c:152:1: warning: the frame size of 1424 bytes is larger than 800 bytes [-Wframe-larger-than=]
> net/netfilter/ipvs/ip_vs_core.c:1195:1: warning: the frame size of 1068 bytes is larger than 800 bytes [-Wframe-larger-than=]
> net/netfilter/ipvs/ip_vs_core.c:395:1: warning: the frame size of 1084 bytes is larger than 800 bytes [-Wframe-larger-than=]
> net/netfilter/ipvs/ip_vs_ftp.c:298:1: warning: the frame size of 928 bytes is larger than 800 bytes [-Wframe-larger-than=]
> net/netfilter/ipvs/ip_vs_ftp.c:418:1: warning: the frame size of 908 bytes is larger than 800 bytes [-Wframe-larger-than=]
> net/netfilter/ipvs/ip_vs_lblcr.c:718:1: warning: the frame size of 960 bytes is larger than 800 bytes [-Wframe-larger-than=]
> drivers/net/xen-netback/netback.c:1500:1: warning: the frame size of 1088 bytes is larger than 800 bytes [-Wframe-larger-than=]
>
> In case of ARC and CRIS, it turns out that the BUG() implementation
> actually does return (or at least the compiler thinks it does), resulting
> in lots of warnings about uninitialized variable use and leaving noreturn
> functions, such as:
>
> block/cfq-iosched.c: In function 'cfq_async_queue_prio':
> block/cfq-iosched.c:3804:1: error: control reaches end of non-void function [-Werror=return-type]
> include/linux/dmaengine.h: In function 'dma_maxpq':
> include/linux/dmaengine.h:1123:1: error: control reaches end of non-void function [-Werror=return-type]
>
> This makes them call __builtin_trap() instead, which should normally
> dump the stack and kill the current process, like some of the other
> architectures already do.
>
> I tried adding barrier_before_unreachable() to panic() and fortify_panic()
> as well, but that had very little effect, so I'm not submitting that
> patch.
>
> Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__gcc.gnu.org_bugzilla_show-5Fbug.cgi-3Fid-3D82365&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=c14YS-cH-kdhTOW89KozFhBtBJgs1zXscZojEZQ0THs&m=3Iu4HWDn1cXkYBpSFh5I80IzDKJi33hs5DbfGM-b3mI&s=sTrcyN5ej_ION8hJvF9eGLUZYwdlwI50vXUp3MK-XWY&e=
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> The name barrier_before_unreachable() is a bit suboptimal here,
> as it fails to describe the fact that it is needed for both
> __builtin_unreachable() and for calling noreturn functions.  Any other
> suggestions would be welcome here.
> ---
>   arch/arc/include/asm/bug.h            |  3 ++-
>   arch/cris/include/arch-v10/arch/bug.h | 11 +++++++++--
>   arch/ia64/include/asm/bug.h           |  6 +++++-
>   arch/m68k/include/asm/bug.h           |  3 +++
>   arch/sparc/include/asm/bug.h          |  6 +++++-
>   include/asm-generic/bug.h             |  1 +
>   include/linux/compiler-gcc.h          | 15 ++++++++++++++-
>   include/linux/compiler.h              |  5 +++++
>   8 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arc/include/asm/bug.h b/arch/arc/include/asm/bug.h
> index ea022d47896c..21ec82466d62 100644
> --- a/arch/arc/include/asm/bug.h
> +++ b/arch/arc/include/asm/bug.h
> @@ -23,7 +23,8 @@ void die(const char *str, struct pt_regs *regs, unsigned long address);
>   
>   #define BUG()	do {								\
>   	pr_warn("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
> -	dump_stack();								\
> +	barrier_before_unreachable();						\
> +	__builtin_trap();							\
>   } while (0)
>   
>   #define HAVE_ARCH_BUG
> diff --git a/arch/cris/include/arch-v10/arch/bug.h b/arch/cris/include/arch-v10/arch/bug.h
> index 905afeacfedf..06da9d49152a 100644
> --- a/arch/cris/include/arch-v10/arch/bug.h
> +++ b/arch/cris/include/arch-v10/arch/bug.h
> @@ -44,18 +44,25 @@ struct bug_frame {
>    * not be used like this with newer versions of gcc.
>    */
>   #define BUG()								\
> +do {									\
>   	__asm__ __volatile__ ("clear.d [" __stringify(BUG_MAGIC) "]\n\t"\
>   			      "movu.w " __stringify(__LINE__) ",$r0\n\t"\
>   			      "jump 0f\n\t"				\
>   			      ".section .rodata\n"			\
>   			      "0:\t.string \"" __FILE__ "\"\n\t"	\
> -			      ".previous")
> +			      ".previous");				\
> +	unreachable();							\
> +} while (0)
>   #endif
>   
>   #else
>   
>   /* This just causes an oops. */
> -#define BUG() (*(int *)0 = 0)
> +#define BUG()								\
> +do {									\
> +	barrier_before_unreachable();					\
> +	__builtin_trap();						\

I suppose BUG() implies "dead end" like semantics - which ARC was lacking before ?

> +} while (0)
>   
>   #endif
>   
> diff --git a/arch/ia64/include/asm/bug.h b/arch/ia64/include/asm/bug.h
> index bd3eeb8d1cfa..66b37a532765 100644
> --- a/arch/ia64/include/asm/bug.h
> +++ b/arch/ia64/include/asm/bug.h
> @@ -4,7 +4,11 @@
>   
>   #ifdef CONFIG_BUG
>   #define ia64_abort()	__builtin_trap()
> -#define BUG() do { printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__); ia64_abort(); } while (0)
> +#define BUG() do {						\
> +	printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__);	\
> +	barrier_before_unreachable();				\
> +	ia64_abort();						\
> +} while (0)
>   
>   /* should this BUG be made generic? */
>   #define HAVE_ARCH_BUG
> diff --git a/arch/m68k/include/asm/bug.h b/arch/m68k/include/asm/bug.h
> index b7e2bf1ba4a6..275dca1435bf 100644
> --- a/arch/m68k/include/asm/bug.h
> +++ b/arch/m68k/include/asm/bug.h
> @@ -8,16 +8,19 @@
>   #ifndef CONFIG_SUN3
>   #define BUG() do { \
>   	pr_crit("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
> +	barrier_before_unreachable(); \
>   	__builtin_trap(); \
>   } while (0)
>   #else
>   #define BUG() do { \
>   	pr_crit("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
> +	barrier_before_unreachable(); \
>   	panic("BUG!"); \
>   } while (0)
>   #endif
>   #else
>   #define BUG() do { \
> +	barrier_before_unreachable(); \
>   	__builtin_trap(); \
>   } while (0)
>   #endif
> diff --git a/arch/sparc/include/asm/bug.h b/arch/sparc/include/asm/bug.h
> index 6f17528356b2..ea53e418f6c0 100644
> --- a/arch/sparc/include/asm/bug.h
> +++ b/arch/sparc/include/asm/bug.h
> @@ -9,10 +9,14 @@
>   void do_BUG(const char *file, int line);
>   #define BUG() do {					\
>   	do_BUG(__FILE__, __LINE__);			\
> +	barrier_before_unreachable();			\
>   	__builtin_trap();				\
>   } while (0)
>   #else
> -#define BUG()		__builtin_trap()
> +#define BUG() do {					\
> +	barrier_before_unreachable();			\
> +	__builtin_trap();				\
> +} while (0)
>   #endif
>   
>   #define HAVE_ARCH_BUG
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 963b755d19b0..a7613e1b0c87 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -52,6 +52,7 @@ struct bug_entry {
>   #ifndef HAVE_ARCH_BUG
>   #define BUG() do { \
>   	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
> +	barrier_before_unreachable(); \
>   	panic("BUG!"); \
>   } while (0)
>   #endif
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 5d595cfdb2c4..66cfdad68f7e 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -205,6 +205,15 @@
>   #endif
>   
>   /*
> + * calling noreturn functions, __builtin_unreachable() and __builtin_trap()
> + * confuse the stack allocation in gcc, leading to overly large stack
> + * frames, see https://urldefense.proofpoint.com/v2/url?u=https-3A__gcc.gnu.org_bugzilla_show-5Fbug.cgi-3Fid-3D82365&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=c14YS-cH-kdhTOW89KozFhBtBJgs1zXscZojEZQ0THs&m=3Iu4HWDn1cXkYBpSFh5I80IzDKJi33hs5DbfGM-b3mI&s=sTrcyN5ej_ION8hJvF9eGLUZYwdlwI50vXUp3MK-XWY&e=
> + *
> + * Adding an empty inline assembly before it works around the problem
> + */
> +#define barrier_before_unreachable() asm volatile("")
> +
> +/*
>    * Mark a position in code as unreachable.  This can be used to
>    * suppress control flow warnings after asm blocks that transfer
>    * control elsewhere.
> @@ -214,7 +223,11 @@
>    * unreleased.  Really, we need to have autoconf for the kernel.
>    */
>   #define unreachable() \
> -	do { annotate_unreachable(); __builtin_unreachable(); } while (0)
> +	do {					\
> +		annotate_unreachable();		\
> +		barrier_before_unreachable();	\
> +		__builtin_unreachable();	\
> +	} while (0)
>   
>   /* Mark a function definition as prohibited from being cloned. */
>   #define __noclone	__attribute__((__noclone__, __optimize__("no-tracer")))
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 52e611ab9a6c..97847f2f86cf 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -86,6 +86,11 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
>   # define barrier_data(ptr) barrier()
>   #endif
>   
> +/* workaround for GCC PR82365 if needed */
> +#ifndef barrier_before_unreachable
> +# define barrier_before_unreachable() do { } while (0)
> +#endif
> +
>   /* Unreachable code */
>   #ifdef CONFIG_STACK_VALIDATION
>   /*
Arnd Bergmann Dec. 19, 2017, 8:13 p.m. | #3
On Tue, Dec 19, 2017 at 5:57 PM, Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
> On 12/19/2017 03:41 AM, Arnd Bergmann wrote:

>> In case of ARC and CRIS, it turns out that the BUG() implementation
>> actually does return (or at least the compiler thinks it does), resulting
>> in lots of warnings about uninitialized variable use and leaving noreturn
>> functions, such as:
>>
>> block/cfq-iosched.c: In function 'cfq_async_queue_prio':
>> block/cfq-iosched.c:3804:1: error: control reaches end of non-void
>> function [-Werror=return-type]
>> include/linux/dmaengine.h: In function 'dma_maxpq':
>> include/linux/dmaengine.h:1123:1: error: control reaches end of non-void
>> function [-Werror=return-type]

>> diff --git a/arch/arc/include/asm/bug.h b/arch/arc/include/asm/bug.h
>> index ea022d47896c..21ec82466d62 100644
>> --- a/arch/arc/include/asm/bug.h
>> +++ b/arch/arc/include/asm/bug.h
>> @@ -23,7 +23,8 @@ void die(const char *str, struct pt_regs *regs, unsigned
>> long address);
>>     #define BUG()       do {
>> \
>>         pr_warn("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__,
>> __func__); \
>> -       dump_stack();
>> \
>> +       barrier_before_unreachable();
>> \
>> +       __builtin_trap();
>> \
>>   } while (0)
>>     #define HAVE_ARCH_BUG
>
>
> I suppose BUG() implies "dead end" like semantics - which ARC was lacking
> before ?

Correct. Using __builtin_trap() here avoids the 'control reaches end of non-void
function' warnings, but then makes us run into the stack size problem that
I work around with the barrier_before_unreachable().

It would be good if you could give this a quick test to see if you get sensible
output from the __builtin_trap();

         Arnd
Vineet Gupta Dec. 19, 2017, 10:38 p.m. | #4
On 12/19/2017 12:13 PM, Arnd Bergmann wrote:
>
>> I suppose BUG() implies "dead end" like semantics - which ARC was lacking
>> before ?
> Correct. Using __builtin_trap() here avoids the 'control reaches end of non-void
> function' warnings, but then makes us run into the stack size problem that
> I work around with the barrier_before_unreachable().
>
> It would be good if you could give this a quick test to see if you get sensible
> output from the __builtin_trap();

It does, added a BUG() arbit, hits an abort()

...
ISA Extn    : atomic ll64 unalign (not used)
         : mpy[opt 9] div_rem norm barrel-shift swap minmax swape
BPU        : partial match, cache:2048, Predict Table:16384
BUG: failure at ../arch/arc/mm/tlb.c:827/arc_mmu_init()!


Tested-by: Vineet Gupta <vgupta@synopsys.com>

FWIW newer ARC gcc actually implements the builtin so we get a trap 5 instruction 
now, vs., abort() calls before.

BTW I missed reading the hunk of your changelog where this addresses the long 
standing mystery with ARC builds and numerous -Wreturn-type warnings. I always 
wondered why they were not fixed upstream already, being too lazy to investigate 
myself, and turns out this was due to this BUG() thingy. phew !

-Vineet
Arnd Bergmann Dec. 20, 2017, 9:01 a.m. | #5
On Tue, Dec 19, 2017 at 11:38 PM, Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
> On 12/19/2017 12:13 PM, Arnd Bergmann wrote:
>>
>>
>>> I suppose BUG() implies "dead end" like semantics - which ARC was lacking
>>> before ?
>>
>> Correct. Using __builtin_trap() here avoids the 'control reaches end of
>> non-void
>> function' warnings, but then makes us run into the stack size problem that
>> I work around with the barrier_before_unreachable().
>>
>> It would be good if you could give this a quick test to see if you get
>> sensible
>> output from the __builtin_trap();
>
>
> It does, added a BUG() arbit, hits an abort()
>
> ...
> ISA Extn    : atomic ll64 unalign (not used)
>         : mpy[opt 9] div_rem norm barrel-shift swap minmax swape
> BPU        : partial match, cache:2048, Predict Table:16384
> BUG: failure at ../arch/arc/mm/tlb.c:827/arc_mmu_init()!
>
>
> Tested-by: Vineet Gupta <vgupta@synopsys.com>

I meant whether it prints the right registers and stack trace, but I
assume you tested that and just did not list it above.

> FWIW newer ARC gcc actually implements the builtin so we get a trap 5
> instruction now, vs., abort() calls before.
>
> BTW I missed reading the hunk of your changelog where this addresses the
> long standing mystery with ARC builds and numerous -Wreturn-type warnings. I
> always wondered why they were not fixed upstream already, being too lazy to
> investigate myself, and turns out this was due to this BUG() thingy. phew !

Hmm, so with the new definition of abort(),

+__weak void abort(void)
+{
+   BUG();
+
+   /* if that doesn't kill us, halt */
+   panic("Oops failed to kill thread");
+}

won't that run into an endless recursion? Or do you then override abort()
for ARC?

         Arnd
Vineet Gupta Dec. 20, 2017, 6:52 p.m. | #6
On 12/20/2017 01:01 AM, Arnd Bergmann wrote:
> On Tue, Dec 19, 2017 at 11:38 PM, Vineet Gupta
> <Vineet.Gupta1@synopsys.com> wrote:
>> On 12/19/2017 12:13 PM, Arnd Bergmann wrote:
>>>
>>>
>>>> I suppose BUG() implies "dead end" like semantics - which ARC was lacking
>>>> before ?
>>>
>>> Correct. Using __builtin_trap() here avoids the 'control reaches end of
>>> non-void
>>> function' warnings, but then makes us run into the stack size problem that
>>> I work around with the barrier_before_unreachable().
>>>
>>> It would be good if you could give this a quick test to see if you get
>>> sensible
>>> output from the __builtin_trap();
>>
>>
>> It does, added a BUG() arbit, hits an abort()
>>
>> ...
>> ISA Extn    : atomic ll64 unalign (not used)
>>          : mpy[opt 9] div_rem norm barrel-shift swap minmax swape
>> BPU        : partial match, cache:2048, Predict Table:16384
>> BUG: failure at ../arch/arc/mm/tlb.c:827/arc_mmu_init()!
>>
>>
>> Tested-by: Vineet Gupta <vgupta@synopsys.com>
> 
> I meant whether it prints the right registers and stack trace, but I
> assume you tested that and just did not list it above.

Sorry, I didn't realize we are missing the stack trace now which you removed from 
the patch - why ? Did u intend to reduce inline generated code for the stack dump 
calls - which sounds like a great idea. But it would only work for the synchronous 
abort() but not when builtin translates to actual trap inducing instruction.

> Hmm, so with the new definition of abort(),
> 
> +__weak void abort(void)
> +{
> +   BUG();
> +
> +   /* if that doesn't kill us, halt */
> +   panic("Oops failed to kill thread");
> +}
> 
> won't that run into an endless recursion? Or do you then override abort()
> for ARC?

Indeed so. I didn't run into this in my testing as my for-curr has an ARC specific 
version (predating Sudip's generic version- because of build failures in our 
internal regression jobs etc). That version only calls panic.

abort() is only likely to be called due to __builtin_trap() for arches where gcc 
doesn't have a target specific defn of it. And thus adding the call from BUG() 
will cause the recursion as you found out with Sudip's generic version and thus 
needs a fixup.


Thx,
-Vineet
Arnd Bergmann Dec. 20, 2017, 8:12 p.m. | #7
On Wed, Dec 20, 2017 at 7:52 PM, Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
> On 12/20/2017 01:01 AM, Arnd Bergmann wrote:
>>
>> On Tue, Dec 19, 2017 at 11:38 PM, Vineet Gupta
>> <Vineet.Gupta1@synopsys.com> wrote:
>>>
>>> On 12/19/2017 12:13 PM, Arnd Bergmann wrote:
>>>>
>>>>
>>>>
>>>>> I suppose BUG() implies "dead end" like semantics - which ARC was
>>>>> lacking
>>>>> before ?
>>>>
>>>>
>>>> Correct. Using __builtin_trap() here avoids the 'control reaches end of
>>>> non-void
>>>> function' warnings, but then makes us run into the stack size problem
>>>> that
>>>> I work around with the barrier_before_unreachable().
>>>>
>>>> It would be good if you could give this a quick test to see if you get
>>>> sensible
>>>> output from the __builtin_trap();
>>>
>>>
>>>
>>> It does, added a BUG() arbit, hits an abort()
>>>
>>> ...
>>> ISA Extn    : atomic ll64 unalign (not used)
>>>          : mpy[opt 9] div_rem norm barrel-shift swap minmax swape
>>> BPU        : partial match, cache:2048, Predict Table:16384
>>> BUG: failure at ../arch/arc/mm/tlb.c:827/arc_mmu_init()!
>>>
>>>
>>> Tested-by: Vineet Gupta <vgupta@synopsys.com>
>>
>>
>> I meant whether it prints the right registers and stack trace, but I
>> assume you tested that and just did not list it above.
>
>
> Sorry, I didn't realize we are missing the stack trace now which you removed
> from the patch - why ? Did u intend to reduce inline generated code for the
> stack dump calls - which sounds like a great idea. But it would only work
> for the synchronous abort() but not when builtin translates to actual trap
> inducing instruction.

I assumed that the trap instruction would trigger the register and
stack dump, as it does on all other architectures. The most common
way this is handled is to have one instruction that is known to trap,
and use that to trigger a BUG(), and have __builtin_trap() issue
that instruction as well.

You might also want to implement CONFIG_DEBUG_BUGVERBOSE
support to attach further data to it.

>> Hmm, so with the new definition of abort(),
>>
>> +__weak void abort(void)
>> +{
>> +   BUG();
>> +
>> +   /* if that doesn't kill us, halt */
>> +   panic("Oops failed to kill thread");
>> +}
>>
>> won't that run into an endless recursion? Or do you then override abort()
>> for ARC?
>
>
> Indeed so. I didn't run into this in my testing as my for-curr has an ARC
> specific version (predating Sudip's generic version- because of build
> failures in our internal regression jobs etc). That version only calls
> panic.
>
> abort() is only likely to be called due to __builtin_trap() for arches where
> gcc doesn't have a target specific defn of it. And thus adding the call from
> BUG() will cause the recursion as you found out with Sudip's generic version
> and thus needs a fixup.

How about overriding abort() with the same instruction that
__builtin_trap() inserts on newer compilers then? That should
make the behavior consistent.

        Arnd
Vineet Gupta Dec. 20, 2017, 8:29 p.m. | #8
On 12/20/2017 12:12 PM, Arnd Bergmann wrote:
>> Sorry, I didn't realize we are missing the stack trace now which you removed
>> from the patch - why ? Did u intend to reduce inline generated code for the
>> stack dump calls - which sounds like a great idea. But it would only work
>> for the synchronous abort() but not when builtin translates to actual trap
>> inducing instruction.
> 
> I assumed that the trap instruction would trigger the register and
> stack dump, as it does on all other architectures.

Only if __builtin_trap() translated to that instruction. Otherwise we need to do 
this inside abort()

> The most common
> way this is handled is to have one instruction that is known to trap,
> and use that to trigger a BUG(), and have __builtin_trap() issue
> that instruction as well.

Good point. So we'll need ARC specific abort anyways.


> You might also want to implement CONFIG_DEBUG_BUGVERBOSE
> support to attach further data to it.

OK I'll take a look !

> How about overriding abort() with the same instruction that
> __builtin_trap() inserts on newer compilers then? That should
> make the behavior consistent.

Yeah this is a great point ! Will do

thx,
-Vineet
Andrew Morton Feb. 8, 2018, 12:01 a.m. | #9
On Wed, 20 Dec 2017 12:29:02 -0800 Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:

> On 12/20/2017 12:12 PM, Arnd Bergmann wrote:
> >> Sorry, I didn't realize we are missing the stack trace now which you removed
> >> from the patch - why ? Did u intend to reduce inline generated code for the
> >> stack dump calls - which sounds like a great idea. But it would only work
> >> for the synchronous abort() but not when builtin translates to actual trap
> >> inducing instruction.
> > 
> > I assumed that the trap instruction would trigger the register and
> > stack dump, as it does on all other architectures.
> 
> Only if __builtin_trap() translated to that instruction. Otherwise we need to do 
> this inside abort()
> 
> > The most common
> > way this is handled is to have one instruction that is known to trap,
> > and use that to trigger a BUG(), and have __builtin_trap() issue
> > that instruction as well.
> 
> Good point. So we'll need ARC specific abort anyways.
> 
> 
> > You might also want to implement CONFIG_DEBUG_BUGVERBOSE
> > support to attach further data to it.
> 
> OK I'll take a look !
> 
> > How about overriding abort() with the same instruction that
> > __builtin_trap() inserts on newer compilers then? That should
> > make the behavior consistent.
> 
> Yeah this is a great point ! Will do

Didn't do ;)

Is Arnd's patch good to merge or do we need a fixup?


From: Arnd Bergmann <arnd@arndb.de>
Subject: bug.h: work around GCC PR82365 in BUG()

Looking at functions with large stack frames across all architectures led
me discovering that BUG() suffers from the same problem as
fortify_panic(), which I've added a workaround for already.  In short,
variables that go out of scope by calling a noreturn function or
__builtin_unreachable() keep using stack space in functions afterwards.

A workaround that was identified is to insert an empty assembler statement
just before calling the function that doesn't return.  I'm adding a macro
"barrier_before_unreachable()" to document this, and insert calls to that
in all instances of BUG() that currently suffer from this problem.

The files that saw the largest change from this had these frame sizes
before, and much less with my patch:

fs/ext4/inode.c:82:1: warning: the frame size of 1672 bytes is larger than 800 bytes [-Wframe-larger-than=]
fs/ext4/namei.c:434:1: warning: the frame size of 904 bytes is larger than 800 bytes [-Wframe-larger-than=]
fs/ext4/super.c:2279:1: warning: the frame size of 1160 bytes is larger than 800 bytes [-Wframe-larger-than=]
fs/ext4/xattr.c:146:1: warning: the frame size of 1168 bytes is larger than 800 bytes [-Wframe-larger-than=]
fs/f2fs/inode.c:152:1: warning: the frame size of 1424 bytes is larger than 800 bytes [-Wframe-larger-than=]
net/netfilter/ipvs/ip_vs_core.c:1195:1: warning: the frame size of 1068 bytes is larger than 800 bytes [-Wframe-larger-than=]
net/netfilter/ipvs/ip_vs_core.c:395:1: warning: the frame size of 1084 bytes is larger than 800 bytes [-Wframe-larger-than=]
net/netfilter/ipvs/ip_vs_ftp.c:298:1: warning: the frame size of 928 bytes is larger than 800 bytes [-Wframe-larger-than=]
net/netfilter/ipvs/ip_vs_ftp.c:418:1: warning: the frame size of 908 bytes is larger than 800 bytes [-Wframe-larger-than=]
net/netfilter/ipvs/ip_vs_lblcr.c:718:1: warning: the frame size of 960 bytes is larger than 800 bytes [-Wframe-larger-than=]
drivers/net/xen-netback/netback.c:1500:1: warning: the frame size of 1088 bytes is larger than 800 bytes [-Wframe-larger-than=]

In case of ARC and CRIS, it turns out that the BUG() implementation
actually does return (or at least the compiler thinks it does), resulting
in lots of warnings about uninitialized variable use and leaving noreturn
functions, such as:

block/cfq-iosched.c: In function 'cfq_async_queue_prio':
block/cfq-iosched.c:3804:1: error: control reaches end of non-void function [-Werror=return-type]
include/linux/dmaengine.h: In function 'dma_maxpq':
include/linux/dmaengine.h:1123:1: error: control reaches end of non-void function [-Werror=return-type]

This makes them call __builtin_trap() instead, which should normally dump
the stack and kill the current process, like some of the other
architectures already do.

I tried adding barrier_before_unreachable() to panic() and fortify_panic()
as well, but that had very little effect, so I'm not submitting that
patch.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
Link: http://lkml.kernel.org/r/20171219114112.939391-1-arnd@arndb.de
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Tested-by: Vineet Gupta <vgupta@synopsys.com>
Cc: Mikael Starvik <starvik@axis.com>
Cc: Jesper Nilsson <jesper.nilsson@axis.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Christopher Li <sparse@chrisli.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/arc/include/asm/bug.h            |    3 ++-
 arch/cris/include/arch-v10/arch/bug.h |   11 +++++++++--
 arch/ia64/include/asm/bug.h           |    6 +++++-
 arch/m68k/include/asm/bug.h           |    3 +++
 arch/sparc/include/asm/bug.h          |    6 +++++-
 include/asm-generic/bug.h             |    1 +
 include/linux/compiler-gcc.h          |   15 ++++++++++++++-
 include/linux/compiler.h              |    5 +++++
 8 files changed, 44 insertions(+), 6 deletions(-)

diff -puN arch/arc/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug arch/arc/include/asm/bug.h
--- a/arch/arc/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug
+++ a/arch/arc/include/asm/bug.h
@@ -23,7 +23,8 @@ void die(const char *str, struct pt_regs
 
 #define BUG()	do {								\
 	pr_warn("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
-	dump_stack();								\
+	barrier_before_unreachable();						\
+	__builtin_trap();							\
 } while (0)
 
 #define HAVE_ARCH_BUG
diff -puN arch/cris/include/arch-v10/arch/bug.h~bugh-work-around-gcc-pr82365-in-bug arch/cris/include/arch-v10/arch/bug.h
--- a/arch/cris/include/arch-v10/arch/bug.h~bugh-work-around-gcc-pr82365-in-bug
+++ a/arch/cris/include/arch-v10/arch/bug.h
@@ -44,18 +44,25 @@ struct bug_frame {
  * not be used like this with newer versions of gcc.
  */
 #define BUG()								\
+do {									\
 	__asm__ __volatile__ ("clear.d [" __stringify(BUG_MAGIC) "]\n\t"\
 			      "movu.w " __stringify(__LINE__) ",$r0\n\t"\
 			      "jump 0f\n\t"				\
 			      ".section .rodata\n"			\
 			      "0:\t.string \"" __FILE__ "\"\n\t"	\
-			      ".previous")
+			      ".previous");				\
+	unreachable();							\
+} while (0)
 #endif
 
 #else
 
 /* This just causes an oops. */
-#define BUG() (*(int *)0 = 0)
+#define BUG()								\
+do {									\
+	barrier_before_unreachable();					\
+	__builtin_trap();						\
+} while (0)
 
 #endif
 
diff -puN arch/ia64/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug arch/ia64/include/asm/bug.h
--- a/arch/ia64/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug
+++ a/arch/ia64/include/asm/bug.h
@@ -4,7 +4,11 @@
 
 #ifdef CONFIG_BUG
 #define ia64_abort()	__builtin_trap()
-#define BUG() do { printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__); ia64_abort(); } while (0)
+#define BUG() do {						\
+	printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__);	\
+	barrier_before_unreachable();				\
+	ia64_abort();						\
+} while (0)
 
 /* should this BUG be made generic? */
 #define HAVE_ARCH_BUG
diff -puN arch/m68k/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug arch/m68k/include/asm/bug.h
--- a/arch/m68k/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug
+++ a/arch/m68k/include/asm/bug.h
@@ -8,16 +8,19 @@
 #ifndef CONFIG_SUN3
 #define BUG() do { \
 	pr_crit("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
+	barrier_before_unreachable(); \
 	__builtin_trap(); \
 } while (0)
 #else
 #define BUG() do { \
 	pr_crit("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
+	barrier_before_unreachable(); \
 	panic("BUG!"); \
 } while (0)
 #endif
 #else
 #define BUG() do { \
+	barrier_before_unreachable(); \
 	__builtin_trap(); \
 } while (0)
 #endif
diff -puN arch/sparc/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug arch/sparc/include/asm/bug.h
--- a/arch/sparc/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug
+++ a/arch/sparc/include/asm/bug.h
@@ -9,10 +9,14 @@
 void do_BUG(const char *file, int line);
 #define BUG() do {					\
 	do_BUG(__FILE__, __LINE__);			\
+	barrier_before_unreachable();			\
 	__builtin_trap();				\
 } while (0)
 #else
-#define BUG()		__builtin_trap()
+#define BUG() do {					\
+	barrier_before_unreachable();			\
+	__builtin_trap();				\
+} while (0)
 #endif
 
 #define HAVE_ARCH_BUG
diff -puN include/asm-generic/bug.h~bugh-work-around-gcc-pr82365-in-bug include/asm-generic/bug.h
--- a/include/asm-generic/bug.h~bugh-work-around-gcc-pr82365-in-bug
+++ a/include/asm-generic/bug.h
@@ -52,6 +52,7 @@ struct bug_entry {
 #ifndef HAVE_ARCH_BUG
 #define BUG() do { \
 	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
+	barrier_before_unreachable(); \
 	panic("BUG!"); \
 } while (0)
 #endif
diff -puN include/linux/compiler-gcc.h~bugh-work-around-gcc-pr82365-in-bug include/linux/compiler-gcc.h
--- a/include/linux/compiler-gcc.h~bugh-work-around-gcc-pr82365-in-bug
+++ a/include/linux/compiler-gcc.h
@@ -205,6 +205,15 @@
 #endif
 
 /*
+ * calling noreturn functions, __builtin_unreachable() and __builtin_trap()
+ * confuse the stack allocation in gcc, leading to overly large stack
+ * frames, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
+ *
+ * Adding an empty inline assembly before it works around the problem
+ */
+#define barrier_before_unreachable() asm volatile("")
+
+/*
  * Mark a position in code as unreachable.  This can be used to
  * suppress control flow warnings after asm blocks that transfer
  * control elsewhere.
@@ -214,7 +223,11 @@
  * unreleased.  Really, we need to have autoconf for the kernel.
  */
 #define unreachable() \
-	do { annotate_unreachable(); __builtin_unreachable(); } while (0)
+	do {					\
+		annotate_unreachable();		\
+		barrier_before_unreachable();	\
+		__builtin_unreachable();	\
+	} while (0)
 
 /* Mark a function definition as prohibited from being cloned. */
 #define __noclone	__attribute__((__noclone__, __optimize__("no-tracer")))
diff -puN include/linux/compiler.h~bugh-work-around-gcc-pr82365-in-bug include/linux/compiler.h
--- a/include/linux/compiler.h~bugh-work-around-gcc-pr82365-in-bug
+++ a/include/linux/compiler.h
@@ -86,6 +86,11 @@ void ftrace_likely_update(struct ftrace_
 # define barrier_data(ptr) barrier()
 #endif
 
+/* workaround for GCC PR82365 if needed */
+#ifndef barrier_before_unreachable
+# define barrier_before_unreachable() do { } while (0)
+#endif
+
 /* Unreachable code */
 #ifdef CONFIG_STACK_VALIDATION
 /*
Vineet Gupta Feb. 8, 2018, 12:51 a.m. | #10
On 02/07/2018 04:01 PM, Andrew Morton wrote:
> On Wed, 20 Dec 2017 12:29:02 -0800 Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
> 
>> On 12/20/2017 12:12 PM, Arnd Bergmann wrote:
>>>> Sorry, I didn't realize we are missing the stack trace now which you removed
>>>> from the patch - why ? Did u intend to reduce inline generated code for the
>>>> stack dump calls - which sounds like a great idea. But it would only work
>>>> for the synchronous abort() but not when builtin translates to actual trap
>>>> inducing instruction.
>>>
>>> I assumed that the trap instruction would trigger the register and
>>> stack dump, as it does on all other architectures.
>>
>> Only if __builtin_trap() translated to that instruction. Otherwise we need to do
>> this inside abort()
>>
>>> The most common
>>> way this is handled is to have one instruction that is known to trap,
>>> and use that to trigger a BUG(), and have __builtin_trap() issue
>>> that instruction as well.
>>
>> Good point. So we'll need ARC specific abort anyways.
>>
>>
>>> You might also want to implement CONFIG_DEBUG_BUGVERBOSE
>>> support to attach further data to it.
>>
>> OK I'll take a look !
>>
>>> How about overriding abort() with the same instruction that
>>> __builtin_trap() inserts on newer compilers then? That should
>>> make the behavior consistent.
>>
>> Yeah this is a great point ! Will do
> 
> Didn't do ;)

Actually I did :-)

The discussion above digressed from original intent of the patch and instead 
veered of into need for handling __builtin_trap() for ARC and a fallback abort() 
with same semantics etc which I'd agreed to do above. They are upstream

2017-12-08 af1be2e21203 ARC: handle gcc generated __builtin_trap for older compiler
2017-12-20 f5a16b93e629 ARC: handle gcc generated __builtin_trap()

However we missed this large stack issue and the -Werror=return-type warning for ARC.

Let me check the patch again and get back here !

> 
> Is Arnd's patch good to merge or do we need a fixup?
> 
> 
> From: Arnd Bergmann <arnd@arndb.de>
> Subject: bug.h: work around GCC PR82365 in BUG()
> 
> Looking at functions with large stack frames across all architectures led
> me discovering that BUG() suffers from the same problem as
> fortify_panic(), which I've added a workaround for already.  In short,
> variables that go out of scope by calling a noreturn function or
> __builtin_unreachable() keep using stack space in functions afterwards.
> 
> A workaround that was identified is to insert an empty assembler statement
> just before calling the function that doesn't return.  I'm adding a macro
> "barrier_before_unreachable()" to document this, and insert calls to that
> in all instances of BUG() that currently suffer from this problem.
> 
> The files that saw the largest change from this had these frame sizes
> before, and much less with my patch:
> 
> fs/ext4/inode.c:82:1: warning: the frame size of 1672 bytes is larger than 800 bytes [-Wframe-larger-than=]
> fs/ext4/namei.c:434:1: warning: the frame size of 904 bytes is larger than 800 bytes [-Wframe-larger-than=]

[...]

> In case of ARC and CRIS, it turns out that the BUG() implementation
> actually does return (or at least the compiler thinks it does), resulting
> in lots of warnings about uninitialized variable use and leaving noreturn
> functions, such as:
> 
> block/cfq-iosched.c: In function 'cfq_async_queue_prio':
> block/cfq-iosched.c:3804:1: error: control reaches end of non-void function [-Werror=return-type]
> include/linux/dmaengine.h: In function 'dma_maxpq':
> include/linux/dmaengine.h:1123:1: error: control reaches end of non-void function [-Werror=return-type]
> 
> This makes them call __builtin_trap() instead, which should normally dump
> the stack and kill the current process, like some of the other
> architectures already do.
> 
> I tried adding barrier_before_unreachable() to panic() and fortify_panic()
> as well, but that had very little effect, so I'm not submitting that
> patch.

[...]

> 
> diff -puN arch/arc/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug arch/arc/include/asm/bug.h
> --- a/arch/arc/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug
> +++ a/arch/arc/include/asm/bug.h
> @@ -23,7 +23,8 @@ void die(const char *str, struct pt_regs
>   
>   #define BUG()	do {								\
>   	pr_warn("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
> -	dump_stack();								\
> +	barrier_before_unreachable();						\
> +	__builtin_trap();							\
>   } while (0)

>
Vineet Gupta Feb. 8, 2018, 1:20 a.m. | #11
On 02/07/2018 04:01 PM, Andrew Morton wrote:
> On Wed, 20 Dec 2017 12:29:02 -0800 Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
> 
>> On 12/20/2017 12:12 PM, Arnd Bergmann wrote:
>>>> Sorry, I didn't realize we are missing the stack trace now which you removed
>>>> from the patch - why ? Did u intend to reduce inline generated code for the
>>>> stack dump calls - which sounds like a great idea. But it would only work
>>>> for the synchronous abort() but not when builtin translates to actual trap
>>>> inducing instruction.
>>>
>>> I assumed that the trap instruction would trigger the register and
>>> stack dump, as it does on all other architectures.
>>
>> Only if __builtin_trap() translated to that instruction. Otherwise we need to do
>> this inside abort()
>>
>>> The most common
>>> way this is handled is to have one instruction that is known to trap,
>>> and use that to trigger a BUG(), and have __builtin_trap() issue
>>> that instruction as well.
>>
>> Good point. So we'll need ARC specific abort anyways.
>>
>>
>>> You might also want to implement CONFIG_DEBUG_BUGVERBOSE
>>> support to attach further data to it.
>>
>> OK I'll take a look !
>>
>>> How about overriding abort() with the same instruction that
>>> __builtin_trap() inserts on newer compilers then? That should
>>> make the behavior consistent.
>>
>> Yeah this is a great point ! Will do
> 
> Didn't do ;)
> 
> Is Arnd's patch good to merge or do we need a fixup?
> 
> 
> From: Arnd Bergmann <arnd@arndb.de>
> Subject: bug.h: work around GCC PR82365 in BUG()
> 
> Looking at functions with large stack frames across all architectures led
> me discovering that BUG() suffers from the same problem as
> fortify_panic(), which I've added a workaround for already.  In short,
> variables that go out of scope by calling a noreturn function or
> __builtin_unreachable() keep using stack space in functions afterwards.
> 
> A workaround that was identified is to insert an empty assembler statement
> just before calling the function that doesn't return.  I'm adding a macro
> "barrier_before_unreachable()" to document this, and insert calls to that
> in all instances of BUG() that currently suffer from this problem.
> 
> The files that saw the largest change from this had these frame sizes
> before, and much less with my patch:
> 
> fs/ext4/inode.c:82:1: warning: the frame size of 1672 bytes is larger than 800 bytes [-Wframe-larger-than=]
> fs/ext4/namei.c:434:1: warning: the frame size of 904 bytes is larger than 800 bytes [-Wframe-larger-than=]
> fs/ext4/super.c:2279:1: warning: the frame size of 1160 bytes is larger than 800 bytes [-Wframe-larger-than=]
> fs/ext4/xattr.c:146:1: warning: the frame size of 1168 bytes is larger than 800 bytes [-Wframe-larger-than=]
> fs/f2fs/inode.c:152:1: warning: the frame size of 1424 bytes is larger than 800 bytes [-Wframe-larger-than=]
> net/netfilter/ipvs/ip_vs_core.c:1195:1: warning: the frame size of 1068 bytes is larger than 800 bytes [-Wframe-larger-than=]
> net/netfilter/ipvs/ip_vs_core.c:395:1: warning: the frame size of 1084 bytes is larger than 800 bytes [-Wframe-larger-than=]
> net/netfilter/ipvs/ip_vs_ftp.c:298:1: warning: the frame size of 928 bytes is larger than 800 bytes [-Wframe-larger-than=]
> net/netfilter/ipvs/ip_vs_ftp.c:418:1: warning: the frame size of 908 bytes is larger than 800 bytes [-Wframe-larger-than=]
> net/netfilter/ipvs/ip_vs_lblcr.c:718:1: warning: the frame size of 960 bytes is larger than 800 bytes [-Wframe-larger-than=]
> drivers/net/xen-netback/netback.c:1500:1: warning: the frame size of 1088 bytes is larger than 800 bytes [-Wframe-larger-than=]
> 
> In case of ARC and CRIS, it turns out that the BUG() implementation
> actually does return (or at least the compiler thinks it does), resulting
> in lots of warnings about uninitialized variable use and leaving noreturn
> functions, such as:
> 
> block/cfq-iosched.c: In function 'cfq_async_queue_prio':
> block/cfq-iosched.c:3804:1: error: control reaches end of non-void function [-Werror=return-type]
> include/linux/dmaengine.h: In function 'dma_maxpq':
> include/linux/dmaengine.h:1123:1: error: control reaches end of non-void function [-Werror=return-type]
> 
> This makes them call __builtin_trap() instead, which should normally dump
> the stack and kill the current process, like some of the other
> architectures already do.
> 
> I tried adding barrier_before_unreachable() to panic() and fortify_panic()
> as well, but that had very little effect, so I'm not submitting that
> patch.
> 
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
> Link: http://lkml.kernel.org/r/20171219114112.939391-1-arnd@arndb.de
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Tested-by: Vineet Gupta <vgupta@synopsys.com>
> Cc: Mikael Starvik <starvik@axis.com>
> Cc: Jesper Nilsson <jesper.nilsson@axis.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Christopher Li <sparse@chrisli.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>   arch/arc/include/asm/bug.h            |    3 ++-
>   arch/cris/include/arch-v10/arch/bug.h |   11 +++++++++--
>   arch/ia64/include/asm/bug.h           |    6 +++++-
>   arch/m68k/include/asm/bug.h           |    3 +++
>   arch/sparc/include/asm/bug.h          |    6 +++++-
>   include/asm-generic/bug.h             |    1 +
>   include/linux/compiler-gcc.h          |   15 ++++++++++++++-
>   include/linux/compiler.h              |    5 +++++
>   8 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff -puN arch/arc/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug arch/arc/include/asm/bug.h
> --- a/arch/arc/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug
> +++ a/arch/arc/include/asm/bug.h
> @@ -23,7 +23,8 @@ void die(const char *str, struct pt_regs
>   
>   #define BUG()	do {								\
>   	pr_warn("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
> -	dump_stack();								\
> +	barrier_before_unreachable();						\
> +	__builtin_trap();							\
>   } while (0)
>   

For ARC, it is double win.

1. Fixes 3 -Wreturn-type warnings

| ../net/core/ethtool.c:311:1: warning: control reaches end of non-void function 
[-Wreturn-type]
| ../kernel/sched/core.c:3246:1: warning: control reaches end of non-void function 
[-Wreturn-type]
| ../include/linux/sunrpc/svc_xprt.h:180:1: warning: control reaches end of 
non-void function [-Wreturn-type]

2. bloat-o-meter reports code size improvements as gcc elides the generated code 
for stack return.


Acked-by: Vineet Gupta <vgupta@synopsys.com>   # for arch/arc
Tested-by: Vineet Gupta <vgupta@synopsys.com>   # for arch/arc
Vineet Gupta Feb. 21, 2018, 9:23 p.m. | #12
On 02/07/2018 05:20 PM, Vineet Gupta wrote:
>
>> Didn't do ;)
>>
>> Is Arnd's patch good to merge or do we need a fixup?
>>
>>
>> From: Arnd Bergmann <arnd@arndb.de>
>> Subject: bug.h: work around GCC PR82365 in BUG()
>>
>> Looking at functions with large stack frames across all architectures led
>> me discovering that BUG() suffers from the same problem as
>> fortify_panic(), which I've added a workaround for already.  In short,
>> variables that go out of scope by calling a noreturn function or
>> __builtin_unreachable() keep using stack space in functions afterwards.
>>
>> A workaround that was identified is to insert an empty assembler statement
>> just before calling the function that doesn't return.  I'm adding a macro
>> "barrier_before_unreachable()" to document this, and insert calls to that
>> in all instances of BUG() that currently suffer from this problem.
>>
..
..

>> ---
>>
>>   arch/arc/include/asm/bug.h            |    3 ++-
>>   arch/cris/include/arch-v10/arch/bug.h |   11 +++++++++--
>>   arch/ia64/include/asm/bug.h           |    6 +++++-
>>   arch/m68k/include/asm/bug.h           |    3 +++
>>   arch/sparc/include/asm/bug.h          |    6 +++++-
>>   include/asm-generic/bug.h             |    1 +
>>   include/linux/compiler-gcc.h          |   15 ++++++++++++++-
>>   include/linux/compiler.h              |    5 +++++
>>   8 files changed, 44 insertions(+), 6 deletions(-)
>>
>> diff -puN arch/arc/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug 
>> arch/arc/include/asm/bug.h
>> --- a/arch/arc/include/asm/bug.h~bugh-work-around-gcc-pr82365-in-bug
>> +++ a/arch/arc/include/asm/bug.h
>> @@ -23,7 +23,8 @@ void die(const char *str, struct pt_regs
>>     #define BUG()    do {                                \
>>       pr_warn("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
>> -    dump_stack();                                \
>> +    barrier_before_unreachable();                        \
>> +    __builtin_trap();                            \
>>   } while (0)
>
> For ARC, it is double win.
>
> 1. Fixes 3 -Wreturn-type warnings
>
> | ../net/core/ethtool.c:311:1: warning: control reaches end of non-void function 
> [-Wreturn-type]
> | ../kernel/sched/core.c:3246:1: warning: control reaches end of non-void 
> function [-Wreturn-type]
> | ../include/linux/sunrpc/svc_xprt.h:180:1: warning: control reaches end of 
> non-void function [-Wreturn-type]
>
> 2. bloat-o-meter reports code size improvements as gcc elides the generated code 
> for stack return.
>
>
> Acked-by: Vineet Gupta <vgupta@synopsys.com>   # for arch/arc
> Tested-by: Vineet Gupta <vgupta@synopsys.com>   # for arch/arc

Ping ?
James Hogan April 10, 2018, 10:48 p.m. | #13
Hi Arnd,

On Tue, Dec 19, 2017 at 12:39:33PM +0100, Arnd Bergmann wrote:
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 5d595cfdb2c4..66cfdad68f7e 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -205,6 +205,15 @@
>  #endif
>  
>  /*
> + * calling noreturn functions, __builtin_unreachable() and __builtin_trap()
> + * confuse the stack allocation in gcc, leading to overly large stack
> + * frames, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
> + *
> + * Adding an empty inline assembly before it works around the problem
> + */
> +#define barrier_before_unreachable() asm volatile("")
> +
> +/*
>   * Mark a position in code as unreachable.  This can be used to
>   * suppress control flow warnings after asm blocks that transfer
>   * control elsewhere.
> @@ -214,7 +223,11 @@
>   * unreleased.  Really, we need to have autoconf for the kernel.
>   */
>  #define unreachable() \
> -	do { annotate_unreachable(); __builtin_unreachable(); } while (0)
> +	do {					\
> +		annotate_unreachable();		\
> +		barrier_before_unreachable();	\
> +		__builtin_unreachable();	\
> +	} while (0)

Unfortunately this breaks microMIPS builds (e.g. MIPS
micro32r2_defconfig and micro32r2el_defconfig) on gcc 7.2, due to the
lack of .insn in the asm volatile. Because of the
__builtin_unreachable() there is no code following it. Without the empty
asm the compiler will apparently put the .insn there automatically, but
with the empty asm it doesn't. Therefore the assembler won't treat an
immediately preceeding label as pointing at 16-bit microMIPS
instructions which need the ISA bit set, i.e. bit 0 of the address.
This causes assembler errors since the branch target is treated as a
different ISA mode:

arch/mips/mm/dma-default.s:3265: Error: branch to a symbol in another ISA mode
arch/mips/mm/dma-default.s:5027: Error: branch to a symbol in another ISA mode

Due to a compiler bug on gcc 4.9.2 -> somewhere before 7.2, Paul
submitted these patches a while back:
https://patchwork.linux-mips.org/patch/13360/
https://patchwork.linux-mips.org/patch/13361/

Your patch (suitably fixed for microMIPS) would I imagine fix that issue
too (it certainly fixes the resulting link error on microMIPS builds
with an old toolchain).

Before I forward port those patches to add .insn for MIPS, is that sort
of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
override barrier_before_unreachable()) an acceptable fix?

Thanks
James
Arnd Bergmann April 11, 2018, 7:30 a.m. | #14
On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
> Hi Arnd,
>
> On Tue, Dec 19, 2017 at 12:39:33PM +0100, Arnd Bergmann wrote:
>> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
>> index 5d595cfdb2c4..66cfdad68f7e 100644
>> --- a/include/linux/compiler-gcc.h
>> +++ b/include/linux/compiler-gcc.h
>> @@ -205,6 +205,15 @@
>>  #endif
>>
>>  /*
>> + * calling noreturn functions, __builtin_unreachable() and __builtin_trap()
>> + * confuse the stack allocation in gcc, leading to overly large stack
>> + * frames, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
>> + *
>> + * Adding an empty inline assembly before it works around the problem
>> + */
>> +#define barrier_before_unreachable() asm volatile("")
>> +
>> +/*
>>   * Mark a position in code as unreachable.  This can be used to
>>   * suppress control flow warnings after asm blocks that transfer
>>   * control elsewhere.
>> @@ -214,7 +223,11 @@
>>   * unreleased.  Really, we need to have autoconf for the kernel.
>>   */
>>  #define unreachable() \
>> -     do { annotate_unreachable(); __builtin_unreachable(); } while (0)
>> +     do {                                    \
>> +             annotate_unreachable();         \
>> +             barrier_before_unreachable();   \
>> +             __builtin_unreachable();        \
>> +     } while (0)
>
> Unfortunately this breaks microMIPS builds (e.g. MIPS
> micro32r2_defconfig and micro32r2el_defconfig) on gcc 7.2, due to the
> lack of .insn in the asm volatile. Because of the
> __builtin_unreachable() there is no code following it. Without the empty
> asm the compiler will apparently put the .insn there automatically, but
> with the empty asm it doesn't. Therefore the assembler won't treat an
> immediately preceeding label as pointing at 16-bit microMIPS
> instructions which need the ISA bit set, i.e. bit 0 of the address.
> This causes assembler errors since the branch target is treated as a
> different ISA mode:
>
> arch/mips/mm/dma-default.s:3265: Error: branch to a symbol in another ISA mode
> arch/mips/mm/dma-default.s:5027: Error: branch to a symbol in another ISA mode

Ok, I see.

> Due to a compiler bug on gcc 4.9.2 -> somewhere before 7.2, Paul
> submitted these patches a while back:
> https://patchwork.linux-mips.org/patch/13360/
> https://patchwork.linux-mips.org/patch/13361/
>
> Your patch (suitably fixed for microMIPS) would I imagine fix that issue
> too (it certainly fixes the resulting link error on microMIPS builds
> with an old toolchain).
>
> Before I forward port those patches to add .insn for MIPS, is that sort
> of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
> override barrier_before_unreachable()) an acceptable fix?

That sounds fine to me. However, I would suggest making that
asm/compiler.h instead of asm/compiler-gcc.h, so we can also
use the same file to include workarounds for clang if needed.

       Arnd
James Hogan April 11, 2018, 9:54 a.m. | #15
On Wed, Apr 11, 2018 at 09:30:56AM +0200, Arnd Bergmann wrote:
> On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
> > Before I forward port those patches to add .insn for MIPS, is that sort
> > of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
> > override barrier_before_unreachable()) an acceptable fix?
> 
> That sounds fine to me. However, I would suggest making that
> asm/compiler.h instead of asm/compiler-gcc.h, so we can also
> use the same file to include workarounds for clang if needed.

Yes, though there are a few asm/compiler.h's already, and the alpha one
includes linux/compiler.h before undefining inline, so seems to have its
own specific purpose...

Cheers
James
Arnd Bergmann April 11, 2018, 10:08 a.m. | #16
On Wed, Apr 11, 2018 at 11:54 AM, James Hogan <jhogan@kernel.org> wrote:
> On Wed, Apr 11, 2018 at 09:30:56AM +0200, Arnd Bergmann wrote:
>> On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
>> > Before I forward port those patches to add .insn for MIPS, is that sort
>> > of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
>> > override barrier_before_unreachable()) an acceptable fix?
>>
>> That sounds fine to me. However, I would suggest making that
>> asm/compiler.h instead of asm/compiler-gcc.h, so we can also
>> use the same file to include workarounds for clang if needed.
>
> Yes, though there are a few asm/compiler.h's already, and the alpha one
> includes linux/compiler.h before undefining inline, so seems to have its
> own specific purpose...

Interesting. For the other ones, including asm/compiler.h from linux/compiler.h
seems appropriate though, so the question would be what to do with the
alpha case. I think we can simply remove that header file and replace
it with this patch:

diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
index b2022885ced8..5502404f54cd 100644
--- a/arch/alpha/Kconfig
+++ b/arch/alpha/Kconfig
@@ -81,6 +81,9 @@ config PGTABLE_LEVELS
        int
        default 3

+config OPTIMIZE_INLINING
+       def_bool y
+
 source "init/Kconfig"
 source "kernel/Kconfig.freezer"

which should have the same effect.
James Hogan April 11, 2018, 10:19 a.m. | #17
On Wed, Apr 11, 2018 at 12:08:51PM +0200, Arnd Bergmann wrote:
> On Wed, Apr 11, 2018 at 11:54 AM, James Hogan <jhogan@kernel.org> wrote:
> > On Wed, Apr 11, 2018 at 09:30:56AM +0200, Arnd Bergmann wrote:
> >> On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
> >> > Before I forward port those patches to add .insn for MIPS, is that sort
> >> > of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
> >> > override barrier_before_unreachable()) an acceptable fix?
> >>
> >> That sounds fine to me. However, I would suggest making that
> >> asm/compiler.h instead of asm/compiler-gcc.h, so we can also
> >> use the same file to include workarounds for clang if needed.
> >
> > Yes, though there are a few asm/compiler.h's already, and the alpha one
> > includes linux/compiler.h before undefining inline, so seems to have its
> > own specific purpose...
> 
> Interesting. For the other ones, including asm/compiler.h from linux/compiler.h
> seems appropriate though, so the question would be what to do with the
> alpha case. I think we can simply remove that header file and replace
> it with this patch:
> 
> diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
> index b2022885ced8..5502404f54cd 100644
> --- a/arch/alpha/Kconfig
> +++ b/arch/alpha/Kconfig
> @@ -81,6 +81,9 @@ config PGTABLE_LEVELS
>         int
>         default 3
> 
> +config OPTIMIZE_INLINING
> +       def_bool y
> +
>  source "init/Kconfig"
>  source "kernel/Kconfig.freezer"
> 
> which should have the same effect.

Hmm yes, and I suppose alpha would need ARCH_SUPPORTS_OPTIMIZED_INLINING
too. I'll give it a try.

Cheers
James

Patch

diff --git a/arch/arc/include/asm/bug.h b/arch/arc/include/asm/bug.h
index ea022d47896c..21ec82466d62 100644
--- a/arch/arc/include/asm/bug.h
+++ b/arch/arc/include/asm/bug.h
@@ -23,7 +23,8 @@  void die(const char *str, struct pt_regs *regs, unsigned long address);
 
 #define BUG()	do {								\
 	pr_warn("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
-	dump_stack();								\
+	barrier_before_unreachable();						\
+	__builtin_trap();							\
 } while (0)
 
 #define HAVE_ARCH_BUG
diff --git a/arch/cris/include/arch-v10/arch/bug.h b/arch/cris/include/arch-v10/arch/bug.h
index 905afeacfedf..06da9d49152a 100644
--- a/arch/cris/include/arch-v10/arch/bug.h
+++ b/arch/cris/include/arch-v10/arch/bug.h
@@ -44,18 +44,25 @@  struct bug_frame {
  * not be used like this with newer versions of gcc.
  */
 #define BUG()								\
+do {									\
 	__asm__ __volatile__ ("clear.d [" __stringify(BUG_MAGIC) "]\n\t"\
 			      "movu.w " __stringify(__LINE__) ",$r0\n\t"\
 			      "jump 0f\n\t"				\
 			      ".section .rodata\n"			\
 			      "0:\t.string \"" __FILE__ "\"\n\t"	\
-			      ".previous")
+			      ".previous");				\
+	unreachable();							\
+} while (0)
 #endif
 
 #else
 
 /* This just causes an oops. */
-#define BUG() (*(int *)0 = 0)
+#define BUG()								\
+do {									\
+	barrier_before_unreachable();					\
+	__builtin_trap();						\
+} while (0)
 
 #endif
 
diff --git a/arch/ia64/include/asm/bug.h b/arch/ia64/include/asm/bug.h
index bd3eeb8d1cfa..66b37a532765 100644
--- a/arch/ia64/include/asm/bug.h
+++ b/arch/ia64/include/asm/bug.h
@@ -4,7 +4,11 @@ 
 
 #ifdef CONFIG_BUG
 #define ia64_abort()	__builtin_trap()
-#define BUG() do { printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__); ia64_abort(); } while (0)
+#define BUG() do {						\
+	printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__);	\
+	barrier_before_unreachable();				\
+	ia64_abort();						\
+} while (0)
 
 /* should this BUG be made generic? */
 #define HAVE_ARCH_BUG
diff --git a/arch/m68k/include/asm/bug.h b/arch/m68k/include/asm/bug.h
index b7e2bf1ba4a6..275dca1435bf 100644
--- a/arch/m68k/include/asm/bug.h
+++ b/arch/m68k/include/asm/bug.h
@@ -8,16 +8,19 @@ 
 #ifndef CONFIG_SUN3
 #define BUG() do { \
 	pr_crit("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
+	barrier_before_unreachable(); \
 	__builtin_trap(); \
 } while (0)
 #else
 #define BUG() do { \
 	pr_crit("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
+	barrier_before_unreachable(); \
 	panic("BUG!"); \
 } while (0)
 #endif
 #else
 #define BUG() do { \
+	barrier_before_unreachable(); \
 	__builtin_trap(); \
 } while (0)
 #endif
diff --git a/arch/sparc/include/asm/bug.h b/arch/sparc/include/asm/bug.h
index 6f17528356b2..ea53e418f6c0 100644
--- a/arch/sparc/include/asm/bug.h
+++ b/arch/sparc/include/asm/bug.h
@@ -9,10 +9,14 @@ 
 void do_BUG(const char *file, int line);
 #define BUG() do {					\
 	do_BUG(__FILE__, __LINE__);			\
+	barrier_before_unreachable();			\
 	__builtin_trap();				\
 } while (0)
 #else
-#define BUG()		__builtin_trap()
+#define BUG() do {					\
+	barrier_before_unreachable();			\
+	__builtin_trap();				\
+} while (0)
 #endif
 
 #define HAVE_ARCH_BUG
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 963b755d19b0..a7613e1b0c87 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -52,6 +52,7 @@  struct bug_entry {
 #ifndef HAVE_ARCH_BUG
 #define BUG() do { \
 	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
+	barrier_before_unreachable(); \
 	panic("BUG!"); \
 } while (0)
 #endif
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 5d595cfdb2c4..66cfdad68f7e 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -205,6 +205,15 @@ 
 #endif
 
 /*
+ * calling noreturn functions, __builtin_unreachable() and __builtin_trap()
+ * confuse the stack allocation in gcc, leading to overly large stack
+ * frames, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
+ *
+ * Adding an empty inline assembly before it works around the problem
+ */
+#define barrier_before_unreachable() asm volatile("")
+
+/*
  * Mark a position in code as unreachable.  This can be used to
  * suppress control flow warnings after asm blocks that transfer
  * control elsewhere.
@@ -214,7 +223,11 @@ 
  * unreleased.  Really, we need to have autoconf for the kernel.
  */
 #define unreachable() \
-	do { annotate_unreachable(); __builtin_unreachable(); } while (0)
+	do {					\
+		annotate_unreachable();		\
+		barrier_before_unreachable();	\
+		__builtin_unreachable();	\
+	} while (0)
 
 /* Mark a function definition as prohibited from being cloned. */
 #define __noclone	__attribute__((__noclone__, __optimize__("no-tracer")))
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 52e611ab9a6c..97847f2f86cf 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -86,6 +86,11 @@  void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 # define barrier_data(ptr) barrier()
 #endif
 
+/* workaround for GCC PR82365 if needed */
+#ifndef barrier_before_unreachable
+# define barrier_before_unreachable() do { } while (0)
+#endif
+
 /* Unreachable code */
 #ifdef CONFIG_STACK_VALIDATION
 /*