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

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Eric Dumazet <edumazet@google.com>
Date: Tue, 2 Apr 2024 14:45:07 +0200

> On Tue, Apr 2, 2024 at 1:55 PM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>>
>> From: Jakub Kicinski <kuba@kernel.org>
>> 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()

Most of them were added before __UNIQUE_ID() became norm, weren't they?
Lots of them were switched to __UNIQUE_ID() because of issues, weren't they?

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

Only those which flood the console when building with W=12 C=1 to
recheck that my new code is fine.

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

Is it unique in some way?

#define _rb_entry_safe(ptr, type, member, ____ptr) \
	({ typeof(ptr) ____ptr = (ptr); \
	   ____ptr ? rb_entry(____ptr, type, member) : NULL; \
	})
#define rb_entry_safe(ptr, type, member)		\
	_rb_entry_safe(ptr, type, member,		\
		       __UNIQUE_ID(ptr_)

(@____ptr can be renamed if needed, this one would give the smallest
 code diff)

If you think +1 layer is a terrible obfuscating, look at
<linux/fortify-string.h> :D

Thanks,
Olek