Opened 3 years ago

Closed 3 years ago

#7270 closed bug (fixed)

Incorrect optimization with Data.ByteString.append

Reported by: ocheron Owned by: duncan
Priority: highest Milestone: 7.6.2
Component: libraries (other) Version: 7.6.1
Keywords: Cc: hackage.haskell.org@…, conrad@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect result at runtime Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

The following program does not give the same result with -O2 and without:

import qualified Data.ByteString as B

main = let a = B.singleton 65 in print (func a a)

func y z = B.append r s
  where
    r = B.map (succ) x
    s = B.map (succ . succ) x
    x = B.append y z

Result observed with GHC 7.6.1 (bytestring-0.10.0.0):

$ ghc --make test -fforce-recomp && ./test
[1 of 1] Compiling Main             ( test.hs, test.o )
Linking test ...
"BBCC"
$ ghc --make test -fforce-recomp -O2 && ./test
[1 of 1] Compiling Main             ( test.hs, test.o )
Linking test ...
"CCCC"

Change History (12)

comment:1 Changed 3 years ago by tab

I can also reproduce without append but using the shorter func:

func x = s `seq` r
  where
    r = B.map succ x
    s = B.map succ r

it fails at O and O2 on ghc 7.6.

comment:2 Changed 3 years ago by igloo

  • difficulty set to Unknown
  • Owner set to igloo

comment:3 Changed 3 years ago by igloo

  • Milestone set to 7.6.2
  • Owner changed from igloo to duncan
  • Priority changed from normal to highest

Thanks for the report. This is due to bytestring's misuse of its inlinePerformIO; Duncan knows what to do.

Here's a small standalone program demonstrating the problem:

{-# LANGUAGE BangPatterns, MagicHash, UnboxedTuples #-}

module Main (main) where

import GHC.Base
import GHC.ForeignPtr
import Foreign

main :: IO ()
main = do let !r = make 68
              !s = make 70
          dump r
          dump s

dump :: ForeignPtr Word8 -> IO ()
dump fp = do print fp
             withForeignPtr fp $ \p -> do x <- peek p
                                          print x

make :: Word8 -> ForeignPtr Word8
make w = inlinePerformIO $ do fp <- mallocPlainForeignPtrBytes 1
                              withForeignPtr fp $ \p -> poke p w
                              return fp

inlinePerformIO :: IO a -> a
inlinePerformIO (IO m) = case m realWorld# of (# _, r #) -> r
$ ghc --make q -O
$ ./q
0x00007f95b0706010
68
0x00007f95b0706010
68

comment:4 follow-up: Changed 3 years ago by duncan

  • Resolution set to fixed
  • Status changed from new to closed

Committed to the upstream bytestring repo. So it'll be fixed in the next bytestring point release.

Tue Sep 25 16:21:14 BST 2012  Duncan Coutts <[email protected]>
  * Fix a few incorrect uses of inlinePerformIO
  The incorrect use of inlinePerformIO resulted in multiple calls to
  mallocByteString being shared, and hence two different strings sharing
  the same memory. See http://hackage.haskell.org/trac/ghc/ticket/7270
  
  Consider:
    foo x = s `seq` r
      where
        r = B.map succ x
        s = B.map succ r
  
  The B.map function used a pattern like:
    map f (PS fp s len) = inlinePerformIO $ ...
       fp' <- mallocByteString len
  
  Now, in the foo function above, we have both r and s where the compiler
  can see that the 'len' is the same in both cases, and with
  inlinePerformIO exposing everything, the compiler is free to share the
  two calls to mallocByteString len.
  
  The answer is not to use inlinePerformIO if we are allocating, but to use
  unsafeDupablePerformIO instead. Another reminder that inlinePerformIO is
  really really unsafe, and that we should be using the ST monad instead.

comment:5 Changed 3 years ago by igloo

  • Owner duncan deleted
  • Resolution fixed deleted
  • Status changed from closed to new

We should make sure this goes into the next GHC release.

comment:6 Changed 3 years ago by igloo

  • Owner set to igloo

comment:7 in reply to: ↑ 4 Changed 3 years ago by ocheron

Thank you, my initial problem (with the package tls-0.9.10) is solved with this patch applied on top of bytestring-0.10.0.0. Though I had to fix imports in Data/ByteString.hs: unsafeDupablePerformIO seems to be missing.

comment:8 Changed 3 years ago by igloo

  • Owner changed from igloo to duncan

Right, the patch doesn't build for me either, in either a GHC tree or as a standalone package:

[ 7 of 21] Compiling Data.ByteString  ( Data/ByteString.hs, dist/build/Data/ByteString.o )

Data/ByteString.hs:488:23: Not in scope: `unsafeDupablePerformIO'

Data/ByteString.hs:679:33: Not in scope: `unsafeDupablePerformIO'

Data/ByteString.hs:699:33: Not in scope: `unsafeDupablePerformIO'

Data/ByteString.hs:728:27: Not in scope: `unsafeDupablePerformIO'

Data/ByteString.hs:759:27: Not in scope: `unsafeDupablePerformIO'

Data/ByteString.hs:1512:38: Not in scope: `unsafeDupablePerformIO'

Duncan, could you take a look please?

comment:9 Changed 3 years ago by liyang

  • Cc hackage.haskell.org@… added

comment:10 Changed 3 years ago by conrad

  • Cc conrad@… added

comment:11 Changed 3 years ago by duncan

  • Status changed from new to merge

Oops, dunno how that happened. Fixed.

comment:12 Changed 3 years ago by igloo

  • Resolution set to fixed
  • Status changed from merge to closed

Merged

Note: See TracTickets for help on using tickets.