[PATCH] Revert "x86_64: Suppress false positive valgrind error"

[PATCH] Revert "x86_64: Suppress false positive valgrind error"

[Florian Weimer]

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

Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"

[H.J. Lu]

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.

Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"

[Florian Weimer]

* 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

Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"

[H.J. Lu]

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.

Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"

[Florian Weimer]

* 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

Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"

[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:

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.

Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"

[H.J. Lu]

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.

Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"

[Florian Weimer]

* 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

Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"

[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.

-- 
H.J.

Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"

[Florian Weimer]

* 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

Re: [PATCH] Revert "x86_64: Suppress false positive valgrind error"

[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.

> ---
>  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.

[PATCH] Revert "x86_64: Suppress false positive valgrind error"

[Florian Weimer]

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