Changeset 179888 in webkit

Timestamp:

Feb 10, 2015, 3:24:24 PM (12 years ago)

Author:

bshafiei@apple.com

Message:

Merged r176083. rdar://problem/19670901

Location:

branches/safari-600.1.4.15-branch/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• assembler/ARMv7Assembler.h (modified) (1 diff)
• disassembler/ARMv7/ARMv7DOpcode.cpp (modified) (2 diffs)
• disassembler/ARMv7/ARMv7DOpcode.h (modified) (3 diffs)

branches/safari-600.1.4.15-branch/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-02-10  Babak Shafiei  <bshafiei@apple.com>
+
+        Merge r176083.
+
+    2014-11-13  Benjamin Poulain  <benjamin@webkit.org>
+
+            ARMv7(s) Assembler: LDRH with immediate offset is loading from the wrong offset
+            https://bugs.webkit.org/show_bug.cgi?id=136914
+
+            Reviewed by Michael Saboff.
+
+            TLDR: the immediate offset of half-word load was divided by 2.
+
+            Story time: So I started getting those weird reports of :nth-child() behaving bizarrely
+            on ARMv7 and ARMv7s. To make things worse, the behavior changes depending on style updates.
+
+            I started looking the disassembly on the tests cases...
+
+            The first thing I noticed was that the computation of An+B looked wrong. For example,
+            in the case of n+6, the instruction should have been:
+                subs r1, r1, #6
+            but was
+                subs r1, r1, #2
+
+            After spending a lot of time trying to find the error in the assembler, I discovered
+            the problem was not real, but just a bug in the disassembler.
+            This is the first fix: ARMv7DOpcodeAddSubtractImmediate3's immediate3() was truncating
+            the value to 2 bits instead of 3 bits.
+
+            The disassembler being fixed, I still have no lead on the weird bug. Some disassembly later,
+            I realize the LDRH instruction is not decoded at all. The reason is that both LDRH and STRH
+            were under the umbrella ARMv7DOpcodeLoadStoreRegisterImmediateHalfWord but the pattern
+            only matched SRTH.
+
+            I fix that next, ARMv7DOpcodeLoadStoreRegisterImmediateHalfWord is split into
+            ARMv7DOpcodeStoreRegisterImmediateHalfWord and ARMv7DOpcodeLoadRegisterImmediateHalfWord,
+            each with their own pattern and their instruction group.
+
+            Now that I can see the LDRHs correctly, there is something fishy about them, their offset
+            is way too small for the data I load.
+
+            This time, looking at the binary, the generated code is indeed incorrect. It turns out that
+            the ARMv7 assembler shifted the offset of half-word load as if they were byte load: divided by 4.
+            As a result, all the load of half-words with more than zero offset were loading
+            values with a smaller offset than what they should have.
+
+            That being fixed, I dump the assembly: still wrong. I am ready to throw my keyboard through
+            my screen at that point.
+
+            Looking at the disassembler, there is yet again a bug. The computation of the scale() adjustment
+            of the offset was incorrect for anything but word loads.
+            I replaced it by a switch-case to make it explicit.
+
+            STRH is likely incorrect too. I'll fix that in a follow up, I want to survey all the 16 bits cases
+            that are not directly used by the CSS JIT.
+
+            * assembler/ARMv7Assembler.h:
+            (JSC::ARMv7Assembler::ldrh):
+            Fix the immediate scaling. Add an assertion to make sure the alignment of the input is correct.
+
+            * disassembler/ARMv7/ARMv7DOpcode.cpp:
+            (JSC::ARMv7Disassembler::ARMv7DOpcodeLoadStoreRegisterImmediate::scale):
+            Fix the scaling code. Just hardcode instruction-to-scale table.
+
+            * disassembler/ARMv7/ARMv7DOpcode.h:
+            (JSC::ARMv7Disassembler::ARMv7DOpcodeAddSubtractImmediate3::immediate3):
+            The mask for a 3 bits immediate is not 3 :)
+
+            (JSC::ARMv7Disassembler::ARMv7DOpcodeLoadStoreRegisterImmediate::scale): Deleted.
+
 2015-02-05  Lucas Forschler  <lforschler@apple.com>
 

branches/safari-600.1.4.15-branch/Source/JavaScriptCore/assembler/ARMv7Assembler.h

@@ -1195 +1195 @@
         ASSERT(rn != ARMRegisters::pc); // LDR (literal)
         ASSERT(imm.isUInt12());
+        ASSERT(!(imm.isUInt12() & 1));
 
         if (!((rt | rn) & 8) && imm.isUInt6())
-            m_formatter.oneWordOp5Imm5Reg3Reg3(OP_LDRH_imm_T1, imm.getUInt6() >> 2, rn, rt);
+            m_formatter.oneWordOp5Imm5Reg3Reg3(OP_LDRH_imm_T1, imm.getUInt6() >> 1, rn, rt);
         else
             m_formatter.twoWordOp12Reg4Reg4Imm12(OP_LDRH_imm_T2, rn, rt, imm.getUInt12());

branches/safari-600.1.4.15-branch/Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp

@@ -92 +92 @@
     OPCODE_GROUP_ENTRY(0xe, ARMv7DOpcodeLoadStoreRegisterImmediateWordAndByte),
     OPCODE_GROUP_ENTRY(0xf, ARMv7DOpcodeLoadStoreRegisterImmediateWordAndByte),
-    OPCODE_GROUP_ENTRY(0x10, ARMv7DOpcodeLoadStoreRegisterImmediateHalfWord),
-    OPCODE_GROUP_ENTRY(0x11, ARMv7DOpcodeLoadStoreRegisterImmediateHalfWord),
+    OPCODE_GROUP_ENTRY(0x10, ARMv7DOpcodeStoreRegisterImmediateHalfWord),
+    OPCODE_GROUP_ENTRY(0x11, ARMv7DOpcodeLoadRegisterImmediateHalfWord),
     OPCODE_GROUP_ENTRY(0x12, ARMv7DOpcodeLoadStoreRegisterSPRelative),
     OPCODE_GROUP_ENTRY(0x13, ARMv7DOpcodeLoadStoreRegisterSPRelative),
@@ -513 +513 @@
 
     return m_formatBuffer;
+}
+
+unsigned ARMv7DOpcodeLoadStoreRegisterImmediate::scale()
+{
+    switch (op()) {
+    case 0:
+    case 1:
+        return 2;
+    case 2:
+    case 3:
+        return 0;
+    case 4:
+    case 5:
+        return 1;
+    default:
+        break;
+    }
+    ASSERT_NOT_REACHED();
+    return 0;
 }
 

branches/safari-600.1.4.15-branch/Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.h

@@ -276 +276 @@
 
     unsigned op() { return (m_opcode >> 9) & 0x1; }
-    unsigned immediate3() { return (m_opcode >> 6) & 0x3; }
+    unsigned immediate3() { return (m_opcode >> 6) & 0x7; }
     unsigned rn() { return (m_opcode >> 3) & 0x7; }
 };
@@ -442 +442 @@
     unsigned rn() { return (m_opcode >> 3) & 0x7; }
     unsigned rt() { return m_opcode & 0x7; }
-    unsigned scale() { return 2 - (op() >> 1); }
+    unsigned scale();
 };
 
@@ -453 +453 @@
 };
 
-class ARMv7DOpcodeLoadStoreRegisterImmediateHalfWord : public ARMv7DOpcodeLoadStoreRegisterImmediate {
+class ARMv7DOpcodeStoreRegisterImmediateHalfWord : public ARMv7DOpcodeLoadStoreRegisterImmediate {
 public:
     static const uint16_t s_mask = 0xf800;
     static const uint16_t s_pattern = 0x8000;
+
+    DEFINE_STATIC_FORMAT16(ARMv7DOpcodeLoadStoreRegisterImmediate, thisObj);
+};
+
+class ARMv7DOpcodeLoadRegisterImmediateHalfWord : public ARMv7DOpcodeLoadStoreRegisterImmediate {
+public:
+    static const uint16_t s_mask = 0xf800;
+    static const uint16_t s_pattern = 0x8800;
 
     DEFINE_STATIC_FORMAT16(ARMv7DOpcodeLoadStoreRegisterImmediate, thisObj);