WebKit Bugzilla
Attachment 362645 Details for
Bug 194911
: Add more doesGC() assertions.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
proposed patch.
bug-194911.patch (text/plain), 13.63 KB, created by
Mark Lam
on 2019-02-21 14:17:57 PST
(
hide
)
Description:
proposed patch.
Filename:
MIME Type:
Creator:
Mark Lam
Created:
2019-02-21 14:17:57 PST
Size:
13.63 KB
patch
obsolete
>Index: Source/JavaScriptCore/ChangeLog >=================================================================== >--- Source/JavaScriptCore/ChangeLog (revision 241900) >+++ Source/JavaScriptCore/ChangeLog (working copy) >@@ -1,3 +1,68 @@ >+2019-02-21 Mark Lam <mark.lam@apple.com> >+ >+ Add more doesGC() assertions. >+ https://bugs.webkit.org/show_bug.cgi?id=194911 >+ <rdar://problem/48285723> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ * dfg/DFGOSRExit.cpp: >+ (JSC::DFG::OSRExit::compileOSRExit): >+ - Set expectDoesGC here because we no longer have to worry about missing store >+ barriers in optimized code after this point. This will prevent false positive >+ assertion failures arising from functions called beneath compileOSRExit(). >+ >+ (JSC::DFG::OSRExit::compileExit): >+ - Add a comment to explain why the generated ramp needs to set expectDoesGC even >+ though compileOSRExit() also sets it. Reason: compileOSRExit() is only called >+ for the first OSR from this code origin, the generated ramp is called for many >+ subsequents OSR exits from this code origin. >+ >+ * ftl/FTLOSRExitCompiler.cpp: >+ (JSC::FTL::compileStub): >+ - Added a comment for the equivalent reason to the one above. >+ >+ (JSC::FTL::compileFTLOSRExit): >+ - Set expectDoesGC here because we no longer have to worry about missing store >+ barriers in optimized code after this point. This will prevent false positive >+ assertion failures arising from functions called beneath compileFTLOSRExit(). >+ >+ * heap/CompleteSubspace.cpp: >+ (JSC::CompleteSubspace::tryAllocateSlow): >+ * heap/CompleteSubspaceInlines.h: >+ (JSC::CompleteSubspace::allocateNonVirtual): >+ - assert expectDoesGC. >+ >+ * heap/DeferGC.h: >+ (JSC::DeferGC::~DeferGC): >+ - assert expectDoesGC if this DeferGC instance is the outermost deferral scope. >+ We try to be conservative about asserting expectDoesGC wherever we can trigger GC. >+ However, with deferral scopes, asserting on inner deferral scopes will produce >+ false positives. So, we'll let the outermost scope do the assertion. >+ - Also added WTF_FORBID_HEAP_ALLOCATION to DeferGC, DeferGCForAWhile, and DisallowGC >+ because all 3 should be stack allocated RAII objects. >+ >+ * heap/Heap.cpp: >+ (JSC::Heap::collectNow): >+ (JSC::Heap::collectAsync): >+ (JSC::Heap::collectSync): >+ (JSC::Heap::stopIfNecessarySlow): >+ (JSC::Heap::collectIfNecessaryOrDefer): >+ - conservatively assert expectDoesGC on these functions that may trigger a GC >+ though they don't always do. >+ >+ * heap/Heap.h: >+ (JSC::Heap::deferralDepth const): >+ - new utility method used by the assertion in DeferGC. >+ >+ * heap/HeapInlines.h: >+ (JSC::Heap::acquireAccess): >+ (JSC::Heap::stopIfNecessary): >+ - assert expectDoesGC. >+ >+ * runtime/DisallowScope.h: >+ - DisallowScope should be stack allocated because it's an RAII object. >+ > 2019-02-20 Yusuke Suzuki <ysuzuki@apple.com> > > [JSC] Remove WatchpointSet creation for SymbolTable entries if VM::canUseJIT() returns false >Index: Source/JavaScriptCore/dfg/DFGOSRExit.cpp >=================================================================== >--- Source/JavaScriptCore/dfg/DFGOSRExit.cpp (revision 241873) >+++ Source/JavaScriptCore/dfg/DFGOSRExit.cpp (working copy) >@@ -1015,6 +1015,12 @@ void JIT_OPERATION OSRExit::compileOSREx > VM* vm = &exec->vm(); > auto scope = DECLARE_THROW_SCOPE(*vm); > >+ if (validateDFGDoesGC) { >+ // We're about to exit optimized code. So, there's no longer any optimized >+ // code running that expects no GC. >+ vm->heap.setExpectDoesGC(true); >+ } >+ > if (vm->callFrameForCatch) > RELEASE_ASSERT(vm->callFrameForCatch == exec); > >@@ -1399,6 +1405,11 @@ void OSRExit::compileExit(CCallHelpers& > // We're about to exit optimized code. So, there's no longer any optimized > // code running that expects no GC. We need to set this before arguments > // materialization below (see emitRestoreArguments()). >+ >+ // Even though we set Heap::m_expectDoesGC in compileOSRExit(), we also need >+ // to set it here because compileOSRExit() is only called on the first time >+ // we exit from this site, but all subsequent exits will take this compiled >+ // ramp without calling compileOSRExit() first. > jit.store8(CCallHelpers::TrustedImm32(true), vm.heap.addressOfExpectDoesGC()); > } > >Index: Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp >=================================================================== >--- Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp (revision 241873) >+++ Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp (working copy) >@@ -248,6 +248,11 @@ static void compileStub( > // We're about to exit optimized code. So, there's no longer any optimized > // code running that expects no GC. We need to set this before object > // materialization below. >+ >+ // Even though we set Heap::m_expectDoesGC in compileFTLOSRExit(), we also need >+ // to set it here because compileFTLOSRExit() is only called on the first time >+ // we exit from this site, but all subsequent exits will take this compiled >+ // ramp without calling compileFTLOSRExit() first. > jit.store8(CCallHelpers::TrustedImm32(true), vm->heap.addressOfExpectDoesGC()); > } > >@@ -526,6 +531,13 @@ extern "C" void* compileFTLOSRExit(ExecS > dataLog("Compiling OSR exit with exitID = ", exitID, "\n"); > > VM& vm = exec->vm(); >+ >+ if (validateDFGDoesGC) { >+ // We're about to exit optimized code. So, there's no longer any optimized >+ // code running that expects no GC. >+ vm.heap.setExpectDoesGC(true); >+ } >+ > if (vm.callFrameForCatch) > RELEASE_ASSERT(vm.callFrameForCatch == exec); > >Index: Source/JavaScriptCore/heap/CompleteSubspace.cpp >=================================================================== >--- Source/JavaScriptCore/heap/CompleteSubspace.cpp (revision 241873) >+++ Source/JavaScriptCore/heap/CompleteSubspace.cpp (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2017-2018 Apple Inc. All rights reserved. >+ * Copyright (C) 2017-2019 Apple Inc. All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions >@@ -122,6 +122,9 @@ void* CompleteSubspace::allocateSlow(VM& > > void* CompleteSubspace::tryAllocateSlow(VM& vm, size_t size, GCDeferralContext* deferralContext) > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(vm.heap.expectDoesGC()); >+ > sanitizeStackForVM(&vm); > > if (Allocator allocator = allocatorFor(size, AllocatorForMode::EnsureAllocator)) >Index: Source/JavaScriptCore/heap/CompleteSubspaceInlines.h >=================================================================== >--- Source/JavaScriptCore/heap/CompleteSubspaceInlines.h (revision 241873) >+++ Source/JavaScriptCore/heap/CompleteSubspaceInlines.h (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2018 Apple Inc. All rights reserved. >+ * Copyright (C) 2018-2019 Apple Inc. All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions >@@ -29,6 +29,9 @@ namespace JSC { > > ALWAYS_INLINE void* CompleteSubspace::allocateNonVirtual(VM& vm, size_t size, GCDeferralContext* deferralContext, AllocationFailureMode failureMode) > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(vm.heap.expectDoesGC()); >+ > if (Allocator allocator = allocatorForNonVirtual(size, AllocatorForMode::AllocatorIfExists)) > return allocator.allocate(deferralContext, failureMode); > return allocateSlow(vm, size, deferralContext, failureMode); >Index: Source/JavaScriptCore/heap/DeferGC.h >=================================================================== >--- Source/JavaScriptCore/heap/DeferGC.h (revision 241873) >+++ Source/JavaScriptCore/heap/DeferGC.h (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2013-2017 Apple Inc. All rights reserved. >+ * Copyright (C) 2013-2019 Apple Inc. All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions >@@ -33,6 +33,7 @@ namespace JSC { > > class DeferGC { > WTF_MAKE_NONCOPYABLE(DeferGC); >+ WTF_FORBID_HEAP_ALLOCATION; > public: > DeferGC(Heap& heap) > : m_heap(heap) >@@ -42,6 +43,12 @@ public: > > ~DeferGC() > { >+ if (validateDFGDoesGC) { >+ // It is only possible to trigger a GC here only if this DeferGC >+ // scope is the outermost deferral scope. >+ if (m_heap.deferralDepth() == 1) >+ RELEASE_ASSERT(m_heap.expectDoesGC()); >+ } > m_heap.decrementDeferralDepthAndGCIfNeeded(); > } > >@@ -51,6 +58,7 @@ private: > > class DeferGCForAWhile { > WTF_MAKE_NONCOPYABLE(DeferGCForAWhile); >+ WTF_FORBID_HEAP_ALLOCATION; > public: > DeferGCForAWhile(Heap& heap) > : m_heap(heap) >@@ -69,6 +77,7 @@ private: > > class DisallowGC : public DisallowScope<DisallowGC> { > WTF_MAKE_NONCOPYABLE(DisallowGC); >+ WTF_FORBID_HEAP_ALLOCATION; > typedef DisallowScope<DisallowGC> Base; > public: > #ifdef NDEBUG >Index: Source/JavaScriptCore/heap/Heap.cpp >=================================================================== >--- Source/JavaScriptCore/heap/Heap.cpp (revision 241873) >+++ Source/JavaScriptCore/heap/Heap.cpp (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2003-2018 Apple Inc. All rights reserved. >+ * Copyright (C) 2003-2019 Apple Inc. All rights reserved. > * Copyright (C) 2007 Eric Seidel <eric@webkit.org> > * > * This library is free software; you can redistribute it and/or >@@ -1029,6 +1029,9 @@ void Heap::collect(Synchronousness synch > > void Heap::collectNow(Synchronousness synchronousness, GCRequest request) > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(expectDoesGC()); >+ > switch (synchronousness) { > case Async: { > collectAsync(request); >@@ -1061,6 +1064,9 @@ void Heap::collectNow(Synchronousness sy > > void Heap::collectAsync(GCRequest request) > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(expectDoesGC()); >+ > if (!m_isSafeToCollect) > return; > >@@ -1082,9 +1088,12 @@ void Heap::collectAsync(GCRequest reques > > void Heap::collectSync(GCRequest request) > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(expectDoesGC()); >+ > if (!m_isSafeToCollect) > return; >- >+ > waitForCollection(requestCollection(request)); > } > >@@ -1737,6 +1746,9 @@ NEVER_INLINE void Heap::resumeTheMutator > > void Heap::stopIfNecessarySlow() > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(expectDoesGC()); >+ > while (stopIfNecessarySlow(m_worldState.load())) { } > > RELEASE_ASSERT(m_worldState.load() & hasAccessBit); >@@ -1749,6 +1761,9 @@ void Heap::stopIfNecessarySlow() > > bool Heap::stopIfNecessarySlow(unsigned oldState) > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(expectDoesGC()); >+ > RELEASE_ASSERT(oldState & hasAccessBit); > RELEASE_ASSERT(!(oldState & stoppedBit)); > >@@ -2538,9 +2553,12 @@ void Heap::reportExternalMemoryVisited(s > void Heap::collectIfNecessaryOrDefer(GCDeferralContext* deferralContext) > { > ASSERT(deferralContext || isDeferred() || !DisallowGC::isInEffectOnCurrentThread()); >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(expectDoesGC()); > > if (!m_isSafeToCollect) > return; >+ > switch (mutatorState()) { > case MutatorState::Running: > case MutatorState::Allocating: >Index: Source/JavaScriptCore/heap/Heap.h >=================================================================== >--- Source/JavaScriptCore/heap/Heap.h (revision 241873) >+++ Source/JavaScriptCore/heap/Heap.h (working copy) >@@ -540,6 +540,7 @@ private: > > bool shouldDoFullCollection(); > >+ unsigned deferralDepth() const { return m_deferralDepth; } > void incrementDeferralDepth(); > void decrementDeferralDepth(); > void decrementDeferralDepthAndGCIfNeeded(); >Index: Source/JavaScriptCore/heap/HeapInlines.h >=================================================================== >--- Source/JavaScriptCore/heap/HeapInlines.h (revision 241873) >+++ Source/JavaScriptCore/heap/HeapInlines.h (working copy) >@@ -238,6 +238,9 @@ inline void Heap::deprecatedReportExtraM > > inline void Heap::acquireAccess() > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(expectDoesGC()); >+ > if (m_worldState.compareExchangeWeak(0, hasAccessBit)) > return; > acquireAccessSlow(); >@@ -262,6 +265,9 @@ inline bool Heap::mayNeedToStop() > > inline void Heap::stopIfNecessary() > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(expectDoesGC()); >+ > if (mayNeedToStop()) > stopIfNecessarySlow(); > } >Index: Source/JavaScriptCore/runtime/DisallowScope.h >=================================================================== >--- Source/JavaScriptCore/runtime/DisallowScope.h (revision 241873) >+++ Source/JavaScriptCore/runtime/DisallowScope.h (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2017-2018 Apple Inc. All rights reserved. >+ * Copyright (C) 2017-2019 Apple Inc. All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions >@@ -25,6 +25,7 @@ > > #pragma once > >+#include <wtf/ForbidHeapAllocation.h> > #include <wtf/Noncopyable.h> > > namespace JSC { >@@ -32,6 +33,7 @@ namespace JSC { > template<class T> > class DisallowScope { > WTF_MAKE_NONCOPYABLE(DisallowScope); >+ WTF_FORBID_HEAP_ALLOCATION; > public: > #ifdef NDEBUG >
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+
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 194911
:
362645
|
362679