This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Implement -Wimplicit-fallthrough (version 8)
- From: Marek Polacek <polacek at redhat dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Joseph Myers <joseph at codesourcery dot com>, Jason Merrill <jason at redhat dot com>
- Date: Tue, 20 Sep 2016 16:24:35 +0200
- Subject: Re: Implement -Wimplicit-fallthrough (version 8)
- Authentication-results: sourceware.org; auth=none
- References: <20160901134049.GA3768@redhat.com> <20160909164444.GG2920@laptop.zalov.cz>
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