Add Windows ASAN support for x64 and ARM64#163
Open
Jarred-Sumner wants to merge 2 commits intomainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEnables AddressSanitizer on Windows across CI, build, CMake, and release tooling: adds Windows ASAN CI variants and artifacts, discovers and links MSVC/clang-cl ASAN runtimes, enables ASAN in Windows debug builds, copies ASAN runtime DLLs into release outputs, and adjusts stack-size for ASAN builds on Windows. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/cmake/WebKitCompilerFlags.cmake`:
- Around line 328-357: The current MSVC path silently clears
SANITIZER_LINK_FLAGS when CLANG_ASAN_LIBRARY and CLANG_ASAN_RT_LIBRARY are not
found, which leads to unresolved __asan_* symbols; update the block that checks
CLANG_ASAN_LIBRARY and CLANG_ASAN_RT_LIBRARY so that when building with MSVC and
those libraries are missing (after the find_library calls for CLANG_ASAN_LIBRARY
and CLANG_ASAN_RT_LIBRARY using CLANG_LIB_PATH) you fail fast via
message(FATAL_ERROR ...) describing the missing ASAN .lib files and the
CLANG_LIB_PATH queried instead of setting SANITIZER_LINK_FLAGS to empty or
relying on the compiler driver; keep the success branch that sets
SANITIZER_LINK_FLAGS and the message(STATUS) when both libs are found.
Jarred-Sumner
force-pushed
the
jarred/webkit-windows-asan
branch
2 times, most recently
from
d9bcca9 to
8ccfefa
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/WTF/wtf/Threading.cpp (1)
112-123:⚠️ Potential issue | 🟠 MajorWindows Graphics stack bump is skipped under ASAN/ASSERT builds.
Because the new
#elif (OS(DARWIN) || OS(WINDOWS)) && (ASAN_ENABLED || ASSERT_ENABLED)matches first, the later#elif OS(WINDOWS)block is not compiled in ASAN/ASSERT builds. That removes the Graphics thread stack-size increase on Windows when ASAN/ASSERT is on, which can reintroduce WebGL stack issues.🔧 Proposed restructuring
-#elif (OS(DARWIN) || OS(WINDOWS)) && (ASAN_ENABLED || ASSERT_ENABLED) - if (threadType == ThreadType::Compiler) - return 1 * MB; // ASan / Debug build needs more stack space. -#elif OS(WINDOWS) +#elif OS(DARWIN) && (ASAN_ENABLED || ASSERT_ENABLED) + if (threadType == ThreadType::Compiler) + return 1 * MB; // ASan / Debug build needs more stack space. +#elif OS(WINDOWS) + if ((ASAN_ENABLED || ASSERT_ENABLED) && threadType == ThreadType::Compiler) + return 1 * MB; // ASan / Debug build needs more stack space. // WebGL conformance tests need more stack space <https://webkit.org/b/261297> if (threadType == ThreadType::Graphics) `#if` defined(NDEBUG) return 2 * MB; `#else` return 4 * MB; `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/WTF/wtf/Threading.cpp` around lines 112 - 123, The ASAN/ASSERT preprocessor branch currently short-circuits the later Windows-specific Graphics stack-size bump, so adjust the conditional logic in Threading.cpp to ensure ThreadType::Graphics on Windows still returns the increased stack size even when ASAN_ENABLED or ASSERT_ENABLED are set: either move the Windows Graphics check before the combined (OS(DARWIN) || OS(WINDOWS)) && (ASAN_ENABLED || ASSERT_ENABLED) branch or add an inner check inside that branch for threadType == ThreadType::Graphics that returns the WebGL-specific values (2 * MB in release, 4 * MB in debug), keeping the existing ThreadType::Compiler handling intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Source/WTF/wtf/Threading.cpp`:
- Around line 112-123: The ASAN/ASSERT preprocessor branch currently
short-circuits the later Windows-specific Graphics stack-size bump, so adjust
the conditional logic in Threading.cpp to ensure ThreadType::Graphics on Windows
still returns the increased stack size even when ASAN_ENABLED or ASSERT_ENABLED
are set: either move the Windows Graphics check before the combined (OS(DARWIN)
|| OS(WINDOWS)) && (ASAN_ENABLED || ASSERT_ENABLED) branch or add an inner check
inside that branch for threadType == ThreadType::Graphics that returns the
WebGL-specific values (2 * MB in release, 4 * MB in debug), keeping the existing
ThreadType::Compiler handling intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-reusable.yml:
- Around line 262-275: Add a brief explanatory comment above the Windows ASAN
job entries where ENABLE_SANITIZERS is set to "address" to clarify that Windows
builds intentionally only use the address sanitizer because UBSan/undefined
behavior sanitizer is not supported or reliable with clang-cl on Windows (hence
macOS/Linux use "address,undefined" but Windows does not); reference the Windows
job block (runner: windows and ENABLE_SANITIZERS) so maintainers understand the
rationale.
Comment on lines
+262
to
+275
| - runner: windows | ||
| build-type: Release | ||
| package_json_arch: "x64" | ||
| label: bun-webkit-windows-amd64-asan | ||
| platform: x64 | ||
| ENABLE_SANITIZERS: "address" | ||
| - runner: windows | ||
| build-type: Debug | ||
| package_json_arch: "x64" | ||
| label: bun-webkit-windows-amd64-debug-asan | ||
| platform: x64 | ||
| ENABLE_SANITIZERS: "address" | ||
| # Note: ARM64 Windows ASAN is not available — LLVM does not ship | ||
| # clang_rt.asan libraries for Windows on ARM64 (woa64) yet. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Windows ASAN uses only address sanitizer, unlike macOS/Linux which use address,undefined.
The Windows ASAN entries only enable the address sanitizer, while macOS and Linux ASAN builds enable both address,undefined. If this is intentional (e.g., UBSan not fully supported with clang-cl on Windows), consider adding a brief comment explaining the difference for maintainability.
The ARM64 exclusion comment is helpful and appreciated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-reusable.yml around lines 262 - 275, Add a brief
explanatory comment above the Windows ASAN job entries where ENABLE_SANITIZERS
is set to "address" to clarify that Windows builds intentionally only use the
address sanitizer because UBSan/undefined behavior sanitizer is not supported or
reliable with clang-cl on Windows (hence macOS/Linux use "address,undefined" but
Windows does not); reference the Windows job block (runner: windows and
ENABLE_SANITIZERS) so maintainers understand the rationale.
Preview Builds
|
Jarred-Sumner
force-pushed
the
jarred/webkit-windows-asan
branch
from
3ed455a to
46fe4b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build.ts (1)
171-189:⚠️ Potential issue | 🟠 MajorAvoid forcing the debug CRT when ASAN is enabled on Windows.
The debug path now enables ASAN on Windows, but
CMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebugis still forced. This conflicts with the sanitizer-enabled Windows configuration used elsewhere and can break local debug ASAN builds. Gate or override the runtime selection when ASAN is enabled to keep Windows behavior consistent.✅ Suggested fix (gate CRT selection on ASAN)
flags.push( "-DCMAKE_BUILD_TYPE=Debug", "-DENABLE_BUN_SKIP_FAILING_ASSERTIONS=ON", "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON", "-DENABLE_REMOTE_INSPECTOR=ON", "-DUSE_VISIBILITY_ATTRIBUTE=1" ); - if (IS_MAC || IS_LINUX || IS_WINDOWS) { + const enableASAN = IS_MAC || IS_LINUX || IS_WINDOWS; + if (enableASAN) { // Enable address sanitizer by default on debug builds (all platforms) flags.push("-DENABLE_SANITIZERS=address"); // To disable asan, comment the line above and uncomment: // flags.push("-DENABLE_MALLOC_HEAP_BREAKDOWN=ON"); } if (IS_WINDOWS) { - flags.push("-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug"); + const msvcRuntime = enableASAN ? "MultiThreaded" : "MultiThreadedDebug"; + flags.push(`-DCMAKE_MSVC_RUNTIME_LIBRARY=${msvcRuntime}`); } break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.ts` around lines 171 - 189, In the "debug" case the script unconditionally pushes "-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug" for IS_WINDOWS which conflicts with the ASAN configuration; modify the logic around the flags array so that the push of "-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug" only happens when running on Windows and ASAN is not enabled (e.g., check for the presence of "-DENABLE_SANITIZERS=address" in flags or a corresponding sanitizer flag variable before pushing the CRT flag); reference the "debug" case, flags array, IS_WINDOWS, "-DENABLE_SANITIZERS=address" and "-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug" when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@windows-release.ps1`:
- Around line 217-228: When sanitizers are enabled the script currently logs a
warning if the ASAN DLL is missing; change this to fail the build by exiting
non-zero or throwing an error so artifacts aren't published without the runtime.
In the block that checks $ENABLE_SANITIZERS and $ClangRtLibPath and computes
$asanDllPath (using symbols $ENABLE_SANITIZERS, $ClangRtLibPath, $asanDllPath,
$asanDll), replace the Write-Host warning branch with a failing action (e.g.,
Write-Error/Throw or exit 1) that outputs a clear message including $asanDllPath
and ensures the script stops rather than continuing to Copy-Item/complete
successfully.
---
Outside diff comments:
In `@build.ts`:
- Around line 171-189: In the "debug" case the script unconditionally pushes
"-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug" for IS_WINDOWS which conflicts
with the ASAN configuration; modify the logic around the flags array so that the
push of "-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug" only happens when
running on Windows and ASAN is not enabled (e.g., check for the presence of
"-DENABLE_SANITIZERS=address" in flags or a corresponding sanitizer flag
variable before pushing the CRT flag); reference the "debug" case, flags array,
IS_WINDOWS, "-DENABLE_SANITIZERS=address" and
"-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug" when making the change.
---
Duplicate comments:
In `@Source/cmake/WebKitCompilerFlags.cmake`:
- Around line 328-357: The current MSVC branch falls back to clearing
SANITIZER_LINK_FLAGS when CLANG_ASAN_LIBRARY or CLANG_ASAN_RT_LIBRARY aren’t
found, but compiler flags still contain -fsanitize=address which leads to
unresolved __asan_* symbols; instead, in the MSVC-specific block where
CLANG_ASAN_LIBRARY and CLANG_ASAN_RT_LIBRARY are checked, replace the non-fatal
fallback with a hard failure (use message(FATAL_ERROR)) that reports
CLANG_LIB_PATH and the missing libraries (CLANG_ASAN_LIBRARY,
CLANG_ASAN_RT_LIBRARY) and instructs to install/point to the correct ASAN .lib
files; keep the success branch that sets SANITIZER_LINK_FLAGS unchanged.
Adds AddressSanitizer support for Windows x64 builds using clang-cl. ## windows-release.ps1 - Read ENABLE_SANITIZERS env var and pass to CMake - Auto-discover CLANG_LIB_PATH from the installed LLVM toolchain - Copy ASAN runtime DLL (clang_rt.asan_dynamic-x86_64.dll) to output - Force ENABLE_ASSERTS=ON when sanitizers are enabled - Use static release CRT (/MT) for ASAN builds — clang-cl ASAN does not support debug CRT variants (/MTd, /MDd) ## Source/cmake/WebKitCompilerFlags.cmake - Support both x86_64 and ARM64 ASAN runtime library discovery - Auto-discover CLANG_LIB_PATH from clang-cl via /clang:--print-resource-dir - Don't pass -fsanitize= to lld-link — pass explicit .lib files instead (CMake with MSVC invokes lld-link directly, not through clang-cl driver) ## .github/workflows/build-reusable.yml - Add x64 Release and Debug ASAN matrix entries - Set fail-fast: false so ASAN builds aren't killed by unrelated failures - Pass ENABLE_SANITIZERS env var in the build step - Add artifact download, rename, and release upload steps - Note: ARM64 ASAN not included — LLVM does not ship ASAN runtime libraries for Windows on ARM64 (woa64) yet ## Source/WTF/wtf/Threading.cpp - Increase compiler thread stack size for ASAN on Windows (was Darwin-only) - ASAN inflates stack frames ~3x, compiler threads need more space ## build.ts - Enable ASAN for Windows debug builds in the local build script
Jarred-Sumner
force-pushed
the
jarred/webkit-windows-asan
branch
from
46fe4b7 to
3a875fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build.ts (1)
180-189:⚠️ Potential issue | 🟠 MajorPotential CRT conflict: ASAN with debug CRT.
The change enables ASAN for Windows debug builds (line 182), but line 188 sets
CMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug. According to the comment inwindows-release.ps1(lines 74-76), clang-cl's ASAN does not support debug CRT (/MTd,/MDd) — only release CRT (/MT,/MD).This configuration may cause build failures or runtime issues. Consider forcing release CRT when ASAN is enabled on Windows:
Suggested fix
if (IS_WINDOWS) { - flags.push("-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug"); + // clang-cl ASAN does not support debug CRT (/MTd), use release CRT (/MT) + flags.push("-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.ts` around lines 180 - 189, When building on Windows, the code unconditionally sets CMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug while also enabling address sanitizer via the "-DENABLE_SANITIZERS=address" flag, which conflicts with ASAN (clang-cl requires a release CRT). Update the logic in build.ts: after pushing "-DENABLE_SANITIZERS=address" (or when ENABLE_SANITIZERS is present in flags) and if IS_WINDOWS, override the runtime selection so CMAKE_MSVC_RUNTIME_LIBRARY is set to a release CRT (e.g., "MultiThreaded" or "MultiThreadedDLL") instead of "MultiThreadedDebug"; use the flags array and the IS_WINDOWS check to implement this conditional assignment so ASAN builds on Windows use a release CRT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@windows-release.ps1`:
- Around line 74-83: The non-ASAN branch assigns $CmakeMsvcRuntimeLibrary =
"MultiThreaded" then overwrites it for Debug builds, which is redundant;
simplify the logic by making the non-ASAN branch choose between
"MultiThreadedDebug" and "MultiThreaded" based on $CMAKE_BUILD_TYPE (or set a
default and only override for Debug). Update the if ($ENABLE_SANITIZERS) / else
block to either (a) set $CmakeMsvcRuntimeLibrary = "MultiThreaded" when
$ENABLE_SANITIZERS is true and in the else branch set it to "MultiThreadedDebug"
if $CMAKE_BUILD_TYPE -eq "Debug" otherwise "MultiThreaded", or (b) set a default
then only change to "MultiThreadedDebug" when $CMAKE_BUILD_TYPE is Debug;
reference $CmakeMsvcRuntimeLibrary, $ENABLE_SANITIZERS, and $CMAKE_BUILD_TYPE
when making the change.
---
Outside diff comments:
In `@build.ts`:
- Around line 180-189: When building on Windows, the code unconditionally sets
CMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug while also enabling address
sanitizer via the "-DENABLE_SANITIZERS=address" flag, which conflicts with ASAN
(clang-cl requires a release CRT). Update the logic in build.ts: after pushing
"-DENABLE_SANITIZERS=address" (or when ENABLE_SANITIZERS is present in flags)
and if IS_WINDOWS, override the runtime selection so CMAKE_MSVC_RUNTIME_LIBRARY
is set to a release CRT (e.g., "MultiThreaded" or "MultiThreadedDLL") instead of
"MultiThreadedDebug"; use the flags array and the IS_WINDOWS check to implement
this conditional assignment so ASAN builds on Windows use a release CRT.
---
Duplicate comments:
In `@windows-release.ps1`:
- Around line 217-228: If sanitizers are enabled but $ClangRtLibPath is empty
the current combined condition silently skips ASAN handling; change the logic so
you first check $ENABLE_SANITIZERS alone and if true fail fast when
$ClangRtLibPath is null/empty (throw a clear error mentioning missing
ClangRtLibPath and that ASAN runtime cannot be copied), otherwise proceed to
compute $asanArch, $asanDll and $asanDllPath and keep the
Test-Path/Copy-Item/throw-on-missing-DLL behavior for the existing copy step.
Comment on lines
+74
to
83
| # clang-cl's ASAN does not support debug CRT (/MTd, /MDd) — only release CRT (/MT, /MD). | ||
| # Always use static release CRT (/MT) for ASAN builds regardless of build type. | ||
| if ($ENABLE_SANITIZERS) { | ||
| $CmakeMsvcRuntimeLibrary = "MultiThreaded" | ||
| } else { | ||
| $CmakeMsvcRuntimeLibrary = "MultiThreaded" | ||
| if ($CMAKE_BUILD_TYPE -eq "Debug") { | ||
| $CmakeMsvcRuntimeLibrary = "MultiThreadedDebug" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Redundant assignment in non-ASAN path.
Line 79 sets $CmakeMsvcRuntimeLibrary = "MultiThreaded" but it's immediately overwritten on line 81 for Debug builds. The assignment on line 79 is only effective for non-Debug builds.
This works correctly but could be simplified:
Optional simplification
} else {
- $CmakeMsvcRuntimeLibrary = "MultiThreaded"
if ($CMAKE_BUILD_TYPE -eq "Debug") {
$CmakeMsvcRuntimeLibrary = "MultiThreadedDebug"
+ } else {
+ $CmakeMsvcRuntimeLibrary = "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.
Suggested change
| # clang-cl's ASAN does not support debug CRT (/MTd, /MDd) — only release CRT (/MT, /MD). | |
| # Always use static release CRT (/MT) for ASAN builds regardless of build type. | |
| if ($ENABLE_SANITIZERS) { | |
| $CmakeMsvcRuntimeLibrary = "MultiThreaded" | |
| } else { | |
| $CmakeMsvcRuntimeLibrary = "MultiThreaded" | |
| if ($CMAKE_BUILD_TYPE -eq "Debug") { | |
| $CmakeMsvcRuntimeLibrary = "MultiThreadedDebug" | |
| } | |
| } | |
| # clang-cl's ASAN does not support debug CRT (/MTd, /MDd) — only release CRT (/MT, /MD). | |
| # Always use static release CRT (/MT) for ASAN builds regardless of build type. | |
| if ($ENABLE_SANITIZERS) { | |
| $CmakeMsvcRuntimeLibrary = "MultiThreaded" | |
| } else { | |
| if ($CMAKE_BUILD_TYPE -eq "Debug") { | |
| $CmakeMsvcRuntimeLibrary = "MultiThreadedDebug" | |
| } else { | |
| $CmakeMsvcRuntimeLibrary = "MultiThreaded" | |
| } | |
| } |
🧰 Tools
🪛 PSScriptAnalyzer (1.24.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'windows-release.ps1'
(PSUseBOMForUnicodeEncodedFile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@windows-release.ps1` around lines 74 - 83, The non-ASAN branch assigns
$CmakeMsvcRuntimeLibrary = "MultiThreaded" then overwrites it for Debug builds,
which is redundant; simplify the logic by making the non-ASAN branch choose
between "MultiThreadedDebug" and "MultiThreaded" based on $CMAKE_BUILD_TYPE (or
set a default and only override for Debug). Update the if ($ENABLE_SANITIZERS) /
else block to either (a) set $CmakeMsvcRuntimeLibrary = "MultiThreaded" when
$ENABLE_SANITIZERS is true and in the else branch set it to "MultiThreadedDebug"
if $CMAKE_BUILD_TYPE -eq "Debug" otherwise "MultiThreaded", or (b) set a default
then only change to "MultiThreadedDebug" when $CMAKE_BUILD_TYPE is Debug;
reference $CmakeMsvcRuntimeLibrary, $ENABLE_SANITIZERS, and $CMAKE_BUILD_TYPE
when making the change.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Source/cmake/WebKitCompilerFlags.cmake (1)
328-360: LGTM! Previous review issue addressed with fail-fast error handling.The FATAL_ERROR at lines 358-360 correctly prevents builds from proceeding with missing ASAN libraries, avoiding the previously flagged unresolved
__asan_*symbol issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/cmake/WebKitCompilerFlags.cmake` around lines 328 - 360, The configuration correctly fails fast when ASAN libs are missing; no code change required—keep the FATAL_ERROR behavior in the MSVC branch that checks CLANG_ASAN_LIBRARY and CLANG_ASAN_RT_LIBRARY and preserves the /WHOLEARCHIVE logic that sets SANITIZER_LINK_FLAGS, ensuring builds abort with the clear error message referencing CLANG_LIB_PATH if the clang_rt.asan libraries aren't found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/cmake/OptionsJSCOnly.cmake`:
- Around line 217-224: The find_library calls for ICU I18N and UC currently list
dynamic names first, so CMake may pick shared libs even when static ones exist;
update the find_library NAMES in the ICU_I18N_LIBRARY_RELEASE and
ICU_UC_LIBRARY_RELEASE calls to list the static names (sicuin before icuin,
sicuuc before icuuc) ahead of the dynamic names so static ICU is consistently
prioritized by the find_library logic (see ICU_I18N_LIBRARY_RELEASE,
ICU_UC_LIBRARY_RELEASE and their find_library invocations).
- Around line 225-230: The current check only logs when all three variables
ICU_DATA_LIBRARY_RELEASE, ICU_I18N_LIBRARY_RELEASE and ICU_UC_LIBRARY_RELEASE
are set; add a guard that detects the partial case (i.e., at least one of those
variables is set but not all) and emit a clear message(WARNING ...) indicating
which ICU libraries were found and which are missing so FindICU won't silently
mix partial static/dynamic ICU; update the block around the existing if(...) in
OptionsJSCOnly.cmake to perform this partial-check and warning before leaving
the logic to FindICU.
In `@Source/cmake/WebKitCompilerFlags.cmake`:
- Around line 342-348: The current conditional only calls find_library for
WTF_CPU_X86_64 or WTF_CPU_ARM64, so if neither is true later checks fail with a
generic "ASAN libraries not found"; add an explicit else branch after the
existing elseif/endif that calls message(FATAL_ERROR ...) (or a clear
message(WARNING) then message(FATAL_ERROR)) to report unsupported/unknown
architecture, referencing the same symbols (WTF_CPU_X86_64, WTF_CPU_ARM64) and
the expected variables CLANG_ASAN_LIBRARY and CLANG_ASAN_RT_LIBRARY so the error
is descriptive; ensure this explicit error runs before the subsequent "ASAN
libraries not found" check to provide clearer diagnostics.
---
Duplicate comments:
In `@Source/cmake/WebKitCompilerFlags.cmake`:
- Around line 328-360: The configuration correctly fails fast when ASAN libs are
missing; no code change required—keep the FATAL_ERROR behavior in the MSVC
branch that checks CLANG_ASAN_LIBRARY and CLANG_ASAN_RT_LIBRARY and preserves
the /WHOLEARCHIVE logic that sets SANITIZER_LINK_FLAGS, ensuring builds abort
with the clear error message referencing CLANG_LIB_PATH if the clang_rt.asan
libraries aren't found.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Source/cmake/OptionsJSCOnly.cmakeSource/cmake/WebKitCompilerFlags.cmake
Comment on lines
+217
to
+224
| find_library(ICU_DATA_LIBRARY_RELEASE NAMES sicudt icudt PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH) | ||
| endif () | ||
| if (NOT ICU_I18N_LIBRARY_RELEASE) | ||
| find_library(ICU_I18N_LIBRARY_RELEASE NAMES icuin sicuin PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH) | ||
| endif () | ||
| if (NOT ICU_UC_LIBRARY_RELEASE) | ||
| find_library(ICU_UC_LIBRARY_RELEASE NAMES icuuc sicuuc PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH) | ||
| endif () |
There was a problem hiding this comment.
Inconsistent library name ordering defeats static ICU intent.
The NAMES order in find_library matters—CMake picks the first match. Line 217 correctly lists sicudt (static) before icudt (dynamic), but lines 220 and 223 list dynamic names first (icuin, icuuc), which will be found preferentially if both exist in the ICU_ROOT/lib directory.
Proposed fix to consistently prioritize static libraries
if (NOT ICU_I18N_LIBRARY_RELEASE)
- find_library(ICU_I18N_LIBRARY_RELEASE NAMES icuin sicuin PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH)
+ find_library(ICU_I18N_LIBRARY_RELEASE NAMES sicuin icuin PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH)
endif ()
if (NOT ICU_UC_LIBRARY_RELEASE)
- find_library(ICU_UC_LIBRARY_RELEASE NAMES icuuc sicuuc PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH)
+ find_library(ICU_UC_LIBRARY_RELEASE NAMES sicuuc icuuc PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH)
endif ()📝 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.
Suggested change
| find_library(ICU_DATA_LIBRARY_RELEASE NAMES sicudt icudt PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH) | |
| endif () | |
| if (NOT ICU_I18N_LIBRARY_RELEASE) | |
| find_library(ICU_I18N_LIBRARY_RELEASE NAMES icuin sicuin PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH) | |
| endif () | |
| if (NOT ICU_UC_LIBRARY_RELEASE) | |
| find_library(ICU_UC_LIBRARY_RELEASE NAMES icuuc sicuuc PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH) | |
| endif () | |
| find_library(ICU_DATA_LIBRARY_RELEASE NAMES sicudt icudt PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH) | |
| endif () | |
| if (NOT ICU_I18N_LIBRARY_RELEASE) | |
| find_library(ICU_I18N_LIBRARY_RELEASE NAMES sicuin icuin PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH) | |
| endif () | |
| if (NOT ICU_UC_LIBRARY_RELEASE) | |
| find_library(ICU_UC_LIBRARY_RELEASE NAMES sicuuc icuuc PATHS "${_icu_lib_dir}" NO_DEFAULT_PATH) | |
| endif () |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/cmake/OptionsJSCOnly.cmake` around lines 217 - 224, The find_library
calls for ICU I18N and UC currently list dynamic names first, so CMake may pick
shared libs even when static ones exist; update the find_library NAMES in the
ICU_I18N_LIBRARY_RELEASE and ICU_UC_LIBRARY_RELEASE calls to list the static
names (sicuin before icuin, sicuuc before icuuc) ahead of the dynamic names so
static ICU is consistently prioritized by the find_library logic (see
ICU_I18N_LIBRARY_RELEASE, ICU_UC_LIBRARY_RELEASE and their find_library
invocations).
Comment on lines
+225
to
+230
| if (ICU_DATA_LIBRARY_RELEASE AND ICU_I18N_LIBRARY_RELEASE AND ICU_UC_LIBRARY_RELEASE) | ||
| message(STATUS "Pre-set static ICU libraries from ICU_ROOT=${ICU_ROOT}:") | ||
| message(STATUS " ICU data: ${ICU_DATA_LIBRARY_RELEASE}") | ||
| message(STATUS " ICU i18n: ${ICU_I18N_LIBRARY_RELEASE}") | ||
| message(STATUS " ICU uc: ${ICU_UC_LIBRARY_RELEASE}") | ||
| endif () |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider warning when static ICU libraries are only partially found.
If only some of the three ICU libraries are found in ICU_ROOT/lib, the current code silently falls through without warning. This could lead to FindICU picking up mismatched libraries from elsewhere, potentially mixing static and dynamic ICU.
Optional: Add a warning for partial matches
if (ICU_DATA_LIBRARY_RELEASE AND ICU_I18N_LIBRARY_RELEASE AND ICU_UC_LIBRARY_RELEASE)
message(STATUS "Pre-set static ICU libraries from ICU_ROOT=${ICU_ROOT}:")
message(STATUS " ICU data: ${ICU_DATA_LIBRARY_RELEASE}")
message(STATUS " ICU i18n: ${ICU_I18N_LIBRARY_RELEASE}")
message(STATUS " ICU uc: ${ICU_UC_LIBRARY_RELEASE}")
+ elseif (ICU_DATA_LIBRARY_RELEASE OR ICU_I18N_LIBRARY_RELEASE OR ICU_UC_LIBRARY_RELEASE)
+ message(WARNING "Only some static ICU libraries found in ICU_ROOT=${ICU_ROOT}. "
+ "FindICU may locate remaining libraries elsewhere, potentially mixing static/dynamic linkage.")
endif ()📝 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.
Suggested change
| if (ICU_DATA_LIBRARY_RELEASE AND ICU_I18N_LIBRARY_RELEASE AND ICU_UC_LIBRARY_RELEASE) | |
| message(STATUS "Pre-set static ICU libraries from ICU_ROOT=${ICU_ROOT}:") | |
| message(STATUS " ICU data: ${ICU_DATA_LIBRARY_RELEASE}") | |
| message(STATUS " ICU i18n: ${ICU_I18N_LIBRARY_RELEASE}") | |
| message(STATUS " ICU uc: ${ICU_UC_LIBRARY_RELEASE}") | |
| endif () | |
| if (ICU_DATA_LIBRARY_RELEASE AND ICU_I18N_LIBRARY_RELEASE AND ICU_UC_LIBRARY_RELEASE) | |
| message(STATUS "Pre-set static ICU libraries from ICU_ROOT=${ICU_ROOT}:") | |
| message(STATUS " ICU data: ${ICU_DATA_LIBRARY_RELEASE}") | |
| message(STATUS " ICU i18n: ${ICU_I18N_LIBRARY_RELEASE}") | |
| message(STATUS " ICU uc: ${ICU_UC_LIBRARY_RELEASE}") | |
| elseif (ICU_DATA_LIBRARY_RELEASE OR ICU_I18N_LIBRARY_RELEASE OR ICU_UC_LIBRARY_RELEASE) | |
| message(WARNING "Only some static ICU libraries found in ICU_ROOT=${ICU_ROOT}. " | |
| "FindICU may locate remaining libraries elsewhere, potentially mixing static/dynamic linkage.") | |
| endif () |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/cmake/OptionsJSCOnly.cmake` around lines 225 - 230, The current check
only logs when all three variables ICU_DATA_LIBRARY_RELEASE,
ICU_I18N_LIBRARY_RELEASE and ICU_UC_LIBRARY_RELEASE are set; add a guard that
detects the partial case (i.e., at least one of those variables is set but not
all) and emit a clear message(WARNING ...) indicating which ICU libraries were
found and which are missing so FindICU won't silently mix partial static/dynamic
ICU; update the block around the existing if(...) in OptionsJSCOnly.cmake to
perform this partial-check and warning before leaving the logic to FindICU.
Comment on lines
+342
to
+348
| if (WTF_CPU_X86_64) | ||
| find_library(CLANG_ASAN_LIBRARY clang_rt.asan_dynamic_runtime_thunk-x86_64 PATHS ${CLANG_LIB_PATH}) | ||
| find_library(CLANG_ASAN_RT_LIBRARY clang_rt.asan_dynamic-x86_64 PATHS ${CLANG_LIB_PATH}) | ||
| elseif (WTF_CPU_ARM64) | ||
| find_library(CLANG_ASAN_LIBRARY clang_rt.asan_dynamic_runtime_thunk-aarch64 PATHS ${CLANG_LIB_PATH}) | ||
| find_library(CLANG_ASAN_RT_LIBRARY clang_rt.asan_dynamic-aarch64 PATHS ${CLANG_LIB_PATH}) | ||
| endif () |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider explicit error for unsupported architectures.
If WTF_CPU_X86_64 and WTF_CPU_ARM64 are both false, no find_library calls execute, causing the subsequent check to fail with "ASAN libraries not found". A more informative error for this edge case would aid debugging.
Optional: Add explicit architecture check
if (WTF_CPU_X86_64)
find_library(CLANG_ASAN_LIBRARY clang_rt.asan_dynamic_runtime_thunk-x86_64 PATHS ${CLANG_LIB_PATH})
find_library(CLANG_ASAN_RT_LIBRARY clang_rt.asan_dynamic-x86_64 PATHS ${CLANG_LIB_PATH})
elseif (WTF_CPU_ARM64)
find_library(CLANG_ASAN_LIBRARY clang_rt.asan_dynamic_runtime_thunk-aarch64 PATHS ${CLANG_LIB_PATH})
find_library(CLANG_ASAN_RT_LIBRARY clang_rt.asan_dynamic-aarch64 PATHS ${CLANG_LIB_PATH})
+ else ()
+ message(FATAL_ERROR "ASAN on MSVC is only supported for x86_64 and ARM64 architectures.")
endif ()📝 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.
Suggested change
| if (WTF_CPU_X86_64) | |
| find_library(CLANG_ASAN_LIBRARY clang_rt.asan_dynamic_runtime_thunk-x86_64 PATHS ${CLANG_LIB_PATH}) | |
| find_library(CLANG_ASAN_RT_LIBRARY clang_rt.asan_dynamic-x86_64 PATHS ${CLANG_LIB_PATH}) | |
| elseif (WTF_CPU_ARM64) | |
| find_library(CLANG_ASAN_LIBRARY clang_rt.asan_dynamic_runtime_thunk-aarch64 PATHS ${CLANG_LIB_PATH}) | |
| find_library(CLANG_ASAN_RT_LIBRARY clang_rt.asan_dynamic-aarch64 PATHS ${CLANG_LIB_PATH}) | |
| endif () | |
| if (WTF_CPU_X86_64) | |
| find_library(CLANG_ASAN_LIBRARY clang_rt.asan_dynamic_runtime_thunk-x86_64 PATHS ${CLANG_LIB_PATH}) | |
| find_library(CLANG_ASAN_RT_LIBRARY clang_rt.asan_dynamic-x86_64 PATHS ${CLANG_LIB_PATH}) | |
| elseif (WTF_CPU_ARM64) | |
| find_library(CLANG_ASAN_LIBRARY clang_rt.asan_dynamic_runtime_thunk-aarch64 PATHS ${CLANG_LIB_PATH}) | |
| find_library(CLANG_ASAN_RT_LIBRARY clang_rt.asan_dynamic-aarch64 PATHS ${CLANG_LIB_PATH}) | |
| else () | |
| message(FATAL_ERROR "ASAN on MSVC is only supported for x86_64 and ARM64 architectures.") | |
| endif () |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/cmake/WebKitCompilerFlags.cmake` around lines 342 - 348, The current
conditional only calls find_library for WTF_CPU_X86_64 or WTF_CPU_ARM64, so if
neither is true later checks fail with a generic "ASAN libraries not found"; add
an explicit else branch after the existing elseif/endif that calls
message(FATAL_ERROR ...) (or a clear message(WARNING) then message(FATAL_ERROR))
to report unsupported/unknown architecture, referencing the same symbols
(WTF_CPU_X86_64, WTF_CPU_ARM64) and the expected variables CLANG_ASAN_LIBRARY
and CLANG_ASAN_RT_LIBRARY so the error is descriptive; ensure this explicit
error runs before the subsequent "ASAN libraries not found" check to provide
clearer diagnostics.
…mismatch Three fixes for jsc.exe from the ASAN build being non-functional: 1. ASAN thunk library needs /WHOLEARCHIVE (WebKitCompilerFlags.cmake) jsc.exe imported __asan_new, __asan_delete, etc. from the ASAN dynamic DLL, but these symbols are provided by the static thunk library. The thunk library must be linked with /WHOLEARCHIVE to force inclusion of all its symbols, including operator new/delete replacements that would otherwise be dropped as unreferenced. 2. Force FindICU to use static ICU build (OptionsJSCOnly.cmake) On Windows CI, find_package(ICU) found system-installed dynamic ICU instead of our static build from build-icu.ps1. Pre-populate FindICU's library cache variables from ICU_ROOT to ensure static libraries are used, eliminating the ICU DLL dependency. 3. Disable MSVC STL ASAN annotations (WebKitCompilerFlags.cmake) MSVC STL emits annotate_string/annotate_vector /failifmismatch pragmas when __SANITIZE_ADDRESS__ is defined. Static ICU (built without ASAN) has these set to 0, causing linker mismatch errors. Disable with _DISABLE_STRING_ANNOTATION and _DISABLE_VECTOR_ANNOTATION. Building ICU with ASAN via MSVC cl.exe is not viable due to lld-link incompatibility with MSVC-generated L___asan_gen_* COMDAT metadata.
Jarred-Sumner
force-pushed
the
jarred/webkit-windows-asan
branch
from
4b217d6 to
ade791d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds AddressSanitizer (ASAN) support for Windows builds using clang-cl, for both x64 and ARM64.
Changes
windows-release.ps1ENABLE_SANITIZERSenv var and pass to CMakeCLANG_LIB_PATHfrom the installed LLVM toolchainclang_rt.asan_dynamic-*.dll) to outputENABLE_ASSERTS=ONwhen sanitizers are enabledSource/cmake/WebKitCompilerFlags.cmakeCLANG_LIB_PATHfrom clang-cl via/clang:--print-resource-dir-fsanitize=to lld-link — instead link the explicit ASAN.libfiles.github/workflows/build-reusable.ymlENABLE_SANITIZERSenv var in the build stepbuild.ts