Title: [241140] trunk/Source/_javascript_Core
- Revision
- 241140
- Author
- [email protected]
- Date
- 2019-02-07 12:20:23 -0800 (Thu, 07 Feb 2019)
Log Message
Fix more doesGC() for CheckTraps, GetMapBucket, and Switch nodes.
https://bugs.webkit.org/show_bug.cgi?id=194399
<rdar://problem/47889777>
Reviewed by Yusuke Suzuki.
Fix doesGC() for the following nodes:
CheckTraps:
We normally will not emit this node because Options::usePollingTraps() is
false by default. However, as it is implemented now, CheckTraps can GC
because it can allocate a TerminatedExecutionException. If we make the
TerminatedExecutionException a singleton allocated at initialization time,
doesGC() can return false for CheckTraps.
https://bugs.webkit.org/show_bug.cgi?id=194323
GetMapBucket:
Can call operationJSMapFindBucket() or operationJSSetFindBucket(),
which calls HashMapImpl::findBucket(), which calls jsMapHash(), which
can resolve a rope.
Switch:
If switchData kind is SwitchChar, can call operationResolveRope() .
If switchData kind is SwitchString and the child use kind is not StringIdentUse,
can call operationSwitchString() which resolves ropes.
DirectTailCall:
ForceOSRExit:
Return:
TailCallForwardVarargs:
TailCallVarargs:
Throw:
These are terminal nodes. It shouldn't really matter what doesGC() returns
for them, but following our conservative practice, unless we have a good
reason for doesGC() to return false, we should just return true.
* dfg/DFGDoesGC.cpp:
(JSC::DFG::doesGC):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (241139 => 241140)
--- trunk/Source/_javascript_Core/ChangeLog 2019-02-07 20:19:43 UTC (rev 241139)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-02-07 20:20:23 UTC (rev 241140)
@@ -1,3 +1,44 @@
+2019-02-07 Mark Lam <[email protected]>
+
+ Fix more doesGC() for CheckTraps, GetMapBucket, and Switch nodes.
+ https://bugs.webkit.org/show_bug.cgi?id=194399
+ <rdar://problem/47889777>
+
+ Reviewed by Yusuke Suzuki.
+
+ Fix doesGC() for the following nodes:
+
+ CheckTraps:
+ We normally will not emit this node because Options::usePollingTraps() is
+ false by default. However, as it is implemented now, CheckTraps can GC
+ because it can allocate a TerminatedExecutionException. If we make the
+ TerminatedExecutionException a singleton allocated at initialization time,
+ doesGC() can return false for CheckTraps.
+ https://bugs.webkit.org/show_bug.cgi?id=194323
+
+ GetMapBucket:
+ Can call operationJSMapFindBucket() or operationJSSetFindBucket(),
+ which calls HashMapImpl::findBucket(), which calls jsMapHash(), which
+ can resolve a rope.
+
+ Switch:
+ If switchData kind is SwitchChar, can call operationResolveRope() .
+ If switchData kind is SwitchString and the child use kind is not StringIdentUse,
+ can call operationSwitchString() which resolves ropes.
+
+ DirectTailCall:
+ ForceOSRExit:
+ Return:
+ TailCallForwardVarargs:
+ TailCallVarargs:
+ Throw:
+ These are terminal nodes. It shouldn't really matter what doesGC() returns
+ for them, but following our conservative practice, unless we have a good
+ reason for doesGC() to return false, we should just return true.
+
+ * dfg/DFGDoesGC.cpp:
+ (JSC::DFG::doesGC):
+
2019-02-07 Robin Morisset <[email protected]>
B3ReduceStrength: missing peephole optimizations for Neg and Sub
Modified: trunk/Source/_javascript_Core/dfg/DFGDoesGC.cpp (241139 => 241140)
--- trunk/Source/_javascript_Core/dfg/DFGDoesGC.cpp 2019-02-07 20:19:43 UTC (rev 241139)
+++ trunk/Source/_javascript_Core/dfg/DFGDoesGC.cpp 2019-02-07 20:20:23 UTC (rev 241140)
@@ -41,8 +41,19 @@
return true;
// Now consider nodes that don't clobber the world but that still may GC. This includes all
- // nodes. By convention we put world-clobbering nodes in the block of "false" cases but we can
- // put them anywhere.
+ // nodes. By default, we should assume every node can GC and return true. This includes the
+ // world-clobbering nodes. We should only return false if we have proven that the node cannot
+ // GC. Typical examples of how a node can GC is if the code emitted for the node does any of the
+ // following:
+ // 1. Allocates any objects.
+ // 2. Resolves a rope string, which allocates a string.
+ // 3. Produces a string (which allocates the string) except when we can prove that
+ // the string will always be one of the pre-allcoated SmallStrings.
+ // 4. Triggers a structure transition (which can allocate a new structure)
+ // unless it is a known transition between previously allocated structures
+ // such as between Array types.
+ // 5. Calls to a JS function, which can execute arbitrary code including allocating objects.
+
switch (node->op()) {
case JSConstant:
case DoubleConstant:
@@ -130,7 +141,6 @@
case CompareEq:
case CompareStrictEq:
case CompareEqPtr:
- case TailCallForwardVarargs:
case ProfileType:
case ProfileControlFlow:
case OverridesHasInstance:
@@ -149,20 +159,12 @@
case LogicalNot:
case Jump:
case Branch:
- case Switch:
case EntrySwitch:
- case Return:
- case DirectTailCall:
- case TailCallVarargs:
- case Throw:
case CountExecution:
case SuperSamplerBegin:
case SuperSamplerEnd:
- case ForceOSRExit:
case CPUIntrinsic:
- case CheckTraps:
case NormalizeMapKey:
- case GetMapBucket:
case GetMapBucketHead:
case GetMapBucketNext:
case LoadKeyFromMapBucket:
@@ -257,6 +259,7 @@
case DataViewSet:
return false;
+#if !ASSERT_DISABLED
case ArrayPush:
case ArrayPop:
case PushWithScope:
@@ -278,7 +281,9 @@
case DeleteByVal:
case DirectCall:
case DirectConstruct:
+ case DirectTailCall:
case DirectTailCallInlinedCaller:
+ case ForceOSRExit:
case GetById:
case GetByIdDirect:
case GetByIdDirectFlush:
@@ -287,6 +292,7 @@
case GetByValWithThis:
case GetDirectPname:
case GetDynamicVar:
+ case GetMapBucket:
case HasGenericProperty:
case HasOwnProperty:
case HasStructureProperty:
@@ -318,10 +324,14 @@
case RegExpTest:
case ResolveScope:
case ResolveScopeForHoistingFuncDeclInEval:
+ case Return:
case TailCall:
+ case TailCallForwardVarargs:
case TailCallForwardVarargsInlinedCaller:
case TailCallInlinedCaller:
+ case TailCallVarargs:
case TailCallVarargsInlinedCaller:
+ case Throw:
case ToNumber:
case ToObject:
case ToPrimitive:
@@ -378,6 +388,11 @@
case ValueMul:
case ValueDiv:
case ValueNegate:
+#else
+ // See comment at the top for why be default for all nodes should be to
+ // return true.
+ default:
+#endif
return true;
case CallStringConstructor:
@@ -391,6 +406,11 @@
}
return true;
+ case CheckTraps:
+ // FIXME: https://bugs.webkit.org/show_bug.cgi?id=194323
+ ASSERT(Options::usePollingTraps());
+ return true;
+
case GetIndexedPropertyStorage:
if (node->arrayMode().type() == Array::String)
return true;
@@ -423,6 +443,22 @@
return false;
return true;
+ case Switch:
+ switch (node->switchData()->kind) {
+ case SwitchCell:
+ ASSERT(graph.m_plan.isFTL());
+ FALLTHROUGH;
+ case SwitchImm:
+ return false;
+ case SwitchChar:
+ return true;
+ case SwitchString:
+ if (node->child1().useKind() == StringIdentUse)
+ return false;
+ ASSERT(node->child1().useKind() == StringUse || node->child1().useKind() == UntypedUse);
+ return true;
+ }
+
case LastNodeType:
RELEASE_ASSERT_NOT_REACHED();
return true;
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
