Zubin pushed to branch wip/25924 at Glasgow Haskell Compiler / GHC
Commits:
-
8bbc4359
by Zubin Duggal at 2026-07-02T15:18:12+05:30
4 changed files:
- changelog.d/fix-absent-dict-projection
- compiler/GHC/Core/Opt/Specialise.hs
- compiler/GHC/Core/Opt/WorkWrap.hs
- compiler/GHC/Core/Opt/WorkWrap/Utils.hs
Changes:
| 1 | 1 | section: compiler
|
| 2 | -synopsis: Fix a CorePrep miscompilation that could project a field out of an absent dictionary, resulting in a segfault.
|
|
| 2 | +synopsis: Fix a miscompilation that could project a field out of an absent dictionary, resulting in a segfault.
|
|
| 3 | 3 | issues: #25924
|
| 4 | 4 | mrs: !16219
|
| 5 | -description: We no longer speculatively evaluate bindings that we have already discovered are absent. |
|
| 5 | +description:
|
|
| 6 | + We no longer make an absent filler (a rubbish literal or error thunk) for an
|
|
| 7 | + absent dictionary or other terminating type. We also no longer speculatively
|
|
| 8 | + evaluate a binding once we have discovered that it is absent. |
| ... | ... | @@ -19,12 +19,13 @@ import GHC.Core.SimpleOpt( defaultSimpleOpts, simpleOptExprWith, exprIsConApp_ma |
| 19 | 19 | import GHC.Core.Predicate
|
| 20 | 20 | import GHC.Core.Class( classMethods )
|
| 21 | 21 | import GHC.Core.Coercion( Coercion )
|
| 22 | -import GHC.Core.DataCon (dataConTyCon)
|
|
| 22 | +import GHC.Core.DataCon (dataConTyCon, StrictnessMark(NotMarkedStrict))
|
|
| 23 | 23 | |
| 24 | 24 | import qualified GHC.Core.Subst as Core
|
| 25 | 25 | import GHC.Core.Unfold.Make
|
| 26 | 26 | import GHC.Core
|
| 27 | -import GHC.Core.Make ( mkLitRubbish, wrapFloats )
|
|
| 27 | +import GHC.Core.Make ( wrapFloats )
|
|
| 28 | +import GHC.Core.Opt.WorkWrap.Utils ( mkAbsentFiller )
|
|
| 28 | 29 | import GHC.Core.Unify ( tcMatchTy )
|
| 29 | 30 | import GHC.Core.Rules
|
| 30 | 31 | import GHC.Core.Subst (substTickish)
|
| ... | ... | @@ -1669,7 +1670,7 @@ specCalls spec_imp env existing_rules calls_for_me fn rhs |
| 1669 | 1670 | | otherwise = UnspecArg
|
| 1670 | 1671 | |
| 1671 | 1672 | ; (useful, subst', rule_bndrs, rule_lhs_args, spec_bndrs, dx_binds, spec_args)
|
| 1672 | - <- specHeader subst rhs_bndrs all_call_args
|
|
| 1673 | + <- specHeader this_mod subst rhs_bndrs all_call_args
|
|
| 1673 | 1674 | ; let env' = env { se_subst = subst' }
|
| 1674 | 1675 | |
| 1675 | 1676 | -- Check for (a) usefulness and (b) not already covered
|
| ... | ... | @@ -2573,7 +2574,8 @@ isSpecDict _ = False |
| 2573 | 2574 | -- , [T1, T2, c, i, dEqT1, dShow1]
|
| 2574 | 2575 | -- )
|
| 2575 | 2576 | specHeader
|
| 2576 | - :: Core.Subst -- This substitution applies to the [InBndr]
|
|
| 2577 | + :: Module -- The module being compiled, for mkAbsentFiller
|
|
| 2578 | + -> Core.Subst -- This substitution applies to the [InBndr]
|
|
| 2577 | 2579 | -> [InBndr] -- Binders from the original function `f`
|
| 2578 | 2580 | -> [SpecArg] -- From the CallInfo
|
| 2579 | 2581 | -> SpecM ( Bool -- True <=> some useful specialisation happened
|
| ... | ... | @@ -2598,13 +2600,13 @@ specHeader |
| 2598 | 2600 | |
| 2599 | 2601 | -- If we run out of binders, stop immediately
|
| 2600 | 2602 | -- See Note [Specialisation Must Preserve Sharing]
|
| 2601 | -specHeader subst [] _ = pure (False, subst, [], [], [], [], [])
|
|
| 2602 | -specHeader subst _ [] = pure (False, subst, [], [], [], [], [])
|
|
| 2603 | +specHeader _ subst [] _ = pure (False, subst, [], [], [], [], [])
|
|
| 2604 | +specHeader _ subst _ [] = pure (False, subst, [], [], [], [], [])
|
|
| 2603 | 2605 | |
| 2604 | 2606 | -- We want to specialise on type 'T1', and so we must construct a substitution
|
| 2605 | 2607 | -- 'a->T1', as well as a LHS argument for the resulting RULE and unfolding
|
| 2606 | 2608 | -- details.
|
| 2607 | -specHeader subst (bndr:bndrs) (SpecType ty : args)
|
|
| 2609 | +specHeader mod subst (bndr:bndrs) (SpecType ty : args)
|
|
| 2608 | 2610 | = do { -- Find free_tvs, the type variables to add to the binders for the rule
|
| 2609 | 2611 | -- Namely those deeply free in `ty` that aren't in scope
|
| 2610 | 2612 | -- See (MP2) in Note [Specialising polymorphic dictionaries]
|
| ... | ... | @@ -2617,7 +2619,7 @@ specHeader subst (bndr:bndrs) (SpecType ty : args) |
| 2617 | 2619 | |
| 2618 | 2620 | ; let subst2 = Core.extendTvSubst subst1 bndr ty
|
| 2619 | 2621 | ; (useful, subst3, rule_bs, rule_args, spec_bs, dx, spec_args)
|
| 2620 | - <- specHeader subst2 bndrs args
|
|
| 2622 | + <- specHeader mod subst2 bndrs args
|
|
| 2621 | 2623 | ; pure ( useful, subst3
|
| 2622 | 2624 | , free_tvs ++ rule_bs, Type ty : rule_args
|
| 2623 | 2625 | , free_tvs ++ spec_bs, dx, Type ty : spec_args ) }
|
| ... | ... | @@ -2626,29 +2628,34 @@ specHeader subst (bndr:bndrs) (SpecType ty : args) |
| 2626 | 2628 | -- a substitution on it (in case the type refers to 'a'). Additionally, we need
|
| 2627 | 2629 | -- to produce a binder, LHS argument and RHS argument for the resulting rule,
|
| 2628 | 2630 | -- /and/ a binder for the specialised body.
|
| 2629 | -specHeader subst (bndr:bndrs) (UnspecType : args)
|
|
| 2631 | +specHeader mod subst (bndr:bndrs) (UnspecType : args)
|
|
| 2630 | 2632 | = do { let (subst1, bndr') = Core.substBndr subst bndr
|
| 2631 | 2633 | ; (useful, subst2, rule_bs, rule_es, spec_bs, dx, spec_args)
|
| 2632 | - <- specHeader subst1 bndrs args
|
|
| 2634 | + <- specHeader mod subst1 bndrs args
|
|
| 2633 | 2635 | ; let ty_e' = Type (mkTyVarTy bndr')
|
| 2634 | 2636 | ; pure ( useful, subst2
|
| 2635 | 2637 | , bndr' : rule_bs, ty_e' : rule_es
|
| 2636 | 2638 | , bndr' : spec_bs, dx, ty_e' : spec_args ) }
|
| 2637 | 2639 | |
| 2638 | -specHeader subst (bndr:bndrs) (_ : args)
|
|
| 2640 | +specHeader mod subst (bndr:bndrs) (_ : args)
|
|
| 2639 | 2641 | | isDeadBinder bndr
|
| 2640 | 2642 | , let (subst1, bndr') = Core.substBndr subst (zapIdOccInfo bndr)
|
| 2641 | - , Just rubbish_lit <- mkLitRubbish (idType bndr')
|
|
| 2643 | + , Just filler <- mkAbsentFiller mod bndr' NotMarkedStrict
|
|
| 2644 | + -- NB: mkAbsentFiller returns Nothing for a terminating type (e.g. a
|
|
| 2645 | + -- dictionary), so this guard fails and we fall through, keeping the
|
|
| 2646 | + -- argument instead of dropping it.
|
|
| 2647 | + -- See Note [Don't make fillers for terminating types]
|
|
| 2648 | + -- in GHC.Core.Opt.WorkWrap.Utils
|
|
| 2642 | 2649 | = -- See Note [Drop dead args from specialisations]
|
| 2643 | - do { (useful, subst2, rule_bs, rule_es, spec_bs, dx, spec_args) <- specHeader subst1 bndrs args
|
|
| 2650 | + do { (useful, subst2, rule_bs, rule_es, spec_bs, dx, spec_args) <- specHeader mod subst1 bndrs args
|
|
| 2644 | 2651 | ; pure ( useful, subst2
|
| 2645 | 2652 | , bndr' : rule_bs, Var bndr' : rule_es
|
| 2646 | - , spec_bs, dx, rubbish_lit : spec_args ) }
|
|
| 2653 | + , spec_bs, dx, filler : spec_args ) }
|
|
| 2647 | 2654 | |
| 2648 | 2655 | -- Next we want to specialise the 'Eq a' dict away. We need to construct
|
| 2649 | 2656 | -- a wildcard binder to match the dictionary (See Note [Specialising Calls] for
|
| 2650 | 2657 | -- the nitty-gritty), as a LHS rule and unfolding details.
|
| 2651 | -specHeader subst (bndr:bndrs) (SpecDict dict_arg : args)
|
|
| 2658 | +specHeader mod subst (bndr:bndrs) (SpecDict dict_arg : args)
|
|
| 2652 | 2659 | = do { -- Make up a fresh binder to use in the RULE
|
| 2653 | 2660 | -- It might turn into a dict binding (via bindAuxiliaryDict) which we
|
| 2654 | 2661 | -- then float, so we use cloneIdBndr to get a completely fresh binder
|
| ... | ... | @@ -2659,7 +2666,7 @@ specHeader subst (bndr:bndrs) (SpecDict dict_arg : args) |
| 2659 | 2666 | -- Extend the substitution to map bndr :-> dict_arg, for use in the RHS
|
| 2660 | 2667 | ; let (subst2, dx_bind, spec_dict) = bindAuxiliaryDict subst1 bndr bndr' dict_arg
|
| 2661 | 2668 | |
| 2662 | - ; (_, subst3, rule_bs, rule_es, spec_bs, dx, spec_args) <- specHeader subst2 bndrs args
|
|
| 2669 | + ; (_, subst3, rule_bs, rule_es, spec_bs, dx, spec_args) <- specHeader mod subst2 bndrs args
|
|
| 2663 | 2670 | |
| 2664 | 2671 | ; let dx' = case dx_bind of { Nothing -> dx; Just d -> d : dx }
|
| 2665 | 2672 | ; pure ( True, subst3 -- Ha! A useful specialisation!
|
| ... | ... | @@ -2674,10 +2681,10 @@ specHeader subst (bndr:bndrs) (SpecDict dict_arg : args) |
| 2674 | 2681 | -- why 'i' doesn't appear in our RULE above. But we have no guarantee that
|
| 2675 | 2682 | -- there aren't 'UnspecArg's which come /before/ all of the dictionaries, so
|
| 2676 | 2683 | -- this case must be here.
|
| 2677 | -specHeader subst (bndr:bndrs) (UnspecArg : args)
|
|
| 2684 | +specHeader mod subst (bndr:bndrs) (UnspecArg : args)
|
|
| 2678 | 2685 | = do { let (subst1, bndr') = Core.substBndr subst (zapIdOccInfo bndr)
|
| 2679 | 2686 | -- zapIdOccInfo: see Note [Zap occ info in rule binders]
|
| 2680 | - ; (useful, subst2, rule_bs, rule_es, spec_bs, dx, spec_args) <- specHeader subst1 bndrs args
|
|
| 2687 | + ; (useful, subst2, rule_bs, rule_es, spec_bs, dx, spec_args) <- specHeader mod subst1 bndrs args
|
|
| 2681 | 2688 | |
| 2682 | 2689 | ; let dummy_arg = varToCoreExpr bndr'
|
| 2683 | 2690 | -- dummy_arg is usually just (Var bndr),
|
| ... | ... | @@ -554,7 +554,7 @@ tryWW ww_opts is_rec fn_id rhs |
| 554 | 554 | -- See Note [Drop absent bindings]
|
| 555 | 555 | | isAbsDmd (demandInfo fn_info)
|
| 556 | 556 | , not (isJoinId fn_id)
|
| 557 | - , Just filler <- mkAbsentFiller ww_opts fn_id NotMarkedStrict
|
|
| 557 | + , Just filler <- mkAbsentFiller (wo_module ww_opts) fn_id NotMarkedStrict
|
|
| 558 | 558 | = return [(new_fn_id, filler)]
|
| 559 | 559 | |
| 560 | 560 | -- See Note [Don't w/w INLINE things]
|
| ... | ... | @@ -30,7 +30,6 @@ import GHC.Core.Subst |
| 30 | 30 | import GHC.Core.Type
|
| 31 | 31 | import GHC.Core.Multiplicity
|
| 32 | 32 | import GHC.Core.Coercion
|
| 33 | -import GHC.Core.Predicate( isDictTy )
|
|
| 34 | 33 | import GHC.Core.Reduction
|
| 35 | 34 | import GHC.Core.FamInstEnv
|
| 36 | 35 | import GHC.Core.Predicate( isEqualityClass )
|
| ... | ... | @@ -996,7 +995,7 @@ mkWWstr_one opts arg str_mark = |
| 996 | 995 | _ | isTyVar arg -> do_nothing
|
| 997 | 996 | |
| 998 | 997 | DropAbsent
|
| 999 | - | Just absent_filler <- mkAbsentFiller opts arg str_mark
|
|
| 998 | + | Just absent_filler <- mkAbsentFiller (wo_module opts) arg str_mark
|
|
| 1000 | 999 | -- Absent case. Drop the argument from the worker.
|
| 1001 | 1000 | -- We can't always handle absence for arbitrary
|
| 1002 | 1001 | -- unlifted types, so we need to choose just the cases we can
|
| ... | ... | @@ -1067,14 +1066,20 @@ unbox_one_arg opts arg_var |
| 1067 | 1066 | --
|
| 1068 | 1067 | -- If @mkAbsentFiller _ id == Just e@, then @e@ is an absent filler with the
|
| 1069 | 1068 | -- same type as @id@. Otherwise, no suitable filler could be found.
|
| 1070 | -mkAbsentFiller :: WwOpts -> Id -> StrictnessMark -> Maybe CoreExpr
|
|
| 1071 | -mkAbsentFiller opts arg str
|
|
| 1069 | +mkAbsentFiller :: Module -> Id -> StrictnessMark -> Maybe CoreExpr
|
|
| 1070 | +mkAbsentFiller mod arg str
|
|
| 1071 | + -- We never make a filler for a terminating type: it might be speculatively
|
|
| 1072 | + -- evaluated or have a field projected out of it.
|
|
| 1073 | + -- See (AF4) in Note [Absent fillers], and
|
|
| 1074 | + -- Note [Don't make fillers for terminating types].
|
|
| 1075 | + | isTerminatingType arg_ty
|
|
| 1076 | + = Nothing
|
|
| 1077 | + |
|
| 1072 | 1078 | -- The lifted case: bind 'absentError'. See (AF1) in Note [Absent fillers]
|
| 1073 | 1079 | -- We want to use this case if possible, because we get a nice runtime panic message
|
| 1074 | 1080 | -- if we are wrong (like we were in #11126). Otherwise we fall through to the
|
| 1075 | 1081 | -- less-desirable mkLitRubbish case.
|
| 1076 | 1082 | | mightBeLiftedType arg_ty
|
| 1077 | - , not (isDictTy arg_ty) -- See (AF4) in Note [Absent fillers]
|
|
| 1078 | 1083 | , not (isStrictDmd (idDemandInfo arg)) -- See (AF2)
|
| 1079 | 1084 | , not (isMarkedStrict str) -- in Note [Absent fillers]
|
| 1080 | 1085 | = Just (mkAbsentErrorApp arg_ty msg)
|
| ... | ... | @@ -1101,7 +1106,7 @@ mkAbsentFiller opts arg str |
| 1101 | 1106 | -- will have different lengths and hence different costs for
|
| 1102 | 1107 | -- the inliner leading to different inlining.
|
| 1103 | 1108 | -- See also Note [Unique Determinism] in GHC.Types.Unique
|
| 1104 | - file_msg = text "In module" <+> quotes (ppr $ wo_module opts)
|
|
| 1109 | + file_msg = text "In module" <+> quotes (ppr mod)
|
|
| 1105 | 1110 | |
| 1106 | 1111 | {- Note [Worker/wrapper for Strictness and Absence]
|
| 1107 | 1112 | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
| ... | ... | @@ -1295,27 +1300,8 @@ Needless to say, there are some wrinkles: |
| 1295 | 1300 | have to be representation monomorphic. But in the future, we might allow
|
| 1296 | 1301 | levity polymorphism, e.g. a polymorphic levity variable in 'BoxedRep'.
|
| 1297 | 1302 | |
| 1298 | -(AF4) Consider (#24934)
|
|
| 1299 | - f :: (a~b) => blah {-# INLINE f #-}
|
|
| 1300 | - f d x = case eq_sel d of co -> body
|
|
| 1301 | - In #24934 it turned out that `co` was unused; and we discarded the
|
|
| 1302 | - entire case-scrutinisation via the `exprOkToDiscard` test in
|
|
| 1303 | - `GHC.Core.Opt.Simplify.Iteration.rebuildCase`. So now `d` is absent.
|
|
| 1304 | - But in the /unfolding/ for some reason we did not discard the `case`;
|
|
| 1305 | - so when we inline `f` we end up evaluating that `d` argument. So we had
|
|
| 1306 | - better not replace it with an error thunk!
|
|
| 1307 | - |
|
| 1308 | - The root of it is this: `exprOkToDiscard` assumes that a dictionary is
|
|
| 1309 | - non-bottom (Note [exprOkForSpeculation and type classes]); but then we replace
|
|
| 1310 | - the (a~b) dictionary with an error thunk, breaking the invariant that every
|
|
| 1311 | - dictionary is non-bottom. (If -XDictsStrict is on, the invariant is even
|
|
| 1312 | - more important.)
|
|
| 1313 | - |
|
| 1314 | - Simple solution: never use an error thunk for a dictionary; instead fall
|
|
| 1315 | - through to mkRubbishLit. (The only downside is that we lose the compiler
|
|
| 1316 | - debugging advantages of (AF1).)
|
|
| 1317 | - |
|
| 1318 | - This is quite delicate.
|
|
| 1303 | +(AF4) We never make an absent filler for a terminating type.
|
|
| 1304 | + See Note [Don't make fillers for terminating types].
|
|
| 1319 | 1305 | |
| 1320 | 1306 | While (AF1) and (AF2) are simply an optimisation in terms of compiler debugging
|
| 1321 | 1307 | experience, (AF3) should be irrelevant in most programs, if not all.
|
| ... | ... | @@ -1337,6 +1323,47 @@ fragile |
| 1337 | 1323 | because `MkT` is strict in its Int# argument, so we get an absentError
|
| 1338 | 1324 | exception when we shouldn't. Very annoying!
|
| 1339 | 1325 | |
| 1326 | +Note [Don't make fillers for terminating types]
|
|
| 1327 | +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
|
| 1328 | +We never make an absent filler, error thunk or rubbish literal, for a terminating
|
|
| 1329 | +type (isTerminatingType): a non-unary class dictionary, a boxed equality, or a
|
|
| 1330 | +constraint tuple.
|
|
| 1331 | + |
|
| 1332 | +GHC relies on a dictionary value never being bottom (see
|
|
| 1333 | +Note [NON-BOTTOM-DICTS invariant] in GHC.Core). GHC uses "speculation" to
|
|
| 1334 | +evaluated guaranteed-non-bottom values: see Note [Speculative evaluation] in
|
|
| 1335 | +GHC.CoreToStg.Prep. This speculative evaluation is fundamentally incompatible
|
|
| 1336 | +with replacing a dictionary with an absent filler. Attempts to to do so gave
|
|
| 1337 | +rise to a succession of bugs including:
|
|
| 1338 | + |
|
| 1339 | + * #24934: we evaluated an absent dictionary
|
|
| 1340 | + * #25924: we selected a superclass from an absent dictionary
|
|
| 1341 | + |
|
| 1342 | +A terminating type is exactly what speculation will force: see
|
|
| 1343 | +Note [exprOkForSpeculation and type classes] in GHC.Core.Utils. So we refuse to
|
|
| 1344 | +make a filler for precisely those types.
|
|
| 1345 | + |
|
| 1346 | +So the safe thing is to make no filler at all for a terminating type. Then there
|
|
| 1347 | +is no bogus dictionary to evaluate or project from. Specifically
|
|
| 1348 | + |
|
| 1349 | + * `mkAbsentFiller` returns `Nothing` for a terminating type, so worker/wrapper
|
|
| 1350 | + keeps the real argument.
|
|
| 1351 | + |
|
| 1352 | + * `Specialise.specHeader` calls `mkAbsentFiller` too, so it likewise keeps the
|
|
| 1353 | + dead dictionary argument rather than dropping it for a filler.
|
|
| 1354 | + |
|
| 1355 | +Prior failed approaches
|
|
| 1356 | + |
|
| 1357 | +We used to paper over this. !13233 replaced the error thunk for an absent
|
|
| 1358 | +dictionary with a rubbish literal, so that it could at least be evaluated
|
|
| 1359 | +without complaint. But #25924 showed that this is not enough, because we do not
|
|
| 1360 | +only evaluate the absent dictionary, we also select a superclass from it.
|
|
| 1361 | + |
|
| 1362 | +We could instead teach speculation to leave absent bindings alone, and we do
|
|
| 1363 | +that too (see Note [Speculative evaluation] in GHC.CoreToStg.Prep). But that is
|
|
| 1364 | +not a guarantee. After optimisation a binding that holds an absent filler may no
|
|
| 1365 | +longer be marked absent, so we cannot rely on the demand to protect us.
|
|
| 1366 | + |
|
| 1340 | 1367 | Note [Unboxing through unboxed tuples]
|
| 1341 | 1368 | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
| 1342 | 1369 | We should not to a worker/wrapper split just for unboxing the components of
|