143396 – In the 64-bit DFG and FTL, Array::Double case for HasIndexedProperty should set its result to true when all is well

Bug 143396 - In the 64-bit DFG and FTL, Array::Double case for HasIndexedProperty should set its result to true when all is well

Summary: In the 64-bit DFG and FTL, Array::Double case for HasIndexedProperty should s...

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Mark Lam

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2015-04-03 19:17 PDT by Mark Lam
Modified:	2015-04-06 15:49 PDT (History)
CC List:	6 users (show)

See Also:

Attachments
the patch. (6.66 KB, patch) 2015-04-06 14:44 PDT, Mark Lam	fpizlo: review-	Details \| Formatted Diff \| Diff
patch 2: with Fil's better doubles comparison. (6.67 KB, patch) 2015-04-06 14:53 PDT, Mark Lam	fpizlo: review+	Details \| Formatted Diff \| Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Mark Lam 2015-04-03 19:17:28 PDT

Currently, all other cases do set the result to true when in the successful path.  Only the Array::Double case has neglected to do so.  This bug does is only present in the 64-bit DFG.

Comment 1 Mark Lam 2015-04-03 19:20:24 PDT

<rdar://problem/20096787>

Comment 2 Mark Lam 2015-04-06 14:19:16 PDT

Turns out that the FTL implementation of HasIndexedProperty has a similar but different problem.  In this case, the FTL is setting the boolean result.  However, the value that it is setting it with needs to be inverted.

Michael helped me out with figuring out how to invert said bit.  My tests appear to be passing now.  Patch for fix coming soon.

Comment 3 Mark Lam 2015-04-06 14:44:34 PDT

Created attachment 250226 [details]
the patch.

Comment 4 Filip Pizlo 2015-04-06 14:48:03 PDT

Comment on attachment 250226 [details]
the patch.

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4712
> -                m_out.doubleNotEqualOrUnordered(doubleValue, doubleValue));
> -            m_out.branch(checkHoleResult.value(), rarely(slowCase), usually(continuation));
> +                m_out.bitNot(m_out.doubleNotEqualOrUnordered(doubleValue, doubleValue)));
> +            m_out.branch(checkHoleResult.value(), usually(continuation), rarely(slowCase));

You should just use doubleEqual() instead of bitNot(doubleNotEqualOrUnordered)

Comment 5 Mark Lam 2015-04-06 14:53:23 PDT

Created attachment 250228 [details]
patch 2: with Fil's better doubles comparison.

Comment 6 Mark Lam 2015-04-06 15:49:24 PDT

Thanks.  Landed in r182444: <http://trac.webkit.org/r182444>.