Fix build on macosx26#587
Conversation
The new compiler/SDK on macos26 need changes in LLVM and the build system. Backport the following commits from the v6-36-00-patches branch: fc2c12e9a2d7f2241c8697ed11d5f23a98508150 86939d9371ed42c5ed5faff5002ef2b2e542c190 9cc2122bf5a81f7063c2a32b2cb78c8d615578a1 21d83324fbdbd91de0115d48f3b55979f57807b7 487cfbe494413e12123b55dead5ef8742ef49fb2 c1c2551a2876f536b5a06f48fa809aeedbc3d7ba Fix an issue in cmake/modules/SetOSX_SDK.cmake which comes with the patches. The problem is a wrong check in an if statement which did not allow to define a SDK on the command line but was always using the standard SDK.
📝 WalkthroughWalkthroughThis PR updates the pythia8 ExternalProject download URL and adds a new patch (backports_macos_sdk_26_4_from_root.patch) applied during the ROOT build. The patch introduces macOS SDK detection via a new CMake module and adds Clang/Sema compiler workarounds for builtins and type traits under macOS SDK 26.4. ChangesBuild Configuration and macOS SDK Compatibility Patch
Estimated code review effort: 4 (Complex) | ~45 minutes Related PRs: None specified. Suggested labels: build, macOS, patch Suggested reviewers: None specified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 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 `@legacy/root/backports_macos_sdk_26_4_from_root.patch`:
- Around line 53-55: Guard the SDK version probe around the execute_process call
that sets OSX_SDK_VERSION so it does not silently proceed when xcrun cannot
resolve the selected SDK. In the backport logic near the SDK version lookup,
check the result/status and whether OSX_SDK_VERSION is non-empty before using it
to decide MAC_SDK_WORKAROUND, and fail or emit a clear message if the probe
cannot determine the SDK version.
- Around line 34-48: The CMAKE_OSX_SYSROOT handling block is overriding valid
explicit SDK inputs by clearing any non-directory value and skipping SDKROOT
entirely. Update the sysroot resolution logic around the CMAKE_OSX_SYSROOT and
xcrun fallback so it preserves SDK-name values like macosx26.5, honors SDKROOT
when present, and only falls back to xcrun --sdk macosx when no explicit SDK was
supplied.
- Around line 257-273: The SemaBuiltinPopcountg check is too permissive because
it accepts any integer type, including signed integers, for __builtin_popcountg.
Update the argument validation in SemaBuiltinPopcountg to require an unsigned
integer type instead of using isIntegerType(), while keeping the existing
argument count and diagnostic flow unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: 0181fb8d-cc70-40db-bac8-f61f09bf5105
📒 Files selected for processing (2)
cmake/legacy.cmakelegacy/root/backports_macos_sdk_26_4_from_root.patch
| +if(NOT IS_DIRECTORY "${CMAKE_OSX_SYSROOT}") | ||
| + unset(CMAKE_OSX_SYSROOT CACHE) | ||
| + unset(CMAKE_OSX_SYSROOT) | ||
| +endif() | ||
| + | ||
| +find_program(XCRUN_EXECUTABLE xcrun) | ||
| +if(EXISTS ${XCRUN_EXECUTABLE}) | ||
| + if(NOT DEFINED CMAKE_OSX_SYSROOT) | ||
| + execute_process(COMMAND ${XCRUN_EXECUTABLE} --sdk macosx --show-sdk-path | ||
| + OUTPUT_VARIABLE SDK_PATH | ||
| + OUTPUT_STRIP_TRAILING_WHITESPACE) | ||
| + if(NOT IS_DIRECTORY "${SDK_PATH}") | ||
| + message(FATAL_ERROR "Could not detect macOS SDK path") | ||
| + endif() | ||
| + set(CMAKE_OSX_SYSROOT "${SDK_PATH}" CACHE PATH "MacOS SDK path" FORCE) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant patch context with line numbers.
file='legacy/root/backports_macos_sdk_26_4_from_root.patch'
wc -l "$file"
sed -n '1,120p' "$file" | cat -n
# Search for any other sysroot-related references in the patch.
rg -n "CMAKE_OSX_SYSROOT|SDKROOT|xcrun|OSX_SDK_VERSION|MAC_SDK_WORKAROUND" "$file"Repository: FairRootGroup/FairSoft
Length of output: 8474
🌐 Web query:
CMAKE_OSX_SYSROOT SDKROOT CMake documentation xcrun --sdk macosx --show-sdk-path
💡 Result:
CMAKE_OSX_SYSROOT is a CMake variable used to specify the location or name of the macOS platform SDK (also applicable to iOS, tvOS, visionOS, or watchOS) [1]. It directly influences the -isysroot compiler flag and helps CMake's find commands locate files within the SDK [1][2]. Relationship with SDKROOT If CMAKE_OSX_SYSROOT is not set explicitly when a project is configured, CMake initializes its value from the SDKROOT environment variable [3][1]. Users can effectively set the SDK path in their environment by running: export SDKROOT="$(xcrun --sdk macosx --show-sdk-path)" [1][4] Key Behavior and Recent Changes 1. Default Behavior: Starting in CMake 4.0, the default value for CMAKE_OSX_SYSROOT is empty, meaning CMake no longer computes a default SDK or passes an -isysroot flag to the compiler by default [1][5]. Compilers are now expected to choose a default macOS SDK on their own [1][5]. 2. Fallback Mechanism: To support compilers that do not select a default SDK, recent versions of CMake (starting with 4.0.2) will automatically pass -isysroot using the path returned by xcrun --show-sdk-path if CMAKE_OSX_SYSROOT remains empty [1][5]. 3. Configuration: The variable should be set as a cache entry prior to the first project or enable_language command to ensure the toolchain is configured correctly [1]. It is intended for local use when creating a build tree, for example: -DCMAKE_OSX_SYSROOT=macosx [1][5]. If you are encountering issues with SDK selection, manually specifying the SDK via -DCMAKE_OSX_SYSROOT or setting the SDKROOT environment variable remains the standard way to ensure a specific SDK is used [1][4].
Citations:
- 1: https://cmake.org/cmake/help/v4.3/variable/CMAKE_OSX_SYSROOT.html
- 2: https://cmake.org/cmake/help/v3.18/variable/CMAKE_OSX_SYSROOT.html
- 3: https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_SYSROOT.html
- 4: https://gitlab.kitware.com/cmake/cmake/-/issues/26863
- 5: [clang] Driver does not select a default -isysroot when targeting macOS llvm/llvm-project#137352
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the patch section around the concern with line numbers.
sed -n '30,60p' legacy/root/backports_macos_sdk_26_4_from_root.patch | cat -nRepository: FairRootGroup/FairSoft
Length of output: 1676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any other handling of CMAKE_OSX_SYSROOT or SDKROOT in the repo.
rg -n "CMAKE_OSX_SYSROOT|SDKROOT" .Repository: FairRootGroup/FairSoft
Length of output: 4578
Honor explicit SDK selection
CMAKE_OSX_SYSROOT can be an SDK name or path, and SDKROOT is also a supported input, but this block drops any non-directory value and never imports SDKROOT before falling back to xcrun --sdk macosx; -DCMAKE_OSX_SYSROOT=macosx26.5 or SDKROOT=... can therefore be ignored and build against the default SDK instead.
🤖 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 `@legacy/root/backports_macos_sdk_26_4_from_root.patch` around lines 34 - 48,
The CMAKE_OSX_SYSROOT handling block is overriding valid explicit SDK inputs by
clearing any non-directory value and skipping SDKROOT entirely. Update the
sysroot resolution logic around the CMAKE_OSX_SYSROOT and xcrun fallback so it
preserves SDK-name values like macosx26.5, honors SDKROOT when present, and only
falls back to xcrun --sdk macosx when no explicit SDK was supplied.
| + execute_process(COMMAND ${XCRUN_EXECUTABLE} --sdk ${CMAKE_OSX_SYSROOT} --show-sdk-version | ||
| + OUTPUT_VARIABLE OSX_SDK_VERSION | ||
| + OUTPUT_STRIP_TRAILING_WHITESPACE) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the patch context around the referenced lines.
git ls-files legacy/root/backports_macos_sdk_26_4_from_root.patch
wc -l legacy/root/backports_macos_sdk_26_4_from_root.patch
sed -n '1,140p' legacy/root/backports_macos_sdk_26_4_from_root.patchRepository: FairRootGroup/FairSoft
Length of output: 6751
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact area around the version-detection logic and the consumer.
sed -n '1,120p' legacy/root/backports_macos_sdk_26_4_from_root.patch | cat -n
# Check how xcrun interprets --sdk values and whether a path is accepted there.
python3 - <<'PY'
import urllib.request, re
urls = [
"https://keith.github.io/xcode-man-pages/xcrun.1.html",
]
for url in urls:
print("URL:", url)
try:
text = urllib.request.urlopen(url, timeout=20).read().decode("utf-8", "replace")
for pat in [r'--sdk', r'show-sdk-version', r'show-sdk-path']:
m = re.search(pat, text, re.I)
print(pat, bool(m))
# print nearby context for --sdk
idx = text.lower().find('--sdk')
print(text[max(0, idx-500):idx+1500] if idx != -1 else "no --sdk")
except Exception as e:
print("ERROR", e)
PYRepository: FairRootGroup/FairSoft
Length of output: 6660
🌐 Web query:
xcrun --sdk accepts path or sdk name show-sdk-version documentation
💡 Result:
The xcrun utility accepts an SDK name or a path to an SDK via the --sdk flag [1][2][3]. This flag specifies which SDK to use, overriding any SDKROOT environment variable [1][2]. To show the version of the selected SDK, you use the --show-sdk-version option [1][4][5]. Example usage: xcrun --sdk macosx --show-sdk-version When the --sdk flag is omitted, xcrun defaults to the SDK specified by the SDKROOT environment variable, if set, or the current OS SDK [1][3]. Common options related to SDK information include: --show-sdk-path: Prints the path to the selected SDK [1][4]. --show-sdk-version: Prints the version number of the selected SDK [1][4][6]. --show-sdk-build-version: Prints the build version number of the selected SDK [1][4][7].
Citations:
- 1: https://keith.github.io/xcode-man-pages/xcrun.1.html
- 2: https://www.manpagez.com/man/1/xcrun/
- 3: https://www.unix.com/man_page/osx/1/xcrun/
- 4: https://leancrew.com/all-this/man/man1/xcrun.html
- 5: https://www.real-world-systems.com/docs/xcrun.1.html
- 6: https://linuxcommandlibrary.com/man/xcrun
- 7: https://marslo.github.io/ibook/osx/apps/xcode.html
Guard the SDK version probe.
execute_process() can leave OSX_SDK_VERSION empty if xcrun can’t resolve the selected SDK, which silently disables MAC_SDK_WORKAROUND and pushes the failure into later Clang/ROOT compilation.
🤖 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 `@legacy/root/backports_macos_sdk_26_4_from_root.patch` around lines 53 - 55,
Guard the SDK version probe around the execute_process call that sets
OSX_SDK_VERSION so it does not silently proceed when xcrun cannot resolve the
selected SDK. In the backport logic near the SDK version lookup, check the
result/status and whether OSX_SDK_VERSION is non-empty before using it to decide
MAC_SDK_WORKAROUND, and fail or emit a clear message if the probe cannot
determine the SDK version.
| +#if defined(MAC_SDK_WORKAROUND) | ||
| +/// Checks that __builtin_popcountg was called with a single argument, which is | ||
| +/// an integer. | ||
| +static bool SemaBuiltinPopcountg(Sema &S, CallExpr *TheCall) { | ||
| + if (checkArgCount(S, TheCall, 1)) | ||
| + return true; | ||
| + | ||
| + Expr *Arg = TheCall->getArg(0); | ||
| + QualType ArgTy = Arg->getType(); | ||
| + | ||
| + if (!ArgTy->isIntegerType()) { | ||
| + S.Diag(Arg->getBeginLoc(), diag::err_builtin_invalid_arg_type) | ||
| + << 1 << /*integer ty*/ 7 << ArgTy; | ||
| + return true; | ||
| + } | ||
| + return false; | ||
| +} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find relevant files and occurrences.
git ls-files | rg 'legacy/root/backports_macos_sdk_26_4_from_root\.patch|__builtin_popcountg|SemaBuiltinPopcountg|popcountg'
# Search for builtin handling and docs references.
rg -n "__builtin_popcountg|SemaBuiltinPopcountg|popcountg" .
# If the patch file is present, show the surrounding lines.
if git ls-files | rg -q '^legacy/root/backports_macos_sdk_26_4_from_root\.patch$'; then
echo "---- patch context ----"
cat -n legacy/root/backports_macos_sdk_26_4_from_root.patch | sed -n '245,285p'
fiRepository: FairRootGroup/FairSoft
Length of output: 2791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search external docs location within repo if vendored docs exist.
rg -n "popcountg" docs include lib test clang . || true
# Print any matches around builtin docs if found.
for f in $(rg -l "popcountg" docs include lib test clang 2>/dev/null || true); do
echo "---- $f ----"
sed -n '1,220p' "$f" | sed -n '/popcountg/,+20p'
doneRepository: FairRootGroup/FairSoft
Length of output: 843
🌐 Web query:
Clang LanguageExtensions __builtin_popcountg unsigned integer types
💡 Result:
builtin_popcountg is a type-generic Clang built-in function designed to return the number of set bits (1s) in an integer argument [1][2]. It serves as a generic alternative to the traditional builtin_popcount, builtin_popcountl, and builtin_popcountll built-ins [1][2]. Key technical details regarding its usage in Clang include: 1. Type Requirements: The argument passed to builtin_popcountg must be an unsigned integer type [3]. This includes standard unsigned integer types, extended unsigned integer types (such as unsigned int128), and C23 bit-precise unsigned integer types (unsigned _BitInt(N)) [2][3]. Clang will reject signed integer arguments with a diagnostic error [3]. 2. Behavior and Semantics: Similar to the GCC implementation, builtin_popcountg does not perform integral argument promotions [3][4]. This ensures consistent behavior across different integer widths without unexpected promotion-related side effects [4]. 3. Motivation: It was implemented to provide a cleaner, type-agnostic interface for population count operations, particularly benefiting codebases that utilize int128 or _BitInt types which are not easily covered by the fixed-width legacy built-ins [2]. While early development versions of the Clang implementation allowed signed integers, this was later restricted to strictly unsigned types to ensure compatibility with GCC's specification and to prevent potential logic errors resulting from implicit promotions of signed values [5][3].
Citations:
- 1: https://clang.llvm.org/docs/LanguageExtensions.html
- 2: llvm/llvm-project@21d8332
- 3: llvm/llvm-project@fc8d481
- 4: https://gcc.gnu.org/onlinedocs/gcc/Bit-Operation-Builtins.html
- 5: [clang] Implement __builtin_popcountg llvm/llvm-project#82359
🌐 Web query:
site:clang.llvm.org __builtin_popcountg unsigned integer type Language Extensions
💡 Result:
builtin_popcountg is a type-generic compiler builtin function provided as a Clang language extension [1]. It is designed to count the number of set bits (population count) in an unsigned integer of any standard integer width [2][3]. Unlike the fixed-width variants such as builtin_popcount (for int), builtin_popcountl (for long), and builtin_popcountll (for long long), builtin_popcountg automatically adapts to the type of the argument provided [4][3]. It requires a single argument, which must be an unsigned integer [2][3].
Citations:
- 1: https://clang.llvm.org/docs/LanguageExtensions.html
- 2: https://clang.llvm.org/doxygen/SemaChecking_8cpp_source.html
- 3: https://clang.llvm.org/doxygen/SemaChecking_8cpp.html
- 4: https://clang.llvm.org/doxygen/InterpBuiltin_8cpp_source.html
Reject signed arguments for __builtin_popcountg.
__builtin_popcountg is unsigned-only, so isIntegerType() is too broad here; use isUnsignedIntegerType() instead.
🤖 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 `@legacy/root/backports_macos_sdk_26_4_from_root.patch` around lines 257 - 273,
The SemaBuiltinPopcountg check is too permissive because it accepts any integer
type, including signed integers, for __builtin_popcountg. Update the argument
validation in SemaBuiltinPopcountg to require an unsigned integer type instead
of using isIntegerType(), while keeping the existing argument count and
diagnostic flow unchanged.
1 participant
The PR fixes the following two problems: