Re: Implement -Wimplicit-fallthrough (version 8)

[Marek Polacek <polacek at redhat dot com>]

On Fri, Sep 09, 2016 at 06:44:44PM +0200, Jakub Jelinek wrote:
> Hi!
 
Finally getting back to this...

> On Thu, Sep 01, 2016 at 03:40:49PM +0200, Marek Polacek wrote:
> > @@ -1749,6 +1758,16 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
> >      {
> >        if (auto_type_p)
> >  	error_at (here, "%<__auto_type%> in empty declaration");
> > +      else if (specs->typespec_kind == ctsk_none && specs->attrs
> > +	       && is_attribute_p ("fallthrough",
> > +				  get_attribute_name (specs->attrs)))
> > +	{
> > +	  if (fallthru_attr_p != NULL)
> > +	    *fallthru_attr_p = true;
> 
> What happens if there are more than one attribute in specs->attrs?
> Either fallthrough not being the first one, or there are other attributes
> afterwards?
> Like:
> 	__attribute__((unused, fallthrough)) ;
> or
> 	__attribute__((fallthrough, unused)) ;
 
I only accepted __attribute__((fallthrough)); or __attribute__((fallthrough,
unused));, but that is probably wrong.  Fixed in another version of the patch,
where I introduced maybe_attribute_fallthrough_p that detects various valid
and invalid cases.

> > +	  tree fn = build_call_expr_internal_loc (here, IFN_FALLTHROUGH,
> > +						  void_type_node, 0);
> > +	  add_stmt (fn);
> > +	}
> >        else if (empty_ok)
> >  	shadow_tag (specs);
> >        else
> > @@ -4845,9 +4864,11 @@ c_parser_compound_statement_nostart (c_parser *parser)
> >  	{
> >  	  last_label = false;
> >  	  mark_valid_location_for_stdc_pragma (false);
> > +	  bool fallthru_attr_p = false;
> >  	  c_parser_declaration_or_fndef (parser, true, true, true, true,
> > -					 true, NULL, vNULL);
> > -	  if (last_stmt)
> > +					 true, NULL, vNULL, NULL,
> > +					 &fallthru_attr_p);
> > +	  if (last_stmt && !fallthru_attr_p)
> >  	    pedwarn_c90 (loc, OPT_Wdeclaration_after_statement,
> >  			 "ISO C90 forbids mixed declarations and code");
> >  	  last_stmt = false;
> 
> So, for fallthru_attr_p here you have a guarantee it has been
> __attribute__((fallthrough)) ; which is actualy not a declaration, but
> (empty) statement?  If so, I'd say that for fallthru_attr_p in that case you
> don't want to clear last_stmt, but want to set it (or let it be unchanged?).
> I mean:
> 	int a;
> 	a = 5;
> 	__attribute__((fallthrough)) ;
> 	int b;
> would for -std=c99 -pedantic-errors not error out on the declaration of b
> being after the statements (a = 5 and empty stmt).
> What about
> 	int a;
> 	__attribute__((fallthrough)) ;
> 	int b;
> ?  Shall we error on that too?  Pedantically it is also a declaration after
> a statement.
 
Yeah, fixed now.

> > @@ -5327,6 +5360,27 @@ c_parser_statement_after_labels (c_parser *parser, bool *if_p,
> >  	  gcc_assert (c_dialect_objc ());
> >  	  c_parser_objc_synchronized_statement (parser);
> >  	  break;
> > +	case RID_ATTRIBUTE:
> > +	  {
> > +	    /* Allow '__attribute__((fallthrough));'.  */
> > +	    tree attrs = c_parser_attributes (parser);
> > +	    if (attrs != NULL_TREE
> > +		&& is_attribute_p ("fallthrough", get_attribute_name (attrs)))
> 
> See above.
 
Fixed with maybe_attribute_fallthrough_p.

> > --- gcc/gcc/cp/parser.c
> > +++ gcc/gcc/cp/parser.c
> > @@ -5135,6 +5135,31 @@ cp_parser_primary_expression (cp_parser *parser,
> >  	case RID_AT_SELECTOR:
> >  	  return cp_parser_objc_expression (parser);
> >  
> > +	case RID_ATTRIBUTE:
> > +	  {
> > +	    /* This might be __attribute__((fallthrough));.  */
> > +	    tree attr = cp_parser_gnu_attributes_opt (parser);
> > +	    if (attr != NULL_TREE
> > +		&& is_attribute_p ("fallthrough", get_attribute_name (attr)))
> 
> Again.

Fixed with maybe_attribute_fallthrough_p.

> > @@ -10585,14 +10610,25 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
> >  	}
> >        /* Look for an expression-statement instead.  */
> >        statement = cp_parser_expression_statement (parser, in_statement_expr);
> > +
> > +      /* Handle [[fallthrough]];.  */
> > +      if (std_attrs != NULL_TREE
> > +	  && is_attribute_p ("fallthrough", get_attribute_name (std_attrs))
> > +	  && statement == NULL_TREE)
> 
> And again.
 
Fixed with maybe_attribute_fallthrough_p.

> > +	{
> > +	  statement = build_call_expr_internal_loc (statement_location,
> > +						    IFN_FALLTHROUGH,
> > +						    void_type_node, 0);
> > +	  finish_expr_stmt (statement);
> > +	  std_attrs = NULL_TREE;
> > +	}
> >      }
> >  
> > diff --git gcc/gcc/cp/pt.c gcc/gcc/cp/pt.c
> > index b0f0664..60c5d43 100644
> > --- gcc/gcc/cp/pt.c
> > +++ gcc/gcc/cp/pt.c
> > @@ -16556,7 +16556,9 @@ tsubst_copy_and_build (tree t,
> >  	tree ret;
> >  
> >  	function = CALL_EXPR_FN (t);
> > -	/* When we parsed the expression,  we determined whether or
> > +	if (function == NULL_TREE)
> > +	  RETURN (t);
> 
> Doesn't this generally assume that all the internal functions have no
> arguments (or no arguments that really should be tsubsted)?
> That is the case for IFN_FALLTHROUGH, but not necessarily the case of any
> future ifn that is added before instantiation in the future.
> So, if you don't want to add this right now, I think it would be better
> to at least assert it has no arguments, otherwise perhaps just do
>   if (function == NULL_TREE)
>     ;
> (to skip over the koenig_p etc. cases) and fallthrough into the argument
> handling and add another if (function == NULL_TREE) handling after that,
> which would just build another internal call with the tsubsted arguments.
 
I added the assert here, but I spent time implementing this, and it didn't
really work, because without a testcase it was hard to say whether what I
had was correct.  E.g., how to determine the return type of the internal
function, etc.

> > @@ -22787,7 +22789,7 @@ instantiation_dependent_scope_ref_p (tree t)
> >  bool
> >  value_dependent_expression_p (tree expression)
> >  {
> > -  if (!processing_template_decl)
> > +  if (!processing_template_decl || expression == NULL_TREE)
> >      return false;
> >  
> >    /* A name declared with a dependent type.  */
> 
> Up to Jason, but I'd think this perhaps should be done instead in the caller
> if it is solely meant for the CALL_EXPR_FN being NULL case.
 
Too bad I don't have the testcase for this so I don't know who was the
caller...

> > +
> > +This warning does not warn when the last statement of a case cannot
> > +fall through, e.g. when there is a return statement of a function
> > +declared with the noreturn attribute.  @option{-Wimplicit-fallthrough}
> 
> Did you mean when there is a return statement or a call to function declared
> with the noreturn attribute?  If there is return, you really don't care
> whether the current function is noreturn or not.

Yes, fixed.
 
> > +C++17 provides a standard way to suppress the @option{-Wimplicit-fallthrough}
> > +warning using @code{[[fallthrough]];} instead of the GNU attribute.  In C++11
> > +or C++14 users can use @code{[[gnu::fallthrough]];}, which is a GNU extension.
> > +Instead of the these attributes, it is also possible to add a "falls through"
> > +comment to silence the warning.  GCC accepts a wide range of such comments,
> > +for example all of "Falls through.", "fallthru", "FALLS-THROUGH" work.
> 
> Wonder if we actually shouldn't document what exactly we accept.
> Make it clear that it has to be a comment with only the one or two works and
> nothing else except for optional . and optional whitespace (and that \n or
> \r isn't allowed) in the comment.

I added something to that effect.

> > --- gcc/gcc/gimple.h
> > +++ gcc/gcc/gimple.h
> > @@ -2921,6 +2921,16 @@ gimple_call_internal_unique_p (const gimple *gs)
> >    return gimple_call_internal_unique_p (gc);
> >  }
> >  
> > +/* Return true if GS is an internal function FN.  */
> > +
> > +static inline bool
> > +gimple_call_internal_p (const gimple *gs, internal_fn fn)
> > +{
> > +  return (is_gimple_call (gs)
> > +	  && gimple_call_internal_p (gs)
> > +	  && gimple_call_internal_fn (gs) == fn);
> > +}
> > +
> 
> This is nice, are you as some follow-up going to change various spots that
> have such sequences to use the new predicate?
 
Yeah, I plan to.

> > +  switch (gimple_code (stmt))
> > +    {
> > +    case GIMPLE_BIND:
> > +      {
> > +	gbind *bind = as_a <gbind *> (stmt);
> > +	stmt = gimple_seq_last_stmt (gimple_bind_body (bind));
> > +	return last_stmt_in_scope (stmt);
> > +      }
> > +
> > +    case GIMPLE_TRY:
> > +      {
> > +	gtry *try_stmt = as_a <gtry *> (stmt);
> > +	stmt = gimple_seq_last_stmt (gimple_try_eval (try_stmt));
> > +	return last_stmt_in_scope (stmt);
> 
> I wonder if what exactly you want to do here doesn't depend on
> the gimple try kind.  For GIMPLE_TRY_FINALLY, where the eval is always
> run, followed by the cleanup, at least for the discovery whether it can
> fallthrough or not, you don't want to do something more complex.
> Say if there is try { ... return; } finally { ... }, it is clear that
> it doesn't fall through, but for try { ... } finally { ... return; }
> it is the same case, because no matter whether an exception is thrown in
> the try block or not, you'll execute the cleanup afterwards and if it
> returns or does anything similar that doesn't fall through, the whole
> GIMPLE_TRY doesn't.  Though of course that doesn't map well with the name of
> the function.  Semantically, in such case the last stmt is the cleanup's
> last stmt.
 
I've implemeted something here, though I ain't sure if it's correct.  Basically
if the last stmt of the eval part doesn't fall through, and we're dealing with
GIMPLE_TRY_FINALLY, return last statement of the cleanup part.

> > +      /* If's are tricky.  */
> 
> Isn't that Ifs or IFs?

Yeah, fixed.

> > +      if (gimple_code (gsi_stmt (*gsi_p)) == GIMPLE_COND)
> > +	{
> > +	  gcond *cond_stmt = as_a <gcond *> (gsi_stmt (*gsi_p));
> > +	  tree false_lab = gimple_cond_false_label (cond_stmt);
> > +	  location_t if_loc = gimple_location (cond_stmt);
> > +
> > +	  /* If we have e.g.
> > +	       if (i > 1) goto <D.2259>; else goto D;
> > +	     we can't do much with the else-branch.  */
> > +	  if (!DECL_ARTIFICIAL (false_lab))
> > +	    break;
> > +
> > +	  /* Go on until the false label, then one step back.  */
> > +	  for (; !gsi_end_p (*gsi_p); gsi_next (gsi_p))
> 
> Do we have a guarantee that the label will be in the same gimple sequence?
> I mean can't it be say in a nested GIMPLE_BIND or in outer GIMPLE_BIND?
> Perhaps gimplification guarantees that for artificial labels.
 
Well, if it doesn't find the false label, it breaks, so we should be safe.

> > +static tree
> > +warn_implicit_fallthrough_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
> > +			     struct walk_stmt_info *)
> > +{
> > +  gimple *stmt = gsi_stmt (*gsi_p);
> > +
> > +  *handled_ops_p = true;
> > +  switch (gimple_code (stmt))
> > +    {
> > +    case GIMPLE_TRY:
> > +    case GIMPLE_BIND:
> > +    case GIMPLE_CATCH:
> > +    case GIMPLE_EH_FILTER:
> > +    case GIMPLE_TRANSACTION:
> > +      /* Walk the sub-statements.  */
> > +      *handled_ops_p = false;
> > +      break;
> 
> The various OpenMP constructs might need similar handling, but guess it will
> be best if I look at it as follow up.
 
Yes ;).

> > +      if (gimple_call_internal_p (stmt, IFN_FALLTHROUGH))
> > +	{
> > +	  gsi_remove (gsi_p, true);
> > +	  if (gsi_end_p (*gsi_p))
> > +	    return integer_zero_node;
> > +	  else if (gimple_code (gsi_stmt (*gsi_p)) == GIMPLE_LABEL
> > +		   || (gimple_code (gsi_stmt (*gsi_p)) == GIMPLE_GOTO
> > +		       && !gimple_has_location (gsi_stmt (*gsi_p))))
> 
> Is any kind of label ok to accept here, or just the selected ones like
> case labels or user labels?  If the latter, perhaps for GIMPLE_GOTO you also
> should prove it is followed by such label at the goto's destination.
> Or perhaps skip all artifical labels and ensure there is a user label or
> case label after them?
 
I rewrote this part -- it now follows the GIMPLE_GOTO and checks whether it's
followed by a case label or default label.

> > --- gcc/gcc/internal-fn.c
> > +++ gcc/gcc/internal-fn.c
> > @@ -244,6 +244,15 @@ expand_TSAN_FUNC_EXIT (internal_fn, gcall *)
> >    gcc_unreachable ();
> >  }
> >  
> > +/* This should get expanded in the lower pass.  */
> > +
> > +static void
> > +expand_FALLTHROUGH (internal_fn, gcall *call)
> > +{
> > +  error_at (gimple_location (call),
> > +	    "invalid use of attribute %<fallthrough%>");
> 
> Shouldn't it be just assertion failure?  Or is there some way how you can
> force the IFN to survive gimplification?

Nope, this should be error.  This will trigged if someone adds [[fallthrough]];
to a wrong spot, e.g. outside a switch statement.

I'm testing a new version, will post it later today.

Thanks a lot for the review.

	Marek