Regression: make TEST=exceptionsrun001 WAY=optasm is failing

The following program, extracted from the test exceptionsrun001, should exit with exitcode 100. Instead, when compiled with -O1, it never gets past the ioTest and somehow manages to exit with exitcode 0.

{-# LANGUAGE ScopedTypeVariables #-}
module Main where

import Control.Exception
import System.IO.Error
import System.Exit

main = do
  ioTest
  exitWith (ExitFailure 100)

ioTest :: IO ()
ioTest = (catch (ioError (userError "wibble"))
                (\(e::IOException) -> return ())

I think this will require a git bisect:

• last known good commit: 34bb4605
• first known bad commit: f83aab95.

changed weight to 7

added Tbug Trac import labels

Other tests that are failing with WAY=optasm, all dealing with exceptions of some sort:

• T3279
• conc012
• conc014

I'll bisect and see what I find (if you aren't doing so already).

Actually I just guessed and checked, 7c0fff41 is the bad commit.

   catch (ioError (userError "wibble"))
         (\(e::IOException) -> return ())

amounts to

   catch# (raiseIO# (toException (userError "wibble")))
          ({- handler -})
          st

but

• raiseIO#'s strictness signature claims it returns |
• catch# is now strict in its first argument, as of the commit 7c0fff41

so the strictness analyser concludes that ioTest will never return, and optimizes main to

Main.main1 =
  \ (@ b_aLH) (s_X2HT [OS=OneShot] :: State# RealWorld) ->
    case Main.ioTest1 s_X2HT of wild_00 { }

and what happens then when ioTest really does return is undefined.

There are apparently good reasons for each of the two bulleted points, but as this test shows they are incompatible.

Bother. I totally missed that.

I think it's reasonable that this should allow the exception to be raised somewhere else (imprecisely):

catch# (\s -> (raise# blah) `seq` blah2) (...) st

because raise# is in the pure world. But raiseIO# is specifically intended to raise an exception precisely at the specified moment, so the new behaviour is unacceptable.

Now I think about this more I'm also worried about

  let r = \st -> raiseIO# blah st
  in
  catch (\st -> ...(r st)..) handler st

Now that I'm given catch a more aggressive strictness, I'll get a demand C(S) for r; that is, it is definitly called with one argument. And so it is! But the danger is that we'll feed C(S) into r's rhs as the demand of the body, and say that that whole let will definitely diverge (which isn't true).

However, we really want this function to be strict in x:

f x st = catch (\s -> case x of I# x' -> ...) handler st

Getting this strictness was the whole point of the offending commit:

        Program           Size    Allocs   Runtime   Elapsed  TotalMem
--------------------------------------------------------------------------------
            hpg          -0.4%     -2.9%     -0.9%     -1.0%     +0.0%
reverse-complem          -0.4%    -10.9%    +10.7%    +10.9%     +0.0%
         simple          -0.3%     -0.0%    +26.2%    +26.2%     +3.7%
         sphere          -0.3%     -6.3%      0.09      0.09     +0.0%
--------------------------------------------------------------------------------
            Min          -0.7%    -10.9%     -4.6%     -4.7%     -1.7%
            Max          -0.2%     +0.0%    +26.2%    +26.2%     +6.5%
 Geometric Mean          -0.4%     -0.3%     +2.1%     +2.1%     +0.1%

There's something very special about catch: it turns divergence into non-divergence. (The strictness analyser treats divergence and exceptions identically.)

I think #8598 (closed) is relevant.

Bother bother. I'm really not sure what to do. Even if we revert, we should not revert all, just the strictness signatures for the catch primops.

mentioned in commit 3b233793

changed milestone to %8.0.1

Milestone renamed

changed the description

changed weight to 10

Trac metadata

Trac field	Value
Priority	high → highest

Just possibly related #7411

mentioned in issue #11222 (closed)

I have opened D1616 with what I believe is a fix for this.

I have also opened #11222 (closed) to track the fact that currently we are forced to pessimize our strictness signatures on catch-like operations.

Trac metadata

Trac field	Value
Differential revisions	- → D1616

mentioned in commit 28638dfe

closed

Fixed.

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

reopened

thomie correctly pointed out that this patch did not actually fix the four failing testcases marked in 3b233793. Looks like I'll need to dive a bit deeper.

Trac metadata

Trac field	Value
Resolution	ResolvedFixed → Unresolved