Re: Fix to long-standing crashes in GC

[Stefan Monnier]

> 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)
     {

alloc.patch
Description: Text Data