Nouveau - Feb 2022 - [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds
<torvalds at linux-foundation.org> wrote:
>
> Side note: we do need *some* way to do it.
Ooh.

This patch is a work of art.

And I mean that in the worst possible way.

We can do

        typeof(pos) pos

in the 'for ()' loop, and never use __iter at all.

That means that inside the for-loop, we use a _different_ 'pos' than
outside.

And then the compiler will not see some "might be uninitialized", but
the outer 'pos' *will* be uninitialized.

Unless, of course, the outer 'pos' had that pointless explicit
initializer.

Here - can somebody poke holes in this "work of art" patch?

                     Linus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: text/x-patch
Size: 1927 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220228/92bce193/attachment-0001.bin>

On Mon, Feb 28, 2022 at 12:10 PM Linus Torvalds
<torvalds at linux-foundation.org> wrote:
>
> We can do
>
>         typeof(pos) pos
>
> in the 'for ()' loop, and never use __iter at all.
>
> That means that inside the for-loop, we use a _different_ 'pos'
than outside.
The thing that makes me throw up in my mouth a bit is that in that

        typeof(pos) pos

the first 'pos' (that we use for just the typeof) is that outer-level
'pos', IOW it's a *different* 'pos' than the second
'pos' in that same
declaration that declares the inner level shadowing new 'pos'
variable.

If I was a compiler person, I would say "Linus, that thing is too ugly
to live", and I would hate it. I'm just hoping that even compiler
people say "that's *so* ugly it's almost beautiful".

Because it does seem to work. It's not pretty, but hey, it's not like
our headers are really ever be winning any beauty contests...

                Linus

On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds
wrote:
> We can do
> 
>         typeof(pos) pos
> 
> in the 'for ()' loop, and never use __iter at all.
> 
> That means that inside the for-loop, we use a _different_ 'pos'
than outside.
Then we can never use -Wshadow ;-(  I'd love to be able to turn it on;
it catches real bugs.
> +#define list_for_each_entry(pos, head, member)					\
> +	for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member);	\
> +	     !list_entry_is_head(pos, head, member);	\
>  	     pos = list_next_entry(pos, member))

> On 28. Feb 2022, at 21:10, Linus Torvalds <torvalds at
linux-foundation.org> wrote:
> 
> On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds
> <torvalds at linux-foundation.org> wrote:
>> 
>> Side note: we do need *some* way to do it.
> 
> Ooh.
> 
> This patch is a work of art.
> 
> And I mean that in the worst possible way.
> 
> We can do
> 
>        typeof(pos) pos
> 
> in the 'for ()' loop, and never use __iter at all.
> 
> That means that inside the for-loop, we use a _different_ 'pos'
than outside.
> 
> And then the compiler will not see some "might be uninitialized",
but
> the outer 'pos' *will* be uninitialized.
> 
> Unless, of course, the outer 'pos' had that pointless explicit
initializer.
The goal of this is to get compiler warnings right? This would indeed be great.

Changing the list_for_each_entry() macro first will break all of those cases
(e.g. the ones using 'list_entry_is_head()).
I assumed it is better to fix those cases first and then have a simple
coccinelle script changing the macro + moving the iterator into the scope
of the macro.
> 
> Here - can somebody poke holes in this "work of art" patch?
With this you are no longer able to set the 'outer' pos within the list
iterator loop body or am I missing something? Like this it stays
uninitialized but you'll probably want to set it from within the loop.

You would then yet again need a variable with another name to use
after the loop.

I fail to see how this will make most of the changes in this
patch obsolete (if that was the intention).
> 
>                     Linus
> <patch.diff>
- Jakob

On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel <jakobkoschel at gmail.com>
wrote:
>
> The goal of this is to get compiler warnings right? This would indeed be
great.
Yes, so I don't mind having a one-time patch that has been gathered
using some automated checker tool, but I don't think that works from a
long-term maintenance perspective.

So if we have the basic rule being "don't use the loop iterator after
the loop has finished, because it can cause all kinds of subtle
issues", then in _addition_ to fixing the existing code paths that
have this issue, I really would want to (a) get a compiler warning for
future cases and (b) make it not actually _work_ for future cases.

Because otherwise it will just happen again.
> Changing the list_for_each_entry() macro first will break all of those
cases
> (e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already
have a patch for due to  your automated tool. There were certainly
more than a handful, but it didn't look _too_ bad to just make the
rule be "don't use the iterator after the loop".

Of course, that's just based on that patch of yours. Maybe there are a
ton of other cases that your patch didn't change, because they didn't
match your trigger case, so I may just be overly optimistic here.

But basically to _me_, the important part is that the end result is
maintainable longer-term. I'm more than happy to have a one-time patch
to fix a lot of dubious cases if we can then have clean rules going
forward.
> I assumed it is better to fix those cases first and then have a simple
> coccinelle script changing the macro + moving the iterator into the scope
> of the macro.
So that had been another plan of mine, until I actually looked at
changing the macro. In the one case I looked at, it was ugly beyond
belief.

It turns out that just syntactically, it's really nice to give the
type of the iterator from outside the way we do now. Yeah, it may be a
bit odd, and maybe it's partly because I'm so used to the
"list_for_each_list_entry()" syntax, but moving the type into the loop
construct really made it nasty - either one very complex line, or
having to split it over two lines which was even worse.

Maybe the place I looked at just happened to have a long typename, but
it's basically always going to be a struct, so it's never a _simple_
type. And it just looked very odd adn unnatural to have the type as
one of the "arguments" to that list_for_each_entry() macro.

So yes, initially my idea had been to just move the iterator entirely
inside the macro. But specifying the type got so ugly that I think
that

        typeof (pos) pos

trick inside the macro really ends up giving us the best of all worlds:

 (a) let's us keep the existing syntax and code for all the nice cases
that did everything inside the loop anyway

 (b) gives us a nice warning for any normal use-after-loop case
(unless you explicitly initialized it like that
sgx_mmu_notifier_release() function did for no good reason

 (c) also guarantees that even if you don't get a warning,
non-converted (or newly written) bad code won't actually _work_

so you end up getting the new rules without any ambiguity or mistaken
> With this you are no longer able to set the 'outer' pos within the
list
> iterator loop body or am I missing something?
Correct. Any assignment inside the loop will be entirely just to the
local loop case. So any "break;" out of the loop will have to set
another variable - like your updated patch did.
> I fail to see how this will make most of the changes in this
> patch obsolete (if that was the intention).
I hope my explanation above clarifies my thinking: I do not dislike
your patch, and in fact your patch is indeed required to make the new
semantics work.

What I disliked was always the maintainability of your patch - making
the rules be something that isn't actually visible in the source code,
and letting the old semantics still work as well as they ever did, and
having to basically run some verification pass to find bad users.

(I also disliked your original patch that mixed up the "CPU
speculation type safety" with the actual non-speculative problems, but
that was another issue).

                Linus