Re: [RFC PATCH v1] perf build: Fix build for clang's -Wunreachable-code

[Namhyung Kim <namhyung@kernel.org>]

On Wed, May 28, 2025 at 04:18:16PM -0700, Ian Rogers wrote:
> On Wed, May 28, 2025 at 2:59 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Wed, May 28, 2025 at 01:32:10PM -0700, Ian Rogers wrote:
> > > On Wed, May 28, 2025 at 12:56 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > On Wed, May 28, 2025 at 11:35:00AM -0700, Ian Rogers wrote:
> > > > > On Wed, May 28, 2025 at 11:24 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > > >
> > > > > > On Tue, May 27, 2025 at 01:53:37PM -0700, Ian Rogers wrote:
> > > > > > > On Fri, Apr 11, 2025 at 3:14 PM Ian Rogers <irogers@google.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, Apr 11, 2025 at 2:34 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Ian,
> > > > > > > > >
> > > > > > > > > On Thu, Apr 10, 2025 at 01:26:47PM -0700, Ian Rogers wrote:
> > > > > > > > > > Clang's unreachable code warning is able to catch bugs like the famous
> > > > > > > > > > "goto fail" which gcc's unreachable code warning fails to warn about
> > > > > > > > > > (it will complain about misleading indent). The changes here are
> > > > > > > > > > sufficient to get perf building with clang with -Wunreachable code,
> > > > > > > > > > but they don't really fix any bugs. Posting as an RFC to see if anyone
> > > > > > > > > > things this is worth pursuing.
> > > > > > > > >
> > > > > > > > > I'm not sure if it's useful and don't see what kind of bugs it can
> > > > > > > > > address.  The proposed changes don't look like an improvement.
> > > > > > > >
> > > > > > > > The goto fail case was in OpenSSL the code from a bad merge:
> > > > > > > > ```
> > > > > > > > if (...)
> > > > > > > >   goto fail;
> > > > > > > >   goto fail;
> > > > > > > > ```
> > > > > > > > Meaning the fail path was always taken and checking on the non-fail
> > > > > > > > code never executed. Newer GCCs will warn of this because of the
> > > > > > > > "misleading indent" but  clang won't. It is easy to imagine similar
> > > > > > > > mistakes creeping in, so using compiler warnings to avoid the bug
> > > > > > > > could be useful.
> > > > > >
> > > > > > It doesn't look very convincing to me but it might be valuable in some
> > > > > > rare cases.  But the proposed changes - basically replace exit() to
> > > > > > __builtin_unreachable() - seem weird.  Why is calling it a problem?  I
> > > > > > guess it already has some kind of annotation like "noreturn"?
> > > > >
> > > > > Yep. The exit is incorrect (depending on your notion of correct, I'd
> > > > > go with clang's notion as they've had to consider this for a while) as
> > > > > it can never be executed. I've added the __builtin_unreachable() to
> > > > > document that you can never get to that statement, as otherwise it can
> > > > > make the code readability harder with the code looking like it will
> > > > > fall through after calling something like usage_with_options (which is
> > > > > noreturn). In unoptimized builds __builtin_unreachable() will fail if
> > > > > executed, so it is a bit more active than just a comment.
> > > >
> > > > Oh I see, usage_with_options() calls exit() inside so any code after
> > > > that won't be executed.  Hmm.. isn't it better to remove those codes
> > > > then?
> > >
> > > Not sure I follow. The patch does remove the code but it replaces it
> > > with __builtin_unreachable() to basically state that the code here and
> > > below can never be reached. Do you mean remove the exit from
> > > usage_with_options? Then we'd need to fix all the callers, which would
> > > be a larger patch. Perhaps it should be usage_with_options_and_exit()
> > > to make it clearer that the code doesn't return. I was after doing
> > > what was minimal for -Wunreachable-code but while trying to keep the
> > > code clear.
> >
> > No, I meant we may not need the __builtin_unreachable() at the callsites.
> >
> > Would it complain this code?
> >
> >   if (some_bad_option_use)
> >     usage_with_options(...);
> >
> >   /* normal code path */
> 
> Right that would fix -Wunreachable, but the existing code would be:
> 
>   if (some_bad_option_use) {
>     usage_with_options(...);
>     exit(..);
>   }
>   /* normal code path */
> 
> Letting you know that "normal code path" couldn't be fallen into after
> usage_with_options(...). To make the behavior more obvious we could
> rename usage_with_options():
> 
>   if (some_bad_option_use)
>     usage_with_options_and_exit(...);
> 
>   /* normal code path */
> 
> What I've done is:
> 
>   if (some_bad_option_use) {
>     usage_with_options(...);
>     __builtin_unreachable();
>   }
>   /* normal code path */
> 
> My reasoning is that usage_with_options() doesn't obviously on the
> face of it call exit and never return. To make that clear we could add
> a comment:
> 
>   if (some_bad_option_use) {
>     usage_with_options(...);
>     /* usage_with_options never returns as it always calls exit */
>   }
>   /* normal code path */
> 
> But my preference is to use __builtin_unreachable as that is the same
> as the comment but is also something the compiler can trap on were it
> not true.

I see, thanks for the explanation.  That part looks ok then.

Thanks,
Namhyung