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

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 Mon, 2022-02-28 at 20:16 +0000, Matthew Wilcox wrote:
> 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.
> 
I was just going to say the same thing...

If we're willing to change the API for the macro, we could do

  list_for_each_entry(type, pos, head, member)

and then actually take advantage of -Wshadow?

johannes

On Mon, Feb 28, 2022 at 12:16 PM Matthew Wilcox <willy at infradead.org>
wrote:
>
> Then we can never use -Wshadow ;-(  I'd love to be able to turn it on;
> it catches real bugs.
Oh, we already can never use -Wshadow regardless of things like this.
That bridge hasn't just been burned, it never existed in the first
place.

The whole '-Wshadow' thing simply cannot work with local variables in
macros - something that we've used since day 1.

Try this (as a "p.c" file):

        #define min(a,b) ({                     \
                typeof(a) __a = (a);            \
                typeof(b) __b = (b);            \
                __a < __b ? __a : __b; })

        int min3(int a, int b, int c)
        {
                return min(a,min(b,c));
        }

and now do "gcc -O2 -S t.c".

Then try it with -Wshadow.

In other words, -Wshadow is simply not acceptable. Never has been,
never will be, and that has nothing to do with the

        typeof(pos) pos

kind of thing.

Your argument just isn't an argument.

              Linus

On Mon, Feb 28, 2022 at 12:37:15PM -0800, Linus Torvalds
wrote:
> On Mon, Feb 28, 2022 at 12:16 PM Matthew Wilcox <willy at
infradead.org> wrote:
> >
> > Then we can never use -Wshadow ;-(  I'd love to be able to turn it
on;
> > it catches real bugs.
> 
> Oh, we already can never use -Wshadow regardless of things like this.
> That bridge hasn't just been burned, it never existed in the first
> place.
> 
> The whole '-Wshadow' thing simply cannot work with local variables
in
> macros - something that we've used since day 1.
> 
> Try this (as a "p.c" file):
> 
>         #define min(a,b) ({                     \
>                 typeof(a) __a = (a);            \
>                 typeof(b) __b = (b);            \
>                 __a < __b ? __a : __b; })
> 
>         int min3(int a, int b, int c)
>         {
>                 return min(a,min(b,c));
>         }
> 
> and now do "gcc -O2 -S t.c".
> 
> Then try it with -Wshadow.
#define ___PASTE(a, b)	a##b
#define __PASTE(a, b) ___PASTE(a, b)
#define _min(a, b, u) ({         \
        typeof(a) __PASTE(__a,u) = (a);            \
        typeof(b) __PASTE(__b,u) = (b);            \
        __PASTE(__a,u) < __PASTE(__b,u) ? __PASTE(__a,u) : __PASTE(__b,u); })

#define min(a, b) _min(a, b, __COUNTER__)

int min3(int a, int b, int c)
{
        return min(a,min(b,c));
}

(probably there's a more elegant way to do this)

From: Matthew Wilcox
> Sent: 28 February 2022 20:16
> 
> 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))
Actually can't you use 'pos' to temporarily hold the address of
'member'.
Something like:
	for (pos = (void *)head; \
		pos ? ((pos = (void *)pos - offsetof(member)), 1) : 0; \
		pos = (void *)pos->next)
So that 'pos' is NULL if the loop terminates.
No pointers outside structures are generated.
Probably need to kill list_entry_is_head() - or it just checks for NULL.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)