| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFd5g46R9L+moE+wcv1HOHPpd4NJus8oJ=JwQgPP9MWOMGE4_A@mail.gmail.com>
Date: Tue, 11 Jan 2022 16:43:29 -0500
From: Brendan Higgins <brendanhiggins@...gle.com>
To: Daniel Latypov <dlatypov@...gle.com>
Cc: davidgow@...gle.com, linux-kernel@...r.kernel.org,
kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
skhan@...uxfoundation.org, torvalds@...ux-foundation.org
Subject: Re: [PATCH v2 5/6] kunit: split out part of kunit_assert into a
static const
On Tue, Jan 11, 2022 at 4:41 PM Daniel Latypov <dlatypov@...gle.com> wrote:
>
> On Tue, Jan 11, 2022 at 1:35 PM Brendan Higgins
> <brendanhiggins@...gle.com> wrote:
> >
> > On Tue, Jan 11, 2022 at 2:42 PM Daniel Latypov <dlatypov@...gle.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@...gle.com>
> > > Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> > > Reviewed-by: David Gow <davidgow@...gle.com>
> >
> > One question below, but other than that,
> >
> > Reviewed-by: Brendan Higgins <brendanhiggins@...gle.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@...gle.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
Powered by blists - more mailing lists