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

[Vincent Mailhol <mailhol@kernel.org>]

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