bug#33014: 26.1.50; 27.0.50; Fatal error after re-evaluating a thread's function

[Paul Eggert <eggert@cs.ucla.edu>]

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

> Any reason not to cherry-pick this to emacs-26?

No, once we fix it up. Although adding 'volatile' happened to work for Gemini's 
compiler, it won't suffice in general as the C standard does not require 
volatile variables to survive their last access (which is what the patch was 
assuming). Furthermore, Fbyte_code bypasses that patch, so the bug could still 
occur even if 'volatile' cured the bug in the more-common code path.

A simple way to ensure that the constant vector survives GC is to have 
exec_byte_code put the vector into a GC-visible slot. As it happens, there's a 
spare slot that we can appropriate, so this won't cost us stack (or heap) space. 
I installed the first attached patch into master to do that, and backported the 
patch series into emacs-26 via the last two attached patches.

Thanks, Gemini, for the good work in debugging this problem and writing that 
test case. GC bugs can be nasty.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-fix-for-Bug-33014.patch --]
[-- Type: text/x-patch; name="0001-Improve-fix-for-Bug-33014.patch", Size: 2262 bytes --]

From cf486a7a920d3d95fa9aa98d7b03ebc61b17518a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 30 Oct 2018 20:57:46 -0700
Subject: [PATCH] Improve fix for Bug#33014
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Although the previously-applied fix worked for its platform,
it doesn’t suffice in general.
* src/bytecode.c (exec_byte_code): Save VECTOR into stack slot
so that it survives GC.  The stack slot was otherwise unused,
so this doesn’t cost us memory, only a store insn.
* src/eval.c (Ffuncall): Do not make FUN volatile, reverting
2018-10-14T19:12:04Z!gazally@runbox.com.  Adding ‘volatile’
does not suffice, since storage for a volatile local can be
reclaimed after its last access (e.g., by tail recursion
elimination), which would make VECTOR invisible to GC.
---
 src/bytecode.c | 1 +
 src/eval.c     | 7 ++-----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/bytecode.c b/src/bytecode.c
index 17457fc574..40389e08f0 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -369,6 +369,7 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
   ptrdiff_t item_bytes = stack_items * word_size;
   Lisp_Object *stack_base = ptr_bounds_clip (alloc, item_bytes);
   Lisp_Object *top = stack_base;
+  *top = vector; /* Ensure VECTOR survives GC (Bug#33014).  */
   Lisp_Object *stack_lim = stack_base + stack_items;
   unsigned char *bytestr_data = alloc;
   bytestr_data = ptr_bounds_clip (bytestr_data + item_bytes, bytestr_length);
diff --git a/src/eval.c b/src/eval.c
index 32cfda24d8..a51d0c9083 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -2820,11 +2820,8 @@ Thus, (funcall \\='cons \\='x \\='y) returns (x . y).
 usage: (funcall FUNCTION &rest ARGUMENTS)  */)
   (ptrdiff_t nargs, Lisp_Object *args)
 {
-  /* Use 'volatile' here to cause optimizing compilers to keep a
-     reference on the stack to the function's bytecode object.  See
-     Bug#33014.  */
-  Lisp_Object volatile fun;
-  Lisp_Object original_fun, funcar;
+  Lisp_Object fun, original_fun;
+  Lisp_Object funcar;
   ptrdiff_t numargs = nargs - 1;
   Lisp_Object val;
   ptrdiff_t count;
-- 
2.17.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Refer-to-bytecode-constant-vectors-Bug-33014.patch --]
[-- Type: text/x-patch; name="0001-Refer-to-bytecode-constant-vectors-Bug-33014.patch", Size: 1200 bytes --]

From 1ad2903a48b682985a2bd0709ec05f67a1351a8e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 30 Oct 2018 21:14:10 -0700
Subject: [PATCH 1/2] Refer to bytecode constant vectors (Bug#33014)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Backport from master.
* src/bytecode.c (exec_byte_code): Save VECTOR into stack slot
so that it survives GC.  The stack slot was otherwise unused,
so this doesn’t cost us memory, only a store insn.
---
 src/bytecode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/bytecode.c b/src/bytecode.c
index e51f9095b3..538cd4f3ca 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -367,6 +367,7 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
   SAFE_ALLOCA_LISP_EXTRA (stack_base, stack_items, bytestr_length);
   Lisp_Object *stack_lim = stack_base + stack_items;
   Lisp_Object *top = stack_base;
+  *top = vector; /* Ensure VECTOR survives GC (Bug#33014).  */
   memcpy (stack_lim, SDATA (bytestr), bytestr_length);
   void *void_stack_lim = stack_lim;
   unsigned char const *bytestr_data = void_stack_lim;
-- 
2.17.1


[-- Attachment #4: 0002-Add-regression-test-for-Bug-33014.patch --]
[-- Type: text/x-patch, Size: 2169 bytes --]

From c3cf85b1c186e13c2d588aa35ffa57981ca481d7 Mon Sep 17 00:00:00 2001
From: Gemini Lasswell <gazally@runbox.com>
Date: Tue, 30 Oct 2018 21:15:51 -0700
Subject: [PATCH 2/2] Add regression test for Bug#33014

Backport from master.
* test/src/eval-tests.el:
(eval-tests-byte-code-being-evaluated-is-protected-from-gc): New test.
(eval-tests-33014-var): New variable.
(eval-tests-33014-func, eval-tests-33014-redefine): New functions.
---
 test/src/eval-tests.el | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/test/src/eval-tests.el b/test/src/eval-tests.el
index e68fd13611..eeb98b0994 100644
--- a/test/src/eval-tests.el
+++ b/test/src/eval-tests.el
@@ -99,4 +99,34 @@ eval-tests--exceed-specbind-limit
         (signal-hook-function #'ignore))
     (should-error (eval-tests--exceed-specbind-limit))))
 
+(ert-deftest eval-tests-byte-code-being-evaluated-is-protected-from-gc ()
+  "Regression test for Bug#33014.
+Check that byte-compiled objects being executed by exec-byte-code
+are found on the stack and therefore not garbage collected."
+  (should (string= (eval-tests-33014-func)
+                   "before after: ok foo: (e) bar: (a b c d e) baz: a bop: c")))
+
+(defvar eval-tests-33014-var "ok")
+(defun eval-tests-33014-func ()
+  "A function which has a non-trivial constants vector when byte-compiled."
+  (let ((result "before "))
+    (eval-tests-33014-redefine)
+    (garbage-collect)
+    (setq result (concat result (format "after: %s" eval-tests-33014-var)))
+    (let ((vals '(0 1 2 3))
+          (things '(a b c d e)))
+      (dolist (val vals)
+        (setq result
+              (concat result " "
+                      (cond
+                       ((= val 0) (format "foo: %s" (last things)))
+                       ((= val 1) (format "bar: %s" things))
+                       ((= val 2) (format "baz: %s" (car things)))
+                       (t (format "bop: %s" (nth 2 things))))))))
+    result))
+
+(defun eval-tests-33014-redefine ()
+  "Remove the Lisp reference to the byte-compiled object."
+  (setf (symbol-function #'eval-tests-33014-func) nil))
+
 ;;; eval-tests.el ends here
-- 
2.17.1