On Tue, Jan 11, 2022 at 1:43 PM Brendan Higgins
<brendanhiggins@xxxxxxxxxx> wrote:
>
> On Tue, Jan 11, 2022 at 4:41 PM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
> >
> > On Tue, Jan 11, 2022 at 1:35 PM Brendan Higgins
> > <brendanhiggins@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Jan 11, 2022 at 2:42 PM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
> > > >
> > > > This is per Linus's suggestion in [1].
> > > >
> > > > The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
> > > > kunit_assert object onto the stack. Normally we rely on compilers to
> > > > elide this, but when that doesn't work out, this blows up the stack
> > > > usage of kunit test functions.
> > > >
> > > > We can move some data off the stack by making it static.
> > > > This change introduces a new `struct kunit_loc` to hold the file and
> > > > line number and then just passing assert_type (EXPECT or ASSERT) as an
> > > > argument.
> > > >
> > > > In [1], it was suggested to also move out the format string as well, but
> > > > users could theoretically craft a format string at runtime, so we can't.
> > > >
> > > > This change leaves a copy of `assert_type` in kunit_assert for now
> > > > because cleaning up all the macros to not pass it around is a bit more
> > > > involved.
> > > >
> > > > Here's an example of the expanded code for KUNIT_FAIL():
> > > > if (__builtin_expect(!!(!(false)), 0)) {
> > > > static const struct kunit_loc loc = { .file = ... };
> > > > struct kunit_fail_assert __assertion = { .assert = { .type ... };
> > > > kunit_do_failed_assertion(test, &loc, KUNIT_EXPECTATION, &__assertion.assert, ...);
> > > > };
> > > >
> > > > [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
> > > >
> > > > Signed-off-by: Daniel Latypov <dlatypov@xxxxxxxxxx>
> > > > Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > > > Reviewed-by: David Gow <davidgow@xxxxxxxxxx>
> > >
> > > One question below, but other than that,
> > >
> > > Reviewed-by: Brendan Higgins <brendanhiggins@xxxxxxxxxx>
> > >
> > > > ---
> > > > include/kunit/assert.h | 25 ++++++++++++++++---------
> > > > include/kunit/test.h | 12 +++++++++++-
> > > > lib/kunit/assert.c | 9 +++++----
> > > > lib/kunit/test.c | 15 +++++++++------
> > > > 4 files changed, 41 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> > > > index 3da6c792496c..4f91dbdb886a 100644
> > > > --- a/include/kunit/assert.h
> > > > +++ b/include/kunit/assert.h
> > > > @@ -28,11 +28,21 @@ enum kunit_assert_type {
> > > > KUNIT_EXPECTATION,
> > > > };
> > > >
> > > > +/**
> > > > + * struct kunit_loc - Identifies the source location of a line of code.
> > > > + * @line: the line number in the file.
> > > > + * @file: the file name.
> > > > + */
> > > > +struct kunit_loc {
> > > > + int line;
> > > > + const char *file;
> > > > +};
> > > > +
> > > > +#define KUNIT_CURRENT_LOC { .file = __FILE__, .line = __LINE__ }
> > > > +
> > > > /**
> > > > * struct kunit_assert - Data for printing a failed assertion or expectation.
> > > > * @type: the type (either an expectation or an assertion) of this kunit_assert.
> > > > - * @line: the source code line number that the expectation/assertion is at.
> > > > - * @file: the file path of the source file that the expectation/assertion is in.
> > > > * @message: an optional message to provide additional context.
> > > > * @format: a function which formats the data in this kunit_assert to a string.
> > > > *
> > > > @@ -40,9 +50,7 @@ enum kunit_assert_type {
> > > > * format a string to a user reporting the failure.
> > > > */
> > > > struct kunit_assert {
> > > > - enum kunit_assert_type type;
> > > > - int line;
> > > > - const char *file;
> > > > + enum kunit_assert_type type; // TODO(dlatypov@xxxxxxxxxx): delete this
> > >
> > > Can you provide some context for this?
> >
> > This is what the commit desc is referring to.
> > We leave in the type field in this change so we can clean that up and
> > all the macros all at once.
> >
> > This TODO is addressed and removed in the next commit, so I was being
> > a bit lazy with the TODO.
> > I was hoping people could check `git blame` and find the context they
> > need, if people do somehow find their way to this intermediate commit.
> >
> > If you want, I can update the TODO message to be more fleshed out.
> > Something like
> >
> > TODO(...): delete this unused field when we've updated all the related
> > KUNIT_INIT_ASSERT macros.
> >
> > ?
>
> Yeah, I like that better.
Ack, applied this locally so now it looks like
struct kunit_assert {
+ // TODO(dlatypov@xxxxxxxxxx): delete this unused field when we've
+ // updated all the related KUNIT_INIT_ASSERT* macros.
enum kunit_assert_type type;
- int line;
- const char *file;
struct va_format message;
I'll wait to see if there's any other tweaks we want before sending out a v3.
>
> > >
> > > > struct va_format message;
> > > > void (*format)(const struct kunit_assert *assert,
> > > > struct string_stream *stream);
> > > > @@ -65,14 +73,13 @@ struct kunit_assert {
> > > > */
> > > > #define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \
> > > > .type = assert_type, \
> > > > - .file = __FILE__, \
> > > > - .line = __LINE__, \
> > > > .message = KUNIT_INIT_VA_FMT_NULL, \
> > > > .format = fmt \
> > > > }
> > > >
> > > > -void kunit_base_assert_format(const struct kunit_assert *assert,
> > > > - struct string_stream *stream);
> > > > +void kunit_assert_prologue(const struct kunit_loc *loc,
> > > > + enum kunit_assert_type type,
> > > > + struct string_stream *stream);
> > > >
> > > > void kunit_assert_print_msg(const struct kunit_assert *assert,
> > > > struct string_stream *stream);
> > > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > > index 25ea3bce6663..7b752175e614 100644
> > > > --- a/include/kunit/test.h
> > > > +++ b/include/kunit/test.h
