From 39b15b9b4d147f6001984c4b7edab00878269da7 Mon Sep 17 00:00:00 2001 From: Nikos Baxevanis Date: Wed, 23 May 2018 06:39:35 +0300 Subject: [PATCH] Avoid weak gamma values in Hedgehog.Internal.Seed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See also: - http://www.pcg-random.org/posts/bugs-in-splitmix.html - https://github.com/hedgehogqa/haskell-hedgehog/issues/191 - http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/SplittableRandom.java In http://www.pcg-random.org/posts/bugs-in-splitmix.html, most of the output is horribly formatted. So I went on and formatted the important part of the output from that article, where it highlights the bug: >``` >Prelude System.Random.SplitMix> map mkSMGen > [ 0x61c8864680b583eb > , 0xf8364607e9c949bd > , 0x88e48f4fcc823718 > , 0x7f83ab8da2e71dd1 > , 0x7957d809e827ff4c > , 0xf8d059aee4c53639 > , 0x9cd9f015db4e58b7 > , 0xf4077b0dbebc73c0 > , 0x305cb877109d0686 > , 0x359e58eeafebd527 > , 0xbeb721c511b0da6d > , 0x86466fd0fcc363a6 > , 0xefee3e7b93db3075 > , 0x79629ee76aa83059 > , 0x05d507d05e785673 > , 0x76442b62dddf926c ] > > [ SMGen {_seed = 16269636050940713694, _gamma = 1} > , SMGen {_seed = 11331281847540010644, _gamma = 1} > , SMGen {_seed = 5619286150931196027 , _gamma = 1} > , SMGen {_seed = 10281499533683189089, _gamma = 1} > , SMGen {_seed = 17485363145453140727, _gamma = 7} > , SMGen {_seed = 17937441790592959953, _gamma = 7} > , SMGen {_seed = 15529190628949695919, _gamma = 7} > , SMGen {_seed = 6957216782316743206 , _gamma = 7} > , SMGen {_seed = 13022414457527836317, _gamma = 15} > , SMGen {_seed = 2127073997383595602 , _gamma = 15} > , SMGen {_seed = 7685868475082380828 , _gamma = 15} > , SMGen {_seed = 6396586367974718702 , _gamma = 15} > , SMGen {_seed = 13838712901582905874, _gamma = 31} > , SMGen {_seed = 15464227283579875280, _gamma = 31} > , SMGen {_seed = 17142043965827948305, _gamma = 31} > , SMGen {_seed = 10266498716653543189, _gamma = 31} ] >Prelude System.Random.SplitMix> >``` It compares just *one* implementation of SplitMix, [this](https://github.com/phadej/splitmix/blob/f6895c4137b14e4752b0575c178199201eb41810/src/System/Random/SplitMix.hs) one, at some particular point in time. (I can see that the author went and fixed it recently as shown from the diff-output [here](https://github.com/phadej/splitmix/commit/bd0692ba0c2dcf51b92f97d97142c1eae4fa0efc) and [here](https://github.com/phadej/splitmix/commit/c6ba2f375394ab9ec3747cbd6941ee559be98835), so this means that now *The Bug* section of http://www.pcg-random.org/posts/bugs-in-splitmix.html is outdated.) --- Here's the exact same test with *Hedgehog.Internal.Seed*: ``` λ :t from from :: GHC.Word.Word64 -> Seed λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 7046029254386353131 11400714819323198485 , Seed 17885559969949501885 11400714819323198485 , Seed 9864166656744503064 11400714819323198485 , Seed 9188376289577737681 11400714819323198485 , Seed 8743694738624347980 11400714819323198485 , Seed 17928928724259255865 11400714819323198485 , Seed 11302328716527294647 11400714819323198485 , Seed 17584158569056203712 11400714819323198485 , Seed 3484863033197266566 11400714819323198485 , Seed 3863623312507393319 11400714819323198485 , Seed 13742489918233434733 11400714819323198485 , Seed 9675543792836633510 11400714819323198485 , Seed 17288824720004427893 11400714819323198485 , Seed 8746728143070965849 11400714819323198485 , Seed 420250731748546163 11400714819323198485 , Seed 8521984098521027180 11400714819323198485 ] λ ``` This looks weird, as I'm getting the *exact same γ* through all the seeds. If I change Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214): ```diff λ :! git diff diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs index 61cacb2..fe1a0be 100644 --- a/hedgehog/src/Hedgehog/Internal/Seed.hs +++ b/hedgehog/src/Hedgehog/Internal/Seed.hs @@ -98,11 +98,11 @@ random = -- | Create a 'Seed' using a 'Word64'. -- from :: Word64 -> Seed from x = - Seed x goldenGamma + Seed (mix64 x) (mixGamma (x + goldenGamma)) λ ``` I can now reproduce the bug [*exactly* as shown in the article](http://www.pcg-random.org/posts/bugs-in-splitmix.html#the-bug): ``` λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 15210016002011668638 1 , Seed 11409286845259996466 1 , Seed 1931727433621677744 1 , Seed 307741759840609752 1 , Seed 8606169619657412120 7 , Seed 13651108307767328632 7 , Seed 125750466559701114 7 , Seed 6781260234005250507 7 , Seed 15306535823716590088 15 , Seed 7344074043290227165 15 , Seed 9920554987610416076 15 , Seed 3341781972484278810 15 , Seed 12360157267739240775 31 , Seed 600595566262245170 31 , Seed 1471112649570176389 31 , Seed 8100917074368564322 31 ] λ ``` This is actually great so far, as it points out that we got Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) wrong and we can now 'fix' it according to the 'reference' source. (At this point, I can tell that the only reliable source is the one found in Oracle's JDK8 implementation.) --- And finally, If I go and change the glorious Hedgehog.Internal.Seed [`mixGamma`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L202) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214) (essentially, reverting #124): ```diff λ :! git diff diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs index 61cacb2..5428927 100644 --- a/hedgehog/src/Hedgehog/Internal/Seed.hs +++ b/hedgehog/src/Hedgehog/Internal/Seed.hs @@ -100,7 +100,7 @@ random = -- from :: Word64 -> Seed from x = - Seed x goldenGamma + Seed (mix64 x) (mixGamma (x + goldenGamma)) -- | A predefined gamma value's needed for initializing the "root" instances of -- 'Seed'. That is, instances not produced by splitting an already existing @@ -192,7 +192,7 @@ mixGamma x = y = mix64variant13 x .|. 1 n = popCount $ y `xor` (y `shiftR` 1) in - if n >= 24 then + if n < 24 then y `xor` 0xaaaaaaaaaaaaaaaa else y λ ``` It 'fixes' the gamma value: ``` λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 15210016002011668638 12297829382473034411 , Seed 11409286845259996466 12297829382473034411 , Seed 1931727433621677744 12297829382473034411 , Seed 307741759840609752 12297829382473034411 , Seed 8606169619657412120 12297829382473034413 , Seed 13651108307767328632 12297829382473034413 , Seed 125750466559701114 12297829382473034413 , Seed 6781260234005250507 12297829382473034413 , Seed 15306535823716590088 12297829382473034405 , Seed 7344074043290227165 12297829382473034405 , Seed 9920554987610416076 12297829382473034405 , Seed 3341781972484278810 12297829382473034405 , Seed 12360157267739240775 12297829382473034421 , Seed 600595566262245170 12297829382473034421 , Seed 1471112649570176389 12297829382473034421 , Seed 8100917074368564322 12297829382473034421 ] λ ``` --- hedgehog/src/Hedgehog/Internal/Seed.hs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs index 542b275b..b224331c 100644 --- a/hedgehog/src/Hedgehog/Internal/Seed.hs +++ b/hedgehog/src/Hedgehog/Internal/Seed.hs @@ -107,7 +107,7 @@ random = -- from :: Word64 -> Seed from x = - Seed x goldenGamma + Seed (mix64 x) (mixGamma (x + goldenGamma)) -- | A predefined gamma value's needed for initializing the "root" instances of -- 'Seed'. That is, instances not produced by splitting an already existing @@ -199,7 +199,7 @@ mixGamma x = y = mix64variant13 x .|. 1 n = popCount $ y `xor` (y `shiftR` 1) in - if n >= 24 then + if n < 24 then y `xor` 0xaaaaaaaaaaaaaaaa else y