Zubin pushed to branch wip/25924 at Glasgow Haskell Compiler / GHC

Commits:

4 changed files:

Changes:

  • changelog.d/fix-absent-dict-projection
    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.

  • compiler/GHC/Core/Opt/Specialise.hs
    ... ... @@ -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),
    

  • compiler/GHC/Core/Opt/WorkWrap.hs
    ... ... @@ -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]
    

  • compiler/GHC/Core/Opt/WorkWrap/Utils.hs
    ... ... @@ -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