master ecaedf2117: (specpdl_unrewind): Fix corner case

[Stefan Monnier]

branch: master
commit ecaedf2117cb015ad4028e4d6fc7058608c98096
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Commit: Stefan Monnier <monnier@iro.umontreal.ca>

    (specpdl_unrewind): Fix corner case
    
    * src/lisp.h (enum specbind_tag): New elem SPECPDL_NOP.
    
    * src/eval.c (specpdl_unrewind): Zap entries that can't be applied any more,
    and simplify.
    (default_toplevel_binding, lexbound_p, Fbacktrace__locals): Simplify.
    (do_one_unbind, mark_specpdl): Handle SPECPDL_NOP.
---
 src/eval.c | 83 ++++++++++++--------------------------------------------------
 src/lisp.h |  1 +
 2 files changed, 17 insertions(+), 67 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index d3342289fb..294d79e67a 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -664,23 +664,7 @@ default_toplevel_binding (Lisp_Object symbol)
            binding = pdl;
          break;
 
-       case SPECPDL_UNWIND:
-       case SPECPDL_UNWIND_ARRAY:
-       case SPECPDL_UNWIND_PTR:
-       case SPECPDL_UNWIND_INT:
-       case SPECPDL_UNWIND_INTMAX:
-       case SPECPDL_UNWIND_EXCURSION:
-       case SPECPDL_UNWIND_VOID:
-       case SPECPDL_BACKTRACE:
-#ifdef HAVE_MODULES
-        case SPECPDL_MODULE_RUNTIME:
-        case SPECPDL_MODULE_ENVIRONMENT:
-#endif
-       case SPECPDL_LET_LOCAL:
-         break;
-
-       default:
-         emacs_abort ();
+       default: break;
        }
     }
   return binding;
@@ -707,23 +691,7 @@ lexbound_p (Lisp_Object symbol)
            }
          break;
 
-       case SPECPDL_UNWIND:
-       case SPECPDL_UNWIND_ARRAY:
-       case SPECPDL_UNWIND_PTR:
-       case SPECPDL_UNWIND_INT:
-       case SPECPDL_UNWIND_INTMAX:
-       case SPECPDL_UNWIND_EXCURSION:
-       case SPECPDL_UNWIND_VOID:
-       case SPECPDL_BACKTRACE:
-#ifdef HAVE_MODULES
-        case SPECPDL_MODULE_RUNTIME:
-        case SPECPDL_MODULE_ENVIRONMENT:
-#endif
-       case SPECPDL_LET_LOCAL:
-         break;
-
-       default:
-         emacs_abort ();
+       default: break;
        }
     }
   return false;
@@ -3721,6 +3689,7 @@ do_one_unbind (union specbinding *this_binding, bool 
unwinding,
                              this_binding->unwind_excursion.window);
       break;
     case SPECPDL_BACKTRACE:
+    case SPECPDL_NOP:
       break;
 #ifdef HAVE_MODULES
     case SPECPDL_MODULE_RUNTIME:
@@ -4044,17 +4013,6 @@ specpdl_unrewind (union specbinding *pdl, int distance, 
bool vars_only)
            save_excursion_restore (marker, window);
          }
          break;
-       case SPECPDL_UNWIND_ARRAY:
-       case SPECPDL_UNWIND_PTR:
-       case SPECPDL_UNWIND_INT:
-       case SPECPDL_UNWIND_INTMAX:
-       case SPECPDL_UNWIND_VOID:
-       case SPECPDL_BACKTRACE:
-#ifdef HAVE_MODULES
-        case SPECPDL_MODULE_RUNTIME:
-        case SPECPDL_MODULE_ENVIRONMENT:
-#endif
-         break;
        case SPECPDL_LET:
          { /* If variable has a trivial value (no forwarding), we can
               just set it.  No need to check for constant symbols here,
@@ -4097,14 +4055,16 @@ specpdl_unrewind (union specbinding *pdl, int distance, 
bool vars_only)
                               SET_INTERNAL_THREAD_SWITCH);
              }
            else
-             /* FIXME: If the var is not local any more, we failed
-                 to swap the old and new values.  As long as the var remains
-                 non-local, this is fine, but if it ever reverts to being
-                 local we may end up using this entry "in the wrong
-                 direction".  */
-             {}
+             /* If the var is not local any more, it can't be undone nor
+                 redone, so just zap it.
+                 This is important in case the buffer re-gains a local value
+                 before we unrewind again, in which case we'd risk applying
+                 this entry in the wrong direction.  */
+             tmp->kind = SPECPDL_NOP;
          }
          break;
+
+       default: break;
        }
     }
 }
@@ -4195,22 +4155,7 @@ NFRAMES and BASE specify the activation frame to use, as 
in `backtrace-frame'.
            }
            break;
 
-         case SPECPDL_UNWIND:
-         case SPECPDL_UNWIND_ARRAY:
-         case SPECPDL_UNWIND_PTR:
-         case SPECPDL_UNWIND_INT:
-         case SPECPDL_UNWIND_INTMAX:
-         case SPECPDL_UNWIND_EXCURSION:
-         case SPECPDL_UNWIND_VOID:
-         case SPECPDL_BACKTRACE:
-#ifdef HAVE_MODULES
-          case SPECPDL_MODULE_RUNTIME:
-          case SPECPDL_MODULE_ENVIRONMENT:
-#endif
-           break;
-
-         default:
-           emacs_abort ();
+         default: break;
          }
       }
   }
@@ -4274,8 +4219,12 @@ mark_specpdl (union specbinding *first, union 
specbinding *ptr)
        case SPECPDL_UNWIND_INT:
        case SPECPDL_UNWIND_INTMAX:
         case SPECPDL_UNWIND_VOID:
+       case SPECPDL_NOP:
          break;
 
+       /* While other loops that scan the specpdl use "default: break;"
+          for simplicity, here we explicitly list all cases and abort
+          if we find an unexpected value, as a sanity check.  */
        default:
          emacs_abort ();
        }
diff --git a/src/lisp.h b/src/lisp.h
index 19788ef07c..deeca9bc86 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3284,6 +3284,7 @@ enum specbind_tag {
   SPECPDL_UNWIND_EXCURSION,    /* Likewise, on an excursion.  */
   SPECPDL_UNWIND_VOID,         /* Likewise, with no arg.  */
   SPECPDL_BACKTRACE,           /* An element of the backtrace.  */
+  SPECPDL_NOP,                 /* A filler.  */
 #ifdef HAVE_MODULES
   SPECPDL_MODULE_RUNTIME,       /* A live module runtime.  */
   SPECPDL_MODULE_ENVIRONMENT,   /* A live module environment.  */