[PATCH] asm-generic: bug.h: add unreachable() in BUG() for CONFIG_BUG not set

[PATCH] asm-generic: bug.h: add unreachable() in BUG() for CONFIG_BUG not set

[Randy Dunlap]

When CONFIG_BUG is not set/enabled, there is a warning
on ARCH=m68k, gcc version 11.1.0-nolibc from Arnd's crosstools:

../fs/afs/dir.c: In function 'afs_dir_set_page_dirty':
../fs/afs/dir.c:51:1: error: no return statement in function returning non-void [-Werror=return-type]

Adding "unreachable()" in the BUG() macro silences the warning.

Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arch@vger.kernel.org
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: linux-m68k@lists.linux-m68k.org
---
 include/asm-generic/bug.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-next-20211015.orig/include/asm-generic/bug.h
+++ linux-next-20211015/include/asm-generic/bug.h
@@ -154,7 +154,7 @@ void __warn(const char *file, int line,
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while (1)
+#define BUG() do {unreachable();} while (1)
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON

Re: [PATCH] asm-generic: bug.h: add unreachable() in BUG() for CONFIG_BUG not set

[Arnd Bergmann]

On Sun, Oct 17, 2021 at 7:49 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> When CONFIG_BUG is not set/enabled, there is a warning
> on ARCH=m68k, gcc version 11.1.0-nolibc from Arnd's crosstools:
>
> ../fs/afs/dir.c: In function 'afs_dir_set_page_dirty':
> ../fs/afs/dir.c:51:1: error: no return statement in function returning non-void [-Werror=return-type]
>
> Adding "unreachable()" in the BUG() macro silences the warning.

No, I don't think this is the right solution:

> -#define BUG() do {} while (1)
> +#define BUG() do {unreachable();} while (1)

Marking this code unreachable() means the compiler is free
to assume any code path leading here will never be entered,
which leads to additional undefined behavior and other warnings
rather than just hanging reproducibly.

The endless loop here should normally be sufficient to tell the
compiler that the function never returns, so it sounds like a
problem in gcc for m68k.

Did you see any other issues like this one on m68k, or the
same one on another architecture?

        Arnd

Re: [PATCH] asm-generic: bug.h: add unreachable() in BUG() for CONFIG_BUG not set

[Randy Dunlap]

On 10/17/21 12:09 PM, Arnd Bergmann wrote:
> On Sun, Oct 17, 2021 at 7:49 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>> When CONFIG_BUG is not set/enabled, there is a warning
>> on ARCH=m68k, gcc version 11.1.0-nolibc from Arnd's crosstools:
>>
>> ../fs/afs/dir.c: In function 'afs_dir_set_page_dirty':
>> ../fs/afs/dir.c:51:1: error: no return statement in function returning non-void [-Werror=return-type]
>>
>> Adding "unreachable()" in the BUG() macro silences the warning.
> 
> No, I don't think this is the right solution:
> 
>> -#define BUG() do {} while (1)
>> +#define BUG() do {unreachable();} while (1)
> 
> Marking this code unreachable() means the compiler is free
> to assume any code path leading here will never be entered,
> which leads to additional undefined behavior and other warnings
> rather than just hanging reproducibly.
> 
> The endless loop here should normally be sufficient to tell the
> compiler that the function never returns, so it sounds like a
> problem in gcc for m68k.

Sounds likely.

> Did you see any other issues like this one on m68k, or the
> same one on another architecture?

No and no.

thanks.
-- 
~Randy

Re: [PATCH] asm-generic: bug.h: add unreachable() in BUG() for CONFIG_BUG not set

[Arnd Bergmann]

On Sun, Oct 17, 2021 at 9:17 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> On 10/17/21 12:09 PM, Arnd Bergmann wrote:
> > On Sun, Oct 17, 2021 at 7:49 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> > Did you see any other issues like this one on m68k, or the
> > same one on another architecture?
>
> No and no.

Ok, maybe before we waste too much time on it, just add an extra
return statement to afs_dir_set_page_dirty()?

       Arnd

Re: [PATCH] asm-generic: bug.h: add unreachable() in BUG() for CONFIG_BUG not set

[Randy Dunlap]

On 10/17/21 12:24 PM, Arnd Bergmann wrote:
> On Sun, Oct 17, 2021 at 9:17 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>> On 10/17/21 12:09 PM, Arnd Bergmann wrote:
>>> On Sun, Oct 17, 2021 at 7:49 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>>> Did you see any other issues like this one on m68k, or the
>>> same one on another architecture?
>>
>> No and no.
> 
> Ok, maybe before we waste too much time on it, just add an extra
> return statement to afs_dir_set_page_dirty()?

I think that patch has already been rejected a few times...

-- 
~Randy

Re: [PATCH] asm-generic: bug.h: add unreachable() in BUG() for CONFIG_BUG not set

[Arnd Bergmann]

On Sun, Oct 17, 2021 at 9:27 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> On 10/17/21 12:24 PM, Arnd Bergmann wrote:
> > On Sun, Oct 17, 2021 at 9:17 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> >> On 10/17/21 12:09 PM, Arnd Bergmann wrote:
> >>> On Sun, Oct 17, 2021 at 7:49 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> >>
> >>> Did you see any other issues like this one on m68k, or the
> >>> same one on another architecture?
> >>
> >> No and no.
> >
> > Ok, maybe before we waste too much time on it, just add an extra
> > return statement to afs_dir_set_page_dirty()?
>
> I think that patch has already been rejected a few times...

Indeed, this is one I had looked at before:

https://lore.kernel.org/all/CAK8P3a3L6B9HXsOXSu9_c6pz1kN91Vig6EPsetLuYVW=M72XaQ@mail.gmail.com/

It seems that this version:

+#define BUG() do {                                             \
+               do {} while (1);                                \
+               unreachable();                                  \
+       } while (0)

ended up being one that didn't see any objections.

        Arnd

Re: [PATCH] asm-generic: bug.h: add unreachable() in BUG() for CONFIG_BUG not set

[Randy Dunlap]

On 10/17/21 12:38 PM, Arnd Bergmann wrote:
> On Sun, Oct 17, 2021 at 9:27 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>> On 10/17/21 12:24 PM, Arnd Bergmann wrote:
>>> On Sun, Oct 17, 2021 at 9:17 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>>> On 10/17/21 12:09 PM, Arnd Bergmann wrote:
>>>>> On Sun, Oct 17, 2021 at 7:49 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>>>
>>>>> Did you see any other issues like this one on m68k, or the
>>>>> same one on another architecture?
>>>>
>>>> No and no.
>>>
>>> Ok, maybe before we waste too much time on it, just add an extra
>>> return statement to afs_dir_set_page_dirty()?
>>
>> I think that patch has already been rejected a few times...
> 
> Indeed, this is one I had looked at before:
> 
> https://lore.kernel.org/all/CAK8P3a3L6B9HXsOXSu9_c6pz1kN91Vig6EPsetLuYVW=M72XaQ@mail.gmail.com/
> 
> It seems that this version:
> 
> +#define BUG() do {                                             \
> +               do {} while (1);                                \
> +               unreachable();                                  \
> +       } while (0)
> 
> ended up being one that didn't see any objections.

Yes, I was just thinking of a change like that while eating lunch. :)

-- 
~Randy