gc: decode ELF stack maps, and make the compact-map refusal say why (#7321) - #7331
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds target-aware ELF stack-map parsing, descriptive validation errors, compact-map round-trip verification, AArch64 ELF exception-symbol handling, and a temporary x86 native-root diagnostic job. ChangesGC map hardening and target diagnostics
Estimated code review effort: 4 (Complex) | ~60 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 refusal was correct but opaque: every parse failure collapsed to None, so a module that could not be compacted said only that it could not be compacted. Return a reason from each step instead. Also fail closed on a directive inside the block whose byte width the rewriter does not model, rather than skipping it -- skipping shifts every structural offset after it, which decodes a root list from the wrong bytes instead of failing.
AArch64/ELF spells the stack map's 32-bit fields `.word`, which GNU as defines as the target's machine word -- 4 bytes on AArch64, 2 on x86. Make the width a function of the target rather than a constant, and add the other spellings LLVM's per-target MCAsmInfo can choose. Fail closed on any directive inside the block whose width is not modelled, rather than skipping it: skipping shifts every structural offset after it, so the decode reads a live set out of the wrong bytes. Verify the compact stream re-decodes to exactly the live set LLVM recorded, on every target, and sabotage-test that check so a pass means the detector works rather than that nothing was tried. Also fix the aarch64-Linux link failure the above uncovered: eh_walker's global_asm defined its two symbols with Mach-O's leading underscore unconditionally, so perry-runtime could not link on aarch64 ELF.
5668a67 to
ea7b489
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-codegen/src/gc_map.rs (1)
238-250: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the
.p2alignexponent;1usize << valuecan panic.
valueis anyu32the assembly names. Forvalue >= 64,1usize << valueoverflows the shift: it panics in debug builds and produces a masked, meaningless alignment in release builds. Both outcomes break the module's contract, which is a named refusal for anything it cannot model. Use a checked shift and report the line.🐛 Proposed fix for the unbounded shift
let align = if directive == ".p2align" { - 1usize << value + 1usize.checked_shl(value).ok_or_else(|| { + format!( + "line {}: `.p2align` exponent {value} is out of range in `{line}`", + index + 1 + ) + })? } else { value as usize };🤖 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 238 - 250, Guard the `.p2align` calculation in the alignment-directive parsing logic with a checked shift so unsupported exponents are rejected instead of panicking or producing an invalid alignment. When the shift cannot be represented, return the existing line-numbered parse error format for the offending assembly line; leave `.align` and `.balign` handling unchanged.
🧹 Nitpick comments (4)
crates/perry-codegen/src/gc_map.rs (4)
114-124: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider covering the bare
x86andi86arch spellings.
word_width_formatchesx86_64*and the four-characteri?86names. A triple whose arch component is writtenx86ori86falls through to 4 bytes, which is the wrong width for those targets. The gap is silent, and this function documents that a wrong width relocates every root.♻️ Proposed widening of the x86 match
fn word_width_for(target: &str) -> usize { let arch = target.split('-').next().unwrap_or_default(); // `x86_64h` (Haswell Mach-O) and the whole i?86 family included. - if arch.starts_with("x86_64") - || (arch.len() == 4 && arch.starts_with('i') && arch.ends_with("86")) - { + let i86_family = arch.starts_with('i') && arch.ends_with("86") && arch.len() <= 4; + if arch.starts_with("x86") || i86_family { 2 } else { 4 } }🤖 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 114 - 124, Update word_width_for so the architecture match also recognizes the bare x86 and i86 spellings as 2-word-width targets, while preserving the existing x86_64* and i?86 handling and the 4-byte fallback for other architectures.
132-141: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
.dc.ais address-width, not a fixed 8 bytes.
.dc.aemits one target address. On a 32-bit target it is 4 bytes, so the fixed 8 repeats the exact class of defect this change removes for.word. Resolve it from the target pointer width, or drop it from the table so an unexpected.dc.arefuses by name instead of shifting every following offset.♻️ Proposed target-dependent handling
-fn directive_width(directive: &str, word_width: usize) -> Option<usize> { +fn directive_width(directive: &str, word_width: usize, pointer_width: usize) -> Option<usize> { match directive { ".byte" | ".1byte" | ".dc.b" => Some(1), ".short" | ".2byte" | ".value" | ".hword" | ".dc.w" => Some(2), ".long" | ".4byte" | ".dc.l" => Some(4), - ".quad" | ".8byte" | ".xword" | ".dc.a" => Some(8), + ".quad" | ".8byte" | ".xword" => Some(8), + ".dc.a" => Some(pointer_width), ".word" => Some(word_width), _ => None, } }🤖 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 132 - 141, Update directive_width so ".dc.a" uses the target address width, reusing the existing word_width parameter like ".word", instead of returning a fixed 8-byte width. Preserve the existing fixed widths for the other directives and ensure ".dc.a" remains recognized on both 32-bit and 64-bit targets.
257-267: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe ignored fill operand and the unbounded count both weaken the fail-closed rule.
.zero,.space, and.skipaccept an optional second operand, the fill byte. This code always fills zeros, so a non-zero fill decodes as different bytes than the assembler emits. The same block refuses unmodelled directives for exactly this reason. A very largecountalso makesbytes.resizeallocate without a bound, andbytes.len() + countcan overflow.♻️ Proposed strictness for the fill operand and the count
if directive == ".zero" || directive == ".space" || directive == ".skip" { - let first = operand.split(',').next().unwrap_or_default().trim(); + let mut fields = operand.split(','); + let first = fields.next().unwrap_or_default().trim(); let count: usize = first .parse() .map_err(|_| format!("line {}: unparseable fill count in `{line}`", index + 1))?; + if let Some(fill) = fields.next() { + if fill.trim().parse::<u64>() != Ok(0) { + return Err(format!( + "line {}: non-zero fill value in `{line}` is not modelled", + index + 1 + )); + } + } + if count > MAX_BLOCK_FILL { + return Err(format!( + "line {}: fill count {count} exceeds the plausible block size", + index + 1 + )); + } bytes.resize(bytes.len() + count, 0); continue; }🤖 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 257 - 267, Update the fill-directive handling in the parser around the `.zero`/`.space`/`.skip` branch to validate the optional fill-byte operand and reject any non-zero or malformed value instead of silently using zero. Bound the parsed count before resizing, use checked length arithmetic, and return a descriptive parse error when the count would overflow or exceed the supported input size.
945-1043: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new shorthand section terminators.
The fixtures end the block with
.subsections_via_symbolsor a full.sectiondirective. No test ends a block with one of the shorthand terminators added at Lines 212-215, such as.textor.rodata. That branch is the one whose absence the comment describes as a refusal far from the cause, so it deserves a direct assertion.♻️ Proposed test
+ /// The shorthand section directives terminate the block. Without this the + /// parser accumulates the following section as map bytes. + #[test] + fn shorthand_section_directive_ends_the_block() { + let asm = aarch64_elf_sample_asm() + .replace("\t.section\t\".note.GNU-stack\",\"\",`@progbits`\n", "\t.text\n\tret\n"); + let (_, stats) = compact_stack_map_asm(&asm, true, "aarch64-unknown-linux-gnu") + .expect("a `.text` shorthand must terminate the block") + .expect("rewritten"); + assert_eq!(stats.roots, 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 `@crates/perry-codegen/src/gc_map.rs` around lines 945 - 1043, Add a focused test alongside the existing stack-map assembly fixtures, such as near compacts_and_keeps_only_real_roots, that terminates the LLVM stack-map block with a shorthand directive handled by the new terminator logic (for example .text or .rodata). Assert compact_stack_map_asm still parses and rewrites the block successfully, verifying the expected output and root statistics while preserving existing behavior for .subsections_via_symbols and full .section terminators.
🤖 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 379-429: Update the x86-diagnose job’s probe loop to count matched
probes and captured refused stack-map blocks, then fail the job if either count
is zero. Add x86-diagnose to the gc-native-roots-complete fan-in job’s needs
list and result checks so its failure cannot leave the required context green.
In `@crates/perry-codegen/src/gc_map.rs`:
- Around line 269-294: Update parse_block’s width-sized directive handling to
process comma-separated operands individually, parsing and emitting one value
per element while preserving the directive’s width for each emitted value.
Ensure symbolic operands are reserved separately with correct offsets, or
explicitly reject list operands by directive name; do not treat the entire
comma-separated operand string as one symbol.
- Around line 210-216: Extend the section terminator matching in the GC map
parsing logic to include the Mach-O shorthands `.literal4`, `.literal16`, and
`.const_data`, alongside the existing section names. Preserve the current
handling for `.section`, `.subsections_via_symbols`, and all existing shorthand
terminators.
In `@crates/perry-runtime/src/eh_walker.rs`:
- Around line 106-107: Update the regression explanation comment near the Mach-O
prefix to clearly state that hardcoding the Mach-O prefix caused the AArch64
Linux link failure, including the undefined perry_eh_capture_context reference.
---
Outside diff comments:
In `@crates/perry-codegen/src/gc_map.rs`:
- Around line 238-250: Guard the `.p2align` calculation in the
alignment-directive parsing logic with a checked shift so unsupported exponents
are rejected instead of panicking or producing an invalid alignment. When the
shift cannot be represented, return the existing line-numbered parse error
format for the offending assembly line; leave `.align` and `.balign` handling
unchanged.
---
Nitpick comments:
In `@crates/perry-codegen/src/gc_map.rs`:
- Around line 114-124: Update word_width_for so the architecture match also
recognizes the bare x86 and i86 spellings as 2-word-width targets, while
preserving the existing x86_64* and i?86 handling and the 4-byte fallback for
other architectures.
- Around line 132-141: Update directive_width so ".dc.a" uses the target address
width, reusing the existing word_width parameter like ".word", instead of
returning a fixed 8-byte width. Preserve the existing fixed widths for the other
directives and ensure ".dc.a" remains recognized on both 32-bit and 64-bit
targets.
- Around line 257-267: Update the fill-directive handling in the parser around
the `.zero`/`.space`/`.skip` branch to validate the optional fill-byte operand
and reject any non-zero or malformed value instead of silently using zero. Bound
the parsed count before resizing, use checked length arithmetic, and return a
descriptive parse error when the count would overflow or exceed the supported
input size.
- Around line 945-1043: Add a focused test alongside the existing stack-map
assembly fixtures, such as near compacts_and_keeps_only_real_roots, that
terminates the LLVM stack-map block with a shorthand directive handled by the
new terminator logic (for example .text or .rodata). Assert
compact_stack_map_asm still parses and rewrites the block successfully,
verifying the expected output and root statistics while preserving existing
behavior for .subsections_via_symbols and full .section terminators.
🪄 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: ad5869a6-d58e-4373-ad26-8b6ee84c8d2b
📒 Files selected for processing (4)
.github/workflows/gc-native-roots.ymlchangelog.d/7331-elf-stack-map-word-width.mdcrates/perry-codegen/src/gc_map.rscrates/perry-runtime/src/eh_walker.rs
| x86-diagnose: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 45 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: dtolnay/rust-toolchain@stable | ||
| - uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| shared-key: gc-native-roots-x86 | ||
| - name: Build compiler and static runtime (perry-dev profile) | ||
| run: | | ||
| export RUSTFLAGS="-C force-frame-pointers=yes -C force-unwind-tables=yes" | ||
| cargo build --profile perry-dev -p perry -p perry-runtime-static -p perry-stdlib-static | ||
| - name: Toolchain | ||
| run: | | ||
| uname -m | ||
| lscpu | head -20 || true | ||
| which clang || true | ||
| clang --version || true | ||
| ls /usr/lib/ | grep -i llvm || true | ||
| - name: Compile and dump the block | ||
| run: | | ||
| set -uo pipefail | ||
| export PERRY_RUNTIME_DIR="$PWD/target/perry-dev" | ||
| export PERRY_NO_AUTO_OPTIMIZE=1 | ||
| for probe in benchmarks/gc_ratchet/probes/*.ts; do | ||
| name=$(basename "$probe" .ts) | ||
| set +e | ||
| PERRY_STATEPOINTS=1 ./target/perry-dev/perry "$probe" -o "/tmp/x86-$name" > "/tmp/x86-$name.log" 2>&1 | ||
| rc=$? | ||
| set -e | ||
| if [ "$rc" -eq 0 ]; then | ||
| echo "== $name: COMPILED" | ||
| readelf -S "/tmp/x86-$name" | grep -E "perry_gcmap|llvm_stackmaps" || echo " (no gc map section!)" | ||
| continue | ||
| fi | ||
| echo "== $name: FAILED" | ||
| grep -E "reason:|target:|assembly left at:|cannot yet express" "/tmp/x86-$name.log" | head -5 | ||
| asm=$(grep -o '/tmp/[^ ]*\.o\.s' "/tmp/x86-$name.log" | head -1) | ||
| [ -n "$asm" ] && [ -f "$asm" ] || continue | ||
| start=$(grep -n 'llvm_stackmaps' "$asm" | head -1 | cut -d: -f1) | ||
| [ -n "$start" ] || continue | ||
| echo " stackmaps at line $start of $(wc -l < "$asm")" | ||
| echo " --- first 40 lines of the block:" | ||
| sed -n "${start},$((start+40))p" "$asm" | ||
| echo " --- directive census from the block onward:" | ||
| tail -n "+${start}" "$asm" | awk '{print $1}' | sort | uniq -c | sort -rn | head -25 | ||
| echo " --- last 15 lines of the file:" | ||
| tail -15 "$asm" | ||
| done | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make x86-diagnose a checked fan-in arm.
gc-native-roots-complete does not depend on x86-diagnose. A diagnostic-job failure can therefore leave the documented required context green.
The loop also has no final assertion that it matched a probe and captured a refused stack-map block. Track both counts and fail when either count is zero. Add x86-diagnose to the fan-in needs list and result checks.
Proposed fix
+ probes=0
+ blocks=0
for probe in benchmarks/gc_ratchet/probes/*.ts; do
+ [ -f "$probe" ] || continue
+ probes=$((probes+1))
name=$(basename "$probe" .ts)
...
[ -n "$start" ] || continue
+ blocks=$((blocks+1))
...
done
+ [ "$probes" -gt 0 ]
+ [ "$blocks" -gt 0 ]
- needs: [native-roots-aarch64, native-roots-rs4gc-aarch64, statepoints-refuse-x86]
+ needs: [native-roots-aarch64, native-roots-rs4gc-aarch64, x86-diagnose, statepoints-refuse-x86]
"native-roots-rs4gc-aarch64=${{ needs.native-roots-rs4gc-aarch64.result }}" \
+ "x86-diagnose=${{ needs.x86-diagnose.result }}" \
"statepoints-refuse-x86=${{ needs.statepoints-refuse-x86.result }}"; doAs per coding guidelines, “A CI gate must … assert that the behavior it measures actually executed.”
🤖 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 379 - 429, Update the
x86-diagnose job’s probe loop to count matched probes and captured refused
stack-map blocks, then fail the job if either count is zero. Add x86-diagnose to
the gc-native-roots-complete fan-in job’s needs list and result checks so its
failure cannot leave the required context green.
Source: Coding guidelines
| if line.starts_with(".section") | ||
| || line.starts_with(".subsections_via_symbols") | ||
| || matches!( | ||
| line.split_whitespace().next().unwrap_or_default(), | ||
| ".text" | ".data" | ".bss" | ".rodata" | ".const" | ".cstring" | ".literal8" | ||
| ) | ||
| { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the remaining Mach-O section shorthands to the terminator set.
The list covers .const, .cstring, and .literal8, but omits .literal4, .literal16, and .const_data, which the Mach-O asm printer also emits as shorthand. If one of those follows the stack-map block, the loop keeps accumulating the following section as map bytes, which is the failure the comment above describes.
♻️ Proposed additional terminators
|| matches!(
line.split_whitespace().next().unwrap_or_default(),
- ".text" | ".data" | ".bss" | ".rodata" | ".const" | ".cstring" | ".literal8"
+ ".text"
+ | ".data"
+ | ".bss"
+ | ".rodata"
+ | ".const"
+ | ".const_data"
+ | ".cstring"
+ | ".literal4"
+ | ".literal8"
+ | ".literal16"
+ | ".tdata"
+ | ".tbss"
)📝 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.
| if line.starts_with(".section") | |
| || line.starts_with(".subsections_via_symbols") | |
| || matches!( | |
| line.split_whitespace().next().unwrap_or_default(), | |
| ".text" | ".data" | ".bss" | ".rodata" | ".const" | ".cstring" | ".literal8" | |
| ) | |
| { | |
| if line.starts_with(".section") | |
| || line.starts_with(".subsections_via_symbols") | |
| || matches!( | |
| line.split_whitespace().next().unwrap_or_default(), | |
| ".text" | |
| | ".data" | |
| | ".bss" | |
| | ".rodata" | |
| | ".const" | |
| | ".const_data" | |
| | ".cstring" | |
| | ".literal4" | |
| | ".literal8" | |
| | ".literal16" | |
| | ".tdata" | |
| | ".tbss" | |
| ) | |
| { |
🤖 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 210 - 216, Extend the
section terminator matching in the GC map parsing logic to include the Mach-O
shorthands `.literal4`, `.literal16`, and `.const_data`, alongside the existing
section names. Preserve the current handling for `.section`,
`.subsections_via_symbols`, and all existing shorthand terminators.
| if let Some(width) = directive_width(directive, word_width) { | ||
| match parse_int(operand) { | ||
| Some(value) => bytes.extend_from_slice(&value.to_le_bytes()[..width]), | ||
| None => { | ||
| // A symbolic operand. Two kinds appear: the `.quad` | ||
| // function address, and — at `-O3` — the `.long` | ||
| // instruction offset as a label difference. Remember the | ||
| // expression and reserve the slot so every later | ||
| // structural offset stays correct. | ||
| symbols.insert(bytes.len(), operand.to_string()); | ||
| bytes.extend_from_slice(&0u64.to_le_bytes()[..width]); | ||
| } | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if !is_zero_width_directive(directive) { | ||
| return Err(format!( | ||
| "line {}: unrecognised directive `{directive}` inside the stack-map block \ | ||
| (`{line}`). Its byte width is unknown, and guessing it would shift every \ | ||
| offset after it — decoding a root list from the wrong bytes rather than \ | ||
| failing. Add it to `directive_width` (with its width) or to \ | ||
| `is_zero_width_directive`.", | ||
| index + 1 | ||
| )); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for existing fixtures or tests that feed grouped operands to parse_block,
# and for any other producer of the stack-map block spelling.
fd -e rs . crates/perry-codegen/src | xargs rg -n -C3 '\.byte\\t\{|join\(","\)|splitn\(2, char::is_whitespace\)'
rg -n -C3 'parse_int\(' crates/perry-codegen/srcRepository: PerryTS/perry
Length of output: 7499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gc_map parser outline =="
ast-grep outline crates/perry-codegen/src/gc_map.rs --view expanded | sed -n '1,220p' || true
echo "== gc_map relevant sections =="
sed -n '180,340p' crates/perry-codegen/src/gc_map.rs
sed -n '700,735p' crates/perry-codegen/src/gc_map.rs
echo "== references to gc_map parsing/emit =="
rg -n -C2 'parse_block|gc_map|emit_gc_map|parse_int|directive_width|is_zero_width_directive' crates/perry-codegen/src/gc_map.rs
echo "== behavioral probe: parse_int/parser behavior for comma groups =="
python3 - <<'PY'
def parse_int(text):
text = text.strip()
hex_val = text.removeprefix("0x").removeprefix("0X")
if hex_val:
try:
return int(hex_val, 16)
except ValueError:
return None
try:
return int(text, 10)
except ValueError:
return None
text = "1,2,3"
print(f"parse_int({text!r}) = {parse_int(text!r)}")
print("current parser would insert symbol", text, "and reserve one byte only")
# For multi-value operand handling, width 1, values 1,2,3 should emit 3 bytes.
print("current bytes: 1")
print("splitting elements and parsing each would emit 3:", len(text.split(",")))
PYRepository: PerryTS/perry
Length of output: 14149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== directive_width implementation =="
sed -n '132,148p' crates/perry-codegen/src/gc_map.rs
echo "== deterministic operand parsing simulation =="
python3 - <<'PY'
def parse_int(text):
text = text.strip()
for prefix in ("0x", "0X"):
if text.startswith(prefix):
try:
return int(text[len(prefix):], 16)
except ValueError:
return None
if text.startswith("-"):
try:
return (int(text[1:]) << 64) & 0xffffffffffffffff
except ValueError:
return None
try:
return int(text, 10)
except ValueError:
return None
for operand in ("1,2,3", "256,257", "func_offset"):
value = parse_int(operand)
print(f"{operand!r} -> parse_int={value!r}")
PYRepository: PerryTS/perry
Length of output: 1063
Handle comma-separated operands for width-sized data directives.
parse_block passes the whole operand to parse_int, so .byte 1,2,3 becomes a symbol reservation of one byte instead of three emitted bytes. These grouped operands are produced by emit_asm for stream chunks, so compact assembly can desynchronise on the next pass unless each comma-separated element is parsed as a separate value or the list operands are refused by name.
🤖 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 269 - 294, Update
parse_block’s width-sized directive handling to process comma-separated operands
individually, parsing and emitting one value per element while preserving the
directive’s width for each emitted value. Ensure symbolic operands are reserved
separately with correct offsets, or explicitly reject list operands by directive
name; do not treat the entire comma-separated operand string as one symbol.
…at it found (#7334) The diagnostic job was merged with #7331 before it had served its purpose; it has now, so it comes out. Its answer contradicts the narrative still at the top of this file: the compact-map rewriter parses x86-64 stack maps fine. Every root is Indirect [RSP + off] (DWARF 7), round-tripping through the explicit- register tag, and no clang version or -march setting reproduced a parse failure. The x86-64 defect is at collection time, which #7324 refuses for. Replace the wrong explanation rather than leave two contradictory ones in the same header. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Makes the compact-map rewriter read the stack maps ELF backends emit, and
makes its refusal say why — which is how the actual defect was found rather
than guessed at.
What was wrong
.word.LLVM picks the spelling of each fixed-width field per target through
MCAsmInfo::Data32bitsDirective, and the AArch64 ELF backend picks.word.GNU
asdefines.wordas the target's natural machine word: 4 bytes onAArch64, ARM, PowerPC, MIPS, SPARC and RISC-V — but 2 on x86, where it dates
to 16-bit. The rewriter had one fixed table (
".long" | ".word" => 4), writtenagainst the Mach-O/AArch64 spelling it was developed on.
A real
aarch64-unknown-linux-gnustack map, captured fromperry --target linuxon08_map_set_sidetables.ts:Every 32-bit field is
.word, every 16-bit field is.hword, every 64-bitfield is
.xword. The width of.wordis therefore load-bearing for the wholeblock: two bytes of drift per field relocates every root after it.
What this changes
.wordis resolved against the target, not assumed..hword/.xwordwere already handled;
.2byte/.4byte/.8byte/.1byte/.dc.*are addedbecause they are the other spellings an
MCAsmInfocan choose.instead of being skipped. Skipping is the unsound option: the block is decoded
by structural offset, so one ignored directive that emits bytes shifts
everything after it, and the decode then either fails somewhere unrelated or
succeeds against the wrong bytes. This is what turned an opaque refusal into a
one-line diagnosis.
byte offset, whether the counts disagreed — plus the target. Previously every
failure collapsed to
Noneand the message could only repeat that it hadfailed.
verify_roundtripdecodes the emitted varint stream exactly asperry-runtime'sparse_gc_mapdoes and asserts it reproduces each record'slive set, refusing otherwise. Unlike
PERRY_STACKMAP_WALKER=verifythis needsno architecture-specific stack walker, so it holds where that check cannot
run. It is always on: it walks bytes already in cache, and an assertion that
has to be switched on is off when it matters.
eh_walker'sglobal_asm!no longer hardcodes Mach-O symbol naming. Itdefined
_perry_eh_capture_context/_perry_eh_install_contextwith aleading underscore unconditionally under
target_arch = "aarch64", so onaarch64 ELF the definitions and the
extern "C"declarations weredifferent symbols and
perry-runtimecould not link at all(
undefined reference to perry_eh_capture_context). Found by building thisbranch for
aarch64-unknown-linux-gnu; it blocks that host entirely, so it isfixed here rather than filed.
Tests
Both new tests fail on
main.aarch64_elf_word_directives_decode_to_the_right_root— a real ELF-spelledmap (
.wordinstruction offset,.word32-bitOffsetper location) mustyield the same single SP-relative root the Mach-O spelling gives.
word_width_is_load_bearing_not_cosmetic— reading that same map with x86's.wordwidth must not agree with the correct width. If it did, the widthwould not actually be in use and the first test would be asserting nothing.
roundtrip_check_catches_a_corrupted_streamsabotages the verifier four ways(dropped root, relocated root, truncation, trailing bytes) so a pass means the
detector works rather than that nothing was tried.
All in
perry-codegen's--libtests, whichcargo-testruns per PR.Verified
cargo test -p perry-codegen --lib: 601 passed, 0 failed.PERRY_STATEPOINTS=1 PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1,against the pinned Node oracle, with
__perry_gcmappresent and__llvm_stackmapsabsent asserted per probe.cargo fmt --all --check,scripts/check_file_size.sh,scripts/gc_store_site_inventory.py,scripts/addr_class_inventory.pyallclean.
What this does NOT claim
This is not yet evidence that
PERRY_STATEPOINTS=1compiles on x86-64Linux (#7321). The
.worddefect is real and is an ELF defect, but it isspecifically an AArch64 ELF defect — x86 ELF spells these fields
.byte/.short/.long/.quad, all of which the old table already handled. Icould not reproduce the x86-64 refusal in any configuration I could build:
Apple clang 21, Homebrew clang 19/20/22 and Ubuntu clang 18, across twelve
-marchsettings, from both a macOS and a Linux host, over all nine probes —every one produced an x86-64 ELF stack map that parses, before and after this
change. The
x86-diagnosejob in this branch exists to get the answer off arunner; GitHub's queue has not started a job for it in over an hour.
The commit that widens that job is temporary and comes out before merge.
Summary by CodeRabbit
Bug Fixes
Diagnostics