- Revision
- 199275
- Author
- [email protected]
- Date
- 2016-04-09 13:41:04 -0700 (Sat, 09 Apr 2016)
Log Message
Debug JSC test failure: stress/multi-put-by-offset-reallocation-butterfly-cse.js.ftl-no-cjit-small-pool
https://bugs.webkit.org/show_bug.cgi?id=156406
Reviewed by Saam Barati.
The failure was because the GC ran from within the butterfly allocation call in a put_by_id
transition AccessCase that had to deal with indexing storage. When the GC runs in a call from a stub,
then we need to be extra careful:
1) The GC may reset the IC and delete the stub. So, the stub needs to tell the GC that it might be on
the stack during GC, so that the GC keeps it alive if it's currently running.
2) If the stub uses (dereferences or stores) some object after the call, then we need to ensure that
the stub routine knows about that object independently of the IC.
In the case of put_by_id transitions that use a helper to allocate the butterfly, we have both
issues. A long time ago, we had to deal with (2), and we still had code to handle that case, although
it appears to be dead. This change revives that code and glues it together with PolymorphicAccess.
* bytecode/PolymorphicAccess.cpp:
(JSC::AccessCase::alternateBase):
(JSC::AccessCase::doesCalls):
(JSC::AccessCase::couldStillSucceed):
(JSC::AccessCase::generate):
(JSC::PolymorphicAccess::regenerate):
* bytecode/PolymorphicAccess.h:
(JSC::AccessCase::customSlotBase):
(JSC::AccessCase::isGetter):
(JSC::AccessCase::doesCalls): Deleted.
* jit/GCAwareJITStubRoutine.cpp:
(JSC::GCAwareJITStubRoutine::markRequiredObjectsInternal):
(JSC::MarkingGCAwareJITStubRoutine::MarkingGCAwareJITStubRoutine):
(JSC::MarkingGCAwareJITStubRoutine::~MarkingGCAwareJITStubRoutine):
(JSC::MarkingGCAwareJITStubRoutine::markRequiredObjectsInternal):
(JSC::GCAwareJITStubRoutineWithExceptionHandler::GCAwareJITStubRoutineWithExceptionHandler):
(JSC::createJITStubRoutine):
(JSC::MarkingGCAwareJITStubRoutineWithOneObject::MarkingGCAwareJITStubRoutineWithOneObject): Deleted.
(JSC::MarkingGCAwareJITStubRoutineWithOneObject::~MarkingGCAwareJITStubRoutineWithOneObject): Deleted.
(JSC::MarkingGCAwareJITStubRoutineWithOneObject::markRequiredObjectsInternal): Deleted.
* jit/GCAwareJITStubRoutine.h:
(JSC::createJITStubRoutine):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (199274 => 199275)
--- trunk/Source/_javascript_Core/ChangeLog 2016-04-09 20:13:12 UTC (rev 199274)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-04-09 20:41:04 UTC (rev 199275)
@@ -1,3 +1,47 @@
+2016-04-09 Filip Pizlo <[email protected]>
+
+ Debug JSC test failure: stress/multi-put-by-offset-reallocation-butterfly-cse.js.ftl-no-cjit-small-pool
+ https://bugs.webkit.org/show_bug.cgi?id=156406
+
+ Reviewed by Saam Barati.
+
+ The failure was because the GC ran from within the butterfly allocation call in a put_by_id
+ transition AccessCase that had to deal with indexing storage. When the GC runs in a call from a stub,
+ then we need to be extra careful:
+
+ 1) The GC may reset the IC and delete the stub. So, the stub needs to tell the GC that it might be on
+ the stack during GC, so that the GC keeps it alive if it's currently running.
+
+ 2) If the stub uses (dereferences or stores) some object after the call, then we need to ensure that
+ the stub routine knows about that object independently of the IC.
+
+ In the case of put_by_id transitions that use a helper to allocate the butterfly, we have both
+ issues. A long time ago, we had to deal with (2), and we still had code to handle that case, although
+ it appears to be dead. This change revives that code and glues it together with PolymorphicAccess.
+
+ * bytecode/PolymorphicAccess.cpp:
+ (JSC::AccessCase::alternateBase):
+ (JSC::AccessCase::doesCalls):
+ (JSC::AccessCase::couldStillSucceed):
+ (JSC::AccessCase::generate):
+ (JSC::PolymorphicAccess::regenerate):
+ * bytecode/PolymorphicAccess.h:
+ (JSC::AccessCase::customSlotBase):
+ (JSC::AccessCase::isGetter):
+ (JSC::AccessCase::doesCalls): Deleted.
+ * jit/GCAwareJITStubRoutine.cpp:
+ (JSC::GCAwareJITStubRoutine::markRequiredObjectsInternal):
+ (JSC::MarkingGCAwareJITStubRoutine::MarkingGCAwareJITStubRoutine):
+ (JSC::MarkingGCAwareJITStubRoutine::~MarkingGCAwareJITStubRoutine):
+ (JSC::MarkingGCAwareJITStubRoutine::markRequiredObjectsInternal):
+ (JSC::GCAwareJITStubRoutineWithExceptionHandler::GCAwareJITStubRoutineWithExceptionHandler):
+ (JSC::createJITStubRoutine):
+ (JSC::MarkingGCAwareJITStubRoutineWithOneObject::MarkingGCAwareJITStubRoutineWithOneObject): Deleted.
+ (JSC::MarkingGCAwareJITStubRoutineWithOneObject::~MarkingGCAwareJITStubRoutineWithOneObject): Deleted.
+ (JSC::MarkingGCAwareJITStubRoutineWithOneObject::markRequiredObjectsInternal): Deleted.
+ * jit/GCAwareJITStubRoutine.h:
+ (JSC::createJITStubRoutine):
+
2016-04-08 Joseph Pecoraro <[email protected]>
Web Inspector: XHRs and Web Worker scripts are not searchable
Modified: trunk/Source/_javascript_Core/bytecode/PolymorphicAccess.cpp (199274 => 199275)
--- trunk/Source/_javascript_Core/bytecode/PolymorphicAccess.cpp 2016-04-09 20:13:12 UTC (rev 199274)
+++ trunk/Source/_javascript_Core/bytecode/PolymorphicAccess.cpp 2016-04-09 20:41:04 UTC (rev 199275)
@@ -413,6 +413,29 @@
return conditionSet().slotBaseCondition().object();
}
+bool AccessCase::doesCalls(Vector<JSCell*>* cellsToMark) const
+{
+ switch (type()) {
+ case Getter:
+ case Setter:
+ case CustomValueGetter:
+ case CustomAccessorGetter:
+ case CustomValueSetter:
+ case CustomAccessorSetter:
+ return true;
+ case Transition:
+ if (newStructure()->outOfLineCapacity() != structure()->outOfLineCapacity()
+ && structure()->couldHaveIndexingHeader()) {
+ if (cellsToMark)
+ cellsToMark->append(newStructure());
+ return true;
+ }
+ return false;
+ default:
+ return false;
+ }
+}
+
bool AccessCase::couldStillSucceed() const
{
return m_conditionSet.structuresEnsureValidityAssumingImpurePropertyWatchpoint();
@@ -1093,6 +1116,8 @@
} else if (verbose)
dataLog("Don't have type.\n");
+ // NOTE: This logic is duplicated in AccessCase::doesCalls(). It's important that doesCalls() knows
+ // exactly when this would make calls.
bool allocating = newStructure()->outOfLineCapacity() != structure()->outOfLineCapacity();
bool reallocating = allocating && structure()->outOfLineCapacity();
bool allocatingInline = allocating && !structure()->couldHaveIndexingHeader();
@@ -1632,10 +1657,11 @@
("%s", toCString("Access stub for ", *codeBlock, " ", stubInfo.codeOrigin, " with return point ", successLabel, ": ", listDump(cases)).data()));
bool doesCalls = false;
+ Vector<JSCell*> cellsToMark;
for (auto& entry : cases)
- doesCalls |= entry->doesCalls();
+ doesCalls |= entry->doesCalls(&cellsToMark);
- m_stubRoutine = createJITStubRoutine(code, vm, codeBlock, doesCalls, nullptr, codeBlockThatOwnsExceptionHandlers, callSiteIndexForExceptionHandling);
+ m_stubRoutine = createJITStubRoutine(code, vm, codeBlock, doesCalls, cellsToMark, codeBlockThatOwnsExceptionHandlers, callSiteIndexForExceptionHandling);
m_watchpoints = WTFMove(state.watchpoints);
if (!state.weakReferences.isEmpty())
m_weakReferences = std::make_unique<Vector<WriteBarrier<JSCell>>>(WTFMove(state.weakReferences));
Modified: trunk/Source/_javascript_Core/bytecode/PolymorphicAccess.h (199274 => 199275)
--- trunk/Source/_javascript_Core/bytecode/PolymorphicAccess.h 2016-04-09 20:13:12 UTC (rev 199274)
+++ trunk/Source/_javascript_Core/bytecode/PolymorphicAccess.h 2016-04-09 20:41:04 UTC (rev 199275)
@@ -153,22 +153,11 @@
}
JSObject* alternateBase() const;
-
- bool doesCalls() const
- {
- switch (type()) {
- case Getter:
- case Setter:
- case CustomValueGetter:
- case CustomAccessorGetter:
- case CustomValueSetter:
- case CustomAccessorSetter:
- return true;
- default:
- return false;
- }
- }
+ // If you supply the optional vector, this will append the set of cells that this will need to keep alive
+ // past the call.
+ bool doesCalls(Vector<JSCell*>* cellsToMark = nullptr) const;
+
bool isGetter() const
{
switch (type()) {
Modified: trunk/Source/_javascript_Core/jit/GCAwareJITStubRoutine.cpp (199274 => 199275)
--- trunk/Source/_javascript_Core/jit/GCAwareJITStubRoutine.cpp 2016-04-09 20:13:12 UTC (rev 199274)
+++ trunk/Source/_javascript_Core/jit/GCAwareJITStubRoutine.cpp 2016-04-09 20:41:04 UTC (rev 199275)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2012 Apple Inc. All rights reserved.
+ * Copyright (C) 2012, 2016 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -79,28 +79,31 @@
{
}
-MarkingGCAwareJITStubRoutineWithOneObject::MarkingGCAwareJITStubRoutineWithOneObject(
+MarkingGCAwareJITStubRoutine::MarkingGCAwareJITStubRoutine(
const MacroAssemblerCodeRef& code, VM& vm, const JSCell* owner,
- JSCell* object)
+ const Vector<JSCell*>& cells)
: GCAwareJITStubRoutine(code, vm)
- , m_object(vm, owner, object)
+ , m_cells(cells.size())
{
+ for (unsigned i = cells.size(); i--;)
+ m_cells[i].set(vm, owner, cells[i]);
}
-MarkingGCAwareJITStubRoutineWithOneObject::~MarkingGCAwareJITStubRoutineWithOneObject()
+MarkingGCAwareJITStubRoutine::~MarkingGCAwareJITStubRoutine()
{
}
-void MarkingGCAwareJITStubRoutineWithOneObject::markRequiredObjectsInternal(SlotVisitor& visitor)
+void MarkingGCAwareJITStubRoutine::markRequiredObjectsInternal(SlotVisitor& visitor)
{
- visitor.append(&m_object);
+ for (auto& entry : m_cells)
+ visitor.append(&entry);
}
GCAwareJITStubRoutineWithExceptionHandler::GCAwareJITStubRoutineWithExceptionHandler(
- const MacroAssemblerCodeRef& code, VM& vm,
+ const MacroAssemblerCodeRef& code, VM& vm, const JSCell* owner, const Vector<JSCell*>& cells,
CodeBlock* codeBlockForExceptionHandlers, CallSiteIndex exceptionHandlerCallSiteIndex)
- : GCAwareJITStubRoutine(code, vm)
+ : MarkingGCAwareJITStubRoutine(code, vm, owner, cells)
, m_codeBlockWithExceptionHandler(codeBlockForExceptionHandlers)
, m_exceptionHandlerCallSiteIndex(exceptionHandlerCallSiteIndex)
{
@@ -132,7 +135,7 @@
VM& vm,
const JSCell* owner,
bool makesCalls,
- JSCell* object,
+ const Vector<JSCell*>& cells,
CodeBlock* codeBlockForExceptionHandlers,
CallSiteIndex exceptionHandlerCallSiteIndex)
{
@@ -140,19 +143,18 @@
return adoptRef(new JITStubRoutine(code));
if (codeBlockForExceptionHandlers) {
- RELEASE_ASSERT(!object); // We're not a marking stub routine.
RELEASE_ASSERT(JITCode::isOptimizingJIT(codeBlockForExceptionHandlers->jitType()));
return static_pointer_cast<JITStubRoutine>(
- adoptRef(new GCAwareJITStubRoutineWithExceptionHandler(code, vm, codeBlockForExceptionHandlers, exceptionHandlerCallSiteIndex)));
+ adoptRef(new GCAwareJITStubRoutineWithExceptionHandler(code, vm, owner, cells, codeBlockForExceptionHandlers, exceptionHandlerCallSiteIndex)));
}
- if (!object) {
+ if (cells.isEmpty()) {
return static_pointer_cast<JITStubRoutine>(
adoptRef(new GCAwareJITStubRoutine(code, vm)));
}
return static_pointer_cast<JITStubRoutine>(
- adoptRef(new MarkingGCAwareJITStubRoutineWithOneObject(code, vm, owner, object)));
+ adoptRef(new MarkingGCAwareJITStubRoutine(code, vm, owner, cells)));
}
} // namespace JSC
Modified: trunk/Source/_javascript_Core/jit/GCAwareJITStubRoutine.h (199274 => 199275)
--- trunk/Source/_javascript_Core/jit/GCAwareJITStubRoutine.h 2016-04-09 20:13:12 UTC (rev 199274)
+++ trunk/Source/_javascript_Core/jit/GCAwareJITStubRoutine.h 2016-04-09 20:41:04 UTC (rev 199275)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2012, 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2012, 2014, 2016 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -76,28 +76,28 @@
// Use this if you want to mark one additional object during GC if your stub
// routine is known to be executing.
-class MarkingGCAwareJITStubRoutineWithOneObject : public GCAwareJITStubRoutine {
+class MarkingGCAwareJITStubRoutine : public GCAwareJITStubRoutine {
public:
- MarkingGCAwareJITStubRoutineWithOneObject(
- const MacroAssemblerCodeRef&, VM&, const JSCell* owner, JSCell*);
- virtual ~MarkingGCAwareJITStubRoutineWithOneObject();
+ MarkingGCAwareJITStubRoutine(
+ const MacroAssemblerCodeRef&, VM&, const JSCell* owner, const Vector<JSCell*>&);
+ virtual ~MarkingGCAwareJITStubRoutine();
protected:
void markRequiredObjectsInternal(SlotVisitor&) override;
private:
- WriteBarrier<JSCell> m_object;
+ Vector<WriteBarrier<JSCell>> m_cells;
};
// The stub has exception handlers in it. So it clears itself from exception
// handling table when it dies. It also frees space in CodeOrigin table
// for new exception handlers to use the same CallSiteIndex.
-class GCAwareJITStubRoutineWithExceptionHandler : public GCAwareJITStubRoutine {
+class GCAwareJITStubRoutineWithExceptionHandler : public MarkingGCAwareJITStubRoutine {
public:
typedef GCAwareJITStubRoutine Base;
- GCAwareJITStubRoutineWithExceptionHandler(const MacroAssemblerCodeRef&, VM&, CodeBlock*, CallSiteIndex);
+ GCAwareJITStubRoutineWithExceptionHandler(const MacroAssemblerCodeRef&, VM&, const JSCell* owner, const Vector<JSCell*>&, CodeBlock*, CallSiteIndex);
void aboutToDie() override;
void observeZeroRefCount() override;
@@ -128,14 +128,9 @@
PassRefPtr<JITStubRoutine> createJITStubRoutine(
const MacroAssemblerCodeRef&, VM&, const JSCell* owner, bool makesCalls,
- JSCell* = nullptr,
+ const Vector<JSCell*>& = { },
CodeBlock* codeBlockForExceptionHandlers = nullptr, CallSiteIndex exceptionHandlingCallSiteIndex = CallSiteIndex(std::numeric_limits<unsigned>::max()));
-// Helper for the creation of simple stub routines that need no help from the GC. Note
-// that codeBlock gets "executed" more than once.
-#define FINALIZE_CODE_FOR_GC_AWARE_STUB(codeBlock, patchBuffer, makesCalls, cell, dataLogFArguments) \
- (createJITStubRoutine(FINALIZE_CODE_FOR((codeBlock), (patchBuffer), dataLogFArguments), *(codeBlock)->vm(), (codeBlock), (makesCalls), (cell)))
-
} // namespace JSC
#endif // ENABLE(JIT)
