|
From: <sv...@va...> - 2011-10-26 15:11:10
|
Author: sewardj
Date: 2011-10-26 16:06:25 +0100 (Wed, 26 Oct 2011)
New Revision: 2227
Log:
Handle "add.w reg, sp, #constT" and "addw reg, sp, #uimm12" for reg !=
PC. Previous handling was overly restrictive -- only allowed
reg == SP. (n-i-bz)
Modified:
trunk/priv/guest_arm_toIR.c
Modified: trunk/priv/guest_arm_toIR.c
===================================================================
--- trunk/priv/guest_arm_toIR.c 2011-10-25 09:10:58 UTC (rev 2226)
+++ trunk/priv/guest_arm_toIR.c 2011-10-26 15:06:25 UTC (rev 2227)
@@ -16077,8 +16077,8 @@
UInt rN = INSN0(3,0);
UInt rD = INSN1(11,8);
Bool valid = !isBadRegT(rN) && !isBadRegT(rD);
- /* but allow "add.w reg, sp, #constT" */
- if (!valid && rN == 13 && rD != 15)
+ /* but allow "add.w reg, sp, #constT" for reg != PC */
+ if (!valid && rD <= 14 && rN == 13)
valid = True;
if (valid) {
IRTemp argL = newTemp(Ity_I32);
@@ -16104,8 +16104,8 @@
UInt rN = INSN0(3,0);
UInt rD = INSN1(11,8);
Bool valid = !isBadRegT(rN) && !isBadRegT(rD);
- /* but allow "addw sp, sp, #uimm12" */
- if (!valid && rD == 13 && rN == 13)
+ /* but allow "addw reg, sp, #uimm12" for reg != PC */
+ if (!valid && rD <= 14 && rN == 13)
valid = True;
if (valid) {
IRTemp argL = newTemp(Ity_I32);
|
|
From: Stephen M. <sm...@al...> - 2011-10-27 21:38:05
|
>>>>> "JS" == svn <sv...@va...> writes:
JS> Author: sewardj
JS> Date: 2011-10-26 16:06:25 +0100 (Wed, 26 Oct 2011)
JS> New Revision: 2227
JS> Log:
JS> Handle "add.w reg, sp, #constT" and "addw reg, sp, #uimm12" for
JS> reg != PC. Previous handling was overly restrictive -- only
JS> allowed reg == SP. (n-i-bz)
Seeing this patch go by reminds me that I'd noticed a similar
seemingly incorrect set of constrains for the versions of the add.w
and sub.w instructions that take another register and an optional
shift rather than an immediate. I'm still working on an appropriate
test suite addition (it's slow going since I only have software
emulators).
If no-one besides me has run into it yet, I was guessing that my
change wasn't important enough to rush into 3.7, but I'm appending it
below in case anyone wants to take a look; it will be in Bugzilla in
the future.
-- Stephen
Index: priv/guest_arm_toIR.c
===================================================================
--- priv/guest_arm_toIR.c (revision 2228)
+++ priv/guest_arm_toIR.c (working copy)
@@ -16358,16 +16358,18 @@
UInt how = INSN1(5,4);
Bool valid = !isBadRegT(rD) && !isBadRegT(rN) && !isBadRegT(rM);
- /* but allow "add.w reg, sp, reg w/ no shift
+ /* but allow "add.w reg, sp, reg w/ lsl up to 3
(T3) "ADD (SP plus register) */
if (!valid && INSN0(8,5) == BITS4(1,0,0,0) // add
- && rD != 15 && rN == 13 && imm5 == 0 && how == 0) {
+ && rD != 15 && rN == 13 && rM != 13 && rM != 15
+ && imm5 <= 3 && how == 0) {
valid = True;
}
- /* also allow "sub.w reg, sp, reg w/ no shift
+ /* also allow "sub.w reg, sp, reg w/ lsl up to 3
(T1) "SUB (SP minus register) */
if (!valid && INSN0(8,5) == BITS4(1,1,0,1) // sub
- && rD != 15 && rN == 13 && imm5 == 0 && how == 0) {
+ && rD != 15 && rN == 13 && rM != 13 && rM != 15
+ && imm5 <= 3 && how == 0) {
valid = True;
}
if (valid) {
|
|
From: Julian S. <js...@ac...> - 2011-10-28 02:35:40
|
Stephen, > Seeing this patch go by reminds me that I'd noticed a similar > seemingly incorrect set of constrains for the versions of the add.w > and sub.w instructions that take another register and an optional > shift rather than an immediate. I'm still working on an appropriate > test suite addition (it's slow going since I only have software > emulators). > > If no-one besides me has run into it yet, I was guessing that my > change wasn't important enough to rush into 3.7, but I'm appending it > below in case anyone wants to take a look; it will be in Bugzilla in > the future. Um, I wasn't aware this patch existed. What bugzilla number tracks this? My (permanent) concern is adding support for instructions without test cases, since it's exceedingly difficult to track down subtle errors without test cases. See https://bugs.kde.org/show_bug.cgi?id=245925#c3 for a sorry tale. So I've taken to fixing any such case that turns up in the wild, or has a regtest to exercise it. Anyway .. if you have a patch + test case then I will be very happy to push that into trunk, + into 3_7_BRANCH if anybody reports it as a failure in practice. J |
|
From: Stephen M. <sm...@al...> - 2011-10-28 23:48:24
|
>>>>> "JS" == Julian Seward <js...@ac...> writes: JS> Stephen, SMcC> Seeing this patch go by reminds me that I'd noticed a similar SMcC> seemingly incorrect set of constrains for the versions of the SMcC> add.w and sub.w instructions that take another register and an SMcC> optional shift rather than an immediate. I'm still working on an SMcC> appropriate test suite addition (it's slow going since I only SMcC> have software emulators). SMcC> If no-one besides me has run into it yet, I was guessing that my SMcC> change wasn't important enough to rush into 3.7, but I'm SMcC> appending it below in case anyone wants to take a look; it will SMcC> be in Bugzilla in the future. JS> Um, I wasn't aware this patch existed. That's to be expected, since this is the first time I've mentioned it. JS> What bugzilla number tracks this? I just created bug #285219 for it. JS> My (permanent) concern is adding support for instructions without JS> test cases, since it's exceedingly difficult to track down subtle JS> errors without test cases. See JS> https://bugs.kde.org/show_bug.cgi?id=245925#c3 for a sorry tale. JS> So I've taken to fixing any such case that turns up in the wild, JS> or has a regtest to exercise it. JS> Anyway .. if you have a patch + test case then I will be very JS> happy to push that into trunk, + into 3_7_BRANCH if anybody JS> reports it as a failure in practice. That all seems like a perfectly reasonable policy. Given this, and since I tend to run into issues with VEX when using it in non-standard ways, my usual approach has been to sit on patches until I have time to create a good regression test suite patch for them. That has been slow going here because this is the first ARM issue I've run into, so I'm running the ARM test suite for the first time, and finding that it has a lot of failures (some of which may be caused by running under a bleeding-edge QEMU rather than on real hardware). In Bugzilla, I've included details showing that such instructions appear statically in a recent Ubuntu libc package, and giving a small test case program to reproduce the failure. From the fact that it appears in libc, I would guess that GCC can generate this instruction, though I wasn't immediately able to see how to trigger that in my test case. Ideally it looks like this instruction should go in none/tests/arm/v6intThumb.c, but I'm reluctant to patch that file when the current version isn't passing on my emulator. (Which is a problem of its own, though whether in Valgrind or the emulator I don't yet know.) -- Stephen |