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 uninitialized with MaybeUninit #238

Merged
merged 4 commits into from
Oct 19, 2019

Conversation

Demi-Marie
Copy link
Contributor

@Demi-Marie Demi-Marie commented Oct 9, 2019

mem::uninitialized is deprecated and unsafe. This replaces its use
with mem::MaybeUninit and adds a proof that the use of
mem::MaybeUninit is correct.

Requested by @niklasad1.

Fixes #238.

`mem::uninitialized` is deprecated and unsafe.  This replaces its use
with `mem::MaybeUninit` and adds a proof that the use of
`mem::MaybeUninit` is correct.

Requested by @niklasad1.
@parity-cla-bot
Copy link

It looks like @demimarie-parity hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@ordian
Copy link
Member

ordian commented Oct 9, 2019

@demimarie-parity have you tried to measure if zeroing memory has no perf impact? (#233 (comment))

@Demi-Marie Demi-Marie mentioned this pull request Oct 9, 2019
@Demi-Marie Demi-Marie requested review from ordian, debris and Robbepop and removed request for debris October 9, 2019 17:26
@Demi-Marie
Copy link
Contributor Author

@ordian I haven’t. It would be worthwhile, though ― LLVM might even be able to optimize out the initialization.

@Demi-Marie Demi-Marie requested review from niklasad1 and removed request for ordian October 12, 2019 19:12
@niklasad1
Copy link
Member

@ordian
I checked it ended up with identical assembly

This generates the same assembly and is safer.
@Demi-Marie
Copy link
Contributor Author

@niklasad1 how did you do this? Also @ordian my latest commit removes all use of uninitialized memory.

@niklasad1
Copy link
Member

niklasad1 commented Oct 14, 2019

I created temporary initialization of U1024 in src/lib.rs

then this:

diff --git a/uint/src/lib.rs b/uint/src/lib.rs
index 352ccf7..751766d 100644
--- a/uint/src/lib.rs
+++ b/uint/src/lib.rs
@@ -31,3 +31,8 @@ pub use crunchy::unroll;
 #[macro_use]
 mod uint;
 pub use crate::uint::*;
+
+
+construct_uint! {
+       pub struct U1024(16);
+}
diff --git a/uint/src/uint.rs b/uint/src/uint.rs
index cc58ee2..0cdec03 100644
--- a/uint/src/uint.rs
+++ b/uint/src/uint.rs
@@ -72,13 +72,14 @@ macro_rules! impl_try_from_for_primitive {
 
 #[macro_export]
 #[doc(hidden)]
+#[inline(never)]
 macro_rules! uint_overflowing_binop {
        ($name:ident, $n_words: tt, $self_expr: expr, $other: expr, $fn:expr) => ({
                let $name(ref me) = $self_expr;
                let $name(ref you) = $other;
 
-               let mut ret = unsafe { $crate::core_::mem::uninitialized() };
-               let ret_ptr = &mut ret as *mut [u64; $n_words] as *mut u64;
+               let mut ret = [0_u64; $n_words];
+               let ret_ptr = ret.as_mut_ptr();
                let mut carry = 0u64;
 
                unroll! {
@@ -876,7 +877,7 @@ macro_rules! construct_uint {
                        }
 
                        /// Add with overflow.
-                       #[inline(always)]
+                       #[inline(never)]
                        pub fn overflowing_add(self, other: $name) -> ($name, bool) {
                                uint_overflowing_binop!(
                                        $name,
(END)

finally,

$ cargo asm --no-color uint::U1024::overflowing_add

let ret_ptr = &mut ret as *mut [u64; $n_words] as *mut u64;
let mut carry = 0u64;
$crate::static_assertions::const_assert!(core::isize::MAX as usize / core::mem::size_of::<u64>() > $n_words);
Copy link
Member

@niklasad1 niklasad1 Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

BTW, is isize::MAX used because of offset?

let $name(ref me) = $self_expr;
let $name(ref you) = $other;

let mut ret = unsafe { $crate::core_::mem::uninitialized() };
let mut ret = [0u64; $n_words];
let ret_ptr = &mut ret as *mut [u64; $n_words] as *mut u64;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let ret_ptr = &mut ret as *mut [u64; $n_words] as *mut u64;
let ret_ptr = ret.as_mut_ptr();

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The zero initialization and safety comments look good to me, there is a small caveat with static_assertions bump, but I think it's fine.

@@ -18,7 +18,7 @@ rand = { version = "0.7", optional = true, default-features = false }
rustc-hex = { version = "2.0", optional = true, default-features = false }
quickcheck = { version = "0.9", optional = true }
byteorder = { version = "1.2", optional = true, default-features = false }
static_assertions = "0.3"
static_assertions = "1.0.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this is technically a breaking change, since we pub use static_assertions;, but this is unlikely to be a problem in practice.

@ordian
Copy link
Member

ordian commented Oct 16, 2019

needs resolving

@dvdplm
Copy link
Contributor

dvdplm commented Oct 16, 2019

needs resolving

Can you please clarify what it is that needs resolving here?

@ordian
Copy link
Member

ordian commented Oct 16, 2019

Can you please clarify what it is that needs resolving here?

Sorry, I meant merge conflict needs to be resolved.

@Demi-Marie Demi-Marie merged commit 2b39ab9 into master Oct 19, 2019
@Demi-Marie Demi-Marie deleted the demi-uninitialized→maybeuninit branch October 19, 2019 22:40
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.

None yet

5 participants