c++: Implement C++ DR 2406 - [[fallthrough]] attribute and iteration statements

  Hi!

The following patch implements
CWG 2406 - [[fallthrough]] attribute and iteration statements
The genericization of some loops leaves nothing at all or just a label
after a body of a loop, so if the loop is later followed by
case or default label in a switch, the fallthrough statement isn't
diagnosed.

The following patch implements it by marking the IFN_FALLTHROUGH call
in such a case, such that during gimplification it can be pedantically
diagnosed even if it is followed by case or default label or some normal
labels followed by case/default labels.

While looking into this, I've discovered other problems.
expand_FALLTHROUGH_r is removing the IFN_FALLTHROUGH calls from the IL,
but wasn't telling that to walk_gimple_stmt/walk_gimple_seq_mod, so
the callers would then skip the next statement after it, and it would
return non-NULL if the removed stmt was last in the sequence.  This could
lead to wi->callback_result being set even if it didn't appear at the very
end of switch sequence.
The patch makes use of wi->removed_stmt such that the callers properly
know what happened, and use different way to handle the end of switch
sequence case.

That change discovered a bug in the gimple-walk handling of
wi->removed_stmt.  If that flag is set, the callback is telling the callers
that the current statement has been removed and so the innermost
walk_gimple_seq_mod shouldn't gsi_next.  The problem is that
wi->removed_stmt is only reset at the start of a walk_gimple_stmt, but that
can be too late for some cases.  If we have two nested gimple sequences,
say GIMPLE_BIND as the last stmt of some gimple seq, we remove the last
statement inside of that GIMPLE_BIND, set wi->removed_stmt there, don't
do gsi_next correctly because already gsi_remove moved us to the next stmt,
there is no next stmt, so we return back to the caller, but wi->removed_stmt
is still set and so we don't do gsi_next even in the outer sequence, despite
the GIMPLE_BIND (etc.) not being removed.  That means we walk the
GIMPLE_BIND with its whole sequence again.
The patch fixes that by resetting wi->removed_stmt after we've used that
flag in walk_gimple_seq_mod.  Nothing really uses that flag after the
outermost walk_gimple_seq_mod, it is just a private notification that
the stmt callback has removed a stmt.

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

2023-08-25  Jakub Jelinek  <jakub@redhat.com>

gcc/
	* gimplify.cc (expand_FALLTHROUGH_r): Use wi->removed_stmt after
	gsi_remove, change the way of passing fallthrough stmt at the end
	of sequence to expand_FALLTHROUGH.  Diagnose IFN_FALLTHROUGH
	with GF_CALL_NOTHROW flag.
	(expand_FALLTHROUGH): Change loc into array of 2 location_t elts,
	don't test wi.callback_result, instead check whether first
	elt is not UNKNOWN_LOCATION and in that case pedwarn with the
	second location.
	* gimple-walk.cc (walk_gimple_seq_mod): Clear wi->removed_stmt
	after the flag has been used.
gcc/c-family/
	* c-gimplify.cc (genericize_c_loop): For C++ mark IFN_FALLTHROUGH
	call at the end of loop body as TREE_NOTHROW.
gcc/testsuite/
	* g++.dg/DRs/dr2406.C: New test.


	Jakub
  

On Fri, 25 Aug 2023, Jakub Jelinek wrote:

> Hi!
> 
> The following patch implements
> CWG 2406 - [[fallthrough]] attribute and iteration statements
> The genericization of some loops leaves nothing at all or just a label
> after a body of a loop, so if the loop is later followed by
> case or default label in a switch, the fallthrough statement isn't
> diagnosed.
> 
> The following patch implements it by marking the IFN_FALLTHROUGH call
> in such a case, such that during gimplification it can be pedantically
> diagnosed even if it is followed by case or default label or some normal
> labels followed by case/default labels.
> 
> While looking into this, I've discovered other problems.
> expand_FALLTHROUGH_r is removing the IFN_FALLTHROUGH calls from the IL,
> but wasn't telling that to walk_gimple_stmt/walk_gimple_seq_mod, so
> the callers would then skip the next statement after it, and it would
> return non-NULL if the removed stmt was last in the sequence.  This could
> lead to wi->callback_result being set even if it didn't appear at the very
> end of switch sequence.
> The patch makes use of wi->removed_stmt such that the callers properly
> know what happened, and use different way to handle the end of switch
> sequence case.
> 
> That change discovered a bug in the gimple-walk handling of
> wi->removed_stmt.  If that flag is set, the callback is telling the callers
> that the current statement has been removed and so the innermost
> walk_gimple_seq_mod shouldn't gsi_next.  The problem is that
> wi->removed_stmt is only reset at the start of a walk_gimple_stmt, but that
> can be too late for some cases.  If we have two nested gimple sequences,
> say GIMPLE_BIND as the last stmt of some gimple seq, we remove the last
> statement inside of that GIMPLE_BIND, set wi->removed_stmt there, don't
> do gsi_next correctly because already gsi_remove moved us to the next stmt,
> there is no next stmt, so we return back to the caller, but wi->removed_stmt
> is still set and so we don't do gsi_next even in the outer sequence, despite
> the GIMPLE_BIND (etc.) not being removed.  That means we walk the
> GIMPLE_BIND with its whole sequence again.
> The patch fixes that by resetting wi->removed_stmt after we've used that
> flag in walk_gimple_seq_mod.  Nothing really uses that flag after the
> outermost walk_gimple_seq_mod, it is just a private notification that
> the stmt callback has removed a stmt.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

The gimple-walk.cc/gimplify.cc changes are OK, I don't understand
the c-gimplify.cc one.

Thanks,
Richard.

> 2023-08-25  Jakub Jelinek  <jakub@redhat.com>
> 
> gcc/
> 	* gimplify.cc (expand_FALLTHROUGH_r): Use wi->removed_stmt after
> 	gsi_remove, change the way of passing fallthrough stmt at the end
> 	of sequence to expand_FALLTHROUGH.  Diagnose IFN_FALLTHROUGH
> 	with GF_CALL_NOTHROW flag.
> 	(expand_FALLTHROUGH): Change loc into array of 2 location_t elts,
> 	don't test wi.callback_result, instead check whether first
> 	elt is not UNKNOWN_LOCATION and in that case pedwarn with the
> 	second location.
> 	* gimple-walk.cc (walk_gimple_seq_mod): Clear wi->removed_stmt
> 	after the flag has been used.
> gcc/c-family/
> 	* c-gimplify.cc (genericize_c_loop): For C++ mark IFN_FALLTHROUGH
> 	call at the end of loop body as TREE_NOTHROW.
> gcc/testsuite/
> 	* g++.dg/DRs/dr2406.C: New test.
> 
> --- gcc/gimplify.cc.jj	2023-08-23 11:22:28.115592483 +0200
> +++ gcc/gimplify.cc	2023-08-25 13:43:58.711847414 +0200
> @@ -2588,17 +2588,33 @@ expand_FALLTHROUGH_r (gimple_stmt_iterat
>        *handled_ops_p = false;
>        break;
>      case GIMPLE_CALL:
> +      static_cast<location_t *>(wi->info)[0] = UNKNOWN_LOCATION;
>        if (gimple_call_internal_p (stmt, IFN_FALLTHROUGH))
>  	{
> +	  location_t loc = gimple_location (stmt);
>  	  gsi_remove (gsi_p, true);
> +	  wi->removed_stmt = true;
> +
> +	  /* nothrow flag is added by genericize_c_loop to mark fallthrough
> +	     statement at the end of some loop's body.  Those should be
> +	     always diagnosed, either because they indeed don't precede
> +	     a case label or default label, or because the next statement
> +	     is not within the same iteration statement.  */
> +	  if ((stmt->subcode & GF_CALL_NOTHROW) != 0)
> +	    {
> +	      pedwarn (loc, 0, "attribute %<fallthrough%> not preceding "
> +			       "a case label or default label");
> +	      break;
> +	    }
> +
>  	  if (gsi_end_p (*gsi_p))
>  	    {
> -	      *static_cast<location_t *>(wi->info) = gimple_location (stmt);
> -	      return integer_zero_node;
> +	      static_cast<location_t *>(wi->info)[0] = BUILTINS_LOCATION;
> +	      static_cast<location_t *>(wi->info)[1] = loc;
> +	      break;
>  	    }
>  
>  	  bool found = false;
> -	  location_t loc = gimple_location (stmt);
>  
>  	  gimple_stmt_iterator gsi2 = *gsi_p;
>  	  stmt = gsi_stmt (gsi2);
> @@ -2648,6 +2664,7 @@ expand_FALLTHROUGH_r (gimple_stmt_iterat
>  	}
>        break;
>      default:
> +      static_cast<location_t *>(wi->info)[0] = UNKNOWN_LOCATION;
>        break;
>      }
>    return NULL_TREE;
> @@ -2659,14 +2676,16 @@ static void
>  expand_FALLTHROUGH (gimple_seq *seq_p)
>  {
>    struct walk_stmt_info wi;
> -  location_t loc;
> +  location_t loc[2];
>    memset (&wi, 0, sizeof (wi));
> -  wi.info = (void *) &loc;
> +  loc[0] = UNKNOWN_LOCATION;
> +  loc[1] = UNKNOWN_LOCATION;
> +  wi.info = (void *) &loc[0];
>    walk_gimple_seq_mod (seq_p, expand_FALLTHROUGH_r, NULL, &wi);
> -  if (wi.callback_result == integer_zero_node)
> +  if (loc[0] != UNKNOWN_LOCATION)
>      /* We've found [[fallthrough]]; at the end of a switch, which the C++
>         standard says is ill-formed; see [dcl.attr.fallthrough].  */
> -    pedwarn (loc, 0, "attribute %<fallthrough%> not preceding "
> +    pedwarn (loc[1], 0, "attribute %<fallthrough%> not preceding "
>  	     "a case label or default label");
>  }
>  
> --- gcc/gimple-walk.cc.jj	2023-01-02 09:32:28.298199849 +0100
> +++ gcc/gimple-walk.cc	2023-08-25 14:21:10.264376130 +0200
> @@ -56,11 +56,21 @@ walk_gimple_seq_mod (gimple_seq *pseq, w
>  	  gcc_assert (wi);
>  	  wi->callback_result = ret;
>  
> -	  return wi->removed_stmt ? NULL : gsi_stmt (gsi);
> +	  gimple *g;
> +	  if (!wi->removed_stmt)
> +	    g = gsi_stmt (gsi);
> +	  else
> +	    {
> +	      g = NULL;
> +	      wi->removed_stmt = false;
> +	    }
> +	  return g;
>  	}
>  
>        if (!wi->removed_stmt)
>  	gsi_next (&gsi);
> +      else
> +	wi->removed_stmt = false;
>      }
>  
>    if (wi)
> --- gcc/c-family/c-gimplify.cc.jj	2023-07-11 13:40:37.594467535 +0200
> +++ gcc/c-family/c-gimplify.cc	2023-08-25 12:38:02.406574469 +0200
> @@ -307,6 +307,27 @@ genericize_c_loop (tree *stmt_p, locatio
>      }
>  
>    append_to_statement_list (body, &stmt_list);
> +  if (c_dialect_cxx ()
> +      && stmt_list
> +      && TREE_CODE (stmt_list) == STATEMENT_LIST)
> +    {
> +      tree_stmt_iterator tsi = tsi_last (stmt_list);
> +      if (!tsi_end_p (tsi))
> +	{
> +	  tree t = *tsi;
> +	  while (TREE_CODE (t) == CLEANUP_POINT_EXPR
> +		 || TREE_CODE (t) == EXPR_STMT
> +		 || CONVERT_EXPR_CODE_P (TREE_CODE (t)))
> +	    t = TREE_OPERAND (t, 0);
> +	  /* For C++, if iteration statement body ends with fallthrough
> +	     statement, mark it such that we diagnose it even if next
> +	     statement would be labeled statement with case/default label.  */
> +	  if (TREE_CODE (t) == CALL_EXPR
> +	      && !CALL_EXPR_FN (t)
> +	      && CALL_EXPR_IFN (t) == IFN_FALLTHROUGH)
> +	    TREE_NOTHROW (t) = 1;
> +	}
> +    }
>    finish_bc_block (&stmt_list, bc_continue, clab);
>    if (incr)
>      {
> --- gcc/testsuite/g++.dg/DRs/dr2406.C.jj	2023-08-25 14:16:53.095670934 +0200
> +++ gcc/testsuite/g++.dg/DRs/dr2406.C	2023-08-25 14:16:04.732290555 +0200
> @@ -0,0 +1,81 @@
> +// DR 2406 - [[fallthrough]] attribute and iteration statements
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-pedantic-errors -Wimplicit-fallthrough" }
> +
> +void bar ();
> +void baz ();
> +void qux ();
> +
> +void
> +foo (int n)
> +{
> +  switch (n)
> +    {
> +    case 1:
> +    case 2:
> +      bar ();
> +      [[fallthrough]];
> +    case 3:
> +      do
> +	{
> +	  [[fallthrough]];	// { dg-error "attribute 'fallthrough' not preceding a case label or default label" }
> +	}
> +      while (false);
> +    case 6:
> +      do
> +	{
> +	  [[fallthrough]];	// { dg-error "attribute 'fallthrough' not preceding a case label or default label" }
> +	}
> +      while (n--);
> +    case 7:
> +      while (false)
> +	{
> +	  [[fallthrough]];	// { dg-error "attribute 'fallthrough' not preceding a case label or default label" }
> +	}
> +    case 5:
> +      baz ();			// { dg-warning "this statement may fall through" }
> +    case 4:			// { dg-message "here" }
> +      qux ();
> +      [[fallthrough]];		// { dg-error "attribute 'fallthrough' not preceding a case label or default label" }
> +    }
> +}
> +
> +void
> +corge (int n)
> +{
> +  switch (n)
> +    {
> +    case 1:
> +      {
> +	int i = 0;
> +	do
> +	  {
> +	    [[fallthrough]];	// { dg-error "attribute 'fallthrough' not preceding a case label or default label" }
> +	  }
> +	while (false);
> +      }
> +    case 2:
> +      bar ();
> +      break;
> +    default:
> +      break;
> +    }
> +}
> +
> +void
> +fred (int n)
> +{
> +  switch (n)
> +    {
> +    case 1:
> +      {
> +	int i = 0;
> +	[[fallthrough]];
> +      }
> +    case 2:
> +      bar ();
> +      break;
> +    default:
> +      break;
> +    }
> +}
> 
> 	Jakub
> 
>
  

On 8/28/23 06:34, Richard Biener wrote:
> On Fri, 25 Aug 2023, Jakub Jelinek wrote:
> 
>> Hi!
>>
>> The following patch implements
>> CWG 2406 - [[fallthrough]] attribute and iteration statements
>> The genericization of some loops leaves nothing at all or just a label
>> after a body of a loop, so if the loop is later followed by
>> case or default label in a switch, the fallthrough statement isn't
>> diagnosed.
>>
>> The following patch implements it by marking the IFN_FALLTHROUGH call
>> in such a case, such that during gimplification it can be pedantically
>> diagnosed even if it is followed by case or default label or some normal
>> labels followed by case/default labels.
>>
>> While looking into this, I've discovered other problems.
>> expand_FALLTHROUGH_r is removing the IFN_FALLTHROUGH calls from the IL,
>> but wasn't telling that to walk_gimple_stmt/walk_gimple_seq_mod, so
>> the callers would then skip the next statement after it, and it would
>> return non-NULL if the removed stmt was last in the sequence.  This could
>> lead to wi->callback_result being set even if it didn't appear at the very
>> end of switch sequence.
>> The patch makes use of wi->removed_stmt such that the callers properly
>> know what happened, and use different way to handle the end of switch
>> sequence case.
>>
>> That change discovered a bug in the gimple-walk handling of
>> wi->removed_stmt.  If that flag is set, the callback is telling the callers
>> that the current statement has been removed and so the innermost
>> walk_gimple_seq_mod shouldn't gsi_next.  The problem is that
>> wi->removed_stmt is only reset at the start of a walk_gimple_stmt, but that
>> can be too late for some cases.  If we have two nested gimple sequences,
>> say GIMPLE_BIND as the last stmt of some gimple seq, we remove the last
>> statement inside of that GIMPLE_BIND, set wi->removed_stmt there, don't
>> do gsi_next correctly because already gsi_remove moved us to the next stmt,
>> there is no next stmt, so we return back to the caller, but wi->removed_stmt
>> is still set and so we don't do gsi_next even in the outer sequence, despite
>> the GIMPLE_BIND (etc.) not being removed.  That means we walk the
>> GIMPLE_BIND with its whole sequence again.
>> The patch fixes that by resetting wi->removed_stmt after we've used that
>> flag in walk_gimple_seq_mod.  Nothing really uses that flag after the
>> outermost walk_gimple_seq_mod, it is just a private notification that
>> the stmt callback has removed a stmt.
>>
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> The gimple-walk.cc/gimplify.cc changes are OK, I don't understand
> the c-gimplify.cc one.

Seems like it would be good to document this non-obvious meaning of 
*_NOTHROW for ICF_FALLTHROUGH.  Maybe at its entry in internal-fn.def?

OK with that change.

Jason