Message-ID: <4B31743F.8070804@caviumnetworks.com>
Date: Tue, 22 Dec 2009 17:37:03 -0800
From: David Daney <ddaney@caviumnetworks.com>
User-Agent: Thunderbird 2.0.0.21 (X11/20090320)
MIME-Version: 1.0
To: Alexander Beregalov <a.beregalov@gmail.com>
CC: arnd@arndb.de, linux-kernel@vger.kernel.org,
       David Miller <davem@davemloft.net>, sam@ravnborg.org,
       dhowells@redhat.com
Subject: Re: [PATCH] BUG(): CONFIG_BUG=n version of BUG() should be unreachable()
References: <1261531032-15225-1-git-send-email-a.beregalov@gmail.com> <4B3171DF.4070903@caviumnetworks.com>
In-Reply-To: <4B3171DF.4070903@caviumnetworks.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2011
Lines: 72

David Daney wrote:
> Alexander Beregalov wrote:
>> Previouss definition of BUG() as 'do {} while(0)' produced compilation
>> warnings when BUG() was used in default branch of switch() statement
>> (control reaches end of non-void function).
>>
>> Example:
>> unsigned long function()
>> {
>>     switch() {
>>     case 1:
>>         return 1;
>>     case 2:
>>         return 2;
>>     default:
>>         BUG();
>> }
>>
>> Using unreachable() fixes the problem.
>>
>> Signed-off-by: Alexander Beregalov <a.beregalov@gmail.com>
>>
> 
> NAK.
> 

Well that may be too strong an objection, but I would really recommend 
deeper consideration.

If you use: #define BUG() __builtin_unreachable()

which is what your patch does for GCC >= 4.5, it is truly undefined what 
happens if it is ever reached.  One typical thing that might happen is 
that you start to try to execute data.  It is unclear to me if it is 
preferable in the kernel to do that, rather than loop endlessly.  You 
would likely achieve smaller code, but at what cost?

David Daney

> 
>> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>> index 18c435d..1106439 100644
>> --- a/include/asm-generic/bug.h
>> +++ b/include/asm-generic/bug.h
>> @@ -89,7 +89,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
> 
> You can only use 'unreachable()' in situations where it is truly 
> unreachable.  In the case above you will reach it in the default case.
> 
> I would suggest one of the following instead:
> 
> #define BUG() BUILD_BUG_ON(1)
> 
> #define BUG() do {} while(1)
> 
> 
> David Daney
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/