[PATCH 0/3] kunit: further reduce stack usage of asserts

[PATCH 0/3] kunit: further reduce stack usage of asserts

[Daniel Latypov]

If the compiler doesn't optimize them away, each kunit assertion (use of
KUNIT_EXPECT_EQ, etc.) can use 88 bytes of stack space in the worst and
most common case. This has led to compiler warnings for certain configs
+ arches [1].

This series builds upon [2] which cut down kunit_assert from 48 => 24
bytes, but only reduced kunit_binary_assert (the most common one) from
88 => 48.

Now we have kunit_assert = 8 and kunit_binary_assert = 32.
The cost is we need to pass around another parameter to some functions
(struct va_format *), and we introduce a new type
(sturct kunit_binary_assert_text) for holding the textual representation
of the KUNIT_EXPECT_EQ arguments.

Note: it's possible to get kunit_assert = 0 and kunit_binary_assert =
24 by removing the `format` function pointer field from kunit_assert.
I think it's an improvement, but others might think that readability
suffers from doing so, so I'm leaving that off from this series.

Meta: this series applies on top of 5.17-rc1 + [2] and [3].
That should be available at in Shuah's kunit branch [4], but my version
of git is constantly segfaulting, so I can't try and rebase to verify.

[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
[2] https://lore.kernel.org/linux-kselftest/20220113165931.451305-6-dlatypov@google.com/
[3] https://lore.kernel.org/linux-kselftest/20220118223506.1701553-1-dlatypov@google.com/
[4] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/?h=kunit 

Daniel Latypov (3):
  kunit: remove va_format from kunit_assert
  kunit: consolidate KUNIT_INIT_BINARY_ASSERT_STRUCT macros
  kunit: factor out str constants from binary assertion structs

 include/kunit/assert.h | 152 ++++++++++++-----------------------------
 include/kunit/test.h   |  32 +++++----
 lib/kunit/assert.c     |  65 ++++++++++--------
 lib/kunit/test.c       |  12 ++--
 4 files changed, 104 insertions(+), 157 deletions(-)

-- 
2.35.0.rc2.247.g8bbb082509-goog

[PATCH 1/3] kunit: remove va_format from kunit_assert

[Daniel Latypov]

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@google.com>
---
 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

Re: [PATCH 1/3] kunit: remove va_format from kunit_assert

[David Gow]

On Wed, Jan 26, 2022 at 5:00 AM Daniel Latypov <dlatypov@google.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@google.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@google.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
>

Re: [PATCH 1/3] kunit: remove va_format from kunit_assert

[Brendan Higgins]

On Tue, Jan 25, 2022 at 4:00 PM Daniel Latypov <dlatypov@google.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@google.com>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

[PATCH 2/3] kunit: consolidate KUNIT_INIT_BINARY_ASSERT_STRUCT macros

[Daniel Latypov]

We currently have 2 other versions of KUNIT_INIT_BINARY_ASSERT_STRUCT.
The only differences are that
* the format funcition they pass is different
* the types of left_val/right_val should be different (integral,
pointer, string).

The latter doesn't actually matter since these macros are just plumbing
them along to KUNIT_ASSERTION where they will get type checked.

So combine them all into a single KUNIT_INIT_BINARY_ASSERT_STRUCT that
now also takes the format function as a parameter.

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 include/kunit/assert.h | 68 +++++++-----------------------------------
 include/kunit/test.h   | 20 +++++++------
 2 files changed, 22 insertions(+), 66 deletions(-)

diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 0b3704db54b6..649bfac9f406 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -178,23 +178,28 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
 				struct string_stream *stream);
 
 /**
- * KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
- *	&struct kunit_binary_assert.
+ * KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a binary assert like
+ *	kunit_binary_assert, kunit_binary_ptr_assert, etc.
+ *
+ * @format_func: a function which formats the assert to a string.
  * @op_str: A string representation of the comparison operator (e.g. "==").
  * @left_str: A string representation of the expression in the left slot.
  * @left_val: The actual evaluated value of the expression in the left slot.
  * @right_str: A string representation of the expression in the right slot.
  * @right_val: The actual evaluated value of the expression in the right slot.
  *
- * Initializes a &struct kunit_binary_assert. Intended to be used in
- * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
+ * Initializes a binary assert like kunit_binary_assert,
+ * kunit_binary_ptr_assert, etc. This relies on these structs having the same
+ * fields but with different types for left_val/right_val.
+ * This is ultimately used by binary assertion macros like KUNIT_EXPECT_EQ, etc.
  */
-#define KUNIT_INIT_BINARY_ASSERT_STRUCT(op_str,				       \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,			       \
+					op_str,				       \
 					left_str,			       \
 					left_val,			       \
 					right_str,			       \
 					right_val) {			       \
-	.assert = { .format = kunit_binary_assert_format },		       \
+	.assert = { .format = format_func },				       \
 	.operation = op_str,						       \
 	.left_text = left_str,						       \
 	.left_value = left_val,						       \
@@ -229,32 +234,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
 				    const struct va_format *message,
 				    struct string_stream *stream);
 
-/**
- * KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT() - Initializes a
- *	&struct kunit_binary_ptr_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
- * @op_str: A string representation of the comparison operator (e.g. "==").
- * @left_str: A string representation of the expression in the left slot.
- * @left_val: The actual evaluated value of the expression in the left slot.
- * @right_str: A string representation of the expression in the right slot.
- * @right_val: The actual evaluated value of the expression in the right slot.
- *
- * Initializes a &struct kunit_binary_ptr_assert. Intended to be used in
- * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
- */
-#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(op_str,			       \
-					    left_str,			       \
-					    left_val,			       \
-					    right_str,			       \
-					    right_val) {		       \
-	.assert = { .format = kunit_binary_ptr_assert_format },		       \
-	.operation = op_str,						       \
-	.left_text = left_str,						       \
-	.left_value = left_val,						       \
-	.right_text = right_str,					       \
-	.right_value = right_val					       \
-}
-
 /**
  * struct kunit_binary_str_assert - An expectation/assertion that compares two
  *	string values (for example, KUNIT_EXPECT_STREQ(test, foo, "bar")).
@@ -282,29 +261,4 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
 				    const struct va_format *message,
 				    struct string_stream *stream);
 
-/**
- * KUNIT_INIT_BINARY_STR_ASSERT_STRUCT() - Initializes a
- *	&struct kunit_binary_str_assert.
- * @op_str: A string representation of the comparison operator (e.g. "==").
- * @left_str: A string representation of the expression in the left slot.
- * @left_val: The actual evaluated value of the expression in the left slot.
- * @right_str: A string representation of the expression in the right slot.
- * @right_val: The actual evaluated value of the expression in the right slot.
- *
- * Initializes a &struct kunit_binary_str_assert. Intended to be used in
- * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
- */
-#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(op_str,			       \
-					    left_str,			       \
-					    left_val,			       \
-					    right_str,			       \
-					    right_val) {		       \
-	.assert = { .format = kunit_binary_str_assert_format },		       \
-	.operation = op_str,						       \
-	.left_text = left_str,						       \
-	.left_value = left_val,						       \
-	.right_text = right_str,					       \
-	.right_value = right_val					       \
-}
-
 #endif /*  _KUNIT_ASSERT_H */
diff --git a/include/kunit/test.h b/include/kunit/test.h
index bf82c313223b..a93dfb8ff393 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -864,7 +864,7 @@ void kunit_do_failed_assertion(struct kunit *test,
  */
 #define KUNIT_BASE_BINARY_ASSERTION(test,				       \
 				    assert_class,			       \
-				    ASSERT_CLASS_INIT,			       \
+				    format_func,			       \
 				    assert_type,			       \
 				    left,				       \
 				    op,					       \
@@ -879,11 +879,12 @@ do {									       \
 			assert_type,					       \
 			__left op __right,				       \
 			assert_class,					       \
-			ASSERT_CLASS_INIT(#op,				       \
-					  #left,			       \
-					  __left,			       \
-					  #right,			       \
-					  __right),			       \
+			KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,	       \
+							#op,		       \
+							#left,		       \
+							__left,		       \
+							#right,		       \
+							__right),	       \
 			fmt,						       \
 			##__VA_ARGS__);					       \
 } while (0)
@@ -897,7 +898,7 @@ do {									       \
 				    ...)				       \
 	KUNIT_BASE_BINARY_ASSERTION(test,				       \
 				    kunit_binary_assert,		       \
-				    KUNIT_INIT_BINARY_ASSERT_STRUCT,	       \
+				    kunit_binary_assert_format,		       \
 				    assert_type,			       \
 				    left, op, right,			       \
 				    fmt,				       \
@@ -912,7 +913,7 @@ do {									       \
 				    ...)				       \
 	KUNIT_BASE_BINARY_ASSERTION(test,				       \
 				    kunit_binary_ptr_assert,		       \
-				    KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT,       \
+				    kunit_binary_ptr_assert_format,	       \
 				    assert_type,			       \
 				    left, op, right,			       \
 				    fmt,				       \
@@ -933,7 +934,8 @@ do {									       \
 			assert_type,					       \
 			strcmp(__left, __right) op 0,			       \
 			kunit_binary_str_assert,			       \
-			KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(#op,	       \
+			KUNIT_INIT_BINARY_ASSERT_STRUCT(kunit_binary_str_assert_format,\
+							#op,		       \
 							#left,		       \
 							__left,		       \
 							#right,		       \
-- 
2.35.0.rc2.247.g8bbb082509-goog

Re: [PATCH 2/3] kunit: consolidate KUNIT_INIT_BINARY_ASSERT_STRUCT macros

[David Gow]

On Wed, Jan 26, 2022 at 5:00 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> We currently have 2 other versions of KUNIT_INIT_BINARY_ASSERT_STRUCT.
> The only differences are that
> * the format funcition they pass is different
Minor nit: s/funcition/function/
> * the types of left_val/right_val should be different (integral,
> pointer, string).
>
> The latter doesn't actually matter since these macros are just plumbing
> them along to KUNIT_ASSERTION where they will get type checked.
>
> So combine them all into a single KUNIT_INIT_BINARY_ASSERT_STRUCT that
> now also takes the format function as a parameter.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

Makes sense to me.

One minor spelling nit: probably not worth a whole new version over,
but if v2 ever happens, worth fixing at the same time...

-- David


>  include/kunit/assert.h | 68 +++++++-----------------------------------
>  include/kunit/test.h   | 20 +++++++------
>  2 files changed, 22 insertions(+), 66 deletions(-)
>
> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> index 0b3704db54b6..649bfac9f406 100644
> --- a/include/kunit/assert.h
> +++ b/include/kunit/assert.h
> @@ -178,23 +178,28 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
>                                 struct string_stream *stream);
>
>  /**
> - * KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
> - *     &struct kunit_binary_assert.
> + * KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a binary assert like
> + *     kunit_binary_assert, kunit_binary_ptr_assert, etc.
> + *
> + * @format_func: a function which formats the assert to a string.
>   * @op_str: A string representation of the comparison operator (e.g. "==").
>   * @left_str: A string representation of the expression in the left slot.
>   * @left_val: The actual evaluated value of the expression in the left slot.
>   * @right_str: A string representation of the expression in the right slot.
>   * @right_val: The actual evaluated value of the expression in the right slot.
>   *
> - * Initializes a &struct kunit_binary_assert. Intended to be used in
> - * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
> + * Initializes a binary assert like kunit_binary_assert,
> + * kunit_binary_ptr_assert, etc. This relies on these structs having the same
> + * fields but with different types for left_val/right_val.
> + * This is ultimately used by binary assertion macros like KUNIT_EXPECT_EQ, etc.
>   */
> -#define KUNIT_INIT_BINARY_ASSERT_STRUCT(op_str,                                       \
> +#define KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,                          \
> +                                       op_str,                                \
>                                         left_str,                              \
>                                         left_val,                              \
>                                         right_str,                             \
>                                         right_val) {                           \
> -       .assert = { .format = kunit_binary_assert_format },                    \
> +       .assert = { .format = format_func },                                   \
>         .operation = op_str,                                                   \
>         .left_text = left_str,                                                 \
>         .left_value = left_val,                                                \
> @@ -229,32 +234,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
>                                     const struct va_format *message,
>                                     struct string_stream *stream);
>
> -/**
> - * KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT() - Initializes a
> - *     &struct kunit_binary_ptr_assert.
> - * @type: The type (assertion or expectation) of this kunit_assert.
> - * @op_str: A string representation of the comparison operator (e.g. "==").
> - * @left_str: A string representation of the expression in the left slot.
> - * @left_val: The actual evaluated value of the expression in the left slot.
> - * @right_str: A string representation of the expression in the right slot.
> - * @right_val: The actual evaluated value of the expression in the right slot.
> - *
> - * Initializes a &struct kunit_binary_ptr_assert. Intended to be used in
> - * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
> - */
> -#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(op_str,                           \
> -                                           left_str,                          \
> -                                           left_val,                          \
> -                                           right_str,                         \
> -                                           right_val) {                       \
> -       .assert = { .format = kunit_binary_ptr_assert_format },                \
> -       .operation = op_str,                                                   \
> -       .left_text = left_str,                                                 \
> -       .left_value = left_val,                                                \
> -       .right_text = right_str,                                               \
> -       .right_value = right_val                                               \
> -}
> -
>  /**
>   * struct kunit_binary_str_assert - An expectation/assertion that compares two
>   *     string values (for example, KUNIT_EXPECT_STREQ(test, foo, "bar")).
> @@ -282,29 +261,4 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
>                                     const struct va_format *message,
>                                     struct string_stream *stream);
>
> -/**
> - * KUNIT_INIT_BINARY_STR_ASSERT_STRUCT() - Initializes a
> - *     &struct kunit_binary_str_assert.
> - * @op_str: A string representation of the comparison operator (e.g. "==").
> - * @left_str: A string representation of the expression in the left slot.
> - * @left_val: The actual evaluated value of the expression in the left slot.
> - * @right_str: A string representation of the expression in the right slot.
> - * @right_val: The actual evaluated value of the expression in the right slot.
> - *
> - * Initializes a &struct kunit_binary_str_assert. Intended to be used in
> - * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
> - */
> -#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(op_str,                           \
> -                                           left_str,                          \
> -                                           left_val,                          \
> -                                           right_str,                         \
> -                                           right_val) {                       \
> -       .assert = { .format = kunit_binary_str_assert_format },                \
> -       .operation = op_str,                                                   \
> -       .left_text = left_str,                                                 \
> -       .left_value = left_val,                                                \
> -       .right_text = right_str,                                               \
> -       .right_value = right_val                                               \
> -}
> -
>  #endif /*  _KUNIT_ASSERT_H */
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index bf82c313223b..a93dfb8ff393 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -864,7 +864,7 @@ void kunit_do_failed_assertion(struct kunit *test,
>   */
>  #define KUNIT_BASE_BINARY_ASSERTION(test,                                     \
>                                     assert_class,                              \
> -                                   ASSERT_CLASS_INIT,                         \
> +                                   format_func,                               \
>                                     assert_type,                               \
>                                     left,                                      \
>                                     op,                                        \
> @@ -879,11 +879,12 @@ do {                                                                             \
>                         assert_type,                                           \
>                         __left op __right,                                     \
>                         assert_class,                                          \
> -                       ASSERT_CLASS_INIT(#op,                                 \
> -                                         #left,                               \
> -                                         __left,                              \
> -                                         #right,                              \
> -                                         __right),                            \
> +                       KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,           \
> +                                                       #op,                   \
> +                                                       #left,                 \
> +                                                       __left,                \
> +                                                       #right,                \
> +                                                       __right),              \
>                         fmt,                                                   \
>                         ##__VA_ARGS__);                                        \
>  } while (0)
> @@ -897,7 +898,7 @@ do {                                                                               \
>                                     ...)                                       \
>         KUNIT_BASE_BINARY_ASSERTION(test,                                      \
>                                     kunit_binary_assert,                       \
> -                                   KUNIT_INIT_BINARY_ASSERT_STRUCT,           \
> +                                   kunit_binary_assert_format,                \
>                                     assert_type,                               \
>                                     left, op, right,                           \
>                                     fmt,                                       \
> @@ -912,7 +913,7 @@ do {                                                                               \
>                                     ...)                                       \
>         KUNIT_BASE_BINARY_ASSERTION(test,                                      \
>                                     kunit_binary_ptr_assert,                   \
> -                                   KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT,       \
> +                                   kunit_binary_ptr_assert_format,            \
>                                     assert_type,                               \
>                                     left, op, right,                           \
>                                     fmt,                                       \
> @@ -933,7 +934,8 @@ do {                                                                               \
>                         assert_type,                                           \
>                         strcmp(__left, __right) op 0,                          \
>                         kunit_binary_str_assert,                               \
> -                       KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(#op,               \
> +                       KUNIT_INIT_BINARY_ASSERT_STRUCT(kunit_binary_str_assert_format,\
> +                                                       #op,                   \
>                                                         #left,                 \
>                                                         __left,                \
>                                                         #right,                \
> --
> 2.35.0.rc2.247.g8bbb082509-goog
>

Re: [PATCH 2/3] kunit: consolidate KUNIT_INIT_BINARY_ASSERT_STRUCT macros

[David Gow]

On Thu, Jan 27, 2022 at 11:36 AM David Gow <davidgow@google.com> wrote:
>
> On Wed, Jan 26, 2022 at 5:00 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > We currently have 2 other versions of KUNIT_INIT_BINARY_ASSERT_STRUCT.
> > The only differences are that
> > * the format funcition they pass is different
> Minor nit: s/funcition/function/
> > * the types of left_val/right_val should be different (integral,
> > pointer, string).
> >
> > The latter doesn't actually matter since these macros are just plumbing
> > them along to KUNIT_ASSERTION where they will get type checked.
> >
> > So combine them all into a single KUNIT_INIT_BINARY_ASSERT_STRUCT that
> > now also takes the format function as a parameter.
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> Makes sense to me.
>
> One minor spelling nit: probably not worth a whole new version over,
> but if v2 ever happens, worth fixing at the same time...
>

Oops, forgot to add the Reviewed-by!

Reviewed-by: David Gow <davidgow@google.com>

> -- David
>

Re: [PATCH 2/3] kunit: consolidate KUNIT_INIT_BINARY_ASSERT_STRUCT macros

[Brendan Higgins]

On Tue, Jan 25, 2022 at 4:00 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> We currently have 2 other versions of KUNIT_INIT_BINARY_ASSERT_STRUCT.
> The only differences are that
> * the format funcition they pass is different
> * the types of left_val/right_val should be different (integral,
> pointer, string).
>
> The latter doesn't actually matter since these macros are just plumbing
> them along to KUNIT_ASSERTION where they will get type checked.
>
> So combine them all into a single KUNIT_INIT_BINARY_ASSERT_STRUCT that
> now also takes the format function as a parameter.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

[PATCH 3/3] kunit: factor out str constants from binary assertion structs

[Daniel Latypov]

If the compiler doesn't optimize them away, each kunit assertion (use of
KUNIT_EXPECT_EQ, etc.) can use 88 bytes of stack space in the worst and
most common case. This has led to compiler warnings and a suggestion
from Linus to move data from the structs into static const's where
possible [1].

This builds upon [2] which did so for the base struct kunit_assert type.
That only reduced sizeof(struct kunit_binary_assert) from 88 to 64.

Given these are by far the most commonly used asserts, this patch
factors out the textual representations of the operands and comparator
into another static const, saving 16 more bytes.

In detail, KUNIT_EXPECT_EQ(test, 2 + 2, 5) yields the following struct
  (struct kunit_binary_assert) {
    .assert = <struct kunit_assert>,
    .operation = "==",
    .left_text = "2 + 2",
    .left_value = 4,
    .right_text = "5",
    .right_value = 5,
  }
After this change
  static const struct kunit_binary_assert_text __text = {
    .operation = "==",
    .left_text = "2 + 2",
    .right_text = "5",
  };
  (struct kunit_binary_assert) {
    .assert = <struct kunit_assert>,
    .text = &__text,
    .left_value = 4,
    .right_value = 5,
  }

This also DRYs the code a bit more since these str fields were repeated
for the string and pointer versions of kunit_binary_assert.

Note: we could name the kunit_binary_assert_text fields left/right
instead of left_text/right_text. But that would require changing the
macros a bit since they have args called "left" and "right" which would
be substituted in `.left = #left` as `.2 + 2 = \"2 + 2\"`.

[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
[2] https://lore.kernel.org/linux-kselftest/20220113165931.451305-6-dlatypov@google.com/

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 include/kunit/assert.h | 49 +++++++++++++++++++-----------------------
 include/kunit/test.h   | 20 +++++++++++------
 lib/kunit/assert.c     | 38 ++++++++++++++++----------------
 3 files changed, 54 insertions(+), 53 deletions(-)

diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 649bfac9f406..4b52e12c2ae8 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -150,14 +150,25 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
 	.value = val							       \
 }
 
+/**
+ * struct kunit_binary_assert_text - holds strings for &struct
+ *	kunit_binary_assert and friends to try and make the structs smaller.
+ * @operation: A string representation of the comparison operator (e.g. "==").
+ * @left_text: A string representation of the left expression (e.g. "2+2").
+ * @right_text: A string representation of the right expression (e.g. "2+2").
+ */
+struct kunit_binary_assert_text {
+	const char *operation;
+	const char *left_text;
+	const char *right_text;
+};
+
 /**
  * struct kunit_binary_assert - An expectation/assertion that compares two
  *	non-pointer values (for example, KUNIT_EXPECT_EQ(test, 1 + 1, 2)).
  * @assert: The parent of this type.
- * @operation: A string representation of the comparison operator (e.g. "==").
- * @left_text: A string representation of the expression in the left slot.
+ * @text: Holds the textual representations of the operands and op (e.g.  "==").
  * @left_value: The actual evaluated value of the expression in the left slot.
- * @right_text: A string representation of the expression in the right slot.
  * @right_value: The actual evaluated value of the expression in the right slot.
  *
  * Represents an expectation/assertion that compares two non-pointer values. For
@@ -166,10 +177,8 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
  */
 struct kunit_binary_assert {
 	struct kunit_assert assert;
-	const char *operation;
-	const char *left_text;
+	const struct kunit_binary_assert_text *text;
 	long long left_value;
-	const char *right_text;
 	long long right_value;
 };
 
@@ -182,10 +191,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
  *	kunit_binary_assert, kunit_binary_ptr_assert, etc.
  *
  * @format_func: a function which formats the assert to a string.
- * @op_str: A string representation of the comparison operator (e.g. "==").
- * @left_str: A string representation of the expression in the left slot.
+ * @text_: Pointer to a kunit_binary_assert_text.
  * @left_val: The actual evaluated value of the expression in the left slot.
- * @right_str: A string representation of the expression in the right slot.
  * @right_val: The actual evaluated value of the expression in the right slot.
  *
  * Initializes a binary assert like kunit_binary_assert,
@@ -194,16 +201,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
  * This is ultimately used by binary assertion macros like KUNIT_EXPECT_EQ, etc.
  */
 #define KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,			       \
-					op_str,				       \
-					left_str,			       \
+					text_,				       \
 					left_val,			       \
-					right_str,			       \
 					right_val) {			       \
 	.assert = { .format = format_func },				       \
-	.operation = op_str,						       \
-	.left_text = left_str,						       \
+	.text = text_,							       \
 	.left_value = left_val,						       \
-	.right_text = right_str,					       \
 	.right_value = right_val					       \
 }
 
@@ -211,10 +214,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
  * struct kunit_binary_ptr_assert - An expectation/assertion that compares two
  *	pointer values (for example, KUNIT_EXPECT_PTR_EQ(test, foo, bar)).
  * @assert: The parent of this type.
- * @operation: A string representation of the comparison operator (e.g. "==").
- * @left_text: A string representation of the expression in the left slot.
+ * @text: Holds the textual representations of the operands and op (e.g.  "==").
  * @left_value: The actual evaluated value of the expression in the left slot.
- * @right_text: A string representation of the expression in the right slot.
  * @right_value: The actual evaluated value of the expression in the right slot.
  *
  * Represents an expectation/assertion that compares two pointer values. For
@@ -223,10 +224,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
  */
 struct kunit_binary_ptr_assert {
 	struct kunit_assert assert;
-	const char *operation;
-	const char *left_text;
+	const struct kunit_binary_assert_text *text;
 	const void *left_value;
-	const char *right_text;
 	const void *right_value;
 };
 
@@ -238,10 +237,8 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
  * struct kunit_binary_str_assert - An expectation/assertion that compares two
  *	string values (for example, KUNIT_EXPECT_STREQ(test, foo, "bar")).
  * @assert: The parent of this type.
- * @operation: A string representation of the comparison operator (e.g. "==").
- * @left_text: A string representation of the expression in the left slot.
+ * @text: Holds the textual representations of the operands and comparator.
  * @left_value: The actual evaluated value of the expression in the left slot.
- * @right_text: A string representation of the expression in the right slot.
  * @right_value: The actual evaluated value of the expression in the right slot.
  *
  * Represents an expectation/assertion that compares two string values. For
@@ -250,10 +247,8 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
  */
 struct kunit_binary_str_assert {
 	struct kunit_assert assert;
-	const char *operation;
-	const char *left_text;
+	const struct kunit_binary_assert_text *text;
 	const char *left_value;
-	const char *right_text;
 	const char *right_value;
 };
 
diff --git a/include/kunit/test.h b/include/kunit/test.h
index a93dfb8ff393..088ff394ae94 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -874,16 +874,19 @@ void kunit_do_failed_assertion(struct kunit *test,
 do {									       \
 	typeof(left) __left = (left);					       \
 	typeof(right) __right = (right);				       \
+	static const struct kunit_binary_assert_text __text = {		       \
+		.operation = #op,					       \
+		.left_text = #left,					       \
+		.right_text = #right,					       \
+	};								       \
 									       \
 	KUNIT_ASSERTION(test,						       \
 			assert_type,					       \
 			__left op __right,				       \
 			assert_class,					       \
 			KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,	       \
-							#op,		       \
-							#left,		       \
+							&__text,	       \
 							__left,		       \
-							#right,		       \
 							__right),	       \
 			fmt,						       \
 			##__VA_ARGS__);					       \
@@ -928,17 +931,20 @@ do {									       \
 				   ...)					       \
 do {									       \
 	const char *__left = (left);					       \
-	const char *__right = (right);				       \
+	const char *__right = (right);					       \
+	static const struct kunit_binary_assert_text __text = {		       \
+		.operation = #op,					       \
+		.left_text = #left,					       \
+		.right_text = #right,					       \
+	};								       \
 									       \
 	KUNIT_ASSERTION(test,						       \
 			assert_type,					       \
 			strcmp(__left, __right) op 0,			       \
 			kunit_binary_str_assert,			       \
 			KUNIT_INIT_BINARY_ASSERT_STRUCT(kunit_binary_str_assert_format,\
-							#op,		       \
-							#left,		       \
+							&__text,	       \
 							__left,		       \
-							#right,		       \
 							__right),	       \
 			fmt,						       \
 			##__VA_ARGS__);					       \
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
index c9c7ee0dfafa..d00d6d181ee8 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -122,18 +122,18 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
 
 	string_stream_add(stream,
 			  KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
-			  binary_assert->left_text,
-			  binary_assert->operation,
-			  binary_assert->right_text);
-	if (!is_literal(stream->test, binary_assert->left_text,
+			  binary_assert->text->left_text,
+			  binary_assert->text->operation,
+			  binary_assert->text->right_text);
+	if (!is_literal(stream->test, binary_assert->text->left_text,
 			binary_assert->left_value, stream->gfp))
 		string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld\n",
-				  binary_assert->left_text,
+				  binary_assert->text->left_text,
 				  binary_assert->left_value);
-	if (!is_literal(stream->test, binary_assert->right_text,
+	if (!is_literal(stream->test, binary_assert->text->right_text,
 			binary_assert->right_value, stream->gfp))
 		string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld",
-				  binary_assert->right_text,
+				  binary_assert->text->right_text,
 				  binary_assert->right_value);
 	kunit_assert_print_msg(message, stream);
 }
@@ -150,14 +150,14 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
 
 	string_stream_add(stream,
 			  KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
-			  binary_assert->left_text,
-			  binary_assert->operation,
-			  binary_assert->right_text);
+			  binary_assert->text->left_text,
+			  binary_assert->text->operation,
+			  binary_assert->text->right_text);
 	string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px\n",
-			  binary_assert->left_text,
+			  binary_assert->text->left_text,
 			  binary_assert->left_value);
 	string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px",
-			  binary_assert->right_text,
+			  binary_assert->text->right_text,
 			  binary_assert->right_value);
 	kunit_assert_print_msg(message, stream);
 }
@@ -190,16 +190,16 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
 
 	string_stream_add(stream,
 			  KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
-			  binary_assert->left_text,
-			  binary_assert->operation,
-			  binary_assert->right_text);
-	if (!is_str_literal(binary_assert->left_text, binary_assert->left_value))
+			  binary_assert->text->left_text,
+			  binary_assert->text->operation,
+			  binary_assert->text->right_text);
+	if (!is_str_literal(binary_assert->text->left_text, binary_assert->left_value))
 		string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"\n",
-				  binary_assert->left_text,
+				  binary_assert->text->left_text,
 				  binary_assert->left_value);
-	if (!is_str_literal(binary_assert->right_text, binary_assert->right_value))
+	if (!is_str_literal(binary_assert->text->right_text, binary_assert->right_value))
 		string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"",
-				  binary_assert->right_text,
+				  binary_assert->text->right_text,
 				  binary_assert->right_value);
 	kunit_assert_print_msg(message, stream);
 }
-- 
2.35.0.rc2.247.g8bbb082509-goog

Re: [PATCH 3/3] kunit: factor out str constants from binary assertion structs

[David Gow]

On Wed, Jan 26, 2022 at 5:00 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> If the compiler doesn't optimize them away, each kunit assertion (use of
> KUNIT_EXPECT_EQ, etc.) can use 88 bytes of stack space in the worst and
> most common case. This has led to compiler warnings and a suggestion
> from Linus to move data from the structs into static const's where
> possible [1].
>
> This builds upon [2] which did so for the base struct kunit_assert type.
> That only reduced sizeof(struct kunit_binary_assert) from 88 to 64.
>
> Given these are by far the most commonly used asserts, this patch
> factors out the textual representations of the operands and comparator
> into another static const, saving 16 more bytes.
>
> In detail, KUNIT_EXPECT_EQ(test, 2 + 2, 5) yields the following struct
>   (struct kunit_binary_assert) {
>     .assert = <struct kunit_assert>,
>     .operation = "==",
>     .left_text = "2 + 2",
>     .left_value = 4,
>     .right_text = "5",
>     .right_value = 5,
>   }
> After this change
>   static const struct kunit_binary_assert_text __text = {
>     .operation = "==",
>     .left_text = "2 + 2",
>     .right_text = "5",
>   };
>   (struct kunit_binary_assert) {
>     .assert = <struct kunit_assert>,
>     .text = &__text,
>     .left_value = 4,
>     .right_value = 5,
>   }
>
> This also DRYs the code a bit more since these str fields were repeated
> for the string and pointer versions of kunit_binary_assert.
>
> Note: we could name the kunit_binary_assert_text fields left/right
> instead of left_text/right_text. But that would require changing the
> macros a bit since they have args called "left" and "right" which would
> be substituted in `.left = #left` as `.2 + 2 = \"2 + 2\"`.
>
> [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
> [2] https://lore.kernel.org/linux-kselftest/20220113165931.451305-6-dlatypov@google.com/
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

This definitely _feels_ like it's adding a bit more complexity than
would be ideal by splitting this out into a separate function, but I
do agree that it's worth it.

I think left_text / right_text are good enough names, too: I wouldn't
bother trying to make them .left/.right.


Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

>  include/kunit/assert.h | 49 +++++++++++++++++++-----------------------
>  include/kunit/test.h   | 20 +++++++++++------
>  lib/kunit/assert.c     | 38 ++++++++++++++++----------------
>  3 files changed, 54 insertions(+), 53 deletions(-)
>
> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> index 649bfac9f406..4b52e12c2ae8 100644
> --- a/include/kunit/assert.h
> +++ b/include/kunit/assert.h
> @@ -150,14 +150,25 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
>         .value = val                                                           \
>  }
>
> +/**
> + * struct kunit_binary_assert_text - holds strings for &struct
> + *     kunit_binary_assert and friends to try and make the structs smaller.
> + * @operation: A string representation of the comparison operator (e.g. "==").
> + * @left_text: A string representation of the left expression (e.g. "2+2").
> + * @right_text: A string representation of the right expression (e.g. "2+2").
> + */
> +struct kunit_binary_assert_text {
> +       const char *operation;
> +       const char *left_text;
> +       const char *right_text;
> +};
> +
>  /**
>   * struct kunit_binary_assert - An expectation/assertion that compares two
>   *     non-pointer values (for example, KUNIT_EXPECT_EQ(test, 1 + 1, 2)).
>   * @assert: The parent of this type.
> - * @operation: A string representation of the comparison operator (e.g. "==").
> - * @left_text: A string representation of the expression in the left slot.
> + * @text: Holds the textual representations of the operands and op (e.g.  "==").
>   * @left_value: The actual evaluated value of the expression in the left slot.
> - * @right_text: A string representation of the expression in the right slot.
>   * @right_value: The actual evaluated value of the expression in the right slot.
>   *
>   * Represents an expectation/assertion that compares two non-pointer values. For
> @@ -166,10 +177,8 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
>   */
>  struct kunit_binary_assert {
>         struct kunit_assert assert;
> -       const char *operation;
> -       const char *left_text;
> +       const struct kunit_binary_assert_text *text;
>         long long left_value;
> -       const char *right_text;
>         long long right_value;
>  };
>
> @@ -182,10 +191,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
>   *     kunit_binary_assert, kunit_binary_ptr_assert, etc.
>   *
>   * @format_func: a function which formats the assert to a string.
> - * @op_str: A string representation of the comparison operator (e.g. "==").
> - * @left_str: A string representation of the expression in the left slot.
> + * @text_: Pointer to a kunit_binary_assert_text.
>   * @left_val: The actual evaluated value of the expression in the left slot.
> - * @right_str: A string representation of the expression in the right slot.
>   * @right_val: The actual evaluated value of the expression in the right slot.
>   *
>   * Initializes a binary assert like kunit_binary_assert,
> @@ -194,16 +201,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
>   * This is ultimately used by binary assertion macros like KUNIT_EXPECT_EQ, etc.
>   */
>  #define KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,                          \
> -                                       op_str,                                \
> -                                       left_str,                              \
> +                                       text_,                                 \
>                                         left_val,                              \
> -                                       right_str,                             \
>                                         right_val) {                           \
>         .assert = { .format = format_func },                                   \
> -       .operation = op_str,                                                   \
> -       .left_text = left_str,                                                 \
> +       .text = text_,                                                         \
>         .left_value = left_val,                                                \
> -       .right_text = right_str,                                               \
>         .right_value = right_val                                               \
>  }
>
> @@ -211,10 +214,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
>   * struct kunit_binary_ptr_assert - An expectation/assertion that compares two
>   *     pointer values (for example, KUNIT_EXPECT_PTR_EQ(test, foo, bar)).
>   * @assert: The parent of this type.
> - * @operation: A string representation of the comparison operator (e.g. "==").
> - * @left_text: A string representation of the expression in the left slot.
> + * @text: Holds the textual representations of the operands and op (e.g.  "==").
>   * @left_value: The actual evaluated value of the expression in the left slot.
> - * @right_text: A string representation of the expression in the right slot.
>   * @right_value: The actual evaluated value of the expression in the right slot.
>   *
>   * Represents an expectation/assertion that compares two pointer values. For
> @@ -223,10 +224,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
>   */
>  struct kunit_binary_ptr_assert {
>         struct kunit_assert assert;
> -       const char *operation;
> -       const char *left_text;
> +       const struct kunit_binary_assert_text *text;
>         const void *left_value;
> -       const char *right_text;
>         const void *right_value;
>  };
>
> @@ -238,10 +237,8 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
>   * struct kunit_binary_str_assert - An expectation/assertion that compares two
>   *     string values (for example, KUNIT_EXPECT_STREQ(test, foo, "bar")).
>   * @assert: The parent of this type.
> - * @operation: A string representation of the comparison operator (e.g. "==").
> - * @left_text: A string representation of the expression in the left slot.
> + * @text: Holds the textual representations of the operands and comparator.
>   * @left_value: The actual evaluated value of the expression in the left slot.
> - * @right_text: A string representation of the expression in the right slot.
>   * @right_value: The actual evaluated value of the expression in the right slot.
>   *
>   * Represents an expectation/assertion that compares two string values. For
> @@ -250,10 +247,8 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
>   */
>  struct kunit_binary_str_assert {
>         struct kunit_assert assert;
> -       const char *operation;
> -       const char *left_text;
> +       const struct kunit_binary_assert_text *text;
>         const char *left_value;
> -       const char *right_text;
>         const char *right_value;
>  };
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index a93dfb8ff393..088ff394ae94 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -874,16 +874,19 @@ void kunit_do_failed_assertion(struct kunit *test,
>  do {                                                                          \
>         typeof(left) __left = (left);                                          \
>         typeof(right) __right = (right);                                       \
> +       static const struct kunit_binary_assert_text __text = {                \
> +               .operation = #op,                                              \
> +               .left_text = #left,                                            \
> +               .right_text = #right,                                          \
> +       };                                                                     \
>                                                                                \
>         KUNIT_ASSERTION(test,                                                  \
>                         assert_type,                                           \
>                         __left op __right,                                     \
>                         assert_class,                                          \
>                         KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,           \
> -                                                       #op,                   \
> -                                                       #left,                 \
> +                                                       &__text,               \
>                                                         __left,                \
> -                                                       #right,                \
>                                                         __right),              \
>                         fmt,                                                   \
>                         ##__VA_ARGS__);                                        \
> @@ -928,17 +931,20 @@ do {                                                                             \
>                                    ...)                                        \
>  do {                                                                          \
>         const char *__left = (left);                                           \
> -       const char *__right = (right);                                 \
> +       const char *__right = (right);                                         \
> +       static const struct kunit_binary_assert_text __text = {                \
> +               .operation = #op,                                              \
> +               .left_text = #left,                                            \
> +               .right_text = #right,                                          \
> +       };                                                                     \
>                                                                                \
>         KUNIT_ASSERTION(test,                                                  \
>                         assert_type,                                           \
>                         strcmp(__left, __right) op 0,                          \
>                         kunit_binary_str_assert,                               \
>                         KUNIT_INIT_BINARY_ASSERT_STRUCT(kunit_binary_str_assert_format,\
> -                                                       #op,                   \
> -                                                       #left,                 \
> +                                                       &__text,               \
>                                                         __left,                \
> -                                                       #right,                \
>                                                         __right),              \
>                         fmt,                                                   \
>                         ##__VA_ARGS__);                                        \
> diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> index c9c7ee0dfafa..d00d6d181ee8 100644
> --- a/lib/kunit/assert.c
> +++ b/lib/kunit/assert.c
> @@ -122,18 +122,18 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
>
>         string_stream_add(stream,
>                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> -                         binary_assert->left_text,
> -                         binary_assert->operation,
> -                         binary_assert->right_text);
> -       if (!is_literal(stream->test, binary_assert->left_text,
> +                         binary_assert->text->left_text,
> +                         binary_assert->text->operation,
> +                         binary_assert->text->right_text);
> +       if (!is_literal(stream->test, binary_assert->text->left_text,
>                         binary_assert->left_value, stream->gfp))
>                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld\n",
> -                                 binary_assert->left_text,
> +                                 binary_assert->text->left_text,
>                                   binary_assert->left_value);
> -       if (!is_literal(stream->test, binary_assert->right_text,
> +       if (!is_literal(stream->test, binary_assert->text->right_text,
>                         binary_assert->right_value, stream->gfp))
>                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld",
> -                                 binary_assert->right_text,
> +                                 binary_assert->text->right_text,
>                                   binary_assert->right_value);
>         kunit_assert_print_msg(message, stream);
>  }
> @@ -150,14 +150,14 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
>
>         string_stream_add(stream,
>                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> -                         binary_assert->left_text,
> -                         binary_assert->operation,
> -                         binary_assert->right_text);
> +                         binary_assert->text->left_text,
> +                         binary_assert->text->operation,
> +                         binary_assert->text->right_text);
>         string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px\n",
> -                         binary_assert->left_text,
> +                         binary_assert->text->left_text,
>                           binary_assert->left_value);
>         string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px",
> -                         binary_assert->right_text,
> +                         binary_assert->text->right_text,
>                           binary_assert->right_value);
>         kunit_assert_print_msg(message, stream);
>  }
> @@ -190,16 +190,16 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
>
>         string_stream_add(stream,
>                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> -                         binary_assert->left_text,
> -                         binary_assert->operation,
> -                         binary_assert->right_text);
> -       if (!is_str_literal(binary_assert->left_text, binary_assert->left_value))
> +                         binary_assert->text->left_text,
> +                         binary_assert->text->operation,
> +                         binary_assert->text->right_text);
> +       if (!is_str_literal(binary_assert->text->left_text, binary_assert->left_value))
>                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"\n",
> -                                 binary_assert->left_text,
> +                                 binary_assert->text->left_text,
>                                   binary_assert->left_value);
> -       if (!is_str_literal(binary_assert->right_text, binary_assert->right_value))
> +       if (!is_str_literal(binary_assert->text->right_text, binary_assert->right_value))
>                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"",
> -                                 binary_assert->right_text,
> +                                 binary_assert->text->right_text,
>                                   binary_assert->right_value);
>         kunit_assert_print_msg(message, stream);
>  }
> --
> 2.35.0.rc2.247.g8bbb082509-goog
>

Re: [PATCH 3/3] kunit: factor out str constants from binary assertion structs

[Daniel Latypov]

On Wed, Jan 26, 2022 at 7:39 PM David Gow <davidgow@google.com> wrote:
>
> On Wed, Jan 26, 2022 at 5:00 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > If the compiler doesn't optimize them away, each kunit assertion (use of
> > KUNIT_EXPECT_EQ, etc.) can use 88 bytes of stack space in the worst and
> > most common case. This has led to compiler warnings and a suggestion
> > from Linus to move data from the structs into static const's where
> > possible [1].
> >
> > This builds upon [2] which did so for the base struct kunit_assert type.
> > That only reduced sizeof(struct kunit_binary_assert) from 88 to 64.
> >
> > Given these are by far the most commonly used asserts, this patch
> > factors out the textual representations of the operands and comparator
> > into another static const, saving 16 more bytes.
> >
> > In detail, KUNIT_EXPECT_EQ(test, 2 + 2, 5) yields the following struct
> >   (struct kunit_binary_assert) {
> >     .assert = <struct kunit_assert>,
> >     .operation = "==",
> >     .left_text = "2 + 2",
> >     .left_value = 4,
> >     .right_text = "5",
> >     .right_value = 5,
> >   }
> > After this change
> >   static const struct kunit_binary_assert_text __text = {
> >     .operation = "==",
> >     .left_text = "2 + 2",
> >     .right_text = "5",
> >   };
> >   (struct kunit_binary_assert) {
> >     .assert = <struct kunit_assert>,
> >     .text = &__text,
> >     .left_value = 4,
> >     .right_value = 5,
> >   }
> >
> > This also DRYs the code a bit more since these str fields were repeated
> > for the string and pointer versions of kunit_binary_assert.
> >
> > Note: we could name the kunit_binary_assert_text fields left/right
> > instead of left_text/right_text. But that would require changing the
> > macros a bit since they have args called "left" and "right" which would
> > be substituted in `.left = #left` as `.2 + 2 = \"2 + 2\"`.
> >
> > [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
> > [2] https://lore.kernel.org/linux-kselftest/20220113165931.451305-6-dlatypov@google.com/
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> This definitely _feels_ like it's adding a bit more complexity than
> would be ideal by splitting this out into a separate function, but I
> do agree that it's worth it.

I'll note that this was *more* of a simplification until I deduped the
binary macros.
Since we only have one macro now passing in the left/right/op strings
now, it doesn't look like as much of an improvement anymore.

So now the main other benefits are DRYing the assert structs.
And I lean towards feeling that + stack size decrease = good enough
reason to go ahead with the refactor.

Re complexity, here's what KUNIT_EXPECT_EQ(test, 1 + 1, 2) turns into

do {
  typeof(1 + 1) __left = (1 + 1);
  typeof(2) __right = (2);
  static const struct kunit_binary_assert_text __text = {
    .operation = "==",
    .left_text = "1 + 1",
    .right_text = "2",
  };
  do {
    if (__builtin_expect(!!(!(__left == __right)), 0)) {
      static const struct kunit_loc loc = {
        .file = "lib/kunit/kunit-example-test.c",
        .line = 29
      };
      struct kunit_binary_assert __assertion = {
        .assert = { .format = kunit_binary_assert_format },
        .text = &__text,
        .left_value = __left,
        .right_value = __right
      };
      kunit_do_failed_assertion(test, &loc, KUNIT_EXPECTATION,
              &__assertion.assert,
              ((void *)0));
    }
  } while (0);
} while (0);

Actually, looking at this, I realize we should probably
1) move the __text decl into the if statement
2) probably should rename loc to __loc, oops.

I'll send out a v2 that does #1.
Maybe I'll include another patch that does #2 at the end of this
series since the source patch already got picked up into Shuah's tree.

>
> I think left_text / right_text are good enough names, too: I wouldn't
> bother trying to make them .left/.right.
>
>
> Reviewed-by: David Gow <davidgow@google.com>
>
> Cheers,
> -- David
>
> >  include/kunit/assert.h | 49 +++++++++++++++++++-----------------------
> >  include/kunit/test.h   | 20 +++++++++++------
> >  lib/kunit/assert.c     | 38 ++++++++++++++++----------------
> >  3 files changed, 54 insertions(+), 53 deletions(-)
> >
> > diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> > index 649bfac9f406..4b52e12c2ae8 100644
> > --- a/include/kunit/assert.h
> > +++ b/include/kunit/assert.h
> > @@ -150,14 +150,25 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
> >         .value = val                                                           \
> >  }
> >
> > +/**
> > + * struct kunit_binary_assert_text - holds strings for &struct
> > + *     kunit_binary_assert and friends to try and make the structs smaller.
> > + * @operation: A string representation of the comparison operator (e.g. "==").
> > + * @left_text: A string representation of the left expression (e.g. "2+2").
> > + * @right_text: A string representation of the right expression (e.g. "2+2").
> > + */
> > +struct kunit_binary_assert_text {
> > +       const char *operation;
> > +       const char *left_text;
> > +       const char *right_text;
> > +};
> > +
> >  /**
> >   * struct kunit_binary_assert - An expectation/assertion that compares two
> >   *     non-pointer values (for example, KUNIT_EXPECT_EQ(test, 1 + 1, 2)).
> >   * @assert: The parent of this type.
> > - * @operation: A string representation of the comparison operator (e.g. "==").
> > - * @left_text: A string representation of the expression in the left slot.
> > + * @text: Holds the textual representations of the operands and op (e.g.  "==").
> >   * @left_value: The actual evaluated value of the expression in the left slot.
> > - * @right_text: A string representation of the expression in the right slot.
> >   * @right_value: The actual evaluated value of the expression in the right slot.
> >   *
> >   * Represents an expectation/assertion that compares two non-pointer values. For
> > @@ -166,10 +177,8 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
> >   */
> >  struct kunit_binary_assert {
> >         struct kunit_assert assert;
> > -       const char *operation;
> > -       const char *left_text;
> > +       const struct kunit_binary_assert_text *text;
> >         long long left_value;
> > -       const char *right_text;
> >         long long right_value;
> >  };
> >
> > @@ -182,10 +191,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> >   *     kunit_binary_assert, kunit_binary_ptr_assert, etc.
> >   *
> >   * @format_func: a function which formats the assert to a string.
> > - * @op_str: A string representation of the comparison operator (e.g. "==").
> > - * @left_str: A string representation of the expression in the left slot.
> > + * @text_: Pointer to a kunit_binary_assert_text.
> >   * @left_val: The actual evaluated value of the expression in the left slot.
> > - * @right_str: A string representation of the expression in the right slot.
> >   * @right_val: The actual evaluated value of the expression in the right slot.
> >   *
> >   * Initializes a binary assert like kunit_binary_assert,
> > @@ -194,16 +201,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> >   * This is ultimately used by binary assertion macros like KUNIT_EXPECT_EQ, etc.
> >   */
> >  #define KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,                          \
> > -                                       op_str,                                \
> > -                                       left_str,                              \
> > +                                       text_,                                 \
> >                                         left_val,                              \
> > -                                       right_str,                             \
> >                                         right_val) {                           \
> >         .assert = { .format = format_func },                                   \
> > -       .operation = op_str,                                                   \
> > -       .left_text = left_str,                                                 \
> > +       .text = text_,                                                         \
> >         .left_value = left_val,                                                \
> > -       .right_text = right_str,                                               \
> >         .right_value = right_val                                               \
> >  }
> >
> > @@ -211,10 +214,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> >   * struct kunit_binary_ptr_assert - An expectation/assertion that compares two
> >   *     pointer values (for example, KUNIT_EXPECT_PTR_EQ(test, foo, bar)).
> >   * @assert: The parent of this type.
> > - * @operation: A string representation of the comparison operator (e.g. "==").
> > - * @left_text: A string representation of the expression in the left slot.
> > + * @text: Holds the textual representations of the operands and op (e.g.  "==").
> >   * @left_value: The actual evaluated value of the expression in the left slot.
> > - * @right_text: A string representation of the expression in the right slot.
> >   * @right_value: The actual evaluated value of the expression in the right slot.
> >   *
> >   * Represents an expectation/assertion that compares two pointer values. For
> > @@ -223,10 +224,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> >   */
> >  struct kunit_binary_ptr_assert {
> >         struct kunit_assert assert;
> > -       const char *operation;
> > -       const char *left_text;
> > +       const struct kunit_binary_assert_text *text;
> >         const void *left_value;
> > -       const char *right_text;
> >         const void *right_value;
> >  };
> >
> > @@ -238,10 +237,8 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
> >   * struct kunit_binary_str_assert - An expectation/assertion that compares two
> >   *     string values (for example, KUNIT_EXPECT_STREQ(test, foo, "bar")).
> >   * @assert: The parent of this type.
> > - * @operation: A string representation of the comparison operator (e.g. "==").
> > - * @left_text: A string representation of the expression in the left slot.
> > + * @text: Holds the textual representations of the operands and comparator.
> >   * @left_value: The actual evaluated value of the expression in the left slot.
> > - * @right_text: A string representation of the expression in the right slot.
> >   * @right_value: The actual evaluated value of the expression in the right slot.
> >   *
> >   * Represents an expectation/assertion that compares two string values. For
> > @@ -250,10 +247,8 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
> >   */
> >  struct kunit_binary_str_assert {
> >         struct kunit_assert assert;
> > -       const char *operation;
> > -       const char *left_text;
> > +       const struct kunit_binary_assert_text *text;
> >         const char *left_value;
> > -       const char *right_text;
> >         const char *right_value;
> >  };
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index a93dfb8ff393..088ff394ae94 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -874,16 +874,19 @@ void kunit_do_failed_assertion(struct kunit *test,
> >  do {                                                                          \
> >         typeof(left) __left = (left);                                          \
> >         typeof(right) __right = (right);                                       \
> > +       static const struct kunit_binary_assert_text __text = {                \
> > +               .operation = #op,                                              \
> > +               .left_text = #left,                                            \
> > +               .right_text = #right,                                          \
> > +       };                                                                     \
> >                                                                                \
> >         KUNIT_ASSERTION(test,                                                  \
> >                         assert_type,                                           \
> >                         __left op __right,                                     \
> >                         assert_class,                                          \
> >                         KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,           \
> > -                                                       #op,                   \
> > -                                                       #left,                 \
> > +                                                       &__text,               \
> >                                                         __left,                \
> > -                                                       #right,                \
> >                                                         __right),              \
> >                         fmt,                                                   \
> >                         ##__VA_ARGS__);                                        \
> > @@ -928,17 +931,20 @@ do {                                                                             \
> >                                    ...)                                        \
> >  do {                                                                          \
> >         const char *__left = (left);                                           \
> > -       const char *__right = (right);                                 \
> > +       const char *__right = (right);                                         \
> > +       static const struct kunit_binary_assert_text __text = {                \
> > +               .operation = #op,                                              \
> > +               .left_text = #left,                                            \
> > +               .right_text = #right,                                          \
> > +       };                                                                     \
> >                                                                                \
> >         KUNIT_ASSERTION(test,                                                  \
> >                         assert_type,                                           \
> >                         strcmp(__left, __right) op 0,                          \
> >                         kunit_binary_str_assert,                               \
> >                         KUNIT_INIT_BINARY_ASSERT_STRUCT(kunit_binary_str_assert_format,\
> > -                                                       #op,                   \
> > -                                                       #left,                 \
> > +                                                       &__text,               \
> >                                                         __left,                \
> > -                                                       #right,                \
> >                                                         __right),              \
> >                         fmt,                                                   \
> >                         ##__VA_ARGS__);                                        \
> > diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> > index c9c7ee0dfafa..d00d6d181ee8 100644
> > --- a/lib/kunit/assert.c
> > +++ b/lib/kunit/assert.c
> > @@ -122,18 +122,18 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> >
> >         string_stream_add(stream,
> >                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> > -                         binary_assert->left_text,
> > -                         binary_assert->operation,
> > -                         binary_assert->right_text);
> > -       if (!is_literal(stream->test, binary_assert->left_text,
> > +                         binary_assert->text->left_text,
> > +                         binary_assert->text->operation,
> > +                         binary_assert->text->right_text);
> > +       if (!is_literal(stream->test, binary_assert->text->left_text,
> >                         binary_assert->left_value, stream->gfp))
> >                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld\n",
> > -                                 binary_assert->left_text,
> > +                                 binary_assert->text->left_text,
> >                                   binary_assert->left_value);
> > -       if (!is_literal(stream->test, binary_assert->right_text,
> > +       if (!is_literal(stream->test, binary_assert->text->right_text,
> >                         binary_assert->right_value, stream->gfp))
> >                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld",
> > -                                 binary_assert->right_text,
> > +                                 binary_assert->text->right_text,
> >                                   binary_assert->right_value);
> >         kunit_assert_print_msg(message, stream);
> >  }
> > @@ -150,14 +150,14 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
> >
> >         string_stream_add(stream,
> >                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> > -                         binary_assert->left_text,
> > -                         binary_assert->operation,
> > -                         binary_assert->right_text);
> > +                         binary_assert->text->left_text,
> > +                         binary_assert->text->operation,
> > +                         binary_assert->text->right_text);
> >         string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px\n",
> > -                         binary_assert->left_text,
> > +                         binary_assert->text->left_text,
> >                           binary_assert->left_value);
> >         string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px",
> > -                         binary_assert->right_text,
> > +                         binary_assert->text->right_text,
> >                           binary_assert->right_value);
> >         kunit_assert_print_msg(message, stream);
> >  }
> > @@ -190,16 +190,16 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
> >
> >         string_stream_add(stream,
> >                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> > -                         binary_assert->left_text,
> > -                         binary_assert->operation,
> > -                         binary_assert->right_text);
> > -       if (!is_str_literal(binary_assert->left_text, binary_assert->left_value))
> > +                         binary_assert->text->left_text,
> > +                         binary_assert->text->operation,
> > +                         binary_assert->text->right_text);
> > +       if (!is_str_literal(binary_assert->text->left_text, binary_assert->left_value))
> >                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"\n",
> > -                                 binary_assert->left_text,
> > +                                 binary_assert->text->left_text,
> >                                   binary_assert->left_value);
> > -       if (!is_str_literal(binary_assert->right_text, binary_assert->right_value))
> > +       if (!is_str_literal(binary_assert->text->right_text, binary_assert->right_value))
> >                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"",
> > -                                 binary_assert->right_text,
> > +                                 binary_assert->text->right_text,
> >                                   binary_assert->right_value);
> >         kunit_assert_print_msg(message, stream);
> >  }
> > --
> > 2.35.0.rc2.247.g8bbb082509-goog
> >

Re: [PATCH 3/3] kunit: factor out str constants from binary assertion structs

[Daniel Latypov]

On Thu, Jan 27, 2022 at 12:21 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Wed, Jan 26, 2022 at 7:39 PM David Gow <davidgow@google.com> wrote:
> >
> > On Wed, Jan 26, 2022 at 5:00 AM Daniel Latypov <dlatypov@google.com> wrote:
> > >
> > > If the compiler doesn't optimize them away, each kunit assertion (use of
> > > KUNIT_EXPECT_EQ, etc.) can use 88 bytes of stack space in the worst and
> > > most common case. This has led to compiler warnings and a suggestion
> > > from Linus to move data from the structs into static const's where
> > > possible [1].
> > >
> > > This builds upon [2] which did so for the base struct kunit_assert type.
> > > That only reduced sizeof(struct kunit_binary_assert) from 88 to 64.
> > >
> > > Given these are by far the most commonly used asserts, this patch
> > > factors out the textual representations of the operands and comparator
> > > into another static const, saving 16 more bytes.
> > >
> > > In detail, KUNIT_EXPECT_EQ(test, 2 + 2, 5) yields the following struct
> > >   (struct kunit_binary_assert) {
> > >     .assert = <struct kunit_assert>,
> > >     .operation = "==",
> > >     .left_text = "2 + 2",
> > >     .left_value = 4,
> > >     .right_text = "5",
> > >     .right_value = 5,
> > >   }
> > > After this change
> > >   static const struct kunit_binary_assert_text __text = {
> > >     .operation = "==",
> > >     .left_text = "2 + 2",
> > >     .right_text = "5",
> > >   };
> > >   (struct kunit_binary_assert) {
> > >     .assert = <struct kunit_assert>,
> > >     .text = &__text,
> > >     .left_value = 4,
> > >     .right_value = 5,
> > >   }
> > >
> > > This also DRYs the code a bit more since these str fields were repeated
> > > for the string and pointer versions of kunit_binary_assert.
> > >
> > > Note: we could name the kunit_binary_assert_text fields left/right
> > > instead of left_text/right_text. But that would require changing the
> > > macros a bit since they have args called "left" and "right" which would
> > > be substituted in `.left = #left` as `.2 + 2 = \"2 + 2\"`.
> > >
> > > [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
> > > [2] https://lore.kernel.org/linux-kselftest/20220113165931.451305-6-dlatypov@google.com/
> > >
> > > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > > ---
> >
> > This definitely _feels_ like it's adding a bit more complexity than
> > would be ideal by splitting this out into a separate function, but I
> > do agree that it's worth it.
>
> I'll note that this was *more* of a simplification until I deduped the
> binary macros.
> Since we only have one macro now passing in the left/right/op strings
> now, it doesn't look like as much of an improvement anymore.
>
> So now the main other benefits are DRYing the assert structs.
> And I lean towards feeling that + stack size decrease = good enough
> reason to go ahead with the refactor.
>
> Re complexity, here's what KUNIT_EXPECT_EQ(test, 1 + 1, 2) turns into
>
> do {
>   typeof(1 + 1) __left = (1 + 1);
>   typeof(2) __right = (2);
>   static const struct kunit_binary_assert_text __text = {
>     .operation = "==",
>     .left_text = "1 + 1",
>     .right_text = "2",
>   };
>   do {
>     if (__builtin_expect(!!(!(__left == __right)), 0)) {
>       static const struct kunit_loc loc = {
>         .file = "lib/kunit/kunit-example-test.c",
>         .line = 29
>       };
>       struct kunit_binary_assert __assertion = {
>         .assert = { .format = kunit_binary_assert_format },
>         .text = &__text,
>         .left_value = __left,
>         .right_value = __right
>       };
>       kunit_do_failed_assertion(test, &loc, KUNIT_EXPECTATION,
>               &__assertion.assert,
>               ((void *)0));
>     }
>   } while (0);
> } while (0);
>
> Actually, looking at this, I realize we should probably
> 1) move the __text decl into the if statement

Nevermind, was a brainfart.
We can't move that into the if, since that happens inside the
KUNIT_ASSERTION macro and so we need to initialize __text outside of
it.

It's a bit unfortunately we need to pay the cost of initializing
__text even when we might not use it, but that's honestly a fairly
minimal cost and performance isn't KUnit's focus anyways.

> 2) probably should rename loc to __loc, oops.
>
> I'll send out a v2 that does #1.
> Maybe I'll include another patch that does #2 at the end of this
> series since the source patch already got picked up into Shuah's tree.
>
> >
> > I think left_text / right_text are good enough names, too: I wouldn't
> > bother trying to make them .left/.right.
> >
> >
> > Reviewed-by: David Gow <davidgow@google.com>

Re: [PATCH 3/3] kunit: factor out str constants from binary assertion structs

[Brendan Higgins]

On Wed, Jan 26, 2022 at 10:39 PM David Gow <davidgow@google.com> wrote:
>
> On Wed, Jan 26, 2022 at 5:00 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > If the compiler doesn't optimize them away, each kunit assertion (use of
> > KUNIT_EXPECT_EQ, etc.) can use 88 bytes of stack space in the worst and
> > most common case. This has led to compiler warnings and a suggestion
> > from Linus to move data from the structs into static const's where
> > possible [1].
> >
> > This builds upon [2] which did so for the base struct kunit_assert type.
> > That only reduced sizeof(struct kunit_binary_assert) from 88 to 64.
> >
> > Given these are by far the most commonly used asserts, this patch
> > factors out the textual representations of the operands and comparator
> > into another static const, saving 16 more bytes.
> >
> > In detail, KUNIT_EXPECT_EQ(test, 2 + 2, 5) yields the following struct
> >   (struct kunit_binary_assert) {
> >     .assert = <struct kunit_assert>,
> >     .operation = "==",
> >     .left_text = "2 + 2",
> >     .left_value = 4,
> >     .right_text = "5",
> >     .right_value = 5,
> >   }
> > After this change
> >   static const struct kunit_binary_assert_text __text = {
> >     .operation = "==",
> >     .left_text = "2 + 2",
> >     .right_text = "5",
> >   };
> >   (struct kunit_binary_assert) {
> >     .assert = <struct kunit_assert>,
> >     .text = &__text,
> >     .left_value = 4,
> >     .right_value = 5,
> >   }
> >
> > This also DRYs the code a bit more since these str fields were repeated
> > for the string and pointer versions of kunit_binary_assert.
> >
> > Note: we could name the kunit_binary_assert_text fields left/right
> > instead of left_text/right_text. But that would require changing the
> > macros a bit since they have args called "left" and "right" which would
> > be substituted in `.left = #left` as `.2 + 2 = \"2 + 2\"`.
> >
> > [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
> > [2] https://lore.kernel.org/linux-kselftest/20220113165931.451305-6-dlatypov@google.com/
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> This definitely _feels_ like it's adding a bit more complexity than
> would be ideal by splitting this out into a separate function, but I
> do agree that it's worth it.

Agreed.

> I think left_text / right_text are good enough names, too: I wouldn't
> bother trying to make them .left/.right.

Yeah, I don't think it matters too much either.

> Reviewed-by: David Gow <davidgow@google.com>
>
> Cheers,
> -- David
>
> >  include/kunit/assert.h | 49 +++++++++++++++++++-----------------------
> >  include/kunit/test.h   | 20 +++++++++++------
> >  lib/kunit/assert.c     | 38 ++++++++++++++++----------------
> >  3 files changed, 54 insertions(+), 53 deletions(-)
> >
> > diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> > index 649bfac9f406..4b52e12c2ae8 100644
> > --- a/include/kunit/assert.h
> > +++ b/include/kunit/assert.h
> > @@ -150,14 +150,25 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
> >         .value = val                                                           \
> >  }
> >
> > +/**
> > + * struct kunit_binary_assert_text - holds strings for &struct
> > + *     kunit_binary_assert and friends to try and make the structs smaller.
> > + * @operation: A string representation of the comparison operator (e.g. "==").
> > + * @left_text: A string representation of the left expression (e.g. "2+2").
> > + * @right_text: A string representation of the right expression (e.g. "2+2").
> > + */
> > +struct kunit_binary_assert_text {
> > +       const char *operation;
> > +       const char *left_text;
> > +       const char *right_text;
> > +};
> > +
> >  /**
> >   * struct kunit_binary_assert - An expectation/assertion that compares two
> >   *     non-pointer values (for example, KUNIT_EXPECT_EQ(test, 1 + 1, 2)).
> >   * @assert: The parent of this type.
> > - * @operation: A string representation of the comparison operator (e.g. "==").
> > - * @left_text: A string representation of the expression in the left slot.
> > + * @text: Holds the textual representations of the operands and op (e.g.  "==").
> >   * @left_value: The actual evaluated value of the expression in the left slot.
> > - * @right_text: A string representation of the expression in the right slot.
> >   * @right_value: The actual evaluated value of the expression in the right slot.
> >   *
> >   * Represents an expectation/assertion that compares two non-pointer values. For
> > @@ -166,10 +177,8 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
> >   */
> >  struct kunit_binary_assert {
> >         struct kunit_assert assert;
> > -       const char *operation;
> > -       const char *left_text;
> > +       const struct kunit_binary_assert_text *text;
> >         long long left_value;
> > -       const char *right_text;
> >         long long right_value;
> >  };
> >
> > @@ -182,10 +191,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> >   *     kunit_binary_assert, kunit_binary_ptr_assert, etc.
> >   *
> >   * @format_func: a function which formats the assert to a string.
> > - * @op_str: A string representation of the comparison operator (e.g. "==").
> > - * @left_str: A string representation of the expression in the left slot.
> > + * @text_: Pointer to a kunit_binary_assert_text.
> >   * @left_val: The actual evaluated value of the expression in the left slot.
> > - * @right_str: A string representation of the expression in the right slot.
> >   * @right_val: The actual evaluated value of the expression in the right slot.
> >   *
> >   * Initializes a binary assert like kunit_binary_assert,
> > @@ -194,16 +201,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> >   * This is ultimately used by binary assertion macros like KUNIT_EXPECT_EQ, etc.
> >   */
> >  #define KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,                          \
> > -                                       op_str,                                \
> > -                                       left_str,                              \
> > +                                       text_,                                 \
> >                                         left_val,                              \
> > -                                       right_str,                             \
> >                                         right_val) {                           \
> >         .assert = { .format = format_func },                                   \
> > -       .operation = op_str,                                                   \
> > -       .left_text = left_str,                                                 \
> > +       .text = text_,                                                         \
> >         .left_value = left_val,                                                \
> > -       .right_text = right_str,                                               \
> >         .right_value = right_val                                               \
> >  }
> >
> > @@ -211,10 +214,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> >   * struct kunit_binary_ptr_assert - An expectation/assertion that compares two
> >   *     pointer values (for example, KUNIT_EXPECT_PTR_EQ(test, foo, bar)).
> >   * @assert: The parent of this type.
> > - * @operation: A string representation of the comparison operator (e.g. "==").
> > - * @left_text: A string representation of the expression in the left slot.
> > + * @text: Holds the textual representations of the operands and op (e.g.  "==").
> >   * @left_value: The actual evaluated value of the expression in the left slot.
> > - * @right_text: A string representation of the expression in the right slot.
> >   * @right_value: The actual evaluated value of the expression in the right slot.
> >   *
> >   * Represents an expectation/assertion that compares two pointer values. For
> > @@ -223,10 +224,8 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> >   */
> >  struct kunit_binary_ptr_assert {
> >         struct kunit_assert assert;
> > -       const char *operation;
> > -       const char *left_text;
> > +       const struct kunit_binary_assert_text *text;
> >         const void *left_value;
> > -       const char *right_text;
> >         const void *right_value;
> >  };
> >
> > @@ -238,10 +237,8 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
> >   * struct kunit_binary_str_assert - An expectation/assertion that compares two
> >   *     string values (for example, KUNIT_EXPECT_STREQ(test, foo, "bar")).
> >   * @assert: The parent of this type.
> > - * @operation: A string representation of the comparison operator (e.g. "==").
> > - * @left_text: A string representation of the expression in the left slot.
> > + * @text: Holds the textual representations of the operands and comparator.
> >   * @left_value: The actual evaluated value of the expression in the left slot.
> > - * @right_text: A string representation of the expression in the right slot.
> >   * @right_value: The actual evaluated value of the expression in the right slot.
> >   *
> >   * Represents an expectation/assertion that compares two string values. For
> > @@ -250,10 +247,8 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
> >   */
> >  struct kunit_binary_str_assert {
> >         struct kunit_assert assert;
> > -       const char *operation;
> > -       const char *left_text;
> > +       const struct kunit_binary_assert_text *text;
> >         const char *left_value;
> > -       const char *right_text;
> >         const char *right_value;
> >  };
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index a93dfb8ff393..088ff394ae94 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -874,16 +874,19 @@ void kunit_do_failed_assertion(struct kunit *test,
> >  do {                                                                          \
> >         typeof(left) __left = (left);                                          \
> >         typeof(right) __right = (right);                                       \
> > +       static const struct kunit_binary_assert_text __text = {                \
> > +               .operation = #op,                                              \
> > +               .left_text = #left,                                            \
> > +               .right_text = #right,                                          \
> > +       };                                                                     \
> >                                                                                \
> >         KUNIT_ASSERTION(test,                                                  \
> >                         assert_type,                                           \
> >                         __left op __right,                                     \
> >                         assert_class,                                          \
> >                         KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func,           \
> > -                                                       #op,                   \
> > -                                                       #left,                 \
> > +                                                       &__text,               \
> >                                                         __left,                \
> > -                                                       #right,                \
> >                                                         __right),              \
> >                         fmt,                                                   \
> >                         ##__VA_ARGS__);                                        \
> > @@ -928,17 +931,20 @@ do {                                                                             \
> >                                    ...)                                        \
> >  do {                                                                          \
> >         const char *__left = (left);                                           \
> > -       const char *__right = (right);                                 \
> > +       const char *__right = (right);                                         \
> > +       static const struct kunit_binary_assert_text __text = {                \
> > +               .operation = #op,                                              \
> > +               .left_text = #left,                                            \
> > +               .right_text = #right,                                          \
> > +       };                                                                     \
> >                                                                                \
> >         KUNIT_ASSERTION(test,                                                  \
> >                         assert_type,                                           \
> >                         strcmp(__left, __right) op 0,                          \
> >                         kunit_binary_str_assert,                               \
> >                         KUNIT_INIT_BINARY_ASSERT_STRUCT(kunit_binary_str_assert_format,\
> > -                                                       #op,                   \
> > -                                                       #left,                 \
> > +                                                       &__text,               \
> >                                                         __left,                \
> > -                                                       #right,                \
> >                                                         __right),              \
> >                         fmt,                                                   \
> >                         ##__VA_ARGS__);                                        \
> > diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> > index c9c7ee0dfafa..d00d6d181ee8 100644
> > --- a/lib/kunit/assert.c
> > +++ b/lib/kunit/assert.c
> > @@ -122,18 +122,18 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> >
> >         string_stream_add(stream,
> >                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> > -                         binary_assert->left_text,
> > -                         binary_assert->operation,
> > -                         binary_assert->right_text);
> > -       if (!is_literal(stream->test, binary_assert->left_text,
> > +                         binary_assert->text->left_text,
> > +                         binary_assert->text->operation,
> > +                         binary_assert->text->right_text);
> > +       if (!is_literal(stream->test, binary_assert->text->left_text,
> >                         binary_assert->left_value, stream->gfp))
> >                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld\n",
> > -                                 binary_assert->left_text,
> > +                                 binary_assert->text->left_text,
> >                                   binary_assert->left_value);
> > -       if (!is_literal(stream->test, binary_assert->right_text,
> > +       if (!is_literal(stream->test, binary_assert->text->right_text,
> >                         binary_assert->right_value, stream->gfp))
> >                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld",
> > -                                 binary_assert->right_text,
> > +                                 binary_assert->text->right_text,
> >                                   binary_assert->right_value);
> >         kunit_assert_print_msg(message, stream);
> >  }
> > @@ -150,14 +150,14 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
> >
> >         string_stream_add(stream,
> >                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> > -                         binary_assert->left_text,
> > -                         binary_assert->operation,
> > -                         binary_assert->right_text);
> > +                         binary_assert->text->left_text,
> > +                         binary_assert->text->operation,
> > +                         binary_assert->text->right_text);
> >         string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px\n",
> > -                         binary_assert->left_text,
> > +                         binary_assert->text->left_text,
> >                           binary_assert->left_value);
> >         string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px",
> > -                         binary_assert->right_text,
> > +                         binary_assert->text->right_text,
> >                           binary_assert->right_value);
> >         kunit_assert_print_msg(message, stream);
> >  }
> > @@ -190,16 +190,16 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
> >
> >         string_stream_add(stream,
> >                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> > -                         binary_assert->left_text,
> > -                         binary_assert->operation,
> > -                         binary_assert->right_text);
> > -       if (!is_str_literal(binary_assert->left_text, binary_assert->left_value))
> > +                         binary_assert->text->left_text,
> > +                         binary_assert->text->operation,
> > +                         binary_assert->text->right_text);
> > +       if (!is_str_literal(binary_assert->text->left_text, binary_assert->left_value))
> >                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"\n",
> > -                                 binary_assert->left_text,
> > +                                 binary_assert->text->left_text,
> >                                   binary_assert->left_value);
> > -       if (!is_str_literal(binary_assert->right_text, binary_assert->right_value))
> > +       if (!is_str_literal(binary_assert->text->right_text, binary_assert->right_value))
> >                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"",
> > -                                 binary_assert->right_text,
> > +                                 binary_assert->text->right_text,
> >                                   binary_assert->right_value);
> >         kunit_assert_print_msg(message, stream);
> >  }
> > --
> > 2.35.0.rc2.247.g8bbb082509-goog
> >

Re: [PATCH 3/3] kunit: factor out str constants from binary assertion structs

[Brendan Higgins]

On Tue, Jan 25, 2022 at 4:00 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> If the compiler doesn't optimize them away, each kunit assertion (use of
> KUNIT_EXPECT_EQ, etc.) can use 88 bytes of stack space in the worst and
> most common case. This has led to compiler warnings and a suggestion
> from Linus to move data from the structs into static const's where
> possible [1].
>
> This builds upon [2] which did so for the base struct kunit_assert type.
> That only reduced sizeof(struct kunit_binary_assert) from 88 to 64.
>
> Given these are by far the most commonly used asserts, this patch
> factors out the textual representations of the operands and comparator
> into another static const, saving 16 more bytes.
>
> In detail, KUNIT_EXPECT_EQ(test, 2 + 2, 5) yields the following struct
>   (struct kunit_binary_assert) {
>     .assert = <struct kunit_assert>,
>     .operation = "==",
>     .left_text = "2 + 2",
>     .left_value = 4,
>     .right_text = "5",
>     .right_value = 5,
>   }
> After this change
>   static const struct kunit_binary_assert_text __text = {
>     .operation = "==",
>     .left_text = "2 + 2",
>     .right_text = "5",
>   };
>   (struct kunit_binary_assert) {
>     .assert = <struct kunit_assert>,
>     .text = &__text,
>     .left_value = 4,
>     .right_value = 5,
>   }
>
> This also DRYs the code a bit more since these str fields were repeated
> for the string and pointer versions of kunit_binary_assert.
>
> Note: we could name the kunit_binary_assert_text fields left/right
> instead of left_text/right_text. But that would require changing the
> macros a bit since they have args called "left" and "right" which would
> be substituted in `.left = #left` as `.2 + 2 = \"2 + 2\"`.
>
> [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
> [2] https://lore.kernel.org/linux-kselftest/20220113165931.451305-6-dlatypov@google.com/
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>