Repository navigation
Conversation
…wrap, clamp or round the search value indexOf and lastIndexOf compare the search value with each element by IsStrictlyEqual, and includes by SameValueZero. JSC converts the search value to the element type once (toNativeFromValueWithoutCoercion) and then scans the raw storage. That conversion must fail when the element type cannot represent the value exactly. Three of its paths did not: * A BigInt needle went through JSBigInt::toBigInt64 / toBigUInt64, which are the modular ToBigInt64 / ToBigUint64 used for stores. So new BigInt64Array([1n]).includes(2n ** 64n + 1n) and new BigUint64Array([2n ** 64n - 1n]).includes(-1n) were true. * An int32 needle for Float32Array / Float16Array was cast to the element type with no round-trip check, so new Float32Array([16777216]) .indexOf(16777217) and new Float16Array([Infinity]).indexOf(65536) were 0. * A double needle for Uint8ClampedArray was converted with static_cast<uint8_t>(double), which is undefined behavior outside (-1, 256). LLVM 22 (bun links its LTO build with rust-lld) folds the round-trip check uint8 -> double == value into trunc(value) == value, so every integral double matched the low byte of cvttsd2si(value): -1 found 255, and 256, 1e10, 2 ** 31 and Infinity found 0. #205 fixed the same cast in IntegralTypedArrayAdaptor but not here. Add JSBigInt::tryExtractInt64 / tryExtractUInt64 next to tryExtractDouble and use them for the BigInt path. Route the int32 path of the two float adaptors through their checked double path. Convert the Uint8Clamped double needle through truncateDoubleToInt64, as IntegralTypedArrayAdaptor does. Also do the range check before the narrowing cast in the float adaptors' double path, since converting a finite double beyond FLT_MAX (or 65504) to float (or _Float16) is undefined behavior as well. * JSTests/stress/typed-array-index-of-unrepresentable-needle.js: Added. * Source/JavaScriptCore/runtime/JSBigInt.h: * Source/JavaScriptCore/runtime/JSBigIntInlines.h: (JSC::JSBigInt::absoluteAsUInt64): (JSC::JSBigInt::tryExtractDouble): Share the digit gathering with the new helpers. (JSC::JSBigInt::tryExtractInt64): (JSC::JSBigInt::tryExtractUInt64): * Source/JavaScriptCore/runtime/ToNativeFromValue.h: (JSC::toNativeFromValueWithoutCoercion): * Source/JavaScriptCore/runtime/TypedArrayAdaptors.h: (JSC::FloatTypedArrayAdaptor::toNativeFromInt32WithoutCoercion): (JSC::FloatTypedArrayAdaptor::toNativeFromDoubleWithoutCoercion): (JSC::Uint8ClampedAdaptor::toNativeFromDoubleWithoutCoercion): (JSC::Float16Adaptor::toNativeFromInt32WithoutCoercion): (JSC::Float16Adaptor::toNativeFromDoubleWithoutCoercion):
|
Warning Review limit reached
On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file. Or wait 5 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Comment |
|
Overlap note: #604 (the Float32 / Float16 int32 path) and #607 (the BigInt64 / BigUint64 path) were opened for the two sibling reports while this was in progress. This PR changes the same helper and covers both of those paths plus the Uint8Clamped double path. Update: #604 and #607 are now closed in favour of this PR, and their Bun bumps oven-sh/bun#42083 and oven-sh/bun#42089 in favour of oven-sh/bun#42093, which carries their test rows. This is the only one of the three to review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Preview Builds
|
JSTests/README.md asks new stress tests to iterate by testLoopCount so the harness can scale them per configuration. Each pass makes several hundred builtin calls, so use testLoopCount / 50 (200 passes by default, 31ms in Release; 8ms with --useJIT=0).
|
Bun-side bump pinned to |
1 participant
Problem
%TypedArray%.prototype.indexOf,lastIndexOfandincludesmatch an element that is not there when the search value is outside what the element type can hold. Three paths of the one conversion helper,toNativeFromValueWithoutCoercion<Adaptor>()(runtime/ToNativeFromValue.h), wrap, round or truncate the needle instead of reporting "not representable":new BigInt64Array([1n]).includes(2n ** 64n + 1n)andnew BigUint64Array([2n ** 64n - 1n]).includes(-1n)aretrue. The path callsJSBigInt::toBigInt64/toBigUInt64, the modular ToBigInt64 / ToBigUint64 meant for stores.new Float32Array([16777216]).indexOf(16777217)andnew Float16Array([Infinity]).indexOf(65536)are0.toNativeFromInt32WithoutCoercioncasts with no round-trip check.u8c.indexOf(-1)finds a 255 and256,1e10,2 ** 31,Infinityfind a 0, in bun's release build only.Uint8ClampedAdaptor::toNativeFromDoubleWithoutCoercion(runtime/TypedArrayAdaptors.h:339) doesstatic_cast<uint8_t>(double), undefined behavior outside (-1, 256). Merge upstream/main (013da9aa8cc1) + cherry-pick WebKit#63906 #205 replaced the same cast inIntegralTypedArrayAdaptorbut not this one.Fix
JSBigInt::tryExtractInt64/tryExtractUInt64next totryExtractDouble(they share a newabsoluteAsUInt64helper) and use them for the BigInt path. They returnstd::nulloptunless the BigInt is in [-2^63, 2^63) or [0, 2^64).FloatTypedArrayAdaptorandFloat16Adaptor: the int32 path now goes through the checked double path (an int32 converts to double exactly). The double path does its range check before the narrowing cast instead of after, because converting a finite double beyond FLT_MAX (or 65504) tofloat(or_Float16) is undefined behavior too.Uint8ClampedAdaptor: convert throughtruncateDoubleToInt64()and then narrow, the form Merge upstream/main (013da9aa8cc1) + cherry-pick WebKit#63906 #205 gaveIntegralTypedArrayAdaptor. The 64-bit truncation is an intrinsic with a defined overflow result, so the optimizer cannot fold the round-trip check away.JSTests/stress/typed-array-index-of-unrepresentable-needle.js(new) covers all 12 element types with int32, double and BigInt needles at and beyond each type's limits, plus -0, NaN and the double-array source from the original report. With a Release JSCOnly build of this branch it passes under default options,--useJIT=0and eager tiering. The unpatched engine fails it atFloat32Array int32 2^24 + 1 indexOf: expected -1 but got 0(any build) and atUint8ClampedArray double -1 indexOf: expected -1 but got 2(bun 1.4.2 release). The 714 test262 cases underbuilt-ins/TypedArray/prototype/{indexOf,lastIndexOf,includes}andbuilt-ins/TypedArrayConstructors/internalspass (strict and sloppy), as do the 266 typed-array, DataView and BigInt64 stress tests that run without ICU data.Background
indexOf,lastIndexOf) or SameValueZero (includes). JSC instead converts the search value to the element type once and scans raw storage withWTF::find8/16/32/64/Float. That is only equivalent when the conversion is exact, so the helper returnsstd::optionaland the callers answer -1 / false onnullopt(with the detached /undefinedspecial case forincludes).fptoui double to i8, which is poison out of range, and the checkstatic_cast<double>(integer) != valuetouitofp+fcmp. LLVM 22's DAG combiner foldsuitofp (fptoui x)toftrunc xwhenever the sign of zero cannot be observed (canIgnoreSignBitOfZero), which anfcmpuse satisfies. LLVM 21 only did this under-fno-signed-zeros. The WebKit prebuilt is compiled by clang 21, but bun's release link is ThinLTO through rust-lld (LLD 22.1.8), so the-ltoprebuilt's bitcode is code-generated by LLVM 22. The check becomesroundsd $0xb+ucomisd("isvalueintegral?") and the needle is the low byte of a 32-bitcvttsd2si, which is0x80000000for anything outside int32 range. bun 1.4.2'stypedArrayViewProtoFuncIncludeshas exactly that sequence at+0xcfb.Notes
toNativeFromDoubleWithoutCoercionto bitcode with clang 21 (-O3 -flto=thin) and linking with--ld-path=<rustup nightly-2026-07-20>/gcc-ld/ld.lldproducescvttsd2si %xmm0,%eax; roundsd $0xb,%xmm0,%xmm1and reports1e10found; thetruncateDoubleToInt64form producescvttsd2si %xmm0,%rax; cvtsi2sd %ecx,%xmm1and is correct. Plain clang 21-O3keeps the round trip for both, which is why the non-LTO prebuilt (localbun run build:release, the release-asan build) never showed it.cvttsd2sigives0x80000000out of range, so its low byte is 0). Any other release target whose LTO link code-generates with LLVM 22 folds the check the same way but searches for a target-specific byte (arm64fcvtzusaturates, so a double-1would match a 0 rather than a 255). Non-LTO builds of every target keep the round trip.tryExtractDoublekeeps its behavior; it now callsabsoluteAsUInt64for the digit gathering it used to inline.toBigInt64/toBigUInt64are unchanged and still used for stores andAtomics, where the modular conversion is the specified behavior.mainhas the same code in all three places.