Attachment 362645 Details for Bug 194911 – proposed patch.

[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:

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
>

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194911: 362645 | 362679