Re: [PATCH v2] selftests/vDSO: fix clang build errors and warnings

[John Hubbard <jhubbard@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxx/
> >
> > Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
> LGTM
> Reviewed-by: Muhammad Usama Anjum <usama.anjum@xxxxxxxxxxxxx>
> Tested-by: Muhammad Usama Anjum <usama.anjum@xxxxxxxxxxxxx>

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