Bug 192908 - StrengthReductionPhase ValueAdd rule handles BigInt handleCommutativity is breaking JSC::DFG::Edge::useKind()
Summary: StrengthReductionPhase ValueAdd rule handles BigInt handleCommutativity is br...
Status: RESOLVED DUPLICATE of bug 192723
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-19 18:15 PST by Caio Lima
Modified: 2018-12-20 02:28 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.64 KB, patch)
2018-12-19 18:19 PST, Caio Lima
ysuzuki: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Caio Lima 2018-12-19 18:19:36 PST
Created attachment 357768 [details]
Patch
Comment 2 Mark Lam 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.
Comment 3 Mark Lam 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.
Comment 4 Yusuke Suzuki 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()`.
Comment 5 Caio Lima 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.
Comment 6 Caio Lima 2018-12-20 02:28:44 PST

*** This bug has been marked as a duplicate of bug 192723 ***