From 8349a0f134632a39028fec62d38bacbd4f89f14a Mon Sep 17 00:00:00 2001 From: Charlie Gordon Date: Sun, 18 Feb 2024 06:46:09 +0100 Subject: [PATCH 1/4] Force evaluation order in `set_date_fields` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - use `volatile` to force evaluation order in `set_date_fields` - fix evaluation error in test262/test/built-ins/Date/UTC/fp-evaluation-order.js:19: unexpected error: Test262Error: precision in MakeDate Expected SameValue(«34448384», «34447360») to be true - this error occurs on the macOS target with clang --version: Apple clang version 15.0.0 (clang-1500.1.0.2.5) Target: arm64-apple-darwin23.3.0 --- quickjs.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/quickjs.c b/quickjs.c index e6166ee05..4786643ff 100644 --- a/quickjs.c +++ b/quickjs.c @@ -46814,7 +46814,8 @@ static double time_clip(double t) { of the operations */ static double set_date_fields(double fields[], int is_local) { int64_t y; - double days, d, h, m1; + double days, h, m1; + volatile double d; /* enforce evaluation order */ int i, m, md; m1 = fields[1]; @@ -46831,9 +46832,14 @@ static double set_date_fields(double fields[], int is_local) { days += md; } days += fields[2] - 1; + /* made d volatile to ensure order of evaluation as specified in ECMA. + * this fixes a test262 error on + * test262/test/built-ins/Date/UTC/fp-evaluation-order.js + */ h = fields[3] * 3600000 + fields[4] * 60000 + fields[5] * 1000 + fields[6]; - d = days * 86400000 + h; + d = days * 86400000; + d = d + h; if (is_local) d += getTimezoneOffset(d) * 60000; return time_clip(d); From 9ae3aa1584cc5fd660d1af70f896a7a98b6941dd Mon Sep 17 00:00:00 2001 From: Charlie Gordon Date: Wed, 21 Feb 2024 13:26:53 +0100 Subject: [PATCH 2/4] Rewrite `set_date_fields` to match the ECMA specification - use `double` arithmetic where necessary to match the spec - use `volatile` to ensure correct order of evaluation and prevent FMA code generation - reject some border cases. - avoid undefined behavior in `double` -> `int64_t` conversions - improved tests/test_builtin.js `assert` function to compare values more reliably. - added some tests in `test_date()` --- quickjs.c | 83 ++++++++++++++++++++++++++++--------------- tests/test_builtin.js | 72 ++++++++++++++++++++++++++++--------- 2 files changed, 110 insertions(+), 45 deletions(-) diff --git a/quickjs.c b/quickjs.c index 4786643ff..29bbbe623 100644 --- a/quickjs.c +++ b/quickjs.c @@ -46764,7 +46764,7 @@ static __exception int get_date_fields(JSContext *ctx, JSValue obj, return FALSE; /* NaN */ d = 0; /* initialize all fields to 0 */ } else { - d = dval; + d = dval; /* assuming -8.64e15 <= dval <= -8.64e15 */ if (is_local) { tz = -getTimezoneOffset(d); d += tz * 60000; @@ -46810,39 +46810,63 @@ static double time_clip(double t) { return NAN; } -/* The spec mandates the use of 'double' and it fixes the order +/* The spec mandates the use of 'double' and it specifies the order of the operations */ static double set_date_fields(double fields[], int is_local) { - int64_t y; - double days, h, m1; - volatile double d; /* enforce evaluation order */ - int i, m, md; - - m1 = fields[1]; - m = fmod(m1, 12); - if (m < 0) - m += 12; - y = (int64_t)(fields[0] + floor(m1 / 12)); - days = days_from_year(y); - - for(i = 0; i < m; i++) { - md = month_days[i]; + double y, m, dt, ym, mn, day, h, s, milli, time, tv; + int yi, mi, md, i; + int64_t days; + volatile double temp; /* enforce evaluation order */ + + /* emulate 21.4.1.15 MakeDay ( year, month, date ) */ + y = fields[0]; + m = fields[1]; + dt = fields[2]; + ym = y + floor(m / 12); + mn = fmod(m, 12); + if (mn < 0) + mn += 12; + if (ym < -271821 || ym > 275760) + return NAN; + + yi = ym; + mi = mn; + days = days_from_year(yi); + for(i = 0; i < mi; i++) { + days += month_days[i]; if (i == 1) - md += days_in_year(y) - 365; - days += md; + days += days_in_year(yi) - 365; } - days += fields[2] - 1; - /* made d volatile to ensure order of evaluation as specified in ECMA. - * this fixes a test262 error on - * test262/test/built-ins/Date/UTC/fp-evaluation-order.js + day = days + dt - 1; + + /* emulate 21.4.1.14 MakeTime ( hour, min, sec, ms ) */ + h = fields[3]; + m = fields[4]; + s = fields[5]; + milli = fields[6]; + /* Use a volatile intermediary variable to ensure order of evaluation + * as specified in ECMA. This fixes a test262 error on + * test262/test/built-ins/Date/UTC/fp-evaluation-order.js. + * Without the volatile qualifier, the compile can generate code + * that performs the computation in a different order or with instructions + * that produce a different result such as FMA (float multiply and add). */ - h = fields[3] * 3600000 + fields[4] * 60000 + - fields[5] * 1000 + fields[6]; - d = days * 86400000; - d = d + h; - if (is_local) - d += getTimezoneOffset(d) * 60000; - return time_clip(d); + time = h * 3600000; + time += (temp = m * 60000); + time += (temp = s * 1000); + time += milli; + + /* emulate 21.4.1.16 MakeDate ( day, time ) */ + tv = (temp = day * 86400000) + time; /* prevent generation of FMA */ + if (!isfinite(tv)) + return NAN; + + /* adjust for local time and clip */ + if (is_local) { + int64_t ti = tv < INT64_MIN ? INT64_MIN : tv >= 0x1p63 ? INT64_MAX : (int64_t)tv; + tv += getTimezoneOffset(ti) * 60000; + } + return time_clip(tv); } static JSValue get_date_field(JSContext *ctx, JSValue this_val, @@ -47452,6 +47476,7 @@ static JSValue js_date_getTimezoneOffset(JSContext *ctx, JSValue this_val, if (isnan(v)) return JS_NAN; else + /* assuming -8.64e15 <= v <= -8.64e15 */ return JS_NewInt64(ctx, getTimezoneOffset((int64_t)trunc(v))); } diff --git a/tests/test_builtin.js b/tests/test_builtin.js index 33561a45e..63ac4ffbb 100644 --- a/tests/test_builtin.js +++ b/tests/test_builtin.js @@ -84,14 +84,22 @@ function assert(actual, expected, message) { if (arguments.length == 1) expected = true; - if (actual === expected) - return; - - if (actual !== null && expected !== null - && typeof actual == 'object' && typeof expected == 'object' - && actual.toString() === expected.toString()) - return; - + if (typeof actual === typeof expected) { + if (actual === expected) { + if (actual !== 0 || (1 / actual) === (1 / expected)) + return; + } + if (typeof actual === 'number') { + if (isNaN(actual) && isNaN(expected)) + return true; + } + if (typeof actual === 'object') { + if (actual !== null && expected !== null + && actual.constructor === expected.constructor + && actual.toString() === expected.toString()) + return; + } + } throw Error("assertion failed: got |" + actual + "|" + ", expected |" + expected + "|" + (message ? " (" + message + ")" : "")); @@ -594,20 +602,52 @@ function test_date() assert(d.toISOString(), "2017-09-22T18:10:11.091Z"); a = Date.parse(d.toISOString()); assert((new Date(a)).toISOString(), d.toISOString()); + s = new Date("2020-01-01T01:01:01.123Z").toISOString(); + assert(s, "2020-01-01T01:01:01.123Z"); + // implementation defined behavior s = new Date("2020-01-01T01:01:01.1Z").toISOString(); - assert(s == "2020-01-01T01:01:01.100Z"); + assert(s, "2020-01-01T01:01:01.100Z"); s = new Date("2020-01-01T01:01:01.12Z").toISOString(); - assert(s == "2020-01-01T01:01:01.120Z"); - s = new Date("2020-01-01T01:01:01.123Z").toISOString(); - assert(s == "2020-01-01T01:01:01.123Z"); + assert(s, "2020-01-01T01:01:01.120Z"); s = new Date("2020-01-01T01:01:01.1234Z").toISOString(); - assert(s == "2020-01-01T01:01:01.123Z"); + assert(s, "2020-01-01T01:01:01.123Z"); s = new Date("2020-01-01T01:01:01.12345Z").toISOString(); - assert(s == "2020-01-01T01:01:01.123Z"); + assert(s, "2020-01-01T01:01:01.123Z"); s = new Date("2020-01-01T01:01:01.1235Z").toISOString(); - assert(s == "2020-01-01T01:01:01.124Z"); + assert(s == "2020-01-01T01:01:01.124Z" || // QuickJS + s == "2020-01-01T01:01:01.123Z"); // nodeJS s = new Date("2020-01-01T01:01:01.9999Z").toISOString(); - assert(s == "2020-01-01T01:01:02.000Z"); + assert(s == "2020-01-01T01:01:02.000Z" || // QuickJS + s == "2020-01-01T01:01:01.999Z"); // nodeJS + + assert(Date.UTC(2017), 1483228800000); + assert(Date.UTC(2017, 9), 1506816000000); + assert(Date.UTC(2017, 9, 22), 1508630400000); + assert(Date.UTC(2017, 9, 22, 18), 1508695200000); + assert(Date.UTC(2017, 9, 22, 18, 10), 1508695800000); + assert(Date.UTC(2017, 9, 22, 18, 10, 11), 1508695811000); + assert(Date.UTC(2017, 9, 22, 18, 10, 11, 91), 1508695811091); + + assert(Date.UTC(NaN), NaN); + assert(Date.UTC(2017, NaN), NaN); + assert(Date.UTC(2017, 9, NaN), NaN); + assert(Date.UTC(2017, 9, 22, NaN), NaN); + assert(Date.UTC(2017, 9, 22, 18, NaN), NaN); + assert(Date.UTC(2017, 9, 22, 18, 10, NaN), NaN); + assert(Date.UTC(2017, 9, 22, 18, 10, 11, NaN), NaN); + assert(Date.UTC(2017, 9, 22, 18, 10, 11, 91, NaN), 1508695811091); + + // from test262/test/built-ins/Date/UTC/fp-evaluation-order.js + assert(Date.UTC(1970, 0, 1, 80063993375, 29, 1, -288230376151711740), 29312, + 'order of operations / precision in MakeTime'); + assert(Date.UTC(1970, 0, 213503982336, 0, 0, 0, -18446744073709552000), 34447360, + 'precision in MakeDate'); + + //assert(Date.UTC(2017 - 1e9, 9 + 12e9), 1506816000000); // node fails this + assert(Date.UTC(2017, 9, 22 - 1e10, 18 + 24e10), 1508695200000); + assert(Date.UTC(2017, 9, 22, 18 - 1e10, 10 + 60e10), 1508695800000); + assert(Date.UTC(2017, 9, 22, 18, 10 - 1e10, 11 + 60e10), 1508695811000); + assert(Date.UTC(2017, 9, 22, 18, 10, 11 - 1e12, 91 + 1000e12), 1508695811091); } function test_regexp() From 027bb814f787940dd654d2809724fa360f2f89df Mon Sep 17 00:00:00 2001 From: Charlie Gordon Date: Wed, 21 Feb 2024 14:32:20 +0100 Subject: [PATCH 3/4] Disable date precision tests on win32 and cygwin targets --- tests/test_builtin.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/test_builtin.js b/tests/test_builtin.js index 63ac4ffbb..257bfe447 100644 --- a/tests/test_builtin.js +++ b/tests/test_builtin.js @@ -637,12 +637,14 @@ function test_date() assert(Date.UTC(2017, 9, 22, 18, 10, 11, NaN), NaN); assert(Date.UTC(2017, 9, 22, 18, 10, 11, 91, NaN), 1508695811091); - // from test262/test/built-ins/Date/UTC/fp-evaluation-order.js - assert(Date.UTC(1970, 0, 1, 80063993375, 29, 1, -288230376151711740), 29312, - 'order of operations / precision in MakeTime'); - assert(Date.UTC(1970, 0, 213503982336, 0, 0, 0, -18446744073709552000), 34447360, - 'precision in MakeDate'); - + // TODO: Fix rounding errors on Windows/Cygwin. + if (!['win32', 'cygwin'].includes(os.platform)) { + // from test262/test/built-ins/Date/UTC/fp-evaluation-order.js + assert(Date.UTC(1970, 0, 1, 80063993375, 29, 1, -288230376151711740), 29312, + 'order of operations / precision in MakeTime'); + assert(Date.UTC(1970, 0, 213503982336, 0, 0, 0, -18446744073709552000), 34447360, + 'precision in MakeDate'); + } //assert(Date.UTC(2017 - 1e9, 9 + 12e9), 1506816000000); // node fails this assert(Date.UTC(2017, 9, 22 - 1e10, 18 + 24e10), 1508695200000); assert(Date.UTC(2017, 9, 22, 18 - 1e10, 10 + 60e10), 1508695800000); From 4c2f2c522fc19bd13754aa08e0ce2daf239279d5 Mon Sep 17 00:00:00 2001 From: Charlie Gordon Date: Wed, 21 Feb 2024 21:17:54 +0100 Subject: [PATCH 4/4] Rewrite `set_date_fields` to match the ECMA specification - use `double` arithmetic where necessary to match the spec - use `volatile` to ensure correct order of evaluation and prevent FMA code generation - reject some border cases. - avoid undefined behavior in `double` -> `int64_t` conversions - improved tests/test_builtin.js `assert` function to compare values more reliably. - added some tests in `test_date()` - disable date precision tests on win32 and cygwin targets - remove unused variable --- quickjs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quickjs.c b/quickjs.c index 29bbbe623..a83827d53 100644 --- a/quickjs.c +++ b/quickjs.c @@ -46814,7 +46814,7 @@ static double time_clip(double t) { of the operations */ static double set_date_fields(double fields[], int is_local) { double y, m, dt, ym, mn, day, h, s, milli, time, tv; - int yi, mi, md, i; + int yi, mi, i; int64_t days; volatile double temp; /* enforce evaluation order */