Summary: | New IBM POWER7 xsnmaddadp instruction causes memcheck to fail on 32bit app | ||
---|---|---|---|
Product: | [Developer tools] valgrind | Reporter: | Maynard Johnson <maynardj> |
Component: | vex | Assignee: | Julian Seward <jseward> |
Status: | RESOLVED FIXED | ||
Severity: | normal | ||
Priority: | NOR | ||
Version: | 3.7 SVN | ||
Target Milestone: | --- | ||
Platform: | Unlisted Binaries | ||
OS: | Linux | ||
Latest Commit: | Version Fixed In: | ||
Attachments: |
Patch to fix this bug
Version 2 of the patch to fix this bug Corrected patch to fix this bug |
Description
Maynard Johnson
2011-04-13 18:45:32 UTC
Created attachment 59142 [details]
Version 2 of the patch to fix this bug
(In reply to comment #1) > Created an attachment (id=59142) [details] That looks like a version of the patch for bug 270851. For sure it doesn't look anything like the patch you attached in comment #0. (In reply to comment #2) > (In reply to comment #1) > > Created an attachment (id=59142) [details] [details] > > That looks like a version of the patch for bug 270851. For sure > it doesn't look anything like the patch you attached in > comment #0. You're right. See my comment in 270851 -- https://bugs.kde.org/show_bug.cgi?id=270851#c5. I mistakenly attached the patch to the wrong bug. I'll make this second attachment obsolete. Attachment #1 [details] is still the patch that fixes the problem reported in this bug. Patch generates Pun_NEG, not Pun_NOT. Did you test it? (In reply to comment #4) > Patch generates Pun_NEG, not Pun_NOT. Did you test it? Bone-headed copy/paste error. Sorry. Yes, I tested it using memcheck, and it did get rid of the error, but evidently that was not a sufficient test. No ppc64 testcases use that code, AFAICS. There are some insns that are 64-bit category only which use Iop_Not64, but nothing where that Iop would be used in 32-bit mode. I'll attach an updated patch. Created attachment 59401 [details]
Corrected patch to fix this bug
Here's the corrected patch. I tested this by adding a temporary ugly hack into guest_ppc_toIR.c (in dis_int_logic) to force the implementation for 'andc' to use Iop_Not64. Then I ran the 32-bit 'jm-insns -i' testcase (from none/test/ppc32) which has a test for 'andc', and I visually inspected the results of this hacked-up andc to make sure the Iop_Not64 did what I expect. The test hack is below:
------------------
case 0x03C: // andc (AND with Complement, PPC32 p357)
DIP("andc%s r%u,r%u,r%u\n",
flag_rC ? ".":"", rA_addr, rS_addr, rB_addr);
if (rA_addr != 17) {
assign(rA, binop( mkSzOp(ty, Iop_And8), mkexpr(rS),
unop( mkSzOp(ty, Iop_Not8), mkexpr(rB))));
} else {
IRTemp t1 = newTemp(Ity_I32);
assign(t1, unop(Iop_64to32, unop(Iop_Not64,
mkU64(0x5555333377771111ULL))));
assign(rA, mkexpr(t1));
}
break;
-----------------------------------
As you can see, I only do the Iop_Not64 stuff when the RA reg number is 17, because that's the output reg number used in the jm-insns testcase. Since the end result is putting the NOT of 0x77771111 into RA, what I expect printed out by the testcase is 0x8888EEEE, and that's what I see. So I'm confident that the attached patch is good. Thanks.
Committed, r2136, w/minor renaming. I also added an assert to check that Iop_Not64 is never handled by iselWordExpr_R when we're generating 32 bit code. That's untested though, so watch out for any failures (I'm pretty sure it's safe). (In reply to comment #6) > by the testcase is 0x8888EEEE, and that's what I see. So I'm confident that > the attached patch is good. Thanks. Thanks for the extra testing -- QA, we like QA :-) In hindsight, generating NEG rather than NOT probably appeared to work most of the time because NEG mostly will simply invert bits, just like NOT, since NEG(x) == 1 + NOT(x). Or something like that. I verified the fix in SVN head. Thanks! |