WebKit Bugzilla
Attachment 362559 Details for
Bug 193938
: We should add code to validate expected GC activity modelled by doesGC() against what the runtime encounters.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
patch for landing.
bug-193938.patch (text/plain), 17.21 KB, created by
Mark Lam
on 2019-02-20 15:59:28 PST
(
hide
)
Description:
patch for landing.
Filename:
MIME Type:
Creator:
Mark Lam
Created:
2019-02-20 15:59:28 PST
Size:
17.21 KB
patch
obsolete
>Index: Source/JavaScriptCore/ChangeLog >=================================================================== >--- Source/JavaScriptCore/ChangeLog (revision 241846) >+++ Source/JavaScriptCore/ChangeLog (working copy) >@@ -1,3 +1,61 @@ >+2019-02-20 Mark Lam <mark.lam@apple.com> >+ >+ Add code to validate expected GC activity modelled by doesGC() against what the runtime encounters. >+ https://bugs.webkit.org/show_bug.cgi?id=193938 >+ <rdar://problem/47616277> >+ >+ Reviewed by Michael Saboff, Saam Barati, and Robin Morisset. >+ >+ In DFG::SpeculativeJIT::compile() and FTL::LowerDFGToB3::compileNode(), before >+ emitting code / B3IR for each DFG node, we emit a write to set Heap::m_expectDoesGC >+ to the value returned by doesGC() for that node. In the runtime (i.e. in allocateCell() >+ and functions that can resolve a rope), we assert that Heap::m_expectDoesGC is >+ true. >+ >+ This validation code is currently only enabled for debug builds. It is disabled >+ for release builds by default, but it can easily be made to run on release builds >+ as well by forcing ENABLE_DFG_DOES_GC_VALIDATION to 1 in Heap.h. >+ >+ To allow this validation code to run on release builds as well, the validation uses >+ RELEASE_ASSERT instead of ASSERT. >+ >+ To ensure that Heap.h is #include'd for all files that needs to do this validation >+ (so that the validation code is accidentally disabled), we guard the validation >+ code with an if conditional on constexpr bool validateDFGDoesGC (instead of using >+ a #if ENABLE(DFG_DOES_GC_VALIDATION)). This way, if Heap.h isn't #include'd, the >+ validation code will fail to build (no silent failures). >+ >+ Currently, all JSC tests and Layout tests should pass with this validation enabled >+ in debug builds. We'll only see new failures if there's a regression or if new >+ tests reveal a previously untested code path that has an undetected issue. >+ >+ * dfg/DFGOSRExit.cpp: >+ (JSC::DFG::OSRExit::executeOSRExit): >+ (JSC::DFG::OSRExit::compileExit): >+ * dfg/DFGSpeculativeJIT64.cpp: >+ (JSC::DFG::SpeculativeJIT::compile): >+ * ftl/FTLLowerDFGToB3.cpp: >+ (JSC::FTL::DFG::LowerDFGToB3::compileNode): >+ * ftl/FTLOSRExitCompiler.cpp: >+ (JSC::FTL::compileStub): >+ * heap/Heap.h: >+ (JSC::Heap::expectDoesGC const): >+ (JSC::Heap::setExpectDoesGC): >+ (JSC::Heap::addressOfExpectDoesGC): >+ * jit/JITArithmetic.cpp: >+ (JSC::JIT::emit_compareAndJump): >+ * runtime/JSCellInlines.h: >+ (JSC::tryAllocateCellHelper): >+ * runtime/JSString.h: >+ (JSC::jsSingleCharacterString): >+ (JSC::JSString::toAtomicString const): >+ (JSC::JSString::toExistingAtomicString const): >+ (JSC::JSString::value const): >+ (JSC::JSString::tryGetValue const): >+ (JSC::JSRopeString::unsafeView const): >+ (JSC::JSRopeString::viewWithUnderlyingString const): >+ (JSC::JSString::unsafeView const): >+ > 2019-02-20 Andy Estes <aestes@apple.com> > > [Xcode] Add SDKVariant.xcconfig to various Xcode projects >Index: Source/JavaScriptCore/dfg/DFGOSRExit.cpp >=================================================================== >--- Source/JavaScriptCore/dfg/DFGOSRExit.cpp (revision 241846) >+++ Source/JavaScriptCore/dfg/DFGOSRExit.cpp (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2011-2018 Apple Inc. All rights reserved. >+ * Copyright (C) 2011-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 >@@ -338,6 +338,12 @@ void OSRExit::executeOSRExit(Context& co > ASSERT(&exec->vm() == &vm); > auto& cpu = context.cpu; > >+ 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) { > exec = vm.callFrameForCatch; > context.fp() = exec; >@@ -1389,6 +1395,13 @@ void OSRExit::compileExit(CCallHelpers& > } > } > >+ if (validateDFGDoesGC) { >+ // 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()). >+ jit.store8(CCallHelpers::TrustedImm32(true), vm.heap.addressOfExpectDoesGC()); >+ } >+ > // Need to ensure that the stack pointer accounts for the worst-case stack usage at exit. This > // could toast some stack that the DFG used. We need to do it before storing to stack offsets > // used by baseline. >Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp >=================================================================== >--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp (revision 241846) >+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2011-2018 Apple Inc. All rights reserved. >+ * Copyright (C) 2011-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 @@ > #include "CallFrameShuffler.h" > #include "DFGAbstractInterpreterInlines.h" > #include "DFGCallArrayAllocatorSlowPathGenerator.h" >+#include "DFGDoesGC.h" > #include "DFGOperations.h" > #include "DFGSlowPathGenerator.h" > #include "DirectArguments.h" >@@ -1899,7 +1900,12 @@ void SpeculativeJIT::emitBranch(Node* no > void SpeculativeJIT::compile(Node* node) > { > NodeType op = node->op(); >- >+ >+ if (validateDFGDoesGC) { >+ bool expectDoesGC = doesGC(m_jit.graph(), node); >+ m_jit.store8(TrustedImm32(expectDoesGC), m_jit.vm()->heap.addressOfExpectDoesGC()); >+ } >+ > #if ENABLE(DFG_REGISTER_ALLOCATION_VALIDATION) > m_jit.clearRegisterAllocationOffsets(); > #endif >Index: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp >=================================================================== >--- Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp (revision 241846) >+++ Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2013-2018 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 >@@ -43,6 +43,7 @@ > #include "CodeBlockWithJITType.h" > #include "DFGAbstractInterpreterInlines.h" > #include "DFGCapabilities.h" >+#include "DFGDoesGC.h" > #include "DFGDominators.h" > #include "DFGInPlaceAbstractState.h" > #include "DFGMayExit.h" >@@ -525,7 +526,12 @@ private: > > m_interpreter.startExecuting(); > m_interpreter.executeKnownEdgeTypes(m_node); >- >+ >+ if (validateDFGDoesGC) { >+ bool expectDoesGC = doesGC(m_graph, m_node); >+ m_out.store(m_out.constBool(expectDoesGC), m_out.absolute(vm().heap.addressOfExpectDoesGC())); >+ } >+ > switch (m_node->op()) { > case DFG::Upsilon: > compileUpsilon(); >Index: Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp >=================================================================== >--- Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp (revision 241846) >+++ Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2013-2018 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 >@@ -244,6 +244,13 @@ static void compileStub( > > saveAllRegisters(jit, registerScratch); > >+ if (validateDFGDoesGC) { >+ // 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. >+ jit.store8(CCallHelpers::TrustedImm32(true), vm->heap.addressOfExpectDoesGC()); >+ } >+ > // Bring the stack back into a sane form and assert that it's sane. > jit.popToRestore(GPRInfo::regT0); > jit.checkStackPointerAlignment(); >Index: Source/JavaScriptCore/heap/Heap.h >=================================================================== >--- Source/JavaScriptCore/heap/Heap.h (revision 241846) >+++ Source/JavaScriptCore/heap/Heap.h (working copy) >@@ -1,7 +1,7 @@ > /* > * Copyright (C) 1999-2000 Harri Porten (porten@kde.org) > * Copyright (C) 2001 Peter Kelly (pmk@post.com) >- * Copyright (C) 2003-2017 Apple Inc. All rights reserved. >+ * Copyright (C) 2003-2019 Apple Inc. All rights reserved. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public >@@ -95,6 +95,13 @@ class SpeculativeJIT; > class Worklist; > } > >+#if !ASSERT_DISABLED >+#define ENABLE_DFG_DOES_GC_VALIDATION 1 >+#else >+#define ENABLE_DFG_DOES_GC_VALIDATION 0 >+#endif >+constexpr bool validateDFGDoesGC = ENABLE_DFG_DOES_GC_VALIDATION; >+ > typedef HashCountedSet<JSCell*> ProtectCountSet; > typedef HashCountedSet<const char*> TypeCountSet; > >@@ -294,6 +301,16 @@ public: > unsigned barrierThreshold() const { return m_barrierThreshold; } > const unsigned* addressOfBarrierThreshold() const { return &m_barrierThreshold; } > >+#if ENABLE(DFG_DOES_GC_VALIDATION) >+ bool expectDoesGC() const { return m_expectDoesGC; } >+ void setExpectDoesGC(bool value) { m_expectDoesGC = value; } >+ bool* addressOfExpectDoesGC() { return &m_expectDoesGC; } >+#else >+ bool expectDoesGC() const { UNREACHABLE_FOR_PLATFORM(); return true; } >+ void setExpectDoesGC(bool) { UNREACHABLE_FOR_PLATFORM(); } >+ bool* addressOfExpectDoesGC() { UNREACHABLE_FOR_PLATFORM(); return nullptr; } >+#endif >+ > // If true, the GC believes that the mutator is currently messing with the heap. We call this > // "having heap access". The GC may block if the mutator is in this state. If false, the GC may > // currently be doing things to the heap that make the heap unsafe to access for the mutator. >@@ -581,6 +598,9 @@ private: > Markable<CollectionScope, EnumMarkableTraits<CollectionScope>> m_collectionScope; > Markable<CollectionScope, EnumMarkableTraits<CollectionScope>> m_lastCollectionScope; > Lock m_raceMarkStackLock; >+#if ENABLE(DFG_DOES_GC_VALIDATION) >+ bool m_expectDoesGC { true }; >+#endif > > StructureIDTable m_structureIDTable; > MarkedSpace m_objectSpace; >Index: Source/JavaScriptCore/jit/JITArithmetic.cpp >=================================================================== >--- Source/JavaScriptCore/jit/JITArithmetic.cpp (revision 241846) >+++ Source/JavaScriptCore/jit/JITArithmetic.cpp (working copy) >@@ -179,13 +179,14 @@ void JIT::emit_compareAndJump(const Inst > int op1 = bytecode.m_lhs.offset(); > int op2 = bytecode.m_rhs.offset(); > unsigned target = jumpTarget(instruction, bytecode.m_targetLabel); >+ bool disallowAllocation = false; > if (isOperandConstantChar(op1)) { > emitGetVirtualRegister(op2, regT0); > addSlowCase(branchIfNotCell(regT0)); > JumpList failures; > emitLoadCharacterString(regT0, regT0, failures); > addSlowCase(failures); >- addJump(branch32(commute(condition), regT0, Imm32(asString(getConstantOperand(op1))->tryGetValue()[0])), target); >+ addJump(branch32(commute(condition), regT0, Imm32(asString(getConstantOperand(op1))->tryGetValue(disallowAllocation)[0])), target); > return; > } > if (isOperandConstantChar(op2)) { >@@ -194,7 +195,7 @@ void JIT::emit_compareAndJump(const Inst > JumpList failures; > emitLoadCharacterString(regT0, regT0, failures); > addSlowCase(failures); >- addJump(branch32(condition, regT0, Imm32(asString(getConstantOperand(op2))->tryGetValue()[0])), target); >+ addJump(branch32(condition, regT0, Imm32(asString(getConstantOperand(op2))->tryGetValue(disallowAllocation)[0])), target); > return; > } > if (isOperandConstantInt(op2)) { >Index: Source/JavaScriptCore/runtime/JSCellInlines.h >=================================================================== >--- Source/JavaScriptCore/runtime/JSCellInlines.h (revision 241846) >+++ Source/JavaScriptCore/runtime/JSCellInlines.h (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2012-2018 Apple Inc. All rights reserved. >+ * Copyright (C) 2012-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 >@@ -166,6 +166,9 @@ 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/JSString.h >=================================================================== >--- Source/JavaScriptCore/runtime/JSString.h (revision 241846) >+++ Source/JavaScriptCore/runtime/JSString.h (working copy) >@@ -1,7 +1,7 @@ > /* > * Copyright (C) 1999-2001 Harri Porten (porten@kde.org) > * Copyright (C) 2001 Peter Kelly (pmk@post.com) >- * Copyright (C) 2003-2018 Apple Inc. All rights reserved. >+ * Copyright (C) 2003-2019 Apple Inc. All rights reserved. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Library General Public >@@ -163,7 +163,7 @@ public: > > inline bool equal(ExecState*, JSString* other) const; > const String& value(ExecState*) const; >- inline const String& tryGetValue() const; >+ inline const String& tryGetValue(bool allocationAllowed = true) const; > const StringImpl* tryGetValueImpl() const; > ALWAYS_INLINE unsigned length() const { return m_length; } > >@@ -515,6 +515,8 @@ inline JSString* jsEmptyString(VM* vm) > > ALWAYS_INLINE JSString* jsSingleCharacterString(VM* vm, UChar c) > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(vm->heap.expectDoesGC()); > if (c <= maxSingleCharacterString) > return vm->smallStrings.singleCharacterString(c); > return JSString::create(*vm, StringImpl::create(&c, 1)); >@@ -539,6 +541,8 @@ ALWAYS_INLINE Identifier JSString::toIde > > ALWAYS_INLINE AtomicString JSString::toAtomicString(ExecState* exec) const > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(vm()->heap.expectDoesGC()); > if (isRope()) > static_cast<const JSRopeString*>(this)->resolveRopeToAtomicString(exec); > return AtomicString(m_value); >@@ -546,6 +550,8 @@ ALWAYS_INLINE AtomicString JSString::toA > > ALWAYS_INLINE RefPtr<AtomicStringImpl> JSString::toExistingAtomicString(ExecState* exec) const > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(vm()->heap.expectDoesGC()); > if (isRope()) > return static_cast<const JSRopeString*>(this)->resolveRopeToExistingAtomicString(exec); > if (m_value.impl()->isAtomic()) >@@ -555,17 +561,24 @@ ALWAYS_INLINE RefPtr<AtomicStringImpl> J > > inline const String& JSString::value(ExecState* exec) const > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(vm()->heap.expectDoesGC()); > if (isRope()) > static_cast<const JSRopeString*>(this)->resolveRope(exec); > return m_value; > } > >-inline const String& JSString::tryGetValue() const >+inline const String& JSString::tryGetValue(bool allocationAllowed) const > { >- if (isRope()) { >- // Pass nullptr for the ExecState so that resolveRope does not throw in the event of an OOM error. >- static_cast<const JSRopeString*>(this)->resolveRope(nullptr); >- } >+ if (allocationAllowed) { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(vm()->heap.expectDoesGC()); >+ if (isRope()) { >+ // Pass nullptr for the ExecState so that resolveRope does not throw in the event of an OOM error. >+ static_cast<const JSRopeString*>(this)->resolveRope(nullptr); >+ } >+ } else >+ RELEASE_ASSERT(!isRope()); > return m_value; > } > >@@ -739,6 +752,8 @@ inline bool isJSString(JSValue v) > > ALWAYS_INLINE StringView JSRopeString::unsafeView(ExecState* exec) const > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(vm()->heap.expectDoesGC()); > if (isSubstring()) { > if (is8Bit()) > return StringView(substringBase()->m_value.characters8() + substringOffset(), length()); >@@ -750,6 +765,8 @@ ALWAYS_INLINE StringView JSRopeString::u > > ALWAYS_INLINE StringViewWithUnderlyingString JSRopeString::viewWithUnderlyingString(ExecState* exec) const > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(vm()->heap.expectDoesGC()); > if (isSubstring()) { > auto& base = substringBase()->m_value; > if (is8Bit()) >@@ -762,6 +779,8 @@ ALWAYS_INLINE StringViewWithUnderlyingSt > > ALWAYS_INLINE StringView JSString::unsafeView(ExecState* exec) const > { >+ if (validateDFGDoesGC) >+ RELEASE_ASSERT(vm()->heap.expectDoesGC()); > if (isRope()) > return static_cast<const JSRopeString*>(this)->unsafeView(exec); > return m_value;
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=362559">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 193938
:
362432
|
362457
| 362559