diff mbox series

Avoid some -Wunreachable-code-ctrl

Message ID q2839s9-o1s-s07s-8orp-47244q7o0o8@fhfr.qr
State Committed
Commit 2acbc4eba33574a5e655c01d1be8b17fad0be535
Headers show
Series Avoid some -Wunreachable-code-ctrl | expand

Commit Message

Richard Biener Nov. 29, 2021, 3:03 p.m. UTC
This cleans up unreachable code diagnosed by -Wunreachable-code-ctrl.
It largely follows the previous series but discovers a few extra
cases, namely dead code after break or continue or loops without
exits.

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

Richard.

2021-11-29  Richard Biener  <rguenther@suse.de>

gcc/c/
	* gimple-parser.c (c_parser_gimple_postfix_expression):
	avoid unreachable code after break.

gcc/
	* cfgrtl.c (skip_insns_after_block): Refactor code to
	be more easily readable.
	* expr.c (op_by_pieces_d::run): Remove unreachable
	assert.
	* sched-deps.c (sched_analyze): Remove unreachable
	gcc_unreachable.
	* sel-sched-ir.c (in_same_ebb_p): Likewise.
	* tree-ssa-alias.c (nonoverlapping_refs_since_match_p):
	Remove unreachable code.
	* tree-vect-slp.c (vectorize_slp_instance_root_stmt):
	Refactor to avoid unreachable loop iteration.
	* tree.c (walk_tree_1): Remove unreachable break.
	* vec-perm-indices.c (vec_perm_indices::series_p): Remove
	unreachable return.

gcc/cp/
	* parser.c (cp_parser_postfix_expression): Remove
	unreachable code.
	* pt.c (tsubst_expr): Remove unreachable breaks.

gcc/fortran/
	* frontend-passes.c (gfc_expr_walker): Remove unreachable
	break.
	* scanner.c (skip_fixed_comments): Remove unreachable
	gcc_unreachable.
	* trans-expr.c (gfc_expr_is_variable): Refactor to make
	control flow more obvious.
---
 gcc/c/gimple-parser.c         |  8 +-------
 gcc/cfgrtl.c                  | 10 ++--------
 gcc/cp/parser.c               |  4 ----
 gcc/cp/pt.c                   |  2 --
 gcc/expr.c                    |  3 ---
 gcc/fortran/frontend-passes.c |  1 -
 gcc/fortran/scanner.c         |  1 -
 gcc/fortran/trans-expr.c      | 11 +++--------
 gcc/sched-deps.c              |  2 --
 gcc/sel-sched-ir.c            |  3 ---
 gcc/tree-ssa-alias.c          |  3 ---
 gcc/tree-vect-slp.c           | 22 ++++++++--------------
 gcc/tree.c                    |  2 --
 gcc/vec-perm-indices.c        |  1 -
 14 files changed, 14 insertions(+), 59 deletions(-)

Comments

Jeff Law Nov. 29, 2021, 3:53 p.m. UTC | #1
On 11/29/2021 8:03 AM, Richard Biener via Gcc-patches wrote:
> This cleans up unreachable code diagnosed by -Wunreachable-code-ctrl.
> It largely follows the previous series but discovers a few extra
> cases, namely dead code after break or continue or loops without
> exits.
>
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
>
> Richard.
>
> 2021-11-29  Richard Biener  <rguenther@suse.de>
>
> gcc/c/
> 	* gimple-parser.c (c_parser_gimple_postfix_expression):
> 	avoid unreachable code after break.
>
> gcc/
> 	* cfgrtl.c (skip_insns_after_block): Refactor code to
> 	be more easily readable.
> 	* expr.c (op_by_pieces_d::run): Remove unreachable
> 	assert.
> 	* sched-deps.c (sched_analyze): Remove unreachable
> 	gcc_unreachable.
> 	* sel-sched-ir.c (in_same_ebb_p): Likewise.
> 	* tree-ssa-alias.c (nonoverlapping_refs_since_match_p):
> 	Remove unreachable code.
> 	* tree-vect-slp.c (vectorize_slp_instance_root_stmt):
> 	Refactor to avoid unreachable loop iteration.
> 	* tree.c (walk_tree_1): Remove unreachable break.
> 	* vec-perm-indices.c (vec_perm_indices::series_p): Remove
> 	unreachable return.
>
> gcc/cp/
> 	* parser.c (cp_parser_postfix_expression): Remove
> 	unreachable code.
> 	* pt.c (tsubst_expr): Remove unreachable breaks.
>
> gcc/fortran/
> 	* frontend-passes.c (gfc_expr_walker): Remove unreachable
> 	break.
> 	* scanner.c (skip_fixed_comments): Remove unreachable
> 	gcc_unreachable.
> 	* trans-expr.c (gfc_expr_is_variable): Refactor to make
> 	control flow more obvious.
OK
jeff
Mikael Morin Nov. 30, 2021, 1 p.m. UTC | #2
Le 29/11/2021 à 16:03, Richard Biener via Gcc-patches a écrit :
> diff --git a/gcc/fortran/frontend-passes.c b/gcc/fortran/frontend-passes.c
> index f5ba7cecd54..16ee2afc9c0 100644
> --- a/gcc/fortran/frontend-passes.c
> +++ b/gcc/fortran/frontend-passes.c
> @@ -5229,7 +5229,6 @@ gfc_expr_walker (gfc_expr **e, walk_expr_fn_t exprfn, void *data)
>   	  case EXPR_OP:
>   	    WALK_SUBEXPR ((*e)->value.op.op1);
>   	    WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
> -	    break;
>   	  case EXPR_FUNCTION:
>   	    for (a = (*e)->value.function.actual; a; a = a->next)
>   	      WALK_SUBEXPR (a->expr);

I’m uncomfortable with the above change.
It makes it look like there is a fall through, but there is not.
Maybe inline the macro to make the continue explicit, or use 
WALK_SUBEXPR instead of WALK_SUBEXPR_TAIL and hope the compiler will do 
the tail call optimization.

Mikael
Richard Biener Nov. 30, 2021, 1:25 p.m. UTC | #3
On Tue, 30 Nov 2021, Mikael Morin wrote:

> Le 29/11/2021 à 16:03, Richard Biener via Gcc-patches a écrit :
> > diff --git a/gcc/fortran/frontend-passes.c b/gcc/fortran/frontend-passes.c
> > index f5ba7cecd54..16ee2afc9c0 100644
> > --- a/gcc/fortran/frontend-passes.c
> > +++ b/gcc/fortran/frontend-passes.c
> > @@ -5229,7 +5229,6 @@ gfc_expr_walker (gfc_expr **e, walk_expr_fn_t exprfn,
> > void *data)
> >      case EXPR_OP:
> >        WALK_SUBEXPR ((*e)->value.op.op1);
> >        WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
> > -	    break;
> >      case EXPR_FUNCTION:
> >        for (a = (*e)->value.function.actual; a; a = a->next)
> >          WALK_SUBEXPR (a->expr);
> 
> I’m uncomfortable with the above change.
> It makes it look like there is a fall through, but there is not.
> Maybe inline the macro to make the continue explicit, or use WALK_SUBEXPR
> instead of WALK_SUBEXPR_TAIL and hope the compiler will do the tail call
> optimization.

Ah, it follows the style in tree.c:walk_tree_1 where break was used
inconsistently after WALK_SUBTREE_TAIL which was then more obvious
to me to clean up.  I didn't realize the fortran FE only had a 
single WALK_SUBEXPR_TAIL.

I'm not sure inlining will make the situation more clear, for
sure using WALK_SUBEXPR would but it might loose the tailcall.

Would you accept an additional comment after WALK_SUBEXPR_TAIL like

          case EXPR_OP:
            WALK_SUBEXPR ((*e)->value.op.op1);
            WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
            /* tail-recurse  */

?  Btw, a fallthru would be diagnosed by GCC unless we put

            /* Fallthru  */

here.  Maybe renaming WALK_SUBEXPR_TAIL to WALK_SUBEXPR_WITH_CONTINUE
or WALK_SUBEXPR_BY_TAIL_RECURSING or WALK_SUBEXPR_TAILRECURSE would
be more obvious?

Thanks,
Richard.
Mikael Morin Nov. 30, 2021, 2:18 p.m. UTC | #4
On 30/11/2021 14:25, Richard Biener wrote:
> On Tue, 30 Nov 2021, Mikael Morin wrote:
> 
>> Le 29/11/2021 à 16:03, Richard Biener via Gcc-patches a écrit :
>>> diff --git a/gcc/fortran/frontend-passes.c b/gcc/fortran/frontend-passes.c
>>> index f5ba7cecd54..16ee2afc9c0 100644
>>> --- a/gcc/fortran/frontend-passes.c
>>> +++ b/gcc/fortran/frontend-passes.c
>>> @@ -5229,7 +5229,6 @@ gfc_expr_walker (gfc_expr **e, walk_expr_fn_t exprfn,
>>> void *data)
>>>       case EXPR_OP:
>>>         WALK_SUBEXPR ((*e)->value.op.op1);
>>>         WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
>>> -	    break;
>>>       case EXPR_FUNCTION:
>>>         for (a = (*e)->value.function.actual; a; a = a->next)
>>>           WALK_SUBEXPR (a->expr);
>>
>> I’m uncomfortable with the above change.
>> It makes it look like there is a fall through, but there is not.
>> Maybe inline the macro to make the continue explicit, or use WALK_SUBEXPR
>> instead of WALK_SUBEXPR_TAIL and hope the compiler will do the tail call
>> optimization.
> 
> Ah, it follows the style in tree.c:walk_tree_1 where break was used
> inconsistently after WALK_SUBTREE_TAIL which was then more obvious
> to me to clean up.  I didn't realize the fortran FE only had a
> single WALK_SUBEXPR_TAIL.
> 
> I'm not sure inlining will make the situation more clear, for
> sure using WALK_SUBEXPR would but it might loose the tailcall.
> 
> Would you accept an additional comment after WALK_SUBEXPR_TAIL like
> 
>            case EXPR_OP:
>              WALK_SUBEXPR ((*e)->value.op.op1);
>              WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
>              /* tail-recurse  */
> 
My preference would be a gcc_unreachable() or something similar, but I 
understand it would get a warning as well?

Without better idea, I’m fine with an even more explicit comment:

     /* No fallthru because of the tail recursion above.  */

> ?  Btw, a fallthru would be diagnosed by GCC unless we put
> 
>              /* Fallthru  */
> 
> here.
Sure, but my main concern was misreading from programmers (including 
me), which is not diagnosed by compilers.

>  Maybe renaming WALK_SUBEXPR_TAIL to WALK_SUBEXPR_WITH_CONTINUE
> or WALK_SUBEXPR_BY_TAIL_RECURSING or WALK_SUBEXPR_TAILRECURSE would
> be more obvious?
> 
I think the comment above would be enough.

Thanks.
Richard Biener Nov. 30, 2021, 2:27 p.m. UTC | #5
On Tue, 30 Nov 2021, Mikael Morin wrote:

> On 30/11/2021 14:25, Richard Biener wrote:
> > On Tue, 30 Nov 2021, Mikael Morin wrote:
> > 
> >> Le 29/11/2021 ? 16:03, Richard Biener via Gcc-patches a ?crit :
> >>> diff --git a/gcc/fortran/frontend-passes.c b/gcc/fortran/frontend-passes.c
> >>> index f5ba7cecd54..16ee2afc9c0 100644
> >>> --- a/gcc/fortran/frontend-passes.c
> >>> +++ b/gcc/fortran/frontend-passes.c
> >>> @@ -5229,7 +5229,6 @@ gfc_expr_walker (gfc_expr **e, walk_expr_fn_t
> >>> exprfn,
> >>> void *data)
> >>>       case EXPR_OP:
> >>>         WALK_SUBEXPR ((*e)->value.op.op1);
> >>>         WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
> >>> -	    break;
> >>>       case EXPR_FUNCTION:
> >>>         for (a = (*e)->value.function.actual; a; a = a->next)
> >>>           WALK_SUBEXPR (a->expr);
> >>
> >> I?m uncomfortable with the above change.
> >> It makes it look like there is a fall through, but there is not.
> >> Maybe inline the macro to make the continue explicit, or use WALK_SUBEXPR
> >> instead of WALK_SUBEXPR_TAIL and hope the compiler will do the tail call
> >> optimization.
> > 
> > Ah, it follows the style in tree.c:walk_tree_1 where break was used
> > inconsistently after WALK_SUBTREE_TAIL which was then more obvious
> > to me to clean up.  I didn't realize the fortran FE only had a
> > single WALK_SUBEXPR_TAIL.
> > 
> > I'm not sure inlining will make the situation more clear, for
> > sure using WALK_SUBEXPR would but it might loose the tailcall.
> > 
> > Would you accept an additional comment after WALK_SUBEXPR_TAIL like
> > 
> >            case EXPR_OP:
> >              WALK_SUBEXPR ((*e)->value.op.op1);
> >              WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
> >              /* tail-recurse  */
> > 
> My preference would be a gcc_unreachable() or something similar, but I
> understand it would get a warning as well?
> 
> Without better idea, I?m fine with an even more explicit comment:
> 
>     /* No fallthru because of the tail recursion above.  */
> 
> > ?  Btw, a fallthru would be diagnosed by GCC unless we put
> > 
> >              /* Fallthru  */
> > 
> > here.
> Sure, but my main concern was misreading from programmers (including me),
> which is not diagnosed by compilers.
> 
> > Maybe renaming WALK_SUBEXPR_TAIL to WALK_SUBEXPR_WITH_CONTINUE
> > or WALK_SUBEXPR_BY_TAIL_RECURSING or WALK_SUBEXPR_TAILRECURSE would
> > be more obvious?
> > 
> I think the comment above would be enough.

Installed as follows.

Richard.

From e5c2a436ef7596d254ffefd279742382b1ff546b Mon Sep 17 00:00:00 2001
From: Richard Biener <rguenther@suse.de>
Date: Tue, 30 Nov 2021 15:25:17 +0100
Subject: [PATCH] Add comment to indicate tail recursion
To: gcc-patches@gcc.gnu.org

My previous change removed an unreachable break; there (an
unreachable continue; would have been more to the point).  The
following re-adds a comment explaining that WALK_SUBEXPR_TAIL
does not fall through but tail recurses.

2021-11-30  Richard Biener  <rguenther@suse.de>

gcc/fortran/
	* frontend-passes.c (gfc_expr_walker): Add comment to
	indicate tail recursion.
---
 gcc/fortran/frontend-passes.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gcc/fortran/frontend-passes.c b/gcc/fortran/frontend-passes.c
index 16ee2afc9c0..4764c834f4f 100644
--- a/gcc/fortran/frontend-passes.c
+++ b/gcc/fortran/frontend-passes.c
@@ -5229,6 +5229,7 @@ gfc_expr_walker (gfc_expr **e, walk_expr_fn_t exprfn, void *data)
 	  case EXPR_OP:
 	    WALK_SUBEXPR ((*e)->value.op.op1);
 	    WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
+	    /* No fallthru because of the tail recursion above.  */
 	  case EXPR_FUNCTION:
 	    for (a = (*e)->value.function.actual; a; a = a->next)
 	      WALK_SUBEXPR (a->expr);
Jason Merrill Nov. 30, 2021, 10:16 p.m. UTC | #6
On 11/29/21 10:03, Richard Biener via Gcc-patches wrote:
> This cleans up unreachable code diagnosed by -Wunreachable-code-ctrl.
> It largely follows the previous series but discovers a few extra
> cases, namely dead code after break or continue or loops without
> exits.
> 
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> 
> Richard.
> 
> 2021-11-29  Richard Biener  <rguenther@suse.de>
> 
> gcc/c/
> 	* gimple-parser.c (c_parser_gimple_postfix_expression):
> 	avoid unreachable code after break.
> 
> gcc/
> 	* cfgrtl.c (skip_insns_after_block): Refactor code to
> 	be more easily readable.
> 	* expr.c (op_by_pieces_d::run): Remove unreachable
> 	assert.
> 	* sched-deps.c (sched_analyze): Remove unreachable
> 	gcc_unreachable.
> 	* sel-sched-ir.c (in_same_ebb_p): Likewise.
> 	* tree-ssa-alias.c (nonoverlapping_refs_since_match_p):
> 	Remove unreachable code.
> 	* tree-vect-slp.c (vectorize_slp_instance_root_stmt):
> 	Refactor to avoid unreachable loop iteration.
> 	* tree.c (walk_tree_1): Remove unreachable break.
> 	* vec-perm-indices.c (vec_perm_indices::series_p): Remove
> 	unreachable return.
> 
> gcc/cp/
> 	* parser.c (cp_parser_postfix_expression): Remove
> 	unreachable code.
> 	* pt.c (tsubst_expr): Remove unreachable breaks.
> 
> gcc/fortran/
> 	* frontend-passes.c (gfc_expr_walker): Remove unreachable
> 	break.
> 	* scanner.c (skip_fixed_comments): Remove unreachable
> 	gcc_unreachable.
> 	* trans-expr.c (gfc_expr_is_variable): Refactor to make
> 	control flow more obvious.
> ---
>   gcc/c/gimple-parser.c         |  8 +-------
>   gcc/cfgrtl.c                  | 10 ++--------
>   gcc/cp/parser.c               |  4 ----
>   gcc/cp/pt.c                   |  2 --
>   gcc/expr.c                    |  3 ---
>   gcc/fortran/frontend-passes.c |  1 -
>   gcc/fortran/scanner.c         |  1 -
>   gcc/fortran/trans-expr.c      | 11 +++--------
>   gcc/sched-deps.c              |  2 --
>   gcc/sel-sched-ir.c            |  3 ---
>   gcc/tree-ssa-alias.c          |  3 ---
>   gcc/tree-vect-slp.c           | 22 ++++++++--------------
>   gcc/tree.c                    |  2 --
>   gcc/vec-perm-indices.c        |  1 -
>   14 files changed, 14 insertions(+), 59 deletions(-)
> 
> diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c
> index 32f22dbb8a7..f594a8ccb31 100644
> --- a/gcc/c/gimple-parser.c
> +++ b/gcc/c/gimple-parser.c
> @@ -1698,13 +1698,7 @@ c_parser_gimple_postfix_expression (gimple_parser &parser)
>   	    }
>   	  break;
>   	}
> -      else
> -	{
> -	  c_parser_error (parser, "expected expression");
> -	  expr.set_error ();
> -	  break;
> -	}
> -      break;
> +      /* Fallthru.  */
>       default:
>         c_parser_error (parser, "expected expression");
>         expr.set_error ();
> diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
> index 3744adcc2ba..287a3db643a 100644
> --- a/gcc/cfgrtl.c
> +++ b/gcc/cfgrtl.c
> @@ -3539,14 +3539,8 @@ skip_insns_after_block (basic_block bb)
>   	  continue;
>   
>   	case NOTE:
> -	  switch (NOTE_KIND (insn))
> -	    {
> -	    case NOTE_INSN_BLOCK_END:
> -	      gcc_unreachable ();
> -	    default:
> -	      continue;
> -	    }
> -	  break;
> +	  gcc_assert (NOTE_KIND (insn) != NOTE_INSN_BLOCK_END);
> +	  continue;
>   
>   	case CODE_LABEL:
>   	  if (NEXT_INSN (insn)
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index 0bd58525726..cc88a36dd39 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -7892,10 +7892,6 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
>               return postfix_expression;
>   	}
>       }
> -
> -  /* We should never get here.  */
> -  gcc_unreachable ();

Hmm, I generally disagree with removing gcc_unreachable() asserts 
because they are unreachable; it seems like it increases the fragility 
of the code in case later changes wrongly make them reachable.

> -  return error_mark_node;
>   }
>   
>   /* Helper function for cp_parser_parenthesized_expression_list and
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 31ed773e145..f4b9d9673fb 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -18242,13 +18242,11 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
>         stmt = finish_co_yield_expr (input_location,
>   				   RECUR (TREE_OPERAND (t, 0)));
>         RETURN (stmt);
> -      break;
>   
>       case CO_AWAIT_EXPR:
>         stmt = finish_co_await_expr (input_location,
>   				   RECUR (TREE_OPERAND (t, 0)));
>         RETURN (stmt);
> -      break;
>   
>       case EXPR_STMT:
>         tmp = RECUR (EXPR_STMT_EXPR (t));
> diff --git a/gcc/expr.c b/gcc/expr.c
> index 5673902b1fc..b2815257509 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -1342,9 +1342,6 @@ op_by_pieces_d::run ()
>   	}
>       }
>     while (1);
> -
> -  /* The code above should have handled everything.  */
> -  gcc_assert (!length);
>   }
>   
>   /* Derived class from op_by_pieces_d, providing support for block move
> diff --git a/gcc/fortran/frontend-passes.c b/gcc/fortran/frontend-passes.c
> index f5ba7cecd54..16ee2afc9c0 100644
> --- a/gcc/fortran/frontend-passes.c
> +++ b/gcc/fortran/frontend-passes.c
> @@ -5229,7 +5229,6 @@ gfc_expr_walker (gfc_expr **e, walk_expr_fn_t exprfn, void *data)
>   	  case EXPR_OP:
>   	    WALK_SUBEXPR ((*e)->value.op.op1);
>   	    WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
> -	    break;
>   	  case EXPR_FUNCTION:
>   	    for (a = (*e)->value.function.actual; a; a = a->next)
>   	      WALK_SUBEXPR (a->expr);
> diff --git a/gcc/fortran/scanner.c b/gcc/fortran/scanner.c
> index 69b81ab97f8..4d72ff78543 100644
> --- a/gcc/fortran/scanner.c
> +++ b/gcc/fortran/scanner.c
> @@ -1159,7 +1159,6 @@ skip_fixed_comments (void)
>   	  skip_comment_line ();
>   	  continue;
>   
> -	  gcc_unreachable ();
>   check_for_digits:
>   	  {
>   	    /* Required for OpenMP's conditional compilation sentinel. */
> diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
> index bc502c0f43c..e413b2d7a1f 100644
> --- a/gcc/fortran/trans-expr.c
> +++ b/gcc/fortran/trans-expr.c
> @@ -11079,14 +11079,9 @@ gfc_expr_is_variable (gfc_expr *expr)
>   	  func_ifc = expr->value.function.esym;
>   	  goto found_ifc;
>   	}
> -      else
> -	{
> -	  gcc_assert (expr->symtree);
> -	  func_ifc = expr->symtree->n.sym;
> -	  goto found_ifc;
> -	}
> -
> -      gcc_unreachable ();
> +      gcc_assert (expr->symtree);
> +      func_ifc = expr->symtree->n.sym;
> +      goto found_ifc;
>       }
>   
>     comp = gfc_get_proc_ptr_comp (expr);
> diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
> index 5814204c681..62aa47a73bd 100644
> --- a/gcc/sched-deps.c
> +++ b/gcc/sched-deps.c
> @@ -3816,7 +3816,6 @@ sched_analyze (class deps_desc *deps, rtx_insn *head, rtx_insn *tail)
>   
>     for (insn = head;; insn = NEXT_INSN (insn))
>       {
> -
>         if (INSN_P (insn))
>   	{
>   	  /* And initialize deps_lists.  */
> @@ -3836,7 +3835,6 @@ sched_analyze (class deps_desc *deps, rtx_insn *head, rtx_insn *tail)
>   	  return;
>   	}
>       }
> -  gcc_unreachable ();
>   }
>   
>   /* Helper for sched_free_deps ().
> diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
> index 48965bfb0ad..b76a48eb16e 100644
> --- a/gcc/sel-sched-ir.c
> +++ b/gcc/sel-sched-ir.c
> @@ -4918,9 +4918,6 @@ in_same_ebb_p (insn_t insn, insn_t succ)
>   
>         ptr = bb_next_bb (ptr);
>       }
> -
> -  gcc_unreachable ();
> -  return false;
>   }
>   
>   /* Recomputes the reverse topological order for the function and
> diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
> index 3c253e2843f..88fd7821c5e 100644
> --- a/gcc/tree-ssa-alias.c
> +++ b/gcc/tree-ssa-alias.c
> @@ -1800,9 +1800,6 @@ nonoverlapping_refs_since_match_p (tree match1, tree ref1,
>   	  return 1;
>   	}
>       }
> -
> -  ++alias_stats.nonoverlapping_refs_since_match_p_must_overlap;
> -  return 0;
>   }
>   
>   /* Return TYPE_UID which can be used to match record types we consider
> diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
> index 94c75497495..cfea0c80be3 100644
> --- a/gcc/tree-vect-slp.c
> +++ b/gcc/tree-vect-slp.c
> @@ -7311,20 +7311,14 @@ vectorize_slp_instance_root_stmt (slp_tree node, slp_instance instance)
>       {
>         if (SLP_TREE_NUMBER_OF_VEC_STMTS (node) == 1)
>   	{
> -	  gimple *child_stmt;
> -	  int j;
> -
> -	  FOR_EACH_VEC_ELT (SLP_TREE_VEC_STMTS (node), j, child_stmt)
> -	    {
> -	      tree vect_lhs = gimple_get_lhs (child_stmt);
> -	      tree root_lhs = gimple_get_lhs (instance->root_stmts[0]->stmt);
> -	      if (!useless_type_conversion_p (TREE_TYPE (root_lhs),
> -					      TREE_TYPE (vect_lhs)))
> -		vect_lhs = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (root_lhs),
> -				   vect_lhs);
> -	      rstmt = gimple_build_assign (root_lhs, vect_lhs);
> -	      break;
> -	    }
> +	  gimple *child_stmt = SLP_TREE_VEC_STMTS (node)[0];
> +	  tree vect_lhs = gimple_get_lhs (child_stmt);
> +	  tree root_lhs = gimple_get_lhs (instance->root_stmts[0]->stmt);
> +	  if (!useless_type_conversion_p (TREE_TYPE (root_lhs),
> +					  TREE_TYPE (vect_lhs)))
> +	    vect_lhs = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (root_lhs),
> +			       vect_lhs);
> +	  rstmt = gimple_build_assign (root_lhs, vect_lhs);
>   	}
>         else if (SLP_TREE_NUMBER_OF_VEC_STMTS (node) > 1)
>   	{
> diff --git a/gcc/tree.c b/gcc/tree.c
> index 910fb06d6f5..4d91fdea758 100644
> --- a/gcc/tree.c
> +++ b/gcc/tree.c
> @@ -11127,7 +11127,6 @@ walk_tree_1 (tree *tp, walk_tree_fn func, void *data,
>       case TREE_LIST:
>         WALK_SUBTREE (TREE_VALUE (*tp));
>         WALK_SUBTREE_TAIL (TREE_CHAIN (*tp));
> -      break;
>   
>       case TREE_VEC:
>         {
> @@ -11206,7 +11205,6 @@ walk_tree_1 (tree *tp, walk_tree_fn func, void *data,
>   	  WALK_SUBTREE (OMP_CLAUSE_OPERAND (*tp, i));
>   	WALK_SUBTREE_TAIL (OMP_CLAUSE_CHAIN (*tp));
>         }
> -      break;
>   
>       case TARGET_EXPR:
>         {
> diff --git a/gcc/vec-perm-indices.c b/gcc/vec-perm-indices.c
> index 31b32ea0589..9e6b5af327c 100644
> --- a/gcc/vec-perm-indices.c
> +++ b/gcc/vec-perm-indices.c
> @@ -228,7 +228,6 @@ vec_perm_indices::series_p (unsigned int out_base, unsigned int out_step,
>   
>         out_base += out_step;
>       }
> -  return true;
>   }
>   
>   /* Return true if all elements of the permutation vector are in the range
>
Richard Biener Dec. 1, 2021, 7:57 a.m. UTC | #7
On Tue, 30 Nov 2021, Jason Merrill wrote:

> On 11/29/21 10:03, Richard Biener via Gcc-patches wrote:
> > This cleans up unreachable code diagnosed by -Wunreachable-code-ctrl.
> > It largely follows the previous series but discovers a few extra
> > cases, namely dead code after break or continue or loops without
> > exits.
> > 
> > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> > 
> > Richard.
> > 
> > 2021-11-29  Richard Biener  <rguenther@suse.de>
> > 
> > gcc/c/
> >  * gimple-parser.c (c_parser_gimple_postfix_expression):
> >  avoid unreachable code after break.
> > 
> > gcc/
> >  * cfgrtl.c (skip_insns_after_block): Refactor code to
> >  be more easily readable.
> >  * expr.c (op_by_pieces_d::run): Remove unreachable
> >  assert.
> >  * sched-deps.c (sched_analyze): Remove unreachable
> >  gcc_unreachable.
> >  * sel-sched-ir.c (in_same_ebb_p): Likewise.
> >  * tree-ssa-alias.c (nonoverlapping_refs_since_match_p):
> >  Remove unreachable code.
> >  * tree-vect-slp.c (vectorize_slp_instance_root_stmt):
> >  Refactor to avoid unreachable loop iteration.
> >  * tree.c (walk_tree_1): Remove unreachable break.
> >  * vec-perm-indices.c (vec_perm_indices::series_p): Remove
> >  unreachable return.
> > 
> > gcc/cp/
> >  * parser.c (cp_parser_postfix_expression): Remove
> >  unreachable code.
> >  * pt.c (tsubst_expr): Remove unreachable breaks.
> > 
> > gcc/fortran/
> >  * frontend-passes.c (gfc_expr_walker): Remove unreachable
> >  break.
> >  * scanner.c (skip_fixed_comments): Remove unreachable
> >  gcc_unreachable.
> >  * trans-expr.c (gfc_expr_is_variable): Refactor to make
> >  control flow more obvious.
> > ---
> >   gcc/c/gimple-parser.c         |  8 +-------
> >   gcc/cfgrtl.c                  | 10 ++--------
> >   gcc/cp/parser.c               |  4 ----
> >   gcc/cp/pt.c                   |  2 --
> >   gcc/expr.c                    |  3 ---
> >   gcc/fortran/frontend-passes.c |  1 -
> >   gcc/fortran/scanner.c         |  1 -
> >   gcc/fortran/trans-expr.c      | 11 +++--------
> >   gcc/sched-deps.c              |  2 --
> >   gcc/sel-sched-ir.c            |  3 ---
> >   gcc/tree-ssa-alias.c          |  3 ---
> >   gcc/tree-vect-slp.c           | 22 ++++++++--------------
> >   gcc/tree.c                    |  2 --
> >   gcc/vec-perm-indices.c        |  1 -
> >   14 files changed, 14 insertions(+), 59 deletions(-)
> > 
> > diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c
> > index 32f22dbb8a7..f594a8ccb31 100644
> > --- a/gcc/c/gimple-parser.c
> > +++ b/gcc/c/gimple-parser.c
> > @@ -1698,13 +1698,7 @@ c_parser_gimple_postfix_expression (gimple_parser
> > &parser)
> >        }
> >      break;
> >   	}
> > -      else
> > -	{
> > -	  c_parser_error (parser, "expected expression");
> > -	  expr.set_error ();
> > -	  break;
> > -	}
> > -      break;
> > +      /* Fallthru.  */
> >       default:
> >         c_parser_error (parser, "expected expression");
> >         expr.set_error ();
> > diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
> > index 3744adcc2ba..287a3db643a 100644
> > --- a/gcc/cfgrtl.c
> > +++ b/gcc/cfgrtl.c
> > @@ -3539,14 +3539,8 @@ skip_insns_after_block (basic_block bb)
> >      continue;
> >   
> >   	case NOTE:
> > -	  switch (NOTE_KIND (insn))
> > -	    {
> > -	    case NOTE_INSN_BLOCK_END:
> > -	      gcc_unreachable ();
> > -	    default:
> > -	      continue;
> > -	    }
> > -	  break;
> > +	  gcc_assert (NOTE_KIND (insn) != NOTE_INSN_BLOCK_END);
> > +	  continue;
> >   
> >    case CODE_LABEL:
> >   	  if (NEXT_INSN (insn)
> > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> > index 0bd58525726..cc88a36dd39 100644
> > --- a/gcc/cp/parser.c
> > +++ b/gcc/cp/parser.c
> > @@ -7892,10 +7892,6 @@ cp_parser_postfix_expression (cp_parser *parser, bool
> > address_p, bool cast_p,
> >                return postfix_expression;
> >    }
> >       }
> > -
> > -  /* We should never get here.  */
> > -  gcc_unreachable ();
> 
> Hmm, I generally disagree with removing gcc_unreachable() asserts because they
> are unreachable; it seems like it increases the fragility of the code in case
> later changes wrongly make them reachable.

It seems to be quite inconsistently used in the code base though.  Do
you suggest the coding conventions to be amended with something like

"If a function returns non-void and the last statement in the outermost
lexical scope is not a return statement you must add a gcc_unreachable ()
call at this place."

?  Most definitely most functions do _not_ follow this.

The case above involves

  while (true)
    {
      loop without exit (break or goto)
    }
}

if somebody would add code doing a break and not add a return he'd
get a diagnostic now.  With the gcc_unreachable () in place he'd
_not_ get a diagnostic but maybe some ICEs at compile-time for some
testcase we'd yet have to discover.

Not sure what I think is better ;)

I'm sure we do not want to diagnose

 "warning: gcc_unreachable () might be reached"

correct?  So placing a gcc_unreachable, while catching errors at
runtime, will suppress diagnostics at compile time.

Richard.

> > -  return error_mark_node;
> >   }
> >   
> >   /* Helper function for cp_parser_parenthesized_expression_list and
> > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > index 31ed773e145..f4b9d9673fb 100644
> > --- a/gcc/cp/pt.c
> > +++ b/gcc/cp/pt.c
> > @@ -18242,13 +18242,11 @@ tsubst_expr (tree t, tree args, tsubst_flags_t
> > complain, tree in_decl,
> >         stmt = finish_co_yield_expr (input_location,
> >         				   RECUR (TREE_OPERAND (t, 0)));
> >         RETURN (stmt);
> > -      break;
> >   
> >       case CO_AWAIT_EXPR:
> >         stmt = finish_co_await_expr (input_location,
> >         				   RECUR (TREE_OPERAND (t, 0)));
> >         RETURN (stmt);
> > -      break;
> >   
> >       case EXPR_STMT:
> >         tmp = RECUR (EXPR_STMT_EXPR (t));
> > diff --git a/gcc/expr.c b/gcc/expr.c
> > index 5673902b1fc..b2815257509 100644
> > --- a/gcc/expr.c
> > +++ b/gcc/expr.c
> > @@ -1342,9 +1342,6 @@ op_by_pieces_d::run ()
> >    }
> >       }
> >     while (1);
> > -
> > -  /* The code above should have handled everything.  */
> > -  gcc_assert (!length);
> >   }
> >   
> >   /* Derived class from op_by_pieces_d, providing support for block move
> > diff --git a/gcc/fortran/frontend-passes.c b/gcc/fortran/frontend-passes.c
> > index f5ba7cecd54..16ee2afc9c0 100644
> > --- a/gcc/fortran/frontend-passes.c
> > +++ b/gcc/fortran/frontend-passes.c
> > @@ -5229,7 +5229,6 @@ gfc_expr_walker (gfc_expr **e, walk_expr_fn_t exprfn,
> > void *data)
> >      case EXPR_OP:
> >        WALK_SUBEXPR ((*e)->value.op.op1);
> >        WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
> > -	    break;
> >      case EXPR_FUNCTION:
> >        for (a = (*e)->value.function.actual; a; a = a->next)
> >   	      WALK_SUBEXPR (a->expr);
> > diff --git a/gcc/fortran/scanner.c b/gcc/fortran/scanner.c
> > index 69b81ab97f8..4d72ff78543 100644
> > --- a/gcc/fortran/scanner.c
> > +++ b/gcc/fortran/scanner.c
> > @@ -1159,7 +1159,6 @@ skip_fixed_comments (void)
> >      skip_comment_line ();
> >      continue;
> >   -	  gcc_unreachable ();
> >   check_for_digits:
> >      {
> >   	    /* Required for OpenMP's conditional compilation sentinel. */
> > diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
> > index bc502c0f43c..e413b2d7a1f 100644
> > --- a/gcc/fortran/trans-expr.c
> > +++ b/gcc/fortran/trans-expr.c
> > @@ -11079,14 +11079,9 @@ gfc_expr_is_variable (gfc_expr *expr)
> >      func_ifc = expr->value.function.esym;
> >      goto found_ifc;
> >   	}
> > -      else
> > -	{
> > -	  gcc_assert (expr->symtree);
> > -	  func_ifc = expr->symtree->n.sym;
> > -	  goto found_ifc;
> > -	}
> > -
> > -      gcc_unreachable ();
> > +      gcc_assert (expr->symtree);
> > +      func_ifc = expr->symtree->n.sym;
> > +      goto found_ifc;
> >       }
> >   
> >     comp = gfc_get_proc_ptr_comp (expr);
> > diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
> > index 5814204c681..62aa47a73bd 100644
> > --- a/gcc/sched-deps.c
> > +++ b/gcc/sched-deps.c
> > @@ -3816,7 +3816,6 @@ sched_analyze (class deps_desc *deps, rtx_insn *head,
> > rtx_insn *tail)
> >   
> >     for (insn = head;; insn = NEXT_INSN (insn))
> >       {
> > -
> >          if (INSN_P (insn))
> >    {
> >   	  /* And initialize deps_lists.  */
> > @@ -3836,7 +3835,6 @@ sched_analyze (class deps_desc *deps, rtx_insn *head,
> > rtx_insn *tail)
> >      return;
> >    }
> >       }
> > -  gcc_unreachable ();
> >   }
> >   
> >   /* Helper for sched_free_deps ().
> > diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
> > index 48965bfb0ad..b76a48eb16e 100644
> > --- a/gcc/sel-sched-ir.c
> > +++ b/gcc/sel-sched-ir.c
> > @@ -4918,9 +4918,6 @@ in_same_ebb_p (insn_t insn, insn_t succ)
> >   
> >         ptr = bb_next_bb (ptr);
> >       }
> > -
> > -  gcc_unreachable ();
> > -  return false;
> >   }
> >   
> >   /* Recomputes the reverse topological order for the function and
> > diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
> > index 3c253e2843f..88fd7821c5e 100644
> > --- a/gcc/tree-ssa-alias.c
> > +++ b/gcc/tree-ssa-alias.c
> > @@ -1800,9 +1800,6 @@ nonoverlapping_refs_since_match_p (tree match1, tree
> > ref1,
> >      return 1;
> >    }
> >       }
> > -
> > -  ++alias_stats.nonoverlapping_refs_since_match_p_must_overlap;
> > -  return 0;
> >   }
> >   
> >   /* Return TYPE_UID which can be used to match record types we consider
> > diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
> > index 94c75497495..cfea0c80be3 100644
> > --- a/gcc/tree-vect-slp.c
> > +++ b/gcc/tree-vect-slp.c
> > @@ -7311,20 +7311,14 @@ vectorize_slp_instance_root_stmt (slp_tree node,
> > slp_instance instance)
> >       {
> >          if (SLP_TREE_NUMBER_OF_VEC_STMTS (node) == 1)
> >   	{
> > -	  gimple *child_stmt;
> > -	  int j;
> > -
> > -	  FOR_EACH_VEC_ELT (SLP_TREE_VEC_STMTS (node), j, child_stmt)
> > -	    {
> > -	      tree vect_lhs = gimple_get_lhs (child_stmt);
> > -	      tree root_lhs = gimple_get_lhs (instance->root_stmts[0]->stmt);
> > -	      if (!useless_type_conversion_p (TREE_TYPE (root_lhs),
> > -					      TREE_TYPE (vect_lhs)))
> > -		vect_lhs = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (root_lhs),
> > -				   vect_lhs);
> > -	      rstmt = gimple_build_assign (root_lhs, vect_lhs);
> > -	      break;
> > -	    }
> > +	  gimple *child_stmt = SLP_TREE_VEC_STMTS (node)[0];
> > +	  tree vect_lhs = gimple_get_lhs (child_stmt);
> > +	  tree root_lhs = gimple_get_lhs (instance->root_stmts[0]->stmt);
> > +	  if (!useless_type_conversion_p (TREE_TYPE (root_lhs),
> > +					  TREE_TYPE (vect_lhs)))
> > +	    vect_lhs = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (root_lhs),
> > +			       vect_lhs);
> > +	  rstmt = gimple_build_assign (root_lhs, vect_lhs);
> >    }
> >          else if (SLP_TREE_NUMBER_OF_VEC_STMTS (node) > 1)
> >   	{
> > diff --git a/gcc/tree.c b/gcc/tree.c
> > index 910fb06d6f5..4d91fdea758 100644
> > --- a/gcc/tree.c
> > +++ b/gcc/tree.c
> > @@ -11127,7 +11127,6 @@ walk_tree_1 (tree *tp, walk_tree_fn func, void
> > *data,
> >       case TREE_LIST:
> >         WALK_SUBTREE (TREE_VALUE (*tp));
> >         WALK_SUBTREE_TAIL (TREE_CHAIN (*tp));
> > -      break;
> >   
> >       case TREE_VEC:
> >         {
> > @@ -11206,7 +11205,6 @@ walk_tree_1 (tree *tp, walk_tree_fn func, void
> > *data,
> >      WALK_SUBTREE (OMP_CLAUSE_OPERAND (*tp, i));
> >    WALK_SUBTREE_TAIL (OMP_CLAUSE_CHAIN (*tp));
> >         }
> > -      break;
> >   
> >       case TARGET_EXPR:
> >         {
> > diff --git a/gcc/vec-perm-indices.c b/gcc/vec-perm-indices.c
> > index 31b32ea0589..9e6b5af327c 100644
> > --- a/gcc/vec-perm-indices.c
> > +++ b/gcc/vec-perm-indices.c
> > @@ -228,7 +228,6 @@ vec_perm_indices::series_p (unsigned int out_base,
> > unsigned int out_step,
> >   
> >         out_base += out_step;
> >       }
> > -  return true;
> >   }
> >   
> >   /* Return true if all elements of the permutation vector are in the range
> > 
> 
> 
>
Jason Merrill Dec. 2, 2021, 8:47 p.m. UTC | #8
On 12/1/21 02:57, Richard Biener wrote:
> On Tue, 30 Nov 2021, Jason Merrill wrote:
> 
>> On 11/29/21 10:03, Richard Biener via Gcc-patches wrote:
>>> This cleans up unreachable code diagnosed by -Wunreachable-code-ctrl.
>>> It largely follows the previous series but discovers a few extra
>>> cases, namely dead code after break or continue or loops without
>>> exits.
>>>
>>> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
>>>
>>> Richard.
>>>
>>> 2021-11-29  Richard Biener  <rguenther@suse.de>
>>>
>>> gcc/c/
>>>   * gimple-parser.c (c_parser_gimple_postfix_expression):
>>>   avoid unreachable code after break.
>>>
>>> gcc/
>>>   * cfgrtl.c (skip_insns_after_block): Refactor code to
>>>   be more easily readable.
>>>   * expr.c (op_by_pieces_d::run): Remove unreachable
>>>   assert.
>>>   * sched-deps.c (sched_analyze): Remove unreachable
>>>   gcc_unreachable.
>>>   * sel-sched-ir.c (in_same_ebb_p): Likewise.
>>>   * tree-ssa-alias.c (nonoverlapping_refs_since_match_p):
>>>   Remove unreachable code.
>>>   * tree-vect-slp.c (vectorize_slp_instance_root_stmt):
>>>   Refactor to avoid unreachable loop iteration.
>>>   * tree.c (walk_tree_1): Remove unreachable break.
>>>   * vec-perm-indices.c (vec_perm_indices::series_p): Remove
>>>   unreachable return.
>>>
>>> gcc/cp/
>>>   * parser.c (cp_parser_postfix_expression): Remove
>>>   unreachable code.
>>>   * pt.c (tsubst_expr): Remove unreachable breaks.
>>>
>>> gcc/fortran/
>>>   * frontend-passes.c (gfc_expr_walker): Remove unreachable
>>>   break.
>>>   * scanner.c (skip_fixed_comments): Remove unreachable
>>>   gcc_unreachable.
>>>   * trans-expr.c (gfc_expr_is_variable): Refactor to make
>>>   control flow more obvious.
>>> ---
>>>    gcc/c/gimple-parser.c         |  8 +-------
>>>    gcc/cfgrtl.c                  | 10 ++--------
>>>    gcc/cp/parser.c               |  4 ----
>>>    gcc/cp/pt.c                   |  2 --
>>>    gcc/expr.c                    |  3 ---
>>>    gcc/fortran/frontend-passes.c |  1 -
>>>    gcc/fortran/scanner.c         |  1 -
>>>    gcc/fortran/trans-expr.c      | 11 +++--------
>>>    gcc/sched-deps.c              |  2 --
>>>    gcc/sel-sched-ir.c            |  3 ---
>>>    gcc/tree-ssa-alias.c          |  3 ---
>>>    gcc/tree-vect-slp.c           | 22 ++++++++--------------
>>>    gcc/tree.c                    |  2 --
>>>    gcc/vec-perm-indices.c        |  1 -
>>>    14 files changed, 14 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c
>>> index 32f22dbb8a7..f594a8ccb31 100644
>>> --- a/gcc/c/gimple-parser.c
>>> +++ b/gcc/c/gimple-parser.c
>>> @@ -1698,13 +1698,7 @@ c_parser_gimple_postfix_expression (gimple_parser
>>> &parser)
>>>         }
>>>       break;
>>>    	}
>>> -      else
>>> -	{
>>> -	  c_parser_error (parser, "expected expression");
>>> -	  expr.set_error ();
>>> -	  break;
>>> -	}
>>> -      break;
>>> +      /* Fallthru.  */
>>>        default:
>>>          c_parser_error (parser, "expected expression");
>>>          expr.set_error ();
>>> diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
>>> index 3744adcc2ba..287a3db643a 100644
>>> --- a/gcc/cfgrtl.c
>>> +++ b/gcc/cfgrtl.c
>>> @@ -3539,14 +3539,8 @@ skip_insns_after_block (basic_block bb)
>>>       continue;
>>>    
>>>    	case NOTE:
>>> -	  switch (NOTE_KIND (insn))
>>> -	    {
>>> -	    case NOTE_INSN_BLOCK_END:
>>> -	      gcc_unreachable ();
>>> -	    default:
>>> -	      continue;
>>> -	    }
>>> -	  break;
>>> +	  gcc_assert (NOTE_KIND (insn) != NOTE_INSN_BLOCK_END);
>>> +	  continue;
>>>    
>>>     case CODE_LABEL:
>>>    	  if (NEXT_INSN (insn)
>>> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
>>> index 0bd58525726..cc88a36dd39 100644
>>> --- a/gcc/cp/parser.c
>>> +++ b/gcc/cp/parser.c
>>> @@ -7892,10 +7892,6 @@ cp_parser_postfix_expression (cp_parser *parser, bool
>>> address_p, bool cast_p,
>>>                 return postfix_expression;
>>>     }
>>>        }
>>> -
>>> -  /* We should never get here.  */
>>> -  gcc_unreachable ();
>>
>> Hmm, I generally disagree with removing gcc_unreachable() asserts because they
>> are unreachable; it seems like it increases the fragility of the code in case
>> later changes wrongly make them reachable.
> 
> It seems to be quite inconsistently used in the code base though.  Do
> you suggest the coding conventions to be amended with something like
> 
> "If a function returns non-void and the last statement in the outermost
> lexical scope is not a return statement you must add a gcc_unreachable ()
> call at this place."
> 
> ?  Most definitely most functions do _not_ follow this.

I think there's room for patterns that are neither required nor 
prohibited.  :)

> The case above involves
> 
>    while (true)
>      {
>        loop without exit (break or goto)
>      }
> }
> 
> if somebody would add code doing a break and not add a return he'd
> get a diagnostic now.  With the gcc_unreachable () in place he'd
> _not_ get a diagnostic but maybe some ICEs at compile-time for some
> testcase we'd yet have to discover.
> 
> Not sure what I think is better ;)

An error at compile time is generally better than a run-time abort, but 
I'm not confident that we would give an error for all functions where 
it's possible to flow off the end under some circumstances, and the 
abort would catch them.

> I'm sure we do not want to diagnose
> 
>   "warning: gcc_unreachable () might be reached"
> 
> correct?  So placing a gcc_unreachable, while catching errors at
> runtime, will suppress diagnostics at compile time.

I see gcc_unreachable as essentially the same as any other assert, in 
that it documents an assumption made by the (lack of) code that follows.

It seems entirely reasonable to me for the compiler to warn that a 
function will *always* call fancy_abort.  I wouldn't nomally warn about 
it being possibly reachable when we don't know the constraints on the input.

I'm not strongly against these changes, but in general I'm skeptical of 
removing defensive code just because it isn't currently reachable.

Jason
diff mbox series

Patch

diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c
index 32f22dbb8a7..f594a8ccb31 100644
--- a/gcc/c/gimple-parser.c
+++ b/gcc/c/gimple-parser.c
@@ -1698,13 +1698,7 @@  c_parser_gimple_postfix_expression (gimple_parser &parser)
 	    }
 	  break;
 	}
-      else
-	{
-	  c_parser_error (parser, "expected expression");
-	  expr.set_error ();
-	  break;
-	}
-      break;
+      /* Fallthru.  */
     default:
       c_parser_error (parser, "expected expression");
       expr.set_error ();
diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
index 3744adcc2ba..287a3db643a 100644
--- a/gcc/cfgrtl.c
+++ b/gcc/cfgrtl.c
@@ -3539,14 +3539,8 @@  skip_insns_after_block (basic_block bb)
 	  continue;
 
 	case NOTE:
-	  switch (NOTE_KIND (insn))
-	    {
-	    case NOTE_INSN_BLOCK_END:
-	      gcc_unreachable ();
-	    default:
-	      continue;
-	    }
-	  break;
+	  gcc_assert (NOTE_KIND (insn) != NOTE_INSN_BLOCK_END);
+	  continue;
 
 	case CODE_LABEL:
 	  if (NEXT_INSN (insn)
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 0bd58525726..cc88a36dd39 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -7892,10 +7892,6 @@  cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
             return postfix_expression;
 	}
     }
-
-  /* We should never get here.  */
-  gcc_unreachable ();
-  return error_mark_node;
 }
 
 /* Helper function for cp_parser_parenthesized_expression_list and
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 31ed773e145..f4b9d9673fb 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -18242,13 +18242,11 @@  tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
       stmt = finish_co_yield_expr (input_location,
 				   RECUR (TREE_OPERAND (t, 0)));
       RETURN (stmt);
-      break;
 
     case CO_AWAIT_EXPR:
       stmt = finish_co_await_expr (input_location,
 				   RECUR (TREE_OPERAND (t, 0)));
       RETURN (stmt);
-      break;
 
     case EXPR_STMT:
       tmp = RECUR (EXPR_STMT_EXPR (t));
diff --git a/gcc/expr.c b/gcc/expr.c
index 5673902b1fc..b2815257509 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -1342,9 +1342,6 @@  op_by_pieces_d::run ()
 	}
     }
   while (1);
-
-  /* The code above should have handled everything.  */
-  gcc_assert (!length);
 }
 
 /* Derived class from op_by_pieces_d, providing support for block move
diff --git a/gcc/fortran/frontend-passes.c b/gcc/fortran/frontend-passes.c
index f5ba7cecd54..16ee2afc9c0 100644
--- a/gcc/fortran/frontend-passes.c
+++ b/gcc/fortran/frontend-passes.c
@@ -5229,7 +5229,6 @@  gfc_expr_walker (gfc_expr **e, walk_expr_fn_t exprfn, void *data)
 	  case EXPR_OP:
 	    WALK_SUBEXPR ((*e)->value.op.op1);
 	    WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
-	    break;
 	  case EXPR_FUNCTION:
 	    for (a = (*e)->value.function.actual; a; a = a->next)
 	      WALK_SUBEXPR (a->expr);
diff --git a/gcc/fortran/scanner.c b/gcc/fortran/scanner.c
index 69b81ab97f8..4d72ff78543 100644
--- a/gcc/fortran/scanner.c
+++ b/gcc/fortran/scanner.c
@@ -1159,7 +1159,6 @@  skip_fixed_comments (void)
 	  skip_comment_line ();
 	  continue;
 
-	  gcc_unreachable ();
 check_for_digits:
 	  {
 	    /* Required for OpenMP's conditional compilation sentinel. */
diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
index bc502c0f43c..e413b2d7a1f 100644
--- a/gcc/fortran/trans-expr.c
+++ b/gcc/fortran/trans-expr.c
@@ -11079,14 +11079,9 @@  gfc_expr_is_variable (gfc_expr *expr)
 	  func_ifc = expr->value.function.esym;
 	  goto found_ifc;
 	}
-      else
-	{
-	  gcc_assert (expr->symtree);
-	  func_ifc = expr->symtree->n.sym;
-	  goto found_ifc;
-	}
-
-      gcc_unreachable ();
+      gcc_assert (expr->symtree);
+      func_ifc = expr->symtree->n.sym;
+      goto found_ifc;
     }
 
   comp = gfc_get_proc_ptr_comp (expr);
diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
index 5814204c681..62aa47a73bd 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -3816,7 +3816,6 @@  sched_analyze (class deps_desc *deps, rtx_insn *head, rtx_insn *tail)
 
   for (insn = head;; insn = NEXT_INSN (insn))
     {
-
       if (INSN_P (insn))
 	{
 	  /* And initialize deps_lists.  */
@@ -3836,7 +3835,6 @@  sched_analyze (class deps_desc *deps, rtx_insn *head, rtx_insn *tail)
 	  return;
 	}
     }
-  gcc_unreachable ();
 }
 
 /* Helper for sched_free_deps ().
diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index 48965bfb0ad..b76a48eb16e 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -4918,9 +4918,6 @@  in_same_ebb_p (insn_t insn, insn_t succ)
 
       ptr = bb_next_bb (ptr);
     }
-
-  gcc_unreachable ();
-  return false;
 }
 
 /* Recomputes the reverse topological order for the function and
diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
index 3c253e2843f..88fd7821c5e 100644
--- a/gcc/tree-ssa-alias.c
+++ b/gcc/tree-ssa-alias.c
@@ -1800,9 +1800,6 @@  nonoverlapping_refs_since_match_p (tree match1, tree ref1,
 	  return 1;
 	}
     }
-
-  ++alias_stats.nonoverlapping_refs_since_match_p_must_overlap;
-  return 0;
 }
 
 /* Return TYPE_UID which can be used to match record types we consider
diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
index 94c75497495..cfea0c80be3 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -7311,20 +7311,14 @@  vectorize_slp_instance_root_stmt (slp_tree node, slp_instance instance)
     {
       if (SLP_TREE_NUMBER_OF_VEC_STMTS (node) == 1)
 	{
-	  gimple *child_stmt;
-	  int j;
-
-	  FOR_EACH_VEC_ELT (SLP_TREE_VEC_STMTS (node), j, child_stmt)
-	    {
-	      tree vect_lhs = gimple_get_lhs (child_stmt);
-	      tree root_lhs = gimple_get_lhs (instance->root_stmts[0]->stmt);
-	      if (!useless_type_conversion_p (TREE_TYPE (root_lhs),
-					      TREE_TYPE (vect_lhs)))
-		vect_lhs = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (root_lhs),
-				   vect_lhs);
-	      rstmt = gimple_build_assign (root_lhs, vect_lhs);
-	      break;
-	    }
+	  gimple *child_stmt = SLP_TREE_VEC_STMTS (node)[0];
+	  tree vect_lhs = gimple_get_lhs (child_stmt);
+	  tree root_lhs = gimple_get_lhs (instance->root_stmts[0]->stmt);
+	  if (!useless_type_conversion_p (TREE_TYPE (root_lhs),
+					  TREE_TYPE (vect_lhs)))
+	    vect_lhs = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (root_lhs),
+			       vect_lhs);
+	  rstmt = gimple_build_assign (root_lhs, vect_lhs);
 	}
       else if (SLP_TREE_NUMBER_OF_VEC_STMTS (node) > 1)
 	{
diff --git a/gcc/tree.c b/gcc/tree.c
index 910fb06d6f5..4d91fdea758 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -11127,7 +11127,6 @@  walk_tree_1 (tree *tp, walk_tree_fn func, void *data,
     case TREE_LIST:
       WALK_SUBTREE (TREE_VALUE (*tp));
       WALK_SUBTREE_TAIL (TREE_CHAIN (*tp));
-      break;
 
     case TREE_VEC:
       {
@@ -11206,7 +11205,6 @@  walk_tree_1 (tree *tp, walk_tree_fn func, void *data,
 	  WALK_SUBTREE (OMP_CLAUSE_OPERAND (*tp, i));
 	WALK_SUBTREE_TAIL (OMP_CLAUSE_CHAIN (*tp));
       }
-      break;
 
     case TARGET_EXPR:
       {
diff --git a/gcc/vec-perm-indices.c b/gcc/vec-perm-indices.c
index 31b32ea0589..9e6b5af327c 100644
--- a/gcc/vec-perm-indices.c
+++ b/gcc/vec-perm-indices.c
@@ -228,7 +228,6 @@  vec_perm_indices::series_p (unsigned int out_base, unsigned int out_step,
 
       out_base += out_step;
     }
-  return true;
 }
 
 /* Return true if all elements of the permutation vector are in the range