Messages in this thread | | | Subject | Re: [PATCHv2] selftests: kselftest.h: mark functions with 'noreturn' | From | Shuah Khan <> | Date | Wed, 17 Nov 2021 06:40:50 -0700 |
| |
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
| |