[PATCH 0/6] kunit: refactor assertions to use less stack
3 views
Skip to first unread message
Daniel Latypov
Jan 7, 2022, 8:23:24 PM1/7/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
Every KUNIT_ASSERT/EXPECT() invocation puts a `kunit_assert` object onto
the stack. The most common one is `kunit_binary_assert` which is 88
bytes on UML. So in the cases where the compiler doesn't optimize this
away, we can very quickly blow up the stack size.
This series implements Linus' suggestion in [1].
Namely, we split out the file, line number, and assert_type
(EXPECT/ASSERT) out of kunit_assert.
We can also drop the entirely unused `struct kunit *test` field, saving
a bit more space as well.
All together, sizeof(struct kunit_assert) went from 48 to 24 on UML.
Note: the other assert types are bigger, see [2].
This series also adds in an example test that uses all the KUNIT_EXPECT
macros to both advertise their existence to new users and serve as a
smoketest for all these changes here.
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
[2] e.g. consider the most commonly used assert (also the biggest)
struct kunit_binary_assert {
struct kunit_assert assert;
const char *operation;
const char *left_text;
long long left_value;
const char *right_text;
long long right_value;
};
So sizeof(struct kunit_binary_assert) = went from 88 to 64.
I.e. only a 27% reduction instead of 50% in the most common case.
All 3 of the `const char*` could be split out into a `static` var as well,
but that's a bit trickier to do with how all the macros are written.
Daniel Latypov (6):
kunit: add example test case showing off all the expect macros
kunit: move check if assertion passed into the macros
kunit: drop unused kunit* field in kunit_assert
kunit: factor out kunit_base_assert_format() call into kunit_fail()
kunit: split out part of kunit_assert into a static const
kunit: drop unused assert_type from kunit_assert and clean up macros
include/kunit/assert.h | 88 +++++++++++-----------------------
include/kunit/test.h | 52 ++++++++++----------
lib/kunit/assert.c | 15 ++----
lib/kunit/kunit-example-test.c | 46 ++++++++++++++++++
lib/kunit/test.c | 27 +++++------
5 files changed, 120 insertions(+), 108 deletions(-)
base-commit: ad659ccb5412874c6a89d3588cb18857c00e9d0f
--
2.34.1.575.g55b058a8bb-goog
the stack. The most common one is `kunit_binary_assert` which is 88
bytes on UML. So in the cases where the compiler doesn't optimize this
away, we can very quickly blow up the stack size.
This series implements Linus' suggestion in [1].
Namely, we split out the file, line number, and assert_type
(EXPECT/ASSERT) out of kunit_assert.
We can also drop the entirely unused `struct kunit *test` field, saving
a bit more space as well.
All together, sizeof(struct kunit_assert) went from 48 to 24 on UML.
Note: the other assert types are bigger, see [2].
This series also adds in an example test that uses all the KUNIT_EXPECT
macros to both advertise their existence to new users and serve as a
smoketest for all these changes here.
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
[2] e.g. consider the most commonly used assert (also the biggest)
struct kunit_binary_assert {
struct kunit_assert assert;
const char *operation;
const char *left_text;
long long left_value;
const char *right_text;
long long right_value;
};
So sizeof(struct kunit_binary_assert) = went from 88 to 64.
I.e. only a 27% reduction instead of 50% in the most common case.
All 3 of the `const char*` could be split out into a `static` var as well,
but that's a bit trickier to do with how all the macros are written.
Daniel Latypov (6):
kunit: add example test case showing off all the expect macros
kunit: move check if assertion passed into the macros
kunit: drop unused kunit* field in kunit_assert
kunit: factor out kunit_base_assert_format() call into kunit_fail()
kunit: split out part of kunit_assert into a static const
kunit: drop unused assert_type from kunit_assert and clean up macros
include/kunit/assert.h | 88 +++++++++++-----------------------
include/kunit/test.h | 52 ++++++++++----------
lib/kunit/assert.c | 15 ++----
lib/kunit/kunit-example-test.c | 46 ++++++++++++++++++
lib/kunit/test.c | 27 +++++------
5 files changed, 120 insertions(+), 108 deletions(-)
base-commit: ad659ccb5412874c6a89d3588cb18857c00e9d0f
--
2.34.1.575.g55b058a8bb-goog
Daniel Latypov
Jan 7, 2022, 8:23:27 PM1/7/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
Currently, these macros are only really documented near the bottom of
https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
E.g. it's likely someone might just not realize that
KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
or similar.
This can also serve as a basic smoketest that the KUnit assert machinery
still works for all the macros.
Signed-off-by: Daniel Latypov <dlat...@google.com>
---
lib/kunit/kunit-example-test.c | 46 ++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
index 51099b0ca29c..182a64c12541 100644
--- a/lib/kunit/kunit-example-test.c
+++ b/lib/kunit/kunit-example-test.c
@@ -69,6 +69,51 @@ static void example_mark_skipped_test(struct kunit *test)
/* This line should run */
kunit_info(test, "You should see this line.");
}
+
+/*
+ * This test shows off all the KUNIT_EXPECT macros.
+ */
+static void example_all_expect_macros_test(struct kunit *test)
+{
+ KUNIT_EXPECT_TRUE(test, true);
+ KUNIT_EXPECT_FALSE(test, false);
+
+ KUNIT_EXPECT_EQ(test, 1, 1);
+ KUNIT_EXPECT_GE(test, 1, 1);
+ KUNIT_EXPECT_LE(test, 1, 1);
+ KUNIT_EXPECT_NE(test, 1, 0);
+ KUNIT_EXPECT_GT(test, 1, 0);
+ KUNIT_EXPECT_LT(test, 0, 1);
+
+ KUNIT_EXPECT_NOT_ERR_OR_NULL(test, test);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, NULL);
+ KUNIT_EXPECT_PTR_NE(test, test, NULL);
+
+ KUNIT_EXPECT_STREQ(test, "hi", "hi");
+ KUNIT_EXPECT_STRNEQ(test, "hi", "bye");
+
+ /*
+ * There are also _MSG variants of all of the above that let you include
+ * additional text on failure.
+ */
+ KUNIT_EXPECT_TRUE_MSG(test, true, "msg");
+ KUNIT_EXPECT_FALSE_MSG(test, false, "msg");
+
+ KUNIT_EXPECT_EQ_MSG(test, 1, 1, "msg");
+ KUNIT_EXPECT_GE_MSG(test, 1, 1, "msg");
+ KUNIT_EXPECT_LE_MSG(test, 1, 1, "msg");
+ KUNIT_EXPECT_NE_MSG(test, 1, 0, "msg");
+ KUNIT_EXPECT_GT_MSG(test, 1, 0, "msg");
+ KUNIT_EXPECT_LT_MSG(test, 0, 1, "msg");
+
+ KUNIT_EXPECT_NOT_ERR_OR_NULL_MSG(test, test, "msg");
+ KUNIT_EXPECT_PTR_EQ_MSG(test, NULL, NULL, "msg");
+ KUNIT_EXPECT_PTR_NE_MSG(test, test, NULL, "msg");
+
+ KUNIT_EXPECT_STREQ_MSG(test, "hi", "hi", "msg");
+ KUNIT_EXPECT_STRNEQ_MSG(test, "hi", "bye", "msg");
+}
+
/*
* Here we make a list of all the test cases we want to add to the test suite
* below.
@@ -83,6 +128,7 @@ static struct kunit_case example_test_cases[] = {
KUNIT_CASE(example_simple_test),
KUNIT_CASE(example_skip_test),
KUNIT_CASE(example_mark_skipped_test),
+ KUNIT_CASE(example_all_expect_macros_test),
{}
};
--
2.34.1.575.g55b058a8bb-goog
https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
E.g. it's likely someone might just not realize that
KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
or similar.
This can also serve as a basic smoketest that the KUnit assert machinery
still works for all the macros.
Signed-off-by: Daniel Latypov <dlat...@google.com>
---
lib/kunit/kunit-example-test.c | 46 ++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
index 51099b0ca29c..182a64c12541 100644
--- a/lib/kunit/kunit-example-test.c
+++ b/lib/kunit/kunit-example-test.c
@@ -69,6 +69,51 @@ static void example_mark_skipped_test(struct kunit *test)
/* This line should run */
kunit_info(test, "You should see this line.");
}
+
+/*
+ * This test shows off all the KUNIT_EXPECT macros.
+ */
+static void example_all_expect_macros_test(struct kunit *test)
+{
+ KUNIT_EXPECT_TRUE(test, true);
+ KUNIT_EXPECT_FALSE(test, false);
+
+ KUNIT_EXPECT_EQ(test, 1, 1);
+ KUNIT_EXPECT_GE(test, 1, 1);
+ KUNIT_EXPECT_LE(test, 1, 1);
+ KUNIT_EXPECT_NE(test, 1, 0);
+ KUNIT_EXPECT_GT(test, 1, 0);
+ KUNIT_EXPECT_LT(test, 0, 1);
+
+ KUNIT_EXPECT_NOT_ERR_OR_NULL(test, test);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, NULL);
+ KUNIT_EXPECT_PTR_NE(test, test, NULL);
+
+ KUNIT_EXPECT_STREQ(test, "hi", "hi");
+ KUNIT_EXPECT_STRNEQ(test, "hi", "bye");
+
+ /*
+ * There are also _MSG variants of all of the above that let you include
+ * additional text on failure.
+ */
+ KUNIT_EXPECT_TRUE_MSG(test, true, "msg");
+ KUNIT_EXPECT_FALSE_MSG(test, false, "msg");
+
+ KUNIT_EXPECT_EQ_MSG(test, 1, 1, "msg");
+ KUNIT_EXPECT_GE_MSG(test, 1, 1, "msg");
+ KUNIT_EXPECT_LE_MSG(test, 1, 1, "msg");
+ KUNIT_EXPECT_NE_MSG(test, 1, 0, "msg");
+ KUNIT_EXPECT_GT_MSG(test, 1, 0, "msg");
+ KUNIT_EXPECT_LT_MSG(test, 0, 1, "msg");
+
+ KUNIT_EXPECT_NOT_ERR_OR_NULL_MSG(test, test, "msg");
+ KUNIT_EXPECT_PTR_EQ_MSG(test, NULL, NULL, "msg");
+ KUNIT_EXPECT_PTR_NE_MSG(test, test, NULL, "msg");
+
+ KUNIT_EXPECT_STREQ_MSG(test, "hi", "hi", "msg");
+ KUNIT_EXPECT_STRNEQ_MSG(test, "hi", "bye", "msg");
+}
+
/*
* Here we make a list of all the test cases we want to add to the test suite
* below.
@@ -83,6 +128,7 @@ static struct kunit_case example_test_cases[] = {
KUNIT_CASE(example_simple_test),
KUNIT_CASE(example_skip_test),
KUNIT_CASE(example_mark_skipped_test),
+ KUNIT_CASE(example_all_expect_macros_test),
{}
};
--
2.34.1.575.g55b058a8bb-goog
Daniel Latypov
Jan 7, 2022, 8:23:29 PM1/7/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
Currently the code always calls kunit_do_assertion() even though it does
nothing when `pass` is true.
This change moves the `if(!(pass))` check into the macro instead
and renames the function to kunit_failed_assertion().
I feel this a bit easier to read and understand.
This has the potential upside of avoiding a function call that does
nothing most of the time (assuming your tests are passing) but comes
with the downside of generating a bit more code and branches.
This also means we don't have to initialize structs that we don't need,
which will become a tiny bit more expensive if we switch over to using
static variables to try and reduce stack usage. (There's runtime code
to check if the variable has been initialized yet or not).
include/kunit/test.h | 20 ++++++++++----------
lib/kunit/test.c | 13 ++++---------
2 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index b26400731c02..690a28dfc795 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -770,18 +770,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
*/
#define KUNIT_SUCCEED(test) do {} while (0)
-void kunit_do_assertion(struct kunit *test,
- struct kunit_assert *assert,
- bool pass,
- const char *fmt, ...);
+void kunit_failed_assertion(struct kunit *test,
+ struct kunit_assert *assert,
+ const char *fmt, ...);
#define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do { \
- struct assert_class __assertion = INITIALIZER; \
- kunit_do_assertion(test, \
- &__assertion.assert, \
- pass, \
- fmt, \
- ##__VA_ARGS__); \
+ if (!(pass)) { \
+ struct assert_class __assertion = INITIALIZER; \
+ kunit_failed_assertion(test, \
+ &__assertion.assert, \
+ fmt, \
+ ##__VA_ARGS__); \
+ } \
} while (0)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c7ed4aabec04..5ad671745483 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -275,16 +275,11 @@ static void __noreturn kunit_abort(struct kunit *test)
WARN_ONCE(true, "Throw could not abort from test!\n");
}
-void kunit_do_assertion(struct kunit *test,
- struct kunit_assert *assert,
- bool pass,
- const char *fmt, ...)
+void kunit_failed_assertion(struct kunit *test,
+ struct kunit_assert *assert,
+ const char *fmt, ...)
{
va_list args;
-
- if (pass)
- return;
-
va_start(args, fmt);
assert->message.fmt = fmt;
@@ -297,7 +292,7 @@ void kunit_do_assertion(struct kunit *test,
if (assert->type == KUNIT_ASSERTION)
kunit_abort(test);
}
-EXPORT_SYMBOL_GPL(kunit_do_assertion);
+EXPORT_SYMBOL_GPL(kunit_failed_assertion);
void kunit_init_test(struct kunit *test, const char *name, char *log)
{
--
2.34.1.575.g55b058a8bb-goog
nothing when `pass` is true.
This change moves the `if(!(pass))` check into the macro instead
and renames the function to kunit_failed_assertion().
I feel this a bit easier to read and understand.
This has the potential upside of avoiding a function call that does
nothing most of the time (assuming your tests are passing) but comes
with the downside of generating a bit more code and branches.
This also means we don't have to initialize structs that we don't need,
which will become a tiny bit more expensive if we switch over to using
static variables to try and reduce stack usage. (There's runtime code
to check if the variable has been initialized yet or not).
include/kunit/test.h | 20 ++++++++++----------
lib/kunit/test.c | 13 ++++---------
2 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index b26400731c02..690a28dfc795 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -770,18 +770,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
*/
#define KUNIT_SUCCEED(test) do {} while (0)
-void kunit_do_assertion(struct kunit *test,
- struct kunit_assert *assert,
- bool pass,
- const char *fmt, ...);
+void kunit_failed_assertion(struct kunit *test,
+ struct kunit_assert *assert,
+ const char *fmt, ...);
#define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do { \
- struct assert_class __assertion = INITIALIZER; \
- kunit_do_assertion(test, \
- &__assertion.assert, \
- pass, \
- fmt, \
- ##__VA_ARGS__); \
+ if (!(pass)) { \
+ struct assert_class __assertion = INITIALIZER; \
+ kunit_failed_assertion(test, \
+ &__assertion.assert, \
+ fmt, \
+ ##__VA_ARGS__); \
+ } \
} while (0)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c7ed4aabec04..5ad671745483 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -275,16 +275,11 @@ static void __noreturn kunit_abort(struct kunit *test)
WARN_ONCE(true, "Throw could not abort from test!\n");
}
-void kunit_do_assertion(struct kunit *test,
- struct kunit_assert *assert,
- bool pass,
- const char *fmt, ...)
+void kunit_failed_assertion(struct kunit *test,
+ struct kunit_assert *assert,
+ const char *fmt, ...)
{
va_list args;
-
- if (pass)
- return;
-
va_start(args, fmt);
assert->message.fmt = fmt;
@@ -297,7 +292,7 @@ void kunit_do_assertion(struct kunit *test,
if (assert->type == KUNIT_ASSERTION)
kunit_abort(test);
}
-EXPORT_SYMBOL_GPL(kunit_do_assertion);
+EXPORT_SYMBOL_GPL(kunit_failed_assertion);
void kunit_init_test(struct kunit *test, const char *name, char *log)
{
--
2.34.1.575.g55b058a8bb-goog
Daniel Latypov
Jan 7, 2022, 8:23:31 PM1/7/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
The `struct kunit* test` field in kunit_assert is unused.
Note: we have access to `test` where we need it via the string_stream
object. I assume `test` in `kunit_assert` predates this and was leftover
after some refactoring.
This patch removes the field and cleans up the macros to avoid
needlessly passing around `test`.
include/kunit/assert.h | 45 ++++++++++++------------------------------
include/kunit/test.h | 14 +++++--------
2 files changed, 18 insertions(+), 41 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index ad889b539ab3..3da6c792496c 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -30,7 +30,6 @@ enum kunit_assert_type {
/**
* struct kunit_assert - Data for printing a failed assertion or expectation.
- * @test: the test case this expectation/assertion is associated with.
* @type: the type (either an expectation or an assertion) of this kunit_assert.
* @line: the source code line number that the expectation/assertion is at.
* @file: the file path of the source file that the expectation/assertion is in.
@@ -41,7 +40,6 @@ enum kunit_assert_type {
* format a string to a user reporting the failure.
*/
struct kunit_assert {
- struct kunit *test;
enum kunit_assert_type type;
int line;
const char *file;
@@ -60,14 +58,12 @@ struct kunit_assert {
/**
* KUNIT_INIT_ASSERT_STRUCT() - Initializer for a &struct kunit_assert.
- * @kunit: The test case that this expectation/assertion is associated with.
* @assert_type: The type (assertion or expectation) of this 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(kunit, assert_type, fmt) { \
- .test = kunit, \
+#define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \
.type = assert_type, \
.file = __FILE__, \
.line = __LINE__, \
@@ -96,15 +92,13 @@ void kunit_fail_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_FAIL_ASSERT_STRUCT() - Initializer for &struct kunit_fail_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
*
* Initializes a &struct kunit_fail_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT(type) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_fail_assert_format) \
}
@@ -129,7 +123,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_UNARY_ASSERT_STRUCT() - Initializes &struct kunit_unary_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
* @cond: A string representation of the expression asserted true or false.
* @expect_true: True if of type KUNIT_{EXPECT|ASSERT}_TRUE, false otherwise.
@@ -137,9 +130,8 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
* Initializes a &struct kunit_unary_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_UNARY_ASSERT_STRUCT(test, type, cond, expect_true) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(type, cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_unary_assert_format), \
.condition = cond, \
.expected_true = expect_true \
@@ -167,7 +159,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
* &struct kunit_ptr_not_err_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
* @txt: A string representation of the expression passed to the expectation.
* @val: The actual evaluated pointer value of the expression.
@@ -175,9 +166,8 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
* 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(test, type, txt, val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(type, txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_ptr_not_err_assert_format), \
.text = txt, \
.value = val \
@@ -212,7 +202,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -223,15 +212,13 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
* Initializes a &struct kunit_binary_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(type, \
op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_assert_format), \
.operation = op_str, \
.left_text = left_str, \
@@ -269,7 +256,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_ptr_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -280,15 +266,13 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
* 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(test, \
- type, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(type, \
op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_ptr_assert_format), \
.operation = op_str, \
.left_text = left_str, \
@@ -326,7 +310,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_BINARY_STR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_str_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -337,15 +320,13 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
* 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(test, \
- type, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(type, \
op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_str_assert_format), \
.operation = op_str, \
.left_text = left_str, \
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 690a28dfc795..ebd45593321e 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -789,7 +789,7 @@ void kunit_failed_assertion(struct kunit *test,
KUNIT_ASSERTION(test, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(test, assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
fmt, \
##__VA_ARGS__)
@@ -819,8 +819,7 @@ void kunit_failed_assertion(struct kunit *test,
KUNIT_ASSERTION(test, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
- KUNIT_INIT_UNARY_ASSERT_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
#condition, \
expected_true), \
fmt, \
@@ -878,8 +877,7 @@ do { \
KUNIT_ASSERTION(test, \
__left op __right, \
assert_class, \
- ASSERT_CLASS_INIT(test, \
- assert_type, \
+ ASSERT_CLASS_INIT(assert_type, \
#op, \
#left, \
__left, \
@@ -1233,8 +1231,7 @@ do { \
KUNIT_ASSERTION(test, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
- KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
#op, \
#left, \
__left, \
@@ -1293,8 +1290,7 @@ do { \
KUNIT_ASSERTION(test, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
- KUNIT_INIT_PTR_NOT_ERR_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
#ptr, \
__ptr), \
fmt, \
--
2.34.1.575.g55b058a8bb-goog
Note: we have access to `test` where we need it via the string_stream
object. I assume `test` in `kunit_assert` predates this and was leftover
after some refactoring.
This patch removes the field and cleans up the macros to avoid
needlessly passing around `test`.
include/kunit/assert.h | 45 ++++++++++++------------------------------
include/kunit/test.h | 14 +++++--------
2 files changed, 18 insertions(+), 41 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index ad889b539ab3..3da6c792496c 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -30,7 +30,6 @@ enum kunit_assert_type {
/**
* struct kunit_assert - Data for printing a failed assertion or expectation.
- * @test: the test case this expectation/assertion is associated with.
* @type: the type (either an expectation or an assertion) of this kunit_assert.
* @line: the source code line number that the expectation/assertion is at.
* @file: the file path of the source file that the expectation/assertion is in.
@@ -41,7 +40,6 @@ enum kunit_assert_type {
* format a string to a user reporting the failure.
*/
struct kunit_assert {
- struct kunit *test;
enum kunit_assert_type type;
int line;
const char *file;
@@ -60,14 +58,12 @@ struct kunit_assert {
/**
* KUNIT_INIT_ASSERT_STRUCT() - Initializer for a &struct kunit_assert.
- * @kunit: The test case that this expectation/assertion is associated with.
* @assert_type: The type (assertion or expectation) of this 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(kunit, assert_type, fmt) { \
- .test = kunit, \
+#define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \
.type = assert_type, \
.file = __FILE__, \
.line = __LINE__, \
@@ -96,15 +92,13 @@ void kunit_fail_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_FAIL_ASSERT_STRUCT() - Initializer for &struct kunit_fail_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
*
* Initializes a &struct kunit_fail_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT(type) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_fail_assert_format) \
}
@@ -129,7 +123,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_UNARY_ASSERT_STRUCT() - Initializes &struct kunit_unary_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
* @cond: A string representation of the expression asserted true or false.
* @expect_true: True if of type KUNIT_{EXPECT|ASSERT}_TRUE, false otherwise.
@@ -137,9 +130,8 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
* Initializes a &struct kunit_unary_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_UNARY_ASSERT_STRUCT(test, type, cond, expect_true) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(type, cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_unary_assert_format), \
.condition = cond, \
.expected_true = expect_true \
@@ -167,7 +159,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
* &struct kunit_ptr_not_err_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
* @txt: A string representation of the expression passed to the expectation.
* @val: The actual evaluated pointer value of the expression.
@@ -175,9 +166,8 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
* 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(test, type, txt, val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(type, txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_ptr_not_err_assert_format), \
.text = txt, \
.value = val \
@@ -212,7 +202,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -223,15 +212,13 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
* Initializes a &struct kunit_binary_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(type, \
op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_assert_format), \
.operation = op_str, \
.left_text = left_str, \
@@ -269,7 +256,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_ptr_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -280,15 +266,13 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
* 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(test, \
- type, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(type, \
op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_ptr_assert_format), \
.operation = op_str, \
.left_text = left_str, \
@@ -326,7 +310,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_BINARY_STR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_str_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -337,15 +320,13 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
* 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(test, \
- type, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(type, \
op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_str_assert_format), \
.operation = op_str, \
.left_text = left_str, \
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 690a28dfc795..ebd45593321e 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -789,7 +789,7 @@ void kunit_failed_assertion(struct kunit *test,
KUNIT_ASSERTION(test, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(test, assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
fmt, \
##__VA_ARGS__)
@@ -819,8 +819,7 @@ void kunit_failed_assertion(struct kunit *test,
KUNIT_ASSERTION(test, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
- KUNIT_INIT_UNARY_ASSERT_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
#condition, \
expected_true), \
fmt, \
@@ -878,8 +877,7 @@ do { \
KUNIT_ASSERTION(test, \
__left op __right, \
assert_class, \
- ASSERT_CLASS_INIT(test, \
- assert_type, \
+ ASSERT_CLASS_INIT(assert_type, \
#op, \
#left, \
__left, \
@@ -1233,8 +1231,7 @@ do { \
KUNIT_ASSERTION(test, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
- KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
#op, \
#left, \
__left, \
@@ -1293,8 +1290,7 @@ do { \
KUNIT_ASSERTION(test, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
- KUNIT_INIT_PTR_NOT_ERR_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
#ptr, \
__ptr), \
fmt, \
--
2.34.1.575.g55b058a8bb-goog
Daniel Latypov
Jan 7, 2022, 8:23:33 PM1/7/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
We call this function first thing for all the assertion `format()`
functions.
This is the part that prints the file and line number and assertion type
(EXPECTATION, ASSERTION).
Having it as part of the format functions lets us have the flexibility
to not print that information (or print it differently) for new
assertion types, but I think this we don't need that.
And in the future, we'd like to consider factoring that data (file,
line#, type) out of the kunit_assert struct and into a `static`
variable, as Linus suggested [1], so we'd need to extract it anyways.
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
lib/kunit/assert.c | 6 ------
lib/kunit/test.c | 1 +
2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
index b972bda61c0c..4d9a1295efc7 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -40,7 +40,6 @@ EXPORT_SYMBOL_GPL(kunit_assert_print_msg);
{
- kunit_base_assert_format(assert, stream);
string_stream_add(stream, "%pV", &assert->message);
}
EXPORT_SYMBOL_GPL(kunit_fail_assert_format);
@@ -52,7 +51,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
unary_assert = container_of(assert, struct kunit_unary_assert, assert);
- kunit_base_assert_format(assert, stream);
if (unary_assert->expected_true)
string_stream_add(stream,
KUNIT_SUBTEST_INDENT "Expected %s to be true, but is false\n",
@@ -73,7 +71,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
ptr_assert = container_of(assert, struct kunit_ptr_not_err_assert,
assert);
- kunit_base_assert_format(assert, stream);
if (!ptr_assert->value) {
string_stream_add(stream,
KUNIT_SUBTEST_INDENT "Expected %s is not null, but is\n",
@@ -119,7 +116,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
binary_assert = container_of(assert, struct kunit_binary_assert,
assert);
- kunit_base_assert_format(assert, stream);
string_stream_add(stream,
KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
binary_assert->left_text,
@@ -147,7 +143,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
binary_assert = container_of(assert, struct kunit_binary_ptr_assert,
assert);
- kunit_base_assert_format(assert, stream);
string_stream_add(stream,
KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
binary_assert->left_text,
@@ -187,7 +182,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
binary_assert = container_of(assert, struct kunit_binary_str_assert,
assert);
- kunit_base_assert_format(assert, stream);
string_stream_add(stream,
KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
binary_assert->left_text,
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 5ad671745483..735c1b67d843 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -255,6 +255,7 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
return;
}
+ kunit_base_assert_format(assert, stream);
assert->format(assert, stream);
kunit_print_string_stream(test, stream);
--
2.34.1.575.g55b058a8bb-goog
functions.
This is the part that prints the file and line number and assertion type
(EXPECTATION, ASSERTION).
Having it as part of the format functions lets us have the flexibility
to not print that information (or print it differently) for new
assertion types, but I think this we don't need that.
And in the future, we'd like to consider factoring that data (file,
line#, type) out of the kunit_assert struct and into a `static`
variable, as Linus suggested [1], so we'd need to extract it anyways.
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
lib/kunit/assert.c | 6 ------
lib/kunit/test.c | 1 +
2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
index b972bda61c0c..4d9a1295efc7 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -40,7 +40,6 @@ EXPORT_SYMBOL_GPL(kunit_assert_print_msg);
void kunit_fail_assert_format(const struct kunit_assert *assert,
struct string_stream *stream)
{
- kunit_base_assert_format(assert, stream);
string_stream_add(stream, "%pV", &assert->message);
}
EXPORT_SYMBOL_GPL(kunit_fail_assert_format);
@@ -52,7 +51,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
unary_assert = container_of(assert, struct kunit_unary_assert, assert);
- kunit_base_assert_format(assert, stream);
if (unary_assert->expected_true)
string_stream_add(stream,
KUNIT_SUBTEST_INDENT "Expected %s to be true, but is false\n",
@@ -73,7 +71,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
ptr_assert = container_of(assert, struct kunit_ptr_not_err_assert,
assert);
- kunit_base_assert_format(assert, stream);
if (!ptr_assert->value) {
string_stream_add(stream,
KUNIT_SUBTEST_INDENT "Expected %s is not null, but is\n",
@@ -119,7 +116,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
binary_assert = container_of(assert, struct kunit_binary_assert,
assert);
- kunit_base_assert_format(assert, stream);
string_stream_add(stream,
KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
binary_assert->left_text,
@@ -147,7 +143,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
binary_assert = container_of(assert, struct kunit_binary_ptr_assert,
assert);
- kunit_base_assert_format(assert, stream);
string_stream_add(stream,
KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
binary_assert->left_text,
@@ -187,7 +182,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
binary_assert = container_of(assert, struct kunit_binary_str_assert,
assert);
- kunit_base_assert_format(assert, stream);
string_stream_add(stream,
KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
binary_assert->left_text,
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 5ad671745483..735c1b67d843 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -255,6 +255,7 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
return;
}
+ kunit_base_assert_format(assert, stream);
assert->format(assert, stream);
kunit_print_string_stream(test, stream);
--
2.34.1.575.g55b058a8bb-goog
Daniel Latypov
Jan 7, 2022, 8:23:36 PM1/7/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
This is per Linus's suggestion in [1].
The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
kunit_assert object onto the stack. Normally we rely on compilers to
elide this, but when that doesn't work out, this blows up the stack
usage of kunit test functions.
We can move some data off the stack by making it static.
This change introduces a new `struct kunit_loc` to hold the file and
line number and then just passing assert_type (EXPECT or ASSERT) as an
argument.
In [1], it was suggested to also move out the format string as well, but
users could theoretically craft a format string at runtime, so we can't.
This change leaves a copy of `assert_type` in kunit_assert for now
because cleaning up all the macros to not pass it around is a bit more
involved.
Here's an example of the expanded code for KUNIT_FAIL():
if (!(false)) {
static const struct kunit_loc loc = { .file = ... };
struct kunit_unary_assert __assertion = { .assert = { .type ... };
kunit_failed_assertion(test, &loc, &__assertion.assert, ((void *)0));
};
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
Signed-off-by: Daniel Latypov <dlat...@google.com>
Suggested-by: Linus Torvalds <torv...@linux-foundation.org>
---
include/kunit/assert.h | 25 ++++++++++++++++---------
include/kunit/test.h | 12 +++++++++++-
lib/kunit/assert.c | 9 +++++----
lib/kunit/test.c | 15 +++++++++------
4 files changed, 41 insertions(+), 20 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 3da6c792496c..4f91dbdb886a 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -28,11 +28,21 @@ enum kunit_assert_type {
KUNIT_EXPECTATION,
};
+/**
+ * struct kunit_loc - Identifies the source location of a line of code.
+ * @line: the line number in the file.
+ * @file: the file name.
+ */
+struct kunit_loc {
+ int line;
+ const char *file;
+};
+
+#define KUNIT_CURRENT_LOC { .file = __FILE__, .line = __LINE__ }
+
- * @file: the file path of the source file that the expectation/assertion is in.
* @message: an optional message to provide additional context.
* @format: a function which formats the data in this kunit_assert to a string.
*
@@ -40,9 +50,7 @@ enum kunit_assert_type {
- int line;
- const char *file;
+ enum kunit_assert_type type; // TODO(dlat...@google.com): delete this
struct va_format message;
void (*format)(const struct kunit_assert *assert,
struct string_stream *stream);
@@ -65,14 +73,13 @@ struct kunit_assert {
*/
- .line = __LINE__, \
.message = KUNIT_INIT_VA_FMT_NULL, \
.format = fmt \
}
-void kunit_base_assert_format(const struct kunit_assert *assert,
- struct string_stream *stream);
+void kunit_assert_prologue(const struct kunit_loc *loc,
+ enum kunit_assert_type type,
+ struct string_stream *stream);
void kunit_assert_print_msg(const struct kunit_assert *assert,
struct string_stream *stream);
diff --git a/include/kunit/test.h b/include/kunit/test.h
index ebd45593321e..6e201b45ada6 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -771,13 +771,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
+ const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct kunit_assert *assert,
const char *fmt, ...);
-#define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do { \
+#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, INITIALIZER, fmt, ...) do { \
if (!(pass)) { \
+ static const struct kunit_loc loc = KUNIT_CURRENT_LOC; \
+ &loc, \
+ assert_type, \
&__assertion.assert, \
fmt, \
##__VA_ARGS__); \
@@ -787,6 +792,7 @@ void kunit_failed_assertion(struct kunit *test,
#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \
KUNIT_ASSERTION(test, \
+ assert_type, \
false, \
kunit_fail_assert, \
KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
@@ -817,6 +823,7 @@ void kunit_failed_assertion(struct kunit *test,
fmt, \
...) \
KUNIT_ASSERTION(test, \
+ assert_type, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
@@ -875,6 +882,7 @@ do { \
typeof(right) __right = (right); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
__left op __right, \
assert_class, \
ASSERT_CLASS_INIT(assert_type, \
@@ -1229,6 +1237,7 @@ do { \
const char *__right = (right); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
@@ -1288,6 +1297,7 @@ do { \
typeof(ptr) __ptr = (ptr); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
index 4d9a1295efc7..9f4492a8e24e 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -10,12 +10,13 @@
#include "string-stream.h"
-void kunit_base_assert_format(const struct kunit_assert *assert,
+void kunit_assert_prologue(const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct string_stream *stream)
{
const char *expect_or_assert = NULL;
- switch (assert->type) {
+ switch (type) {
case KUNIT_EXPECTATION:
expect_or_assert = "EXPECTATION";
break;
@@ -25,9 +26,9 @@ void kunit_base_assert_format(const struct kunit_assert *assert,
}
string_stream_add(stream, "%s FAILED at %s:%d\n",
- expect_or_assert, assert->file, assert->line);
+ expect_or_assert, loc->file, loc->line);
}
-EXPORT_SYMBOL_GPL(kunit_base_assert_format);
+EXPORT_SYMBOL_GPL(kunit_assert_prologue);
void kunit_assert_print_msg(const struct kunit_assert *assert,
struct string_stream *stream)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 735c1b67d843..3108ed0575d4 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -240,7 +240,8 @@ static void kunit_print_string_stream(struct kunit *test,
}
}
-static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
+static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
+ enum kunit_assert_type type, struct kunit_assert *assert)
{
struct string_stream *stream;
@@ -250,12 +251,12 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
if (!stream) {
WARN(true,
"Could not allocate stream to print failed assertion in %s:%d\n",
- assert->file,
- assert->line);
+ loc->file,
+ loc->line);
return;
}
- kunit_base_assert_format(assert, stream);
+ kunit_assert_prologue(loc, type, stream);
}
void kunit_failed_assertion(struct kunit *test,
+ const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct kunit_assert *assert,
const char *fmt, ...)
{
@@ -286,11 +289,11 @@ void kunit_failed_assertion(struct kunit *test,
assert->message.fmt = fmt;
assert->message.va = &args;
- kunit_fail(test, assert);
+ kunit_fail(test, loc, type, assert);
va_end(args);
- if (assert->type == KUNIT_ASSERTION)
+ if (type == KUNIT_ASSERTION)
kunit_abort(test);
}
EXPORT_SYMBOL_GPL(kunit_failed_assertion);
--
2.34.1.575.g55b058a8bb-goog
The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
kunit_assert object onto the stack. Normally we rely on compilers to
elide this, but when that doesn't work out, this blows up the stack
usage of kunit test functions.
We can move some data off the stack by making it static.
This change introduces a new `struct kunit_loc` to hold the file and
line number and then just passing assert_type (EXPECT or ASSERT) as an
argument.
In [1], it was suggested to also move out the format string as well, but
users could theoretically craft a format string at runtime, so we can't.
This change leaves a copy of `assert_type` in kunit_assert for now
because cleaning up all the macros to not pass it around is a bit more
involved.
Here's an example of the expanded code for KUNIT_FAIL():
if (!(false)) {
static const struct kunit_loc loc = { .file = ... };
struct kunit_unary_assert __assertion = { .assert = { .type ... };
kunit_failed_assertion(test, &loc, &__assertion.assert, ((void *)0));
};
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
Signed-off-by: Daniel Latypov <dlat...@google.com>
---
include/kunit/assert.h | 25 ++++++++++++++++---------
include/kunit/test.h | 12 +++++++++++-
lib/kunit/assert.c | 9 +++++----
lib/kunit/test.c | 15 +++++++++------
4 files changed, 41 insertions(+), 20 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 3da6c792496c..4f91dbdb886a 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -28,11 +28,21 @@ enum kunit_assert_type {
KUNIT_EXPECTATION,
};
+/**
+ * struct kunit_loc - Identifies the source location of a line of code.
+ * @line: the line number in the file.
+ * @file: the file name.
+ */
+struct kunit_loc {
+ int line;
+ const char *file;
+};
+
+#define KUNIT_CURRENT_LOC { .file = __FILE__, .line = __LINE__ }
+
/**
* struct kunit_assert - Data for printing a failed assertion or expectation.
* struct kunit_assert - Data for printing a failed assertion or expectation.
* @type: the type (either an expectation or an assertion) of this kunit_assert.
- * @line: the source code line number that the expectation/assertion is at.
- * @file: the file path of the source file that the expectation/assertion is in.
* @message: an optional message to provide additional context.
* @format: a function which formats the data in this kunit_assert to a string.
*
@@ -40,9 +50,7 @@ enum kunit_assert_type {
* format a string to a user reporting the failure.
*/
struct kunit_assert {
- enum kunit_assert_type type;
*/
struct kunit_assert {
- int line;
- const char *file;
+ enum kunit_assert_type type; // TODO(dlat...@google.com): delete this
struct va_format message;
void (*format)(const struct kunit_assert *assert,
struct string_stream *stream);
@@ -65,14 +73,13 @@ struct kunit_assert {
*/
#define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \
.type = assert_type, \
- .file = __FILE__, \
.type = assert_type, \
- .line = __LINE__, \
.message = KUNIT_INIT_VA_FMT_NULL, \
.format = fmt \
}
-void kunit_base_assert_format(const struct kunit_assert *assert,
- struct string_stream *stream);
+void kunit_assert_prologue(const struct kunit_loc *loc,
+ enum kunit_assert_type type,
+ struct string_stream *stream);
void kunit_assert_print_msg(const struct kunit_assert *assert,
struct string_stream *stream);
diff --git a/include/kunit/test.h b/include/kunit/test.h
index ebd45593321e..6e201b45ada6 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -771,13 +771,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
#define KUNIT_SUCCEED(test) do {} while (0)
void kunit_failed_assertion(struct kunit *test,
+ const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct kunit_assert *assert,
const char *fmt, ...);
-#define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do { \
+#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, INITIALIZER, fmt, ...) do { \
if (!(pass)) { \
+ static const struct kunit_loc loc = KUNIT_CURRENT_LOC; \
struct assert_class __assertion = INITIALIZER; \
kunit_failed_assertion(test, \
+ &loc, \
+ assert_type, \
&__assertion.assert, \
fmt, \
##__VA_ARGS__); \
@@ -787,6 +792,7 @@ void kunit_failed_assertion(struct kunit *test,
#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \
KUNIT_ASSERTION(test, \
+ assert_type, \
false, \
kunit_fail_assert, \
KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
@@ -817,6 +823,7 @@ void kunit_failed_assertion(struct kunit *test,
fmt, \
...) \
KUNIT_ASSERTION(test, \
+ assert_type, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
@@ -875,6 +882,7 @@ do { \
typeof(right) __right = (right); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
__left op __right, \
assert_class, \
ASSERT_CLASS_INIT(assert_type, \
@@ -1229,6 +1237,7 @@ do { \
const char *__right = (right); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
kunit_binary_str_assert, \
@@ -1288,6 +1297,7 @@ do { \
typeof(ptr) __ptr = (ptr); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
index 4d9a1295efc7..9f4492a8e24e 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -10,12 +10,13 @@
#include "string-stream.h"
-void kunit_base_assert_format(const struct kunit_assert *assert,
+void kunit_assert_prologue(const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct string_stream *stream)
{
const char *expect_or_assert = NULL;
- switch (assert->type) {
+ switch (type) {
case KUNIT_EXPECTATION:
expect_or_assert = "EXPECTATION";
break;
@@ -25,9 +26,9 @@ void kunit_base_assert_format(const struct kunit_assert *assert,
}
string_stream_add(stream, "%s FAILED at %s:%d\n",
- expect_or_assert, assert->file, assert->line);
+ expect_or_assert, loc->file, loc->line);
}
-EXPORT_SYMBOL_GPL(kunit_base_assert_format);
+EXPORT_SYMBOL_GPL(kunit_assert_prologue);
void kunit_assert_print_msg(const struct kunit_assert *assert,
struct string_stream *stream)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 735c1b67d843..3108ed0575d4 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -240,7 +240,8 @@ static void kunit_print_string_stream(struct kunit *test,
}
}
-static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
+static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
+ enum kunit_assert_type type, struct kunit_assert *assert)
{
struct string_stream *stream;
@@ -250,12 +251,12 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
if (!stream) {
WARN(true,
"Could not allocate stream to print failed assertion in %s:%d\n",
- assert->file,
- assert->line);
+ loc->file,
+ loc->line);
return;
}
- kunit_base_assert_format(assert, stream);
+ kunit_assert_prologue(loc, type, stream);
assert->format(assert, stream);
kunit_print_string_stream(test, stream);
@@ -277,6 +278,8 @@ static void __noreturn kunit_abort(struct kunit *test)
kunit_print_string_stream(test, stream);
}
void kunit_failed_assertion(struct kunit *test,
+ const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct kunit_assert *assert,
const char *fmt, ...)
{
@@ -286,11 +289,11 @@ void kunit_failed_assertion(struct kunit *test,
assert->message.fmt = fmt;
assert->message.va = &args;
- kunit_fail(test, assert);
+ kunit_fail(test, loc, type, assert);
va_end(args);
- if (assert->type == KUNIT_ASSERTION)
+ if (type == KUNIT_ASSERTION)
kunit_abort(test);
}
EXPORT_SYMBOL_GPL(kunit_failed_assertion);
--
2.34.1.575.g55b058a8bb-goog
Daniel Latypov
Jan 7, 2022, 8:23:38 PM1/7/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
This field has been split out from kunit_assert to make the struct less
heavy along with the filename and line number.
This change drops the assert_type field and cleans up all the macros
that were plumbing assert_type into kunit_assert.
include/kunit/assert.h | 46 +++++++++++++-----------------------------
include/kunit/test.h | 14 +++++--------
2 files changed, 19 insertions(+), 41 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 4f91dbdb886a..21299232c120 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -42,7 +42,6 @@ struct kunit_loc {
/**
* struct kunit_assert - Data for printing a failed assertion or expectation.
- * @type: the type (either an expectation or an assertion) of this kunit_assert.
/**
* KUNIT_INIT_ASSERT_STRUCT() - Initializer for a &struct kunit_assert.
- * @assert_type: The type (assertion or expectation) of this kunit_assert.
- .type = assert_type, \
+#define KUNIT_INIT_ASSERT_STRUCT(fmt) { \
struct string_stream *stream);
/**
- * KUNIT_INIT_FAIL_ASSERT_STRUCT() - Initializer for &struct kunit_fail_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
+ * KUNIT_INIT_FAIL_ASSERT_STRUCT - Initializer for &struct kunit_fail_assert.
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_fail_assert_format) \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_fail_assert_format) \
}
/**
@@ -130,16 +124,14 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_UNARY_ASSERT_STRUCT() - Initializes &struct kunit_unary_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_unary_assert_format), \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_unary_assert_format), \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_ptr_not_err_assert_format), \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_ptr_not_err_assert_format), \
@@ -219,14 +208,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
- op_str, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_assert_format), \
@@ -273,14 +260,12 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
- op_str, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_ptr_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_ptr_assert_format), \
@@ -317,7 +302,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
@@ -327,14 +311,12 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
- op_str, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_str_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_str_assert_format), \
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 6e201b45ada6..6f9074ec1995 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -795,7 +795,7 @@ void kunit_failed_assertion(struct kunit *test,
assert_type, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT, \
fmt, \
##__VA_ARGS__)
@@ -826,8 +826,7 @@ void kunit_failed_assertion(struct kunit *test,
- #condition, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
expected_true), \
fmt, \
##__VA_ARGS__)
@@ -885,8 +884,7 @@ do { \
- #op, \
+ ASSERT_CLASS_INIT(#op, \
#left, \
__left, \
#right, \
@@ -1240,8 +1238,7 @@ do { \
- #op, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(#op, \
#left, \
__left, \
#right, \
@@ -1300,8 +1297,7 @@ do { \
assert_type, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
- KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
- #ptr, \
+ KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \
__ptr), \
fmt, \
##__VA_ARGS__); \
--
2.34.1.575.g55b058a8bb-goog
heavy along with the filename and line number.
This change drops the assert_type field and cleans up all the macros
that were plumbing assert_type into kunit_assert.
include/kunit/assert.h | 46 +++++++++++++-----------------------------
include/kunit/test.h | 14 +++++--------
2 files changed, 19 insertions(+), 41 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 4f91dbdb886a..21299232c120 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -42,7 +42,6 @@ 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.
*
@@ -50,7 +49,6 @@ struct kunit_loc {
* @format: a function which formats the data in this kunit_assert to a string.
*
* format a string to a user reporting the failure.
*/
struct kunit_assert {
- enum kunit_assert_type type; // TODO(dlat...@google.com): delete this
*/
struct kunit_assert {
struct va_format message;
void (*format)(const struct kunit_assert *assert,
struct string_stream *stream);
@@ -66,13 +64,11 @@ struct kunit_assert {
void (*format)(const struct kunit_assert *assert,
struct string_stream *stream);
/**
* 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(assert_type, fmt) { \
*
* The base initializer for a &struct kunit_assert.
*/
- .type = assert_type, \
+#define KUNIT_INIT_ASSERT_STRUCT(fmt) { \
.message = KUNIT_INIT_VA_FMT_NULL, \
.format = fmt \
}
@@ -98,15 +94,13 @@ void kunit_fail_assert_format(const struct kunit_assert *assert,
.format = fmt \
}
struct string_stream *stream);
/**
- * KUNIT_INIT_FAIL_ASSERT_STRUCT() - Initializer for &struct kunit_fail_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
+ * 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(type) { \
* Initializes a &struct kunit_fail_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_fail_assert_format) \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_fail_assert_format) \
}
/**
@@ -130,16 +124,14 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
/**
* 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.
*
* @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(type, cond, expect_true) { \
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_unary_assert_format), \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_unary_assert_format), \
.condition = cond, \
.expected_true = expect_true \
}
@@ -166,16 +158,14 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
.expected_true = expect_true \
}
/**
* KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
* &struct kunit_ptr_not_err_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
* 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.
*
* @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(type, txt, val) { \
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_ptr_not_err_assert_format), \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_ptr_not_err_assert_format), \
.text = txt, \
.value = val \
}
@@ -209,7 +199,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
.value = val \
}
/**
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_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.
* @left_str: A string representation of the expression in the left slot.
@@ -219,14 +208,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
* Initializes a &struct kunit_binary_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_ASSERT_STRUCT(type, \
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
- op_str, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_assert_format), \
.operation = op_str, \
.left_text = left_str, \
.left_value = left_val, \
.left_text = left_str, \
@@ -273,14 +260,12 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
* 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(type, \
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
- op_str, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_ptr_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_ptr_assert_format), \
.operation = op_str, \
.left_text = left_str, \
.left_value = left_val, \
.left_text = left_str, \
@@ -317,7 +302,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_BINARY_STR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_str_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
* 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.
* @left_str: A string representation of the expression in the left slot.
@@ -327,14 +311,12 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
* 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(type, \
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
- op_str, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(op_str, \
left_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_str_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_str_assert_format), \
.operation = op_str, \
.left_text = left_str, \
.left_value = left_val, \
.left_text = left_str, \
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 6e201b45ada6..6f9074ec1995 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -795,7 +795,7 @@ void kunit_failed_assertion(struct kunit *test,
assert_type, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT, \
fmt, \
##__VA_ARGS__)
@@ -826,8 +826,7 @@ void kunit_failed_assertion(struct kunit *test,
assert_type, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
- KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
- #condition, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
expected_true), \
fmt, \
##__VA_ARGS__)
@@ -885,8 +884,7 @@ do { \
assert_type, \
__left op __right, \
assert_class, \
- ASSERT_CLASS_INIT(assert_type, \
__left op __right, \
assert_class, \
- #op, \
+ ASSERT_CLASS_INIT(#op, \
#left, \
__left, \
#right, \
@@ -1240,8 +1238,7 @@ do { \
assert_type, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
- KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
- #op, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(#op, \
#left, \
__left, \
#right, \
@@ -1300,8 +1297,7 @@ do { \
assert_type, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
- KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
- #ptr, \
+ KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \
__ptr), \
fmt, \
##__VA_ARGS__); \
--
2.34.1.575.g55b058a8bb-goog
Brendan Higgins
Jan 10, 2022, 5:14:12 PM1/10/22
to Daniel Latypov, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
On Fri, Jan 7, 2022 at 8:23 PM Daniel Latypov <dlat...@google.com> wrote:
>
> Currently, these macros are only really documented near the bottom of
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
>
> E.g. it's likely someone might just not realize that
> KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
> or similar.
>
> This can also serve as a basic smoketest that the KUnit assert machinery
> still works for all the macros.
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
I still don't like how much this bloats the example test; aside from
>
> Currently, these macros are only really documented near the bottom of
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
>
> E.g. it's likely someone might just not realize that
> KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
> or similar.
>
> This can also serve as a basic smoketest that the KUnit assert machinery
> still works for all the macros.
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
that, this looks good.
Reviewed-by: Brendan Higgins <brendan...@google.com>
Brendan Higgins
Jan 10, 2022, 5:21:54 PM1/10/22
to Daniel Latypov, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
> Currently the code always calls kunit_do_assertion() even though it does
> nothing when `pass` is true.
>
> This change moves the `if(!(pass))` check into the macro instead
> and renames the function to kunit_failed_assertion().
> I feel this a bit easier to read and understand.
>
> This has the potential upside of avoiding a function call that does
> nothing most of the time (assuming your tests are passing) but comes
> with the downside of generating a bit more code and branches.
>
> This also means we don't have to initialize structs that we don't need,
> which will become a tiny bit more expensive if we switch over to using
> static variables to try and reduce stack usage. (There's runtime code
> to check if the variable has been initialized yet or not).
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
Tiny nit, see below. Otherwise:
> nothing when `pass` is true.
>
> This change moves the `if(!(pass))` check into the macro instead
> and renames the function to kunit_failed_assertion().
> I feel this a bit easier to read and understand.
>
> This has the potential upside of avoiding a function call that does
> nothing most of the time (assuming your tests are passing) but comes
> with the downside of generating a bit more code and branches.
>
> This also means we don't have to initialize structs that we don't need,
> which will become a tiny bit more expensive if we switch over to using
> static variables to try and reduce stack usage. (There's runtime code
> to check if the variable has been initialized yet or not).
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
> ---
> include/kunit/test.h | 20 ++++++++++----------
> lib/kunit/test.c | 13 ++++---------
> 2 files changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index b26400731c02..690a28dfc795 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -770,18 +770,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
> */
> #define KUNIT_SUCCEED(test) do {} while (0)
>
> -void kunit_do_assertion(struct kunit *test,
> - struct kunit_assert *assert,
> - bool pass,
> - const char *fmt, ...);
> +void kunit_failed_assertion(struct kunit *test,
> + struct kunit_assert *assert,
> + const char *fmt, ...);
functions should be in the active tense, imperative mood since when
you call a function you are telling it to do something.
Also, do we need to worry about this getting confused with KUNIT_FAIL,
or KUNIT_FAIL_ASSERTION:
https://elixir.bootlin.com/linux/v5.16/source/include/kunit/test.h#L788
?
Brendan Higgins
Jan 10, 2022, 5:24:19 PM1/10/22
to Daniel Latypov, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
> The `struct kunit* test` field in kunit_assert is unused.
> Note: we have access to `test` where we need it via the string_stream
> object. I assume `test` in `kunit_assert` predates this and was leftover
> after some refactoring.
>
> This patch removes the field and cleans up the macros to avoid
> needlessly passing around `test`.
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
Looks good. Thanks!
> Note: we have access to `test` where we need it via the string_stream
> object. I assume `test` in `kunit_assert` predates this and was leftover
> after some refactoring.
>
> This patch removes the field and cleans up the macros to avoid
> needlessly passing around `test`.
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
Daniel Latypov
Jan 10, 2022, 5:25:18 PM1/10/22
to Brendan Higgins, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
I just wanted something *somewhere* I could use to smoketest the later changes.
I just remembered how people weren't very aware of the _MSG variants
and thought this could help.
If others have a preference, I'll happily move out and into kunit-test.c.
I'm fine either way as I initially was going to put it there to begin with.
>
> Reviewed-by: Brendan Higgins <brendan...@google.com>
Brendan Higgins
Jan 10, 2022, 5:32:10 PM1/10/22
to Daniel Latypov, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
> We call this function first thing for all the assertion `format()`
> functions.
> This is the part that prints the file and line number and assertion type
> (EXPECTATION, ASSERTION).
>
> Having it as part of the format functions lets us have the flexibility
> to not print that information (or print it differently) for new
> assertion types, but I think this we don't need that.
nit: drop the "this".
> functions.
> This is the part that prints the file and line number and assertion type
> (EXPECTATION, ASSERTION).
>
> Having it as part of the format functions lets us have the flexibility
> to not print that information (or print it differently) for new
> assertion types, but I think this we don't need that.
functions was to take advantage of inheritance. I was treating
kunit_base_assert_format as the parent method that other methods were
inheriting from, so I wanted to have them inherit some of the common
behavior by calling the original function.
If you decide to make this change, I think it would be a good idea to
change the name of kunit_base_assert_format to not mislead to this
effect.
Daniel Latypov
Jan 10, 2022, 5:33:02 PM1/10/22
to Brendan Higgins, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
kunit_fail_assertion() to avoid confusion with the macro?
That's partly why I went with past tense.
Perhaps: "kunit_do_assertion() => kunit_do_failed_assertion()" instead?
Tangent: we have some similar confusing names, e.g. KUNIT_ASSERTION is
both the name of a macro and an enum (kunit_assert_type), and those
have the exact same case.
Daniel Latypov
Jan 10, 2022, 5:35:19 PM1/10/22
to Brendan Higgins, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
I can rename it in this change if you prefer.
I had just initially left it with the same name to keep this diff a
bit smaller and more focused.
But now you point it out, I think it would be cleaner to rename it here.
>
> > assert->format(assert, stream);
> >
> > kunit_print_string_stream(test, stream);
> > --
> > 2.34.1.575.g55b058a8bb-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+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAFd5g47r8aQBWPtt6ffHokqqN2sMi10p1Q5QA3xGVLTVDQh98Q%40mail.gmail.com.
David Gow
Jan 11, 2022, 1:51:04 AM1/11/22
to Daniel Latypov, Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Linus Torvalds
On Sat, Jan 8, 2022 at 9:23 AM Daniel Latypov <dlat...@google.com> wrote:
>
> Currently, these macros are only really documented near the bottom of
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
>
> E.g. it's likely someone might just not realize that
> KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
> or similar.
>
> This can also serve as a basic smoketest that the KUnit assert machinery
> still works for all the macros.
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> ---
I think this is a great idea. I will note that this definitely isn't a
>
> Currently, these macros are only really documented near the bottom of
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
>
> E.g. it's likely someone might just not realize that
> KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
> or similar.
>
> This can also serve as a basic smoketest that the KUnit assert machinery
> still works for all the macros.
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> ---
full test _of_ the assertion macros (in that it only exercises the
success case), so keeping it as an example is probably best.
A few possible ideas below, but I'm happy enough with this as-is regardless.
Reviewed-by: David Gow <davi...@google.com>
> lib/kunit/kunit-example-test.c | 46 ++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> index 51099b0ca29c..182a64c12541 100644
> --- a/lib/kunit/kunit-example-test.c
> +++ b/lib/kunit/kunit-example-test.c
> @@ -69,6 +69,51 @@ static void example_mark_skipped_test(struct kunit *test)
> /* This line should run */
> kunit_info(test, "You should see this line.");
> }
> +
> +/*
> + * This test shows off all the KUNIT_EXPECT macros.
> + */
> +static void example_all_expect_macros_test(struct kunit *test)
> +{
> + KUNIT_EXPECT_TRUE(test, true);
> + KUNIT_EXPECT_FALSE(test, false);
assertions', 'integer assertions', 'pointer assertions', etc)?
> +
> + KUNIT_EXPECT_EQ(test, 1, 1);
> + KUNIT_EXPECT_GE(test, 1, 1);
> + KUNIT_EXPECT_LE(test, 1, 1);
> + KUNIT_EXPECT_NE(test, 1, 0);
> + KUNIT_EXPECT_GT(test, 1, 0);
> + KUNIT_EXPECT_LT(test, 0, 1);
> +
> + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, test);
> + KUNIT_EXPECT_PTR_EQ(test, NULL, NULL);
> + KUNIT_EXPECT_PTR_NE(test, test, NULL);
> +
> + KUNIT_EXPECT_STREQ(test, "hi", "hi");
> + KUNIT_EXPECT_STRNEQ(test, "hi", "bye");
> +
> + /*
> + * There are also _MSG variants of all of the above that let you include
> + * additional text on failure.
> + */
excessive to also include all of these, particularly in an example, it
might be worth mentioning them in a comment somewhere?
Alternatively, if this is bloating the example too much, we could have
only one example each of the ASSERT and _MSG variants.
> + KUNIT_EXPECT_TRUE_MSG(test, true, "msg");
> + KUNIT_EXPECT_FALSE_MSG(test, false, "msg");
have here, but I can't think of a good one. Maybe (particularly for
the less obvious integer/string/pointer macros below), having a
description of what's being asserted?
David Gow
Jan 11, 2022, 1:51:25 AM1/11/22
to Daniel Latypov, Brendan Higgins, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
join the bikeshedding, I'd probably go with kunit_assertion_fail() or
similar (kunit_assertion_failed works too, past-tense-wise.)
But kunit_do_fail{,ed}_assertion() would work too.
David Gow
Jan 11, 2022, 1:51:36 AM1/11/22
to Daniel Latypov, brendan...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
On Sat, Jan 8, 2022 at 9:23 AM Daniel Latypov <dlat...@google.com> wrote:
>
> The `struct kunit* test` field in kunit_assert is unused.
> Note: we have access to `test` where we need it via the string_stream
> object. I assume `test` in `kunit_assert` predates this and was leftover
> after some refactoring.
Note that I don't like the idea of accessing 'test' via the string
>
> The `struct kunit* test` field in kunit_assert is unused.
> Note: we have access to `test` where we need it via the string_stream
> object. I assume `test` in `kunit_assert` predates this and was leftover
> after some refactoring.
stream in general, but we don't seem to ever actually do this (as far
as I can tell). Maybe if we wanted to be super nitpicky, rewording the
note to say "if we need it" rather than "where we need it" would be
clearer.
>
> This patch removes the field and cleans up the macros to avoid
> needlessly passing around `test`.
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> ---
Reviewed-by: David Gow <davi...@google.com>
David Gow
Jan 11, 2022, 1:51:45 AM1/11/22
to Daniel Latypov, brendan...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
> We call this function first thing for all the assertion `format()`
> functions.
> This is the part that prints the file and line number and assertion type
> (EXPECTATION, ASSERTION).
>
> Having it as part of the format functions lets us have the flexibility
> to not print that information (or print it differently) for new
> assertion types, but I think this we don't need that.
>
> And in the future, we'd like to consider factoring that data (file,
> line#, type) out of the kunit_assert struct and into a `static`
> variable, as Linus suggested [1], so we'd need to extract it anyways.
>
> [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> ---
Looks good to me, thanks!
> functions.
> This is the part that prints the file and line number and assertion type
> (EXPECTATION, ASSERTION).
>
> Having it as part of the format functions lets us have the flexibility
> to not print that information (or print it differently) for new
> assertion types, but I think this we don't need that.
>
> And in the future, we'd like to consider factoring that data (file,
> line#, type) out of the kunit_assert struct and into a `static`
> variable, as Linus suggested [1], so we'd need to extract it anyways.
>
> [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> ---
Reviewed-by: David Gow <davi...@google.com>
David Gow
Jan 11, 2022, 1:57:21 AM1/11/22
to Daniel Latypov, brendan...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
> This is per Linus's suggestion in [1].
>
> The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
> kunit_assert object onto the stack. Normally we rely on compilers to
> elide this, but when that doesn't work out, this blows up the stack
> usage of kunit test functions.
>
> We can move some data off the stack by making it static.
> This change introduces a new `struct kunit_loc` to hold the file and
> line number and then just passing assert_type (EXPECT or ASSERT) as an
> argument.
A part of me feels that logically, this struct is really
>
> The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
> kunit_assert object onto the stack. Normally we rely on compilers to
> elide this, but when that doesn't work out, this blows up the stack
> usage of kunit test functions.
>
> We can move some data off the stack by making it static.
> This change introduces a new `struct kunit_loc` to hold the file and
> line number and then just passing assert_type (EXPECT or ASSERT) as an
> argument.
"kunit_assert_static_data" (the collection of everything we can make
static), rather than specifically reserved for location information,
but that's an uglier name. My only concern is that, if we do manage to
make more things static const, will we want to rename this struct?
>
> In [1], it was suggested to also move out the format string as well, but
> users could theoretically craft a format string at runtime, so we can't.
>
> This change leaves a copy of `assert_type` in kunit_assert for now
> because cleaning up all the macros to not pass it around is a bit more
> involved.
>
> Here's an example of the expanded code for KUNIT_FAIL():
> if (!(false)) {
> static const struct kunit_loc loc = { .file = ... };
> struct kunit_unary_assert __assertion = { .assert = { .type ... };
> kunit_failed_assertion(test, &loc, &__assertion.assert, ((void *)0));
> };
>
> [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> Suggested-by: Linus Torvalds <torv...@linux-foundation.org>
> ---
Reviewed-by: David Gow <davi...@google.com>
Cheers,
-- David
David Gow
Jan 11, 2022, 1:57:26 AM1/11/22
to Daniel Latypov, Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Linus Torvalds
> This field has been split out from kunit_assert to make the struct less
> heavy along with the filename and line number.
>
> This change drops the assert_type field and cleans up all the macros
> that were plumbing assert_type into kunit_assert.
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> ---
Looks good to me. Anything that simplifies these macros is a good
> heavy along with the filename and line number.
>
> This change drops the assert_type field and cleans up all the macros
> that were plumbing assert_type into kunit_assert.
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> ---
thing, in my opinion.
One minor formatting nitpick below.
Reviewed-by: David Gow <davi...@google.com>
-- David
but it's possible I missed one.)
Daniel Latypov
Jan 11, 2022, 12:07:51 PM1/11/22
to David Gow, brendan...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
On Mon, Jan 10, 2022 at 10:57 PM David Gow <davi...@google.com> wrote:
>
> On Sat, Jan 8, 2022 at 9:23 AM Daniel Latypov <dlat...@google.com> wrote:
> >
> > This is per Linus's suggestion in [1].
> >
> > The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
> > kunit_assert object onto the stack. Normally we rely on compilers to
> > elide this, but when that doesn't work out, this blows up the stack
> > usage of kunit test functions.
> >
> > We can move some data off the stack by making it static.
> > This change introduces a new `struct kunit_loc` to hold the file and
> > line number and then just passing assert_type (EXPECT or ASSERT) as an
> > argument.
>
> A part of me feels that logically, this struct is really
> "kunit_assert_static_data" (the collection of everything we can make
> static), rather than specifically reserved for location information,
That's exactly what I originally called it.
>
> On Sat, Jan 8, 2022 at 9:23 AM Daniel Latypov <dlat...@google.com> wrote:
> >
> > This is per Linus's suggestion in [1].
> >
> > The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
> > kunit_assert object onto the stack. Normally we rely on compilers to
> > elide this, but when that doesn't work out, this blows up the stack
> > usage of kunit test functions.
> >
> > We can move some data off the stack by making it static.
> > This change introduces a new `struct kunit_loc` to hold the file and
> > line number and then just passing assert_type (EXPECT or ASSERT) as an
> > argument.
>
> A part of me feels that logically, this struct is really
> "kunit_assert_static_data" (the collection of everything we can make
> static), rather than specifically reserved for location information,
I was passing it around as
const struct kunit_assert_data *data,
and later
const struct kunit_assert_static_data *data,
But "data" isn't really a readable name.
Especially not when being plumbed through layers of macros as we'd
have to do to move more data into it, see below.
So I settled for dropping kunit_assert_type from the struct so we
could have a more explicit name (kunit_loc) for now.
> but that's an uglier name. My only concern is that, if we do manage to
> make more things static const, will we want to rename this struct?
are specific to each assertion type, thus making that trickier.
We could have
struct kunit_assert_static_data {
struct kunit_loc loc;
enum kunit_assert_type type;
/* Optional fields: not all assertions will use these */
const char *left;
const char *right;
};
if we go down that path in the future.
I initially tried out going this way.
But it involved changing the macros a lot more, i.e. we pass around an
initializer for the assert struct right now, right? We'd need to also
pass around the kunit_assert_static_data initializer to allow the
flexibility to optionally set some of these fields now too.
Perhaps after trimming off a layer of indirection in the macros then
it won't be as unreadable...
Daniel Latypov
Jan 11, 2022, 12:27:38 PM1/11/22
to David Gow, Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Linus Torvalds
On Mon, Jan 10, 2022 at 10:51 PM David Gow <davi...@google.com> wrote:
>
> On Sat, Jan 8, 2022 at 9:23 AM Daniel Latypov <dlat...@google.com> wrote:
> >
> > Currently, these macros are only really documented near the bottom of
> > https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
> >
> > E.g. it's likely someone might just not realize that
> > KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
> > or similar.
> >
> > This can also serve as a basic smoketest that the KUnit assert machinery
> > still works for all the macros.
> >
> > Signed-off-by: Daniel Latypov <dlat...@google.com>
> > ---
>
> I think this is a great idea. I will note that this definitely isn't a
> full test _of_ the assertion macros (in that it only exercises the
> success case), so keeping it as an example is probably best.
>
> A few possible ideas below, but I'm happy enough with this as-is regardless.
Applied the ideas locally.
>
> On Sat, Jan 8, 2022 at 9:23 AM Daniel Latypov <dlat...@google.com> wrote:
> >
> > Currently, these macros are only really documented near the bottom of
> > https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
> >
> > E.g. it's likely someone might just not realize that
> > KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
> > or similar.
> >
> > This can also serve as a basic smoketest that the KUnit assert machinery
> > still works for all the macros.
> >
> > Signed-off-by: Daniel Latypov <dlat...@google.com>
> > ---
>
> I think this is a great idea. I will note that this definitely isn't a
> full test _of_ the assertion macros (in that it only exercises the
> success case), so keeping it as an example is probably best.
>
> A few possible ideas below, but I'm happy enough with this as-is regardless.
It led to this diffstat
lib/kunit/kunit-example-test.c | 42 +++++++++++++++++++-----------------------
1 file changed, 19 insertions(+), 23 deletions(-)
So it's now a bit shorter and 8 of the added lines are new comments.
>
> Reviewed-by: David Gow <davi...@google.com>
>
> > lib/kunit/kunit-example-test.c | 46 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 46 insertions(+)
> >
> > diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> > index 51099b0ca29c..182a64c12541 100644
> > --- a/lib/kunit/kunit-example-test.c
> > +++ b/lib/kunit/kunit-example-test.c
> > @@ -69,6 +69,51 @@ static void example_mark_skipped_test(struct kunit *test)
> > /* This line should run */
> > kunit_info(test, "You should see this line.");
> > }
> > +
> > +/*
> > + * This test shows off all the KUNIT_EXPECT macros.
> > + */
> > +static void example_all_expect_macros_test(struct kunit *test)
> > +{
> > + KUNIT_EXPECT_TRUE(test, true);
> > + KUNIT_EXPECT_FALSE(test, false);
>
> _Maybe_ it's worth having a comment for each of these groups ('boolean
> assertions', 'integer assertions', 'pointer assertions', etc)?
>
> > +
> > + KUNIT_EXPECT_EQ(test, 1, 1);
> > + KUNIT_EXPECT_GE(test, 1, 1);
> > + KUNIT_EXPECT_LE(test, 1, 1);
> > + KUNIT_EXPECT_NE(test, 1, 0);
> > + KUNIT_EXPECT_GT(test, 1, 0);
> > + KUNIT_EXPECT_LT(test, 0, 1);
> > +
> > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, test);
> > + KUNIT_EXPECT_PTR_EQ(test, NULL, NULL);
> > + KUNIT_EXPECT_PTR_NE(test, test, NULL);
> > +
> > + KUNIT_EXPECT_STREQ(test, "hi", "hi");
> > + KUNIT_EXPECT_STRNEQ(test, "hi", "bye");
> > +
> > + /*
> > + * There are also _MSG variants of all of the above that let you include
> > + * additional text on failure.
> > + */
>
> There are also the ASSERT vs EXPECT variations. While it may be
> excessive to also include all of these, particularly in an example, it
> might be worth mentioning them in a comment somewhere?
+ /*
+ * There are also ASSERT variants of all of the above that abort test
+ * execution if they fail. Useful for memory allocations, etc.
+ */
+ KUNIT_ASSERT_GT(test, sizeof(char), 0);
+
>
> Alternatively, if this is bloating the example too much, we could have
> only one example each of the ASSERT and _MSG variants.
>
> > + KUNIT_EXPECT_TRUE_MSG(test, true, "msg");
> > + KUNIT_EXPECT_FALSE_MSG(test, false, "msg");
>
> Part of me feels that a better message than "msg" would be nice to
> have here, but I can't think of a good one. Maybe (particularly for
> the less obvious integer/string/pointer macros below), having a
> description of what's being asserted?
+ KUNIT_EXPECT_GT_MSG(test, sizeof(int), 0, "Your ints are 0-bit?!");
+ KUNIT_ASSERT_GT_MSG(test, sizeof(int), 0, "Your ints are 0-bit?!");
Daniel Latypov
Jan 11, 2022, 1:34:25 PM1/11/22
to David Gow, brendan...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
> On Sat, Jan 8, 2022 at 9:23 AM Daniel Latypov <dlat...@google.com> wrote:
> >
> > The `struct kunit* test` field in kunit_assert is unused.
> > Note: we have access to `test` where we need it via the string_stream
> > object. I assume `test` in `kunit_assert` predates this and was leftover
> > after some refactoring.
>
> Note that I don't like the idea of accessing 'test' via the string
> stream in general, but we don't seem to ever actually do this (as far
> as I can tell). Maybe if we wanted to be super nitpicky, rewording the
> note to say "if we need it" rather than "where we need it" would be
> clearer.
Ah, I had meant "where we need it" == "where string_stream needs it,
> >
> > The `struct kunit* test` field in kunit_assert is unused.
> > Note: we have access to `test` where we need it via the string_stream
> > object. I assume `test` in `kunit_assert` predates this and was leftover
> > after some refactoring.
>
> Note that I don't like the idea of accessing 'test' via the string
> stream in general, but we don't seem to ever actually do this (as far
> as I can tell). Maybe if we wanted to be super nitpicky, rewording the
> note to say "if we need it" rather than "where we need it" would be
> clearer.
as its the only user that doesn't get test passed as a parameter
everywhere".
Updated the wording to
Note: string_stream needs it, but it has its own `test` field.
Daniel Latypov
Jan 11, 2022, 1:42:18 PM1/11/22
to David Gow, Brendan Higgins, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
Talking offline, Brendan seemed ok with it, so we have 2 votes of
"it's good enough".
Daniel Latypov
Jan 11, 2022, 2:21:27 PM1/11/22
to David Gow, Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Linus Torvalds
Daniel Latypov
Jan 11, 2022, 2:42:38 PM1/11/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
Every KUNIT_ASSERT/EXPECT() invocation puts a `kunit_assert` object onto
the stack. The most common one is `kunit_binary_assert` which is 88
bytes on UML. So in the cases where the compiler doesn't optimize this
away, we can very quickly blow up the stack size.
This series implements Linus' suggestion in [1].
Namely, we split out the file, line number, and assert_type
(EXPECT/ASSERT) out of kunit_assert.
We can also drop the entirely unused `struct kunit *test` field, saving
a bit more space as well.
All together, sizeof(struct kunit_assert) went from 48 to 24 on UML.
Note: the other assert types are bigger, see [2].
This series also adds in an example test that uses all the base
the stack. The most common one is `kunit_binary_assert` which is 88
bytes on UML. So in the cases where the compiler doesn't optimize this
away, we can very quickly blow up the stack size.
This series implements Linus' suggestion in [1].
Namely, we split out the file, line number, and assert_type
(EXPECT/ASSERT) out of kunit_assert.
We can also drop the entirely unused `struct kunit *test` field, saving
a bit more space as well.
All together, sizeof(struct kunit_assert) went from 48 to 24 on UML.
Note: the other assert types are bigger, see [2].
KUNIT_EXPECT macros to both advertise their existence to new users and
serve as a smoketest for all these changes here.
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
[2] e.g. consider the most commonly used assert (also the biggest)
struct kunit_binary_assert {
struct kunit_assert assert;
const char *operation;
const char *left_text;
long long left_value;
const char *right_text;
long long right_value;
};
So sizeof(struct kunit_binary_assert) = went from 88 to 64.
I.e. only a 27% reduction instead of 50% in the most common case.
All 3 of the `const char*` could be split out into a `static` var as well,
but that's a bit trickier to do with how all the macros are written.
=== Changelog ===
serve as a smoketest for all these changes here.
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
[2] e.g. consider the most commonly used assert (also the biggest)
struct kunit_binary_assert {
struct kunit_assert assert;
const char *operation;
const char *left_text;
long long left_value;
const char *right_text;
long long right_value;
};
So sizeof(struct kunit_binary_assert) = went from 88 to 64.
I.e. only a 27% reduction instead of 50% in the most common case.
All 3 of the `const char*` could be split out into a `static` var as well,
but that's a bit trickier to do with how all the macros are written.
v1 -> v2:
* made the new example test more focused on documenting the macros
rather than using them all as a smoketest
* s/kunit_failed_assertion()/kunit_do_failed_assertion()
* added `unlikely()` to `if(!(pass))` check in KUNIT_ASSERTION()
Daniel Latypov (6):
kunit: add example test case showing off all the expect macros
kunit: move check if assertion passed into the macros
kunit: drop unused kunit* field in kunit_assert
kunit: factor out kunit_base_assert_format() call into kunit_fail()
kunit: split out part of kunit_assert into a static const
kunit: drop unused assert_type from kunit_assert and clean up macros
include/kunit/assert.h | 88 +++++++++++-----------------------
lib/kunit/assert.c | 15 ++----
lib/kunit/kunit-example-test.c | 42 ++++++++++++++++
lib/kunit/test.c | 27 +++++------
5 files changed, 117 insertions(+), 108 deletions(-)
base-commit: ad659ccb5412874c6a89d3588cb18857c00e9d0f
--
2.34.1.575.g55b058a8bb-goog
Daniel Latypov
Jan 11, 2022, 2:42:41 PM1/11/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
Currently, these macros are only really documented near the bottom of
https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
E.g. it's likely someone might just not realize that
KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
or similar.
This can also serve as a basic smoketest that the KUnit assert machinery
still works for all the macros.
Signed-off-by: Daniel Latypov <dlat...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
E.g. it's likely someone might just not realize that
KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
or similar.
This can also serve as a basic smoketest that the KUnit assert machinery
still works for all the macros.
Signed-off-by: Daniel Latypov <dlat...@google.com>
Reviewed-by: David Gow <davi...@google.com>
---
lib/kunit/kunit-example-test.c | 42 ++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
index 51099b0ca29c..4bbf37c04eba 100644
--- a/lib/kunit/kunit-example-test.c
+++ b/lib/kunit/kunit-example-test.c
@@ -69,6 +69,47 @@ static void example_mark_skipped_test(struct kunit *test)
/* This line should run */
kunit_info(test, "You should see this line.");
}
+
+/*
+ * This test shows off all the types of KUNIT_EXPECT macros.
kunit_info(test, "You should see this line.");
}
+
+/*
+ */
+static void example_all_expect_macros_test(struct kunit *test)
+{
+ /* Boolean assertions */
+static void example_all_expect_macros_test(struct kunit *test)
+{
+ KUNIT_EXPECT_TRUE(test, true);
+ KUNIT_EXPECT_FALSE(test, false);
+
+ KUNIT_EXPECT_FALSE(test, false);
+ /* Integer assertions */
+ KUNIT_EXPECT_EQ(test, 1, 1); /* check == */
+ KUNIT_EXPECT_GE(test, 1, 1); /* check >= */
+ KUNIT_EXPECT_LE(test, 1, 1); /* check <= */
+ KUNIT_EXPECT_NE(test, 1, 0); /* check != */
+ KUNIT_EXPECT_GT(test, 1, 0); /* check > */
+ KUNIT_EXPECT_LT(test, 0, 1); /* check < */
+
+ /* Pointer assertions */
+ KUNIT_EXPECT_NOT_ERR_OR_NULL(test, test);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, NULL);
+ KUNIT_EXPECT_PTR_NE(test, test, NULL);
+
+ /* String assertions */
+ KUNIT_EXPECT_PTR_EQ(test, NULL, NULL);
+ KUNIT_EXPECT_PTR_NE(test, test, NULL);
+
+ KUNIT_EXPECT_STREQ(test, "hi", "hi");
+ KUNIT_EXPECT_STRNEQ(test, "hi", "bye");
+
+ /*
+ KUNIT_EXPECT_STRNEQ(test, "hi", "bye");
+
+ /*
+ * There are also ASSERT variants of all of the above that abort test
+ * execution if they fail. Useful for memory allocations, etc.
+ */
+ KUNIT_ASSERT_GT(test, sizeof(char), 0);
+
+ * execution if they fail. Useful for memory allocations, etc.
+ */
+ KUNIT_ASSERT_GT(test, sizeof(char), 0);
+
+ /*
+ * There are also _MSG variants of all of the above that let you include
+ * additional text on failure.
+ */
+ * There are also _MSG variants of all of the above that let you include
+ * additional text on failure.
+ */
+ KUNIT_EXPECT_GT_MSG(test, sizeof(int), 0, "Your ints are 0-bit?!");
+ KUNIT_ASSERT_GT_MSG(test, sizeof(int), 0, "Your ints are 0-bit?!");
+}
+
+ KUNIT_ASSERT_GT_MSG(test, sizeof(int), 0, "Your ints are 0-bit?!");
+}
+
/*
* Here we make a list of all the test cases we want to add to the test suite
* below.
@@ -83,6 +124,7 @@ static struct kunit_case example_test_cases[] = {
* Here we make a list of all the test cases we want to add to the test suite
* below.
Daniel Latypov
Jan 11, 2022, 2:42:43 PM1/11/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
Currently the code always calls kunit_do_assertion() even though it does
nothing when `pass` is true.
This change moves the `if(!(pass))` check into the macro instead
and renames the function to kunit_do_failed_assertion().
nothing when `pass` is true.
This change moves the `if(!(pass))` check into the macro instead
I feel this a bit easier to read and understand.
This has the potential upside of avoiding a function call that does
nothing most of the time (assuming your tests are passing) but comes
with the downside of generating a bit more code and branches. We try to
This has the potential upside of avoiding a function call that does
nothing most of the time (assuming your tests are passing) but comes
mitigate the branches by tagging them with `unlikely()`.
This also means we don't have to initialize structs that we don't need,
which will become a tiny bit more expensive if we switch over to using
static variables to try and reduce stack usage. (There's runtime code
to check if the variable has been initialized yet or not).
Signed-off-by: Daniel Latypov <dlat...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
---
Reviewed-by: Brendan Higgins <brendan...@google.com>
include/kunit/test.h | 21 +++++++++++----------
lib/kunit/test.c | 13 ++++---------
2 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index b26400731c02..12cabd15449a 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -12,6 +12,7 @@
#include <kunit/assert.h>
#include <kunit/try-catch.h>
+#include <linux/compiler.h>
#include <linux/container_of.h>
#include <linux/err.h>
#include <linux/init.h>
@@ -770,18 +771,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
*/
#define KUNIT_SUCCEED(test) do {} while (0)
-void kunit_do_assertion(struct kunit *test,
- struct kunit_assert *assert,
- bool pass,
- const char *fmt, ...);
+void kunit_do_failed_assertion(struct kunit *test,
#define KUNIT_SUCCEED(test) do {} while (0)
-void kunit_do_assertion(struct kunit *test,
- struct kunit_assert *assert,
- bool pass,
- const char *fmt, ...);
+ struct kunit_assert *assert,
+ const char *fmt, ...);
+ const char *fmt, ...);
#define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do { \
- struct assert_class __assertion = INITIALIZER; \
- kunit_do_assertion(test, \
- &__assertion.assert, \
- pass, \
- fmt, \
- ##__VA_ARGS__); \
+ if (unlikely(!(pass))) { \
- struct assert_class __assertion = INITIALIZER; \
- kunit_do_assertion(test, \
- &__assertion.assert, \
- pass, \
- fmt, \
- ##__VA_ARGS__); \
+ struct assert_class __assertion = INITIALIZER; \
+ kunit_do_failed_assertion(test, \
+ &__assertion.assert, \
+ fmt, \
+ ##__VA_ARGS__); \
+ } \
} while (0)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c7ed4aabec04..3a52c321c280 100644
+ fmt, \
+ ##__VA_ARGS__); \
+ } \
} while (0)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -275,16 +275,11 @@ static void __noreturn kunit_abort(struct kunit *test)
WARN_ONCE(true, "Throw could not abort from test!\n");
}
-void kunit_do_assertion(struct kunit *test,
- struct kunit_assert *assert,
- bool pass,
- const char *fmt, ...)
+void kunit_do_failed_assertion(struct kunit *test,
+++ b/lib/kunit/test.c
@@ -275,16 +275,11 @@ static void __noreturn kunit_abort(struct kunit *test)
WARN_ONCE(true, "Throw could not abort from test!\n");
}
-void kunit_do_assertion(struct kunit *test,
- struct kunit_assert *assert,
- bool pass,
- const char *fmt, ...)
+ struct kunit_assert *assert,
+ const char *fmt, ...)
{
va_list args;
-
- if (pass)
- return;
-
va_start(args, fmt);
assert->message.fmt = fmt;
@@ -297,7 +292,7 @@ void kunit_do_assertion(struct kunit *test,
if (assert->type == KUNIT_ASSERTION)
kunit_abort(test);
}
-EXPORT_SYMBOL_GPL(kunit_do_assertion);
+EXPORT_SYMBOL_GPL(kunit_do_failed_assertion);
+ const char *fmt, ...)
{
va_list args;
-
- if (pass)
- return;
-
va_start(args, fmt);
assert->message.fmt = fmt;
@@ -297,7 +292,7 @@ void kunit_do_assertion(struct kunit *test,
if (assert->type == KUNIT_ASSERTION)
kunit_abort(test);
}
-EXPORT_SYMBOL_GPL(kunit_do_assertion);
Daniel Latypov
Jan 11, 2022, 2:42:46 PM1/11/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
The `struct kunit* test` field in kunit_assert is unused.
Note: string_stream needs it, but it has its own `test` field. I assume
`test` in `kunit_assert` predates this and was leftover after some
refactoring.
refactoring.
This patch removes the field and cleans up the macros to avoid
needlessly passing around `test`.
needlessly passing around `test`.
Signed-off-by: Daniel Latypov <dlat...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
include/kunit/assert.h | 45 ++++++++++++------------------------------
Reviewed-by: Brendan Higgins <brendan...@google.com>
include/kunit/test.h | 14 +++++--------
2 files changed, 18 insertions(+), 41 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index ad889b539ab3..3da6c792496c 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -30,7 +30,6 @@ enum kunit_assert_type {
/**
* struct kunit_assert - Data for printing a failed assertion or expectation.
* struct kunit_assert - Data for printing a failed assertion or expectation.
- * @test: the test case this expectation/assertion is associated with.
* @type: the type (either an expectation or an assertion) of this kunit_assert.
* @line: the source code line number that the expectation/assertion is at.
* @file: the file path of the source file that the expectation/assertion is in.
@@ -41,7 +40,6 @@ enum kunit_assert_type {
* @file: the file path of the source file that the expectation/assertion is in.
@@ -41,7 +40,6 @@ enum kunit_assert_type {
* format a string to a user reporting the failure.
*/
struct kunit_assert {
*/
struct kunit_assert {
- struct kunit *test;
enum kunit_assert_type type;
int line;
const char *file;
@@ -60,14 +58,12 @@ struct kunit_assert {
enum kunit_assert_type type;
int line;
const char *file;
/**
* KUNIT_INIT_ASSERT_STRUCT() - Initializer for a &struct kunit_assert.
- * @kunit: The test case that this expectation/assertion is associated with.
* @assert_type: The type (assertion or expectation) of this kunit_assert.
* @fmt: The formatting function which builds a string out of this kunit_assert.
*
* The base 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(kunit, assert_type, fmt) { \
- .test = kunit, \
+#define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \
.type = assert_type, \
.file = __FILE__, \
.line = __LINE__, \
@@ -96,15 +92,13 @@ void kunit_fail_assert_format(const struct kunit_assert *assert,
- .test = kunit, \
+#define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \
.type = assert_type, \
.file = __FILE__, \
.line = __LINE__, \
/**
* KUNIT_INIT_FAIL_ASSERT_STRUCT() - Initializer for &struct kunit_fail_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
*
*
* Initializes a &struct kunit_fail_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT(type) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_fail_assert_format) \
}
@@ -129,7 +123,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT(type) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_fail_assert_format) \
}
/**
* KUNIT_INIT_UNARY_ASSERT_STRUCT() - Initializes &struct kunit_unary_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
* @cond: A string representation of the expression asserted true or false.
* @expect_true: True if of type KUNIT_{EXPECT|ASSERT}_TRUE, false otherwise.
@@ -137,9 +130,8 @@ void kunit_unary_assert_format(const struct kunit_assert *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.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_UNARY_ASSERT_STRUCT(test, type, cond, expect_true) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(type, cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_unary_assert_format), \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(type, cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_unary_assert_format), \
.condition = cond, \
.expected_true = expect_true \
@@ -167,7 +159,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
.expected_true = expect_true \
/**
* KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
* &struct kunit_ptr_not_err_assert.
* KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
* &struct kunit_ptr_not_err_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
* @txt: A string representation of the expression passed to the expectation.
* @val: The actual evaluated pointer value of the expression.
@@ -175,9 +166,8 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *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.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(test, type, txt, val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(type, txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_ptr_not_err_assert_format), \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(type, txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_ptr_not_err_assert_format), \
.text = txt, \
.value = val \
@@ -212,7 +202,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
.value = val \
/**
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_assert.
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -223,15 +212,13 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
* @op_str: A string representation of the comparison operator (e.g. "==").
* @left_str: A string representation of the expression in the left slot.
* Initializes a &struct kunit_binary_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(type, \
op_str, \
left_str, \
- type, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(type, \
op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_assert_format), \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_assert_format), \
.operation = op_str, \
.left_text = left_str, \
.left_text = left_str, \
@@ -269,7 +256,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_ptr_assert.
- * @test: The test case that this expectation/assertion is associated with.
/**
* KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_ptr_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -280,15 +266,13 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
* @op_str: A string representation of the comparison operator (e.g. "==").
* @left_str: A string representation of the expression in the left slot.
* Initializes a &struct kunit_binary_ptr_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(type, \
op_str, \
left_str, \
- type, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(type, \
op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_ptr_assert_format), \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_ptr_assert_format), \
.operation = op_str, \
.left_text = left_str, \
@@ -326,7 +310,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
.left_text = left_str, \
/**
* KUNIT_INIT_BINARY_STR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_str_assert.
* KUNIT_INIT_BINARY_STR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_str_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -337,15 +320,13 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
* @op_str: A string representation of the comparison operator (e.g. "==").
* @left_str: A string representation of the expression in the left slot.
* Initializes a &struct kunit_binary_str_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(type, \
op_str, \
left_str, \
- type, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(type, \
op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_str_assert_format), \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_str_assert_format), \
.operation = op_str, \
.left_text = left_str, \
diff --git a/include/kunit/test.h b/include/kunit/test.h
.left_text = left_str, \
index 12cabd15449a..25ea3bce6663 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -790,7 +790,7 @@ void kunit_do_failed_assertion(struct kunit *test,
KUNIT_ASSERTION(test, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(test, assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
fmt, \
##__VA_ARGS__)
@@ -820,8 +820,7 @@ void kunit_do_failed_assertion(struct kunit *test,
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(test, assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
fmt, \
##__VA_ARGS__)
KUNIT_ASSERTION(test, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
- KUNIT_INIT_UNARY_ASSERT_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
#condition, \
expected_true), \
fmt, \
@@ -879,8 +878,7 @@ do { \
- assert_type, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
#condition, \
expected_true), \
fmt, \
KUNIT_ASSERTION(test, \
__left op __right, \
assert_class, \
- ASSERT_CLASS_INIT(test, \
- assert_type, \
+ ASSERT_CLASS_INIT(assert_type, \
#op, \
#left, \
__left, \
@@ -1234,8 +1232,7 @@ do { \
- assert_type, \
+ ASSERT_CLASS_INIT(assert_type, \
#op, \
#left, \
__left, \
KUNIT_ASSERTION(test, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
kunit_binary_str_assert, \
- KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
#op, \
#left, \
__left, \
@@ -1294,8 +1291,7 @@ do { \
- assert_type, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
#op, \
#left, \
__left, \
Daniel Latypov
Jan 11, 2022, 2:42:48 PM1/11/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
We call this function first thing for all the assertion `format()`
functions.
This is the part that prints the file and line number and assertion type
(EXPECTATION, ASSERTION).
Having it as part of the format functions lets us have the flexibility
to not print that information (or print it differently) for new
assertion types, but I think this we don't need that.
And in the future, we'd like to consider factoring that data (file,
line#, type) out of the kunit_assert struct and into a `static`
variable, as Linus suggested [1], so we'd need to extract it anyways.
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
Signed-off-by: Daniel Latypov <dlat...@google.com>
functions.
This is the part that prints the file and line number and assertion type
(EXPECTATION, ASSERTION).
Having it as part of the format functions lets us have the flexibility
to not print that information (or print it differently) for new
assertion types, but I think this we don't need that.
And in the future, we'd like to consider factoring that data (file,
line#, type) out of the kunit_assert struct and into a `static`
variable, as Linus suggested [1], so we'd need to extract it anyways.
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
Reviewed-by: David Gow <davi...@google.com>
---
lib/kunit/assert.c | 6 ------
lib/kunit/test.c | 1 +
2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
lib/kunit/test.c | 1 +
2 files changed, 1 insertion(+), 6 deletions(-)
index b972bda61c0c..4d9a1295efc7 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -40,7 +40,6 @@ EXPORT_SYMBOL_GPL(kunit_assert_print_msg);
void kunit_fail_assert_format(const struct kunit_assert *assert,
index 3a52c321c280..345a9dd88c27 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -255,6 +255,7 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
return;
}
+ kunit_base_assert_format(assert, stream);
assert->format(assert, stream);
kunit_print_string_stream(test, stream);
--
kunit_print_string_stream(test, stream);
2.34.1.575.g55b058a8bb-goog
Daniel Latypov
Jan 11, 2022, 2:42:50 PM1/11/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
This is per Linus's suggestion in [1].
The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
kunit_assert object onto the stack. Normally we rely on compilers to
elide this, but when that doesn't work out, this blows up the stack
usage of kunit test functions.
We can move some data off the stack by making it static.
This change introduces a new `struct kunit_loc` to hold the file and
line number and then just passing assert_type (EXPECT or ASSERT) as an
argument.
The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
kunit_assert object onto the stack. Normally we rely on compilers to
elide this, but when that doesn't work out, this blows up the stack
usage of kunit test functions.
We can move some data off the stack by making it static.
This change introduces a new `struct kunit_loc` to hold the file and
line number and then just passing assert_type (EXPECT or ASSERT) as an
argument.
In [1], it was suggested to also move out the format string as well, but
users could theoretically craft a format string at runtime, so we can't.
This change leaves a copy of `assert_type` in kunit_assert for now
because cleaning up all the macros to not pass it around is a bit more
involved.
Here's an example of the expanded code for KUNIT_FAIL():
if (__builtin_expect(!!(!(false)), 0)) {
users could theoretically craft a format string at runtime, so we can't.
This change leaves a copy of `assert_type` in kunit_assert for now
because cleaning up all the macros to not pass it around is a bit more
involved.
Here's an example of the expanded code for KUNIT_FAIL():
static const struct kunit_loc loc = { .file = ... };
struct kunit_fail_assert __assertion = { .assert = { .type ... };
kunit_do_failed_assertion(test, &loc, KUNIT_EXPECTATION, &__assertion.assert, ...);
};
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
Signed-off-by: Daniel Latypov <dlat...@google.com>
include/kunit/assert.h | 25 ++++++++++++++++---------
include/kunit/test.h | 12 +++++++++++-
lib/kunit/assert.c | 9 +++++----
lib/kunit/test.c | 15 +++++++++------
4 files changed, 41 insertions(+), 20 deletions(-)
include/kunit/test.h | 12 +++++++++++-
lib/kunit/assert.c | 9 +++++----
lib/kunit/test.c | 15 +++++++++------
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 3da6c792496c..4f91dbdb886a 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -28,11 +28,21 @@ enum kunit_assert_type {
KUNIT_EXPECTATION,
};
+/**
+ * struct kunit_loc - Identifies the source location of a line of code.
+ * @line: the line number in the file.
+ * @file: the file name.
+ */
+struct kunit_loc {
+ int line;
+ const char *file;
+};
+
+#define KUNIT_CURRENT_LOC { .file = __FILE__, .line = __LINE__ }
KUNIT_EXPECTATION,
};
+/**
+ * struct kunit_loc - Identifies the source location of a line of code.
+ * @line: the line number in the file.
+ * @file: the file name.
+ */
+struct kunit_loc {
+ int line;
+ const char *file;
+};
+
+
/**
* struct kunit_assert - Data for printing a failed assertion or expectation.
* struct kunit_assert - Data for printing a failed assertion or expectation.
* @type: the type (either an expectation or an assertion) of this kunit_assert.
- * @line: the source code line number that the expectation/assertion is at.
- * @file: the file path of the source file that the expectation/assertion is in.
* @message: an optional message to provide additional context.
* @format: a function which formats the data in this kunit_assert to a string.
*
@@ -40,9 +50,7 @@ enum kunit_assert_type {
* @format: a function which formats the data in this kunit_assert to a string.
*
* format a string to a user reporting the failure.
*/
struct kunit_assert {
*/
struct kunit_assert {
- enum kunit_assert_type type;
- int line;
- const char *file;
+ enum kunit_assert_type type; // TODO(dlat...@google.com): delete this
- int line;
- const char *file;
struct va_format message;
void (*format)(const struct kunit_assert *assert,
struct string_stream *stream);
@@ -65,14 +73,13 @@ struct kunit_assert {
*/
struct string_stream *stream);
@@ -65,14 +73,13 @@ struct kunit_assert {
*/
#define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \
.type = assert_type, \
.type = assert_type, \
- .file = __FILE__, \
- .line = __LINE__, \
- .line = __LINE__, \
.message = KUNIT_INIT_VA_FMT_NULL, \
.format = fmt \
}
.format = fmt \
}
-void kunit_base_assert_format(const struct kunit_assert *assert,
- struct string_stream *stream);
+void kunit_assert_prologue(const struct kunit_loc *loc,
+ enum kunit_assert_type type,
+ struct string_stream *stream);
void kunit_assert_print_msg(const struct kunit_assert *assert,
- struct string_stream *stream);
+void kunit_assert_prologue(const struct kunit_loc *loc,
+ enum kunit_assert_type type,
+ struct string_stream *stream);
struct string_stream *stream);
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 25ea3bce6663..7b752175e614 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -772,13 +772,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
#define KUNIT_SUCCEED(test) do {} while (0)
void kunit_do_failed_assertion(struct kunit *test,
+ const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct kunit_assert *assert,
const char *fmt, ...);
-#define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do { \
+ enum kunit_assert_type type,
struct kunit_assert *assert,
const char *fmt, ...);
+#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, 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, \
fmt, \
##__VA_ARGS__); \
@@ -788,6 +793,7 @@ void kunit_do_failed_assertion(struct kunit *test,
+ assert_type, \
&__assertion.assert, \
fmt, \
##__VA_ARGS__); \
#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \
KUNIT_ASSERTION(test, \
+ assert_type, \
false, \
kunit_fail_assert, \
KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
fmt, \
...) \
KUNIT_ASSERTION(test, \
+ assert_type, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
@@ -876,6 +883,7 @@ do { \
typeof(right) __right = (right); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
\
KUNIT_ASSERTION(test, \
__left op __right, \
assert_class, \
ASSERT_CLASS_INIT(assert_type, \
@@ -1230,6 +1238,7 @@ do { \
const char *__right = (right); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
kunit_binary_str_assert, \
@@ -1289,6 +1298,7 @@ do { \
typeof(ptr) __ptr = (ptr); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
\
KUNIT_ASSERTION(test, \
+ assert_type, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
index 4d9a1295efc7..9f4492a8e24e 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -10,12 +10,13 @@
#include "string-stream.h"
-void kunit_base_assert_format(const struct kunit_assert *assert,
+void kunit_assert_prologue(const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct string_stream *stream)
{
const char *expect_or_assert = NULL;
- switch (assert->type) {
+ switch (type) {
case KUNIT_EXPECTATION:
expect_or_assert = "EXPECTATION";
break;
@@ -25,9 +26,9 @@ void kunit_base_assert_format(const struct kunit_assert *assert,
}
string_stream_add(stream, "%s FAILED at %s:%d\n",
- expect_or_assert, assert->file, assert->line);
+ expect_or_assert, loc->file, loc->line);
}
-EXPORT_SYMBOL_GPL(kunit_base_assert_format);
+EXPORT_SYMBOL_GPL(kunit_assert_prologue);
void kunit_assert_print_msg(const struct kunit_assert *assert,
#include "string-stream.h"
-void kunit_base_assert_format(const struct kunit_assert *assert,
+void kunit_assert_prologue(const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct string_stream *stream)
{
const char *expect_or_assert = NULL;
- switch (assert->type) {
+ switch (type) {
case KUNIT_EXPECTATION:
expect_or_assert = "EXPECTATION";
break;
@@ -25,9 +26,9 @@ void kunit_base_assert_format(const struct kunit_assert *assert,
}
string_stream_add(stream, "%s FAILED at %s:%d\n",
- expect_or_assert, assert->file, assert->line);
+ expect_or_assert, loc->file, loc->line);
}
-EXPORT_SYMBOL_GPL(kunit_base_assert_format);
+EXPORT_SYMBOL_GPL(kunit_assert_prologue);
struct string_stream *stream)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 345a9dd88c27..7dec3248562f 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -240,7 +240,8 @@ static void kunit_print_string_stream(struct kunit *test,
}
}
-static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
+static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
+ enum kunit_assert_type type, struct kunit_assert *assert)
{
struct string_stream *stream;
@@ -250,12 +251,12 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
if (!stream) {
WARN(true,
"Could not allocate stream to print failed assertion in %s:%d\n",
- assert->file,
- assert->line);
+ loc->file,
+ loc->line);
return;
}
- kunit_base_assert_format(assert, stream);
+ kunit_assert_prologue(loc, type, stream);
}
}
-static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
+static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
+ enum kunit_assert_type type, struct kunit_assert *assert)
{
struct string_stream *stream;
@@ -250,12 +251,12 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
if (!stream) {
WARN(true,
"Could not allocate stream to print failed assertion in %s:%d\n",
- assert->file,
- assert->line);
+ loc->file,
+ loc->line);
return;
}
- kunit_base_assert_format(assert, stream);
assert->format(assert, stream);
kunit_print_string_stream(test, stream);
@@ -277,6 +278,8 @@ static void __noreturn kunit_abort(struct kunit *test)
kunit_print_string_stream(test, stream);
}
void kunit_do_failed_assertion(struct kunit *test,
+ const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct kunit_assert *assert,
const char *fmt, ...)
{
@@ -286,11 +289,11 @@ void kunit_do_failed_assertion(struct kunit *test,
+ enum kunit_assert_type type,
struct kunit_assert *assert,
const char *fmt, ...)
{
assert->message.fmt = fmt;
assert->message.va = &args;
- kunit_fail(test, assert);
+ kunit_fail(test, loc, type, assert);
va_end(args);
- if (assert->type == KUNIT_ASSERTION)
+ if (type == KUNIT_ASSERTION)
kunit_abort(test);
}
EXPORT_SYMBOL_GPL(kunit_do_failed_assertion);
assert->message.va = &args;
- kunit_fail(test, assert);
+ kunit_fail(test, loc, type, assert);
va_end(args);
- if (assert->type == KUNIT_ASSERTION)
+ if (type == KUNIT_ASSERTION)
kunit_abort(test);
}
--
2.34.1.575.g55b058a8bb-goog
Daniel Latypov
Jan 11, 2022, 2:42:52 PM1/11/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
This field has been split out from kunit_assert to make the struct less
heavy along with the filename and line number.
This change drops the assert_type field and cleans up all the macros
that were plumbing assert_type into kunit_assert.
Signed-off-by: Daniel Latypov <dlat...@google.com>
include/kunit/assert.h | 46 +++++++++++++-----------------------------
heavy along with the filename and line number.
This change drops the assert_type field and cleans up all the macros
that were plumbing assert_type into kunit_assert.
Signed-off-by: Daniel Latypov <dlat...@google.com>
include/kunit/test.h | 14 +++++--------
2 files changed, 19 insertions(+), 41 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 4f91dbdb886a..21299232c120 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -42,7 +42,6 @@ struct kunit_loc {
/**
* struct kunit_assert - Data for printing a failed assertion or expectation.
- * @type: the type (either an expectation or an assertion) of this kunit_assert.
* 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.
*
* @format: a function which formats the data in this kunit_assert to a string.
*
@@ -50,7 +49,6 @@ struct kunit_loc {
* format a string to a user reporting the failure.
*/
struct kunit_assert {
- enum kunit_assert_type type; // TODO(dlat...@google.com): delete this
*/
struct kunit_assert {
struct va_format message;
void (*format)(const struct kunit_assert *assert,
struct string_stream *stream);
@@ -66,13 +64,11 @@ struct kunit_assert {
void (*format)(const struct kunit_assert *assert,
struct string_stream *stream);
/**
* 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.
*/
*
* The base initializer for a &struct kunit_assert.
*/
-#define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \
- .type = assert_type, \
+#define KUNIT_INIT_ASSERT_STRUCT(fmt) { \
- .type = assert_type, \
+#define KUNIT_INIT_ASSERT_STRUCT(fmt) { \
.message = KUNIT_INIT_VA_FMT_NULL, \
.format = fmt \
}
@@ -98,15 +94,13 @@ void kunit_fail_assert_format(const struct kunit_assert *assert,
.format = fmt \
}
struct string_stream *stream);
/**
- * KUNIT_INIT_FAIL_ASSERT_STRUCT() - Initializer for &struct kunit_fail_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
+ * 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.
*/
* Initializes a &struct kunit_fail_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_FAIL_ASSERT_STRUCT(type) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_fail_assert_format) \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_fail_assert_format) \
}
/**
@@ -130,16 +124,14 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_fail_assert_format) \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_fail_assert_format) \
}
/**
/**
* 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.
*
* @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.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_UNARY_ASSERT_STRUCT(type, cond, expect_true) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_unary_assert_format), \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_unary_assert_format), \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_unary_assert_format), \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_unary_assert_format), \
.condition = cond, \
.expected_true = expect_true \
}
@@ -166,16 +158,14 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
.expected_true = expect_true \
}
/**
* KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
* &struct kunit_ptr_not_err_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
* 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.
*
* @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.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(type, txt, val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_ptr_not_err_assert_format), \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_ptr_not_err_assert_format), \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_ptr_not_err_assert_format), \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_ptr_not_err_assert_format), \
.text = txt, \
.value = val \
}
@@ -209,7 +199,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
.value = val \
}
/**
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_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.
* @left_str: A string representation of the expression in the left slot.
@@ -219,14 +208,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
* Initializes a &struct kunit_binary_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_ASSERT_STRUCT(type, \
- op_str, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(op_str, \
left_str, \
- op_str, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_assert_format), \
- kunit_binary_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_assert_format), \
.operation = op_str, \
.left_text = left_str, \
.left_value = left_val, \
.left_text = left_str, \
@@ -273,14 +260,12 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
* Initializes a &struct kunit_binary_ptr_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(type, \
- op_str, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(op_str, \
left_str, \
- op_str, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_ptr_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_ptr_assert_format), \
- kunit_binary_ptr_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_ptr_assert_format), \
.operation = op_str, \
.left_text = left_str, \
.left_value = left_val, \
.left_text = left_str, \
@@ -317,7 +302,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_BINARY_STR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_str_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
* 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.
* @left_str: A string representation of the expression in the left slot.
@@ -327,14 +311,12 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
* Initializes a &struct kunit_binary_str_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(type, \
- op_str, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(op_str, \
left_str, \
- op_str, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_str_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_str_assert_format), \
- kunit_binary_str_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_str_assert_format), \
.operation = op_str, \
.left_text = left_str, \
.left_value = left_val, \
.left_text = left_str, \
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 7b752175e614..5964af750d93 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -796,7 +796,7 @@ void kunit_do_failed_assertion(struct kunit *test,
assert_type, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT, \
fmt, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT, \
##__VA_ARGS__)
@@ -827,8 +827,7 @@ void kunit_do_failed_assertion(struct kunit *test,
assert_type, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
- KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
- #condition, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
expected_true), \
fmt, \
##__VA_ARGS__)
@@ -886,8 +885,7 @@ do { \
- #condition, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
expected_true), \
fmt, \
##__VA_ARGS__)
assert_type, \
__left op __right, \
assert_class, \
__left op __right, \
assert_class, \
- ASSERT_CLASS_INIT(assert_type, \
- #op, \
+ ASSERT_CLASS_INIT(#op, \
#left, \
__left, \
#right, \
@@ -1241,8 +1239,7 @@ do { \
- #op, \
+ ASSERT_CLASS_INIT(#op, \
#left, \
__left, \
#right, \
assert_type, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
- KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
- #op, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(#op, \
#left, \
__left, \
#right, \
@@ -1301,8 +1298,7 @@ do { \
- #op, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(#op, \
#left, \
__left, \
#right, \
Brendan Higgins
Jan 11, 2022, 4:35:35 PM1/11/22
to Daniel Latypov, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
On Tue, Jan 11, 2022 at 2:42 PM Daniel Latypov <dlat...@google.com> wrote:
>
> This is per Linus's suggestion in [1].
>
> The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
> kunit_assert object onto the stack. Normally we rely on compilers to
> elide this, but when that doesn't work out, this blows up the stack
> usage of kunit test functions.
>
> We can move some data off the stack by making it static.
> This change introduces a new `struct kunit_loc` to hold the file and
> line number and then just passing assert_type (EXPECT or ASSERT) as an
> argument.
>
> In [1], it was suggested to also move out the format string as well, but
> users could theoretically craft a format string at runtime, so we can't.
>
> This change leaves a copy of `assert_type` in kunit_assert for now
> because cleaning up all the macros to not pass it around is a bit more
> involved.
>
> Here's an example of the expanded code for KUNIT_FAIL():
> if (__builtin_expect(!!(!(false)), 0)) {
> static const struct kunit_loc loc = { .file = ... };
> struct kunit_fail_assert __assertion = { .assert = { .type ... };
> kunit_do_failed_assertion(test, &loc, KUNIT_EXPECTATION, &__assertion.assert, ...);
> };
>
> [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> Suggested-by: Linus Torvalds <torv...@linux-foundation.org>
> Reviewed-by: David Gow <davi...@google.com>
One question below, but other than that,
>
> This is per Linus's suggestion in [1].
>
> The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
> kunit_assert object onto the stack. Normally we rely on compilers to
> elide this, but when that doesn't work out, this blows up the stack
> usage of kunit test functions.
>
> We can move some data off the stack by making it static.
> This change introduces a new `struct kunit_loc` to hold the file and
> line number and then just passing assert_type (EXPECT or ASSERT) as an
> argument.
>
> In [1], it was suggested to also move out the format string as well, but
> users could theoretically craft a format string at runtime, so we can't.
>
> This change leaves a copy of `assert_type` in kunit_assert for now
> because cleaning up all the macros to not pass it around is a bit more
> involved.
>
> Here's an example of the expanded code for KUNIT_FAIL():
> if (__builtin_expect(!!(!(false)), 0)) {
> static const struct kunit_loc loc = { .file = ... };
> struct kunit_fail_assert __assertion = { .assert = { .type ... };
> kunit_do_failed_assertion(test, &loc, KUNIT_EXPECTATION, &__assertion.assert, ...);
> };
>
> [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> Suggested-by: Linus Torvalds <torv...@linux-foundation.org>
> Reviewed-by: David Gow <davi...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
Daniel Latypov
Jan 11, 2022, 4:41:12 PM1/11/22
to Brendan Higgins, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
We leave in the type field in this change so we can clean that up and
all the macros all at once.
This TODO is addressed and removed in the next commit, so I was being
a bit lazy with the TODO.
I was hoping people could check `git blame` and find the context they
need, if people do somehow find their way to this intermediate commit.
If you want, I can update the TODO message to be more fleshed out.
Something like
TODO(...): delete this unused field when we've updated all the related
KUNIT_INIT_ASSERT macros.
Brendan Higgins
Jan 11, 2022, 4:42:41 PM1/11/22
to Daniel Latypov, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
it is removed in a later patch in the comment?
Brendan Higgins
Jan 11, 2022, 4:43:41 PM1/11/22
to Daniel Latypov, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
Daniel Latypov
Jan 11, 2022, 5:12:07 PM1/11/22
to Brendan Higgins, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
On Tue, Jan 11, 2022 at 1:43 PM Brendan Higgins
Ack, applied this locally so now it looks like
struct kunit_assert {
+ // TODO(dlat...@google.com): delete this unused field when we've
+ // updated all the related KUNIT_INIT_ASSERT* macros.
I'll wait to see if there's any other tweaks we want before sending out a v3.
struct kunit_assert {
+ // TODO(dlat...@google.com): delete this unused field when we've
+ // updated all the related KUNIT_INIT_ASSERT* macros.
enum kunit_assert_type type;
- int line;
- const char *file;
struct va_format message;
- int line;
- const char *file;
I'll wait to see if there's any other tweaks we want before sending out a v3.
Brendan Higgins
Jan 11, 2022, 5:42:21 PM1/11/22
to Daniel Latypov, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
> This field has been split out from kunit_assert to make the struct less
> heavy along with the filename and line number.
>
> This change drops the assert_type field and cleans up all the macros
> that were plumbing assert_type into kunit_assert.
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> Reviewed-by: David Gow <davi...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
> heavy along with the filename and line number.
>
> This change drops the assert_type field and cleans up all the macros
> that were plumbing assert_type into kunit_assert.
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> Reviewed-by: David Gow <davi...@google.com>
David Gow
Jan 11, 2022, 8:27:27 PM1/11/22
to Daniel Latypov, brendan...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
On Wed, Jan 12, 2022 at 3:42 AM Daniel Latypov <dlat...@google.com> wrote:
>
> Currently the code always calls kunit_do_assertion() even though it does
> nothing when `pass` is true.
>
> This change moves the `if(!(pass))` check into the macro instead
> and renames the function to kunit_do_failed_assertion().
> I feel this a bit easier to read and understand.
>
> This has the potential upside of avoiding a function call that does
> nothing most of the time (assuming your tests are passing) but comes
> with the downside of generating a bit more code and branches. We try to
> mitigate the branches by tagging them with `unlikely()`.
>
> This also means we don't have to initialize structs that we don't need,
> which will become a tiny bit more expensive if we switch over to using
> static variables to try and reduce stack usage. (There's runtime code
> to check if the variable has been initialized yet or not).
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> Reviewed-by: Brendan Higgins <brendan...@google.com>
> ---
This looks good. I'm still not 100% sold that putting the if() outside
>
> Currently the code always calls kunit_do_assertion() even though it does
> nothing when `pass` is true.
>
> This change moves the `if(!(pass))` check into the macro instead
> and renames the function to kunit_do_failed_assertion().
> I feel this a bit easier to read and understand.
>
> This has the potential upside of avoiding a function call that does
> nothing most of the time (assuming your tests are passing) but comes
> with the downside of generating a bit more code and branches. We try to
> mitigate the branches by tagging them with `unlikely()`.
>
> This also means we don't have to initialize structs that we don't need,
> which will become a tiny bit more expensive if we switch over to using
> static variables to try and reduce stack usage. (There's runtime code
> to check if the variable has been initialized yet or not).
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> Reviewed-by: Brendan Higgins <brendan...@google.com>
> ---
the function is significantly easier to read, but I don't think it's
harder to read either, and getting rid of the function call is
probably worth it.
Daniel Latypov
Jan 13, 2022, 11:59:41 AM1/13/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
*** BLURB HERE ***
v2 -> v3:
* elaborate on intermediate TODO in patch 5 (deleted in patch 6)
* update with more Reviewed-by's
Daniel Latypov (6):
kunit: add example test case showing off all the expect macros
kunit: move check if assertion passed into the macros
kunit: drop unused kunit* field in kunit_assert
kunit: factor out kunit_base_assert_format() call into kunit_fail()
kunit: split out part of kunit_assert into a static const
kunit: drop unused assert_type from kunit_assert and clean up macros
include/kunit/assert.h | 88 +++++++++++-----------------------
include/kunit/test.h | 53 ++++++++++----------
lib/kunit/assert.c | 15 ++----
lib/kunit/kunit-example-test.c | 42 ++++++++++++++++
lib/kunit/test.c | 27 +++++------
5 files changed, 117 insertions(+), 108 deletions(-)
base-commit: ad659ccb5412874c6a89d3588cb18857c00e9d0f
--
2.34.1.703.g22d0c6ccf7-goog
* elaborate on intermediate TODO in patch 5 (deleted in patch 6)
* update with more Reviewed-by's
Daniel Latypov (6):
kunit: add example test case showing off all the expect macros
kunit: move check if assertion passed into the macros
kunit: drop unused kunit* field in kunit_assert
kunit: factor out kunit_base_assert_format() call into kunit_fail()
kunit: split out part of kunit_assert into a static const
kunit: drop unused assert_type from kunit_assert and clean up macros
include/kunit/assert.h | 88 +++++++++++-----------------------
include/kunit/test.h | 53 ++++++++++----------
lib/kunit/assert.c | 15 ++----
lib/kunit/kunit-example-test.c | 42 ++++++++++++++++
lib/kunit/test.c | 27 +++++------
5 files changed, 117 insertions(+), 108 deletions(-)
base-commit: ad659ccb5412874c6a89d3588cb18857c00e9d0f
--
Daniel Latypov
Jan 13, 2022, 11:59:50 AM1/13/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
Currently, these macros are only really documented near the bottom of
https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
E.g. it's likely someone might just not realize that
KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
or similar.
This can also serve as a basic smoketest that the KUnit assert machinery
still works for all the macros.
https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html#c.KUNIT_FAIL.
E.g. it's likely someone might just not realize that
KUNIT_EXPECT_STREQ() exists and instead use KUNIT_EXPECT_FALSE(strcmp())
or similar.
This can also serve as a basic smoketest that the KUnit assert machinery
still works for all the macros.
Signed-off-by: Daniel Latypov <dlat...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
Daniel Latypov
Jan 13, 2022, 11:59:53 AM1/13/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
Currently the code always calls kunit_do_assertion() even though it does
nothing when `pass` is true.
This change moves the `if(!(pass))` check into the macro instead
and renames the function to kunit_do_failed_assertion().
I feel this a bit easier to read and understand.
This has the potential upside of avoiding a function call that does
nothing most of the time (assuming your tests are passing) but comes
with the downside of generating a bit more code and branches. We try to
mitigate the branches by tagging them with `unlikely()`.
This also means we don't have to initialize structs that we don't need,
which will become a tiny bit more expensive if we switch over to using
static variables to try and reduce stack usage. (There's runtime code
to check if the variable has been initialized yet or not).
nothing when `pass` is true.
This change moves the `if(!(pass))` check into the macro instead
and renames the function to kunit_do_failed_assertion().
I feel this a bit easier to read and understand.
This has the potential upside of avoiding a function call that does
nothing most of the time (assuming your tests are passing) but comes
with the downside of generating a bit more code and branches. We try to
mitigate the branches by tagging them with `unlikely()`.
This also means we don't have to initialize structs that we don't need,
which will become a tiny bit more expensive if we switch over to using
static variables to try and reduce stack usage. (There's runtime code
to check if the variable has been initialized yet or not).
Signed-off-by: Daniel Latypov <dlat...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
Reviewed-by: David Gow <davi...@google.com>
---
Reviewed-by: Brendan Higgins <brendan...@google.com>
Reviewed-by: David Gow <davi...@google.com>
---
Daniel Latypov
Jan 13, 2022, 11:59:55 AM1/13/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
The `struct kunit* test` field in kunit_assert is unused.
Note: string_stream needs it, but it has its own `test` field. I assume
`test` in `kunit_assert` predates this and was leftover after some
refactoring.
This patch removes the field and cleans up the macros to avoid
needlessly passing around `test`.
Note: string_stream needs it, but it has its own `test` field. I assume
`test` in `kunit_assert` predates this and was leftover after some
refactoring.
This patch removes the field and cleans up the macros to avoid
needlessly passing around `test`.
Signed-off-by: Daniel Latypov <dlat...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
Reviewed-by: David Gow <davi...@google.com>
---
include/kunit/assert.h | 45 ++++++++++++------------------------------
Reviewed-by: Brendan Higgins <brendan...@google.com>
Reviewed-by: David Gow <davi...@google.com>
---
include/kunit/test.h | 14 +++++--------
2 files changed, 18 insertions(+), 41 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index ad889b539ab3..3da6c792496c 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -30,7 +30,6 @@ enum kunit_assert_type {
/**
* struct kunit_assert - Data for printing a failed assertion or expectation.
* struct kunit_assert - Data for printing a failed assertion or expectation.
- * @test: the test case this expectation/assertion is associated with.
* @type: the type (either an expectation or an assertion) of this kunit_assert.
* @line: the source code line number that the expectation/assertion is at.
* @file: the file path of the source file that the expectation/assertion is in.
@@ -41,7 +40,6 @@ enum kunit_assert_type {
* format a string to a user reporting the failure.
*/
struct kunit_assert {
*/
struct kunit_assert {
- struct kunit *test;
enum kunit_assert_type type;
int line;
const char *file;
@@ -60,14 +58,12 @@ struct kunit_assert {
enum kunit_assert_type type;
int line;
const char *file;
/**
* KUNIT_INIT_ASSERT_STRUCT() - Initializer for a &struct kunit_assert.
- * @kunit: The test case that this expectation/assertion is associated with.
* @assert_type: The type (assertion or expectation) of this kunit_assert.
* @fmt: The formatting function which builds a string out of this kunit_assert.
*
* The base 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(kunit, assert_type, fmt) { \
- .test = kunit, \
+#define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \
- .test = kunit, \
.type = assert_type, \
.file = __FILE__, \
.line = __LINE__, \
@@ -96,15 +92,13 @@ void kunit_fail_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_FAIL_ASSERT_STRUCT() - Initializer for &struct kunit_fail_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
*
*
* Initializes a &struct kunit_fail_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT(type) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_fail_assert_format) \
}
@@ -129,7 +123,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT(type) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_fail_assert_format) \
}
/**
* KUNIT_INIT_UNARY_ASSERT_STRUCT() - Initializes &struct kunit_unary_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
* @cond: A string representation of the expression asserted true or false.
* @expect_true: True if of type KUNIT_{EXPECT|ASSERT}_TRUE, false otherwise.
@@ -137,9 +130,8 @@ void kunit_unary_assert_format(const struct kunit_assert *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.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_UNARY_ASSERT_STRUCT(test, type, cond, expect_true) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(type, cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_unary_assert_format), \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(type, cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_unary_assert_format), \
.condition = cond, \
.expected_true = expect_true \
@@ -167,7 +159,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
.expected_true = expect_true \
/**
* KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
* &struct kunit_ptr_not_err_assert.
* KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
* &struct kunit_ptr_not_err_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @type: The type (assertion or expectation) of this kunit_assert.
* @txt: A string representation of the expression passed to the expectation.
* @val: The actual evaluated pointer value of the expression.
@@ -175,9 +166,8 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *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.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(test, type, txt, val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(type, txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_ptr_not_err_assert_format), \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(type, txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_ptr_not_err_assert_format), \
.text = txt, \
.value = val \
@@ -212,7 +202,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
.value = val \
/**
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_assert.
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -223,15 +212,13 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
* @op_str: A string representation of the comparison operator (e.g. "==").
* @left_str: A string representation of the expression in the left slot.
* Initializes a &struct kunit_binary_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(type, \
op_str, \
left_str, \
- type, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(type, \
op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_assert_format), \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_assert_format), \
.operation = op_str, \
.left_text = left_str, \
.left_text = left_str, \
@@ -269,7 +256,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_ptr_assert.
- * @test: The test case that this expectation/assertion is associated with.
/**
* KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_ptr_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -280,15 +266,13 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
* @op_str: A string representation of the comparison operator (e.g. "==").
* @left_str: A string representation of the expression in the left slot.
* Initializes a &struct kunit_binary_ptr_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(type, \
op_str, \
left_str, \
- type, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(type, \
op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_ptr_assert_format), \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_ptr_assert_format), \
.operation = op_str, \
.left_text = left_str, \
@@ -326,7 +310,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
.left_text = left_str, \
/**
* KUNIT_INIT_BINARY_STR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_str_assert.
* KUNIT_INIT_BINARY_STR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_str_assert.
- * @test: The test case that this expectation/assertion is associated with.
* @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.
@@ -337,15 +320,13 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
* @op_str: A string representation of the comparison operator (e.g. "==").
* @left_str: A string representation of the expression in the left slot.
* Initializes a &struct kunit_binary_str_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(test, \
- type, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(type, \
op_str, \
left_str, \
- type, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(type, \
op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_str_assert_format), \
- type, \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
kunit_binary_str_assert_format), \
.operation = op_str, \
.left_text = left_str, \
diff --git a/include/kunit/test.h b/include/kunit/test.h
.left_text = left_str, \
index 12cabd15449a..25ea3bce6663 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -790,7 +790,7 @@ void kunit_do_failed_assertion(struct kunit *test,
KUNIT_ASSERTION(test, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(test, assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
fmt, \
##__VA_ARGS__)
@@ -820,8 +820,7 @@ void kunit_do_failed_assertion(struct kunit *test,
KUNIT_ASSERTION(test, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(test, assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
fmt, \
##__VA_ARGS__)
KUNIT_ASSERTION(test, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
- KUNIT_INIT_UNARY_ASSERT_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
#condition, \
expected_true), \
fmt, \
@@ -879,8 +878,7 @@ do { \
KUNIT_ASSERTION(test, \
__left op __right, \
- assert_type, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
#condition, \
expected_true), \
fmt, \
@@ -879,8 +878,7 @@ do { \
KUNIT_ASSERTION(test, \
assert_class, \
- ASSERT_CLASS_INIT(test, \
- assert_type, \
+ ASSERT_CLASS_INIT(assert_type, \
#op, \
#left, \
__left, \
@@ -1234,8 +1232,7 @@ do { \
KUNIT_ASSERTION(test, \
- assert_type, \
+ ASSERT_CLASS_INIT(assert_type, \
#op, \
#left, \
__left, \
@@ -1234,8 +1232,7 @@ do { \
KUNIT_ASSERTION(test, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
kunit_binary_str_assert, \
- KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
#op, \
#left, \
__left, \
@@ -1294,8 +1291,7 @@ do { \
KUNIT_ASSERTION(test, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
- KUNIT_INIT_PTR_NOT_ERR_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
#ptr, \
__ptr), \
fmt, \
--
2.34.1.703.g22d0c6ccf7-goog
- assert_type, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
#op, \
#left, \
__left, \
@@ -1294,8 +1291,7 @@ do { \
KUNIT_ASSERTION(test, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
- KUNIT_INIT_PTR_NOT_ERR_STRUCT(test, \
- assert_type, \
+ KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
#ptr, \
__ptr), \
fmt, \
--
Daniel Latypov
Jan 13, 2022, 11:59:57 AM1/13/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
We call this function first thing for all the assertion `format()`
functions.
This is the part that prints the file and line number and assertion type
(EXPECTATION, ASSERTION).
Having it as part of the format functions lets us have the flexibility
to not print that information (or print it differently) for new
assertion types, but I think this we don't need that.
And in the future, we'd like to consider factoring that data (file,
line#, type) out of the kunit_assert struct and into a `static`
variable, as Linus suggested [1], so we'd need to extract it anyways.
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
Signed-off-by: Daniel Latypov <dlat...@google.com>
functions.
This is the part that prints the file and line number and assertion type
(EXPECTATION, ASSERTION).
Having it as part of the format functions lets us have the flexibility
to not print that information (or print it differently) for new
assertion types, but I think this we don't need that.
And in the future, we'd like to consider factoring that data (file,
line#, type) out of the kunit_assert struct and into a `static`
variable, as Linus suggested [1], so we'd need to extract it anyways.
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
Reviewed-by: David Gow <davi...@google.com>
---
lib/kunit/assert.c | 6 ------
lib/kunit/test.c | 1 +
2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
---
lib/kunit/assert.c | 6 ------
lib/kunit/test.c | 1 +
2 files changed, 1 insertion(+), 6 deletions(-)
index b972bda61c0c..4d9a1295efc7 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -40,7 +40,6 @@ EXPORT_SYMBOL_GPL(kunit_assert_print_msg);
void kunit_fail_assert_format(const struct kunit_assert *assert,
index 3a52c321c280..345a9dd88c27 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -255,6 +255,7 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
return;
}
+ kunit_base_assert_format(assert, stream);
assert->format(assert, stream);
kunit_print_string_stream(test, stream);
--
kunit_print_string_stream(test, stream);
2.34.1.703.g22d0c6ccf7-goog
Daniel Latypov
Jan 13, 2022, 12:00:00 PM1/13/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
This is per Linus's suggestion in [1].
The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
kunit_assert object onto the stack. Normally we rely on compilers to
elide this, but when that doesn't work out, this blows up the stack
usage of kunit test functions.
We can move some data off the stack by making it static.
This change introduces a new `struct kunit_loc` to hold the file and
line number and then just passing assert_type (EXPECT or ASSERT) as an
argument.
In [1], it was suggested to also move out the format string as well, but
users could theoretically craft a format string at runtime, so we can't.
This change leaves a copy of `assert_type` in kunit_assert for now
because cleaning up all the macros to not pass it around is a bit more
involved.
Here's an example of the expanded code for KUNIT_FAIL():
if (__builtin_expect(!!(!(false)), 0)) {
static const struct kunit_loc loc = { .file = ... };
struct kunit_fail_assert __assertion = { .assert = { .type ... };
kunit_do_failed_assertion(test, &loc, KUNIT_EXPECTATION, &__assertion.assert, ...);
};
The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a
kunit_assert object onto the stack. Normally we rely on compilers to
elide this, but when that doesn't work out, this blows up the stack
usage of kunit test functions.
We can move some data off the stack by making it static.
This change introduces a new `struct kunit_loc` to hold the file and
line number and then just passing assert_type (EXPECT or ASSERT) as an
argument.
In [1], it was suggested to also move out the format string as well, but
users could theoretically craft a format string at runtime, so we can't.
This change leaves a copy of `assert_type` in kunit_assert for now
because cleaning up all the macros to not pass it around is a bit more
involved.
Here's an example of the expanded code for KUNIT_FAIL():
if (__builtin_expect(!!(!(false)), 0)) {
static const struct kunit_loc loc = { .file = ... };
struct kunit_fail_assert __assertion = { .assert = { .type ... };
kunit_do_failed_assertion(test, &loc, KUNIT_EXPECTATION, &__assertion.assert, ...);
};
[1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
Signed-off-by: Daniel Latypov <dlat...@google.com>
Signed-off-by: Daniel Latypov <dlat...@google.com>
Suggested-by: Linus Torvalds <torv...@linux-foundation.org>
Reviewed-by: David Gow <davi...@google.com>
include/kunit/assert.h | 25 +++++++++++++++++--------
Reviewed-by: David Gow <davi...@google.com>
include/kunit/test.h | 12 +++++++++++-
lib/kunit/assert.c | 9 +++++----
lib/kunit/test.c | 15 +++++++++------
4 files changed, 42 insertions(+), 19 deletions(-)
lib/kunit/assert.c | 9 +++++----
lib/kunit/test.c | 15 +++++++++------
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 3da6c792496c..3d7e46901478 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -28,11 +28,21 @@ enum kunit_assert_type {
KUNIT_EXPECTATION,
};
+/**
+ * struct kunit_loc - Identifies the source location of a line of code.
+ * @line: the line number in the file.
+ * @file: the file name.
+ */
+struct kunit_loc {
+ int line;
+ const char *file;
+};
+
+#define KUNIT_CURRENT_LOC { .file = __FILE__, .line = __LINE__ }
KUNIT_EXPECTATION,
};
+/**
+ * struct kunit_loc - Identifies the source location of a line of code.
+ * @line: the line number in the file.
+ * @file: the file name.
+ */
+struct kunit_loc {
+ int line;
+ const char *file;
+};
+
+
/**
* struct kunit_assert - Data for printing a failed assertion or expectation.
* struct kunit_assert - Data for printing a failed assertion or expectation.
* @type: the type (either an expectation or an assertion) of this kunit_assert.
- * @line: the source code line number that the expectation/assertion is at.
- * @file: the file path of the source file that the expectation/assertion is in.
* @message: an optional message to provide additional context.
* @format: a function which formats the data in this kunit_assert to a string.
*
@@ -40,9 +50,9 @@ enum kunit_assert_type {
- * @file: the file path of the source file that the expectation/assertion is in.
* @message: an optional message to provide additional context.
* @format: a function which formats the data in this kunit_assert to a string.
*
* format a string to a user reporting the failure.
*/
struct kunit_assert {
*/
struct kunit_assert {
+ // TODO(dlat...@google.com): delete this unused field when we've
+ // updated all the related KUNIT_INIT_ASSERT* macros.
enum kunit_assert_type type;
- int line;
- const char *file;
struct va_format message;
void (*format)(const struct kunit_assert *assert,
+ // updated all the related KUNIT_INIT_ASSERT* macros.
enum kunit_assert_type type;
- int line;
- const char *file;
struct va_format message;
struct string_stream *stream);
@@ -65,14 +75,13 @@ struct kunit_assert {
*/
#define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \
.type = assert_type, \
.type = assert_type, \
- .file = __FILE__, \
- .line = __LINE__, \
.message = KUNIT_INIT_VA_FMT_NULL, \
.format = fmt \
}
-void kunit_base_assert_format(const struct kunit_assert *assert,
- struct string_stream *stream);
+void kunit_assert_prologue(const struct kunit_loc *loc,
+ enum kunit_assert_type type,
+ struct string_stream *stream);
void kunit_assert_print_msg(const struct kunit_assert *assert,
- .line = __LINE__, \
.message = KUNIT_INIT_VA_FMT_NULL, \
.format = fmt \
}
-void kunit_base_assert_format(const struct kunit_assert *assert,
- struct string_stream *stream);
+void kunit_assert_prologue(const struct kunit_loc *loc,
+ enum kunit_assert_type type,
+ struct string_stream *stream);
struct string_stream *stream);
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 25ea3bce6663..7b752175e614 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -772,13 +772,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
#define KUNIT_SUCCEED(test) do {} while (0)
void kunit_do_failed_assertion(struct kunit *test,
+ const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct kunit_assert *assert,
const char *fmt, ...);
-#define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do { \
const char *fmt, ...);
+#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, 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, \
fmt, \
##__VA_ARGS__); \
@@ -788,6 +793,7 @@ void kunit_do_failed_assertion(struct kunit *test,
#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \
KUNIT_ASSERTION(test, \
+ assert_type, \
false, \
kunit_fail_assert, \
KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
@@ -818,6 +824,7 @@ void kunit_do_failed_assertion(struct kunit *test,
+ &loc, \
+ assert_type, \
&__assertion.assert, \
fmt, \
##__VA_ARGS__); \
@@ -788,6 +793,7 @@ void kunit_do_failed_assertion(struct kunit *test,
#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \
KUNIT_ASSERTION(test, \
+ assert_type, \
false, \
kunit_fail_assert, \
KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
fmt, \
...) \
KUNIT_ASSERTION(test, \
+ assert_type, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
@@ -876,6 +883,7 @@ do { \
typeof(right) __right = (right); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
@@ -876,6 +883,7 @@ do { \
typeof(right) __right = (right); \
\
KUNIT_ASSERTION(test, \
__left op __right, \
assert_class, \
ASSERT_CLASS_INIT(assert_type, \
@@ -1230,6 +1238,7 @@ do { \
const char *__right = (right); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
@@ -1230,6 +1238,7 @@ do { \
const char *__right = (right); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
kunit_binary_str_assert, \
KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
@@ -1289,6 +1298,7 @@ do { \
typeof(ptr) __ptr = (ptr); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
@@ -1289,6 +1298,7 @@ do { \
typeof(ptr) __ptr = (ptr); \
\
KUNIT_ASSERTION(test, \
+ assert_type, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
index 4d9a1295efc7..9f4492a8e24e 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -10,12 +10,13 @@
#include "string-stream.h"
#include "string-stream.h"
-void kunit_base_assert_format(const struct kunit_assert *assert,
+void kunit_assert_prologue(const struct kunit_loc *loc,
+ enum kunit_assert_type type,
+ enum kunit_assert_type type,
struct string_stream *stream)
{
const char *expect_or_assert = NULL;
- switch (assert->type) {
+ switch (type) {
case KUNIT_EXPECTATION:
expect_or_assert = "EXPECTATION";
break;
@@ -25,9 +26,9 @@ void kunit_base_assert_format(const struct kunit_assert *assert,
}
string_stream_add(stream, "%s FAILED at %s:%d\n",
- expect_or_assert, assert->file, assert->line);
+ expect_or_assert, loc->file, loc->line);
}
-EXPORT_SYMBOL_GPL(kunit_base_assert_format);
+EXPORT_SYMBOL_GPL(kunit_assert_prologue);
void kunit_assert_print_msg(const struct kunit_assert *assert,
{
const char *expect_or_assert = NULL;
- switch (assert->type) {
+ switch (type) {
case KUNIT_EXPECTATION:
expect_or_assert = "EXPECTATION";
break;
@@ -25,9 +26,9 @@ void kunit_base_assert_format(const struct kunit_assert *assert,
}
string_stream_add(stream, "%s FAILED at %s:%d\n",
- expect_or_assert, assert->file, assert->line);
+ expect_or_assert, loc->file, loc->line);
}
-EXPORT_SYMBOL_GPL(kunit_base_assert_format);
+EXPORT_SYMBOL_GPL(kunit_assert_prologue);
struct string_stream *stream)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 345a9dd88c27..7dec3248562f 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -240,7 +240,8 @@ static void kunit_print_string_stream(struct kunit *test,
}
}
-static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
+static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
+ enum kunit_assert_type type, struct kunit_assert *assert)
{
struct string_stream *stream;
@@ -250,12 +251,12 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
if (!stream) {
WARN(true,
"Could not allocate stream to print failed assertion in %s:%d\n",
- assert->file,
- assert->line);
+ loc->file,
+ loc->line);
return;
}
- kunit_base_assert_format(assert, stream);
+ kunit_assert_prologue(loc, type, stream);
}
}
-static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
+static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
+ enum kunit_assert_type type, struct kunit_assert *assert)
{
struct string_stream *stream;
@@ -250,12 +251,12 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
if (!stream) {
WARN(true,
"Could not allocate stream to print failed assertion in %s:%d\n",
- assert->file,
- assert->line);
+ loc->file,
+ loc->line);
return;
}
- kunit_base_assert_format(assert, stream);
assert->format(assert, stream);
kunit_print_string_stream(test, stream);
@@ -277,6 +278,8 @@ static void __noreturn kunit_abort(struct kunit *test)
kunit_print_string_stream(test, stream);
}
void kunit_do_failed_assertion(struct kunit *test,
+ const struct kunit_loc *loc,
+ enum kunit_assert_type type,
struct kunit_assert *assert,
const char *fmt, ...)
{
@@ -286,11 +289,11 @@ void kunit_do_failed_assertion(struct kunit *test,
assert->message.fmt = fmt;
assert->message.va = &args;
- kunit_fail(test, assert);
+ kunit_fail(test, loc, type, assert);
va_end(args);
- if (assert->type == KUNIT_ASSERTION)
+ if (type == KUNIT_ASSERTION)
kunit_abort(test);
}
EXPORT_SYMBOL_GPL(kunit_do_failed_assertion);
--
2.34.1.703.g22d0c6ccf7-goog
const char *fmt, ...)
{
@@ -286,11 +289,11 @@ void kunit_do_failed_assertion(struct kunit *test,
assert->message.fmt = fmt;
assert->message.va = &args;
- kunit_fail(test, assert);
+ kunit_fail(test, loc, type, assert);
va_end(args);
- if (assert->type == KUNIT_ASSERTION)
+ if (type == KUNIT_ASSERTION)
kunit_abort(test);
}
EXPORT_SYMBOL_GPL(kunit_do_failed_assertion);
--
Daniel Latypov
Jan 13, 2022, 12:00:02 PM1/13/22
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org, Daniel Latypov
This field has been split out from kunit_assert to make the struct less
heavy along with the filename and line number.
This change drops the assert_type field and cleans up all the macros
that were plumbing assert_type into kunit_assert.
Signed-off-by: Daniel Latypov <dlat...@google.com>
heavy along with the filename and line number.
This change drops the assert_type field and cleans up all the macros
that were plumbing assert_type into kunit_assert.
Signed-off-by: Daniel Latypov <dlat...@google.com>
Reviewed-by: David Gow <davi...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
---
include/kunit/assert.h | 48 ++++++++++++------------------------------
Reviewed-by: Brendan Higgins <brendan...@google.com>
---
include/kunit/test.h | 14 +++++-------
2 files changed, 19 insertions(+), 43 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 3d7e46901478..21299232c120 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -42,7 +42,6 @@ struct kunit_loc {
/**
* struct kunit_assert - Data for printing a failed assertion or expectation.
- * @type: the type (either an expectation or an assertion) of this kunit_assert.
* 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.
*
@@ -50,9 +49,6 @@ struct kunit_loc {
* @format: a function which formats the data in this kunit_assert to a string.
*
* format a string to a user reporting the failure.
*/
struct kunit_assert {
- // TODO(dlat...@google.com): delete this unused field when we've
*/
struct kunit_assert {
- // updated all the related KUNIT_INIT_ASSERT* macros.
- enum kunit_assert_type type;
struct va_format message;
void (*format)(const struct kunit_assert *assert,
struct string_stream *stream);
@@ -68,13 +64,11 @@ struct kunit_assert {
void (*format)(const struct kunit_assert *assert,
struct string_stream *stream);
/**
* 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.
*/
*
* The base initializer for a &struct kunit_assert.
*/
-#define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \
- .type = assert_type, \
+#define KUNIT_INIT_ASSERT_STRUCT(fmt) { \
- .type = assert_type, \
+#define KUNIT_INIT_ASSERT_STRUCT(fmt) { \
.message = KUNIT_INIT_VA_FMT_NULL, \
.format = fmt \
}
@@ -100,15 +94,13 @@ void kunit_fail_assert_format(const struct kunit_assert *assert,
.format = fmt \
}
struct string_stream *stream);
/**
- * KUNIT_INIT_FAIL_ASSERT_STRUCT() - Initializer for &struct kunit_fail_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
+ * 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.
*/
* Initializes a &struct kunit_fail_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_FAIL_ASSERT_STRUCT(type) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_fail_assert_format) \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_fail_assert_format) \
}
/**
@@ -132,16 +124,14 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_fail_assert_format) \
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_fail_assert_format) \
}
/**
/**
* 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.
*
* @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.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_UNARY_ASSERT_STRUCT(type, cond, expect_true) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_unary_assert_format), \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_unary_assert_format), \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_unary_assert_format), \
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_unary_assert_format), \
.condition = cond, \
.expected_true = expect_true \
}
@@ -168,16 +158,14 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
.expected_true = expect_true \
}
/**
* KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
* &struct kunit_ptr_not_err_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
* 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.
*
* @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.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(type, txt, val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_ptr_not_err_assert_format), \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_ptr_not_err_assert_format), \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_ptr_not_err_assert_format), \
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_ptr_not_err_assert_format), \
.text = txt, \
.value = val \
}
@@ -211,7 +199,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
.value = val \
}
/**
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
* KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_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.
* @left_str: A string representation of the expression in the left slot.
@@ -221,14 +208,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
* Initializes a &struct kunit_binary_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_ASSERT_STRUCT(type, \
- op_str, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(op_str, \
left_str, \
- op_str, \
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_assert_format), \
- kunit_binary_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_assert_format), \
.operation = op_str, \
.left_text = left_str, \
.left_value = left_val, \
.left_text = left_str, \
@@ -275,14 +260,12 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
* Initializes a &struct kunit_binary_ptr_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(type, \
- op_str, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(op_str, \
left_str, \
- op_str, \
+#define KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT(op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_ptr_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_ptr_assert_format), \
- kunit_binary_ptr_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_ptr_assert_format), \
.operation = op_str, \
.left_text = left_str, \
.left_value = left_val, \
.left_text = left_str, \
@@ -319,7 +302,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
/**
* KUNIT_INIT_BINARY_STR_ASSERT_STRUCT() - Initializes a
* &struct kunit_binary_str_assert.
- * @type: The type (assertion or expectation) of this kunit_assert.
* 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.
* @left_str: A string representation of the expression in the left slot.
@@ -329,14 +311,12 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
* Initializes a &struct kunit_binary_str_assert. Intended to be used in
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
* KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/
-#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(type, \
- op_str, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(op_str, \
left_str, \
- op_str, \
+#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(op_str, \
left_val, \
right_str, \
right_val) { \
- .assert = KUNIT_INIT_ASSERT_STRUCT(type, \
- kunit_binary_str_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_str_assert_format), \
- kunit_binary_str_assert_format), \
+ .assert = KUNIT_INIT_ASSERT_STRUCT(kunit_binary_str_assert_format), \
.operation = op_str, \
.left_text = left_str, \
.left_value = left_val, \
.left_text = left_str, \
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 7b752175e614..5964af750d93 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -796,7 +796,7 @@ void kunit_do_failed_assertion(struct kunit *test,
assert_type, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT, \
fmt, \
##__VA_ARGS__)
@@ -827,8 +827,7 @@ void kunit_do_failed_assertion(struct kunit *test,
assert_type, \
false, \
kunit_fail_assert, \
- KUNIT_INIT_FAIL_ASSERT_STRUCT(assert_type), \
+ KUNIT_INIT_FAIL_ASSERT_STRUCT, \
fmt, \
##__VA_ARGS__)
assert_type, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
!!(condition) == !!expected_true, \
kunit_unary_assert, \
- KUNIT_INIT_UNARY_ASSERT_STRUCT(assert_type, \
- #condition, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
expected_true), \
fmt, \
##__VA_ARGS__)
@@ -886,8 +885,7 @@ do { \
- #condition, \
+ KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \
expected_true), \
fmt, \
##__VA_ARGS__)
@@ -886,8 +885,7 @@ do { \
assert_type, \
__left op __right, \
assert_class, \
__left op __right, \
assert_class, \
- ASSERT_CLASS_INIT(assert_type, \
- #op, \
+ ASSERT_CLASS_INIT(#op, \
#left, \
__left, \
#right, \
@@ -1241,8 +1239,7 @@ do { \
- #op, \
+ ASSERT_CLASS_INIT(#op, \
#left, \
__left, \
#right, \
@@ -1241,8 +1239,7 @@ do { \
assert_type, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
strcmp(__left, __right) op 0, \
kunit_binary_str_assert, \
- KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(assert_type, \
- #op, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(#op, \
#left, \
__left, \
#right, \
@@ -1301,8 +1298,7 @@ do { \
assert_type, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
- KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
- #ptr, \
+ KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \
__ptr), \
fmt, \
##__VA_ARGS__); \
--
2.34.1.703.g22d0c6ccf7-goog
- #op, \
+ KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(#op, \
#left, \
__left, \
#right, \
@@ -1301,8 +1298,7 @@ do { \
assert_type, \
!IS_ERR_OR_NULL(__ptr), \
kunit_ptr_not_err_assert, \
- KUNIT_INIT_PTR_NOT_ERR_STRUCT(assert_type, \
- #ptr, \
+ KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \
__ptr), \
fmt, \
##__VA_ARGS__); \
--
Brendan Higgins
Jan 21, 2022, 6:01:30 PM1/21/22
to Daniel Latypov, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, torv...@linux-foundation.org
On Thu, Jan 13, 2022 at 11:59 AM Daniel Latypov <dlat...@google.com> wrote:
>
> We call this function first thing for all the assertion `format()`
> functions.
> This is the part that prints the file and line number and assertion type
> (EXPECTATION, ASSERTION).
>
> Having it as part of the format functions lets us have the flexibility
> to not print that information (or print it differently) for new
> assertion types, but I think this we don't need that.
>
> And in the future, we'd like to consider factoring that data (file,
> line#, type) out of the kunit_assert struct and into a `static`
> variable, as Linus suggested [1], so we'd need to extract it anyways.
>
> [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> Reviewed-by: David Gow <davi...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
>
> We call this function first thing for all the assertion `format()`
> functions.
> This is the part that prints the file and line number and assertion type
> (EXPECTATION, ASSERTION).
>
> Having it as part of the format functions lets us have the flexibility
> to not print that information (or print it differently) for new
> assertion types, but I think this we don't need that.
>
> And in the future, we'd like to consider factoring that data (file,
> line#, type) out of the kunit_assert struct and into a `static`
> variable, as Linus suggested [1], so we'd need to extract it anyways.
>
> [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> Reviewed-by: David Gow <davi...@google.com>
Reply all
Reply to author
Forward
0 new messages