PATCH for c++/87068, missing diagnostic with fallthrough statement

PATCH for c++/87068, missing diagnostic with fallthrough statement

[Marek Polacek]

The C++ standard says that [[fallthrough]]; at the end of a switch statement
is ill-formed: <http://eel.is/c++draft/dcl.attr.fallthrough>

Currently we do check that the statement after a fallthrough statement is
a labeled statement, but we didn't warn if a fallthrough statement is at
the very end of a switch statement.  This patch adds this warning.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2018-08-23  Marek Polacek  <polacek@redhat.com>

	PR c++/87068
	* gimplify.c (expand_FALLTHROUGH_r): If IFN_FALLTHROUGH was found
	at the end of a seq, save its location to walk_stmt_info.
	(expand_FALLTHROUGH): Warn if IFN_FALLTHROUGH is at the end of
	a switch.

	* c-c++-common/Wimplicit-fallthrough-37.c: New test.

diff --git gcc/gimplify.c gcc/gimplify.c
index e35137aec2c..04c15016f18 100644
--- gcc/gimplify.c
+++ gcc/gimplify.c
@@ -2231,7 +2231,7 @@ maybe_warn_implicit_fallthrough (gimple_seq seq)
 
 static tree
 expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
-		      struct walk_stmt_info *)
+		      struct walk_stmt_info *wi)
 {
   gimple *stmt = gsi_stmt (*gsi_p);
 
@@ -2250,11 +2250,14 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
       if (gimple_call_internal_p (stmt, IFN_FALLTHROUGH))
 	{
 	  gsi_remove (gsi_p, true);
+	  location_t loc = gimple_location (stmt);
 	  if (gsi_end_p (*gsi_p))
-	    return integer_zero_node;
+	    {
+	      wi->info = &loc;
+	      return integer_zero_node;
+	    }
 
 	  bool found = false;
-	  location_t loc = gimple_location (stmt);
 
 	  gimple_stmt_iterator gsi2 = *gsi_p;
 	  stmt = gsi_stmt (gsi2);
@@ -2317,6 +2320,14 @@ expand_FALLTHROUGH (gimple_seq *seq_p)
   struct walk_stmt_info wi;
   memset (&wi, 0, sizeof (wi));
   walk_gimple_seq_mod (seq_p, expand_FALLTHROUGH_r, NULL, &wi);
+  if (wi.callback_result == integer_zero_node)
+    {
+      /* We've found [[fallthrough]]; at the end of a switch, which the C++
+	 standard says is ill-formed; see [dcl.attr.fallthrough].  */
+      location_t *loc = static_cast<location_t *>(wi.info);
+      warning_at (*loc, 0, "attribute %<fallthrough%> not preceding "
+		  "a case label or default label");
+    }
 }
 
 \f
diff --git gcc/testsuite/c-c++-common/Wimplicit-fallthrough-37.c gcc/testsuite/c-c++-common/Wimplicit-fallthrough-37.c
index e69de29bb2d..644003af47d 100644
--- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-37.c
+++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-37.c
@@ -0,0 +1,13 @@
+/* PR c++/87068 */
+/* { dg-do compile } */
+
+void
+f (int n)
+{
+  switch (n)
+    {
+    case 4:
+      ++n;
+      __attribute__((fallthrough)); /* { dg-warning "not preceding" } */
+    }
+}

Re: PATCH for c++/87068, missing diagnostic with fallthrough statement

[Marek Polacek]

Ping.

On Thu, Aug 23, 2018 at 04:38:03PM -0400, Marek Polacek wrote:
> The C++ standard says that [[fallthrough]]; at the end of a switch statement
> is ill-formed: <http://eel.is/c++draft/dcl.attr.fallthrough>
> 
> Currently we do check that the statement after a fallthrough statement is
> a labeled statement, but we didn't warn if a fallthrough statement is at
> the very end of a switch statement.  This patch adds this warning.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2018-08-23  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/87068
> 	* gimplify.c (expand_FALLTHROUGH_r): If IFN_FALLTHROUGH was found
> 	at the end of a seq, save its location to walk_stmt_info.
> 	(expand_FALLTHROUGH): Warn if IFN_FALLTHROUGH is at the end of
> 	a switch.
> 
> 	* c-c++-common/Wimplicit-fallthrough-37.c: New test.
> 
> diff --git gcc/gimplify.c gcc/gimplify.c
> index e35137aec2c..04c15016f18 100644
> --- gcc/gimplify.c
> +++ gcc/gimplify.c
> @@ -2231,7 +2231,7 @@ maybe_warn_implicit_fallthrough (gimple_seq seq)
>  
>  static tree
>  expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
> -		      struct walk_stmt_info *)
> +		      struct walk_stmt_info *wi)
>  {
>    gimple *stmt = gsi_stmt (*gsi_p);
>  
> @@ -2250,11 +2250,14 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
>        if (gimple_call_internal_p (stmt, IFN_FALLTHROUGH))
>  	{
>  	  gsi_remove (gsi_p, true);
> +	  location_t loc = gimple_location (stmt);
>  	  if (gsi_end_p (*gsi_p))
> -	    return integer_zero_node;
> +	    {
> +	      wi->info = &loc;
> +	      return integer_zero_node;
> +	    }
>  
>  	  bool found = false;
> -	  location_t loc = gimple_location (stmt);
>  
>  	  gimple_stmt_iterator gsi2 = *gsi_p;
>  	  stmt = gsi_stmt (gsi2);
> @@ -2317,6 +2320,14 @@ expand_FALLTHROUGH (gimple_seq *seq_p)
>    struct walk_stmt_info wi;
>    memset (&wi, 0, sizeof (wi));
>    walk_gimple_seq_mod (seq_p, expand_FALLTHROUGH_r, NULL, &wi);
> +  if (wi.callback_result == integer_zero_node)
> +    {
> +      /* We've found [[fallthrough]]; at the end of a switch, which the C++
> +	 standard says is ill-formed; see [dcl.attr.fallthrough].  */
> +      location_t *loc = static_cast<location_t *>(wi.info);
> +      warning_at (*loc, 0, "attribute %<fallthrough%> not preceding "
> +		  "a case label or default label");
> +    }
>  }
>  
>  \f
> diff --git gcc/testsuite/c-c++-common/Wimplicit-fallthrough-37.c gcc/testsuite/c-c++-common/Wimplicit-fallthrough-37.c
> index e69de29bb2d..644003af47d 100644
> --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-37.c
> +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-37.c
> @@ -0,0 +1,13 @@
> +/* PR c++/87068 */
> +/* { dg-do compile } */
> +
> +void
> +f (int n)
> +{
> +  switch (n)
> +    {
> +    case 4:
> +      ++n;
> +      __attribute__((fallthrough)); /* { dg-warning "not preceding" } */
> +    }
> +}

	Marek

Re: PATCH for c++/87068, missing diagnostic with fallthrough statement

[Jakub Jelinek]

On Thu, Aug 30, 2018 at 05:17:03PM -0400, Marek Polacek wrote:
> > 2018-08-23  Marek Polacek  <polacek@redhat.com>
> > 
> > 	PR c++/87068
> > 	* gimplify.c (expand_FALLTHROUGH_r): If IFN_FALLTHROUGH was found
> > 	at the end of a seq, save its location to walk_stmt_info.
> > 	(expand_FALLTHROUGH): Warn if IFN_FALLTHROUGH is at the end of
> > 	a switch.
> > 
> > 	* c-c++-common/Wimplicit-fallthrough-37.c: New test.
> > 
> > diff --git gcc/gimplify.c gcc/gimplify.c
> > index e35137aec2c..04c15016f18 100644
> > --- gcc/gimplify.c
> > +++ gcc/gimplify.c
> > @@ -2231,7 +2231,7 @@ maybe_warn_implicit_fallthrough (gimple_seq seq)
> >  
> >  static tree
> >  expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
> > -		      struct walk_stmt_info *)
> > +		      struct walk_stmt_info *wi)
> >  {
> >    gimple *stmt = gsi_stmt (*gsi_p);
> >  
> > @@ -2250,11 +2250,14 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
> >        if (gimple_call_internal_p (stmt, IFN_FALLTHROUGH))
> >  	{
> >  	  gsi_remove (gsi_p, true);
> > +	  location_t loc = gimple_location (stmt);
> >  	  if (gsi_end_p (*gsi_p))
> > -	    return integer_zero_node;
> > +	    {
> > +	      wi->info = &loc;
> > +	      return integer_zero_node;

This looks incorrect.  loc is an automatic variable in
expand_FALLTHROUGH_r, you set wi->info to the address of that variable,
then immediately return and then:

> > @@ -2317,6 +2320,14 @@ expand_FALLTHROUGH (gimple_seq *seq_p)
> >    struct walk_stmt_info wi;
> >    memset (&wi, 0, sizeof (wi));
> >    walk_gimple_seq_mod (seq_p, expand_FALLTHROUGH_r, NULL, &wi);
> > +  if (wi.callback_result == integer_zero_node)
> > +    {
> > +      /* We've found [[fallthrough]]; at the end of a switch, which the C++
> > +	 standard says is ill-formed; see [dcl.attr.fallthrough].  */
> > +      location_t *loc = static_cast<location_t *>(wi.info);
> > +      warning_at (*loc, 0, "attribute %<fallthrough%> not preceding "

dereference it here, when the loc variable is no longer in scope.
-fsanitize-use-after-scope should complain here...

> > +		  "a case label or default label");
> > +    }

If the only spot that ises wi->info is in these two new spots, can't you
e.g. set in the expand_FALLTHROUGH function:
  location_t loc;
  wi.info = (void *) &loc;
and then in expand_FALLTHROUGH_r do:
  *static_cast<location_t *>(wi.info) = gimple_location (stmt);
?

	Jakub

Re: PATCH for c++/87068, missing diagnostic with fallthrough statement

[Marek Polacek]

On Thu, Aug 30, 2018 at 11:24:50PM +0200, Jakub Jelinek wrote:
> On Thu, Aug 30, 2018 at 05:17:03PM -0400, Marek Polacek wrote:
> > > 2018-08-23  Marek Polacek  <polacek@redhat.com>
> > > 
> > > 	PR c++/87068
> > > 	* gimplify.c (expand_FALLTHROUGH_r): If IFN_FALLTHROUGH was found
> > > 	at the end of a seq, save its location to walk_stmt_info.
> > > 	(expand_FALLTHROUGH): Warn if IFN_FALLTHROUGH is at the end of
> > > 	a switch.
> > > 
> > > 	* c-c++-common/Wimplicit-fallthrough-37.c: New test.
> > > 
> > > diff --git gcc/gimplify.c gcc/gimplify.c
> > > index e35137aec2c..04c15016f18 100644
> > > --- gcc/gimplify.c
> > > +++ gcc/gimplify.c
> > > @@ -2231,7 +2231,7 @@ maybe_warn_implicit_fallthrough (gimple_seq seq)
> > >  
> > >  static tree
> > >  expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
> > > -		      struct walk_stmt_info *)
> > > +		      struct walk_stmt_info *wi)
> > >  {
> > >    gimple *stmt = gsi_stmt (*gsi_p);
> > >  
> > > @@ -2250,11 +2250,14 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
> > >        if (gimple_call_internal_p (stmt, IFN_FALLTHROUGH))
> > >  	{
> > >  	  gsi_remove (gsi_p, true);
> > > +	  location_t loc = gimple_location (stmt);
> > >  	  if (gsi_end_p (*gsi_p))
> > > -	    return integer_zero_node;
> > > +	    {
> > > +	      wi->info = &loc;
> > > +	      return integer_zero_node;
> 
> This looks incorrect.  loc is an automatic variable in
> expand_FALLTHROUGH_r, you set wi->info to the address of that variable,
> then immediately return and then:
> 
> > > @@ -2317,6 +2320,14 @@ expand_FALLTHROUGH (gimple_seq *seq_p)
> > >    struct walk_stmt_info wi;
> > >    memset (&wi, 0, sizeof (wi));
> > >    walk_gimple_seq_mod (seq_p, expand_FALLTHROUGH_r, NULL, &wi);
> > > +  if (wi.callback_result == integer_zero_node)
> > > +    {
> > > +      /* We've found [[fallthrough]]; at the end of a switch, which the C++
> > > +	 standard says is ill-formed; see [dcl.attr.fallthrough].  */
> > > +      location_t *loc = static_cast<location_t *>(wi.info);
> > > +      warning_at (*loc, 0, "attribute %<fallthrough%> not preceding "
> 
> dereference it here, when the loc variable is no longer in scope.
> -fsanitize-use-after-scope should complain here...
> 
> > > +		  "a case label or default label");
> > > +    }
> 
> If the only spot that ises wi->info is in these two new spots, can't you
> e.g. set in the expand_FALLTHROUGH function:
>   location_t loc;
>   wi.info = (void *) &loc;
> and then in expand_FALLTHROUGH_r do:
>   *static_cast<location_t *>(wi.info) = gimple_location (stmt);
> ?

Sorry for the ridiculous delay and an embarassing thinko :(.

Here's an updated patch.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2019-02-28  Marek Polacek  <polacek@redhat.com>

	PR c++/87068 - missing diagnostic with fallthrough statement.
	* gimplify.c (expand_FALLTHROUGH_r): If IFN_FALLTHROUGH was found
	at the end of a seq, save its location to walk_stmt_info.
	(expand_FALLTHROUGH): Warn if IFN_FALLTHROUGH is at the end of
	a switch.

	* c-c++-common/Wimplicit-fallthrough-37.c: New test.

diff --git gcc/gimplify.c gcc/gimplify.c
index 7e37e500d81..983635ba21f 100644
--- gcc/gimplify.c
+++ gcc/gimplify.c
@@ -2263,7 +2263,7 @@ maybe_warn_implicit_fallthrough (gimple_seq seq)
 
 static tree
 expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
-		      struct walk_stmt_info *)
+		      struct walk_stmt_info *wi)
 {
   gimple *stmt = gsi_stmt (*gsi_p);
 
@@ -2283,7 +2283,10 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
 	{
 	  gsi_remove (gsi_p, true);
 	  if (gsi_end_p (*gsi_p))
-	    return integer_zero_node;
+	    {
+	      *static_cast<location_t *>(wi->info) = gimple_location (stmt);
+	      return integer_zero_node;
+	    }
 
 	  bool found = false;
 	  location_t loc = gimple_location (stmt);
@@ -2347,8 +2350,15 @@ static void
 expand_FALLTHROUGH (gimple_seq *seq_p)
 {
   struct walk_stmt_info wi;
+  location_t loc;
   memset (&wi, 0, sizeof (wi));
+  wi.info = (void *) &loc;
   walk_gimple_seq_mod (seq_p, expand_FALLTHROUGH_r, NULL, &wi);
+  if (wi.callback_result == integer_zero_node)
+    /* We've found [[fallthrough]]; at the end of a switch, which the C++
+       standard says is ill-formed; see [dcl.attr.fallthrough].  */
+    warning_at (loc, 0, "attribute %<fallthrough%> not preceding "
+		"a case label or default label");
 }
 
 \f
diff --git gcc/testsuite/c-c++-common/Wimplicit-fallthrough-37.c gcc/testsuite/c-c++-common/Wimplicit-fallthrough-37.c
new file mode 100644
index 00000000000..644003af47d
--- /dev/null
+++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-37.c
@@ -0,0 +1,13 @@
+/* PR c++/87068 */
+/* { dg-do compile } */
+
+void
+f (int n)
+{
+  switch (n)
+    {
+    case 4:
+      ++n;
+      __attribute__((fallthrough)); /* { dg-warning "not preceding" } */
+    }
+}

Re: PATCH for c++/87068, missing diagnostic with fallthrough statement

[Jakub Jelinek]

On Thu, Feb 28, 2019 at 05:16:14PM -0500, Marek Polacek wrote:
> 2019-02-28  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/87068 - missing diagnostic with fallthrough statement.
> 	* gimplify.c (expand_FALLTHROUGH_r): If IFN_FALLTHROUGH was found
> 	at the end of a seq, save its location to walk_stmt_info.
> 	(expand_FALLTHROUGH): Warn if IFN_FALLTHROUGH is at the end of
> 	a switch.
> 
> 	* c-c++-common/Wimplicit-fallthrough-37.c: New test.

Ok, thanks.

	Jakub