ci(gc): consume the setup-llvm22 action's LLVM 22 pair in the RS4GC job - #7384
Conversation
📝 WalkthroughWalkthroughThe Ubuntu workflow now uses the ChangesLLVM toolchain setup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
What this unblocksThis fixes the It's not a required check, so it doesn't hard-block merges, but it's a red gate that should land first to clear the inherited failure on the open PRs that currently trip it:
x86-64, where |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/gc-native-roots.yml:
- Around line 220-229: Update the LLVM installation block around the versioned
apt-get command so the generic llvm clang installation runs whenever the
discovered versioned package install fails, rather than exiting under set -e.
After either installation path, re-resolve llvm_bin and the opt/clang
executables before the final same-directory pair check, preserving the existing
versioned clang co-location behavior.
🪄 Autofix
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 Plus
Run ID: 33a0469d-6025-4a0e-8978-ce183733dd7f
📒 Files selected for processing (1)
.github/workflows/gc-native-roots.yml
|
Your diagnosis is right — I think the cause sits one level up, so I've opened #7388 against the composite action instead: There's also a sharper problem in the block you're editing, independent of the missing clang. This workflow already does llvm_bin="$(dirname "$(command -v opt || true)")"
...
sudo apt-get install -y -qq llvm clang # Ubuntu 24.04 → LLVM 18If Once #7388 lands there's a guaranteed co-located 22 pair at One other thing: there's no |
* ci: install clang-22 in setup-llvm22 and assert the pair apt ships opt and clang as separate packages, so llvm-22-dev alone leaves /usr/lib/llvm-22/bin/opt with no clang beside it -- the state ubuntu-24.04-arm runners land in. Consumers needing a matched opt+clang pair (the RS4GC arm in gc-native-roots.yml) then fail the pair-check, or fall back to hand-rolled discovery and install the distro's unversioned llvm clang, which on Ubuntu 24.04 is LLVM 18 -- running opt 18 over IR emitted by Perry's linked LLVM 22. Install clang-22, symlink it under the llvm-config-22 prefix so directory-based resolution finds a co-located pair, and assert clang's major alongside the existing llvm-config check. A green setup step that leaves a mismatched clang is the failure this action exists to prevent. Fixes the root cause of #7384 for all 18 workflows rather than one. * docs: name the fragment for its real PR (#7388) --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
e2f88d5 to
d129943
Compare
|
Thanks — this is the better framing, and you're right that the sharper bug is the skew, not the missing clang. I took the workflow-block simplification. #7384 now drops the hand-discovery entirely and uses the Until #7388 lands, the block installs the matching Also added a |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/gc-native-roots.yml (1)
219-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one LLVM 22 provisioning contract.
The supplied
.github/actions/setup-llvm22/action.ymlalready installs and verifies the matched LLVM 22 tools. The workflow duplicates that setup, and the changelog documents the old ownership model.
.github/workflows/gc-native-roots.yml#L219-L224: remove the duplicateclang-22installation and symlink block. Keep the configured prefix and final pair check.changelog.d/7384-rs4gc-llvm22-prefix.md#L1-L1: state that the workflow consumes the action-provided LLVM 22 pair. Remove the claim that the action omitsclang-22and the temporary#7388bridge.🤖 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/gc-native-roots.yml around lines 219 - 224, Use the setup-llvm22 action as the sole LLVM 22 provisioning contract: in .github/workflows/gc-native-roots.yml lines 219-224, remove the duplicate clang-22 installation and symlink block while preserving the configured prefix and final compiler pair check. In changelog.d/7384-rs4gc-llvm22-prefix.md line 1, document that the workflow consumes the action-provided LLVM 22 pair and remove the outdated claim that clang-22 is omitted and the temporary `#7388` bridge is needed.
🤖 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.
Nitpick comments:
In @.github/workflows/gc-native-roots.yml:
- Around line 219-224: Use the setup-llvm22 action as the sole LLVM 22
provisioning contract: in .github/workflows/gc-native-roots.yml lines 219-224,
remove the duplicate clang-22 installation and symlink block while preserving
the configured prefix and final compiler pair check. In
changelog.d/7384-rs4gc-llvm22-prefix.md line 1, document that the workflow
consumes the action-provided LLVM 22 pair and remove the outdated claim that
clang-22 is omitted and the temporary `#7388` bridge is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b09fa5a-f6d5-49f1-a7ad-f84d2504eea0
📒 Files selected for processing (2)
.github/workflows/gc-native-roots.ymlchangelog.d/7384-rs4gc-llvm22-prefix.md
d129943 to
c9ecaf6
Compare
|
Follow-up now that #7388 has merged: I dropped the |
The native-roots-rs4gc job re-discovered opt by hand and could resolve an unversioned /usr/bin/opt, then install the distro's LLVM 18 — both binaries present, the same-directory pair-check passing, and RS4GC running opt 18 over IR from Perry's linked LLVM 22 (a green gate on the wrong LLVM). Consume the matched LLVM 22 opt+clang pair the setup-llvm22 action installs and verifies under $LLVM_SYS_221_PREFIX instead, dropping the hand-discovery and its generic llvm/clang fallback.
c9ecaf6 to
37ab182
Compare
jdalton
changed the title
|
This is better than what I suggested — dropping the bridge entirely rather than keeping it alongside the prefix is the right call, and the nine lines of hand-discovery going away takes the LLVM-18 skew hazard with them. Two things from my side that this now composes with:
With those in, the arm now compiles everything and runs probe 1 with real GC metrics, then segfaults in One caveat on that issue worth knowing before anyone chases it: I checked ten consecutive |
* docs(plan): fold in the 2026-08-04 findings Two things this plan treated as measured were not. Statepoints could not compile on aarch64-ELF at all -- a hard failure on a default-on path, from two stacked bugs (#7390: the compact stack-map parser did not model GNU-as `sym = expr`, emitted only at -O3 and only on ELF; and the assembler was not told the -mcpu the code generator was told, so Graviton-emitted SVE was rejected) behind two toolchain ones (#7384, #7388). And three of the four RS4GC matrix arms had NEVER executed, in any run, for want of a concurrency group (#7393). Every "the ELF arm is the only one red" conclusion rested on arms that never reached a runner. That is a fifth way a gate cannot fail, and it is now written down. Also folded in: nine Layer 3 rooting fixes and the rule they share (ordering, not missing roots; a fault that MOVES is a real fix, one that does not move by a byte was already dead before you rooted it); #7380's type confusion and the `gc_type == GC_TYPE_OBJECT` generalisation; RSS -69% (#7377); and the first honest performance measurement -- two benchmarks that measure nothing (#7395) and the array-store guard's siting cost (#7396). The Layer 1 framing is corrected: lower_exprs_rooted already implements the RFC's proposal for codegen operands, gated on any_later_ref_may_trigger_gc, and all four arms of func_ref.rs use it. So the gap is Layer 3, where #7389 supplies the first structural answer. * docs: name the fragment for its real PR (#7397) --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
2 participants
Summary
The
native-roots-rs4gcmatrix'subuntu-24.04-arm(aarch64/ELF) arm has been failing withno matched opt+clang pair under /usr/lib/llvm-22/bin — RS4GC cannot run. This is a toolchain-discovery gap in the workflow, not a GC-code problem: that runner image ships llvm-22'soptbut no co-locatedclang, and the discovery only installed a toolchain whenoptwas missing — so on this arm it installed nothing, and the finalopt && clangpair-check then failed. The x86-64 arm, whereclangsits next toopt, was never affected.Changes
.github/workflows/gc-native-roots.yml— in the Ubuntu branch of the opt/clang discovery: after resolvingopt's directory, derive its LLVM major version and install the version-matchedclang-<N>(falling back to the unversionedllvm/clangmetapackages when the dir isn't a/usr/lib/llvm-<N>/bin) whenever either half of the pair is missing — not only whenoptis absent. If apt lands the versioned clang at/usr/bin/clang-<N>instead of the llvm bin dir, symlink it in so the same-directory pair-check (andPERRY_LLVM_CLANG) resolves.Ubuntu packages
opt(inllvm-<N>) andclang(inclang-<N>) separately, so an image can genuinely have one without the other — which is exactly what the arm runner does.Why the old logic couldn't recover, step by step
On
ubuntu-24.04-arm(LLVM_SYS_221_PREFIX=/usr/lib/llvm-22):command -v optfinds no unversionedopt, sollvm_binfalls to the newest/usr/lib/llvm-*/binglob →/usr/lib/llvm-22/bin./usr/lib/llvm-22/bin/optexists, so the install-fallback (guarded on! -x opt) is skipped./usr/lib/llvm-22/bin/clangdoes not exist, so the final[ ! -x opt ] || [ ! -x clang ]check trips and the job exits 1.The install condition now also fires on a missing
clang, and it installsclang-22(matched to theoptalready present) rather than an unversioned clang that could skew versions.Related issue
n/a — CI gate fix for the RS4GC native-roots matrix. Unblocks the
ubuntu-24.04-armarm for every PR that inherits this workflow.Test plan
Validated the changed shell locally (the arm runner itself is the real proof, on CI):
cargo build --releaseclean — n/a (CI-workflow-only change, no Rust touched)cargo test …— n/abash -nvalidated; package-selection dry-run correct on versioned and unversioned dirsChecklist
ci:prefix conventioncrates/touched, so no changelog fragment required (skip-changelogterritory)Summary by CodeRabbit