bug#59029: Dumping Emacs crashes when buffers have overlays

[Matt Armstrong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> IMO, we should have a proper separate pdump test, instead.

I tend to agree with Stefan about the relatively low importance of
supporting overlays owned by buffers in the dump.  I have deferred that
work, though the tests I have added will be useful if someone wants to
take it up in the future.

See attached patch, which:

a) Adds a test that exercises portable dumping from a batch mode
subprocess, using the Emacs executable hosting the test as a basis.

b) Performs the dump while a buffer with overlays is live in the
subprocess.

c) Verifies the dump by running a second subprocess and checking how
many overlays are in the buffer that had overlays.

The test took most of the time.  I'm quite open to suggestions there; my
elisp knowledge is novice level.

>From eee0419f7d702e67671197a5c65c83b77af3c489 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Tue, 8 Nov 2022 17:10:02 -0800
Subject: [PATCH] Fix portable dumping when buffers have overlays.

See bug#59029.  While it is possible to fully support overlays in dump
files, the code involved is non-trivial and we have decided to defer
this work.  Instead, a buffer's overlays are dumped only if they are
reachable from something other than their buffer, and as if they had
been deleted.

* src/itree.c (itree_node_init): Initialize fields in declared order.
Zero-initialize unspecified fields.
* src/pdumper.c (dump_itree_node): Always dump as if the node had
been deleted from its buffer.  Renamed from dump_interval_node.
(dump_buffer): Never dump a buffer's overlays.
* test/src/pdumper-tests.el: New test to exercise the above.
---
 src/itree.c               |  15 +++--
 src/pdumper.c             |  50 ++++----------
 test/src/pdumper-tests.el | 133 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+), 46 deletions(-)
 create mode 100644 test/src/pdumper-tests.el

diff --git a/src/itree.c b/src/itree.c
index 989173db4e..fecad886e3 100644
--- a/src/itree.c
+++ b/src/itree.c
@@ -534,14 +534,15 @@ itree_node_init (struct itree_node *node,
                 bool front_advance, bool rear_advance,
                 Lisp_Object data)
 {
-  node->parent = NULL;
-  node->left = NULL;
-  node->right = NULL;
-  node->begin = -1;
-  node->end = -1;
-  node->front_advance = front_advance;
-  node->rear_advance = rear_advance;
+  node->begin = 0;
+  node->end = 0;
+  node->limit = 0;
+  node->offset = 0;
+  node->otick = 0;
   node->data = data;
+  node->red = 0;
+  node->rear_advance = rear_advance;
+  node->front_advance = front_advance;
 }
 
 /* Return NODE's begin value, computing it if necessary. */
diff --git a/src/pdumper.c b/src/pdumper.c
index 0a5d96dbb7..652dfc2e71 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -2134,46 +2134,20 @@ dump_marker (struct dump_context *ctx, const struct 
Lisp_Marker *marker)
 }
 
 static dump_off
-dump_interval_node (struct dump_context *ctx, struct itree_node *node,
-                    dump_off parent_offset)
+dump_itree_node (struct dump_context *ctx, struct itree_node *node,
+                dump_off parent_offset)
 {
 #if CHECK_STRUCTS && !defined (HASH_itree_node_50DE304F13)
 # error "itree_node changed. See CHECK_STRUCTS comment in config.h."
 #endif
+  /* Dumping of a buffer's overlays is not implemented.  Overlays are
+     dumped as if they had been deleted.  See bug#59029.  */
   struct itree_node out;
   dump_object_start (ctx, &out, sizeof (out));
-  if (node->parent)
-    dump_field_fixup_later (ctx, &out, node, &node->parent);
-  if (node->left)
-    dump_field_fixup_later (ctx, &out, node, &node->parent);
-  if (node->right)
-    dump_field_fixup_later (ctx, &out, node, &node->parent);
-  DUMP_FIELD_COPY (&out, node, begin);
-  DUMP_FIELD_COPY (&out, node, end);
-  DUMP_FIELD_COPY (&out, node, limit);
-  DUMP_FIELD_COPY (&out, node, offset);
-  DUMP_FIELD_COPY (&out, node, otick);
+  itree_node_init (&out, node->front_advance, node->rear_advance,
+                  node->data);
   dump_field_lv (ctx, &out, node, &node->data, WEIGHT_STRONG);
-  DUMP_FIELD_COPY (&out, node, red);
-  DUMP_FIELD_COPY (&out, node, rear_advance);
-  DUMP_FIELD_COPY (&out, node, front_advance);
-  dump_off offset = dump_object_finish (ctx, &out, sizeof (out));
-  if (node->parent)
-      dump_remember_fixup_ptr_raw
-       (ctx,
-        offset + dump_offsetof (struct itree_node, parent),
-        dump_interval_node (ctx, node->parent, offset));
-  if (node->left)
-      dump_remember_fixup_ptr_raw
-       (ctx,
-        offset + dump_offsetof (struct itree_node, left),
-        dump_interval_node (ctx, node->left, offset));
-  if (node->right)
-      dump_remember_fixup_ptr_raw
-       (ctx,
-        offset + dump_offsetof (struct itree_node, right),
-        dump_interval_node (ctx, node->right, offset));
-  return offset;
+  return dump_object_finish (ctx, &out, sizeof (out));
 }
 
 static dump_off
@@ -2189,7 +2163,7 @@ dump_overlay (struct dump_context *ctx, const struct 
Lisp_Overlay *overlay)
   dump_remember_fixup_ptr_raw
     (ctx,
      offset + dump_offsetof (struct Lisp_Overlay, interval),
-     dump_interval_node (ctx, overlay->interval, offset));
+     dump_itree_node (ctx, overlay->interval, offset));
   return offset;
 }
 
@@ -2863,11 +2837,9 @@ dump_buffer (struct dump_context *ctx, const struct 
buffer *in_buffer)
   DUMP_FIELD_COPY (out, buffer, inhibit_buffer_hooks);
   DUMP_FIELD_COPY (out, buffer, long_line_optimizations_p);
 
-  if (buffer->overlays && buffer->overlays->root != NULL)
-    /* We haven't implemented the code to dump overlays.  */
-    emacs_abort ();
-  else
-    out->overlays = NULL;
+  /* Dumping of a buffer's overlays is not implemented.  See
+     bug#59029.  */
+  out->overlays = NULL;
 
   dump_field_lv (ctx, out, buffer, &buffer->undo_list_,
                  WEIGHT_STRONG);
diff --git a/test/src/pdumper-tests.el b/test/src/pdumper-tests.el
new file mode 100644
index 0000000000..18b7a7afb5
--- /dev/null
+++ b/test/src/pdumper-tests.el
@@ -0,0 +1,133 @@
+;;; pdumper-tests.el --- pdumper tests               -*- lexical-binding: t; 
-*-
+
+;; Copyright (C) 2022  Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert-x)
+
+(defun pdumper-tests--readable-arg (form)
+  (let ((print-gensym t)
+        (print-circle t)
+        (print-length nil)
+        (print-level nil)
+        (print-escape-control-characters t)
+        (print-escape-newlines t)
+        (print-escape-multibyte t)
+        (print-escape-nonascii t))
+    (if-let ((str (readablep form)))
+        str
+      (error "unreadable form %S" form))))
+
+(defun pdumper-tests--eval-arg (form)
+  (concat "--eval=" (pdumper-tests--readable-arg form)))
+
+(defun pdumper-tests--batch-eval-command (emacs form)
+  `(,@emacs "--quick" "--batch"
+            ,(pdumper-tests--eval-arg form)))
+
+(defun pdumper-tests--emacs-binary ()
+  (expand-file-name invocation-name invocation-directory))
+
+(defun pdumper-tests--check-output (dir command)
+  "Run COMMAND as a synchronous subprocess.
+
+The process' current directory is set to DIR.  Fail the test if
+the process exits with a non-zero status.  Otherwise, return its
+output as a string."
+  (with-temp-buffer
+    (setq default-directory dir)
+    (let ((result (apply #'call-process
+                         (car command)
+                         nil t nil
+                         (cdr command))))
+      (unless (equal 0 result)
+        (ert-info ((format "In directory %S" dir))
+          (ert-info ((format "Ran command %S" command))
+            (ert-info ((format "With output %S" (buffer-string)))
+              (should (equal 0 result)))))))
+    (buffer-string)))
+
+(defun pdumper-tests--batch-redump (loadup-form redumped-form)
+  "Redump the current Emacs binary and verify it works.
+
+First, run the current Emacs binary in a synchronous subprocess.
+Evaluate LOADUP-FORM in it, then dump it with
+`dump-emacs-portable'.
+
+Second, run the current Emacs binary in a second subprocess, this
+time with \"--dump-file\" set to the dump file just created.
+Evaluate REDUMPED-FORM in this second process.
+
+Returns the output of the second run as a string."
+  (ert-with-temp-directory tmpdir
+    (let* ((dump-file (concat tmpdir "redump.pdmp"))
+           (loadup-file (concat tmpdir "redump.el"))
+           (emacs-binary (pdumper-tests--emacs-binary))
+           (dump-command (pdumper-tests--batch-eval-command
+                          (list emacs-binary)
+                          `(progn
+                             (load ,loadup-file)
+                             (dump-emacs-portable ,dump-file)))))
+      (with-temp-file loadup-file
+        (insert (prin1-to-string loadup-form)))
+      (should (string-search "Dump complete"
+                             (pdumper-tests--check-output
+                              tmpdir dump-command)))
+      (let* ((redumped-emacs-command
+              (list emacs-binary
+                    (concat "--dump-file=" dump-file)))
+             (redumped-emacs-validate
+              (pdumper-tests--batch-eval-command
+               redumped-emacs-command
+               redumped-form)))
+        (pdumper-tests--check-output
+         tmpdir
+         redumped-emacs-validate)))))
+
+(ert-deftest pdumper-tests-redump-overlays ()
+  "Test portable dumping when overlays are in a buffer.
+
+In an Emacs subprocess create a buffer with some overlays, then
+dump it.  Run with the new dump file and verify the buffer exists
+and is in the correct state."
+  (skip-unless (fboundp 'pdumper-stats))
+  (let* ((overlay-buffer " *redump-overlays*")
+         (phrase "Lorem ipsum dolor sit amet, consectetur...")
+         (redump-output
+          (pdumper-tests--batch-redump
+           `(with-current-buffer (get-buffer-create ,overlay-buffer)
+              (insert ,phrase)
+              (dotimes (i 8)
+                (make-overlay (+ (point-min) i)
+                              (+ (point-min) (+ i 10)))))
+           `(with-current-buffer (get-buffer-create ,overlay-buffer)
+              (unless (equal ,phrase (buffer-string))
+                (error "buffer text not as expected"))
+              (let ((overlays (overlays-in (point-min) (point-max))))
+                (princ (format "Redumped overlay count: <%d>"
+                               (length overlays))))))))
+    ;; Dumping of a buffer's overlays is not implemented.  Overlays
+    ;; are dumped as if they had been deleted, so the redumped Emacs
+    ;; should have zero overlays in the *redump-overlays* buffer.  See
+    ;; bug#59029.  */
+    (should (string-search "Redumped overlay count: <0>"
+                           redump-output))))
+
+(provide 'pdumper-tests)
+;;; pdumper-tests.el ends here
-- 
2.35.1