Changeset 176083 in webkit

Timestamp:

Nov 13, 2014 12:43:32 PM (9 years ago)

Author:

benjamin@webkit.org

Message:

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.

Location:

trunk/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)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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.
+
 2014-11-13  Andreas Kling  <akling@apple.com>
 

trunk/Source/JavaScriptCore/assembler/ARMv7Assembler.h

@@ -1156 +1156 @@
         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());

trunk/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),
@@ -525 +525 @@
 }
 
+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;
+}
+
 const char* const ARMv7DOpcodeLoadStoreRegisterOffsetT1::s_opNames[8] = {
     "str", "strh", "strb", "ldrsb", "ldr", "ldrh", "ldrb", "ldrsh"

trunk/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);