All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] afs: fix no return statement in function returning non-void
@ 2021-06-15 11:55 David Howells
  2021-06-15 12:03 ` David Howells
  2021-06-15 14:49 ` Linus Torvalds
  0 siblings, 2 replies; 19+ messages in thread
From: David Howells @ 2021-06-15 11:55 UTC (permalink / raw)
  To: torvalds
  Cc: Hulk Robot, Zheng Zengkai, Randy Dunlap, Tom Rix, linux-afs,
	dhowells, marc.dionne, linux-afs, linux-fsdevel, linux-kernel

From: Zheng Zengkai <zhengzengkai@huawei.com>

Add missing return to fix following compilation issue:

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]

Fixes: f3ddee8dc4e2 ("afs: Fix directory handling")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Zheng Zengkai <zhengzengkai@huawei.com>
Acked-by: Randy Dunlap <rdunlap@infradead.org>
cc: Tom Rix <trix@redhat.com>
cc: linux-afs@lists.infradead.org
Link: https://lore.kernel.org/r/20210327121624.194639-1-zhengzengkai@huawei.com/
---

 fs/afs/dir.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 78719f2f567e..c31c21afd2e1 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -48,6 +48,7 @@ static void afs_dir_invalidatepage(struct page *page, unsigned int offset,
 static int afs_dir_set_page_dirty(struct page *page)
 {
 	BUG(); /* This should never happen. */
+	return 0;
 }
 
 const struct file_operations afs_dir_file_operations = {



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-15 11:55 [PATCH] afs: fix no return statement in function returning non-void David Howells
@ 2021-06-15 12:03 ` David Howells
  2021-06-15 14:49 ` Linus Torvalds
  1 sibling, 0 replies; 19+ messages in thread
From: David Howells @ 2021-06-15 12:03 UTC (permalink / raw)
  To: torvalds
  Cc: dhowells, Hulk Robot, Zheng Zengkai, Randy Dunlap, Tom Rix,
	linux-afs, marc.dionne, linux-fsdevel, linux-kernel

Hi Linus,

Note that this isn't really a fix, so can wait to the next merge window if you
prefer.

David


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-15 11:55 [PATCH] afs: fix no return statement in function returning non-void David Howells
  2021-06-15 12:03 ` David Howells
@ 2021-06-15 14:49 ` Linus Torvalds
  2021-06-15 23:58   ` Randy Dunlap
  1 sibling, 1 reply; 19+ messages in thread
From: Linus Torvalds @ 2021-06-15 14:49 UTC (permalink / raw)
  To: David Howells
  Cc: Hulk Robot, Zheng Zengkai, Randy Dunlap, Tom Rix, linux-afs,
	Marc Dionne, linux-fsdevel, Linux Kernel Mailing List

On Tue, Jun 15, 2021 at 4:55 AM David Howells <dhowells@redhat.com> wrote:
>
> From: Zheng Zengkai <zhengzengkai@huawei.com>
>
> Add missing return to fix following compilation issue:
>
> 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]

This warning is actively wrong, and the patch is the wrong thing to do.

What compiler / architecture / config?

Because BUG() should have an "unreachable()", and the compiler should
know that a return statement isn't needed (and adding it shouldn't
make any difference).

And it's not warning for me when I build that code. So I really think
the real bug is entirely somewhere else, and this patch is papering
over the real problem.

               Linus

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-15 14:49 ` Linus Torvalds
@ 2021-06-15 23:58   ` Randy Dunlap
  2021-06-16  0:32     ` Linus Torvalds
  0 siblings, 1 reply; 19+ messages in thread
From: Randy Dunlap @ 2021-06-15 23:58 UTC (permalink / raw)
  To: Linus Torvalds, David Howells
  Cc: Hulk Robot, Zheng Zengkai, Tom Rix, linux-afs, Marc Dionne,
	linux-fsdevel, Linux Kernel Mailing List

On 6/15/21 7:49 AM, Linus Torvalds wrote:
> On Tue, Jun 15, 2021 at 4:55 AM David Howells <dhowells@redhat.com> wrote:
>>
>> From: Zheng Zengkai <zhengzengkai@huawei.com>
>>
>> Add missing return to fix following compilation issue:
>>
>> 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]
> 
> This warning is actively wrong, and the patch is the wrong thing to do.
> 
> What compiler / architecture / config?
> 
> Because BUG() should have an "unreachable()", and the compiler should
> know that a return statement isn't needed (and adding it shouldn't
> make any difference).
> 
> And it's not warning for me when I build that code. So I really think
> the real bug is entirely somewhere else, and this patch is papering
> over the real problem.

Hi,

Some implementations of BUG() are macros, not functions, so "unreachable"
is not applicable AFAIK.


-- 
~Randy


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-15 23:58   ` Randy Dunlap
@ 2021-06-16  0:32     ` Linus Torvalds
  2021-06-16  1:38       ` Randy Dunlap
  0 siblings, 1 reply; 19+ messages in thread
From: Linus Torvalds @ 2021-06-16  0:32 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: David Howells, Hulk Robot, Zheng Zengkai, Tom Rix, linux-afs,
	Marc Dionne, linux-fsdevel, Linux Kernel Mailing List

On Tue, Jun 15, 2021 at 4:58 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> Some implementations of BUG() are macros, not functions,

Not "some", I think. Most.

> so "unreachable" is not applicable AFAIK.

Sure it is. One common pattern is the x86 one:

  #define BUG()                                                   \
  do {                                                            \
          instrumentation_begin();                                \
          _BUG_FLAGS(ASM_UD2, 0);                                 \
          unreachable();                                          \
  } while (0)

and that "unreachable()" is exactly what I'm talking about.

So I repeat: what completely broken compiler / config / architecture
is it that needs that "return 0" after a BUG() statement?

Because that environment is broken, and the warning is bogus and wrong.

It might not be the compiler. It might be some architecture that does
this wrong. It might be some very particular configuration that does
something bad and makes the "unreachable()" not work (or not exist).

But *that* is the bug that should be fixed. Not adding a pointless and
incorrect line that makes no sense, just to hide the real bug.

               Linus

                 Linus

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-16  0:32     ` Linus Torvalds
@ 2021-06-16  1:38       ` Randy Dunlap
  2021-06-16  2:19         ` Randy Dunlap
  2021-06-16  3:15         ` Zheng Zengkai
  0 siblings, 2 replies; 19+ messages in thread
From: Randy Dunlap @ 2021-06-16  1:38 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: David Howells, Hulk Robot, Zheng Zengkai, Tom Rix, linux-afs,
	Marc Dionne, linux-fsdevel, Linux Kernel Mailing List

On 6/15/21 5:32 PM, Linus Torvalds wrote:
> On Tue, Jun 15, 2021 at 4:58 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>> Some implementations of BUG() are macros, not functions,
> 
> Not "some", I think. Most.
> 
>> so "unreachable" is not applicable AFAIK.
> 
> Sure it is. One common pattern is the x86 one:
> 
>   #define BUG()                                                   \
>   do {                                                            \
>           instrumentation_begin();                                \
>           _BUG_FLAGS(ASM_UD2, 0);                                 \
>           unreachable();                                          \
>   } while (0)

duh.

> and that "unreachable()" is exactly what I'm talking about.
> 
> So I repeat: what completely broken compiler / config / architecture
> is it that needs that "return 0" after a BUG() statement?

I have seen it on ia64 -- most likely GCC 9.3.0, but I'll have to
double check that.

> Because that environment is broken, and the warning is bogus and wrong.
> 
> It might not be the compiler. It might be some architecture that does
> this wrong. It might be some very particular configuration that does
> something bad and makes the "unreachable()" not work (or not exist).
> 
> But *that* is the bug that should be fixed. Not adding a pointless and
> incorrect line that makes no sense, just to hide the real bug.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-16  1:38       ` Randy Dunlap
@ 2021-06-16  2:19         ` Randy Dunlap
  2021-06-16  3:15         ` Zheng Zengkai
  1 sibling, 0 replies; 19+ messages in thread
From: Randy Dunlap @ 2021-06-16  2:19 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: David Howells, Hulk Robot, Zheng Zengkai, Tom Rix, linux-afs,
	Marc Dionne, linux-fsdevel, Linux Kernel Mailing List

On 6/15/21 6:38 PM, Randy Dunlap wrote:
> On 6/15/21 5:32 PM, Linus Torvalds wrote:
>> On Tue, Jun 15, 2021 at 4:58 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>>
>>> Some implementations of BUG() are macros, not functions,
>>
>> Not "some", I think. Most.
>>
>>> so "unreachable" is not applicable AFAIK.
>>
>> Sure it is. One common pattern is the x86 one:
>>
>>   #define BUG()                                                   \
>>   do {                                                            \
>>           instrumentation_begin();                                \
>>           _BUG_FLAGS(ASM_UD2, 0);                                 \
>>           unreachable();                                          \
>>   } while (0)
> 
> duh.
> 
>> and that "unreachable()" is exactly what I'm talking about.
>>
>> So I repeat: what completely broken compiler / config / architecture
>> is it that needs that "return 0" after a BUG() statement?
> 
> I have seen it on ia64 -- most likely GCC 9.3.0, but I'll have to
> double check that.

Nope, I cannot repro that now. I'll check a few more arches...

>> Because that environment is broken, and the warning is bogus and wrong.
>>
>> It might not be the compiler. It might be some architecture that does
>> this wrong. It might be some very particular configuration that does
>> something bad and makes the "unreachable()" not work (or not exist).
>>
>> But *that* is the bug that should be fixed. Not adding a pointless and
>> incorrect line that makes no sense, just to hide the real bug.


-- 
~Randy


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-16  1:38       ` Randy Dunlap
  2021-06-16  2:19         ` Randy Dunlap
@ 2021-06-16  3:15         ` Zheng Zengkai
  2021-06-16 12:56           ` Tom Rix
  2021-06-16 13:41           ` David Howells
  1 sibling, 2 replies; 19+ messages in thread
From: Zheng Zengkai @ 2021-06-16  3:15 UTC (permalink / raw)
  To: Randy Dunlap, Linus Torvalds
  Cc: David Howells, Hulk Robot, Tom Rix, linux-afs, Marc Dionne,
	linux-fsdevel, Linux Kernel Mailing List

Oops, Sorry for the late reply and missing the compilation details.

> On 6/15/21 5:32 PM, Linus Torvalds wrote:
>> On Tue, Jun 15, 2021 at 4:58 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>> Some implementations of BUG() are macros, not functions,
>> Not "some", I think. Most.
>>
>>> so "unreachable" is not applicable AFAIK.
>> Sure it is. One common pattern is the x86 one:
>>
>>    #define BUG()                                                   \
>>    do {                                                            \
>>            instrumentation_begin();                                \
>>            _BUG_FLAGS(ASM_UD2, 0);                                 \
>>            unreachable();                                          \
>>    } while (0)
> duh.
>
>> and that "unreachable()" is exactly what I'm talking about.
>>
>> So I repeat: what completely broken compiler / config / architecture
>> is it that needs that "return 0" after a BUG() statement?
> I have seen it on ia64 -- most likely GCC 9.3.0, but I'll have to
> double check that.

Actually we build the kernel with the following compiler, config and 
architecture :

powerpc64-linux-gnu-gcc --version
powerpc64-linux-gnu-gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

CONFIG_AFS_FS=y
# CONFIG_AFS_DEBUG is not set
CONFIG_AFS_DEBUG_CURSOR=y

make ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu- -j64

...

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 | }
       | ^
cc1: some warnings being treated as errors

>> Because that environment is broken, and the warning is bogus and wrong.
>>
>> It might not be the compiler. It might be some architecture that does
>> this wrong. It might be some very particular configuration that does
>> something bad and makes the "unreachable()" not work (or not exist).
>>
>> But *that* is the bug that should be fixed. Not adding a pointless and
>> incorrect line that makes no sense, just to hide the real bug.
> .
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-16  3:15         ` Zheng Zengkai
@ 2021-06-16 12:56           ` Tom Rix
  2021-06-16 14:34             ` Linus Torvalds
  2021-06-16 13:41           ` David Howells
  1 sibling, 1 reply; 19+ messages in thread
From: Tom Rix @ 2021-06-16 12:56 UTC (permalink / raw)
  To: Zheng Zengkai, Randy Dunlap, Linus Torvalds
  Cc: David Howells, Hulk Robot, linux-afs, Marc Dionne, linux-fsdevel,
	Linux Kernel Mailing List


On 6/15/21 8:15 PM, Zheng Zengkai wrote:
> Oops, Sorry for the late reply and missing the compilation details.
>
>> On 6/15/21 5:32 PM, Linus Torvalds wrote:
>>> On Tue, Jun 15, 2021 at 4:58 PM Randy Dunlap <rdunlap@infradead.org> 
>>> wrote:
>>>> Some implementations of BUG() are macros, not functions,
>>> Not "some", I think. Most.
>>>
>>>> so "unreachable" is not applicable AFAIK.
>>> Sure it is. One common pattern is the x86 one:
>>>
>>>    #define BUG()                                                   \
>>>    do {                                                            \
>>> instrumentation_begin();                                \
>>>            _BUG_FLAGS(ASM_UD2, 0);                                 \
>>> unreachable();                                          \
>>>    } while (0)
>> duh.
>>
>>> and that "unreachable()" is exactly what I'm talking about.
>>>
>>> So I repeat: what completely broken compiler / config / architecture
>>> is it that needs that "return 0" after a BUG() statement?
>> I have seen it on ia64 -- most likely GCC 9.3.0, but I'll have to
>> double check that.
>
> Actually we build the kernel with the following compiler, config and 
> architecture :
>
> powerpc64-linux-gnu-gcc --version
> powerpc64-linux-gnu-gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
> Copyright (C) 2019 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR 
> PURPOSE.
>
> CONFIG_AFS_FS=y
> # CONFIG_AFS_DEBUG is not set
> CONFIG_AFS_DEBUG_CURSOR=y
>
> make ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu- -j64
>
> ...
>
> 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 | }
>       | ^
> cc1: some warnings being treated as errors
>
powerpc64 gcc 10.3.1 is what I used to find this problem.

A fix is to use the __noreturn attribute on this function and not add a 
return like this

-static int afs_dir_set_page_dirty(struct page *page)
+static int __noreturn afs_dir_set_page_dirty(struct page *page)

and to the set of ~300 similar functions in these files.

$ grep -r -P "^\tBUG\(\)" .

If folks are ok with this, I'll get that set started.

Tom

>>> Because that environment is broken, and the warning is bogus and wrong.
>>>
>>> It might not be the compiler. It might be some architecture that does
>>> this wrong. It might be some very particular configuration that does
>>> something bad and makes the "unreachable()" not work (or not exist).
>>>
>>> But *that* is the bug that should be fixed. Not adding a pointless and
>>> incorrect line that makes no sense, just to hide the real bug.
>> .
>>
>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-16  3:15         ` Zheng Zengkai
  2021-06-16 12:56           ` Tom Rix
@ 2021-06-16 13:41           ` David Howells
  1 sibling, 0 replies; 19+ messages in thread
From: David Howells @ 2021-06-16 13:41 UTC (permalink / raw)
  To: Tom Rix
  Cc: dhowells, Zheng Zengkai, Randy Dunlap, Linus Torvalds,
	Hulk Robot, linux-afs, Marc Dionne, linux-fsdevel,
	Linux Kernel Mailing List

Tom Rix <trix@redhat.com> wrote:

> A fix is to use the __noreturn attribute on this function and not add a return
> like this
> 
> -static int afs_dir_set_page_dirty(struct page *page)
> +static int __noreturn afs_dir_set_page_dirty(struct page *page)
> 
> and to the set of ~300 similar functions in these files.

BUG() really ought to handle it.  Do you have CONFIG_BUG=y?

David


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-16 12:56           ` Tom Rix
@ 2021-06-16 14:34             ` Linus Torvalds
  2021-06-16 16:22               ` Tom Rix
  0 siblings, 1 reply; 19+ messages in thread
From: Linus Torvalds @ 2021-06-16 14:34 UTC (permalink / raw)
  To: Tom Rix
  Cc: Zheng Zengkai, Randy Dunlap, David Howells, Hulk Robot,
	linux-afs, Marc Dionne, linux-fsdevel, Linux Kernel Mailing List

On Wed, Jun 16, 2021 at 5:56 AM Tom Rix <trix@redhat.com> wrote:
>
> A fix is to use the __noreturn attribute on this function

That's certainly a better thing. It would be better yet to figure out
why BUG() didn't do it automatically.

Without CONFIG_BUG, it looks like powerpc picks up

  #ifndef HAVE_ARCH_BUG
  #define BUG() do {} while (1)

which should still make it pointless to have the return.  But I might
have missed something.

             Linus

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-16 14:34             ` Linus Torvalds
@ 2021-06-16 16:22               ` Tom Rix
  2021-06-16 16:29                 ` Linus Torvalds
  0 siblings, 1 reply; 19+ messages in thread
From: Tom Rix @ 2021-06-16 16:22 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Zheng Zengkai, Randy Dunlap, David Howells, Hulk Robot,
	linux-afs, Marc Dionne, linux-fsdevel, Linux Kernel Mailing List


On 6/16/21 7:34 AM, Linus Torvalds wrote:
> On Wed, Jun 16, 2021 at 5:56 AM Tom Rix <trix@redhat.com> wrote:
>> A fix is to use the __noreturn attribute on this function
> That's certainly a better thing. It would be better yet to figure out
> why BUG() didn't do it automatically.
>
> Without CONFIG_BUG, it looks like powerpc picks up
>
>    #ifndef HAVE_ARCH_BUG
>    #define BUG() do {} while (1)
>
> which should still make it pointless to have the return.  But I might
> have missed something.

This looks like a problem the generic BUG().

with CONFIG_BUG=y, the *.i is

static int afs_dir_set_page_dirty(struct page *page)
{
  do { __asm__ __volatile__( "1:    " "twi 31, 0, 0" "\n" ".section 
__bug_table,\"aw\"\n" "2:\t.4byte 1b - 2b, %0 - 2b\n" "\t.short %1, 
%2\n" ".org 2b+%3\n" ".previous\n" : : "i" ("fs/afs/dir.c"), "i" (50), 
"i" (0), "i" (sizeof(struct bug_entry))); do { ; asm volatile(""); 
__builtin_unreachable(); } while (0); } while (0);
}
BUG() expanded from
#define BUG() do {                        \
     BUG_ENTRY("twi 31, 0, 0", 0);                \
     unreachable();                        \
} while (0)


with CONFIG_BUG=n, the *.i is

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

BUG() expanded from
  do {} while (1)

to fix, add an unreachable() to the generic BUG()

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index f152b9bb916f..b250e06d7de2 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

the new *.i

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

The assembly is unchanged.

The key being the unreachable builtin

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."

Tom

>
>               Linus
>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-16 16:22               ` Tom Rix
@ 2021-06-16 16:29                 ` Linus Torvalds
  2021-06-18 15:23                   ` Arnd Bergmann
  0 siblings, 1 reply; 19+ messages in thread
From: Linus Torvalds @ 2021-06-16 16:29 UTC (permalink / raw)
  To: Tom Rix
  Cc: Zheng Zengkai, Randy Dunlap, David Howells, Hulk Robot,
	linux-afs, Marc Dionne, linux-fsdevel, Linux Kernel Mailing List

On Wed, Jun 16, 2021 at 9:22 AM Tom Rix <trix@redhat.com> wrote:
>
> to fix, add an unreachable() to the generic BUG()
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index f152b9bb916f..b250e06d7de2 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

I'm a bit surprised that the compiler doesn't make that code after an
infinite loop automatically be marked "unreachable". But at the same I
can imagine the compiler doing some checks without doing real flow
analysis, and doing "oh, that conditional branch is unconditional".

So this patch at least makes sense to me and I have no objections to
it, even if it makes me go "silly compiler, we shouldn't have to tell
you this".

So Ack from me on this.

           Linus

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-06-16 16:29                 ` Linus Torvalds
@ 2021-06-18 15:23                   ` Arnd Bergmann
  0 siblings, 0 replies; 19+ messages in thread
From: Arnd Bergmann @ 2021-06-18 15:23 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Tom Rix, Zheng Zengkai, Randy Dunlap, David Howells, Hulk Robot,
	linux-afs, Marc Dionne, linux-fsdevel, Linux Kernel Mailing List

On Thu, Jun 17, 2021 at 12:51 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Jun 16, 2021 at 9:22 AM Tom Rix <trix@redhat.com> wrote:
> >
> > to fix, add an unreachable() to the generic BUG()
> >
> > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> > index f152b9bb916f..b250e06d7de2 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
>
> I'm a bit surprised that the compiler doesn't make that code after an
> infinite loop automatically be marked "unreachable". But at the same I
> can imagine the compiler doing some checks without doing real flow
> analysis, and doing "oh, that conditional branch is unconditional".
>
> So this patch at least makes sense to me and I have no objections to
> it, even if it makes me go "silly compiler, we shouldn't have to tell
> you this".
>
> So Ack from me on this.

I've tried to figure out what the compiler is trying to do here, and it's
still weird. When I saw the patch posted, I misread it as having just
unreachable() without the loop, and that would have been bad
because it triggers undefined behavior.

What I found is a minimal test case of

static int f(void)
{
   do {} while (1);
}

to trigger the warning with any version of gcc (not clang), but none of
these other variations cause a warning:

 // not static -> no warning!
int f(void)
{
   do {} while (1);
}

// some return statement anywhere in the function, no warning
static int f(int i)
{
  if (i)
      return 0;
   do {} while (1);
}

// annotated as never returning, as discussed in this thread
static int __attribute__((noreturn)) f(void)
{
   do {} while (1);
}

// unreachable annotation, as suggested by Tom
static int f(void)
{
   do {} while (1);
   __builtin_unreachable();
}

The last three are obviously intentional, as the warning is only for functions
that can *never* return but lack an annotation. I have no idea why the warning
is only for static functions though.

All my randconfig builds for arm/arm64/x86 missed this problem since those
architectures have a custom BUG() implementation with an inline asm.
I've taken them out now and found only two other instances of the issue so far:
arbitrary_virt_to_machine() and ppc64 get_hugepd_cache_index(). My preference
would be to annotate these as __noreturn, but change to the asm-generic
BUG() is probably better.

        Arnd

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-04-19 22:31   ` Randy Dunlap
@ 2021-05-27 19:48     ` Randy Dunlap
  0 siblings, 0 replies; 19+ messages in thread
From: Randy Dunlap @ 2021-05-27 19:48 UTC (permalink / raw)
  To: David Howells, Zheng Zengkai; +Cc: linux-afs, linux-kernel, huawei.libin

On 4/19/21 3:31 PM, Randy Dunlap wrote:
> On 4/8/21 7:06 AM, David Howells wrote:
>> Zheng Zengkai <zhengzengkai@huawei.com> wrote:
>>
>>>  static int afs_dir_set_page_dirty(struct page *page)
>>>  {
>>>  	BUG(); /* This should never happen. */
>>> +	return 0;
>>>  }
>>
>> That shouldn't be necessary.  BUG() should be marked as 'no return' to the
>> compiler.  What arch and compiler are you using?
> 
> How do mark a #define BUG() as __noreturn?
> 
> Several arch-es use #define for BUG() instead of using a function.

Hi David,

So you are counting of BUG() being a function and not a macro?

Doesn't seem like a good idea.

-- 
~Randy


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-04-08 14:06 ` David Howells
@ 2021-04-19 22:31   ` Randy Dunlap
  2021-05-27 19:48     ` Randy Dunlap
  0 siblings, 1 reply; 19+ messages in thread
From: Randy Dunlap @ 2021-04-19 22:31 UTC (permalink / raw)
  To: David Howells, Zheng Zengkai; +Cc: linux-afs, linux-kernel, huawei.libin

On 4/8/21 7:06 AM, David Howells wrote:
> Zheng Zengkai <zhengzengkai@huawei.com> wrote:
> 
>>  static int afs_dir_set_page_dirty(struct page *page)
>>  {
>>  	BUG(); /* This should never happen. */
>> +	return 0;
>>  }
> 
> That shouldn't be necessary.  BUG() should be marked as 'no return' to the
> compiler.  What arch and compiler are you using?

How do mark a #define BUG() as __noreturn?

Several arch-es use #define for BUG() instead of using a function.

-- 
~Randy


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-03-27 12:16 Zheng Zengkai
  2021-03-31  2:32 ` Zheng Zengkai
@ 2021-04-08 14:06 ` David Howells
  2021-04-19 22:31   ` Randy Dunlap
  1 sibling, 1 reply; 19+ messages in thread
From: David Howells @ 2021-04-08 14:06 UTC (permalink / raw)
  To: Zheng Zengkai; +Cc: dhowells, linux-afs, linux-kernel, huawei.libin

Zheng Zengkai <zhengzengkai@huawei.com> wrote:

>  static int afs_dir_set_page_dirty(struct page *page)
>  {
>  	BUG(); /* This should never happen. */
> +	return 0;
>  }

That shouldn't be necessary.  BUG() should be marked as 'no return' to the
compiler.  What arch and compiler are you using?

David


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] afs: fix no return statement in function returning non-void
  2021-03-27 12:16 Zheng Zengkai
@ 2021-03-31  2:32 ` Zheng Zengkai
  2021-04-08 14:06 ` David Howells
  1 sibling, 0 replies; 19+ messages in thread
From: Zheng Zengkai @ 2021-03-31  2:32 UTC (permalink / raw)
  To: dhowells, linux-afs, linux-kernel; +Cc: huawei.libin

Hi David and Reviewers,

> Add missing return to fix following compilation issue:
>
> 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]
>
> Fixes: f3ddee8dc4e2 ("afs: Fix directory handling")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Zheng Zengkai <zhengzengkai@huawei.com>
> ---
>   fs/afs/dir.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index 17548c1faf02..1795a05b7cb7 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c
> @@ -48,6 +48,7 @@ static void afs_dir_invalidatepage(struct page *page, unsigned int offset,
>   static int afs_dir_set_page_dirty(struct page *page)
>   {
>   	BUG(); /* This should never happen. */
> +	return 0;
>   }
>   
>   const struct file_operations afs_dir_file_operations = {

Is there anyone who can take a look?  ;-)

Thanks!


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] afs: fix no return statement in function returning non-void
@ 2021-03-27 12:16 Zheng Zengkai
  2021-03-31  2:32 ` Zheng Zengkai
  2021-04-08 14:06 ` David Howells
  0 siblings, 2 replies; 19+ messages in thread
From: Zheng Zengkai @ 2021-03-27 12:16 UTC (permalink / raw)
  To: dhowells, linux-afs, linux-kernel; +Cc: zhengzengkai, huawei.libin

Add missing return to fix following compilation issue:

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]

Fixes: f3ddee8dc4e2 ("afs: Fix directory handling")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Zheng Zengkai <zhengzengkai@huawei.com>
---
 fs/afs/dir.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 17548c1faf02..1795a05b7cb7 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -48,6 +48,7 @@ static void afs_dir_invalidatepage(struct page *page, unsigned int offset,
 static int afs_dir_set_page_dirty(struct page *page)
 {
 	BUG(); /* This should never happen. */
+	return 0;
 }
 
 const struct file_operations afs_dir_file_operations = {
-- 
2.20.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-06-18 15:27 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-15 11:55 [PATCH] afs: fix no return statement in function returning non-void David Howells
2021-06-15 12:03 ` David Howells
2021-06-15 14:49 ` Linus Torvalds
2021-06-15 23:58   ` Randy Dunlap
2021-06-16  0:32     ` Linus Torvalds
2021-06-16  1:38       ` Randy Dunlap
2021-06-16  2:19         ` Randy Dunlap
2021-06-16  3:15         ` Zheng Zengkai
2021-06-16 12:56           ` Tom Rix
2021-06-16 14:34             ` Linus Torvalds
2021-06-16 16:22               ` Tom Rix
2021-06-16 16:29                 ` Linus Torvalds
2021-06-18 15:23                   ` Arnd Bergmann
2021-06-16 13:41           ` David Howells
  -- strict thread matches above, loose matches on Subject: below --
2021-03-27 12:16 Zheng Zengkai
2021-03-31  2:32 ` Zheng Zengkai
2021-04-08 14:06 ` David Howells
2021-04-19 22:31   ` Randy Dunlap
2021-05-27 19:48     ` Randy Dunlap

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.