Remove more stray returns and gcc_unreachable ()s
Commit Message
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
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
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
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.
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.
>
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.
@@ -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
@@ -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))
{
@@ -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.
@@ -138,7 +138,6 @@ gfc_element_size (gfc_expr *e, size_t *siz)
*siz = 0;
return false;
}
- return true;
}
@@ -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
@@ -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
@@ -1493,8 +1493,6 @@ bb_next_bb (basic_block bb)
default:
return bb->next_bb;
}
-
- gcc_unreachable ();
}
@@ -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. */
@@ -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.
@@ -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
@@ -330,8 +330,6 @@ operand_less_p (tree val, tree val2)
else
return -2;
}
-
- return 0;
}
/* Compare two values VAL1 and VAL2. Return
@@ -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. */
@@ -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':