Skip to content

Define default .ra rules for PPC and ARM#949

Merged
Dav1dde merged 3 commits intogetsentry:masterfrom
kuzaxak:fix-lr-for-init
Jan 12, 2026
Merged

Define default .ra rules for PPC and ARM#949
Dav1dde merged 3 commits intogetsentry:masterfrom
kuzaxak:fix-lr-for-init

Conversation

@kuzaxak
Copy link
Contributor

@kuzaxak kuzaxak commented Jan 7, 2026

Breakpad CFI readers require a .ra rule at every unwindable address, but DWARF INIT rows can omit the return-address register rule. That leaves function entry CFI unusable even though the return address lives in a fixed register by ABI.

Add PowerPC register naming so LR can be emitted and use the CPU family to supply a default .ra on INIT rows for ARM, MIPS, and PowerPC when no explicit rule is present.

Based on my research, this is required for Arm32, Arm64, PPC.

Closes #948

Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good! Mind adding 1 or 2 integration tests to tests/?

@Dav1dde
Copy link
Member

Dav1dde commented Jan 8, 2026

Fixed the unused import in the master branch, CI should pass if you update the branch.

Breakpad CFI readers [require][4] a .ra rule at every unwindable
address, but DWARF INIT rows can omit the return-address
register rule. That leaves function entry CFI unusable even
though the return address lives in a fixed register by ABI.

Add PowerPC register naming so LR can be emitted and use the
CPU family to supply a default .ra on INIT rows for ARM, MIPS,
and PowerPC when no explicit rule is present.

Based on my research, this is required for [Arm32][2], [Arm64][3],
[PPC][1].

Closes getsentry#948

[1]: https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html
[2]: https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst
[3]: https://student.cs.uwaterloo.ca/~cs452/docs/rpi4b/aapcs64.pdf
[4]: https://github.com/rust-minidump/rust-minidump/blob/df483d5774cae0fa1bb847eaa3dd8f4a15c7e9d9/breakpad-symbols/src/sym_file/walker.rs#L514-L517
@kuzaxak kuzaxak requested a review from Dav1dde January 9, 2026 09:49
@Dav1dde
Copy link
Member

Dav1dde commented Jan 9, 2026

@kuzaxak can you add 1 or 2 tests to the integration tests for aarch64 (or other architectures)?

Or if you have rough instructions how to generate some test binaries, I can also look into it!

@kuzaxak
Copy link
Contributor Author

kuzaxak commented Jan 9, 2026

@kuzaxak can you add 1 or 2 tests to the integration tests for aarch64 (or other architectures)?

Or if you have rough instructions how to generate some test binaries, I can also look into it!

@Dav1dde I was thinking to do it but I don't see a source code for the fixture app used in tests in the repo.

I can add a simple rust app that will be compiled into a binary and commit both.

@Dav1dde
Copy link
Member

Dav1dde commented Jan 9, 2026

I can add a simple rust app that will be compiled into a binary and commit both.

That'd be awesome, but realizing we don't really have a good way setup to manage/generate these files. I guess I can just compile the files locally and check them in and in a follow-up think about a better organization.

@kuzaxak
Copy link
Contributor Author

kuzaxak commented Jan 12, 2026

I can add a simple rust app that will be compiled into a binary and commit both.

That'd be awesome, but realizing we don't really have a good way setup to manage/generate these files. I guess I can just compile the files locally and check them in and in a follow-up think about a better organization.

@Dav1dde, yes, and I don't see a way to run these tests in the GHA right now, as I don't see an ARM runner.

@Dav1dde
Copy link
Member

Dav1dde commented Jan 12, 2026

@kuzaxak I think easiest is just if I compile the binaries and check them in. The tests themselves should then not need an ARM runner as we just load the binaries.

If you have a test file I can compile that'd be helpful otherwise I'll just reserve some time and figure out something, just a lot of other stuff on my plate.

Commit befa7de adds logic to emit a default `.ra` rule on INIT rows
for architectures where the return address is stored in a register by
ABI convention. The DWARF CFI for ARM64 binaries specifies which column
represents the return address (column 30 / x30) but does not emit an
explicit rule stating the RA is in that register at function entry.
The CIE only defines `DW_CFA_def_cfa` for the stack pointer, and leaf
functions have no CFI operations beyond `DW_CFA_nop`.

Without the default rule, unwinders lack the information needed to
retrieve the return address from x30 at the start of functions that
have not yet pushed LR to the stack.

This commit adds an ARM64 ELF fixture and snapshot test to verify the
fix works correctly. The fixture is compiled with -O2 to be
representative of real-world binaries. The snapshot confirms all INIT
rows include `.ra: x30` where DWARF omits an explicit rule.
@kuzaxak
Copy link
Contributor Author

kuzaxak commented Jan 12, 2026

@kuzaxak I think easiest is just if I compile the binaries and check them in. The tests themselves should then not need an ARM runner as we just load the binaries.

If you have a test file I can compile that'd be helpful otherwise I'll just reserve some time and figure out something, just a lot of other stuff on my plate.

@Dav1dde Added a commit with a simple C binary compiled to a binary, and based on it added a snapshot test.

Without a fix, it is failing:

---- cfi_from_elf_arm64 stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: symbolic-cfi/tests/snapshots/test_cfi__cfi_elf_arm64.snap
Snapshot: cfi_elf_arm64
Source: symbolic-cfi/tests/test_cfi.rs:118
──────────────────────────────────────────────────────────────────────────────────────────────────────────
Expression: cfi
──────────────────────────────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬─────────────────────────────────────────────────────────────────────────────────────────────
    0       │-STACK CFI INIT 574 30 .cfa: sp 0 + .ra: x30
    1       │-STACK CFI INIT 5a4 3c .cfa: sp 0 + .ra: x30
    2       │-STACK CFI INIT 5e0 30 .cfa: sp 0 + .ra: x30
          0 │+STACK CFI INIT 574 30 .cfa: sp 0 +
          1 │+STACK CFI INIT 5a4 3c .cfa: sp 0 +
          2 │+STACK CFI INIT 5e0 30 .cfa: sp 0 +
    3     3 │ STACK CFI 5e4 .cfa: sp 32 + x29: .cfa -32 + ^ .ra: .cfa -24 + ^
    4     4 │ STACK CFI 5ec x19: .cfa -16 + ^
    5     5 │ STACK CFI 60c .cfa: sp 0 +
    6       │-STACK CFI INIT 610 4 .cfa: sp 0 + .ra: x30
    7       │-STACK CFI INIT 620 4 .cfa: sp 0 + .ra: x30
    8       │-STACK CFI INIT 624 8 .cfa: sp 0 + .ra: x30
    9       │-STACK CFI INIT 630 c .cfa: sp 0 + .ra: x30
   10       │-STACK CFI INIT 640 24 .cfa: sp 0 + .ra: x30
   11       │-STACK CFI INIT 510 8 .cfa: sp 0 + .ra: x30
   12       │-STACK CFI INIT 670 7c .cfa: sp 0 + .ra: x30
          6 │+STACK CFI INIT 610 4 .cfa: sp 0 +
          7 │+STACK CFI INIT 620 4 .cfa: sp 0 +
          8 │+STACK CFI INIT 624 8 .cfa: sp 0 +
          9 │+STACK CFI INIT 630 c .cfa: sp 0 +
         10 │+STACK CFI INIT 640 24 .cfa: sp 0 +
         11 │+STACK CFI INIT 510 8 .cfa: sp 0 +
         12 │+STACK CFI INIT 670 7c .cfa: sp 0 +
   13    13 │ STACK CFI 674 .cfa: sp 64 + x29: .cfa -64 + ^ .ra: .cfa -56 + ^
   14    14 │ STACK CFI 67c x19: .cfa -48 + ^ x20: .cfa -40 + ^
   15    15 │ STACK CFI 688 x21: .cfa -32 + ^ x22: .cfa -24 + ^
   16    16 │ STACK CFI 690 x23: .cfa -16 + ^ x24: .cfa -8 + ^
   17    17 │ STACK CFI 6e8 .cfa: sp 0 +
   18       │-STACK CFI INIT 6f0 4 .cfa: sp 0 + .ra: x30
         18 │+STACK CFI INIT 6f0 4 .cfa: sp 0 +
────────────┴─────────────────────────────────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
Stopped on the first failure. Run `cargo insta test` to run all snapshots.

@kuzaxak
Copy link
Contributor Author

kuzaxak commented Jan 12, 2026

I think it is enough to consider the feature covered, WDYT?

@Dav1dde
Copy link
Member

Dav1dde commented Jan 12, 2026

@kuzaxak thanks for the contribution, will merge after tests and release a new version

@Dav1dde Dav1dde merged commit 30853c4 into getsentry:master Jan 12, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AArch64 CFI INIT rows omit .ra when LR is still in x30 (breakpad unwinding requires .ra)

2 participants