Re: kunit stack usage, was: pmwg-ci report v5.5-rc4-147-gc62d43442481

[Arnd Bergmann]

On Wed, Jan 8, 2020 at 3:41 PM Brendan Higgins
<brendanhiggins@xxxxxxxxxx> wrote:
> On Tue, Jan 7, 2020 at 4:37 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > On Mon, Dec 30, 2019 at 6:16 PM PMWG CI <pmwg-ci@xxxxxxxxxx> wrote:
> > pe_test_uint_arrays() contains a couple of larger variables, plus 41
> > instances of KUNIT_EXPECT_*() or KUNIT_ASSERT_*(), each one
> > of these adds its own copy of the structure, eventually exceeding
> > the warning limit.
>
> Uh oh, sorry about that.

No worries, I don't think anyone could have foreseen that bug.

> Another idea, the struct just exists to pack up data and ship it off
> to a function which handles the expectation/assertion. Consequently,
> the struct is only used inside the expectation/assertion macro; it is
> not needed before the macro block and is not needed after it. So could
> we maybe make some kind of expectation/assertion union so that we know
> they are all the same size, and then somehow tag the stack allocation
> so that we only ever have one copy in a stack frame? I am not sure if
> that kind of compiler magic exists. I guess one way to accomplish this
> is to make a dummy function in KUnit whose job it is to call the unit
> test function, which allocates the object, and then calls the unit
> test function with a reference to the object allocation; then we could
> just reuse that allocation and we can avoid making a bunch of
> piecemeal heap allocations.
>
> What do people think? Any other ideas?

The idea of annotating it got me thinking about what could be
done to improve the structleak plugin, and that in turn got me on
the right track to a silly but trivial fix for the issue: The only thing
that structleak does here is to initialize the implied padding in
the kunit_binary_assert structure. If there is no padding, it all
works out find and the structures don't get pinned to the stack
because the plugin can simply ignore them.

I tried out this patch and it works:

diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index db6a0fca09b4..5b09439fa8ae 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -200,8 +200,9 @@ struct kunit_binary_assert {
        struct kunit_assert assert;
        const char *operation;
        const char *left_text;
-       long long left_value;
        const char *right_text;
+       long __pad;
+       long long left_value;
        long long right_value;
 };

There may also be a problem in 'struct kunit_assert' depending on the
architecture, if there are any on which the 'enum kunit_assert_type'
type is 64 bit wide (which I think is allowed in C, but may not happen
on any toolchain that builds kernels).

      Arnd