[MLton] RE: Crash during cheney-copy on Windows

[Matthew Fluet]

On Tue, 1 Sep 2009, Nicolas Bertolotti wrote:
> I am facing a sporadic signal 11 (segmentation fault) on Linux which 
> could be caused by the same bug.

I'm not sure this bug would cause the previous failure.  An invalid 
card/cross map would cause problems for a minor gc, but a major gc ignores 
the card/cross map.  The previous failure occured during a major gc.  Of 
course, an invalid card/cross map might cause the minor gc to leave the 
heap in a bad state that triggers the failure in the major gc, but I would 
expect the failure to come during the minor gc.

> I finally identified that the SVN revision r6776 introduced a change 
> that was motivated by the fact we need to clear the cross map after 
> every major GC. But, if we look at the code, we can see that the cross 
> map is only cleared when the 'mayResize' flag is set (and, as a matter 
> of fact, this flag is not set by GC_pack()) :
> void majorGC (GC_state s, size_t bytesRequested, bool mayResize) {
> ...
>  if (mayResize) {
>    resizeHeap (s, s->lastMajorStatistics.bytesLive + bytesRequested);
>   setCardMapAndCrossMap (s);
> }
> ...
> }
> As the revision r6776 also introduces the removal of some calls to 
> clearCrossMap() which were performed systematically at the end of a 
> major cheney-copy or major mark-compact, it seems to me that the call to 
> setCardMapAndCrossMap(s) should actually always be performed (or maybe 
> adding a else { clearCrossMap(s); } would be enough).

Your analysis looks completely right.  I will note that a major Cheney 
Copy GC does clear the card/cross map (via swapHeapsForCheneyCopy), so it 
is only a GC_pack that induces a Mark Compact GC that would leave 
the card/cross maps in an invalid state.  Performing GC_packs in 
close succession is likely to induce a Mark Compact GC (since the 
packed heap isn't big enough for a Cheney Copy).

Further investigation might find a way to avoid unnecessarily setting and 
clearing the maps, but your solution to unconditionally 
setCardMapAndCrossMap at the end of a majorGC is certainly correct and 
expedient, so I've committed it (SVN r7228).  It simply performs 
setCardMapAndCrossMap twice in the case of a major Cheney Copy; it might 
be possible to eliminate the setCardMapAndCrossMap call from 
swapHeapsForCheneyCopy, if there are no assertions that the current heap 
and the current card/cross maps agree between the swapHeapsForCheneyCopy 
and the unconditional setCardMapAndCrossMap at the end of majorGC; there 
don't appear to be any.  SVN r6776 was attempting to systematically 
setCardMapAndCrossMap immediately after any change the heap pointer or the 
heap size, but that can probably be relaxed with respect to the 
swapHeapsForCheneyCopy.