bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Noam Postavsky]

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

X-Debbugs-CC: Paul Eggert <eggert@cs.ucla.edu>


gdb --args ./temacs --batch  --load loadup bootstrap

(gdb) p specpdl_ptr->kind
$1 = SPECPDL_UNWIND_ARRAY
(gdb) p specpdl_ptr->unwind_ptr.func
$2 = (void (*)(void *)) 0x53898
(gdb) p specpdl_ptr->unwind_array.func
$3 = (void (*)(Lisp_Object)) 0x53898

I guess it's related to the #31750 changes. The problem doesn't seem
especially platform-specific, but I've haven't seen this on my
GNU/Linux box.
The attached patch fixes it, though I'm not sure if it's
entirely correct.

[-- Attachment #2: unwind-array.diff --]
[-- Type: application/octet-stream, Size: 986 bytes --]

--- i/src/lisp.h
+++ w/src/lisp.h
@@ -4543,7 +4543,8 @@ safe_free (ptrdiff_t sa_count)
   while (specpdl_ptr != specpdl + sa_count)
     {
       specpdl_ptr--;
-      eassert ((specpdl_ptr->kind == SPECPDL_UNWIND_PTR || specpdl_ptr->kind == SPECPDL_UNWIND_ARRAY)
+      eassert ((specpdl_ptr->kind == SPECPDL_UNWIND_PTR
+                || specpdl_ptr->kind == SPECPDL_UNWIND_ARRAY)
 	       && specpdl_ptr->unwind_ptr.func == xfree);
       xfree (specpdl_ptr->unwind_ptr.arg);
     }
diff --git i/src/w32fns.c w/src/w32fns.c
index 3bd3209..59edb31 100644
--- i/src/w32fns.c
+++ w/src/w32fns.c
@@ -4551,7 +4551,7 @@ w32_wnd_proc (HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam)
 	 dialog boxes, such as the file selection dialog or font
 	 selection dialog.  So something else is needed to fix the
 	 former without breaking the latter.  See bug#11732.  */
-      break;
+      goto dflt;
 
     case WM_IME_ENDCOMPOSITION:
       ignore_ime_char = 0;

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Thu, 28 Jun 2018 15:10:30 -0400
> Cc: paul eggert <eggert@cs.ucla.edu>
> 
> The attached patch fixes it, though I'm not sure if it's
> entirely correct.

The second part seems unrelated, yes?

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Noam Postavsky]

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

On 28 June 2018 at 15:18, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Noam Postavsky <npostavs@gmail.com>
>> Date: Thu, 28 Jun 2018 15:10:30 -0400
>> Cc: paul eggert <eggert@cs.ucla.edu>
>>
>> The attached patch fixes it, though I'm not sure if it's
>> entirely correct.
>
> The second part seems unrelated, yes?

Oops, that patch is mixed up, here's what I meant instead.

[-- Attachment #2: unwind-array.diff --]
[-- Type: application/octet-stream, Size: 1137 bytes --]

--- c/src/eval.c
+++ i/src/eval.c
@@ -3411,6 +3411,7 @@ void
 record_unwind_protect_array (Lisp_Object *array, ptrdiff_t nelts)
 {
   specpdl_ptr->unwind_array.kind = SPECPDL_UNWIND_ARRAY;
+  specpdl_ptr->unwind_array.func = xfree;
   specpdl_ptr->unwind_array.array = array;
   specpdl_ptr->unwind_array.nelts = nelts;
   grow_specpdl ();
diff --git c/src/lisp.h i/src/lisp.h
index b544d81..8cb543d 100644
--- c/src/lisp.h
+++ i/src/lisp.h
@@ -3058,7 +3058,7 @@ union specbinding
     } unwind;
     struct {
       ENUM_BF (specbind_tag) kind : CHAR_BIT;
-      void (*func) (Lisp_Object);
+      void (*func) (void *);
       Lisp_Object *array;
       ptrdiff_t nelts;
     } unwind_array;
@@ -4543,7 +4543,8 @@ safe_free (ptrdiff_t sa_count)
   while (specpdl_ptr != specpdl + sa_count)
     {
       specpdl_ptr--;
-      eassert (specpdl_ptr->kind == SPECPDL_UNWIND_PTR
+      eassert ((specpdl_ptr->kind == SPECPDL_UNWIND_PTR
+                || specpdl_ptr->kind == SPECPDL_UNWIND_ARRAY)
 	       && specpdl_ptr->unwind_ptr.func == xfree);
       xfree (specpdl_ptr->unwind_ptr.arg);
     }

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Andy Moreton]

On Thu 28 Jun 2018, Noam Postavsky wrote:

> On 28 June 2018 at 15:18, Eli Zaretskii <eliz@gnu.org> wrote:
>>> From: Noam Postavsky <npostavs@gmail.com>
>>> Date: Thu, 28 Jun 2018 15:10:30 -0400
>>> Cc: paul eggert <eggert@cs.ucla.edu>
>>>
>>> The attached patch fixes it, though I'm not sure if it's
>>> entirely correct.
>>
>> The second part seems unrelated, yes?
>
> Oops, that patch is mixed up, here's what I meant instead.

Applying the fixed patch bootstraps cleanly for me with 64bit mingw64 on
Windows (MSYS2).

    AndyM

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Paul Eggert]

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

Thanks for reporting that. I didn't observe the problem on Fedora 28. Anyway, I 
installed the attached patch, which should fix it.

[-- Attachment #2: 0001-Fix-recently-introduced-SAFE_FREE-bug.patch --]
[-- Type: text/x-patch, Size: 1592 bytes --]

From ddc4371a89e5500e0203bed4b0ad453925b1c74f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 28 Jun 2018 13:49:48 -0700
Subject: [PATCH] Fix recently-introduced SAFE_FREE bug

Problem reported by Andy Moreton (Bug#31996).
* src/lisp.h (union specbinding.unwind_array):
Remove unused member func.  Move array after nelts, as this is
likely to generate more efficient code in safe_free, which can
call xfree with the same value either way.
(safe_free): Also handle SPECPDL_UNWIND_AWAY.
---
 src/lisp.h | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/lisp.h b/src/lisp.h
index b544d81..cf7b8c0 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3058,9 +3058,8 @@ union specbinding
     } unwind;
     struct {
       ENUM_BF (specbind_tag) kind : CHAR_BIT;
-      void (*func) (Lisp_Object);
-      Lisp_Object *array;
       ptrdiff_t nelts;
+      Lisp_Object *array;
     } unwind_array;
     struct {
       ENUM_BF (specbind_tag) kind : CHAR_BIT;
@@ -4543,9 +4542,16 @@ safe_free (ptrdiff_t sa_count)
   while (specpdl_ptr != specpdl + sa_count)
     {
       specpdl_ptr--;
-      eassert (specpdl_ptr->kind == SPECPDL_UNWIND_PTR
-	       && specpdl_ptr->unwind_ptr.func == xfree);
-      xfree (specpdl_ptr->unwind_ptr.arg);
+      if (specpdl_ptr->kind == SPECPDL_UNWIND_PTR)
+	{
+	  eassert (specpdl_ptr->unwind_ptr.func == xfree);
+	  xfree (specpdl_ptr->unwind_ptr.arg);
+	}
+      else
+	{
+	  eassert (specpdl_ptr->kind == SPECPDL_UNWIND_ARRAY);
+	  xfree (specpdl_ptr->unwind_array.array);
+	}
     }
 }
 
-- 
2.7.4

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Noam Postavsky]

On 28 June 2018 at 16:55, Paul Eggert <eggert@cs.ucla.edu> wrote:
> Thanks for reporting that. I didn't observe the problem on Fedora 28.
> Anyway, I installed the attached patch, which should fix it.

Thanks, works for me.

> Move array after nelts, as this is likely to generate more efficient
> code in safe_free, which can call xfree with the same value either
> way.

Maybe that kind of trick deserves a code comment?

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Paul Eggert]

Noam Postavsky wrote:
> Maybe that kind of trick deserves a code comment?

I thought about doing that, but it seemed like the maintenance cost of having 
the comment there indefinitely wasn't worth the minor performance benefit (a 
benefit that applies only to typical platforms and when optimization is enabled, 
facts that'd also probably need to be in the comment). For what it's worth, this 
stuff is now documented in the bug report....

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 28 Jun 2018 15:57:46 -0700
> Cc: 31996@debbugs.gnu.org
> 
> Noam Postavsky wrote:
> > Maybe that kind of trick deserves a code comment?
> 
> I thought about doing that, but it seemed like the maintenance cost of having 
> the comment there indefinitely wasn't worth the minor performance benefit (a 
> benefit that applies only to typical platforms and when optimization is enabled, 
> facts that'd also probably need to be in the comment). For what it's worth, this 
> stuff is now documented in the bug report....

But maybe a comment explaining why the order might matter in general
could be of more value, and require less maintenance.

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Paul Eggert]

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

Eli Zaretskii wrote:
>   maybe a comment explaining why the order might matter in general
> could be of more value, and require less maintenance.

I gave that a shot by installing the attached.

[-- Attachment #2: 0001-src-lisp.h-Add-comment-Bug-31996-25.patch --]
[-- Type: text/x-patch, Size: 656 bytes --]

From 9ffc9ec7a70c2e971b616146cb49b084981b74a9 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 29 Jun 2018 00:29:13 -0700
Subject: [PATCH] * src/lisp.h: Add comment (Bug#31996#25).

---
 src/lisp.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/lisp.h b/src/lisp.h
index cf7b8c0..6203a74 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3050,6 +3050,8 @@ enum specbind_tag {
 
 union specbinding
   {
+    /* Aligning similar members consistently might help efficiency slightly
+       (Bug#31996#25).  */
     ENUM_BF (specbind_tag) kind : CHAR_BIT;
     struct {
       ENUM_BF (specbind_tag) kind : CHAR_BIT;
-- 
2.7.4

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Eli Zaretskii]

> Cc: npostavs@gmail.com, 31996@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 29 Jun 2018 00:30:33 -0700
> 
> Eli Zaretskii wrote:
> >   maybe a comment explaining why the order might matter in general
> > could be of more value, and require less maintenance.
> 
> I gave that a shot by installing the attached.

Thanks.

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Noam Postavsky]

Paul Eggert <eggert@cs.ucla.edu> writes:

> I thought about doing that, but it seemed like the maintenance cost of
> having the comment

Oh, this talk about comment maintenance reminds me, there's this comment
in lisp.h just above the enum specbind_tag:

/* Elisp uses several stacks:
   - the C stack.
   - the bytecode stack: used internally by the bytecode interpreter.
     Allocated from the C stack.
   - The specpdl stack:[...]

You removed the bytecode stack in [1: a815e5f195], right?

[1: a815e5f195]: 2016-12-23 21:46:53 -0800
  Remove interpreter’s byte stack
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=a815e5f19581344af5e143636039064a7fbe83ed

bug#31996: 27.0.50; [w32] while dumping: assertion failed: specpdl_ptr->kind == SPECPDL_UNWIND_PTR && specpdl_ptr->unwind_ptr.func == xfree

[Paul Eggert]

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

On 06/29/2018 05:30 AM, Noam Postavsky wrote:
> You removed the bytecode stack in [1: a815e5f195], right?

Yes I did. Thanks for catching that. I fixed the comment by installing 
the attached into the emacs-26 branch.


[-- Attachment #2: 0001-src-lisp.h-Omit-obsolete-comment-re-bytecode-stack.patch --]
[-- Type: text/x-patch, Size: 2119 bytes --]

From e6be48a508c4d25c8f8973dc23708c924b403927 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 29 Jun 2018 11:14:36 -0700
Subject: [PATCH] * src/lisp.h: Omit obsolete comment re bytecode stack.

---
 src/lisp.h | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/src/lisp.h b/src/lisp.h
index 56ad8b814b..b2449cb87d 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3044,15 +3044,13 @@ extern void defvar_kboard (struct Lisp_Kboard_Objfwd *, const char *, int);
   } while (false)
 
 \f
-/* Elisp uses several stacks:
-   - the C stack.
-   - the bytecode stack: used internally by the bytecode interpreter.
-     Allocated from the C stack.
-   - The specpdl stack: keeps track of active unwind-protect and
-     dynamic-let-bindings.  Allocated from the `specpdl' array, a manually
-     managed stack.
-   - The handler stack: keeps track of active catch tags and condition-case
-     handlers.  Allocated in a manually managed stack implemented by a
+/* Elisp uses multiple stacks:
+   - The C stack.
+   - The specpdl stack keeps track of backtraces, unwind-protects and
+     dynamic let-bindings.  It is allocated from the 'specpdl' array,
+     a manually managed stack.
+   - The handler stack keeps track of active catch tags and condition-case
+     handlers.  It is allocated in a manually managed stack implemented by a
      doubly-linked list allocated via xmalloc and never freed.  */
 
 /* Structure for recording Lisp call stack for backtrace purposes.  */
@@ -3131,7 +3129,7 @@ SPECPDL_INDEX (void)
    control structures.  A struct handler contains all the information needed to
    restore the state of the interpreter after a non-local jump.
 
-   handler structures are chained together in a doubly linked list; the `next'
+   Handler structures are chained together in a doubly linked list; the `next'
    member points to the next outer catchtag and the `nextfree' member points in
    the other direction to the next inner element (which is typically the next
    free element since we mostly use it on the deepest handler).
-- 
2.17.1