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 atomic support for pre-ARMv6 on Linux #115

Merged
merged 1 commit into from
Oct 7, 2017

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Oct 31, 2016

This uses the kernel user helpers which are available starting from kernel 2.6.15. Since Rust currently requires 2.6.18 at a minimum, this should be fine in practice. I did not include support for 64-bit atomics since that functionality is only available in kernel 3.1.

This PR allows Rust to work on older ARM versions such as ARMv4 and ARMv5 with the full libstd.

@alexcrichton
Copy link
Member

Perhaps this could be behind an off-by-default feature? For inclusion into rustc at least we'll want to turn off these intrinsics for now.

@Amanieu
Copy link
Member Author

Amanieu commented Nov 1, 2016

I don't see why these intrinsics should be hidden behind a feature. They are never called by rustc on any of the currently available ARM targets.

@alexcrichton
Copy link
Member

The compiler likes to have very strict control over the symbols it exposes and provides, so if we don't need these symbols we wouldn't want them exported. Because they're not called is just motivation for not having them in the first place as part of the compiler build (e.g. an off-by-default feature)

@Amanieu
Copy link
Member Author

Amanieu commented Nov 1, 2016

Actually compiler-rt already provides an implementation of these symbols, however that implementation only works on ARMv6. But if you insist, I will put these behind a feature flag.

@Amanieu
Copy link
Member Author

Amanieu commented Nov 1, 2016

I moved the code behind the arm_linux_atomic feature.

@atilag
Copy link

atilag commented Nov 8, 2016

Hi team! Are there any further steps left before landing this patch?
Thanks!

@nelsonjchen
Copy link

This pull request was brought up here. The OP of this reddit had to revert to 1.13 to get things working on ARMv5TE since newer std requires atomic instructions or some similar service. Would this patch allow compiling for ARMv5TE with the current std?

@mattico
Copy link
Contributor

mattico commented Apr 11, 2017

@nelsonjchen
rustc still uses compiler-rt (just renamed to compiler-builtins) from LLVM. If you built rustc using this compiler-builtins instead, then I think it would work.

@Amanieu
Copy link
Member Author

Amanieu commented Apr 12, 2017

@mattico Actually, the compiler-rt implementations of these intrinsics in compiler-rt uses ARMv6 instructions and won't work on ARMv4/ARMv5. The libgcc version does support ARMv4/ARMv5 though.

@bors
Copy link
Contributor

bors commented Sep 16, 2017

☔ The latest upstream changes (presumably #194) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

@Amanieu I think with discussion on rust-lang/rust#44978 as of recently we're leaning towards merging this now? Would you be willing to rebase this on master?

@Amanieu
Copy link
Member Author

Amanieu commented Oct 4, 2017

Rebased. Do we still want this behind the "arm_linux_atomic" feature? I think these should always be available (the symbols are already exported by libgcc/libcompiler-rt, so we aren't adding any new symbols).

@alexcrichton
Copy link
Member

Thanks! With the discussion on the linked PR I think we'd unconditionally provide these symbols on armv5 Linux, but nowhere else.

@Amanieu
Copy link
Member Author

Amanieu commented Oct 4, 2017

Do I need to do anything in the PR or can I just leave it as a feature?

@alexcrichton
Copy link
Member

Yes let's have compilation or not be automatic, detected based on the target (no features required)

@Amanieu
Copy link
Member Author

Amanieu commented Oct 4, 2017

Do we have any way to differentiate the different ARM targets? AFAIK they all return the same thing for target_arch/target_os/target_env.

@alexcrichton
Copy link
Member

We've got a build script!

@Amanieu
Copy link
Member Author

Amanieu commented Oct 5, 2017

Done.

@alexcrichton
Copy link
Member

Looks great! I'll hold off on merging until rust-lang/rust#44978 is decided on, but I think we're very likely to merging that. Thanks for the updates here!

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 6, 2017

📌 Commit 73e38dc has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 6, 2017

⌛ Testing commit 73e38dc with merge 5b96bef...

bors added a commit that referenced this pull request Oct 6, 2017
Add atomic support for pre-ARMv6 on Linux

This uses the [kernel user helpers](https://www.kernel.org/doc/Documentation/arm/kernel_user_helpers.txt) which are available starting from kernel 2.6.15. Since Rust currently requires 2.6.18 at a minimum, this should be fine in practice. I did not include support for 64-bit atomics since that functionality is only available in kernel 3.1.

This PR allows Rust to work on older ARM versions such as ARMv4 and ARMv5 with the full libstd.
@bors
Copy link
Contributor

bors commented Oct 7, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 5b96bef to master...

unsafe fn __kuser_cmpxchg(oldval: u32, newval: u32, ptr: *mut u32) -> bool {
let out: u32;
// FIXME: we can't use BLX on ARMv4
asm!("blx ${0}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, couldn't 0xffff0fc0u32 just be transmuted to a function pointer and then called instead of using inline assembly, then the compiler will take care of what branch instruction is used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using inline asm is actually more efficient in this case since we can precisely specify which registers are clobbered by the call. This allows the compiler to avoid unnecessary spills. The set of clobbered registers is guaranteed by the kernel ABI.

Now that you mention it though, for __kuser_cmpxchg the clobber list is pretty much identical to that of a normal function, so there probably isn't much benefit there...

@glaubitz
Copy link

glaubitz commented Mar 3, 2018

Just as a heads-up: The kernel helpers for older ARM targets conflict with the helpers provided by newer gcc versions, see: rust-lang/rust#48625

So we need to make the use of the helpers more fine-grained.

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.

8 participants