Re: [PATCH net-next 2/2] netdev_queues: fix -Wshadow / Sparse shadow warnings throughout the file

[Eric Dumazet]

On Tue, Apr 2, 2024 at 1:55 PM Alexander Lobakin
<aleksander.lobakin@xxxxxxxxx> wrote:
>
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Date: Fri, 29 Mar 2024 13:53:44 -0700
>
> > On Fri, 29 Mar 2024 13:18:57 -0700 Jakub Kicinski wrote:
> >>> Sparse:
> >>>
> >>> drivers/net/ethernet/intel/idpf/idpf_txrx.c:1992:16: warning: symbol '_res' shadows an earlier one
> >>> drivers/net/ethernet/intel/idpf/idpf_txrx.c:1992:16: originally declared here
> >>
> >> I don't see these building with LLVM=1 W=12 C=1
> >> and I really don't like complicating the code because the compiler
> >> is stupid. Can't you solve this with some renames? Add another
>
> It's not the compiler, its warnings are valid actually. Shadowing makes
> it very easy to confuse variables and make bugs...
>
> >> underscore or something?
> >
> > I'm stupid I tried on the test branch which already had your fix..
>
> :D Sometimes it happens.
>
> >
> > This is enough:
> >
> > diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> > index 1ec408585373..2270fbb99cf7 100644
> > --- a/include/net/netdev_queues.h
> > +++ b/include/net/netdev_queues.h
> > @@ -89,7 +89,7 @@ struct netdev_stat_ops {
> >
> >  #define netif_txq_try_stop(txq, get_desc, start_thrs)                        \
> >       ({                                                              \
> > -             int _res;                                               \
> > +             int __res;                                              \
> >                                                                       \
> >               netif_tx_stop_queue(txq);                               \
> >               /* Producer index and stop bit must be visible          \
> > @@ -101,12 +101,12 @@ struct netdev_stat_ops {
> >               /* We need to check again in a case another             \
> >                * CPU has just made room available.                    \
> >                */                                                     \
> > -             _res = 0;                                               \
> > +             __res = 0;                                              \
> >               if (unlikely(get_desc >= start_thrs)) {                 \
> >                       netif_tx_start_queue(txq);                      \
> > -                     _res = -1;                                      \
> > +                     __res = -1;                                     \
> >               }                                                       \
> > -             _res;                                                   \
> > +             __res;                                                  \
> >       })                                                              \
> >
> >  /**
>
> But what if there's a function which calls one of these functions and
> already has _res or __res or something? I know renaming is enough for
> the warnings I mentioned, but without __UNIQUE_ID() anything can happen
> anytime, so I wanted to fix that once and for all :z
>
> I already saw some macros which have a layer of indirection for
> __UNIQUE_ID(), but previously they didn't and then there were fixes
> which added underscores, renamed variables etc etc...
>

We have hundreds of macros in include/ directory which use local names without
__UNIQUE_ID()

What is the plan ? Hundreds of patches obfuscating them more than they are ?

Can you show us how rb_entry_safe() (random choice) would be rewritten ?