bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

[Paul Pogonyshev]

To reproduce:

    (defun overflow ()
      (overflow))
    (defun test ()
      (interactive)
      (message "BEFORE")
      (unwind-protect
          (overflow)
        (message "CLEANUP")))

Invocation of `test' never issues message "CLEANUP", whether it is run
interactively or non-interactively.

By comparison, if you _catch_ the error with `condition-case':

    (defun test-2 ()
      (interactive)
      (message "BEFORE")
      (unwind-protect
          (ignore-errors (overflow))
        (message "CLEANUP")))

then cleanup form is executed properly.

But if your error catcher is "above" the `unwind-protect' form, the
cleanup is not executed again, even though the error is eaten as
expected:

    (defun test-3 ()
      (interactive)
      (message "BEFORE")
      (ignore-errors
        (unwind-protect
            (overflow)
          (message "CLEANUP"))))

This is a perfect way to screw up your Emacs permanently (until full
restart): when some `unwind-protect' cleanups are not run, you can be
left with unexpected function advices, permanently altered global
state etc., without any good way to undestand what's wrong.

Paul

bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

[Eli Zaretskii]

> From: Paul Pogonyshev <pogonyshev@gmail.com>
> Date: Sat, 13 Oct 2018 12:07:48 +0200
> 
> To reproduce:
> 
>     (defun overflow ()
>       (overflow))
>     (defun test ()
>       (interactive)
>       (message "BEFORE")
>       (unwind-protect
>           (overflow)
>         (message "CLEANUP")))
> 
> Invocation of `test' never issues message "CLEANUP", whether it is run
> interactively or non-interactively.

You are not supposed to continue using Emacs as usual after it
recovered from a C stack overflow.  You are supposed to exit Emacs and
restart the session.

The C stack overflow recovery is provided to allow you to save your
edits instead of losing them.

P.S.  I was somehow certain that we say the above somewhere in the
docs, but I cannot find it, so maybe I was dreaming.  Patches to add
that are welcome.

bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

[Paul Pogonyshev]

I see. Wonderful approach. At least I'm not supposed to restart my machine.
On Sat, 13 Oct 2018 at 12:29, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Paul Pogonyshev <pogonyshev@gmail.com>
> > Date: Sat, 13 Oct 2018 12:07:48 +0200
> >
> > To reproduce:
> >
> >     (defun overflow ()
> >       (overflow))
> >     (defun test ()
> >       (interactive)
> >       (message "BEFORE")
> >       (unwind-protect
> >           (overflow)
> >         (message "CLEANUP")))
> >
> > Invocation of `test' never issues message "CLEANUP", whether it is run
> > interactively or non-interactively.
>
> You are not supposed to continue using Emacs as usual after it
> recovered from a C stack overflow.  You are supposed to exit Emacs and
> restart the session.
>
> The C stack overflow recovery is provided to allow you to save your
> edits instead of losing them.
>
> P.S.  I was somehow certain that we say the above somewhere in the
> docs, but I cannot find it, so maybe I was dreaming.  Patches to add
> that are welcome.

bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

[Eli Zaretskii]

> From: Paul Pogonyshev <pogonyshev@gmail.com>
> Date: Sat, 13 Oct 2018 12:35:46 +0200
> Cc: 33034@debbugs.gnu.org
> 
> I see. Wonderful approach.

If you have ideas for better approaches, I'm sure they will be
welcome.

C stack overflow results in SIGSEGV; the current code attempts
recovery by using OS-dependent techniques that analyze the data
provided by the segfault to detect when it's a stack overflow, and if
so, do the moral equivalent of (throw 'top-level), bypassing any
possible unwind forms, because evaluating those forms when there is no
available stack space might very well trigger another, nested
segfault.

It's a hard problem, and the only justification for it is to give
users some imperfect chance of saving their edits.

Some people think we shouldn't even attempt to recover from such
calamities, and instead just crash, which is why we have the
attempt-stack-overflow-recovery variable to let those people have what
they want.

bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

[Paul Pogonyshev]

I haven't looked into the source code, but it seems that these
examples don't involve C-level stack overflow.  I tried setting
`attempt-stack-overflow-recovery' to nil and re-evaluated the examples
with exactly the same results: cleanup forms are not executed, but
Emacs doesn't crash. Looks like stack that is overflown here is only
Lisp-level. Besides, it's hard to imagine that `max-specpdl-size' or
`max-lisp-eval-depth' somehow affect C-level stack.

On Sat, 13 Oct 2018 at 12:45, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Paul Pogonyshev <pogonyshev@gmail.com>
> > Date: Sat, 13 Oct 2018 12:35:46 +0200
> > Cc: 33034@debbugs.gnu.org
> >
> > I see. Wonderful approach.
>
> If you have ideas for better approaches, I'm sure they will be
> welcome.
>
> C stack overflow results in SIGSEGV; the current code attempts
> recovery by using OS-dependent techniques that analyze the data
> provided by the segfault to detect when it's a stack overflow, and if
> so, do the moral equivalent of (throw 'top-level), bypassing any
> possible unwind forms, because evaluating those forms when there is no
> available stack space might very well trigger another, nested
> segfault.
>
> It's a hard problem, and the only justification for it is to give
> users some imperfect chance of saving their edits.
>
> Some people think we shouldn't even attempt to recover from such
> calamities, and instead just crash, which is why we have the
> attempt-stack-overflow-recovery variable to let those people have what
> they want.

bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

[Eli Zaretskii]

> From: Paul Pogonyshev <pogonyshev@gmail.com>
> Date: Sat, 13 Oct 2018 12:52:13 +0200
> Cc: 33034@debbugs.gnu.org
> 
> I haven't looked into the source code, but it seems that these
> examples don't involve C-level stack overflow.

You are right, this issue is unrelated to C stack overflow.

bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

[Eli Zaretskii]

> Date: Sat, 13 Oct 2018 14:01:00 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 33034@debbugs.gnu.org
> 
> > I haven't looked into the source code, but it seems that these
> > examples don't involve C-level stack overflow.
> 
> You are right, this issue is unrelated to C stack overflow.

What actually happens here is that the cleanup form _is_ called, but
it again hits the limit of Lisp local bindings, and therefore itself
signals an error.  And unwind-protect does not protect cleanup forms
(this is documented).

bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

[Paul Pogonyshev]

OK, but why does it hit the limit? Logically, by the time cleanup form
is called, all the (overflow) stack frames should be removed and the
cleanup form should see practically empty stack. It shouldn't be much
different from calling cleanup without overflowing the stack to begin
with.

Paul
On Sat, 13 Oct 2018 at 13:30, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > Date: Sat, 13 Oct 2018 14:01:00 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: 33034@debbugs.gnu.org
> >
> > > I haven't looked into the source code, but it seems that these
> > > examples don't involve C-level stack overflow.
> >
> > You are right, this issue is unrelated to C stack overflow.
>
> What actually happens here is that the cleanup form _is_ called, but
> it again hits the limit of Lisp local bindings, and therefore itself
> signals an error.  And unwind-protect does not protect cleanup forms
> (this is documented).

bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

[Eli Zaretskii]

> From: Paul Pogonyshev <pogonyshev@gmail.com>
> Date: Sat, 13 Oct 2018 13:38:11 +0200
> Cc: 33034@debbugs.gnu.org
> 
> OK, but why does it hit the limit? Logically, by the time cleanup form
> is called, all the (overflow) stack frames should be removed and the
> cleanup form should see practically empty stack. It shouldn't be much
> different from calling cleanup without overflowing the stack to begin
> with.

I don't think your expectation, that the stack should be unwound
before the cleanup runs, is correct.  The implementation calls the
cleanup forms before it jumps to top-level, and I see nothing in the
documentation to promise anything different.

bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

[Paul Pogonyshev]

(defvar global-test nil)
(unwind-protect
    (let ((global-test t))
      (message "inside, global-test = %s" global-test)
      (error "test"))
  (message "in cleanup, global-test = %s" global-test))

This gives the following output (outside the error):
    inside, global-test = t
    in cleanup, global-test = nil

So, global variables are unwound, but stack is not? This doesn't make
much sense to me.

Besides, what is the purpose of current implementation? Current state
has at least one disadvantage, highlighted by this bug: cleanup forms
after a stack overflow error always fail to work, because stack is
still full. Are there any advantages? I feel like it is more of
coincidence than deliberate decision. Would fixing it break backwards
compatibility?
On Sat, 13 Oct 2018 at 14:35, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Paul Pogonyshev <pogonyshev@gmail.com>
> > Date: Sat, 13 Oct 2018 13:38:11 +0200
> > Cc: 33034@debbugs.gnu.org
> >
> > OK, but why does it hit the limit? Logically, by the time cleanup form
> > is called, all the (overflow) stack frames should be removed and the
> > cleanup form should see practically empty stack. It shouldn't be much
> > different from calling cleanup without overflowing the stack to begin
> > with.
>
> I don't think your expectation, that the stack should be unwound
> before the cleanup runs, is correct.  The implementation calls the
> cleanup forms before it jumps to top-level, and I see nothing in the
> documentation to promise anything different.

bug#33034: `unwind-protect' cleanup form is not executed if body dies in stack overflow

[Paul Eggert]

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

Thanks for the bug report; I installed the attached to fix it. The problem with 
your test case was neither C stack overflow nor failure to unwind the Lisp 
stack: it was failure to restore the Lisp evaluation depth (which is a separate 
thing from the Lisp stack size).

By the way, why are there two different limits? That slows the interpreter down 
a bit.  Why don't we simply have a limit for the Lisp stack size? Every time 
lisp_eval_depth grows, the stack size grows, so limiting the stack limits the 
evaluation depth for free. If we had done things this way, the interpreter would 
be a bit faster and this bug would never have occurred.

[-- Attachment #2: 0001-Fix-lisp_eval_depth-in-unwind-protect-cleanup.patch --]
[-- Type: text/x-patch, Size: 1804 bytes --]

From 5dfbdff57be6e93efe79f9b07e8b6383ec02959a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 14 Oct 2018 09:51:32 -0700
Subject: [PATCH] Fix lisp_eval_depth in unwind-protect cleanup

Problem reported by Paul Pogonyshev (Bug#33034).
* src/lisp.h (union specbinding): New member unwind.eval_depth.
* src/eval.c (record_unwind_protect, set_unwind_protect): Set it.
(do_one_unbind): Use it.
---
 src/eval.c | 3 +++
 src/lisp.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/src/eval.c b/src/eval.c
index 42c275de6b..a51d0c9083 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -3429,6 +3429,7 @@ record_unwind_protect (void (*function) (Lisp_Object), Lisp_Object arg)
   specpdl_ptr->unwind.kind = SPECPDL_UNWIND;
   specpdl_ptr->unwind.func = function;
   specpdl_ptr->unwind.arg = arg;
+  specpdl_ptr->unwind.eval_depth = lisp_eval_depth;
   grow_specpdl ();
 }
 
@@ -3501,6 +3502,7 @@ do_one_unbind (union specbinding *this_binding, bool unwinding,
   switch (this_binding->kind)
     {
     case SPECPDL_UNWIND:
+      lisp_eval_depth = this_binding->unwind.eval_depth;
       this_binding->unwind.func (this_binding->unwind.arg);
       break;
     case SPECPDL_UNWIND_ARRAY:
@@ -3595,6 +3597,7 @@ set_unwind_protect (ptrdiff_t count, void (*func) (Lisp_Object),
   p->unwind.kind = SPECPDL_UNWIND;
   p->unwind.func = func;
   p->unwind.arg = arg;
+  p->unwind.eval_depth = lisp_eval_depth;
 }
 
 void
diff --git a/src/lisp.h b/src/lisp.h
index 5ecc48b025..a7a26ef350 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3071,6 +3071,7 @@ union specbinding
       ENUM_BF (specbind_tag) kind : CHAR_BIT;
       void (*func) (Lisp_Object);
       Lisp_Object arg;
+      EMACS_INT eval_depth;
     } unwind;
     struct {
       ENUM_BF (specbind_tag) kind : CHAR_BIT;
-- 
2.17.1