ci(gc): let the statepoint arm pass, and put ELF back under test - #7344
Closed
proggeramlug wants to merge 1 commit into
Closed
ci(gc): let the statepoint arm pass, and put ELF back under test#7344proggeramlug wants to merge 1 commit into
proggeramlug wants to merge 1 commit into
Conversation
The gate has been red on main since it landed, five runs, and neither cause was a code regression. It could not pass. native-roots-aarch64 runs on macos-14, whose image ships Apple clang 15 — which does not register the statepoint-example GC strategy — so every run died with 'error in backend: unsupported GC: statepoint-example' before a probe executed. A gate that cannot pass is as useless as one that cannot fail, and this one read as a real regression for a day. It now pins Homebrew LLVM the way the RS4GC job beside it already did, and asserts the pinned toolchain registers the strategy so a future image change fails with a clear message instead of deep inside a probe compile. Newer Apple clang does register it (21 does locally), so this is the runner image, not Apple clang. It stopped testing ELF. Both GC arms are on macOS, so nothing exercised the object format the compact map was reworked for — and every bug that reached main there was ELF-only and invisible on Mach-O: SHF_GNU_RETAIN or --gc-sections drops the section; SHF_WRITE or the relocated addresses force a DT_TEXTREL in a PIE; eh_walker's asm used the Mach-O underscore convention so aarch64-Linux could not link. native-roots-elf-aarch64 runs the same matrix on ubuntu-24.04-arm with both liveness asserts. No toolchain install needed — stock Ubuntu clang registers the strategy, verified on 18.1.3 on a real host.
📝 WalkthroughWalkthroughThe workflow pins and validates the macOS LLVM toolchain. It adds ARM64 Ubuntu ELF coverage for statepoints, GC sections, relocations, assembly, oracle output, and GC liveness. The completion job now depends on the ELF job. ChangesNative roots CI gates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UbuntuARM64
participant Clang
participant StatepointProbe
participant ELFChecks
UbuntuARM64->>Clang: Validate statepoint support
UbuntuARM64->>StatepointProbe: Run the probe matrix
StatepointProbe->>ELFChecks: Check GC sections and oracle output
StatepointProbe->>ELFChecks: Check GC metrics
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 618-624: Update the fan-in gate’s result-validation loop to read
and validate the needs result for native-roots-elf-aarch64, alongside the
existing required jobs. Preserve the always() behavior while ensuring failure,
cancellation, or skipped completion of the ELF arm prevents the gate from
passing.
- Around line 563-574: Update the ELF probe loop around the PERRY_GC_*
environment variables to include the same minor-path configuration used by the
macOS arm matrix, ensuring forced evacuation exercises precise roots rather than
the conservative path. After each probe execution, invoke
gc_evacuation_liveness_assert.py for that probe and make its failure terminate
the workflow, while preserving the existing oracle comparison and aggregate
checks.
🪄 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 Plus
Run ID: dffb5135-0fcf-464f-b6e4-84d86423174d
📒 Files selected for processing (2)
.github/workflows/gc-native-roots.ymlchangelog.d/7341-gate-toolchain-and-elf-arm.md
Comment on lines
+563
to
+574
| PERRY_STATEPOINTS=1 PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 \ | ||
| "/tmp/$name" > "/tmp/$name.out" 2> "/tmp/$name.err" | ||
| diff "/tmp/$name.oracle" "/tmp/$name.out" \ | ||
| || { echo "::error::$name output diverged from the pinned oracle"; exit 1; } | ||
| errs="$errs /tmp/$name.err" | ||
| pass=$((pass+1)) | ||
| done | ||
| echo "ELF statepoint matrix: $pass/$total" | ||
| [ "$total" -gt 0 ] || { echo "::error::no probes matched — the matrix ran on nothing"; exit 1; } | ||
| [ "$pass" -eq "$total" ] | ||
| grep -l "#gcmetric" $errs >/dev/null \ | ||
| || { echo "::error::no probe emitted gc metrics — the collector never ran"; exit 1; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Drive and prove the precise-root evacuation path.
The ELF matrix sets PERRY_GC_FORCE_EVACUATE=1, but it omits the minor-path configuration used by the macOS arm. The probes can therefore collect through the manual conservative path, where forced evacuation is inert. #gcmetric only proves that a metric was emitted. It does not prove object movement or precise-root use.
Add the minor-path settings and run gc_evacuation_liveness_assert.py for each probe.
Proposed fix
PERRY_STATEPOINTS=1 PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 \
+ PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off \
+ PERRY_GC_DIAG=1 \
"/tmp/$name" > "/tmp/$name.out" 2> "/tmp/$name.err"
diff "/tmp/$name.oracle" "/tmp/$name.out" \
|| { echo "::error::$name output diverged from the pinned oracle"; exit 1; }
+ python3 scripts/gc_evacuation_liveness_assert.py "/tmp/$name.err" --probe "$name"
errs="$errs /tmp/$name.err"As per coding guidelines, a CI gate must assert that the behavior it measures actually executed.
📝 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
| PERRY_STATEPOINTS=1 PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 \ | |
| "/tmp/$name" > "/tmp/$name.out" 2> "/tmp/$name.err" | |
| diff "/tmp/$name.oracle" "/tmp/$name.out" \ | |
| || { echo "::error::$name output diverged from the pinned oracle"; exit 1; } | |
| errs="$errs /tmp/$name.err" | |
| pass=$((pass+1)) | |
| done | |
| echo "ELF statepoint matrix: $pass/$total" | |
| [ "$total" -gt 0 ] || { echo "::error::no probes matched — the matrix ran on nothing"; exit 1; } | |
| [ "$pass" -eq "$total" ] | |
| grep -l "#gcmetric" $errs >/dev/null \ | |
| || { echo "::error::no probe emitted gc metrics — the collector never ran"; exit 1; } | |
| PERRY_STATEPOINTS=1 PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 \ | |
| PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off \ | |
| PERRY_GC_DIAG=1 \ | |
| "/tmp/$name" > "/tmp/$name.out" 2> "/tmp/$name.err" | |
| diff "/tmp/$name.oracle" "/tmp/$name.out" \ | |
| || { echo "::error::$name output diverged from the pinned oracle"; exit 1; } | |
| python3 scripts/gc_evacuation_liveness_assert.py "/tmp/$name.err" --probe "$name" | |
| errs="$errs /tmp/$name.err" | |
| pass=$((pass+1)) | |
| done | |
| echo "ELF statepoint matrix: $pass/$total" | |
| [ "$total" -gt 0 ] || { echo "::error::no probes matched — the matrix ran on nothing"; exit 1; } | |
| [ "$pass" -eq "$total" ] | |
| grep -l "`#gcmetric`" $errs >/dev/null \ | |
| || { echo "::error::no probe emitted gc metrics — the collector never ran"; exit 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/gc-native-roots.yml around lines 563 - 574, Update the ELF
probe loop around the PERRY_GC_* environment variables to include the same
minor-path configuration used by the macOS arm matrix, ensuring forced
evacuation exercises precise roots rather than the conservative path. After each
probe execution, invoke gc_evacuation_liveness_assert.py for that probe and make
its failure terminate the workflow, while preserving the existing oracle
comparison and aggregate checks.
Source: Coding guidelines
Comment on lines
+618
to
+624
| needs: | ||
| [ | ||
| native-roots-aarch64, | ||
| native-roots-rs4gc-aarch64, | ||
| native-roots-elf-aarch64, | ||
| statepoints-refuse-x86, | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check the ELF arm result in the fan-in gate.
native-roots-elf-aarch64 is in needs, but the result loop does not read its result. Because this job uses if: always(), the completion context can pass when the new ELF arm fails, is cancelled, or is skipped.
Add the new result to the loop.
Proposed fix
for arm in \
"native-roots-aarch64=${{ needs.native-roots-aarch64.result }}" \
"native-roots-rs4gc-aarch64=${{ needs.native-roots-rs4gc-aarch64.result }}" \
+ "native-roots-elf-aarch64=${{ needs.native-roots-elf-aarch64.result }}" \
"statepoints-refuse-x86=${{ needs.statepoints-refuse-x86.result }}"; doAs per coding guidelines, a CI gate must be included in required branch-protection contexts.
🤖 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 618 - 624, Update the
fan-in gate’s result-validation loop to read and validate the needs result for
native-roots-elf-aarch64, alongside the existing required jobs. Preserve the
always() behavior while ensuring failure, cancellation, or skipped completion of
the ELF arm prevents the gate from passing.
Source: Coding guidelines
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.
gc-native-rootshas been red onmainsince it landed — five consecutive runs — and neither cause was a code regression. Both are fixed here.It could not pass
native-roots-aarch64runs onmacos-14, whose image ships Apple clang 15.0.0, which does not register thestatepoint-exampleGC strategy:Every run died there before a single probe executed. That is CLAUDE.md's failure mode inverted — a gate that cannot pass is as useless as one that cannot fail, and this one read as a real regression for a day while nothing was actually wrong with the code.
I could not reproduce it locally at first because my Apple clang is 21, which does register the strategy; the log's
clang --version:line is what pinned it down. So this is a property of the runner image, not of Apple clang forever.The job now pins Homebrew LLVM — exactly what the RS4GC job beside it already does — and asserts the pinned toolchain registers the strategy, so a future image change fails with a clear message instead of deep inside a probe compile.
It stopped testing ELF
Both GC arms run on macOS, so nothing exercises the object format the compact map had to be reworked for. Every bug that reached
mainin that area was ELF-only and invisible on Mach-O:SHF_GNU_RETAIN, or--gc-sectionsdiscarded it — nothing references it, and Mach-O covers this with.no_dead_strip;SHF_WRITEtoo, or the relocated function addresses forced aDT_TEXTRELin a PIE;eh_walker's asm defined_perry_eh_capture_contextwith the Mach-O underscore convention, so aarch64-Linux could not even linkperry-runtime— which also broke the Linux ARM64 release artifact.A
native-roots-elf-aarch64arm runs the same matrix onubuntu-24.04-arm, carrying both liveness asserts:.perry_gcmappresent and.llvm_stackmapsabsent, so a run where the rewrite silently stopped cannot read as green.ARM64 rather than x86-64 because the backend is aarch64-only (#7324); on x86-64 it would exercise only the refusal, which
statepoints-refuse-x86already covers.Verified on a real host, not assumed
The ELF arm needs no toolchain install: stock Ubuntu clang registers
statepoint-example— I confirmed that on a real Ubuntu box with clang 18.1.3 by compiling a module carrying the strategy, rather than inferring it from the macOS behaviour. That is why this job pins nothing while the macOS one must.I also verified on that host that the default shadow-stack path passes 9/9 on Linux x86-64 under forced evacuation with verification — the first Linux verification of the merged work, since everything before it was macOS-only.
Note on overlap
#7339 and #7340 also touch this workflow. Neither adds an LLVM pin to the bridge job (
brew install llvmappears zero times in #7339's diff) and neither adds an ELF arm, so this should not conflict beyond ordinary rebasing — but worth a look if they land first.Summary by CodeRabbit
Bug Fixes
Documentation