* [PATCH 1/4] kunit: remove format func from struct kunit_assert, get it to 0 bytes
2022-10-01 0:26 [PATCH 0/4] kunit: more assertion reworking Daniel Latypov
@ 2022-10-01 0:26 ` Daniel Latypov
2022-10-01 3:26 ` David Gow
2022-10-01 0:26 ` [PATCH 2/4] kunit: rename base KUNIT_ASSERTION macro to _KUNIT_FAILED Daniel Latypov
` (3 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Daniel Latypov @ 2022-10-01 0:26 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan,
miguel.ojeda.sandonis, Daniel Latypov
Each calll to a KUNIT_EXPECT_*() macro creates a local variable which
contains a struct kunit_assert.
Normally, we'd hope the compiler would be able to optimize this away,
but we've seen cases where it hasn't, see
https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/GbrMNej2BAAJ.
In changes like commit 21957f90b28f ("kunit: split out part of
kunit_assert into a static const"), we've moved more and more parts out
of struct kunit_assert and its children types (kunit_binary_assert).
This patch removes the final field and gets us to:
sizeof(struct kunit_assert) == 0
sizeof(struct kunit_binary_assert) == 24 (on UML x86_64).
This also reduces the amount of macro plumbing going on at the cost of
passing in one more arg to the base KUNIT_ASSERTION macro and
kunit_do_failed_assertion().
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
include/kunit/assert.h | 28 ++++++----------------------
include/kunit/test.h | 17 +++++++++++------
lib/kunit/test.c | 7 ++++---
3 files changed, 21 insertions(+), 31 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 4b52e12c2ae8..ace3de8d1ee7 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -42,16 +42,15 @@ struct kunit_loc {
/**
* struct kunit_assert - Data for printing a failed assertion or expectation.
- * @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 {
- void (*format)(const struct kunit_assert *assert,
- const struct va_format *message,
- struct string_stream *stream);
-};
+struct kunit_assert {};
+
+typedef void (*assert_format_t)(const struct kunit_assert *assert,
+ const struct va_format *message,
+ struct string_stream *stream);
void kunit_assert_prologue(const struct kunit_loc *loc,
enum kunit_assert_type type,
@@ -71,16 +70,6 @@ void kunit_fail_assert_format(const struct kunit_assert *assert,
const struct va_format *message,
struct string_stream *stream);
-/**
- * KUNIT_INIT_FAIL_ASSERT_STRUCT - Initializer for &struct kunit_fail_assert.
- *
- * Initializes a &struct kunit_fail_assert. Intended to be used in
- * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
- */
-#define KUNIT_INIT_FAIL_ASSERT_STRUCT { \
- .assert = { .format = kunit_fail_assert_format }, \
-}
-
/**
* struct kunit_unary_assert - Represents a KUNIT_{EXPECT|ASSERT}_{TRUE|FALSE}
* @assert: The parent of this type.
@@ -110,7 +99,6 @@ 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 = { .format = kunit_unary_assert_format }, \
.condition = cond, \
.expected_true = expect_true \
}
@@ -145,7 +133,6 @@ 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 = { .format = kunit_ptr_not_err_assert_format }, \
.text = txt, \
.value = val \
}
@@ -190,7 +177,6 @@ void kunit_binary_assert_format(const struct kunit_assert *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.
* @text_: Pointer to a kunit_binary_assert_text.
* @left_val: The actual evaluated value of the expression in the left slot.
* @right_val: The actual evaluated value of the expression in the right slot.
@@ -200,11 +186,9 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
* 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(format_func, \
- text_, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(text_, \
left_val, \
right_val) { \
- .assert = { .format = format_func }, \
.text = text_, \
.left_value = left_val, \
.right_value = right_val \
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 840a2c375065..3476549106f7 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -472,9 +472,10 @@ void kunit_do_failed_assertion(struct kunit *test,
const struct kunit_loc *loc,
enum kunit_assert_type type,
const struct kunit_assert *assert,
+ assert_format_t assert_format,
const char *fmt, ...);
-#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, INITIALIZER, fmt, ...) do { \
+#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
if (unlikely(!(pass))) { \
static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
struct assert_class __assertion = INITIALIZER; \
@@ -482,6 +483,7 @@ void kunit_do_failed_assertion(struct kunit *test,
&__loc, \
assert_type, \
&__assertion.assert, \
+ assert_format, \
fmt, \
##__VA_ARGS__); \
} \
@@ -493,7 +495,8 @@ void kunit_do_failed_assertion(struct kunit *test,
assert_type, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT, \
+ kunit_fail_assert_format, \
+ {}, \
fmt, \
##__VA_ARGS__)
@@ -524,6 +527,7 @@ void kunit_do_failed_assertion(struct kunit *test,
assert_type, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
+ kunit_unary_assert_format, \
KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
expected_true), \
fmt, \
@@ -581,8 +585,8 @@ do { \
assert_type, \
__left op __right, \
assert_class, \
- KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func, \
- &__text, \
+ format_func, \
+ KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
__left, \
__right), \
fmt, \
@@ -639,8 +643,8 @@ do { \
assert_type, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
- KUNIT_INIT_BINARY_ASSERT_STRUCT(kunit_binary_str_assert_format,\
- &__text, \
+ kunit_binary_str_assert_format, \
+ KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
__left, \
__right), \
fmt, \
@@ -659,6 +663,7 @@ do { \
assert_type, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
+ kunit_ptr_not_err_assert_format, \
KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \
__ptr), \
fmt, \
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index b73d5bb5c473..53bf1793f915 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -247,7 +247,7 @@ 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, const struct kunit_assert *assert,
- const struct va_format *message)
+ assert_format_t assert_format, const struct va_format *message)
{
struct string_stream *stream;
@@ -263,7 +263,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
}
kunit_assert_prologue(loc, type, stream);
- assert->format(assert, message, stream);
+ assert_format(assert, message, stream);
kunit_print_string_stream(test, stream);
@@ -287,6 +287,7 @@ void kunit_do_failed_assertion(struct kunit *test,
const struct kunit_loc *loc,
enum kunit_assert_type type,
const struct kunit_assert *assert,
+ assert_format_t assert_format,
const char *fmt, ...)
{
va_list args;
@@ -296,7 +297,7 @@ void kunit_do_failed_assertion(struct kunit *test,
message.fmt = fmt;
message.va = &args;
- kunit_fail(test, loc, type, assert, &message);
+ kunit_fail(test, loc, type, assert, assert_format, &message);
va_end(args);
--
2.38.0.rc1.362.ged0d419d3c-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 1/4] kunit: remove format func from struct kunit_assert, get it to 0 bytes
2022-10-01 0:26 ` [PATCH 1/4] kunit: remove format func from struct kunit_assert, get it to 0 bytes Daniel Latypov
@ 2022-10-01 3:26 ` David Gow
0 siblings, 0 replies; 19+ messages in thread
From: David Gow @ 2022-10-01 3:26 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Miguel Ojeda
[-- Attachment #1: Type: text/plain, Size: 13017 bytes --]
On Sat, Oct 1, 2022 at 8:26 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> Each calll to a KUNIT_EXPECT_*() macro creates a local variable which
> contains a struct kunit_assert.
>
> Normally, we'd hope the compiler would be able to optimize this away,
> but we've seen cases where it hasn't, see
> https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/GbrMNej2BAAJ.
>
> In changes like commit 21957f90b28f ("kunit: split out part of
> kunit_assert into a static const"), we've moved more and more parts out
> of struct kunit_assert and its children types (kunit_binary_assert).
>
> This patch removes the final field and gets us to:
> sizeof(struct kunit_assert) == 0
> sizeof(struct kunit_binary_assert) == 24 (on UML x86_64).
>
> This also reduces the amount of macro plumbing going on at the cost of
> passing in one more arg to the base KUNIT_ASSERTION macro and
> kunit_do_failed_assertion().
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
Very glad to see this finally happening: we've definitely still been
seeing these stack size warnings, so reducing the stack use is good
And as someone who has always been a little uneasy with the number of
nested macros, the simplification is also a big win.
Reviewed-by: David Gow <davidgow@google.com>
Cheers,
-- David
> include/kunit/assert.h | 28 ++++++----------------------
> include/kunit/test.h | 17 +++++++++++------
> lib/kunit/test.c | 7 ++++---
> 3 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> index 4b52e12c2ae8..ace3de8d1ee7 100644
> --- a/include/kunit/assert.h
> +++ b/include/kunit/assert.h
> @@ -42,16 +42,15 @@ struct kunit_loc {
>
> /**
> * struct kunit_assert - Data for printing a failed assertion or expectation.
> - * @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 {
> - void (*format)(const struct kunit_assert *assert,
> - const struct va_format *message,
> - struct string_stream *stream);
> -};
> +struct kunit_assert {};
> +
> +typedef void (*assert_format_t)(const struct kunit_assert *assert,
> + const struct va_format *message,
> + struct string_stream *stream);
>
> void kunit_assert_prologue(const struct kunit_loc *loc,
> enum kunit_assert_type type,
> @@ -71,16 +70,6 @@ void kunit_fail_assert_format(const struct kunit_assert *assert,
> const struct va_format *message,
> struct string_stream *stream);
>
> -/**
> - * KUNIT_INIT_FAIL_ASSERT_STRUCT - Initializer for &struct kunit_fail_assert.
> - *
> - * Initializes a &struct kunit_fail_assert. Intended to be used in
> - * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
> - */
> -#define KUNIT_INIT_FAIL_ASSERT_STRUCT { \
> - .assert = { .format = kunit_fail_assert_format }, \
> -}
> -
> /**
> * struct kunit_unary_assert - Represents a KUNIT_{EXPECT|ASSERT}_{TRUE|FALSE}
> * @assert: The parent of this type.
> @@ -110,7 +99,6 @@ 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 = { .format = kunit_unary_assert_format }, \
> .condition = cond, \
> .expected_true = expect_true \
> }
> @@ -145,7 +133,6 @@ 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 = { .format = kunit_ptr_not_err_assert_format }, \
> .text = txt, \
> .value = val \
> }
> @@ -190,7 +177,6 @@ void kunit_binary_assert_format(const struct kunit_assert *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.
> * @text_: Pointer to a kunit_binary_assert_text.
> * @left_val: The actual evaluated value of the expression in the left slot.
> * @right_val: The actual evaluated value of the expression in the right slot.
> @@ -200,11 +186,9 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> * 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(format_func, \
> - text_, \
> +#define KUNIT_INIT_BINARY_ASSERT_STRUCT(text_, \
> left_val, \
> right_val) { \
> - .assert = { .format = format_func }, \
> .text = text_, \
> .left_value = left_val, \
> .right_value = right_val \
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 840a2c375065..3476549106f7 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -472,9 +472,10 @@ void kunit_do_failed_assertion(struct kunit *test,
> const struct kunit_loc *loc,
> enum kunit_assert_type type,
> const struct kunit_assert *assert,
> + assert_format_t assert_format,
> const char *fmt, ...);
>
> -#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, INITIALIZER, fmt, ...) do { \
> +#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
> if (unlikely(!(pass))) { \
> static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
> struct assert_class __assertion = INITIALIZER; \
> @@ -482,6 +483,7 @@ void kunit_do_failed_assertion(struct kunit *test,
> &__loc, \
> assert_type, \
> &__assertion.assert, \
> + assert_format, \
> fmt, \
> ##__VA_ARGS__); \
> } \
> @@ -493,7 +495,8 @@ void kunit_do_failed_assertion(struct kunit *test,
> assert_type, \
> false, \
> kunit_fail_assert, \
> - KUNIT_INIT_FAIL_ASSERT_STRUCT, \
> + kunit_fail_assert_format, \
> + {}, \
> fmt, \
> ##__VA_ARGS__)
>
> @@ -524,6 +527,7 @@ void kunit_do_failed_assertion(struct kunit *test,
> assert_type, \
> !!(condition) == !!expected_true, \
> kunit_unary_assert, \
> + kunit_unary_assert_format, \
> KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
> expected_true), \
> fmt, \
> @@ -581,8 +585,8 @@ do { \
> assert_type, \
> __left op __right, \
> assert_class, \
> - KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func, \
> - &__text, \
> + format_func, \
> + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
> __left, \
> __right), \
> fmt, \
> @@ -639,8 +643,8 @@ do { \
> assert_type, \
> strcmp(__left, __right) op 0, \
> kunit_binary_str_assert, \
> - KUNIT_INIT_BINARY_ASSERT_STRUCT(kunit_binary_str_assert_format,\
> - &__text, \
> + kunit_binary_str_assert_format, \
> + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
> __left, \
> __right), \
> fmt, \
> @@ -659,6 +663,7 @@ do { \
> assert_type, \
> !IS_ERR_OR_NULL(__ptr), \
> kunit_ptr_not_err_assert, \
> + kunit_ptr_not_err_assert_format, \
> KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \
> __ptr), \
> fmt, \
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index b73d5bb5c473..53bf1793f915 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -247,7 +247,7 @@ 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, const struct kunit_assert *assert,
> - const struct va_format *message)
> + assert_format_t assert_format, const struct va_format *message)
> {
> struct string_stream *stream;
>
> @@ -263,7 +263,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
> }
>
> kunit_assert_prologue(loc, type, stream);
> - assert->format(assert, message, stream);
> + assert_format(assert, message, stream);
>
> kunit_print_string_stream(test, stream);
>
> @@ -287,6 +287,7 @@ void kunit_do_failed_assertion(struct kunit *test,
> const struct kunit_loc *loc,
> enum kunit_assert_type type,
> const struct kunit_assert *assert,
> + assert_format_t assert_format,
> const char *fmt, ...)
> {
> va_list args;
> @@ -296,7 +297,7 @@ void kunit_do_failed_assertion(struct kunit *test,
> message.fmt = fmt;
> message.va = &args;
>
> - kunit_fail(test, loc, type, assert, &message);
> + kunit_fail(test, loc, type, assert, assert_format, &message);
>
> va_end(args);
>
> --
> 2.38.0.rc1.362.ged0d419d3c-goog
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/4] kunit: rename base KUNIT_ASSERTION macro to _KUNIT_FAILED
2022-10-01 0:26 [PATCH 0/4] kunit: more assertion reworking Daniel Latypov
2022-10-01 0:26 ` [PATCH 1/4] kunit: remove format func from struct kunit_assert, get it to 0 bytes Daniel Latypov
@ 2022-10-01 0:26 ` Daniel Latypov
2022-10-01 3:26 ` David Gow
2022-10-01 0:26 ` [PATCH 3/4] kunit: eliminate KUNIT_INIT_*_ASSERT_STRUCT macros Daniel Latypov
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Daniel Latypov @ 2022-10-01 0:26 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan,
miguel.ojeda.sandonis, Daniel Latypov
Context:
Currently this macro's name, KUNIT_ASSERTION conflicts with the name of
an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}.
It's hard to think of a better name for the enum, so rename this macro.
It's also a bit strange that the macro might do nothing depending on the
boolean argument `pass`. Why not have callers check themselves?
This patch:
Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now
we only call it when the check has failed.
Then we rename the macro the _KUNIT_FAILED() to reflect the new
semantics.
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
include/kunit/test.h | 123 +++++++++++++++++++++++--------------------
1 file changed, 65 insertions(+), 58 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 3476549106f7..fec437c8a2b7 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -475,30 +475,27 @@ void kunit_do_failed_assertion(struct kunit *test,
assert_format_t assert_format,
const char *fmt, ...);
-#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
- if (unlikely(!(pass))) { \
- static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
- struct assert_class __assertion = INITIALIZER; \
- kunit_do_failed_assertion(test, \
- &__loc, \
- assert_type, \
- &__assertion.assert, \
- assert_format, \
- fmt, \
- ##__VA_ARGS__); \
- } \
+#define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
+ static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
+ struct assert_class __assertion = INITIALIZER; \
+ kunit_do_failed_assertion(test, \
+ &__loc, \
+ assert_type, \
+ &__assertion.assert, \
+ assert_format, \
+ fmt, \
+ ##__VA_ARGS__); \
} while (0)
#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \
- KUNIT_ASSERTION(test, \
- assert_type, \
- false, \
- kunit_fail_assert, \
- kunit_fail_assert_format, \
- {}, \
- fmt, \
- ##__VA_ARGS__)
+ _KUNIT_FAILED(test, \
+ assert_type, \
+ kunit_fail_assert, \
+ kunit_fail_assert_format, \
+ {}, \
+ fmt, \
+ ##__VA_ARGS__)
/**
* KUNIT_FAIL() - Always causes a test to fail when evaluated.
@@ -523,15 +520,19 @@ void kunit_do_failed_assertion(struct kunit *test,
expected_true, \
fmt, \
...) \
- KUNIT_ASSERTION(test, \
- assert_type, \
- !!(condition) == !!expected_true, \
- kunit_unary_assert, \
- kunit_unary_assert_format, \
- KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
- expected_true), \
- fmt, \
- ##__VA_ARGS__)
+do { \
+ if (likely(!!(condition) == !!expected_true)) \
+ break; \
+ \
+ _KUNIT_FAILED(test, \
+ assert_type, \
+ kunit_unary_assert, \
+ kunit_unary_assert_format, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
+ expected_true), \
+ fmt, \
+ ##__VA_ARGS__); \
+} while (0)
#define KUNIT_TRUE_MSG_ASSERTION(test, assert_type, condition, fmt, ...) \
KUNIT_UNARY_ASSERTION(test, \
@@ -581,16 +582,18 @@ do { \
.right_text = #right, \
}; \
\
- KUNIT_ASSERTION(test, \
- assert_type, \
- __left op __right, \
- assert_class, \
- format_func, \
- KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
- __left, \
- __right), \
- fmt, \
- ##__VA_ARGS__); \
+ if (likely(__left op __right)) \
+ break; \
+ \
+ _KUNIT_FAILED(test, \
+ assert_type, \
+ assert_class, \
+ format_func, \
+ KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
+ __left, \
+ __right), \
+ fmt, \
+ ##__VA_ARGS__); \
} while (0)
#define KUNIT_BINARY_INT_ASSERTION(test, \
@@ -639,16 +642,19 @@ do { \
.right_text = #right, \
}; \
\
- KUNIT_ASSERTION(test, \
- assert_type, \
- strcmp(__left, __right) op 0, \
- kunit_binary_str_assert, \
- kunit_binary_str_assert_format, \
- KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
- __left, \
- __right), \
- fmt, \
- ##__VA_ARGS__); \
+ if (likely(strcmp(__left, __right) op 0)) \
+ break; \
+ \
+ \
+ _KUNIT_FAILED(test, \
+ assert_type, \
+ kunit_binary_str_assert, \
+ kunit_binary_str_assert_format, \
+ KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
+ __left, \
+ __right), \
+ fmt, \
+ ##__VA_ARGS__); \
} while (0)
#define KUNIT_PTR_NOT_ERR_OR_NULL_MSG_ASSERTION(test, \
@@ -659,15 +665,16 @@ do { \
do { \
const typeof(ptr) __ptr = (ptr); \
\
- KUNIT_ASSERTION(test, \
- assert_type, \
- !IS_ERR_OR_NULL(__ptr), \
- kunit_ptr_not_err_assert, \
- kunit_ptr_not_err_assert_format, \
- KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \
- __ptr), \
- fmt, \
- ##__VA_ARGS__); \
+ if (!IS_ERR_OR_NULL(__ptr)) \
+ break; \
+ \
+ _KUNIT_FAILED(test, \
+ assert_type, \
+ kunit_ptr_not_err_assert, \
+ kunit_ptr_not_err_assert_format, \
+ KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, __ptr), \
+ fmt, \
+ ##__VA_ARGS__); \
} while (0)
/**
--
2.38.0.rc1.362.ged0d419d3c-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/4] kunit: rename base KUNIT_ASSERTION macro to _KUNIT_FAILED
2022-10-01 0:26 ` [PATCH 2/4] kunit: rename base KUNIT_ASSERTION macro to _KUNIT_FAILED Daniel Latypov
@ 2022-10-01 3:26 ` David Gow
2022-10-01 3:50 ` Daniel Latypov
0 siblings, 1 reply; 19+ messages in thread
From: David Gow @ 2022-10-01 3:26 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Miguel Ojeda
[-- Attachment #1: Type: text/plain, Size: 14019 bytes --]
On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> Context:
> Currently this macro's name, KUNIT_ASSERTION conflicts with the name of
> an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}.
>
> It's hard to think of a better name for the enum, so rename this macro.
> It's also a bit strange that the macro might do nothing depending on the
> boolean argument `pass`. Why not have callers check themselves?
>
> This patch:
> Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now
> we only call it when the check has failed.
> Then we rename the macro the _KUNIT_FAILED() to reflect the new
> semantics.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
Looks good to me. I can't say the name _KUNIT_FAILED() feels perfect
to me, but I can't think of anything better, either. We've not used a
leading underscore for internal macros much thus far, as well, though
I've no personal objections to starting.
Regardless, let's get this in.
Reviewed-by: David Gow <davidgow@google.com>
Cheers,
-- David
> include/kunit/test.h | 123 +++++++++++++++++++++++--------------------
> 1 file changed, 65 insertions(+), 58 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 3476549106f7..fec437c8a2b7 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -475,30 +475,27 @@ void kunit_do_failed_assertion(struct kunit *test,
> assert_format_t assert_format,
> const char *fmt, ...);
>
> -#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
> - if (unlikely(!(pass))) { \
> - static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
> - struct assert_class __assertion = INITIALIZER; \
> - kunit_do_failed_assertion(test, \
> - &__loc, \
> - assert_type, \
> - &__assertion.assert, \
> - assert_format, \
> - fmt, \
> - ##__VA_ARGS__); \
> - } \
> +#define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
> + static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
> + struct assert_class __assertion = INITIALIZER; \
> + kunit_do_failed_assertion(test, \
> + &__loc, \
> + assert_type, \
> + &__assertion.assert, \
> + assert_format, \
> + fmt, \
> + ##__VA_ARGS__); \
> } while (0)
>
>
> #define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \
> - KUNIT_ASSERTION(test, \
> - assert_type, \
> - false, \
> - kunit_fail_assert, \
> - kunit_fail_assert_format, \
> - {}, \
> - fmt, \
> - ##__VA_ARGS__)
> + _KUNIT_FAILED(test, \
> + assert_type, \
> + kunit_fail_assert, \
> + kunit_fail_assert_format, \
> + {}, \
> + fmt, \
> + ##__VA_ARGS__)
>
> /**
> * KUNIT_FAIL() - Always causes a test to fail when evaluated.
> @@ -523,15 +520,19 @@ void kunit_do_failed_assertion(struct kunit *test,
> expected_true, \
> fmt, \
> ...) \
> - KUNIT_ASSERTION(test, \
> - assert_type, \
> - !!(condition) == !!expected_true, \
> - kunit_unary_assert, \
> - kunit_unary_assert_format, \
> - KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
> - expected_true), \
> - fmt, \
> - ##__VA_ARGS__)
> +do { \
> + if (likely(!!(condition) == !!expected_true)) \
> + break; \
> + \
> + _KUNIT_FAILED(test, \
> + assert_type, \
> + kunit_unary_assert, \
> + kunit_unary_assert_format, \
> + KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
> + expected_true), \
> + fmt, \
> + ##__VA_ARGS__); \
> +} while (0)
>
> #define KUNIT_TRUE_MSG_ASSERTION(test, assert_type, condition, fmt, ...) \
> KUNIT_UNARY_ASSERTION(test, \
> @@ -581,16 +582,18 @@ do { \
> .right_text = #right, \
> }; \
> \
> - KUNIT_ASSERTION(test, \
> - assert_type, \
> - __left op __right, \
> - assert_class, \
> - format_func, \
> - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
> - __left, \
> - __right), \
> - fmt, \
> - ##__VA_ARGS__); \
> + if (likely(__left op __right)) \
> + break; \
> + \
> + _KUNIT_FAILED(test, \
> + assert_type, \
> + assert_class, \
> + format_func, \
> + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
> + __left, \
> + __right), \
> + fmt, \
> + ##__VA_ARGS__); \
> } while (0)
>
> #define KUNIT_BINARY_INT_ASSERTION(test, \
> @@ -639,16 +642,19 @@ do { \
> .right_text = #right, \
> }; \
> \
> - KUNIT_ASSERTION(test, \
> - assert_type, \
> - strcmp(__left, __right) op 0, \
> - kunit_binary_str_assert, \
> - kunit_binary_str_assert_format, \
> - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
> - __left, \
> - __right), \
> - fmt, \
> - ##__VA_ARGS__); \
> + if (likely(strcmp(__left, __right) op 0)) \
> + break; \
> + \
> + \
> + _KUNIT_FAILED(test, \
> + assert_type, \
> + kunit_binary_str_assert, \
> + kunit_binary_str_assert_format, \
> + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
> + __left, \
> + __right), \
> + fmt, \
> + ##__VA_ARGS__); \
> } while (0)
>
> #define KUNIT_PTR_NOT_ERR_OR_NULL_MSG_ASSERTION(test, \
> @@ -659,15 +665,16 @@ do { \
> do { \
> const typeof(ptr) __ptr = (ptr); \
> \
> - KUNIT_ASSERTION(test, \
> - assert_type, \
> - !IS_ERR_OR_NULL(__ptr), \
> - kunit_ptr_not_err_assert, \
> - kunit_ptr_not_err_assert_format, \
> - KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \
> - __ptr), \
> - fmt, \
> - ##__VA_ARGS__); \
> + if (!IS_ERR_OR_NULL(__ptr)) \
> + break; \
> + \
> + _KUNIT_FAILED(test, \
> + assert_type, \
> + kunit_ptr_not_err_assert, \
> + kunit_ptr_not_err_assert_format, \
> + KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, __ptr), \
> + fmt, \
> + ##__VA_ARGS__); \
> } while (0)
>
> /**
> --
> 2.38.0.rc1.362.ged0d419d3c-goog
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20221001002638.2881842-3-dlatypov%40google.com.
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/4] kunit: rename base KUNIT_ASSERTION macro to _KUNIT_FAILED
2022-10-01 3:26 ` David Gow
@ 2022-10-01 3:50 ` Daniel Latypov
2022-10-01 4:13 ` David Gow
0 siblings, 1 reply; 19+ messages in thread
From: Daniel Latypov @ 2022-10-01 3:50 UTC (permalink / raw)
To: David Gow
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Miguel Ojeda
On Fri, Sep 30, 2022 at 8:26 PM David Gow <davidgow@google.com> wrote:
>
> On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development
> <kunit-dev@googlegroups.com> wrote:
> >
> > Context:
> > Currently this macro's name, KUNIT_ASSERTION conflicts with the name of
> > an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}.
> >
> > It's hard to think of a better name for the enum, so rename this macro.
> > It's also a bit strange that the macro might do nothing depending on the
> > boolean argument `pass`. Why not have callers check themselves?
> >
> > This patch:
> > Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now
> > we only call it when the check has failed.
> > Then we rename the macro the _KUNIT_FAILED() to reflect the new
> > semantics.
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> Looks good to me. I can't say the name _KUNIT_FAILED() feels perfect
> to me, but I can't think of anything better, either. We've not used a
> leading underscore for internal macros much thus far, as well, though
> I've no personal objections to starting.
Yeah, I also didn't add a leading underscore on the new
KUNIT_INIT_ASSERT() macro elsewhere in this series.
So I'm not necessarily proposing that we should start doing so here.
It feels like that KUNIT_FAILED is far too similar to the enum
55 enum kunit_status {
56 KUNIT_SUCCESS,
57 KUNIT_FAILURE,
58 KUNIT_SKIPPED,
59 };
I.e. we'd be remove one naming conflict between a macro and enum, but
basically introducing a new one in its place :P
Tbh, I was originally going to have this patch just be
s/KUNIT_ASSERTION()/_KUNIT_ASSERTION() to reduce the conflict.
But I figured we could reduce the number of arguments to the macro
(drop `pass`) and have a reason to give it a different name.
I'm also not entirely convinced about _KUNIT_FAILED(), but I haven't
had any significantly better ideas since I sent the RFC in May.
Daniel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/4] kunit: rename base KUNIT_ASSERTION macro to _KUNIT_FAILED
2022-10-01 3:50 ` Daniel Latypov
@ 2022-10-01 4:13 ` David Gow
0 siblings, 0 replies; 19+ messages in thread
From: David Gow @ 2022-10-01 4:13 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Miguel Ojeda
[-- Attachment #1: Type: text/plain, Size: 2859 bytes --]
On Sat, Oct 1, 2022 at 11:50 AM 'Daniel Latypov' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> On Fri, Sep 30, 2022 at 8:26 PM David Gow <davidgow@google.com> wrote:
> >
> > On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development
> > <kunit-dev@googlegroups.com> wrote:
> > >
> > > Context:
> > > Currently this macro's name, KUNIT_ASSERTION conflicts with the name of
> > > an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}.
> > >
> > > It's hard to think of a better name for the enum, so rename this macro.
> > > It's also a bit strange that the macro might do nothing depending on the
> > > boolean argument `pass`. Why not have callers check themselves?
> > >
> > > This patch:
> > > Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now
> > > we only call it when the check has failed.
> > > Then we rename the macro the _KUNIT_FAILED() to reflect the new
> > > semantics.
> > >
> > > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > > ---
> >
> > Looks good to me. I can't say the name _KUNIT_FAILED() feels perfect
> > to me, but I can't think of anything better, either. We've not used a
> > leading underscore for internal macros much thus far, as well, though
> > I've no personal objections to starting.
>
> Yeah, I also didn't add a leading underscore on the new
> KUNIT_INIT_ASSERT() macro elsewhere in this series.
> So I'm not necessarily proposing that we should start doing so here.
Yeah: I noticed that. In general, I think I come down slightly on the
side of avoiding leading underscores. (And there's also the debate
about whether to have one or two, as while two underscores is
nominally "reserved for the system", the kernel uses it a lot --
probably because it considers itself "the system"). So I'd tend to
lean away from making all of our "internal" macros start with
underscores.
>
> It feels like that KUNIT_FAILED is far too similar to the enum
> 55 enum kunit_status {
> 56 KUNIT_SUCCESS,
> 57 KUNIT_FAILURE,
> 58 KUNIT_SKIPPED,
> 59 };
>
> I.e. we'd be remove one naming conflict between a macro and enum, but
> basically introducing a new one in its place :P
> Tbh, I was originally going to have this patch just be
> s/KUNIT_ASSERTION()/_KUNIT_ASSERTION() to reduce the conflict.
> But I figured we could reduce the number of arguments to the macro
> (drop `pass`) and have a reason to give it a different name.
>
> I'm also not entirely convinced about _KUNIT_FAILED(), but I haven't
> had any significantly better ideas since I sent the RFC in May.
Agreed: particularly since SKIPPED would seem to pair better with
FAILED than FAILURE, so they conflict quite a bit.
Let's stick with what we have in this change, and we can change it
later if someone comes up with a drastically better name.
Cheers,
-- David
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/4] kunit: eliminate KUNIT_INIT_*_ASSERT_STRUCT macros
2022-10-01 0:26 [PATCH 0/4] kunit: more assertion reworking Daniel Latypov
2022-10-01 0:26 ` [PATCH 1/4] kunit: remove format func from struct kunit_assert, get it to 0 bytes Daniel Latypov
2022-10-01 0:26 ` [PATCH 2/4] kunit: rename base KUNIT_ASSERTION macro to _KUNIT_FAILED Daniel Latypov
@ 2022-10-01 0:26 ` Daniel Latypov
2022-10-01 3:26 ` David Gow
2022-10-01 10:12 ` Miguel Ojeda
2022-10-01 0:26 ` [PATCH 4/4] kunit: declare kunit_assert structs as const Daniel Latypov
2022-10-01 10:15 ` [PATCH 0/4] kunit: more assertion reworking Miguel Ojeda
4 siblings, 2 replies; 19+ messages in thread
From: Daniel Latypov @ 2022-10-01 0:26 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan,
miguel.ojeda.sandonis, Daniel Latypov
These macros exist because passing an initializer list to other macros
is hard.
The goal of these macros is to generate a line like
struct $ASSERT_TYPE __assertion = $APPROPRIATE_INITIALIZER;
e.g.
struct kunit_unary_assertion __assertion = {
.condition = "foo()",
.expected_true = true
};
But the challenge is you can't pass `{.condition=..., .expect_true=...}`
as a macro argument, since the comma means you're actually passing two
arguments, `{.condition=...` and `.expect_true=....}`.
So we'd made custom macros for each different initializer-list shape.
But we can work around this with the following generic macro
#define KUNIT_INIT_ASSERT(initializers...) { initializers }
Note: this has the downside that we have to rename some macros arguments
to not conflict with the struct field names (e.g. `expected_true`).
It's a bit gross, but probably worth reducing the # of macros.
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
include/kunit/assert.h | 48 ------------------------------------------
include/kunit/test.h | 27 +++++++++++++-----------
2 files changed, 15 insertions(+), 60 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index ace3de8d1ee7..01b191fa17c3 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -90,19 +90,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
const struct va_format *message,
struct string_stream *stream);
-/**
- * KUNIT_INIT_UNARY_ASSERT_STRUCT() - Initializes &struct kunit_unary_assert.
- * @cond: A string representation of the expression asserted true or false.
- * @expect_true: True if of type KUNIT_{EXPECT|ASSERT}_TRUE, false otherwise.
- *
- * Initializes a &struct kunit_unary_assert. Intended to be used in
- * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
- */
-#define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \
- .condition = cond, \
- .expected_true = expect_true \
-}
-
/**
* struct kunit_ptr_not_err_assert - An expectation/assertion that a pointer is
* not NULL and not a -errno.
@@ -123,20 +110,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
const struct va_format *message,
struct string_stream *stream);
-/**
- * KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
- * &struct kunit_ptr_not_err_assert.
- * @txt: A string representation of the expression passed to the expectation.
- * @val: The actual evaluated pointer value of the expression.
- *
- * Initializes a &struct kunit_ptr_not_err_assert. Intended to be used in
- * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
- */
-#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \
- .text = txt, \
- .value = val \
-}
-
/**
* struct kunit_binary_assert_text - holds strings for &struct
* kunit_binary_assert and friends to try and make the structs smaller.
@@ -173,27 +146,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
const struct va_format *message,
struct string_stream *stream);
-/**
- * KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a binary assert like
- * kunit_binary_assert, kunit_binary_ptr_assert, etc.
- *
- * @text_: Pointer to a kunit_binary_assert_text.
- * @left_val: The actual evaluated value of the expression in the left slot.
- * @right_val: The actual evaluated value of the expression in the right slot.
- *
- * 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(text_, \
- left_val, \
- right_val) { \
- .text = text_, \
- .left_value = left_val, \
- .right_value = right_val \
-}
-
/**
* struct kunit_binary_ptr_assert - An expectation/assertion that compares two
* pointer values (for example, KUNIT_EXPECT_PTR_EQ(test, foo, bar)).
diff --git a/include/kunit/test.h b/include/kunit/test.h
index fec437c8a2b7..e49348bbc6ee 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -514,22 +514,25 @@ void kunit_do_failed_assertion(struct kunit *test,
fmt, \
##__VA_ARGS__)
+/* Helper to safely pass around an initializer list to other macros. */
+#define KUNIT_INIT_ASSERT(initializers...) { initializers }
+
#define KUNIT_UNARY_ASSERTION(test, \
assert_type, \
- condition, \
- expected_true, \
+ condition_, \
+ expected_true_, \
fmt, \
...) \
do { \
- if (likely(!!(condition) == !!expected_true)) \
+ if (likely(!!(condition_) == !!expected_true_)) \
break; \
\
_KUNIT_FAILED(test, \
assert_type, \
kunit_unary_assert, \
kunit_unary_assert_format, \
- KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
- expected_true), \
+ KUNIT_INIT_ASSERT(.condition = #condition_, \
+ .expected_true = expected_true_), \
fmt, \
##__VA_ARGS__); \
} while (0)
@@ -589,9 +592,9 @@ do { \
assert_type, \
assert_class, \
format_func, \
- KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
- __left, \
- __right), \
+ KUNIT_INIT_ASSERT(.text = &__text, \
+ .left_value = __left, \
+ .right_value = __right), \
fmt, \
##__VA_ARGS__); \
} while (0)
@@ -650,9 +653,9 @@ do { \
assert_type, \
kunit_binary_str_assert, \
kunit_binary_str_assert_format, \
- KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
- __left, \
- __right), \
+ KUNIT_INIT_ASSERT(.text = &__text, \
+ .left_value = __left, \
+ .right_value = __right), \
fmt, \
##__VA_ARGS__); \
} while (0)
@@ -672,7 +675,7 @@ do { \
assert_type, \
kunit_ptr_not_err_assert, \
kunit_ptr_not_err_assert_format, \
- KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, __ptr), \
+ KUNIT_INIT_ASSERT(.text = #ptr, .value = __ptr), \
fmt, \
##__VA_ARGS__); \
} while (0)
--
2.38.0.rc1.362.ged0d419d3c-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] kunit: eliminate KUNIT_INIT_*_ASSERT_STRUCT macros
2022-10-01 0:26 ` [PATCH 3/4] kunit: eliminate KUNIT_INIT_*_ASSERT_STRUCT macros Daniel Latypov
@ 2022-10-01 3:26 ` David Gow
2022-10-01 10:12 ` Miguel Ojeda
1 sibling, 0 replies; 19+ messages in thread
From: David Gow @ 2022-10-01 3:26 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Miguel Ojeda
[-- Attachment #1: Type: text/plain, Size: 10435 bytes --]
On Sat, Oct 1, 2022 at 8:26 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> These macros exist because passing an initializer list to other macros
> is hard.
>
> The goal of these macros is to generate a line like
> struct $ASSERT_TYPE __assertion = $APPROPRIATE_INITIALIZER;
> e.g.
> struct kunit_unary_assertion __assertion = {
> .condition = "foo()",
> .expected_true = true
> };
>
> But the challenge is you can't pass `{.condition=..., .expect_true=...}`
> as a macro argument, since the comma means you're actually passing two
> arguments, `{.condition=...` and `.expect_true=....}`.
> So we'd made custom macros for each different initializer-list shape.
>
> But we can work around this with the following generic macro
> #define KUNIT_INIT_ASSERT(initializers...) { initializers }
>
> Note: this has the downside that we have to rename some macros arguments
> to not conflict with the struct field names (e.g. `expected_true`).
> It's a bit gross, but probably worth reducing the # of macros.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
I agree with you on both fronts here: this is 'a bit gross', and is
also 'worth it'.
Reviewed-by: David Gow <davidgow@google.com>
Cheers,
-- David
> include/kunit/assert.h | 48 ------------------------------------------
> include/kunit/test.h | 27 +++++++++++++-----------
> 2 files changed, 15 insertions(+), 60 deletions(-)
>
> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> index ace3de8d1ee7..01b191fa17c3 100644
> --- a/include/kunit/assert.h
> +++ b/include/kunit/assert.h
> @@ -90,19 +90,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
> const struct va_format *message,
> struct string_stream *stream);
>
> -/**
> - * KUNIT_INIT_UNARY_ASSERT_STRUCT() - Initializes &struct kunit_unary_assert.
> - * @cond: A string representation of the expression asserted true or false.
> - * @expect_true: True if of type KUNIT_{EXPECT|ASSERT}_TRUE, false otherwise.
> - *
> - * Initializes a &struct kunit_unary_assert. Intended to be used in
> - * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
> - */
> -#define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \
> - .condition = cond, \
> - .expected_true = expect_true \
> -}
> -
> /**
> * struct kunit_ptr_not_err_assert - An expectation/assertion that a pointer is
> * not NULL and not a -errno.
> @@ -123,20 +110,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
> const struct va_format *message,
> struct string_stream *stream);
>
> -/**
> - * KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
> - * &struct kunit_ptr_not_err_assert.
> - * @txt: A string representation of the expression passed to the expectation.
> - * @val: The actual evaluated pointer value of the expression.
> - *
> - * Initializes a &struct kunit_ptr_not_err_assert. Intended to be used in
> - * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
> - */
> -#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \
> - .text = txt, \
> - .value = val \
> -}
> -
> /**
> * struct kunit_binary_assert_text - holds strings for &struct
> * kunit_binary_assert and friends to try and make the structs smaller.
> @@ -173,27 +146,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
> const struct va_format *message,
> struct string_stream *stream);
>
> -/**
> - * KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a binary assert like
> - * kunit_binary_assert, kunit_binary_ptr_assert, etc.
> - *
> - * @text_: Pointer to a kunit_binary_assert_text.
> - * @left_val: The actual evaluated value of the expression in the left slot.
> - * @right_val: The actual evaluated value of the expression in the right slot.
> - *
> - * 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(text_, \
> - left_val, \
> - right_val) { \
> - .text = text_, \
> - .left_value = left_val, \
> - .right_value = right_val \
> -}
> -
> /**
> * struct kunit_binary_ptr_assert - An expectation/assertion that compares two
> * pointer values (for example, KUNIT_EXPECT_PTR_EQ(test, foo, bar)).
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index fec437c8a2b7..e49348bbc6ee 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -514,22 +514,25 @@ void kunit_do_failed_assertion(struct kunit *test,
> fmt, \
> ##__VA_ARGS__)
>
> +/* Helper to safely pass around an initializer list to other macros. */
> +#define KUNIT_INIT_ASSERT(initializers...) { initializers }
> +
> #define KUNIT_UNARY_ASSERTION(test, \
> assert_type, \
> - condition, \
> - expected_true, \
> + condition_, \
> + expected_true_, \
> fmt, \
> ...) \
> do { \
> - if (likely(!!(condition) == !!expected_true)) \
> + if (likely(!!(condition_) == !!expected_true_)) \
> break; \
> \
> _KUNIT_FAILED(test, \
> assert_type, \
> kunit_unary_assert, \
> kunit_unary_assert_format, \
> - KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
> - expected_true), \
> + KUNIT_INIT_ASSERT(.condition = #condition_, \
> + .expected_true = expected_true_), \
> fmt, \
> ##__VA_ARGS__); \
> } while (0)
> @@ -589,9 +592,9 @@ do { \
> assert_type, \
> assert_class, \
> format_func, \
> - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
> - __left, \
> - __right), \
> + KUNIT_INIT_ASSERT(.text = &__text, \
> + .left_value = __left, \
> + .right_value = __right), \
> fmt, \
> ##__VA_ARGS__); \
> } while (0)
> @@ -650,9 +653,9 @@ do { \
> assert_type, \
> kunit_binary_str_assert, \
> kunit_binary_str_assert_format, \
> - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \
> - __left, \
> - __right), \
> + KUNIT_INIT_ASSERT(.text = &__text, \
> + .left_value = __left, \
> + .right_value = __right), \
> fmt, \
> ##__VA_ARGS__); \
> } while (0)
> @@ -672,7 +675,7 @@ do { \
> assert_type, \
> kunit_ptr_not_err_assert, \
> kunit_ptr_not_err_assert_format, \
> - KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, __ptr), \
> + KUNIT_INIT_ASSERT(.text = #ptr, .value = __ptr), \
> fmt, \
> ##__VA_ARGS__); \
> } while (0)
> --
> 2.38.0.rc1.362.ged0d419d3c-goog
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] kunit: eliminate KUNIT_INIT_*_ASSERT_STRUCT macros
2022-10-01 0:26 ` [PATCH 3/4] kunit: eliminate KUNIT_INIT_*_ASSERT_STRUCT macros Daniel Latypov
2022-10-01 3:26 ` David Gow
@ 2022-10-01 10:12 ` Miguel Ojeda
2022-10-01 17:48 ` Daniel Latypov
1 sibling, 1 reply; 19+ messages in thread
From: Miguel Ojeda @ 2022-10-01 10:12 UTC (permalink / raw)
To: Daniel Latypov
Cc: brendanhiggins, davidgow, linux-kernel, kunit-dev,
linux-kselftest, skhan
On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> But we can work around this with the following generic macro
> #define KUNIT_INIT_ASSERT(initializers...) { initializers }
Is it intended to be internal, right? Should be prefixed by `_` then?
Cheers,
Miguel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] kunit: eliminate KUNIT_INIT_*_ASSERT_STRUCT macros
2022-10-01 10:12 ` Miguel Ojeda
@ 2022-10-01 17:48 ` Daniel Latypov
0 siblings, 0 replies; 19+ messages in thread
From: Daniel Latypov @ 2022-10-01 17:48 UTC (permalink / raw)
To: Miguel Ojeda
Cc: brendanhiggins, davidgow, linux-kernel, kunit-dev,
linux-kselftest, skhan
On Sat, Oct 1, 2022 at 3:12 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > But we can work around this with the following generic macro
> > #define KUNIT_INIT_ASSERT(initializers...) { initializers }
>
> Is it intended to be internal, right? Should be prefixed by `_` then?
Yeah, 100% internal.
We don't have such a convention in KUnit yet, see the discussion in
https://lore.kernel.org/linux-kselftest/CABVgOSmcheQvBRKqc-0ftmbthx=EReoQ-910QV0QMNuxLWjTUQ@mail.gmail.com/T/#u
I'd be personally fine with _s, but this patch just tried to keep
things consistent with what was there before.
Daniel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/4] kunit: declare kunit_assert structs as const
2022-10-01 0:26 [PATCH 0/4] kunit: more assertion reworking Daniel Latypov
` (2 preceding siblings ...)
2022-10-01 0:26 ` [PATCH 3/4] kunit: eliminate KUNIT_INIT_*_ASSERT_STRUCT macros Daniel Latypov
@ 2022-10-01 0:26 ` Daniel Latypov
2022-10-01 3:26 ` David Gow
2022-10-01 10:06 ` Miguel Ojeda
2022-10-01 10:15 ` [PATCH 0/4] kunit: more assertion reworking Miguel Ojeda
4 siblings, 2 replies; 19+ messages in thread
From: Daniel Latypov @ 2022-10-01 0:26 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan,
miguel.ojeda.sandonis, Daniel Latypov
Everywhere we use the assert structs now takes them via const*, as of
commit 7466886b400b ("kunit: take `kunit_assert` as `const`").
So now let's properly declare the structs as const as well.
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
include/kunit/test.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index e49348bbc6ee..d574c871dd9f 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -477,7 +477,7 @@ void kunit_do_failed_assertion(struct kunit *test,
#define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
- struct assert_class __assertion = INITIALIZER; \
+ const struct assert_class __assertion = INITIALIZER; \
kunit_do_failed_assertion(test, \
&__loc, \
assert_type, \
--
2.38.0.rc1.362.ged0d419d3c-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/4] kunit: declare kunit_assert structs as const
2022-10-01 0:26 ` [PATCH 4/4] kunit: declare kunit_assert structs as const Daniel Latypov
@ 2022-10-01 3:26 ` David Gow
2022-10-01 10:06 ` Miguel Ojeda
1 sibling, 0 replies; 19+ messages in thread
From: David Gow @ 2022-10-01 3:26 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Miguel Ojeda
[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]
On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> Everywhere we use the assert structs now takes them via const*, as of
> commit 7466886b400b ("kunit: take `kunit_assert` as `const`").
>
> So now let's properly declare the structs as const as well.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
Thanks!
Reviewed-by: David Gow <davidgow@google.com>
Cheers,
-- David
> include/kunit/test.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index e49348bbc6ee..d574c871dd9f 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -477,7 +477,7 @@ void kunit_do_failed_assertion(struct kunit *test,
>
> #define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
> static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
> - struct assert_class __assertion = INITIALIZER; \
> + const struct assert_class __assertion = INITIALIZER; \
> kunit_do_failed_assertion(test, \
> &__loc, \
> assert_type, \
> --
> 2.38.0.rc1.362.ged0d419d3c-goog
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20221001002638.2881842-5-dlatypov%40google.com.
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/4] kunit: declare kunit_assert structs as const
2022-10-01 0:26 ` [PATCH 4/4] kunit: declare kunit_assert structs as const Daniel Latypov
2022-10-01 3:26 ` David Gow
@ 2022-10-01 10:06 ` Miguel Ojeda
1 sibling, 0 replies; 19+ messages in thread
From: Miguel Ojeda @ 2022-10-01 10:06 UTC (permalink / raw)
To: Daniel Latypov
Cc: brendanhiggins, davidgow, linux-kernel, kunit-dev,
linux-kselftest, skhan
On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> Everywhere we use the assert structs now takes them via const*, as of
> commit 7466886b400b ("kunit: take `kunit_assert` as `const`").
>
> So now let's properly declare the structs as const as well.
Always good to be `const`-correct :)
Reviewed-by: Miguel Ojeda <ojeda@kernel.org>
Cheers,
Miguel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] kunit: more assertion reworking
2022-10-01 0:26 [PATCH 0/4] kunit: more assertion reworking Daniel Latypov
` (3 preceding siblings ...)
2022-10-01 0:26 ` [PATCH 4/4] kunit: declare kunit_assert structs as const Daniel Latypov
@ 2022-10-01 10:15 ` Miguel Ojeda
2022-10-01 18:00 ` Daniel Latypov
4 siblings, 1 reply; 19+ messages in thread
From: Miguel Ojeda @ 2022-10-01 10:15 UTC (permalink / raw)
To: Daniel Latypov
Cc: brendanhiggins, davidgow, linux-kernel, kunit-dev,
linux-kselftest, skhan
On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> Note: this does change the function signature of
> kunit_do_failed_assertion, so we'd need to update the rust wrapper in
> https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/kunit.rs,
> but hopefully it's just a simple change, e.g. maybe just like:
Yeah, should be simple. Thanks for pointing it out!
The series looks like a great cleanup on top of the stack reduction.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 0/4] kunit: more assertion reworking
2022-10-01 10:15 ` [PATCH 0/4] kunit: more assertion reworking Miguel Ojeda
@ 2022-10-01 18:00 ` Daniel Latypov
2022-10-18 23:20 ` Miguel Ojeda
0 siblings, 1 reply; 19+ messages in thread
From: Daniel Latypov @ 2022-10-01 18:00 UTC (permalink / raw)
To: Miguel Ojeda
Cc: brendanhiggins, davidgow, linux-kernel, kunit-dev,
linux-kselftest, skhan
On Sat, Oct 1, 2022 at 3:15 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > Note: this does change the function signature of
> > kunit_do_failed_assertion, so we'd need to update the rust wrapper in
> > https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/kunit.rs,
> > but hopefully it's just a simple change, e.g. maybe just like:
>
> Yeah, should be simple. Thanks for pointing it out!
>
> The series looks like a great cleanup on top of the stack reduction.
Thanks for taking a look at the rest of the series as well.
While I have you here, any thoughts on how to coordinate the change?
I made the breaking change patch#1 so it should be easier to pull out.
One option I was thinking was:
* wait till this lands in Shuah's tree
* I create a Github PR that contains patch#1 + a patch for kunit.rs
I was not clear on how the RfL Github pulls in upstream changes or how often.
But my assumption is patch#1 would fall away naturally if rebasing
onto 6.1 (and maybe we can squash the kunit.rs change).
Thanks,
Daniel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] kunit: more assertion reworking
2022-10-01 18:00 ` Daniel Latypov
@ 2022-10-18 23:20 ` Miguel Ojeda
2022-10-18 23:26 ` Daniel Latypov
0 siblings, 1 reply; 19+ messages in thread
From: Miguel Ojeda @ 2022-10-18 23:20 UTC (permalink / raw)
To: Daniel Latypov
Cc: brendanhiggins, davidgow, linux-kernel, kunit-dev,
linux-kselftest, skhan
On Sat, Oct 1, 2022 at 8:00 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> While I have you here, any thoughts on how to coordinate the change?
My bad, I forgot to reply to this, sorry. I noticed it again when
merging 6.1-rc1 into our branch.
Normally I fix the issues when I merge back from Linus. Since we
intend to keep the CI green on every PR, I added the fix for this in
the merge itself.
In any case, to clarify, the `rust` branch in GitHub is just where we
placed a lot of things that we wanted to eventually submit (and some
that should not, e.g. the GitHub CI files). We will be minimizing the
differences w.r.t. upstream in that branch by preparing proper patches
from scratch and submitting them through `rust-next` and other trees,
and eventually remove it (or reusing the name for the fixes branch
since that is the name other trees use, but we will see).
Cheers,
Miguel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] kunit: more assertion reworking
2022-10-18 23:20 ` Miguel Ojeda
@ 2022-10-18 23:26 ` Daniel Latypov
2022-10-18 23:39 ` Miguel Ojeda
0 siblings, 1 reply; 19+ messages in thread
From: Daniel Latypov @ 2022-10-18 23:26 UTC (permalink / raw)
To: Miguel Ojeda
Cc: brendanhiggins, davidgow, linux-kernel, kunit-dev,
linux-kselftest, skhan
On Tue, Oct 18, 2022 at 4:20 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sat, Oct 1, 2022 at 8:00 PM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > While I have you here, any thoughts on how to coordinate the change?
>
> My bad, I forgot to reply to this, sorry. I noticed it again when
> merging 6.1-rc1 into our branch.
No worries.
You've had a very busy couple of weeks, I imagine.
>
> Normally I fix the issues when I merge back from Linus. Since we
> intend to keep the CI green on every PR, I added the fix for this in
> the merge itself.
Thanks!
Daniel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] kunit: more assertion reworking
2022-10-18 23:26 ` Daniel Latypov
@ 2022-10-18 23:39 ` Miguel Ojeda
0 siblings, 0 replies; 19+ messages in thread
From: Miguel Ojeda @ 2022-10-18 23:39 UTC (permalink / raw)
To: Daniel Latypov
Cc: brendanhiggins, davidgow, linux-kernel, kunit-dev,
linux-kselftest, skhan
On Wed, Oct 19, 2022 at 1:26 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> No worries.
> You've had a very busy couple of weeks, I imagine.
Just a bit :) Nevertheless, it was my intention to reply :(
I have linked this thread from the PR noting that you warned me about
the future conflict [1], thanks again!
[1] https://github.com/Rust-for-Linux/linux/pull/915#issuecomment-1283138279
Cheers,
Miguel
^ permalink raw reply [flat|nested] 19+ messages in thread