Received-SPF: pass (google.com: domain of linux-kernel+bounces-195825-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <b29ca6e1-82a0-4682-ba29-cb31f53a5502@nvidia.com>
Date: Thu, 30 May 2024 12:24:52 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] selftests/vDSO: fix clang build errors and warnings
To: Muhammad Usama Anjum <usama.anjum@collabora.com>,
 Shuah Khan <shuah@kernel.org>, Andy Lutomirski <luto@kernel.org>
Cc: 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>
 <0816b38b-41a1-431a-90a4-1ac33401a671@collabora.com>
Content-Language: en-US
From: John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <0816b38b-41a1-431a-90a4-1ac33401a671@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?eWJ5WjlreFZXWGJwRnJIODVpUGU4QmRjWGhQeVNyRkN1YTdrM1dpbG84OHBC?=
 =?utf-8?B?VTVHOWVsMXNFY3gvRHNCc1g5dGI5K2hyMmdjMm1VUlZkNVZNZkhWeFB6UTFw?=
 =?utf-8?B?WWhXM3FSTmduU2UxZzlDbURRb21VNENqV0hnT3RkVnB5ZmUweUdQS3Z4SGd2?=
 =?utf-8?B?dnNMMDdQdDYvcThwQmNPQlRYTE5nbDlDZngwNnZNaHF5ajFoT1NudXQxYkYv?=
 =?utf-8?B?K3luNWpkZ0hzaTk4MnpWb2xyNjZ3bkg3QUlWdVJRWWF2M2dsVWI5ekdzaE14?=
 =?utf-8?B?d3hQeDlvNUVwTzg0RDNHQ1M0VkVJWVpzSFJEc3NuV0FXenllMVlBSTR1OGRj?=
 =?utf-8?B?VDhyTTFUc3ZFZFFWa1BNdWd1NWxPZk0wWVFydWhWcDhGbXVRWERCRldRN2NC?=
 =?utf-8?B?dXBDRzNMTXc3ZDdaL2RONjc0enp6Y09IMFA0K01uZUNCd3AyVG4wUWtGVWhC?=
 =?utf-8?B?L21XNTMxSHlXSUxReGlVOHZRd1YrTWpET0toL3RnSjRqT1JETlcxR2lkZHBw?=
 =?utf-8?B?TFlJV0RVMG5jRUk2WEE0bTJURkIxc1VrV2hKTGtLSWtkZ3lWem4wQ09wemZG?=
 =?utf-8?B?YzJ2MHV4Z09JL2ZjV0tSTGhoWmJ4Zjg3RUNHTVZ1RnFzeEtqS1VmdVFhaGpX?=
 =?utf-8?B?aiszQjFSUlN3ZjZ5NEpQTDNyWmhrOEs2a09XTzAyVWt1RndONnZZbm90RjQy?=
 =?utf-8?B?UnhtTEYyRkFvU08rTlRuMDFSazVEaVJkdkRWdFlaMWtzL3M3NjkyUzdRcEpH?=
 =?utf-8?B?SXBvUm96V1pwaXVIMlJlM0h2U3VUOTUxb1RBZlhkck8rZ0VrZXN5T0VjbXB3?=
 =?utf-8?B?VlVoemwyS3JwTXl5V01rVnRoSVhseTdrMktYbThLMlQ2UXBqMTZyV3lXOGFU?=
 =?utf-8?B?YllSZlIvUVlLTDZGbGNlUFVhbFhsVk83bzFlUTc4b0tzZCtOanNubmhsQXpB?=
 =?utf-8?B?WmpoWlg0ZmtjRE5hd21NNFNvWEdrWVlsanRqUm5ETnRMYzh5aDBZTDRzQ1hB?=
 =?utf-8?B?ci92YkNZano0cTRHMkJSWjRQSVRtMU0wL0tBZUJZYTh5UTBCdG9Ld01WTWNM?=
 =?utf-8?B?VDNlTjQ5N2szNzlvc0RJeTUxaFRnVVJPdytSVGJCWlpEQWo4aXNuaWg1WFZD?=
 =?utf-8?B?TDY0dUdLWGhRbk92TmpJUkNnMHVhQkVUTDZwV3hYOERGRWZUSFFXRDVOMUZj?=
 =?utf-8?B?NmM1ZVQvMlpmMTVEOCtCWjh0ZnQyc3hWMExuSFQwdWR6ejRsc2F4NGIzeHJS?=
 =?utf-8?B?RjBMZzRzV1NBRlh1TFNQb0pGMFhCNUozZVlkRVJ0dTl4SnIrRDFVelRlbnpS?=
 =?utf-8?B?QzhXcGJ3aEhEYVBqQWRVMzdqRGFaMXBzQTA1dlpxK21zRlZ6RGpnTGxsdzZr?=
 =?utf-8?B?ZG1xaENzeXhhVVc2MjBqaTgraEs1R3UwSDFhMDBIcnR5S3JnTVRMaUlBUkJD?=
 =?utf-8?B?UWFIcS90aGphdmVQYVBac0JSWUZrSlgzaHFkTTV2WHdlT0RyeWpzWk1XWGpL?=
 =?utf-8?B?OFhCZHdScEJWZDVEb0M2RDRuMnQrR0NzTGp0WndpbGF5MHhTbnhWWU5XYWVl?=
 =?utf-8?B?KzlBaWhWUlg0U000aFh5R2tkLzh6cURFOHVEMUNkUXpaOWJVMndtTThnVjll?=
 =?utf-8?B?eWtEQmJ4aDFNcUNYdXdjeXVOeXVBNE5hcEdYRFRWSWlxTTk2WjFoejByQ3or?=
 =?utf-8?B?QU5zb0VCVEt4MTZNWVFOclkyNWlJK0hrUU9FTzJYb0RVU2RFb0dWZUdpS3hO?=
 =?utf-8?B?dVlaaVdmMklmSU9Nb2RQc1BKMDdSdGtmY01wRjA3YWZ6MStoeFpXN2N5VkUx?=
 =?utf-8?B?Vm9Xckwwa1l0a3ZsTDlVVnNoQjBxalV0cHFCZHplaklqU0RnbVJYT0ZCUVhs?=
 =?utf-8?B?UlBwd3k1V3NrRnNScXc0OGxpWnZ0c3JIZ0xIdGxpeThmMXpzZDhkUFBsVUc3?=
 =?utf-8?B?ZzFFd0pEazRsT3l4WkNOcGhhVkJWUVo2MVFBVW9iT0RVZjlOb1JGbTVFNWl2?=
 =?utf-8?B?Ulc1UmhwdTdNWVNlTnRnRTl6WnlwWGFpTkVCaFhuWEhPZTdaK0hROUNnZmcz?=
 =?utf-8?B?bTEzYmFJa3F1T0Q3cDZHQ3VwSUp4VWkxUnFCWVVxVmY4bFZhRXNISWlheW5o?=
 =?utf-8?B?N1BkRHd1ZG8zMGpTdlMybkx2MTZVS2Y5c0NrWHNjY2hXS2tEcEl2K0MvYnVO?=
 =?utf-8?B?aGc9PQ==?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b253c4ef-8db9-48a5-03ad-08dc80de3cd7
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 May 2024 19:25:17.2225
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: dXC9/FhG7R/Oj3pt9WWKU2lkau7QsgwPjiDk19jT0mwxynVut35Hg2N0TvctAfYL/qtky8pQXiw/+Qy3KZa53g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB8426

On 5/29/24 1:05 AM, Muhammad Usama Anjum wrote:
> On 5/28/24 2:16 AM, 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.
>>
>> 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.
>>
>> 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.
>>
>> 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.
> Just checked with GCC 5.1, it builds fine without any errors.
> 
>>
>> [1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c49f@valentinobst.de/
>>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> LGTM
> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> 

Thanks for the review and testing, Muhammad!

I'm also adding Andy Lutomirski to this thread, in case Shuah would like
the vDSO maintainer to ack or review. (scripts/get_maintainer.pl is letting
me down, I will need to run "git blame" more I guess.

thanks,
-- 
John Hubbard
NVIDIA

>> ---
>>
>> Changes since the first version:
>>
>> 1) Rebased onto Linux 6.10-rc1
>>
>> thanks,
>> John Hubbard
>>
>>   tools/testing/selftests/vDSO/parse_vdso.c      | 16 +++++++++++-----
>>   .../selftests/vDSO/vdso_standalone_test_x86.c  | 18 ++++++++++++++++--
>>   2 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/testing/selftests/vDSO/parse_vdso.c b/tools/testing/selftests/vDSO/parse_vdso.c
>> index 413f75620a35..4ae417372e9e 100644
>> --- a/tools/testing/selftests/vDSO/parse_vdso.c
>> +++ b/tools/testing/selftests/vDSO/parse_vdso.c
>> @@ -55,14 +55,20 @@ static struct vdso_info
>>   	ELF(Verdef) *verdef;
>>   } vdso_info;
>>   
>> -/* Straight from the ELF specification. */
>> -static unsigned long elf_hash(const unsigned char *name)
>> +/*
>> + * Straight from the ELF specification...and then tweaked slightly, in order to
>> + * avoid a few clang warnings.
>> + */
>> +static unsigned long elf_hash(const char *name)
>>   {
>>   	unsigned long h = 0, g;
>> -	while (*name)
>> +	const unsigned char *uch_name = (const unsigned char *)name;
>> +
>> +	while (*uch_name)
>>   	{
>> -		h = (h << 4) + *name++;
>> -		if (g = h & 0xf0000000)
>> +		h = (h << 4) + *uch_name++;
>> +		g = h & 0xf0000000;
>> +		if (g)
>>   			h ^= g >> 24;
>>   		h &= ~g;
>>   	}
>> diff --git a/tools/testing/selftests/vDSO/vdso_standalone_test_x86.c b/tools/testing/selftests/vDSO/vdso_standalone_test_x86.c
>> index 8a44ff973ee1..27f6fdf11969 100644
>> --- a/tools/testing/selftests/vDSO/vdso_standalone_test_x86.c
>> +++ b/tools/testing/selftests/vDSO/vdso_standalone_test_x86.c
>> @@ -18,7 +18,7 @@
>>   
>>   #include "parse_vdso.h"
>>   
>> -/* We need a libc functions... */
>> +/* We need some libc functions... */
>>   int strcmp(const char *a, const char *b)
>>   {
>>   	/* This implementation is buggy: it never returns -1. */
>> @@ -34,6 +34,20 @@ int strcmp(const char *a, const char *b)
>>   	return 0;
>>   }
>>   
>> +/*
>> + * The clang build needs this, although gcc does not.
>> + * Stolen from lib/string.c.
>> + */
>> +void *memcpy(void *dest, const void *src, size_t count)
>> +{
>> +	char *tmp = dest;
>> +	const char *s = src;
>> +
>> +	while (count--)
>> +		*tmp++ = *s++;
>> +	return dest;
>> +}
>> +
>>   /* ...and two syscalls.  This is x86-specific. */
>>   static inline long x86_syscall3(long nr, long a0, long a1, long a2)
>>   {
>> @@ -70,7 +84,7 @@ void to_base10(char *lastdig, time_t n)
>>   	}
>>   }
>>   
>> -__attribute__((externally_visible)) void c_main(void **stack)
>> +void c_main(void **stack)
>>   {
>>   	/* Parse the stack */
>>   	long argc = (long)*stack;
>>
>> base-commit: 2bfcfd584ff5ccc8bb7acde19b42570414bf880b
>