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