Attachment 362679 Details for Bug 194911 – proposed patch after applying Saam's and Yusuke's feedback.

[patch] proposed patch after applying Saam's and Yusuke's feedback.

bug-194911.patch (text/plain), 17.86 KB, created by Mark Lam on 2019-02-21 17:54:00 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2019-02-21 17:54:00 PST

Size: 17.86 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 241924)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,78 @@
>+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.
>+        - Also added WTF_FORBID_HEAP_ALLOCATION to DeferGC, DeferGCForAWhile, and DisallowGC
>+          because all 3 should be stack allocated RAII objects.
>+
>+        * heap/GCDeferralContext.h:
>+        * heap/GCDeferralContextInlines.h:
>+        (JSC::GCDeferralContext::~GCDeferralContext):
>+        - Added WTF_FORBID_HEAP_ALLOCATION.
>+        - assert expectDoesGC.
>+
>+        * heap/Heap.cpp:
>+        (JSC::Heap::collectNow):
>+        (JSC::Heap::collectAsync):
>+        (JSC::Heap::collectSync):
>+        (JSC::Heap::stopIfNecessarySlow):
>+        (JSC::Heap::collectIfNecessaryOrDefer):
>+        * heap/HeapInlines.h:
>+        (JSC::Heap::acquireAccess):
>+        (JSC::Heap::stopIfNecessary):
>+        * heap/LargeAllocation.cpp:
>+        (JSC::LargeAllocation::tryCreate):
>+        * heap/LocalAllocatorInlines.h:
>+        (JSC::LocalAllocator::allocate):
>+        - conservatively assert expectDoesGC on these functions that may trigger a GC
>+          though they don't always do.
>+
>+        * runtime/DisallowScope.h:
>+        - DisallowScope should be stack allocated because it's an RAII object.
>+
>+        * runtime/JSCellInlines.h:
>+        (JSC::tryAllocateCellHelper):
>+        - Remove the expectDoesGC assertion because it is now covered by assertions in
>+          CompleteSubspace, LargeAllocation, and LocalAllocator.
>+
>+        * runtime/RegExpMatchesArray.h:
>+        (JSC::createRegExpMatchesArray):
>+        - assert expectDoesGC.
>+
> 2019-02-21  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] Use Fast Malloc as much as possible
>Index: Source/JavaScriptCore/dfg/DFGOSRExit.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGOSRExit.cpp	(revision 241924)
>+++ 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 241924)
>+++ 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 241924)
>+++ 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 241924)
>+++ 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 241924)
>+++ 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,8 @@ public:
>     
>     ~DeferGC()
>     {
>+        if (validateDFGDoesGC)
>+            RELEASE_ASSERT(m_heap.expectDoesGC());
>         m_heap.decrementDeferralDepthAndGCIfNeeded();
>     }
> 
>@@ -51,6 +54,7 @@ private:
> 
> class DeferGCForAWhile {
>     WTF_MAKE_NONCOPYABLE(DeferGCForAWhile);
>+    WTF_FORBID_HEAP_ALLOCATION;
> public:
>     DeferGCForAWhile(Heap& heap)
>         : m_heap(heap)
>@@ -69,6 +73,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/GCDeferralContext.h
>===================================================================
>--- Source/JavaScriptCore/heap/GCDeferralContext.h	(revision 241924)
>+++ Source/JavaScriptCore/heap/GCDeferralContext.h	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2016-2018 Apple Inc. All rights reserved.
>+ * Copyright (C) 2016-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,8 @@
> 
> #pragma once
> 
>+#include <wtf/ForbidHeapAllocation.h>
>+
> namespace JSC {
> 
> class Heap;
>@@ -32,6 +34,8 @@ class BlockDirectory;
> class LocalAllocator;
> 
> class GCDeferralContext {
>+    WTF_FORBID_HEAP_ALLOCATION;
>+
>     friend class Heap;
>     friend class BlockDirectory;
>     friend class LocalAllocator;
>Index: Source/JavaScriptCore/heap/GCDeferralContextInlines.h
>===================================================================
>--- Source/JavaScriptCore/heap/GCDeferralContextInlines.h	(revision 241924)
>+++ Source/JavaScriptCore/heap/GCDeferralContextInlines.h	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
>+ * Copyright (C) 2016-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
>@@ -37,6 +37,9 @@ ALWAYS_INLINE GCDeferralContext::GCDefer
> 
> ALWAYS_INLINE GCDeferralContext::~GCDeferralContext()
> {
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(m_heap.expectDoesGC());
>+
>     if (UNLIKELY(m_shouldGC))
>         m_heap.collectIfNecessaryOrDefer();
> }
>Index: Source/JavaScriptCore/heap/Heap.cpp
>===================================================================
>--- Source/JavaScriptCore/heap/Heap.cpp	(revision 241924)
>+++ 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/HeapInlines.h
>===================================================================
>--- Source/JavaScriptCore/heap/HeapInlines.h	(revision 241924)
>+++ 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/heap/LargeAllocation.cpp
>===================================================================
>--- Source/JavaScriptCore/heap/LargeAllocation.cpp	(revision 241924)
>+++ Source/JavaScriptCore/heap/LargeAllocation.cpp	(working copy)
>@@ -36,6 +36,9 @@ namespace JSC {
> 
> LargeAllocation* LargeAllocation::tryCreate(Heap& heap, size_t size, Subspace* subspace)
> {
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(heap.expectDoesGC());
>+
>     // This includes padding at the end of the allocation to maintain the distancing constraint.
>     constexpr size_t distancing = minimumDistanceBetweenCellsFromDifferentOrigins;
>     size_t sizeBeforeDistancing = headerSize() + size;
>Index: Source/JavaScriptCore/heap/LocalAllocatorInlines.h
>===================================================================
>--- Source/JavaScriptCore/heap/LocalAllocatorInlines.h	(revision 241924)
>+++ Source/JavaScriptCore/heap/LocalAllocatorInlines.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
>@@ -31,6 +31,8 @@ namespace JSC {
> 
> ALWAYS_INLINE void* LocalAllocator::allocate(GCDeferralContext* deferralContext, AllocationFailureMode failureMode)
> {
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(m_directory->heap()->expectDoesGC());
>     return m_freeList.allocate(
>         [&] () -> HeapCell* {
>             sanitizeStackForVM(m_directory->heap()->vm());
>Index: Source/JavaScriptCore/runtime/DisallowScope.h
>===================================================================
>--- Source/JavaScriptCore/runtime/DisallowScope.h	(revision 241924)
>+++ 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
> 
>Index: Source/JavaScriptCore/runtime/JSCellInlines.h
>===================================================================
>--- Source/JavaScriptCore/runtime/JSCellInlines.h	(revision 241924)
>+++ Source/JavaScriptCore/runtime/JSCellInlines.h	(working copy)
>@@ -166,9 +166,6 @@ template<typename T>
> ALWAYS_INLINE void* tryAllocateCellHelper(Heap& heap, size_t size, GCDeferralContext* deferralContext, AllocationFailureMode failureMode)
> {
>     VM& vm = *heap.vm();
>-    if (validateDFGDoesGC)
>-        RELEASE_ASSERT(heap.expectDoesGC());
>-
>     ASSERT(deferralContext || !DisallowGC::isInEffectOnCurrentThread());
>     ASSERT(size >= sizeof(T));
>     JSCell* result = static_cast<JSCell*>(subspaceFor<T>(vm)->allocateNonVirtual(vm, size, deferralContext, failureMode));
>Index: Source/JavaScriptCore/runtime/RegExpMatchesArray.h
>===================================================================
>--- Source/JavaScriptCore/runtime/RegExpMatchesArray.h	(revision 241924)
>+++ Source/JavaScriptCore/runtime/RegExpMatchesArray.h	(working copy)
>@@ -62,6 +62,9 @@ ALWAYS_INLINE JSArray* createRegExpMatch
>     VM& vm, JSGlobalObject* globalObject, JSString* input, const String& inputValue,
>     RegExp* regExp, unsigned startOffset, MatchResult& result)
> {
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(vm.heap.expectDoesGC());
>+
>     Vector<int, 32> subpatternResults;
>     int position = regExp->matchInline(vm, inputValue, startOffset, subpatternResults);
>     if (position == -1) {

Flags:

ysuzuki: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194911: 362645 | 362679