chore: upgrade LLVM to 22.1.4#448
Conversation
Replace the 95-entry manual component list with 8 umbrella targets (llvm-libraries, clang-libraries, etc.) and add cmake-exports so the LLVM install includes CMake config files for find_package support. Add skip_clice_build input to build-llvm workflow to allow building LLVM without requiring clice to compile (useful when upgrading LLVM with API changes).
clice is a language server and does not need all 20+ codegen backends. Keep targets that have tablegen-generated resource headers (ARM/AArch64 for arm_neon.h etc, RISCV for riscv_vector.h) plus X86 for desktops.
Remove iOS/tvOS/watchOS/visionOS simulators, unused Xcode platforms, Android SDK, .NET, PowerShell, and Haskell to reclaim disk space. Revert LLVM_TARGETS_TO_BUILD back to all.
Revert temporary cleanup-only test state, restore all 14 matrix entries with macOS disk cleanup included. Delete test-macos-cleanup.yml.
Key breaking changes handled: - clang/Driver/Options.h moved to clang/Options/Options.h - NestedNameSpecifier changed from pointer to value type - ElaboratedType removed from Clang AST - DependentTemplateSpecializationType merged into TemplateSpecializationType - CompilerInstance::createDiagnostics/createFileManager API changed - TypedefTypeLoc::getTypedefNameDecl() renamed to getDecl() - TraverseTypeLoc gained TraverseQualifier parameter - llvm::sys::fs::make_absolute moved to llvm::sys::path
- Add --skip-pattern to prune-llvm-bin.py to protect clang-tidy modules - New workflow downloads pre-built LLVM artifacts, builds clice, and discovers which static libraries can be safely pruned - Skip Debug builds (shared libs, unsafe to prune)
- Record each file's size before deletion so manifest includes
total_saved_bytes and per-file sizes
- Fix skip-pattern from *TidyModule* to *Tidy*Module* to correctly
match libclangTidyAbseilModule.a etc.
- Update manifest format: removed is now [{name, size}, ...]
- apply_manifest handles both old (string) and new (dict) formats
…solution Replace file(GLOB) and manual library lists with proper CMake imported targets. Transitive dependencies are now resolved automatically via LLVMConfig.cmake and ClangConfig.cmake. Also adds clangTidyCustomModule and clangTidyMPIModule (new in LLVM 22).
Aggregate shared libraries (libclang-cpp.so, libLTO.so, libRemarks.so) contain all symbols from their static counterparts, causing false positives during prune detection. Replace them with empty stubs before discovery. Also delete build output binaries before each test rebuild to force ninja to re-link.
…libs - Record nullified .so/.dylib files in manifest with original sizes - Apply mode replaces files with empty archives (!<arch>\n) instead of deleting, preserving cmake export integrity - Shared libs replaced with empty stubs (0 bytes)
- Replace prune-llvm.yml with release-llvm.yml that discovers prunable libs, applies prune manifests to all 14 artifacts, and uploads to clice-io/clice-llvm - Rewrite cmake/llvm.cmake to use native file(DOWNLOAD) instead of calling setup-llvm.py, removing the Python dependency at configure time - Workflow triggered by workflow_dispatch with source_run_id and version
Rename prune-llvm-bin.py to release-llvm.py and add a `repackage` action that downloads, prunes, and repackages all 14 LLVM artifacts in parallel using concurrent.futures. Move release job to macos-15 for faster arm64 compression.
Each of the 14 artifacts gets its own runner, uploads directly to the GitHub release via gh release upload. Metadata is collected via Actions artifacts and merged in a finalize job. Removed auto-PR creation.
macOS LTO artifacts exceeded GitHub's 2GB release asset limit at -3. With per-job parallelism, compression speed is no longer a bottleneck.
macOS LTO artifacts exceeded GitHub's 2GB release asset limit at -3. With per-job parallelism, compression speed is no longer a bottleneck.
- Always use xz -9 for repackage (sufficient to keep all artifacts under 2GB)
- Change manifest format from flat array to {version, artifacts: {name: {sha256, ...}}}
- Simplify CMake download: direct key lookup instead of iterating array
- Extract _download_and_extract and _compress_tar_xz helpers
|
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:
📝 WalkthroughWalkthroughConvert LLVM packaging to a manifest-driven CMake download/extract flow, add a release CLI and workflows to discover/apply/repackage pruned artifacts, bump LLVM to 22.1.4, and update C++ sources/tests to Clang/LLVM 22 API changes (value-based NestedNameSpecifier/TypeLoc, diagnostics/VFS, and template resolver refactor). ChangesLLVM Artifact Infrastructure & Manifest
Release Automation & Artifact Pruning
Clang/LLVM 22 API Adaptation
Sequence Diagram(s)sequenceDiagram
participant BuildRun as Build job (source run)
participant Discover as discover job (per-OS)
participant ReleaseScript as scripts/release-llvm.py
participant Repackage as repackage job
participant GitHubRelease as GitHub Release
BuildRun->>Discover: provide build artifact (SOURCE_RUN_ID)
Discover->>ReleaseScript: run discover (install_dir, build_dir)
ReleaseScript-->>Discover: pruned-libs.json (per-OS)
Repackage->>ReleaseScript: repackage (source_run_id, manifests_dir)
ReleaseScript-->>Repackage: pruned tar.xz + *.meta.json
Repackage->>GitHubRelease: upload artifacts and metadata
Repackage->>Finalize: produce merged llvm-manifest.json
Finalize->>GitHubRelease: upload llvm-manifest.json
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Replaced by native CMake file(DOWNLOAD) in cmake/llvm.cmake.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bb19b604a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| llvm::SmallVector<clang::TemplateArgument, 4> arguments; | ||
| for(auto& arg: info.arguments()) { | ||
| arguments.push_back(arg.getArgument()); | ||
| for(auto& arg: TST->template_arguments()) { | ||
| arguments.push_back(arg); |
There was a problem hiding this comment.
In instantiated dependent template specializations, copying TST->template_arguments() directly can leave outer template parameters unsubstituted; the previous DTST path transformed the TemplateArgumentLocs before lookup. For cases such as resolving typename Traits<T>::template rebind<T> while resugaring an instantiation like T=int, hole()/deduce_template_arguments() now sees T instead of int, so the resolved type can remain dependent or point at the wrong specialization. Transform the argument locs before performing the lookup/deduction, and only fall back to the original type if that transform fails.
Useful? React with 👍 / 👎.
| set(_ARCH "x64") | ||
| endif() | ||
| else() | ||
| set(_ARCH "x64") |
There was a problem hiding this comment.
This native Linux branch always selects the x64 archive, even though the manifest/release includes aarch64-linux-gnu-... artifacts and the previous setup path detected platform.machine(). On an arm64/aarch64 Linux host configuring without CLICE_TARGET_TRIPLE, CMake will download the x64 LLVM package and later link/run against the wrong architecture instead of using the arm64 artifact.
Useful? React with 👍 / 👎.
| path.write_bytes(b"") | ||
| nullified.append({"name": path.name, "size": size}) |
There was a problem hiding this comment.
In the build-llvm workflow the discover step still runs for Debug builds, and scripts/build-llvm.py sets BUILD_SHARED_LIBS=ON for non-Windows Debug/ASAN LLVM builds. This line zeroes every .so/.dylib before any successful relink proves it is unused, so the Debug packages can be uploaded with empty shared libraries even though those are the actual LLVM/Clang libraries clice links against; failed relinks only mark later static archives as required and never restore these shared libraries.
Useful? React with 👍 / 👎.
|
|
||
| # add to include directories | ||
| target_include_directories(llvm-libs INTERFACE "${LLVM_INSTALL_PATH}/include") | ||
| if(NOT EXISTS "${_INSTALL_ROOT}/lib/cmake/llvm/LLVMConfig.cmake") |
There was a problem hiding this comment.
Because _INSTALL_ROOT is reused whenever LLVMConfig.cmake already exists, reconfiguring an existing build directory after setup_llvm() changes version, LTO mode, or artifact selection will silently keep the old extracted LLVM tree while the rest of the checkout expects the new manifest/version. The previous setup path selected and hash-checked the artifact on each configure; this guard should also record/compare the selected artifact or clear the install root before skipping the download/extract.
Useful? React with 👍 / 👎.
| instance.createFileManager(std::move(remapping)); | ||
| instance.setVirtualFileSystem(std::move(remapping)); | ||
| } | ||
| instance.createFileManager(); |
There was a problem hiding this comment.
When there are no remapped files in the invocation, createVFSFromCompilerInvocation() can leave remapping null, and this unconditional createFileManager() falls back to Clang's default real filesystem instead of params.vfs. That bypasses the project's ThreadSafeFS for background/index compilations of files without unsaved buffers, so non-PCH source files are no longer opened as volatile and can be read through unsafe cached/mmap paths while editors rewrite them.
Useful? React with 👍 / 👎.
| if(DEFINED LLVM_INSTALL_PATH AND NOT LLVM_INSTALL_PATH STREQUAL "") | ||
| get_filename_component(LLVM_INSTALL_PATH "${LLVM_INSTALL_PATH}" ABSOLUTE) | ||
| if(NOT EXISTS "${LLVM_INSTALL_PATH}") | ||
| message(FATAL_ERROR "LLVM_INSTALL_PATH does not exist: ${LLVM_INSTALL_PATH}") | ||
| endif() |
There was a problem hiding this comment.
Accepting a user-provided LLVM_INSTALL_PATH here now goes straight to find_package, but this project includes private headers such as clang/Sema/TreeTransform.h that normal LLVM/Clang installations do not ship. The removed setup path fetched or supplied those headers when they were missing; with this path, configuring against a system or manually installed LLVM succeeds initially but the build fails as soon as src/semantic/resolver.cpp includes the private Sema header.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/update-llvm-version.py (1)
10-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast if the manifest version and
--versiondiverge.This step now updates
cmake/package.cmakeand copiesllvm-manifest.jsontogether, but it never checks thatdata["version"]matchesargs.version. A stale manifest will leavesetup_llvm("22.1.4")pointing at21.1.8hashes, and the break only shows up later during download/verification. Pass the requested version intocopy_manifest()and reject mismatches before writing either file.Suggested fix
-def copy_manifest(src: Path, dest: Path) -> None: +def copy_manifest(src: Path, dest: Path, version: str) -> None: text = src.read_text(encoding="utf-8") @@ if not isinstance(data, dict) or "artifacts" not in data: print(f"Error: {src} must be a JSON object with 'artifacts' key", file=sys.stderr) sys.exit(1) + if data.get("version") != version: + print( + f"Error: {src} has version {data.get('version')!r}, expected {version!r}", + file=sys.stderr, + ) + sys.exit(1) @@ - copy_manifest(manifest_src, manifest_dest) + copy_manifest(manifest_src, manifest_dest, args.version)Also applies to: 154-155
🤖 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 `@scripts/update-llvm-version.py` around lines 10 - 28, copy_manifest currently never verifies that the manifest's version matches the requested CLI version, so a stale llvm-manifest.json can be copied under the wrong version; modify copy_manifest(src: Path, dest: Path, expected_version: str) to read data["version"] and compare it with expected_version, and if they differ print an error to stderr and exit non‑zero before writing the dest file; update any callers (where copy_manifest is invoked) to pass args.version into the new parameter so the manifest is rejected early on mismatch.
🤖 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 @.github/workflows/release-llvm.yml:
- Around line 38-40: Replace the mutable tag for actions/checkout with a pinned
immutable SHA: locate the workflow line using the external action reference
"actions/checkout@v4" and change it to the full commit SHA for the
actions/checkout repository (e.g., "actions/checkout@<full-commit-sha>"),
leaving the local action "./.github/actions/setup-pixi" unchanged; ensure you
copy the exact full commit SHA from the actions/checkout repo to guarantee
immutability.
In `@cmake/llvm.cmake`:
- Around line 26-41: The branch that sets _ARCH for native hosts always forces
"x64" for non-APPLE platforms; update the non-APPLE branches (the WIN32/else
branches shown) to detect ARM64 by checking CMAKE_SYSTEM_PROCESSOR like the
APPLE branch does: replace the hardcoded set(_ARCH "x64") in the Windows and
Linux (else) branches with a conditional that tests CMAKE_SYSTEM_PROCESSOR
MATCHES "arm64|aarch64|ARM64" and sets _ARCH to "arm64" when matched and "x64"
otherwise, keeping _PLATFORM and _TOOLCHAIN assignments intact; use the same
pattern as the APPLE branch so manifest filenames resolve correctly.
- Around line 62-118: The cache is not keyed by LLVM version so
_download_and_extract/_download_llvm may reuse a wrong archive or install;
change the logic to include the requested LLVM_VERSION (or the artifact SHA256)
in _DOWNLOAD_PATH and _INSTALL_ROOT (e.g. append "${LLVM_VERSION}" or
"${_SHA256}" to those paths) or validate the existing install before reuse by
reading a marker (e.g. create/check a VERSION or SHA marker file in
_INSTALL_ROOT or compare the manifest SHA256 for _FILENAME) and if it mismatches
remove/re-download; update references to _DOWNLOAD_PATH and _INSTALL_ROOT and
add the validation step inside _download_llvm before short-circuiting to ensure
the cached files correspond to the requested LLVM_VERSION/_SHA256.
In `@scripts/release-llvm.py`:
- Around line 118-157: discover() mutates the install tree by calling
_nullify_shared_libs() (which truncates shared libs) and _try_delete() (which
moves/deletes safe files) and then returns without restoring the original files;
to fix, change discover() to operate on a temporary copy of install_dir (or copy
files to a temp dir at the start) and run _nullify_shared_libs(), _try_delete(),
_remove_binaries(), and _run_build() against that copy (or, alternatively,
restore files after testing by moving backups back before returning); update
references to _candidate_files(), _nullify_shared_libs(), and _try_delete() to
use the copied path so the original install tree remains unchanged for
subsequent packaging.
In `@src/semantic/resolver.cpp`:
- Around line 965-968: The code currently memoizes a fallback unresolved
dependent specialization (using resolved.insert of TST/original and the
TLB.pushTrivial path), which makes a first miss permanent across later resolve()
calls; change the logic so that the resolved map is only updated when resolution
actually succeeds: do not insert the unresolved fallback (the TST returned via
TLB.pushTrivial or the variable named original in the later block) into
resolved; instead only cache the successful concrete result, or restrict storing
unresolved entries to a single top-level resolution pass scope. Locate the
checks around resolved.find(TST), the TLB.pushTrivial call, and the place that
currently assigns original into resolved, and gate those inserts so
unresolved/fallback results are never stored.
In `@src/semantic/semantic_visitor.h`:
- Line 518: The code incorrectly calls loc.getDecl() on a clang::TypedefTypeLoc;
replace that call with loc.getTypedefNameDecl() so the variable receives a
TypedefNameDecl* as intended (update the declaration that uses decl from auto
decl = loc.getDecl() to use loc.getTypedefNameDecl()). Ensure any subsequent
uses expect a TypedefNameDecl* (adjust variable type or casts if needed) in
semantic_visitor.h where TypedefTypeLoc is handled.
---
Outside diff comments:
In `@scripts/update-llvm-version.py`:
- Around line 10-28: copy_manifest currently never verifies that the manifest's
version matches the requested CLI version, so a stale llvm-manifest.json can be
copied under the wrong version; modify copy_manifest(src: Path, dest: Path,
expected_version: str) to read data["version"] and compare it with
expected_version, and if they differ print an error to stderr and exit non‑zero
before writing the dest file; update any callers (where copy_manifest is
invoked) to pass args.version into the new parameter so the manifest is rejected
early on mismatch.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e36c2b4-4fda-4cf8-9dd8-a35c5050a4f6
📒 Files selected for processing (27)
.github/workflows/build-llvm.yml.github/workflows/release-llvm.ymlcmake/llvm.cmakecmake/package.cmakeconfig/llvm-manifest.jsonscripts/build-llvm.pyscripts/llvm-components.jsonscripts/prune-llvm-bin.pyscripts/release-llvm.pyscripts/update-llvm-version.pyscripts/upload-llvm.pyscripts/validate-llvm-components.pysrc/command/argument_parser.cppsrc/command/search_config.cppsrc/command/toolchain.cppsrc/compile/compilation.cppsrc/index/usr_generation.cppsrc/semantic/ast_utility.cppsrc/semantic/filtered_ast_visitor.hsrc/semantic/resolver.cppsrc/semantic/resolver.hsrc/semantic/selection.cppsrc/semantic/semantic_visitor.hsrc/syntax/scan.cpptests/unit/command/argument_parser_tests.cpptests/unit/feature/inlay_hint_tests.cpptests/unit/semantic/selection_tests.cpp
💤 Files with no reviewable changes (3)
- scripts/validate-llvm-components.py
- scripts/llvm-components.json
- scripts/prune-llvm-bin.py
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: ./.github/actions/setup-pixi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact referenced section in the workflow
echo "== release-llvm.yml (lines 1-120) =="
nl -ba .github/workflows/release-llvm.yml | sed -n '1,120p'
echo
echo "== release-llvm.yml (lines 30-70) =="
nl -ba .github/workflows/release-llvm.yml | sed -n '30,70p'
echo
echo "== External action uses in release-llvm.yml =="
rg -n "^\s*uses:\s*[a-zA-Z0-9_.-]+/[^@ ]+@[^ ]+" .github/workflows/release-llvm.yml || true
echo
echo "== External action uses across .github/workflows (pinned vs not pinned) =="
rg -n "^\s*uses:\s*(actions|docker|github|microsoft|google|aws|azure|hashicorp|helm)/[^@ ]+@([^0-9a-f]{7,40}|v[0-9]+|main|master|master|latest)" .github/workflows || true
echo
echo "== Local action uses across .github/workflows =="
rg -n "^\s*uses:\s*\./\.github/actions/" .github/workflows || trueRepository: clice-io/clice
Length of output: 1950
Pin external actions to immutable SHAs in .github/workflows/release-llvm.yml (lines 38-40).
uses: actions/checkout@v4 is a mutable tag; switch it to actions/checkout@<full-commit-sha>. (./.github/actions/setup-pixi is a local action, so this doesn’t apply.)
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 38-38: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 38-38: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/release-llvm.yml around lines 38 - 40, Replace the mutable
tag for actions/checkout with a pinned immutable SHA: locate the workflow line
using the external action reference "actions/checkout@v4" and change it to the
full commit SHA for the actions/checkout repository (e.g.,
"actions/checkout@<full-commit-sha>"), leaving the local action
"./.github/actions/setup-pixi" unchanged; ensure you copy the exact full commit
SHA from the actions/checkout repo to guarantee immutability.
Source: Linters/SAST tools
| if(auto iter = resolved.find(TST); iter != resolved.end()) { | ||
| --indent; | ||
| TLB.pushTrivial(context, iter->second, {}); | ||
| return iter->second; |
There was a problem hiding this comment.
Don't memoize the unresolved dep-TST fallback.
Lines 965-968 treat any cached TST as final, and Lines 1038-1040 cache original even when resolution failed. That makes the first miss sticky across later resolve() calls, even if the same AST node is revisited with a richer instantiation stack and would succeed on retry.
At minimum, keep unresolved dependent specializations out of resolved; otherwise scope this cache to a single top-level resolution pass.
Minimal correctness fix
LOG_DEBUG("{}→ <unresolved dep-TST>", pad());
--indent;
auto original = clang::QualType(TST, 0);
- resolved.try_emplace(TST, original);
TLB.pushTrivial(context, original, {});
return original;Also applies to: 1036-1040
🤖 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 `@src/semantic/resolver.cpp` around lines 965 - 968, The code currently
memoizes a fallback unresolved dependent specialization (using resolved.insert
of TST/original and the TLB.pushTrivial path), which makes a first miss
permanent across later resolve() calls; change the logic so that the resolved
map is only updated when resolution actually succeeds: do not insert the
unresolved fallback (the TST returned via TLB.pushTrivial or the variable named
original in the later block) into resolved; instead only cache the successful
concrete result, or restrict storing unresolved entries to a single top-level
resolution pass scope. Locate the checks around resolved.find(TST), the
TLB.pushTrivial call, and the place that currently assigns original into
resolved, and gate those inserts so unresolved/fallback results are never
stored.
| /// ^~~~ reference | ||
| VISIT_TYPELOC(TypedefTypeLoc) { | ||
| auto decl = loc.getTypedefNameDecl(); | ||
| auto decl = loc.getDecl(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for TypedefTypeLoc API documentation/usage in LLVM headers
rg -n "class TypedefTypeLoc" --type=cpp -A 20 | rg -E "(getDecl|getTypedefNameDecl)"Repository: clice-io/clice
Length of output: 1931
🌐 Web query:
Does LLVM 22 TypedefTypeLoc::getDecl() return TypedefNameDecl pointer?
💡 Result:
In LLVM/Clang, TypedefTypeLoc does not have a member function named getDecl. Instead, it provides the getTypedefNameDecl method, which returns a pointer to a TypedefNameDecl [1][2][3]. The getTypedefNameDecl method is defined as follows: TypedefNameDecl* getTypedefNameDecl const { return getTypePtr->getDecl; } The underlying TypedefType, which TypedefTypeLoc wraps, does have a getDecl method that returns a TypedefNameDecl* [4]. The confusion often arises because other TypeLoc classes, such as TagTypeLoc (or classes inheriting from InheritingConcreteTypeLoc that wrap tags), may have a getDecl method that returns a different type (e.g., TagDecl*) [5][3].
Citations:
- 1: https://codebrowser.dev/llvm/clang/include/clang/AST/TypeLoc.h.html
- 2: https://fossies.org/dox/clang-21.1.8.src/classclang_1_1TypedefTypeLoc.html
- 3: https://fossies.org/dox/clang-21.1.8.src/TypeLoc_8h_source.html
- 4: https://fossies.org/dox/clang-21.1.8.src/classclang_1_1TypedefType.html
- 5: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TypeLoc.h
Fix TypedefTypeLoc accessor: loc.getDecl() is not valid
clang::TypedefTypeLoc provides getTypedefNameDecl() (returns TypedefNameDecl*) and does not have getDecl(). The auto decl = loc.getDecl(); change at src/semantic/semantic_visitor.h:518 should be replaced with loc.getTypedefNameDecl() to match the prior API/behavior.
🤖 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 `@src/semantic/semantic_visitor.h` at line 518, The code incorrectly calls
loc.getDecl() on a clang::TypedefTypeLoc; replace that call with
loc.getTypedefNameDecl() so the variable receives a TypedefNameDecl* as intended
(update the declaration that uses decl from auto decl = loc.getDecl() to use
loc.getTypedefNameDecl()). Ensure any subsequent uses expect a TypedefNameDecl*
(adjust variable type or casts if needed) in semantic_visitor.h where
TypedefTypeLoc is handled.
- Remove stale prune logic from build-llvm.yml (now in release-llvm.yml) - Fix release race condition: reuse existing release + --clobber uploads - Add actions: read permission to repackage job - Restore TransformNestedNameSpecifierLoc + TransformTemplateArguments in resolver dependent-TST path to match old DTST behavior - Add ARM64 detection for Windows/Linux in cmake artifact name selection - Unify artifact naming: release-llvm.py is single source of truth with build_artifact_name(), PLATFORM_INFO, and artifact-name subcommand - Deduplicate upload-llvm.py by importing from release-llvm.py - Convert release-llvm.py CLI to argparse subcommands - Remove stale LLVM-version-referencing comments
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/release-llvm.py (1)
49-67: ⚡ Quick winRename ambiguous loop variable
ltolto.The single-letter
lis easily confused with the digit1. Renaming improves readability.♻️ Suggested fix
ARTIFACTS = [ - build_artifact_name(p, a, m, lto=l, asan=s) - for p, a, m, l, s in [ + build_artifact_name(p, a, m, lto=lto, asan=asan) + for p, a, m, lto, asan in [ ("linux", "aarch64", "RelWithDebInfo", True, False), ... ] ]🤖 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 `@scripts/release-llvm.py` around lines 49 - 67, The list comprehension uses an ambiguous loop variable named `l`; rename it to `lto` in the tuple unpacking and subsequent use so it’s clear this flag represents LTO—update the comprehension header (for p, a, m, l, s -> for p, a, m, lto, s) and any place that passes that variable into build_artifact_name (e.g., the call in ARTIFACTS that references l) to use `lto`, keeping build_artifact_name and other identifiers unchanged.Source: Linters/SAST tools
🤖 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 `@scripts/upload-llvm.py`:
- Around line 40-44: The loop that inserts artifact metadata into
manifest["artifacts"] uses the basename key (variable filename from
release_llvm.build_metadata_entry) and will silently overwrite if two
artifact_files share the same basename; update the loop (where artifact_files is
iterated and entry/filename are created) to detect duplicates before insertion:
check if filename already exists in manifest["artifacts"] and fail fast
(raise/exit with a clear error mentioning the current path and the conflicting
existing entry) or otherwise produce a unique key (for example include the
original relative path) so no overwrite occurs; reference
release_llvm.build_metadata_entry, artifact_files, filename, and
manifest["artifacts"] when making the change.
---
Nitpick comments:
In `@scripts/release-llvm.py`:
- Around line 49-67: The list comprehension uses an ambiguous loop variable
named `l`; rename it to `lto` in the tuple unpacking and subsequent use so it’s
clear this flag represents LTO—update the comprehension header (for p, a, m, l,
s -> for p, a, m, lto, s) and any place that passes that variable into
build_artifact_name (e.g., the call in ARTIFACTS that references l) to use
`lto`, keeping build_artifact_name and other identifiers 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ed2f1c3-5b18-4191-86ba-76274f552d6d
📒 Files selected for processing (7)
.github/workflows/build-llvm.yml.github/workflows/release-llvm.ymlcmake/llvm.cmakescripts/release-llvm.pyscripts/upload-llvm.pysrc/semantic/resolver.cppsrc/semantic/semantic_visitor.h
💤 Files with no reviewable changes (1)
- src/semantic/semantic_visitor.h
🚧 Files skipped from review as they are similar to previous changes (2)
- cmake/llvm.cmake
- src/semantic/resolver.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c609dc80b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if(CLICE_TARGET_TRIPLE MATCHES "^aarch64") | ||
| set(_ARCH "aarch64") |
There was a problem hiding this comment.
When CLICE_TARGET_TRIPLE is a Darwin arm64 triple such as aarch64-apple-darwin, this branch sets _ARCH to aarch64, so _detect_llvm_artifact_name later looks for aarch64-macos-clang-...tar.xz. The manifest and release artifacts use the macOS spelling arm64-macos-clang-..., and the removed setup script mapped aarch64 targets to arm64, so configuring this supported-style target now fails with “No matching LLVM artifact in manifest” instead of downloading the macOS arm64 package.
Useful? React with 👍 / 👎.
| ARCHIVE=$(python3 scripts/release-llvm.py artifact-name $NAME_ARGS) | ||
|
|
||
| set -eo pipefail | ||
| tar -C .llvm -cf - build-install | xz -T0 -9 -c > "${ARCHIVE}" |
There was a problem hiding this comment.
With the build-llvm dispatch defaults (skip_upload is false), the later upload job still downloads this archive and scripts/upload-llvm.py publishes it and saves the manifest used by update-clice; because this commit removed all pruning/apply steps from this workflow, that default path now records hashes for unpruned packages, or gets out of sync if the new release-llvm workflow subsequently overwrites the same assets with pruned repackages. Either gate the old upload path behind the repackaging workflow or make skip_upload the default for LLVM-only source builds.
Useful? React with 👍 / 👎.
| push: | ||
| branches: [chore/upgrade-llvm-22] | ||
| paths: | ||
| - ".github/workflows/release-llvm.yml" | ||
| - "scripts/release-llvm.py" |
There was a problem hiding this comment.
On pushes to chore/upgrade-llvm-22 that touch this workflow or scripts/release-llvm.py, workflow_dispatch inputs are absent, so the env block falls back to the hard-coded run ID/version and the later gh release upload --clobber steps can overwrite the external clice-llvm 22.1.4 release from a stale build just because this branch was updated. Keep this release job manual-only, or require explicit run/version values before any clobbering upload.
Useful? React with 👍 / 👎.
- Remove push trigger from release-llvm.yml; only workflow_dispatch with required inputs can publish to clice-llvm, preventing accidental overwrites - Add version stamp (.llvm-version) to cmake download cache so LLVM version upgrades trigger re-download instead of silently reusing stale installs; also verify SHA256 of cached download archives - Fail hard when prune manifest is missing during repackage instead of warning and publishing unpruned artifacts
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 (2)
.github/workflows/release-llvm.yml (2)
177-183:⚠️ Potential issue | 🟠 MajorFail immediately if a
*.meta.jsonsidecar is missing to avoid publishing an incompletellvm-manifest.json.
finalizebuildsllvm-manifest.jsonsolely from downloadedmetadata-*/**.meta.json. Ifartifacts/${{ matrix.artifact }}.meta.jsonis missing,actions/upload-artifact@v4defaultsif-no-files-foundtowarn, so the workflow won’t fail and the release can publish a partial manifest.Suggested fix
- name: Upload metadata uses: actions/upload-artifact@v4 with: name: metadata-${{ matrix.artifact }} path: artifacts/${{ matrix.artifact }}.meta.json + if-no-files-found: error compression-level: 0 retention-days: 1🤖 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 @.github/workflows/release-llvm.yml around lines 177 - 183, The Upload metadata step (uses: actions/upload-artifact@v4) currently can silently succeed when artifacts/${{ matrix.artifact }}.meta.json is missing because upload-artifact defaults if-no-files-found to warn; change the step to fail fast by setting the upload action input if-no-files-found: error or add an explicit pre-check step that verifies the file artifacts/${{ matrix.artifact }}.meta.json exists (failing the job if not) before calling upload-artifact, so finalize will never build llvm-manifest.json from a partial set of metadata sidecars.
1-18:⚠️ Potential issue | 🟠 MajorSerialize publishes per
${{ inputs.llvm_version }}to prevent release asset racesTwo concurrent
workflow_dispatchruns with the same${{ inputs.llvm_version }}reuse the same GitHub release tag and each run overwrites release assets withgh release upload ... --clobber(binaries inrepackage,llvm-manifest.jsoninfinalize). That can leaveclice-io/clice-llvmwith binaries from one run and a manifest from another.
- Add workflow concurrency for this tag.
- Optional hardening: add
permissions: actions: readto thediscoverjob to matchrepackageforgh run download.- Optional hardening: set
if-no-files-found: erroron theUpload metadatastep (defaults towarn), sincefinalizewill crash if no*.meta.jsonare present.Suggested fix
name: release llvm on: workflow_dispatch: @@ env: SOURCE_RUN_ID: ${{ inputs.source_run_id }} LLVM_VERSION: ${{ inputs.llvm_version }} + +concurrency: + group: release-llvm-${{ inputs.llvm_version }} + cancel-in-progress: false🤖 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 @.github/workflows/release-llvm.yml around lines 1 - 18, Add workflow-level concurrency keyed on the LLVM version to serialize runs for the same tag (use concurrency: { group: "release-llvm-${{ inputs.llvm_version }}", cancel-in-progress: false }) so concurrent workflow_dispatch runs with the same inputs.llvm_version cannot clobber release assets; also add permissions: { actions: read } to the discover job to match repackage (for gh run download), and change the Upload metadata step in the finalize job to set if-no-files-found: error (instead of the default warn) so missing *.meta.json fails early.
🧹 Nitpick comments (1)
.github/workflows/release-llvm.yml (1)
20-31: ⚡ Quick winDeclare
actions: readondiscover.This job also calls
gh run download, but unlikerepackageit inherits whatever the repository default token permissions happen to be. On installations that defaultGITHUB_TOKENto contents-only,discoverwill fail before pruning even starts.Suggested fix
discover: + permissions: + contents: read + actions: read strategy: fail-fast: falseAlso applies to: 52-61
🤖 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 @.github/workflows/release-llvm.yml around lines 20 - 31, The discover job currently runs gh run download but doesn't declare repository token permissions, so add an explicit permissions block (permissions: actions: read) to the discover job to ensure gh run download can access artifacts; locate the job named "discover" in the workflow and add permissions: actions: read at the same indentation as runs-on, and apply the same change to the other similar job referenced in the file (the job that also invokes gh run download, e.g. "repackage") so both jobs explicitly grant actions read permission for GITHUB_TOKEN.
🤖 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.
Outside diff comments:
In @.github/workflows/release-llvm.yml:
- Around line 177-183: The Upload metadata step (uses:
actions/upload-artifact@v4) currently can silently succeed when artifacts/${{
matrix.artifact }}.meta.json is missing because upload-artifact defaults
if-no-files-found to warn; change the step to fail fast by setting the upload
action input if-no-files-found: error or add an explicit pre-check step that
verifies the file artifacts/${{ matrix.artifact }}.meta.json exists (failing the
job if not) before calling upload-artifact, so finalize will never build
llvm-manifest.json from a partial set of metadata sidecars.
- Around line 1-18: Add workflow-level concurrency keyed on the LLVM version to
serialize runs for the same tag (use concurrency: { group: "release-llvm-${{
inputs.llvm_version }}", cancel-in-progress: false }) so concurrent
workflow_dispatch runs with the same inputs.llvm_version cannot clobber release
assets; also add permissions: { actions: read } to the discover job to match
repackage (for gh run download), and change the Upload metadata step in the
finalize job to set if-no-files-found: error (instead of the default warn) so
missing *.meta.json fails early.
---
Nitpick comments:
In @.github/workflows/release-llvm.yml:
- Around line 20-31: The discover job currently runs gh run download but doesn't
declare repository token permissions, so add an explicit permissions block
(permissions: actions: read) to the discover job to ensure gh run download can
access artifacts; locate the job named "discover" in the workflow and add
permissions: actions: read at the same indentation as runs-on, and apply the
same change to the other similar job referenced in the file (the job that also
invokes gh run download, e.g. "repackage") so both jobs explicitly grant actions
read permission for GITHUB_TOKEN.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14e5e94c-86ce-408a-8060-037b3b13c53e
📒 Files selected for processing (3)
.github/workflows/release-llvm.ymlcmake/llvm.cmakescripts/release-llvm.py
🚧 Files skipped from review as they are similar to previous changes (2)
- cmake/llvm.cmake
- scripts/release-llvm.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4adaa1ea88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type = ET->getNamedType(); | ||
| } | ||
|
|
||
| if(auto TST = type->getAs<clang::TemplateSpecializationType>()) { |
There was a problem hiding this comment.
When a type is spelled with an elaborated keyword or qualifier such as struct Foo / typename ns::Foo, Clang wraps the named type in an ElaboratedType. With the unwrapping removed, decl_of() falls through the type-class switch and returns nullptr for that wrapper, so callers like VisitVarDecl, VisitFieldDecl, VisitTypedefNameDecl, and base-class relation recording stop emitting the referenced type relation for those valid spellings. Strip ElaboratedType to its named type before the template-specialization and type-class handling as before.
Useful? React with 👍 / 👎.
| if(const InjectedClassNameType* InjT = T->getAs<InjectedClassNameType>()) { | ||
| T = InjT->getInjectedSpecializationType(); | ||
| continue; | ||
| Out << '$'; | ||
| VisitTagDecl(InjT->getDecl()); | ||
| return; |
There was a problem hiding this comment.
For an injected class name inside a class template, this now mangles the template pattern (S) instead of the injected specialization (S<T>). That makes USRs disagree between declarations that spell a parameter as S inside the class and out-of-line definitions that spell the same type as S<T>, so the index can treat one member function as two unrelated symbols. Keep normalizing InjectedClassNameType through getInjectedSpecializationType() before visiting it.
Useful? React with 👍 / 👎.
Remove clice build/test steps, test-cross job, upload job, and update-clice job. These responsibilities now live in release-llvm.yml.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/build-llvm.yml:
- Around line 145-150: The workflow step "Clone llvm-project" currently
interpolates the user input directly into the run script via `${{
inputs.llvm_version }}`, which allows template injection; instead pass the input
through the step's env context (set an env key like VERSION: ${{
inputs.llvm_version }}) and then reference the safe shell variable (`$VERSION`)
inside the run script used by that step (keep the step name "Clone llvm-project"
and the shell variable `VERSION` so you can find it). Ensure the run script uses
the env variable (quoted) when printing and when supplying the git --branch
argument to avoid direct template interpolation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 740fd0ce-8759-4bf3-95fc-bb709140cbab
📒 Files selected for processing (1)
.github/workflows/build-llvm.yml
| - name: Clone llvm-project | ||
| shell: bash | ||
| run: | | ||
| VERSION="${{ inputs.llvm_version || '21.1.8' }}" | ||
| VERSION="${{ inputs.llvm_version }}" | ||
| echo "Cloning LLVM ${VERSION}..." | ||
| git clone --branch "llvmorg-${VERSION}" --depth 1 https://github.com/llvm/llvm-project.git .llvm |
There was a problem hiding this comment.
Template injection vulnerability from user-controlled input.
Direct interpolation of ${{ inputs.llvm_version }} into the shell script enables code injection if a malicious value is provided. While the attack surface is limited to users with workflow dispatch permissions, defense-in-depth recommends using the env context instead.
🔒 Recommended fix: pass input via env context
- name: Clone llvm-project
shell: bash
+ env:
+ LLVM_VERSION: ${{ inputs.llvm_version }}
run: |
- VERSION="${{ inputs.llvm_version }}"
+ VERSION="${LLVM_VERSION}"
echo "Cloning LLVM ${VERSION}..."
git clone --branch "llvmorg-${VERSION}" --depth 1 https://github.com/llvm/llvm-project.git .llvm🧰 Tools
🪛 zizmor (1.25.2)
[error] 148-148: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/build-llvm.yml around lines 145 - 150, The workflow step
"Clone llvm-project" currently interpolates the user input directly into the run
script via `${{ inputs.llvm_version }}`, which allows template injection;
instead pass the input through the step's env context (set an env key like
VERSION: ${{ inputs.llvm_version }}) and then reference the safe shell variable
(`$VERSION`) inside the run script used by that step (keep the step name "Clone
llvm-project" and the shell variable `VERSION` so you can find it). Ensure the
run script uses the env variable (quoted) when printing and when supplying the
git --branch argument to avoid direct template interpolation.
Source: Linters/SAST tools
- Add clangOptions to llvm-libs link list (new LLVM 22 library for driver option table, fixes undefined symbol in Debug shared builds) - Apply ruff/clang-format to Python scripts and resolver.cpp
These are superseded by release-llvm.yml which handles the full discover → repackage → upload pipeline.
1 participant
Summary
find_package(LLVM/Clang)with umbrella distribution componentsrelease-llvm.ymlworkflow: discover unused libs → prune → repackage → upload to clice-llvmxz -9eextreme compression (arm64-macos-lto: 2021 MB → 1688 MB)Test plan
Summary by CodeRabbit
Dependencies
New Features
Improvements
Bug Fixes / Behavior