[Emacs-diffs] pdumper 3290cda: Fix conservative GC bugs

[Daniel Colascione]

branch: pdumper
commit 3290cda6743d25f20e78312296f26fba3749d6f2
Author: Daniel Colascione <address@hidden>
Commit: Daniel Colascione <address@hidden>

    Fix conservative GC bugs
---
 src/alloc.c   | 37 ++++++++++++++++++++++---------------
 src/pdumper.c | 30 +++++++++++++++++++++++++-----
 src/pdumper.h | 23 ++++++++++++++++++++---
 3 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index be34371..0070b46 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4120,13 +4120,13 @@ vector_marked_p (const struct Lisp_Vector *v)
 {
   if (pdumper_object_p (v))
     {
-      /* TODO: look into using a range test against
-         hot_discardable_start instead of loading the pvec header.
-         We'll have already loaded the dump header cache line, after
-         all.  */
-      enum pvec_type pvectype = PSEUDOVECTOR_TYPE (v);
-      if (pvectype == PVEC_BOOL_VECTOR)
-        return true;
+      /* Look at cold_start first so that we don't have to fault in
+         the vector header just to tell that it's a bool vector.  */
+      if (pdumper_cold_object_p (v))
+        {
+          eassert (PSEUDOVECTOR_TYPE (v) == PVEC_BOOL_VECTOR);
+          return true;
+        }
       return pdumper_marked_p (v);
     }
   return XVECTOR_MARKED_P (v);
@@ -5014,7 +5014,10 @@ mark_maybe_object (Lisp_Object obj)
      definitely _don't_ have an object.  */
   if (pdumper_object_p (po))
     {
-      if (pdumper_object_p_precise (po))
+      /* Don't use pdumper_object_p_precise here! It doesn't check the
+         tag bits. OBJ here might be complete garbage, so we need to
+         verify both the pointer and the tag.  */
+      if (XTYPE (obj) == pdumper_find_object_type (po))
         mark_object (obj);
       return;
     }
@@ -6968,11 +6971,14 @@ mark_object (Lisp_Object arg)
            mark_char_table (ptr, (enum pvec_type) pvectype);
            break;
 
-         case PVEC_BOOL_VECTOR:
-            /* Do not mark bool vectors in a dump image: these objects
-               are "cold" and don't have mark bits.  */
-            if (!pdumper_object_p (ptr))
-              set_vector_marked (ptr);
+          case PVEC_BOOL_VECTOR:
+            /* bool vectors in a dump are permanently "marked", since
+               they're in the old section and don't have mark bits.
+               If we're looking at a dumped bool vector, we should
+               have aborted above when we called vector_marked_p(), so
+               we should never get here.  */
+            eassert (!pdumper_object_p (ptr));
+            set_vector_marked (ptr);
            break;
 
          case PVEC_SUBR:
@@ -7097,8 +7103,9 @@ mark_object (Lisp_Object arg)
       CHECK_ALLOCATED_AND_LIVE (live_float_p);
       /* Do not mark floats stored in a dump image: these floats are
          "cold" and do not have mark bits.  */
-      if (!pdumper_object_p (XFLOAT (obj)) &&
-          !XFLOAT_MARKED_P (XFLOAT (obj)))
+      if (pdumper_object_p (XFLOAT (obj)))
+        eassert (pdumper_cold_object_p (XFLOAT (obj)));
+      else if (!XFLOAT_MARKED_P (XFLOAT (obj)))
         XFLOAT_MARK (XFLOAT (obj));
       break;
 
diff --git a/src/pdumper.c b/src/pdumper.c
index 148dcd9..15a3c1b 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -422,6 +422,11 @@ struct dump_flags
      referents normally, but dump an object itself separately,
      later.  */
   bool_bf dump_object_contents : 1;
+  /* Record object starts. We turn this flag off when writing to the
+     discardable section so that we don't trick conservative GC into
+     thinking we have objects there.  Ignored (we never record object
+     starts) if dump_object_contents is false.  */
+  bool_bf dump_object_starts : 1;
   /* Pack objects tighter than GC memory alignment would normally
      require.  Useful for objects copied into the Emacs image instead
      of used directly from the loaded dump.
@@ -2146,6 +2151,7 @@ dump_misc_any (struct dump_context *ctx, struct 
Lisp_Misc_Any *misc_any)
 static dump_off
 dump_float (struct dump_context *ctx, const struct Lisp_Float *lfloat)
 {
+  eassert (ctx->header.cold_start);
   struct Lisp_Float out;
   dump_object_start (ctx, GCALIGNMENT, &out, sizeof (out));
   DUMP_FIELD_COPY (&out, lfloat, u.data);
@@ -2907,9 +2913,10 @@ dump_object_1 (struct dump_context *ctx, Lisp_Object 
object)
         {
           eassert (offset % (1<<DUMP_RELOC_ALIGNMENT_BITS) == 0);
           dump_remember_object (ctx, object, offset);
-          dump_push (&ctx->object_starts,
-                     list2 (dump_off_to_lisp (XTYPE (object)),
-                            dump_off_to_lisp (offset)));
+          if (ctx->flags.dump_object_starts)
+            dump_push (&ctx->object_starts,
+                       list2 (dump_off_to_lisp (XTYPE (object)),
+                              dump_off_to_lisp (offset)));
         }
     }
 
@@ -3704,6 +3711,7 @@ types.  */)
      circumstances below, we temporarily change this default
      behavior.  */
   ctx->flags.dump_object_contents = true;
+  ctx->flags.dump_object_starts = true;
 
   /* We want to consolidate certain object types that we know are very likely
      to be modified.  */
@@ -3800,6 +3808,8 @@ types.  */)
      This section consists of objects that need to be memcpy()ed into
      the Emacs data section instead of just used directly.  */
   ctx->header.discardable_start = ctx->offset;
+  ctx->flags.dump_object_starts = false;
+
   dump_copied_objects (ctx);
   eassert (dump_queue_empty_p (&ctx->dump_queue));
   eassert (NILP (ctx->copied_queue));
@@ -3807,6 +3817,10 @@ types.  */)
   dump_align_output (ctx, dump_get_page_size ());
   ctx->header.cold_start = ctx->offset;
 
+  /* Resume recording object starts, since the cold section will stick
+     around.  */
+  ctx->flags.dump_object_starts = true;
+
   /* Start the cold section.  This section contains bytes that should
      never change and so can be direct-mapped from the dump without
      special processing.  */
@@ -4689,9 +4703,13 @@ dump_loaded_p (void)
 }
 
 bool
-pdumper_object_p_precise_impl (const void *obj)
+pdumper_cold_object_p_impl (const void *obj)
 {
-  return pdumper_find_object_type (obj) != PDUMPER_NO_OBJECT;
+  eassert (pdumper_object_p (obj));
+  eassert (pdumper_object_p_precise (obj));
+  dump_off offset = ptrdiff_t_to_dump_off (
+    (intptr_t) obj - dump_public.start);
+  return offset >= dump_private.header.cold_start;
 }
 
 enum Lisp_Type
@@ -4715,6 +4733,7 @@ pdumper_marked_p_impl (const void *obj)
   eassert (pdumper_object_p (obj));
   ptrdiff_t offset = (intptr_t) obj - dump_public.start;
   eassert (offset % GCALIGNMENT == 0);
+  eassert (offset < dump_private.header.cold_start);
   eassert (offset < dump_private.header.discardable_start);
   ptrdiff_t bitno = offset / GCALIGNMENT;
   return dump_bitset_bit_set_p (&dump_private.mark_bits, bitno);
@@ -4726,6 +4745,7 @@ pdumper_set_marked_impl (const void *obj)
   eassert (pdumper_object_p (obj));
   ptrdiff_t offset = (intptr_t) obj - dump_public.start;
   eassert (offset % GCALIGNMENT == 0);
+  eassert (offset < dump_private.header.cold_start);
   eassert (offset < dump_private.header.discardable_start);
   ptrdiff_t bitno = offset / GCALIGNMENT;
   dump_bitset_set_bit (&dump_private.mark_bits, bitno);
diff --git a/src/pdumper.h b/src/pdumper.h
index 3bf80d7..c407619 100644
--- a/src/pdumper.h
+++ b/src/pdumper.h
@@ -157,6 +157,25 @@ pdumper_object_p (const void *obj)
 #endif
 }
 
+extern bool pdumper_cold_object_p_impl (const void *obj);
+
+/* Return whether the OBJ is in the cold section of the dump.
+   Only bool-vectors and floats should end up there.
+   pdumper_object_p() and pdumper_object_p_precise() must have
+   returned true for OBJ before calling this function.  */
+INLINE _GL_ATTRIBUTE_CONST
+bool
+pdumper_cold_object_p (const void *obj)
+{
+#ifdef HAVE_PDUMPER
+  return pdumper_cold_object_p_impl (obj);
+#else
+  (void) obj;
+  return false;
+#endif
+}
+
+
 extern enum Lisp_Type pdumper_find_object_type_impl (const void *obj);
 
 /* Return the type of the dumped object that starts at OBJ.  It is a
@@ -174,8 +193,6 @@ pdumper_find_object_type (const void *obj)
 #endif
 }
 
-extern bool pdumper_object_p_precise_impl (const void *obj);
-
 /* Return whether OBJ points exactly to the start of some object in
    the loaded dump image.  It is a programming error to call this
    routine for an OBJ for which pdumper_object_p would return
@@ -185,7 +202,7 @@ bool
 pdumper_object_p_precise (const void *obj)
 {
 #ifdef HAVE_PDUMPER
-  return pdumper_object_p_precise_impl (obj);
+  return pdumper_find_object_type (obj) != PDUMPER_NO_OBJECT;
 #else
   (void) obj;
   emacs_abort ();