Bug 182249

Summary: REGRESSION (r227341): DFG_ASSERT failure at JSC::DFG::AtTailAbstractState::forNode()
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=184452
https://bugs.webkit.org/show_bug.cgi?id=184455
Attachments:
Description Flags
Patch
none
Patch with updates from review none

Description Michael Saboff 2018-01-29 10:17:51 PST
Change set r227341 (<https://trac.webkit.org/changeset/227341>) changed the abstract interpreter for the non-strict Compare nodes like CompareEq.  That change caused a DFG_ASSERT() to fire:

DFG ASSERTION FAILED: iter != valuesAtTail.end()
./dfg/DFGAtTailAbstractState.cpp(59) : JSC::DFG::AbstractValue &JSC::DFG::AtTailAbstractState::forNode(JSC::DFG::NodeFlowProjection)

There needs to be a corresponding change in clobberize() for UntypedUse as well.
Comment 1 Michael Saboff 2018-01-29 10:18:15 PST
<rdar://problem/36833641>
Comment 2 Michael Saboff 2018-01-29 10:29:07 PST
Created attachment 332553 [details]
Patch
Comment 3 Keith Miller 2018-01-29 10:32:44 PST
Comment on attachment 332553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332553&action=review

> Source/JavaScriptCore/dfg/DFGClobberize.h:1550
> +        if (node->child1().useKind() == UntypedUse || node->child1().useKind() == ObjectUse
> +            || node->child2().useKind() == UntypedUse || node->child2().useKind() == ObjectUse) {

You should have a check for node->isBinaryUseKind(ObjectUse) since that won't convert.

Nit: Also, I would make a local for the useKinds...
Comment 4 Michael Saboff 2018-01-29 10:46:54 PST
Created attachment 332556 [details]
Patch with updates from review
Comment 5 Keith Miller 2018-01-29 10:50:20 PST
Comment on attachment 332556 [details]
Patch with updates from review

r=me.
Comment 6 WebKit Commit Bot 2018-01-29 11:13:49 PST
Comment on attachment 332556 [details]
Patch with updates from review

Clearing flags on attachment: 332556

Committed r227742: <https://trac.webkit.org/changeset/227742>
Comment 7 WebKit Commit Bot 2018-01-29 11:13:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Filip Pizlo 2018-04-10 10:05:51 PDT
Comment on attachment 332556 [details]
Patch with updates from review

View in context: https://bugs.webkit.org/attachment.cgi?id=332556&action=review

> Source/JavaScriptCore/dfg/DFGClobberize.h:1560
> -        if (!node->isBinaryUseKind(UntypedUse)) {
> +
> +        if (node->op() == CompareEq && node->isBinaryUseKind(ObjectUse)) {
>              def(PureValue(node));
>              return;
>          }
> -        read(World);
> -        write(Heap);
> +        if (node->child1().useKind() == UntypedUse || node->child1().useKind() == ObjectUse
> +            || node->child2().useKind() == UntypedUse || node->child2().useKind() == ObjectUse) {
> +            read(World);
> +            write(Heap);
> +            return;
> +        }
> +
> +        def(PureValue(node));

This patch creates a major discrepancy between AI's handling of CompareEq and clobberize's handling of CompareEq.

Also, from what I can tell, it's only CompareEq(Untyped:, Untyped:) that can have effects.  CompareEq(ObjectUse:, _) cannot have effects.
Comment 9 Filip Pizlo 2018-04-10 10:15:23 PDT
I don't think it's correct to fix assertions in AtTailAbstractState by pretending that something is effectful.  It seems that all that this patch did was mask a more fundamental bug by turning off hoisting of CompareEq in this case.  But CompareEq is effectless here, so if hoisting it causes a problem then pretending that it's effectful is not the right solution.
Comment 10 Filip Pizlo 2018-04-10 10:18:00 PDT
I see the problem now: in r227341, you incorrectly made ComapreEq(Untyped:, Other:) pretend to have effect even though it doesn't.  But this means that attempting to hoist it causes clobberWorld() to get called with AtTailAbstractState, which causes crashes.

The correct solution is to fix AI so that it's precise about the effectfulness of CompareEq(Untyped:, Other:).