Closed koute closed 1 year ago
tarcieri
commented
1 year ago FWIW, in lieu of unsafe_target_feature in @RustCrypto project we've used #[inline(always)] in the context of things like trait impls where the code is one or two lines long, though that can slow compile times.
In this PR though, it seems like you're applying it to larger code blocks.
koute
commented
1 year ago FWIW, in lieu of
unsafe_target_featurein @RustCrypto project we've used#[inline(always)]in the context of things like trait impls where the code is one or two lines long, though that can slow compile times.In this PR though, it seems like you're applying it to larger code blocks.
The #[inline(always)] is only used for the thin wrapper functions though, e.g. this:
#[unsafe_target_feature("sse2")]
fn function() {
/* ... */
}gets turned into this:
#[inline(always)]
fn function() {
#[target_feature(enable = "sse2")]
unsafe fn _impl_function() {
/* ... */
}
unsafe { _impl_function() }
}So the inner function which actually contains the body of the function is not marked with #[inline(always)].
koute
commented
1 year ago I've also ran the benchmarks for this PR; here are the results for anyone interested.
The benches were ran on a Xeon Platinum 8481C 2.70GHz on Google Cloud.
tarcieri
commented
1 year ago @koute any chance you could push a commit that would show the difference of what this PR would look like without unsafe_target_feature?
I ask because I think we'd really like to keep 3rd party dependencies to a minimum. There's currently only two enabled by default: cfg-if (@rust-lang) and zeroize (@RustCrypto, i.e. me), with subtle being a @dalek-cryptography crate.
koute
commented
1 year ago @koute any chance you could push a commit that would show the difference of what this PR would look like without
unsafe_target_feature?
Done. Please take a look at the latest commit. I've only done it partially though, as doing it is really tedious and error-prone (it's easy to forget e.g. an #[inline(always)] annotation somewhere and completely crater the performance). Let me know if you want me to do this for the rest of the code too (which would need to be modified in a similar fashion).
tarcieri
commented
1 year ago @koute c67e430 looks good to me, thanks for attempting it!
@rozbb do you have an opinion on which way to proceed? I think it's worth removing the additional dependency.
pinkforest
commented
1 year ago Can I suggest to have the features as optional by way of them being negative ?
This way if using negative "disallow" features these can be explicitly ruled out and if one rules them out it applies wholesale
Also this would make them non-default so managing the featureset becomes easier when people use one-size-fits-all featureset
This would make sense also given most people should not need to be disallowing the detection.
fwiw - I'm beginning to think that disallow / forbid should be cfg flags as well giben it's niche e.g.
cfg(curve25519_dalek_forbid = "simd" | "simd_avx512" "simd_avx2")
Having niche configuration scattered between featuresets and cfg() would be confusing otherwise and having them via cfg would make it better to document and educate while leaving the top-level binary in control as intended.
Depending on which one is chosen I can send documentation PR after.
koute
commented
1 year ago So do we have a consensus regarding removing the unsafe_target_features dependency and doing everything by hand? (I'm happy to do it, but I don't really want to do all of this legwork and then end up having to revert it in the end.)
@pinkforest Usually cargo features should be only additive, but I guess in this case this doesn't really matter as those features don't actually change any functionality per-se, so sure, we could make those negative.
However, I'd really prefer to keep them as cargo features though; the issue with specifying anything through RUSTFLAGS is that it's really awkward to use, especially for bigger framework-like projects with hundreds/thousands of downstream users where telling everyone "hey, you now need to specify those RUSTFLAGS to compile your stuff because it depends on our stuff" is just not feasible. If we had to specify the flags we need through RUSTFLAGS we'd be most likely forced to fork the crate and change the configuration knob to be a cargo feature.
tarcieri
commented
1 year ago So do we have a consensus regarding removing the unsafe_target_features dependency and doing everything by hand?
@koute that would be my preference out of paranoia regarding adding third-party dependencies
@rozbb do you agree?
pinkforest
commented
1 year ago However, I'd really prefer to keep them as cargo features though [ .. ]
Yeah I hear you - we can have both via build.rs - similarly in build.rs for the cfg(curve25519_dalek_bits) acts as an override we can put feature-gate there that sets the cfg-flags in build.rs - which means both can be used. I've seen often feature-chains that get broken and then things like openssl might get stuck in the dependency tree 10 layers down :)
No need to change anything on this PR - I can send a PR straight after this to address this as negative feature that works both ways.
koute
commented
1 year ago No need to change anything on this PR - I can send a PR straight after this to address this as negative feature that works both ways.
I'm happy to change it if you want, but that works for me too. Thank you!
jrose-signal
commented
1 year ago Just to offer an alternate perspective: I think the usual justification for "no negative features" is that they're not composable: if client crate A enables "no_apples" and client crate B enables "no_bananas", a project might not be able to use A and B together because A depends on bananas and B on apples. But in this case it's "avx2_backend" and "avx512_backend" that aren't composable; you cannot have more than one backend enabled at once. If you want to phrase these positively, you could call them "pre_avx2_compatibility" and "pre_avx512_compatibility" or something; then at least --all-features does something meaningful, if probably not desirable.
(The cfg vs. feature debate is not unrelated; I agree with @koute that cfg flags are much less discoverable and more awkward to work with, and I agree with @tarcieri that some controls really are "one choice for the entire build" and cannot be represented composably.)
rozbb
commented
1 year ago Sorry all, this thread flew under the radar for me. Getting up to date now.
pinkforest
commented
1 year ago Yeah that's valid point re: --all-features most (?) would not need (I suspect?) these negative features in any case and that would veer towards using cfg() given it would be niche (?) - is there use-case where this would happen more often that people would really need to disable ?
Should the feature be around to disable detection and forcing backend. In any case I'll create a separate issue as we've visited this issue before and it needs to be re-visited properly.
koute
commented
1 year ago But in this case it's "avx2_backend" and "avx512_backend" that aren't composable
FYI, actually in this case they are composable. (: That's because they don't force a given backend, they just make it available for selection at runtime if the host on which the program's running supports it.
rozbb
commented
1 year ago @rozbb do you have an opinion on which way to proceed? I think it's worth removing the additional dependency.
Just reviewed everything. My thought is: removing unsafe_target_feature consistently would 1) add a lot of noise to the code and 2) make it easier to mess up in the future if we make changes or additions. As an intermediate solution, I think we could vendor the dep, or (my preference) just pin to a specific version and call it a day. Thoughts?
tarcieri
commented
1 year ago If we had an in-tree dependency like curve25519-dalek-derive, that'd be fine with me.
Note that it will make the custom derive stack at least a default dependency where it isn't right now (i.e. syn has long compile times, although it is commonly found in most projects)
pinkforest
commented
1 year ago Would be good reason to do the monorepo base here (now) for this + git combine (later) then
May I suggest landing this PR pinning to version first and then I can just send another PR to rename some files and add it in-tree as monorepo ?
rozbb
commented
1 year ago I like that!
koute
commented
1 year ago That sounds good to me! So I'll revert the last commit and I'll pin the version of the unsafe_target_features crate, and later we can just vendor it.
Or perhaps alternatively I could just transfer it to dalek-cryptography? Then it wouldn't be a third-party crate anymore. (:
My three cents regarding the syn dependency: I don't think it's that big of a deal, considering that most projects probably already have it in their dependency tree. I agree that we shouldn't add extra unnecessary dependencies, but all things considered I personally think the tradeoff here is worth it.
pinkforest
commented
1 year ago Yeah better just pin it for now - we were going to re-organise everything to monorepo anyways so this was a good reason to start it
koute
commented
1 year ago Sorry for the delay.
I have reverted the commit, pinned the version of the dependency, and fixed failing clippy. Should be ready to go!
koute
commented
1 year ago Hmm, if
fiatis the only backend we have, perhaps we should reconsider how gating works.
Indeed. Not sure if this is a good idea, and this would be a big change and require more refactoring, but maybe we could parametrize all of the public types by the backend? For example, instead of Scalar we could have Scalar<B>, and B would be a type through which the user could pick whatever backend they want, like e.g. Scalar<FiatU64> or Scalar<U32> (with an appropriate default). Then we wouldn't need any --cfg knobs. (Although the compile times would probably suffer.)
but I worry this might persist the problem where
curve25519-dalekis a transitive dependency with these features being enabled via intermediate dependencies
Hm, this is a fair point, but I think doing what @pinkforest suggested would probably fix this -- make the features be negative and non-default, so that would make it very unlikely that an intermediate crate using curve25519-dalek would enable them (most likely it'd either use the default features, or no default features and pick a subset).
Curious how this is all going to work with SIMD backends for non-x86 platforms
From what I can see it should work mostly fine, although it might still require some minor refactoring.
tarcieri
commented
1 year ago make the features be negative and non-default, so that would make it very unlikely that an intermediate crate using curve25519-dalek would enable them
FWIW we did this with @RustCrypto and I still consider it a mistake. We have/had force-soft feature(s) to disable all hardware optimizations, and intermediate dependencies would turn it on. All it takes is one misbehaving crate in your dependency tree to force a performance downgrade.
That was the major impetus for using cfg attributes: it removes the ability of intermediate dependencies to do this, and relegates all control to the toplevel binary.
daira
commented
1 year ago That was the major impetus for using cfg attributes: it removes the ability of intermediate dependencies to do this, and relegates all control to the toplevel binary.
Agreed. Using either positive or negative features for selection of crypto implementations is also a nightmare for auditing, because it greatly expands the number of crates in which a backdoor could potentially force use of an insecure implementation.
(Example: suppose that you don't trust RDRAND. Now try to audit that the getrandom crate is not relying on it, given that it might if the rdrand feature is enabled. Ugh.)
This PR makes the following changes:
-C target_feature) then that particular feature is assumed to be always available (courtesy of thecpufeaturescrate), and we won't try to detect whether it's available at runtime.x86_64-fortanix-unknown-sgx) then no runtime autodetection will take place (courtesy of thecpufeaturescrate)--cfg curve25519_dalek_backendconfiguration knob doesn't acceptsimdanymore; now this can only be used to switch to thefiatbackend.avx512ifmafeature is sufficient. This is incorrect. This backend actually uses two features:avx512ifmaandavx512vl, so I've made this explicit and corrected the README. In particular, if you've previously compiled with only-C target_feature=+avx512ifmawithout theavx512vlfeature the performance of the resulting code was utterly horrible since Rust disables inlining if the appropriate target feature is not enabled (but the code still accidentally worked since CPUs which supportavx512ifmausually also supportavx512vl)unsafe_target_featurecrate's description for why it's necessary.Since the code in
src/backend/vector/scalar_mulhad to be nested in an extramodthe diff is a little messy; I suggest reviewing commit-by-commit and just ignoring the commit whichrustfmts those files.