[2/3] kunit: Improve format of the PTR_EQ|NE|NULL assertion

Diagnostic message for failed KUNIT_ASSERT|EXPECT_PTR_EQ|NE|NULL
shows only raw pointer value, like for this example:

  void *ptr1 = ERR_PTR(-ENOMEM);
  void *ptr2 = NULL;
  KUNIT_EXPECT_PTR_EQ(test, ptr1, ptr2);
  KUNIT_EXPECT_NULL(test, ptr1);

we will get:

  [ ] Expected ptr1 == ptr2, but
  [ ]     ptr1 == fffffffffffffff4
  [ ]     ptr2 == 0000000000000000
  [ ] Expected ptr1 == ((void *)0), but
  [ ]     ptr1 == ffffffffffffffe4
  [ ]     ((void *)0) == 0000000000000000

but we can improve this by detecting whether pointer was NULL or
error, and use friendly error pointer format if possible:

  [ ] Expected ptr1 == ptr2, but
  [ ]     ptr1 is -ENOMEM
  [ ]     ptr2 is NULL
  [ ] Expected ptr1 == ((void *)0), but
  [ ]     ptr1 is -ENOMEM
  [ ]     ((void *)0) is NULL

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
Cc: David Gow <davidgow@google.com>
Cc: Rae Moar <rmoar@google.com>
---
 lib/kunit/assert.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

On Thu, 22 Aug 2024 at 03:15, Michal Wajdeczko
<michal.wajdeczko@intel.com> wrote:
>
> Diagnostic message for failed KUNIT_ASSERT|EXPECT_PTR_EQ|NE|NULL
> shows only raw pointer value, like for this example:
>
>   void *ptr1 = ERR_PTR(-ENOMEM);
>   void *ptr2 = NULL;
>   KUNIT_EXPECT_PTR_EQ(test, ptr1, ptr2);
>   KUNIT_EXPECT_NULL(test, ptr1);
>
> we will get:
>
>   [ ] Expected ptr1 == ptr2, but
>   [ ]     ptr1 == fffffffffffffff4
>   [ ]     ptr2 == 0000000000000000
>   [ ] Expected ptr1 == ((void *)0), but
>   [ ]     ptr1 == ffffffffffffffe4
>   [ ]     ((void *)0) == 0000000000000000
>
> but we can improve this by detecting whether pointer was NULL or
> error, and use friendly error pointer format if possible:
>
>   [ ] Expected ptr1 == ptr2, but
>   [ ]     ptr1 is -ENOMEM
>   [ ]     ptr2 is NULL
>   [ ] Expected ptr1 == ((void *)0), but
>   [ ]     ptr1 is -ENOMEM
>   [ ]     ((void *)0) is NULL
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
> Cc: David Gow <davidgow@google.com>
> Cc: Rae Moar <rmoar@google.com>
> ---

I have some mixed feelings about this one. Personally, I'd rather this
continue to use '==' rather than 'is', just for consistency in case
anyone wants to parse these.

Equally, I'd like to have the actual value printed for error pointers.
The PTR_NULL assertions are not intended for use with error pointers,
and the PTR_{EQ,NE} ones may or may not treat high pointers as actual
addresses, or as errors. (We often need the exact value in debugging
some of the usercopy tests, which do horrific things like rely on
pointer wraparound, so have non-error 0xffffffxx pointers around.)

I'd personally go for, e.g, "ptr1 == fffffffffffffff4 (-ENOMEM)".

Thanks,
-- David




>  lib/kunit/assert.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> index 6e4333d0c6a0..8da89043b734 100644
> --- a/lib/kunit/assert.c
> +++ b/lib/kunit/assert.c
> @@ -155,12 +155,28 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
>                           binary_assert->text->left_text,
>                           binary_assert->text->operation,
>                           binary_assert->text->right_text);
> -       string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px\n",
> -                         binary_assert->text->left_text,
> -                         binary_assert->left_value);
> -       string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px",
> -                         binary_assert->text->right_text,
> -                         binary_assert->right_value);
> +       if (!binary_assert->left_value)
> +               string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s is NULL\n",
> +                                 binary_assert->text->left_text);
> +       else if (IS_ERR(binary_assert->left_value))
> +               string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s is %pe\n",
> +                                 binary_assert->text->left_text,
> +                                 binary_assert->left_value);
> +       else
> +               string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px\n",
> +                                 binary_assert->text->left_text,
> +                                 binary_assert->left_value);
> +       if (!binary_assert->right_value)
> +               string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s is NULL\n",
> +                                 binary_assert->text->right_text);
> +       else if (IS_ERR(binary_assert->right_value))
> +               string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s is %pe\n",
> +                                 binary_assert->text->right_text,
> +                                 binary_assert->right_value);
> +       else
> +               string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px\n",
> +                                 binary_assert->text->right_text,
> +                                 binary_assert->right_value);
>         kunit_assert_print_msg(message, stream);
>  }
>  EXPORT_SYMBOL_GPL(kunit_binary_ptr_assert_format);
> --
> 2.43.0
>