Re: [PATCH 06/10] siphash: Use more hygienic state initialiser

On Thu, 28 Sep 2023 11:20:21 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  On Wed, Sep 27, 2023 at 07:04:50PM +0200, Stefano
Brivio wrote:
  On Sat, 23 Sep 2023 00:06:26 +1000
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
   
  The PREAMBLE macro sets up the SipHash initial
internal state.  It also
 defines that state as a variable, which isn't macro hygeinic.  With
 previous changes simplifying this premable, it's now possible to replace it
 with a macro which simply expands to a C initialisedrfor that state.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  siphash.c | 29 ++++++++++++-----------------
  1 file changed, 12 insertions(+), 17 deletions(-)
 
 diff --git a/siphash.c b/siphash.c
 index 6932da2..21c560d 100644
 --- a/siphash.c
 +++ b/siphash.c
 @@ -58,15 +58,12 @@
  
  #define ROTL(x, b) (uint64_t)(((x) << (b)) | ((x) >> (64 - (b))))
  
 -#define PREAMBLE							  \
 -	uint64_t v[4] = { 0x736f6d6570736575ULL, 0x646f72616e646f6dULL,	  \
 -			  0x6c7967656e657261ULL, 0x7465646279746573ULL }; \
 -	int __i;							  \
 -									  \
 -	do {								  \
 -		for (__i = sizeof(v) / sizeof(v[0]) - 1; __i >= 0; __i--) \
 -			v[__i] ^= k[__i % 2];				  \
 -	} while (0)
 +#define SIPHASH_INIT(k) {						\
 +		0x736f6d6570736575ULL ^ (k)[0],				\
 +		0x646f72616e646f6dULL ^ (k)[1],				\
 +		0x6c7967656e657261ULL ^ (k)[0],				\
 +		0x7465646279746573ULL ^ (k)[1]				\   
 
 I don't think it actually matters (given the rationale for the choice
 of these constants given in the paper), but earlier this was equivalent
 to:
 
   0x736f6d6570736575ULL ^ (k)[1],
   0x646f72616e646f6dULL ^ (k)[0],
   0x6c7967656e657261ULL ^ (k)[1],
   0x7465646279746573ULL ^ (k)[0]   
 
 No... I don't think it was.  We had:
 	for (__i = sizeof(v) / sizeof(v[0]) - 1; __i >= 0; __i--)                            
                   
 		v[__i] ^= k[__i % 2];
 
 So in the first iteration __i == 3, so we get v[3] ^= k[1], and v[3]
 is 0x7465646279746573. 
Oops, sorry, I missed the fact that I was actually starting from the
end of the array.

...
 
  and it matched
both reference implementations linked in the file
 header.   
 
 Again, I don't think that's correct.  In
 https://github.com/veorq/SipHash.git we have:
 	v3 ^= k1;
 	v2 ^= k0;
 	v1 ^= k1;
 	v0 ^= k0;
 
 In both cases the order of operations is reversed, but since they're
 independent that doesn't matter.  But the point is that the reference
 implementation has v0 <-> k0 and v3 <-> k1, rather than the other way
 around. 
Right.

-- 
Stefano