ghc 8.4.1 and trac 13930

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

ghc 8.4.1 and trac 13930

Evan Laforge
I recently noticed that with -O1, ghc was optimizing some code

    if Trace.traceShowId "b" $ True
        then return $ Left $ Serialize.BadMagic (Serialize.magicBytes
magic) file_magic
        else first Serialize.UnserializeError <$> Exception.evaluate
(Serialize.decode rest)

to always evaluate the 'else' branch.  This is fixed in 8.4.2, so I'm
pretty sure it's https://ghc.haskell.org/trac/ghc/ticket/13930

I assume there's no point trying to get a minimal reproduction, since
it's a known issue and fixed.  Still,
https://downloads.haskell.org/~ghc/8.4.2/docs/html/users_guide/8.4.2-notes.html
says "incorrectly optimised, resulting in space leaks".

Maybe it should instead say "incorrectly optimised, resulting in
taking the wrong branch in 'if' expressions"?  That's a bit more
alarming, and is a stronger "upgrade to 8.4.2" signal.
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: ghc 8.4.1 and trac 13930

Ben Gamari-2
Evan Laforge <[hidden email]> writes:

> I recently noticed that with -O1, ghc was optimizing some code
>
>     if Trace.traceShowId "b" $ True
>         then return $ Left $ Serialize.BadMagic (Serialize.magicBytes
> magic) file_magic
>         else first Serialize.UnserializeError <$> Exception.evaluate
> (Serialize.decode rest)
>
> to always evaluate the 'else' branch.  This is fixed in 8.4.2, so I'm
> pretty sure it's https://ghc.haskell.org/trac/ghc/ticket/13930
>
> I assume there's no point trying to get a minimal reproduction, since
> it's a known issue and fixed.  Still,
> https://downloads.haskell.org/~ghc/8.4.2/docs/html/users_guide/8.4.2-notes.html
> says "incorrectly optimised, resulting in space leaks".
>
> Maybe it should instead say "incorrectly optimised, resulting in
> taking the wrong branch in 'if' expressions"?  That's a bit more
> alarming, and is a stronger "upgrade to 8.4.2" signal.
Yes, I suppose the language in the release notes does rather understate
the degree of the incorrectness.

I'll push a new version of the manual with some stronger language
tomorrow.

Cheers,

- Ben

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ghc 8.4.1 and trac 13930

Evan Laforge
On Tue, May 1, 2018 at 9:58 PM, Ben Gamari <[hidden email]> wrote:
> Yes, I suppose the language in the release notes does rather understate
> the degree of the incorrectness.
>
> I'll push a new version of the manual with some stronger language
> tomorrow.

Good deal, thanks for the quick response.
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

RE: ghc 8.4.1 and trac 13930

GHC - devs mailing list
In reply to this post by Evan Laforge
|  I recently noticed that with -O1, ghc was optimizing some code
|  
|      if Trace.traceShowId "b" $ True
|          then ...
|          else ...
|  
|  to always evaluate the 'else' branch.  

Are you certain this is #13930?  I don't see an obvious connection.  It seems really really terrible to "optimise" True to False!

I think #13930 was fixed by #5129, which in turn was about discarding a call to 'evaluate'.  That is different to turning True to False.

But there's probably some more complicated context to your use-case that means my understanding is faulty.

If you are confident that it's securely fixed, well and good. But when bugs disappear I always worry that they are still there, just concealed by some other change.

Simon


|  -----Original Message-----
|  From: ghc-devs <[hidden email]> On Behalf Of Evan
|  Laforge
|  Sent: 02 May 2018 04:10
|  To: [hidden email]
|  Subject: ghc 8.4.1 and trac 13930
|  
|  I recently noticed that with -O1, ghc was optimizing some code
|  
|      if Trace.traceShowId "b" $ True
|          then return $ Left $ Serialize.BadMagic (Serialize.magicBytes
|  magic) file_magic
|          else first Serialize.UnserializeError <$> Exception.evaluate
|  (Serialize.decode rest)
|  
|  to always evaluate the 'else' branch.  This is fixed in 8.4.2, so I'm
|  pretty sure it's https://ghc.haskell.org/trac/ghc/ticket/13930
|  
|  I assume there's no point trying to get a minimal reproduction, since
|  it's a known issue and fixed.  Still,
|  https://downloads.haskell.org/~ghc/8.4.2/docs/html/users_guide/8.4.2-
|  notes.html
|  says "incorrectly optimised, resulting in space leaks".
|  
|  Maybe it should instead say "incorrectly optimised, resulting in
|  taking the wrong branch in 'if' expressions"?  That's a bit more
|  alarming, and is a stronger "upgrade to 8.4.2" signal.
|  _______________________________________________
|  ghc-devs mailing list
|  [hidden email]
|  http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: ghc 8.4.1 and trac 13930

Evan Laforge
On Wed, May 2, 2018 at 1:24 AM, Simon Peyton Jones
<[hidden email]> wrote:

> |  I recently noticed that with -O1, ghc was optimizing some code
> |
> |      if Trace.traceShowId "b" $ True
> |          then ...
> |          else ...
> |
> |  to always evaluate the 'else' branch.
>
> Are you certain this is #13930?  I don't see an obvious connection.  It seems really really terrible to "optimise" True to False!
>
> I think #13930 was fixed by #5129, which in turn was about discarding a call to 'evaluate'.  That is different to turning True to False.
>
> But there's probably some more complicated context to your use-case that means my understanding is faulty.
>
> If you are confident that it's securely fixed, well and good. But when bugs disappear I always worry that they are still there, just concealed by some other change.

I'm not totally confident, which is I why I asked.  It does seem to be
related to the presence of Exception.evaluate, but it also comes and
goes depending on how many things are in the condition and branches.

It does seem to be gone in 8.4.1, but I'm also a bit nervous when I
don't know exactly why something was fixed.  I'll try to reduce this
to as small an expression as possible that still triggers True ->
False.
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: ghc 8.4.1 and trac 13930

Evan Laforge
Ok, here's a short module:

import qualified Control.Exception as Exception

main :: IO ()
main = do
    unserialize
    putStrLn "all is well"

unserialize :: IO Char
unserialize =
    if definitelyTrue
        then do
            return 'a'
        else do
            Exception.evaluate (error "wrong place")

{-# NOINLINE definitelyTrue #-}
definitelyTrue :: Bool
definitelyTrue = True


When compiled with -O on 8.4.1, this should print "wrong place".
Without -O, or with 8.4.2, or if True can be inlined, or without
evaluate, all is well.
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

RE: ghc 8.4.1 and trac 13930

GHC - devs mailing list
Wow.  Could you open a ticket?

I just tried with 8.2.2 which is what I have on this laptop, but it printed "all is well".  Does that mean it was fine in 8.2, went wrong in 8.4.1 and was fixed in 8.4.2?

Simon

| -----Original Message-----
| From: Evan Laforge <[hidden email]>
| Sent: 02 May 2018 19:39
| To: Simon Peyton Jones <[hidden email]>
| Cc: [hidden email]
| Subject: Re: ghc 8.4.1 and trac 13930
|
| Ok, here's a short module:
|
| import qualified Control.Exception as Exception
|
| main :: IO ()
| main = do
|     unserialize
|     putStrLn "all is well"
|
| unserialize :: IO Char
| unserialize =
|     if definitelyTrue
|         then do
|             return 'a'
|         else do
|             Exception.evaluate (error "wrong place")
|
| {-# NOINLINE definitelyTrue #-}
| definitelyTrue :: Bool
| definitelyTrue = True
|
|
| When compiled with -O on 8.4.1, this should print "wrong place".
| Without -O, or with 8.4.2, or if True can be inlined, or without
| evaluate, all is well.
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: ghc 8.4.1 and trac 13930

Evan Laforge
On Wed, May 2, 2018 at 12:27 PM, Simon Peyton Jones
<[hidden email]> wrote:
> Wow.  Could you open a ticket?

Done: https://ghc.haskell.org/trac/ghc/ticket/15114

> I just tried with 8.2.2 which is what I have on this laptop, but it printed "all is well".  Does that mean it was fine in 8.2, went wrong in 8.4.1 and was fixed in 8.4.2?

It seems likely!
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs