Log Message
WebContent crash loading page on seas.upenn.edu @ _javascript_Core: vmEntryToJavaScript https://bugs.webkit.org/show_bug.cgi?id=185231
Reviewed by Saam Barati. We weren't clearing the scratch register cache when switching back and forth between allowing scratch register usage. We disallow scratch register usage when we are in code that will freely allocate and use any register. Such usage can change the contents of scratch registers. For ARM64, where we cache the contents of scratch registers to reuse some or all of the contained values, we need to invalidate these caches. We do this when re-enabling scratch register usage, that is when we transition from disallow to allow scratch register usage. 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:
Modified Paths
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/_javascript_Core/assembler/AllowMacroScratchRegisterUsage.h
- trunk/Source/_javascript_Core/assembler/AllowMacroScratchRegisterUsageIf.h
- trunk/Source/_javascript_Core/assembler/DisallowMacroScratchRegisterUsage.h
- trunk/Source/_javascript_Core/b3/air/testair.cpp
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (231316 => 231317)
--- trunk/Source/_javascript_Core/ChangeLog 2018-05-03 18:18:34 UTC (rev 231316)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-05-03 18:39:13 UTC (rev 231317)
@@ -1,3 +1,28 @@
+2018-05-03 Michael Saboff <[email protected]>
+
+ WebContent crash loading page on seas.upenn.edu @ _javascript_Core: vmEntryToJavaScript
+ https://bugs.webkit.org/show_bug.cgi?id=185231
+
+ Reviewed by Saam Barati.
+
+ We weren't clearing the scratch register cache when switching back and forth between
+ allowing scratch register usage. We disallow scratch register usage when we are in
+ code that will freely allocate and use any register. Such usage can change the
+ contents of scratch registers. For ARM64, where we cache the contents of scratch
+ registers to reuse some or all of the contained values, we need to invalidate these
+ caches. We do this when re-enabling scratch register usage, that is when we transition
+ from disallow to allow scratch register usage.
+
+ 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 Keith Miller <[email protected]>
Remove the prototype caching for get_by_id in the LLInt
Modified: trunk/Source/_javascript_Core/assembler/AllowMacroScratchRegisterUsage.h (231316 => 231317)
--- trunk/Source/_javascript_Core/assembler/AllowMacroScratchRegisterUsage.h 2018-05-03 18:18:34 UTC (rev 231316)
+++ trunk/Source/_javascript_Core/assembler/AllowMacroScratchRegisterUsage.h 2018-05-03 18:39:13 UTC (rev 231317)
@@ -37,6 +37,10 @@
: m_masm(masm)
, m_oldValueOfAllowScratchRegister(masm.m_allowScratchRegister)
{
+#if CPU(ARM64)
+ if (!m_oldValueOfAllowScratchRegister)
+ m_masm.invalidateAllTempRegisters();
+#endif
masm.m_allowScratchRegister = true;
}
Modified: trunk/Source/_javascript_Core/assembler/AllowMacroScratchRegisterUsageIf.h (231316 => 231317)
--- trunk/Source/_javascript_Core/assembler/AllowMacroScratchRegisterUsageIf.h 2018-05-03 18:18:34 UTC (rev 231316)
+++ trunk/Source/_javascript_Core/assembler/AllowMacroScratchRegisterUsageIf.h 2018-05-03 18:39:13 UTC (rev 231317)
@@ -38,8 +38,13 @@
, 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()
Modified: trunk/Source/_javascript_Core/assembler/DisallowMacroScratchRegisterUsage.h (231316 => 231317)
--- trunk/Source/_javascript_Core/assembler/DisallowMacroScratchRegisterUsage.h 2018-05-03 18:18:34 UTC (rev 231316)
+++ trunk/Source/_javascript_Core/assembler/DisallowMacroScratchRegisterUsage.h 2018-05-03 18:39:13 UTC (rev 231317)
@@ -42,6 +42,10 @@
~DisallowMacroScratchRegisterUsage()
{
+#if CPU(ARM64)
+ if (m_oldValueOfAllowScratchRegister)
+ m_masm.invalidateAllTempRegisters();
+#endif
m_masm.m_allowScratchRegister = m_oldValueOfAllowScratchRegister;
}
Modified: trunk/Source/_javascript_Core/b3/air/testair.cpp (231316 => 231317)
--- trunk/Source/_javascript_Core/b3/air/testair.cpp 2018-05-03 18:18:34 UTC (rev 231316)
+++ trunk/Source/_javascript_Core/b3/air/testair.cpp 2018-05-03 18:39:13 UTC (rev 231317)
@@ -1834,6 +1834,93 @@
}
#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 @@
RUN(testX86VMULSDBaseIndexNeedRex());
#endif
+#if CPU(ARM64)
+ RUN(testInvalidateCachedTempRegisters());
+#endif
+
RUN(testArgumentRegPinned());
RUN(testArgumentRegPinned2());
RUN(testArgumentRegPinned3());
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
