Fixing up at least some fallout from '-Werror'

[Linus Torvalds]

So I just committed these three fixes:

4b93c544e90e ("thunderbolt: test: split up test cases in
tb_test_credit_alloc_all")
ba7b1f861086 ("lib/test_scanf: split up number parsing test routines")
1476ff21abb4 ("iwl: fix debug printf format strings")

for the fallout from -Werror that I could easily check (mainly i386
'allyesconfig' - a situation I don't normally test).

The printk format string one was trivial and I hopefully didn't screw
anything up, but I'd ask people to look at and verify the two other
ones. I tried to be very careful, and organizing the code movement in
such a way that 'git diff' shows that it's doing the same thing before
and after, but hey, mistakes happen.

I found those two test-based ones somewhat annoying, because they both
showed how little the test infrastructure tries to follow kernel
rules. I bet those warnings have been showing up for a long long time,
and people went "that's not a relevant configuration" or had some
other reason to ignore them.

No, the test cases may not be relevant in most situations, but it's
not a good thing when something that is supposed to verify kernel
behavior then violates some very fundamental and core kernel rules.

And maybe it was simply missed. The one thing that was clear when I
did that thunderbolt thing in particular is how easy it is to create
variations of those 'struct some-assertion-struct' things on stack as
part of the KUNIT infrastructure. That's unfortunate. It is possible
that the solution to the kernel stack usage might have been to make
those structures static instead, but I didn't check whether the
description structs really can be.

It would be even nicer if they were 'static const'. Being fully
initialized instead of generating not only code that uses up stack,
but also the code to dynamically initialize them on the stack is all
kinds of nasty. I took one look at the generated code, and ran away
screaming.

Anyway, I'm adding the Kunit maintainer and lists here too, just to
see if maybe it could be possible to make those 'struct kunit_assert'
things and friends be static and const, but at least for the cases
that caused problems for i386, those three commits should make the
build pass.

The test_scanf case didn't actually use the Kunit infrastructure, the
stack use explosion is because gcc doesn't seem to combine stack
allocations in many situations. I know gcc *sometimes* does that stack
allocation combining, but not here. I suspect it might be related to
type aliasing, and only merging stack slots when they have the same
types, and thus triggered by the different result buffer sizes. Maybe.

Linus

[Coelho, Luciano]

On Mon, 2021-09-06 at 13:00 -0700, Linus Torvalds wrote:
> So I just committed these three fixes:
>
>    4b93c544e90e ("thunderbolt: test: split up test cases in
> tb_test_credit_alloc_all")
>    ba7b1f861086 ("lib/test_scanf: split up number parsing test routines")
>    1476ff21abb4 ("iwl: fix debug printf format strings")

Thanks, Linus! We already had the same fix in iwlwifi on the way, it
was already in the wireless-drivers tree.

Luckily the chang was identical t yours, so there shouldn't be any
conflicts in the iwlwifi part.

--
Cheers,
Luca.

[Mika Westerberg]

Thanks for doing this! I certainly have received few mails from the
kbuildbot about this but haven't figured how to fix them properly.
Splitting the test to several small functions sounds like a good way to
do this. I'll keep this in mind in the future when adding more test
cases.

[Richard Fitzgerald]

> The test_scanf case didn't actually use the Kunit infrastructure, the
> stack use explosion is because gcc doesn't seem to combine stack
> allocations in many situations. I know gcc *sometimes* does that stack
> allocation combining, but not here. I suspect it might be related to
> type aliasing, and only merging stack slots when they have the same
> types, and thus triggered by the different result buffer sizes. Maybe.
>

I'll have to take another look at this test.

The build robot reported a stack explosion recently but despite trying
various configurations, GCC versions and X86/ARM targets I couldn't
reproduce. Robot got a ~8k stack, but in all my test builds GCC merged
the stack structs and produced only ~100-200 bytes. Unfortunately
haven't been able to spend the time on this.

I wanted to avoid the quick fix of multiple functions because really
that's saying "make stack use (which is up to GCC) < X". The ultimate
stack reduction would be one-test-per-function but that gives really
bloated source. Any attempt to group several tests into a function is
relying on an assumption about what GCC will merge or not merge on the
stack, and that could change.

I think the best fix would be to re-work the code to use a work buffer
instead of stack allocations (as it already does for the format
strings). With the benefit of hindsight, this is what I should have done
originally. When I have the time I'll work on it.