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

Add rand, srand, and rand_r implementations #31

Closed
wants to merge 6 commits into from

Conversation

Sympatron
Copy link
Contributor

This PR adds implementations for the C functions rand, srand, and rand_r. They all return the same deterministic values as glibc would.

rand_r is quite straightforward, but rand and srand require global mutable state to be implemented according to the standard. I chose to use critical-section to implement this safely, because it is platform independent and should work almost everywhere.

@thejpster
Copy link
Member

344 32-bit words? On the stack? Oof.

I'd prefer something a little less heavyweight. Like an LFSR we can store in a portable AtomicU32.

@thejpster
Copy link
Member

I think there are also serious licence questions about transliterating LGPL code from glibc into Rust and labelling it as Blue Oak licensed.

@Sympatron
Copy link
Contributor Author

I thought compatibility with glibc would be nice. It's also a tested solution.

But you are correct, that it has some drawbacks including the licensing issue.

If you are interested I could look into implementing rand more like rand_r maybe with a commonly used LFSR.

@thejpster
Copy link
Member

I could look into implementing rand more like rand_r maybe with a commonly used LFSR.

I would prefer that, thanks.

@jonathanpallant
Copy link
Collaborator

There's also a question about how we work out what RAND_MAX is defined as in stdlib.h on any given platform.

  1. We could run bindgen on stdlib.h and try and pull out the value of RAND_MAX
  2. We could assume that RAND_MAX is 32768, because that's a very common choice
  3. We could provide some flags that let you configure what you want RAND_MAX to be, with 32768 as the default.

@Sympatron
Copy link
Contributor Author

The problem with bindgen is that it does not necessarily use the same compiler/header that cc uses. So the C code that actually needs rand is not guaranteed to see the same RAND_MAX that bindgen in tinyrlibc sees...

Apart from that, what do you think about using the same implementation as rand_r for rand?

@jonathanpallant
Copy link
Collaborator

what do you think about using the same implementation

That seems like a good idea. I'd use a portable_atomic::AtomicU32 to do a compare-and-swap, so if you have actual atomics they get used and if not, it disables interrupts for a few clock cycles to atomically update the rand state. Then you don't need critical_section.

@Sympatron Sympatron force-pushed the rand branch 2 times, most recently from d0c6daa to 251d7dc Compare October 25, 2024 22:32
@Sympatron
Copy link
Contributor Author

I tested the rand_r algorithm and it seems to produce values quite uniformly. It did not produce 0. That is why I subtract 1 in rand_r. Not having 0 as a possible return value of rand seemed odd to me.

The maximum value is exported as RAND_MAX = 0x7FFF_FFFC, because that is the largest value rand_r returned (empirically determined).

All in all it seems Good Enough™ for anything non-cryptographic.

@thejpster
Copy link
Member

I thought portable-atomic could emulate CAS using a critical section? Then we'd only need one version of this code and it would never be a race hazard.

@Sympatron
Copy link
Contributor Author

You are right. portable-atomic can emulate CAS. You either have to enable the unsafe-assume-single-core or the critical-section feature. I documented this and added a rand-cs feature similar to signal-cs.

@thejpster
Copy link
Member

thejpster commented Oct 28, 2024

I think it's ok to leave it to the binary to set those flags.

Edit: ok I looked and the rand-cs flag is fine.

src/rand.rs Outdated Show resolved Hide resolved
src/rand.rs Outdated Show resolved Hide resolved
src/rand.rs Outdated Show resolved Hide resolved
@Sympatron
Copy link
Contributor Author

Done.

src/rand.rs Outdated
match RAND_STATE.compare_exchange_weak(
current_state,
new_state,
Ordering::SeqCst,
Copy link
Member

Choose a reason for hiding this comment

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

Is this required? No other accesses are gated on this CAS operation so I think Relaxed is OK.

Copy link
Contributor Author

@Sympatron Sympatron Oct 30, 2024

Choose a reason for hiding this comment

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

Honestly, I am not sure and wanted to play it safe. I basically copied the code from the docs. If you are sure, I can change it though.

Copy link
Member

Choose a reason for hiding this comment

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

Reading https://marabos.nl/atomics/memory-ordering.html I'm pretty sure that Relaxed for both is OK here. We're only modifying that single value, and nothing else related to it. So there are no other writes that must happen strictly before or strictly after we modify this one value.


#[cfg_attr(not(feature = "rand_r"), export_name = "tinyrlibc_RAND_MAX")]
#[cfg_attr(feature = "rand_r", no_mangle)]
pub static RAND_MAX: c_int = 0x7FFF_FFFC as _;
Copy link
Member

Choose a reason for hiding this comment

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

This should be odd.

As you're pulling 31 random bits, I'm pretty sure this should be 0x7fff_ffff.

Copy link
Contributor Author

@Sympatron Sympatron Oct 31, 2024

Choose a reason for hiding this comment

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

I tested all 2^32 possible seeds and the value never went over 0x7FFF_FFFC. This seemed strange to me and I didn't really know what to do with RAND_MAX in that case. The original algorithm goes to up 0x7FFF_FFFD, but does not include 0. That is why I subtracted 1, because I thought it would be way more unexpected to never get 0 then to never get 0x7FFF_FFFD.

Since it may be odd to have RAND_MAX not be 2^n - 1, we could either reduce it to 30 bits at 0x3FFF_FFFF or "lie" and set it to 0x7FFF_FFFF which could be bad, because knowing RAND_MAX is necessary if you want to use rand to get a non-biased distribution.

Since tinyrlibc is mainly used by C code, RAND_MAX will probably never be used directly, because in C RAND_MAX is a ´#defineanyway, so this value will not be seen by the code usingrand`.

Copy link
Member

Choose a reason for hiding this comment

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

How many times did you call rand()? The 10 or 11 bits that get pulled should be relatively uniform. It might be worth logging them individually. Then if they are uniform, you should get values from 0 to 2^31 - 1 by combining 31 random bits together.

@thejpster
Copy link
Member

thejpster commented Nov 1, 2024

OK, I did some digging on this one.

  1. Turns out rand() isn't required to be thread safe. So we don't actually need portable-atomic - a load/store pair is fine. It'll be no worse than a C implementation.

  2. Also, newlib sets RAND_MAX to INT_MAX - and handles both 16-bit and 32-bit ints. I propose we do the same.

  3. The algorithm proposed produces what look like uniformly random bits

    • To confirm, I took 15 bits from the top 16 bits, sampled 10 million times, and every value from 0 to 32767 was produced roughly the same number of times, with no missing values.
    • Therefore it should be fine do do that three times and combine the results into a random value from 0..(1<<31)-1.

@thejpster
Copy link
Member

See https://github.com/rust-embedded-community/tinyrlibc/tree/updated-rand as an example of what I was thinking.

@thejpster
Copy link
Member

This went quiet, so I opened a PR with my proposed updates: #32

@Sympatron
Copy link
Contributor Author

I tried to optimized it for a while, but wasn't 100% happy with the results. I will test your version this week.

@jonathanpallant
Copy link
Collaborator

The other PR was approved. If you'd like some changes, feel free to open another PR.

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