Disable default-features on rand dependency to avoid std version #107

therealprof · 2018-09-06T20:49:10Z

Due to the single dependency tree, the attempted use of a std version
flips all depending crates to the std version as well which will not
compile on no_std systems.

Fixes #105

Signed-off-by: Daniel Egger daniel@eggers-club.de

japaric · 2018-09-06T20:50:37Z

LGTM. Could you add an example that links to rand with default features disabled? That should serve as a regression test.

japaric · 2018-09-06T20:52:44Z

note that to get the example to build on CI you'll have to add it:

cortex-m-rt/ci/script.sh

Lines 14 to 28 in 5baa082

    
           local examples=( 
        
               alignment 
        
               divergent-default-handler 
        
               divergent-exception 
        
               entry-static 
        
               main 
        
               minimal 
        
               override-exception 
        
               pre_init 
        
               state 
        
               unsafe-default-handler 
        
               unsafe-hard-fault 
        
               unsafe-entry 
        
               unsafe-exception 
        
           )

therealprof · 2018-09-06T20:53:35Z

Shouldn't any of the existing examples do? The rand stuff is used for generation of unique linker symbols, no?

therealprof · 2018-09-06T21:14:24Z

@japaric Please have a look now.

japaric · 2018-09-06T21:18:52Z

Hmm, the build is failing since the first commit with: "error[E0658]: use of unstable library feature 'rustc_private'", and I don't understand why ...

therealprof · 2018-09-06T21:19:29Z

😀. And now travis-ci is running into the exact same problem I mentioned in the ticket before...

therealprof · 2018-09-06T21:20:40Z

I think it's because the version seems to be newer than the upstream version so it uses that instead which triggers this problem. As I said: I'm having the same troubles locally.

japaric · 2018-09-06T21:24:53Z

macros/Cargo.toml

 proc-macro2 = "0.4.15"

 [dependencies.syn]
 features = ["extra-traits", "full"]
 version = "0.14.8"

+[depedencies.rand]


oh lol. I see the problem. This is a typo; the "n" is missing after "depe".

japaric · 2018-09-06T21:25:59Z

The typo solves the weird "rustc_private" issue, but this still doesn't work because thread_rng doesn't exist in rand w/o default features.

@therealprof how did you get this working locally?

therealprof · 2018-09-06T21:28:17Z

I simply compiled the example via cargo build --example rand. Worked without problems.

therealprof · 2018-09-06T21:30:06Z

LOL and of course once I fixed the type it does not compile anymore. 😀

Raining lemons today...

japaric · 2018-09-06T21:31:26Z

@therealprof you didn't add #![feature(rustc_private)] to macros/src/lib.rs right? That would "fix" the problem by making the crate using the rand shipped with the toolchain but we can't use that because it's unstable.

therealprof · 2018-09-06T21:33:04Z

That's indeed what I did. Do you want me to rewrite the rand stuff in macros to use the seedable rng which works in no_std?

japaric · 2018-09-06T21:36:59Z

Do you want me to rewrite the rand stuff in macros to use the seedable rng which works in no_std?

Yes, please. Though, I'm concerned that using a seed which is not randomly generated will result in (a) predictable identifiers which then the user can the use to call exception handlers breaking the non-reentrant invariant, and / or (b) collisions between #[entry] fn foo() -> ! { .. } and #[pre_init] unsafe fn foo() {} if the seed is derived from the identifier.

therealprof · 2018-09-06T21:44:28Z

@japaric Well, the stuff is not really cryptographic anyway. If someone wants to desperately do bad stuff with the entry points, they can still do it. Collisions would be indeed a problem but a rand::SeedableRng is not going to have more of a collision than the threaded rng. We could use mem::uninitialized() to seed the RNG, that would make it slightly more unpredictable, if you're worried about reusing the same seed over and over again.

japaric · 2018-09-06T21:51:12Z

If someone wants to desperately do bad stuff with the entry points, they can still do it.

They can't with the current implementation.

If they call cargo expand and see that entry was renamed to bsb49kvt39ff0w29 and then modify their code to call bsb49kvt39ff0w29 it won't compile next time because the attribute will generate a different identifier. The key property here is that the attribute produces a different identifier on each run. It's not important what RNG is used; it's important that the seed of the RNG is different on each run.

We could use mem::uninitialized() to seed the RNG

That could turn into UB in any future LLVM release so it's not an option.

adamgreig · 2018-09-06T21:54:32Z

Can we just prevent the function from being called from safe code? I get that we can't just hide it in a private module (sadly, as that seems like it would totally fix the issue) but perhaps we can stick it inside an unsafe function wrapper with a predictable name? Or any other options to avoid rand?

Otherwise... seed with the build time... 😈

therealprof · 2018-09-06T21:57:42Z

If they call cargo expand and see that entry was renamed to bsb49kvt39ff0w29 and then modify their code to call bsb49kvt39ff0w29 it won't compile next time because the attribute will generate a different identifier.

Sure... but then again if someone really wants to do it they can do still it after the fact, i.e. without recompiling from source.

it's important that the seed of the RNG is different on each run.

Well, it's a new feature anyway. 😏

That could turn into UB in any future LLVM release so it's not an option.

Uhm....

Otherwise... seed with the build time... 😈

Or address of something in the running process. ;)

japaric · 2018-09-06T21:57:53Z

perhaps we can stick it inside an unsafe function wrapper with a predictable name?

That's an option but we would need a trampoline to preserve the safety that the user declared:

#[exception]
fn SysTick() { .. }

// expands into
#[no_mangle]
pub unsafe fn SysTic() {
    // copy paste the user function here
    #[inline(always)]
    fn SysTick() { .. }

    SysTick()
}

Otherwise... seed with the build time...

Seed with Instant::now().nanos() or something like that, maybe.

japaric · 2018-09-06T22:14:35Z

Uhm....

You should read through the Unsafe Code Guidelines team discussions. Apparently most unsafe operations are UB now. In any case, mem::uninitialized is going to be deprecated in favor of MaybeUninit and it's UB to read out its inner value if it has not been previously initialized.

therealprof · 2018-09-06T22:25:44Z

You should read through the Unsafe Code Guidelines team discussions. Apparently most unsafe operations are UB now. In any case, mem::uninitialized is going to be deprecated in favor of MaybeUninit and it's UB to read out its inner value if it has not been previously initialized.

I'm aware of MaybeUninit and general I agree with the notion of having everything sane and safe. But sometimes it is sufficient to know something exists (like a address in memory) without caring about it's value...

Due to the single dependency tree, the attempted use of a std version flips all depending crates to the std version as well which will not compile on no_std systems. Fixes rust-embedded#105 Signed-off-by: Daniel Egger <daniel@eggers-club.de>

Signed-off-by: Daniel Egger <daniel@eggers-club.de>

therealprof · 2018-09-07T21:54:30Z

... and of course the Rng is seeded on each invocation of the random token function while the seed is only increasing each second which one fast machines means: crap (and I've been able to experience it first hand). I've added an atomic counter, increased on each invocation (much easier than the pro way of initialising once and reusing the same Rng), to continuously change the seed. Now collisions are a thing of the past.

japaric

LGTM. r=me with nits addressed.

japaric · 2018-09-07T22:37:46Z

macros/src/lib.rs


+use core::mem;


Why core::mem and not std::mem?

japaric · 2018-09-07T22:39:18Z

macros/src/lib.rs

@@ -492,7 +499,10 @@ pub fn pre_init(args: TokenStream, input: TokenStream) -> TokenStream {

 // Creates a random identifier
 fn random_ident() -> Ident {
-    let mut rng = rand::thread_rng();
+    let seed = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs();
+    CALL_COUNT.fetch_add(1, Ordering::SeqCst);


This returns the previous value. Why not use that instead of doing another atomic load?

japaric · 2018-09-07T22:41:18Z

macros/src/lib.rs

+    let seed = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs();
+    CALL_COUNT.fetch_add(1, Ordering::SeqCst);
+    let seed = [seed, CALL_COUNT.load(Ordering::SeqCst) as u64];
+    let mut rng = rand::rngs::SmallRng::from_seed( unsafe { mem::transmute(seed) } );


Let's not use unsafe code here. There are methods to go from u64 to [u8; 8] and vice versa that could be used here.

To the best of my knowledge only in nightly...
https://doc.rust-lang.org/nightly/std/primitive.u64.html#method.to_be_bytes

Of course I could do some bitmasking. ;)

To the best of my knowledge only in nightly...

Still? I thought it was already stabilized. Perhaps I was thinking of f32.to_bits.

Of course I could do some bitmasking. ;)

I would prefer that over unsafe code.

adamgreig · 2018-09-07T22:57:34Z

Could we use .subsec_nanos() instead of .as_secs() to avoid the issue? I'm not sure which systems have std but don't give nanosecond-level timers...

therealprof · 2018-09-07T23:08:48Z

Could we use .subsec_nanos() instead of .as_secs() to avoid the issue? I'm not sure which systems have std but don't give nanosecond-level timers...

Where did you dig that out? 😲
However that only yields a 32 bit value so that doesn't simplify things...

adamgreig · 2018-09-07T23:15:49Z

I was more thinking you could seed it once with that and then not worry about the counter. Though I guess you have to do the same twiddling to turn it into [u8; 8].

The RNG is seeded from the current time and an counter increasing with each call to yield different random values on each invocation. Signed-off-by: Daniel Egger <daniel@eggers-club.de>

therealprof · 2018-09-07T23:48:54Z

So... bit twiddling it is.

japaric

Thanks!

bors r+

107: Disable default-features on rand dependency to avoid std version r=japaric a=therealprof Due to the single dependency tree, the attempted use of a std version flips all depending crates to the std version as well which will not compile on no_std systems. Fixes #105 Signed-off-by: Daniel Egger <daniel@eggers-club.de> Co-authored-by: Daniel Egger <daniel@eggers-club.de>

bors · 2018-09-08T22:01:27Z

Build succeeded

continuous-integration/travis-ci/push

korken89 · 2018-09-09T07:41:38Z

Great work!

therealprof requested a review from a team as a code owner September 6, 2018 20:49

japaric reviewed Sep 6, 2018

View reviewed changes

therealprof force-pushed the fix-for-105 branch from 475cbba to 5f62fd8 Compare September 6, 2018 23:01

therealprof added 2 commits September 7, 2018 21:23

Added example using rand crate to ensure no_std compatibility

698d0ab

Signed-off-by: Daniel Egger <daniel@eggers-club.de>

therealprof force-pushed the fix-for-105 branch from 5f62fd8 to b300508 Compare September 7, 2018 21:50

japaric reviewed Sep 7, 2018

View reviewed changes

Use SmallRng instead of thread_rng which is not available on no_std

ca4e52b

The RNG is seeded from the current time and an counter increasing with each call to yield different random values on each invocation. Signed-off-by: Daniel Egger <daniel@eggers-club.de>

therealprof force-pushed the fix-for-105 branch from b300508 to ca4e52b Compare September 7, 2018 23:47

japaric approved these changes Sep 8, 2018

View reviewed changes

bors bot merged commit ca4e52b into rust-embedded:master Sep 8, 2018

therealprof mentioned this pull request Oct 11, 2018

[RFC] Managing breaking changes to existing traits rust-embedded/embedded-hal#100

Closed

Disable default-features on rand dependency to avoid std version #107

Disable default-features on rand dependency to avoid std version #107

Conversation

therealprof commented Sep 6, 2018

japaric commented Sep 6, 2018

japaric commented Sep 6, 2018

therealprof commented Sep 6, 2018 • edited Loading

therealprof commented Sep 6, 2018

japaric commented Sep 6, 2018

therealprof commented Sep 6, 2018

therealprof commented Sep 6, 2018

japaric Sep 6, 2018

Choose a reason for hiding this comment

japaric commented Sep 6, 2018

therealprof commented Sep 6, 2018

therealprof commented Sep 6, 2018

japaric commented Sep 6, 2018

therealprof commented Sep 6, 2018

japaric commented Sep 6, 2018

therealprof commented Sep 6, 2018 • edited Loading

japaric commented Sep 6, 2018

adamgreig commented Sep 6, 2018

therealprof commented Sep 6, 2018

japaric commented Sep 6, 2018 • edited Loading

japaric commented Sep 6, 2018

therealprof commented Sep 6, 2018

therealprof commented Sep 7, 2018

japaric left a comment

Choose a reason for hiding this comment

japaric Sep 7, 2018

Choose a reason for hiding this comment

japaric Sep 7, 2018

Choose a reason for hiding this comment

japaric Sep 7, 2018

Choose a reason for hiding this comment

therealprof Sep 7, 2018

Choose a reason for hiding this comment

japaric Sep 7, 2018

Choose a reason for hiding this comment

adamgreig commented Sep 7, 2018

therealprof commented Sep 7, 2018

adamgreig commented Sep 7, 2018

therealprof commented Sep 7, 2018

japaric left a comment

Choose a reason for hiding this comment

bors bot commented Sep 8, 2018

Build succeeded

korken89 commented Sep 9, 2018

therealprof commented Sep 6, 2018 •

edited

Loading

therealprof commented Sep 6, 2018 •

edited

Loading

japaric commented Sep 6, 2018 •

edited

Loading