Issue with BUG() in asm-gemeric/bug.h if CONFIG_BUG=n

PATCH Ralf Baechle 4 participants 6 messages Sep 30, 2013

Subsystems (2)

MIPS Generic include/asm header files

lore

Share & tools

lore

David Daney

2013-09-30 15:53 UTC #1 lore raw

Quoted 35 lines · Ralf Baechle

Lately I received several patches for build issues that only strike if CONFIG_BUG is disabled. Here's a test case extracted from one of them:
/*
* Definition of BUG taken from asm-generic/bug.h for the CONFIG_BUG=n case */
#define BUG() do {} while(0)
int foo(int arg)
{
	int res;

	if (arg == 1)
		res = 23;
	else if (arg == 2)
		res = 42;
	else
		BUG();

	return res;
}
[ralf@h7 ~]$ gcc -O2 -Wall -c bug.c
bug.c: In function ‘foo’:
bug.c:17:2: warning: ‘res’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   return res;
   ^
It's fairly obvious to see what's happening here - GCC doesn't know that the else case can not be reached, thus razorsharply concludes that res may be used uninitialized.
There several locations where MIPS - possibly other architectures as well - is affected by this.

Quoted 7 lines

I think the definition of BUG should be changed to something like
#define BUG() unreachable()
16304
unreachable() will depending on the compiler being used, expand either into a call to __builtin_unreachable() or where that function is unavailable, into do {} while (1).

The *only* reason we have CONFIG_BUG=n is to reduce code size.

Sticking in that empty loop, negates the entire point.

IMHO: We should do one of:
o Make CONFIG_BUG=y mandatory
o Ignore the warnings.
o Fix the warning sites so they quit Warning.

So I don't think the patch is really an improvement over the status quo.

David Daney

Quoted 31 lines

__builtin_unreachable() was introduce for GCC 4.5.0.
This means there'd be minor bloat for antique compilers - but probably even better code generation for compilers supporting __builtin_unreachable().
Ralf
Signed-off-by Ralf Baechle <[email protected]>
  include/asm-generic/bug.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
include/asm-generic/bug.h +1 -2
index 7d10f96..6f78771 100644
@@ -108,7 +108,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 
  #else /* !CONFIG_BUG */
  #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
+#define BUG() unreachable()
  #endif
 
  #ifndef HAVE_ARCH_BUG_ON
 
----- End forwarded message -----
 
   Ralf

Pinski, Andrew

2013-09-30 17:16 UTC #2 lore raw

Quoted 49 lines · David Daney

Quoted 38 lines · Ralf Baechle
Lately I received several patches for build issues that only strike if CONFIG_BUG is disabled. Here's a test case extracted from one of them:
/*
* Definition of BUG taken from asm-generic/bug.h for the CONFIG_BUG=n case */
#define BUG() do {} while(0)
int foo(int arg)
{
   int res;

   if (arg == 1)
       res = 23;
   else if (arg == 2)
       res = 42;
   else
       BUG();

   return res;
}
[ralf@h7 ~]$ gcc -O2 -Wall -c bug.c
bug.c: In function ‘foo’:
bug.c:17:2: warning: ‘res’ may be used uninitialized in this function [-Wmaybe-uninitialized]
return res;
^
It's fairly obvious to see what's happening here - GCC doesn't know that the else case can not be reached, thus razorsharply concludes that res may be used uninitialized.
There several locations where MIPS - possibly other architectures as well - is affected by this.
Quoted 7 lines
I think the definition of BUG should be changed to something like
#define BUG() unreachable()
16304
unreachable() will depending on the compiler being used, expand either into a call to __builtin_unreachable() or where that function is unavailable, into do {} while (1).
The *only* reason we have CONFIG_BUG=n is to reduce code size.

Quoted 8 lines

Sticking in that empty loop, negates the entire point.
IMHO: We should do one of:
o Make CONFIG_BUG=y mandatory
o Ignore the warnings.
o Fix the warning sites so they quit Warning.
So I don't think the patch is really an improvement over the status quo.

What about using __builtin_unreachable when we can but turn off warnings and use do{}while(0) when __builtin_unreachable does not exist? This seems the both worlds. Newer compilers produce better code with unreachable anyways.

Thanks,
Andrew

Quoted 34 lines

David Daney
Quoted 30 lines
__builtin_unreachable() was introduce for GCC 4.5.0.
This means there'd be minor bloat for antique compilers - but probably even better code generation for compilers supporting __builtin_unreachable().
Ralf
Signed-off-by Ralf Baechle <[email protected]>
 include/asm-generic/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/asm-generic/bug.h +1 -2
index 7d10f96..6f78771 100644
@@ -108,7 +108,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
+#define BUG() unreachable()
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
 
----- End forwarded message -----
 
  Ralf

David Daney

2013-09-30 17:45 UTC #3 lore raw

Quoted 51 lines · Andrew

Quoted 51 lines · David Daney
Quoted 38 lines · Ralf Baechle
Lately I received several patches for build issues that only strike if CONFIG_BUG is disabled. Here's a test case extracted from one of them:
/*
* Definition of BUG taken from asm-generic/bug.h for the CONFIG_BUG=n case */
#define BUG() do {} while(0)
int foo(int arg)
{
    int res;

    if (arg == 1)
        res = 23;
    else if (arg == 2)
        res = 42;
    else
        BUG();

    return res;
}
[ralf@h7 ~]$ gcc -O2 -Wall -c bug.c
bug.c: In function ‘foo’:
bug.c:17:2: warning: ‘res’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   return res;
   ^
It's fairly obvious to see what's happening here - GCC doesn't know that the else case can not be reached, thus razorsharply concludes that res may be used uninitialized.
There several locations where MIPS - possibly other architectures as well - is affected by this.
Quoted 7 lines
I think the definition of BUG should be changed to something like
#define BUG() unreachable()
16304
unreachable() will depending on the compiler being used, expand either into a call to __builtin_unreachable() or where that function is unavailable, into do {} while (1).
The *only* reason we have CONFIG_BUG=n is to reduce code size.
Sticking in that empty loop, negates the entire point.

Quoted 9 lines

Quoted 6 lines
IMHO: We should do one of:
o Make CONFIG_BUG=y mandatory
o Ignore the warnings.
o Fix the warning sites so they quit Warning.
So I don't think the patch is really an improvement over the status quo.
What about using __builtin_unreachable when we can but turn off warnings and use do{}while(0) when __builtin_unreachable does not exist? This seems the both worlds. Newer compilers produce better code with unreachable anyways.

Simply not true.

do{}while(0) is a NOP it is no more useful than an ';' statement. It doesn't serve as a magic uninitialized variable hiding mechanism.

David Daney

Quoted 40 lines

Thanks,
Andrew
Quoted 34 lines
David Daney
Quoted 30 lines
__builtin_unreachable() was introduce for GCC 4.5.0.
This means there'd be minor bloat for antique compilers - but probably even better code generation for compilers supporting __builtin_unreachable().
Ralf
Signed-off-by Ralf Baechle <[email protected]>
  include/asm-generic/bug.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
include/asm-generic/bug.h +1 -2
index 7d10f96..6f78771 100644
@@ -108,7 +108,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 
  #else /* !CONFIG_BUG */
  #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
+#define BUG() unreachable()
  #endif
 
  #ifndef HAVE_ARCH_BUG_ON
 
----- End forwarded message -----
 
   Ralf

Geert Uytterhoeven

2013-09-30 19:03 UTC #4 lore raw

Quoted 10 lines · David Daney

Quoted 5 lines
What about using __builtin_unreachable when we can but turn off warnings and use do{}while(0) when __builtin_unreachable does not exist? This seems the both worlds. Newer compilers produce better code with unreachable anyways.
Simply not true.
do{}while(0) is a NOP it is no more useful than an ';' statement. It doesn't serve as a magic uninitialized variable hiding mechanism.

You missed the "turn off warnings" part of the "and".

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

David Daney

2013-09-30 19:17 UTC #5 lore raw

Quoted 7 lines · Geert Uytterhoeven

Quoted 6 lines · David Daney
Quoted 5 lines
What about using __builtin_unreachable when we can but turn off warnings and use do{}while(0) when __builtin_unreachable does not exist? This seems the both worlds. Newer compilers produce better code with unreachable anyways.

Quoted 6 lines

Quoted 4 lines
Simply not true.
do{}while(0) is a NOP it is no more useful than an ';' statement. It doesn't serve as a magic uninitialized variable hiding mechanism.
You missed the "turn off warnings" part of the "and".

You are correct, I did miss it.

The real problem here is that the kernel is written to expect that BUG() never returns. Any implementation that has BUG() return, is almost certainly *not* what we want.

But wieh people select CONFIG_BUG=n they expect the smallest possible code.

These two criteria are mutually exclusive, so something should change.

It is not just the uninitialized variable warning, there can be others as well (control reaching the end of a non-void function comes to mind).
So I don't think turning off the warnings is a good solution.

That leaves:

1) Remove CONFIG_BUG and make it unconditionally enabled.

2) Make CONFIG_BUG=n imply "static inline void BUG(void){do{}while(1);}" which might be bigger than with CONFIG_BUG=y

David Daney

Quoted 13 lines

Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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