All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/debug: define BUG() againfor !CONFIG_BUG
@ 2017-03-29 21:16 Arnd Bergmann
  2017-03-30  7:10 ` Peter Zijlstra
  2017-03-30  7:29 ` [tip:x86/asm] x86/debug: Define BUG() again for !CONFIG_BUG tip-bot for Arnd Bergmann
  0 siblings, 2 replies; 9+ messages in thread
From: Arnd Bergmann @ 2017-03-29 21:16 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra
  Cc: Arnd Bergmann, Josh Triplett, Thomas Gleixner, H. Peter Anvin,
	x86, Josh Poimboeuf, linux-kernel

The latest change to the BUG() macro inadvertently reverted the earlier
commit b06dd879f5db ("x86: always define BUG() and HAVE_ARCH_BUG, even
with !CONFIG_BUG") that sanitized the behavior with CONFIG_BUG=n.

I noticed this as some warnings have appeared again that were previously
fixed as a side effect of that patch:

kernel/seccomp.c: In function '__seccomp_filter':
kernel/seccomp.c:670:1: error: no return statement in function returning non-void [-Werror=return-type]

drivers/gpu/drm/i915/intel_sprite.c: In function 'intel_check_sprite_plane':
drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_h' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   src->y2 = (src_y + src_h) << 16;
             ~~~~~~~^~~~~~~~
drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_w' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   src->x2 = (src_x + src_w) << 16;
             ~~~~~~~^~~~~~~~
drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_y' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   src->y2 = (src_y + src_h) << 16;
             ~~~~~~~^~~~~~~~
drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_x' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   src->x2 = (src_x + src_w) << 16;
             ~~~~~~~^~~~~~~~

This combines the two patches and uses the ud2 macro to define BUG()
in case of CONFIG_BUG=n.

Fixes: 9a93848fe787 ("x86/debug: Implement __WARN() using UD0")
Cc: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/x86/include/asm/bug.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 4fde330c44b7..cecf559d0012 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -23,7 +23,6 @@
 #define LEN_UD0		2
 
 #ifdef CONFIG_GENERIC_BUG
-#define HAVE_ARCH_BUG
 
 #ifdef CONFIG_X86_32
 # define __BUG_REL(val)	".long " __stringify(val)
@@ -64,6 +63,13 @@ do {									\
 
 #endif /* CONFIG_DEBUG_BUGVERBOSE */
 
+#else
+
+#define _BUG_FLAGS(ins, flags)  asm volatile(ins)
+
+#endif /* CONFIG_GENERIC_BUG */
+
+#define HAVE_ARCH_BUG
 #define BUG()							\
 do {								\
 	_BUG_FLAGS(ASM_UD2, 0);					\
@@ -72,8 +78,6 @@ do {								\
 
 #define __WARN_TAINT(taint)	_BUG_FLAGS(ASM_UD0, BUGFLAG_TAINT(taint))
 
-#endif /* CONFIG_GENERIC_BUG */
-
 #include <asm-generic/bug.h>
 
 #endif /* _ASM_X86_BUG_H */
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86/debug: define BUG() againfor !CONFIG_BUG
  2017-03-29 21:16 [PATCH] x86/debug: define BUG() againfor !CONFIG_BUG Arnd Bergmann
@ 2017-03-30  7:10 ` Peter Zijlstra
  2017-03-30  7:17   ` Ingo Molnar
  2017-03-30  7:25   ` Arnd Bergmann
  2017-03-30  7:29 ` [tip:x86/asm] x86/debug: Define BUG() again for !CONFIG_BUG tip-bot for Arnd Bergmann
  1 sibling, 2 replies; 9+ messages in thread
From: Peter Zijlstra @ 2017-03-30  7:10 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Ingo Molnar, Josh Triplett, Thomas Gleixner, H. Peter Anvin, x86,
	Josh Poimboeuf, linux-kernel

On Wed, Mar 29, 2017 at 11:16:31PM +0200, Arnd Bergmann wrote:
> The latest change to the BUG() macro inadvertently reverted the earlier
> commit b06dd879f5db ("x86: always define BUG() and HAVE_ARCH_BUG, even
> with !CONFIG_BUG") that sanitized the behavior with CONFIG_BUG=n.
> 
> I noticed this as some warnings have appeared again that were previously
> fixed as a side effect of that patch:
> 
> kernel/seccomp.c: In function '__seccomp_filter':
> kernel/seccomp.c:670:1: error: no return statement in function returning non-void [-Werror=return-type]
> 
> drivers/gpu/drm/i915/intel_sprite.c: In function 'intel_check_sprite_plane':
> drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_h' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    src->y2 = (src_y + src_h) << 16;
>              ~~~~~~~^~~~~~~~
> drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_w' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    src->x2 = (src_x + src_w) << 16;
>              ~~~~~~~^~~~~~~~
> drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_y' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    src->y2 = (src_y + src_h) << 16;
>              ~~~~~~~^~~~~~~~
> drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_x' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    src->x2 = (src_x + src_w) << 16;
>              ~~~~~~~^~~~~~~~
> 
> This combines the two patches and uses the ud2 macro to define BUG()
> in case of CONFIG_BUG=n.

OK, fair enough I suppose. However, I cribbed this from arm64. What does
that do for BUG=n ?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86/debug: define BUG() againfor !CONFIG_BUG
  2017-03-30  7:10 ` Peter Zijlstra
@ 2017-03-30  7:17   ` Ingo Molnar
  2017-03-30  7:26     ` Ingo Molnar
  2017-03-30  7:35     ` Peter Zijlstra
  2017-03-30  7:25   ` Arnd Bergmann
  1 sibling, 2 replies; 9+ messages in thread
From: Ingo Molnar @ 2017-03-30  7:17 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Arnd Bergmann, Ingo Molnar, Josh Triplett, Thomas Gleixner,
	H. Peter Anvin, x86, Josh Poimboeuf, linux-kernel


* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Mar 29, 2017 at 11:16:31PM +0200, Arnd Bergmann wrote:
> > The latest change to the BUG() macro inadvertently reverted the earlier
> > commit b06dd879f5db ("x86: always define BUG() and HAVE_ARCH_BUG, even
> > with !CONFIG_BUG") that sanitized the behavior with CONFIG_BUG=n.
> > 
> > I noticed this as some warnings have appeared again that were previously
> > fixed as a side effect of that patch:
> > 
> > kernel/seccomp.c: In function '__seccomp_filter':
> > kernel/seccomp.c:670:1: error: no return statement in function returning non-void [-Werror=return-type]
> > 
> > drivers/gpu/drm/i915/intel_sprite.c: In function 'intel_check_sprite_plane':
> > drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_h' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >    src->y2 = (src_y + src_h) << 16;
> >              ~~~~~~~^~~~~~~~
> > drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_w' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >    src->x2 = (src_x + src_w) << 16;
> >              ~~~~~~~^~~~~~~~
> > drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_y' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >    src->y2 = (src_y + src_h) << 16;
> >              ~~~~~~~^~~~~~~~
> > drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_x' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >    src->x2 = (src_x + src_w) << 16;
> >              ~~~~~~~^~~~~~~~
> > 
> > This combines the two patches and uses the ud2 macro to define BUG()
> > in case of CONFIG_BUG=n.
> 
> OK, fair enough I suppose. However, I cribbed this from arm64. What does
> that do for BUG=n ?

I think we'll get a U2D crash in this case, without any bug information.

I.e. only marginally debuggable, but it's a deterministic outcome - instead of the 
crazy GCC code generation variant of the day when the warning triggers, or the 
similarly crazy infinite loop hang.

I'm not entirely sure though, I don't think many people actually _use_ 
CONFIG_BUG=n, it's essentially a crazy thing to do even on constrainted hardware. 
Debugging and maintenance costs almost always trump marginal hardware costs of a 
bit more debugging code.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86/debug: define BUG() againfor !CONFIG_BUG
  2017-03-30  7:10 ` Peter Zijlstra
  2017-03-30  7:17   ` Ingo Molnar
@ 2017-03-30  7:25   ` Arnd Bergmann
  1 sibling, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2017-03-30  7:25 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Ingo Molnar, Josh Triplett, Thomas Gleixner, H. Peter Anvin,
	the arch/x86 maintainers, Josh Poimboeuf,
	Linux Kernel Mailing List

On Thu, Mar 30, 2017 at 9:10 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 29, 2017 at 11:16:31PM +0200, Arnd Bergmann wrote:
>> The latest change to the BUG() macro inadvertently reverted the earlier
>> commit b06dd879f5db ("x86: always define BUG() and HAVE_ARCH_BUG, even
>> with !CONFIG_BUG") that sanitized the behavior with CONFIG_BUG=n.
>>
>> I noticed this as some warnings have appeared again that were previously
>> fixed as a side effect of that patch:
>>
>> kernel/seccomp.c: In function '__seccomp_filter':
>> kernel/seccomp.c:670:1: error: no return statement in function returning non-void [-Werror=return-type]
>>
>> drivers/gpu/drm/i915/intel_sprite.c: In function 'intel_check_sprite_plane':
>> drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_h' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>    src->y2 = (src_y + src_h) << 16;
>>              ~~~~~~~^~~~~~~~
>> drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_w' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>    src->x2 = (src_x + src_w) << 16;
>>              ~~~~~~~^~~~~~~~
>> drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_y' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>    src->y2 = (src_y + src_h) << 16;
>>              ~~~~~~~^~~~~~~~
>> drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_x' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>    src->x2 = (src_x + src_w) << 16;
>>              ~~~~~~~^~~~~~~~
>>
>> This combines the two patches and uses the ud2 macro to define BUG()
>> in case of CONFIG_BUG=n.
>
> OK, fair enough I suppose. However, I cribbed this from arm64.

Catalin just merged my patch to make arm64 work like x86 and arm32 ;-)

> What does that do for BUG=n ?

Without the patch, running into a BUG() often leads to undefined
behavior, e.g. running off at the end of a function that is marked
noreturn(), or simply continuing a thread after we know that this
leads to data corruption (which is the reason to put the BUG() in there
in the first place).

With my patch, we will insert a ud2 instruction and mark the context
as unreachable(), so gcc can discard any assembler output after the
unreachable() and the kernel will kill the task at that point, but will
not have a bug_entry and just report an invalid instruction similar
to what newer gcc generates for an unconditional division by zero or
NULL pointer dereference.

     Arnd

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86/debug: define BUG() againfor !CONFIG_BUG
  2017-03-30  7:17   ` Ingo Molnar
@ 2017-03-30  7:26     ` Ingo Molnar
  2017-03-30  7:35     ` Peter Zijlstra
  1 sibling, 0 replies; 9+ messages in thread
From: Ingo Molnar @ 2017-03-30  7:26 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Arnd Bergmann, Ingo Molnar, Josh Triplett, Thomas Gleixner,
	H. Peter Anvin, x86, Josh Poimboeuf, linux-kernel


* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Wed, Mar 29, 2017 at 11:16:31PM +0200, Arnd Bergmann wrote:
> > > The latest change to the BUG() macro inadvertently reverted the earlier
> > > commit b06dd879f5db ("x86: always define BUG() and HAVE_ARCH_BUG, even
> > > with !CONFIG_BUG") that sanitized the behavior with CONFIG_BUG=n.
> > > 
> > > I noticed this as some warnings have appeared again that were previously
> > > fixed as a side effect of that patch:
> > > 
> > > kernel/seccomp.c: In function '__seccomp_filter':
> > > kernel/seccomp.c:670:1: error: no return statement in function returning non-void [-Werror=return-type]
> > > 
> > > drivers/gpu/drm/i915/intel_sprite.c: In function 'intel_check_sprite_plane':
> > > drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_h' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >    src->y2 = (src_y + src_h) << 16;
> > >              ~~~~~~~^~~~~~~~
> > > drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_w' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >    src->x2 = (src_x + src_w) << 16;
> > >              ~~~~~~~^~~~~~~~
> > > drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_y' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >    src->y2 = (src_y + src_h) << 16;
> > >              ~~~~~~~^~~~~~~~
> > > drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_x' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >    src->x2 = (src_x + src_w) << 16;
> > >              ~~~~~~~^~~~~~~~
> > > 
> > > This combines the two patches and uses the ud2 macro to define BUG()
> > > in case of CONFIG_BUG=n.
> > 
> > OK, fair enough I suppose. However, I cribbed this from arm64. What does
> > that do for BUG=n ?
> 
> I think we'll get a U2D crash in this case, without any bug information.
> 
> I.e. only marginally debuggable, but it's a deterministic outcome - instead of the 
> crazy GCC code generation variant of the day when the warning triggers, or the 
> similarly crazy infinite loop hang.
> 
> I'm not entirely sure though, I don't think many people actually _use_ 
> CONFIG_BUG=n, it's essentially a crazy thing to do even on constrainted hardware. 
> Debugging and maintenance costs almost always trump marginal hardware costs of a 
> bit more debugging code.

So I've applied Arnd's patch to restore the previous behavior - but I agree with 
you that the situation isn't entirely logical at the moment. At minimum we need a 
comment explaining it or so.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [tip:x86/asm] x86/debug: Define BUG() again for !CONFIG_BUG
  2017-03-29 21:16 [PATCH] x86/debug: define BUG() againfor !CONFIG_BUG Arnd Bergmann
  2017-03-30  7:10 ` Peter Zijlstra
@ 2017-03-30  7:29 ` tip-bot for Arnd Bergmann
  1 sibling, 0 replies; 9+ messages in thread
From: tip-bot for Arnd Bergmann @ 2017-03-30  7:29 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: peterz, josh, jpoimboe, dvlasenk, luto, hpa, tglx, linux-kernel,
	torvalds, brgerst, arnd, mingo, bp

Commit-ID:  70579a86e3c8eb2ce57999e594a73b4dfe095959
Gitweb:     http://git.kernel.org/tip/70579a86e3c8eb2ce57999e594a73b4dfe095959
Author:     Arnd Bergmann <arnd@arndb.de>
AuthorDate: Wed, 29 Mar 2017 23:16:31 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 30 Mar 2017 09:12:10 +0200

x86/debug: Define BUG() again for !CONFIG_BUG

The latest change to the BUG() macro inadvertently reverted the earlier
commit:

  b06dd879f5db ("x86: always define BUG() and HAVE_ARCH_BUG, even with !CONFIG_BUG")

... that sanitized the behavior with CONFIG_BUG=n.

I noticed this as some warnings have appeared again that were previously
fixed as a side effect of that patch:

  kernel/seccomp.c: In function '__seccomp_filter':
  kernel/seccomp.c:670:1: error: no return statement in function returning non-void [-Werror=return-type]
  ...

This combines the two patches and uses the ud2 macro to define BUG()
in case of CONFIG_BUG=n.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: 9a93848fe787 ("x86/debug: Implement __WARN() using UD0")
Link: http://lkml.kernel.org/r/20170329211646.2707365-1-arnd@arndb.de
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/bug.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 4fde330..cecf559 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -23,7 +23,6 @@
 #define LEN_UD0		2
 
 #ifdef CONFIG_GENERIC_BUG
-#define HAVE_ARCH_BUG
 
 #ifdef CONFIG_X86_32
 # define __BUG_REL(val)	".long " __stringify(val)
@@ -64,6 +63,13 @@ do {									\
 
 #endif /* CONFIG_DEBUG_BUGVERBOSE */
 
+#else
+
+#define _BUG_FLAGS(ins, flags)  asm volatile(ins)
+
+#endif /* CONFIG_GENERIC_BUG */
+
+#define HAVE_ARCH_BUG
 #define BUG()							\
 do {								\
 	_BUG_FLAGS(ASM_UD2, 0);					\
@@ -72,8 +78,6 @@ do {								\
 
 #define __WARN_TAINT(taint)	_BUG_FLAGS(ASM_UD0, BUGFLAG_TAINT(taint))
 
-#endif /* CONFIG_GENERIC_BUG */
-
 #include <asm-generic/bug.h>
 
 #endif /* _ASM_X86_BUG_H */

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86/debug: define BUG() againfor !CONFIG_BUG
  2017-03-30  7:17   ` Ingo Molnar
  2017-03-30  7:26     ` Ingo Molnar
@ 2017-03-30  7:35     ` Peter Zijlstra
  2017-03-30  7:47       ` Ingo Molnar
  1 sibling, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2017-03-30  7:35 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Arnd Bergmann, Ingo Molnar, Josh Triplett, Thomas Gleixner,
	H. Peter Anvin, x86, Josh Poimboeuf, linux-kernel

On Thu, Mar 30, 2017 at 09:17:07AM +0200, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Wed, Mar 29, 2017 at 11:16:31PM +0200, Arnd Bergmann wrote:
> > > The latest change to the BUG() macro inadvertently reverted the earlier
> > > commit b06dd879f5db ("x86: always define BUG() and HAVE_ARCH_BUG, even
> > > with !CONFIG_BUG") that sanitized the behavior with CONFIG_BUG=n.
> > > 
> > > I noticed this as some warnings have appeared again that were previously
> > > fixed as a side effect of that patch:
> > > 
> > > kernel/seccomp.c: In function '__seccomp_filter':
> > > kernel/seccomp.c:670:1: error: no return statement in function returning non-void [-Werror=return-type]
> > > 
> > > drivers/gpu/drm/i915/intel_sprite.c: In function 'intel_check_sprite_plane':
> > > drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_h' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >    src->y2 = (src_y + src_h) << 16;
> > >              ~~~~~~~^~~~~~~~
> > > drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_w' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >    src->x2 = (src_x + src_w) << 16;
> > >              ~~~~~~~^~~~~~~~
> > > drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_y' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >    src->y2 = (src_y + src_h) << 16;
> > >              ~~~~~~~^~~~~~~~
> > > drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_x' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >    src->x2 = (src_x + src_w) << 16;
> > >              ~~~~~~~^~~~~~~~
> > > 
> > > This combines the two patches and uses the ud2 macro to define BUG()
> > > in case of CONFIG_BUG=n.
> > 
> > OK, fair enough I suppose. However, I cribbed this from arm64. What does
> > that do for BUG=n ?
> 
> I think we'll get a U2D crash in this case, without any bug information.
> 
> I.e. only marginally debuggable, but it's a deterministic outcome - instead of the 
> crazy GCC code generation variant of the day when the warning triggers, or the 
> similarly crazy infinite loop hang.
> 
> I'm not entirely sure though, I don't think many people actually _use_ 
> CONFIG_BUG=n, it's essentially a crazy thing to do even on constrainted hardware. 
> Debugging and maintenance costs almost always trump marginal hardware costs of a 
> bit more debugging code.

So should we then, for x86, disable BUG=n instead?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86/debug: define BUG() againfor !CONFIG_BUG
  2017-03-30  7:35     ` Peter Zijlstra
@ 2017-03-30  7:47       ` Ingo Molnar
  2017-03-30  8:03         ` Arnd Bergmann
  0 siblings, 1 reply; 9+ messages in thread
From: Ingo Molnar @ 2017-03-30  7:47 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Arnd Bergmann, Ingo Molnar, Josh Triplett, Thomas Gleixner,
	H. Peter Anvin, x86, Josh Poimboeuf, linux-kernel


* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Mar 30, 2017 at 09:17:07AM +0200, Ingo Molnar wrote:
> > 
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Wed, Mar 29, 2017 at 11:16:31PM +0200, Arnd Bergmann wrote:
> > > > The latest change to the BUG() macro inadvertently reverted the earlier
> > > > commit b06dd879f5db ("x86: always define BUG() and HAVE_ARCH_BUG, even
> > > > with !CONFIG_BUG") that sanitized the behavior with CONFIG_BUG=n.
> > > > 
> > > > I noticed this as some warnings have appeared again that were previously
> > > > fixed as a side effect of that patch:
> > > > 
> > > > kernel/seccomp.c: In function '__seccomp_filter':
> > > > kernel/seccomp.c:670:1: error: no return statement in function returning non-void [-Werror=return-type]
> > > > 
> > > > drivers/gpu/drm/i915/intel_sprite.c: In function 'intel_check_sprite_plane':
> > > > drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_h' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > > >    src->y2 = (src_y + src_h) << 16;
> > > >              ~~~~~~~^~~~~~~~
> > > > drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_w' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > > >    src->x2 = (src_x + src_w) << 16;
> > > >              ~~~~~~~^~~~~~~~
> > > > drivers/gpu/drm/i915/intel_sprite.c:936:20: error: 'src_y' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > > >    src->y2 = (src_y + src_h) << 16;
> > > >              ~~~~~~~^~~~~~~~
> > > > drivers/gpu/drm/i915/intel_sprite.c:934:20: error: 'src_x' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > > >    src->x2 = (src_x + src_w) << 16;
> > > >              ~~~~~~~^~~~~~~~
> > > > 
> > > > This combines the two patches and uses the ud2 macro to define BUG()
> > > > in case of CONFIG_BUG=n.
> > > 
> > > OK, fair enough I suppose. However, I cribbed this from arm64. What does
> > > that do for BUG=n ?
> > 
> > I think we'll get a U2D crash in this case, without any bug information.
> > 
> > I.e. only marginally debuggable, but it's a deterministic outcome - instead of the 
> > crazy GCC code generation variant of the day when the warning triggers, or the 
> > similarly crazy infinite loop hang.
> > 
> > I'm not entirely sure though, I don't think many people actually _use_ 
> > CONFIG_BUG=n, it's essentially a crazy thing to do even on constrainted hardware. 
> > Debugging and maintenance costs almost always trump marginal hardware costs of a 
> > bit more debugging code.
> 
> So should we then, for x86, disable BUG=n instead?

Arnd, does CONFIG_BUG=n give any (marginal) text savings wrt. 
CONFIG_BUG=y && CONFIG_BUG_VERBOSE=n?

If not then we should indeed just disable CONFIG_BUG=n.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86/debug: define BUG() againfor !CONFIG_BUG
  2017-03-30  7:47       ` Ingo Molnar
@ 2017-03-30  8:03         ` Arnd Bergmann
  0 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2017-03-30  8:03 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Peter Zijlstra, Ingo Molnar, Josh Triplett, Thomas Gleixner,
	H. Peter Anvin, the arch/x86 maintainers, Josh Poimboeuf,
	Linux Kernel Mailing List

On Thu, Mar 30, 2017 at 9:47 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Peter Zijlstra <peterz@infradead.org> wrote:
>> So should we then, for x86, disable BUG=n instead?
>
> Arnd, does CONFIG_BUG=n give any (marginal) text savings wrt.
> CONFIG_BUG=y && CONFIG_BUG_VERBOSE=n?

My patch makes it slightly worse, but the difference is still
around 1% of the total vmlinux size:

# CONFIG_BUG=n without my patch
   text   data    bss    dec    hex filename
9543165 4190288 843776 14577229 de6e4d obj-x86/vmlinux

# CONFIG_BUG=n with my patch
   text   data    bss    dec    hex filename
9578700 4190480 843776 14612956 def9dc obj-x86/vmlinux

# CONFIG_BUG=y, CONFIG_BUGVERBOSE=n
   text   data    bss    dec    hex filename
9698166 4212208 843776 14754150 e12166 obj-x86/vmlinux

Among the things we save are:

- WARN()/WARN_ON() gets left out entirely
- The bug_entry section (46192 bytes in defconfig)
- The code for decoding and printing the bug

> If not then we should indeed just disable CONFIG_BUG=n.

CONFIG_BUG is already guarded by CONFIG_EXPERT, along with
this help text in Kconfig:

          Disabling this option eliminates support for BUG and WARN, reducing
          the size of your kernel image and potentially quietly ignoring
          numerous fatal conditions. You should only consider disabling this
          option for embedded systems with no facilities for reporting errors.
          Just say Y.

I think this is reasonable to disable for systems that have no console
at all.

        Arnd

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-03-30  8:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-29 21:16 [PATCH] x86/debug: define BUG() againfor !CONFIG_BUG Arnd Bergmann
2017-03-30  7:10 ` Peter Zijlstra
2017-03-30  7:17   ` Ingo Molnar
2017-03-30  7:26     ` Ingo Molnar
2017-03-30  7:35     ` Peter Zijlstra
2017-03-30  7:47       ` Ingo Molnar
2017-03-30  8:03         ` Arnd Bergmann
2017-03-30  7:25   ` Arnd Bergmann
2017-03-30  7:29 ` [tip:x86/asm] x86/debug: Define BUG() again for !CONFIG_BUG tip-bot for Arnd Bergmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.