On Wed, Jan 26, 2022 at 5:00 AM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote: > > The concern is that having a lot of redundant fields in kunit_assert can > blow up stack usage if the compiler doesn't optimize them away [1]. > > The comment on this field implies that it was meant to be initialized > when the expect/assert was declared, but this only happens when we run > kunit_do_failed_assertion(). > > We don't need to access it outside of that function, so move it out of > the struct and make it a local variable there. > > This change also takes the chance to reduce the number of macros by > inlining the now simplified KUNIT_INIT_ASSERT_STRUCT() macro. > > [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ > > Signed-off-by: Daniel Latypov <dlatypov@xxxxxxxxxx> > --- Looks good to me. I particularly like the removal of the KUNIT_INIT_ASSERT_STRUCT() macro. I do feel that there's not much point having a kunit_assert struct at all now that it's just one function pointer, but the indirection is probably still useful enough given that things are still changing, and function pointers are always a little ugly. Reviewed-by: David Gow <davidgow@xxxxxxxxxx> -- David > include/kunit/assert.h | 43 +++++++++++++----------------------------- > lib/kunit/assert.c | 27 +++++++++++++++----------- > lib/kunit/test.c | 12 +++++++----- > 3 files changed, 36 insertions(+), 46 deletions(-) > > diff --git a/include/kunit/assert.h b/include/kunit/assert.h > index f2b3ae5cc2de..0b3704db54b6 100644 > --- a/include/kunit/assert.h > +++ b/include/kunit/assert.h > @@ -42,44 +42,21 @@ struct kunit_loc { > > /** > * struct kunit_assert - Data for printing a failed assertion or expectation. > - * @message: an optional message to provide additional context. > * @format: a function which formats the data in this kunit_assert to a string. > * > * Represents a failed expectation/assertion. Contains all the data necessary to > * format a string to a user reporting the failure. > */ > struct kunit_assert { > - struct va_format message; > void (*format)(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream); > }; > > -/** > - * KUNIT_INIT_VA_FMT_NULL - Default initializer for struct va_format. > - * > - * Used inside a struct initialization block to initialize struct va_format to > - * default values where fmt and va are null. > - */ > -#define KUNIT_INIT_VA_FMT_NULL { .fmt = NULL, .va = NULL } > - > -/** > - * KUNIT_INIT_ASSERT_STRUCT() - Initializer for a &struct kunit_assert. > - * @fmt: The formatting function which builds a string out of this kunit_assert. > - * > - * The base initializer for a &struct kunit_assert. > - */ > -#define KUNIT_INIT_ASSERT_STRUCT(fmt) { \ > - .message = KUNIT_INIT_VA_FMT_NULL, \ > - .format = fmt \ > -} > - > 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); > - > /** > * struct kunit_fail_assert - Represents a plain fail expectation/assertion. > * @assert: The parent of this type. > @@ -91,6 +68,7 @@ struct kunit_fail_assert { > }; > > void kunit_fail_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream); > > /** > @@ -100,7 +78,7 @@ void kunit_fail_assert_format(const struct kunit_assert *assert, > * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros. > */ > #define KUNIT_INIT_FAIL_ASSERT_STRUCT { \ > - .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_fail_assert_format) \ > + .assert = { .format = kunit_fail_assert_format }, \ > } > > /** > @@ -120,6 +98,7 @@ struct kunit_unary_assert { > }; > > void kunit_unary_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream); > > /** > @@ -131,7 +110,7 @@ void kunit_unary_assert_format(const struct kunit_assert *assert, > * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros. > */ > #define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \ > - .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_unary_assert_format), \ > + .assert = { .format = kunit_unary_assert_format }, \ > .condition = cond, \ > .expected_true = expect_true \ > } > @@ -153,6 +132,7 @@ struct kunit_ptr_not_err_assert { > }; > > void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream); > > /** > @@ -165,7 +145,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, > * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros. > */ > #define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \ > - .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_ptr_not_err_assert_format), \ > + .assert = { .format = kunit_ptr_not_err_assert_format }, \ > .text = txt, \ > .value = val \ > } > @@ -194,6 +174,7 @@ struct kunit_binary_assert { > }; > > void kunit_binary_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream); > > /** > @@ -213,7 +194,7 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, > left_val, \ > right_str, \ > right_val) { \ > - .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_assert_format), \ > + .assert = { .format = kunit_binary_assert_format }, \ > .operation = op_str, \ > .left_text = left_str, \ > .left_value = left_val, \ > @@ -245,6 +226,7 @@ struct kunit_binary_ptr_assert { > }; > > void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream); > > /** > @@ -265,7 +247,7 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, > left_val, \ > right_str, \ > right_val) { \ > - .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_ptr_assert_format), \ > + .assert = { .format = kunit_binary_ptr_assert_format }, \ > .operation = op_str, \ > .left_text = left_str, \ > .left_value = left_val, \ > @@ -297,6 +279,7 @@ struct kunit_binary_str_assert { > }; > > void kunit_binary_str_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream); > > /** > @@ -316,7 +299,7 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert, > left_val, \ > right_str, \ > right_val) { \ > - .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_str_assert_format), \ > + .assert = { .format = kunit_binary_str_assert_format }, \ > .operation = op_str, \ > .left_text = left_str, \ > .left_value = left_val, \ > diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c > index 9f4492a8e24e..c9c7ee0dfafa 100644 > --- a/lib/kunit/assert.c > +++ b/lib/kunit/assert.c > @@ -30,22 +30,23 @@ void kunit_assert_prologue(const struct kunit_loc *loc, > } > EXPORT_SYMBOL_GPL(kunit_assert_prologue); > > -void kunit_assert_print_msg(const struct kunit_assert *assert, > - struct string_stream *stream) > +static void kunit_assert_print_msg(const struct va_format *message, > + struct string_stream *stream) > { > - if (assert->message.fmt) > - string_stream_add(stream, "\n%pV", &assert->message); > + if (message->fmt) > + string_stream_add(stream, "\n%pV", message); > } > -EXPORT_SYMBOL_GPL(kunit_assert_print_msg); > > void kunit_fail_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream) > { > - string_stream_add(stream, "%pV", &assert->message); > + string_stream_add(stream, "%pV", message); > } > EXPORT_SYMBOL_GPL(kunit_fail_assert_format); > > void kunit_unary_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream) > { > struct kunit_unary_assert *unary_assert; > @@ -60,11 +61,12 @@ void kunit_unary_assert_format(const struct kunit_assert *assert, > string_stream_add(stream, > KUNIT_SUBTEST_INDENT "Expected %s to be false, but is true\n", > unary_assert->condition); > - kunit_assert_print_msg(assert, stream); > + kunit_assert_print_msg(message, stream); > } > EXPORT_SYMBOL_GPL(kunit_unary_assert_format); > > void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream) > { > struct kunit_ptr_not_err_assert *ptr_assert; > @@ -82,7 +84,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, > ptr_assert->text, > PTR_ERR(ptr_assert->value)); > } > - kunit_assert_print_msg(assert, stream); > + kunit_assert_print_msg(message, stream); > } > EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format); > > @@ -110,6 +112,7 @@ static bool is_literal(struct kunit *test, const char *text, long long value, > } > > void kunit_binary_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream) > { > struct kunit_binary_assert *binary_assert; > @@ -132,11 +135,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, > string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld", > binary_assert->right_text, > binary_assert->right_value); > - kunit_assert_print_msg(assert, stream); > + kunit_assert_print_msg(message, stream); > } > EXPORT_SYMBOL_GPL(kunit_binary_assert_format); > > void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream) > { > struct kunit_binary_ptr_assert *binary_assert; > @@ -155,7 +159,7 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, > string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px", > binary_assert->right_text, > binary_assert->right_value); > - kunit_assert_print_msg(assert, stream); > + kunit_assert_print_msg(message, stream); > } > EXPORT_SYMBOL_GPL(kunit_binary_ptr_assert_format); > > @@ -176,6 +180,7 @@ static bool is_str_literal(const char *text, const char *value) > } > > void kunit_binary_str_assert_format(const struct kunit_assert *assert, > + const struct va_format *message, > struct string_stream *stream) > { > struct kunit_binary_str_assert *binary_assert; > @@ -196,6 +201,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert, > string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"", > binary_assert->right_text, > binary_assert->right_value); > - kunit_assert_print_msg(assert, stream); > + kunit_assert_print_msg(message, stream); > } > EXPORT_SYMBOL_GPL(kunit_binary_str_assert_format); > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index 7dec3248562f..3bca3bf5c15b 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -241,7 +241,8 @@ static void kunit_print_string_stream(struct kunit *test, > } > > static void kunit_fail(struct kunit *test, const struct kunit_loc *loc, > - enum kunit_assert_type type, struct kunit_assert *assert) > + enum kunit_assert_type type, struct kunit_assert *assert, > + const struct va_format *message) > { > struct string_stream *stream; > > @@ -257,7 +258,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc, > } > > kunit_assert_prologue(loc, type, stream); > - assert->format(assert, stream); > + assert->format(assert, message, stream); > > kunit_print_string_stream(test, stream); > > @@ -284,12 +285,13 @@ void kunit_do_failed_assertion(struct kunit *test, > const char *fmt, ...) > { > va_list args; > + struct va_format message; > va_start(args, fmt); > > - assert->message.fmt = fmt; > - assert->message.va = &args; > + message.fmt = fmt; > + message.va = &args; > > - kunit_fail(test, loc, type, assert); > + kunit_fail(test, loc, type, assert, &message); > > va_end(args); > > -- > 2.35.0.rc2.247.g8bbb082509-goog >
