Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 3 Nov 2021 12:27:54 +0200
From:   Mike Rapoport <rppt@linux.ibm.com>
To:     Anders Roxell <anders.roxell@linaro.org>
Cc:     Nick Desaulniers <ndesaulniers@google.com>,
        Shuah Khan <skhan@linuxfoundation.org>, shuah@kernel.org,
        fenghua.yu@intel.com, reinette.chatre@intel.com,
        john.stultz@linaro.org, tglx@linutronix.de,
        akpm@linux-foundation.org, nathan@kernel.org,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-mm@kvack.org, llvm@lists.linux.dev
Subject: Re: [PATCH] selftests: kselftest.h: mark functions with 'noreturn'
Message-ID: <YYJkKjNwWycYfs8a@linux.ibm.com>
References: <20211029114312.1921603-1-anders.roxell@linaro.org>
 <834d18b6-4106-045f-0264-20e54edf47bc@linuxfoundation.org>
 <CAKwvOdk8D5=AxzSpqjvXJc4XXL8CA7O=WY-LW0mZb3eQRK_EWg@mail.gmail.com>
 <CADYN=9+iueC3rJ4=32OM9rOUDLLmvcKY-y_By4hwAj1+9gxRiQ@mail.gmail.com>
 <CADYN=9+e=qLGBN+qxmKObiOp0hTQ_sGHSusn+4YvAXuprGVp2A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CADYN=9+e=qLGBN+qxmKObiOp0hTQ_sGHSusn+4YvAXuprGVp2A@mail.gmail.com>
Precedence: bulk

On Wed, Nov 03, 2021 at 10:38:17AM +0100, Anders Roxell wrote:
> On Tue, 2 Nov 2021 at 23:04, Anders Roxell <anders.roxell@linaro.org> wrote:
> >
> > On Sat, 30 Oct 2021 at 00:08, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > >
> > > On Fri, Oct 29, 2021 at 11:19 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
> > > >
> > > > On 10/29/21 5:43 AM, Anders Roxell wrote:
> > > > > When building kselftests/capabilities the following warning shows up:
> > > > >
> > > > > clang -O2 -g -std=gnu99 -Wall    test_execve.c -lcap-ng -lrt -ldl -o test_execve
> > > > > test_execve.c:121:13: warning: variable 'have_outer_privilege' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> > > > >          } else if (unshare(CLONE_NEWUSER | CLONE_NEWNS) == 0) {
> > > > >                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > test_execve.c:136:9: note: uninitialized use occurs here
> > > > >          return have_outer_privilege;
> > > > >                 ^~~~~~~~~~~~~~~~~~~~
> > > > > test_execve.c:121:9: note: remove the 'if' if its condition is always true
> > > > >          } else if (unshare(CLONE_NEWUSER | CLONE_NEWNS) == 0) {
> > > > >                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > test_execve.c:94:27: note: initialize the variable 'have_outer_privilege' to silence this warning
> > > > >          bool have_outer_privilege;
> > > > >                                   ^
> > > > >                                    = false
> > > > >
> > > > > Rework so all the ksft_exit_*() functions have attribue
> > > > > '__attribute__((noreturn))' so the compiler knows that there wont be
> > > > > any return from the function. That said, without
> > > > > '__attribute__((noreturn))' the compiler warns about the above issue
> > > > > since it thinks that it will get back from the ksft_exit_skip()
> > > > > function, which it wont.
> > > > > Cleaning up the callers that rely on ksft_exit_*() return code, since
> > > > > the functions ksft_exit_*() have never returned anything.
> > > > >
> > > > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> > > >
> > > > Lot of changes to fix this warning. Is this necessary? I would
> > > > like to explore if there is an easier and localized change that
> > > > can fix the problem.
> > >
> > > via `man 3 exit`:
> > > ```
> > > The  exit() function causes normal process termination ...
> > > ...
> > > RETURN VALUE
> > >        The exit() function does not return.
> > > ```
> > > so seeing `ksft_exit_pass`, `ksft_exit_fail`, `ksft_exit_fail_msg`,
> > > `ksft_exit_xfail`, `ksft_exit_xpass`, and `ksft_exit_skip` all
> > > unconditional call `exit` yet return an `int` looks wrong to me on
> > > first glance. So on that point this patch and its resulting diffstat
> > > LGTM.
> >
> > I'll respin the patch with these changes only.
> >
> > >
> > > That said, there are many changes that explicitly call `ksft_exit`
> > > with an expression; are those setting the correct exit code? Note that
> > > ksft_exit_pass is calling exit with KSFT_PASS which is 0.  So some of
> > > the negations don't look quite correct to me.  For example:
> > >
> > > -       return !ksft_get_fail_cnt() ? ksft_exit_pass() : ksft_exit_fail();
> > > +       ksft_exit(!ksft_get_fail_cnt());
> > >
> > > so if ksft_get_fail_cnt() returns 0, then we were calling
> > > ksft_exit_pass() which exited with 0. Now we'd be exiting with 1?
> >
> > oh, right, thank you for your review.
> > I will drop all the 'ksft_exit()' changes, they should be fixed and go
> > in as separete patches.
> 
> tools/testing/selftests/vm/memfd_secret.c has the
> 'ksft_exit(!ksft_get_fail_cnt())'
> statement and when I looked at it it when I did this patch it looked correct.
> However, when I look at it now I get a bit confused how ksft_exit() can be used
> with ksft_get_fail_cnt(). @Mike can you please clarify the
> 'ksft_exit(!ksft_get_fail_cnt())' instance in
> tools/testing/selftests/vm/memfd_secret.c.

ksft_exit() does not take the error code but rather a condition:

/**
 * ksft_exit() - Exit selftest based on truth of condition
 *
 * @condition: if true, exit self test with success, otherwise fail.
 */
#define ksft_exit(condition) do {	\
	if (!!(condition))		\
		ksft_exit_pass();	\
	else				\
		ksft_exit_fail();	\
	} while (0)

So

	!ksft_get_fail_cnt() ? ksft_exit_pass() : ksft_exit_fail();

and

	ksft_exit(!ksft_get_fail_cnt())

are equivalent.

-- 
Sincerely yours,
Mike.