Received-SPF: pass (google.com: domain of linux-kernel+bounces-143013-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240411-mark-kselftest-exit-funcs-noreturn-v1-1-b027c948f586@kernel.org>
In-Reply-To: <20240411-mark-kselftest-exit-funcs-noreturn-v1-1-b027c948f586@kernel.org>
From: Bill Wendling <morbo@google.com>
Date: Fri, 12 Apr 2024 09:08:20 -0700
Message-ID: <CAGG=3QUNPGtQ7OqzEtNW9BQET+a+1WW=h++CqRTHRY=YLrOAcw@mail.gmail.com>
Subject: Re: [PATCH] kselftest: Mark functions that unconditionally call
 exit() as __noreturn
To: Nathan Chancellor <nathan@kernel.org>
Cc: tglx@linutronix.de, shuah@kernel.org, oleg@redhat.com, 
	anna-maria@linutronix.de, frederic@kernel.org, ndesaulniers@google.com, 
	justinstitt@google.com, linux-kselftest@vger.kernel.org, 
	linux-kernel@vger.kernel.org, llvm@lists.linux.dev, patches@lists.linux.dev, 
	John Stultz <jstultz@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 11, 2024 at 11:45=E2=80=AFAM Nathan Chancellor <nathan@kernel.o=
rg> wrote:
>
> After commit 6d029c25b71f ("selftests/timers/posix_timers: Reimplement
> check_timer_distribution()"), clang warns:
>
>   tools/testing/selftests/timers/../kselftest.h:398:6: warning: variable =
'major' is used uninitialized whenever '||' condition is true [-Wsometimes-=
uninitialized]
>     398 |         if (uname(&info) || sscanf(info.release, "%u.%u.", &maj=
or, &minor) !=3D 2)
>         |             ^~~~~~~~~~~~
>   tools/testing/selftests/timers/../kselftest.h:401:9: note: uninitialize=
d use occurs here
>     401 |         return major > min_major || (major =3D=3D min_major && =
minor >=3D min_minor);
>         |                ^~~~~
>   tools/testing/selftests/timers/../kselftest.h:398:6: note: remove the '=
||' if its condition is always false
>     398 |         if (uname(&info) || sscanf(info.release, "%u.%u.", &maj=
or, &minor) !=3D 2)
>         |             ^~~~~~~~~~~~~~~
>   tools/testing/selftests/timers/../kselftest.h:395:20: note: initialize =
the variable 'major' to silence this warning
>     395 |         unsigned int major, minor;
>         |                           ^
>         |                            =3D 0
>
> This is a false positive because if uname() fails, ksft_exit_fail_msg()
> will be called, which unconditionally calls exit(), a noreturn function.
> However, clang does not know that ksft_exit_fail_msg() will call exit()
> at the point in the pipeline that the warning is emitted because
> inlining has not occurred, so it assumes control flow will resume
> normally after ksft_exit_fail_msg() is called.
>
> Make it clear to clang that all of the functions that call exit()
> unconditionally in kselftest.h are noreturn transitively by marking them
> explicitly with '__attribute__((__noreturn__))', which clears up the
> warning above and any future warnings that may appear for the same
> reason.
>
> Fixes: 6d029c25b71f ("selftests/timers/posix_timers: Reimplement check_ti=
mer_distribution()")
> Reported-by: John Stultz <jstultz@google.com>
> Closes: https://lore.kernel.org/all/20240410232637.4135564-2-jstultz@goog=
le.com/
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
> I have based this change on timers/urgent, as the commit that introduces
> this particular warning is there and it is marked for stable, even
> though this appears to be a generic kselftest issue. I think it makes
> the most sense for this change to go via timers/urgent with Shuah's ack.
> While __noreturn with a return type other than 'void' does not make much
> sense semantically, there are many places that these functions are used
> as the return value for other functions such as main(), so I did not
> change the return type of these functions from 'int' to 'void' to
> minimize the necessary changes for a backport (it is an existing issue
> anyways).
>
> I see there is another instance of this problem that will need to be
> addressed in -next, introduced by commit f07041728422 ("selftests: add
> ksft_exit_fail_perror()").
> ---
>  tools/testing/selftests/kselftest.h | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftest=
s/kselftest.h
> index 973b18e156b2..0591974b57e0 100644
> --- a/tools/testing/selftests/kselftest.h
> +++ b/tools/testing/selftests/kselftest.h
> @@ -80,6 +80,9 @@
>  #define KSFT_XPASS 3
>  #define KSFT_SKIP  4
>
> +#ifndef __noreturn
> +#define __noreturn       __attribute__((__noreturn__))
> +#endif
>  #define __printf(a, b)   __attribute__((format(printf, a, b)))
>
>  /* counters */
> @@ -300,13 +303,13 @@ void ksft_test_result_code(int exit_code, const cha=
r *test_name,
>         va_end(args);
>  }
>
> -static inline int ksft_exit_pass(void)
> +static inline __noreturn int ksft_exit_pass(void)

Orthogonal to this fix, but why does a "no return" function have a
non-void return type?

>  {
>         ksft_print_cnts();
>         exit(KSFT_PASS);
>  }
>
> -static inline int ksft_exit_fail(void)
> +static inline __noreturn int ksft_exit_fail(void)
>  {
>         ksft_print_cnts();
>         exit(KSFT_FAIL);
> @@ -333,7 +336,7 @@ static inline int ksft_exit_fail(void)
>                   ksft_cnt.ksft_xfail + \
>                   ksft_cnt.ksft_xskip)
>
> -static inline __printf(1, 2) int ksft_exit_fail_msg(const char *msg, ...=
)
> +static inline __noreturn __printf(1, 2) int ksft_exit_fail_msg(const cha=
r *msg, ...)
>  {
>         int saved_errno =3D errno;
>         va_list args;
> @@ -348,19 +351,19 @@ static inline __printf(1, 2) int ksft_exit_fail_msg=
(const char *msg, ...)
>         exit(KSFT_FAIL);
>  }
>
> -static inline int ksft_exit_xfail(void)
> +static inline __noreturn int ksft_exit_xfail(void)
>  {
>         ksft_print_cnts();
>         exit(KSFT_XFAIL);
>  }
>
> -static inline int ksft_exit_xpass(void)
> +static inline __noreturn int ksft_exit_xpass(void)
>  {
>         ksft_print_cnts();
>         exit(KSFT_XPASS);
>  }
>
> -static inline __printf(1, 2) int ksft_exit_skip(const char *msg, ...)
> +static inline __noreturn __printf(1, 2) int ksft_exit_skip(const char *m=
sg, ...)
>  {
>         int saved_errno =3D errno;
>         va_list args;
>
> ---
> base-commit: 076361362122a6d8a4c45f172ced5576b2d4a50d
> change-id: 20240411-mark-kselftest-exit-funcs-noreturn-17d8ff729a7a
>
> Best regards,
> --
> Nathan Chancellor <nathan@kernel.org>
>
>