On Thu, Feb 11, 2021 at 7:40 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> On Thu, 11 Feb 2021, David Gow wrote:
>
> > On Wed, Feb 10, 2021 at 6:14 AM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
> > >
> > > From: Uriel Guajardo <urielguajardo@xxxxxxxxxx>
> > >
> > > Add a kunit_fail_current_test() function to fail the currently running
> > > test, if any, with an error message.
> > >
> > > This is largely intended for dynamic analysis tools like UBSAN and for
> > > fakes.
> > > E.g. say I had a fake ops struct for testing and I wanted my `free`
> > > function to complain if it was called with an invalid argument, or
> > > caught a double-free. Most return void and have no normal means of
> > > signalling failure (e.g. super_operations, iommu_ops, etc.).
> > >
> > > Key points:
> > > * Always update current->kunit_test so anyone can use it.
> > > * commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for
> > > CONFIG_KASAN=y
> > >
> > > * Create a new header <kunit/test-bug.h> so non-test code doesn't have
> > > to include all of <kunit/test.h> (e.g. lib/ubsan.c)
> > >
> > > * Forward the file and line number to make it easier to track down
> > > failures
> > >
> > > * Declare the helper function for nice __printf() warnings about mismatched
> > > format strings even when KUnit is not enabled.
> > >
> > > Example output from kunit_fail_current_test("message"):
> > > [15:19:34] [FAILED] example_simple_test
> > > [15:19:34] # example_simple_test: initializing
> > > [15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: message
> > > [15:19:34] not ok 1 - example_simple_test
> > >
> > > Co-developed-by: Daniel Latypov <dlatypov@xxxxxxxxxx>
> > > Signed-off-by: Uriel Guajardo <urielguajardo@xxxxxxxxxx>
> > > Signed-off-by: Daniel Latypov <dlatypov@xxxxxxxxxx>
> > > ---
> > > include/kunit/test-bug.h | 30 ++++++++++++++++++++++++++++++
> > > lib/kunit/test.c | 37 +++++++++++++++++++++++++++++++++----
> > > 2 files changed, 63 insertions(+), 4 deletions(-)
> > > create mode 100644 include/kunit/test-bug.h
> > >
> > > diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
> > > new file mode 100644
> > > index 000000000000..18b1034ec43a
> > > --- /dev/null
> > > +++ b/include/kunit/test-bug.h
> > > @@ -0,0 +1,30 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * KUnit API allowing dynamic analysis tools to interact with KUnit tests
> > > + *
> > > + * Copyright (C) 2020, Google LLC.
> > > + * Author: Uriel Guajardo <urielguajardo@xxxxxxxxxx>
> > > + */
> > > +
> > > +#ifndef _KUNIT_TEST_BUG_H
> > > +#define _KUNIT_TEST_BUG_H
> > > +
> > > +#define kunit_fail_current_test(fmt, ...) \
> > > + __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__)
> > > +
> > > +#if IS_ENABLED(CONFIG_KUNIT)
> >
> > As the kernel test robot has pointed out on the second patch, this
> > probably should be IS_BUILTIN(), otherwise this won't build if KUnit
> > is a module, and the code calling it isn't.
> >
> > This does mean that things like UBSAN integration won't work if KUnit
> > is a module, which is a shame.
> >
> > (It's worth noting that the KASAN integration worked around this by
> > only calling inline functions, which would therefore be built-in even
> > if the rest of KUnit was built as a module. I don't think it's quite
> > as convenient to do that here, though.)
> >
>
> Right, static inline'ing __kunit_fail_current_test() seems problematic
> because it calls other exported functions; more below....
>
> > > +
> > > +extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
> > > + const char *fmt, ...);
> > > +
> > > +#else
> > > +
> > > +static __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
> > > + const char *fmt, ...)
> > > +{
> > > +}
> > > +
> > > +#endif
> > > +
> > > +
> > > +#endif /* _KUNIT_TEST_BUG_H */
> > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > > index ec9494e914ef..5794059505cf 100644
> > > --- a/lib/kunit/test.c
> > > +++ b/lib/kunit/test.c
> > > @@ -7,6 +7,7 @@
> > > */
> > >
> > > #include <kunit/test.h>
> > > +#include <kunit/test-bug.h>
> > > #include <linux/kernel.h>
> > > #include <linux/kref.h>
> > > #include <linux/sched/debug.h>
> > > @@ -16,6 +17,38 @@
> > > #include "string-stream.h"
> > > #include "try-catch-impl.h"
> > >
> > > +/*
> > > + * Fail the current test and print an error message to the log.
> > > + */
> > > +void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
> > > +{
> > > + va_list args;
> > > + int len;
> > > + char *buffer;
> > > +
> > > + if (!current->kunit_test)
> > > + return;
> > > +
> > > + kunit_set_failure(current->kunit_test);
> > > +
>
> currently kunit_set_failure() is static, but it could be inlined I
> suspect.
>
> > > + /* kunit_err() only accepts literals, so evaluate the args first. */
> > > + va_start(args, fmt);
> > > + len = vsnprintf(NULL, 0, fmt, args) + 1;
> > > + va_end(args);
> > > +
> > > + buffer = kunit_kmalloc(current->kunit_test, len, GFP_KERNEL);
>
> kunit_kmalloc()/kunit_kfree() are exported also, but we could probably
> dodge allocation with a static buffer. In fact since we end up
> using an on-stack buffer for logging in kunit_log_append(), it might make
Ah, right there's those as well.
I originally had it on the stack, but the fact we use an on-stack
buffer is why I switched over.
I originally had it as a macro as you do now but liked the __printf()
annotation to be closer* to the user's code and now down through
several layers of macros (kunit_fail_current_test => kunit_err =>
kunit_printk => kunit_log => printk).
And then having it on the stack and then calling into
kunit_log_append() would (naively) use up 2 *KUNIT_LOG_SIZE stack
space.
So only a minor concern, and so I like the simpler def using a macro
given the messiness.
(But we'd give up the __printf checking when compiling w/o KUnit,
which is a bit sad)
*E.g. if one misuses kunit_err(), we get this message which is
understandable, but a bit more noisy than I'd prefer.
../include/linux/kern_levels.h:5:18: warning: format ‘%d’ expects
argument of type ‘int’, but argument 3 has type ‘char *’ [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
../include/kunit/test.h:621:10: note: in definition of macro ‘kunit_log’
621 | printk(lvl fmt, ##__VA_ARGS__); \
| ^~~
../include/kunit/test.h:662:2: note: in expansion of macro ‘kunit_printk’
662 | kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~~
../include/linux/kern_levels.h:11:18: note: in expansion of macro ‘KERN_SOH’
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
../include/kunit/test.h:662:15: note: in expansion of macro ‘KERN_ERR’
662 | kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
| ^~~~~~~~
../lib/kunit/kunit-example-test.c:30:2: note: in expansion of macro ‘kunit_err’
30 | kunit_err(test, "invalid format string: %d", "hi");
| ^~~~~~~~~
> sense to #define __kunit_fail_current_test() instead, i.e.
>
> #define __kunit_fail_current_test(file, line, fmt, ...) \
> do { \
> kunit_set_failure(current->kunit_test); \
> kunit_err(current->kunit_test, "%s:%d: " fmt, \
> ##__VA_ARGS__); \
> } while (0)
>
> > > + if (!buffer)
> > > + return;
> > > +
> > > + va_start(args, fmt);
> > > + vsnprintf(buffer, len, fmt, args);
> > > + va_end(args);
> > > +
> > > + kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer);
>
> To get kunit_err() to work, we'd need to "static inline"
> kunit_log_append(). It's not a trivial function, but on the plus
> side it doesn't call any other exported kunit functions AFAICT.
>
> So while any of the above suggestions aren't intended to block
> Daniel's work, does the above seem reasonable for a follow-up
> series to get UBSAN working with module-based KUnit? Thanks!
Ack, so sounds like we'd want to go ahead with making it only work w/
CONFIG_KUNIT=y?
I can simplify it down into a macro since the __printf() bit isn't too
big of a deal.
And then it'd let us only depend on kunit_log_append(), making it
easier to get CONFIG_KUNIT=m working.
Thanks both for digging into this!
I saw KTR's email and was dreading having to dig into what the
smallest needed change would be.
>
> Alan
