The IBM ISA 3.0 has been released. This bugzilla is the first in a series of five for adding the needed ISA 3.0 support to Valgrind. Reproducible: Always
Created attachment 97546 [details] patch 1 of 5 to add ISA 3.0 functionality to Valgrind This is the first patch in a series of five for adding the ISA 3.0 instruction support to valgrind.
Created attachment 97547 [details] patch 1 of 5 to add ISA 3.0 instruction testing to the Valgrding test suite This is the first of 5 patches for the Power ISA 3.0 instruction testing. It tests the instructions added by the corresponding patch that adds ISA 3.0 instruction support to Valgrind.
Adding Will to CC list
(In reply to Carl Love from comment #1) > Created attachment 97546 [details] > patch 1 of 5 to add ISA 3.0 functionality to Valgrind > > This is the first patch in a series of five for adding the ISA 3.0 > instruction support to valgrind. This looks fine. I have only a couple of minor comments: @@ -833,7 +833,8 @@ VexGuestLayout /* 7 */ ALWAYSDEFD32(guest_NRADDR_GPR2), /* 8 */ ALWAYSDEFD32(guest_REDIR_SP), /* 9 */ ALWAYSDEFD32(guest_REDIR_STACK), - /* 10 */ ALWAYSDEFD32(guest_IP_AT_SYSCALL) + /* 10 */ ALWAYSDEFD32(guest_IP_AT_SYSCALL), + /* 11 */ ALWAYSDEFD32(guest_FPCC) } }; You need to change .n_alwaysDefd from 11 to 12, else this won't have any effect. @@ -874,7 +875,8 @@ VexGuestLayout /* 7 */ ALWAYSDEFD64(guest_NRADDR_GPR2), /* 8 */ ALWAYSDEFD64(guest_REDIR_SP), /* 9 */ ALWAYSDEFD64(guest_REDIR_STACK), - /* 10 */ ALWAYSDEFD64(guest_IP_AT_SYSCALL) + /* 10 */ ALWAYSDEFD64(guest_IP_AT_SYSCALL), + /* 11 */ ALWAYSDEFD64(guest_FPCC) } }; Same here. @@ -2785,15 +2786,16 @@ static IRExpr* /* ::Ity_I32 */ getGST_masked ( PPC_GST reg, UInt mask ) - if (mask & MASK_FPSCR_RN) { + if ( mask == MASK_FPSCR_RN ) { assign( val, unop( Iop_8Uto32, IRExpr_Get( OFFB_FPROUND, Ity_I8 ) ) ); Just a sanity check (I don't really follow the logic here, but ..) did you really mean to change the guarding condition from "mask has MASK_FPSCR_RN set" to "mask is exactly equal to MASK_FPSCR_RN" ?
(In reply to Carl Love from comment #2) > Created attachment 97547 [details] > patch 1 of 5 to add ISA 3.0 instruction testing to the Valgrding test suite This also looks fine, with just the following small change: + * This testfile contains tests for the ISA 3.0 instructions. + * The framework of this test file was based on the framework + * of the jm-insns.c testfile. Please add ", whose original author was Jocelyn Mayer."
Ok to land once comments 4 and 5 are addressed.
> You need to change .n_alwaysDefd from 11 to 12, else this won't have any effect. Fixed for the 32-bit and 64-bit cases. Missed that, thanks for catching it. >- if (mask & MASK_FPSCR_RN) { >+ if ( mask == MASK_FPSCR_RN ) { Not sure why that changed, I think it was OK before. Removed the change. > Please add ", whose original author was Jocelyn Mayer." Done. Will re-base patch on latest upstream. I will retest on all of the various Power 7, 8 BE and LE platforms before committing. Thanks for the review.
Patches committed: Valgrind support: VEX commit 3214 valgrind commit 15837 Test suite support valgrind commit 15838
Created attachment 98158 [details] Fix issues caught by the nightly regression tests, compiler warning, broken testcase Fix memcheck test memcheck/tests/origin2-not-quite Remove allow_isa_3_0, from patch to fix compiler warnings allow_isa_3_0 is not need in the first patch. Move declaration to subsequent patch were it is used.
The fixes from the nightly regression tests, see patch 3, were committed in VEX commit 3215 Valgrind commit 15841 These fixed the compiler warnings and the failure of test memcheck/tests/origin2-not-quite
Created attachment 100641 [details] regression test fixes for 32-bit mode Patches applied Valgrind commit 15938, VEX commit 3244. A few fixes for the 32-bit mode were added to the valgrind patch, see patch5-fix.diff, as part of the VEX commit 3244. Fix for instruction xscvdphp VEX commit 3245. The call to generate_store_FPRF( Ity_I32, value ); was changed to generate_store_FPRF( Ity_I16, value );