Changeset 291339 in webkit for trunk/Source/JavaScriptCore/assembler

Timestamp:

Mar 16, 2022, 5:05:08 AM (3 years ago)

Author:

Angelos Oikonomopoulos

Message:

MacroAssemblerARMv7: Be friendlier to DisallowMacroScratchRegisterUsage
​https://bugs.webkit.org/show_bug.cgi?id=237888

Reviewed by Žan Doberšek.

Only check that we're allowed to use the scratch register at sites
where we're using it implicitly. When it's explicitly passed in by the
caller, use invalidateCachedAddressTempRegister to invalidate it
without asserting anything about m_allowScratchRegister.

Since helpers can explictly make use of addressTempRegister, an
argument can be made that this is still fragile (i.e. future changes
could run into this). The alternative would be to have the topmost caller
do fine-grained management of DisallowMacroScratchRegisterUsage,
allowing it around explicit calls to MacroAssemblerARMv7 with
scratchRegister() in the arguments and disallowing it for helpers.

As there are currently no helpers that would trip this, this patch opts
for the former approach, to make DisallowMacroScratchRegisterUsage
easier to work with (there'll be more usage of the API in an upcoming
wasm32 patch).

• assembler/MacroAssemblerARMv7.h:

(JSC::MacroAssemblerARMv7::scratchRegister):
(JSC::MacroAssemblerARMv7::load32):
(JSC::MacroAssemblerARMv7::load16):
(JSC::MacroAssemblerARMv7::load16SignedExtendTo32):
(JSC::MacroAssemblerARMv7::load8):
(JSC::MacroAssemblerARMv7::load8SignedExtendTo32):
(JSC::MacroAssemblerARMv7::loadPair32):
(JSC::MacroAssemblerARMv7::move):
(JSC::MacroAssemblerARMv7::farJump):
(JSC::MacroAssemblerARMv7::setupArmAddress):
(JSC::MacroAssemblerARMv7::invalidateCachedAddressTempRegister):

• bytecode/CallLinkInfo.cpp:

(JSC::CallLinkInfo::emitFastPathImpl):

• jit/BaselineJITRegisters.h:

File:

• trunk/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h (modified) (12 diffs)

trunk/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h

@@ -79 +79 @@
     static constexpr unsigned numFPRs = std::initializer_list<int>({ FOR_EACH_FP_REGISTER(DUMMY_REGISTER_VALUE) }).size();
 #undef DUMMY_REGISTER_VALUE
-    static constexpr RegisterID scratchRegister() { return addressTempRegister; }
+    static constexpr RegisterID s_scratchRegister = addressTempRegister;
+    RegisterID scratchRegister()
+    {
+        RELEASE_ASSERT(m_allowScratchRegister);
+        return s_scratchRegister;
+    }
 
     MacroAssemblerARMv7()
@@ -680 +685 @@
     {
         if (dest == addressTempRegister)
-            cachedAddressTempRegister().invalidate();
+            invalidateCachedAddressTempRegister();
         else if (dest == dataTempRegister)
             cachedDataTempRegister().invalidate();
@@ -699 +704 @@
     {
         if (dest == addressTempRegister)
-            cachedAddressTempRegister().invalidate();
+            invalidateCachedAddressTempRegister();
         else if (dest == dataTempRegister)
             cachedDataTempRegister().invalidate();
@@ -719 +724 @@
         ASSERT(address.type == ArmAddress::HasIndex);
         if (dest == addressTempRegister)
-            cachedAddressTempRegister().invalidate();
+            invalidateCachedAddressTempRegister();
         else if (dest == dataTempRegister)
             cachedDataTempRegister().invalidate();
@@ -729 +734 @@
     {
         if (dest == addressTempRegister)
-            cachedAddressTempRegister().invalidate();
+            invalidateCachedAddressTempRegister();
         else if (dest == dataTempRegister)
             cachedDataTempRegister().invalidate();
@@ -749 +754 @@
         ASSERT(address.type == ArmAddress::HasIndex);
         if (dest == addressTempRegister)
-            cachedAddressTempRegister().invalidate();
+            invalidateCachedAddressTempRegister();
         else if (dest == dataTempRegister)
             cachedDataTempRegister().invalidate();
@@ -934 +939 @@
             if (!(absOffset & ~0x3fc)) {
                 if ((dest1 == addressTempRegister) || (dest2 == addressTempRegister))
-                    cachedAddressTempRegister().invalidate();
+                    invalidateCachedAddressTempRegister();
                 if ((dest1 == dataTempRegister) || (dest2 == dataTempRegister))
                     cachedDataTempRegister().invalidate();
@@ -1666 +1671 @@
             cachedDataTempRegister().invalidate();
         else if (dest == addressTempRegister)
-            cachedAddressTempRegister().invalidate();
+            invalidateCachedAddressTempRegister();
     }
 
@@ -2055 +2060 @@
     {
         cachedDataTempRegister().invalidate();
-        cachedAddressTempRegister().invalidate();
+        invalidateCachedAddressTempRegister();
         m_assembler.bx(target);
     }
@@ -2063 +2068 @@
         move(target, addressTempRegister);
         cachedDataTempRegister().invalidate();
-        cachedAddressTempRegister().invalidate();
+        invalidateCachedAddressTempRegister();
         m_assembler.bx(addressTempRegister);
     }
@@ -2530 +2535 @@
             } else {
                 move(TrustedImm32(address.offset), addressTempRegister);
+                m_assembler.add(addressTempRegister, addressTempRegister, address.base);
                 cachedAddressTempRegister().invalidate();
-                m_assembler.add(addressTempRegister, addressTempRegister, address.base);
             }
 
@@ -2621 +2626 @@
     }
 
+    ALWAYS_INLINE void invalidateCachedAddressTempRegister()
+    {
+        // This function is intended for when we are explicitly using
+        // addressTempRegister (because the caller supplied it), so it can
+        // ignore m_allowScratchRegister.
+        m_cachedAddressTempRegister.invalidate();
+    }
+
     ALWAYS_INLINE CachedTempRegister& cachedAddressTempRegister()
     {