| Message ID | 20160727165339.GX7007@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/27/2016 10:53 AM, Marek Polacek wrote: > These are the cases where I wasn't sure if the falls through were intentional > or not. > > This patch has been tested on powerpc64le-unknown-linux-gnu, aarch64-linux-gnu, > and x86_64-redhat-linux. > > 2016-07-27 Marek Polacek <polacek@redhat.com> > > PR c/7652 > gcc/ > * config/i386/i386.c (ix86_expand_branch): Add gcc_fallthrough.. I'm pretty sure this is an intended fallthru. > * cselib.c (cselib_expand_value_rtx_1): Likewise. I'm pretty sure this is an intended fallthru. But rather than fallthru, can we just "return orig;"? If more code gets added here it'll be less and less clear if we continue to fall thru. > * gensupport.c (get_alternatives_number): Likewise. > (subst_dup): Likewise. These are definitely an intended fallthrough. E & V are essentially the same thing, except that for V the vector may be NULL. > * gimplify.c (goa_stabilize_expr): Likewise. Definitely intended fallthrough. We're "cleverly" handling the 2nd operand of binary expressions, then falling into the unary expression cases which handle the 1st operand. > * hsa-gen.c (gen_hsa_insn_for_internal_fn_call): Likewise. I think this is a bug, but please contact Martin Jambor directly on this to confirm. > * reg-stack.c (get_true_reg): Likewise. Pretty sure this is intentional. Like the cselib.c case, I'd prefer to just duplicate the code from teh fallthru path since it's trivial and makes the intent clearer. > * tree-complex.c (expand_complex_division): Likewise. No sure on this one. > * tree-data-ref.c (get_references_in_stmt): Likewise. Intentional fallthru. See how we change ref.is_read for IFN_MASK_LOAD, then test it in the fallthru path. > * tree-pretty-print.c (dump_generic_node): Likewise. ?!? This looks like a false positive from your warning. We're not falling into any case statements here AFAICT. > * var-tracking.c (adjust_mems): Likewise. Definitely an intended fallthru. Though I don't like the way this code is structured at all. > gcc/cp/ > * call.c (add_builtin_candidate): Add gcc_fallthrough. > * cxx-pretty-print.c (pp_cxx_unqualified_id): Likewise. > * parser.c (cp_parser_skip_to_end_of_statement): Likewise. > (cp_parser_cache_defarg): Likewise. No idea on these. > gcc/c-family/ > * c-ada-spec.c (dump_generic_ada_node): Add gcc_fallthrough. I don't think this is supposed to fall thru. If it falls through then it's going to issue an error about an unexpected TREE_VEC node when we were actually working on a TREE_BINFO node. I think a return 0 is appropriate here. > libcpp/ > * pch.c (write_macdef): Add CPP_FALLTHRU. > * lex.c (_cpp_lex_direct): Likewise. No idea on these. Sooo. For those where I've indicated fallthru is intentional, drop the comment and/or rewrite to avoid fallthru as indicated. You've got one that looks like a error in your warning (tree-pretty-print.c). Sync with Martin J. on the hsa warning. I think it's a bug in the hsa code, but confirm with Martin. I think the c-ada-spec should be twiddled to avoid the fallthru with a simple "return 0;" For the rest, use your patch as-is. That preserves behavior and indicates we're not sure if fallthru is appropriate or not. For cp/ feel free to ping Jason directly on those for his thoughts. jeff
On Wed, Aug 03, 2016 at 10:56:08AM -0600, Jeff Law wrote: > On 07/27/2016 10:53 AM, Marek Polacek wrote: > > These are the cases where I wasn't sure if the falls through were intentional > > or not. > > > > This patch has been tested on powerpc64le-unknown-linux-gnu, aarch64-linux-gnu, > > and x86_64-redhat-linux. > > > > 2016-07-27 Marek Polacek <polacek@redhat.com> > > > > PR c/7652 > > gcc/ > > * config/i386/i386.c (ix86_expand_branch): Add gcc_fallthrough.. > I'm pretty sure this is an intended fallthru. Ok. > > * cselib.c (cselib_expand_value_rtx_1): Likewise. > I'm pretty sure this is an intended fallthru. But rather than fallthru, can > we just "return orig;"? If more code gets added here it'll be less and less > clear if we continue to fall thru. Done in the patch I just posted. > > * gensupport.c (get_alternatives_number): Likewise. > > (subst_dup): Likewise. > These are definitely an intended fallthrough. E & V are essentially the > same thing, except that for V the vector may be NULL. Ok. > > * gimplify.c (goa_stabilize_expr): Likewise. > Definitely intended fallthrough. We're "cleverly" handling the 2nd operand > of binary expressions, then falling into the unary expression cases which > handle the 1st operand. Ok. > > * hsa-gen.c (gen_hsa_insn_for_internal_fn_call): Likewise. > I think this is a bug, but please contact Martin Jambor directly on this to > confirm. Done. > > * reg-stack.c (get_true_reg): Likewise. > Pretty sure this is intentional. Like the cselib.c case, I'd prefer to just > duplicate the code from teh fallthru path since it's trivial and makes the > intent clearer. Done. > > * tree-complex.c (expand_complex_division): Likewise. > No sure on this one. Ok, I kept it as it was. > > * tree-data-ref.c (get_references_in_stmt): Likewise. > Intentional fallthru. See how we change ref.is_read for IFN_MASK_LOAD, then > test it in the fallthru path. Ok. > > * tree-pretty-print.c (dump_generic_node): Likewise. > ?!? This looks like a false positive from your warning. We're not falling > into any case statements here AFAICT. I've fixed the warning to not warn here anymore. > > * var-tracking.c (adjust_mems): Likewise. > Definitely an intended fallthru. Though I don't like the way this code is > structured at all. Yea, it's ugly. > > gcc/cp/ > > * call.c (add_builtin_candidate): Add gcc_fallthrough. > > * cxx-pretty-print.c (pp_cxx_unqualified_id): Likewise. > > * parser.c (cp_parser_skip_to_end_of_statement): Likewise. > > (cp_parser_cache_defarg): Likewise. > No idea on these. > > > > gcc/c-family/ > > * c-ada-spec.c (dump_generic_ada_node): Add gcc_fallthrough. > I don't think this is supposed to fall thru. If it falls through then it's > going to issue an error about an unexpected TREE_VEC node when we were > actually working on a TREE_BINFO node. I think a return 0 is appropriate > here. > > > > libcpp/ > > * pch.c (write_macdef): Add CPP_FALLTHRU. > > * lex.c (_cpp_lex_direct): Likewise. > No idea on these. > > Sooo. For those where I've indicated fallthru is intentional, drop the > comment and/or rewrite to avoid fallthru as indicated. Done. > You've got one that looks like a error in your warning > (tree-pretty-print.c). Fixed. > Sync with Martin J. on the hsa warning. I think it's a bug in the hsa code, > but confirm with Martin. Done. > I think the c-ada-spec should be twiddled to avoid the fallthru with a > simple "return 0;" Done. > For the rest, use your patch as-is. That preserves behavior and indicates > we're not sure if fallthru is appropriate or not. For cp/ feel free to ping > Jason directly on those for his thoughts. Thanks a lot for reviewing this. I'll post an updated version later on. Marek
On Wed, Jul 27, 2016 at 06:53:39PM +0200, Marek Polacek wrote: > --- gcc/gcc/var-tracking.c > +++ gcc/gcc/var-tracking.c > @@ -1056,6 +1056,8 @@ adjust_mems (rtx loc, const_rtx old_rtx, void *data) > ? GET_MODE_SIZE (amd->mem_mode) > : -GET_MODE_SIZE (amd->mem_mode), > GET_MODE (loc))); > + /* XXX Really fallthru? */ > + gcc_fallthrough (); > case POST_INC: > case POST_DEC: > if (addr == loc) > @@ -1076,6 +1078,8 @@ adjust_mems (rtx loc, const_rtx old_rtx, void *data) > return addr; > case PRE_MODIFY: > addr = XEXP (loc, 1); > + /* XXX Really fallthru? */ > + gcc_fallthrough (); > case POST_MODIFY: > if (addr == loc) > addr = XEXP (loc, 0); Indeed, these two are intentional fallthrus. But, can you just add /* FALLTHRU */ comment in there, rather than gcc_fallthrough () or similar? Patch preapproved. Jakub
diff --git gcc/gcc/c-family/c-ada-spec.c gcc/gcc/c-family/c-ada-spec.c index e33fdff..659ffa0 100644 --- gcc/gcc/c-family/c-ada-spec.c +++ gcc/gcc/c-family/c-ada-spec.c @@ -1862,6 +1862,8 @@ dump_generic_ada_node (pretty_printer *buffer, tree node, tree type, int spc, case TREE_BINFO: dump_generic_ada_node (buffer, BINFO_TYPE (node), type, spc, limited_access, name_only); + /* XXX Really fallthru? */ + gcc_fallthrough (); case TREE_VEC: pp_string (buffer, "--- unexpected node: TREE_VEC"); --- gcc/gcc/config/i386/i386.c +++ gcc/gcc/config/i386/i386.c @@ -22484,6 +22495,8 @@ ix86_expand_branch (enum rtx_code code, rtx op0, rtx op1, rtx label) op0 = force_reg (mode, gen_rtx_XOR (mode, op0, op1)); op1 = const0_rtx; } + /* XXX Really fallthru? */ + gcc_fallthrough (); case TImode: /* Expand DImode branch into multiple compare+branch. */ { --- gcc/gcc/cp/call.c +++ gcc/gcc/cp/call.c @@ -2542,6 +2544,8 @@ add_builtin_candidate (struct z_candidate **candidates, enum tree_code code, type2 = ptrdiff_type_node; break; } + /* XXX Really fallthru? */ + gcc_fallthrough (); case MULT_EXPR: case TRUNC_DIV_EXPR: if (ARITHMETIC_TYPE_P (type1) && ARITHMETIC_TYPE_P (type2)) --- gcc/gcc/cp/cxx-pretty-print.c +++ gcc/gcc/cp/cxx-pretty-print.c @@ -142,6 +142,8 @@ pp_cxx_unqualified_id (cxx_pretty_printer *pp, tree t) case OVERLOAD: t = OVL_CURRENT (t); + /* XXX Really fallthru? */ + gcc_fallthrough (); case VAR_DECL: case PARM_DECL: case CONST_DECL: --- gcc/gcc/cp/parser.c +++ gcc/gcc/cp/parser.c @@ -3488,6 +3488,8 @@ cp_parser_skip_to_end_of_statement (cp_parser* parser) cp_lexer_consume_token (parser->lexer); return; } + /* XXX Really fallthru? */ + gcc_fallthrough (); case CPP_OPEN_BRACE: ++nesting_depth; @@ -27640,6 +27646,8 @@ cp_parser_cache_defarg (cp_parser *parser, bool nsdmi) parser->in_template_argument_list_p = saved_italp; break; } + /* XXX Really fallthru? */ + gcc_fallthrough (); case CPP_CLOSE_PAREN: case CPP_ELLIPSIS: /* If we run into a non-nested `;', `}', or `]', --- gcc/gcc/cselib.c +++ gcc/gcc/cselib.c @@ -1619,6 +1619,8 @@ cselib_expand_value_rtx_1 (rtx orig, struct expand_value_data *evd, return orig; } } + /* XXX Really fallthru? */ + gcc_fallthrough (); CASE_CONST_ANY: case SYMBOL_REF: diff --git gcc/gcc/gensupport.c gcc/gcc/gensupport.c index 0eb4591..795f88c 100644 --- gcc/gcc/gensupport.c +++ gcc/gcc/gensupport.c @@ -1038,6 +1038,8 @@ get_alternatives_number (rtx pattern, int *n_alt, file_location loc) case 'V': if (XVEC (pattern, i) == NULL) break; + /* XXX Really fallthru? */ + gcc_fallthrough (); case 'E': for (j = XVECLEN (pattern, i) - 1; j >= 0; --j) @@ -2156,6 +2158,8 @@ subst_dup (rtx pattern, int n_alt, int n_subst_alt) case 'V': if (XVEC (pattern, i) == NULL) break; + /* XXX Really fallthru? */ + gcc_fallthrough (); case 'E': if (code != MATCH_DUP && code != MATCH_OP_DUP) for (j = XVECLEN (pattern, i) - 1; j >= 0; --j) --- gcc/gcc/gimplify.c +++ gcc/gcc/gimplify.c @@ -10320,6 +10324,8 @@ goa_stabilize_expr (tree *expr_p, gimple_seq *pre_p, tree lhs_addr, case tcc_comparison: saw_lhs |= goa_stabilize_expr (&TREE_OPERAND (expr, 1), pre_p, lhs_addr, lhs_var); + /* XXX Really fallthru? */ + gcc_fallthrough (); case tcc_unary: saw_lhs |= goa_stabilize_expr (&TREE_OPERAND (expr, 0), pre_p, lhs_addr, lhs_var); @@ -10334,6 +10340,8 @@ goa_stabilize_expr (tree *expr_p, gimple_seq *pre_p, tree lhs_addr, case TRUTH_XOR_EXPR: saw_lhs |= goa_stabilize_expr (&TREE_OPERAND (expr, 1), pre_p, lhs_addr, lhs_var); + /* XXX Really fallthru? */ + gcc_fallthrough (); case TRUTH_NOT_EXPR: saw_lhs |= goa_stabilize_expr (&TREE_OPERAND (expr, 0), pre_p, lhs_addr, lhs_var); --- gcc/gcc/hsa-gen.c +++ gcc/gcc/hsa-gen.c @@ -5039,6 +5039,8 @@ gen_hsa_insn_for_internal_fn_call (gcall *stmt, hsa_bb *hbb) case IFN_FMIN: case IFN_FMAX: gen_hsa_insns_for_call_of_internal_fn (stmt, hbb); + /* XXX Really fallthru? */ + gcc_fallthrough (); default: HSA_SORRY_ATV (gimple_location (stmt), diff --git gcc/gcc/reg-stack.c gcc/gcc/reg-stack.c index c931349..a4a3ac2 100644 --- gcc/gcc/reg-stack.c +++ gcc/gcc/reg-stack.c @@ -423,6 +423,8 @@ get_true_reg (rtx *pat) GET_MODE (subreg)); return pat; } + /* XXX Really fallthru? */ + gcc_fallthrough (); } case FLOAT: case FIX: diff --git gcc/gcc/tree-complex.c gcc/gcc/tree-complex.c index d7baf22..9cb8986 100644 --- gcc/gcc/tree-complex.c +++ gcc/gcc/tree-complex.c @@ -1336,6 +1336,8 @@ expand_complex_division (gimple_stmt_iterator *gsi, tree inner_type, rr = gimplify_build2 (gsi, code, inner_type, ai, bi); ri = gimplify_build2 (gsi, code, inner_type, ar, bi); ri = gimplify_build1 (gsi, NEGATE_EXPR, inner_type, ri); + /* XXX Really fallthru? */ + gcc_fallthrough (); case PAIR (ONLY_REAL, VARYING): case PAIR (ONLY_IMAG, VARYING): --- gcc/gcc/tree-data-ref.c +++ gcc/gcc/tree-data-ref.c @@ -3889,7 +3890,9 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, va_heap> *references) case IFN_MASK_LOAD: if (gimple_call_lhs (stmt) == NULL_TREE) break; + /* XXX Really fallthru? */ ref.is_read = true; + gcc_fallthrough (); case IFN_MASK_STORE: ptr = build_int_cst (TREE_TYPE (gimple_call_arg (stmt, 1)), 0); align = tree_to_shwi (gimple_call_arg (stmt, 1)); --- gcc/gcc/tree-pretty-print.c +++ gcc/gcc/tree-pretty-print.c @@ -2946,6 +2947,8 @@ dump_generic_node (pretty_printer *pp, tree node, int spc, int flags, dump_omp_loop: dump_omp_clauses (pp, OMP_FOR_CLAUSES (node), spc, flags); + /* XXX Really fallthru? */ + gcc_fallthrough (); dump_omp_loop_cilk_for: if (!(flags & TDF_SLIM)) --- gcc/gcc/var-tracking.c +++ gcc/gcc/var-tracking.c @@ -1056,6 +1056,8 @@ adjust_mems (rtx loc, const_rtx old_rtx, void *data) ? GET_MODE_SIZE (amd->mem_mode) : -GET_MODE_SIZE (amd->mem_mode), GET_MODE (loc))); + /* XXX Really fallthru? */ + gcc_fallthrough (); case POST_INC: case POST_DEC: if (addr == loc) @@ -1076,6 +1078,8 @@ adjust_mems (rtx loc, const_rtx old_rtx, void *data) return addr; case PRE_MODIFY: addr = XEXP (loc, 1); + /* XXX Really fallthru? */ + gcc_fallthrough (); case POST_MODIFY: if (addr == loc) addr = XEXP (loc, 0); --- gcc/libcpp/pch.c +++ gcc/libcpp/pch.c @@ -55,6 +55,8 @@ write_macdef (cpp_reader *pfile, cpp_hashnode *hn, void *file_p) case NT_VOID: if (! (hn->flags & NODE_POISONED)) return 1; + /* XXX Really fallthru? */ + CPP_FALLTHRU; case NT_MACRO: if ((hn->flags & NODE_BUILTIN) --- gcc/libcpp/lex.c +++ gcc/libcpp/lex.c @@ -2815,6 +2817,8 @@ _cpp_lex_direct (cpp_reader *pfile) } buffer->cur++; } + /* XXX Really fallthru? */ + CPP_FALLTHRU; default: create_literal (pfile, result, buffer->cur - 1, 1, CPP_OTHER);