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

[David Daney <ddaney.cavm@gmail.com>]

On 09/30/2013 07:56 AM, Ralf Baechle wrote:
> 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.
>
> 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

>
> __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 <ralf@linux-mips.org>
>
>   include/asm-generic/bug.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 7d10f96..6f78771 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -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
>