Skip to content

Use correct CFA register on ARM64#13595

Merged
kayceesrk merged 1 commit into
ocaml:trunkfrom
NickBarnes:nick-macos-unwind
Dec 2, 2024
Merged

Use correct CFA register on ARM64#13595
kayceesrk merged 1 commit into
ocaml:trunkfrom
NickBarnes:nick-macos-unwind

Conversation

@NickBarnes

@NickBarnes NickBarnes commented Nov 7, 2024

Copy link
Copy Markdown
Contributor

I noticed that testsuite/tests/unwind/driver.ml was failing on my Mac. @tmcgilchrist identified and fixed the problem (wrong CFA register), which was related to my use of an old version of XCode, not exercised by the CI. This solution is his but with a tweak from me for the check-linker-version.sh script.

@NickBarnes NickBarnes changed the title Use correct CFA register on MacOS Use correct CFA register on ARM64; fix MacOS CI Nov 7, 2024
@NickBarnes

Copy link
Copy Markdown
Contributor Author

Updated to remove a CI workflow change which was breaking the MacOS CI. https://github.com/ocaml/ocaml/actions/runs/11720868392/job/32647116985

@NickBarnes NickBarnes changed the title Use correct CFA register on ARM64; fix MacOS CI Use correct CFA register on ARM64 Nov 7, 2024
Comment thread Changes Outdated

@tmcgilchrist tmcgilchrist left a comment

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.

This works well for Xcode versions 14 through to 16 on both ARM64 and x86_64.
Originally this bug (which I wrote) was missed because the test suite was incorrectly skipping this test.

@ghost

ghost commented Nov 13, 2024

Copy link
Copy Markdown

FWIW, asmcomp/riscv/emit.mlp has a similar bug, where the cfi_def_cfa_register stanza on line 342 refers to register 21 which is the correct number for s0 in int_reg_name but is the wrong DWARF number for it, which is 8.

Edit: per @tmcgilchrist's proposal, submitted as separate PR: #13607.

@ghost ghost mentioned this pull request Nov 13, 2024

@dra27 dra27 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit in the bash script - I haven't actually reviewed

Comment thread testsuite/tests/unwind/check-linker-version.sh Outdated
Comment thread Changes Outdated

@gasche gasche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I morally approve of this change (even though I don't know anything about 29 and 19 as magic numbers for ARM64). I would suggest that you credit one of yourselves (Tim or Nick) as reviewers, and then we merge.

@tmcgilchrist

Copy link
Copy Markdown
Contributor

@gasche I introduced the error in #13500. x29 is the frame pointer register on ARM64. Copying my comment from that PR explains why x19 matters.

22fee2d Stores sp in temporary register x19 during Iextcall in the same way as amd64 backend. Previously this was implemented in #13079 as push/pop on the stack, however this interferes with frame pointers.

My analysis is this test was failing because the logic for parsing the ld -v response was incorrect and it wasn't noticed when I submitted the Frame Pointer work.

@NickBarnes NickBarnes force-pushed the nick-macos-unwind branch 2 times, most recently from 5879a22 to c359c12 Compare November 18, 2024 16:47
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com>
@kayceesrk kayceesrk merged commit 1ad1804 into ocaml:trunk Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants