| Message ID | 20160711195939.GD13963@redhat.com |
|---|---|
| State | New |
| Headers | show |
I'm curious about this. In the process of developing a code analysis
tool, I found some GCC code that was, basically:
switch (v) {
case 0:
if (e) {
do_something();
} else {
case 1:
do_something_else();
}
}
Does this patch mean that the above got fixed? I mean, if you're
going to fret over linguistic tags to make falling through explicit,
it would seem the above code is pretty sore-thumby, yes?
Hi Marek, On Mon, Jul 11, 2016 at 09:59:39PM +0200, Marek Polacek wrote: > 2016-07-11 Marek Polacek <polacek@redhat.com> > > PR c/7652 > * config/rs6000/rs6000.c (rs6000_builtin_vectorized_libmass): Likewise. Likewise? Like what? :-) > --- gcc/gcc/config/rs6000/rs6000.c > +++ gcc/gcc/config/rs6000/rs6000.c > @@ -5459,6 +5459,7 @@ rs6000_builtin_vectorized_libmass (combined_fn fn, tree type_out, > CASE_CFN_POW: > n_args = 2; > /* fall through */ > + gcc_fallthrough (); As mentioned elsewhere, please remove comments saying just "fall through" when adding the new statement. > @@ -14398,6 +14401,8 @@ altivec_expand_ld_builtin (tree exp, rtx target, bool *expandedp) > break; > case ALTIVEC_BUILTIN_LD_INTERNAL_2di: > icode = CODE_FOR_vector_altivec_load_v2di; > + /* XXX Really? */ > + gcc_fallthrough (); > case ALTIVEC_BUILTIN_LD_INTERNAL_1ti: > icode = CODE_FOR_vector_altivec_load_v1ti; > break; Yeah, that's a bug obviously. We'll fix it. > @@ -30191,6 +30200,7 @@ rs6000_adjust_cost (rtx_insn *insn, rtx link, rtx_insn *dep_insn, int cost) > && (INSN_CODE (dep_insn) >= 0) > && (get_attr_type (dep_insn) == TYPE_MFFGPR)) > return 2; > + gcc_fallthrough (); > > default: > break; Better to put an extra "break" here. That is usually true if the next statement (after one or more labels) is a break. > --- gcc/gcc/config/rs6000/rs6000.md > +++ gcc/gcc/config/rs6000/rs6000.md > @@ -8094,6 +8094,8 @@ > case 0: > if (TARGET_STRING) > return \"stswi %1,%P0,16\"; > + /* XXX Really fallthru? */ > + gcc_fallthrough (); > case 1: > return \"#\"; Yes, really :-) This code is probably better written without a switch statement at all, but oh well. I like the new warning, and I'd like to see it in -Wextra. It finds real problems that are easy to correct. It does trigger a lot on existing code, but -Wextra is not -Wall. Segher
On Tue, Jul 12, 2016 at 09:10:54AM -0500, Segher Boessenkool wrote: > Hi Marek, > > On Mon, Jul 11, 2016 at 09:59:39PM +0200, Marek Polacek wrote: > > 2016-07-11 Marek Polacek <polacek@redhat.com> > > > > PR c/7652 > > * config/rs6000/rs6000.c (rs6000_builtin_vectorized_libmass): Likewise. > > Likewise? Like what? :-) Yikes, I goofed this up when spliting the big patch. > > --- gcc/gcc/config/rs6000/rs6000.c > > +++ gcc/gcc/config/rs6000/rs6000.c > > @@ -5459,6 +5459,7 @@ rs6000_builtin_vectorized_libmass (combined_fn fn, tree type_out, > > CASE_CFN_POW: > > n_args = 2; > > /* fall through */ > > + gcc_fallthrough (); > > As mentioned elsewhere, please remove comments saying just "fall through" > when adding the new statement. Yep, I'm working on this. > > @@ -14398,6 +14401,8 @@ altivec_expand_ld_builtin (tree exp, rtx target, bool *expandedp) > > break; > > case ALTIVEC_BUILTIN_LD_INTERNAL_2di: > > icode = CODE_FOR_vector_altivec_load_v2di; > > + /* XXX Really? */ > > + gcc_fallthrough (); > > case ALTIVEC_BUILTIN_LD_INTERNAL_1ti: > > icode = CODE_FOR_vector_altivec_load_v1ti; > > break; > > Yeah, that's a bug obviously. We'll fix it. Great. > > @@ -30191,6 +30200,7 @@ rs6000_adjust_cost (rtx_insn *insn, rtx link, rtx_insn *dep_insn, int cost) > > && (INSN_CODE (dep_insn) >= 0) > > && (get_attr_type (dep_insn) == TYPE_MFFGPR)) > > return 2; > > + gcc_fallthrough (); > > > > default: > > break; > > Better to put an extra "break" here. That is usually true if the next > statement (after one or more labels) is a break. The next version of the warning should recognize this scenario and shouldn't warn, thus no change will be needed. > > --- gcc/gcc/config/rs6000/rs6000.md > > +++ gcc/gcc/config/rs6000/rs6000.md > > @@ -8094,6 +8094,8 @@ > > case 0: > > if (TARGET_STRING) > > return \"stswi %1,%P0,16\"; > > + /* XXX Really fallthru? */ > > + gcc_fallthrough (); > > case 1: > > return \"#\"; > > Yes, really :-) This code is probably better written without a switch > statement at all, but oh well. Yeah, or it at least should have a comment. > I like the new warning, and I'd like to see it in -Wextra. It finds real > problems that are easy to correct. It does trigger a lot on existing code, > but -Wextra is not -Wall. The next version of the warning should be significantly less verbose, because it will take "falls through" comments into account. Thanks. Marek
On 07/12/2016 04:19 PM, Marek Polacek wrote: > >>> @@ -30191,6 +30200,7 @@ rs6000_adjust_cost (rtx_insn *insn, rtx link, rtx_insn *dep_insn, int cost) >>> && (INSN_CODE (dep_insn) >= 0) >>> && (get_attr_type (dep_insn) == TYPE_MFFGPR)) >>> return 2; >>> + gcc_fallthrough (); >>> >>> default: >>> break; >> >> Better to put an extra "break" here. That is usually true if the next >> statement (after one or more labels) is a break. > > The next version of the warning should recognize this scenario and shouldn't > warn, thus no change will be needed. Actually I think this is one case where you could unconditionally warn - falling through to a break seems like bad practice, it invites errors when someone places new code before the break. Bernd
On Tue, Jul 12, 2016 at 04:42:21PM +0200, Bernd Schmidt wrote: > On 07/12/2016 04:19 PM, Marek Polacek wrote: > > > >>>@@ -30191,6 +30200,7 @@ rs6000_adjust_cost (rtx_insn *insn, rtx link, rtx_insn *dep_insn, int cost) > >>> && (INSN_CODE (dep_insn) >= 0) > >>> && (get_attr_type (dep_insn) == TYPE_MFFGPR)) > >>> return 2; > >>>+ gcc_fallthrough (); > >>> > >>> default: > >>> break; > >> > >>Better to put an extra "break" here. That is usually true if the next > >>statement (after one or more labels) is a break. > > > >The next version of the warning should recognize this scenario and shouldn't > >warn, thus no change will be needed. > > Actually I think this is one case where you could unconditionally warn - > falling through to a break seems like bad practice, it invites errors when > someone places new code before the break. You get the warning when you place the new code there. I think the falling into a case with only break; or falling into default:;} is harmless. Jakub
On Mon, Jul 11, 2016 at 01:36:02PM -0700, Bruce Korb wrote: > I'm curious about this. In the process of developing a code analysis > tool, I found some GCC code that was, basically: > > switch (v) { > case 0: > if (e) { > do_something(); > } else { > case 1: > do_something_else(); > } > } Yeah, I saw this too, somewhere in the C++ FE. > Does this patch mean that the above got fixed? I mean, if you're > going to fret over linguistic tags to make falling through explicit, > it would seem the above code is pretty sore-thumby, yes? My current implementation warns here, but the warning can be suppressed by adding /* FALLTHRU */ or __builtin_fallthrough(); before the do_something_else() line. Does that answer your question? Marek
> On Mon, Jul 11, 2016 at 01:36:02PM -0700, Bruce Korb wrote: > [[putrid code deleted]] >> Does this patch mean that the above got fixed? I mean, if you're >> going to fret over linguistic tags to make falling through explicit, >> it would seem the above code is pretty sore-thumby, yes? > > My current implementation warns here, but the warning can be suppressed > by adding /* FALLTHRU */ or __builtin_fallthrough(); before the > do_something_else() line. > > Does that answer your question? Not in a satisfying way. :) I understand that syntactically, you can drop a "case" in wherever you can drop a statement label. That doesn't mean you should. Since I wasn't fixing GCC code, I just rolled my eyes and moved on. However, if the code is going to be changed, then contortions like that ought to be addressed. Both for aesthetics and for code clarity.
On Wed, Jul 13, 2016 at 11:30:34AM -0700, Bruce Korb wrote: > > On Mon, Jul 11, 2016 at 01:36:02PM -0700, Bruce Korb wrote: > > [[putrid code deleted]] > >> Does this patch mean that the above got fixed? I mean, if you're > >> going to fret over linguistic tags to make falling through explicit, > >> it would seem the above code is pretty sore-thumby, yes? > > > > My current implementation warns here, but the warning can be suppressed > > by adding /* FALLTHRU */ or __builtin_fallthrough(); before the > > do_something_else() line. > > > > Does that answer your question? > > Not in a satisfying way. :) I understand that syntactically, you can Ah, sorry. > drop a "case" in wherever you can drop a statement label. That > doesn't mean you should. Since I wasn't fixing GCC code, I just > rolled my eyes and moved on. However, if the code is going to be > changed, then contortions like that ought to be addressed. Both for > aesthetics and for code clarity. Most likely what you saw was in cxx_pretty_printer::declaration_specifiers. I will have to do something about this, either add /* FALLTHRU */ or rewrite the code slightly, as you suggest. But that will involve duplicating the c_pretty_printer::declaration_specifiers (t); line, so some people might be against it. Marek
On Wed, Jul 13, 2016 at 11:39 AM, Marek Polacek <polacek@redhat.com> wrote:
> Most likely what you saw was in cxx_pretty_printer::declaration_specifiers.
I only saw it once and, of course, it was once too often. ;)
If you fix it, it would sooth my sensibilities as the fixincludes maintainer,
making mine a small voice in the GCC world.
Still, for what it's worth, I'd back you :)
Cheers - Bruce
Actually, it occurs to me: On Wed, Jul 13, 2016 at 11:23 AM, Marek Polacek <polacek@redhat.com> wrote: > My current implementation warns here, but the warning can be suppressed > by adding /* FALLTHRU */ or [...] that the traditional "lint-ean" spelling is "/* FALLTHROUGH */", so why would the abbrev be accepted and the full spelling not? I think "FALLTHRU" was added a mere 20 years ago and I go back farther than that. (40+, actually).
On Wed, Jul 13, 2016 at 03:35:10PM -0700, Bruce Korb wrote: > Actually, it occurs to me: > > On Wed, Jul 13, 2016 at 11:23 AM, Marek Polacek <polacek@redhat.com> wrote: > > My current implementation warns here, but the warning can be suppressed > > by adding /* FALLTHRU */ or [...] > > that the traditional "lint-ean" spelling is "/* FALLTHROUGH */", so > why would the abbrev be accepted and the full spelling not? I think > "FALLTHRU" was added a mere 20 years ago and I go back farther than > that. (40+, actually). We plan to support wide range of such "falls through" comments, see the list here <https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00647.html>. We're hoping this will be enough. Thanks, Marek
diff --git gcc/gcc/config/rs6000/rs6000.c gcc/gcc/config/rs6000/rs6000.c index dd77e1b..a41d6be 100644 --- gcc/gcc/config/rs6000/rs6000.c +++ gcc/gcc/config/rs6000/rs6000.c @@ -5459,6 +5459,7 @@ rs6000_builtin_vectorized_libmass (combined_fn fn, tree type_out, CASE_CFN_POW: n_args = 2; /* fall through */ + gcc_fallthrough (); CASE_CFN_ACOS: CASE_CFN_ACOSH: @@ -7675,6 +7676,7 @@ rs6000_legitimate_offset_address_p (machine_mode mode, rtx x, return (SPE_CONST_OFFSET_OK (offset) && SPE_CONST_OFFSET_OK (offset + 8)); /* fall through */ + gcc_fallthrough (); case TDmode: case TImode: @@ -9778,6 +9780,7 @@ rs6000_emit_move (rtx dest, rtx source, machine_mode mode) if (FLOAT128_2REG_P (mode)) rs6000_eliminate_indexed_memrefs (operands); /* fall through */ + gcc_fallthrough (); case DFmode: case DDmode: @@ -14398,6 +14401,8 @@ altivec_expand_ld_builtin (tree exp, rtx target, bool *expandedp) break; case ALTIVEC_BUILTIN_LD_INTERNAL_2di: icode = CODE_FOR_vector_altivec_load_v2di; + /* XXX Really? */ + gcc_fallthrough (); case ALTIVEC_BUILTIN_LD_INTERNAL_1ti: icode = CODE_FOR_vector_altivec_load_v1ti; break; @@ -14459,6 +14464,8 @@ altivec_expand_st_builtin (tree exp, rtx target ATTRIBUTE_UNUSED, break; case ALTIVEC_BUILTIN_ST_INTERNAL_2di: icode = CODE_FOR_vector_altivec_store_v2di; + /* XXX Really? */ + gcc_fallthrough (); case ALTIVEC_BUILTIN_ST_INTERNAL_1ti: icode = CODE_FOR_vector_altivec_store_v1ti; break; @@ -20961,6 +20968,7 @@ print_operand (FILE *file, rtx x, int code) return; fputc (',', file); /* FALLTHRU */ + gcc_fallthrough (); case 'R': /* X is a CR register. Print the mask for `mtcrf'. */ @@ -22532,6 +22540,7 @@ rs6000_emit_vector_compare_inner (enum rtx_code code, rtx op0, rtx op1) case GE: if (GET_MODE_CLASS (mode) == MODE_VECTOR_INT) return NULL_RTX; + gcc_fallthrough (); case EQ: case GT: @@ -30191,6 +30200,7 @@ rs6000_adjust_cost (rtx_insn *insn, rtx link, rtx_insn *dep_insn, int cost) && (INSN_CODE (dep_insn) >= 0) && (get_attr_type (dep_insn) == TYPE_MFFGPR)) return 2; + gcc_fallthrough (); default: break; @@ -30227,6 +30237,7 @@ rs6000_adjust_cost (rtx_insn *insn, rtx link, rtx_insn *dep_insn, int cost) } } /* Fall through, no cost for output dependency. */ + gcc_fallthrough (); case REG_DEP_ANTI: /* Anti dependency; DEP_INSN reads a register that INSN writes some @@ -31373,6 +31384,7 @@ insn_must_be_first_in_group (rtx_insn *insn) case PROCESSOR_POWER5: if (is_cracked_insn (insn)) return true; + gcc_fallthrough (); case PROCESSOR_POWER4: if (is_microcoded_insn (insn)) return true; @@ -32290,6 +32302,7 @@ rs6000_handle_altivec_attribute (tree *node, case V4SImode: case V8HImode: case V16QImode: case V4SFmode: case V2DImode: case V2DFmode: result = type; + gcc_fallthrough (); default: break; } break; @@ -32300,6 +32313,7 @@ rs6000_handle_altivec_attribute (tree *node, case SImode: case V4SImode: result = bool_V4SI_type_node; break; case HImode: case V8HImode: result = bool_V8HI_type_node; break; case QImode: case V16QImode: result = bool_V16QI_type_node; + gcc_fallthrough (); default: break; } break; @@ -32307,6 +32321,7 @@ rs6000_handle_altivec_attribute (tree *node, switch (mode) { case V8HImode: result = pixel_V8HI_type_node; + gcc_fallthrough (); default: break; } default: break; @@ -33912,6 +33927,7 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code, return true; } /* FALLTHRU */ + gcc_fallthrough (); case CONST_DOUBLE: case CONST_WIDE_INT: @@ -33973,6 +33989,7 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code, return false; } /* FALLTHRU */ + gcc_fallthrough (); case UDIV: case UMOD: @@ -34086,6 +34103,7 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code, return false; } /* fall through */ + gcc_fallthrough (); case ASHIFTRT: case LSHIFTRT: @@ -34124,6 +34142,7 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code, return false; } /* FALLTHRU */ + gcc_fallthrough (); case FLOAT: case UNSIGNED_FLOAT: @@ -34196,6 +34215,7 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code, } } /* FALLTHRU */ + gcc_fallthrough (); case GT: case LT: @@ -34970,6 +34990,7 @@ altivec_expand_vec_perm_const (rtx operands[4]) if (!rtx_equal_p (op0, op1)) break; /* FALLTHRU */ + gcc_fallthrough (); case 2: for (i = 0; i < 16; ++i) @@ -38540,6 +38561,7 @@ rtx_is_swappable_p (rtx op, unsigned int *special) return 1; } } + gcc_fallthrough (); default: break; diff --git gcc/gcc/config/rs6000/rs6000.md gcc/gcc/config/rs6000/rs6000.md index a7615b1..3d3cfaa 100644 --- gcc/gcc/config/rs6000/rs6000.md +++ gcc/gcc/config/rs6000/rs6000.md @@ -8094,6 +8094,8 @@ case 0: if (TARGET_STRING) return \"stswi %1,%P0,16\"; + /* XXX Really fallthru? */ + gcc_fallthrough (); case 1: return \"#\"; case 2: @@ -8103,6 +8105,7 @@ && ! reg_overlap_mentioned_p (operands[0], operands[1])) return \"lswi %0,%P1,16\"; /* ... fall through ... */ + gcc_fallthrough (); case 3: case 4: case 5: