diff mbox series

Remove more stray returns and gcc_unreachable ()s

Message ID 416nn390-9q5o-6sq0-4p83-nop7q87op6pr@fhfr.qr
State Committed
Commit fa01e206c87581186f64f4500f926cdb70549de0
Headers show
Series Remove more stray returns and gcc_unreachable ()s | expand

Commit Message

Richard Biener Nov. 29, 2021, 1:09 p.m. UTC
This removes more cases that appear when bootstrap with
-Wunreachable-code-return progresses.

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

Richard.

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

	* config/i386/i386.c (ix86_shift_rotate_cost): Remove
	unreachable return.
	* tree-chrec.c (evolution_function_is_invariant_rec_p):
	Likewise.
	* tree-if-conv.c (if_convertible_stmt_p): Likewise.
	* tree-ssa-pre.c (fully_constant_expression): Likewise.
	* tree-vrp.c (operand_less_p): Likewise.
	* reload.c (reg_overlap_mentioned_for_reload_p): Remove
	unreachable gcc_unreachable ().
	* sel-sched-ir.c (bb_next_bb): Likewise.
	* varasm.c (compare_constant): Likewise.

gcc/cp/
	* logic.cc (cnf_size_r): Remove unreachable and inconsistently
	placed gcc_unreachable ()s.
	* pt.c (iterative_hash_template_arg): Remove unreachable
	gcc_unreachable and return.

gcc/fortran/
	* target-memory.c (gfc_element_size): Remove unreachable return.

gcc/objc/
	* objc-act.c (objc_build_setter_call): Remove unreachable
	return.

libcpp/
	* charset.c (convert_escape): Remove unreachable break.
---
 gcc/config/i386/i386.c      | 1 -
 gcc/cp/logic.cc             | 2 --
 gcc/cp/pt.c                 | 3 ---
 gcc/fortran/target-memory.c | 1 -
 gcc/objc/objc-act.c         | 3 ---
 gcc/reload.c                | 7 +++----
 gcc/sel-sched-ir.h          | 2 --
 gcc/tree-chrec.c            | 2 --
 gcc/tree-if-conv.c          | 2 --
 gcc/tree-ssa-pre.c          | 1 -
 gcc/tree-vrp.c              | 2 --
 gcc/varasm.c                | 2 --
 libcpp/charset.c            | 1 -
 13 files changed, 3 insertions(+), 26 deletions(-)

Comments

Martin Sebor Nov. 29, 2021, 6:53 p.m. UTC | #1
On 11/29/21 6:09 AM, Richard Biener via Gcc-patches wrote:
> This removes more cases that appear when bootstrap with
> -Wunreachable-code-return progresses.
> 
...
> diff --git a/gcc/sel-sched-ir.h b/gcc/sel-sched-ir.h
> index 8ee0529d5a8..18e03c4cb96 100644
> --- a/gcc/sel-sched-ir.h
> +++ b/gcc/sel-sched-ir.h
> @@ -1493,8 +1493,6 @@ bb_next_bb (basic_block bb)
>       default:
>         return bb->next_bb;
>       }
> -
> -  gcc_unreachable ();
>   }

Just skiming the changes out of curiosity, this one makes me
wonder if the warning shouldn't be taught to avoid triggering
on calls to __builtin_unreachable().  They can help make code
more readable (e.g., after a case and switch statement that
handles all values).

Martin
Martin Sebor Nov. 29, 2021, 7:04 p.m. UTC | #2
On 11/29/21 11:53 AM, Martin Sebor wrote:
> On 11/29/21 6:09 AM, Richard Biener via Gcc-patches wrote:
>> This removes more cases that appear when bootstrap with
>> -Wunreachable-code-return progresses.
>>
> ...
>> diff --git a/gcc/sel-sched-ir.h b/gcc/sel-sched-ir.h
>> index 8ee0529d5a8..18e03c4cb96 100644
>> --- a/gcc/sel-sched-ir.h
>> +++ b/gcc/sel-sched-ir.h
>> @@ -1493,8 +1493,6 @@ bb_next_bb (basic_block bb)
>>       default:
>>         return bb->next_bb;
>>       }
>> -
>> -  gcc_unreachable ();
>>   }
> 
> Just skiming the changes out of curiosity, this one makes me
> wonder if the warning shouldn't be taught to avoid triggering
> on calls to __builtin_unreachable().  They can help make code
> more readable (e.g., after a case and switch statement that
> handles all values).

I see someone else raised the same question in a patch I hadn't
gotten to yet:

https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585431.html

If you do end up removing the gcc_unreachable() calls, I would
suggest to replace them with a comment so as not to lose
the readability benefit.

But I still wonder if it might make sense to teach the warning
not just about __builtin_unreachable() but also about noreturn
calls like abort() that (as you explained in the thread above)
gcc_unreachable() expands to.  Is there a benefit to warning
on such calls?

> 
> Martin
Richard Biener Nov. 30, 2021, 7:51 a.m. UTC | #3
On Mon, 29 Nov 2021, Martin Sebor wrote:

> On 11/29/21 11:53 AM, Martin Sebor wrote:
> > On 11/29/21 6:09 AM, Richard Biener via Gcc-patches wrote:
> >> This removes more cases that appear when bootstrap with
> >> -Wunreachable-code-return progresses.
> >>
> > ...
> >> diff --git a/gcc/sel-sched-ir.h b/gcc/sel-sched-ir.h
> >> index 8ee0529d5a8..18e03c4cb96 100644
> >> --- a/gcc/sel-sched-ir.h
> >> +++ b/gcc/sel-sched-ir.h
> >> @@ -1493,8 +1493,6 @@ bb_next_bb (basic_block bb)
> >>       default:
> >>         return bb->next_bb;
> >>       }
> >> -
> >> -  gcc_unreachable ();
> >>   }
> > 
> > Just skiming the changes out of curiosity, this one makes me
> > wonder if the warning shouldn't be taught to avoid triggering
> > on calls to __builtin_unreachable().  They can help make code
> > more readable (e.g., after a case and switch statement that
> > handles all values).
> 
> I see someone else raised the same question in a patch I hadn't
> gotten to yet:
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585431.html
> 
> If you do end up removing the gcc_unreachable() calls, I would
> suggest to replace them with a comment so as not to lose
> the readability benefit.
> 
> But I still wonder if it might make sense to teach the warning
> not just about __builtin_unreachable() but also about noreturn
> calls like abort() that (as you explained in the thread above)
> gcc_unreachable() expands to.  Is there a benefit to warning
> on such calls?

I'm not sure.  I've chosen to eliminate only the "obvious"
cases, like above where there's a default: that returns immediately
visible (not always in the patch context).  I've left some in
the code base where that's not so obvious.

IMHO making the flow obvious without a unreachable marker is
superior to obfuscating it and clearing that up with one.

Yes, I thought about not diagnosing things like

   return 1;
   return 1;

but then what about

   return 1;
   return 0;

?  I've seen cases like

   gcc_unreachable ();
   return 0;

was that meant to be

   return 0;
   gcc_unreachable ();

?  So it's not entirely clear.  I think that if there was a way
to denote definitive 'code should not reach here' function
(a new attribute?) then it would make sense to not warn about
control flow not reaching that.  But then it would make sense
to warn about stmts following such annotation.

Richard.
Martin Sebor Nov. 30, 2021, 11:08 p.m. UTC | #4
On 11/30/21 12:51 AM, Richard Biener wrote:
> On Mon, 29 Nov 2021, Martin Sebor wrote:
> 
>> On 11/29/21 11:53 AM, Martin Sebor wrote:
>>> On 11/29/21 6:09 AM, Richard Biener via Gcc-patches wrote:
>>>> This removes more cases that appear when bootstrap with
>>>> -Wunreachable-code-return progresses.
>>>>
>>> ...
>>>> diff --git a/gcc/sel-sched-ir.h b/gcc/sel-sched-ir.h
>>>> index 8ee0529d5a8..18e03c4cb96 100644
>>>> --- a/gcc/sel-sched-ir.h
>>>> +++ b/gcc/sel-sched-ir.h
>>>> @@ -1493,8 +1493,6 @@ bb_next_bb (basic_block bb)
>>>>        default:
>>>>          return bb->next_bb;
>>>>        }
>>>> -
>>>> -  gcc_unreachable ();
>>>>    }
>>>
>>> Just skiming the changes out of curiosity, this one makes me
>>> wonder if the warning shouldn't be taught to avoid triggering
>>> on calls to __builtin_unreachable().  They can help make code
>>> more readable (e.g., after a case and switch statement that
>>> handles all values).
>>
>> I see someone else raised the same question in a patch I hadn't
>> gotten to yet:
>>
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585431.html
>>
>> If you do end up removing the gcc_unreachable() calls, I would
>> suggest to replace them with a comment so as not to lose
>> the readability benefit.
>>
>> But I still wonder if it might make sense to teach the warning
>> not just about __builtin_unreachable() but also about noreturn
>> calls like abort() that (as you explained in the thread above)
>> gcc_unreachable() expands to.  Is there a benefit to warning
>> on such calls?
> 
> I'm not sure.  I've chosen to eliminate only the "obvious"
> cases, like above where there's a default: that returns immediately
> visible (not always in the patch context).  I've left some in
> the code base where that's not so obvious.
> 
> IMHO making the flow obvious without a unreachable marker is
> superior to obfuscating it and clearing that up with one.
> 
> Yes, I thought about not diagnosing things like
> 
>     return 1;
>     return 1;
> 
> but then what about
> 
>     return 1;
>     return 0;
> 
> ?  I've seen cases like
> 
>     gcc_unreachable ();
>     return 0;
> 
> was that meant to be
> 
>     return 0;
>     gcc_unreachable ();
> 
> ?  So it's not entirely clear.  I think that if there was a way
> to denote definitive 'code should not reach here' function
> (a new attribute?) then it would make sense to not warn about
> control flow not reaching that.  But then it would make sense
> to warn about stmts following such annotation.

How would such an attribute be different from
__builtin_unreachable?  (By the way, there is or was a proposal
before WG14 to add an annotation like it:
   http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2816.pdf
If I recall, a function was preferred by more in a discussion
of the proposal than an attribute.)

I agree the cases above are not entirely clear but it occurs to
me that it's possible to discern at least two broad categories
of cases: 1) a statement made unreachable by a prior one with
the same effect where swapping the two wouldn't change anything
(the double return 1; above), and 2) an unreachable statement
(or a series of statements) with a different effect than
the prior one (the last three above).

The set in (1) are completely innocuous and removing them might
considered just a matter of cleanup.  Those in (2) are less
clear cut and more likely to harbor bugs and so when adopting
the warning in a code base like Binutils with lots of instances
of both kinds I'd expect to focus on (2) first and worry about
(1) later.

Even within (2) there might be separable subsets, like a return
statement followed by a break in a switch (common in Binutils
and I think you also cleaned up some in GCC).  In at least some
of these the return is hidden in a macro so the break after it
might serve as a visual clue that the case isn't meant to fall
through.  This subset would be different from two apparently
contradictory return statements each with a different value,
or from a return followed by more than one statement.  It might
make sense to treat these two classes separately (e.g., add
a level for them).

But these are just ideas for heuristics based on my limited
insight, and YMMV.  It's just food for thought.

Martin

> 
> Richard.
>
Richard Biener Dec. 1, 2021, 8:05 a.m. UTC | #5
On Tue, 30 Nov 2021, Martin Sebor wrote:

> On 11/30/21 12:51 AM, Richard Biener wrote:
> > On Mon, 29 Nov 2021, Martin Sebor wrote:
> > 
> >> On 11/29/21 11:53 AM, Martin Sebor wrote:
> >>> On 11/29/21 6:09 AM, Richard Biener via Gcc-patches wrote:
> >>>> This removes more cases that appear when bootstrap with
> >>>> -Wunreachable-code-return progresses.
> >>>>
> >>> ...
> >>>> diff --git a/gcc/sel-sched-ir.h b/gcc/sel-sched-ir.h
> >>>> index 8ee0529d5a8..18e03c4cb96 100644
> >>>> --- a/gcc/sel-sched-ir.h
> >>>> +++ b/gcc/sel-sched-ir.h
> >>>> @@ -1493,8 +1493,6 @@ bb_next_bb (basic_block bb)
> >>>>        default:
> >>>>          return bb->next_bb;
> >>>>        }
> >>>> -
> >>>> -  gcc_unreachable ();
> >>>>    }
> >>>
> >>> Just skiming the changes out of curiosity, this one makes me
> >>> wonder if the warning shouldn't be taught to avoid triggering
> >>> on calls to __builtin_unreachable().  They can help make code
> >>> more readable (e.g., after a case and switch statement that
> >>> handles all values).
> >>
> >> I see someone else raised the same question in a patch I hadn't
> >> gotten to yet:
> >>
> >> https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585431.html
> >>
> >> If you do end up removing the gcc_unreachable() calls, I would
> >> suggest to replace them with a comment so as not to lose
> >> the readability benefit.
> >>
> >> But I still wonder if it might make sense to teach the warning
> >> not just about __builtin_unreachable() but also about noreturn
> >> calls like abort() that (as you explained in the thread above)
> >> gcc_unreachable() expands to.  Is there a benefit to warning
> >> on such calls?
> > 
> > I'm not sure.  I've chosen to eliminate only the "obvious"
> > cases, like above where there's a default: that returns immediately
> > visible (not always in the patch context).  I've left some in
> > the code base where that's not so obvious.
> > 
> > IMHO making the flow obvious without a unreachable marker is
> > superior to obfuscating it and clearing that up with one.
> > 
> > Yes, I thought about not diagnosing things like
> > 
> >     return 1;
> >     return 1;
> > 
> > but then what about
> > 
> >     return 1;
> >     return 0;
> > 
> > ?  I've seen cases like
> > 
> >     gcc_unreachable ();
> >     return 0;
> > 
> > was that meant to be
> > 
> >     return 0;
> >     gcc_unreachable ();
> > 
> > ?  So it's not entirely clear.  I think that if there was a way
> > to denote definitive 'code should not reach here' function
> > (a new attribute?) then it would make sense to not warn about
> > control flow not reaching that.  But then it would make sense
> > to warn about stmts following such annotation.
> 
> How would such an attribute be different from
> __builtin_unreachable?  (By the way, there is or was a proposal
> before WG14 to add an annotation like it:
>   http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2816.pdf
> If I recall, a function was preferred by more in a discussion
> of the proposal than an attribute.)

__builtin_unrechable () would be exactly such a thing.  But
gcc_unreachable () expands to fatal_error () which is marked
noreturn and _not_ "unreachable".  The idea was to add
a wrapper that has the suggested new "unreachable" attribute
on it.

> I agree the cases above are not entirely clear but it occurs to
> me that it's possible to discern at least two broad categories
> of cases: 1) a statement made unreachable by a prior one with
> the same effect where swapping the two wouldn't change anything
> (the double return 1; above), and 2) an unreachable statement
> (or a series of statements) with a different effect than
> the prior one (the last three above).
> 
> The set in (1) are completely innocuous and removing them might
> considered just a matter of cleanup.  Those in (2) are less
> clear cut and more likely to harbor bugs and so when adopting
> the warning in a code base like Binutils with lots of instances
> of both kinds I'd expect to focus on (2) first and worry about
> (1) later.
> 
> Even within (2) there might be separable subsets, like a return
> statement followed by a break in a switch (common in Binutils
> and I think you also cleaned up some in GCC).  In at least some
> of these the return is hidden in a macro so the break after it
> might serve as a visual clue that the case isn't meant to fall
> through.  This subset would be different from two apparently
> contradictory return statements each with a different value,
> or from a return followed by more than one statement.  It might
> make sense to treat these two classes separately (e.g., add
> a level for them).
> 
> But these are just ideas for heuristics based on my limited
> insight, and YMMV.  It's just food for thought.

Agreed.  I concentrated on getting the basics working - it
would indeed be nice to suppress the 100% harmless cases, but
it starts to look like GIMPLE lowering is already too late
and too early at the same time to recover the important details.
Like a 'break' is visible as 'goto' only and w/o a built CFG
it's difficult to tell if it was a 'break' originally or if it
was

  switch (i)
  {
   case 0:
     return 0;
     goto fail;
...
  }

fail:
  return -1;

since GENERIC doesn't have BREAK or CONTINUE stmts we'd have to
annotate the GOTO_EXPR somehow to tell those apart.

Richard.
diff mbox series

Patch

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 2657e7817ae..523cb55e279 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -20364,7 +20364,6 @@  ix86_shift_rotate_cost (const struct processor_costs *cost,
       else
 	return cost->shift_var;
     }
-  return cost->shift_const;
 }
 
 /* Compute a (partial) cost for rtx X.  Return true if the complete
diff --git a/gcc/cp/logic.cc b/gcc/cp/logic.cc
index 9d892b1473b..f31ae8c58ae 100644
--- a/gcc/cp/logic.cc
+++ b/gcc/cp/logic.cc
@@ -495,7 +495,6 @@  cnf_size_r (tree t)
 	  else
 	    /* Neither LHS nor RHS is a conjunction.  */
 	    return std::make_pair (0, false);
-	  gcc_unreachable ();
 	}
       if (conjunction_p (lhs))
 	{
@@ -536,7 +535,6 @@  cnf_size_r (tree t)
 	  else
 	    /* Neither LHS nor RHS is a conjunction.  */
 	    return std::make_pair (2, false);
-	  gcc_unreachable ();
 	}
       if (conjunction_p (lhs))
 	{
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 74323701a7d..31ed773e145 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -1969,9 +1969,6 @@  iterative_hash_template_arg (tree arg, hashval_t val)
 	val = iterative_hash_template_arg (TREE_OPERAND (arg, i), val);
       return val;
     }
-
-  gcc_unreachable ();
-  return 0;
 }
 
 /* Unregister the specialization SPEC as a specialization of TMPL.
diff --git a/gcc/fortran/target-memory.c b/gcc/fortran/target-memory.c
index 7b21a9e04e8..ab4665c6782 100644
--- a/gcc/fortran/target-memory.c
+++ b/gcc/fortran/target-memory.c
@@ -138,7 +138,6 @@  gfc_element_size (gfc_expr *e, size_t *siz)
       *siz = 0;
       return false;
     }
-  return true;
 }
 
 
diff --git a/gcc/objc/objc-act.c b/gcc/objc/objc-act.c
index 9baa46d2243..89f46294123 100644
--- a/gcc/objc/objc-act.c
+++ b/gcc/objc/objc-act.c
@@ -1904,9 +1904,6 @@  objc_build_setter_call (tree lhs, tree rhs)
 					 setter_argument, NULL);
       return setter;
     }
-
-  /* Unreachable, but the compiler may not realize.  */
-  return error_mark_node;
 }
 
 /* This hook routine is called when a MODIFY_EXPR is being built.  We
diff --git a/gcc/reload.c b/gcc/reload.c
index 190db6ac47f..9ee3439709b 100644
--- a/gcc/reload.c
+++ b/gcc/reload.c
@@ -6602,11 +6602,10 @@  reg_overlap_mentioned_for_reload_p (rtx x, rtx in)
 	return (rtx_equal_p (x, in)
 		|| reg_overlap_mentioned_for_reload_p (x, XEXP (in, 0))
 		|| reg_overlap_mentioned_for_reload_p (x, XEXP (in, 1)));
-      else return (reg_overlap_mentioned_for_reload_p (XEXP (x, 0), in)
-		   || reg_overlap_mentioned_for_reload_p (XEXP (x, 1), in));
+      else
+	return (reg_overlap_mentioned_for_reload_p (XEXP (x, 0), in)
+		|| reg_overlap_mentioned_for_reload_p (XEXP (x, 1), in));
     }
-
-  gcc_unreachable ();
 }
 
 /* Return nonzero if anything in X contains a MEM.  Look also for pseudo
diff --git a/gcc/sel-sched-ir.h b/gcc/sel-sched-ir.h
index 8ee0529d5a8..18e03c4cb96 100644
--- a/gcc/sel-sched-ir.h
+++ b/gcc/sel-sched-ir.h
@@ -1493,8 +1493,6 @@  bb_next_bb (basic_block bb)
     default:
       return bb->next_bb;
     }
-
-  gcc_unreachable ();
 }
 
 
diff --git a/gcc/tree-chrec.c b/gcc/tree-chrec.c
index eeb67ded3dc..ad6f981191f 100644
--- a/gcc/tree-chrec.c
+++ b/gcc/tree-chrec.c
@@ -1148,8 +1148,6 @@  evolution_function_is_invariant_rec_p (tree chrec, int loopnum)
     default:
       return false;
     }
-
-  return false;
 }
 
 /* Return true if CHREC is invariant in loop LOOPNUM, false otherwise. */
diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index e88ddc9f788..d166a879be2 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -1109,8 +1109,6 @@  if_convertible_stmt_p (gimple *stmt, vec<data_reference_p> refs)
 	}
       return false;
     }
-
-  return true;
 }
 
 /* Assumes that BB has more than 1 predecessors.
diff --git a/gcc/tree-ssa-pre.c b/gcc/tree-ssa-pre.c
index 0669aaaac47..a49f87fbe6b 100644
--- a/gcc/tree-ssa-pre.c
+++ b/gcc/tree-ssa-pre.c
@@ -1234,7 +1234,6 @@  fully_constant_expression (pre_expr e)
     default:
       return e;
     }
-  return e;
 }
 
 /* Translate the VUSE backwards through phi nodes in E->dest, so that
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index dd7723629ba..b3adc26a933 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -330,8 +330,6 @@  operand_less_p (tree val, tree val2)
       else
 	return -2;
     }
-
-  return 0;
 }
 
 /* Compare two values VAL1 and VAL2.  Return
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 8c7aba2db61..d6031d6f1a2 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -3415,8 +3415,6 @@  compare_constant (const tree t1, const tree t2)
     default:
       return 0;
     }
-
-  gcc_unreachable ();
 }
 
 /* Return the section into which constant EXP should be placed.  */
diff --git a/libcpp/charset.c b/libcpp/charset.c
index 0b0ccc6c021..bcfd3ad44e1 100644
--- a/libcpp/charset.c
+++ b/libcpp/charset.c
@@ -1534,7 +1534,6 @@  convert_escape (cpp_reader *pfile, const uchar *from, const uchar *limit,
     case 'x':
       return convert_hex (pfile, from, limit, tbuf, cvt,
 			  char_range, loc_reader, ranges);
-      break;
 
     case '0':  case '1':  case '2':  case '3':
     case '4':  case '5':  case '6':  case '7':