Skip to content

feat(native): add support for win arm64 unwind codes#978

Open
klochek wants to merge 7 commits into
masterfrom
christopherklochek/win_arm64_unwind
Open

feat(native): add support for win arm64 unwind codes#978
klochek wants to merge 7 commits into
masterfrom
christopherklochek/win_arm64_unwind

Conversation

@klochek
Copy link
Copy Markdown

@klochek klochek commented May 14, 2026

No description provided.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 14, 2026

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against d002319

@klochek klochek force-pushed the christopherklochek/win_arm64_unwind branch 2 times, most recently from 08f62b8 to 614ab8b Compare May 15, 2026 13:58
Copy link
Copy Markdown
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

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

Very impressive! I basically only have some stylistic nits.

Comment thread symbolic-cfi/src/lib.rs Outdated
writer: &'a mut dyn Write,
}

// The kinds of registers the can be saved for unwinding.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The kinds of registers the can be saved for unwinding.
// The kinds of registers that can be saved for unwinding.

Comment thread symbolic-cfi/src/lib.rs Outdated
}
}

// Computes the memory location relative to CFI, offset above SP
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These could all be doc comments.

Suggested change
// Computes the memory location relative to CFI, offset above SP
/// Computes the memory location relative to CFI, offset above SP

Comment thread symbolic-cfi/src/lib.rs Outdated
Comment on lines +1331 to +1332
" .{typ}{first_reg}: .cfa {} + ^ .{typ}{second_reg}: .cfa {} + ^",
o1, o2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
" .{typ}{first_reg}: .cfa {} + ^ .{typ}{second_reg}: .cfa {} + ^",
o1, o2
" .{typ}{first_reg}: .cfa {o1} + ^ .{typ}{second_reg}: .cfa {o2} + ^"

Comment thread symbolic-cfi/src/lib.rs Outdated
Comment on lines +1346 to +1347
" .x{reg}: .cfa {} + ^ .lr: .cfa {} + ^",
o1, o2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
" .x{reg}: .cfa {} + ^ .lr: .cfa {} + ^",
o1, o2
" .x{reg}: .cfa {o1} + ^ .lr: .cfa {o2} + ^"

Comment thread symbolic-cfi/src/lib.rs Outdated
let second_reg = first_reg + 1;

self.last_reg_kind = typ;
self.last_reg_num = first_reg + 1;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.last_reg_num = first_reg + 1;
self.last_reg_num = second_reg;

Comment thread symbolic-cfi/src/lib.rs Outdated
Comment on lines +1456 to +1457
if matches!(code.code, Arm64UnwindCode::End)
|| matches!(code.code, Arm64UnwindCode::EndC)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if matches!(code.code, Arm64UnwindCode::End)
|| matches!(code.code, Arm64UnwindCode::EndC)
if matches!(code.code, Arm64UnwindCode::End | Arm64UnwindCode::EndC)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I did not know this form! So much nicer :)

Comment thread symbolic-cfi/src/lib.rs
function.function_length(),
);

for (instruction_num, code) in unwind_codes.iter().rev().enumerate() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the reason for the rev? Are codes just generally returned in reverse order?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

They're actually stored in reverse order; I think the idea is that, because codes are in 1:1 with the instruction prolog instructions, if you crash in the prolog, a native stalk-unwinder could jump to the correct unwind code based on the crashed PC, and then just walk forward, executing unwind codes, undoing all the work the prolog did, until it encounters the "end" code.

Comment thread symbolic-cfi/src/lib.rs Outdated
Comment on lines +1473 to +1483
Arm64UnwindCode::AllocSmall { size_bytes } => {
enc.alloc_stack(size_bytes)?;
}

Arm64UnwindCode::AllocMedium { size_bytes } => {
enc.alloc_stack(size_bytes)?;
}

Arm64UnwindCode::AllocLarge { size_bytes } => {
enc.alloc_stack(size_bytes)?;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could unify these arms:

Suggested change
Arm64UnwindCode::AllocSmall { size_bytes } => {
enc.alloc_stack(size_bytes)?;
}
Arm64UnwindCode::AllocMedium { size_bytes } => {
enc.alloc_stack(size_bytes)?;
}
Arm64UnwindCode::AllocLarge { size_bytes } => {
enc.alloc_stack(size_bytes)?;
}
Arm64UnwindCode::AllocSmall { size_bytes } | Arm64UnwindCode::AllocMedium { size_bytes } | Arm64UnwindCode::AllocLarge { size_bytes } => {
enc.alloc_stack(size_bytes)?;
}

Comment thread symbolic-cfi/src/lib.rs
first_reg,
offset_bytes,
} => {
// save pair <x(19+2*#X),lr> at [sp+#Z*8], offset <= 504
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to make sure (this applies to some of the other branches too): first_reg is already the number 19 + 2 *#X?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah, goblins did the work for us :)

Comment thread symbolic-cfi/src/lib.rs Outdated
Comment on lines +1578 to +1590
if pair {
if preindexed {
enc.save_pre_indexed_pair(typ, reg, offset_bytes)?;
} else {
enc.save_indexed_pair(typ, reg, offset_bytes)?;
}
} else {
if preindexed {
enc.save_pre_indexed(typ, reg, offset_bytes)?;
} else {
enc.save_indexed(typ, reg, offset_bytes)?;
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could match on (pair, preindexed) here instead.

Comment thread symbolic-cfi/src/lib.rs Outdated
self.writer,
" .{typ}{first_reg}: .cfa {} + ^ .{typ}{second_reg}: .cfa {} + ^",
o1, o2
" .{typ}{first_reg}: .cfa {o1} + ^ .{typ}{second_reg}: .cfa {o1} + ^"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is a typo here which causes tests to fail:

Suggested change
" .{typ}{first_reg}: .cfa {o1} + ^ .{typ}{second_reg}: .cfa {o1} + ^"
" .{typ}{first_reg}: .cfa {o1} + ^ .{typ}{second_reg}: .cfa {o2} + ^"

@klochek klochek force-pushed the christopherklochek/win_arm64_unwind branch from 49c6031 to 322dbaf Compare May 26, 2026 15:03
@klochek klochek marked this pull request as ready for review May 26, 2026 15:03
@klochek klochek requested a review from a team as a code owner May 26, 2026 15:03
Comment thread symbolic-cfi/src/lib.rs
Comment thread symbolic-cfi/src/lib.rs
Comment thread symbolic-cfi/src/lib.rs
Comment thread symbolic-cfi/src/lib.rs
Comment thread symbolic-cfi/src/lib.rs
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a6c067b. Configure here.

Comment thread symbolic-cfi/src/lib.rs Outdated
@klochek klochek requested a review from loewenheim May 26, 2026 16:47
Comment thread symbolic-cfi/src/lib.rs
Copy link
Copy Markdown
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

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

One nit in the changelog, other than that this LGTM :)

Comment thread CHANGELOG.md

**Features**

- Add support for win arm64 unwind codes ([#978](https://github.com/getsentry/symbolic/pull/978))
Copy link
Copy Markdown
Contributor

@loewenheim loewenheim May 27, 2026

Choose a reason for hiding this comment

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

Suggested change
- Add support for win arm64 unwind codes ([#978](https://github.com/getsentry/symbolic/pull/978))
- Add support for win arm64 unwind codes in PE files. ([#978](https://github.com/getsentry/symbolic/pull/978))

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.

2 participants