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.
