fix(gc): parse GNU-as symbol assignments in the stack-map block - #7390
Merged
Conversation
proggeramlug
pushed a commit
that referenced
this pull request
Aug 4, 2026
📝 WalkthroughWalkthroughThe stack-map parser now accepts valid GNU-as symbol assignments as zero-byte content. The assembly step receives the compiler’s CPU-selection flags. Tests cover parser exclusions and assembler argument filtering. The changelog documents the AArch64 ELF failures and fixes. ChangesAArch64 ELF stack-map and assembler fixes
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
The compact stack-map rewrite refused every module on aarch64-ELF at -O3, so native-roots-rs4gc (ubuntu-24.04-arm) could never pass and statepoints -- on by default for aarch64 -- could not compile on Linux arm64. gc_map's parser walks the block directive by directive because it is a byte stream decoded by structural offset: one unmodelled directive that emits bytes shifts everything after it. Refusing the unknown is right, and it is why this surfaced as a refusal rather than a corrupt decode. What it did not model is the GNU-as symbol assignment `sym = expr` -- the bare spelling of `.set`, zero bytes, no leading directive -- so the dispatch reported the SYMBOL as an unrecognised directive. Only -O3 emits it (absolute-symbol aliases like `perry_null_guard_zero = 0` and `.Lperry_ic_8 = .Ltmp3-4`) and only on ELF, so every macOS arm stayed green. The guard tests for "not a directive this module already models" rather than "no leading dot": ELF local labels start with `.L` and appear on the left of these assignments. Expression operators are excluded so an `.if` is never mistaken for one. Reproduced without a Linux host by retargeting a traced module to aarch64-unknown-linux-gnu, running rewrite-statepoints-for-gc, and emitting with `llc -O3 -mattr=+jsconv,+v8.3a`: the real assembly parses at -O2, refuses at -O3 exactly as CI reported, and parses at both with the fix. Both regression tests fail with the guard disabled.
proggeramlug
force-pushed
the
fix/gcmap-elf-symbol-assignment
branch
from
August 4, 2026 18:14
19e7564 to
7a28460
Compare
![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 `@crates/perry-codegen/src/gc_map.rs`:
- Around line 166-196: Exclude the exact symbol name "." in is_symbol_assignment
so ". = . + 4" is not treated as a zero-byte assignment and remains available
for proper width accounting. Preserve existing handling for other symbol
assignments; only add explicit width modeling and a regression case if
supporting this spelling is required.
🪄 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: 856dea56-51e6-41fb-aeae-d3a1eaeff924
📒 Files selected for processing (2)
changelog.d/7390-gcmap-elf-symbol-assignment.mdcrates/perry-codegen/src/gc_map.rs
Comment on lines
+166
to
+196
| /// Is this a GNU-as symbol assignment (`sym = expr`) rather than a directive? | ||
| /// | ||
| /// Assemblers accept `.set sym, expr` and the bare `sym = expr` for the same | ||
| /// thing. Only the former starts with a `.`, so the directive dispatch sees the | ||
| /// SYMBOL as the mnemonic and refuses it. Both emit zero bytes. | ||
| /// | ||
| /// Deliberately narrow: the name must be a single token that is not itself a | ||
| /// directive, and the `=` must not be part of a comparison inside a longer | ||
| /// expression. `.size sym, .-sym` and `.byte 1` are unaffected. | ||
| fn is_symbol_assignment(line: &str) -> bool { | ||
| let Some((lhs, _rhs)) = line.split_once('=') else { | ||
| return false; | ||
| }; | ||
| // `==`, `>=`, `<=`, `!=` are expression operators, not an assignment. | ||
| if lhs.ends_with(['=', '>', '<', '!']) { | ||
| return false; | ||
| } | ||
| let name = lhs.trim(); | ||
| // ELF local labels start with `.L`, so "does not start with a dot" is the | ||
| // wrong test -- it would reject `.Lperry_ic_8 = …`, which -O3 emits. Test | ||
| // what actually matters instead: the LHS must not be a directive this | ||
| // module already models. Anything else that is a single bare token before | ||
| // an `=` is a symbol assignment. | ||
| !name.is_empty() | ||
| && !name.contains(char::is_whitespace) | ||
| && directive_width(name, 8).is_none() | ||
| && !is_zero_width_directive(name) | ||
| && name | ||
| .chars() | ||
| .all(|c| c.is_alphanumeric() || c == '_' || c == '$' || c == '.') | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n 'name != "\."' crates/perry-codegen/src/gc_map.rs
rg -n '\. = \. \+ 4' crates/perry-codegen/src/gc_map.rs
cargo check -p perry --profile perry-devRepository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "files matching gc_map:"
fd -a 'gc_map\.rs$' . || true
echo "relevant sections:"
if [ -f crates/perry-codegen/src/gc_map.rs ]; then
wc -l crates/perry-codegen/src/gc_map.rs
sed -n '130,220p' crates/perry-codegen/src/gc_map.rs
sed -n '250,310p' crates/perry-codegen/src/gc_map.rs
rg -n 'directive_width|is_zero_width_directive|is_symbol_assignment|RawBlock::bytes' crates/perry-codegen/src/gc_map.rs
fiRepository: PerryTS/perry
Length of output: 7390
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "behavior probe for is_symbol_assignment conditions:"
python3 - <<'PY'
def is_symbol_assignment(line: str) -> bool:
if '=' not in line:
return False
lhs, _rhs = line.split('=', 1)
if lhs.endswith(['=', '>', '<', '!']):
return False
name = lhs.strip()
return bool(
name
and not any(c.isspace() for c in name)
and name != "."
and all(c.isalnum() or c in '_$. ' for c in name)
)
for line in [". = . + 4", ". =.+4", "sym = 42", "perry_null_guard_zero = 0"]:
stripped = line.strip()
lhs = stripped.split("=", 1)[0].strip()
print(stripped, "lhs=", repr(lhs), "matches=", "dot assignment" if lhs == "." else str(is_symbol_assignment(stripped)))
PY
echo "around line 1377-1395:"
sed -n '1360,1405p' crates/perry-codegen/src/gc_map.rs
echo "targeted searches:"
rg -n '"\."\s*(\+|-|=)|\s*=.*\s*\.|local labels|Elf|ELF|location counter' crates/perry-codegen/src/gc_map.rs
echo "cargo check:"
cargo check -p perry --profile perry-devRepository: PerryTS/perry
Length of output: 480
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "behavior probe for dot assignment:"
python3 - <<'PY'
lines = [". = . + 4", ". =.+4", "sym = 42"]
for line in lines:
if '=' not in line:
print(line, "no equals")
continue
lhs, _ = line.split("=", 1)
if lhs.strip() == ".":
print(line, "matches dot assignment guard")
PY
echo "around line 1377-1395:"
sed -n '1360,1405p' crates/perry-codegen/src/gc_map.rs
echo "cargo check:"
cargo check -p perry --profile perry-devRepository: PerryTS/perry
Length of output: 2076
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "cargo check:"
cargo "+nightly" check -p perry --profile perry-dev || true
echo "current diff/stat around modified file (if available):"
git diff --stat crates/perry-codegen/src/gc_map.rs || trueRepository: PerryTS/perry
Length of output: 143
Do not treat . as a zero-byte symbol assignment.
name == "." passes is_symbol_assignment, so . = . + 4 is skipped before byte-width accounting. GNU as treats assignment to . like .org, so this emits four bytes. Skipping it removes emitted padding and can shift later stack-map offsets.
Exclude . from is_symbol_assignment. If this spelling must be supported, model the width instead of skipping it, and add a . = . + 4 regression case.
🤖 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 `@crates/perry-codegen/src/gc_map.rs` around lines 166 - 196, Exclude the exact
symbol name "." in is_symbol_assignment so ". = . + 4" is not treated as a
zero-byte assignment and remains available for proper width accounting. Preserve
existing handling for other symbol assignments; only add explicit width modeling
and a regression case if supporting this spelling is required.
Source: Coding guidelines
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 `@changelog.d/7390-gcmap-elf-symbol-assignment.md`:
- Around line 1-3: The opening sentence in the changelog entry is ungrammatical
as written—it incorrectly combines "Fixed" with "refused" creating a broken
clause structure. Rewrite the sentence to be grammatically correct by clearly
stating that the compact stack-map rewrite had an issue, while preserving the
specific details about the aarch64-ELF rejection at -O3, the native-roots-rs4gc
test failure, and the statepoint compilation problem on Linux arm64.
🪄 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: 14473598-4c0d-4aa5-b94c-7673fe8e0cdf
📒 Files selected for processing (2)
changelog.d/7390-gcmap-elf-symbol-assignment.mdcrates/perry-codegen/src/gc_map.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-codegen/src/gc_map.rs
Comment on lines
+1
to
+3
| **Fixed** the compact stack-map rewrite refused every module on aarch64-ELF at | ||
| `-O3`, so `native-roots-rs4gc (ubuntu-24.04-arm)` could never pass and statepoints | ||
| — on by default for aarch64 — could not compile on Linux arm64. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the opening sentence.
The sentence is not grammatical. State that the rewrite had an issue.
Proposed fix
-**Fixed** the compact stack-map rewrite refused every module on aarch64-ELF at
-`-O3`, so `native-roots-rs4gc (ubuntu-24.04-arm)` could never pass and statepoints
+**Fixed** an issue where the compact stack-map rewrite refused every module on
+aarch64-ELF at `-O3`, so `native-roots-rs4gc (ubuntu-24.04-arm)` could never pass and statepoints📝 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
| **Fixed** the compact stack-map rewrite refused every module on aarch64-ELF at | |
| `-O3`, so `native-roots-rs4gc (ubuntu-24.04-arm)` could never pass and statepoints | |
| — on by default for aarch64 — could not compile on Linux arm64. | |
| **Fixed** an issue where the compact stack-map rewrite refused every module on | |
| aarch64-ELF at `-O3`, so `native-roots-rs4gc (ubuntu-24.04-arm)` could never pass and statepoints | |
| — on by default for aarch64 — could not compile on Linux arm64. |
🤖 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 `@changelog.d/7390-gcmap-elf-symbol-assignment.md` around lines 1 - 3, The
opening sentence in the changelog entry is ungrammatical as written—it
incorrectly combines "Fixed" with "refused" creating a broken clause structure.
Rewrite the sentence to be grammatically correct by clearly stating that the
compact stack-map rewrite had an issue, while preserving the specific details
about the aarch64-ELF rejection at -O3, the native-roots-rs4gc test failure, and
the statepoint compilation problem on Linux arm64.
Second aarch64-linux failure, surfaced once the symbol-assignment parse
was fixed:
error: instruction requires: sve or sme
mov z1.d, #0x7fffffffffffffff
Perry compiles with `-mcpu=native`. On a host whose CPU has SVE --
Graviton, and any aarch64 server part -- LLVM emits SVE instructions.
`compact_and_assemble` then handed that text to clang with no `-mcpu` at
all, so the assembler applied the portable baseline and rejected what the
generator had just produced.
The two invocations describe the same machine and now say so: the codegen
argv's -mcpu=/-march=/-mtune= flags are forwarded to the assembler.
Optimisation and output flags deliberately are not -- they mean nothing
to an assembler, and forwarding wholesale would be a second way for the
two to disagree.
Invisible on the macOS arms, whose runner CPUs have no SVE.
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 `@changelog.d/7390-gcmap-elf-symbol-assignment.md`:
- Around line 34-35: Revise the changelog sentence beginning “Also fixed the
assembler…” so it explicitly states that the assembler CPU mismatch issue was
fixed, while preserving the existing details about the code generator CPU and
aarch64-linux failure.
🪄 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: 767f2a67-71e5-4e34-9058-a182c19dcd15
📒 Files selected for processing (3)
changelog.d/7390-gcmap-elf-symbol-assignment.mdcrates/perry-codegen/src/gc_map.rscrates/perry-codegen/src/linker.rs
Comment on lines
+34
to
+35
| **Also fixed** the assembler was invoked without the CPU the code generator was | ||
| given, so aarch64-linux failed a second time once the parse was fixed: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the sentence structure.
Also fixed the assembler was invoked... is not grammatical. State that an issue was fixed.
Proposed wording
-**Also fixed** the assembler was invoked without the CPU the code generator was
-given, so aarch64-linux failed a second time once the parse fix was fixed:
+**Also fixed an issue where** the assembler was invoked without the CPU the
+code generator was given, so aarch64-linux failed a second time after the parse fix:🤖 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 `@changelog.d/7390-gcmap-elf-symbol-assignment.md` around lines 34 - 35, Revise
the changelog sentence beginning “Also fixed the assembler…” so it explicitly
states that the assembler CPU mismatch issue was fixed, while preserving the
existing details about the code generator CPU and aarch64-linux failure.
This was referenced Aug 4, 2026
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
* 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>
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.
Fixes the red
native-roots-rs4gc (ubuntu-24.04-arm, aarch64, ELF)arm. Statepoints are on by default for aarch64, and aarch64-Linux is inside the allowed set — so this was a hard compile failure on a default-on path, not just a CI annoyance.The bug
gc_map's parser walks the stack-map block directive by directive, because the block is a byte stream decoded by structural offset: one unmodelled directive that does emit bytes shifts everything after it. So anything unrecognised is a hard error rather than a skip — the right default, and why this surfaced as a refusal instead of a corrupt decode.What it did not model is the GNU-as symbol assignment —
sym = expr, the bare spelling of.set. Zero bytes, no leading directive, so the dispatch reported the symbol as the mnemonic:Only
-O3emits it — the optimiser materialises absolute-symbol aliases likeperry_null_guard_zero = 0and.Lperry_ic_8 = .Ltmp3-4— and only on ELF. Mach-O's asm printer doesn't use this spelling, which is why every macOS arm stayed green.The guard tests for "not a directive this module already models" rather than "no leading dot": ELF local labels start with
.Land appear on the left of exactly these assignments. Expression operators (==,!=,>=,<=) are excluded so an.ifis never mistaken for one.Reproduced locally, without a Linux host
My first instinct was to defer this as unverifiable from macOS. That was wrong — the parser is a pure function over assembly text, so what I needed was ELF text, not an ELF machine:
--trace llvmon the failing probeaarch64-unknown-linux-gnu, drop the Mach-O-only.no_dead_stripmodule asmopt -passes=rewrite-statepoints-for-gc→ 109 statepointsllc -mattr=+jsconv,+v8.3a(generic ARMv8.0 can't selectfjcvtzs)The real assembly then parses at
-O2and refuses at-O3exactly as CI reported — and parses at both with the fix.Verification
-O2-O3gc_mapsuiteBoth new tests fail with the guard disabled — checked, not assumed.
Summary by CodeRabbit
Bug Fixes
Tests