Received-SPF: pass (google.com: domain of linux-kernel+bounces-70046-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Sat, 17 Feb 2024 20:02:23 +0000
From: Yueh-Shun Li <shamrocklee@posteo.net>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Mark Brown <broonie@kernel.org>, Andy Shevchenko
 <andriy.shevchenko@linux.intel.com>, Herve Codina
 <herve.codina@bootlin.com>, Christophe Leroy <christophe.leroy@csgroup.eu>,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] minmax: substitute local variables using
 =?UTF-8?Q?=5F=5FUNIQUE=5FID=28=29?=
In-Reply-To: <20240215140704.7989cc915a8f92a6358e7455@linux-foundation.org>
References: <20240215185820.2285834-1-shamrocklee@posteo.net>
 <20240215140704.7989cc915a8f92a6358e7455@linux-foundation.org>
Message-ID: <60c9477d80241b2aa363ed8ab4e3a84b@posteo.net>

On 2024-02-16 06:07, Andrew Morton wrote:
> On Thu, 15 Feb 2024 18:58:15 +0000 Yueh-Shun Li 
> <shamrocklee@posteo.net> wrote:
> 
>> Substitute identifier names of local variables used in macro
>> definitions inside minmax.h with those generated by 
>> __UNIQUE_ID(prefix)
>> to eliminate passible naming collisions.
>> 
>> Identifier names like __x, __y and __tmp are everywhere inside the
>> kernel source. This patch ensures that macros provided by minmax.h
>> will work even when identifiers of these names appear in the expanded
>> input arguments.
> 
> Makes sense I guess.  However I do wonder how far this goes:
> 
> # grep typeof include/linux/*.h | wc -l
> 313
> 
> Many of these are locals being defined within macros. Do they all need
> changing?

Regarding the extent of changes needed, you raise a valid point.
Searching in include/linux with regular expression
`typeof\([A-Za-z0-9]+\) __`, there are about 20 header files contain
macros with temporary variables prior to this patch, and 10 of them
(including minmax.h) uses generic variable names such as __a, __x, __v,
__val, __p or __ptr.

> If so, do we really want to implement this fix for what has
> always been, to my knowledge, a non-problem?

While the occurrence of collision is minimal in the current Linux kernel
source, the presence of variable names prefixed with long underscores
hints the potential issue. Although Linux kernel coding style recommends
suffixing temporary variable names per-macro to avoid collisions[1],
prefix more underscore (_) before the colliding local variable name
seems to be a workaround that receives wider adoption. Examples include
____ptr in three include/linux/*.h files and ______r, ______f in
compiler.h. commit 24ba53017e18 ("rcu: Replace ________p1 and
_________p1 with __UNIQUE_ID(rcu)") shows how __UNIQUE_ID() could be a
more systematic solution.

A probable cause of __UNIQUE_ID() not being widely adopted as
long-underscore variables is that the __COUNTER__ support was not
available until to GCC 4 and Clang 2. It was until recently that
__UNIQUE_ID() provided by compiler.h was unified to use __COUNTER__ by
commit a8306f2d4dce ("compiler.h: unify __UNIQUE_ID").

This patch aims toward minmax.h because it provides general
functionality, such as minimum, maximum and variable swapping, that are
used across subsystems. As for other headers, those with long-underscore
names and those with wide application could be changed first.

Thank you for your valuable feedback.

Best regards,

Shamrock

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