Re: Implement -Wimplicit-fallthrough (version 8)

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

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)) ;

> +	  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.

> @@ -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.

> --- 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.

> @@ -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.

> +	{
> +	  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.

> @@ -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.

> +
> +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.

> +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.

> --- 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?

> +  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.

> +      /* If's are tricky.  */

Isn't that Ifs or IFs?

> +      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.

> +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.

> +      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?

> --- 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?

	Jakub