[Haskell-cafe] dangerous inlinePerformIO in Data.Binary(?)

[Donald Bruce Stewart]

Moved to libraries@

u.stenzel:
> Greetings,
> 
> I was trying to understand the magic inside Data.Binary, and found two
> somewhat suspicious uses of inlinePerformIO, which imho has a far too
> innocuous name:
> 
> | toLazyByteString :: Builder -> L.ByteString
> | toLazyByteString m = S.LPS $ inlinePerformIO $ do
> |     buf <- newBuffer defaultSize
> |     return (runBuilder (m `append` flush) (const []) buf)
> 
> Why is this safe?  Considering the GHC implementation of IO, isn't there
> a real danger that 'newBuffer defaultSize' is floated out and therefore
> every invocation of 'toLazyByteString' starts out with the same buffer?
> Isn't that exactly the reason why unsafePerformIO and runST are declared
> NOINLINE?

Yes, that breaks the first rule of inlinePerformIO:

    * don't use it around code that directly allocates.

I've submitted a patch to use unsafePerformIO here.

> The other occurence is:
> 
> | unsafeLiftIO :: (Buffer -> IO Buffer) -> Builder
> | unsafeLiftIO f =  Builder $ \ k buf -> inlinePerformIO $ do
> |     buf' <- f buf
> |     return (k buf')
> 
> which might be safe, since 'f buf' cannot float out of the lambda which
> binds 'buf', but still, all this stuff is inlined, something constant
> might get plugged in the place of buf and the result might be floated
> out to give us an even harder to find Heisenbug.

And this use is indeed also critical for performance, its how we use
direct cascades of FFI writes to fill buffers in a an ST style.

> Am I missing something and this is actually safe?  If not, what can be

We belive the latter use is safe, but it's been 6 months since I've
thought about this.  

Duncan, you've thought a bit more about this recently. Any rules we can
use here?

> done to avoid such errors?  I'd really hate to find building blocks that
> crumble under pressure in standard libraries...

-- Don