| Message ID | 20230526075355.586335-1-davidgow@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [RFC] kunit: Move kunit_abort() call out of kunit_do_failed_assertion() | expand |
On Fri, May 26, 2023 at 03:53:54PM +0800, David Gow wrote: > KUnit aborts the current thread when an assertion fails. Currently, this > is done conditionally as part of the kunit_do_failed_assertion() > function, but this hides the kunit_abort() call from the compiler > (particularly if it's in another module). This, in turn, can lead to > both suboptimal code generation (the compiler can't know if > kunit_do_failed_assertion() will return), and to static analysis tools > like smatch giving false positives. > > Moving the kunit_abort() call into the macro should give the compiler > and tools a better chance at understanding what's going on. Doing so > requires exporting kunit_abort(), though it's recommended to continue to > use assertions in lieu of aborting directly. > > Suggested-by: Dan Carpenter <dan.carpenter@linaro.org> > Signed-off-by: David Gow <davidgow@google.com> Awesome thanks! I had already hardcoded the current behavior but I'm happy to delete that code. This works automatically and will continue to work if we change the parameters to kunit_do_failed_assertion() in the future. regards, dan carpenter
On Fri, May 26, 2023 at 12:54 AM David Gow <davidgow@google.com> wrote: > > KUnit aborts the current thread when an assertion fails. Currently, this > is done conditionally as part of the kunit_do_failed_assertion() > function, but this hides the kunit_abort() call from the compiler > (particularly if it's in another module). This, in turn, can lead to > both suboptimal code generation (the compiler can't know if > kunit_do_failed_assertion() will return), and to static analysis tools > like smatch giving false positives. > > Moving the kunit_abort() call into the macro should give the compiler > and tools a better chance at understanding what's going on. Doing so > requires exporting kunit_abort(), though it's recommended to continue to > use assertions in lieu of aborting directly. Should we rename it to __kunit_abort() to discourage that? That would match what we do with __kunit_test_suites_init(). Daniel
On Fri, May 26, 2023 at 12:54 AM David Gow <davidgow@google.com> wrote: > > KUnit aborts the current thread when an assertion fails. Currently, this > is done conditionally as part of the kunit_do_failed_assertion() > function, but this hides the kunit_abort() call from the compiler > (particularly if it's in another module). This, in turn, can lead to > both suboptimal code generation (the compiler can't know if > kunit_do_failed_assertion() will return), and to static analysis tools > like smatch giving false positives. Another thought: this impacts https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/kunit.rs. They're currently calling kunit_do_failed_assert() always with type=ASSERTION. This change would actually make things better since they could handle shutting down the thread themselves instead of having it happen behind an opaque FFI layer. But we'd just need to make sure we get that code updated around when this change goes in. Daniel
diff --git a/include/kunit/test.h b/include/kunit/test.h index 2f23d6efa505..6a35e3e2a1e5 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -481,6 +481,8 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...); */ #define KUNIT_SUCCEED(test) do {} while (0) +void __noreturn kunit_abort(struct kunit *test); + void kunit_do_failed_assertion(struct kunit *test, const struct kunit_loc *loc, enum kunit_assert_type type, @@ -498,6 +500,8 @@ void kunit_do_failed_assertion(struct kunit *test, assert_format, \ fmt, \ ##__VA_ARGS__); \ + if (assert_type == KUNIT_ASSERTION) \ + kunit_abort(test); \ } while (0) diff --git a/lib/kunit/test.c b/lib/kunit/test.c index d3fb93a23ccc..3b350e50cab9 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -310,7 +310,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc, string_stream_destroy(stream); } -static void __noreturn kunit_abort(struct kunit *test) +void __noreturn kunit_abort(struct kunit *test) { kunit_try_catch_throw(&test->try_catch); /* Does not return. */ @@ -340,9 +340,6 @@ void kunit_do_failed_assertion(struct kunit *test, kunit_fail(test, loc, type, assert, assert_format, &message); va_end(args); - - if (type == KUNIT_ASSERTION) - kunit_abort(test); } EXPORT_SYMBOL_GPL(kunit_do_failed_assertion);
KUnit aborts the current thread when an assertion fails. Currently, this is done conditionally as part of the kunit_do_failed_assertion() function, but this hides the kunit_abort() call from the compiler (particularly if it's in another module). This, in turn, can lead to both suboptimal code generation (the compiler can't know if kunit_do_failed_assertion() will return), and to static analysis tools like smatch giving false positives. Moving the kunit_abort() call into the macro should give the compiler and tools a better chance at understanding what's going on. Doing so requires exporting kunit_abort(), though it's recommended to continue to use assertions in lieu of aborting directly. Suggested-by: Dan Carpenter <dan.carpenter@linaro.org> Signed-off-by: David Gow <davidgow@google.com> --- include/kunit/test.h | 4 ++++ lib/kunit/test.c | 5 +---- 2 files changed, 5 insertions(+), 4 deletions(-)