Bug 133660

Summary: [ftlopt] Call and Construct DFG nodes aren't always safe to execute
Product: WebKit Reporter: Matthew Mirman <mmirman>
Component: JavaScriptCoreAssignee: Matthew Mirman <mmirman>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, mmirman, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Split Call and Construct DFG nodes into NativeCall and NativeConstruct
fpizlo: review+
Split Call and Construct DFG nodes into NativeCall and NativeConstruct
fpizlo: review-, fpizlo: commit-queue-
Split Call and Construct DFG nodes into NativeCall and NativeConstruct fpizlo: review+

Description Matthew Mirman 2014-06-09 15:15:52 PDT
Also needs native calling without patchpoints when not inlining in the ftl.
Comment 1 Matthew Mirman 2014-06-09 15:22:29 PDT
Created attachment 232738 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct

Ran webkit tests without new or relevant failures.

Should also increase the speed a bit by removing patch-points for calls which are speculated to be native by the DFG.
Comment 2 Filip Pizlo 2014-06-09 21:27:35 PDT
Nice!

I will land this shortly...
Comment 3 Filip Pizlo 2014-06-10 18:20:09 PDT
Sorry it's taking so long to land - I got some test failures, but I think they were spurious, so I'm rerunning...
Comment 4 Filip Pizlo 2014-06-10 18:52:42 PDT
Landed in http://trac.webkit.org/changeset/169786
Comment 5 Filip Pizlo 2014-06-10 21:30:21 PDT
Lol, this broke 32-bit.  I'll roll it out.
Comment 6 Filip Pizlo 2014-06-10 21:32:04 PDT
Rolled out in http://trac.webkit.org/changeset/169793
Comment 7 Filip Pizlo 2014-06-10 21:32:32 PDT
Comment on attachment 232738 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct

This is obsolete - this patch was landed but then rolled out because it broke internal run-javascriptcore-tests --32-bit.
Comment 9 Filip Pizlo 2014-06-11 00:01:35 PDT
(In reply to comment #8)
> and it made layout tests crash too on public Apple Mac bots:
> - http://build.webkit.org/builders/Apple%20Mavericks%20Release%20WK1%20%28Tests%29/builds/6543
> - http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20%28Tests%29/builds/5806

That would be amazing since this was landed on a branch.
Comment 10 Csaba Osztrogonác 2014-06-11 00:03:56 PDT
(In reply to comment #9)
> That would be amazing since this was landed on a branch.
Good point. :) Sorry, I commented wrong bug report.
Comment 11 Matthew Mirman 2014-06-11 11:49:54 PDT
Created attachment 232879 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct 

Passed jsc tests on 32 bit and 64 bit.  Should be applied after https://bug-133718-attachments.webkit.org/attachment.cgi?id=232878
Comment 12 Filip Pizlo 2014-06-11 13:00:43 PDT
Comment on attachment 232879 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct 

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

> Source/JavaScriptCore/dfg/DFGSafeToExecute.h:261
> +        return false; // TODO: add a check for already checked. 

Can you file a bugzilla bug for this and reference it here?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:644
> -    if (node->op() != Call)
> -        ASSERT(node->op() == Construct);
> +    bool isCall = node->op() == Call || node->op() == NativeCall;
> +    if (!isCall)
> +        ASSERT(node->op() == Construct || node->op() == NativeConstruct);

Why does this check for NativeCall/NativeConstruct?  The DFG backend shouldn't see them.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4131
> +    case NativeCall:
> +    case NativeConstruct:

Seems like these cases should be RELEASE_ASSERT_NOT_REACHED().

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:631
> -    if (node->op() != Call)
> -        RELEASE_ASSERT(node->op() == Construct);
> +
> +    bool isCall = node->op() == Call || node->op() == NativeCall;
> +    if (!isCall)
> +        RELEASE_ASSERT(node->op() == Construct || node->op() == NativeConstruct);

Why does this check for NativeCall/NativeConstruct?  The DFG backend shouldn't see them.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4163
> +    case NativeCall:
> +    case NativeConstruct:

Seems like these cases should be RELEASE_ASSERT_NOT_REACHED().
Comment 13 Matthew Mirman 2014-06-11 15:45:45 PDT
Created attachment 232917 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct
Comment 14 Filip Pizlo 2014-06-11 16:42:04 PDT
Comment on attachment 232917 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct 

Running tests on this now, will land shortly...
Comment 15 Filip Pizlo 2014-06-11 17:25:44 PDT
Landed in http://trac.webkit.org/changeset/169864