Merge "Fix off-by-one errors in limit checking for ldr/str instructions."

3 files changed, 64 insertions, 9 deletions

diff --git a/compiler/utils/arm/assembler_thumb2.cc b/compiler/utils/arm/assembler_thumb2.cc
index 30aa625..604f59e 100644
--- a/compiler/utils/arm/assembler_thumb2.cc
+++ b/compiler/utils/arm/assembler_thumb2.cc
@@ -1260,23 +1260,23 @@ void Thumb2Assembler::EmitLoadStore(Condition cond,
     int32_t offset = ad.GetOffset();
 
     // The 16 bit SP relative instruction can only have a 10 bit offset.
-    if (rn == SP && offset > 1024) {
+    if (rn == SP && offset >= (1 << 10)) {
       must_be_32bit = true;
     }
 
     if (byte) {
       // 5 bit offset, no shift.
-      if (offset > 32) {
+      if (offset >= (1 << 5)) {
         must_be_32bit = true;
       }
     } else if (half) {
       // 6 bit offset, shifted by 1.
-      if (offset > 64) {
+      if (offset >= (1 << 6)) {
         must_be_32bit = true;
       }
     } else {
       // 7 bit offset, shifted by 2.
-      if (offset > 128) {
+      if (offset >= (1 << 7)) {
         must_be_32bit = true;
       }
     }
@@ -1312,7 +1312,7 @@ void Thumb2Assembler::EmitLoadStore(Condition cond,
       CHECK_GE(offset, 0);
       if (sp_relative) {
         // SP relative, 10 bit offset.
-        CHECK_LT(offset, 1024);
+        CHECK_LT(offset, (1 << 10));
         CHECK_EQ((offset & 0b11), 0);
         encoding |= rd << 8 | offset >> 2;
       } else {
@@ -1322,15 +1322,15 @@ void Thumb2Assembler::EmitLoadStore(Condition cond,
 
         if (byte) {
           // 5 bit offset, no shift.
-          CHECK_LT(offset, 32);
+          CHECK_LT(offset, (1 << 5));
         } else if (half) {
           // 6 bit offset, shifted by 1.
-          CHECK_LT(offset, 64);
+          CHECK_LT(offset, (1 << 6));
           CHECK_EQ((offset & 0b1), 0);
           offset >>= 1;
         } else {
           // 7 bit offset, shifted by 2.
-          CHECK_LT(offset, 128);
+          CHECK_LT(offset, (1 << 7));
           CHECK_EQ((offset & 0b11), 0);
           offset >>= 2;
         }
@@ -1344,7 +1344,7 @@ void Thumb2Assembler::EmitLoadStore(Condition cond,
     if (ad.GetRegister() == PC) {
        // PC relative literal encoding.
       int32_t offset = ad.GetOffset();
-      if (must_be_32bit || offset < 0 || offset > (1 << 10) || !load) {
+      if (must_be_32bit || offset < 0 || offset >= (1 << 10) || !load) {
         int32_t up = B23;
         if (offset < 0) {
           offset = -offset;
diff --git a/compiler/utils/assembler_thumb_test.cc b/compiler/utils/assembler_thumb_test.cc
index 1e3e569..68cb656 100644
--- a/compiler/utils/assembler_thumb_test.cc
+++ b/compiler/utils/assembler_thumb_test.cc
@@ -1358,6 +1358,41 @@ TEST(Thumb2AssemblerTest, LoadStoreLiteral) {
   delete assembler;
 }
 
+TEST(Thumb2AssemblerTest, LoadStoreLimits) {
+  arm::Thumb2Assembler* assembler = static_cast<arm::Thumb2Assembler*>(Assembler::Create(kThumb2));
+
+  __ ldr(R0, Address(R4, 124));     // 16 bit.
+  __ ldr(R0, Address(R4, 128));     // 32 bit.
+
+  __ ldrb(R0, Address(R4, 31));     // 16 bit.
+  __ ldrb(R0, Address(R4, 32));     // 32 bit.
+
+  __ ldrh(R0, Address(R4, 62));     // 16 bit.
+  __ ldrh(R0, Address(R4, 64));     // 32 bit.
+
+  __ ldrsb(R0, Address(R4, 31));     // 32 bit.
+  __ ldrsb(R0, Address(R4, 32));     // 32 bit.
+
+  __ ldrsh(R0, Address(R4, 62));     // 32 bit.
+  __ ldrsh(R0, Address(R4, 64));     // 32 bit.
+
+  __ str(R0, Address(R4, 124));     // 16 bit.
+  __ str(R0, Address(R4, 128));     // 32 bit.
+
+  __ strb(R0, Address(R4, 31));     // 16 bit.
+  __ strb(R0, Address(R4, 32));     // 32 bit.
+
+  __ strh(R0, Address(R4, 62));     // 16 bit.
+  __ strh(R0, Address(R4, 64));     // 32 bit.
+
+  size_t cs = __ CodeSize();
+  std::vector<uint8_t> managed_code(cs);
+  MemoryRegion code(&managed_code[0], managed_code.size());
+  __ FinalizeInstructions(code);
+  dump(managed_code, "LoadStoreLimits");
+  delete assembler;
+}
+
 #undef __
 }  // namespace arm
 }  // namespace art
diff --git a/compiler/utils/assembler_thumb_test_expected.cc.inc b/compiler/utils/assembler_thumb_test_expected.cc.inc
index c2e7fe8..3943e37 100644
--- a/compiler/utils/assembler_thumb_test_expected.cc.inc
+++ b/compiler/utils/assembler_thumb_test_expected.cc.inc
@@ -4799,6 +4799,25 @@ const char* LoadStoreLiteralResults[] = {
   "  18:   f8cf 07ff       str.w   r0, [pc, #2047] ; 81b <LoadStoreLiteral+0x81b>\n",
   nullptr
 };
+const char* LoadStoreLimitsResults[] = {
+  "   0:   6fe0            ldr     r0, [r4, #124]  ; 0x7c\n",
+  "   2:   f8d4 0080       ldr.w   r0, [r4, #128]  ; 0x80\n",
+  "   6:   7fe0            ldrb    r0, [r4, #31]\n",
+  "   8:   f894 0020       ldrb.w  r0, [r4, #32]\n",
+  "   c:   8fe0            ldrh    r0, [r4, #62]   ; 0x3e\n",
+  "   e:   f8b4 0040       ldrh.w  r0, [r4, #64]   ; 0x40\n",
+  "  12:   f994 001f       ldrsb.w r0, [r4, #31]\n",
+  "  16:   f994 0020       ldrsb.w r0, [r4, #32]\n",
+  "  1a:   f9b4 003e       ldrsh.w r0, [r4, #62]   ; 0x3e\n",
+  "  1e:   f9b4 0040       ldrsh.w r0, [r4, #64]   ; 0x40\n",
+  "  22:   67e0            str     r0, [r4, #124]  ; 0x7c\n",
+  "  24:   f8c4 0080       str.w   r0, [r4, #128]  ; 0x80\n",
+  "  28:   77e0            strb    r0, [r4, #31]\n",
+  "  2a:   f884 0020       strb.w  r0, [r4, #32]\n",
+  "  2e:   87e0            strh    r0, [r4, #62]   ; 0x3e\n",
+  "  30:   f8a4 0040       strh.w  r0, [r4, #64]   ; 0x40\n",
+  nullptr
+};
 std::map<std::string, const char**> test_results;
 void setup_results() {
     test_results["SimpleMov"] = SimpleMovResults;
@@ -4845,4 +4864,5 @@ void setup_results() {
     test_results["Shifts"] = ShiftsResults;
     test_results["LoadStoreRegOffset"] = LoadStoreRegOffsetResults;
     test_results["LoadStoreLiteral"] = LoadStoreLiteralResults;
+    test_results["LoadStoreLimits"] = LoadStoreLimitsResults;
 }