Bug 192908

Summary: StrengthReductionPhase ValueAdd rule handles BigInt handleCommutativity is breaking JSC::DFG::Edge::useKind()
Product: WebKit Reporter: Caio Lima <ticaiolima>
Component: JavaScriptCoreAssignee: Caio Lima <ticaiolima>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch ysuzuki: review-

Caio Lima
Reported 2018-12-19 18:15:33 PST
Now, there are 35 tests failing into Debug builds: https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/1926/steps/jscore-test/logs/stdio
Attachments
Patch (1.64 KB, patch)
2018-12-19 18:19 PST, Caio Lima 		ysuzuki: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Caio Lima
Comment 1 2018-12-19 18:19:36 PST
Created attachment 357768 [details] Patch
Mark Lam
Comment 2 2018-12-19 20:26:13 PST
Comment on attachment 357768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357768&action=review > Source/JavaScriptCore/ChangeLog:10 > + Current rule is considering that both operands of ValueAdd have nodes, > + but this is not quite true. In such case, we are checking the > + existence of both nodes before using m_node->binaryUseKind. I don't understand: when would ValueAdd ever not have children nodes? Also, please provide a test case. The test will help illustrate the issue.
Mark Lam
Comment 3 2018-12-19 20:27:36 PST
Comment on attachment 357768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357768&action=review >> Source/JavaScriptCore/ChangeLog:10 >> + existence of both nodes before using m_node->binaryUseKind. > > I don't understand: when would ValueAdd ever not have children nodes? Also, please provide a test case. The test will help illustrate the issue. Oh, I see there are already failing tests. I'm still surprised that ValueAdd has edges that don't have nodes. Perhaps there's something I don't know.
Yusuke Suzuki
Comment 4 2018-12-19 23:24:49 PST
Comment on attachment 357768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357768&action=review >>> Source/JavaScriptCore/ChangeLog:10 >>> + existence of both nodes before using m_node->binaryUseKind. >> >> I don't understand: when would ValueAdd ever not have children nodes? Also, please provide a test case. The test will help illustrate the issue. > > Oh, I see there are already failing tests. I'm still surprised that ValueAdd has edges that don't have nodes. Perhaps there's something I don't know. Yeah, I don't think the current situation is right. ValueAdd must have two children. If it happens, it's a bug. We should fix that bug instead of checking `child1().node() && child2().node()`.
Caio Lima
Comment 5 2018-12-20 02:28:14 PST
Thx for the review. (In reply to Yusuke Suzuki from comment #4) > Comment on attachment 357768 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357768&action=review > > >>> Source/JavaScriptCore/ChangeLog:10 > >>> + existence of both nodes before using m_node->binaryUseKind. > >> > >> I don't understand: when would ValueAdd ever not have children nodes? Also, please provide a test case. The test will help illustrate the issue. > > > > Oh, I see there are already failing tests. I'm still surprised that ValueAdd has edges that don't have nodes. Perhaps there's something I don't know. > > Yeah, I don't think the current situation is right. ValueAdd must have two > children. > If it happens, it's a bug. We should fix that bug instead of checking > `child1().node() && child2().node()`. Makes sense. The bug happens when there are 2 constant Strings into ValueAdd and we perform the folding. When this happened we should not execute "m_node->binaryUseKind()". I'm closing this to fix into original Bug https://bugs.webkit.org/show_bug.cgi?id=192723 since we rolled this out.
Caio Lima
Comment 6 2018-12-20 02:28:44 PST
*** This bug has been marked as a duplicate of bug 192723 ***
Note You need to log in before you can comment on or make changes to this bug.