Bug 179002 - [ESNext][BigInt] Implement support for addition operations
Summary: [ESNext][BigInt] Implement support for addition operations
Status: RESOLVED FIXED
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: InRadar
Depends on: 182214 186238
Blocks: 179001
  Show dependency treegraph
 
Reported: 2017-10-30 04:03 PDT by Caio Lima
Modified: 2018-06-02 19:22 PDT (History)
11 users (show)

See Also:


Attachments
Patch (71.72 KB, patch)
2018-01-21 15:56 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (479.36 KB, application/zip)
2018-01-21 16:51 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (889.62 KB, application/zip)
2018-01-21 16:59 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (1.01 MB, application/zip)
2018-01-21 17:09 PST, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (501.46 KB, application/zip)
2018-01-21 17:16 PST, Build Bot
no flags Details
Patch (73.00 KB, patch)
2018-01-23 14:37 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (73.00 KB, patch)
2018-01-23 15:26 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.20 MB, application/zip)
2018-01-23 16:40 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.37 MB, application/zip)
2018-01-23 16:41 PST, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.17 MB, application/zip)
2018-01-23 17:08 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.16 MB, application/zip)
2018-01-23 17:26 PST, Build Bot
no flags Details
Patch (70.55 KB, patch)
2018-01-24 02:48 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.55 MB, application/zip)
2018-01-24 04:03 PST, Build Bot
no flags Details
Patch (70.54 KB, patch)
2018-01-24 14:04 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (73.25 KB, patch)
2018-01-25 14:31 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (73.23 KB, patch)
2018-01-25 14:44 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmark Reports (87.65 KB, text/plain)
2018-01-25 14:46 PST, Caio Lima
no flags Details
Patch (60.80 KB, patch)
2018-05-31 20:00 PDT, Caio Lima
utatane.tea: review+
Details | Formatted Diff | Diff
Benchmark Report (93.41 KB, text/plain)
2018-06-01 07:00 PDT, Caio Lima
no flags Details
Patch for landing (69.52 KB, patch)
2018-06-01 19:22 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch for landing (68.05 KB, patch)
2018-06-02 18:41 PDT, Caio Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2017-10-30 04:03:20 PDT
Implement here support for additional arithmetic operations "+" and "-"
Comment 1 Caio Lima 2018-01-21 15:56:23 PST
Created attachment 331884 [details]
Patch
Comment 2 Build Bot 2018-01-21 15:58:49 PST
Attachment 331884 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:78:  The parameter name "sign" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:129:  The parameter name "sign" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:129:  The parameter name "parseIntMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 3 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2018-01-21 16:51:01 PST
Comment on attachment 331884 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 4 Build Bot 2018-01-21 16:51:02 PST
Created attachment 331885 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 Build Bot 2018-01-21 16:59:39 PST
Comment on attachment 331884 [details]
Patch

Attachment 331884 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6162682

Number of test failures exceeded the failure limit.
Comment 6 Build Bot 2018-01-21 16:59:40 PST
Created attachment 331886 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 7 Build Bot 2018-01-21 17:09:10 PST
Comment on attachment 331884 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 8 Build Bot 2018-01-21 17:09:11 PST
Created attachment 331887 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 Build Bot 2018-01-21 17:16:53 PST
Comment on attachment 331884 [details]
Patch

Attachment 331884 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6162718

Number of test failures exceeded the failure limit.
Comment 10 Build Bot 2018-01-21 17:16:54 PST
Created attachment 331888 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 11 Caio Lima 2018-01-23 14:37:19 PST
Created attachment 332079 [details]
Patch
Comment 12 Caio Lima 2018-01-23 15:26:23 PST
Created attachment 332085 [details]
Patch
Comment 13 Build Bot 2018-01-23 16:40:13 PST
Comment on attachment 332085 [details]
Patch

Attachment 332085 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6187861

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-baseline
mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla
mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla
mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-baseline
mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-llint
mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-no-ftl
mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla
mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-baseline
mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-baseline
mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-baseline
mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla
mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla
mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-baseline
mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla
mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-no-ftl
mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-baseline
mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla
mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-baseline
mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-baseline
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla
mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla
mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline
mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases
Comment 14 Build Bot 2018-01-23 16:40:49 PST
Comment on attachment 332085 [details]
Patch

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

New failing tests:
http/tests/misc/no-last-modified.html
imported/w3c/IndexedDB-private-browsing/keyorder.html
imported/w3c/web-platform-tests/IndexedDB/keyorder.htm
imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html
imported/w3c/web-platform-tests/user-timing/measure.html
media/video-controller-child-rate.html
Comment 15 Build Bot 2018-01-23 16:40:50 PST
Created attachment 332096 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 16 Build Bot 2018-01-23 16:41:39 PST
Comment on attachment 332085 [details]
Patch

Attachment 332085 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6187870

New failing tests:
http/tests/misc/no-last-modified.html
imported/w3c/IndexedDB-private-browsing/keyorder.html
imported/w3c/web-platform-tests/user-timing/measure.html
imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html
imported/w3c/web-platform-tests/IndexedDB/keyorder.htm
media/video-controller-child-rate.html
Comment 17 Build Bot 2018-01-23 16:41:41 PST
Created attachment 332097 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 18 Build Bot 2018-01-23 17:08:11 PST
Comment on attachment 332085 [details]
Patch

Attachment 332085 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6187973

New failing tests:
http/tests/misc/no-last-modified.html
imported/w3c/IndexedDB-private-browsing/keyorder.html
imported/w3c/web-platform-tests/IndexedDB/keyorder.htm
imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html
imported/w3c/web-platform-tests/user-timing/measure.html
media/video-controller-child-rate.html
Comment 19 Build Bot 2018-01-23 17:08:12 PST
Created attachment 332101 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 20 Build Bot 2018-01-23 17:26:46 PST
Comment on attachment 332085 [details]
Patch

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

New failing tests:
http/tests/misc/no-last-modified.html
imported/w3c/IndexedDB-private-browsing/keyorder.html
imported/w3c/web-platform-tests/user-timing/measure.html
imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html
imported/w3c/web-platform-tests/IndexedDB/keyorder.htm
media/video-controller-child-rate.html
Comment 21 Build Bot 2018-01-23 17:26:47 PST
Created attachment 332106 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 22 Caio Lima 2018-01-24 02:48:01 PST
Created attachment 332144 [details]
Patch
Comment 23 Build Bot 2018-01-24 04:03:23 PST
Comment on attachment 332144 [details]
Patch

Attachment 332144 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6193545

New failing tests:
fast/workers/worker-cloneport.html
Comment 24 Build Bot 2018-01-24 04:03:25 PST
Created attachment 332146 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 25 Caio Lima 2018-01-24 14:04:46 PST
Created attachment 332195 [details]
Patch
Comment 26 Joseph Pecoraro 2018-01-25 12:03:46 PST
Comment on attachment 332195 [details]
Patch

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

These tests are great! I'd like to see a few more edge cases:

• BigInt [+/-] with exotic values like NaN / Infinity:
  1n + Infinity; Infinity + 1n;
  1n + NaN; NaN + 1n;

• BigInt [+/-] with other types / falsey values like null / undefined / bool / empty string:
  1n + null;
  1n + undefined;
  1n + true;
  1n + false;
  1n + "";

• Did I read the ChangeLog correctly that unary minus isn't supported just yet? If it is we could add tests like:
  x = 1n; y = -x; z = -y; x === z;

> JSTests/stress/big-int-addition-type-error.js:17
> +assertThrowTypeError(30n, Symbol("foo"), "BingInt + Symbol");

Typos: "BingInt" in this file should be "BigInt"

> JSTests/stress/big-int-subtraction-basic.js:39
> +testSub(0xFEDCBA987654320Fn, 0x0n, 0xFEDCBA987654320Fn); testSub(0xFEDCBA987654320Fn, -0x1n, 0xFEDCBA9876543210n);

Style: Probably want to split these up into two lines.
Comment 27 Caio Lima 2018-01-25 13:31:50 PST
(In reply to Joseph Pecoraro from comment #26)
> Comment on attachment 332195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332195&action=review
> 
> These tests are great! I'd like to see a few more edge cases:
> 
> • BigInt [+/-] with exotic values like NaN / Infinity:
>   1n + Infinity; Infinity + 1n;
>   1n + NaN; NaN + 1n;
> 
> • BigInt [+/-] with other types / falsey values like null / undefined / bool
> / empty string:
>   1n + null;
>   1n + undefined;
>   1n + true;
>   1n + false;
>   1n + "";

Thank you very much to remember these cases!

> • Did I read the ChangeLog correctly that unary minus isn't supported just
> yet? If it is we could add tests like:
>   x = 1n; y = -x; z = -y; x === z;

No. I just implemented the -[BigInt literal] support into AST. I'm not liking it so much, so probably I would make this Bug dependent on unary operations. I'm going to start work on them today.

> > JSTests/stress/big-int-addition-type-error.js:17
> > +assertThrowTypeError(30n, Symbol("foo"), "BingInt + Symbol");
> 
> Typos: "BingInt" in this file should be "BigInt"

Oops.

> > JSTests/stress/big-int-subtraction-basic.js:39
> > +testSub(0xFEDCBA987654320Fn, 0x0n, 0xFEDCBA987654320Fn); testSub(0xFEDCBA987654320Fn, -0x1n, 0xFEDCBA9876543210n);
> 
> Style: Probably want to split these up into two lines.

Oops.
Comment 28 Caio Lima 2018-01-25 14:31:37 PST
Created attachment 332317 [details]
Patch
Comment 29 Caio Lima 2018-01-25 14:44:26 PST
Created attachment 332318 [details]
Patch
Comment 30 Caio Lima 2018-01-25 14:46:54 PST
Created attachment 332320 [details]
Benchmark Reports

This is the run comparing possible overhead of BigInt support into "+" and "-" operations. The benchmarks are neutral.
Comment 31 Joseph Pecoraro 2018-01-29 11:17:25 PST
Comment on attachment 332318 [details]
Patch

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

> JSTests/stress/big-int-subtraction-type-error.js:17
> +assertThrowTypeError(30n, Symbol("foo"), "BingInt - Symbol");

Nice! A few more "BingInt"s to clean up in this file.
Comment 32 Caio Lima 2018-05-31 20:00:21 PDT
Created attachment 341725 [details]
Patch
Comment 33 Caio Lima 2018-06-01 07:00:43 PDT
Created attachment 341759 [details]
Benchmark Report

This patch is perf neutral.
Comment 34 Yusuke Suzuki 2018-06-01 08:43:28 PDT
Comment on attachment 341725 [details]
Patch

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

r=me with nits.

> Source/JavaScriptCore/ChangeLog:10
> +        and FTL JIT layers, but he plan is to improve this support in future

"he" => "we" ?

> Source/JavaScriptCore/ChangeLog:11
> +        patches.

Yeah, it is broken in DFG and FTL right now. For example, DFG prediction propagation phase for ValueAdd does not account BigInt.
Before enabling BigInt, we need to complete DFG and FTL changes for BigInt.
Right now, it is disabled. So it's ok to implement Baseline and LLInt first.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:527
> +            JSBigInt* result = JSBigInt::sub(exec->vm(), WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric));

Use `vm` (defined in `BEGIN()`), instead of `exec->vm()`. And please fix `JSBigInt::unaryMinus(exec->vm()` to `JSBigInt::unaryMinus(vm` in this file.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1346
> +JSObject* JSBigInt::toObject(ExecState* state, JSGlobalObject* globalObject) const
>  {
> -    return BigIntObject::create(exec->vm(), globalObject, const_cast<JSBigInt*>(this));
> +    return BigIntObject::create(state->vm(), globalObject, const_cast<JSBigInt*>(this));

Please use `exec`. In JSC, it is named `exec` basically.

> Source/JavaScriptCore/runtime/Operations.cpp:78
> +        throwTypeError(callFrame, scope, ASCIILiteral("Invalid mix of BigInt and other type in addition."));
> +        return { };

`return throwTypeError(callFrame, scope, ASCIILiteral("Invalid mix of BigInt and other type in addition."));` is better.

> Source/JavaScriptCore/runtime/Operations.cpp:83
> +    double p1Number = WTF::get<double>(leftNumeric);
> +    double p2Number = WTF::get<double>(rightNumeric);
> +    return jsNumber(p1Number + p2Number);

Let's `return jsNumber(WTF::get<double>(leftNumeric) + WTF::get<double>(rightNumeric));`

> Source/JavaScriptCore/runtime/Operations.h:345
> +        throwTypeError(state, scope, ASCIILiteral("Invalid mix of BigInt and other type in subtraction."));
> +        return { };

`return throwTypeError(state, scope, ASCIILiteral("Invalid mix of BigInt and other type in subtraction."));` is better.
Comment 35 Caio Lima 2018-06-01 18:56:41 PDT
Comment on attachment 341725 [details]
Patch

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

Thank you for the review!

>> Source/JavaScriptCore/ChangeLog:11
>> +        patches.
> 
> Yeah, it is broken in DFG and FTL right now. For example, DFG prediction propagation phase for ValueAdd does not account BigInt.
> Before enabling BigInt, we need to complete DFG and FTL changes for BigInt.
> Right now, it is disabled. So it's ok to implement Baseline and LLInt first.

That's is the current plan, BTW.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:527
>> +            JSBigInt* result = JSBigInt::sub(exec->vm(), WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric));
> 
> Use `vm` (defined in `BEGIN()`), instead of `exec->vm()`. And please fix `JSBigInt::unaryMinus(exec->vm()` to `JSBigInt::unaryMinus(vm` in this file.

Good to know. Fixed.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:1346
>> +    return BigIntObject::create(state->vm(), globalObject, const_cast<JSBigInt*>(this));
> 
> Please use `exec`. In JSC, it is named `exec` basically.

The discussion around exec/state was started with the reason that "state" is the subject of ExecState. However, I think it is better keep consistence with all other JSC and use "exec" instead. I'm changing all usage into JSBigInt.

>> Source/JavaScriptCore/runtime/Operations.cpp:78
>> +        return { };
> 
> `return throwTypeError(callFrame, scope, ASCIILiteral("Invalid mix of BigInt and other type in addition."));` is better.

Fixed.

>> Source/JavaScriptCore/runtime/Operations.cpp:83
>> +    return jsNumber(p1Number + p2Number);
> 
> Let's `return jsNumber(WTF::get<double>(leftNumeric) + WTF::get<double>(rightNumeric));`

Fixed

>> Source/JavaScriptCore/runtime/Operations.h:345
>> +        return { };
> 
> `return throwTypeError(state, scope, ASCIILiteral("Invalid mix of BigInt and other type in subtraction."));` is better.

Fixed
Comment 36 Caio Lima 2018-06-01 19:22:03 PDT
Created attachment 341815 [details]
Patch for landing
Comment 37 WebKit Commit Bot 2018-06-02 09:03:59 PDT
Comment on attachment 341815 [details]
Patch for landing

Clearing flags on attachment: 341815

Committed r232439: <https://trac.webkit.org/changeset/232439>
Comment 38 WebKit Commit Bot 2018-06-02 09:04:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 Radar WebKit Bug Importer 2018-06-02 09:10:06 PDT
<rdar://problem/40748651>
Comment 40 WebKit Commit Bot 2018-06-02 15:05:14 PDT
Re-opened since this is blocked by bug 186238
Comment 41 Yusuke Suzuki 2018-06-02 15:05:38 PDT
Comment on attachment 341815 [details]
Patch for landing

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

> Source/JavaScriptCore/jit/JITOperations.cpp:2835
> +    JSValue result = jsSub(exec, op1, op2);

I missed this, we should add exception check here: before profiling the value.
Comment 42 Caio Lima 2018-06-02 16:23:04 PDT
Comment on attachment 341815 [details]
Patch for landing

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

> Source/JavaScriptCore/runtime/JSBigInt.cpp:756
> +    result->rightTrim(vm);

And I also missed this one. It returns invalid BigInt format with non-trimmed zeros.
Comment 43 Caio Lima 2018-06-02 18:41:24 PDT
Created attachment 341860 [details]
Patch for landing
Comment 44 WebKit Commit Bot 2018-06-02 19:22:51 PDT
Comment on attachment 341860 [details]
Patch for landing

Clearing flags on attachment: 341860

Committed r232449: <https://trac.webkit.org/changeset/232449>
Comment 45 WebKit Commit Bot 2018-06-02 19:22:53 PDT
All reviewed patches have been landed.  Closing bug.