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 <[email protected]>
LGTM
Reviewed-by: Muhammad Usama Anjum <[email protected]>
Tested-by: Muhammad Usama Anjum <[email protected]>
> ---
>
> 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
--
BR,
Muhammad Usama Anjum
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 <[email protected]>
> LGTM
> Reviewed-by: Muhammad Usama Anjum <[email protected]>
> Tested-by: Muhammad Usama Anjum <[email protected]>
>
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
>