[On Hold] Add support for builds without assembly.

[samscott89]

Set feature "no_asm" to use. This currently disables use of
aead, ec, and rsa/signature modules due to missing code.

Also adds the mipsel-unknown-linux-gnu target to the travis
builds. These builds currently fail in anticipation of PR#199.

[samscott89]

See #199, in particular this comment for more background on this.

One thing different from there is I left the digest module in for no_asm builds. It occurs in too many places, and since it will be fixed imminently by #199, didn't seem worth trying to be clever.

[coveralls]

Coverage decreased (-0.5%) to 88.355% when pulling b7570c6cfc84ea074b0d758cd0108d5d7af471bb on samscott89:no_asm into 2c3a95e on briansmith:master.

[coveralls]

Coverage decreased (-0.5%) to 88.355% when pulling ef27d50 on samscott89:no_asm into 2c3a95e on briansmith:master.

[coveralls]

Coverage decreased (-0.5%) to 88.355% when pulling ef27d50 on samscott89:no_asm into 2c3a95e on briansmith:master.

[coveralls]

Coverage decreased (-0.2%) to 88.632% when pulling ef27d50 on samscott89:no_asm into 2c3a95e on briansmith:master.

[briansmith]

This looks great. It will be at least a couple of days before I can get to this review.

[coveralls]

Coverage decreased (-0.001%) to 89.924% when pulling 4ac34e8b85631bb906c937317edfe45346913019 on samscott89:no_asm into 08c729e on briansmith:master.

[briansmith]

In the no_asm case, we could actually make block_data_order a normal, non-unsafe function, right? In the SHA-1 code, we could skip over the unsafe -> safe adapter function, for example.

[briansmith]

Let's drop these definitions that call unimplemented!(). I'll keep this commit separate from the one that adds the Rust implementations of these functions, but I'll land them together. I'd rather have unbuilding code in the intermediate state between those two commits than building-but-broken code.

[briansmith]

What I mean by my comment above is that we should make it so that in the no_asm mode, block_data_order_safe is used as the block_data_order function. But, I can see now that that requires additional effort, so we can defer that to a later commit.

[briansmith]

This all looks good to me. I'll land it when the Rust SHA-2 code is landed.

[briansmith]

One question: You chose little-endian just because there's places where we have little-endian logic and no big-endian counterpart, right? Or, was there some other reason?

[samscott89]

One question: You chose little-endian just because there's places where we have little-endian logic and no big-endian counterpart, right? Or, was there some other reason?

Correct. Although since the ec module is disabled for the no_asm builds anyway, it's actually only the u32x2 macro which would need to be ported.

What I mean by my comment above is that we should make it so that in the no_asm mode, block_data_order_safe is used as the block_data_order function. But, I can see now that that requires additional effort, so we can defer that to a later commit.

Wouldn't be a huge amount of additional work. The main reason I opted to keep it marked as unsafe was to be compatible with the unsafe { ... } declarations in the body of the update and finish function. We could go the other direction instead, and wrap the extern definitions (similar to in the poly1305 code) so that block_data_order is always a safe function.

[samscott89]

Ok, made the suggested changes.

I re-factored block_data_order to be a safe function, and wrapped the extern versions. What's nice is that no unsafe code blocks are needed in digest if no_asm is used.

[coveralls]

Coverage decreased (-0.3%) to 89.634% when pulling 80994ddffac0f189a92e362ec273ffbb23ed11a0 on samscott89:no_asm into f87e2e8 on briansmith:master.

[coveralls]

Coverage decreased (-0.3%) to 89.634% when pulling 80994ddffac0f189a92e362ec273ffbb23ed11a0 on samscott89:no_asm into f87e2e8 on briansmith:master.

[coveralls]

Coverage decreased (-0.3%) to 89.634% when pulling 80994ddffac0f189a92e362ec273ffbb23ed11a0 on samscott89:no_asm into f87e2e8 on briansmith:master.

[samscott89]

The approach I took in the heap PR was to make use_heap default-enabled. Stuff which doesn't need it can opt out, and if nothing needs it, then it doesn't get enabled.

There's a slight distinction with asm though. While it does reduce (for now at least) the API, it also replaces existing functionality. In practice, very few projects will prefer to use the non-assembly version. So if asm was a default feature, then then vast majority of crates would prefer to use it. However, that means that if you depend on a crate using ring with asm, you wont ever be able to build it without that feature.

Unless of course the crate itself allows you to specify not to use asm.

As it is, the minority cases which require no_asm builds can at least attempt to use that feature and see if the build works (this will need to be documented as such). In the future, the full API of ring should be available in either mode, in which case there are no longer any problems - other than perhaps a syntactic one, where we could change no_asm to use_rust_code or something.

Target/platform specific builds does sound like a tempting option and would probably help to catch most of the unsatisfactory cases above. Whether it captures everything depends on whether no_asm has a use case outside of supporting platforms which don't currently have assembly code?

[eternaleye]

The simple fact of the matter is that Cargo features that reduce API surface when enabled break Cargo.

The two solutions are to make it increase API surface instead (asm feature), or make it neutral (API parity).

After all, if the crate you depend on that needs the assembly does so because of the API it causes to be exposed, disabling it will cause spooky build failures at a distance. This is, I think, kind of unacceptable. As a user, I would consider that a huge mark against using this crate - it does not cooperate well with the ecosystem.

Such build failures would be attributed to other, downstream crates, not to ring, and would cause nontrivial work for those having to deal with them.

[briansmith]

The simple fact of the matter is that Cargo features that reduce API surface when enabled break Cargo.

The two solutions are to make it increase API surface instead (asm feature), or make it neutral (API parity).

Keep in mind that the long-term goal is API parity but we'll need to deal with interim states where we don't have API parity.

[...] Such build failures would be attributed to other, downstream crates, not to ring, and would cause nontrivial work for those having to deal with them.

No library crate should be using the no_asm feature in the first place; only application crates should. If we document this then we can work on the assumption that no library crates will use the no_asm feature. Under that assumption, is the no_asm feature still problematic?

Here are the requirements, as I see them:
(1) Libraries that depend on ring shouldn't have to explicitly add a dependency on an asm feature.
(2) Libraries that depend on ring shouldn't hav eto explicitly add a dependency on any features except rsa_signing (and that exception might be temporary) to use any basic ring feature.
(3) It should be possible to build applications targetting platforms we don't have full asm support for, for some subset of the functionality of ring that will increase to "all functionality" over time.
(4) It should be possible to force the test suite to run the test suite against the non-asm implementations for any algorithm that exists.

I have a feeling that there's not a 100% perfect and we'll have to make compromises. I'm open to considering any solution that has a solution that meets the above requirements.

[eternaleye]

No library crate should be using the no_asm feature in the first place; only application crates should. If we document this then we can work on the assumption that no library crates will use the no_asm feature. Under that assumption, is the no_asm feature still problematic?

Yes. Specifically, library foo depends on a function only available with the asm implementation, and application bar enables no_asm. foo then fails to compile, with ring only implicated very indirectly. This can occur arbitrarily deep in the dependency graph of bar.

Here are the requirements, as I see them:
(1) Libraries that depend on ring shouldn't have to explicitly add a dependency on an asm feature.
(2) Libraries that depend on ring shouldn't have to explicitly add a dependency on any features except rsa_signing (and that exception might be temporary) to use any basic ring feature.

These are covered by making it a default feature.

(3) It should be possible to build applications targetting platforms we don't have full asm support for, for some subset of the functionality of ring that will increase to "all functionality" over time.

This is covered by opting out of default features. Note that trying to build a no_asm application bar which depends on a library foo which uses functions only available in asm mode fails no matter what we do here: What's important is that with no_asm, the failure is "spooky" and occurs in some dependent crate, whereas with asm, ring fails to build, and there are ways to figure out what pulled in the asm feature (i.e., failed to opt out).

In particular, cargo tree --package ring --invert will show what packages depend on ring, complete with versions - and I may submit a PR for it to show what features each dependent pulls in.

Either platform-dependent features or scenarios would further improve the situation, allowing Cargo to error out up-front, before building anything. However, those only work with asm, not no_asm.

(4) It should be possible to force the test suite to run the test suite against the non-asm implementations for any algorithm that exists.

This either requires that even in the asm case, the no_asm functions are built, or that the tests are run twice (cargo test can take --no-default-features after all)

[briansmith]

No library crate should be using the no_asm feature in the first place; only application crates should. If we document this then we can work on the assumption that no library crates will use the no_asm feature. Under that assumption, is the no_asm feature still problematic?

Yes. Specifically, library foo depends on a function only available with the asm implementation, and application bar enables no_asm. foo then fails to compile, with ring only implicated very indirectly. This can occur arbitrarily deep in the dependency graph of bar.

Thanks. The problem that causes this is that ring doesn't expose its entire API in no_asm mode. It can be fixed by making sure that ring exposes the entire API in no_asm mode, and then have the build succeed up to the point of linking, where it will fail due to missing function definitions. WDYT?

There is a serious problem with defining an "asm" feature and then having people who don't want asm code to use --no-default-features: We don't want people who DO want asm code to lose it because they built with --no-default-features and didn't explicitly specify the asm feature. Although it probably doesn't matter at all for the digest code, it would likely matter much more for other things like ECC and maybe AES where the no-asm code won't be as safe and/or as efficient as the asm code.

So, I feel comfortable right now with the following plan:

• Document that the no_asm feature is a work in progress and a internal developer-only feature to help with testing, and is not for end-users of ring. This means, in particular, not documenting it in the crate-level rustdoc documentation and instead documenting it in the BUILDING.md documentation.
• Change the implementation of no_asm so that ring exposes the same exact API either way, and where the build will fail only at link time, only if a feature without a non-asm polyfill is used.
• Agree to work towards automatically detecting platforms that we don't have asm code for and falling back to no-asm mode automatically for algorithms for which it is safe to do so.
• Agree on the understanding that the missing functionality in no_asm mode is a temporary problem that we'll eventually solve by implementing things in a way where all default features are always available.

Obviously this isn't ideal but the problem is that "asm" vs "no-asm" isn't really a feature choice, but the feature mechanism only way we have to control how ring is built.

[eternaleye]

Er, I think you misunderstand what I meant regarding --no-default-features. The commandline argument is only relevant when ring is the top-level crate - that is, when running tests, such as via ring's CI. All it does is disable the default features of that crate. For any crate depending on ring, it's part of Cargo.toml just like other feature flag handling.

It's not something you build with, it's a dependency constraint you opt out of in Cargo.toml. An example from the Cargo docs:

[dependencies.awesome]
version = "1.3.5"
default-features = false # do not include the default features, and optionally
                         # cherry-pick individual features
features = ["secure-password", "civet"]

As a result, your reasoning in that entire second paragraph is erroneous. If any dependent does not opt-out of ring's default features (because it's per-dependency) in its own Cargo.toml, then asm gets enabled, because Cargo features are a ratchet.

Similarly, if any dependent enables no_asm, every other crate that depends on ring will fail to build, with poor error messages, which will look like the dependent crates are broken - leading to bugs being reported against those crates' upstream, which is a waste of their time, because the cause is ring.

All it takes is one crate pulled in arbitrarily deep in the dependency graph enabling the "experimental" no_asm feature to break the build in ways that waste other people's time. Meanwhile, asm fails safe (in the case that asm is supported on that platform), and in the case it's not, at least reports only one error, and attributes it to ring.

To build on the example I gave before:

foo
    ring
    bar
        ring
    baz
        corge
        grault
            ring
    qux
        garply
            zurg
                ring + no-asm

At this point, grault and bar fail to build, because of zurg enabling no-asm. Both error messages will report nothing but code from those crates calling nonexistent functions or using nonexistent types, not implicating ring at all.

Even if someone patched them to only use no-asm stuff, then foo would fail in the same manner. The user would have to know ahead of time that ring misbehaves in this manner, or else they would waste the time of the maintainers of grault, bar, and possibly foo as well.

Moreover, the user has to already know ring is at fault in order to even start looking for zurg, and the only fix is making zurg not pass no-asm - which may not be workable.

It's poor citizenship.

[briansmith]

I see you understand what I meant regarding --no-default-features vs. the Cargo.toml. I think we both understand the situation identically.

If some crate enables the no-asm feature then that's an unsupported configuration and so I don't care what happens in that situation.

If there's an alternative besides using feature flags for doing this, I'd love to use it. We need to find a solution that doesn't cause default-features = false to disable the assembly language implementations while still allowing us (the ring developers) to disable the assembly language implementations for testing purposes.

It's poor citizenship.

Increase your level of politeness, please.

[eternaleye]

I'm sorry; I didn't intend the "poor citizenship" comment as impoliteness - I see it as meaningfully descriptive. The reason is that the burden and cost of non-root crates enabling no_asm being an "unsupported" configuration falls on other upstreams, increasing the workload of other members of the community.

It creates a negative externality - ring behaving in a manner counter to Cargo best-practices then makes more work for other people, and not even the ones who chose to do something unsupported. Instead, it places the burden on people who use ring perfectly as intended.

and so I don't care what happens in that situation.

In that context, the above statement then does parse as "I'm unconcerned with causing negative externalities in the greater Cargo ecosystem" - something that, to me, is the literal definition of "poor citizenship."

[briansmith]

Please suggest a better alternative considering the constraints I gave above. Right now there doesn't seem to be a better alternative. What you've suggested so far is worse insofar as I understand it. People accidentally misusing the no_asm feature isn't worth worrying about. They could easily name a no_mangle function GFp_x25519_public_from_private and cause even worse problems, but we accept that risk because ultimately we're limited in our choices.

[eternaleye]

I'm actually curious as to why default-features = false is such a problem. There are, currently, (potentially) two default features - one for enabling RSA features which require allocation; and the second for enabling assembly implementations of certain functions.

The fact is, the cases in which one is unsuitable are strongly correlated with the cases where neither is suitable - unsupported platforms and constrained environments. Moreover, opting out of default features doesn't mean the dependent crate can't then re-enable some subset of the defaults. For example, a crate that runs in a no-allocation context on a supported architecture might do this:

[dependencies.ring]
default-features = false
features = ["asm"]

Similarly, a crate that wants to do RSA on an unsupported platform could do this:

[dependencies.ring]
default-features = false
features = ["use_heap"]

This is the exact model used by pretty much all crates that support both std and core - there's some default feature, which when enabled causes the crate to use std, and when it's not enabled the crate falls back to core. Various crates have other features that default to on in addition to the std/core switch; default-features = false is merely a directive allowing finer-grained feature selection - not a blanket off-switch.

[samscott89]

@eternaleye, running with your previous example since it's a more concrete way to discuss these implications. It would be expected that none of bar, grault, nor zurg should ever use anything but the default feature asm. Even if the features they require are supported in either mode, disabling asm would likely upset the majority of their users as they find their operations become a lot slower.

However, suppose none of these crates strictly require asm, i.e. all functionality is also supported by the pure rust alternatives. If someone wished to use these crates on some platform for which assembly builds aren't supported, what are their options? As far as I'm aware default-features only applies to the top level crate. Which means building qux with default-features = false wont propgate down the tree to zurg?

On the other hand, in the no_asm variant, the user can simply add ring with the no_asm feature as a dependency and test whether the build still works.

[samscott89]

@briansmith

So, I feel comfortable right now with the following plan:

• Document that the no_asm feature is a work in progress and a internal developer-only feature to help with testing, and is not for end-users of ring. This means, in particular, not documenting it in the crate-level rustdoc documentation and instead documenting it in the BUILDING.md documentation.
• Change the implementation of no_asm so that ring exposes the same exact API either way, and where the build will fail only at link time, only if a feature without a non-asm polyfill is used.
• Agree to work towards automatically detecting platforms that we don't have asm code for and falling back to no-asm mode automatically for algorithms for which it is safe to do so.
• Agree on the understanding that the missing functionality in no_asm mode is a temporary problem that we'll eventually solve by implementing things in a way where all default features are always available.

Obviously this isn't ideal but the problem is that "asm" vs "no-asm" isn't really a feature choice, but the feature mechanism only way we have to control how ring is built.

This seems to be to be the most pragmatic way forwards. In particular, the second bullet point seems to address @eternaleye's main complaint. I can put together a proof of concept, but presumably the build would fail with an undefined reference type failure.

Hopefully cargo will support target-specific features at some point (@eternaleye mentioned them earlier in this thread) so we can enforce no_asm on anything outside of the supported architectures.

[samscott89]

One problem with this approach is running tests. Since cargo test will still attempt to build all tests, this will always fail at link time. The no_asm feature could be used temporarily to disable tests for unsupported functionality.

Then, the only other place no_asm will be used is to choose between native rust and asm implementations. Therefore, building with the no_asm feature on most systems would build as much as possible with native rust implementations, and use assembly for the others.

Probably best to change the semantics in this case. Perhaps native_rust to indicate the build is preferring rust code over assembly.

[samscott89]

Ok, so I've made a new commit based on the above. This instead adds a feature native_rust, which will try to use native Rust code wherever implementations exist. Currently there are no alternative implementations, but this will be a lot more relevant with #199.

Running cargo test --features=native_rust will only run tests which have Rust implementations. I can confirm this builds + tests fine for mipsel for example (hopefully we'll see this with the Travis build too).

Enabling native_rust cannot break a build since it does not remove anything, but will only try to replace assembly with Rust code where possible. In the future, this will hopefully include everything. But in the meantime, nothing breaks.

[eternaleye]

@briansmith

As far as I'm aware default-features only applies to the top level crate.

This is incorrect. Any crate can opt out of the default features of individual dependencies, separately.

That does explain all the other confusion, though. Basically, default-features = false is a per-dependency flag capable of being set in any Cargo.toml file. By comparison, because the top-level create has nothing depending on it, the only way to opt out of its defaults is the cargo command line flag --disable-default-features, which applies only to the top-level crate.

Essentially, default-features = false just says "I don't need all of the default features of this dependency, and will manually select the features I do need."

Combined with Cargo features being additive, this results in features only being enabled if things actually need them.

@samscott89
While native-rust is better than no-asm, it fundamentally can't support any cases where there can never be a native rust implementation that is safe, because it tries to build the assembly unconditionally. As a result, ring will always fail to build for unsupported platforms, while an asm feature would enable it to build without those.

Basically, no-asm breaks Cargo, native-rust breaks platforms that don't support assembly, making it of marginal benefit at best, and asm handles both.

[samscott89]

That was my understanding, not necessarily shared by @briansmith.

That does explain all the other confusion, though. Basically, default-features = false is a per-dependency flag capable of being set in any Cargo.toml file. By comparison, because the top-level create has nothing depending on it, the only way to disable it's defaults is the cargo command line flag --disable-default-features, which applies only to top-level crate.

Sorry, to be clearer I meant the default-features will only apply to the immediate dependencies of the top level crate. So for this dependency tree:

foo
    ring
    bar
        ring

Let's say bar depends on ring and uses the defaults for simplicity. If somebody wants to build foo without default features (e.g. not using asm) then my understanding was that setting default-features = false for ring in the Cargo.toml for foo wont override the choice made by bar? I.e. cargo will still compute the union of selected features which would be [ ] and ["asm", ...] in this example?

[eternaleye]

@samscott89: Yes, and this is the exact same situation as core vs. std, which is directly analogous. The community convention is to have a default std feature, rather than a no_std (no_asm) or maybe_core (native-rust) feature.

Platform specific features, which I linked to, or scenarios, which I also linked to, would improve the situation when implemented.

In addition, there's a strong sentiment against link time errors in the rust community, because they generally are unreadable. They also mean that a lot of stuff gets built uselessly, wasting the developer's time.

[samscott89]

Yes, and this is the exact same situation as core vs. std, which is directly analogous.

To help me understand: why is std the default feature? What's the downside of using #![no_std] by default if a crate already doesn't need anything from std?

[eternaleye]

I'm speaking specifically of the case where, depending on a feature flag, a crate may or may not use code from std - just like how ring, depending on a feature flag, may or may not use assembly.

This is, in fact, exactly how the RSA code that requires allocation works in ring today.

The situation matches because, just like ring's assembly code doesn't support some platforms, there are platforms not supported by all of std - systems that lack filesystems, or cannot do dynamic memory allocation, etc.

The no_std approach has the issues I've described above for no_asm - it violates Cargo's invariants, causing breakage at a distance in nasty ways.

The native-rust approach, meanwhile, would work if and only if all code can eventually be done (safely!) in pure rust - if not, it will never be able to support platforms that lack assembly, even if dependents don't use the code that lacks safe, pure-rust implementations.

It also is useless for assembly-lacking platforms in the middle period, where not everything has been ported.

[briansmith]

To follow up on this:

The concerns @eternaleye mentioned are valid but unfortunately Rust simply doesn't give us any features we need to do what @eternaleye suggests and still make progress in a useful way. So I'm perfectly happy to take a PR like this and do things the imperfect way until the Rust developers enable us to do things the perfect way.

However, I already hate all the #[cfg(...)] logic we have. I'd rather work on reducing the existing #[cfg(...)] mess. Also, right now supporting platforms that we don't have assembly language code isn't a high priority. So, unless/until it becomes a high priority, let's just park this.

[briansmith]

Thanks for submitting this, long ago.

Now is a good time for us to add support for builds without assembly but the code has diverged so much I think it isn't practical to rebase is PR on the current tree, so I'm going to close this.