WebKit Bugzilla
Attachment 339423 Details for
Bug 185231
: WebContent crash loading page on seas.upenn.edu @ JavaScriptCore: vmEntryToJavaScript
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
185231.patch (text/plain), 8.10 KB, created by
Michael Saboff
on 2018-05-03 10:18:36 PDT
(
hide
)
Description:
Patch
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());
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Flags:
saam
:
review+
ews-watchlist
:
commit-queue-
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 185231
: 339423 |
339434