This is one of just 3 remaining "Test Module" kselftests (the others
being bitmap and scanf), the rest having been converted to KUnit.

I tested this using:

$ tools/testing/kunit/kunit.py run --arch arm64 --make_options LLVM=1 printf

I have also sent out a series converting scanf[0].

Link: https://lore.kernel.org/all/20250204-scanf-kunit-convert-v3-0-386d7c3ee714@gmail.com/T/#u [0]

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Tamir Duberstein (2):
      printf: convert self-test to KUnit
      printf: break kunit into test cases

 Documentation/core-api/printk-formats.rst |   2 +-
 MAINTAINERS                               |   2 +-
 arch/m68k/configs/amiga_defconfig         |   1 -
 arch/m68k/configs/apollo_defconfig        |   1 -
 arch/m68k/configs/atari_defconfig         |   1 -
 arch/m68k/configs/bvme6000_defconfig      |   1 -
 arch/m68k/configs/hp300_defconfig         |   1 -
 arch/m68k/configs/mac_defconfig           |   1 -
 arch/m68k/configs/multi_defconfig         |   1 -
 arch/m68k/configs/mvme147_defconfig       |   1 -
 arch/m68k/configs/mvme16x_defconfig       |   1 -
 arch/m68k/configs/q40_defconfig           |   1 -
 arch/m68k/configs/sun3_defconfig          |   1 -
 arch/m68k/configs/sun3x_defconfig         |   1 -
 arch/powerpc/configs/ppc64_defconfig      |   1 -
 lib/Kconfig.debug                         |  20 +-
 lib/Makefile                              |   2 +-
 lib/printf_kunit.c                        | 757 ++++++++++++++++++++++++++
 lib/test_printf.c                         | 863 ------------------------------
 tools/testing/selftests/lib/config        |   1 -
 tools/testing/selftests/lib/printf.sh     |   4 -
 21 files changed, 777 insertions(+), 887 deletions(-)
---
base-commit: a86bf2283d2c9769205407e2b54777c03d012939
change-id: 20250131-printf-kunit-convert-fd4012aa2ec6

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

On Tue, 4 Feb 2025 at 20:36, Tamir Duberstein <tamird@gmail.com> wrote:
>
> This is one of just 3 remaining "Test Module" kselftests (the others
> being bitmap and scanf), the rest having been converted to KUnit.
>
> I tested this using:
>
> $ tools/testing/kunit/kunit.py run --arch arm64 --make_options LLVM=1 printf
>
> I have also sent out a series converting scanf[0].
>
> Link: https://lore.kernel.org/all/20250204-scanf-kunit-convert-v3-0-386d7c3ee714@gmail.com/T/#u [0]
>

Sorry, but NAK, not in this form.

Please read the previous threads to understand what is wrong with this
mechanical approach. Not only is it wrong, it also actively makes the
test suite much less useful.

https://lore.kernel.org/lkml/f408efbd-10f7-f1dd-9baa-0f1233cafffc@rasmusvillemoes.dk/
https://lore.kernel.org/lkml/876cc862-56f1-7330-f988-0248bec2fbad@rasmusvillemoes.dk/
https://lore.kernel.org/lkml/0ab618c7-8c5c-00ae-8e08-0c1b99f3bf5c@rasmusvillemoes.dk/

I think the previous attempt was close to something acceptable (around
https://lore.kernel.org/lkml/57976ff4-7845-d721-ced1-1fe439000a9b@rasmusvillemoes.dk/),
but I don't know what happened to it.

Rasmus

On Thu, Feb 6, 2025 at 4:27 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On Tue, 4 Feb 2025 at 20:36, Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > This is one of just 3 remaining "Test Module" kselftests (the others
> > being bitmap and scanf), the rest having been converted to KUnit.
> >
> > I tested this using:
> >
> > $ tools/testing/kunit/kunit.py run --arch arm64 --make_options LLVM=1 printf
> >
> > I have also sent out a series converting scanf[0].
> >
> > Link: https://lore.kernel.org/all/20250204-scanf-kunit-convert-v3-0-386d7c3ee714@gmail.com/T/#u [0]
> >
>
> Sorry, but NAK, not in this form.
>
> Please read the previous threads to understand what is wrong with this
> mechanical approach. Not only is it wrong, it also actively makes the
> test suite much less useful.
>
> https://lore.kernel.org/lkml/f408efbd-10f7-f1dd-9baa-0f1233cafffc@rasmusvillemoes.dk/
> https://lore.kernel.org/lkml/876cc862-56f1-7330-f988-0248bec2fbad@rasmusvillemoes.dk/
> https://lore.kernel.org/lkml/0ab618c7-8c5c-00ae-8e08-0c1b99f3bf5c@rasmusvillemoes.dk/
>
> I think the previous attempt was close to something acceptable (around
> https://lore.kernel.org/lkml/57976ff4-7845-d721-ced1-1fe439000a9b@rasmusvillemoes.dk/),
> but I don't know what happened to it.
>
> Rasmus

Thanks Rasmus, I wasn't aware of that prior effort. I've gone through
and adopted your comments - the result is a first patch that is much
smaller (104 insertions(+), 104 deletions(-)) and failure messages
that are quite close to what is emitted now. I've taken care to keep
all the control flow the same, as you requested. The previous
discussion concluded with a promise to send another patch which didn't
happen. May I send a v2 with these changes, or are there more
fundamental objections? I'll also cc Arpitha and Brendan. The new
failure output:

    # ip4: EXPECTATION FAILED at lib/printf_kunit.c:95
vsnprintf(buf, 256, "%piS|%pIS", ...) wrote
'127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
    # ip4: EXPECTATION FAILED at lib/printf_kunit.c:95
vsnprintf(buf, 19, "%piS|%pIS", ...) wrote '127.000.000.001|12',
expected '127-000.000.001|12'
    # ip4: EXPECTATION FAILED at lib/printf_kunit.c:131
kvasprintf(..., "%piS|%pIS", ...) returned
'127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'

Cheers,
Tamir

On Thu, Feb 06 2025, Tamir Duberstein <tamird@gmail.com> wrote:

> On Thu, Feb 6, 2025 at 4:27 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> On Tue, 4 Feb 2025 at 20:36, Tamir Duberstein <tamird@gmail.com> wrote:
>> >
>> > This is one of just 3 remaining "Test Module" kselftests (the others
>> > being bitmap and scanf), the rest having been converted to KUnit.
>> >
>> > I tested this using:
>> >
>> > $ tools/testing/kunit/kunit.py run --arch arm64 --make_options LLVM=1 printf
>> >
>> > I have also sent out a series converting scanf[0].
>> >
>> > Link: https://lore.kernel.org/all/20250204-scanf-kunit-convert-v3-0-386d7c3ee714@gmail.com/T/#u [0]
>> >
>>
>> Sorry, but NAK, not in this form.
>>
>> Please read the previous threads to understand what is wrong with this
>> mechanical approach. Not only is it wrong, it also actively makes the
>> test suite much less useful.
>>
>> https://lore.kernel.org/lkml/f408efbd-10f7-f1dd-9baa-0f1233cafffc@rasmusvillemoes.dk/
>> https://lore.kernel.org/lkml/876cc862-56f1-7330-f988-0248bec2fbad@rasmusvillemoes.dk/
>> https://lore.kernel.org/lkml/0ab618c7-8c5c-00ae-8e08-0c1b99f3bf5c@rasmusvillemoes.dk/
>>
>> I think the previous attempt was close to something acceptable (around
>> https://lore.kernel.org/lkml/57976ff4-7845-d721-ced1-1fe439000a9b@rasmusvillemoes.dk/),
>> but I don't know what happened to it.
>>
>> Rasmus
>
> Thanks Rasmus, I wasn't aware of that prior effort. I've gone through
> and adopted your comments - the result is a first patch that is much
> smaller (104 insertions(+), 104 deletions(-)) and failure messages
> that are quite close to what is emitted now. I've taken care to keep
> all the control flow the same, as you requested. The previous
> discussion concluded with a promise to send another patch which didn't
> happen. May I send a v2 with these changes, or are there more
> fundamental objections? I'll also cc Arpitha and Brendan. The new
> failure output:
>
>     # ip4: EXPECTATION FAILED at lib/printf_kunit.c:95
> vsnprintf(buf, 256, "%piS|%pIS", ...) wrote
> '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
>     # ip4: EXPECTATION FAILED at lib/printf_kunit.c:95
> vsnprintf(buf, 19, "%piS|%pIS", ...) wrote '127.000.000.001|12',
> expected '127-000.000.001|12'
>     # ip4: EXPECTATION FAILED at lib/printf_kunit.c:131
> kvasprintf(..., "%piS|%pIS", ...) returned
> '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'

That certainly addresses one of my main objections; I really don't want to see
"memcmp(..., ...) == 1, expected memcmp(..., ...) == 0". And you said
you've kept the control flow/early returns the same, so that should also
be ok.

I'll have to see the actual code, of course. In general, I find reading
code using those KUNIT macros quite hard, because I'm not familiar with
those macros and when I try to look up what they do they turn out to be
defined in terms of other KUNIT macros 10 levels deep.

But that still leaves a few points. First, I really like that "388 test
cases passed" tally or some other free-form summary (so that I can see
that I properly hooked up, compiled, and ran a new testcase inside
test_number(), so any kind of aggregation on those top-level test_* is
too coarse).

The other thing I want to know is if this would make it harder for me to
finish up that "deterministic random testing" thing I wrote [*], but
never got around to actually get it upstream. It seems like something
that a framework like kunit could usefully provide out-of-the-box (which
is why I attempted to get it into kselftest), but as long as I'd still
be able to add in something like that, and get a "xxx failed, random
seed used was 0xabcdef" line printed, and run the test again setting the
seed explicitly to that 0xabcdef value, I'm good.

[*] https://lore.kernel.org/lkml/20201025214842.5924-4-linux@rasmusvillemoes.dk/

Rasmus

On Fri, Feb 7, 2025 at 5:01 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On Thu, Feb 06 2025, Tamir Duberstein <tamird@gmail.com> wrote:
>
> > On Thu, Feb 6, 2025 at 4:27 AM Rasmus Villemoes
> > <linux@rasmusvillemoes.dk> wrote:
> >>
> >> On Tue, 4 Feb 2025 at 20:36, Tamir Duberstein <tamird@gmail.com> wrote:
> >> >
> >> > This is one of just 3 remaining "Test Module" kselftests (the others
> >> > being bitmap and scanf), the rest having been converted to KUnit.
> >> >
> >> > I tested this using:
> >> >
> >> > $ tools/testing/kunit/kunit.py run --arch arm64 --make_options LLVM=1 printf
> >> >
> >> > I have also sent out a series converting scanf[0].
> >> >
> >> > Link: https://lore.kernel.org/all/20250204-scanf-kunit-convert-v3-0-386d7c3ee714@gmail.com/T/#u [0]
> >> >
> >>
> >> Sorry, but NAK, not in this form.
> >>
> >> Please read the previous threads to understand what is wrong with this
> >> mechanical approach. Not only is it wrong, it also actively makes the
> >> test suite much less useful.
> >>
> >> https://lore.kernel.org/lkml/f408efbd-10f7-f1dd-9baa-0f1233cafffc@rasmusvillemoes.dk/
> >> https://lore.kernel.org/lkml/876cc862-56f1-7330-f988-0248bec2fbad@rasmusvillemoes.dk/
> >> https://lore.kernel.org/lkml/0ab618c7-8c5c-00ae-8e08-0c1b99f3bf5c@rasmusvillemoes.dk/
> >>
> >> I think the previous attempt was close to something acceptable (around
> >> https://lore.kernel.org/lkml/57976ff4-7845-d721-ced1-1fe439000a9b@rasmusvillemoes.dk/),
> >> but I don't know what happened to it.
> >>
> >> Rasmus
> >
> > Thanks Rasmus, I wasn't aware of that prior effort. I've gone through
> > and adopted your comments - the result is a first patch that is much
> > smaller (104 insertions(+), 104 deletions(-)) and failure messages
> > that are quite close to what is emitted now. I've taken care to keep
> > all the control flow the same, as you requested. The previous
> > discussion concluded with a promise to send another patch which didn't
> > happen. May I send a v2 with these changes, or are there more
> > fundamental objections? I'll also cc Arpitha and Brendan. The new
> > failure output:
> >
> >     # ip4: EXPECTATION FAILED at lib/printf_kunit.c:95
> > vsnprintf(buf, 256, "%piS|%pIS", ...) wrote
> > '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
> >     # ip4: EXPECTATION FAILED at lib/printf_kunit.c:95
> > vsnprintf(buf, 19, "%piS|%pIS", ...) wrote '127.000.000.001|12',
> > expected '127-000.000.001|12'
> >     # ip4: EXPECTATION FAILED at lib/printf_kunit.c:131
> > kvasprintf(..., "%piS|%pIS", ...) returned
> > '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
>
> That certainly addresses one of my main objections; I really don't want to see
> "memcmp(..., ...) == 1, expected memcmp(..., ...) == 0". And you said
> you've kept the control flow/early returns the same, so that should also
> be ok.
>
> I'll have to see the actual code, of course. In general, I find reading
> code using those KUNIT macros quite hard, because I'm not familiar with
> those macros and when I try to look up what they do they turn out to be
> defined in terms of other KUNIT macros 10 levels deep.
>
> But that still leaves a few points. First, I really like that "388 test
> cases passed" tally or some other free-form summary (so that I can see
> that I properly hooked up, compiled, and ran a new testcase inside
> test_number(), so any kind of aggregation on those top-level test_* is
> too coarse).

This one I'm not sure how to address. What you're calling test cases
here would typically be referred to as assertions, and I'm not aware
of a way to report a count of assertions.

> The other thing I want to know is if this would make it harder for me to
> finish up that "deterministic random testing" thing I wrote [*], but
> never got around to actually get it upstream. It seems like something
> that a framework like kunit could usefully provide out-of-the-box (which
> is why I attempted to get it into kselftest), but as long as I'd still
> be able to add in something like that, and get a "xxx failed, random
> seed used was 0xabcdef" line printed, and run the test again setting the
> seed explicitly to that 0xabcdef value, I'm good.
>
> [*] https://lore.kernel.org/lkml/20201025214842.5924-4-linux@rasmusvillemoes.dk/

I can't speak for the framework, but it wouldn't be any harder to do
in printf itself. I did it this way:

+static struct rnd_state rnd_state;
+static u64 seed;
+
 static int printf_suite_init(struct kunit_suite *suite)
 {
  alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL);
  if (!alloced_buffer)
  return -1;
  test_buffer = alloced_buffer + PAD_SIZE;
+
+ seed = get_random_u64();
+ prandom_seed_state(&rnd_state, seed);
  return 0;
 }

 static void printf_suite_exit(struct kunit_suite *suite)
 {
  kfree(alloced_buffer);
+ if (kunit_suite_has_succeeded(suite) == KUNIT_FAILURE) {
+ pr_info("Seed: %llu\n", seed);
+ }
 }

and the result (once I made one of the cases fail):

printf_kunit: Seed: 11480747578984087668
# printf: pass:27 fail:1 skip:0 total:28
# Totals: pass:27 fail:1 skip:0 total:28
not ok 1 printf

Thank you both for engaging with me here. I'll send v2 in a few
minutes and we can continue the discussion there.
Tamir

On Fri, Feb 07 2025, Tamir Duberstein <tamird@gmail.com> wrote:

> On Fri, Feb 7, 2025 at 5:01 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> On Thu, Feb 06 2025, Tamir Duberstein <tamird@gmail.com> wrote:
>>
>>
>> I'll have to see the actual code, of course. In general, I find reading
>> code using those KUNIT macros quite hard, because I'm not familiar with
>> those macros and when I try to look up what they do they turn out to be
>> defined in terms of other KUNIT macros 10 levels deep.
>>
>> But that still leaves a few points. First, I really like that "388 test
>> cases passed" tally or some other free-form summary (so that I can see
>> that I properly hooked up, compiled, and ran a new testcase inside
>> test_number(), so any kind of aggregation on those top-level test_* is
>> too coarse).
>
> This one I'm not sure how to address. What you're calling test cases
> here would typically be referred to as assertions, and I'm not aware
> of a way to report a count of assertions.
>

I'm not sure that's accurate.

The thing is, each of the current test() instances results in four
different tests being done, which is roughly why we end up at the 4*97
== 388, but each of those tests has several assertions being done -
depending on which variant of the test we're doing (i.e. the buffer
length used or if we're passing it through kasprintf), we may do only
some of those assertions, and we do an early return in case one of those
assertions fail (because it wouldn't be safe to do the following
assertions, and the test as such has failed already). So there are far
more assertions than those 388.

OTOH, that the number reported is 388 is more a consequence of the
implementation than anything explicitly designed. I can certainly live
with 388 being replaced by 97, i.e. that each current test() invocation
would count as one KUNIT case, as that would still allow me to detect a
PEBKAC when I've added a new test() instance and failed to actually run
that.

>> The other thing I want to know is if this would make it harder for me to
>> finish up that "deterministic random testing" thing I wrote [*], but
>> never got around to actually get it upstream. It seems like something
>> that a framework like kunit could usefully provide out-of-the-box (which
>> is why I attempted to get it into kselftest), but as long as I'd still
>> be able to add in something like that, and get a "xxx failed, random
>> seed used was 0xabcdef" line printed, and run the test again setting the
>> seed explicitly to that 0xabcdef value, I'm good.
>>
>> [*] https://lore.kernel.org/lkml/20201025214842.5924-4-linux@rasmusvillemoes.dk/
>
> I can't speak for the framework, but it wouldn't be any harder to do
> in printf itself. I did it this way:
>
> +static struct rnd_state rnd_state;
> +static u64 seed;
> +
>  static int printf_suite_init(struct kunit_suite *suite)
>  {
>   alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL);
>   if (!alloced_buffer)
>   return -1;
>   test_buffer = alloced_buffer + PAD_SIZE;
> +
> + seed = get_random_u64();
> + prandom_seed_state(&rnd_state, seed);
>   return 0;
>  }
>
>  static void printf_suite_exit(struct kunit_suite *suite)
>  {
>   kfree(alloced_buffer);
> + if (kunit_suite_has_succeeded(suite) == KUNIT_FAILURE) {
> + pr_info("Seed: %llu\n", seed);
> + }
>  }
>
> and the result (once I made one of the cases fail):
>
> printf_kunit: Seed: 11480747578984087668
> # printf: pass:27 fail:1 skip:0 total:28
> # Totals: pass:27 fail:1 skip:0 total:28
> not ok 1 printf
>

OK, that's good. I think one of the problems previously was that there
no longer was such an _init/_exit pair one could hook into to do the
seed logic and afterwards do something depending on the success/fail of
the whole thing; that was all hidden away by some KUNIT_ wrapping.

Is it still possible to trivially make that seed into a module
parameter, and do the "modprobe test_printf seed=0xabcd", or otherwise
inject a module parameter when run/loaded via the kunit framework?

Rasmus

On Mon, 10 Feb 2025 at 19:57, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
> On Fri, Feb 07 2025, Tamir Duberstein <tamird@gmail.com> wrote:
>
> > On Fri, Feb 7, 2025 at 5:01 AM Rasmus Villemoes
> > <linux@rasmusvillemoes.dk> wrote:
> >>
> >> On Thu, Feb 06 2025, Tamir Duberstein <tamird@gmail.com> wrote:
> >>
> >>
> >> I'll have to see the actual code, of course. In general, I find reading
> >> code using those KUNIT macros quite hard, because I'm not familiar with
> >> those macros and when I try to look up what they do they turn out to be
> >> defined in terms of other KUNIT macros 10 levels deep.
> >>
> >> But that still leaves a few points. First, I really like that "388 test
> >> cases passed" tally or some other free-form summary (so that I can see
> >> that I properly hooked up, compiled, and ran a new testcase inside
> >> test_number(), so any kind of aggregation on those top-level test_* is
> >> too coarse).
> >
> > This one I'm not sure how to address. What you're calling test cases
> > here would typically be referred to as assertions, and I'm not aware
> > of a way to report a count of assertions.
> >
>
> I'm not sure that's accurate.
>
> The thing is, each of the current test() instances results in four
> different tests being done, which is roughly why we end up at the 4*97
> == 388, but each of those tests has several assertions being done -
> depending on which variant of the test we're doing (i.e. the buffer
> length used or if we're passing it through kasprintf), we may do only
> some of those assertions, and we do an early return in case one of those
> assertions fail (because it wouldn't be safe to do the following
> assertions, and the test as such has failed already). So there are far
> more assertions than those 388.
>
> OTOH, that the number reported is 388 is more a consequence of the
> implementation than anything explicitly designed. I can certainly live
> with 388 being replaced by 97, i.e. that each current test() invocation
> would count as one KUNIT case, as that would still allow me to detect a
> PEBKAC when I've added a new test() instance and failed to actually run
> that.

It'd be possible to split things up further into tests, at the cost of
it being a more extensive refactoring, if having the more granular
count tracked by KUnit were desired. It'd also be possible to make
these more explicitly data driven via a parameterised test (so each
input/output pair is listed in an array, and automatically gets
converted to a KUnit subtest).

There are some advantages to having these counts done by the
framework, particularly in that any inconsistencies can be picked up
by the tooling. Ultimately, though, it's up to you as to what is most
useful.

> >> The other thing I want to know is if this would make it harder for me to
> >> finish up that "deterministic random testing" thing I wrote [*], but
> >> never got around to actually get it upstream. It seems like something
> >> that a framework like kunit could usefully provide out-of-the-box (which
> >> is why I attempted to get it into kselftest), but as long as I'd still
> >> be able to add in something like that, and get a "xxx failed, random
> >> seed used was 0xabcdef" line printed, and run the test again setting the
> >> seed explicitly to that 0xabcdef value, I'm good.
> >>
> >> [*] https://lore.kernel.org/lkml/20201025214842.5924-4-linux@rasmusvillemoes.dk/
> >
> > I can't speak for the framework, but it wouldn't be any harder to do
> > in printf itself. I did it this way:
> >
> > +static struct rnd_state rnd_state;
> > +static u64 seed;
> > +
> >  static int printf_suite_init(struct kunit_suite *suite)
> >  {
> >   alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL);
> >   if (!alloced_buffer)
> >   return -1;
> >   test_buffer = alloced_buffer + PAD_SIZE;
> > +
> > + seed = get_random_u64();
> > + prandom_seed_state(&rnd_state, seed);
> >   return 0;
> >  }
> >
> >  static void printf_suite_exit(struct kunit_suite *suite)
> >  {
> >   kfree(alloced_buffer);
> > + if (kunit_suite_has_succeeded(suite) == KUNIT_FAILURE) {
> > + pr_info("Seed: %llu\n", seed);
> > + }
> >  }
> >
> > and the result (once I made one of the cases fail):
> >
> > printf_kunit: Seed: 11480747578984087668
> > # printf: pass:27 fail:1 skip:0 total:28
> > # Totals: pass:27 fail:1 skip:0 total:28
> > not ok 1 printf
> >
>
> OK, that's good. I think one of the problems previously was that there
> no longer was such an _init/_exit pair one could hook into to do the
> seed logic and afterwards do something depending on the success/fail of
> the whole thing; that was all hidden away by some KUNIT_ wrapping.

Yeah, KUnit has since added the suite_init/suite_exit functions in
order to do this sort of thing. Previously we had an _init/_exit pair,
but it was run per-test-case, which doesn't work as well here.

>
> Is it still possible to trivially make that seed into a module
> parameter, and do the "modprobe test_printf seed=0xabcd", or otherwise
> inject a module parameter when run/loaded via the kunit framework?

It should be just the same as any other module. As mentioned, one day
I'd like to standardise this in KUnit so that we can have this also
change the test execution order and fit in with tooling, but I'd
definitely support doing this via an ad-hoc parameter in the meantime.

Cheers,
-- David

On Tue, Feb 11 2025, David Gow <davidgow@google.com> wrote:

> On Mon, 10 Feb 2025 at 19:57, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>>
>> On Fri, Feb 07 2025, Tamir Duberstein <tamird@gmail.com> wrote:
>>
>> > On Fri, Feb 7, 2025 at 5:01 AM Rasmus Villemoes
>> > <linux@rasmusvillemoes.dk> wrote:
>> >>
>> >> On Thu, Feb 06 2025, Tamir Duberstein <tamird@gmail.com> wrote:
>> >>
>> >>
>> >> I'll have to see the actual code, of course. In general, I find reading
>> >> code using those KUNIT macros quite hard, because I'm not familiar with
>> >> those macros and when I try to look up what they do they turn out to be
>> >> defined in terms of other KUNIT macros 10 levels deep.
>> >>
>> >> But that still leaves a few points. First, I really like that "388 test
>> >> cases passed" tally or some other free-form summary (so that I can see
>> >> that I properly hooked up, compiled, and ran a new testcase inside
>> >> test_number(), so any kind of aggregation on those top-level test_* is
>> >> too coarse).
>> >
>> > This one I'm not sure how to address. What you're calling test cases
>> > here would typically be referred to as assertions, and I'm not aware
>> > of a way to report a count of assertions.
>> >
>>
>> I'm not sure that's accurate.
>>
>> The thing is, each of the current test() instances results in four
>> different tests being done, which is roughly why we end up at the 4*97
>> == 388, but each of those tests has several assertions being done -
>> depending on which variant of the test we're doing (i.e. the buffer
>> length used or if we're passing it through kasprintf), we may do only
>> some of those assertions, and we do an early return in case one of those
>> assertions fail (because it wouldn't be safe to do the following
>> assertions, and the test as such has failed already). So there are far
>> more assertions than those 388.
>>
>> OTOH, that the number reported is 388 is more a consequence of the
>> implementation than anything explicitly designed. I can certainly live
>> with 388 being replaced by 97, i.e. that each current test() invocation
>> would count as one KUNIT case, as that would still allow me to detect a
>> PEBKAC when I've added a new test() instance and failed to actually run
>> that.
>
> It'd be possible to split things up further into tests, at the cost of
> it being a more extensive refactoring, if having the more granular
> count tracked by KUnit were desired.

I think the problem is that kunit is simply not a good framework to do
these kinds of tests in, and certainly it's very hard to retrofit kunit
after the fact.

It'd also be possible to make
> these more explicitly data driven via a parameterised test (so each
> input/output pair is listed in an array, and automatically gets
> converted to a KUnit subtest).

So that "array of input/output" very much doesn't work for these
specific tests: We really want the format string/varargs to be checked
by the compiler, and besides, there's no way to store the necessary
varargs and generate a call from those in an array. Moreover, we verify a
lot more than just that the correct string is produced; it's also a
matter of the right return value regardless of the passed buffer size, etc.

That's also why is nigh impossible to simply change __test() into
(another) macro that expands to something that defines an individual
struct kunit_case, because the framework is really built around the
notion that each case can be represented by a void function call and the
name of the test is the stringification of the function name. 

So I don't mind the conversion to kunit if that really helps other
people, as long as the basic functionality is still present and doesn't
impede future extensions - and certainly I don't want to end up in a
situation where somebody adds a new %p extension but cannot really add a
test for it because kunit makes that hard.

But I hope you all agree that it doesn't make much _sense_ to consider
test_number() and test_string() and so on individual "test cases"; the
atomic units of test being done in the printf suite is each invocation
of the __test() function, with one specific format string/varargs
combination.

Rasmus

On Tue, 11 Feb 2025 at 16:40, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
> On Tue, Feb 11 2025, David Gow <davidgow@google.com> wrote:
>
> > On Mon, 10 Feb 2025 at 19:57, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >>
> >> On Fri, Feb 07 2025, Tamir Duberstein <tamird@gmail.com> wrote:
> >>
> >> > On Fri, Feb 7, 2025 at 5:01 AM Rasmus Villemoes
> >> > <linux@rasmusvillemoes.dk> wrote:
> >> >>
> >> >> On Thu, Feb 06 2025, Tamir Duberstein <tamird@gmail.com> wrote:
> >> >>
> >> >>
> >> >> I'll have to see the actual code, of course. In general, I find reading
> >> >> code using those KUNIT macros quite hard, because I'm not familiar with
> >> >> those macros and when I try to look up what they do they turn out to be
> >> >> defined in terms of other KUNIT macros 10 levels deep.
> >> >>
> >> >> But that still leaves a few points. First, I really like that "388 test
> >> >> cases passed" tally or some other free-form summary (so that I can see
> >> >> that I properly hooked up, compiled, and ran a new testcase inside
> >> >> test_number(), so any kind of aggregation on those top-level test_* is
> >> >> too coarse).
> >> >
> >> > This one I'm not sure how to address. What you're calling test cases
> >> > here would typically be referred to as assertions, and I'm not aware
> >> > of a way to report a count of assertions.
> >> >
> >>
> >> I'm not sure that's accurate.
> >>
> >> The thing is, each of the current test() instances results in four
> >> different tests being done, which is roughly why we end up at the 4*97
> >> == 388, but each of those tests has several assertions being done -
> >> depending on which variant of the test we're doing (i.e. the buffer
> >> length used or if we're passing it through kasprintf), we may do only
> >> some of those assertions, and we do an early return in case one of those
> >> assertions fail (because it wouldn't be safe to do the following
> >> assertions, and the test as such has failed already). So there are far
> >> more assertions than those 388.
> >>
> >> OTOH, that the number reported is 388 is more a consequence of the
> >> implementation than anything explicitly designed. I can certainly live
> >> with 388 being replaced by 97, i.e. that each current test() invocation
> >> would count as one KUNIT case, as that would still allow me to detect a
> >> PEBKAC when I've added a new test() instance and failed to actually run
> >> that.
> >
> > It'd be possible to split things up further into tests, at the cost of
> > it being a more extensive refactoring, if having the more granular
> > count tracked by KUnit were desired.
>
> I think the problem is that kunit is simply not a good framework to do
> these kinds of tests in, and certainly it's very hard to retrofit kunit
> after the fact.
>

I think I'd have to disagree on the whole (though I'm admittedly
biased in KUnit's favour), but I can definitely see that the printf
tests do provide some unique challenges, and that either way, a port
would require either some code churn or bloat, a need to reinterpret
things (such as what unit a 'test' is), or both.

Ultimately, I don't want to force KUnit on anyone if it's going to
make things more difficult, so it's ultimately up to you. My personal
feeling is that this could work well as a KUnit test, but due to the
churn involved, it may not be worth it if no-one wants to take
advantage of the tooling.

> It'd also be possible to make
> > these more explicitly data driven via a parameterised test (so each
> > input/output pair is listed in an array, and automatically gets
> > converted to a KUnit subtest).
>
> So that "array of input/output" very much doesn't work for these
> specific tests: We really want the format string/varargs to be checked
> by the compiler, and besides, there's no way to store the necessary
> varargs and generate a call from those in an array. Moreover, we verify a
> lot more than just that the correct string is produced; it's also a
> matter of the right return value regardless of the passed buffer size, etc.

Ah, that makes sense. I suspect with enough work and some friendly
compiler developers, this could be make to work, but it definitely
doesn't seem worth the effort to me.

> That's also why is nigh impossible to simply change __test() into
> (another) macro that expands to something that defines an individual
> struct kunit_case, because the framework is really built around the
> notion that each case can be represented by a void function call and the
> name of the test is the stringification of the function name.

Yeah: it may be possible to do something with KUnit's parameter
generating functions (you can have a function which generates a void*
test context, as well as a string test name: this could be a struct
with a format string and a va_list), but it's definitely getting
complicated.

> So I don't mind the conversion to kunit if that really helps other
> people, as long as the basic functionality is still present and doesn't
> impede future extensions - and certainly I don't want to end up in a
> situation where somebody adds a new %p extension but cannot really add a
> test for it because kunit makes that hard.
>
> But I hope you all agree that it doesn't make much _sense_ to consider
> test_number() and test_string() and so on individual "test cases"; the
> atomic units of test being done in the printf suite is each invocation
> of the __test() function, with one specific format string/varargs
> combination.

I think this is -- to some extent -- a matter of interpretation. I
don't think it's wrong to use KUnit test cases to refer to a "thing
being tested" (e.g., a specific format specifier) rather than an
"individual invocation": lots of KUnit tests already group very
related things together. But given the way there are several "checks"
within each __test() invocation mirrors this already, I understand why
it'd make sense to keep that as the "test case".

I don't have any immediate plans personally to work on the
printf/scanf code, so your opinion here definitely matters more than
mine. But if this does end up as a KUnit test, I'll definitely keep an
eye on it as part of my regular KUnit test runs.

Cheers,
-- David

>
> Rasmus

On Tue, Feb 11, 2025 at 3:40 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On Tue, Feb 11 2025, David Gow <davidgow@google.com> wrote:
>
> > On Mon, 10 Feb 2025 at 19:57, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >>
> >> On Fri, Feb 07 2025, Tamir Duberstein <tamird@gmail.com> wrote:
> >>
> >> > On Fri, Feb 7, 2025 at 5:01 AM Rasmus Villemoes
> >> > <linux@rasmusvillemoes.dk> wrote:
> >> >>
> >> >> On Thu, Feb 06 2025, Tamir Duberstein <tamird@gmail.com> wrote:
> >> >>
> >> >>
> >> >> I'll have to see the actual code, of course. In general, I find reading
> >> >> code using those KUNIT macros quite hard, because I'm not familiar with
> >> >> those macros and when I try to look up what they do they turn out to be
> >> >> defined in terms of other KUNIT macros 10 levels deep.
> >> >>
> >> >> But that still leaves a few points. First, I really like that "388 test
> >> >> cases passed" tally or some other free-form summary (so that I can see
> >> >> that I properly hooked up, compiled, and ran a new testcase inside
> >> >> test_number(), so any kind of aggregation on those top-level test_* is
> >> >> too coarse).
> >> >
> >> > This one I'm not sure how to address. What you're calling test cases
> >> > here would typically be referred to as assertions, and I'm not aware
> >> > of a way to report a count of assertions.
> >> >
> >>
> >> I'm not sure that's accurate.
> >>
> >> The thing is, each of the current test() instances results in four
> >> different tests being done, which is roughly why we end up at the 4*97
> >> == 388, but each of those tests has several assertions being done -
> >> depending on which variant of the test we're doing (i.e. the buffer
> >> length used or if we're passing it through kasprintf), we may do only
> >> some of those assertions, and we do an early return in case one of those
> >> assertions fail (because it wouldn't be safe to do the following
> >> assertions, and the test as such has failed already). So there are far
> >> more assertions than those 388.
> >>
> >> OTOH, that the number reported is 388 is more a consequence of the
> >> implementation than anything explicitly designed. I can certainly live
> >> with 388 being replaced by 97, i.e. that each current test() invocation
> >> would count as one KUNIT case, as that would still allow me to detect a
> >> PEBKAC when I've added a new test() instance and failed to actually run
> >> that.
> >
> > It'd be possible to split things up further into tests, at the cost of
> > it being a more extensive refactoring, if having the more granular
> > count tracked by KUnit were desired.
>
> I think the problem is that kunit is simply not a good framework to do
> these kinds of tests in, and certainly it's very hard to retrofit kunit
> after the fact.
>
> It'd also be possible to make
> > these more explicitly data driven via a parameterised test (so each
> > input/output pair is listed in an array, and automatically gets
> > converted to a KUnit subtest).
>
> So that "array of input/output" very much doesn't work for these
> specific tests: We really want the format string/varargs to be checked
> by the compiler, and besides, there's no way to store the necessary
> varargs and generate a call from those in an array. Moreover, we verify a
> lot more than just that the correct string is produced; it's also a
> matter of the right return value regardless of the passed buffer size, etc.
>
> That's also why is nigh impossible to simply change __test() into
> (another) macro that expands to something that defines an individual
> struct kunit_case, because the framework is really built around the
> notion that each case can be represented by a void function call and the
> name of the test is the stringification of the function name.
>
> So I don't mind the conversion to kunit if that really helps other
> people, as long as the basic functionality is still present and doesn't
> impede future extensions - and certainly I don't want to end up in a
> situation where somebody adds a new %p extension but cannot really add a
> test for it because kunit makes that hard.
>
> But I hope you all agree that it doesn't make much _sense_ to consider
> test_number() and test_string() and so on individual "test cases"; the
> atomic units of test being done in the printf suite is each invocation
> of the __test() function, with one specific format string/varargs
> combination.
>
> Rasmus

Rasmus, does v3 meet your needs?

https://lore.kernel.org/all/20250210-printf-kunit-convert-v3-1-ee6ac5500f5e@gmail.com/

Weirdly the cover letter seems to be missing on lore, should I resend?

On Tue, Feb 11, 2025 at 6:34 AM Tamir Duberstein <tamird@gmail.com> wrote:
>
> https://lore.kernel.org/all/20250210-printf-kunit-convert-v3-1-ee6ac5500f5e@gmail.com/
>
> Weirdly the cover letter seems to be missing on lore, should I resend?

It's there now.
https://lore.kernel.org/all/20250210-printf-kunit-convert-v3-0-ee6ac5500f5e@gmail.com/

On Thu, 6 Feb 2025 at 23:42, Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Thu, Feb 6, 2025 at 4:27 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> >
> > On Tue, 4 Feb 2025 at 20:36, Tamir Duberstein <tamird@gmail.com> wrote:
> > >
> > > This is one of just 3 remaining "Test Module" kselftests (the others
> > > being bitmap and scanf), the rest having been converted to KUnit.
> > >
> > > I tested this using:
> > >
> > > $ tools/testing/kunit/kunit.py run --arch arm64 --make_options LLVM=1 printf
> > >
> > > I have also sent out a series converting scanf[0].
> > >
> > > Link: https://lore.kernel.org/all/20250204-scanf-kunit-convert-v3-0-386d7c3ee714@gmail.com/T/#u [0]
> > >
> >
> > Sorry, but NAK, not in this form.
> >
> > Please read the previous threads to understand what is wrong with this
> > mechanical approach. Not only is it wrong, it also actively makes the
> > test suite much less useful.
> >
> > https://lore.kernel.org/lkml/f408efbd-10f7-f1dd-9baa-0f1233cafffc@rasmusvillemoes.dk/
> > https://lore.kernel.org/lkml/876cc862-56f1-7330-f988-0248bec2fbad@rasmusvillemoes.dk/
> > https://lore.kernel.org/lkml/0ab618c7-8c5c-00ae-8e08-0c1b99f3bf5c@rasmusvillemoes.dk/
> >
> > I think the previous attempt was close to something acceptable (around
> > https://lore.kernel.org/lkml/57976ff4-7845-d721-ced1-1fe439000a9b@rasmusvillemoes.dk/),
> > but I don't know what happened to it.
> >
> > Rasmus
>
> Thanks Rasmus, I wasn't aware of that prior effort. I've gone through
> and adopted your comments - the result is a first patch that is much
> smaller (104 insertions(+), 104 deletions(-)) and failure messages
> that are quite close to what is emitted now. I've taken care to keep
> all the control flow the same, as you requested. The previous
> discussion concluded with a promise to send another patch which didn't
> happen. May I send a v2 with these changes, or are there more
> fundamental objections? I'll also cc Arpitha and Brendan. The new
> failure output:
>
>     # ip4: EXPECTATION FAILED at lib/printf_kunit.c:95
> vsnprintf(buf, 256, "%piS|%pIS", ...) wrote
> '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
>     # ip4: EXPECTATION FAILED at lib/printf_kunit.c:95
> vsnprintf(buf, 19, "%piS|%pIS", ...) wrote '127.000.000.001|12',
> expected '127-000.000.001|12'
>     # ip4: EXPECTATION FAILED at lib/printf_kunit.c:131
> kvasprintf(..., "%piS|%pIS", ...) returned
> '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
>

This failure message looks good to me. The ones in the current patch
are very verbose, and while the memory comparisons could be useful for
the overflow/buffer size tests, for simple string comparisons, having
the string in a readable format is best.

Rasmus: the KUnit framework has since added a summary line to the
output by default, which should also make this less of a regression
from the existing format:
# printf: pass:28 fail:0 skip:0 total:28

Cheers,
-- David