Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace black_box with std::hint::black_box #96

Merged
merged 4 commits into from
Feb 7, 2023

Conversation

dsprenkels
Copy link
Contributor

@dsprenkels dsprenkels commented Dec 16, 2022

Rust version 1.66 now comes with a stabilized version of a black_box function that emits an "empty" assembly directive. This patch replaces the older optimization barrier, which was based on a volatile read with the newly standardized function.

Announcement from Mara:
https://hachyderm.io/@Mara/109518823276877083

Current implementation of black_box in rustc:
https://github.com/rust-lang/rust/blob/a803f313fdf8f6eb2d674d7dfb3694a2b437ee1e/compiler/rustc_codegen_llvm/src/intrinsic.rs#L341-L375


Review suggestions:

  • This patch bumps the MSRV to the most recent version (1.66), which might be undesirable. One workaround is to wait for merging this until cfg_version is stabilized, and then write a fallback black_box based for rustc before 1.66.

isislovecruft and others added 2 commits July 13, 2021 05:06
Rust version 1.66 now comes with a stabilized version of a black_box
function that emits an "empty" assembly directive.  This patch
replaces the older optimization barrier, which was based on a
volatile read with the newly standardized function.

Announcement from Mara:
	<https://hachyderm.io/@Mara/109518823276877083>

Current implementation of black_box in rustc:
	https://github.com/rust-lang/rust/blob/a803f313fdf8f6eb2d674d7dfb3694a2b437ee1e/compiler/rustc_codegen_llvm/src/intrinsic.rs#L341-L375
README.md Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
This commit partially reverts the changes from 4978f42, and adds
the new optimization barrier implementation under a new feature
`core_hint_black_box`.
@dsprenkels
Copy link
Contributor Author

dsprenkels commented Jan 5, 2023

@tarcieri Thanks for the review! I added back the original implementation (based on the volatile read) and added the new barrier implementation under the core_hint_black_box feature flag.

I also updated the README file a bit to better represent how the barrier got updated over time

Cargo.toml Show resolved Hide resolved
@tarcieri
Copy link
Contributor

tarcieri commented Jan 5, 2023

FYI: I don't actually have commit access to subtle 😦

I've asked about what to do with these PRs. Looks good to me now aside from bikeshedding the feature name.

@isislovecruft
Copy link
Member

Mild preference for core-hint-black-box or core_hint_black_box since we've had at least three versions of the blackbox function so far, one of my assembly, one of my wat/wasm, and now the core::hint::pointer based one. It seems like a good reason to be explicit in the naming (albeit annoying verbose) about which "black box" users are opting into.

isislovecruft
isislovecruft previously approved these changes Feb 7, 2023
@isislovecruft
Copy link
Member

@dsprenkels Thanks for updating the documentation as well!

@isislovecruft isislovecruft changed the base branch from develop to main February 7, 2023 04:12
@isislovecruft isislovecruft dismissed their stale review February 7, 2023 04:12

The base branch was changed.

@isislovecruft isislovecruft self-requested a review February 7, 2023 04:12
@isislovecruft isislovecruft merged commit bd282be into dalek-cryptography:main Feb 7, 2023
@dsprenkels dsprenkels deleted the black-box branch February 7, 2023 13:11
@tarcieri
Copy link
Contributor

tarcieri commented Mar 1, 2023

Heads up: the rustdoc for this function on the beta channel of rustc now contains a big scary warning saying black_box cannot be depended upon for cryptographic purposes:

https://doc.rust-lang.org/beta/core/hint/fn.black_box.html#when-is-this-useful

I think this should probably be reverted.

@dsprenkels
Copy link
Contributor Author

I completely agree.

@dsprenkels
Copy link
Contributor Author

The clarification docs were added in rust-lang/rust#108088. When I try to follow the rabbit hole I cannot seem to find where it was exactly specified first that black_box may not do anything useful. We should definitely revert the changes, but maybe consider keeping the feature flag to prevent a breaking change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants