From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vineet Gupta Subject: Re: [PATCH] bug.h: Work around GCC PR82365 in BUG() Date: Wed, 7 Feb 2018 17:20:38 -0800 Message-ID: References: <20171219114112.939391-1-arnd@arndb.de> <8e42a1de-f619-7a4e-6d58-f90f53f5f38f@synopsys.com> <27bfdc23-1774-cf07-dab4-ab253a41d2f7@synopsys.com> <861992dd-a28d-0f8c-572f-4194d15238df@synopsys.com> <20180207160138.0705eeca67d8cc4d6309381c@linux-foundation.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20180207160138.0705eeca67d8cc4d6309381c@linux-foundation.org> Content-Language: en-US Sender: linux-kbuild-owner@vger.kernel.org To: Andrew Morton Cc: Arnd Bergmann , "linux-arch@vger.kernel.org" , "linux-kbuild@vger.kernel.org" , Mikael Starvik , Jesper Nilsson , Tony Luck , Fenghua Yu , Geert Uytterhoeven , "David S. Miller" , Christopher Li , Thomas Gleixner , Peter Zijlstra , Kees Cook , Ingo Molnar , Josh Poimboeuf , Will Deacon , "Steven Rostedt (VMware)" , Mark Rutland , arcml List-Id: linux-arch.vger.kernel.org On 02/07/2018 04:01 PM, Andrew Morton wrote: > On Wed, 20 Dec 2017 12:29:02 -0800 Vineet Gupta 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 > 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 > Tested-by: Vineet Gupta > Cc: Mikael Starvik > Cc: Jesper Nilsson > Cc: Tony Luck > Cc: Fenghua Yu > Cc: Geert Uytterhoeven > Cc: "David S. Miller" > Cc: Christopher Li > Cc: Thomas Gleixner > Cc: Peter Zijlstra > Cc: Kees Cook > Cc: Ingo Molnar > Cc: Josh Poimboeuf > Cc: Will Deacon > Cc: "Steven Rostedt (VMware)" > Cc: Mark Rutland > Signed-off-by: Andrew Morton > --- > > 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 # for arch/arc Tested-by: Vineet Gupta # for arch/arc From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us01smtprelay-2.synopsys.com ([198.182.60.111]:55607 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750736AbeBHBU4 (ORCPT ); Wed, 7 Feb 2018 20:20:56 -0500 Subject: Re: [PATCH] bug.h: Work around GCC PR82365 in BUG() References: <20171219114112.939391-1-arnd@arndb.de> <8e42a1de-f619-7a4e-6d58-f90f53f5f38f@synopsys.com> <27bfdc23-1774-cf07-dab4-ab253a41d2f7@synopsys.com> <861992dd-a28d-0f8c-572f-4194d15238df@synopsys.com> <20180207160138.0705eeca67d8cc4d6309381c@linux-foundation.org> From: Vineet Gupta Message-ID: Date: Wed, 7 Feb 2018 17:20:38 -0800 MIME-Version: 1.0 In-Reply-To: <20180207160138.0705eeca67d8cc4d6309381c@linux-foundation.org> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Andrew Morton Cc: Arnd Bergmann , "linux-arch@vger.kernel.org" , "linux-kbuild@vger.kernel.org" , Mikael Starvik , Jesper Nilsson , Tony Luck , Fenghua Yu , Geert Uytterhoeven , "David S. Miller" , Christopher Li , Thomas Gleixner , Peter Zijlstra , Kees Cook , Ingo Molnar , Josh Poimboeuf , Will Deacon , "Steven Rostedt (VMware)" , Mark Rutland , arcml Message-ID: <20180208012038.YrJVLPzjc6T-0RjdLKpHl79dASe-m3LmPfjfSAonZ2g@z> On 02/07/2018 04:01 PM, Andrew Morton wrote: > On Wed, 20 Dec 2017 12:29:02 -0800 Vineet Gupta 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 > 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 > Tested-by: Vineet Gupta > Cc: Mikael Starvik > Cc: Jesper Nilsson > Cc: Tony Luck > Cc: Fenghua Yu > Cc: Geert Uytterhoeven > Cc: "David S. Miller" > Cc: Christopher Li > Cc: Thomas Gleixner > Cc: Peter Zijlstra > Cc: Kees Cook > Cc: Ingo Molnar > Cc: Josh Poimboeuf > Cc: Will Deacon > Cc: "Steven Rostedt (VMware)" > Cc: Mark Rutland > Signed-off-by: Andrew Morton > --- > > 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 # for arch/arc Tested-by: Vineet Gupta # for arch/arc