[v2,4/6] Do not use HP_TIMING_NOW for random bits

Hi Adhemerval,

> +# include <random-bits.h>
> +# define RANDOM_BITS(Var) ((Var) = random_bits ())
> 
> This define is not used (removed above).

> I think we need to still define it if we eventually decide to sync it back
> to gnulib.

Well this is the question - do we really need all this clutter just for gnulib?
It looks to me we should keep the code as clean as possible (so we don't
need any the !_LIBC code given these files are always in LIBC).

> I fact the new line should not be added, since random_time_bits should already
> get the random_bits() value. In any case I think we can remove random_time_bits
> altogether and just call RANDOM_BITS on value instead.

Agreed.

> And it seems 'value' is static by design, but I do agree there is no impeding
> reason to continue to do so.

Indeed.

---
these changes to gnulib, we could just add random_bits to gnulib.


-  value += random_time_bits ^ __getpid ();
-  value += random_bits () ^ __getpid ();
+  RANDOM_BITS (value);
+  value ^= __getpid ();
+  /* Shuffle the lower bits to minimize the pid bias due low maximum value.  */
+  value = (value << 24) | (value >> 8);

random_bits already does that shuffle, so doing it again doesn't help. It's better
to avoid the aliasing of getpid with the random bits, eg. value ^= __get_pid << 32
so we end up with more than 32 random bits.

Wilco

On 20/03/2019 12:32, Wilco Dijkstra wrote:
> Hi Adhemerval,
> 
>> +# include <random-bits.h>
>> +# define RANDOM_BITS(Var) ((Var) = random_bits ())
>>
>> This define is not used (removed above).
> 
>> I think we need to still define it if we eventually decide to sync it back
>> to gnulib.
> 
> Well this is the question - do we really need all this clutter just for gnulib?
> It looks to me we should keep the code as clean as possible (so we don't
> need any the !_LIBC code given these files are always in LIBC).

I think we can address is in a subsequent patch, to either sync it with gnulib
or just cleanup the non required parts.

> 
>> I fact the new line should not be added, since random_time_bits should already
>> get the random_bits() value. In any case I think we can remove random_time_bits
>> altogether and just call RANDOM_BITS on value instead.
> 
> Agreed.
> 
>> And it seems 'value' is static by design, but I do agree there is no impeding
>> reason to continue to do so.
> 
> Indeed.
> 
> ---
> diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
> index 5217cb38e1..d062e4b82f 100644
> --- a/sysdeps/posix/tempname.c
> +++ b/sysdeps/posix/tempname.c
> @@ -73,6 +73,13 @@
>  #ifdef _LIBC
>  # include <random-bits.h>
>  # define RANDOM_BITS(Var) ((Var) = random_bits ())
> +# else
> +# define RANDOM_BITS(Var) \
> +    {                                                                         \
> +      struct timeval tv;                                                      \
> +      __gettimeofday (&tv, NULL);                                             \
> +      (Var) = ((uint64_t) tv.tv_usec << 16) ^ tv.tv_sec;                      \
> +    }
>  #endif
>  
> I don't see the point of this, especially using gettimeofday. If we want to export
> these changes to gnulib, we could just add random_bits to gnulib.

From gnulib doc doc/posix-functions/clock_gettime.texi, clock_gettime is missing
on some platforms (OS X 10.11, Minix 3.1.8, IRIX 5.3, Solaris 2.4, mingw, MSVC 14,
Interix 3.5, BeOS) and implements gettime module using gettimeofday. No sure how
gnulib would want to to implement it, or if it is willing to add another module
to provide random_bits.

In any case I think best course of action to still use the RANDOM_BITS with the
fallback case using gettimeofday (even if it is not actively used on glibc) and
once it is upstream I will send a patch to cleanup this and check with Paul Eggert
what gnulib would do (deviate from glibc or implement something random_bits).

> 
> 
> -  value += random_time_bits ^ __getpid ();
> -  value += random_bits () ^ __getpid ();
> +  RANDOM_BITS (value);
> +  value ^= __getpid ();
> +  /* Shuffle the lower bits to minimize the pid bias due low maximum value.  */
> +  value = (value << 24) | (value >> 8);
> 
> random_bits already does that shuffle, so doing it again doesn't help. It's better
> to avoid the aliasing of getpid with the random bits, eg. value ^= __get_pid << 32
> so we end up with more than 32 random bits.

Alright I change it locally to ^= __getpid () << 32;