Attachment 339423 Details for Bug 185231 – Patch

[patch] Patch

185231.patch (text/plain), 8.10 KB, created by Michael Saboff on 2018-05-03 10:18:36 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Michael Saboff

Created: 2018-05-03 10:18:36 PDT

Size: 8.10 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 231310)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,24 @@
>+2018-05-03  Michael Saboff  <msaboff@apple.com>
>+
>+        WebContent crash loading page on seas.upenn.edu @ JavaScriptCore: vmEntryToJavaScript
>+        https://bugs.webkit.org/show_bug.cgi?id=185231
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We weren't clearing the scratch register cache when switching back and forth between 
>+        allowing scratch register usage.  Now we clear when we transition from disallow to
>+        allow.
>+
>+        Added a new Air regression test.
>+
>+        * assembler/AllowMacroScratchRegisterUsage.h:
>+        (JSC::AllowMacroScratchRegisterUsage::AllowMacroScratchRegisterUsage):
>+        * assembler/AllowMacroScratchRegisterUsageIf.h:
>+        (JSC::AllowMacroScratchRegisterUsageIf::AllowMacroScratchRegisterUsageIf):
>+        * assembler/DisallowMacroScratchRegisterUsage.h:
>+        (JSC::DisallowMacroScratchRegisterUsage::~DisallowMacroScratchRegisterUsage):
>+        * b3/air/testair.cpp:
>+
> 2018-05-03  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r231197.
>Index: Source/JavaScriptCore/assembler/AllowMacroScratchRegisterUsage.h
>===================================================================
>--- Source/JavaScriptCore/assembler/AllowMacroScratchRegisterUsage.h	(revision 231310)
>+++ Source/JavaScriptCore/assembler/AllowMacroScratchRegisterUsage.h	(working copy)
>@@ -37,6 +37,10 @@ public:
>         : m_masm(masm)
>         , m_oldValueOfAllowScratchRegister(masm.m_allowScratchRegister)
>     {
>+#if CPU(ARM64)
>+        if (!m_oldValueOfAllowScratchRegister)
>+            m_masm.invalidateAllTempRegisters();
>+#endif
>         masm.m_allowScratchRegister = true;
>     }
> 
>Index: Source/JavaScriptCore/assembler/AllowMacroScratchRegisterUsageIf.h
>===================================================================
>--- Source/JavaScriptCore/assembler/AllowMacroScratchRegisterUsageIf.h	(revision 231310)
>+++ Source/JavaScriptCore/assembler/AllowMacroScratchRegisterUsageIf.h	(working copy)
>@@ -38,8 +38,13 @@ public:
>         , m_allowIfTrue(allowIfTrue)
>         , m_oldValueOfAllowScratchRegister(masm.m_allowScratchRegister)
>     {
>-        if (m_allowIfTrue)
>+        if (m_allowIfTrue) {
>+#if CPU(ARM64)
>+            if (!m_oldValueOfAllowScratchRegister)
>+                m_masm.invalidateAllTempRegisters();
>+#endif
>             masm.m_allowScratchRegister = true;
>+        }
>     }
> 
>     ~AllowMacroScratchRegisterUsageIf()
>Index: Source/JavaScriptCore/assembler/DisallowMacroScratchRegisterUsage.h
>===================================================================
>--- Source/JavaScriptCore/assembler/DisallowMacroScratchRegisterUsage.h	(revision 231310)
>+++ Source/JavaScriptCore/assembler/DisallowMacroScratchRegisterUsage.h	(working copy)
>@@ -42,6 +42,10 @@ public:
> 
>     ~DisallowMacroScratchRegisterUsage()
>     {
>+#if CPU(ARM64)
>+        if (m_oldValueOfAllowScratchRegister)
>+            m_masm.invalidateAllTempRegisters();
>+#endif
>         m_masm.m_allowScratchRegister = m_oldValueOfAllowScratchRegister;
>     }
> 
>Index: Source/JavaScriptCore/b3/air/testair.cpp
>===================================================================
>--- Source/JavaScriptCore/b3/air/testair.cpp	(revision 231310)
>+++ Source/JavaScriptCore/b3/air/testair.cpp	(working copy)
>@@ -1834,6 +1834,93 @@ void testX86VMULSDBaseIndexNeedRex()
> }
> #endif // #if CPU(X86) || CPU(X86_64)
> 
>+#if CPU(ARM64)
>+void testInvalidateCachedTempRegisters()
>+{
>+    B3::Procedure proc;
>+    Code& code = proc.code();
>+    BasicBlock* root = code.addBlock();
>+
>+    int32_t things[4];
>+    things[0] = 0x12000000;
>+    things[1] = 0x340000;
>+    things[2] = 0x5600;
>+    things[3] = 0x78;
>+    Tmp base = code.newTmp(GP);
>+    GPRReg tmp = GPRInfo::regT1;
>+    proc.pinRegister(tmp);
>+
>+    root->append(Move, nullptr, Arg::bigImm(bitwise_cast<intptr_t>(&things)), base);
>+
>+    B3::BasicBlock* patchPoint1Root = proc.addBlock();
>+    B3::Air::Special* patchpointSpecial = code.addSpecial(std::make_unique<B3::PatchpointSpecial>());
>+
>+    // In Patchpoint, Load things[0] -> tmp. This will materialize the address in x17 (dataMemoryRegister).
>+    B3::PatchpointValue* patchpoint1 = patchPoint1Root->appendNew<B3::PatchpointValue>(proc, B3::Void, B3::Origin());
>+    patchpoint1->clobber(RegisterSet::macroScratchRegisters());
>+    patchpoint1->setGenerator(
>+        [=] (CCallHelpers& jit, const B3::StackmapGenerationParams&) {
>+            AllowMacroScratchRegisterUsage allowScratch(jit);
>+            jit.load32(&things, tmp);
>+        });
>+    root->append(Patch, patchpoint1, Arg::special(patchpointSpecial));
>+
>+    // Load things[1] -> x17, trashing dataMemoryRegister.
>+    root->append(Move32, nullptr, Arg::addr(base, 1 * sizeof(int32_t)), Tmp(ARM64Registers::x17));
>+    root->append(Add32, nullptr, Tmp(tmp), Tmp(ARM64Registers::x17), Tmp(GPRInfo::returnValueGPR));
>+
>+    // In Patchpoint, Load things[2] -> tmp. This should not reuse the prior contents of x17.
>+    B3::BasicBlock* patchPoint2Root = proc.addBlock();
>+    B3::PatchpointValue* patchpoint2 = patchPoint2Root->appendNew<B3::PatchpointValue>(proc, B3::Void, B3::Origin());
>+    patchpoint2->clobber(RegisterSet::macroScratchRegisters());
>+    patchpoint2->setGenerator(
>+        [=] (CCallHelpers& jit, const B3::StackmapGenerationParams&) {
>+            AllowMacroScratchRegisterUsage allowScratch(jit);
>+            jit.load32(&things[2], tmp);
>+        });
>+    root->append(Patch, patchpoint2, Arg::special(patchpointSpecial));
>+
>+    root->append(Add32, nullptr, Tmp(tmp), Tmp(GPRInfo::returnValueGPR), Tmp(GPRInfo::returnValueGPR));
>+
>+    // In patchpoint, Store 0x78 -> things[3].
>+    // This will use and cache both x16 (dataMemoryRegister) and x17 (dataTempRegister).
>+    B3::BasicBlock* patchPoint3Root = proc.addBlock();
>+    B3::PatchpointValue* patchpoint3 = patchPoint3Root->appendNew<B3::PatchpointValue>(proc, B3::Void, B3::Origin());
>+    patchpoint3->clobber(RegisterSet::macroScratchRegisters());
>+    patchpoint3->setGenerator(
>+        [=] (CCallHelpers& jit, const B3::StackmapGenerationParams&) {
>+            AllowMacroScratchRegisterUsage allowScratch(jit);
>+            jit.store32(CCallHelpers::TrustedImm32(0x78), &things[3]);
>+        });
>+    root->append(Patch, patchpoint3, Arg::special(patchpointSpecial));
>+
>+    // Set x16 to 0xdead, trashing x16.
>+    root->append(Move, nullptr, Arg::bigImm(0xdead), Tmp(ARM64Registers::x16));
>+    root->append(Xor32, nullptr, Tmp(ARM64Registers::x16), Tmp(GPRInfo::returnValueGPR));
>+
>+    // In patchpoint, again Store 0x78 -> things[3].
>+    // This should rematerialize both x16 (dataMemoryRegister) and x17 (dataTempRegister).
>+    B3::BasicBlock* patchPoint4Root = proc.addBlock();
>+    B3::PatchpointValue* patchpoint4 = patchPoint4Root->appendNew<B3::PatchpointValue>(proc, B3::Void, B3::Origin());
>+    patchpoint4->clobber(RegisterSet::macroScratchRegisters());
>+    patchpoint4->setGenerator(
>+        [=] (CCallHelpers& jit, const B3::StackmapGenerationParams&) {
>+            AllowMacroScratchRegisterUsage allowScratch(jit);
>+            jit.store32(CCallHelpers::TrustedImm32(0x78), &things[3]);
>+        });
>+    root->append(Patch, patchpoint4, Arg::special(patchpointSpecial));
>+
>+    root->append(Move, nullptr, Arg::bigImm(0xdead), Tmp(tmp));
>+    root->append(Xor32, nullptr, Tmp(tmp), Tmp(GPRInfo::returnValueGPR));
>+    root->append(Move32, nullptr, Arg::addr(base, 3 * sizeof(int32_t)), Tmp(tmp));
>+    root->append(Add32, nullptr, Tmp(tmp), Tmp(GPRInfo::returnValueGPR), Tmp(GPRInfo::returnValueGPR));
>+    root->append(Ret32, nullptr, Tmp(GPRInfo::returnValueGPR));
>+
>+    int32_t r = compileAndRun<int32_t>(proc);
>+    CHECK(r == 0x12345678);
>+}
>+#endif // #if CPU(ARM64)
>+
> void testArgumentRegPinned()
> {
>     B3::Procedure proc;
>@@ -2017,6 +2104,10 @@ void run(const char* filter)
>     RUN(testX86VMULSDBaseIndexNeedRex());
> #endif
> 
>+#if CPU(ARM64)
>+    RUN(testInvalidateCachedTempRegisters());
>+#endif
>+
>     RUN(testArgumentRegPinned());
>     RUN(testArgumentRegPinned2());
>     RUN(testArgumentRegPinned3());

Flags:

saam: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 185231: 339423 | 339434