Skip to content
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

add weak linkage to the ARM AEABI division functions #478

Merged
merged 3 commits into from
Jul 28, 2022

Conversation

Lokathor
Copy link
Contributor

Closes #477

I know in the issue Amanieu said that all functions in this crate should probably get weak linkage when possible, but that seems overkill for my own needs and i didn't want to have to determine when the target will use ELF or not for a lot of functions i'd never heard of.

@Lokathor
Copy link
Contributor Author

Note: It was mentioned in the issue that these functions are also incorrect on v4T. I'll resolve that as a separate PR so that we're just talking about one thing at a time.

@Lokathor
Copy link
Contributor Author

I didn't touch those memory functions, so the test error seem unrelated.

@Lokathor
Copy link
Contributor Author

@Amanieu ping

@Amanieu
Copy link
Member

Amanieu commented Jul 28, 2022

Sorry about the delay, I'm still looking into the powerpc issue. It seems like it might be an LLVM regression.

@Amanieu
Copy link
Member

Amanieu commented Jul 28, 2022

CI should be fixed now, could you rebase on the latest master?

@Amanieu
Copy link
Member

Amanieu commented Jul 28, 2022

Could you make the weak linkage conditional on !windows && !apple? Basically only on ELF targets.

@Lokathor
Copy link
Contributor Author

do you detect for apple like this?

    #[cfg_attr(all(not(windows), not(target_vendor="apple")), linkage = "weak")]

or something else?

@Amanieu
Copy link
Member

Amanieu commented Jul 28, 2022

Yes, that's correct.

on windows and apple (which don't use ELF) we can't apply weak linkage
@Lokathor
Copy link
Contributor Author

should be done i guess

@Lokathor
Copy link
Contributor Author

also, does this automatically get picked up in every nightly release once merged, or is there a separate PR required for that?

@Amanieu Amanieu merged commit bbf14f3 into rust-lang:master Jul 28, 2022
@Amanieu
Copy link
Member

Amanieu commented Jul 28, 2022

I need to publish a new version of the crate (I want to get #474 merged first), and then a PR is needed in rust-lang/rust to update the crate version in Cargo.lock.

@Lokathor Lokathor deleted the weak-linkage-for-division branch July 28, 2022 16:46
@Amanieu
Copy link
Member

Amanieu commented Jul 28, 2022

I've published version 0.1.77.

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.

Please make aeabi division functions use weak linkage, because they are incorrect for ARMv4T
2 participants