[PATCH 0/1] bug: mark generic BUG() as unreachable

[PATCH 0/1] bug: mark generic BUG() as unreachable

[trix]

From: Tom Rix <trix@redhat.com>

This is a followup patch discussed here
https://lore.kernel.org/lkml/162375813191.653958.11993495571264748407.stgit@warthog.procyon.org.uk/

Arches that defined BUG() were reviewed either by inspection or
by cross building the fs/afs/dir.c reproducer, all lotm.

There was no change to the assembly for the ppc64 reproducer.

Tom Rix (1):
  bug: mark generic BUG() as unreachable

 include/asm-generic/bug.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.26.3

[PATCH 1/1] bug: mark generic BUG() as unreachable

[trix]

From: Tom Rix <trix@redhat.com>

This spurious error is reported for powerpc64, CONFIG_BUG=n

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]
   51 | }
      | ^

When CONFIG_BUG=y is BUG is expanded from
  #define BUG() do {
 	BUG_ENTRY("twi 31, 0, 0", 0);
 	unreachable();
   } while (0)

to

static int afs_dir_set_page_dirty(struct page *page)
{
 do { __asm__ __volatile__( "1:	" "twi 31, 0, 0"  ...
   do { ; asm volatile(""); __builtin_unreachable(); } while (0);
 } while (0);
}

When CONFIG_BUG=n, the generic BUG() is used which
expands out to

static int afs_dir_set_page_dirty(struct page *page)
{
 do {} while (1);
}

Without the __builtin_unreachable(), gcc reports the
warning

ref: gcc docs https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
" ... without the __builtin_unreachable, GCC issues a
  warning that control reaches the end of a non-void function."

So add an unreachable() to the generic BUG(), the resulting
expansiion will be

static int afs_dir_set_page_dirty(struct page *page)
{
 do {
   do {} while (1);
   do { ; asm volatile(""); __builtin_unreachable(); } while (0);
 } while (0);
}

Signed-off-by: Tom Rix <trix@redhat.com>
---
 include/asm-generic/bug.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index f152b9bb916fc..b250e06d7de26 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -177,7 +177,10 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while (1)
+#define BUG() do {						\
+		do {} while (1);				\
+		unreachable();					\
+	} while (0)
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-- 
2.26.3

Re: [PATCH 1/1] bug: mark generic BUG() as unreachable

[Arnd Bergmann]

On Thu, Jun 17, 2021 at 11:44 PM <trix@redhat.com> wrote:
> From: Tom Rix <trix@redhat.com>
>
> This spurious error is reported for powerpc64, CONFIG_BUG=n
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index f152b9bb916fc..b250e06d7de26 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -177,7 +177,10 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
>
>  #else /* !CONFIG_BUG */
>  #ifndef HAVE_ARCH_BUG
> -#define BUG() do {} while (1)
> +#define BUG() do {                                             \
> +               do {} while (1);                                \
> +               unreachable();                                  \
> +       } while (0)
>  #endif

Please let's not go back to this version, we had good reasons to use
the infinite loop,
mostly to avoid undefined behavior that would lead to the compiler producing
completely random output in code paths that lead to a BUG() statement. Those
do cause other kinds of warnings from objtool and from other compilers.

The obvious workaround here would be to add a return statement locally, but
it may also help to figure out what exactly triggers the warning, as I don't see
it in my randconfig builds and it may be that there is a bug elsewhere.

I've tried a simple reproducer on https://godbolt.org/z/341P949bG that did not
show this warning in any of the compilers I tried. Can you try to narrow down
the exact compiler versions and commmand line options that produce the
warning? https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/ has
most of the supported gcc versions in case you need those.

      Arnd

Re: [PATCH 1/1] bug: mark generic BUG() as unreachable

[Tom Rix]

On 6/18/21 1:20 AM, Arnd Bergmann wrote:
> On Thu, Jun 17, 2021 at 11:44 PM <trix@redhat.com> wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> This spurious error is reported for powerpc64, CONFIG_BUG=n
>>
>> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>> index f152b9bb916fc..b250e06d7de26 100644
>> --- a/include/asm-generic/bug.h
>> +++ b/include/asm-generic/bug.h
>> @@ -177,7 +177,10 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
>>
>>   #else /* !CONFIG_BUG */
>>   #ifndef HAVE_ARCH_BUG
>> -#define BUG() do {} while (1)
>> +#define BUG() do {                                             \
>> +               do {} while (1);                                \
>> +               unreachable();                                  \
>> +       } while (0)
>>   #endif
> Please let's not go back to this version, we had good reasons to use
> the infinite loop,
> mostly to avoid undefined behavior that would lead to the compiler producing
> completely random output in code paths that lead to a BUG() statement. Those
> do cause other kinds of warnings from objtool and from other compilers.
>
> The obvious workaround here would be to add a return statement locally, but
> it may also help to figure out what exactly triggers the warning, as I don't see
> it in my randconfig builds and it may be that there is a bug elsewhere.
>
> I've tried a simple reproducer on https://godbolt.org/z/341P949bG that did not
> show this warning in any of the compilers I tried. Can you try to narrow down
> the exact compiler versions and commmand line options that produce the
> warning? https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/ has
> most of the supported gcc versions in case you need those.

Please follow the link in the cover letter to the original issue 
reported for fs/afs/dir + gcc ppc64 9.x / 10.3.1

Adding the return was the first, rejected solution.

Tom

>
>        Arnd
>

Re: [PATCH 1/1] bug: mark generic BUG() as unreachable

[Arnd Bergmann]

On Fri, Jun 18, 2021 at 3:43 PM Tom Rix <trix@redhat.com> wrote:
> On 6/18/21 1:20 AM, Arnd Bergmann wrote:
> > On Thu, Jun 17, 2021 at 11:44 PM <trix@redhat.com> wrote:
> >> From: Tom Rix <trix@redhat.com>
> >>
> >> This spurious error is reported for powerpc64, CONFIG_BUG=n
> >>
> >> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> >> index f152b9bb916fc..b250e06d7de26 100644
> >> --- a/include/asm-generic/bug.h
> >> +++ b/include/asm-generic/bug.h
> >> @@ -177,7 +177,10 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
> >>
> >>   #else /* !CONFIG_BUG */
> >>   #ifndef HAVE_ARCH_BUG
> >> -#define BUG() do {} while (1)
> >> +#define BUG() do {                                             \
> >> +               do {} while (1);                                \
> >> +               unreachable();                                  \
> >> +       } while (0)
> >>   #endif
> > Please let's not go back to this version, we had good reasons to use
> > the infinite loop,
> > mostly to avoid undefined behavior that would lead to the compiler producing
> > completely random output in code paths that lead to a BUG() statement. Those
> > do cause other kinds of warnings from objtool and from other compilers.
> >
> > The obvious workaround here would be to add a return statement locally, but
> > it may also help to figure out what exactly triggers the warning, as I don't see
> > it in my randconfig builds and it may be that there is a bug elsewhere.
> >
> > I've tried a simple reproducer on https://godbolt.org/z/341P949bG that did not
> > show this warning in any of the compilers I tried. Can you try to narrow down
> > the exact compiler versions and commmand line options that produce the
> > warning? https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/ has
> > most of the supported gcc versions in case you need those.
>
> Please follow the link in the cover letter to the original issue
> reported for fs/afs/dir + gcc ppc64 9.x / 10.3.1
>
> Adding the return was the first, rejected solution.

Ok, I was able to reproduce it now and have a better idea of what is going on.

I also misread your patch (sorry about that), and missed that you keep the
"do {} while (1)" loop ahead of the unreachable(), and that should address
all the concerns I had. I would still try to find the old email thread about
the change, just to make sure we don't already know about other problems
with your version.

      Arnd