Intrinsics get compiled even if appropriate target_features are not enabled

[newpavlov]

If we'll take aesni crate and will remove the following lines from lib.rs:

#![cfg(any(target_arch = "x86_64", target_arch = "x86"))]
#![cfg(target_feature = "aes")]

Crate will compile even without RUSTFLAGS="-C target_feature=+aes". (although resulting code will be 4 times slower) Is it intended behavior?

[alexcrichton]

This is currently intended, to allow compiling binaries that use features that aren't available at runtime (aka turns out to be dead code at runtime). It's true though that consumers who which for desired performance will need to enable the correct features as well.

In any case though I think this is working as intended, so closing.

[gnzlbg]

@alexcrichton are you sure that this is intended? the AES intrinsics have #[target_feature(enable = "aes")], that is, they should compile assuming AES is enabled. If the user calls them on a host that does not has the feature enabled, then that's undefined behavior, but that cannot affect code generation.

[gnzlbg]

A comment by @newpavlov in the inline assembly tracking issue seems to expand on this issue, but IMO the issue as filled its pretty much ambiguous: @newpavlov where is the problem? Are the intrinsics from coresimd generating bad code? Or are you writing an algorithm using intrinsics that require AES without marking it with #[target_feature(enable = "aes")], and that algorithm is generating bad code?

If its the first case, then its a bug here. If it is the second, then that code is probably invoking undefined behavior and you are lucky that it works at all.

[gnzlbg]

This function looks like undefined behavior to me: https://github.com/RustCrypto/block-ciphers/blob/c3619695bac12e03f61a5eafd08d900d4575bfb6/aesni/src/aes128/expand.rs#L28 (if you remove the global #![cfg(target_feature = "aes")]).

[newpavlov]

I was under the wrong impression about how stdsimd supposed to work which was based on how LLVM intrinsics operate. (i.e. compilation fails if intrinsic feature is not enabled) But for runtime detection and switching it's indeed required to have ability to enable feature locally for intrinsic through #[target_feature(enable = "...")]. But I feel that the lack of compile time checks for such mistakes can be a noticeable source of footguns in future. I don't have any concrete proposals right now, but I think there was some relevant suggestions in the target_feature RFC discussion.

[gnzlbg]

But I feel that the lack of compile time checks for such mistakes can be a noticeable source of footguns in future.

The only mistake I see in that library is opening unsafe blocks to call unsafe functions without knowing which pre-conditions must be upholded for those unsafe functions to not cause undefined behavior. Or what do you mean by footguns?

[newpavlov]

I mean for aesni it's enough to use #![cfg(..)] on target_arch and target_feature, but in more complex codebases it can be quite easy to slip some intrinsics usage without appropriate checks. Yes, that code will use unsafe blocks, so no wonders you can get UB, but ideally I would like to have compiler to check if intrinsics are called only if feature is accessible, i.e. code with intrinsics only called after appropriate compile-time or runtime guards. To make situation worse such mistakes have high probability of staying undetected, as everything will work fine on developer CPU, but for some users with old or rare CPU program will just crash.

[gnzlbg]

The problem is that the compiler cannot know on which CPUs you might intend to run your binaries. If you are always going to run the binaries on some sets of CPUs you might not need to do (and pay) for the checks. There is an RFC about allowing calling the intrinsics without unsafe blocks when the compiler can infer that doing so is safe, but it still allows opening an unsafe block to call them in other situations :/ At least, we haven't found a way to do this.

mean for aesni it's enough to use #![cfg(..)] on target_arch and target_feature,

You don't need to do that. You can use #[target_feature] and make calling the functions unsafe. And then have some functions that are not unsafe but use cfg_feature_enabled! to do run-time detection, and for example panic if the CPU does not have the AES feature. Or in other words, there are many alternatives to what a library might want to do here. Not exposing the functions is only one of the alternatives.

[newpavlov]

The problem is that the compiler cannot know on which CPUs you might intend to run your binaries. If you are always going to run the binaries on some sets of CPUs you might not need to do (and pay) for the checks.

In that case your can just use #![cfg(..)] to specify on which CPUs you intend to use the code.

I think I've seen somewhere proposal for introducing something like this (not precise, just a coarse strawman draft):

#[target_features(feature1)]
fn foo1() {
    // safe to use intrinsics dependent on "feature1"
}

#[target_features(feature2)]
fn foo2() {
    // safe to use intrinsics dependent on "feature2"
}

fn foo_fallback() {}

// will not compile unless `feature1` is enabled for the whole crate
// through `#![cfg(..)]` or something similar
fn func1() {
    foo1()
}

fn func2() {
    // this macro allows to call feature-gated functions using runtime detection
    runtime_match_feature!{
        "feature1" => foo1(),
        "feature2" => foo2(),
        _ => foo_fallback(),
    }
}

// macro will refuse to compile without crate-wise enabled `any(feature1, feature2)` or
// by marking this function `#[target_features(any(feature1, feature2))]`
fn func3() {
    runtime_match_feature!{
        "feature1" => foo1(),
        "feature2" => foo2(),
    }
}

// behaves same as the `runtime_match_feature`, but performs switching at compile time
// depending on the enabled features.
fn func4() {
    compile_match_feature!{
        "feature1" => foo1(),
        "feature2" => foo2(),
        _ => foo_fallback(),
    }
}

Here compiler build sort-of tree of feature restrictions and runtime_match_feature allows to branch this tree either at runtime, or at compile time.

You don't need to do that. You can use #[target_feature] and make calling the functions unsafe. And then have some functions that are not unsafe but use cfg_feature_enabled! to do run-time detection, and for example panic if the CPU does not have the AES feature.

Personally I strongly dislike this approach, as it introduces runtime errors for cases which can be detected at compile time, and strong compile time checks is one of the main selling points of Rust for me.

[gnzlbg]

Something like that is what rust-lang/rfcs#2212 proposes. But

Personally I strongly dislike this approach, as it introduces runtime errors for cases which can be detected at compile time, and strong compile time checks is one of the main selling points of Rust for me.

You can also use #[cfg(...)] + compile_error! to provide an error of your choosing (just like cfg_feature_enabled does). In any case, at least until RFC2212 is merged and implemented, how to guarantee a safe interface for your intrinsics is up to you. You have many tools available to chose how to do this.

[newpavlov]

Thank you for the link! Yes, I know about compile_error!, but I am not using it right now because of the rust-lang/rust#46189.

[alexcrichton]

@alexcrichton are you sure that this is intended? the AES intrinsics have #[target_feature(enable = "aes")], that is, they should compile assuming AES is enabled. If the user calls them on a host that does not has the feature enabled, then that's undefined behavior, but that cannot affect code generation.

Yeah the codegen differences happen because LLVM can't inline across functions which enable different sets of target featues. If the AES feature isn't enabled then no calls to the AES intrinsics can get inlined, which likely destroys performance.

It's also technically not undefined behavior I think if you run it on the right CPU, so I think LLVM can't do too much as it has to ensure the program works in that case at least.