-
-
Notifications
You must be signed in to change notification settings - Fork 155
fix(gc): parse GNU-as symbol assignments in the stack-map block #7390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| **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. | ||
|
|
||
| `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. That is the right default and it is 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`, which emits zero bytes and has no leading directive. The | ||
| dispatch therefore reported the *symbol* as an unrecognised directive: | ||
|
|
||
| ``` | ||
| line 5114: unrecognised directive `perry_class_keys__…__AnonShape_…` | ||
| inside the stack-map block | ||
| ``` | ||
|
|
||
| Only `-O3` emits it (the optimiser materialises absolute-symbol aliases such as | ||
| `perry_null_guard_zero = 0` and `.Lperry_ic_8 = .Ltmp3-4`), and only on ELF — | ||
| Mach-O's asm printer does not use this spelling, so every macOS arm stayed green. | ||
|
|
||
| The guard tests for "not a directive this module already models" rather than "no | ||
| leading dot", because ELF local labels legitimately start with `.L` and appear on | ||
| the left of these assignments. Expression operators (`==`, `!=`, `>=`, `<=`) are | ||
| excluded so an `.if` is never mistaken for one. | ||
|
|
||
| Reproduced locally 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` and | ||
| refused at `-O3` exactly as CI reported, and parses at both with the fix. | ||
|
|
||
| **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: | ||
|
Comment on lines
+34
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Fix the sentence structure.
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 |
||
|
|
||
| ``` | ||
| 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 are deliberately not, since they mean nothing to an assembler | ||
| and forwarding them wholesale would be a second way for the two to disagree. | ||
|
|
||
| This is invisible on the macOS arms, whose runner CPUs have no SVE. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,6 +163,38 @@ fn directive_width(directive: &str, word_width: usize) -> Option<usize> { | |
| /// everything after it; the decode then either fails somewhere unrelated or, | ||
| /// worse, succeeds against garbage. Anything not on this list and not in | ||
| /// `directive_width` is a refusal that names the directive. | ||
| /// 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 == '.') | ||
| } | ||
|
Comment on lines
+166
to
+196
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ 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
Exclude 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| fn is_zero_width_directive(directive: &str) -> bool { | ||
| matches!( | ||
| directive, | ||
|
|
@@ -235,6 +267,20 @@ fn parse_block(lines: &[&str], word_width: usize) -> Result<RawBlock, String> { | |
| { | ||
| continue; | ||
| } | ||
| // GNU-as symbol assignment: `sym = expr`, the bare form of `.set`. | ||
| // It defines a symbol and emits ZERO bytes. `.set`/`.equ` are already on | ||
| // the zero-width list; this spelling is not a directive at all, so the | ||
| // dispatch below would report the SYMBOL as an unrecognised directive. | ||
| // | ||
| // It only shows up at -O3, which is why the arm CI caught it and the -O2 | ||
| // paths never did: the optimiser materialises absolute-symbol aliases | ||
| // (`perry_null_guard_zero = …`, `perry_class_keys__… = …`) and the ELF | ||
| // asm printer emits them in this form. Mach-O output does not, so this | ||
| // is invisible on the macOS arms. | ||
| if is_symbol_assignment(line) { | ||
| continue; | ||
| } | ||
|
|
||
| let mut parts = line.splitn(2, char::is_whitespace); | ||
| let directive = parts.next().unwrap_or_default(); | ||
| let operand = parts.next().unwrap_or_default(); | ||
|
|
@@ -867,6 +913,7 @@ pub fn compact_and_assemble( | |
| target: &str, | ||
| asm_path: &Path, | ||
| obj_path: &Path, | ||
| codegen_args: &[String], | ||
| ) -> Result<()> { | ||
| let asm = fs::read_to_string(asm_path) | ||
| .with_context(|| format!("Failed to read assembly at {}", asm_path.display()))?; | ||
|
|
@@ -967,15 +1014,47 @@ pub fn compact_and_assemble( | |
| crate::statepoint_report::note_gc_map(stats.functions, stats.records, stats.roots); | ||
| } | ||
|
|
||
| assemble(clang, target, asm_path, obj_path) | ||
| assemble(clang, target, asm_path, obj_path, codegen_args) | ||
| } | ||
|
|
||
| fn assemble(clang: &Path, target: &str, asm_path: &Path, obj_path: &Path) -> Result<()> { | ||
| /// The subset of the codegen's clang argv that selects the target MACHINE. | ||
| /// | ||
| /// The assembler must be told the same machine the code generator was told, or | ||
| /// it rejects instructions the generator legitimately emitted. Optimisation and | ||
| /// output flags are excluded: they mean nothing to an assembler, and forwarding | ||
| /// them wholesale would be a second way for the two invocations to disagree. | ||
| fn cpu_selection_flags(codegen_args: &[String]) -> Vec<&String> { | ||
| codegen_args | ||
| .iter() | ||
| .filter(|a| a.starts_with("-mcpu=") || a.starts_with("-march=") || a.starts_with("-mtune=")) | ||
| .collect() | ||
| } | ||
|
|
||
| fn assemble( | ||
| clang: &Path, | ||
| target: &str, | ||
| asm_path: &Path, | ||
| obj_path: &Path, | ||
| codegen_args: &[String], | ||
| ) -> Result<()> { | ||
| // Mirror the codegen's CPU selection onto the assembler. | ||
| // | ||
| // Perry compiles with `-mcpu=native`, so on a host with SVE (Graviton, and | ||
| // any aarch64 server part) LLVM emits SVE instructions -- `mov z1.d, #…`. | ||
| // Assembling that text with a clang that was given no `-mcpu` fails with | ||
| // `instruction requires: sve or sme`, because the assembler defaults to the | ||
| // portable baseline while the code generator did not. | ||
| // | ||
| // The two invocations describe the SAME machine and must agree. Forwarding | ||
| // only the CPU-selection flags keeps that contract without dragging along | ||
| // optimisation or output flags, which mean nothing to an assembler. | ||
| let cpu_flags = cpu_selection_flags(codegen_args); | ||
| let output = Command::new(clang) | ||
| .arg("-c") | ||
| .arg(asm_path) | ||
| .arg("-o") | ||
| .arg(obj_path) | ||
| .args(&cpu_flags) | ||
| .arg("-target") | ||
| .arg(target) | ||
| .output() | ||
|
|
@@ -1302,6 +1381,79 @@ mod tests { | |
| assert!(!out.contains("__LLVM_StackMaps")); | ||
| // The dead-strip guard must survive, retargeted. | ||
| assert!(out.contains(".no_dead_strip\t_perry_gc_map")); | ||
|
|
||
| // Guard the -O3 ELF shapes that broke the aarch64-linux arm. Both are | ||
| // GNU-as symbol assignments -- zero bytes, no leading directive -- so | ||
| // the dispatch reported the SYMBOL as an unrecognised directive and | ||
| // refused the module. Mach-O never emits this spelling, which is why | ||
| // every macOS arm stayed green. | ||
| } | ||
|
|
||
| #[test] | ||
| fn the_assembler_is_told_the_same_machine_as_the_code_generator() { | ||
| // `-mcpu=native` on a host with SVE makes LLVM emit `mov z1.d, #…`. | ||
| // Assembling that with a clang given no `-mcpu` fails with | ||
| // `instruction requires: sve or sme` — the aarch64-linux arm's second | ||
| // failure, after the parse fix. Forward machine selection, nothing else. | ||
| let args: Vec<String> = [ | ||
| "-O3", | ||
| "-mcpu=native", | ||
| "-fno-math-errno", | ||
| "-march=armv8.3-a", | ||
| "-mtune=neoverse-n1", | ||
| "-o", | ||
| "out.o", | ||
| ] | ||
| .iter() | ||
| .map(|s| s.to_string()) | ||
| .collect(); | ||
| let got: Vec<&str> = super::cpu_selection_flags(&args) | ||
| .into_iter() | ||
| .map(|s| s.as_str()) | ||
| .collect(); | ||
| assert_eq!( | ||
| got, | ||
| ["-mcpu=native", "-march=armv8.3-a", "-mtune=neoverse-n1"] | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn elf_symbol_assignments_parse_as_zero_width() { | ||
| let asm = concat!( | ||
| "\t.section\t.llvm_stackmaps,\"a\",@progbits\n", | ||
| "__LLVM_StackMaps:\n", | ||
| "\t.byte\t3\n", | ||
| "perry_null_guard_zero = 0\n", | ||
| "\t.byte\t0\n", | ||
| ".Lperry_ic_8 = .Ltmp3-4\n", | ||
| "\t.hword\t0\n", | ||
| "\t.word\t2\n", | ||
| ); | ||
| let lines: Vec<&str> = asm.lines().collect(); | ||
| let block = super::parse_block(&lines, 4).expect("ELF symbol assignments must parse"); | ||
| // 1 + 1 + 2 + 4 -- the assignments contribute nothing. | ||
| assert_eq!(block.bytes.len(), 8); | ||
| } | ||
|
|
||
| #[test] | ||
| fn expression_operators_are_not_mistaken_for_assignments() { | ||
| for line in [ | ||
| ".byte\t1", | ||
| "\t.size\tsym, .-sym", | ||
| "\t.if a == b", | ||
| "\t.if a != b", | ||
| ] { | ||
| assert!( | ||
| !super::is_symbol_assignment(line), | ||
| "`{line}` must not be treated as a symbol assignment" | ||
| ); | ||
| } | ||
| for line in ["sym = 1", ".Lfoo = .Ltmp1-4", "a$b = 7"] { | ||
| assert!( | ||
| super::is_symbol_assignment(line), | ||
| "`{line}` is a symbol assignment" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the opening sentence.
The sentence is not grammatical. State that the rewrite had an issue.
Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents