Bug 210978 - BigInt math runtime shouldn't convert BigInt32 input operands to a heap cell when doing math
Summary: BigInt math runtime shouldn't convert BigInt32 input operands to a heap cell ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on: 211165
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-24 10:40 PDT by Saam Barati
Modified: 2020-04-28 23:57 PDT (History)
14 users (show)

See Also:


Attachments
WIP (19.45 KB, patch)
2020-04-24 19:31 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (86.60 KB, patch)
2020-04-27 14:45 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (102.18 KB, patch)
2020-04-27 17:23 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (112.86 KB, patch)
2020-04-27 18:54 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (114.06 KB, patch)
2020-04-27 19:10 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (113.86 KB, patch)
2020-04-27 19:14 PDT, Saam Barati
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2020-04-24 10:40:16 PDT
We could define an abstract interface and use that in the math runtime instead.
Comment 1 Saam Barati 2020-04-24 19:31:53 PDT
Created attachment 397536 [details]
WIP

it begins
Comment 2 Keith Miller 2020-04-24 23:01:17 PDT
The title of this bug seems a bit ambitious in the abstract... 🤔
Comment 3 Saam Barati 2020-04-27 10:28:39 PDT
(In reply to Keith Miller from comment #2)
> The title of this bug seems a bit ambitious in the abstract... 🤔

How so?
Comment 4 Saam Barati 2020-04-27 14:45:52 PDT
Created attachment 397743 [details]
WIP
Comment 5 Saam Barati 2020-04-27 17:23:15 PDT
Created attachment 397771 [details]
WIP

it compiles
Comment 6 Saam Barati 2020-04-27 18:54:11 PDT
Created attachment 397782 [details]
patch
Comment 7 Saam Barati 2020-04-27 19:10:34 PDT
Created attachment 397785 [details]
patch
Comment 8 Saam Barati 2020-04-27 19:14:57 PDT
Created attachment 397786 [details]
patch
Comment 9 Yusuke Suzuki 2020-04-27 19:52:24 PDT
Comment on attachment 397785 [details]
patch

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

r=me

> Source/JavaScriptCore/runtime/Operations.cpp:79
> +    RELEASE_AND_RETURN(scope, arithmeticBinaryOp(globalObject, p1, p2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in addition."_s));

See the comment below.

> Source/JavaScriptCore/runtime/Operations.h:531
> +ALWAYS_INLINE JSValue arithmeticBinaryOp(JSGlobalObject* globalObject, JSValue v1, JSValue v2, DoubleOperation&& doubleOp, JSValue (*bigInt32)(JSGlobalObject*, int32_t, int32_t), JSValue (*mixedBigInt1)(JSGlobalObject*, JSBigInt*, int32_t), JSValue (*mixedBigInt2)(JSGlobalObject*, int32_t, JSBigInt*), JSValue (*heapBigInt)(JSGlobalObject*, JSBigInt*, JSBigInt*), const char* errorMessage)

Why not taking auto-parameterized lambda as a parameterized type-ed value and call it for all the BigInt cases?

tempalte<typename DoubleOperation, typename BigIntOperation>
ALWAYS_INLINE JSValue arithmeticBinaryOp(JSGlobalObject* globalObject, JSValue v1, JSValue v2, DoubleOperation&& doubleOp, BigIntOperation&& bigIntOp, const char* errorMessage)

And always call bigIntOp for BigInts.

> Source/JavaScriptCore/runtime/Operations.h:547
> +            RELEASE_AND_RETURN(scope, bigInt32(globalObject, leftNumeric.bigInt32AsInt32(), rightNumeric.bigInt32AsInt32()));

So, then, you can write it as `ELEASE_AND_RETURN(scope, bigIntOp(globalObject, leftNumeric.bigInt32AsInt32(), rightNumeric.bigInt32AsInt32()));`

> Source/JavaScriptCore/runtime/Operations.h:549
> +            RELEASE_AND_RETURN(scope, mixedBigInt2(globalObject, leftNumeric.bigInt32AsInt32(), rightNumeric.asHeapBigInt()));

`RELEASE_AND_RETURN(scope, bigIntOp(globalObject, leftNumeric.bigInt32AsInt32(), rightNumeric.asHeapBigInt()));`

> Source/JavaScriptCore/runtime/Operations.h:552
> +            RELEASE_AND_RETURN(scope, mixedBigInt1(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.bigInt32AsInt32()));

RELEASE_AND_RETURN(scope, bigIntOp(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.bigInt32AsInt32()));

> Source/JavaScriptCore/runtime/Operations.h:554
> +            RELEASE_AND_RETURN(scope, heapBigInt(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.asHeapBigInt()));

RELEASE_AND_RETURN(scope, bigIntOp(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.asHeapBigInt()));

> Source/JavaScriptCore/runtime/Operations.h:559
> +        RELEASE_AND_RETURN(scope, heapBigInt(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.asHeapBigInt()));

RELEASE_AND_RETURN(scope, bigIntOp(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.asHeapBigInt()));

> Source/JavaScriptCore/runtime/Operations.h:575
> +    return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in subtraction."_s);

In that case, we do not need to pass 4 bigIntOp here. You can just call it,

return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, "Invalid mix of BigInt and other type in subtraction."_s);

> Source/JavaScriptCore/runtime/Operations.h:588
> +    return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in multiplication."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:601
> +    return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in division."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:614
> +    return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in remainder."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:627
> +    return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in exponentiation."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:774
> -    Optional<uint32_t> leftUint32 = left.toUInt32AfterToNumeric(globalObject);
> +    auto leftNumeric = left.toBigIntOrInt32(globalObject);
>      RETURN_IF_EXCEPTION(scope, { });
> -    Optional<uint32_t> rightUint32 = right.toUInt32AfterToNumeric(globalObject);
> +    auto rightNumeric = right.toBigIntOrInt32(globalObject);
>      RETURN_IF_EXCEPTION(scope, { });
>  
> -    if (UNLIKELY(!leftUint32 || !rightUint32)) {
> +    if (UNLIKELY(!leftNumeric.isInt32() || !rightNumeric.isInt32())) {
>          throwTypeError(globalObject, scope, "BigInt does not support >>> operator"_s);
>          return { };
>      }
>  
> -    return jsNumber(static_cast<int32_t>(leftUint32.value() >> (rightUint32.value() & 31)));
> +    uint32_t leftUint32 = static_cast<uint32_t>(leftNumeric.asInt32());
> +    uint32_t rightUint32 = static_cast<uint32_t>(rightNumeric.asInt32());
> +
> +    return jsNumber(static_cast<int32_t>(leftUint32 >> (rightUint32 & 31)));

Why is it changed?

> Source/JavaScriptCore/runtime/Operations.h:778
> +ALWAYS_INLINE JSValue bitwiseBinaryOp(JSGlobalObject* globalObject, JSValue v1, JSValue v2, Int32Operation&& int32Op, JSValue (*bigInt32)(JSGlobalObject*, int32_t, int32_t), JSValue (*mixedBigInt1)(JSGlobalObject*, JSBigInt*, int32_t), JSValue (*mixedBigInt2)(JSGlobalObject*, int32_t, JSBigInt*), JSValue (*heapBigInt)(JSGlobalObject*, JSBigInt*, JSBigInt*), const char* errorMessage)

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:824
> +    return bitwiseBinaryOp(globalObject, v1, v2, int32Op, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in bitwise 'and' operation."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:840
> +    return bitwiseBinaryOp(globalObject, v1, v2, int32Op, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in bitwise 'or' operation."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:855
> +    return bitwiseBinaryOp(globalObject, v1, v2, int32Op, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in bitwise 'xor' operation."_s);

Ditto.
Comment 10 Yusuke Suzuki 2020-04-27 19:53:06 PDT
Comment on attachment 397786 [details]
patch

r=me if EWS is happy :)
Comment 11 Saam Barati 2020-04-27 19:58:26 PDT
Comment on attachment 397785 [details]
patch

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

>> Source/JavaScriptCore/runtime/Operations.h:774
>> +    return jsNumber(static_cast<int32_t>(leftUint32 >> (rightUint32 & 31)));
> 
> Why is it changed?

I think this is a bad rebase. Will revert
Comment 12 Saam Barati 2020-04-27 23:25:45 PDT
landed in:
https://trac.webkit.org/changeset/260809/webkit
Comment 13 Radar WebKit Bug Importer 2020-04-27 23:26:18 PDT
<rdar://problem/62490218>
Comment 14 Yusuke Suzuki 2020-04-28 23:09:38 PDT
Committed r260876: <https://trac.webkit.org/changeset/260876>
Comment 15 Yusuke Suzuki 2020-04-28 23:14:45 PDT
Committed r260877: <https://trac.webkit.org/changeset/260877>
Comment 16 WebKit Commit Bot 2020-04-28 23:21:38 PDT
Re-opened since this is blocked by bug 211165
Comment 17 Saam Barati 2020-04-28 23:57:07 PDT
This shouldn't have been re-opened.

Build fix in:
https://trac.webkit.org/changeset/260880/webkit