Repository navigation
[JSC] TypedArray indexOf/lastIndexOf/includes: an int32 needle that a Float16Array or Float32Array cannot hold exactly matches nothing - #604
Conversation
… Float16Array or Float32Array cannot hold exactly matches nothing %TypedArray%.prototype.indexOf and lastIndexOf compare with IsStrictlyEqual, and includes with SameValueZero, on Number values. 16777217 is not equal to any element of a Float32Array, because no float has that value. The search converts the needle to the element type first and rejects it when the conversion is not exact. The double path did that. The int32 path did a plain static_cast, so an int32 needle was rounded to the nearest float or half and then matched that neighbour: new Float32Array([16777216]).indexOf(16777217) // 0, expected -1 new Float32Array([2 ** 31]).includes(2 ** 31 - 1) // true, expected false new Float16Array([2048]).indexOf(2049) // 0, expected -1 new Float16Array([Infinity]).indexOf(65520) // 0, expected -1 The same needle read back from a Float64Array (a double-encoded JSValue) already returned -1, so the result depended on the value's internal representation. Make FloatTypedArrayAdaptor and Float16Adaptor check that the int32 round-trips through the element type, like IntegralTypedArrayAdaptor and the double path already do. Float64Array is unaffected: every int32 is a double. * JSTests/stress/typed-array-index-of-int32-needle-not-representable.js: Added. * Source/JavaScriptCore/runtime/TypedArrayAdaptors.h: (JSC::FloatTypedArrayAdaptor::toNativeFromInt32WithoutCoercion): (JSC::Float16Adaptor::toNativeFromInt32WithoutCoercion):
|
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 3 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 (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| for (let i = 0; i < 1e3; ++i) |
There was a problem hiding this comment.
🟡 (optional) New stress test hardcodes 1e3 iterations instead of using testLoopCount, violating JSTests/README.md rule 2 (imported by JSTests/CLAUDE.md): tests must use testLoopCount so the harness can tune iteration count per configuration for tier-up and early exit. Fix: replace for (let i = 0; i < 1e3; ++i) with for (let i = 0; i < testLoopCount; ++i).
Extended reasoning...
JSTests/CLAUDE.md @-imports JSTests/README.md, whose "Adding Tests" section states new tests are required to use testLoopCount to control iteration count because the jsc CLI sets it based on the test configuration (so tests tier up where that matters and exit early where it doesn't). Line 89 of the new test uses a literal 1e3. In eager-tier configurations this may over-iterate (risking the 200ms budget in rule 1), and in no-JIT/interpreter-only configurations it wastes time; more importantly it breaks the mandated convention that lets the harness control tier coverage. Existing stress tests (e.g. JSTests/stress/zero-to-string.js) follow the testLoopCount pattern.
Verification: nit — /home/claude/webkit/JSTests/stress/typed-array-index-of-int32-needle-not-representable.js:89 reads for (let i = 0; i < 1e3; ++i), a hardcoded iteration count. /home/claude/webkit/JSTests/CLAUDE.md line 1 @-imports README.md, and /home/claude/webkit/JSTests/README.md:17-20 states new tests are "required" to "Use testLoopCount … to control how many iterations a test runs. The jsc…
Preview Builds
|
|
Superseded by #609. That change routes |
1 participant
Problem
indexOf,lastIndexOfandincludeson aFloat32ArrayorFloat16Arraymatch a neighbouring element when the needle is an integer-valued Number that the element type cannot hold exactly:The spec compares with IsStrictlyEqual (
indexOf,lastIndexOf) and SameValueZero (includes) on Number values, and16777217 !== 16777216. V8 returns -1 / false for all of the above.The result also depends on how JSC happens to encode the needle. The same value read back from a
Float64Arrayis a double-encoded JSValue and already returns -1, as does any fractional needle. Only int32-encoded needles hit: integer literals,parseIntresults, integer arithmetic.The cause is in
TypedArrayAdaptors.h.toNativeFromValueWithoutCoercionconverts the needle to the element type and rejects it when the conversion is not exact.toNativeFromDoubleWithoutCoerciondoes the round-trip check.FloatTypedArrayAdaptor::toNativeFromInt32WithoutCoercionandFloat16Adaptor::toNativeFromInt32WithoutCoercionare a plainstatic_cast, so an int32 above 2^24 (float) or 2^11 (half) is rounded to the nearest representable value, and a half at or above 65520 becomes an infinity. The search then finds that rounded value. Upstream WebKit has the same code.Fix
Check that the int32 round-trips through the element type and return
std::nulloptwhen it does not, the same idiomIntegralTypedArrayAdaptorand the double path use.Float64Arrayis unaffected because every int32 is a double, and the three callers (genericTypedArrayViewProtoFuncIncludes,IndexOf,LastIndexOf) already treatnulloptas "not found".Verification
JSTests/stress/typed-array-index-of-int32-needle-not-representable.js: Float32 needles around ±2^24 and ±2^31, Float16 needles around ±2048, 65504..65536 and ±2^31, each passed both as an int32 and as the same value read from aFloat64Array, through all three methods, plusFloat64ArrayandInt32Arraycontrols. Fails on the currentjscat the first case (Float32Array 16777217 indexOf: got 0, expected -1), passes with this change.new Float16Array([..., -2048]).indexOf(-2048), where V8 returns -1 for any negative finite half (new Float16Array([-1]).indexOf(-1)is -1 there). That is a V8 bug, JSC returns the index before and after this change.