Re: [PATCH v2 6/7] kunit: Print last test location on fault

[David Gow <davidgow@google.com>]

[-- Attachment #1: Type: text/plain, Size: 9294 bytes --]

On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün <mic@digikod.net> wrote:
>
> This helps identify the location of test faults.
>
> Cc: Brendan Higgins <brendanhiggins@google.com>
> Cc: David Gow <davidgow@google.com>
> Cc: Rae Moar <rmoar@google.com>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20240301194037.532117-7-mic@digikod.net
> ---

I like the idea of this, but am a little bit worried about how
confusing it might be, given that the location only updates on those
particular macros.

Maybe the answer is to make the __KUNIT_SAVE_LOC() macro, or something
equivalent, a supported API.

One possibility would be to have a KUNIT_MARKER() macro. If we really
wanted to, we could expand it to take a string so we can have a more
user-friendly KUNIT_MARKER(test, "parsing packet") description of
where things went wrong. Another could be to extend this to use the
code tagging framework[1], if that lands.

That being said, I think this is still an improvement without any of
those features. I've left a few comments below. Let me know what you
think.

Cheers,
-- David

[1]: https://lwn.net/Articles/906660/
>
> Changes since v1:
> * Added Kees's Reviewed-by.
> ---
>  include/kunit/test.h  | 24 +++++++++++++++++++++---
>  lib/kunit/try-catch.c | 10 +++++++---
>  2 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index fcb4a4940ace..f3aa66eb0087 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -301,6 +301,8 @@ struct kunit {
>         struct list_head resources; /* Protected by lock. */
>
>         char status_comment[KUNIT_STATUS_COMMENT_SIZE];
> +       /* Saves the last seen test. Useful to help with faults. */
> +       struct kunit_loc last_seen;
>  };
>
>  static inline void kunit_set_failure(struct kunit *test)
> @@ -567,6 +569,15 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
>  #define kunit_err(test, fmt, ...) \
>         kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
>
> +/*
> + * Must be called at the beginning of each KUNIT_*_ASSERTION().
> + * Cf. KUNIT_CURRENT_LOC.
> + */
> +#define _KUNIT_SAVE_LOC(test) do {                                            \
> +       WRITE_ONCE(test->last_seen.file, __FILE__);                            \
> +       WRITE_ONCE(test->last_seen.line, __LINE__);                            \
> +} while (0)

Can we get rid of the leading '_', make this public, and document it?
If we want to rename it to KUNIT_MARKER() or similar, that might work
better, too.

> +
>  /**
>   * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
>   * @test: The test context object.
> @@ -575,7 +586,7 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
>   * words, it does nothing and only exists for code clarity. See
>   * KUNIT_EXPECT_TRUE() for more information.
>   */
> -#define KUNIT_SUCCEED(test) do {} while (0)
> +#define KUNIT_SUCCEED(test) _KUNIT_SAVE_LOC(test)
>
>  void __noreturn __kunit_abort(struct kunit *test);
>
> @@ -601,14 +612,16 @@ void __kunit_do_failed_assertion(struct kunit *test,
>  } while (0)
>
>
> -#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...)                     \
> +#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) do {                \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         _KUNIT_FAILED(test,                                                    \
>                       assert_type,                                             \
>                       kunit_fail_assert,                                       \
>                       kunit_fail_assert_format,                                \
>                       {},                                                      \
>                       fmt,                                                     \
> -                     ##__VA_ARGS__)
> +                     ##__VA_ARGS__);                                          \
> +} while (0)
>
>  /**
>   * KUNIT_FAIL() - Always causes a test to fail when evaluated.
> @@ -637,6 +650,7 @@ void __kunit_do_failed_assertion(struct kunit *test,
>                               fmt,                                             \
>                               ...)                                             \
>  do {                                                                          \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         if (likely(!!(condition_) == !!expected_true_))                        \
>                 break;                                                         \
>                                                                                \
> @@ -698,6 +712,7 @@ do {                                                                               \
>                 .right_text = #right,                                          \
>         };                                                                     \
>                                                                                \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         if (likely(__left op __right))                                         \
>                 break;                                                         \
>                                                                                \
> @@ -758,6 +773,7 @@ do {                                                                               \
>                 .right_text = #right,                                          \
>         };                                                                     \
>                                                                                \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         if (likely((__left) && (__right) && (strcmp(__left, __right) op 0)))   \
>                 break;                                                         \
>                                                                                \
> @@ -791,6 +807,7 @@ do {                                                                               \
>                 .right_text = #right,                                          \
>         };                                                                     \
>                                                                                \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         if (likely(__left && __right))                                         \
>                 if (likely(memcmp(__left, __right, __size) op 0))              \
>                         break;                                                 \
> @@ -815,6 +832,7 @@ do {                                                                               \
>  do {                                                                          \
>         const typeof(ptr) __ptr = (ptr);                                       \
>                                                                                \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         if (!IS_ERR_OR_NULL(__ptr))                                            \
>                 break;                                                         \
>                                                                                \
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index c6ee4db0b3bd..2ec21c6918f3 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -91,9 +91,13 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
>
>         if (exit_code == -EFAULT)
>                 try_catch->try_result = 0;
> -       else if (exit_code == -EINTR)
> -               kunit_err(test, "try faulted\n");
> -       else if (exit_code == -ETIMEDOUT)
> +       else if (exit_code == -EINTR) {
> +               if (test->last_seen.file)
> +                       kunit_err(test, "try faulted after %s:%d\n",
> +                                 test->last_seen.file, test->last_seen.line);

It's possibly a bit confusing to just say "after file:line",
particularly if we then loop or call a function "higher up" in the
file. Maybe something like "try faulted: last line seen %s:%d" is
clearer.

> +               else
> +                       kunit_err(test, "try faulted before the first test\n");

I don't like using "test" here, as it introduces ambiguity between
"kunit tests" and "assertions/expectations" if we call them both
tests. Maybe just "try faulted" here, or "try faulted (no markers
seen)" or similar?


> +       } else if (exit_code == -ETIMEDOUT)
>                 kunit_err(test, "try timed out\n");
>         else if (exit_code)
>                 kunit_err(test, "Unknown error: %d\n", exit_code);
> --
> 2.44.0
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]