| Message ID | 20250409-mdb-max7360-support-v6-5-7a2535876e39@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for MAX7360 | expand |
On Wed, Apr 09, 2025 at 04:55:52PM +0200, Mathieu Dubois-Briand wrote:
> BUG() never returns, so code after it is unreachable: remove it.
BUG() can be compiled out, CONFIG_BUG.
On Wed, Apr 09, 2025 at 06:38:32PM +0300, Andy Shevchenko wrote: > On Wed, Apr 09, 2025 at 04:19:27PM +0100, Mark Brown wrote: > > BUG() can be compiled out, CONFIG_BUG. > Yes, and it's still has unreachable() there. So, this change is correct. > See include/asm-generic/bug.h for the details of the implementation. > And yes, if we have an architecture that does not do this way, it has to > be fixed. unreachable() just annotates things, AFAICT it doesn't actually guarantee to do anything in particular if the annotation turns out to be incorrect.
On Wed, Apr 09, 2025 at 04:46:04PM +0100, Mark Brown wrote: > On Wed, Apr 09, 2025 at 06:38:32PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 09, 2025 at 04:19:27PM +0100, Mark Brown wrote: > > > > BUG() can be compiled out, CONFIG_BUG. > > > Yes, and it's still has unreachable() there. So, this change is correct. > > See include/asm-generic/bug.h for the details of the implementation. > > And yes, if we have an architecture that does not do this way, it has to > > be fixed. > > unreachable() just annotates things, AFAICT it doesn't actually > guarantee to do anything in particular if the annotation turns out to be > incorrect. I;m not sure I follow. unreachable is a wrapper on top of __builtin_unreachable() which is intrinsic of the compiler. https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable
On Wed, Apr 09, 2025 at 07:00:24PM +0300, Andy Shevchenko wrote: > On Wed, Apr 09, 2025 at 04:46:04PM +0100, Mark Brown wrote: > > unreachable() just annotates things, AFAICT it doesn't actually > > guarantee to do anything in particular if the annotation turns out to be > > incorrect. > I;m not sure I follow. unreachable is a wrapper on top of > __builtin_unreachable() which is intrinsic of the compiler. > https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable That just says that the program is undefined if we get to the __builtin_undefined() and documents some behaviour around warnings. One example of undefined behaviour would be doing nothing.
On Wed, Apr 09, 2025 at 07:45:42PM +0300, Andy Shevchenko wrote: > On Wed, Apr 09, 2025 at 05:32:55PM +0100, Mark Brown wrote: > > On Wed, Apr 09, 2025 at 07:00:24PM +0300, Andy Shevchenko wrote: > > > On Wed, Apr 09, 2025 at 04:46:04PM +0100, Mark Brown wrote: > > > > > > unreachable() just annotates things, AFAICT it doesn't actually > > > > guarantee to do anything in particular if the annotation turns out to be > > > > incorrect. > > > > > I;m not sure I follow. unreachable is a wrapper on top of > > > __builtin_unreachable() which is intrinsic of the compiler. > > > > > https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable > > > > That just says that the program is undefined if we get to the > > __builtin_undefined() and documents some behaviour around warnings. One > > example of undefined behaviour would be doing nothing. > > Theoretically yes, practically return after a BUG() makes no sense. Note, > that compiler effectively removes 'goto exit;' here (that's also mentioned > in the documentation independently on the control flow behaviour), so > I don't know what you expect from it. So unreachable() sometimes lears to weird behavior from compiler, for example as mentioned here where we ended up removing it to prevent miscompilations: https://lore.kernel.org/all/20241010222451.GA3571761@thelio-3990X/ Thanks.
On Wed Apr 9, 2025 at 5:21 PM CEST, Mark Brown wrote: > On Wed, Apr 09, 2025 at 04:19:34PM +0100, Mark Brown wrote: >> On Wed, Apr 09, 2025 at 04:55:52PM +0200, Mathieu Dubois-Briand wrote: >> > BUG() never returns, so code after it is unreachable: remove it. >> >> BUG() can be compiled out, CONFIG_BUG. > > Also, please don't mix irrelevant patches into random serieses. It just > makes everything noisier for everyone. Hi Mark, Just to provide the context about why this change is part of this series: this goto, if left unmodified, would have to replaced by a return. This is how the topic of dropping it came in the previous iteration of this series. Thanks for your review. Mathieu
Wed, Apr 09, 2025 at 10:16:40AM -0700, Dmitry Torokhov kirjoitti: > On Wed, Apr 09, 2025 at 07:45:42PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 09, 2025 at 05:32:55PM +0100, Mark Brown wrote: > > > On Wed, Apr 09, 2025 at 07:00:24PM +0300, Andy Shevchenko wrote: > > > > On Wed, Apr 09, 2025 at 04:46:04PM +0100, Mark Brown wrote: > > > > > > > > unreachable() just annotates things, AFAICT it doesn't actually > > > > > guarantee to do anything in particular if the annotation turns out to be > > > > > incorrect. > > > > > > > I;m not sure I follow. unreachable is a wrapper on top of > > > > __builtin_unreachable() which is intrinsic of the compiler. > > > > > > > https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable > > > > > > That just says that the program is undefined if we get to the > > > __builtin_undefined() and documents some behaviour around warnings. One > > > example of undefined behaviour would be doing nothing. > > > > Theoretically yes, practically return after a BUG() makes no sense. Note, > > that compiler effectively removes 'goto exit;' here (that's also mentioned > > in the documentation independently on the control flow behaviour), so > > I don't know what you expect from it. > > So unreachable() sometimes lears to weird behavior from compiler, for > example as mentioned here where we ended up removing it to prevent > miscompilations: > > https://lore.kernel.org/all/20241010222451.GA3571761@thelio-3990X/ How does it affect the BUG()?
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 6c6869188c31..14f5fcc3ec1d 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -436,7 +436,6 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) break; default: BUG(); - goto exit; } }
BUG() never returns, so code after it is unreachable: remove it. Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> --- drivers/base/regmap/regmap-irq.c | 1 - 1 file changed, 1 deletion(-)