| Message ID | 20240614233105.265009-2-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8dea5c3f51ff6c09c9705cbba3a4b275f05a0658 |
| Headers | show |
| Series | selftests/vDSO: fix clang build errors, and Makefile cleanup | expand |
Also Cc'ing Thomas Gleixner on this one, sorry for the omission. On 6/14/24 4:31 PM, 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. > > Reviewed-by: Carlos Llamas <cmllamas@google.com> > Reviewed-by: Edward Liaw <edliaw@google.com> > Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > 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; thanks,
On Fri, Jun 14, 2024 at 4:31 PM John Hubbard <jhubbard@nvidia.com> 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. > > Reviewed-by: Carlos Llamas <cmllamas@google.com> > Reviewed-by: Edward Liaw <edliaw@google.com> > Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > 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; > -- > 2.45.2 > Hi John, Could you try re-submitting this series with the RESEND prefix? Thanks, Edward
On 7/2/24 11:12 AM, Edward Liaw wrote: > On Fri, Jun 14, 2024 at 4:31 PM John Hubbard <jhubbard@nvidia.com> wrote: ... > Hi John, > Could you try re-submitting this series with the RESEND prefix? > > Thanks, > Edward Sure. Is that the key, for kselftests? Because I've got 5+ small patchsets that are languishing there, and I'm at a loss for how to even get a response, much less get something merged. thanks,
On Tue, Jul 2, 2024 at 11:26 AM John Hubbard <jhubbard@nvidia.com> wrote: > > On 7/2/24 11:12 AM, Edward Liaw wrote: > > On Fri, Jun 14, 2024 at 4:31 PM John Hubbard <jhubbard@nvidia.com> wrote: > ... > > Hi John, > > Could you try re-submitting this series with the RESEND prefix? > > > > Thanks, > > Edward > > Sure. Is that the key, for kselftests? Because I've got 5+ small > patchsets that are languishing there, and I'm at a loss for how to even > get a response, much less get something merged. I don't really know, but it was suggested to me that it can help to point out that the patch might have been missed before. > > > thanks, > -- > John Hubbard > NVIDIA >
On 7/2/24 11:49 AM, Edward Liaw wrote: > On Tue, Jul 2, 2024 at 11:26 AM John Hubbard <jhubbard@nvidia.com> wrote: >> >> On 7/2/24 11:12 AM, Edward Liaw wrote: >>> On Fri, Jun 14, 2024 at 4:31 PM John Hubbard <jhubbard@nvidia.com> wrote: >> ... >>> Hi John, >>> Could you try re-submitting this series with the RESEND prefix? >>> >>> Thanks, >>> Edward >> >> Sure. Is that the key, for kselftests? Because I've got 5+ small >> patchsets that are languishing there, and I'm at a loss for how to even >> get a response, much less get something merged. > > I don't really know, but it was suggested to me that it can help to > point out that the patch might have been missed before. > I wish. But we are three versions into it, plus a special email [1] specifically to point out that this is still pending. I think either people are on vacation, or these fixes are not considered sufficiently important, or both. :) [1] https://lore.kernel.org/e9de90dd-c541-45bf-a364-0e7aaf320314@nvidia.com thanks,
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;
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. Reviewed-by: Carlos Llamas <cmllamas@google.com> Reviewed-by: Edward Liaw <edliaw@google.com> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com> Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- tools/testing/selftests/vDSO/parse_vdso.c | 16 +++++++++++----- .../selftests/vDSO/vdso_standalone_test_x86.c | 18 ++++++++++++++++-- 2 files changed, 27 insertions(+), 7 deletions(-)