Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#10712 closed bug (fixed)

Regression: make TEST=exceptionsrun001 WAY=optasm is failing

Reported by: thomie Owned by:
Priority: highest Milestone: 8.0.1
Component: Compiler Version: 7.11
Keywords: Exceptions Cc: simonmar
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: base/tests/exceptionsrun001
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D1616
Wiki Page:

Description (last modified by bgamari)

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:

Change History (23)

comment:1 Changed 4 years ago by thomie

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

  • T3279
  • conc012
  • conc014

comment:2 Changed 4 years ago by rwbarton

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

comment:3 Changed 4 years ago by rwbarton

Actually I just guessed and checked, 7c0fff41789669450b02dc1db7f5d7babba5dee6 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 7c0fff41789

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.

comment:4 Changed 4 years ago by simonpj

Cc: simonmar added

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 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.

comment:5 Changed 4 years ago by Thomas Miedema <thomasmiedema@…>

In 3b233793/ghc:

Testsuite: mark 4 tests expect_broken_for(#10712, opt_ways)

Please revert when #10712 is fixed.

comment:6 Changed 4 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:7 Changed 4 years ago by bgamari

Description: modified (diff)
Priority: highhighest

comment:8 Changed 4 years ago by simonpj

Just possibly related #7411

comment:9 Changed 4 years ago by bgamari

Differential Rev(s): Phab:D1616
Status: newpatch

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

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

comment:10 Changed 4 years ago by Ben Gamari <ben@…>

In 28638df/ghc:

primops: Mark actions evaluated by `catch*` as lazy

There is something very peculiar about the `catch` family of operations
with respect to strictness analysis: they turn divergence into
non-divergence. For this reason, it isn't safe to mark them as strict in
the expression whose exceptions they are catching. The reason is this:

Consider,

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

If we give the first argument of catch a strict signature, we'll get a
demand 'C(S)' for 'r'; that is, 'r' is definitely called with one
argument, which indeed it is. The trouble comes when we feed 'C(S)' into
'r's RHS as the demand of the body as this will lead us to conclude that
the whole 'let' will diverge; clearly this isn't right.

This is essentially the problem in #10712, which arose when
7c0fff41789669450b02dc1db7f5d7babba5dee6 marked the `catch*` primops as
being strict in the thing to be evaluated. Here I've partially reverted
this commit, again marking the first argument of these primops as lazy.

Fixes #10712.

Test Plan: Validate checking `exceptionsrun001`

Reviewers: simonpj, austin

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D1616

GHC Trac Issues: #10712, #11222

comment:11 Changed 4 years ago by bgamari

Resolution: fixed
Status: patchclosed

Fixed.

comment:12 Changed 4 years ago by bgamari

Resolution: fixed
Status: closednew

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.

comment:13 Changed 4 years ago by Thomas Miedema <thomasmiedema@…>

In a2f04a2/ghc:

Testsuite: #10712 is fixed

Verified by running:

  make TEST='exceptionsrun001 T3279 conc012 conc014' slowtest

comment:14 Changed 4 years ago by thomie

Resolution: fixed
Status: newclosed

Your patch did fix the bug, the four failing tests just had to marked as 'normal' again. All done now.

comment:15 Changed 4 years ago by Simon Peyton Jones <simonpj@…>

In 9915b656/ghc:

Make demand analysis understand catch

As Trac #11222, and #10712 note, the strictness analyser
needs to be rather careful about exceptions.  Previously
it treated them as identical to divergence, but that
won't quite do.

See Note [Exceptions and strictness] in Demand, which
explains the deal.

Getting more strictness in 'catch' and friends is a
very good thing.  Here is the nofib summary, keeping
only the big ones.

--------------------------------------------------------------------------------
        Program           Size    Allocs   Runtime   Elapsed  TotalMem
--------------------------------------------------------------------------------
          fasta          -0.1%     -6.9%     -3.0%     -3.0%     +0.0%
            hpg          -0.1%     -2.0%     -6.2%     -6.2%     +0.0%
       maillist          -0.1%     -0.3%      0.08      0.09     +1.2%
reverse-complem          -0.1%    -10.9%     -6.0%     -5.9%     +0.0%
         sphere          -0.1%     -4.3%      0.08      0.08     +0.0%
           x2n1          -0.1%     -0.0%      0.00      0.00     +0.0%
--------------------------------------------------------------------------------
            Min          -0.2%    -10.9%    -17.4%    -17.3%     +0.0%
            Max          -0.0%     +0.0%     +4.3%     +4.4%     +1.2%
 Geometric Mean          -0.1%     -0.3%     -2.9%     -3.0%     +0.0%

On the way I did quite a bit of refactoring in Demand.hs

comment:16 in reply to:  15 Changed 4 years ago by rwbarton

Resolution: fixed
Status: closednew

Commit 9915b656/ghc broke the tests that had been fixed in 28638df/ghc again, namely:

make slowtest TEST='exceptionsrun001 T3279 conc012 conc014'

(Note that the ways that broke, including optasm, are not run by validate by default.)

comment:17 Changed 4 years ago by simonpj

Are you sure? exceptionsrun001 seems OK to me.

It turns out that the other three all relied on imprecise exceptions being precise. Eg conc012 has

forkIO $ Control.Exception.catch (x `seq` putMVar result Finished) $

If you want to be sure that x will only be evaluated under the catch you must use evaluate thus:

forkIO $ Control.Exception.catch (evaluate x >> putMVar result Finished) $

Similarly conc014 had

     error "wibble"  `Control.Exception.catch`  ...

But the error "wibble" is a pure bottom value and we make no guarantees about when it is evaluated. It should be more like

    throwIO (ErrorCall "wibble")  `Control.Exception.catch` ....

which raises the exception in the IO monad (i.e. precisely).

Same with T3279, which had

        error "foo" `catch` \(SomeException e) -> do

Again that error "foo" should be throwIO (ErrorCall "foo").

Incidentally, is there a function for throwIO . ErrorCall?

I'll make a patch for these three tests. Then I claim we are done.

comment:18 Changed 4 years ago by Simon Peyton Jones <simonpj@…>

In 3798b2a/ghc:

Fix three broken tests involving exceptions

See comment:16 in Trac #10712. The tests were wrong, not GHC!

comment:19 Changed 4 years ago by simonpj

Resolution: fixed
Status: newclosed

comment:20 Changed 4 years ago by rwbarton

exceptionsrun001 still fails for me in the hpc way. Could it be OS-dependent?

rwbarton@morphism:~/ghc-newest/testsuite/tests$ make TEST=exceptionsrun001 WAY=hpc
[...]
=====> exceptionsrun001(hpc) 1 of 1 [0, 0, 0] 
cd ../../libraries/base/tests &&  "/home/rwbarton/ghc-newest/inplace/test   spaces/ghc-stage2" -o exceptionsrun001 exceptionsrun001.hs -fforce-recomp -dcore-lint -dcmm-lint -dno-debug-output -no-user-package-db -rtsopts -fno-warn-tabs -fno-warn-missed-specialisations -fno-ghci-history  -O -fhpc -hpcdir .hpc.exceptionsrun001  > exceptionsrun001.comp.stderr 2>&1
cd ../../libraries/base/tests && ./exceptionsrun001    </dev/null > exceptionsrun001.run.stdout 2> exceptionsrun001.run.stderr
Wrong exit code (expected 0 , actual 1 )
Stdout:
user exception caught
error call caught
no method error

Stderr:
exceptionsrun001: exceptionsrun001.hs:38:1-13: Non-exhaustive patterns in function test1


*** unexpected failure for exceptionsrun001(hpc)

Unexpected results from:
TEST="exceptionsrun001"

OVERALL SUMMARY for test run started at Thu Jan 28 11:30:04 2016 EST
 0:00:01 spent to go through
       1 total tests, which gave rise to
      10 test cases, of which
       9 were skipped

       0 had missing libraries
       0 expected passes
       0 expected failures

       0 caused framework failures
       0 unexpected passes
       1 unexpected failures
       0 unexpected stat failures

Unexpected failures:
   ../../libraries/base/tests  exceptionsrun001 [bad exit code] (hpc)

I will try to look into why.

comment:21 Changed 4 years ago by rwbarton

Well the test is wrong in the same way as the others.

patMatchTest = catch (case test1 [1..10] of () -> return ())
  (...)

where test1 [1..10] results in a pattern match failure.

Maybe the best question is why the test doesn't fail with way optasm. After all why not evaluate case test1 [1..10] of () -> return () first, if catch is strict in that argument.

That argument becomes (up to a coercion)

a_s2uX
  :: GHC.Prim.State# GHC.Prim.RealWorld
     -> (# GHC.Prim.State# GHC.Prim.RealWorld, () #)
[LclId,
 Arity=1,
 Str=DmdType,
 Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
         WorkFree=True, Expandable=True, Guidance=IF_ARGS [0] 290 30}]
a_s2uX =
  \ (eta_B1 [OS=OneShot] :: GHC.Prim.State# GHC.Prim.RealWorld) ->
    case GHC.Base.build
           @ Integer
           (\ (@ b_a2uz)
              (c_a2uA [OS=OneShot] :: Integer -> b_a2uz -> b_a2uz)
              (n_a2uB [OS=OneShot] :: b_a2uz) ->
              GHC.Enum.enumDeltaToInteger1FB @ b_a2uz c_a2uA n_a2uB 1 10)
    of _ [Occ=Dead] {
      [] -> (# eta_B1, GHC.Tuple.() #);
      : ipv_s2hb ipv_s2hc -> case lvl_s2v4 of wild_00 { }
    }

Perhaps some of the Value=True, ConLike=True, WorkFree=True, Expandable=True flags are causing the value not to get evaluated eagerly. Is that what GHC should be doing? (The hpc version has all those flags set to False, maybe because of the ticks wrapping the expression, or because the coercion was not removed yet: the hpc version of this binding has type IO () still.)

Anyways, GHC's behavior is correct either way, but maybe there is something to learn here before fixing the test.

comment:22 Changed 4 years ago by simonpj

catch is not strict in its argument. Its argument is a function; catch just guarantees to call that function. So yes build (...) is sure to be evaluated, but GHC doesn't aggressively evaluate things as early as possible. It just uses strictness info to avoid building thunks; and none are built here. So I think it's fine.

Might be wroth fixing the test though.

Thanks for the accurate diagnosis.

Simon

comment:23 Changed 3 years ago by simonpj

Keywords: Exceptions added
Note: See TracTickets for help on using tickets.