- Revision
- 242425
- Author
- carlo...@webkit.org
- Date
- 2019-03-05 00:43:55 -0800 (Tue, 05 Mar 2019)
Log Message
Merge r241849 - 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):
Modified Paths
Diff
Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog (242424 => 242425)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog 2019-03-05 08:43:49 UTC (rev 242424)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog 2019-03-05 08:43:55 UTC (rev 242425)
@@ -1,3 +1,61 @@
+2019-02-20 Mark Lam <mark....@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-18 Mark Lam <mark....@apple.com>
Fix DFG doesGC() for CompareEq/Less/LessEq/Greater/GreaterEq and CompareStrictEq nodes.
Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/dfg/DFGOSRExit.cpp (242424 => 242425)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/dfg/DFGOSRExit.cpp 2019-03-05 08:43:49 UTC (rev 242424)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/dfg/DFGOSRExit.cpp 2019-03-05 08:43:55 UTC (rev 242425)
@@ -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 @@
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 @@
}
}
+ 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.
Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (242424 => 242425)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2019-03-05 08:43:49 UTC (rev 242424)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2019-03-05 08:43:55 UTC (rev 242425)
@@ -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::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
Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (242424 => 242425)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2019-03-05 08:43:49 UTC (rev 242424)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2019-03-05 08:43:55 UTC (rev 242425)
@@ -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 @@
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();
Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ftl/FTLOSRExitCompiler.cpp (242424 => 242425)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ftl/FTLOSRExitCompiler.cpp 2019-03-05 08:43:49 UTC (rev 242424)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ftl/FTLOSRExitCompiler.cpp 2019-03-05 08:43:55 UTC (rev 242425)
@@ -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 @@
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();
Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/heap/Heap.h (242424 => 242425)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/heap/Heap.h 2019-03-05 08:43:49 UTC (rev 242424)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/heap/Heap.h 2019-03-05 08:43:55 UTC (rev 242425)
@@ -1,7 +1,7 @@
/*
* Copyright (C) 1999-2000 Harri Porten (por...@kde.org)
* Copyright (C) 2001 Peter Kelly (p...@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 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 @@
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 @@
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;
Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/jit/JITArithmetic.cpp (242424 => 242425)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/jit/JITArithmetic.cpp 2019-03-05 08:43:49 UTC (rev 242424)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/jit/JITArithmetic.cpp 2019-03-05 08:43:55 UTC (rev 242425)
@@ -179,6 +179,7 @@
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));
@@ -185,7 +186,7 @@
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 @@
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)) {
Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/JSCellInlines.h (242424 => 242425)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/JSCellInlines.h 2019-03-05 08:43:49 UTC (rev 242424)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/JSCellInlines.h 2019-03-05 08:43:55 UTC (rev 242425)
@@ -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 @@
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));
Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/JSString.h (242424 => 242425)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/JSString.h 2019-03-05 08:43:49 UTC (rev 242424)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/JSString.h 2019-03-05 08:43:55 UTC (rev 242425)
@@ -1,7 +1,7 @@
/*
* Copyright (C) 1999-2001 Harri Porten (por...@kde.org)
* Copyright (C) 2001 Peter Kelly (p...@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 @@
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 @@
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 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 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 @@
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 @@
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 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 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;
