compiler-builtins icon indicating copy to clipboard operation
compiler-builtins copied to clipboard

Add `__chkstk` for `arm64ec-pc-windows-msvc`

Open codeOverFlow opened this issue 9 months ago • 4 comments

This is my first PR so I tried one that did not looked overly complicated.

This is base on llvm/llvm-project@946a81f

As this is my first PR fill free to fire all mistake you can spot :)

I am also not sure for the #[cfg] gate I put.

codeOverFlow avatar Apr 07 '25 22:04 codeOverFlow

I’ll take a look at the implementation soon, do you have a repo that gets linker errors without this function?

tgross35 avatar Apr 07 '25 23:04 tgross35

  • This will currently always get configured out currently as the arm module is cfg(target_arch = "arm") (which is 32-bit ARM): https://github.com/rust-lang/compiler-builtins/blob/1901c415975eae741b7b072e789e74c6e422c22a/compiler-builtins/src/lib.rs#L50-L51 whereas the new assembly is only enabled when cfg(target_arch = "arm64ec").
  • The assembly from https://github.com/llvm/llvm-project/commit/946a81f is for 32-bit ARM, whereas arm64ec is a 64-bit platform (LLVM's 64-bit ARM assembly can be found here). compiler-builtins already has the 64-bit ARM version of the assembly in the aarch64 module, although it is currently cfg(target_os = "uefi").
  • According to Understanding Arm64EC ABI and assembly code, the __chkstk function for Arm64EC code is called __chkstk_arm64ec.

beetrees avatar Apr 08 '25 08:04 beetrees

I did not know what to put :/ maybe I should just put a "cfg (windows)" as LLVM says it is windows only ?

I just tried to see which arm target is for Windows :/ and just found this one

Le mar. 8 avr. 2025 à 10:49, beetrees @.***> a écrit :

— Reply to this email directly, view it on GitHub https://github.com/rust-lang/compiler-builtins/pull/812#issuecomment-2785705991, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGJPV632FRDEU7IT6WA3CT2YOETFAVCNFSM6AAAAAB2UWP2J6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOBVG4YDKOJZGE . You are receiving this because you authored the thread.Message ID: @.***> [image: beetrees]beetrees left a comment (rust-lang/compiler-builtins#812) https://github.com/rust-lang/compiler-builtins/pull/812#issuecomment-2785705991

— Reply to this email directly, view it on GitHub https://github.com/rust-lang/compiler-builtins/pull/812#issuecomment-2785705991, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGJPV632FRDEU7IT6WA3CT2YOETFAVCNFSM6AAAAAB2UWP2J6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOBVG4YDKOJZGE . You are receiving this because you authored the thread.Message ID: @.***>

codeOverFlow avatar Apr 08 '25 09:04 codeOverFlow

Could you clarify whether this is attempting to solve a problem you have experienced (e.g. linker error) or just adding an implementation that is missing?

We don't really want to add these implementations if Rust never emits them, but it's possible that is happening in circumstances we aren't yet testing. thumbv7a-pc-windows-msvc and thumbv7a-uwp-windows-msvc are the only Windows targets for 32-bit Arm and arm64ec-pc-windows-msvc is the only arm64ec target; I'm not sure that either of these need __chkstk or __chkstk_arm64ec, but maybe they do for #![no_std] binaries? Cc @bdbaia and @dpaoliello (listed target maintainers) in case you have any ideas.

If it's possible to create a linker error looking for these symbols then we can add them, but otherwise we could skip it and just update README to mention these in the "Unimplemented functions" section.

tgross35 avatar Apr 08 '25 19:04 tgross35