| Message ID | 87a5lx2u9a.fsf@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "x86_64: Suppress false positive valgrind error" | expand |
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. 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
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. 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
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 >
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. 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
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. 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
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 -}