| Message ID | 20240411-mark-kselftest-exit-funcs-noreturn-v1-1-b027c948f586@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | kselftest: Mark functions that unconditionally call exit() as __noreturn | expand |
On 4/11/24 12:45, Nathan Chancellor 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.", &major, &minor) != 2) > | ^~~~~~~~~~~~ > tools/testing/selftests/timers/../kselftest.h:401:9: note: uninitialized use occurs here > 401 | return major > min_major || (major == min_major && minor >= 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.", &major, &minor) != 2) > | ^~~~~~~~~~~~~~~ > tools/testing/selftests/timers/../kselftest.h:395:20: note: initialize the variable 'major' to silence this warning > 395 | unsigned int major, minor; > | ^ > | = 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_timer_distribution()") > Reported-by: John Stultz <jstultz@google.com> > Closes: https://lore.kernel.org/all/20240410232637.4135564-2-jstultz@google.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()"). Thank you. Assuming this is going through tip/timers/urgent Acked-by: Shuah Khan <skhan@linuxfoundation.org> Usama, please send patch fixing this problem in next on top of commit f07041728422 ("selftests: add > ksft_exit_fail_perror()"). thanks, -- Shuah
On Thu, Apr 11 2024 at 11:45, Nathan Chancellor wrote: > 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). Hrmm. This really want's to be fixed once the change hits Linus tree as this: static inline __noreturn int ksft_exit_pass(void) looks seriously broken :)
On Thu, Apr 11, 2024 at 11:45 AM Nathan Chancellor <nathan@kernel.org> 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.", &major, &minor) != 2) > | ^~~~~~~~~~~~ > tools/testing/selftests/timers/../kselftest.h:401:9: note: uninitialized use occurs here > 401 | return major > min_major || (major == min_major && minor >= 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.", &major, &minor) != 2) > | ^~~~~~~~~~~~~~~ > tools/testing/selftests/timers/../kselftest.h:395:20: note: initialize the variable 'major' to silence this warning > 395 | unsigned int major, minor; > | ^ > | = 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_timer_distribution()") > Reported-by: John Stultz <jstultz@google.com> > Closes: https://lore.kernel.org/all/20240410232637.4135564-2-jstultz@google.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/selftests/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 char *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 char *msg, ...) > { > int saved_errno = 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 *msg, ...) > { > int saved_errno = errno; > va_list args; > > --- > base-commit: 076361362122a6d8a4c45f172ced5576b2d4a50d > change-id: 20240411-mark-kselftest-exit-funcs-noreturn-17d8ff729a7a > > Best regards, > -- > Nathan Chancellor <nathan@kernel.org> > >
On Fri, Apr 12, 2024 at 02:05:47PM +0200, Thomas Gleixner wrote: > On Thu, Apr 11 2024 at 11:45, Nathan Chancellor wrote: > > 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). > > Hrmm. This really want's to be fixed once the change hits Linus tree as this: > > static inline __noreturn int ksft_exit_pass(void) > > looks seriously broken :) Yeah, I only realized this morning that prior to this change, making these functions return void instead of int would have broken int main(void) { <code> ksft_exit_pass(); } because without __noreturn, the compiler will complain that main() is missing a return value. So 'int' -> '__noreturn void' would have been the proper atomic change but the use of 'return ksft_exit_...();' made that seem rather difficult when I was writing/testing that change on top of this one. However, now that I am actually sitting down and looking at it with a fresh perspective, I am able to produce a pretty mechanical looking change with just two sed commands: sed -i 's;__noreturn\(.*\)int;__noreturn\1void;g' tools/testing/selftests/kselftest.h && sed -i 's/\(\s\+\)return\s\+\(.*ksft_exit_x\?\(fail\|pass\|skip\)\)/\1\2/g' $(git grep -lP 'return.*ksft_exit_x?(fail|pass|skip)' | sed s/:/-/g) Perhaps Shuah could just run that in the kselftest tree and commit the result once the change from Linus's tree is merged there? Otherwise, I am happy to send a formal patch once I have something proper to base on. Thanks for taking just the minimal change :) Cheers, Nathan
On Fri, Apr 12, 2024 at 9:08 AM Bill Wendling <morbo@google.com> wrote: > > On Thu, Apr 11, 2024 at 11:45 AM Nathan Chancellor <nathan@kernel.org> 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.", &major, &minor) != 2) > > | ^~~~~~~~~~~~ > > tools/testing/selftests/timers/../kselftest.h:401:9: note: uninitialized use occurs here > > 401 | return major > min_major || (major == min_major && minor >= 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.", &major, &minor) != 2) > > | ^~~~~~~~~~~~~~~ > > tools/testing/selftests/timers/../kselftest.h:395:20: note: initialize the variable 'major' to silence this warning > > 395 | unsigned int major, minor; > > | ^ > > | = 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_timer_distribution()") > > Reported-by: John Stultz <jstultz@google.com> > > Closes: https://lore.kernel.org/all/20240410232637.4135564-2-jstultz@google.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/selftests/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 char *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? > Oops...That's been asked. Sorry for the extra message. -bw
diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/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 char *test_name, va_end(args); } -static inline int ksft_exit_pass(void) +static inline __noreturn int ksft_exit_pass(void) { 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 char *msg, ...) { int saved_errno = 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 *msg, ...) { int saved_errno = errno; va_list args;
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.", &major, &minor) != 2) | ^~~~~~~~~~~~ tools/testing/selftests/timers/../kselftest.h:401:9: note: uninitialized use occurs here 401 | return major > min_major || (major == min_major && minor >= 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.", &major, &minor) != 2) | ^~~~~~~~~~~~~~~ tools/testing/selftests/timers/../kselftest.h:395:20: note: initialize the variable 'major' to silence this warning 395 | unsigned int major, minor; | ^ | = 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_timer_distribution()") Reported-by: John Stultz <jstultz@google.com> Closes: https://lore.kernel.org/all/20240410232637.4135564-2-jstultz@google.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(-) --- base-commit: 076361362122a6d8a4c45f172ced5576b2d4a50d change-id: 20240411-mark-kselftest-exit-funcs-noreturn-17d8ff729a7a Best regards,