* [PATCH] Revert "x86_64: Suppress false positive valgrind error"
@ 2024-04-13 15:21 Florian Weimer
2024-04-13 15:27 ` H.J. Lu
0 siblings, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2024-04-13 15:21 UTC (permalink / raw)
To: libc-alpha; +Cc: H.J. Lu, Sunil K Pandey
This reverts commit a1735e0aa858f0c8b15e5ee9975bff4279423680.
The test failure is a real valgrind bug that needs to be fixed before
valgrind is usable with a glibc that has been built with
CC="gcc -march=x86-64-v3". The proposed valgrind patch teaches
valgrind to replace ld.so strcmp with an unoptimized scalar
implementation, thus avoiding any AVX2-related problems.
Valgrind bug: <https://bugs.kde.org/show_bug.cgi?id=485487>
---
elf/Makefile | 3 +--
elf/tst-valgrind-smoke.sh | 5 ++---
sysdeps/x86_64/Makefile | 7 -------
sysdeps/x86_64/tst-valgrind-smoke.supp | 17 -----------------
4 files changed, 3 insertions(+), 29 deletions(-)
diff --git a/elf/Makefile b/elf/Makefile
index 6dad11bcfb..fb3c3c4dff 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -618,8 +618,7 @@ tests-special += $(objpfx)tst-valgrind-smoke.out
endif
$(objpfx)tst-valgrind-smoke.out: tst-valgrind-smoke.sh $(objpfx)ld.so $(objpfx)valgrind-test
$(SHELL) $< $(objpfx)ld.so $(rtlddir)/$(rtld-installed-name) '$(test-wrapper-env)' \
- '$(run-program-env)' '$(rpath-link)' $(objpfx)valgrind-test \
- '$(valgrind-suppressions-tst-valgrind-smoke)' > $@; $(evaluate-test)
+ '$(run-program-env)' '$(rpath-link)' $(objpfx)valgrind-test > $@; $(evaluate-test)
ifeq ($(run-built-tests),yes)
tests-special += $(objpfx)tst-rtld-does-not-exist.out
diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh
index 7d0ceac6b1..e33b2fa1d7 100644
--- a/elf/tst-valgrind-smoke.sh
+++ b/elf/tst-valgrind-smoke.sh
@@ -25,7 +25,6 @@ test_wrapper_env="$3"
run_program_env="$4"
library_path="$5"
test_prog="$6"
-valgrind_suppressions="$7"
# Test whether valgrind is available in the test
# environment. If not, skip the test.
@@ -35,7 +34,7 @@ ${test_wrapper_env} ${run_program_env} \
# Test valgrind works with the system ld.so in the test environment
/bin/sh -c \
"${test_wrapper_env} ${run_program_env} \
- valgrind -q --error-exitcode=1 ${valgrind_suppressions} \
+ valgrind -q --error-exitcode=1 \
${system_rtld} /bin/echo ${system_rtld}" || exit 77
# Finally the actual test inside the test environment,
@@ -43,5 +42,5 @@ ${test_wrapper_env} ${run_program_env} \
# the smoke test under valgrind.
/bin/sh -c \
"${test_wrapper_env} ${run_program_env} \
- valgrind -q --error-exitcode=1 ${valgrind_suppressions} \
+ valgrind -q --error-exitcode=1 \
${rtld} --library-path ${library_path} ${test_prog} ${rtld}"
diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
index 39048b12e0..ce949dba27 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -211,14 +211,7 @@ tst-plt-rewrite2-ENV = GLIBC_TUNABLES=glibc.cpu.plt_rewrite=2
$(objpfx)tst-plt-rewrite2: $(objpfx)tst-plt-rewritemod2.so
endif
-# Check if ISA level is 3 or above.
-ifneq (,$(filter $(have-x86-isa-level),$(x86-isa-level-3-or-above)))
-valgrind-suppressions-tst-valgrind-smoke = \
- --suppressions=$(..)sysdeps/x86_64/tst-valgrind-smoke.supp
-endif
-
test-internal-extras += tst-gnu2-tls2mod1
-
endif # $(subdir) == elf
ifeq ($(subdir),csu)
diff --git a/sysdeps/x86_64/tst-valgrind-smoke.supp b/sysdeps/x86_64/tst-valgrind-smoke.supp
deleted file mode 100644
index 533c2deaff..0000000000
--- a/sysdeps/x86_64/tst-valgrind-smoke.supp
+++ /dev/null
@@ -1,17 +0,0 @@
-{
- False positive at strcmp-avx2.S:462
- Memcheck:Cond
- fun:strcmp
- fun:_dl_name_match_p
- fun:_dl_map_object
- fun:map_doit
- fun:_dl_catch_exception
- fun:_dl_catch_error
- fun:do_preload
- fun:handle_preload_list
- fun:dl_main
- fun:_dl_sysdep_start
- fun:_dl_start_final
- fun:_dl_start
- obj:*/elf/ld.so
-}
base-commit: ae7468a7b0bcf22e9cd5fcae42bb9e4f65de83ee
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"
2024-04-13 15:21 [PATCH] Revert "x86_64: Suppress false positive valgrind error" Florian Weimer
@ 2024-04-13 15:27 ` H.J. Lu
0 siblings, 0 replies; 12+ messages in thread
From: H.J. Lu @ 2024-04-13 15:27 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha, Sunil K Pandey
On Sat, Apr 13, 2024 at 8:21 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> This reverts commit a1735e0aa858f0c8b15e5ee9975bff4279423680.
>
> The test failure is a real valgrind bug that needs to be fixed before
> valgrind is usable with a glibc that has been built with
> CC="gcc -march=x86-64-v3". The proposed valgrind patch teaches
> valgrind to replace ld.so strcmp with an unoptimized scalar
> implementation, thus avoiding any AVX2-related problems.
>
> Valgrind bug: <https://bugs.kde.org/show_bug.cgi?id=485487>
>
> ---
> elf/Makefile | 3 +--
> elf/tst-valgrind-smoke.sh | 5 ++---
> sysdeps/x86_64/Makefile | 7 -------
> sysdeps/x86_64/tst-valgrind-smoke.supp | 17 -----------------
> 4 files changed, 3 insertions(+), 29 deletions(-)
>
> diff --git a/elf/Makefile b/elf/Makefile
> index 6dad11bcfb..fb3c3c4dff 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -618,8 +618,7 @@ tests-special += $(objpfx)tst-valgrind-smoke.out
> endif
> $(objpfx)tst-valgrind-smoke.out: tst-valgrind-smoke.sh $(objpfx)ld.so $(objpfx)valgrind-test
> $(SHELL) $< $(objpfx)ld.so $(rtlddir)/$(rtld-installed-name) '$(test-wrapper-env)' \
> - '$(run-program-env)' '$(rpath-link)' $(objpfx)valgrind-test \
> - '$(valgrind-suppressions-tst-valgrind-smoke)' > $@; $(evaluate-test)
> + '$(run-program-env)' '$(rpath-link)' $(objpfx)valgrind-test > $@; $(evaluate-test)
>
> ifeq ($(run-built-tests),yes)
> tests-special += $(objpfx)tst-rtld-does-not-exist.out
> diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh
> index 7d0ceac6b1..e33b2fa1d7 100644
> --- a/elf/tst-valgrind-smoke.sh
> +++ b/elf/tst-valgrind-smoke.sh
> @@ -25,7 +25,6 @@ test_wrapper_env="$3"
> run_program_env="$4"
> library_path="$5"
> test_prog="$6"
> -valgrind_suppressions="$7"
>
> # Test whether valgrind is available in the test
> # environment. If not, skip the test.
> @@ -35,7 +34,7 @@ ${test_wrapper_env} ${run_program_env} \
> # Test valgrind works with the system ld.so in the test environment
> /bin/sh -c \
> "${test_wrapper_env} ${run_program_env} \
> - valgrind -q --error-exitcode=1 ${valgrind_suppressions} \
> + valgrind -q --error-exitcode=1 \
> ${system_rtld} /bin/echo ${system_rtld}" || exit 77
>
> # Finally the actual test inside the test environment,
> @@ -43,5 +42,5 @@ ${test_wrapper_env} ${run_program_env} \
> # the smoke test under valgrind.
> /bin/sh -c \
> "${test_wrapper_env} ${run_program_env} \
> - valgrind -q --error-exitcode=1 ${valgrind_suppressions} \
> + valgrind -q --error-exitcode=1 \
> ${rtld} --library-path ${library_path} ${test_prog} ${rtld}"
> diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
> index 39048b12e0..ce949dba27 100644
> --- a/sysdeps/x86_64/Makefile
> +++ b/sysdeps/x86_64/Makefile
> @@ -211,14 +211,7 @@ tst-plt-rewrite2-ENV = GLIBC_TUNABLES=glibc.cpu.plt_rewrite=2
> $(objpfx)tst-plt-rewrite2: $(objpfx)tst-plt-rewritemod2.so
> endif
>
> -# Check if ISA level is 3 or above.
> -ifneq (,$(filter $(have-x86-isa-level),$(x86-isa-level-3-or-above)))
> -valgrind-suppressions-tst-valgrind-smoke = \
> - --suppressions=$(..)sysdeps/x86_64/tst-valgrind-smoke.supp
> -endif
> -
> test-internal-extras += tst-gnu2-tls2mod1
> -
> endif # $(subdir) == elf
>
> ifeq ($(subdir),csu)
> diff --git a/sysdeps/x86_64/tst-valgrind-smoke.supp b/sysdeps/x86_64/tst-valgrind-smoke.supp
> deleted file mode 100644
> index 533c2deaff..0000000000
> --- a/sysdeps/x86_64/tst-valgrind-smoke.supp
> +++ /dev/null
> @@ -1,17 +0,0 @@
> -{
> - False positive at strcmp-avx2.S:462
> - Memcheck:Cond
> - fun:strcmp
> - fun:_dl_name_match_p
> - fun:_dl_map_object
> - fun:map_doit
> - fun:_dl_catch_exception
> - fun:_dl_catch_error
> - fun:do_preload
> - fun:handle_preload_list
> - fun:dl_main
> - fun:_dl_sysdep_start
> - fun:_dl_start_final
> - fun:_dl_start
> - obj:*/elf/ld.so
> -}
>
> base-commit: ae7468a7b0bcf22e9cd5fcae42bb9e4f65de83ee
>
LGTM.
Reviewed-by: H.J. Lu <hjl.tools@gmail.com>
Thanks.
--
H.J.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"
2024-04-13 15:56 ` H.J. Lu
@ 2024-04-13 17:50 ` Florian Weimer
0 siblings, 0 replies; 12+ messages in thread
From: Florian Weimer @ 2024-04-13 17:50 UTC (permalink / raw)
To: H.J. Lu; +Cc: libc-alpha, Sunil K Pandey
* H. J. Lu:
>> >> > Could someone proficient in x86-64 assembly have a look at my strcmp
>> >> > implementation?
>> >>
>> >> Can you adapt sysdeps/i386/i686/strcmp.S for valgrind?
>> >>
>> >
>> > You can use sysdeps/x86_64/strcmp.S from glibc 2.10:
>>
>> True. It's mostly a licensing matter at this point. The copyright
>> notice is longer than the assemly …
>
> You can hook your strcmp into glibc and run the glibc strcmp test.
> If it passes the test, it is good to go.
Good suggestion. I verified that it runs, and there are no errors.
So I think we have a solution.
Thanks,
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"
2024-04-13 15:52 ` Florian Weimer
@ 2024-04-13 15:56 ` H.J. Lu
2024-04-13 17:50 ` Florian Weimer
0 siblings, 1 reply; 12+ messages in thread
From: H.J. Lu @ 2024-04-13 15:56 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha, Sunil K Pandey
On Sat, Apr 13, 2024 at 8:52 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Sat, Apr 13, 2024 at 8:27 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>
> >> On Sat, Apr 13, 2024 at 8:20 AM Florian Weimer <fweimer@redhat.com> wrote:
> >> >
> >> > * H. J. Lu:
> >> >
> >> > > On Sat, Apr 13, 2024 at 8:03 AM Florian Weimer <fweimer@redhat.com> wrote:
> >> > >>
> >> > >> * H. J. Lu:
> >> > >>
> >> > >> > On Sat, Apr 13, 2024 at 7:49 AM Florian Weimer <fweimer@redhat.com> wrote:
> >> > >> >>
> >> > >> >> This reverts commit a1735e0aa858f0c8b15e5ee9975bff4279423680.
> >> > >> >>
> >> > >> >> The test failure is a real valgrind bug that needs to be fixed before
> >> > >> >> valgrind is usable with a glibc that has been built with
> >> > >> >> CC="gcc -march=x86-64-v3". The valgrind fix addresses the observed
> >> > >> >> test failure as well.
> >> > >> >>
> >> > >> >> Upstream bug: <https://bugs.kde.org/show_bug.cgi?id=485487>
> >> > >> >>
> >> > >> >> Tested on x86_64-linux-gnu with and without CC="gcc -march=x86-64-v3".
> >> > >> >> The elf/tst-valgrind-smoke tests again fails with an unpatched valgrind,
> >> > >> >> and the proposed upstream patch fixes it. (Apparently, valgrind
> >> > >> >> matches on soname, and not the file name, which is ld.so in the test.)
> >> > >> >
> >> > >> > Please clarify that your patch fixes the glibc valgrind test by not using
> >> > >> > strcmp-avx2.S in the valgrind test.
> >> > >>
> >> > >> Sorry, clarify where? In the commit message here? On the upstream bug?
> >> > >
> >> > > In the glibc commit message.
> >> > >
> >> > >> My valgrind patch replaces the strcmp implementation in ld.so with its
> >> > >> own.
> >> > >>
> >> > >
> >> > > Please mention it in the glibc commit message.
> >> >
> >> > Okay, I'll send a v2.
> >> >
> >> > Could someone proficient in x86-64 assembly have a look at my strcmp
> >> > implementation?
> >>
> >> Can you adapt sysdeps/i386/i686/strcmp.S for valgrind?
> >>
> >
> > You can use sysdeps/x86_64/strcmp.S from glibc 2.10:
>
> True. It's mostly a licensing matter at this point. The copyright
> notice is longer than the assemly …
>
You can hook your strcmp into glibc and run the glibc strcmp test.
If it passes the test, it is good to go.
--
H.J.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"
2024-04-13 15:49 ` H.J. Lu
@ 2024-04-13 15:52 ` Florian Weimer
2024-04-13 15:56 ` H.J. Lu
0 siblings, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2024-04-13 15:52 UTC (permalink / raw)
To: H.J. Lu; +Cc: libc-alpha, Sunil K Pandey
* H. J. Lu:
> On Sat, Apr 13, 2024 at 8:27 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Sat, Apr 13, 2024 at 8:20 AM Florian Weimer <fweimer@redhat.com> wrote:
>> >
>> > * H. J. Lu:
>> >
>> > > On Sat, Apr 13, 2024 at 8:03 AM Florian Weimer <fweimer@redhat.com> wrote:
>> > >>
>> > >> * H. J. Lu:
>> > >>
>> > >> > On Sat, Apr 13, 2024 at 7:49 AM Florian Weimer <fweimer@redhat.com> wrote:
>> > >> >>
>> > >> >> This reverts commit a1735e0aa858f0c8b15e5ee9975bff4279423680.
>> > >> >>
>> > >> >> The test failure is a real valgrind bug that needs to be fixed before
>> > >> >> valgrind is usable with a glibc that has been built with
>> > >> >> CC="gcc -march=x86-64-v3". The valgrind fix addresses the observed
>> > >> >> test failure as well.
>> > >> >>
>> > >> >> Upstream bug: <https://bugs.kde.org/show_bug.cgi?id=485487>
>> > >> >>
>> > >> >> Tested on x86_64-linux-gnu with and without CC="gcc -march=x86-64-v3".
>> > >> >> The elf/tst-valgrind-smoke tests again fails with an unpatched valgrind,
>> > >> >> and the proposed upstream patch fixes it. (Apparently, valgrind
>> > >> >> matches on soname, and not the file name, which is ld.so in the test.)
>> > >> >
>> > >> > Please clarify that your patch fixes the glibc valgrind test by not using
>> > >> > strcmp-avx2.S in the valgrind test.
>> > >>
>> > >> Sorry, clarify where? In the commit message here? On the upstream bug?
>> > >
>> > > In the glibc commit message.
>> > >
>> > >> My valgrind patch replaces the strcmp implementation in ld.so with its
>> > >> own.
>> > >>
>> > >
>> > > Please mention it in the glibc commit message.
>> >
>> > Okay, I'll send a v2.
>> >
>> > Could someone proficient in x86-64 assembly have a look at my strcmp
>> > implementation?
>>
>> Can you adapt sysdeps/i386/i686/strcmp.S for valgrind?
>>
>
> You can use sysdeps/x86_64/strcmp.S from glibc 2.10:
True. It's mostly a licensing matter at this point. The copyright
notice is longer than the assemly …
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"
2024-04-13 15:27 ` H.J. Lu
@ 2024-04-13 15:49 ` H.J. Lu
2024-04-13 15:52 ` Florian Weimer
0 siblings, 1 reply; 12+ messages in thread
From: H.J. Lu @ 2024-04-13 15:49 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha, Sunil K Pandey
On Sat, Apr 13, 2024 at 8:27 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Sat, Apr 13, 2024 at 8:20 AM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * H. J. Lu:
> >
> > > On Sat, Apr 13, 2024 at 8:03 AM Florian Weimer <fweimer@redhat.com> wrote:
> > >>
> > >> * H. J. Lu:
> > >>
> > >> > On Sat, Apr 13, 2024 at 7:49 AM Florian Weimer <fweimer@redhat.com> wrote:
> > >> >>
> > >> >> This reverts commit a1735e0aa858f0c8b15e5ee9975bff4279423680.
> > >> >>
> > >> >> The test failure is a real valgrind bug that needs to be fixed before
> > >> >> valgrind is usable with a glibc that has been built with
> > >> >> CC="gcc -march=x86-64-v3". The valgrind fix addresses the observed
> > >> >> test failure as well.
> > >> >>
> > >> >> Upstream bug: <https://bugs.kde.org/show_bug.cgi?id=485487>
> > >> >>
> > >> >> Tested on x86_64-linux-gnu with and without CC="gcc -march=x86-64-v3".
> > >> >> The elf/tst-valgrind-smoke tests again fails with an unpatched valgrind,
> > >> >> and the proposed upstream patch fixes it. (Apparently, valgrind
> > >> >> matches on soname, and not the file name, which is ld.so in the test.)
> > >> >
> > >> > Please clarify that your patch fixes the glibc valgrind test by not using
> > >> > strcmp-avx2.S in the valgrind test.
> > >>
> > >> Sorry, clarify where? In the commit message here? On the upstream bug?
> > >
> > > In the glibc commit message.
> > >
> > >> My valgrind patch replaces the strcmp implementation in ld.so with its
> > >> own.
> > >>
> > >
> > > Please mention it in the glibc commit message.
> >
> > Okay, I'll send a v2.
> >
> > Could someone proficient in x86-64 assembly have a look at my strcmp
> > implementation?
>
> Can you adapt sysdeps/i386/i686/strcmp.S for valgrind?
>
You can use sysdeps/x86_64/strcmp.S from glibc 2.10:
ENTRY (BP_SYM (strcmp))
L(oop): movb (%rdi), %al
cmpb (%rsi), %al
jne L(neq)
incq %rdi
incq %rsi
testb %al, %al
jnz L(oop)
xorl %eax, %eax
ret
L(neq): movl $1, %eax
movl $-1, %ecx
cmovbl %ecx, %eax
ret
END (BP_SYM (strcmp))
--
H.J.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"
2024-04-13 15:20 ` Florian Weimer
@ 2024-04-13 15:27 ` H.J. Lu
2024-04-13 15:49 ` H.J. Lu
0 siblings, 1 reply; 12+ messages in thread
From: H.J. Lu @ 2024-04-13 15:27 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha, Sunil K Pandey
On Sat, Apr 13, 2024 at 8:20 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Sat, Apr 13, 2024 at 8:03 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu:
> >>
> >> > On Sat, Apr 13, 2024 at 7:49 AM Florian Weimer <fweimer@redhat.com> wrote:
> >> >>
> >> >> This reverts commit a1735e0aa858f0c8b15e5ee9975bff4279423680.
> >> >>
> >> >> The test failure is a real valgrind bug that needs to be fixed before
> >> >> valgrind is usable with a glibc that has been built with
> >> >> CC="gcc -march=x86-64-v3". The valgrind fix addresses the observed
> >> >> test failure as well.
> >> >>
> >> >> Upstream bug: <https://bugs.kde.org/show_bug.cgi?id=485487>
> >> >>
> >> >> Tested on x86_64-linux-gnu with and without CC="gcc -march=x86-64-v3".
> >> >> The elf/tst-valgrind-smoke tests again fails with an unpatched valgrind,
> >> >> and the proposed upstream patch fixes it. (Apparently, valgrind
> >> >> matches on soname, and not the file name, which is ld.so in the test.)
> >> >
> >> > Please clarify that your patch fixes the glibc valgrind test by not using
> >> > strcmp-avx2.S in the valgrind test.
> >>
> >> Sorry, clarify where? In the commit message here? On the upstream bug?
> >
> > In the glibc commit message.
> >
> >> My valgrind patch replaces the strcmp implementation in ld.so with its
> >> own.
> >>
> >
> > Please mention it in the glibc commit message.
>
> Okay, I'll send a v2.
>
> Could someone proficient in x86-64 assembly have a look at my strcmp
> implementation?
Can you adapt sysdeps/i386/i686/strcmp.S for valgrind?
> <https://bugsfiles.kde.org/attachment.cgi?id=168460>
>
> It seems to work, but …
>
> Thanks,
> Florian
>
--
H.J.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"
2024-04-13 15:09 ` H.J. Lu
@ 2024-04-13 15:20 ` Florian Weimer
2024-04-13 15:27 ` H.J. Lu
0 siblings, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2024-04-13 15:20 UTC (permalink / raw)
To: H.J. Lu; +Cc: libc-alpha, Sunil K Pandey
* H. J. Lu:
> On Sat, Apr 13, 2024 at 8:03 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > On Sat, Apr 13, 2024 at 7:49 AM Florian Weimer <fweimer@redhat.com> wrote:
>> >>
>> >> This reverts commit a1735e0aa858f0c8b15e5ee9975bff4279423680.
>> >>
>> >> The test failure is a real valgrind bug that needs to be fixed before
>> >> valgrind is usable with a glibc that has been built with
>> >> CC="gcc -march=x86-64-v3". The valgrind fix addresses the observed
>> >> test failure as well.
>> >>
>> >> Upstream bug: <https://bugs.kde.org/show_bug.cgi?id=485487>
>> >>
>> >> Tested on x86_64-linux-gnu with and without CC="gcc -march=x86-64-v3".
>> >> The elf/tst-valgrind-smoke tests again fails with an unpatched valgrind,
>> >> and the proposed upstream patch fixes it. (Apparently, valgrind
>> >> matches on soname, and not the file name, which is ld.so in the test.)
>> >
>> > Please clarify that your patch fixes the glibc valgrind test by not using
>> > strcmp-avx2.S in the valgrind test.
>>
>> Sorry, clarify where? In the commit message here? On the upstream bug?
>
> In the glibc commit message.
>
>> My valgrind patch replaces the strcmp implementation in ld.so with its
>> own.
>>
>
> Please mention it in the glibc commit message.
Okay, I'll send a v2.
Could someone proficient in x86-64 assembly have a look at my strcmp
implementation?
<https://bugsfiles.kde.org/attachment.cgi?id=168460>
It seems to work, but …
Thanks,
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"
2024-04-13 15:03 ` Florian Weimer
@ 2024-04-13 15:09 ` H.J. Lu
2024-04-13 15:20 ` Florian Weimer
0 siblings, 1 reply; 12+ messages in thread
From: H.J. Lu @ 2024-04-13 15:09 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha, Sunil K Pandey
On Sat, Apr 13, 2024 at 8:03 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Sat, Apr 13, 2024 at 7:49 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> This reverts commit a1735e0aa858f0c8b15e5ee9975bff4279423680.
> >>
> >> The test failure is a real valgrind bug that needs to be fixed before
> >> valgrind is usable with a glibc that has been built with
> >> CC="gcc -march=x86-64-v3". The valgrind fix addresses the observed
> >> test failure as well.
> >>
> >> Upstream bug: <https://bugs.kde.org/show_bug.cgi?id=485487>
> >>
> >> Tested on x86_64-linux-gnu with and without CC="gcc -march=x86-64-v3".
> >> The elf/tst-valgrind-smoke tests again fails with an unpatched valgrind,
> >> and the proposed upstream patch fixes it. (Apparently, valgrind
> >> matches on soname, and not the file name, which is ld.so in the test.)
> >
> > Please clarify that your patch fixes the glibc valgrind test by not using
> > strcmp-avx2.S in the valgrind test.
>
> Sorry, clarify where? In the commit message here? On the upstream bug?
In the glibc commit message.
> My valgrind patch replaces the strcmp implementation in ld.so with its
> own.
>
Please mention it in the glibc commit message.
--
H.J.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"
2024-04-13 14:58 ` H.J. Lu
@ 2024-04-13 15:03 ` Florian Weimer
2024-04-13 15:09 ` H.J. Lu
0 siblings, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2024-04-13 15:03 UTC (permalink / raw)
To: H.J. Lu; +Cc: libc-alpha, Sunil K Pandey
* H. J. Lu:
> On Sat, Apr 13, 2024 at 7:49 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> This reverts commit a1735e0aa858f0c8b15e5ee9975bff4279423680.
>>
>> The test failure is a real valgrind bug that needs to be fixed before
>> valgrind is usable with a glibc that has been built with
>> CC="gcc -march=x86-64-v3". The valgrind fix addresses the observed
>> test failure as well.
>>
>> Upstream bug: <https://bugs.kde.org/show_bug.cgi?id=485487>
>>
>> Tested on x86_64-linux-gnu with and without CC="gcc -march=x86-64-v3".
>> The elf/tst-valgrind-smoke tests again fails with an unpatched valgrind,
>> and the proposed upstream patch fixes it. (Apparently, valgrind
>> matches on soname, and not the file name, which is ld.so in the test.)
>
> Please clarify that your patch fixes the glibc valgrind test by not using
> strcmp-avx2.S in the valgrind test.
Sorry, clarify where? In the commit message here? On the upstream bug?
My valgrind patch replaces the strcmp implementation in ld.so with its
own.
Thanks,
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"
2024-04-13 14:48 Florian Weimer
@ 2024-04-13 14:58 ` H.J. Lu
2024-04-13 15:03 ` Florian Weimer
0 siblings, 1 reply; 12+ messages in thread
From: H.J. Lu @ 2024-04-13 14:58 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha, Sunil K Pandey
On Sat, Apr 13, 2024 at 7:49 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> This reverts commit a1735e0aa858f0c8b15e5ee9975bff4279423680.
>
> The test failure is a real valgrind bug that needs to be fixed before
> valgrind is usable with a glibc that has been built with
> CC="gcc -march=x86-64-v3". The valgrind fix addresses the observed
> test failure as well.
>
> Upstream bug: <https://bugs.kde.org/show_bug.cgi?id=485487>
>
> Tested on x86_64-linux-gnu with and without CC="gcc -march=x86-64-v3".
> The elf/tst-valgrind-smoke tests again fails with an unpatched valgrind,
> and the proposed upstream patch fixes it. (Apparently, valgrind
> matches on soname, and not the file name, which is ld.so in the test.)
Please clarify that your patch fixes the glibc valgrind test by not using
strcmp-avx2.S in the valgrind test.
> ---
> elf/Makefile | 3 +--
> elf/tst-valgrind-smoke.sh | 5 ++---
> sysdeps/x86_64/Makefile | 7 -------
> sysdeps/x86_64/tst-valgrind-smoke.supp | 17 -----------------
> 4 files changed, 3 insertions(+), 29 deletions(-)
>
> diff --git a/elf/Makefile b/elf/Makefile
> index 6dad11bcfb..fb3c3c4dff 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -618,8 +618,7 @@ tests-special += $(objpfx)tst-valgrind-smoke.out
> endif
> $(objpfx)tst-valgrind-smoke.out: tst-valgrind-smoke.sh $(objpfx)ld.so $(objpfx)valgrind-test
> $(SHELL) $< $(objpfx)ld.so $(rtlddir)/$(rtld-installed-name) '$(test-wrapper-env)' \
> - '$(run-program-env)' '$(rpath-link)' $(objpfx)valgrind-test \
> - '$(valgrind-suppressions-tst-valgrind-smoke)' > $@; $(evaluate-test)
> + '$(run-program-env)' '$(rpath-link)' $(objpfx)valgrind-test > $@; $(evaluate-test)
>
> ifeq ($(run-built-tests),yes)
> tests-special += $(objpfx)tst-rtld-does-not-exist.out
> diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh
> index 7d0ceac6b1..e33b2fa1d7 100644
> --- a/elf/tst-valgrind-smoke.sh
> +++ b/elf/tst-valgrind-smoke.sh
> @@ -25,7 +25,6 @@ test_wrapper_env="$3"
> run_program_env="$4"
> library_path="$5"
> test_prog="$6"
> -valgrind_suppressions="$7"
>
> # Test whether valgrind is available in the test
> # environment. If not, skip the test.
> @@ -35,7 +34,7 @@ ${test_wrapper_env} ${run_program_env} \
> # Test valgrind works with the system ld.so in the test environment
> /bin/sh -c \
> "${test_wrapper_env} ${run_program_env} \
> - valgrind -q --error-exitcode=1 ${valgrind_suppressions} \
> + valgrind -q --error-exitcode=1 \
> ${system_rtld} /bin/echo ${system_rtld}" || exit 77
>
> # Finally the actual test inside the test environment,
> @@ -43,5 +42,5 @@ ${test_wrapper_env} ${run_program_env} \
> # the smoke test under valgrind.
> /bin/sh -c \
> "${test_wrapper_env} ${run_program_env} \
> - valgrind -q --error-exitcode=1 ${valgrind_suppressions} \
> + valgrind -q --error-exitcode=1 \
> ${rtld} --library-path ${library_path} ${test_prog} ${rtld}"
> diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
> index 39048b12e0..ce949dba27 100644
> --- a/sysdeps/x86_64/Makefile
> +++ b/sysdeps/x86_64/Makefile
> @@ -211,14 +211,7 @@ tst-plt-rewrite2-ENV = GLIBC_TUNABLES=glibc.cpu.plt_rewrite=2
> $(objpfx)tst-plt-rewrite2: $(objpfx)tst-plt-rewritemod2.so
> endif
>
> -# Check if ISA level is 3 or above.
> -ifneq (,$(filter $(have-x86-isa-level),$(x86-isa-level-3-or-above)))
> -valgrind-suppressions-tst-valgrind-smoke = \
> - --suppressions=$(..)sysdeps/x86_64/tst-valgrind-smoke.supp
> -endif
> -
> test-internal-extras += tst-gnu2-tls2mod1
> -
> endif # $(subdir) == elf
>
> ifeq ($(subdir),csu)
> diff --git a/sysdeps/x86_64/tst-valgrind-smoke.supp b/sysdeps/x86_64/tst-valgrind-smoke.supp
> deleted file mode 100644
> index 533c2deaff..0000000000
> --- a/sysdeps/x86_64/tst-valgrind-smoke.supp
> +++ /dev/null
> @@ -1,17 +0,0 @@
> -{
> - False positive at strcmp-avx2.S:462
> - Memcheck:Cond
> - fun:strcmp
> - fun:_dl_name_match_p
> - fun:_dl_map_object
> - fun:map_doit
> - fun:_dl_catch_exception
> - fun:_dl_catch_error
> - fun:do_preload
> - fun:handle_preload_list
> - fun:dl_main
> - fun:_dl_sysdep_start
> - fun:_dl_start_final
> - fun:_dl_start
> - obj:*/elf/ld.so
> -}
>
> base-commit: ae7468a7b0bcf22e9cd5fcae42bb9e4f65de83ee
>
--
H.J.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] Revert "x86_64: Suppress false positive valgrind error"
@ 2024-04-13 14:48 Florian Weimer
2024-04-13 14:58 ` H.J. Lu
0 siblings, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2024-04-13 14:48 UTC (permalink / raw)
To: libc-alpha; +Cc: H.J. Lu, Sunil K Pandey
This reverts commit a1735e0aa858f0c8b15e5ee9975bff4279423680.
The test failure is a real valgrind bug that needs to be fixed before
valgrind is usable with a glibc that has been built with
CC="gcc -march=x86-64-v3". The valgrind fix addresses the observed
test failure as well.
Upstream bug: <https://bugs.kde.org/show_bug.cgi?id=485487>
Tested on x86_64-linux-gnu with and without CC="gcc -march=x86-64-v3".
The elf/tst-valgrind-smoke tests again fails with an unpatched valgrind,
and the proposed upstream patch fixes it. (Apparently, valgrind
matches on soname, and not the file name, which is ld.so in the test.)
---
elf/Makefile | 3 +--
elf/tst-valgrind-smoke.sh | 5 ++---
sysdeps/x86_64/Makefile | 7 -------
sysdeps/x86_64/tst-valgrind-smoke.supp | 17 -----------------
4 files changed, 3 insertions(+), 29 deletions(-)
diff --git a/elf/Makefile b/elf/Makefile
index 6dad11bcfb..fb3c3c4dff 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -618,8 +618,7 @@ tests-special += $(objpfx)tst-valgrind-smoke.out
endif
$(objpfx)tst-valgrind-smoke.out: tst-valgrind-smoke.sh $(objpfx)ld.so $(objpfx)valgrind-test
$(SHELL) $< $(objpfx)ld.so $(rtlddir)/$(rtld-installed-name) '$(test-wrapper-env)' \
- '$(run-program-env)' '$(rpath-link)' $(objpfx)valgrind-test \
- '$(valgrind-suppressions-tst-valgrind-smoke)' > $@; $(evaluate-test)
+ '$(run-program-env)' '$(rpath-link)' $(objpfx)valgrind-test > $@; $(evaluate-test)
ifeq ($(run-built-tests),yes)
tests-special += $(objpfx)tst-rtld-does-not-exist.out
diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh
index 7d0ceac6b1..e33b2fa1d7 100644
--- a/elf/tst-valgrind-smoke.sh
+++ b/elf/tst-valgrind-smoke.sh
@@ -25,7 +25,6 @@ test_wrapper_env="$3"
run_program_env="$4"
library_path="$5"
test_prog="$6"
-valgrind_suppressions="$7"
# Test whether valgrind is available in the test
# environment. If not, skip the test.
@@ -35,7 +34,7 @@ ${test_wrapper_env} ${run_program_env} \
# Test valgrind works with the system ld.so in the test environment
/bin/sh -c \
"${test_wrapper_env} ${run_program_env} \
- valgrind -q --error-exitcode=1 ${valgrind_suppressions} \
+ valgrind -q --error-exitcode=1 \
${system_rtld} /bin/echo ${system_rtld}" || exit 77
# Finally the actual test inside the test environment,
@@ -43,5 +42,5 @@ ${test_wrapper_env} ${run_program_env} \
# the smoke test under valgrind.
/bin/sh -c \
"${test_wrapper_env} ${run_program_env} \
- valgrind -q --error-exitcode=1 ${valgrind_suppressions} \
+ valgrind -q --error-exitcode=1 \
${rtld} --library-path ${library_path} ${test_prog} ${rtld}"
diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
index 39048b12e0..ce949dba27 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -211,14 +211,7 @@ tst-plt-rewrite2-ENV = GLIBC_TUNABLES=glibc.cpu.plt_rewrite=2
$(objpfx)tst-plt-rewrite2: $(objpfx)tst-plt-rewritemod2.so
endif
-# Check if ISA level is 3 or above.
-ifneq (,$(filter $(have-x86-isa-level),$(x86-isa-level-3-or-above)))
-valgrind-suppressions-tst-valgrind-smoke = \
- --suppressions=$(..)sysdeps/x86_64/tst-valgrind-smoke.supp
-endif
-
test-internal-extras += tst-gnu2-tls2mod1
-
endif # $(subdir) == elf
ifeq ($(subdir),csu)
diff --git a/sysdeps/x86_64/tst-valgrind-smoke.supp b/sysdeps/x86_64/tst-valgrind-smoke.supp
deleted file mode 100644
index 533c2deaff..0000000000
--- a/sysdeps/x86_64/tst-valgrind-smoke.supp
+++ /dev/null
@@ -1,17 +0,0 @@
-{
- False positive at strcmp-avx2.S:462
- Memcheck:Cond
- fun:strcmp
- fun:_dl_name_match_p
- fun:_dl_map_object
- fun:map_doit
- fun:_dl_catch_exception
- fun:_dl_catch_error
- fun:do_preload
- fun:handle_preload_list
- fun:dl_main
- fun:_dl_sysdep_start
- fun:_dl_start_final
- fun:_dl_start
- obj:*/elf/ld.so
-}
base-commit: ae7468a7b0bcf22e9cd5fcae42bb9e4f65de83ee
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-04-13 17:51 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-13 15:21 [PATCH] Revert "x86_64: Suppress false positive valgrind error" Florian Weimer
2024-04-13 15:27 ` H.J. Lu
-- strict thread matches above, loose matches on Subject: below --
2024-04-13 14:48 Florian Weimer
2024-04-13 14:58 ` H.J. Lu
2024-04-13 15:03 ` Florian Weimer
2024-04-13 15:09 ` H.J. Lu
2024-04-13 15:20 ` Florian Weimer
2024-04-13 15:27 ` H.J. Lu
2024-04-13 15:49 ` H.J. Lu
2024-04-13 15:52 ` Florian Weimer
2024-04-13 15:56 ` H.J. Lu
2024-04-13 17:50 ` Florian Weimer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).