`-fno-state-hack` produces incorrect results in nofib

mentioned in issue #7411

changed milestone to %8.6.1

changed weight to 5

added Tbug Trac import labels

assigned to @tdammers

Running only nofib with -fno-state-hack, but with GHC and libraries compiled normally, the problem does not appear.

Two more test runs:

GHC with state hack, libraries without state hack, nofib without state hack triggers problem
GHC with state hack, libraries without state hack, nofib with state hack does not trigger problem

So apparently, in order to trigger the problem, both nofib and libraries must be compiled with -fno-state-hack.

So here's a hint. The documentation to unsafeUseAsCString mentions this:

After calling this function the CString shares the underlying byte buffer with the original ByteString. Thus **modifying the CString**, either in C, or **using poke**, will cause the contents of the ByteString to change, **breaking referential transparency**. Other ByteStrings created by sharing (such as those produced via 'take' or 'drop') will also reflect these changes. Modifying the CString will break referential transparency. To avoid this, use useAsCString, which makes a copy of the original ByteString.

(emphasis mine).

And then in the fasta/Main.hs source code, we see this:

  unsafeUseAsCString line $ \ptr -> do
    --- snip ---
    plusPtr ptr i `poke` unsafeIndex lookupTable newseed
    --- snip ---

In other words, fasta does exactly what the documentation says not to do.

Which would suggest that the state hack itself isn't really to blame, it just causes different sharing behavior.

I'll run a few tests to get more certainty here.

Yikes! Indeed that does look quite suspicious since line is also used elsewhere in this block. Sounds like this is the culprit.

I quickly and naively tried just using useAsCString instead of unsafeUseAsCString, but of course that doesn't quite work - probably because the code relies on actually manipulating the bytestring in place, whereas useAsCString creates a copy and manipulates that. So the output no longer matches that of the C implementation, but the -fno-state-hack flag no longer causes the output to differ.

So I went a step further, and rewrote the relevant part in more idiomatic Haskell, using ByteString.unfoldrN instead of manipulating C strings in-place:

commit dc2753f4e10fec79c02ed293b72573f1aeaa2271
Author: Tobias Dammers <tdammers@gmail.com>
Date:   Fri Jun 15 11:27:36 2018 +0200

    Rewrite fasta in more idiomatic Haskell

diff --git a/shootout/fasta/Main.hs b/shootout/fasta/Main.hs
index 4bd0849..795a470 100644
--- a/shootout/fasta/Main.hs
+++ b/shootout/fasta/Main.hs
@@ -11,7 +11,9 @@ import Foreign.Ptr
 import Foreign.Storable
 import System.Environment
 import qualified Data.ByteString.Char8 as B
+import qualified Data.ByteString as BS
 import qualified Data.ByteString.Lazy.Char8 as L
+import Data.Word (Word8)
 
 main = do
     n <- getArgs >>= readIO.head
@@ -27,21 +29,30 @@ make name n0 tbl seed0 = do
   B.putStrLn name
   let modulus = 139968
       fill ((c,p):cps) j =
-	let !k = min modulus (floor (fromIntegral modulus * (p::Float) + 1))
-	in B.replicate (k - j) c : fill cps k
+        let !k = min modulus (floor (fromIntegral modulus * (p::Float) + 1))
+        in B.replicate (k - j) c : fill cps k
       fill _ _ = []
       lookupTable = B.concat $ fill (scanl1 (\(_,p) (c,q) -> (c,p+q)) tbl) 0
-      line = B.replicate 60 '\0'
-  unsafeUseAsCString line $ \ptr -> do
-    let make' n !i seed
-	    | n > (0::Int) = do
-		let newseed = rem (seed * 3877 + 29573) modulus
-		plusPtr ptr i `poke` unsafeIndex lookupTable newseed
-		if i+1 >= 60
-		    then puts line 60 >> make' (n-1) 0 newseed
-		    else make' (n-1) (i+1) newseed
-	    | otherwise = when (i > 0) (puts line i) >> return seed
-    make' n0 0 seed0
+
+  let next :: Int -> Maybe (Word8, Int)
+      next seed =
+        let newseed = rem (seed * 3877 + 29573) modulus
+            val = unsafeIndex lookupTable newseed
+        in Just (val, newseed)
+
+  let make' :: Int -> Int -> IO Int
+      make' n seed = do
+        if n <= (0 :: Int) then
+          return seed
+        else if n >= 60 then do
+          let (line, Just newseed) = BS.unfoldrN 60 next seed
+          puts line 60
+          make' (n-60) newseed
+        else do
+          let (line, Just newseed) = BS.unfoldrN n next seed
+          puts line n
+          return newseed
+  make' n0 seed0
 
 alu = "GGCCGGGCGCGGTGGCTCACGCCTGTAATCCCAGCACTTTGGGAGGCCGAGGCGGGCGGATCACCTGAGG\
     \TCAGGAGTTCGAGACCAGCCTGGCCAACATGGTGAAACCCCGTCTCTACTAAAAATACAAAAATTAGCCGGG\

The patched version produces the correct output regardless of the state hack; however, I haven't done a full profiling run yet to see how it affects performance.

However, considering that the unidiomatic version is essentially wrong, and, well, unidiomatic, I'm not so sure whether it told us anything meaningful in the first place.

Trac metadata

Trac field	Value
Differential revisions	- → D4868

closed

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

added incorrect runtime result label

added Pnormal label

Trac field	Value
Version	8.5
Type	Bug
TypeOfFailure	OtherFailure
Priority	normal
Resolution	Unresolved
Component	Compiler
Test case
Differential revisions
BlockedBy
Related
Blocking
CC
Operating system
Architecture

`-fno-state-hack` produces incorrect results in nofib

Child items 0

Activity

`-fno-state-hack` produces incorrect results in nofib

Relates to

Activity