aes: feature flags are not additive

[SergioBenitez]

The aes crate has two feature flags, both of which remove functionality:

block-ciphers/aes/Cargo.toml

Lines 29 to 31 in f68ad0d

	[features]
	compact = [] # Reduce code size at the cost of slower performance
	force-soft = [] # Disable support for AES hardware intrinsics

This is an incorrect use of Cargo feature flags. From my comments at rwf2/cookie-rs#176 (comment), which focus on force-soft:

This seems like an ill-fitted, perhaps incorrect use of features. Features should only be used to add capabilities, but force-soft removes the ability to make use of AES-NI. The consequence is that a consumer of aes has little control over whether AES-NI is actually used: all it takes is a single dependency, direct or indirect, enabling force-soft to disable AES-NI completely, without recourse.

I would suggest what I would consider to be the "correct" approach: always enable AES-NI if it is known to statically exist, and otherwise, add a feature that adds the ability to dynamically detect its existence, which of course becomes a no-op if the first condition is met.

Somewhere in the codebase, you already have 2 versions of the code. Let's call them aes::ni and aes::soft.

Effectively, we want this:

fn aes() {
    #[cfg(have_aesni)]
    return aes::ni();

    #[cfg(feature(autodetect)]
    if detect(aes-ni) { return aes::ni() }

    aes::soft()
}

This structure makes it clear that autodetect is additive; it adds the ability to dynamically detect AES-NI and removes nothing otherwise; there is no not(feature(autodetect)). This is the only fully correct use of features. If the code that requires 1.49 is restricted to detect, then MSRV can remain at 1.41 as long as autodetect is not enabled.

The same reasoning applies to the compact feature. If it must exist at all, a non-feature --cfg aes_compact is the correct approach. This restores control of expected performance to the top-level binary as opposed to any crate in the transitive closure of dependencies.

See also the full comment thread in rwf2/cookie-rs#176.

[tarcieri]

This would be a breaking change, so if we were to try to implement something like this, it would be in aes v0.8 at the earliest.

I'll let @newpavlov speak to the rationale for force-soft as I switched my original implementation in #208 to that from an autodetect feature at his request.

Regarding compact, that was added in #195 as a semi_fixslice feature which I later renamed to compact. I think it's valuable for embedded users, but also tricky to expose.

[tarcieri]

Another dimension to this problem would be potentially adding support for compact round keys in addition to semi-fixslicing: #191

Should that be gated under something like compact? Separate?

[tarcieri]

Regarding force-soft, I'm wondering if perhaps instead an intrinsics feature might be a better way to go.

This could have the following behavior:

• without intrinsics: soft backend
• with intrinsics, without target_feature(s): autodetect CPU features
• with intrinsics, with target_feature: intrinsics only
• without intrinsics, with target_feature: still soft backend

[newpavlov]

This seems like an ill-fitted, perhaps incorrect use of features. Features should only be used to add capabilities, but force-soft removes the ability to make use of AES-NI.

I do not support this interpretation. In my opinion features MUST be additive only in the sense of public API and MUST NOT cause compilation errors if several features are enabled simultaneously. Meanwhile adding the under the hood functionality as in our case at best is SHOULD (yeah, I see that the quote uses "should", but I am not sure if it was done with the RFC in mind).

As mentioned in the IRLO thread, unfortunately, Cargo does not have proper tools to handle such use-cases (i.e. mutually exclusive features and features which must be toggled only by an end app, not libraries), so we have to work with what we got. In other words, it boils down to matter of defaults. Most users want for AES-NI intrinsics to be used if CPU supports them, after all force-soft use cases are quite rare.

Yes, we could state that intrinsics features should not be enabled by library crates outside dev builds, but in my experience this approach not only makes life significantly harder for crate users (who have to bother configuration and may have to discover that for best performance they have to enable a feature of a crate buried deep in their dependency tree). Also with high probability there will be crates which enable such feature unconditionally, "poisoning" all projects dependent on it and thus making it virtually impossible switching to the software implementation for any sizable enough project. Same goes for making it a default feature, most users will not bother with default-features = false.

To summarize: I think the current approach is the most practical one at the moment and should not be changed until Cargo will improve support of such use cases.

[SergioBenitez]

This seems like an ill-fitted, perhaps incorrect use of features. Features should only be used to add capabilities, but force-soft removes the ability to make use of AES-NI.

I do not support this interpretation. In my opinion features MUST be additive only in the sense of public API and MUST NOT cause compilation errors if several features are enabled simultaneously. Meanwhile adding the under the hood functionality as in our case at best is SHOULD (yeah, I see that the quote uses "should", but I am not sure if it was done with the RFC in mind).

As mentioned in the IRLO thread, unfortunately, Cargo does not have proper tools to handle such use-cases (i.e. mutually exclusive features and features which must be toggled only by an end app, not libraries), so we have to work with what we got. In other words, it boils down to matter of defaults. Most users want for AES-NI intrinsics to be used if CPU supports them, after all force-soft use cases are quite rare.

Cargo does have such a feature via non-feature cfgs. Seeing as these functionality changes should only be reasonably enabled by end-applications, non-feature cfgs seem like a perfect fit.

Yes, we could state that intrinsics features should not be enabled by library crates outside dev builds, but in my experience this approach not only makes life significantly harder for crate users (who have to bother configuration and may have to discover that for best performance they have to enable a feature of a crate buried deep in their dependency tree). Also with high probability there will be crates which enable such feature unconditionally, "poisoning" all projects dependent on it and thus making it virtually impossible switching to the software implementation for any sizable enough project. Same goes for making it a default feature, most users will not bother with default-features = false.

To summarize: I think the current approach is the most practical one at the moment and should not be changed until Cargo will improve support of such use cases.

I'm a bit confused. The primary feature in question is force-soft which disables the use of AES-NI intrinsics, but this block of text seems to be arguing as if the reality was the inverse. There is no intrinsics feature; this is a proposal by @tarcieri to ameliorate the concern I am raising. Can you clarify?

[tarcieri]

Between the AES-NI backend, ARMv8 backend, and 32-bit and 64-bit fixsliced backends, and trying to support various combinations of software-only and autodetected intrinsics+software-only fallback, the #[cfg(...)] gating, even with cfg-if, has gotten quite convoluted.

I think it might be worth trying to PR using an additive intrinsics feature versus a subtractive force-soft feature just to see whether it's an improvement, and if so, how much.

[SergioBenitez]

Between the AES-NI backend, ARMv8 backend, and 32-bit and 64-bit fixsliced backends, and trying to support various combinations of software-only and autodetected intrinsics+software-only fallback, the #[cfg(...)] gating, even with cfg-if, has gotten quite convoluted.

I'm not sure I follow why there should be a difference. You'll need a #[cfg(..)] whether you use a feature cfg or a "regular" cfg.

I think it might be worth trying to PR using an additive intrinsics feature versus a subtractive force-soft feature just to see whether it's an improvement, and if so, how much.

Yes, this would assuage my concern. What kind of improvement are you referring to?

[newpavlov]

I'm a bit confused. The primary feature in question is force-soft which disables the use of AES-NI intrinsics, but this block of text seems to be arguing as if the reality was the inverse. There is no intrinsics feature; this is a proposal by @tarcieri to ameliorate the concern I am raising. Can you clarify?

The cited text is about the proposed intrinsics feature and why in my opinion it will not be a good solution in practice.

Cargo does have such a feature via non-feature cfgs. Seeing as these functionality changes should only be reasonably enabled by end-applications, non-feature cfgs seem like a perfect fit.

It could work in theory, but it has the same problems with discoverability. Also enabling it is a bit annoying with the current tooling (IIUC you either have to edit .cargo/config or enable it using RUSTFLAGS). But since force-soft uses are quite rare, I think it may work. At the very least, I prefer this approach to the intrinsics feature. Though I am still not sure if it's worth to remove the force-soft feature, since it does not create any problems in practice (unlike mutually exclusive features used in some crates).

Ideally we would declare such "non-features" in a crate's Cargo.toml and maybe somehow indicate mutually exclusive combinations, so cargo would be able to query all possible configuration options for a project's dependency tree and you would be able to enable them via its Cargo.toml. Do you know if there is an RFC for something similar? If not, I think it may be worth to draft it.

P.S.: BTW force-soft does enable capability of building the aes crate on pre-1.49 toolchains, so even from the theoretic point of view the situation is not so clear-cut. ;)

[SergioBenitez]

The cited text is about the proposed intrinsics feature and why in my opinion it will not be a good solution in practice.

I've reinterpreted your comments in this light and offer my commentary below.

Yes, we could state that intrinsics features should not be enabled by library crates outside dev builds, but in my experience this approach not only makes life significantly harder for crate users (who have to bother configuration and may have to discover that for best performance they have to enable a feature of a crate buried deep in their dependency tree).

The idea would be that intrinsics is enabled by default and is safe to use in libraries given the fall-back to dynamic feature detection and, failing that, the software implementation. This makes "best performance" the default. Also note that the present conditions run directly against your argument: as it stands, a single dependency can make it irreversibly impossible for any crate in the graph to benefit from AES-NI.

Also with high probability there will be crates which enable such feature unconditionally, "poisoning" all projects dependent on it and thus making it virtually impossible switching to the software implementation for any sizable enough project. Same goes for making it a default feature, most users will not bother with default-features = false.

The presence of a non-feature cfg would make this a non-issue, making it possible for an end-user who unconditionally desires a software implementation to request it and receive it. Given the rarity of such users and likelihood that they are already operating under environments with non-traditional requirements, the trade-off in extra effort to enable such a cfg seems more than worth it.

Ideally we would declare such "non-features" in a crate's Cargo.toml and maybe somehow indicate mutually exclusive combinations, so cargo would be able to query all possible configuration options for a project's dependency tree and you would be able to enable them via its Cargo.toml. Do you know if there is an RFC for something similar? If not, I think it may be worth to draft it.

You can do this yourself using compile_error!. It is always incorrect to error if two feature cfgs are enabled, but doing the same for non-feature cfgs is totally okay:

#[cfg(all(force_soft, not_force_soft))]
compile_error!("`force_soft` and `not_force_soft` are mutually exclusive `cfg`s: enable one or the other, but not both");

P.S.: BTW force-soft does enable capability of building the aes crate on pre-1.49 toolchains, so even from the theoretic point of view the situation is not so clear-cut. ;)

Sure, but using an older version of the crate accomplishes the same thing without the impossible to surmount downsides. Further, if enough crates do use the feature, then eventually no crate with any significant dependencies will benefit from AES-NI.

[newpavlov]

The idea would be that intrinsics is enabled by default and is safe to use in libraries given the fall-back to dynamic feature detection and, failing that, the software implementation.

You do not address the default-features = false issue of the intrinsics feature approach. In the next paragraph you switch to the cfg-based approach.

Also note that the present conditions run directly against your argument: as it stands, a single dependency can make it irreversibly impossible for any crate in the graph to benefit from AES-NI.

Yes, but none of the higher-level crates in this org enable this feature by default. And there is a very little incentive to do so (unlike with the intrinsics feature approach), since as you correctly note we want performance (i.e. AES-NI autodetection) enabled by default. Thus my statement about practicality of the current approach.

the trade-off in extra effort to enable such a cfg seems more than worth it.

As I've said in the previous comment, I do find the cfg-based approach viable and we probably should consider switching to it in the next breaking release. But I am on the fence about its worth with the current state of tooling (especially considering that we would have to migrate several other crates to this approach as well), so I probably will leave this decision to @tarcieri.

You can do this yourself using compile_error!. It is always incorrect to error if two feature cfgs are enabled, but doing the same for non-feature cfgs is totally okay

You are missing the point about discoverability and convenience of the declarative approach. The aes crate can be buried quite deep in a dependency tree and I am quite confident that we do not want to introduce two config options and emit a compile-time error if one of options does not get enabled. While such compilation error would help with discoverability, it will be really annoying.

Sure, but using an older version of the crate accomplishes the same thing without the impossible to surmount downsides.

No, the older aes versions are not compatible with the up-to-date version of the cipher crate and thus incompatible with recent versions of higher-level crates.

[SergioBenitez]

You do not address the default-features = false issue of the intrinsics feature approach. In the next paragraph you switch to the cfg-based approach.

I am not proposing one approach xor the other, I'm proposing both. intrinsics enabled by default with a non-feature cfg that can overwrite it. Or, alternatively, just the latter.

Yes, but none of the higher-level crates in this org enable this feature by default. And there is a very little incentive to do so (unlike with the intrinsics feature approach), since as you correctly note we want performance (i.e. AES-NI autodetection) enabled by default. Thus my statement about practicality of the current approach.

It's not about crates in this org, it's about the crates in an arbitrary dependency graph. For instance, @tarcieri enabled force-soft in a PR to the cookie crate to maintain MSRV. cookie has 84 dependents, among them reqwest, rocket, actix, and servo. Effectively every crate dealing with HTTP in any way eventually depends on cookie. Every single one of these crates would have been unable to make use of AES-NI in aes without recourse if I had accepted the PR.

You are missing the point about discoverability and convenience of the declarative approach. The aes crate can be buried quite deep in a dependency tree and I am quite confident that we do not want to introduce two config options and emit a compile-time error if one of options does not get enabled. While such compilation error would help with discoverability, it will be really annoying.

No, I don't think I am. Features and cfgs should be called out somewhere besides Cargo.toml: features declared in Cargo.toml may or may not be public, so it is not a reliable documentation source. Unless you document public features and cfgs, reading a Cargo.toml only gives you an idea of the superset of features. Furthermore, Cargo.toml can obviously contain comments indicating the presence of cfgs, if for some reason you want Cargo.toml to be a canonical reference. I assumed all of this was understood. My suggestion to use compile_error!() was as a matter of UX: you get immediate, actionable feedback if you configure things incorrectly, not type errors that have no meaning to the developer.

Or perhaps I'm misunderstanding you completely. Why would a deep dependency, or any dependency at all, use non-feature cfgs? I also don't understand which options you think should be mutually exclusive. Nothing that we've stated thus far would seem to implicate such a requirement.

No, the older aes versions are not compatible with the up-to-date version of the cipher crate and thus incompatible with recent versions of higher-level crates.

I'm not sure how this is relevant? You'd clearly need to match up your dependencies, but this is orthogonal to the issue at hand.

[SergioBenitez]

Checking in. Any chance of moving forward?

[tarcieri]

I'm personally for it in general, but probably not for making another breaking release for awhile.

Just for refactoring the internals I'd like to go to 1.51 and use const generics (as I've done in the nightly-only ARMv8 backend), but that's probably a bit of an aggressive version bump in the interim.