| 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: <CABVgOSnEdoRJVozkn+EHJCDhqC-KVShFmniFYdWHka55_GzhMA@mail.gmail.com>
Date: Thu, 27 Jan 2022 11:34:54 +0800
From: David Gow <davidgow@...gle.com>
To: Daniel Latypov <dlatypov@...gle.com>
Cc: brendanhiggins@...gle.com, linux-kernel@...r.kernel.org,
kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
skhan@...uxfoundation.org
Subject: Re: [PATCH 1/3] kunit: remove va_format from kunit_assert
On Wed, Jan 26, 2022 at 5:00 AM Daniel Latypov <dlatypov@...gle.com> 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@...gle.com>
> ---
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@...gle.com>
-- 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
>
Powered by blists - more mailing lists