Bug 170961 – r211670 broke double to int conversion.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

This is because operationToInt32SensibleSlow() assumes that left shifts of greater than 31 bits on an 31-bit value will produce a 0.  However, the spec says that "if the value of the right operand is negative or is greater or equal to the number of bits in the promoted left operand, the behavior is undefined."  See http://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators.

This patch fixes this by restoring the check to prevent a shift of greater than 31 bits.
It also consolidates the optimization in operationToInt32SensibleSlow() back into toInt32() so that we don't have 2 copies of the same code with only a slight variation.

[Radar WebKit Bug Importer]

<rdar://problem/31687696>

[Yusuke Suzuki]

(In reply to Mark Lam from comment #0)
> This is because operationToInt32SensibleSlow() assumes that left shifts of
> greater than 31 bits on an 31-bit value will produce a 0.  However, the spec
> says that "if the value of the right operand is negative or is greater or
> equal to the number of bits in the promoted left operand, the behavior is
> undefined."  See
> http://en.cppreference.com/w/cpp/language/
> operator_arithmetic#Bitwise_shift_operators.

Oops! Right.

> 
> This patch fixes this by restoring the check to prevent a shift of greater
> than 31 bits.

Yup, I think adding the following check,

if (exp > 83)
    return 0;

will solve.

> It also consolidates the optimization in operationToInt32SensibleSlow() back
> into toInt32() so that we don't have 2 copies of the same code with only a
> slight variation.

Hm, my concern is that there is no way to emit cvttss2si_rr like thing in an architecture independent way (like, emitting this semantics operation in ARM64).

[Mark Lam]

Created attachment 307406 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 307406 [details]
proposed patch.

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

r=me if it does not regress DoubleAsInt32.

> Source/JavaScriptCore/runtime/MathCommon.h:93
> +    if (static_cast<uint32_t>(exp) > 83u)

Nice!

[Build Bot]

Comment on attachment 307406 [details]
proposed patch.

Attachment 307406 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3559103

New failing tests:
http/tests/inspector/network/resource-sizes-network.html

[Build Bot]

Created attachment 307421 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Mark Lam]

Created attachment 307423 [details]
patch for landing.

I added a microbenchmark to test that the r211670 progression is not negated.  It is not i.e. this patch does not appear to regress performance.  Will land shortly.

[Mark Lam]

Thanks for the review.  Landed in r215482: <http://trac.webkit.org/r215482>.

[Yusuke Suzuki]

(In reply to Mark Lam from comment #8)
> Thanks for the review.  Landed in r215482: <http://trac.webkit.org/r215482>.

Hmmm, it seems that it causes 5% regression in pbkdf2.
https://arewefastyet.com/#machine=29&view=single&suite=kraken&subtest=crypto-pbkdf2

I've just checked out r215482, and compared it with the r215482 + this patch's change version.

Benchmark report for Kraken on sakura-trick.

VMs tested:
"baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/int32sensible/Release/bin/jsc
"patched" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/int32sensible2/Release/bin/jsc

Collected 30 samples per benchmark/VM, with 30 VM invocations per benchmark. Emitted a call to gc()
between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the
jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times
with 95% confidence intervals in milliseconds.

                                 baseline                  patched

stanford-crypto-pbkdf2       147.383+-4.238      !     161.362+-5.761         ! definitely 1.0948x slower

<arithmetic>                 147.383+-4.238      !     161.362+-5.761         ! definitely 1.0948x slower


So, I suggest sharing the implementation by using the template. I'll upload the patch.

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 307472 [details]
Patch

[Mark Lam]

Comment on attachment 307472 [details]
Patch

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

r=me with fixes.

> Source/JavaScriptCore/ChangeLog:11
> +        and fixes kraken pbkdf2 regression.

Can you add "on linux" after "regression" to clarify where this can be observed?

> Source/JavaScriptCore/runtime/MathCommon.h:36
> +int32_t JIT_OPERATION operationToInt32SensibleSlow(double) WTF_INTERNAL;

You need to revert the DFG and FTL code back to using operationToInt32SensibleSlow() too.

> Source/JavaScriptCore/runtime/MathCommon.h:156
> +    return bits < 0 ? -static_cast<int32_t>(result) : static_cast<int32_t>(result);

Let's make bits uint64_t and only cast it to int64_t here instead for the sign check.  This will reduce the amount of casting in this function.  It also makes it consistent that we do all our bit manipulation on unsigned bits, and only re-apply the sign at the end.

[Yusuke Suzuki]

Comment on attachment 307472 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:11
>> +        and fixes kraken pbkdf2 regression.
> 
> Can you add "on linux" after "regression" to clarify where this can be observed?

Fixed.

>> Source/JavaScriptCore/runtime/MathCommon.h:36
>> +int32_t JIT_OPERATION operationToInt32SensibleSlow(double) WTF_INTERNAL;
> 
> You need to revert the DFG and FTL code back to using operationToInt32SensibleSlow() too.

Oops, right. I moved modified version of MathCommon.[h,cpp] from the old revision and forgot to bring the changes in FTL and DFG.

>> Source/JavaScriptCore/runtime/MathCommon.h:156
>> +    return bits < 0 ? -static_cast<int32_t>(result) : static_cast<int32_t>(result);
> 
> Let's make bits uint64_t and only cast it to int64_t here instead for the sign check.  This will reduce the amount of casting in this function.  It also makes it consistent that we do all our bit manipulation on unsigned bits, and only re-apply the sign at the end.

OK, fixed.

[Yusuke Suzuki]

Committed r215516: <http://trac.webkit.org/changeset/215516>

[Yusuke Suzuki]

OK, perf is recovered.
https://arewefastyet.com/#machine=29&view=single&suite=kraken&subtest=crypto-pbkdf2