Re: [PATCH] locking/local_lock: fix shadowing in __local_lock_acquire()

[Vincent Mailhol <mailhol@kernel.org>]

On 24/09/2025 at 19:26, Vincent Mailhol wrote:
> On 24/09/2025 at 04:38, Alexei Starovoitov wrote:
>> On Tue, Sep 23, 2025 at 7:02 AM Vincent Mailhol <mailhol@kernel.org> wrote:
>>>
>>> The __local_lock_acquire() macro uses a local variable named 'l'. This
>>> being a common name, there is a risk of shadowing other variables.
>>>
>>> For example, it is currently shadowing the parameter 'l' of the:
>>>
>>>   class_##_name##_t class_##_name##_constructor(_type *l)
>>>
>>> function factory from linux/cleanup.h.
>>>
>>> Both sparse (with default options) and GCC (with W=2 option) warn
>>> about this shadowing.
>>>
>>> This is a bening warning, but because the issue appears in a header,
>>> it is spamming whoever is using it. So better to fix to remove some
>>> noise.
>>>
>>> Rename the variable from 'l' to '__lock' (with two underscore prefixes
>>> as suggested in the Linux kernel coding style [1]) in order to prevent
>>> the name collision.
>>
>> lockdep has __lock as a local variable.
> 
> OK. I didn't saw this one.
> 
> But there is a major difference between a shadowing in lockdep.c versus a
> shadowing in an header: the shadowing warning is local to lockdep.c and does not
> pollute the other users.
> 
> My worry is only about the spam created by warnings in headers.
> 
> Regardless, would renaming to __locked or __l instead of __lock help to address
> your concern?

__locked was a bad suggestion. It could be named __local_lock (there is already
a __local_lock() function like macro, but function like macro does not conflict
with variable names).

But now, my current preference goes to __ll (and also, to keep things
consistent, __tl for the trylock).

>> So the patch won't really fix the paranoid warning.
>> I think it's better to fix sparse to silence this warn.
> 
> Well, it is not only about the tooling but also about the Linux kernel coding
> style. I quote:
> 
>   5) namespace collisions when defining local variables in macros resembling
>   functions:
> 
> 	#define FOO(x)                          \
> 	({                                      \
> 	        typeof(x) ret;                  \
> 	        ret = calc_ret(x);              \
> 	        (ret);                          \
> 	})
> 
>   ret is a common name for a local variable - __foo_ret is less likely to
>   collide with an existing variable.
> 
> I really do not want to open the debate on whether shadow warnings are useful or
> not. The coding style says that we should care about this, let's just follow the
> rule.
> 
> Your patch is causing noise in the files I am using. So I hope you can find a
> solution for the annoyance your are causing me. I do not mind which solution it
> is. I am proposing one, if you do not like it, I am also fine if you prefer to
> go on a quest to rewrite the coding style and modify the tooling so that we do
> not warn anymore about shadowing. But I think we can agree that it is not *my*
> job to rewrite the kernel coding style and the tooling to *your* liking.

Yours sincerely,
Vincent Mailhol