Marge Bot pushed to branch master at Glasgow Haskell Compiler / GHC
Commits:
-
f8de456f
by Sylvain Henry at 2026-03-27T04:43:22-04:00
12 changed files:
- rts/PrimOps.cmm
- rts/RaiseAsync.c
- rts/STM.c
- rts/STM.h
- rts/Schedule.c
- + testsuite/tests/lib/stm/T26028.hs
- + testsuite/tests/lib/stm/T26028.stdout
- + testsuite/tests/lib/stm/T26291a.hs
- + testsuite/tests/lib/stm/T26291a.stdout
- + testsuite/tests/lib/stm/T26291b.hs
- + testsuite/tests/lib/stm/T26291b.stdout
- + testsuite/tests/lib/stm/all.T
Changes:
| ... | ... | @@ -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 | }
|
| ... | ... | @@ -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) {
|
| ... | ... | @@ -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,
|
| ... | ... | @@ -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
|
| ... | ... | @@ -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 | }
|
| 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 |
| 1 | +"terminates" |
| 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" |
| 1 | +OK |
| 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" |
| 1 | +OK |
| 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']) |