-
Notifications
You must be signed in to change notification settings - Fork 228
Fix missing extern "C"
on unsafe
intrinsics
#642
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
Conversation
This seems to fix the android problem that is blocking Rust's CB update, tested at rust-lang/rust#127561 (comment).
More notes at #641 |
ae00838
to
e7fd5be
Compare
extern "C"
on unsafe
intrinsics
Wow, that is interesting. I realized that #598 removed So I guess it was an ABI issue and system symbols were never being linked. Which I suppose means this PR is fine with only the first two commits. @Amanieu after this merges could you do a release so we can try rust-lang/rust#125016 again? |
Patches to add support for always-strong symbols if it comes up later Add `strong_if` to `intrinsics!`From 0bd840cfb822eb1db0bfc6f88b32fd39d8cb121b Mon Sep 17 00:00:00 2001
From: Trevor Gross <[email protected]>
Date: Thu, 18 Jul 2024 04:52:50 -0500
Subject: [PATCH] Create a `strong_if` attribute for the `intrinsics!` macro
This can be specified with a `cfg` predicate that disables weak linkage
for specific intrinsics.
---
src/macros.rs | 69 +++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 62 insertions(+), 7 deletions(-)
diff --git a/src/macros.rs b/src/macros.rs
index fb14660a..6885c70b 100644
--- a/src/macros.rs
+++ b/src/macros.rs
@@ -435,7 +435,56 @@ macro_rules! intrinsics {
intrinsics!($($rest)*);
);
- // This is the final catch-all rule. At this point we generate an
+ // This is the last rule for anything that has `strong_if`, which can be
+ // provided to opt out of the default weak linkage.
+ //
+ // This attribute takes a `cfg` predicate, e.g. `#![strong_if(target_arch = "mips")]`
+ (
+
+ #[strong_if($($strong:tt)*)]
+ $(#[$($attr:tt)*])*
+ pub $(unsafe $(@ $empty:tt)?)? extern $abi:tt fn $name:ident( $($argname:ident: $ty:ty),* ) $(-> $ret:ty)? {
+ $($body:tt)*
+ }
+
+ $($rest:tt)*
+ ) => (
+ intrinsics!(
+ @final
+ func:
+ $(#[$($attr)*])*
+ pub $(unsafe $($empty)?)? extern $abi fn $name( $($argname: $ty),* ) $(-> $ret)? {
+ $($body)*
+ }
+ strong_if: ( $($strong)* )
+ rest: $($rest)*
+ );
+ );
+
+ // This is the final catch-all rule, used by everything that does not have
+ // `strong_if` (above). The rule just converts matched syntax into the
+ // struct-like syntax used by `@final`.
+ (
+ $(#[$($attr:tt)*])*
+ pub $(unsafe $(@ $empty:tt)?)? extern $abi:tt fn $name:ident( $($argname:ident: $ty:ty),* ) $(-> $ret:ty)? {
+ $($body:tt)*
+ }
+
+ $($rest:tt)*
+ ) => (
+ intrinsics!(
+ @final
+ func:
+ $(#[$($attr)*])*
+ pub $(unsafe $($empty)?)? extern $abi fn $name( $($argname: $ty),* ) $(-> $ret)? {
+ $($body)*
+ }
+ strong_if:
+ rest: $($rest)*
+ );
+ );
+
+ // This is where everything gets instantiated. At this point we generate an
// intrinsic with a conditional `#[no_mangle]` directive to avoid
// interfering with duplicate symbols and whatnot during testing.
//
@@ -448,12 +497,15 @@ macro_rules! intrinsics {
// After the intrinsic is defined we just continue with the rest of the
// input we were given.
(
- $(#[$($attr:tt)*])*
- pub $(unsafe $(@ $empty:tt)?)? extern $abi:tt fn $name:ident( $($argname:ident: $ty:ty),* ) $(-> $ret:ty)? {
- $($body:tt)*
- }
+ @final
+ func:
+ $(#[$($attr:tt)*])*
+ pub $(unsafe $(@ $empty:tt)?)? extern $abi:tt fn $name:ident( $($argname:ident: $ty:ty),* ) $(-> $ret:ty)? {
+ $($body:tt)*
+ }
- $($rest:tt)*
+ strong_if: $( ( $($strong:tt)* ) )?
+ rest: $($rest:tt)*
) => (
$(#[$($attr)*])*
pub $(unsafe $($empty)?)? extern $abi fn $name( $($argname: $ty),* ) $(-> $ret)? {
@@ -464,7 +516,10 @@ macro_rules! intrinsics {
mod $name {
$(#[$($attr)*])*
#[no_mangle]
- #[cfg_attr(all(not(windows), not(target_vendor = "apple")), linkage = "weak")]
+ #[cfg_attr(
+ not(any(windows, target_vendor = "apple" $(, $($strong)* )?)),
+ linkage = "weak"
+ )]
$(unsafe $($empty)?)? extern $abi fn $name( $($argname: $ty),* ) $(-> $ret)? {
super::$name($($argname),*)
}
Make Android `__sync_*` intrinsics strongFrom c0cb6b6b8e7eba46573ce6a012f4c8bdf57bd953 Mon Sep 17 00:00:00 2001
From: Trevor Gross <[email protected]>
Date: Thu, 18 Jul 2024 04:56:00 -0500
Subject: [PATCH] Mark atomic `__sync_*` intrinsics always strong on android
After <https://github.com/rust-lang/compiler-builtins/pull/598>,
arm-android was failing to complete in CI because it was hanging on some
tests. This issue appears to have been caused by symbols related to
atomics, e.g. `__sync_val_compare_and_swap_4`, to have become weak.
It turns out that these symbols were always strong before, even though
Android was always setting the `weak-intrinsics` feature. So, making
them weak presumably caused the system implementation to get linked,
which appears buggy.
Resolve this by making `__sync_*` symbols weak on Android.
(this includes a recursion limit increase, our macros are getting big).
Link: https://github.com/rust-lang/compiler-builtins/issues/641
---
src/arm_linux.rs | 3 +++
src/lib.rs | 1 +
2 files changed, 4 insertions(+)
diff --git a/src/arm_linux.rs b/src/arm_linux.rs
index 8f22eb62..66dfe830 100644
--- a/src/arm_linux.rs
+++ b/src/arm_linux.rs
@@ -91,6 +91,7 @@ unsafe fn atomic_cmpxchg<T>(ptr: *mut T, oldval: u32, newval: u32) -> u32 {
macro_rules! atomic_rmw {
($name:ident, $ty:ty, $op:expr, $fetch:expr) => {
intrinsics! {
+ #[always_strong_if(target_os = "android")]
pub unsafe extern "C" fn $name(ptr: *mut $ty, val: $ty) -> $ty {
atomic_rmw(ptr, |x| $op(x as $ty, val) as u32, |old, new| $fetch(old, new)) as $ty
}
@@ -105,9 +106,11 @@ macro_rules! atomic_rmw {
atomic_rmw!($name, $ty, $op, |_, new| new);
};
}
+
macro_rules! atomic_cmpxchg {
($name:ident, $ty:ty) => {
intrinsics! {
+ #[always_strong_if(target_os = "android")]
pub unsafe extern "C" fn $name(ptr: *mut $ty, oldval: $ty, newval: $ty) -> $ty {
atomic_cmpxchg(ptr, oldval as u32, newval as u32) as $ty
}
diff --git a/src/lib.rs b/src/lib.rs
index 0d207a91..e14f5d5a 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -27,6 +27,7 @@
#![allow(clippy::manual_swap)]
// Support compiling on both stage0 and stage1 which may differ in supported stable features.
#![allow(stable_features)]
+#![recursion_limit = "256"] // We have some large macros
// We disable #[no_mangle] for tests so that we can verify the test results
// against the native compiler-rt implementations of the builtins.
|
`unsafe` functions were being matched in a different block that did not include `extern $abi`. This means that some intrinsics were getting generated with the Rust ABI rather than C. Combine the last two blocks using an optional token matcher, which fixes this problem and is cleaner.
e7fd5be
to
6c37129
Compare
#598 removed
extern "C"
from generatedunsafe
intrinsics, see the diff at https://github.com/rust-lang/compiler-builtins/pull/598/files#diff-315c02cd05738da173861537577d159833f70f79cfda8cd7cf1a0d7a28ace31bR457-R465. It seems this was causing issues building Android; see failures at rust-lang/rust#125016, and asm differences at #641.Update the macro so the ABI specifier gets added correctly again.
Fixes #641