Marge Bot pushed to branch master at Glasgow Haskell Compiler / GHC

Commits:

12 changed files:

Changes:

  • rts/PrimOps.cmm
    ... ... @@ -1229,16 +1229,27 @@ INFO_TABLE_RET(stg_catch_retry_frame, CATCH_RETRY_FRAME,
    1229 1229
         gcptr trec, outer, arg;
    
    1230 1230
     
    
    1231 1231
         trec = StgTSO_trec(CurrentTSO);
    
    1232
    -    outer  = StgTRecHeader_enclosing_trec(trec);
    
    1233
    -    (r) = ccall stmCommitNestedTransaction(MyCapability() "ptr", trec "ptr");
    
    1234
    -    if (r != 0) {
    
    1235
    -        // Succeeded (either first branch or second branch)
    
    1236
    -        StgTSO_trec(CurrentTSO) = outer;
    
    1237
    -        return (ret);
    
    1238
    -    } else {
    
    1239
    -        // Did not commit: abort and restart.
    
    1240
    -        StgTSO_trec(CurrentTSO) = outer;
    
    1241
    -        jump stg_abort();
    
    1232
    +    if (running_alt_code != 1) {
    
    1233
    +      // When exiting the lhs code of catchRetry# lhs rhs, we need to cleanup
    
    1234
    +      // the nested transaction.
    
    1235
    +      // See Note [catchRetry# implementation]
    
    1236
    +      outer  = StgTRecHeader_enclosing_trec(trec);
    
    1237
    +      (r) = ccall stmCommitNestedTransaction(MyCapability() "ptr", trec "ptr");
    
    1238
    +      if (r != 0) {
    
    1239
    +          // Succeeded in first branch
    
    1240
    +          StgTSO_trec(CurrentTSO) = outer;
    
    1241
    +          return (ret);
    
    1242
    +      } else {
    
    1243
    +          // Did not commit: abort and restart.
    
    1244
    +          StgTSO_trec(CurrentTSO) = outer;
    
    1245
    +          jump stg_abort();
    
    1246
    +      }
    
    1247
    +    }
    
    1248
    +    else {
    
    1249
    +      // nothing to do in the rhs code of catchRetry# lhs rhs, it's already
    
    1250
    +      // using the parent transaction (not a nested one).
    
    1251
    +      // See Note [catchRetry# implementation]
    
    1252
    +      return (ret);
    
    1242 1253
         }
    
    1243 1254
     }
    
    1244 1255
     
    
    ... ... @@ -1471,21 +1482,26 @@ retry_pop_stack:
    1471 1482
         outer  = StgTRecHeader_enclosing_trec(trec);
    
    1472 1483
     
    
    1473 1484
         if (frame_type == CATCH_RETRY_FRAME) {
    
    1474
    -        // The retry reaches a CATCH_RETRY_FRAME before the atomic frame
    
    1475
    -        ASSERT(outer != NO_TREC);
    
    1476
    -        // Abort the transaction attempting the current branch
    
    1477
    -        ccall stmAbortTransaction(MyCapability() "ptr", trec "ptr");
    
    1478
    -        ccall stmFreeAbortedTRec(MyCapability() "ptr", trec "ptr");
    
    1485
    +        // The retry reaches a CATCH_RETRY_FRAME before the ATOMICALLY_FRAME
    
    1486
    +
    
    1479 1487
             if (!StgCatchRetryFrame_running_alt_code(frame) != 0) {
    
    1480
    -            // Retry in the first branch: try the alternative
    
    1481
    -            ("ptr" trec) = ccall stmStartTransaction(MyCapability() "ptr", outer "ptr");
    
    1482
    -            StgTSO_trec(CurrentTSO) = trec;
    
    1488
    +            // Retrying in the lhs of catchRetry# lhs rhs, i.e. in a nested
    
    1489
    +            // transaction. See Note [catchRetry# implementation]
    
    1490
    +
    
    1491
    +            // check that we have a parent transaction
    
    1492
    +            ASSERT(outer != NO_TREC);
    
    1493
    +
    
    1494
    +            // Abort the nested transaction
    
    1495
    +            ccall stmAbortTransaction(MyCapability() "ptr", trec "ptr");
    
    1496
    +            ccall stmFreeAbortedTRec(MyCapability() "ptr", trec "ptr");
    
    1497
    +
    
    1498
    +            // As we are retrying in the lhs code, we must now try the rhs code
    
    1499
    +            StgTSO_trec(CurrentTSO) = outer;
    
    1483 1500
                 StgCatchRetryFrame_running_alt_code(frame) = 1 :: CInt; // true;
    
    1484 1501
                 R1 = StgCatchRetryFrame_alt_code(frame);
    
    1485 1502
                 jump stg_ap_v_fast [R1];
    
    1486 1503
             } else {
    
    1487
    -            // Retry in the alternative code: propagate the retry
    
    1488
    -            StgTSO_trec(CurrentTSO) = outer;
    
    1504
    +            // Retry in the rhs code: propagate the retry
    
    1489 1505
                 Sp = Sp + SIZEOF_StgCatchRetryFrame;
    
    1490 1506
                 goto retry_pop_stack;
    
    1491 1507
             }
    

  • rts/RaiseAsync.c
    ... ... @@ -1043,8 +1043,7 @@ raiseAsync(Capability *cap, StgTSO *tso, StgClosure *exception,
    1043 1043
                 }
    
    1044 1044
     
    
    1045 1045
             case CATCH_STM_FRAME:
    
    1046
    -        case CATCH_RETRY_FRAME:
    
    1047
    -            // CATCH frames within an atomically block: abort the
    
    1046
    +            // CATCH_STM frame within an atomically block: abort the
    
    1048 1047
                 // inner transaction and continue.  Eventually we will
    
    1049 1048
                 // hit the outer transaction that will get frozen (see
    
    1050 1049
                 // above).
    
    ... ... @@ -1056,14 +1055,30 @@ raiseAsync(Capability *cap, StgTSO *tso, StgClosure *exception,
    1056 1055
             {
    
    1057 1056
                 StgTRecHeader *trec = tso -> trec;
    
    1058 1057
                 StgTRecHeader *outer = trec -> enclosing_trec;
    
    1059
    -            debugTraceCap(DEBUG_stm, cap,
    
    1060
    -                          "found atomically block delivering async exception");
    
    1058
    +            debugTraceCap(DEBUG_stm, cap, "raiseAsync: traversing CATCH_STM frame");
    
    1061 1059
                 stmAbortTransaction(cap, trec);
    
    1062 1060
                 stmFreeAbortedTRec(cap, trec);
    
    1063 1061
                 tso -> trec = outer;
    
    1064 1062
                 break;
    
    1065 1063
             };
    
    1066 1064
     
    
    1065
    +        case CATCH_RETRY_FRAME:
    
    1066
    +            // CATCH_RETRY frame within an atomically block: if we're executing
    
    1067
    +            // the lhs code, abort the inner transaction and continue; if we're
    
    1068
    +            // executing the rhs, continue (no nested transaction to abort. See
    
    1069
    +            // Note [catchRetry# implementation]). Eventually we will hit the
    
    1070
    +            // outer transaction that will get frozen (see above).
    
    1071
    +            //
    
    1072
    +            // As for the CATCH_STM_FRAME case above, we do not care
    
    1073
    +            // whether the transaction is valid or not because its
    
    1074
    +            // possible validity cannot have caused the exception
    
    1075
    +            // and will not be visible after the abort.
    
    1076
    +        {
    
    1077
    +            debugTraceCap(DEBUG_stm, cap, "raiseAsync: traversing CATCH_RETRY frame");
    
    1078
    +            stmAbortNestedCatchRetryTransaction(cap, tso, (StgCatchRetryFrame *)frame);
    
    1079
    +            break;
    
    1080
    +        };
    
    1081
    +
    
    1067 1082
             default:
    
    1068 1083
                 // see Note [Update async masking state on unwind] in Schedule.c
    
    1069 1084
                 if (*frame == (W_)&stg_unmaskAsyncExceptionszh_ret_info) {
    

  • rts/STM.c
    ... ... @@ -961,6 +961,46 @@ void stmFreeAbortedTRec(Capability *cap,
    961 961
       TRACE("%p : stmFreeAbortedTRec done", trec);
    
    962 962
     }
    
    963 963
     
    
    964
    +/*
    
    965
    +Note [catchRetry# implementation]
    
    966
    +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    
    967
    +catchRetry# creates a nested transaction for its lhs:
    
    968
    +- if the lhs transaction succeeds:
    
    969
    +    - the lhs transaction is committed
    
    970
    +    - its read-variables are merged with those of the parent transaction
    
    971
    +    - the rhs code is ignored
    
    972
    +- if the lhs transaction retries:
    
    973
    +    - the lhs transaction is aborted
    
    974
    +    - its read-variables are merged with those of the parent transaction
    
    975
    +    - the rhs code is executed directly in the parent transaction (see #26028).
    
    976
    +
    
    977
    +So note that:
    
    978
    +- lhs code uses a nested transaction
    
    979
    +- rhs code doesn't use a nested transaction
    
    980
    +
    
    981
    +We have to take which case we're in into account (using the running_alt_code
    
    982
    +field of the catchRetry frame) in catchRetry's entry code, in retry#
    
    983
    +implementation, and also when an async exception is received (to cleanup the
    
    984
    +right number of transactions).
    
    985
    +*/
    
    986
    +
    
    987
    +/* Called when unwinding past a CATCH_RETRY_FRAME.
    
    988
    + * Only aborts the transaction if we're executing the lhs (running_alt_code=0),
    
    989
    + * because rhs code uses the parent transaction directly with no nested trec.
    
    990
    + * See Note [catchRetry# implementation].
    
    991
    + */
    
    992
    +void stmAbortNestedCatchRetryTransaction(Capability *cap,
    
    993
    +                                         StgTSO *tso,
    
    994
    +                                         StgCatchRetryFrame *frame) {
    
    995
    +  if (!frame->running_alt_code) {
    
    996
    +    StgTRecHeader *trec = tso->trec;
    
    997
    +    StgTRecHeader *outer = trec->enclosing_trec;
    
    998
    +    stmAbortTransaction(cap, trec);
    
    999
    +    stmFreeAbortedTRec(cap, trec);
    
    1000
    +    tso->trec = outer;
    
    1001
    +  }
    
    1002
    +}
    
    1003
    +
    
    964 1004
     /*......................................................................*/
    
    965 1005
     
    
    966 1006
     void stmCondemnTransaction(Capability *cap,
    

  • rts/STM.h
    ... ... @@ -67,6 +67,9 @@ StgTRecHeader *stmStartNestedTransaction(Capability *cap, StgTRecHeader *outer
    67 67
     
    
    68 68
     void stmAbortTransaction(Capability *cap, StgTRecHeader *trec);
    
    69 69
     void stmFreeAbortedTRec(Capability *cap, StgTRecHeader *trec);
    
    70
    +void stmAbortNestedCatchRetryTransaction(Capability *cap,
    
    71
    +                                         StgTSO *tso,
    
    72
    +                                         StgCatchRetryFrame *frame);
    
    70 73
     
    
    71 74
     /*
    
    72 75
      * Ensure that a subsequent commit / validation will fail.  We use this
    

  • rts/Schedule.c
    ... ... @@ -3088,14 +3088,9 @@ raiseExceptionHelper (StgRegTable *reg, StgTSO *tso, StgClosure *exception)
    3088 3088
                 return STOP_FRAME;
    
    3089 3089
     
    
    3090 3090
             case CATCH_RETRY_FRAME: {
    
    3091
    -            StgTRecHeader *trec = tso -> trec;
    
    3092
    -            StgTRecHeader *outer = trec -> enclosing_trec;
    
    3093 3091
                 debugTrace(DEBUG_stm,
    
    3094 3092
                            "found CATCH_RETRY_FRAME at %p during raise", p);
    
    3095
    -            debugTrace(DEBUG_stm, "trec=%p outer=%p", trec, outer);
    
    3096
    -            stmAbortTransaction(cap, trec);
    
    3097
    -            stmFreeAbortedTRec(cap, trec);
    
    3098
    -            tso -> trec = outer;
    
    3093
    +            stmAbortNestedCatchRetryTransaction(cap, tso, (StgCatchRetryFrame *)p);
    
    3099 3094
                 p = next;
    
    3100 3095
                 continue;
    
    3101 3096
             }
    
    ... ... @@ -3248,14 +3243,9 @@ findAtomicallyFrameHelper (Capability *cap, StgTSO *tso)
    3248 3243
             return ATOMICALLY_FRAME;
    
    3249 3244
     
    
    3250 3245
         case CATCH_RETRY_FRAME: {
    
    3251
    -        StgTRecHeader *trec = tso -> trec;
    
    3252
    -        StgTRecHeader *outer = trec -> enclosing_trec;
    
    3253 3246
             debugTrace(DEBUG_stm,
    
    3254 3247
                        "found CATCH_RETRY_FRAME at %p while aborting after orElse", p);
    
    3255
    -        debugTrace(DEBUG_stm, "trec=%p outer=%p", trec, outer);
    
    3256
    -        stmAbortTransaction(cap, trec);
    
    3257
    -        stmFreeAbortedTRec(cap, trec);
    
    3258
    -        tso -> trec = outer;
    
    3248
    +        stmAbortNestedCatchRetryTransaction(cap, tso, (StgCatchRetryFrame *)p);
    
    3259 3249
             p = next;
    
    3260 3250
             continue;
    
    3261 3251
         }
    

  • testsuite/tests/lib/stm/T26028.hs
    1
    +module Main where
    
    2
    +
    
    3
    +import GHC.Conc
    
    4
    +
    
    5
    +forever :: IO String
    
    6
    +forever = delay 10 >> forever
    
    7
    +
    
    8
    +terminates :: IO String
    
    9
    +terminates = delay 1 >> pure "terminates"
    
    10
    +
    
    11
    +delay s = threadDelay (1000000 * s)
    
    12
    +
    
    13
    +async :: IO a -> IO (STM a)
    
    14
    +async a = do
    
    15
    +  var <- atomically (newTVar Nothing)
    
    16
    +  forkIO (a >>= atomically . writeTVar var . Just)
    
    17
    +  pure (readTVar var >>= maybe retry pure)
    
    18
    +
    
    19
    +main :: IO ()
    
    20
    +main = do
    
    21
    +  x <- mapM async $ terminates : replicate 50000 forever
    
    22
    +  r <- atomically (foldr1 orElse x)
    
    23
    +  print r

  • testsuite/tests/lib/stm/T26028.stdout
    1
    +"terminates"

  • testsuite/tests/lib/stm/T26291a.hs
    1
    +module Main where
    
    2
    +
    
    3
    +import Control.Concurrent.STM
    
    4
    +import Control.Exception
    
    5
    +
    
    6
    +main :: IO ()
    
    7
    +main = do
    
    8
    +  result <- try @SomeException $ atomically $
    
    9
    +    -- LHS retries → CATCH_RETRY_FRAME gets running_alt_code=1, RHS executes.
    
    10
    +    -- RHS throws → raiseExceptionHelper walks the stack, finds the
    
    11
    +    -- CATCH_RETRY_FRAME (running_alt_code=1), and must NOT abort tso->trec.
    
    12
    +    orElse retry (throwSTM (ErrorCall "test"))
    
    13
    +  case result of
    
    14
    +    Left _  -> putStrLn "OK"
    
    15
    +    Right _ -> putStrLn "impossible"

  • testsuite/tests/lib/stm/T26291a.stdout
    1
    +OK

  • testsuite/tests/lib/stm/T26291b.hs
    1
    +-- Test for the findAtomicallyFrameHelper crash when running_alt_code=1.
    
    2
    +--
    
    3
    +-- findAtomicallyFrameHelper is called by stg_abort, which fires when a nested
    
    4
    +-- transaction's stmCommitNestedTransaction fails (due to a concurrent TVar
    
    5
    +-- write conflicting with the nested trec's read set).  If the walk encounters
    
    6
    +-- a CATCH_RETRY_FRAME with running_alt_code=1, the old code unconditionally
    
    7
    +-- called stmAbortTransaction on tso->trec, which is the *parent* transaction
    
    8
    +-- (no nested trec exists for the RHS).  That freed the parent trec, leaving
    
    9
    +-- tso->trec as garbage; stg_abort then dereferenced it and crashed.
    
    10
    +--
    
    11
    +-- The structure that exercises this:
    
    12
    +--   outer orElse: LHS retries → RHS runs (outer CATCH_RETRY_FRAME has running_alt_code=1)
    
    13
    +--   inner orElse: LHS reads a TVar in a nested trec and tries to commit
    
    14
    +--     → if a concurrent writer invalidates the read, stmCommitNestedTransaction fails
    
    15
    +--     → stg_abort → findAtomicallyFrameHelper encounters the outer CATCH_RETRY_FRAME
    
    16
    +--       (running_alt_code=1) → crash without the fix.
    
    17
    +module Main where
    
    18
    +
    
    19
    +import Control.Concurrent
    
    20
    +import Control.Concurrent.STM
    
    21
    +
    
    22
    +main :: IO ()
    
    23
    +main = do
    
    24
    +  tv <- newTVarIO (0 :: Int)
    
    25
    +
    
    26
    +  -- Continuously modify tv to provoke nested-commit failures.
    
    27
    +  _ <- forkIO $ let loop = atomically (modifyTVar' tv (+1)) >> loop in loop
    
    28
    +
    
    29
    +  -- Run the critical orElse pattern many times.  Each iteration the inner LHS
    
    30
    +  -- reads tv (nested trec) and tries to commit; concurrent writes will
    
    31
    +  -- occasionally cause the commit to fail and trigger stg_abort.
    
    32
    +  let loop 0 = return ()
    
    33
    +      loop n = do
    
    34
    +        _ <- atomically $
    
    35
    +          orElse
    
    36
    +            retry                             -- outer LHS: always retries
    
    37
    +            (orElse (readTVar tv) (return 0)) -- outer RHS (running_alt_code=1):
    
    38
    +                                              --   inner LHS reads tv (nested trec)
    
    39
    +        loop (n - 1)
    
    40
    +  loop (100000 :: Int)
    
    41
    +
    
    42
    +  putStrLn "OK"

  • testsuite/tests/lib/stm/T26291b.stdout
    1
    +OK

  • testsuite/tests/lib/stm/all.T
    1
    +test('T26028', only_ways(['threaded1']), compile_and_run, ['-O2'])
    
    2
    +test('T26291a', normal, compile_and_run, ['-O2'])
    
    3
    +test('T26291b', only_ways(['threaded1']), compile_and_run, ['-O2'])