Re: [PATCH] Remove more stray returns and gcc_unreachable ()s

[Martin Sebor <msebor@gmail.com>]

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.
>