Bug 229826 – [JSC] Implement Temporal.Instant

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Philip Chimento]

Temporal.Instant and its methods except for toZonedDateTime/toZonedDateTimeISO, and Temporal.Now.instant()

Blocks bug 223166

[Philip Chimento]

Created attachment 437328 [details]
Patch

[Philip Chimento]

Created attachment 437329 [details]
Various tweaks in preparation for Temporal.Instant

[Philip Chimento]

Created attachment 437332 [details]
WIP - Temporal.Instant - not ready for review

[Philip Chimento]

The WIP patch doesn't cover string parsing yet. Additionally one corner that I intentionally cut was to store epochNanoseconds as a double, so I'll have to go back and change that. It actually requires at least 74 bits to store with the required precision.

I haven't decided yet if it's best to store epochNanoseconds as a JSBigInt in TemporalInstant, or, e.g. store epochMilliseconds and up to 1e6 nanoseconds as separate integers, or even store epochMilliseconds as a double (using that convenient WTF::Seconds type) and nanoseconds separately.

[Radar WebKit Bug Importer]

<rdar://problem/82940066>

[Philip Chimento]

Created attachment 438302 [details]
Various tweaks in preparation for Temporal.Instant

[Philip Chimento]

Created attachment 438304 [details]
Various tweaks in preparation for Temporal.Instant

[Yusuke Suzuki]

Comment on attachment 438304 [details]
Various tweaks in preparation for Temporal.Instant

Talked with Philip. This patch is not "Implement Temporal.Instant", so it should be uploaded in a different bugzilla.

[Philip Chimento]

Created attachment 439114 [details]
Patch for preliminary review

[Philip Chimento]

There are still a few outstanding FIXMEs and I need to add more tests, but I think this patch has reached the point where some preliminary review is worth it, especially for a relative JSC newbie like me. I'll also be AFK until Monday so that's another reason I'm uploading something now.

(I think the 3 style checker failures are spurious: there is a missing copyright in wtf/Int128.h, and the variable name checker flags `unsigned s` and `unsigned ns` as unnecessary whereas the other arguments to that function (y, mon, d, h, min, ms, micros) are OK)

[Yusuke Suzuki]

Comment on attachment 439114 [details]
Patch for preliminary review

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

Nice, some quick comments.

> Source/JavaScriptCore/runtime/ISO8601.h:185
> +    static constexpr Int128 DAY_RANGE_S {86400'00000000}; // 1e8 days
> +    static constexpr Int128 NS_PER_MICROS {1000};
> +    static constexpr Int128 NS_PER_MS {1'000'000};
> +    static constexpr Int128 NS_PER_S {1'000'000'000};
> +    static constexpr Int128 NS_PER_MINUTE = NS_PER_S * 60;
> +    static constexpr Int128 NS_PER_HOUR = NS_PER_MINUTE * 60;
> +    static constexpr Int128 MIN_VALUE = -DAY_RANGE_S * NS_PER_S;
> +    static constexpr Int128 MAX_VALUE = DAY_RANGE_S * NS_PER_S;

We do not use capital constants. Rename them like, dayRangeSeconds etc.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:251
> +    if ((unsignedValue & 0xffff'ffff'ffff'ffff) == unsignedValue)

unsignedValue <= UINT64_MAX would be simpler.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:254
> +    if (sizeof(Digit) == 8) {

Use `if constexpr`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:259
> +        Digit lowBits = static_cast<Digit>(unsignedValue & 0xffff'ffff'ffff'ffff);
> +        Digit highBits = static_cast<Digit>((unsignedValue >> 64) & 0xffff'ffff'ffff'ffff);

I think we do not need these &. Just casting is suffice.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:274
> +    Digit digit0 = static_cast<Digit>(unsignedValue & 0xffff'ffff);
> +    Digit digit1 = static_cast<Digit>((unsignedValue >> 32) & 0xffff'ffff);
> +    Digit digit2 = static_cast<Digit>((unsignedValue >> 64) & 0xffff'ffff);
> +    Digit digit3 = static_cast<Digit>((unsignedValue >> 96) & 0xffff'ffff);

Ditto.

[Philip Chimento]

Created attachment 439371 [details]
Patch for preliminary review

[Philip Chimento]

Created attachment 439756 [details]
Patch for preliminary review

[Yusuke Suzuki]

Comment on attachment 439756 [details]
Patch for preliminary review

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

Commented.
Please update DerivedSources-input.xcfilelist and DerivedSources-output.xcfilelist too.

> Source/JavaScriptCore/runtime/ISO8601.cpp:907
> +        return {ExactTime::fromISOPartsAndOffset(date.year(), date.month(), date.day(), time.hour(), time.minute(), time.second(), time.millisecond(), time.microsecond(), time.nanosecond(), offset)};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1017
> +    double utcMs = (dateDays * msPerDay + timeMs);

() is not necessary.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1018
> +    Int128 utcNs = Int128 {static_cast<int64_t>(utcMs)} * 1'000'000 + micros * 1000 + ns;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1019
> +    return ExactTime {utcNs - offset};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1038
> +    resultNs += Int128 {static_cast<int64_t>(duration.hours())} * ExactTime::nsPerHour;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1041
> +    resultNs += Int128 {static_cast<int64_t>(duration.minutes())} * ExactTime::nsPerMinute;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1044
> +    resultNs += Int128 {static_cast<int64_t>(duration.seconds())} * ExactTime::nsPerSecond;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1047
> +    resultNs += Int128 {static_cast<int64_t>(duration.milliseconds())} * ExactTime::nsPerMillisecond;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1050
> +    resultNs += Int128 {static_cast<int64_t>(duration.microseconds())} * ExactTime::nsPerMicrosecond;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1051
> +    resultNs += Int128 {static_cast<int64_t>(duration.nanoseconds())};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1053
> +    ExactTime result {resultNs};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1056
> +    return {result};

{} is not necessary.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1061
> +    Int128 incrementNs {increment};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1085
> +    return ExactTime {round(m_epochNanoseconds, increment, unit, roundingMode)};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.h:88
> +    constexpr ExactTime(const ExactTime& other) : m_epochNanoseconds(other.m_epochNanoseconds) { }

This is not necessary.

> Source/JavaScriptCore/runtime/ISO8601.h:92
> +        return ExactTime(Int128 {epochSeconds} * ExactTime::nsPerSecond);

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.h:96
> +        return ExactTime(Int128 {epochMilliseconds} * ExactTime::nsPerMillisecond);

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.h:100
> +        return ExactTime(Int128 {epochMicroseconds} * ExactTime::nsPerMicrosecond);

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.h:139
> +    String print() const
> +    {
> +        StringBuilder builder;
> +        if (m_epochNanoseconds < 0) {
> +            builder.append('-');
> +            printImpl(builder, -m_epochNanoseconds);
> +        } else
> +            printImpl(builder, m_epochNanoseconds);
> +        return builder.toString();
> +    }

"print" function should be used for printing string to dataLog. Please rename it.

> Source/JavaScriptCore/runtime/ISO8601.h:181
> +    static constexpr Int128 dayRangeSeconds {86400'00000000}; // 1e8 days
> +    static constexpr Int128 nsPerMicrosecond {1000};
> +    static constexpr Int128 nsPerMillisecond {1'000'000};
> +    static constexpr Int128 nsPerSecond {1'000'000'000};

Add space around { and }.

> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:99
> +        return dateNowImpl().toNumber(globalObject);

Use RELEASE_AND_RETURN(scope, dateNowImpl().toNumber(globalObject));

> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:111
> +    auto scope = DECLARE_THROW_SCOPE(vm);

VM& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);
should be defined in the prologue of the function.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:58
> +void TemporalInstant::finishCreation(VM& vm)
> +{
> +    Base::finishCreation(vm);
> +    ASSERT(inherits(vm, info()));
> +}

We do not need to define it.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:91
> +    JSBigInt* bigint = asHeapBigInt(epochNanoseconds);

Add BIGINT32 case.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:94
> +    ISO8601::ExactTime exactTime {(high << 64 | low) * (bigint->sign() ? -1 : 1)};

Add space around {.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:97
> +        String argAsString {bigint->toString(globalObject, 10)};

In this case, use argsAsString = bigint->toString(globalObject, 10)

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:120
> +    // TODO: when Temporal.ZonedDateTime lands

TODO is not used in WebKit. Use FIXME

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:200
> +    JSBigInt* bigint = asHeapBigInt(epochMicroseconds);

Add BIGINT32 case.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:222
> +    VM& vm = globalObject->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);
> +

This is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:281
> +    double seconds {static_cast<double>(static_cast<int64_t>(roundedDiff / 1'000'000'000))};

Add space around {.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:282
> +    double nanosecondsRemainder {static_cast<double>(static_cast<int64_t>(roundedDiff % 1'000'000'000))};

Add space around {.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:283
> +    ISO8601::Duration result {0, 0, 0, 0, 0, 0, seconds, 0, 0, nanosecondsRemainder};

Add space around {.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:406
> +    char buffer[20];
> +    // If the year is outside the bounds of 0 and 9999 inclusive we want to use
> +    // the extended year format (PadISOYear).
> +    int charactersWritten;
> +    if (gregorianDateTime.year() > 9999 || gregorianDateTime.year() < 1000)
> +        charactersWritten = snprintf(buffer, sizeof(buffer), "%+07d-%02d-%02dT%02d:%02d", gregorianDateTime.year(), gregorianDateTime.month() + 1, gregorianDateTime.monthDay(), gregorianDateTime.hour(), gregorianDateTime.minute());
> +    else
> +        charactersWritten = snprintf(buffer, sizeof(buffer), "%04d-%02d-%02dT%02d:%02d", gregorianDateTime.year(), gregorianDateTime.month() + 1, gregorianDateTime.monthDay(), gregorianDateTime.hour(), gregorianDateTime.minute());
> +    ASSERT(charactersWritten > 0 && static_cast<unsigned>(charactersWritten) < sizeof(buffer));
> +    if (static_cast<unsigned>(charactersWritten) >= sizeof(buffer))
> +        return ""_s;
> +
> +    StringBuilder builder;
> +    builder.append(buffer);

Let's not use snprintf. It is very slow compared to the other way. You can use makeString, pad() etc. instead.

> Source/JavaScriptCore/runtime/TemporalInstantPrototype.cpp:121
> +    const std::optional<ISO8601::ExactTime> newExactTime = instant->exactTime().add(duration);

We don't use const in this case.

> Source/JavaScriptCore/runtime/TemporalInstantPrototype.cpp:142
> +    const std::optional<ISO8601::ExactTime> newExactTime = instant->exactTime().add(-duration);

We don't use const in this case.

[Philip Chimento]

(In reply to Yusuke Suzuki from comment #14)
> > Source/JavaScriptCore/runtime/TemporalInstant.cpp:406
> > +    char buffer[20];
> > +    // If the year is outside the bounds of 0 and 9999 inclusive we want to use
> > +    // the extended year format (PadISOYear).
> > +    int charactersWritten;
> > +    if (gregorianDateTime.year() > 9999 || gregorianDateTime.year() < 1000)
> > +        charactersWritten = snprintf(buffer, sizeof(buffer), "%+07d-%02d-%02dT%02d:%02d", gregorianDateTime.year(), gregorianDateTime.month() + 1, gregorianDateTime.monthDay(), gregorianDateTime.hour(), gregorianDateTime.minute());
> > +    else
> > +        charactersWritten = snprintf(buffer, sizeof(buffer), "%04d-%02d-%02dT%02d:%02d", gregorianDateTime.year(), gregorianDateTime.month() + 1, gregorianDateTime.monthDay(), gregorianDateTime.hour(), gregorianDateTime.minute());
> > +    ASSERT(charactersWritten > 0 && static_cast<unsigned>(charactersWritten) < sizeof(buffer));
> > +    if (static_cast<unsigned>(charactersWritten) >= sizeof(buffer))
> > +        return ""_s;
> > +
> > +    StringBuilder builder;
> > +    builder.append(buffer);
> 
> Let's not use snprintf. It is very slow compared to the other way. You can
> use makeString, pad() etc. instead.

Got it, thanks. Originally I adapted this code from dateProtoFuncToISOString() in runtime/DatePrototype.cpp; I assume we should not change that?

[Philip Chimento]

(In reply to Yusuke Suzuki from comment #14)
> > Source/JavaScriptCore/runtime/ISO8601.h:88
> > +    constexpr ExactTime(const ExactTime& other) : m_epochNanoseconds(other.m_epochNanoseconds) { }
> 
> This is not necessary.

I remembered why I put this in; it seems that Clang doesn't correctly generate the copy constructor when we have a __int128 member. I've added a comment explaining why this "useless" copy constructor is here, and will try to isolate a minimum reproducible example and hopefully report a Clang bug. (Or find out what my mistake was :-)

[Yusuke Suzuki]

Comment on attachment 439756 [details]
Patch for preliminary review

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

Added some more style nits.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:170
> +const ISO8601::Duration TemporalDuration::toLimitedDuration(JSGlobalObject* globalObject, JSValue itemValue, std::initializer_list<TemporalUnit> disallowedUnits)

This const is not necessary.

> Source/JavaScriptCore/runtime/TemporalDuration.h:49
> +    static const ISO8601::Duration toLimitedDuration(JSGlobalObject*, JSValue, std::initializer_list<TemporalUnit> disallowedUnits);

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:48
> +TemporalInstant::TemporalInstant(VM& vm, Structure* structure, const ISO8601::ExactTime exactTime)

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:60
> +TemporalInstant* TemporalInstant::create(VM& vm, Structure* structure, const ISO8601::ExactTime exactTime)

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:70
> +TemporalInstant* TemporalInstant::tryCreateIfValid(JSGlobalObject* globalObject, const ISO8601::ExactTime exactTime, Structure* structure)

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:246
> +const ISO8601::Duration TemporalInstant::difference(JSGlobalObject* globalObject, TemporalInstant* other, JSValue optionsValue) const

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:308
> +const ISO8601::ExactTime TemporalInstant::round(JSGlobalObject* globalObject, JSValue optionsValue) const

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:385
> +String TemporalInstant::toString(const ISO8601::ExactTime exactTime, JSObject* timeZone, PrecisionData precision)

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.h:46
> +    static TemporalInstant* create(VM&, Structure*, const ISO8601::ExactTime);

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.h:47
> +    static TemporalInstant* tryCreateIfValid(JSGlobalObject*, const ISO8601::ExactTime, Structure* = nullptr);

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.h:61
> +    constexpr const ISO8601::ExactTime exactTime() const { return m_exactTime; }

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.h:64
> +    const ISO8601::Duration difference(JSGlobalObject*, TemporalInstant*, JSValue options) const;
> +    const ISO8601::ExactTime round(JSGlobalObject*, JSValue options) const;

const iISO8601::XXX's const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.h:72
> +    TemporalInstant(VM&, Structure*, const ISO8601::ExactTime);

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.h:77
> +    static const ISO8601::ExactTime fromObject(JSGlobalObject*, JSObject*);

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.h:79
> +    static String toString(const ISO8601::ExactTime, JSObject* timeZone, PrecisionData);

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstantPrototype.cpp:118
> +    const ISO8601::Duration duration = TemporalDuration::toLimitedDuration(globalObject, callFrame->argument(0), disallowedAdditionUnits);

const is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstantPrototype.cpp:139
> +    const ISO8601::Duration duration = TemporalDuration::toLimitedDuration(globalObject, callFrame->argument(0), disallowedAdditionUnits);

const is not necessary.

[Yusuke Suzuki]

 (In reply to Philip Chimento from comment #16)
> (In reply to Yusuke Suzuki from comment #14)
> > > Source/JavaScriptCore/runtime/ISO8601.h:88
> > > +    constexpr ExactTime(const ExactTime& other) : m_epochNanoseconds(other.m_epochNanoseconds) { }
> > 
> > This is not necessary.
> 
> I remembered why I put this in; it seems that Clang doesn't correctly
> generate the copy constructor when we have a __int128 member. I've added a
> comment explaining why this "useless" copy constructor is here, and will try
> to isolate a minimum reproducible example and hopefully report a Clang bug.
> (Or find out what my mistake was :-)

I think probably I found what is happening. This statement itself is not related. So let's remove that. The problem is that, `alignof(__int128_t)` is 16!, which is probably only the data type usually we see.
JSC GC-managed object is allocated with 8byte alignment (for MarkBlock allocations, it is 16-byte aligned. But for PreciseAllocation, it is 8-byte aligned. And first several objects of the same type can be allocated from PreciseAllocation). This means that, 16 byte alignment requirement can be broken.

I reproduced JSC EWS crash locally. And the crash is

    0x102ecb2f4 <+164>: jne    0x102ecb325               ; <+213> at TemporalInstant.cpp:351:20
    0x102ecb2f6 <+166>: xorl   %eax, %eax
    0x102ecb2f8 <+168>: movq   %rax, -0x80(%rbp)
    0x102ecb2fc <+172>: jmp    0x102ecb344               ; <+244> at TemporalInstant.cpp:355:26
->  0x102ecb2fe <+174>: movaps 0x10(%r12), %xmm0
    0x102ecb304 <+180>: movaps %xmm0, -0x70(%rbp)
    0x102ecb308 <+184>: leaq   -0x70(%rbp), %rsi
    0x102ecb30c <+188>: movabsq $0x100000009, %r8         ; imm = 0x100000009
    0x102ecb316 <+198>: movl   $0x2, %ecx

And r12 is 0x00000001005958d8. So 0x10(%r12) is not 16-byte aligned while the compiler is using movaps since it assumes that TemporalInstant is allocated with 16byte aligned, but it is not.

For now, you can just try simple way like this. https://gist.github.com/Constellation/71dd37e75b013e3130104e64a85e254b

[Philip Chimento]

Created attachment 440419 [details]
Patch (in progress)

[Philip Chimento]

Thanks for the comments. Here's a new patch - no need to review it right away, I'm mainly posting it to make sure the bots now pass it. I'll be adding some more tests and trying to resolve the remaining open questions.

[Philip Chimento]

Created attachment 440440 [details]
Patch (in progress)

[Philip Chimento]

Created attachment 441022 [details]
Patch

[Philip Chimento]

I rebased the patch again. I still need to add more tests, so this isn't 100% ready for review, but any thoughts on the remaining FIXMEs would be appreciated. I will intend to resume this next week.

[Yusuke Suzuki]

Comment on attachment 441022 [details]
Patch

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

Commented about HeapBigInt.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:85
> +    JSBigInt* bigint = asHeapBigInt(epochNanoseconds);

This is not correct. In BIGINT32 environment, epochMicroseconds can be BigInt32. So we should check it first before casting it to HeapBigInt via asHeapBigInt.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:210
> +    JSBigInt* bigint = asHeapBigInt(epochMicroseconds);
> +#if USE(BIGINT32)
> +    bool bigIntTooLong = bigint->length() > 2;
> +#else
> +    bool bigIntTooLong = bigint->length() > 1;
> +#endif
> +    ISO8601::ExactTime exactTime = ISO8601::ExactTime::fromEpochMicroseconds(JSBigInt::toBigUInt64(bigint));

This is not correct. In BIGINT32 environment, epochMicroseconds can be BigInt32. So we should check it first before casting it to HeapBigInt via asHeapBigInt.

[Philip Chimento]

Created attachment 442118 [details]
Patch

[Philip Chimento]

Created attachment 442122 [details]
Patch

[Philip Chimento]

Created attachment 442242 [details]
Patch

[Philip Chimento]

Created attachment 442414 [details]
Patch

[Philip Chimento]

Created attachment 442434 [details]
Patch

[Philip Chimento]

Created attachment 442436 [details]
Patch

[Philip Chimento]

This patch temporarily contains the one from bug 232270, should be rebased when that one lands

[Philip Chimento]

Created attachment 442445 [details]
Patch

[Philip Chimento]

Created attachment 442498 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 442498 [details]
Patch

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

r=me with nits.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:109
> +    if constexpr (sizeof(JSBigInt::Digit) == 4) {
> +        Int128 d0 { bigint->length() > 0 ? bigint->digit(0) : 0 };
> +        Int128 d1 { bigint->length() > 1 ? bigint->digit(1) : 0 };
> +        Int128 d2 { bigint->length() > 2 ? bigint->digit(2) : 0 };
> +        total = d2 << 64 | d1 << 32 | d0;
> +        bigIntTooLong = bigint->length() > 3;
> +    } else {
> +        ASSERT(sizeof(JSBigInt::Digit) == 8);
> +        Int128 low { bigint->length() > 0 ? bigint->digit(0) : 0 };
> +        Int128 high { bigint->length() > 1 ? bigint->digit(1) : 0 };
> +        total = high << 64 | low;
> +        bigIntTooLong = bigint->length() > 2;
> +    }

INT128_MIN case is not handled properly (-1 * INT128_MIN is undefined).
Can you check it? (and can you add a test?)

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:233
> +    int64_t microseconds = JSBigInt::toBigInt64(bigint);

It is not handling negative Int128 ranges (if JSBigInt's 2 digits are larger than INT128_MAX, then if it is signed, it is not within Int128 range).
Can you fix it and add test for this?

> Source/JavaScriptCore/runtime/TemporalObject.cpp:470
> +

Let's add `ASSERT(increment != 0)`

[Philip Chimento]

Created attachment 443019 [details]
Patch

[Philip Chimento]

(In reply to Yusuke Suzuki from comment #34)
> > Source/JavaScriptCore/runtime/TemporalInstant.cpp:233
> > +    int64_t microseconds = JSBigInt::toBigInt64(bigint);
> 
> It is not handling negative Int128 ranges (if JSBigInt's 2 digits are larger
> than INT128_MAX, then if it is signed, it is not within Int128 range).
> Can you fix it and add test for this?

I guess you mean INT64_MAX here?

I've fixed these cases, added tests, and added the assertion.

Additionally I adjusted the test with the BigInt('9'.repeat(2147483648)) case, because I suspect that constructing that string was what was making the jsc-armv7 tests fail. (Anyway, the test was not actually testing what we thought it would, because BigInts that large are not allowed.)

Hopefully ready to land this time.

[EWS]

Committed r285178 (243811@main): <https://commits.webkit.org/243811@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443019 [details].