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

"paca" and "pacg" features are tied in rustc, but detection isn't tied #1352

Closed
calebzulawski opened this issue Nov 11, 2022 · 14 comments
Closed

Comments

@calebzulawski
Copy link
Member

In rustc, the "paca" and "pacg" features are tied together: https://github.com/rust-lang/rust/blob/a876a4df32b402e3886cd9f2af02cff3dd8e21c8/compiler/rustc_codegen_ssa/src/target_features.rs#L147-L149
Enabling one always enables the other, so I think detection should check both as well.

I don't know if there are any CPUs that have one and not the other, but this could potentially be unsound:

#[target_feature(enable = "paca")] // enables both "paca" and "pacg"
unsafe fn foo() {}

if is_aarch64_feature_detected!("paca") {
    // this would perhaps fail:
    // assert!(is_aarch64_feature_detected!("pacg"))
    unsafe { foo() }
}
@bjorn3
Copy link
Member

bjorn3 commented Nov 11, 2022

They are enabled by different registers: https://github.com/torvalds/linux/blob/4bbf3422df78029f03161640dcb1e9d1ed64d1ea/arch/arm64/kernel/cpufeature.c#L2704-L2735 I don't know if there exist cpu's that support one but not the other.

@calebzulawski
Copy link
Member Author

Yeah, they are definitely distinct features and I think it's likely "more wrong" to tie them together, but this limitation seems to come from LLVM which ties them together as a single "pauth" feature.

The way I look at it, detection should match codegen, because there's no way of knowing something like this happens without reading rustc's source.

@Amanieu
Copy link
Member

Amanieu commented Nov 12, 2022

IIRC the split was introduced because some OS only provide one of the 2 features. At the CPU level this is a single feature but availability depends on the OS exposing the necessary functionality.

@calebzulawski
Copy link
Member Author

In that case, I wonder if paca should not enable pacg and vice versa, but could still enable the same pauth LLVM feature? I think the enabled features should match detected features, just not sure which to change.

@Amanieu
Copy link
Member

Amanieu commented Nov 18, 2022

Well it's a bit tricky: we can't safely enable the LLVM feature without OS support. But perhaps something like a JIT compiler could use the feature detection to decide whether it wants to make use of one of the two sub-features.

@calebzulawski
Copy link
Member Author

I guess it's a question of should is_*_feature_detected detect OS features or Rust features?

@Amanieu
Copy link
Member

Amanieu commented Nov 18, 2022

See rust-lang/rust#86941 for the background on the split.

cc @adamgemmell

@calebzulawski
Copy link
Member Author

Your earlier comment here hints at one of the ideas I had, enabling the entirety of pauth with paca, though that sounds risky: rust-lang/rust#86941 (comment)

The other idea of only allowing enabling the features together seems... reasonable? This would still allow separate detection and still imply that you need to detect both paca and pacg before using either feature (as opposed to silently enabling the other as rustc does now)

@Amanieu
Copy link
Member

Amanieu commented Nov 18, 2022

I agree with your other idea, that would be the best way to handle this. Perhaps in the future LLVM will also split this into two separate features like they did for the crypto feature.

@adamgemmell
Copy link
Contributor

Splitting the two features was a precautionary decision mainly to ensure we'd keep stability if LLVM split pauth into two. It's not particularly user-friendly at the moment, I agree, but functionally I still think it's the right thing to do.

Enabling one always enables the other, so I think detection should check both as well.

#[target_feature(enable = "paca")] // enables both "paca" and "pacg"

It shouldn't and your example doesn't compile when I tried just now. What should happen is the other idea - it should error saying that pacg should be enabled.

I guess it's a question of should is_*_feature_detected detect OS features or Rust features?

We made a point of having the same feature set for target_feature and feature detection as the two are often used together. I think a stable set of features handled by Rust, portable across OSes is quite valuable. There's also the possibility of backends other than LLVM to consider too.

Given that, the intention was that if both must be explicitly enabled together then it's a programmer error to only check for one. Perhaps a new pauth feature which enables/checks for both paca and pacg might help or maybe the duplication only makes the situation more confusing.

@calebzulawski
Copy link
Member Author

calebzulawski commented Nov 18, 2022

Hmm you're right... which is good news. I think there are a few things that don't work quite right and I jumped to conclusions

@adamgemmell
Copy link
Contributor

* if the function is never used, it is accepted? I admittedly only wrote the function in godbolt and added the hypothetical `if` just for this issue confused https://rust.godbolt.org/z/b6bjzeKz3

This could be due to issues with how late the feature validation happens - it happens right before we hand over to LLVM. Could the function be optimised out in debug builds before getting to rustc_codegen_llvm? The validation should probably occur earlier given the move towards a unified Rust feature set across all backends.

* passing `-Ctarget-feature=+paca` silently enables `pacg`: https://rust.godbolt.org/z/bKdTe4rGb

+paca will get converted to pauth, but we have tests for erroring when only one of the two is present so this won't actually ever compile: https://github.com/rust-lang/rust/blob/master/src/test/ui/target-feature/tied-features-cli.rs (side note, I don't think the fourth pass case actually runs here now)

It looks like a bug with the cfg directive. Maybe both the command-line and cfg(target_feature...) attribute features are passed through to_llvm_features() (converting both to pauth) before the comparison, but I can't find where this checking occurs in the limited time I have now.

@calebzulawski
Copy link
Member Author

calebzulawski commented Dec 1, 2022

A minor victory, but now both paca and pacg appear in rustc --print target-features: rust-lang/rust#104627

@calebzulawski
Copy link
Member Author

I added rust-lang/rust#105110 and rust-lang/rust#105111 to address those issues. I think detection is correct so I'm closing this.

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

No branches or pull requests

4 participants