| Message ID | 162375813191.653958.11993495571264748407.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | afs: fix no return statement in function returning non-void | expand |
Hi Linus, Note that this isn't really a fix, so can wait to the next merge window if you prefer. David
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
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.
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
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.
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.
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. > . >
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. >> . >> >
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
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
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 >
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
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
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 = {