Patchwork Fix implicit-fallthrough warnings in tst-setjmp.c

login
register
mail settings
Submitter Joseph Myers
Date Feb. 14, 2019, 10:03 p.m.
Message ID <alpine.DEB.2.21.1902142203310.4022@digraph.polyomino.org.uk>
Download mbox | patch
Permalink /patch/31484/
State Committed
Headers show

Comments

Joseph Myers - Feb. 14, 2019, 10:03 p.m.
Building the testsuite with -Wextra (together with
-Wno-cast-function-type -Wno-clobbered -Wno-expansion-to-defined
-Wno-missing-field-initializers -Wno-old-style-declaration
-Wno-shift-negative-value -Wno-sign-compare -Wno-type-limits
-Wno-unused-parameter, which reflect the set of -Wextra warnings for
which glibc itself is not currently clean on x86_64) showed up
implicit-fallthrough warnings in tst-setjmp.c.  Those warnings appear
to be false positives, arising from a function "jump" that calls
longjmp not itself being marked as noreturn; thus, this patch adds the
noreturn marking to that function to fix the warnings.

Tested for x86_64.

2019-02-14  Joseph Myers  <joseph@codesourcery.com>

	* setjmp/tst-setjmp.c (jump): Use __attribute__ ((__noreturn__)).
Paul Eggert - Feb. 14, 2019, 10:21 p.m.
On 2/14/19 2:03 PM, Joseph Myers wrote:
> -static void
> +static __attribute__ ((__noreturn__)) void

Shouldn't we use the more-standard _Noreturn for this sort of thing? 
cdefs.h defines _Noreturn for pre-C11 GCCs.
Joseph Myers - Feb. 14, 2019, 10:23 p.m.
On Thu, 14 Feb 2019, Paul Eggert wrote:

> On 2/14/19 2:03 PM, Joseph Myers wrote:
> > -static void
> > +static __attribute__ ((__noreturn__)) void
> 
> Shouldn't we use the more-standard _Noreturn for this sort of thing? cdefs.h
> defines _Noreturn for pre-C11 GCCs.

The attribute seems much more widely used in glibc .c files at present.
Paul Eggert - Feb. 14, 2019, 10:27 p.m.
On 2/14/19 2:23 PM, Joseph Myers wrote:
>> Shouldn't we use the more-standard _Noreturn for this sort of thing? cdefs.h
>> defines _Noreturn for pre-C11 GCCs.
> The attribute seems much more widely used in glibc .c files at present.

Sure, but that habit predates C11. Now that we have a standard way to 
say "this function does not return" it's surely better to use the 
standard way, at least for new code and arguably even for old code.
Joseph Myers - Feb. 14, 2019, 10:36 p.m.
On Thu, 14 Feb 2019, Paul Eggert wrote:

> On 2/14/19 2:23 PM, Joseph Myers wrote:
> > > Shouldn't we use the more-standard _Noreturn for this sort of thing?
> > > cdefs.h
> > > defines _Noreturn for pre-C11 GCCs.
> > The attribute seems much more widely used in glibc .c files at present.
> 
> Sure, but that habit predates C11. Now that we have a standard way to say
> "this function does not return" it's surely better to use the standard way, at
> least for new code and arguably even for old code.

The consistent, universal style in glibc, for all files not coming from 
external sources, other than a single use of _Noreturn in 
nptl/thrd_exit.c, is to use the attribute.  You can of course propose 
(with corresponding patch) a change to the style used (recalling that for 
installed headers _Noreturn would reduce portability because of prefix 
attributes not working with GCC 2.7), but it seems clear my patch is 
consistent with the current style and _Noreturn would be inconsistent with 
it.
Florian Weimer - Feb. 15, 2019, 8:05 a.m.
* Joseph Myers:

> diff --git a/setjmp/tst-setjmp.c b/setjmp/tst-setjmp.c
> index e83e896ebe..b377393ecf 100644
> --- a/setjmp/tst-setjmp.c
> +++ b/setjmp/tst-setjmp.c
> @@ -22,7 +22,7 @@
>  static jmp_buf env;
>  static int last_value = -1, lose = 0;
>  
> -static void
> +static __attribute__ ((__noreturn__)) void
>  jump (int val)
>  {
>    longjmp (env, val);

I think this patch is okay to push.

Patch

diff --git a/setjmp/tst-setjmp.c b/setjmp/tst-setjmp.c
index e83e896ebe..b377393ecf 100644
--- a/setjmp/tst-setjmp.c
+++ b/setjmp/tst-setjmp.c
@@ -22,7 +22,7 @@ 
 static jmp_buf env;
 static int last_value = -1, lose = 0;
 
-static void
+static __attribute__ ((__noreturn__)) void
 jump (int val)
 {
   longjmp (env, val);