Switch Linux LTO builds to ThinLTO (stacked on #244)#246
Conversation
static_cast<int> of a double outside the int32 range is undefined
behavior. Newer compilers assume the cast is in-range and elide the
following round-trip check, so parseInt results >= 2^31 (e.g.
parseInt("80000000", 16)) are boxed as a negative int32 instead of the
correct double. Use truncateDoubleToInt32() for a defined conversion.
LiteralParser JSONP index, JSValue::getUInt32, and String.fromCodePoint each cast a double to a narrow int and validate with a round-trip check the compiler may delete (the cast is UB out of range). Make the cast defined / guard the range before narrowing.
The ToInteger argument was cast to int before the range check, so an out-of-range or infinite value hit UB. Range-check the double first.
weakMapEntries/iteratorEntries cast a ToIntegerOrInfinity result to unsigned; +Infinity passes the >= 0 guard and makes the cast UB. clampTo defines it.
truncateDoubleToInt32 makes the conversion defined for all doubles, so the existing v == d round-trip check is enough on its own.
truncateDoubleToInt32 returns an architecture sentinel for d >= 2^31, so the round-trip check would have rejected valid uint32 indices in [2^31, 2^32). truncateDoubleToUint32 returns d exactly for d in [0, 2^32) and is in scope via the existing MathExtras.h include.
JSValue(double) already calls tryConvertToStrictInt32 and boxes int32 iff exactly representable, else as double — this function was duplicating that. The one-liner is bit-identical and removes the UB by deletion.
Restore the original cast-then-round-trip structure; only the cast is made defined via truncateDoubleToUint32 so the value is bit-identical for every input the original cast was defined on.
WalkthroughThis PR makes two independent updates: build infrastructure configuration switches from full LTO to ThinLTO compilation across workflows and Dockerfiles, and JavaScriptCore numeric conversions are refactored to use dedicated truncation functions instead of direct C++ casts, with improved argument validation semantics in number formatting. ChangesThinLTO Configuration Across Build Infrastructure
JavaScriptCore Numeric Conversion Semantics
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/JavaScriptCore/runtime/NumberPrototype.cpp (1)
441-444:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winApply the same UB fix to
toFixedfor consistency.The
toFixedfunction still uses the old pattern wheretoIntegerOrInfinityis cast tointbefore range validation. If the argument converts to ±Infinity, thestatic_cast<int>on line 441 exhibits undefined behavior before the range check on line 443 can reject it.The same fix applied to
toExponentialandtoPrecisionshould be applied here: store the result in adouble, validate the range on the double, then cast toint.🔧 Proposed fix matching toExponential/toPrecision pattern
- int decimalPlaces = static_cast<int>(callFrame->argument(0).toIntegerOrInfinity(globalObject)); + double decimalPlacesDouble = callFrame->argument(0).toIntegerOrInfinity(globalObject); RETURN_IF_EXCEPTION(scope, { }); - if (decimalPlaces < 0 || decimalPlaces > 100) + if (decimalPlacesDouble < 0 || decimalPlacesDouble > 100) return throwVMRangeError(globalObject, scope, "toFixed() argument must be between 0 and 100"_s); + int decimalPlaces = static_cast<int>(decimalPlacesDouble);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/JavaScriptCore/runtime/NumberPrototype.cpp` around lines 441 - 444, The toFixed implementation in NumberPrototype.cpp currently casts callFrame->argument(0).toIntegerOrInfinity(globalObject) directly to int (static_cast<int>) which UB if the value is ±Infinity; change it to first store the result in a double (e.g. double decimalPlacesDouble = callFrame->argument(0).toIntegerOrInfinity(globalObject)), check for exceptions, validate the double is within 0..100 (and not ±Infinity) and only then cast to int (int decimalPlaces = static_cast<int>(decimalPlacesDouble)); update references to decimalPlaces accordingly in the toFixed function to match the toExponential/toPrecision pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.ts`:
- Around line 197-198: The Windows LTO flags in the build config are incomplete:
update the CMake flag strings for CMAKE_C_FLAGS and CMAKE_CXX_FLAGS so they
match the workflow by using the /clang: prefixed ThinLTO flags and include
-fno-split-lto-unit; specifically replace the "-flto=thin" entry in the
CMAKE_C_FLAGS string and the "/clang:-fno-c++-static-destructors -flto=thin"
portion in the CMAKE_CXX_FLAGS string with "/clang:-flto=thin
/clang:-fno-split-lto-unit" (keeping existing tokens like
/DU_STATIC_IMPLEMENTATION and /clang:-fno-c++-static-destructors) so local
Windows builds use the same "/clang:-flto=thin /clang:-fno-split-lto-unit" flags
as CI.
- Around line 203-204: Update the non-Windows CMake LTO flags so they match
CI/Docker configuration: replace the current "-DCMAKE_C_FLAGS=-flto=thin" and
"-DCMAKE_CXX_FLAGS=-flto=thin" entries with flags that include "-flto=thin
-fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables"; ensure you
change both the C and CXX flag strings in the same place where these options are
added so local non-Windows builds use the identical LTO settings as CI.
In `@Dockerfile`:
- Line 4: The ARG LTO_FLAG value contains a trailing space; update the
Dockerfile ARG LTO_FLAG definition to remove the trailing whitespace from the
flag string (the symbol to change is ARG LTO_FLAG) so the value ends with
"-fforce-emit-vtables" without the extra space.
In `@Dockerfile.musl`:
- Line 4: The ARG LTO_FLAG value in Dockerfile.musl contains a trailing space;
remove the trailing space from the string assigned to ARG LTO_FLAG (the
"-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables "
value) so the flag list is clean and consistent while keeping the same flags and
spacing between them.
---
Outside diff comments:
In `@Source/JavaScriptCore/runtime/NumberPrototype.cpp`:
- Around line 441-444: The toFixed implementation in NumberPrototype.cpp
currently casts callFrame->argument(0).toIntegerOrInfinity(globalObject)
directly to int (static_cast<int>) which UB if the value is ±Infinity; change it
to first store the result in a double (e.g. double decimalPlacesDouble =
callFrame->argument(0).toIntegerOrInfinity(globalObject)), check for exceptions,
validate the double is within 0..100 (and not ±Infinity) and only then cast to
int (int decimalPlaces = static_cast<int>(decimalPlacesDouble)); update
references to decimalPlaces accordingly in the toFixed function to match the
toExponential/toPrecision pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e5b66ab-7ae1-4f14-900b-ee7ba3490247
📒 Files selected for processing (11)
.github/workflows/build-reusable.ymlDockerfileDockerfile.muslSource/JavaScriptCore/dfg/DFGOperations.cppSource/JavaScriptCore/inspector/JSInjectedScriptHost.cppSource/JavaScriptCore/llint/LLIntSlowPaths.cppSource/JavaScriptCore/runtime/JSCJSValue.hSource/JavaScriptCore/runtime/LiteralParser.cppSource/JavaScriptCore/runtime/NumberPrototype.cppSource/JavaScriptCore/runtime/StringConstructor.cppbuild.ts
💤 Files with no reviewable changes (1)
- Source/JavaScriptCore/dfg/DFGOperations.cpp
| "-DCMAKE_C_FLAGS=/DU_STATIC_IMPLEMENTATION -flto=thin", | ||
| "-DCMAKE_CXX_FLAGS=/DU_STATIC_IMPLEMENTATION /clang:-fno-c++-static-destructors -flto=thin", |
There was a problem hiding this comment.
Incomplete ThinLTO flags for Windows.
The Windows LTO configuration is missing flags compared to the workflow configuration. The workflow uses /clang:-flto=thin /clang:-fno-split-lto-unit (lines 185, 205 of the workflow file), but this uses only -flto=thin without the /clang: prefix or -fno-split-lto-unit.
This inconsistency means local Windows LTO builds will differ from CI builds, potentially missing the benefits of -fno-split-lto-unit that the PR objectives specifically mention adding.
🔧 Proposed fix to match workflow configuration
flags.push(
- "-DCMAKE_C_FLAGS=/DU_STATIC_IMPLEMENTATION -flto=thin",
- "-DCMAKE_CXX_FLAGS=/DU_STATIC_IMPLEMENTATION /clang:-fno-c++-static-destructors -flto=thin",
+ "-DCMAKE_C_FLAGS=/DU_STATIC_IMPLEMENTATION /clang:-flto=thin /clang:-fno-split-lto-unit",
+ "-DCMAKE_CXX_FLAGS=/DU_STATIC_IMPLEMENTATION /clang:-fno-c++-static-destructors /clang:-flto=thin /clang:-fno-split-lto-unit",
"-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "-DCMAKE_C_FLAGS=/DU_STATIC_IMPLEMENTATION -flto=thin", | |
| "-DCMAKE_CXX_FLAGS=/DU_STATIC_IMPLEMENTATION /clang:-fno-c++-static-destructors -flto=thin", | |
| flags.push( | |
| "-DCMAKE_C_FLAGS=/DU_STATIC_IMPLEMENTATION /clang:-flto=thin /clang:-fno-split-lto-unit", | |
| "-DCMAKE_CXX_FLAGS=/DU_STATIC_IMPLEMENTATION /clang:-fno-c++-static-destructors /clang:-flto=thin /clang:-fno-split-lto-unit", | |
| "-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build.ts` around lines 197 - 198, The Windows LTO flags in the build config
are incomplete: update the CMake flag strings for CMAKE_C_FLAGS and
CMAKE_CXX_FLAGS so they match the workflow by using the /clang: prefixed ThinLTO
flags and include -fno-split-lto-unit; specifically replace the "-flto=thin"
entry in the CMAKE_C_FLAGS string and the "/clang:-fno-c++-static-destructors
-flto=thin" portion in the CMAKE_CXX_FLAGS string with "/clang:-flto=thin
/clang:-fno-split-lto-unit" (keeping existing tokens like
/DU_STATIC_IMPLEMENTATION and /clang:-fno-c++-static-destructors) so local
Windows builds use the same "/clang:-flto=thin /clang:-fno-split-lto-unit" flags
as CI.
| "-DCMAKE_C_FLAGS=-flto=thin", | ||
| "-DCMAKE_CXX_FLAGS=-flto=thin" |
There was a problem hiding this comment.
Incomplete ThinLTO flags for non-Windows platforms.
The non-Windows LTO configuration is missing flags compared to the Dockerfile and workflow configurations. Those use -flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables, but this uses only -flto=thin.
The PR objectives specifically mention adding -fno-split-lto-unit as a key difference from the previous attempt, but it's missing here. This inconsistency means local non-Windows LTO builds will differ from CI builds.
🔧 Proposed fix to match Dockerfile configuration
} else {
flags.push(
- "-DCMAKE_C_FLAGS=-flto=thin",
- "-DCMAKE_CXX_FLAGS=-flto=thin"
+ "-DCMAKE_C_FLAGS=-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables",
+ "-DCMAKE_CXX_FLAGS=-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "-DCMAKE_C_FLAGS=-flto=thin", | |
| "-DCMAKE_CXX_FLAGS=-flto=thin" | |
| } else { | |
| flags.push( | |
| "-DCMAKE_C_FLAGS=-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables", | |
| "-DCMAKE_CXX_FLAGS=-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build.ts` around lines 203 - 204, Update the non-Windows CMake LTO flags so
they match CI/Docker configuration: replace the current
"-DCMAKE_C_FLAGS=-flto=thin" and "-DCMAKE_CXX_FLAGS=-flto=thin" entries with
flags that include "-flto=thin -fno-split-lto-unit -fwhole-program-vtables
-fforce-emit-vtables"; ensure you change both the C and CXX flag strings in the
same place where these options are added so local non-Windows builds use the
identical LTO settings as CI.
| ARG WEBKIT_RELEASE_TYPE=Release | ||
| ARG CPU=native | ||
| ARG LTO_FLAG="-flto=full -fwhole-program-vtables -fforce-emit-vtables " | ||
| ARG LTO_FLAG="-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables " |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Minor: trailing space in flag string.
The flag string has a trailing space. This is harmless but could be cleaned up for consistency.
✨ Proposed cleanup
-ARG LTO_FLAG="-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables "
+ARG LTO_FLAG="-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ARG LTO_FLAG="-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables " | |
| ARG LTO_FLAG="-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile` at line 4, The ARG LTO_FLAG value contains a trailing space;
update the Dockerfile ARG LTO_FLAG definition to remove the trailing whitespace
from the flag string (the symbol to change is ARG LTO_FLAG) so the value ends
with "-fforce-emit-vtables" without the extra space.
| ARG WEBKIT_RELEASE_TYPE=Release | ||
| ARG CPU=native | ||
| ARG LTO_FLAG="-flto=full -fwhole-program-vtables -fforce-emit-vtables " | ||
| ARG LTO_FLAG="-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables " |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Minor: trailing space in flag string.
The flag string has a trailing space. This is harmless but could be cleaned up for consistency.
✨ Proposed cleanup
-ARG LTO_FLAG="-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables "
+ARG LTO_FLAG="-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ARG LTO_FLAG="-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables " | |
| ARG LTO_FLAG="-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile.musl` at line 4, The ARG LTO_FLAG value in Dockerfile.musl
contains a trailing space; remove the trailing space from the string assigned to
ARG LTO_FLAG (the "-flto=thin -fno-split-lto-unit -fwhole-program-vtables
-fforce-emit-vtables " value) so the flag list is clean and consistent while
keeping the same flags and spacing between them.
Preview Builds
|
Tested in bun — the parseInt fix is not sufficient; closing for now (2026-06-02)Tested end-to-end via oven-sh/bun#31705: bun pinned to The DFG-tier miscompile this PR aimed to unblock is still present on linux-x64 glibc: Also: the aarch64-musl bun link segfaulted in rust-lld once per-CGU ThinLTO Rust bitcode rejoined the link (the old globalopt-crash shape) — bun gated cross-language LTO off on musl as a workaround. For the next attempt: the failure persists at every LTO opt level above |
2 participants
Switches the Linux LTO build variants (glibc and musl, all arches) from
-flto=fullto ThinLTO, mirroring the exact flag set the macOS and Windows ThinLTO lanes already use:-flto=thin -fno-split-lto-unit -fwhole-program-vtables -fforce-emit-vtables. Also switches the ICU build flags inbuild.tsfrom full to thin so the link doesn't mix a serial full-LTO partition into an otherwise ThinLTO build.Why now
#208 tried this and was closed: ThinLTO on Linux exploited undefined double→int conversions in JSC and miscompiled the overflow guards (visible symptom:
parseInt("80000000", 16)returning a sign-wrapped negative int32 once call sites tiered up). #244 fixes that entire class at the source (7 sites made defined-behavior). This branch is stacked on #244 to test whether those fixes were the only thing blocking ThinLTO on Linux.Differences from #208
-fno-split-lto-unit(the macOS/Windows thin lanes use it; Switch from full LTO to ThinLTO #208 predated them)lto_flagentries + both DockerfileLTO_FLAGdefaults + 4 ICU flags inbuild.ts; no-flto=fullremains anywhereValidation
dfg-parseIntResult-should-not-wrap-above-int32.js,switch-imm-scrutinee-above-int32.js) are the regression check for the original miscompile — run them against the ThinLTO artifacts (manually until the stress-test workflow from Run JSC stress tests on PRs #243 lands)Stacked on #244 — contains its commits; rebase or merge after #244 lands.