From: Shuah Khan <skhan@linuxfoundation.org>
To: Anders Roxell <anders.roxell@linaro.org>,
Nick Desaulniers <ndesaulniers@google.com>,
Shuah Khan <shuah@kernel.org>
Cc: 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,
Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCHv2] selftests: kselftest.h: mark functions with 'noreturn'
Date: Wed, 17 Nov 2021 06:40:50 -0700 [thread overview]
Message-ID: <e00fb2ce-b931-c725-7506-027bab0a6c47@linuxfoundation.org> (raw)
In-Reply-To: <CADYN=9+_UU9qZX56uahGXxz00iayqJLRAaQrRXh1CMXTvwSbAg@mail.gmail.com>
On 11/17/21 4:01 AM, Anders Roxell wrote:
> On Wed, 3 Nov 2021 at 19:38, Nick Desaulniers <ndesaulniers@google.com> wrote:
>>
>> On Wed, Nov 3, 2021 at 2:47 AM Anders Roxell <anders.roxell@linaro.org> 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>
>>> ---
>>> tools/testing/selftests/clone3/clone3.c | 4 +++-
>>> .../testing/selftests/clone3/clone3_clear_sighand.c | 2 +-
>>> tools/testing/selftests/clone3/clone3_set_tid.c | 4 +++-
>>> tools/testing/selftests/ipc/msgque.c | 10 +++++-----
>>> tools/testing/selftests/kcmp/kcmp_test.c | 2 +-
>>> tools/testing/selftests/kselftest.h | 12 ++++++------
>>> .../membarrier/membarrier_test_multi_thread.c | 2 +-
>>> .../membarrier/membarrier_test_single_thread.c | 2 +-
>>> tools/testing/selftests/pidfd/pidfd_fdinfo_test.c | 2 +-
>>> tools/testing/selftests/pidfd/pidfd_open_test.c | 4 +++-
>>> tools/testing/selftests/pidfd/pidfd_poll_test.c | 2 +-
>>> tools/testing/selftests/pidfd/pidfd_test.c | 2 +-
>>> tools/testing/selftests/resctrl/resctrl_tests.c | 6 +++---
>>> tools/testing/selftests/sync/sync_test.c | 2 +-
>>> tools/testing/selftests/timers/adjtick.c | 4 ++--
>>> tools/testing/selftests/timers/alarmtimer-suspend.c | 4 ++--
>>> tools/testing/selftests/timers/change_skew.c | 4 ++--
>>> tools/testing/selftests/timers/clocksource-switch.c | 4 ++--
>>> tools/testing/selftests/timers/freq-step.c | 4 ++--
>>> tools/testing/selftests/timers/inconsistency-check.c | 4 ++--
>>> tools/testing/selftests/timers/leap-a-day.c | 10 +++++-----
>>> tools/testing/selftests/timers/leapcrash.c | 4 ++--
>>> tools/testing/selftests/timers/mqueue-lat.c | 4 ++--
>>> tools/testing/selftests/timers/nanosleep.c | 4 ++--
>>> tools/testing/selftests/timers/nsleep-lat.c | 4 ++--
>>> tools/testing/selftests/timers/posix_timers.c | 12 ++++++------
>>> tools/testing/selftests/timers/raw_skew.c | 6 +++---
>>> tools/testing/selftests/timers/set-2038.c | 4 ++--
>>> tools/testing/selftests/timers/set-tai.c | 4 ++--
>>> tools/testing/selftests/timers/set-timer-lat.c | 4 ++--
>>> tools/testing/selftests/timers/set-tz.c | 4 ++--
>>> tools/testing/selftests/timers/skew_consistency.c | 4 ++--
>>> tools/testing/selftests/timers/threadtest.c | 2 +-
>>> tools/testing/selftests/timers/valid-adjtimex.c | 6 +++---
>>> tools/testing/selftests/vm/madv_populate.c | 2 +-
>>> 35 files changed, 80 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c
>>> index 42be3b925830..ede5da0c67b4 100644
>>> --- a/tools/testing/selftests/clone3/clone3.c
>>> +++ b/tools/testing/selftests/clone3/clone3.c
>>> @@ -191,5 +191,7 @@ int main(int argc, char *argv[])
>>> test_clone3(CLONE_NEWPID, getpagesize() + 8, -E2BIG,
>>> CLONE3_ARGS_NO_TEST);
>>>
>>> - return !ksft_get_fail_cnt() ? ksft_exit_pass() : ksft_exit_fail();
>>> + if (ksft_get_fail_cnt())
>>> + ksft_exit_fail();
>>> + ksft_exit_pass();
>>> }
>>> diff --git a/tools/testing/selftests/clone3/clone3_clear_sighand.c b/tools/testing/selftests/clone3/clone3_clear_sighand.c
>>> index 47a8c0fc3676..dcd9448eaeec 100644
>>> --- a/tools/testing/selftests/clone3/clone3_clear_sighand.c
>>> +++ b/tools/testing/selftests/clone3/clone3_clear_sighand.c
>>> @@ -124,5 +124,5 @@ int main(int argc, char **argv)
>>>
>>> test_clone3_clear_sighand();
>>>
>>> - return ksft_exit_pass();
>>> + ksft_exit_pass();
>>> }
>>> diff --git a/tools/testing/selftests/clone3/clone3_set_tid.c b/tools/testing/selftests/clone3/clone3_set_tid.c
>>> index 0229e9ebb995..a755fcd3af89 100644
>>> --- a/tools/testing/selftests/clone3/clone3_set_tid.c
>>> +++ b/tools/testing/selftests/clone3/clone3_set_tid.c
>>> @@ -393,5 +393,7 @@ int main(int argc, char *argv[])
>>> out:
>>> ret = 0;
>>>
>>> - return !ret ? ksft_exit_pass() : ksft_exit_fail();
>>> + if (ret)
>>> + ksft_exit_fail();
>>> + ksft_exit_pass();
>>
>> This case is weird (pre-existing-ly) and looks broken. The assignment
>> to ret should probably occur _before_ the out: label. Honestly, I'd
>> remove the use of goto from this function and just call either
>> ksft_exit_pass() or ksft_exit_fail().
>
> I can do that in a followup patch.
>
Let's fold this change in. Please send me v3 with.
thanks,
-- Shuah
next prev parent reply other threads:[~2021-11-17 13:40 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-03 9:47 [PATCHv2] selftests: kselftest.h: mark functions with 'noreturn' Anders Roxell
2021-11-03 18:38 ` Nick Desaulniers
2021-11-17 11:01 ` Anders Roxell
2021-11-17 13:40 ` Shuah Khan [this message]
2021-11-18 9:58 ` [PATCH v3 1/2] " Anders Roxell
2021-11-18 9:58 ` [PATCH v3 2/2] selftests: clone3: simplify return logic in clone3_set_tid() Anders Roxell
2021-11-18 10:03 ` Anders Roxell
2021-11-18 18:43 ` Nick Desaulniers
2021-11-18 18:40 ` [PATCH v3 1/2] selftests: kselftest.h: mark functions with 'noreturn' Nick Desaulniers
2021-11-18 18:41 ` Nick Desaulniers
2021-11-19 23:35 ` Shuah Khan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e00fb2ce-b931-c725-7506-027bab0a6c47@linuxfoundation.org \
--to=skhan@linuxfoundation.org \
--cc=akpm@linux-foundation.org \
--cc=anders.roxell@linaro.org \
--cc=fenghua.yu@intel.com \
--cc=john.stultz@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=llvm@lists.linux.dev \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=reinette.chatre@intel.com \
--cc=shuah@kernel.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).