bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Istvan Marko]

There seems to be a byte-code change between versions
0ec0a610ed226419269f519021cbe8fb2dde2ed5 (old) and
a4aa32bdfff7aaf54efbacbb04b7f2b52fef92a7 (new) which causes the new
version to crash with SIGSEGV when executing certain code from an .elc
built using the old version. Recompiling the file with the new version
causes the new .elc to work correctly.

I am able to reproduce this by calling (pdf-tools-install-noverify) from
the pdf-tools.elc (compiled with the older emacs version) from the
pdf-tools package which is available at
https://github.com/politza/pdf-tools

This snippet in particular triggers the crash:

  (dolist (buf (buffer-list))
    ;; This when check should not be necessary, but somehow dead
    ;; buffers are showing up here. See
    ;; https://github.com/vedang/pdf-tools/pull/93
    (when (buffer-live-p buf)
      (with-current-buffer buf
        (when (and (not (derived-mode-p 'pdf-view-mode))
                   (pdf-tools-pdf-buffer-p)
                   (buffer-file-name))
          (pdf-view-mode)))))

The crash happens in the pdf-tools-pdf-buffer-p function:

(defun pdf-tools-pdf-buffer-p (&optional buffer)
  "Check if the current buffer is a PDF document.

Optionally, take BUFFER as an argument and check if it is a PDF document."
  (save-current-buffer
    (when buffer (set-buffer buffer))
    (save-excursion
      (save-restriction
        (widen)
        (goto-char 1)
        (looking-at "%PDF")))))

I can try to create a smaller standalone reproducer if needed.

backtrace:

Thread 1 "emacs" received signal SIGSEGV, Segmentation fault.
0x00001555529d17a7 in free () from /lib64/libc.so.6

(gdb) bt
#0  0x00001555529d17a7 in free () from /lib64/libc.so.6
#1  0x00000000001e66bc in ?? ()
#2  0x00005555557693d4 in safe_free (sa_count=...) at /mnt/sdc1/tmp/emacs/src/lisp.h:5385
#3  apply_lambda (fun=0x555557654ccd, args=<optimized out>, count=...) at eval.c:3109
#4  0x00005555557679e6 in eval_sub (form=<optimized out>) at eval.c:2588
#5  0x000055555576800d in Fprogn (body=0x555557678033) at eval.c:436
#6  0x0000555555767cf2 in eval_sub (form=<optimized out>) at eval.c:2449
#7  0x000055555576a2b9 in internal_lisp_condition_case (var=0x927910, bodyform=0x555557677753, handlers=<optimized out>) at eval.c:1428
#8  0x0000555555767cf2 in eval_sub (form=<optimized out>) at eval.c:2449
#9  0x00005555557688bd in Fprogn (body=0x0) at eval.c:436
#10 Fif (args=<optimized out>) at eval.c:392
#11 Fif (args=<optimized out>) at eval.c:378
#12 0x0000555555767cf2 in eval_sub (form=<optimized out>) at eval.c:2449
#13 0x000055555576800d in Fprogn (body=0x5555576b4ee3) at eval.c:436
#14 0x0000555555767cf2 in eval_sub (form=<optimized out>) at eval.c:2449
#15 0x000055555576a2b9 in internal_lisp_condition_case (var=0x927910, bodyform=0x5555576ad993, handlers=<optimized out>) at eval.c:1428
#16 0x0000555555767cf2 in eval_sub (form=<optimized out>) at eval.c:2449
#17 0x000055555578f800 in readevalloop_eager_expand_eval (val=0x0, macroexpand=0xffffbffff97a1430) at /mnt/sdc1/tmp/emacs/src/lisp.h:1516
#18 0x0000555555797a7b in readevalloop (readcharfun=0x555555f7ecad, infile0=0x0, sourcename=0x55555618c784, printflag=false, unibyte=<optimized out>, readfun=0x0, start=0x0,
    end=0x0) at lread.c:2347
#19 0x0000555555798cfc in Feval_buffer (buffer=<optimized out>, printflag=0x0, filename=0x55555618c784, unibyte=0x0, do_allow_print=0x30) at lread.c:2420
#20 0x00005555557ab3e7 in exec_byte_code (fun=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>) at bytecode.c:809
#21 0x0000555555763f63 in Ffuncall (nargs=nargs@entry=5, args=args@entry=0x7fffffffdae0) at eval.c:2995
#22 0x0000555555798a9d in call4 (arg4=0x30, arg3=0x30, arg2=0x55555618c784, arg1=0x55555618c784, fn=<optimized out>) at /mnt/sdc1/tmp/emacs/src/lisp.h:3269
#23 Fload (file=<optimized out>, noerror=0xffffbffff9503bb0, nomessage=0xffffbffff9503ab0, nosuffix=<optimized out>, must_suffix=<optimized out>) at lread.c:1484
#24 0x00005555557ab3e7 in exec_byte_code (fun=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>) at bytecode.c:809
#25 0x0000555555769387 in apply_lambda (fun=0x15554f4bac2d, args=<optimized out>, count=...) at eval.c:3103
#26 0x00005555557679e6 in eval_sub (form=<optimized out>) at eval.c:2588
#27 0x000055555576a4b7 in Feval (form=0x15554f8f188b, lexical=<optimized out>) at eval.c:2361
#28 0x0000555555762627 in internal_condition_case (bfun=bfun@entry=0x5555556d5510 <top_level_2>, handlers=handlers@entry=0x90, hfun=hfun@entry=0x5555556dcd10 <cmd_error>)
    at eval.c:1474
#29 0x00005555556d5ee6 in top_level_1 (ignore=ignore@entry=0x0) at keyboard.c:1141
#30 0x0000555555762581 in internal_catch (tag=tag@entry=0xffc0, func=func@entry=0x5555556d5ec0 <top_level_1>, arg=arg@entry=0x0) at eval.c:1197
#31 0x00005555556d548f in command_loop () at keyboard.c:1101
#32 0x00005555556dc894 in recursive_edit_1 () at keyboard.c:711
#33 0x00005555556dcc1c in Frecursive_edit () at keyboard.c:794
#34 0x00005555555aa4bd in main (argc=<optimized out>, argv=<optimized out>) at emacs.c:2529

(gdb) xbacktrace
"pdf-tools-install-noverify" (0x4ed262b8)
"pdf-tools-install" (0xffffd120)
"progn" (0xffffd2a0)
"condition-case" (0xffffd400)
"if" (0xffffd4e0)
"progn" (0xffffd5c0)
"condition-case" (0xffffd720)
"eval-buffer" (0x4ed26248)
"load-with-code-conversion" (0xffffdae8)
"load" (0x4ed26168)
"startup--load-user-init-file" (0x4ed260c0)
"command-line" (0x4ed26040)
"normal-top-level" (0xffffdc50)



In GNU Emacs 29.0.60 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.34, cairo version 1.16.0) of 2023-02-13 built on foo.bar.com
Repository revision: a4aa32bdfff7aaf54efbacbb04b7f2b52fef92a7
Repository branch: HEAD
System Description: Gentoo/Linux

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LCMS2 LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND
SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XDBE XIM
XINPUT2 XPM GTK3 ZLIB


-- 
	Istvan

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Gregory Heytings]

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


>
> There seems to be a byte-code change between versions 
> 0ec0a610ed226419269f519021cbe8fb2dde2ed5 (old) and 
> a4aa32bdfff7aaf54efbacbb04b7f2b52fef92a7 (new) which causes the new 
> version to crash with SIGSEGV when executing certain code from an .elc 
> built using the old version. Recompiling the file with the new version 
> causes the new .elc to work correctly.
>

The bytecode did not change, but the byte codes generated by the 
save-restriction form did change.

Now that I think of it again, it is possible make that change while 
preserving backward compatibility.

Eli, what do you think of the attached patch?  It restores the 'unbind 1' 
at the end of save-restriction, and puts the two data elements into a cons 
instead of pushing them separately.  (Of course this passes make and make 
check, with and without native compilation.)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Improve-backward-compatibility-of-save-restriction.patch --]
[-- Type: text/x-diff; name=Improve-backward-compatibility-of-save-restriction.patch, Size: 4934 bytes --]

From 85f4a2851514fad981b74abe14b833196d5987b0 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 14 Feb 2023 09:22:22 +0000
Subject: [PATCH] Improve backward compatibility of save-restriction

* src/editfns.c (save_restriction_and_narrowing_locks_restore):
New function, combining 'save_restriction_restore' and
'narrowing_locks_restore'.
(narrowing_locks_restore): Make static.
(Fsave_restriction): Use the new function instead of the two ones
it combines.

* src/lisp.h: Make the new function externally visible.

* src/bytecode.c (exec_byte_code): Use the new function instead of
the two ones it combines.

* src/comp.c (helper_save_restriction): Use the new function
instead of the two ones it combines.

* lisp/emacs-lisp/bytecomp.el (byte-compile-save-restriction):
Decrement unbinding count.
---
 lisp/emacs-lisp/bytecomp.el |  2 +-
 src/bytecode.c              |  7 +++----
 src/comp.c                  |  7 +++----
 src/editfns.c               | 14 +++++++++++---
 src/lisp.h                  |  2 +-
 5 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index c6cda6b588a..5df1205869c 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -4900,7 +4900,7 @@ byte-compile-save-excursion
 (defun byte-compile-save-restriction (form)
   (byte-compile-out 'byte-save-restriction 0)
   (byte-compile-body-do-effect (cdr form))
-  (byte-compile-out 'byte-unbind 2))
+  (byte-compile-out 'byte-unbind 1))
 
 (defun byte-compile-save-current-buffer (form)
   (byte-compile-out 'byte-save-current-buffer 0)
diff --git a/src/bytecode.c b/src/bytecode.c
index 8e214560f30..ca7ae4766e1 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -940,10 +940,9 @@ #define DEFINE(name, value) [name] = &&insn_ ## name,
 	  }
 
 	CASE (Bsave_restriction):
-	  record_unwind_protect (save_restriction_restore,
-				 save_restriction_save ());
-	  record_unwind_protect (narrowing_locks_restore,
-				 narrowing_locks_save ());
+	  record_unwind_protect (save_restriction_and_narrowing_locks_restore,
+				 Fcons (save_restriction_save (),
+					narrowing_locks_save ()));
 	  NEXT;
 
 	CASE (Bcatch):		/* Obsolete since 25.  */
diff --git a/src/comp.c b/src/comp.c
index 0e2dfd3913b..fe2c9b054b6 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -5061,10 +5061,9 @@ helper_unbind_n (Lisp_Object n)
 static void
 helper_save_restriction (void)
 {
-  record_unwind_protect (save_restriction_restore,
-			 save_restriction_save ());
-  record_unwind_protect (narrowing_locks_restore,
-			 narrowing_locks_save ());
+  record_unwind_protect (save_restriction_and_narrowing_locks_restore,
+			 Fcons (save_restriction_save (),
+				narrowing_locks_save ()));
 }
 
 static bool
diff --git a/src/editfns.c b/src/editfns.c
index ce133785e0b..dbcc5728dda 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -2804,7 +2804,7 @@ narrowing_locks_save (void)
   return Fcons (buf, Fcopy_sequence (locks));
 }
 
-void
+static void
 narrowing_locks_restore (Lisp_Object buf_and_saved_locks)
 {
   Lisp_Object buf = XCAR (buf_and_saved_locks);
@@ -3068,6 +3068,13 @@ save_restriction_restore (Lisp_Object data)
     set_buffer_internal (cur);
 }
 
+void
+save_restriction_and_narrowing_locks_restore (Lisp_Object data)
+{
+  save_restriction_restore (XCAR (data));
+  narrowing_locks_restore (XCDR (data));
+}
+
 DEFUN ("save-restriction", Fsave_restriction, Ssave_restriction, 0, UNEVALLED, 0,
        doc: /* Execute BODY, saving and restoring current buffer's restrictions.
 The buffer's restrictions make parts of the beginning and end invisible.
@@ -3091,8 +3098,9 @@ DEFUN ("save-restriction", Fsave_restriction, Ssave_restriction, 0, UNEVALLED, 0
   register Lisp_Object val;
   specpdl_ref count = SPECPDL_INDEX ();
 
-  record_unwind_protect (save_restriction_restore, save_restriction_save ());
-  record_unwind_protect (narrowing_locks_restore, narrowing_locks_save ());
+  record_unwind_protect (save_restriction_and_narrowing_locks_restore,
+			 Fcons (save_restriction_save (),
+				narrowing_locks_save ()));
   val = Fprogn (body);
   return unbind_to (count, val);
 }
diff --git a/src/lisp.h b/src/lisp.h
index 93197d38176..a86a2e823f9 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4685,7 +4685,7 @@ XMODULE_FUNCTION (Lisp_Object o)
 extern Lisp_Object save_restriction_save (void);
 extern void save_restriction_restore (Lisp_Object);
 extern Lisp_Object narrowing_locks_save (void);
-extern void narrowing_locks_restore (Lisp_Object);
+extern void save_restriction_and_narrowing_locks_restore (Lisp_Object);
 extern Lisp_Object make_buffer_string (ptrdiff_t, ptrdiff_t, bool);
 extern Lisp_Object make_buffer_string_both (ptrdiff_t, ptrdiff_t, ptrdiff_t,
 					    ptrdiff_t, bool);
-- 
2.39.0

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Eli Zaretskii]

> Date: Tue, 14 Feb 2023 09:29:32 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Eli Zaretskii <eliz@gnu.org>, 61504@debbugs.gnu.org
> 
> > There seems to be a byte-code change between versions 
> > 0ec0a610ed226419269f519021cbe8fb2dde2ed5 (old) and 
> > a4aa32bdfff7aaf54efbacbb04b7f2b52fef92a7 (new) which causes the new 
> > version to crash with SIGSEGV when executing certain code from an .elc 
> > built using the old version. Recompiling the file with the new version 
> > causes the new .elc to work correctly.
> >
> 
> The bytecode did not change, but the byte codes generated by the 
> save-restriction form did change.
> 
> Now that I think of it again, it is possible make that change while 
> preserving backward compatibility.
> 
> Eli, what do you think of the attached patch?  It restores the 'unbind 1' 
> at the end of save-restriction, and puts the two data elements into a cons 
> instead of pushing them separately.  (Of course this passes make and make 
> check, with and without native compilation.)

I guess we have no choice.

Mattias, Stefan: any better alternatives, or comments/objections?

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Eli, what do you think of the attached patch?  It restores the 'unbind 1' at
> the end of save-restriction, and puts the two data elements into a cons
> instead of pushing them separately.  (Of course this passes make and make
> check, with and without native compilation.)

Looks right to me.
Some comments below.


        Stefan


> @@ -940,10 +940,9 @@ #define DEFINE(name, value) [name] = &&insn_ ## name,
>  	  }
>  
>  	CASE (Bsave_restriction):
> -	  record_unwind_protect (save_restriction_restore,
> -				 save_restriction_save ());
> -	  record_unwind_protect (narrowing_locks_restore,
> -				 narrowing_locks_save ());
> +	  record_unwind_protect (save_restriction_and_narrowing_locks_restore,
> +				 Fcons (save_restriction_save (),
> +					narrowing_locks_save ()));
>  	  NEXT;

Shouldn't the value returned by `save_restriction_save` include the
narrowing locks already?

IOW rather than changing this `bytecode.c` code, the locks handling
should be "hidden" inside `save_restriction_restore` and
`save_restriction_save`, don't you think?


        Stefan

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Gregory Heytings]

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


>
> Looks right to me.
>

Many thanks for your review!

>> @@ -940,10 +940,9 @@ #define DEFINE(name, value) [name] = &&insn_ ## name,
>>  	  }
>>
>>  	CASE (Bsave_restriction):
>> -	  record_unwind_protect (save_restriction_restore,
>> -				 save_restriction_save ());
>> -	  record_unwind_protect (narrowing_locks_restore,
>> -				 narrowing_locks_save ());
>> +	  record_unwind_protect (save_restriction_and_narrowing_locks_restore,
>> +				 Fcons (save_restriction_save (),
>> +					narrowing_locks_save ()));
>>  	  NEXT;
>
> Shouldn't the value returned by `save_restriction_save` include the 
> narrowing locks already?
>
> IOW rather than changing this `bytecode.c` code, the locks handling 
> should be "hidden" inside `save_restriction_restore` and 
> `save_restriction_save`, don't you think?
>

You mean, the attached patch?  That's probably even better, indeed. 
(Again it passes make and make check, with and without native 
compilation.)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Improve-backward-compatibility-of-save-restriction.patch --]
[-- Type: text/x-diff; name=Improve-backward-compatibility-of-save-restriction.patch, Size: 5535 bytes --]

From 79c5581d34da4baecf5514c1d0d25cfbcc33f05d Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 14 Feb 2023 15:59:16 +0000
Subject: [PATCH] Improve backward compatibility of save-restriction

* src/editfns.c (save_restriction_save_1): Renamed from
'save_restrictions_save'.
(save_restriction_restore_1): Renamed from
'save_restriction_restore'.
(save_restriction_restore): New function, combining
'save_restriction_save_1' and 'narrowing_locks_save'.
(save_restriction_save): New function, combining
'save_restriction_restore_1' and 'narrowing_locks_restore'.
(Fsave_restriction): Restore the previous code.
(narrowing_locks_save, narrowing_locks_restore): Make them static.

* src/lisp.h: Remove two functions that are not externally visible
anymore.

* src/comp.c (helper_save_restriction): Restore the previous code.

* src/bytecode.c (exec_byte_code): Restore the previous code.

* lisp/emacs-lisp/bytecomp.el (byte-compile-save-restriction):
Decrement unbinding count.
---
 lisp/emacs-lisp/bytecomp.el |  2 +-
 src/bytecode.c              |  2 --
 src/comp.c                  |  2 --
 src/editfns.c               | 26 +++++++++++++++++++-------
 src/lisp.h                  |  2 --
 5 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index c6cda6b588a..5df1205869c 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -4900,7 +4900,7 @@ byte-compile-save-excursion
 (defun byte-compile-save-restriction (form)
   (byte-compile-out 'byte-save-restriction 0)
   (byte-compile-body-do-effect (cdr form))
-  (byte-compile-out 'byte-unbind 2))
+  (byte-compile-out 'byte-unbind 1))
 
 (defun byte-compile-save-current-buffer (form)
   (byte-compile-out 'byte-save-current-buffer 0)
diff --git a/src/bytecode.c b/src/bytecode.c
index 8e214560f30..124348e5b35 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -942,8 +942,6 @@ #define DEFINE(name, value) [name] = &&insn_ ## name,
 	CASE (Bsave_restriction):
 	  record_unwind_protect (save_restriction_restore,
 				 save_restriction_save ());
-	  record_unwind_protect (narrowing_locks_restore,
-				 narrowing_locks_save ());
 	  NEXT;
 
 	CASE (Bcatch):		/* Obsolete since 25.  */
diff --git a/src/comp.c b/src/comp.c
index 0e2dfd3913b..10cf7962ba1 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -5063,8 +5063,6 @@ helper_save_restriction (void)
 {
   record_unwind_protect (save_restriction_restore,
 			 save_restriction_save ());
-  record_unwind_protect (narrowing_locks_restore,
-			 narrowing_locks_save ());
 }
 
 static bool
diff --git a/src/editfns.c b/src/editfns.c
index ce133785e0b..1534f78ace7 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -2794,7 +2794,7 @@ reset_outermost_narrowings (void)
 
 /* Helper functions to save and restore the narrowing locks of the
    current buffer in Fsave_restriction.  */
-Lisp_Object
+static Lisp_Object
 narrowing_locks_save (void)
 {
   Lisp_Object buf = Fcurrent_buffer ();
@@ -2804,7 +2804,7 @@ narrowing_locks_save (void)
   return Fcons (buf, Fcopy_sequence (locks));
 }
 
-void
+static void
 narrowing_locks_restore (Lisp_Object buf_and_saved_locks)
 {
   Lisp_Object buf = XCAR (buf_and_saved_locks);
@@ -2975,8 +2975,8 @@ DEFUN ("internal--unlock-narrowing", Finternal__unlock_narrowing,
   return Qnil;
 }
 
-Lisp_Object
-save_restriction_save (void)
+static Lisp_Object
+save_restriction_save_1 (void)
 {
   if (BEGV == BEG && ZV == Z)
     /* The common case that the buffer isn't narrowed.
@@ -2999,8 +2999,8 @@ save_restriction_save (void)
     }
 }
 
-void
-save_restriction_restore (Lisp_Object data)
+static void
+save_restriction_restore_1 (Lisp_Object data)
 {
   struct buffer *cur = NULL;
   struct buffer *buf = (CONSP (data)
@@ -3068,6 +3068,19 @@ save_restriction_restore (Lisp_Object data)
     set_buffer_internal (cur);
 }
 
+Lisp_Object
+save_restriction_save (void)
+{
+  return Fcons (save_restriction_save_1 (), narrowing_locks_save ());
+}
+
+void
+save_restriction_restore (Lisp_Object data)
+{
+  save_restriction_restore_1 (XCAR (data));
+  narrowing_locks_restore (XCDR (data));
+}
+
 DEFUN ("save-restriction", Fsave_restriction, Ssave_restriction, 0, UNEVALLED, 0,
        doc: /* Execute BODY, saving and restoring current buffer's restrictions.
 The buffer's restrictions make parts of the beginning and end invisible.
@@ -3092,7 +3105,6 @@ DEFUN ("save-restriction", Fsave_restriction, Ssave_restriction, 0, UNEVALLED, 0
   specpdl_ref count = SPECPDL_INDEX ();
 
   record_unwind_protect (save_restriction_restore, save_restriction_save ());
-  record_unwind_protect (narrowing_locks_restore, narrowing_locks_save ());
   val = Fprogn (body);
   return unbind_to (count, val);
 }
diff --git a/src/lisp.h b/src/lisp.h
index 93197d38176..1276285e2f2 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4684,8 +4684,6 @@ XMODULE_FUNCTION (Lisp_Object o)
 extern void save_excursion_restore (Lisp_Object, Lisp_Object);
 extern Lisp_Object save_restriction_save (void);
 extern void save_restriction_restore (Lisp_Object);
-extern Lisp_Object narrowing_locks_save (void);
-extern void narrowing_locks_restore (Lisp_Object);
 extern Lisp_Object make_buffer_string (ptrdiff_t, ptrdiff_t, bool);
 extern Lisp_Object make_buffer_string_both (ptrdiff_t, ptrdiff_t, ptrdiff_t,
 					    ptrdiff_t, bool);
-- 
2.39.0

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> You mean, the attached patch?

Yes.


        Stefan

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Mattias Engdegård]

The patch is all right, I suppose, but it would be nice to do without the extra cons. Maybe a new specpdl case is warranted? `save-excursion` has one.

By the way, doesn't the patch switch the restoration order of narrowing and restriction, respectively? Maybe it doesn't matter?

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Gregory Heytings]

>> You mean, the attached patch?
>
> Yes.
>

Great, thanks again!

Eli, OK to install?

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Gregory Heytings]

Thanks for the review!

>
> The patch is all right, I suppose, but it would be nice to do without 
> the extra cons. Maybe a new specpdl case is warranted? `save-excursion` 
> has one.
>

That's a possible improvement, but I think it's not safe enough for Emacs 
29.

>
> By the way, doesn't the patch switch the restoration order of narrowing 
> and restriction, respectively? Maybe it doesn't matter?
>

Hmmm, that's a good question!  The evaluation order of parameters is 
unspecified in C, so actually the order could be switched or not, 
depending on what the compiler chooses to do.  That being said, AFAICS it 
doesn't matter in this case, indeed.

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Eli Zaretskii]

> Date: Tue, 14 Feb 2023 17:00:55 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Eli Zaretskii <eliz@gnu.org>, mi-ebugs@kismala.com, 
>     Stefan Monnier <monnier@iro.umontreal.ca>, 61504@debbugs.gnu.org
> 
> > By the way, doesn't the patch switch the restoration order of narrowing 
> > and restriction, respectively? Maybe it doesn't matter?
> 
> Hmmm, that's a good question!  The evaluation order of parameters is 
> unspecified in C, so actually the order could be switched or not, 
> depending on what the compiler chooses to do.

But you could rewrite the code so that the parameters are evaluated
one after the other, and only after that call Fcons.  The compiler
could still change the order, but that would be less probable.

> That being said, AFAICS it doesn't matter in this case, indeed.

It is IME better to write code that doesn't trigger such questions to
begin with.

> Eli, OK to install?

With the change of order per the above, yes.

Thanks.

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Mattias Engdegård]

14 feb. 2023 kl. 18.00 skrev Gregory Heytings <gregory@heytings.org>:

>> By the way, doesn't the patch switch the restoration order of narrowing and restriction, respectively? Maybe it doesn't matter?
>> 
> 
> Hmmm, that's a good question!  The evaluation order of parameters is unspecified in C, so actually the order could be switched or not, depending on what the compiler chooses to do.

Yes, the saving order is undefined but the restoring order seems well-defined. It currently restores narrowing locks first, then the restriction, but your patch flips the order.

Please at least make the saving order well-defined, preferably in the reverse order of restoration for symmetry.

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Istvan Marko]

I've tested with Gregory's second patch on top of current emacs-29 and
it avoids the crash with my original test case.

-- 
	Istvan

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> By the way, doesn't the patch switch the restoration order of narrowing
>> and restriction, respectively? Maybe it doesn't matter?
> Hmmm, that's a good question!  The evaluation order of parameters is
> unspecified in C,

The problem is not in the evaluation order of params in
`save_restriction_save` (this order doesn't matter because the code is
pure), it's in `+save_restriction_restore`.


        Stefan

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Gregory Heytings]

>> Hmmm, that's a good question!  The evaluation order of parameters is 
>> unspecified in C, so actually the order could be switched or not, 
>> depending on what the compiler chooses to do.
>
> But you could rewrite the code so that the parameters are evaluated one 
> after the other, and only after that call Fcons.  The compiler could 
> still change the order, but that would be less probable.
>

Agreed.

>> That being said, AFAICS it doesn't matter in this case, indeed.
>
> It is IME better to write code that doesn't trigger such questions to 
> begin with.
>

Agreed again.

>> Eli, OK to install?
>
> With the change of order per the above, yes.
>

Now done.

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Gregory Heytings]

>>> By the way, doesn't the patch switch the restoration order of 
>>> narrowing and restriction, respectively? Maybe it doesn't matter?
>>
>> Hmmm, that's a good question!  The evaluation order of parameters is 
>> unspecified in C, so actually the order could be switched or not, 
>> depending on what the compiler chooses to do.
>
> Yes, the saving order is undefined but the restoring order seems 
> well-defined. It currently restores narrowing locks first, then the 
> restriction, but your patch flips the order.
>

Indeed, I misunderstood what you said above, now I got it!

>
> Please at least make the saving order well-defined, preferably in the 
> reverse order of restoration for symmetry.
>

I did that.

Thanks again for your review/feedback.

bug#61504: 29.0.60; executing byte-code from previous build causes SIGSEGV crash

[Gregory Heytings]

>>> By the way, doesn't the patch switch the restoration order of 
>>> narrowing and restriction, respectively? Maybe it doesn't matter?
>>
>> Hmmm, that's a good question!  The evaluation order of parameters is 
>> unspecified in C,
>
> The problem is not in the evaluation order of params in 
> `save_restriction_save` (this order doesn't matter because the code is 
> pure), it's in `save_restriction_restore`.
>

Indeed, I initially misunderstood what Mattias said, but after reading it 
again I got it.  This is now fixed.

Thanks!