Re: Fix to long-standing crashes in GC

[Stefan Monnier <monnier@iro.umontreal.ca>]

[-- Attachment #1: Type: text/plain, Size: 3444 bytes --]

> I have now installed some further changes to the GC code to
> fix problems with invalid Lisp_Misc objects.

> The main problem was that marker blocks could be freed while
> still being pointed to by buffer undo lists, but I made other
> minor changes to make the code more robust.

I still don't understand how this could happen.
Your fix looks rather intricate and introduces lots of fragile and
undocumented invariants, breaking some of the more traditional invariants
that the old code enforced (or at least assumed).

While I still don't know which scenario you were trying to fix, my
understanding is that it had to do with live objects pointing to
unmarked objects.

How about the patch below *instead* of the one you installed?

I.e. instead of sweeping first and then looking for misc_free objects in
undo_list, thus introducing some strange intermediate state where we
actually have misc_free objects inside reachable data,
I suggest we simply don't do the half-assed marking of undo_list in
mark_buffer and instead we do it just after we removed the unwanted entries
from the list.

I think the change below is a good change in any case (it makes the code
simpler and more robust), but my question is: do you think that it fixes
the problem you were trying to fix?


        Stefan


PS: the patch below is against an older version of alloc.c: it's mostly
    meant to be read rather than applied.  For people who want to try the
    patch in vivo, see the attached patch which is against the latest
    alloc.c.


Index: src/alloc.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/alloc.c,v
retrieving revision 1.336
diff -u -r1.336 alloc.c
--- src/alloc.c	18 May 2004 16:22:46 -0000	1.336
+++ src/alloc.c	28 May 2004 21:34:26 -0000
@@ -4476,7 +4454,9 @@
   }
 #endif
 
-  /* Look thru every buffer's undo list
+  /* Everything is now marked, except for the things that require special
+     finalization, i.e. the undo_list.
+     Look thru every buffer's undo list
      for elements that update markers that were not marked,
      and delete them.  */
   {
@@ -4514,6 +4494,9 @@
 		  }
 	      }
 	  }
+	/* Now that we have stripped the elements that need not be in the
+	   undo_list any more, we can finally mark the list.  */
+	mark_object (nextb->undo_list);
 
 	nextb = nextb->next;
       }
@@ -5095,41 +5070,9 @@
 
   MARK_INTERVAL_TREE (BUF_INTERVALS (buffer));
 
-  if (CONSP (buffer->undo_list))
-    {
-      Lisp_Object tail;
-      tail = buffer->undo_list;
-
-      /* We mark the undo list specially because
-	 its pointers to markers should be weak.  */
-
-      while (CONSP (tail))
-	{
-	  register struct Lisp_Cons *ptr = XCONS (tail);
-
-	  if (CONS_MARKED_P (ptr))
-	    break;
-	  CONS_MARK (ptr);
-	  if (GC_CONSP (ptr->car)
-	      && !CONS_MARKED_P (XCONS (ptr->car))
-	      && GC_MARKERP (XCAR (ptr->car)))
-	    {
-	      CONS_MARK (XCONS (ptr->car));
-	      mark_object (XCDR (ptr->car));
-	    }
-	  else
-	    mark_object (ptr->car);
-
-	  if (CONSP (ptr->cdr))
-	    tail = ptr->cdr;
-	  else
-	    break;
-	}
-
-      mark_object (XCDR (tail));
-    }
-  else
-    mark_object (buffer->undo_list);
+  /* For now, we just don't mark the undo_list.  It's done later in
+     a special way just before the sweep phase, and after stripping
+     some of its elements that are not needed any more.  */
 
   if (buffer->overlays_before)
     {


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: alloc.patch --]
[-- Type: text/x-patch, Size: 5557 bytes --]

--- alloc.c	28 mai 2004 16:57:39 -0400	1.340
+++ alloc.c	28 mai 2004 17:33:20 -0400	
@@ -2866,10 +2866,6 @@
 
 union Lisp_Misc *marker_free_list;
 
-/* Marker blocks which should be freed at end of GC.  */
-
-struct marker_block *marker_blocks_pending_free;
-
 /* Total number of marker blocks now in use.  */
 
 int n_marker_blocks;
@@ -2880,7 +2876,6 @@
   marker_block = NULL;
   marker_block_index = MARKER_BLOCK_SIZE;
   marker_free_list = 0;
-  marker_blocks_pending_free = 0;
   n_marker_blocks = 0;
 }
 
@@ -4459,42 +4454,36 @@
   }
 #endif
 
-  gc_sweep ();
-
-  /* Look thru every buffer's undo list for elements that used to
-     contain update markers that were changed to Lisp_Misc_Free
-     objects and delete them.  This may leave a few cons cells
-     unchained, but we will get those on the next sweep.  */
+  /* Everything is now marked, except for the things that require special
+     finalization, i.e. the undo_list.
+     Look thru every buffer's undo list
+     for elements that update markers that were not marked,
+     and delete them.  */
   {
     register struct buffer *nextb = all_buffers;
 
     while (nextb)
       {
 	/* If a buffer's undo list is Qt, that means that undo is
-	   turned off in that buffer.  */
+	   turned off in that buffer.  Calling truncate_undo_list on
+	   Qt tends to return NULL, which effectively turns undo back on.
+	   So don't call truncate_undo_list if undo_list is Qt.  */
 	if (! EQ (nextb->undo_list, Qt))
 	  {
-	    Lisp_Object tail, prev, elt, car;
+	    Lisp_Object tail, prev;
 	    tail = nextb->undo_list;
 	    prev = Qnil;
 	    while (CONSP (tail))
 	      {
-		if ((elt = XCAR (tail), GC_CONSP (elt))
-		    && (car = XCAR (elt), GC_MISCP (car))
-		    && XMISCTYPE (car) == Lisp_Misc_Free)
-		  {
-		    Lisp_Object cdr = XCDR (tail);
-		    /* Do not use free_cons here, as we don't know if
-		       anybody else has a pointer to these conses.  */
-		    XSETCAR (elt, Qnil);
-		    XSETCDR (elt, Qnil);
-		    XSETCAR (tail, Qnil);
-		    XSETCDR (tail, Qnil);
+		if (GC_CONSP (XCAR (tail))
+		    && GC_MARKERP (XCAR (XCAR (tail)))
+		    && !XMARKER (XCAR (XCAR (tail)))->gcmarkbit)
+		  {
 		    if (NILP (prev))
-		      nextb->undo_list = tail = cdr;
+		      nextb->undo_list = tail = XCDR (tail);
 		    else
 		      {
-			tail = cdr;
+			tail = XCDR (tail);
 			XSETCDR (prev, tail);
 		      }
 		  }
@@ -4505,22 +4494,15 @@
 		  }
 	      }
 	  }
+	/* Now that we have stripped the elements that need not be in the
+	   undo_list any more, we can finally mark the list.  */
+	mark_object (nextb->undo_list);
 
 	nextb = nextb->next;
       }
   }
 
-  /* Undo lists have been cleaned up, so we can free marker blocks now.  */
-
-  {
-    struct marker_block *mblk;
-
-    while ((mblk = marker_blocks_pending_free) != 0)
-      {
-	marker_blocks_pending_free = mblk->next;
-	lisp_free (mblk);
-      }
-  }
+  gc_sweep ();
 
   /* Clear the mark bits that we set in certain root slots.  */
 
@@ -5088,41 +5070,9 @@
 
   MARK_INTERVAL_TREE (BUF_INTERVALS (buffer));
 
-  if (CONSP (buffer->undo_list))
-    {
-      Lisp_Object tail;
-      tail = buffer->undo_list;
-
-      /* We mark the undo list specially because
-	 its pointers to markers should be weak.  */
-
-      while (CONSP (tail))
-	{
-	  register struct Lisp_Cons *ptr = XCONS (tail);
-
-	  if (CONS_MARKED_P (ptr))
-	    break;
-	  CONS_MARK (ptr);
-	  if (GC_CONSP (ptr->car)
-	      && !CONS_MARKED_P (XCONS (ptr->car))
-	      && GC_MARKERP (XCAR (ptr->car)))
-	    {
-	      CONS_MARK (XCONS (ptr->car));
-	      mark_object (XCDR (ptr->car));
-	    }
-	  else
-	    mark_object (ptr->car);
-
-	  if (CONSP (ptr->cdr))
-	    tail = ptr->cdr;
-	  else
-	    break;
-	}
-
-      mark_object (XCDR (tail));
-    }
-  else
-    mark_object (buffer->undo_list);
+  /* For now, we just don't mark the undo_list.  It's done later in
+     a special way just before the sweep phase, and after stripping
+     some of its elements that are not needed any more.  */
 
   if (buffer->overlays_before)
     {
@@ -5202,6 +5152,16 @@
 static void
 gc_sweep ()
 {
+  /* Remove or mark entries in weak hash tables.
+     This must be done before any object is unmarked.  */
+  sweep_weak_hash_tables ();
+
+  sweep_strings ();
+#ifdef GC_CHECK_STRING_BYTES
+  if (!noninteractive)
+    check_string_bytes (1);
+#endif
+
   /* Put all unmarked conses on free list */
   {
     register struct cons_block *cblk;
@@ -5252,16 +5212,6 @@
     total_free_conses = num_free;
   }
 
-  /* Remove or mark entries in weak hash tables.
-     This must be done before any object is unmarked.  */
-  sweep_weak_hash_tables ();
-
-  sweep_strings ();
-#ifdef GC_CHECK_STRING_BYTES
-  if (!noninteractive)
-    check_string_bytes (1);
-#endif
-
   /* Put all unmarked floats on free list */
   {
     register struct float_block *fblk;
@@ -5430,7 +5380,6 @@
     register int num_free = 0, num_used = 0;
 
     marker_free_list = 0;
-    marker_blocks_pending_free = 0;
 
     for (mblk = marker_block; mblk; mblk = *mprev)
       {
@@ -5466,13 +5415,8 @@
 	    *mprev = mblk->next;
 	    /* Unhook from the free list.  */
 	    marker_free_list = mblk->markers[0].u_free.chain;
+	    lisp_free (mblk);
 	    n_marker_blocks--;
-
-	    /* It is not safe to free the marker block at this stage,
-	       since there may still be pointers to these markers from
-	       a buffer's undo list.  KFS 2004-05-25.  */
-	    mblk->next = marker_blocks_pending_free;
-	    marker_blocks_pending_free = mblk;
 	  }
 	else
 	  {

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel