Re: [PATCH v2 5/6] kunit: split out part of kunit_assert into a static const

[Brendan Higgins <brendanhiggins@google.com>]

On Tue, Jan 11, 2022 at 4:41 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Tue, Jan 11, 2022 at 1:35 PM Brendan Higgins
> <brendanhiggins@google.com> wrote:
> >
> > On Tue, Jan 11, 2022 at 2:42 PM Daniel Latypov <dlatypov@google.com> 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@google.com>
> > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > Reviewed-by: David Gow <davidgow@google.com>
> >
> > One question below, but other than that,
> >
> > Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> >
> > > ---
> > >  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@google.com): 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.

> >
> > >         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