Created attachment 173510 [details] Patch SUMMARY On current Valgrind (VALGRIND_3_23_0-107-g1a1343b13), the translation for `xchg ax, r16` will mistakenly clear rax[63:16]. Attached patch corrects the issue. STEPS TO REPRODUCE The following test program demonstrates this issue: ```c // gcc -o test_xchg_ax_dx test_xchg_ax_dx.c #include <stdint.h> #include <stdio.h> #include <assert.h> int main(int argc, char const *argv[]) { uint64_t rax = 0xfbcadd99fbca7654; uint64_t rdx = 0x1234fdb512345678; asm volatile ( "xchg %%ax, %%dx;" : "=a"(rax), "=d"(rdx) : "a"(rax), "d"(rdx) ); printf("rax = %016lx, rdx = %016lx\n", rax, rdx); assert(rax == 0xfbcadd99fbca5678); assert(rdx == 0x1234fdb512347654); return 0; } ``` OBSERVED RESULT ``` matt@iron:~/valgrind-src$ ./bin/valgrind ./test_xchg_ax_dx ==61465== Memcheck, a memory error detector ==61465== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al. ==61465== Using Valgrind-3.24.0.GIT and LibVEX; rerun with -h for copyright info ==61465== Command: ./test_xchg_ax_dx ==61465== rax = 0000000000005678, rdx = 1234fdb512347654 test_xchg_ax_dx: test_xchg_ax_dx.c:15: main: Assertion `rax == 0xfbcadd99fbca5678' failed. ==61465== ==61465== Process terminating with default action of signal 6 (SIGABRT) ==61465== at 0x4912B1C: __pthread_kill_implementation (pthread_kill.c:44) ==61465== by 0x4912B1C: __pthread_kill_internal (pthread_kill.c:78) ==61465== by 0x4912B1C: pthread_kill@@GLIBC_2.34 (pthread_kill.c:89) ==61465== by 0x48B926D: raise (raise.c:26) ==61465== by 0x489C8FE: abort (abort.c:79) ==61465== by 0x489C81A: __assert_fail_base.cold (assert.c:94) ==61465== by 0x48AF506: __assert_fail (assert.c:103) ==61465== by 0x109200: main (in /home/matt/valgrind-src/test_xchg_ax_dx) ==61465== ==61465== HEAP SUMMARY: ==61465== in use at exit: 1,024 bytes in 1 blocks ==61465== total heap usage: 6 allocs, 5 frees, 2,972 bytes allocated ==61465== ==61465== LEAK SUMMARY: ==61465== definitely lost: 0 bytes in 0 blocks ==61465== indirectly lost: 0 bytes in 0 blocks ==61465== possibly lost: 0 bytes in 0 blocks ==61465== still reachable: 1,024 bytes in 1 blocks ==61465== suppressed: 0 bytes in 0 blocks ==61465== Rerun with --leak-check=full to see details of leaked memory ==61465== ==61465== For lists of detected and suppressed errors, rerun with: -s ==61465== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) Aborted ``` EXPECTED RESULT ``` matt@iron:~/valgrind-src$ ./test_xchg_ax_dx rax = fbcadd99fbca5678, rdx = 1234fdb512347654 ``` SOFTWARE/OS VERSIONS Windows: macOS: (available in the Info Center app, or by running `kinfo` in a terminal window) Linux/KDE Plasma: KDE Plasma Version: KDE Frameworks Version: Qt Version: ADDITIONAL INFORMATION Issue originally reported to angr at https://github.com/angr/angr/issues/3878 by mnd-c.
Comment on attachment 173510 [details] Patch >From 2b3142152cfc315bbdcd504d5d7cdc0ddaeb9b27 Mon Sep 17 00:00:00 2001 >From: Matt Borgerson <contact@mborgerson.com> >Date: Mon, 9 Sep 2024 14:48:12 -0700 >Subject: [PATCH] codegen_xchg_rAX_Reg: Preserve RAX[63:16] in `xchg ax, r16` > >Unlike 32-bit xchg, 16-bit `xchg ax, r16` should not modify the upper 48 >bits of operand registers. Before this patch, RAX[63:16] will be cleared >in call to putIReg16 for RAX assignment. > >This patch reworks codegen_xchg_rAX_Reg to unify size handling and use >putIRegRAX to handle this size-dependent extension correctly. >--- > VEX/priv/guest_amd64_toIR.c | 29 ++++++----------------------- > 1 file changed, 6 insertions(+), 23 deletions(-) > >diff --git a/VEX/priv/guest_amd64_toIR.c b/VEX/priv/guest_amd64_toIR.c >index 57a8a434b..f6f8d4abe 100644 >--- a/VEX/priv/guest_amd64_toIR.c >+++ b/VEX/priv/guest_amd64_toIR.c >@@ -1152,13 +1152,6 @@ static IRExpr* getIReg16 ( UInt regno ) > Ity_I64 )); > } > >-static void putIReg16 ( UInt regno, IRExpr* e ) >-{ >- vassert(typeOfIRExpr(irsb->tyenv,e) == Ity_I16); >- stmt( IRStmt_Put( integerGuestReg64Offset(regno), >- unop(Iop_16Uto64,e) ) ); >-} >- > static const HChar* nameIReg16 ( UInt regno ) > { > return nameIReg( 2, regno, False ); >@@ -8485,22 +8478,12 @@ void codegen_xchg_rAX_Reg ( Prefix pfx, Int sz, UInt regLo3 ) > IRTemp t2 = newTemp(ty); > vassert(sz == 2 || sz == 4 || sz == 8); > vassert(regLo3 < 8); >- if (sz == 8) { >- assign( t1, getIReg64(R_RAX) ); >- assign( t2, getIRegRexB(8, pfx, regLo3) ); >- putIReg64( R_RAX, mkexpr(t2) ); >- putIRegRexB(8, pfx, regLo3, mkexpr(t1) ); >- } else if (sz == 4) { >- assign( t1, getIReg32(R_RAX) ); >- assign( t2, getIRegRexB(4, pfx, regLo3) ); >- putIReg32( R_RAX, mkexpr(t2) ); >- putIRegRexB(4, pfx, regLo3, mkexpr(t1) ); >- } else { >- assign( t1, getIReg16(R_RAX) ); >- assign( t2, getIRegRexB(2, pfx, regLo3) ); >- putIReg16( R_RAX, mkexpr(t2) ); >- putIRegRexB(2, pfx, regLo3, mkexpr(t1) ); >- } >+ >+ assign( t1, getIRegRAX(sz) ); >+ assign( t2, getIRegRexB(sz, pfx, regLo3) ); >+ putIRegRAX( sz, mkexpr(t2) ); >+ putIRegRexB( sz, pfx, regLo3, mkexpr(t1) ); >+ > DIP("xchg%c %s, %s\n", > nameISize(sz), nameIRegRAX(sz), > nameIRegRexB(sz,pfx, regLo3)); >-- >2.43.0 >
The above commented patch revision fixes a typo in the commit message (`unifify` -> `unify`). Unfortunately it appears I cannot edit the comment to fix formatting.
Created attachment 173545 [details] Patch