Bug 489088

Created attachment 170895 [details]
foo.xz

foo.xz is `test` from above (compiled binary from `test.c` attached earlier, using \gcc-15 test.c -o foo -O3 -ggdb3 -march=native -fipa-pta -fno-vect-cost-model`).

On this system, -march=native is -march=znver2 (pretty much).

```
$ gcc-15 --version
gcc-15 (Gentoo Hardened 15.0.9999 p, commit e275a9e2e7e85e8c0071f7cffbd7948bc891573d) 15.0.0 20240621 (experimental) 52a82359073653e312aaa5703f7e0ce339588961
Copyright (C) 2024 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
```

Note that this is also unreleased gcc and using binutils from recent trunk too.

```
$ ld --version
GNU ld (Gentoo 9999 p1) 2.42.50.20240615
Copyright (C) 2024 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) a later version.
This program has absolutely no warranty.
```

Again, I don't mind if the bug is junk / not a Valgrind problem :)

This looks like vmovq:

   0x0000000000109df7 <+1111>:	vshufps $0x55,%xmm1,%xmm1,%xmm4
   0x0000000000109dfc <+1116>:	vmovshdup %xmm0,%xmm1
=> 0x0000000000109e00 <+1120>:	vmovq  %xmm12,%xmm0
   0x0000000000109e04 <+1124>:	vmovq  %xmm3,%xmm3
   0x0000000000109e08 <+1128>:	vmovaps %xmm2,-0x50(%rbp)

See also https://bugs.kde.org/show_bug.cgi?id=391148 which comes with a patch.

(In reply to Mark Wielaard from comment #4)
> See also https://bugs.kde.org/show_bug.cgi?id=391148 which comes with a
> patch.

Hi Mark, thanks for looking!

That patch seems to work:

```
==144687== Invalid read of size 8
==144687==    at 0x109F43: filter (test.c:59)
==144687==    by 0x109F43: AnalyzeSamples (test.c:100)
==144687==    by 0x109366: main (test.c:167)
==144687==  Address 0x4637bfeabfe4bbd4 is not stack'd, malloc'd or (recently) free'd
==144687==
==144687==
==144687== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==144687==  General Protection Fault
==144687==    at 0x109F43: filter (test.c:59)
==144687==    by 0x109F43: AnalyzeSamples (test.c:100)
==144687==    by 0x109366: main (test.c:167)
```
although the address looks a bit suspicious and I wasn't aware of out of bounds access in the testcase, so not sure if it really is decoding fully correctly?

(In reply to Sam James from comment #5)
> (In reply to Mark Wielaard from comment #4)
> > See also https://bugs.kde.org/show_bug.cgi?id=391148 which comes with a
> > patch.
> 
> Hi Mark, thanks for looking!
> 
> That patch seems to work:
> 
> ```
> ==144687== Invalid read of size 8
> ==144687==    at 0x109F43: filter (test.c:59)
> ==144687==    by 0x109F43: AnalyzeSamples (test.c:100)
> ==144687==    by 0x109366: main (test.c:167)
> ==144687==  Address 0x4637bfeabfe4bbd4 is not stack'd, malloc'd or
> (recently) free'd
> ==144687==
> ==144687==
> ==144687== Process terminating with default action of signal 11 (SIGSEGV):
> dumping core
> ==144687==  General Protection Fault
> ==144687==    at 0x109F43: filter (test.c:59)
> ==144687==    by 0x109F43: AnalyzeSamples (test.c:100)
> ==144687==    by 0x109366: main (test.c:167)
> ```
> although the address looks a bit suspicious and I wasn't aware of out of
> bounds access in the testcase, so not sure if it really is decoding fully
> correctly?

hmmm, yeah that is kind of odd. That address is so weird that it really must be invalid. Does the original program also try to read from that address and produce a SIGSEGV when not run under valgrind?

Maybe as a quick hack check you could change that DIP("vmovq %s,%s\n", nameXMMReg(rG), nameIReg64(rE)); in the patch to vex_printf(...)

If it decodes correctly it should print vmovq  %xmm12,%xmm0

(You can also get some of that with something like --trace-flags=10000000 --trace-notbelow=1000 but that is really verbose)

(In reply to Mark Wielaard from comment #6)
> (In reply to Sam James from comment #5)
> > (In reply to Mark Wielaard from comment #4)
> > > See also https://bugs.kde.org/show_bug.cgi?id=391148 which comes with a
> > > patch.
> > 
> > Hi Mark, thanks for looking!
> > 
> > That patch seems to work:
> > 
> > ```
> > ==144687== Invalid read of size 8
> > ==144687==    at 0x109F43: filter (test.c:59)
> > ==144687==    by 0x109F43: AnalyzeSamples (test.c:100)
> > ==144687==    by 0x109366: main (test.c:167)
> > ==144687==  Address 0x4637bfeabfe4bbd4 is not stack'd, malloc'd or
> > (recently) free'd
> > ==144687==
> > ==144687==
> > ==144687== Process terminating with default action of signal 11 (SIGSEGV):
> > dumping core
> > ==144687==  General Protection Fault
> > ==144687==    at 0x109F43: filter (test.c:59)
> > ==144687==    by 0x109F43: AnalyzeSamples (test.c:100)
> > ==144687==    by 0x109366: main (test.c:167)
> > ```
> > although the address looks a bit suspicious and I wasn't aware of out of
> > bounds access in the testcase, so not sure if it really is decoding fully
> > correctly?
> 
> hmmm, yeah that is kind of odd. That address is so weird that it really must
> be invalid. Does the original program also try to read from that address and
> produce a SIGSEGV when not run under valgrind?

Nope!

> 
> Maybe as a quick hack check you could change that DIP("vmovq %s,%s\n",
> nameXMMReg(rG), nameIReg64(rE)); in the patch to vex_printf(...)
> 
> If it decodes correctly it should print vmovq  %xmm12,%xmm0
> 

==402260==
vmovq %xmm12,%r8
vmovq %xmm14,%r10
==402260== Invalid read of size 8
==402260==    at 0x109F46: filter (test.c:59)
==402260==    by 0x109F46: AnalyzeSamples (test.c:100)
==402260==    by 0x109366: main (test.c:167)
==402260==  Address 0x4637bfeabfe4bbd4 is not stack'd, malloc'd or (recently) free'd

So I think the implementation should be something like:

diff --git a/VEX/priv/guest_amd64_toIR.c b/VEX/priv/guest_amd64_toIR.c
index 28c37f092211..a7ea5951aac7 100644
--- a/VEX/priv/guest_amd64_toIR.c
+++ b/VEX/priv/guest_amd64_toIR.c
@@ -27026,6 +27026,14 @@ Long dis_ESC_0F__VEX (
          if (epartIsReg(modrm)) {
             /* fall through, awaiting test case */
             /* dst: lo half copied, hi half zeroed */
+            UInt rE = eregOfRexRM(pfx,modrm);
+            putXMMRegLane64( rE, 0, getXMMRegLane64( rG, 0 ));
+            /* zero bits 255:64 */
+            putXMMRegLane64( rG, 1, mkU64(0) );
+            putYMMRegLane128( rG, 1, mkV128(0) );
+            DIP("vmovq %s,%s\n", nameXMMReg(rG), nameXMMReg(rE));
+            delta += 1;
+            goto decode_success;
          } else {
             addr = disAMode ( &alen, vbi, pfx, delta, dis_buf, 0 );
             storeLE( mkexpr(addr), getXMMRegLane64( rG, 0 ));

But obviously we need some real test cases.

> But obviously we need some real test cases.

I was confused by what you meant, then I checked the See Also'd bugs and saw there's no testcases there except for an artificial one :|

Created attachment 171207 [details]
Implement VMOVQ xmm1, xmm2/m64

Added testcase, which showed we cleared the wrong register (oops). Fixed that in the implementation.

Thank you very much - that works great!

commit 10a22445d747817932692b1c1ee3faa726121cb4
Author: Mark Wielaard <mark@klomp.org>
Date:   Sun Jun 30 20:17:32 2024 +0200

    Implement VMOVQ xmm1, xmm2/m64
    
    We implemented the memory variant already, but not the reg variant.
    Add a separate avx-vmovq testcase, because avx-1 is already really big.
    
    https://bugs.kde.org/show_bug.cgi?id=391148
    https://bugs.kde.org/show_bug.cgi?id=417572
    https://bugs.kde.org/show_bug.cgi?id=489088