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