

<HTML><BODY><div>Hi,</div><div> </div><div>Thanks for your review!</div><div> </div><div>Sent v4 considering the comments.</div><div>Some answers below.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 3 ноября 2020, 0:38 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16043531240184730840_BODY">Hi! Thanks for the patch!<br><br>Almost finished!<br><br>See 5 comments below.<br><br>> diff --git a/src/lib/core/random.c b/src/lib/core/random.c<br>> index f4fa75b1c..cfca2b7a9 100644<br>> --- a/src/lib/core/random.c<br>> +++ b/src/lib/core/random.c<br>> @@ -97,3 +102,85 @@ rand:<br>> while (generated < size)<br>> buf[generated++] = rand();<br>> }<br>> +<br>> +uint64_t<br>> +real_random(void)<br>> +{<br>> + uint64_t result;<br>> + random_bytes((char *)(&result), sizeof(result));<br><br>1. Why do you need () around &result?</div></div></div></div></blockquote><div>Right, no need here, changed in v4.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> + return result;<br>> +}<br>> +<br>> +uint64_t *<br>> +xoshiro_state(void)<br>> +{<br>> + return state;<br>> +}<br>> +<br>> +size_t<br>> +xoshiro_state_size(void)<br>> +{<br>> + return sizeof(state) / sizeof(state[0]);<br>> +}<br><br>2. Or instead of these two functions it could be simpler<br>to implement xoshiro_state_str(), which would return<br>its 4 uint64 numbers as tt_sprintf(). Anyway we need to<br>print the state in a human readable format.</div></div></div></div></blockquote><div>Ok, right, i wanted to make it more multi-purpose, but actually</div><div>looks like no one need internal xoshiro state out there. So the</div><div>string will be enough for cases where it is needed to be printed.</div><div>Changed in v4.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +<br>> +int64_t<br>> +real_random_in_range(int64_t min, int64_t max)<br>> +{<br>> + assert(max >= min);<br>> + uint64_t range = (uint64_t)max - min;<br><br>3. This is UB, if max is negative.</div></div></div></div></blockquote><div>Signed to unsigned int cast is not UB.</div><div>Here is the quote from C99 standard (same in C11):</div><div>6.3.1.3 Signed and unsigned integers</div><div>1 When a value with integer type is converted to another</div><div>integer type other than _Bool, if the value can be represented</div><div>by the new type, it is unchanged.</div><div>2 Otherwise, if the new type is unsigned, the value is converted</div><div>by repeatedly adding or subtracting one more than the maximum value</div><div>that can be represented in the new type until the value is in the range</div><div>of the new type.</div><div>3 Otherwise, the new type is signed and the value cannot be</div><div>represented in it; either the result is implementation-defined or</div><div>an implementation-defined signal is raised.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> + uint64_t mask = UINT64_MAX >> bit_clz_u64(range | 1);<br>> + uint64_t result;<br>> + do {<br>> + result = real_random() & mask;<br>> + } while (result > range);<br>> + return min + result;<br>> +}<br>> +<br>> +int64_t<br>> +pseudo_random_in_range(int64_t min, int64_t max)<br>> +{<br>> + assert(max >= min);<br>> + uint64_t range = (uint64_t)max - min;<br>> + uint64_t mask = UINT64_MAX >> bit_clz_u64(range | 1);<br>> + uint64_t result;<br>> + do {<br>> + result = xoshiro_random() & mask;<br>> + } while (result > range);<br>> + return min + result;<br>> +}<br>> diff --git a/test/unit/random.c b/test/unit/random.c<br>> new file mode 100644<br>> index 000000000..3d7416988<br>> --- /dev/null<br>> +++ b/test/unit/random.c<br>> @@ -0,0 +1,48 @@<br>> +#include <core/random.h><br>> +<br>> +#include <stdio.h><br>> +#include <assert.h><br>> +<br>> +#include "unit.h"<br>> +<br>> +static void<br>> +test_xoshiro_random_in_range_one(int64_t min, int64_t max)<br>> +{<br>> + int64_t result = pseudo_random_in_range(min, max);<br>> + assert(min <= result && result <= max);<br>> + if (min == max)<br>> + printf("pseudo_random_in_range(%lld, %lld) = %lld\n",<br>> + (long long)min, (long long)max, (long long)result);<br>> +}<br><br>4. Why no tests for real random?</div></div></div></div></blockquote><div>My bad, added in v4.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> diff --git a/test/unit/swim_test_utils.c b/test/unit/swim_test_utils.c<br>> index 9dbd28a9f..590ce97a1 100644<br>> --- a/test/unit/swim_test_utils.c<br>> +++ b/test/unit/swim_test_utils.c<br>> @@ -875,7 +875,15 @@ swim_run_test(const char *log_file, fiber_func test)<br>> * Print the seed to be able to reproduce a bug with the<br>> * same seed.<br>> */<br>> - say_info("Random seed = %llu", (unsigned long long) seed);<br>> + say_info("Random seed = %llu", (unsigned long long)seed);<br>> + uint64_t *state = xoshiro_state();<br>> + char state_buf[100] = "";<br>> + int filled = 0;<br>> + for (size_t i = 0; i < xoshiro_state_size(); i++)<br>> + filled += snprintf(state_buf + filled,<br>> + sizeof(state_buf) - filled,<br>> + " %llu", (unsigned long long)state[i]);<br>> + say_info("xoshiro random state =%s", state_buf);<br><br>5. snprintf() returns number of bytes that would have been<br>printed if the buffer would be long enough. It means, that<br>"sizeof(state_buf) - filled" may become negative with such<br>accumulation. If state size will even become bigger,<br>for example. And since snprintf()'s second argument is<br>size_t, it will overflow to a huge value, leading to the<br>stack corruption.<br><br>Better return a string on tt_sprintf() right from random.c,<br>so as not to care about snprintf specifics in the tests.</div></div></div></div></blockquote><div>All right, fixed in v4.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>><br>> struct fiber *main_fiber = fiber_new("main", test);<br>> fiber_set_joinable(main_fiber, true);<br>></div></div></div></div></blockquote><div> <div> <div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Ilya Kosarev</div></div></div></div></div></BODY></HTML>