Bug 129884 – Make Math.imul ES6 conform

• Component: JavaScriptCore
• Status: NEW

[Tibor Mészáros]

I had made some test about the current solution of this method, and I had found some problem with it:

The standard say:

- Let a be ToUint32(x).  >> the current conversion is: ToInt32(x)
- ReturnIfAbrupt(a).
- Let b be ToUint32(y). >> the current conversion is: ToInt32(x)
- ReturnIfAbrupt(b). >> there is no check for it.
- Let product be (a × b) modulo 2^32. >> the current count is: (a x b)
- If product ≥ 2<<31, return product − 2<<32, otherwise return product.

Failed test:

- If the second parameter is an exception, it will follow run, and try to do the (a x b), but it cant. So it must return after the thrown exception, instead of follow with next step.

[Tibor Mészáros]

Created attachment 226129 [details]
Patch v1

ES6 support patch for Math.imul

[Darin Adler]

Comment on attachment 226129 [details]
Patch v1

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

> Source/JavaScriptCore/ChangeLog:3
> +        Make Math.imul ES6 conform

This patch does not come along with sufficient test coverage for the exception code. We know that because there are bugs in it that I saw by code inspection, but no tests failed.

Also, this patch adds no tests at all. That violates the WebKit project policy. When fixing a bug we include a test that shows the bug and that it is now fixed.

> Source/JavaScriptCore/runtime/MathObject.cpp:289
>  EncodedJSValue JSC_HOST_CALL mathProtoFuncIMul(ExecState* exec)

Generally unclear what the algorithm here is trying to do. May need some more comments. Definitely need a lot more test coverage or we cannot be confident this is correct.

> Source/JavaScriptCore/runtime/MathObject.cpp:293
> +        JSValue::encode(jsNumber(left));

Two things wrong here:

1) Missing a "return" so this doesn’t do what you think it does.
2) There is no reason to do any work to return a particular value when there is an exception. The value will be ignored. I suggest returning JSValue(), which is probably the most efficient.

> Source/JavaScriptCore/runtime/MathObject.cpp:296
> +    if (exec->hadException())
> +        JSValue::encode(jsNumber(right));

Same problem again.

> Source/JavaScriptCore/runtime/MathObject.cpp:297
> +    uint32_t raised = mathPow(2, 32);

This doesn’t do what you think it does. The value of raised here will be 0, since 2^32 does not fit in a uint32_t.

> Source/JavaScriptCore/runtime/MathObject.cpp:298
> +    int32_t product = (int32_t)((left * right)%raised);

Incorrect formatting here (need spaces around the % operator). Incorrect style, using C-style casts. Likely also that this does not handle overflow properly. Need test cases that involve overflow and ideally test cases that almost overflow, but not quite. This code likely doesn’t work very well.

> Source/JavaScriptCore/runtime/MathObject.cpp:299
> +    if (product >= mathPow(2, 31))

I don’t think the compiler constant-folds the mathPow expression so this is unnecessarily inefficient. I personally would write:

    if (product >= (1U << 31))

Or something along those lines. Maybe better to use a named constant.

> Source/JavaScriptCore/runtime/MathObject.cpp:300
> +        return JSValue::encode(jsNumber(product-raised));

Need spaces around the "-" sign.

Also, I am pretty sure that overflow of uint32_t will produce these results without explicit special cases. Once you have a test in place that covers enough edge cases we can do some experiments with that.

Long term we are going to want to build an efficient version of this into the compiler, so the test cases are actually more important than the C++ code.

[Tibor Mészáros]

Created attachment 226692 [details]
Patch v2

The problems with the previous patch has been fixed, and added new tests.

[Build Bot]

Comment on attachment 226692 [details]
Patch v2

Attachment 226692 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6184583959674880

New failing tests:
js/dom/imul.html

[Build Bot]

Created attachment 226700 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Tibor Mészáros]

Created attachment 226702 [details]
Patch v3

[Darin Adler]

Comment on attachment 226702 [details]
Patch v3

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

Please only add code changes that either make the code easier to read, faster, or actually fix something we can demonstrate is wrong with a test. Most of the code change here is not needed.

> Source/JavaScriptCore/runtime/MathObject.cpp:293
> +        return JSValue::encode(jsNumber(left));

When there is an exception, the return value is ignored. So this should just say

    return JSValue();

We shouldn’t waste any code computing a value that will be ignored.

> Source/JavaScriptCore/runtime/MathObject.cpp:296
> +    if (exec->hadException())
> +        return JSValue::encode(jsNumber(right));

Same issue here. But also, since there is no code execution after this point, there is no reason to do an early exit when an exception occurred. We can just let the product be computed and then the caller will ignore it. So this code should be deleted.

> Source/JavaScriptCore/runtime/MathObject.cpp:301
> +    uint32_t intMax = std::numeric_limits<int>::max();
> +    uint64_t product = left * right;
> +    if (product > intMax)
> +        return JSValue::encode(jsNumber((int32_t)(product-mathPow(2, 32))));
> +    return JSValue::encode(jsNumber((int32_t)product));

This code is written strangely. Why are we getting the maximum of "int" when the rest of this function is written in terms of types like int32_t? But also, it’s making a big mistake. The whole point of how imul is specified is that it defines behavior that can be efficiently implemented in C. The old code should have worked. Please try just changing the types from int32_t to uint32_t without adding this explicit overflow logic. That should fix the function. If it doesn’t fix the function, please provide a test case showing that it’s wrong.

To word this another way, I don’t see any added test case that covers this overflow issue fixed here, and I suspect this is fixing something that is not broken.

[Oliver Hunt]

Comment on attachment 226702 [details]
Patch v3

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

>> Source/JavaScriptCore/runtime/MathObject.cpp:293
>> +        return JSValue::encode(jsNumber(left));
> 
> When there is an exception, the return value is ignored. So this should just say
> 
>     return JSValue();
> 
> We shouldn’t waste any code computing a value that will be ignored.

No, we should always return a real value - return JSValue() has a tendency to result in a real null entering the vm.  Our normal approach is to return jsUndefined() or jsNull()

>> Source/JavaScriptCore/runtime/MathObject.cpp:296
>> +        return JSValue::encode(jsNumber(right));
> 
> Same issue here. But also, since there is no code execution after this point, there is no reason to do an early exit when an exception occurred. We can just let the product be computed and then the caller will ignore it. So this code should be deleted.

as above don't return JSValue()

[Darin Adler]

Comment on attachment 226702 [details]
Patch v3

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

>>> Source/JavaScriptCore/runtime/MathObject.cpp:293
>>> +        return JSValue::encode(jsNumber(left));
>> 
>> When there is an exception, the return value is ignored. So this should just say
>> 
>>     return JSValue();
>> 
>> We shouldn’t waste any code computing a value that will be ignored.
> 
> No, we should always return a real value - return JSValue() has a tendency to result in a real null entering the vm.  Our normal approach is to return jsUndefined() or jsNull()

Sounds good.

    return jsUndefined();

>>> Source/JavaScriptCore/runtime/MathObject.cpp:296
>>> +        return JSValue::encode(jsNumber(right));
>> 
>> Same issue here. But also, since there is no code execution after this point, there is no reason to do an early exit when an exception occurred. We can just let the product be computed and then the caller will ignore it. So this code should be deleted.
> 
> as above don't return JSValue()

Sounds good.

    return jsUndefined();