Summary: | [ftlopt] Call and Construct DFG nodes aren't always safe to execute | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matthew Mirman <mmirman> | ||||||||
Component: | JavaScriptCore | Assignee: | 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
Matthew Mirman
2014-06-09 15:15:52 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.
Nice! I will land this shortly... Sorry it's taking so long to land - I got some test failures, but I think they were spurious, so I'm rerunning... Landed in http://trac.webkit.org/changeset/169786 Lol, this broke 32-bit. I'll roll it out. Rolled out in http://trac.webkit.org/changeset/169793 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.
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 (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. (In reply to comment #9) > That would be amazing since this was landed on a branch. Good point. :) Sorry, I commented wrong bug report. 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 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(). Created attachment 232917 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct
Comment on attachment 232917 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct
Running tests on this now, will land shortly...
Landed in http://trac.webkit.org/changeset/169864 |