Bug 183721

Created attachment 336016 [details] WIP - Patch It starts. Need to write tests and ChangeLog yet.

Attachment 336016 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:264: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:267: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:489: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:490: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:490: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:491: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:492: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:493: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:494: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:495: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:511: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:513: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:513: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:522: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 14 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 336027 [details] Patch

Ping Review

Comment on attachment 336027 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336027&action=review > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:494 > + if (UNLIKELY(throwScope.exception())) > + RETURN(JSValue()); CHECK_EXCEPTION() here > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:504 > + THROW(createTypeError(exec, "Invalid operand in BigInt operation.")); Let's have this be more descriptive. You know this is a multiply. Also, is this really a type error? There is no automatic conversion to BigInt? Like, if I want to multiply a BigInt by 2, I must write "* 2n"? > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:509 > + if (UNLIKELY(throwScope.exception())) > + RETURN(JSValue()); CHECK_EXCEPTION() > Source/JavaScriptCore/runtime/JSBigInt.cpp:258 > + int resultLength = x->length() + y->length(); Why int here? Should definitely be using unsigned or size_t. Also, are we worried about overflow here? > Source/JavaScriptCore/runtime/JSBigInt.cpp:262 > + for (int i = 0; i < x->length(); i++) ditto for type. > Source/JavaScriptCore/runtime/JSBigInt.cpp:493 > + for (int i = 0; i < multiplicand->length(); i++, accumulatorIndex++) { ditto for type. I see we're using int everywhere instead of unsigned/size_t. What's the reason for that?

Comment on attachment 336027 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336027&action=review Thank you for the review >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:494 >> + RETURN(JSValue()); > > CHECK_EXCEPTION() here ok >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:504 >> + THROW(createTypeError(exec, "Invalid operand in BigInt operation.")); > > Let's have this be more descriptive. You know this is a multiply. Also, is this really a type error? There is no automatic conversion to BigInt? > > Like, if I want to multiply a BigInt by 2, I must write "* 2n"? Yes. According to current spec, we should throw if type of operands doesn't match https://tc39.github.io/proposal-bigint/#sec-multiplicative-operators-runtime-semantics-evaluation >> Source/JavaScriptCore/runtime/JSBigInt.cpp:258 >> + int resultLength = x->length() + y->length(); > > Why int here? Should definitely be using unsigned or size_t. > Also, are we worried about overflow here? It is from the original implementation in V8. AFIK, we could always use unsigned or size_t with no problems. About overflow, I don't think we should be worried because current implementation max length is no greater than 2 ** 11 (https://github.com/caiolima/webkit/blob/master/Source/JavaScriptCore/runtime/JSBigInt.h#L100) >> Source/JavaScriptCore/runtime/JSBigInt.cpp:493 >> + for (int i = 0; i < multiplicand->length(); i++, accumulatorIndex++) { > > ditto for type. I see we're using int everywhere instead of unsigned/size_t. What's the reason for that? Ditto.

Created attachment 336955 [details] Patch

Ping Review

Png review

Ping Review

Created attachment 338110 [details] Patch

Ping Review

Ping Review

Comment on attachment 338110 [details] Patch r=me

Created attachment 338959 [details] Patch for landing

Created attachment 338960 [details] Patch for landing

Comment on attachment 338960 [details] Patch for landing Clearing flags on attachment: 338960 Committed r231086: <https://trac.webkit.org/changeset/231086>

All reviewed patches have been landed. Closing bug.

<rdar://problem/39781587>

This change caused 100 failures on the JSC debug bot due to unchecked exceptions: stress/untyped-mul.js.default: ERROR: Unchecked JS exception: stress/untyped-mul.js.default: This scope can throw a JS exception: toNumber @ ./runtime/JSString.cpp:418 stress/untyped-mul.js.default: (ExceptionScope::m_recursionDepth was 5) stress/untyped-mul.js.default: But the exception was unchecked as of this scope: jsMul @ /Volumes/Data/slave/highsierra-debug/build/Source/JavaScriptCore/runtime/Operations.h:263 stress/untyped-mul.js.default: (ExceptionScope::m_recursionDepth was 4) stress/untyped-mul.js.default: stress/untyped-mul.js.default: Unchecked exception detected at: stress/untyped-mul.js.default: 1 0x110047623 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&) stress/untyped-mul.js.default: 2 0x11002210b JSC::ThrowScope::~ThrowScope() stress/untyped-mul.js.default: 3 0x1100224e5 JSC::ThrowScope::~ThrowScope() stress/untyped-mul.js.default: 4 0x10fb8a997 JSC::jsMul(JSC::ExecState*, JSC::JSValue, JSC::JSValue) stress/untyped-mul.js.default: 5 0x10fb79d06 JSC::profiledMul(JSC::ExecState*, long long, long long, JSC::ArithProfile&, bool) stress/untyped-mul.js.default: 6 0x10fb79c75 operationValueMulProfiled stress/untyped-mul.js.default: 7 0x5ab6aefac79 stress/untyped-mul.js.default: 8 0x10ecf1ca1 llint_entry stress/untyped-mul.js.default: 9 0x10ece9552 vmEntryToJavaScript stress/untyped-mul.js.default: 10 0x10fb58e3a JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) stress/untyped-mul.js.default: 11 0x10faf8b13 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) stress/untyped-mul.js.default: 12 0x10fdc2857 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) stress/untyped-mul.js.default: 13 0x10eb3cf49 runWithOptions(GlobalObject*, CommandLine&, bool&) stress/untyped-mul.js.default: 14 0x10eb14fac jscmain(int, char**)::$_3::operator()(JSC::VM&, GlobalObject*, bool&) const stress/untyped-mul.js.default: 15 0x10eafccc4 int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool, jscmain(int, char**)::$_3 const&) stress/untyped-mul.js.default: 16 0x10eafb7af jscmain(int, char**) stress/untyped-mul.js.default: 17 0x10eafb70e main stress/untyped-mul.js.default: 18 0x7fff6e830015 start https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/939

Reverted r231086 for reason: Caused JSC test failures due to an unchecked exception. Committed r231104: <https://trac.webkit.org/changeset/231104>

Created attachment 339014 [details] Patch

Comment on attachment 339014 [details] Patch Clearing flags on attachment: 339014 Committed r231131: <https://trac.webkit.org/changeset/231131>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 185112

Created attachment 339068 [details] Patch

Comment on attachment 339068 [details] Patch Clearing flags on attachment: 339068 Committed r231137: <https://trac.webkit.org/changeset/231137>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 185118

Created attachment 339214 [details] Patch

Created attachment 339218 [details] Benchmark This new Path is perf neutral.

Ping review?

Created attachment 339413 [details] Patch

Hey Saam, could you take a new look at it? I added toNumeric to JSValue to make this change Spec compliant. https://tc39.github.io/proposal-bigint/#sec-tonumeric

Comment on attachment 339413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339413&action=review > Source/JavaScriptCore/jit/JITOperations.cpp:2573 > + JSValue result = jsMul(exec, op1, op2); You're missing an exception after this. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:504 > + Variant<JSBigInt*, double> leftNumeric = left.toNumeric(exec); > + CHECK_EXCEPTION(); > + Variant<JSBigInt*, double> rightNumeric = right.toNumeric(exec); > + CHECK_EXCEPTION(); > + > + if (WTF::holds_alternative<JSBigInt*>(leftNumeric) || WTF::holds_alternative<JSBigInt*>(rightNumeric)) { > + if (WTF::holds_alternative<JSBigInt*>(leftNumeric) && WTF::holds_alternative<JSBigInt*>(rightNumeric)) { > + JSValue result(JSBigInt::multiply(exec, WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric))); > + RETURN_WITH_PROFILING(result, { > + updateArithProfileForBinaryArithOp(exec, pc, result, left, right); > + }); > + } > + > + THROW(createTypeError(exec, "Invalid mix of BigInt and other type in multiplication.")); > + } Why isn't this just calling jsMul? > Source/JavaScriptCore/runtime/JSCJSValue.h:262 > + Variant<JSBigInt*, double> toNumeric(ExecState*) const; This seems like a reasonable interface. I wonder if we care about making it faster or not since users of this branch on type. > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:742 > + VM& vm = exec->vm(); > + auto scope = DECLARE_THROW_SCOPE(vm); > + JSValue primValue = this->toPrimitive(exec, PreferNumber); > + RETURN_IF_EXCEPTION(scope, 0); > + if (primValue.isBigInt()) > + return asBigInt(primValue); > + double value = primValue.toNumber(exec); > + RETURN_IF_EXCEPTION(scope, 0); > + return value; You should probably have fast paths for the type being int/double/bigint before calling toPrimitive.

Comment on attachment 339413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339413&action=review >> Source/JavaScriptCore/jit/JITOperations.cpp:2573 >> + JSValue result = jsMul(exec, op1, op2); > > You're missing an exception after this. exception check*

Created attachment 339634 [details] Patch

Ping Review

Created attachment 340159 [details] Patch

Created attachment 340160 [details] Patch

Comment on attachment 340160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340160&action=review r=me > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:150 > // FIXME: Revisit this condition when introducing BigInt to JSC. Remove this FIXME. And add a test for this case (in particular, we need to loop things to get compiled in DFG). > Source/JavaScriptCore/jit/JITOperations.cpp:2560 > + VM& vm = exec->vm(); > auto scope = DECLARE_THROW_SCOPE(vm); Remove this. > Source/JavaScriptCore/jit/JITOperations.cpp:2564 > + JSValue jsResult = jsMul(exec, op1, op2); Just return `JSValue::encode(jsMul(exec, op1, op2))`. > Source/JavaScriptCore/jit/JITOperations.cpp:2565 > RETURN_IF_EXCEPTION(scope, encodedJSValue()); Since `jsMul()` just returns the value, we do not need to have scope and exception checks. > Source/JavaScriptCore/jit/JITOperations.cpp:2580 > RETURN_IF_EXCEPTION(scope, encodedJSValue()); On the other hand, this profiledMul uses arithProfile after `jsMul`. So this function should keep scope and exception checks.`

Comment on attachment 340160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340160&action=review Thx for the review! >> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:150 >> // FIXME: Revisit this condition when introducing BigInt to JSC. > > Remove this FIXME. And add a test for this case (in particular, we need to loop things to get compiled in DFG). ```JSTests/stress/big-int-mul-jit.js``` is already testing this.

Created attachment 340240 [details] Patch for landing

Comment on attachment 340240 [details] Patch for landing Rejecting attachment 340240 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 340240, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit NOBODY Yusuke Suzuki found in /Volumes/Data/EWS/WebKit/JSTests/ChangeLog does not appear to be a valid reviewer according to contributors.json. /Volumes/Data/EWS/WebKit/JSTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.webkit.org/results/7657434

(In reply to WebKit Commit Bot from comment #44) > Comment on attachment 340240 [details] > Patch for landing > > Rejecting attachment 340240 [details] from commit-queue. > > Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', > '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', > 'validate-changelog', '--check-oops', '--non-interactive', 340240, > '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit > > NOBODY Yusuke Suzuki found in /Volumes/Data/EWS/WebKit/JSTests/ChangeLog > does not appear to be a valid reviewer according to contributors.json. > /Volumes/Data/EWS/WebKit/JSTests/ChangeLog neither lists a valid reviewer > nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). > > Full output: http://webkit-queues.webkit.org/results/7657434 NOBODY remains.

Created attachment 340242 [details] Patch for landing

Comment on attachment 340242 [details] Patch for landing Clearing flags on attachment: 340242 Committed r231733: <https://trac.webkit.org/changeset/231733>

All reviewed patches have been landed. Closing bug.

Remember ArithMul can return number only. We need to add ValueMul too in DFG.

(In reply to Yusuke Suzuki from comment #49) > Remember ArithMul can return number only. We need to add ValueMul too in DFG. Yes. I created umbrella bug https://bugs.webkit.org/show_bug.cgi?id=186173 to keep track of each node we need to add Value<op>. Probably it is also going to be true for unary operators.