Add feature to disable aesni target feature compile time checks

[cheme]

When using runtime is_x86_feature_detected macro to check if I could use aesni, I had to disable the checks.
Is it possible to include this little feature to allow usage of aesni without compile time checks?

[newpavlov]

Problem with such feature is that intrinsics will not get inlined in such setup, which will severely impact performance. I am not sure if #[target_feature(enable = "aes")] on your function will help here, as I think methods from aesni crate will be compiled first.

I guess this PR can be merge for now, but ideally we need the language support for such use-cases.

@gnzlbg
Do you have ideas what we can do here?

[gnzlbg]

The problem is that this crate requires aes to be enabled at compile-time. That obviously makes the crate unusable for people that want to use it if the CPU supports aes at run-time, but IIUC this is by design.

I don't really understand why it was designed this way.

A better design would be to have the crate just use #[target_feature(enable = "aes")] and provide an unsafe API.

This allows those who want to do run-time feature detection for aes to use this crate by just writing: if is_x86_feature_detected!("aes") { unsafe { aesni::foo(...) } } - note that this won't do run-time feature detection iff aes is enabled at compile-time.

This also allows those using compile-time feature detection to use the crate by just writing: #[cfg(target_feature = "aes")] { unsafe { aesni::foo(...) } }.

You can then provide safe APIs on top of this in this same crate or other crates. For example:

#[cfg(target_feature = "aes")] 
pub mod ct { // compile-time 
    pub fn foo() { unsafe { ::foo() /* always inlined */ } }
}
//^^^ or move this to another crate and use compile_error! for a nice error message

// you could also provide this, but that is probably a bad idea because of inlining
pub mod rt { // run-time
   pub fn foo() {
        if is_x86_feature_detected!("aes") { 
            unsafe { ::foo() /* never inlined */ }
        } else {
            // panic!();
            // fallback_foo();
            // Err(...) ...
        }
   }
}

Problem with such feature is that intrinsics will not get inlined in such setup,

If a user writes in their own crate:

fn my_alorithm() {
    if is_x86_feature_detected!("aes") {
        unsafe { foo() }
    } 
}

#[target_feature(enabled = "aes")] 
unsafe fn foo() {
    // calls to aesni functions
}

All aesni functions that have #[target_feature(enabled = "aes")] and #[inline] will be inlined into foo (unless inlining is not profitable). If you think a function should be inlined there, but it is not, then that's probably a compiler bug, but we haven't seen any of those for a long while.

OTOH, foo won't be inlined into my_algorithm, because that would introduce undefined behavior.

---

In any case, for libraries, compile-time feature detection is almost always a bad idea. Whether features are detected at compile-time or run-time is something that applications should control. If you make the decision in a library, then you cut half the world for which this might be the wrong decision / trade-off from using the library.

Arguably, having to make your APIs unsafe while using #[target_feature] is annoying, but that is currently how things are. The target_feature 1.1 RFC fixes this (rust-lang/rfcs#2396), but it hasn't been merged yet.

[newpavlov]

I don't really understand why it was designed this way.

The main reason for the current design is that block ciphers expose their functionality via BlockCipherTrait methods of which are safe. Another reason is that I think that the current unsafe approach to target features is a wrong solution to the problem. (albeit the easiest one to implement and stabilize, and compatible with potential future changes, so I understand why it was done like this) This is why I've wrote a Pre-RFC for target restriction contexts, but it looks like improving target features handling is not a priority for Rust team right now. Also I don't think that is_x86_feature_detected! should be used on block cipher level, ideally this check should be pushed as higher level as possible, but it does not play nice with the modular design of RustCrypto, because language does not provide any tools for having several crate versions with different features enabled.

I guess as a stop-gap solution I could expose trait-less feature-gated unsafe methods with #[target_feature(enabled = "aes")].

[gnzlbg]

I guess as a stop-gap solution I could expose trait-less feature-gated unsafe methods with #[target_feature(enabled = "aes")].

That is probably a good step forward. You can reuse these in the safe ones by just calling them via an unsafe { }.

Also I don't think that is_x86_feature_detected! should be used on block cipher level, ideally this check should be pushed as higher level as possible, but it contradicts to the modular design of RustCrypto.

There is a "pattern" in the wild which allows you to provide a safe API that performs run-time feature detection once, while letting your users control when that happens:

[derive(Clone, Copy, Debug)]
pub struct AesniBuilder(());

impl AesniBuilder {
    pub fn new() -> Option<AesniBuilder> {
        if is_x86_feature_detected!("aes") {
            Some(AesniBuilder(()))
        } else {
            None
        }
    }
    #[inline] 
    // maybe also:
    // #[target_feature(enable = "aes")]
    // unsafe
    fn foo() { unsafe { aesni::foo() } }
}

People are using it without #[target_feature] and without unsafe with success (that is, without inlining issues), but even if you have to make it #[target_feature]+unsafe for it to inline properly, you know that calling all these methods is safe if you have an AESNIBuilder.

Then, whether the user wants to construct a new builder every time, or put one in the whole program behind a static, that kind of is up to them.

[newpavlov]

Arguably, having to make your APIs unsafe while using #[target_feature] is annoying, but that is currently how things are. The target_feature 1.1 RFC fixes this (rust-lang/rfcs#2396), but it hasn't been merged yet.

It's not only about annoyance of unsafe. I think ideally language should support compiling several versions of a function with different sets of enabled target features. (I've heard gcc supports something similar?) This way crates will be able to use a simple compile-time feature detection without builders bloat, while users will be able to use run-time detection with several hierarchies if they want to. But drawback can be a binary bloat and increased compilation times (as you'll have to compile several versions of upstream dependencies), especially if done naively.

[gnzlbg]

I think ideally language should support compiling several versions of a function with different sets of enabled target features.

The language already supports this. If you just write plain old portable Rust, you can compile the same function for multiple target features.

However, you have specifically decided to 1) use an aes intrinsic directly and 2) not to provide an "instrinsic-less" pure Rust implementation, so that that feature cannot help you here.

[newpavlov]

My wording was a bit unclear, I've meant that if you'll use function foo from crate bar with enabled target features, then the whole crate will be compiled with those features, so compile time checks will not create problems for users who'll use run-time detection and #[target_feature(enable = "..")]. But I guess it will make compilation process significantly more complex, so we better aim for target restriction contexts system instead.

[gnzlbg]

My wording was a bit unclear, I've meant that if you'll use function foo from crate bar with enabled target features, then the whole crate will be compiled with those features, so compile time checks will not create problems for users who'll use run-time detection and #[target_feature(enable = "..")].

I still don't understand what you mean here.

if you'll use function foo from crate bar with enabled target features,

Enabled how exactly? If you enable them for your whole binary, it doesn't really matter.

Here the user doesn't have them enabled for their whole binary but an upstream crate requires that, and therefore, they can't use the crate.

[newpavlov]

Enabled how?

With #[target_feature(enable = "..")].

The system which I've tried to describe will be able to compile the following code:

#[target_feature(enable = "aes")]
unsafe fn aesni_encrypt(data: &mut [u8]) {
    // use aesni crate here
}

if is_x86_feature_detected!("aes") { 
    unsafe { aesni_encrypt(block) };
} else {
    // software AES
    soft_encrypt(block);
}

Because aesni used only in the context with enabled aes target feature, aesni compile-time checks will pass, as compiler will build only one version of aesni with the enabled target feature.

If somewhere in the code aesni crate will be used in the context with two enabled features, say aes and sse3, then two versions of aesni crate will be compiled for both sets of target features.

And if in addition to the previous two target feature contexts aesni will be used in a context without enabled aes target feature, the third version of the crate will be compiled, which will produce compilation error.

I guess it was just an ersatz of target restriction contexts idea.

[gnzlbg]

The system which I've tried to describe will be able to compile the following code:

But that code already compiles in Rust, just exactly as you wrote it :D

Because aesni used only in the context with enabled aes target feature, aesni compile-time checks will pass, as compiler will build only one version of aesni with the enabled target feature.

If somewhere in the code aesni crate will be used in the context with two enabled features, say aes and sse3, then two versions of aesni crate will be compiled for both sets of target features.

Wouldn't that break separate compilation, in that you would have to wait till you use a crate to know how to compile it (e.g. which target features to enable), but you can't use the crate yet, because you haven't compiled it?

Maybe you are looking for #[inline], which will inline the code from the dependency into the crate in which it is used? That will recompile the code with whatever target features are enabled at the call site, e.g.,

// crate A
#[inline] 
fn foo() {}

// crate B
#[target_feature(enable = "avx2")]
unsafe fn bar() { foo() } // foo is compiled with AVX2

[newpavlov]

But that code already works in Rust, just as you wrote it.

No, with the current version of aesni it will not work, because it checks if target features are enabled in the very beginning of the crate. This check allows me to be sure that necessary target features are enabled. Thus when you try to compile it, even if you use crate in contexts with enabled aes it will still produce compile time error. Manually disabling this check via feature as was done in this PR will allow to compile the snippet, but it will mark "unsafe" code (in the sense of the used intrinsics) as a completely safe one.

Also IIUC aes-ctr crate will not switch to AES-NI based implementation, even if you'll use Aes128Ctr in the context with enabled necessary target features.

Wouldn't that break separate compilation, in that you would have to wait till you use a crate to know how to compile it?

I guess, yes, it will. Technically it's possible to work around this issue (e.g. by compiling crate without features first and postponing any errors until crate will be used in feature-less context), but overall I agree it was not the best idea.

Maybe you are looking for #[inline], which will inline the code from the dependency into the crate in which it is used?

No, I am using target feature based cfgs at crate level, so #[inline] will not help here.

[gnzlbg]

No, with the current version of aesni it will not work, because it checks if target features are enabled in the very beginning of the crate.

I think I missed the // use aesni crate here part. That part makes sense now.

This check allows me to be sure that necessary target features are enabled.

While you could use cfg(target_feature) for that, it will also check that the whole binary is compiled with a target feature enabled, which appears it is not what you want. If you just want to force a single function to be compiled with some target features enabled, that's what #[target_feature] is for. If you want to force a whole crate to be compiled with some target features enabled, expose an API that only contains functions, and apply #[target_feature] to all of them (or use a proc macro to do that for you).

Technically it's possible to work around this issue (e.g. by compiling crate without features first and postponing any errors until crate will be used in feature-less context)

Features affect everything since you can do conditional compilation with cfg, so they can affect path resolution, macro expansion, type checking, ... For example, in the case of aesni, the crate doesn't compile without features, so trying to compile it "first" somehow, won't give you any information, and even if it would give you any information, that could completely change once the target features change because the whole crate can change.

So at least with the current system, I don't think this is technically possible. One would need probably many language features and technical work to come up with a system here that makes sense and works as the user expects.

[newpavlov]

If you just want to force a single function to be compiled with some target features enabled, that's what #[target_feature] is for.

But it's not a single functions, all functions and structs in the crate require enabled aes target feature (and ssse3 if you'll enable ctr feature) and want to provide safe interface (so target_feature v1.1 will not help here much), so it's essentially something like wanting working #![target_feature].

I guess for now we will have to use feature for disabling target feature checks. Personally I don't think that builder pattern proposed earlier is a good solution to the problem in this particular case.

@cheme
I'll merge this PR and after doc improvement with CI fix, I'll publish an update to crates.io.