Re: Failing to GC killed buffers considered harmful

[Stefan Monnier]

> FWIW, maybe we should get rid of all_buffers.
> AFAICT the only reason we need it is to unmark the buffers during the
> sweep phase, and we could allocate buffers like any other
> pseudovector instead.

The patch below seems to work.

The only worrisome part I think is that `live_buffer_holding` allowed
pointers into buffer objects, whereas `live_vector_p` only treats
pointers to the beginning of the object as a valid reference.

Not sure why buffers would be more likely to have valid pointers into
them (and can't remember discussions about that either), so I assume the
difference was not important (it just happened to be easier to support
that for buffers).

Any objection?


        Stefan


diff --git a/src/alloc.c b/src/alloc.c
index eed73bcdc4..a5e6ad038f 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -432,7 +432,6 @@ no_sanitize_memcpy (void *dest, void const *src, size_t 
size)
 enum mem_type
 {
   MEM_TYPE_NON_LISP,
-  MEM_TYPE_BUFFER,
   MEM_TYPE_CONS,
   MEM_TYPE_STRING,
   MEM_TYPE_SYMBOL,
@@ -3331,12 +3330,10 @@ allocate_pseudovector (int memlen, int lisplen,
 struct buffer *
 allocate_buffer (void)
 {
-  struct buffer *b = lisp_malloc (sizeof *b, false, MEM_TYPE_BUFFER);
-
+  struct buffer *b
+    = ALLOCATE_PSEUDOVECTOR (struct buffer, cursor_in_non_selected_windows_,
+                            PVEC_BUFFER);
   BUFFER_PVEC_INIT (b);
-  /* Put B on the chain of all buffers including killed ones.  */
-  b->next = all_buffers;
-  all_buffers = b;
   /* Note that the rest fields of B are not initialized.  */
   return b;
 }
@@ -4596,40 +4593,6 @@ live_vector_p (struct mem_node *m, void *p)
   return !NILP (live_vector_holding (m, p));
 }
 
-/* If P is a pointer into a valid buffer object, return the buffer.
-   Otherwise, return nil.  M is a pointer to the mem_block for P.
-   If IGNORE_KILLED is non-zero, treat killed buffers as invalid.  */
-
-static Lisp_Object
-live_buffer_holding (struct mem_node *m, void *p, bool ignore_killed)
-{
-  /* P must point into the block, and the buffer must not
-     have been killed unless ALL-BUFFERS is true.  */
-  if (m->type == MEM_TYPE_BUFFER)
-    {
-      struct buffer *b = m->start;
-      char *cb = m->start;
-      char *cp = p;
-      ptrdiff_t offset = cp - cb;
-      if (0 <= offset && offset < sizeof *b
-         && !(ignore_killed && NILP (b->name_)))
-       {
-         Lisp_Object obj;
-         XSETBUFFER (obj, b);
-         return obj;
-       }
-    }
-  return Qnil;
-}
-
-/* If P is a pointer into a live (valid and not killed) buffer object,
-   return non-zero.  */
-static bool
-live_buffer_p (struct mem_node *m, void *p)
-{
-  return !NILP (live_buffer_holding (m, p, true));
-}
-
 /* Mark OBJ if we can prove it's a Lisp_Object.  */
 
 static void
@@ -4684,8 +4647,7 @@ mark_maybe_object (Lisp_Object obj)
          break;
 
        case Lisp_Vectorlike:
-         mark_p = (EQ (obj, live_vector_holding (m, po))
-                   || EQ (obj, live_buffer_holding (m, po, false)));
+         mark_p = (EQ (obj, live_vector_holding (m, po)));
          break;
 
        default:
@@ -4754,10 +4716,6 @@ mark_maybe_pointer (void *p)
          /* Nothing to do; not a pointer to Lisp memory.  */
          break;
 
-       case MEM_TYPE_BUFFER:
-         obj = live_buffer_holding (m, p, false);
-         break;
-
        case MEM_TYPE_CONS:
          obj = live_cons_holding (m, p);
          break;
@@ -5157,9 +5115,6 @@ valid_lisp_object_p (Lisp_Object obj)
     case MEM_TYPE_SPARE:
       return 0;
 
-    case MEM_TYPE_BUFFER:
-      return live_buffer_p (m, p) ? 1 : 2;
-
     case MEM_TYPE_CONS:
       return live_cons_p (m, p);
 
@@ -5976,7 +5931,7 @@ maybe_garbage_collect (void)
 void
 garbage_collect (void)
 {
-  struct buffer *nextb;
+  Lisp_Object tail, buffer;
   char stack_top_variable;
   bool message_p;
   ptrdiff_t count = SPECPDL_INDEX ();
@@ -5992,8 +5947,8 @@ garbage_collect (void)
 
   /* Don't keep undo information around forever.
      Do this early on, so it is no problem if the user quits.  */
-  FOR_EACH_BUFFER (nextb)
-    compact_buffer (nextb);
+  FOR_EACH_LIVE_BUFFER (tail, buffer)
+    compact_buffer (XBUFFER (buffer));
 
   byte_ct tot_before = (profiler_memory_running
                        ? total_bytes_of_live_objects ()
@@ -6083,8 +6038,9 @@ garbage_collect (void)
 
   compact_font_caches ();
 
-  FOR_EACH_BUFFER (nextb)
+  FOR_EACH_LIVE_BUFFER (tail, buffer)
     {
+      struct buffer *nextb = XBUFFER (buffer);
       if (!EQ (BVAR (nextb, undo_list), Qt))
        bset_undo_list (nextb, compact_undo_list (BVAR (nextb, undo_list)));
       /* Now that we have stripped the elements that need not be
@@ -6613,23 +6569,12 @@ #define CHECK_ALLOCATED_AND_LIVE_SYMBOL()       ((void) 
0)
           = PSEUDOVECTOR_TYPE (ptr);
 
         if (pvectype != PVEC_SUBR &&
-            pvectype != PVEC_BUFFER &&
             !main_thread_p (po))
           CHECK_LIVE (live_vector_p);
 
        switch (pvectype)
          {
          case PVEC_BUFFER:
-#if GC_CHECK_MARKED_OBJECTS
-           {
-             struct buffer *b;
-             FOR_EACH_BUFFER (b)
-               if (b == po)
-                 break;
-             if (b == NULL)
-               emacs_abort ();
-           }
-#endif /* GC_CHECK_MARKED_OBJECTS */
            mark_buffer ((struct buffer *) ptr);
             break;
 
@@ -7108,25 +7053,17 @@ unchain_dead_markers (struct buffer *buffer)
 static void
 sweep_buffers (void)
 {
-  struct buffer *buffer, **bprev = &all_buffers;
+  Lisp_Object tail, buf;
 
   gcstat.total_buffers = 0;
-  for (buffer = all_buffers; buffer; buffer = *bprev)
-    if (!vectorlike_marked_p (&buffer->header))
-      {
-        *bprev = buffer->next;
-        lisp_free (buffer);
-      }
-    else
-      {
-        if (!pdumper_object_p (buffer))
-          XUNMARK_VECTOR (buffer);
-        /* Do not use buffer_(set|get)_intervals here.  */
-        buffer->text->intervals = balance_intervals (buffer->text->intervals);
-        unchain_dead_markers (buffer);
-       gcstat.total_buffers++;
-        bprev = &buffer->next;
-      }
+  FOR_EACH_LIVE_BUFFER (tail, buf)
+    {
+      struct buffer *buffer = XBUFFER (buf);
+      /* Do not use buffer_(set|get)_intervals here.  */
+      buffer->text->intervals = balance_intervals (buffer->text->intervals);
+      unchain_dead_markers (buffer);
+      gcstat.total_buffers++;
+    }
 }
 
 /* Sweep: find all structures not marked, and free them.  */
diff --git a/src/buffer.c b/src/buffer.c
index 4e121ca4ca..355b34a980 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -51,11 +51,6 @@ Copyright (C) 1985-1989, 1993-1995, 1997-2020 Free Software 
Foundation,
 #include "w32heap.h"           /* for mmap_* */
 #endif
 
-/* First buffer in chain of all buffers (in reverse order of creation).
-   Threaded through ->header.next.buffer.  */
-
-struct buffer *all_buffers;
-
 /* This structure holds the default values of the buffer-local variables
    defined with DEFVAR_PER_BUFFER, that have special slots in each buffer.
    The default value occupies the same slot in this structure
@@ -1786,15 +1781,11 @@ DEFUN ("kill-buffer", Fkill_buffer, Skill_buffer, 0, 1, 
"bKill buffer: ",
      ask questions or their hooks get errors.  */
   if (!b->base_buffer && b->indirections > 0)
     {
-      struct buffer *other;
+      Lisp_Object tail, other;
 
-      FOR_EACH_BUFFER (other)
-       if (other->base_buffer == b)
-         {
-           Lisp_Object buf;
-           XSETBUFFER (buf, other);
-           Fkill_buffer (buf);
-         }
+      FOR_EACH_LIVE_BUFFER (tail, other)
+       if (XBUFFER (other)->base_buffer == b)
+         Fkill_buffer (other);
 
       /* Exit if we now have killed the base buffer (Bug#11665).  */
       if (!BUFFER_LIVE_P (b))
@@ -2351,10 +2342,10 @@ DEFUN ("buffer-swap-text", Fbuffer_swap_text, 
Sbuffer_swap_text,
     error ("Cannot swap indirect buffers's text");
 
   { /* This is probably harder to make work.  */
-    struct buffer *other;
-    FOR_EACH_BUFFER (other)
-      if (other->base_buffer == other_buffer
-         || other->base_buffer == current_buffer)
+    Lisp_Object tail, other;
+    FOR_EACH_LIVE_BUFFER (tail, other)
+      if (XBUFFER (other)->base_buffer == other_buffer
+         || XBUFFER (other)->base_buffer == current_buffer)
        error ("One of the buffers to swap has indirect buffers");
   }
 
@@ -2502,7 +2493,7 @@ DEFUN ("set-buffer-multibyte", Fset_buffer_multibyte, 
Sset_buffer_multibyte,
   (Lisp_Object flag)
 {
   struct Lisp_Marker *tail, *markers;
-  struct buffer *other;
+  Lisp_Object btail, other;
   ptrdiff_t begv, zv;
   bool narrowed = (BEG != BEGV || Z != ZV);
   bool modified_p = !NILP (Fbuffer_modified_p (Qnil));
@@ -2755,13 +2746,16 @@ DEFUN ("set-buffer-multibyte", Fset_buffer_multibyte, 
Sset_buffer_multibyte,
 
   /* Copy this buffer's new multibyte status
      into all of its indirect buffers.  */
-  FOR_EACH_BUFFER (other)
-    if (other->base_buffer == current_buffer && BUFFER_LIVE_P (other))
-      {
-       BVAR (other, enable_multibyte_characters)
-         = BVAR (current_buffer, enable_multibyte_characters);
-       other->prevent_redisplay_optimizations_p = 1;
-      }
+  FOR_EACH_LIVE_BUFFER (btail, other)
+    {
+      struct buffer *o = XBUFFER (other);
+      if (o->base_buffer == current_buffer && BUFFER_LIVE_P (o))
+       {
+         BVAR (o, enable_multibyte_characters)
+           = BVAR (current_buffer, enable_multibyte_characters);
+         o->prevent_redisplay_optimizations_p = true;
+       }
+    }
 
   /* Restore the modifiedness of the buffer.  */
   if (!modified_p && !NILP (Fbuffer_modified_p (Qnil)))
@@ -5327,8 +5321,6 @@ init_buffer_once (void)
   Vbuffer_alist = Qnil;
   current_buffer = 0;
   pdumper_remember_lv_ptr_raw (&current_buffer, Lisp_Vectorlike);
-  all_buffers = 0;
-  pdumper_remember_lv_ptr_raw (&all_buffers, Lisp_Vectorlike);
 
   QSFundamental = build_pure_c_string ("Fundamental");
 
@@ -5359,7 +5351,7 @@ init_buffer (void)
 #ifdef USE_MMAP_FOR_BUFFERS
   if (dumped_with_unexec_p ())
     {
-      struct buffer *b;
+      Lisp_Object tail, buffer;
 
 #ifndef WINDOWSNT
       /* These must be reset in the dumped Emacs, to avoid stale
@@ -5381,8 +5373,9 @@ init_buffer (void)
         " *code-conversion-work*".  They are created by
         init_buffer_once and init_window_once (which are not called
         in the dumped Emacs), and by the first call to coding.c routines.  */
-      FOR_EACH_BUFFER (b)
+      FOR_EACH_LIVE_BUFFER (tail, buffer)
         {
+         struct buffer *b = XBUFFER (buffer);
          b->text->beg = NULL;
          enlarge_buffer_text (b, 0);
        }
diff --git a/src/buffer.h b/src/buffer.h
index 31f497ea40..abb1294d03 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -570,9 +570,6 @@ #define BVAR(buf, field) ((buf)->field ## _)
      In an indirect buffer, this is the own_text field of another buffer.  */
   struct buffer_text *text;
 
-  /* Next buffer, in chain of all buffers, including killed ones.  */
-  struct buffer *next;
-
   /* Char position of point in buffer.  */
   ptrdiff_t pt;
 
@@ -1104,15 +1101,6 @@ BUFFER_CHECK_INDIRECTION (struct buffer *b)
     }
 }
 
-/* Chain of all buffers, including killed ones.  */
-
-extern struct buffer *all_buffers;
-
-/* Used to iterate over the chain above.  */
-
-#define FOR_EACH_BUFFER(b) \
-  for ((b) = all_buffers; (b); (b) = (b)->next)
-
 /* This structure holds the default values of the buffer-local variables
    that have special slots in each buffer.
    The default value occupies the same slot in this structure
diff --git a/src/pdumper.c b/src/pdumper.c
index e52163cdae..0f3835578a 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -2845,8 +2844,6 @@ dump_buffer (struct dump_context *ctx, const struct 
buffer *in_buffer)
      ctx->obj_offset + dump_offsetof (struct buffer, text),
      base_offset + dump_offsetof (struct buffer, own_text));
 
-  dump_field_lv_rawptr (ctx, out, buffer, &buffer->next,
-                        Lisp_Vectorlike, WEIGHT_NORMAL);
   DUMP_FIELD_COPY (out, buffer, pt);
   DUMP_FIELD_COPY (out, buffer, pt_byte);
   DUMP_FIELD_COPY (out, buffer, begv);