From 890f5a1a6e70ff4021cd58463f5152f10c47b395 Mon Sep 17 00:00:00 2001 From: Simon Marlow Date: Tue, 29 Jul 2008 15:05:18 +0000 Subject: [PATCH] FIX #2327: a fault in the thunk-selector machinery (again) This program contains an expression of the form let x = snd (_, snd (_, snd (_, x))) (probably not explicitly, but that's what appears in the heap at runtime). Obviously the program should deadlock if it ever enters this thing, but apparently the test program in #2327 never does. The GC tries to evaluate the snd closures, and gets confused due to the loop. In particular the earlier fix for #1038 was to blame. --- rts/sm/Evac.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/rts/sm/Evac.c b/rts/sm/Evac.c index ab20470..6bf0c56 100644 --- a/rts/sm/Evac.c +++ b/rts/sm/Evac.c @@ -773,14 +773,25 @@ unchain_thunk_selectors(StgSelector *p, StgClosure *val) prev = (StgSelector*)((StgClosure *)p)->payload[0]; // Update the THUNK_SELECTOR with an indirection to the - // EVACUATED closure now at p. Why do this rather than - // upd_evacuee(q,p)? Because we have an invariant that an - // EVACUATED closure always points to an object in the - // same or an older generation (required by the short-cut - // test in the EVACUATED case, below). - ((StgInd *)p)->indirectee = val; - write_barrier(); - SET_INFO(p, &stg_IND_info); + // value. The value is still in from-space at this stage. + // + // (old note: Why not do upd_evacuee(q,p)? Because we have an + // invariant that an EVACUATED closure always points to an + // object in the same or an older generation (required by + // the short-cut test in the EVACUATED case, below). + if ((StgClosure *)p == val) { + // must be a loop; just leave a BLACKHOLE in place. This + // can happen when we have a chain of selectors that + // eventually loops back on itself. We can't leave an + // indirection pointing to itself, and we want the program + // to deadlock if it ever enters this closure, so + // BLACKHOLE is correct. + SET_INFO(p, &stg_BLACKHOLE_info); + } else { + ((StgInd *)p)->indirectee = val; + write_barrier(); + SET_INFO(p, &stg_IND_info); + } // For the purposes of LDV profiling, we have created an // indirection. @@ -957,12 +968,18 @@ selector_loop: prev_thunk_selector = p; *q = val; - if (evac) evacuate(q); - val = *q; + + // update the other selectors in the chain *before* + // evacuating the value. This is necessary in the case + // where the value turns out to be one of the selectors + // in the chain (i.e. we have a loop), and evacuating it + // would corrupt the chain. + unchain_thunk_selectors(prev_thunk_selector, val); + // evacuate() cannot recurse through // eval_thunk_selector(), because we know val is not // a THUNK_SELECTOR. - unchain_thunk_selectors(prev_thunk_selector, val); + if (evac) evacuate(q); return; } -- 1.7.10.4