Re: [PATCH] afs: fix no return statement in function returning non-void

From: Tom Rix
Date: Wed Jun 16 2021 - 08:56:48 EST

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@xxxxxxxxxxxxx> 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?
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

# CONFIG_AFS_DEBUG is not set

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.


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.