- Revision
- 261325
- Author
- mark....@apple.com
- Date
- 2020-05-07 12:25:32 -0700 (Thu, 07 May 2020)
Log Message
Add stack checks to the DFG and FTL bytecode parser.
https://bugs.webkit.org/show_bug.cgi?id=211547
<rdar://problem/62958880>
Reviewed by Yusuke Suzuki.
Source/_javascript_Core:
Inlining can cause some level of recursion of the DFG bytecode parser. We should
do a stack check at each inlining check before recursing. If a stack overflow
appears to be imminent, then just refuse to inline, and therefore, don't recurse
deeper into the parser.
This issue is more noticeable on ASan debug builds where stack frames can be
humongous.
Removed the SUPPRESS_ASAN on cloberrize() and the associated comment from r260692.
It was a mis-diagnosis. The stack checks are what we need.
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleVarargsInlining):
(JSC::DFG::ByteCodeParser::handleInlining):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGGraph.h:
Source/WTF:
Added a StackCheck::Scope RAII object to help verify that the default reserved
zone size is at least adequate for known work loads. If this the StackCheck::Scope
assertions fail, then we either need more stack checks, or the reserved zone size
needs to be increased.
Note that the assertions are usually only on in Debug builds. Ideally, we would
want to measure the reserved zone size with a Release build. To do that, we
can just set VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE to 1 unconditionally in
StackCheck.h and rebuild.
* wtf/StackCheck.h:
(WTF::StackCheck::Scope::Scope):
(WTF::StackCheck::Scope::~Scope):
(WTF::StackCheck::Scope::isSafeToRecurse):
(WTF::StackCheck::StackCheck):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (261324 => 261325)
--- trunk/Source/_javascript_Core/ChangeLog 2020-05-07 19:12:30 UTC (rev 261324)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-05-07 19:25:32 UTC (rev 261325)
@@ -1,3 +1,29 @@
+2020-05-07 Mark Lam <mark....@apple.com>
+
+ Add stack checks to the DFG and FTL bytecode parser.
+ https://bugs.webkit.org/show_bug.cgi?id=211547
+ <rdar://problem/62958880>
+
+ Reviewed by Yusuke Suzuki.
+
+ Inlining can cause some level of recursion of the DFG bytecode parser. We should
+ do a stack check at each inlining check before recursing. If a stack overflow
+ appears to be imminent, then just refuse to inline, and therefore, don't recurse
+ deeper into the parser.
+
+ This issue is more noticeable on ASan debug builds where stack frames can be
+ humongous.
+
+ Removed the SUPPRESS_ASAN on cloberrize() and the associated comment from r260692.
+ It was a mis-diagnosis. The stack checks are what we need.
+
+ * dfg/DFGByteCodeParser.cpp:
+ (JSC::DFG::ByteCodeParser::handleVarargsInlining):
+ (JSC::DFG::ByteCodeParser::handleInlining):
+ * dfg/DFGClobberize.h:
+ (JSC::DFG::clobberize):
+ * dfg/DFGGraph.h:
+
2020-05-07 Darin Adler <da...@apple.com>
REGRESSION (r261257): Lifetime problem with upconverted characters in toLocaleCase
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (261324 => 261325)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2020-05-07 19:12:30 UTC (rev 261324)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2020-05-07 19:25:32 UTC (rev 261325)
@@ -1926,6 +1926,12 @@
NodeType callOp, InlineCallFrame::Kind kind)
{
VERBOSE_LOG("Handling inlining (Varargs)...\nStack: ", currentCodeOrigin(), "\n");
+
+ StackCheck::Scope stackChecker(m_graph.m_stackChecker);
+ if (!stackChecker.isSafeToRecurse()) {
+ VERBOSE_LOG("Bailing inlining (compiler thread stack overflow eminent).\nStack: ", currentCodeOrigin(), "\n");
+ return false;
+ }
if (callLinkStatus.maxArgumentCountIncludingThis() > Options::maximumVarargsForInlining()) {
VERBOSE_LOG("Bailing inlining: too many arguments for varargs inlining.\n");
return false;
@@ -2070,6 +2076,12 @@
BytecodeIndex osrExitIndex, NodeType callOp, InlineCallFrame::Kind kind, SpeculatedType prediction)
{
VERBOSE_LOG("Handling inlining...\nStack: ", currentCodeOrigin(), "\n");
+
+ StackCheck::Scope stackChecker(m_graph.m_stackChecker);
+ if (!stackChecker.isSafeToRecurse()) {
+ VERBOSE_LOG("Bailing inlining (compiler thread stack overflow eminent).\nStack: ", currentCodeOrigin(), "\n");
+ return CallOptimizationResult::DidNothing;
+ }
CodeSpecializationKind specializationKind = InlineCallFrame::specializationKindFor(kind);
unsigned inliningBalance = getInliningBalance(callLinkStatus, specializationKind);
Modified: trunk/Source/_javascript_Core/dfg/DFGClobberize.h (261324 => 261325)
--- trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2020-05-07 19:12:30 UTC (rev 261324)
+++ trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2020-05-07 19:25:32 UTC (rev 261325)
@@ -39,25 +39,8 @@
namespace JSC { namespace DFG {
-// FIXME: SUPPRESS_ASAN is needed here because ASan can mistakenly think that
-// we're accesing out of invalid bounds stack memory when we're not. For example,
-// in the CheckIsConstant case below, we compute:
-// AdjacencyList(AdjacencyList::Fixed, node->child1())
-//
-// 1. The AdjacencyList constructor takes an Edge value.
-// 2. node->child1() returns an Edge&.
-// 3. Clang generates a call to __asan_memcpy to copy the return value of
-// node->child1() to a temp local on the stack used for passing the Edge
-// argument to the AdjacencyList constructor.
-// 4. Inside __asan_memcpy, it attempts to write to the temp local Edge in
-// clobberize's frame (not __asan_memcpy's frame), and ASan will wrongly
-// flag this as an invalid out of stack bounds write.
-//
-// This manifested with a debug ASan build.
-// See <rdar://problem/62362403>.
-
template<typename ReadFunctor, typename WriteFunctor, typename DefFunctor>
-SUPPRESS_ASAN void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFunctor& write, const DefFunctor& def)
+void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFunctor& write, const DefFunctor& def)
{
clobberize(graph, node, read, write, def, [] { });
}
Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.h (261324 => 261325)
--- trunk/Source/_javascript_Core/dfg/DFGGraph.h 2020-05-07 19:12:30 UTC (rev 261324)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.h 2020-05-07 19:25:32 UTC (rev 261325)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2011-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2020 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -41,9 +41,10 @@
#include "MethodOfGettingAValueProfile.h"
#include <wtf/BitVector.h>
#include <wtf/HashMap.h>
-#include <wtf/Vector.h>
+#include <wtf/StackCheck.h>
#include <wtf/StdLibExtras.h>
#include <wtf/StdUnorderedMap.h>
+#include <wtf/Vector.h>
namespace WTF {
template <typename T> class SingleRootGraph;
@@ -1065,11 +1066,12 @@
Prefix& prefix() { return m_prefix; }
void nextPhase() { m_prefix.phaseNumber++; }
+ StackCheck m_stackChecker;
VM& m_vm;
Plan& m_plan;
CodeBlock* m_codeBlock;
CodeBlock* m_profiledBlock;
-
+
Vector<RefPtr<BasicBlock>, 8> m_blocks;
Vector<BasicBlock*, 1> m_roots;
Vector<Edge, 16> m_varArgChildren;
Modified: trunk/Source/WTF/ChangeLog (261324 => 261325)
--- trunk/Source/WTF/ChangeLog 2020-05-07 19:12:30 UTC (rev 261324)
+++ trunk/Source/WTF/ChangeLog 2020-05-07 19:25:32 UTC (rev 261325)
@@ -1,3 +1,27 @@
+2020-05-07 Mark Lam <mark....@apple.com>
+
+ Add stack checks to the DFG and FTL bytecode parser.
+ https://bugs.webkit.org/show_bug.cgi?id=211547
+ <rdar://problem/62958880>
+
+ Reviewed by Yusuke Suzuki.
+
+ Added a StackCheck::Scope RAII object to help verify that the default reserved
+ zone size is at least adequate for known work loads. If this the StackCheck::Scope
+ assertions fail, then we either need more stack checks, or the reserved zone size
+ needs to be increased.
+
+ Note that the assertions are usually only on in Debug builds. Ideally, we would
+ want to measure the reserved zone size with a Release build. To do that, we
+ can just set VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE to 1 unconditionally in
+ StackCheck.h and rebuild.
+
+ * wtf/StackCheck.h:
+ (WTF::StackCheck::Scope::Scope):
+ (WTF::StackCheck::Scope::~Scope):
+ (WTF::StackCheck::Scope::isSafeToRecurse):
+ (WTF::StackCheck::StackCheck):
+
2020-05-06 Yoshiaki Jitsukawa <yoshiaki.jitsuk...@sony.com> and Fujii Hironori <hironori.fu...@sony.com>
[Win] Implement DisplayRefreshMonitor by using RunLoop::Timer
Modified: trunk/Source/WTF/wtf/StackCheck.h (261324 => 261325)
--- trunk/Source/WTF/wtf/StackCheck.h 2020-05-07 19:12:30 UTC (rev 261324)
+++ trunk/Source/WTF/wtf/StackCheck.h 2020-05-07 19:25:32 UTC (rev 261325)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2019-2020 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -30,17 +30,85 @@
namespace WTF {
+// We only enable the reserved zone size check by default on ASSERT_ENABLED
+// builds (which usually mean Debug builds). However, it is more valuable to
+// run this test on Release builds. That said, we don't want to do pay this
+// cost always because we may need to do stack checks on hot paths too.
+// Note also that we're deliberately using RELEASE_ASSERTs if the verification
+// is turned on. This makes it easier for us to turn this on for Release builds
+// for a reserved zone size calibration test runs.
+
+#define VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE ASSERT_ENABLED
+
class StackCheck {
WTF_MAKE_FAST_ALLOCATED;
public:
- StackCheck(const StackBounds& bounds = Thread::current().stack(), size_t minReservedZone = StackBounds::DefaultReservedZone)
+ class Scope {
+ public:
+ Scope(StackCheck& checker)
+ : m_checker(checker)
+ {
+ m_checker.isSafeToRecurse();
+#if VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE
+ RELEASE_ASSERT(checker.m_ownerThread == &Thread::current());
+
+ // Make sure that the stack interval between checks never exceed the
+ // reservedZone size. If the interval exceeds the reservedZone size,
+ // then we either need to:
+ // 1. break the interval into smaller pieces (i.e. insert more checks), or
+ // 2. use a larger reservedZone size.
+ uint8_t* currentStackCheckpoint = reinterpret_cast<uint8_t*>(currentStackPointer());
+ uint8_t* previousStackCheckpoint = m_checker.m_lastStackCheckpoint;
+ RELEASE_ASSERT(previousStackCheckpoint - currentStackCheckpoint > 0);
+ RELEASE_ASSERT(previousStackCheckpoint - currentStackCheckpoint < static_cast<ptrdiff_t>(m_checker.m_reservedZone));
+
+ m_savedLastStackCheckpoint = m_checker.m_lastStackCheckpoint;
+ m_checker.m_lastStackCheckpoint = currentStackCheckpoint;
+#endif
+ }
+
+#if VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE
+ ~Scope()
+ {
+ m_checker.m_lastStackCheckpoint = m_savedLastStackCheckpoint;
+ }
+#endif
+
+ bool isSafeToRecurse() { return m_checker.isSafeToRecurse(); }
+
+ private:
+ StackCheck& m_checker;
+#if VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE
+ uint8_t* m_savedLastStackCheckpoint;
+#endif
+ };
+
+ StackCheck(const StackBounds& bounds = Thread::current().stack(), size_t minReservedZone = defaultReservedZoneSize)
: m_stackLimit(bounds.recursionLimit(minReservedZone))
+#if VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE
+ , m_ownerThread(&Thread::current())
+ , m_lastStackCheckpoint(reinterpret_cast<uint8_t*>(currentStackPointer()))
+ , m_reservedZone(minReservedZone)
+#endif
{ }
inline bool isSafeToRecurse() { return currentStackPointer() >= m_stackLimit; }
private:
+#if ASAN_ENABLED
+ static constexpr size_t defaultReservedZoneSize = StackBounds::DefaultReservedZone * 2;
+#else
+ static constexpr size_t defaultReservedZoneSize = StackBounds::DefaultReservedZone;
+#endif
+
void* m_stackLimit;
+#if VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE
+ Thread* m_ownerThread;
+ uint8_t* m_lastStackCheckpoint;
+ size_t m_reservedZone;
+#endif
+
+ friend class Scope;
};
} // namespace WTF
