cpu: Have "rdrand" feature take precedence over "custom"

[josephlr]

Right now, features are always enabled across all targets. This means
that if any Custom RNG is used anywhere in a crate's dependancy
graph on any target. The "custom" feature will be active on all
targets.

This old code made it impossible to have a bare metal target that uses "rdrand" on
x86_64 and a Custom RNG on aarch64. This change fixes this issue, and makes
sure that any implementation getrandom itself provides cannot be overridden.

I proposed the old precedence order in #84 (comment), but I think part of that was a mistake, given how Cargo treats features.

This also renames the "cpu" feature to "rdrand". While other CPU architectures have RNG intrinsic, they can behave differently than x86's, so it doesn't make sense to group them.

Signed-off-by: Joe Richey [email protected]

[newpavlov]

Personally I would prefer to move CPU randomness into a custom crate, assuming you'll find the approach proposed here viable.

Strictly speaking adding ARM support would require bumping getrandom's MSRV, which is not ideal. Yes, we could say that it's fine, since majority of the users will not be affected, but having the functionality in a separate crate would allow us more flexibility (e.g. we could bump getrandom-cpu version independently from getrandom) and would solve the issue as well.

@dhardy
Thoughts?

[josephlr]

Personally I would prefer to move CPU randomness into a custom crate, assuming you'll find the approach proposed here viable.

I agree that CPU-based RNG would ideally just use the custom RNG mechanism. In fact, I think this makes more sense than having the wasm32-unknown-unknown stuff use it. However, doing this runs into a bunch of issues in practice (see the #84 thread).

We need the rdrand implementation for SGX, so having a separate crate poses issues regarding out-of-tree modules and code duplication. After thinking about this some more (and incorporating your feedback), I have two proposals:

Proposal 1: Just rename the feature to "rdrand"

This would keep the same functionality, but would make stronger guarantees about forward compatibility. It would only ever do anything on x86_64 and x86 targets, preventing a future breaking changes. Going forward, if we support other CPU ISAs, they would be added under different features. This is reasonable, as different ISAs have different rules for using their RNG intrinsics (i.e. ARM's requires that you are in kernel mode), so having all "CPU-based randomness" under one feature might not be wise.

Proposal 2: Expose a cpurand function

We would change the semantics of the "cpu" feature to expose a cpurand function on platforms that support CPU-based randomness. This wouldn't change anything about how getrandom behaves. If a user wants to use CPU-based randomness to back getrandom, then then:

• Enable both "cpu" and "custom" features
• Put register_custom_getrandom!(cpurand); in their root crate

Alternatively, we could just always expose a cpurand function on x86.

Preferred approach

I'm leaning towards Proposal 1, as I don't think the rng instructions on other CPUs are similar enough to RDRAND to warrant combining them, but I could go either way.

[newpavlov]

Just rename the feature to "rdrand"

Sounds good.

[josephlr]

Just rename the feature to "rdrand"

Sounds good.

Done, let me know what you think. I updated the PR description. I didn't update the docs, I'm going to do all of that in #135