| Message ID | 20220108012304.1049587-1-dlatypov@google.com |
|---|---|
| Headers | show |
| Series | kunit: refactor assertions to use less stack | expand |
On Fri, Jan 7, 2022 at 8:23 PM Daniel Latypov <dlatypov@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 <dlatypov@google.com> I still don't like how much this bloats the example test; aside from that, this looks good. Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
On Fri, Jan 7, 2022 at 8:23 PM Daniel Latypov <dlatypov@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. > > This patch removes the field and cleans up the macros to avoid > needlessly passing around `test`. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> Looks good. Thanks! Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
On Mon, Jan 10, 2022 at 2:14 PM Brendan Higgins <brendanhiggins@google.com> wrote: > > On Fri, Jan 7, 2022 at 8:23 PM Daniel Latypov <dlatypov@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 <dlatypov@google.com> > > I still don't like how much this bloats the example test; aside from > that, this looks good. Agreed, it does add bloat. 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 <brendanhiggins@google.com>
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