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

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

[Vincent Mailhol]

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.

For consistency, also rename 'tl' to '__trylock'.

[1] https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
I did not include a Fixes tag because I do not think that this is
worth backporting. But if you do mind, there it is:

Fixes: 51339d99c013 ("locking/local_lock, mm: replace localtry_ helpers with local_trylock_t type")
---
 include/linux/local_lock_internal.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
index d80b5306a2c0ccf95a3405b6b947b5f1f9a3bd38..69feb161c5e6c94e36ba30b2c625eac0065d1e3d 100644
--- a/include/linux/local_lock_internal.h
+++ b/include/linux/local_lock_internal.h
@@ -96,18 +96,18 @@ do {								\
 
 #define __local_lock_acquire(lock)					\
 	do {								\
-		local_trylock_t *tl;					\
-		local_lock_t *l;					\
+		local_trylock_t *__trylock;				\
+		local_lock_t *__lock;					\
 									\
-		l = (local_lock_t *)(lock);				\
-		tl = (local_trylock_t *)l;				\
+		__lock = (local_lock_t *)(lock);			\
+		__trylock = (local_trylock_t *)__lock;			\
 		_Generic((lock),					\
 			local_trylock_t *: ({				\
-				lockdep_assert(tl->acquired == 0);	\
-				WRITE_ONCE(tl->acquired, 1);		\
+				lockdep_assert(__trylock->acquired == 0);\
+				WRITE_ONCE(__trylock->acquired, 1);	\
 			}),						\
 			local_lock_t *: (void)0);			\
-		local_lock_acquire(l);					\
+		local_lock_acquire(__lock);				\
 	} while (0)
 
 #define __local_lock(lock)					\

---
base-commit: cec1e6e5d1ab33403b809f79cd20d6aff124ccfe
change-id: 20250923-local_lock_internal_fix_shadow-2e2a24e95e76

Best regards,
-- 
Vincent Mailhol <mailhol@kernel.org>

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

[Alexei Starovoitov]

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.
So the patch won't really fix the paranoid warning.
I think it's better to fix sparse to silence this warn.

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

[Vincent Mailhol]

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?

> 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

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

[Vincent Mailhol]

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

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

[Alexei Starovoitov]

On Wed, Sep 24, 2025 at 4:07 PM Vincent Mailhol <mailhol@kernel.org> wrote:
>
> 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).

I think s/l/__l/ and s/tl/__tl/ is fine,
but do it for all macros in that file,
since renaming one is fishy. It doesn't fix what
you're claiming to fix, hence the pushback.

Better yet, learn to ignore overly verbose tools.
sparse/checkpatch/gcc are not always correct.

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

[Vincent Mailhol]

On 25/09/2025 at 02:07, Alexei Starovoitov wrote:
> On Wed, Sep 24, 2025 at 4:07 PM Vincent Mailhol <mailhol@kernel.org> wrote:
>>
>> 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).
> 
> I think s/l/__l/ and s/tl/__tl/ is fine,
> but do it for all macros in that file,
> since renaming one is fishy. It doesn't fix what
> you're claiming to fix, hence the pushback.

OK. This was a fair pushback :)

I took your suggestion and renamed all instances in the file. I just sent the v2.

Thanks!


Yours sincerely,
Vincent Mailhol