[PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[Vincent Mailhol]

The Linux kernel coding style [1] advises to avoid common variable
names in function-like macros to reduce the risk of collisions.

Throughout local_lock_internal.h, several macros use the rather common
variable names 'l' and 'tl'. This already resulted in an actual
collision: the __local_lock_acquire() function like macro is currently
shadowing the parameter 'l' of the:

  class_##_name##_t class_##_name##_constructor(_type *l)

function factory from linux/cleanup.h.

Rename the variable 'l' to '__l' and the variable 'tl' to '__tl'
throughout the file to fix the current name collision and to prevent
future ones.

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

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Changes in v2:

  - __lock conflicted with an existing definition in lockdep.c. Use
    instead __l (and also, to keep things consistent, use __tl instead
    of tl for the trylock).

  - Apply the renaming to the entire file and not just to
    __local_lock_acquire().

  - Rewrite the patch description accordingly.

Link to v1: https://lore.kernel.org/r/20250923-local_lock_internal_fix_shadow-v1-1-14e313c88a46@kernel.org
---
 include/linux/local_lock_internal.h | 56 ++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
index d80b5306a2c0ccf95a3405b6b947b5f1f9a3bd38..a80b3fd7552376cd926aeaac8f53bcc44c8d2173 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 *__tl;					\
+		local_lock_t *__l;					\
 									\
-		l = (local_lock_t *)(lock);				\
-		tl = (local_trylock_t *)l;				\
+		__l = (local_lock_t *)(lock);				\
+		__tl = (local_trylock_t *)__l;				\
 		_Generic((lock),					\
 			local_trylock_t *: ({				\
-				lockdep_assert(tl->acquired == 0);	\
-				WRITE_ONCE(tl->acquired, 1);		\
+				lockdep_assert(__tl->acquired == 0);	\
+				WRITE_ONCE(__tl->acquired, 1);		\
 			}),						\
 			local_lock_t *: (void)0);			\
-		local_lock_acquire(l);					\
+		local_lock_acquire(__l);				\
 	} while (0)
 
 #define __local_lock(lock)					\
@@ -130,50 +130,50 @@ do {								\
 
 #define __local_trylock(lock)					\
 	({							\
-		local_trylock_t *tl;				\
+		local_trylock_t *__tl;				\
 								\
 		preempt_disable();				\
-		tl = (lock);					\
-		if (READ_ONCE(tl->acquired)) {			\
+		__tl = (lock);					\
+		if (READ_ONCE(__tl->acquired)) {		\
 			preempt_enable();			\
-			tl = NULL;				\
+			__tl = NULL;				\
 		} else {					\
-			WRITE_ONCE(tl->acquired, 1);		\
+			WRITE_ONCE(__tl->acquired, 1);		\
 			local_trylock_acquire(			\
-				(local_lock_t *)tl);		\
+				(local_lock_t *)__tl);		\
 		}						\
-		!!tl;						\
+		!!__tl;						\
 	})
 
 #define __local_trylock_irqsave(lock, flags)			\
 	({							\
-		local_trylock_t *tl;				\
+		local_trylock_t *__tl;				\
 								\
 		local_irq_save(flags);				\
-		tl = (lock);					\
-		if (READ_ONCE(tl->acquired)) {			\
+		__tl = (lock);					\
+		if (READ_ONCE(__tl->acquired)) {		\
 			local_irq_restore(flags);		\
-			tl = NULL;				\
+			__tl = NULL;				\
 		} else {					\
-			WRITE_ONCE(tl->acquired, 1);		\
+			WRITE_ONCE(__tl->acquired, 1);		\
 			local_trylock_acquire(			\
-				(local_lock_t *)tl);		\
+				(local_lock_t *)__tl);		\
 		}						\
-		!!tl;						\
+		!!__tl;						\
 	})
 
 #define __local_lock_release(lock)					\
 	do {								\
-		local_trylock_t *tl;					\
-		local_lock_t *l;					\
+		local_trylock_t *__tl;					\
+		local_lock_t *__l;					\
 									\
-		l = (local_lock_t *)(lock);				\
-		tl = (local_trylock_t *)l;				\
-		local_lock_release(l);					\
+		__l = (local_lock_t *)(lock);				\
+		__tl = (local_trylock_t *)__l;				\
+		local_lock_release(__l);				\
 		_Generic((lock),					\
 			local_trylock_t *: ({				\
-				lockdep_assert(tl->acquired == 1);	\
-				WRITE_ONCE(tl->acquired, 0);		\
+				lockdep_assert(__tl->acquired == 1);	\
+				WRITE_ONCE(__tl->acquired, 0);		\
 			}),						\
 			local_lock_t *: (void)0);			\
 	} while (0)

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

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

Re: [PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[Vlastimil Babka]

+CC LOCKING PRIMITIVES maintainers. Looks like local_lock files were never
added to the section, should we?

On 9/24/25 20:03, Vincent Mailhol wrote:
> The Linux kernel coding style [1] advises to avoid common variable
> names in function-like macros to reduce the risk of collisions.

I think it would be better if the tools like sparse could recognize if the
shadowing happens inside a macro only and thus really unlikely to cause a
misuse due to confusion (code thinks it's manipulating an outer instance but
instead it's the inner one), because macros in their definition would never
intend to manipulate a possible outer instance, right? Or are there any
other problems due to shadowing besides this risk?

> Throughout local_lock_internal.h, several macros use the rather common
> variable names 'l' and 'tl'. This already resulted in an actual
> collision: the __local_lock_acquire() function like macro is currently
> shadowing the parameter 'l' of the:
> 
>   class_##_name##_t class_##_name##_constructor(_type *l)
> 
> function factory from linux/cleanup.h.
> 
> Rename the variable 'l' to '__l' and the variable 'tl' to '__tl'
> throughout the file to fix the current name collision and to prevent
> future ones.
> 
> [1] https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
> 
> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>

That said I don't oppose the change, but not my call.

> ---
> Changes in v2:
> 
>   - __lock conflicted with an existing definition in lockdep.c. Use
>     instead __l (and also, to keep things consistent, use __tl instead
>     of tl for the trylock).
> 
>   - Apply the renaming to the entire file and not just to
>     __local_lock_acquire().
> 
>   - Rewrite the patch description accordingly.
> 
> Link to v1: https://lore.kernel.org/r/20250923-local_lock_internal_fix_shadow-v1-1-14e313c88a46@kernel.org
> ---
>  include/linux/local_lock_internal.h | 56 ++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
> index d80b5306a2c0ccf95a3405b6b947b5f1f9a3bd38..a80b3fd7552376cd926aeaac8f53bcc44c8d2173 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 *__tl;					\
> +		local_lock_t *__l;					\
>  									\
> -		l = (local_lock_t *)(lock);				\
> -		tl = (local_trylock_t *)l;				\
> +		__l = (local_lock_t *)(lock);				\
> +		__tl = (local_trylock_t *)__l;				\
>  		_Generic((lock),					\
>  			local_trylock_t *: ({				\
> -				lockdep_assert(tl->acquired == 0);	\
> -				WRITE_ONCE(tl->acquired, 1);		\
> +				lockdep_assert(__tl->acquired == 0);	\
> +				WRITE_ONCE(__tl->acquired, 1);		\
>  			}),						\
>  			local_lock_t *: (void)0);			\
> -		local_lock_acquire(l);					\
> +		local_lock_acquire(__l);				\
>  	} while (0)
>  
>  #define __local_lock(lock)					\
> @@ -130,50 +130,50 @@ do {								\
>  
>  #define __local_trylock(lock)					\
>  	({							\
> -		local_trylock_t *tl;				\
> +		local_trylock_t *__tl;				\
>  								\
>  		preempt_disable();				\
> -		tl = (lock);					\
> -		if (READ_ONCE(tl->acquired)) {			\
> +		__tl = (lock);					\
> +		if (READ_ONCE(__tl->acquired)) {		\
>  			preempt_enable();			\
> -			tl = NULL;				\
> +			__tl = NULL;				\
>  		} else {					\
> -			WRITE_ONCE(tl->acquired, 1);		\
> +			WRITE_ONCE(__tl->acquired, 1);		\
>  			local_trylock_acquire(			\
> -				(local_lock_t *)tl);		\
> +				(local_lock_t *)__tl);		\
>  		}						\
> -		!!tl;						\
> +		!!__tl;						\
>  	})
>  
>  #define __local_trylock_irqsave(lock, flags)			\
>  	({							\
> -		local_trylock_t *tl;				\
> +		local_trylock_t *__tl;				\
>  								\
>  		local_irq_save(flags);				\
> -		tl = (lock);					\
> -		if (READ_ONCE(tl->acquired)) {			\
> +		__tl = (lock);					\
> +		if (READ_ONCE(__tl->acquired)) {		\
>  			local_irq_restore(flags);		\
> -			tl = NULL;				\
> +			__tl = NULL;				\
>  		} else {					\
> -			WRITE_ONCE(tl->acquired, 1);		\
> +			WRITE_ONCE(__tl->acquired, 1);		\
>  			local_trylock_acquire(			\
> -				(local_lock_t *)tl);		\
> +				(local_lock_t *)__tl);		\
>  		}						\
> -		!!tl;						\
> +		!!__tl;						\
>  	})
>  
>  #define __local_lock_release(lock)					\
>  	do {								\
> -		local_trylock_t *tl;					\
> -		local_lock_t *l;					\
> +		local_trylock_t *__tl;					\
> +		local_lock_t *__l;					\
>  									\
> -		l = (local_lock_t *)(lock);				\
> -		tl = (local_trylock_t *)l;				\
> -		local_lock_release(l);					\
> +		__l = (local_lock_t *)(lock);				\
> +		__tl = (local_trylock_t *)__l;				\
> +		local_lock_release(__l);				\
>  		_Generic((lock),					\
>  			local_trylock_t *: ({				\
> -				lockdep_assert(tl->acquired == 1);	\
> -				WRITE_ONCE(tl->acquired, 0);		\
> +				lockdep_assert(__tl->acquired == 1);	\
> +				WRITE_ONCE(__tl->acquired, 0);		\
>  			}),						\
>  			local_lock_t *: (void)0);			\
>  	} while (0)
> 
> ---
> base-commit: cec1e6e5d1ab33403b809f79cd20d6aff124ccfe
> change-id: 20250923-local_lock_internal_fix_shadow-2e2a24e95e76
> 
> Best regards,

Re: [PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[Sebastian Andrzej Siewior]

On 2025-09-26 16:16:56 [+0200], Vlastimil Babka wrote:
> +CC LOCKING PRIMITIVES maintainers. Looks like local_lock files were never
> added to the section, should we?
Yes, indeed. Would you be so kind or do you prefer me doing it?

Sebastian

Re: [PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[Vlastimil Babka]

On 9/26/25 16:20, Sebastian Andrzej Siewior wrote:
> On 2025-09-26 16:16:56 [+0200], Vlastimil Babka wrote:
>> +CC LOCKING PRIMITIVES maintainers. Looks like local_lock files were never
>> added to the section, should we?
> Yes, indeed. Would you be so kind or do you prefer me doing it?

I'd prefer you as the author of it, but can do if you're too busy.
> Sebastian

Re: [PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[Sebastian Andrzej Siewior]

On 2025-09-26 16:29:33 [+0200], Vlastimil Babka wrote:
> On 9/26/25 16:20, Sebastian Andrzej Siewior wrote:
> > On 2025-09-26 16:16:56 [+0200], Vlastimil Babka wrote:
> >> +CC LOCKING PRIMITIVES maintainers. Looks like local_lock files were never
> >> added to the section, should we?
> > Yes, indeed. Would you be so kind or do you prefer me doing it?
> 
> I'd prefer you as the author of it, but can do if you're too busy.
Sure thing, will do.

Sebastian

Re: [PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[Vincent Mailhol]

On 9/26/25 11:16 PM, Vlastimil Babka wrote:
> +CC LOCKING PRIMITIVES maintainers. Looks like local_lock files were never
> added to the section, should we?
> 
> On 9/24/25 20:03, Vincent Mailhol wrote:
>> The Linux kernel coding style [1] advises to avoid common variable
>> names in function-like macros to reduce the risk of collisions.
> 
> I think it would be better if the tools like sparse could recognize if the
> shadowing happens inside a macro only and thus really unlikely to cause a
> misuse due to confusion (code thinks it's manipulating an outer instance but
> instead it's the inner one), because macros in their definition would never
> intend to manipulate a possible outer instance, right? Or are there any
> other problems due to shadowing besides this risk?

Thank would mean:

  - rewriting the shadowing check in sparse
  - removing the -Wshadow from the W=2 list
  - modifying the kernel coding style

I am not against this. But I am not unhappy with the current status quo either.

So far, I kept sending patches whenever I saw such shadow warning in header
files. And over the last five years, this resulted in only three occurrences:

  - commit 146034fed6ee ("x86/asm/bitops: Use __builtin_ffs() to evaluate
    constant expressions")
    Link: https://git.kernel.org/torvalds/c/146034fed6ee


  - commit 9ce02f0fc683 ("x86/bug: Prevent shadowing in __WARN_FLAGS")
    Link: https://git.kernel.org/torvalds/c/9ce02f0fc683

  - this patch

Between sending one patch every couple year or enrolling to a quest to modify
the tooling, my choice is already made. If someone else want to do this change,
I would be supportive, but that person will not be me.

On a side note, I want to highlight that it is not that I am reluctant to modify
the tooling. For example, I sent contributed this commit to sparse last week:

commit 366ad4b2fa3e ("Warn about "unsigned value that used to be signed against
zero"")

Link:
https://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git/commit/?id=366ad4b2fa3e

As anyone here, I choose my battles, and rewriting the shadow checks is not in
my list.

>> Throughout local_lock_internal.h, several macros use the rather common
>> variable names 'l' and 'tl'. This already resulted in an actual
>> collision: the __local_lock_acquire() function like macro is currently
>> shadowing the parameter 'l' of the:
>>
>>   class_##_name##_t class_##_name##_constructor(_type *l)
>>
>> function factory from linux/cleanup.h.
>>
>> Rename the variable 'l' to '__l' and the variable 'tl' to '__tl'
>> throughout the file to fix the current name collision and to prevent
>> future ones.
>>
>> [1] https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
>>
>> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
> 
> That said I don't oppose the change, but not my call.

Thanks!


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[Vlastimil Babka]

On 9/27/25 05:04, Vincent Mailhol wrote:
> On 9/26/25 11:16 PM, Vlastimil Babka wrote:
>> +CC LOCKING PRIMITIVES maintainers. Looks like local_lock files were never
>> added to the section, should we?
>> 
>> On 9/24/25 20:03, Vincent Mailhol wrote:
>>> The Linux kernel coding style [1] advises to avoid common variable
>>> names in function-like macros to reduce the risk of collisions.
>> 
>> I think it would be better if the tools like sparse could recognize if the
>> shadowing happens inside a macro only and thus really unlikely to cause a
>> misuse due to confusion (code thinks it's manipulating an outer instance but
>> instead it's the inner one), because macros in their definition would never
>> intend to manipulate a possible outer instance, right? Or are there any
>> other problems due to shadowing besides this risk?
> 
> Thank would mean:
> 
>   - rewriting the shadowing check in sparse
>   - removing the -Wshadow from the W=2 list
>   - modifying the kernel coding style
> 
> I am not against this. But I am not unhappy with the current status quo either.
> 
> So far, I kept sending patches whenever I saw such shadow warning in header
> files. And over the last five years, this resulted in only three occurrences:
> 
>   - commit 146034fed6ee ("x86/asm/bitops: Use __builtin_ffs() to evaluate
>     constant expressions")
>     Link: https://git.kernel.org/torvalds/c/146034fed6ee
> 
> 
>   - commit 9ce02f0fc683 ("x86/bug: Prevent shadowing in __WARN_FLAGS")
>     Link: https://git.kernel.org/torvalds/c/9ce02f0fc683
> 
>   - this patch
> 
> Between sending one patch every couple year or enrolling to a quest to modify
> the tooling, my choice is already made. If someone else want to do this change,
> I would be supportive, but that person will not be me.

Thanks for that perspective, with that it seems now clear to me that a rare
fixup of some macro is indeed much easier.

> On a side note, I want to highlight that it is not that I am reluctant to modify
> the tooling. For example, I sent contributed this commit to sparse last week:
> 
> commit 366ad4b2fa3e ("Warn about "unsigned value that used to be signed against
> zero"")
> 
> Link:
> https://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git/commit/?id=366ad4b2fa3e

Re: [PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[Vincent Mailhol]

On 9/26/25 11:16 PM, Vlastimil Babka wrote:
> +CC LOCKING PRIMITIVES maintainers. Looks like local_lock files were never
> added to the section, should we?

Shall I resend with the correct list of persons in CC? Or is it OK as-is?


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[Sebastian Andrzej Siewior]

On 2025-09-29 17:50:47 [+0900], Vincent Mailhol wrote:
> On 9/26/25 11:16 PM, Vlastimil Babka wrote:
> > +CC LOCKING PRIMITIVES maintainers. Looks like local_lock files were never
> > added to the section, should we?
> 
> Shall I resend with the correct list of persons in CC? Or is it OK as-is?

I think you you got them. I didn't have the time to look at this yet.

> Yours sincerely,
> Vincent Mailhol

Sebastian