Received-SPF: pass (google.com: domain of linux-kernel+bounces-215544-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
Message-ID: <2461f002-fdd4-4bf5-b797-827069cafc4b@nvidia.com>
Date: Fri, 14 Jun 2024 15:51:56 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] selftests/vDSO: fix clang build errors and warnings
To: Carlos Llamas <cmllamas@google.com>
CC: Shuah Khan <shuah@kernel.org>, Mark Brown <broonie@kernel.org>, "Vincenzo
 Frascino" <vincenzo.frascino@arm.com>, Colin Ian King
	<colin.i.king@gmail.com>, Valentin Obst <kernel@valentinobst.de>,
	<linux-kselftest@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	<llvm@lists.linux.dev>
References: <20240527211622.290635-1-jhubbard@nvidia.com>
 <ZmiXqOHYaLLX557z@google.com>
Content-Language: en-US
From: John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <ZmiXqOHYaLLX557z@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 22:52:13.6408
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 357ddfb9-54af-435f-9d05-08dc8cc4a1fe
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource:
	BN1PEPF00004683.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB6967

On 6/11/24 11:30 AM, Carlos Llamas wrote:
> On Mon, May 27, 2024 at 02:16:22PM -0700, John Hubbard wrote:
>> When building with clang, via:
>>
>>      make LLVM=1 -C tools/testing/selftests
>>
>> ...there are several warnings, and an error. This fixes all of those and
>> allows these tests to run and pass.
> 
> It might be best to split the 4 _different_ fixes into separate patches.

If necessary, that can be done. It's sufficiently difficult to attract
attention for selftests (see below) that I'm reluctant to make it even
harder to get it all fixed, though.

> 
>>
>> 1. Fix linker error (undefined reference to memcpy) by providing a local
>>     version of memcpy.
>>
>> 2. clang complains about using this form:
>>
>>      if (g = h & 0xf0000000)
>>
>> ...so factor out the assignment into a separate step.
> 
> There has been multiple attempts to fix this. I can see these two:
> https://lore.kernel.org/all/20211206102931.1433871-1-anders.roxell@linaro.org/
> https://lore.kernel.org/all/20240501002150.1370861-1-edliaw@google.com/
> 
> ... I guess we somehow missed those?

I guess so. :)

> 
>>
>> 3. The code is passing a signed const char* to elf_hash(), which expects
>>     a const unsigned char *. There are several callers, so fix this at
>>     the source by allowing the function to accept a signed argument, and
>>     then converting to unsigned operations, once inside the function.
>>
> 
> There is also a v4 fix for this item that was sent out here:
> https://lore.kernel.org/all/20240506181951.1804451-1-edliaw@google.com/

No idea why these fixes are not getting picked up.

> 
>> 4. clang doesn't have __attribute__((externally_visible)) and generates
>>     a warning to that effect. Fortunately, gcc 12 and gcc 13 do not seem
>>     to require that attribute in order to build, run and pass tests here,
>>     so remove it.
>>
>> [1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c49f@valentinobst.de/
> 
> What is this about? Left over from v1 maybe?

Maybe.

> 
>>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> ---
> 
> I would prefer to pick up the fixes from folks who sent out the patches
> first but I'm fine either way.

I'm perfectly fine with dropping this and letting the other patches go
in, but *something* needs to go in.

> 
> Reviewed-by: Carlos Llamas <cmllamas@google.com>

Thanks for the review! I have no earthly idea what will happen next.
I'd like to hear at least something from the maintainers about their
intentions here.

thanks,
-- 
John Hubbard
NVIDIA