bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Noam Postavsky]

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

Evaluate the following from 'emacs -Q':

    (setq-local foo 1)

    ;; Simulate (debug-watch 'foo) + continue from *Backtrace*
    (add-variable-watcher 'foo (lambda (symbol newval operation where)
                                 (with-temp-buffer
                                   (kill-all-local-variables))))
    (fundamental-mode)

This results in

../../src/data.c:98: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)

Backtrace attached.  I guess it has something to do with the recursive
`kill-all-local-variables' call, although I'm not familiar enough with
the local variable machinery to say more about it.


[-- Attachment #2: gdb backtrace --]
[-- Type: text/plain, Size: 8861 bytes --]

#0  terminate_due_to_signal (sig=6, backtrace_limit=2147483647) at ../../src/emacs.c:364
#1  0x0000000000614655 in die (msg=0x75e788 "found == !EQ (blv->defcell, blv->valcell)", 
    file=0x75e738 "../../src/data.c", line=98) at ../../src/alloc.c:7419
#2  0x0000000000614d5e in set_blv_found (blv=0x2e794b0, found=0) at ../../src/data.c:98
#3  0x0000000000619694 in Fmake_local_variable (variable=XIL(0xba4a0)) at ../../src/data.c:1963
#4  0x000000000063c8da in funcall_subr (subr=0xd750e0 <Smake_local_variable>, numargs=1, 
    args=0x7fffffff9880) at ../../src/eval.c:2844
#5  0x000000000063c419 in Ffuncall (nargs=2, args=0x7fffffff9878) at ../../src/eval.c:2769
#6  0x000000000068d994 in exec_byte_code (bytestr=XIL(0xaecfd4), vector=XIL(0xaecff5), 
    maxdepth=make_number(5), args_template=XIL(0), nargs=0, args=0x0) at ../../src/bytecode.c:629
#7  0x000000000063d500 in funcall_lambda (fun=XIL(0xaecfa5), nargs=1, 
    arg_vector=0xaecff5 <pure+1167285>) at ../../src/eval.c:3052
#8  0x000000000063c45d in Ffuncall (nargs=2, args=0x7fffffff9d78) at ../../src/eval.c:2771
#9  0x000000000068d994 in exec_byte_code (bytestr=XIL(0xaecd84), vector=XIL(0xaecda5), 
    maxdepth=make_number(5), args_template=XIL(0), nargs=0, args=0x0) at ../../src/bytecode.c:629
#10 0x000000000063d500 in funcall_lambda (fun=XIL(0xaecd45), nargs=0, 
    arg_vector=0xaecda5 <pure+1166693>) at ../../src/eval.c:3052
#11 0x000000000063c45d in Ffuncall (nargs=1, args=0x7fffffffa298) at ../../src/eval.c:2771
#12 0x000000000068d994 in exec_byte_code (bytestr=XIL(0xaed084), vector=XIL(0xaed0a5), 
    maxdepth=make_number(1), args_template=XIL(0), nargs=0, args=0x0) at ../../src/bytecode.c:629
#13 0x000000000063d500 in funcall_lambda (fun=XIL(0xaed055), nargs=0, 
    arg_vector=0xaed0a5 <pure+1167461>) at ../../src/eval.c:3052
#14 0x000000000063c45d in Ffuncall (nargs=1, args=0x7fffffffa728) at ../../src/eval.c:2771
#15 0x000000000068d994 in exec_byte_code (bytestr=XIL(0xaed314), vector=XIL(0xaed335), 
    maxdepth=make_number(2), args_template=XIL(0), nargs=0, args=0x0) at ../../src/bytecode.c:629
#16 0x000000000063d500 in funcall_lambda (fun=XIL(0xaed2e5), nargs=0, 
    arg_vector=0xaed335 <pure+1168117>) at ../../src/eval.c:3052
#17 0x000000000063c45d in Ffuncall (nargs=1, args=0x7fffffffabd8) at ../../src/eval.c:2771
#18 0x000000000068d994 in exec_byte_code (bytestr=XIL(0xaed634), vector=XIL(0xaed655), 
    maxdepth=make_number(3), args_template=XIL(0), nargs=0, args=0x0) at ../../src/bytecode.c:629
#19 0x000000000063d500 in funcall_lambda (fun=XIL(0xaed605), nargs=0, 
    arg_vector=0xaed655 <pure+1168917>) at ../../src/eval.c:3052
#20 0x000000000063c45d in Ffuncall (nargs=1, args=0x7fffffffb148) at ../../src/eval.c:2771
#21 0x000000000063b6aa in funcall_nil (nargs=1, args=0x7fffffffb148) at ../../src/eval.c:2400
#22 0x000000000063bbb2 in run_hook_with_args (nargs=1, args=0x7fffffffb148, 
    funcall=0x63b687 <funcall_nil>) at ../../src/eval.c:2577
#23 0x000000000063b730 in Frun_hook_with_args (nargs=1, args=0x7fffffffb148) at ../../src/eval.c:2442
#24 0x000000000063bc43 in run_hook (hook=XIL(0x9f47b0)) at ../../src/eval.c:2590
#25 0x000000000063b6eb in Frun_hooks (nargs=1, args=0x7fffffffb298) at ../../src/eval.c:2424
#26 0x000000000063c7d3 in funcall_subr (subr=0xd77a60 <Srun_hooks>, numargs=1, args=0x7fffffffb298)
    at ../../src/eval.c:2824
#27 0x000000000063c419 in Ffuncall (nargs=2, args=0x7fffffffb290) at ../../src/eval.c:2769
#28 0x000000000068d994 in exec_byte_code (bytestr=XIL(0x9ed45c), vector=XIL(0x9ed47d), 
    maxdepth=make_number(5), args_template=make_number(128), nargs=0, args=0x7fffffffb790)
    at ../../src/bytecode.c:629
#29 0x000000000063d04a in funcall_lambda (fun=XIL(0x9ed42d), nargs=0, arg_vector=0x7fffffffb790)
    at ../../src/eval.c:2970
#30 0x000000000063c45d in Ffuncall (nargs=1, args=0x7fffffffb788) at ../../src/eval.c:2771
#31 0x000000000068d994 in exec_byte_code (bytestr=XIL(0xa2bbac), vector=XIL(0xa9050d), 
    maxdepth=make_number(1), args_template=make_number(0), nargs=0, args=0x7fffffffbbb0)
    at ../../src/bytecode.c:629
#32 0x000000000063d04a in funcall_lambda (fun=XIL(0xa904cd), nargs=0, arg_vector=0x7fffffffbbb0)
    at ../../src/eval.c:2970
#33 0x000000000063cc84 in apply_lambda (fun=XIL(0xa904cd), args=XIL(0), count=39)
    at ../../src/eval.c:2906
#34 0x000000000063ae85 in eval_sub (form=XIL(0x1756233)) at ../../src/eval.c:2279
#35 0x00000000006721fc in readevalloop_eager_expand_eval (val=XIL(0x1756233), 
    macroexpand=XIL(0xd9ac0)) at ../../src/lread.c:1850
#36 0x0000000000672b7d in readevalloop (readcharfun=XIL(0x2e50995), infile0=0x0, 
    sourcename=XIL(0x2eb84e4), printflag=false, unibyte=XIL(0), readfun=XIL(0), start=XIL(0), 
    end=XIL(0)) at ../../src/lread.c:2036
#37 0x0000000000672f97 in Feval_buffer (buffer=XIL(0x2e50995), printflag=XIL(0), 
    filename=XIL(0x3069604), unibyte=XIL(0), do_allow_print=XIL(0xc090)) at ../../src/lread.c:2103
#38 0x000000000063c9a0 in funcall_subr (subr=0xd7a4a0 <Seval_buffer>, numargs=5, args=0x7fffffffbfd0)
    at ../../src/eval.c:2856
#39 0x000000000063c419 in Ffuncall (nargs=6, args=0x7fffffffbfc8) at ../../src/eval.c:2769
#40 0x000000000068d994 in exec_byte_code (bytestr=XIL(0x9fb57c), vector=XIL(0x9fb59d), 
    maxdepth=make_number(6), args_template=XIL(0), nargs=0, args=0x0) at ../../src/bytecode.c:629
#41 0x000000000063d500 in funcall_lambda (fun=XIL(0x9fb4fd), nargs=4, 
    arg_vector=0x9fb59d <pure+177501>) at ../../src/eval.c:3052
#42 0x000000000063c45d in Ffuncall (nargs=5, args=0x7fffffffc550) at ../../src/eval.c:2771
#43 0x000000000063be16 in call4 (fn=XIL(0x43abe0), arg1=XIL(0x3069604), arg2=XIL(0x3069604), 
    arg3=XIL(0), arg4=XIL(0xc090)) at ../../src/eval.c:2645
#44 0x0000000000670c38 in Fload (file=XIL(0x3068ff4), noerror=XIL(0), nomessage=XIL(0xc090), 
    nosuffix=XIL(0), must_suffix=XIL(0)) at ../../src/lread.c:1365
#45 0x000000000063c9a0 in funcall_subr (subr=0xd7a420 <Sload>, numargs=3, args=0x7fffffffc938)
    at ../../src/eval.c:2856
#46 0x000000000063c419 in Ffuncall (nargs=4, args=0x7fffffffc930) at ../../src/eval.c:2769
#47 0x000000000068d994 in exec_byte_code (bytestr=XIL(0xadcc2c), vector=XIL(0xadcc4d), 
    maxdepth=make_number(23), args_template=make_number(257), nargs=1, args=0x7fffffffd298)
    at ../../src/bytecode.c:629
#48 0x000000000063d04a in funcall_lambda (fun=XIL(0xadcbfd), nargs=1, arg_vector=0x7fffffffd290)
    at ../../src/eval.c:2970
#49 0x000000000063c45d in Ffuncall (nargs=2, args=0x7fffffffd288) at ../../src/eval.c:2771
#50 0x000000000068d994 in exec_byte_code (bytestr=XIL(0xad7424), vector=XIL(0xad7445), 
    maxdepth=make_number(21), args_template=make_number(0), nargs=0, args=0x7fffffffdea8)
    at ../../src/bytecode.c:629
#51 0x000000000063d04a in funcall_lambda (fun=XIL(0xad73f5), nargs=0, arg_vector=0x7fffffffdea8)
    at ../../src/eval.c:2970
#52 0x000000000063c45d in Ffuncall (nargs=1, args=0x7fffffffdea0) at ../../src/eval.c:2771
#53 0x000000000068d994 in exec_byte_code (bytestr=XIL(0xad6614), vector=XIL(0xad6635), 
    maxdepth=make_number(12), args_template=make_number(0), nargs=0, args=0x7fffffffe4d0)
    at ../../src/bytecode.c:629
#54 0x000000000063d04a in funcall_lambda (fun=XIL(0xad65e5), nargs=0, arg_vector=0x7fffffffe4d0)
    at ../../src/eval.c:2970
#55 0x000000000063cc84 in apply_lambda (fun=XIL(0xad65e5), args=XIL(0), count=4)
    at ../../src/eval.c:2906
#56 0x000000000063ae85 in eval_sub (form=XIL(0x149a8e3)) at ../../src/eval.c:2279
#57 0x000000000063a1fa in Feval (form=XIL(0x149a8e3), lexical=XIL(0)) at ../../src/eval.c:2054
#58 0x000000000058156a in top_level_2 () at ../../src/keyboard.c:1119
#59 0x0000000000638529 in internal_condition_case (bfun=0x581547 <top_level_2>, handlers=XIL(0x5250), 
    hfun=0x580f4c <cmd_error>) at ../../src/eval.c:1332
#60 0x00000000005815af in top_level_1 (ignore=XIL(0)) at ../../src/keyboard.c:1127
#61 0x0000000000637a32 in internal_catch (tag=XIL(0xc6f0), func=0x58156c <top_level_1>, arg=XIL(0))
    at ../../src/eval.c:1097
#62 0x0000000000581499 in command_loop () at ../../src/keyboard.c:1088
#63 0x0000000000580a36 in recursive_edit_1 () at ../../src/keyboard.c:695
#64 0x0000000000580c2b in Frecursive_edit () at ../../src/keyboard.c:766
#65 0x000000000057e7c6 in main (argc=4, argv=0x7fffffffea38) at ../../src/emacs.c:1713

Lisp Backtrace:
"make-local-variable" (0xffff9880)
"font-lock-default-function" (0xffff9d80)
"font-lock-mode" (0xffffa2a0)
"turn-on-font-lock" (0xffffa730)
"turn-on-font-lock-if-desired" (0xffffabe0)
"global-font-lock-mode-enable-in-buffers" (0xffffb150)
"run-hooks" (0xffffb298)
"run-mode-hooks" (0xffffb790)
"fundamental-mode" (0xffffbbb0)
"eval-buffer" (0xffffbfd0)
"load-with-code-conversion" (0xffffc558)
"load" (0xffffc938)
"command-line-1" (0xffffd290)
"command-line" (0xffffdea8)
"normal-top-level" (0xffffe4d0)

[-- Attachment #3: Type: text/plain, Size: 431 bytes --]


In GNU Emacs 26.0.91 (build 54, x86_64-pc-linux-gnu, X toolkit, Xaw scroll bars)
 of 2018-03-18 built on zebian
Repository revision: 10bd3b3af8acfc226acadc654298865cffc19cc9

Configured using:
 'configure --cache-file=../../debug-config.cache 'CC=ccache gcc'
 'CFLAGS=-O0 -g3 -march=native' --enable-checking=yes
 --enable-check-lisp-object-type --with-x-toolkit=lucid
 --with-toolkit-scroll-bars --with-gif=no --with-jpeg=no'

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Sun, 18 Mar 2018 09:10:41 -0400
> 
> Evaluate the following from 'emacs -Q':
> 
>     (setq-local foo 1)
> 
>     ;; Simulate (debug-watch 'foo) + continue from *Backtrace*
>     (add-variable-watcher 'foo (lambda (symbol newval operation where)
>                                  (with-temp-buffer
>                                    (kill-all-local-variables))))
>     (fundamental-mode)
> 
> This results in
> 
> ../../src/data.c:98: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)
> 
> Backtrace attached.  I guess it has something to do with the recursive
> `kill-all-local-variables' call, although I'm not familiar enough with
> the local variable machinery to say more about it.

Do you mean that the inner call to kill-all-local-variables steps on
toes of the outer call, and thus corrupts the local values or
something?  If so, do you see any signs of such a corruption?

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Sun, 18 Mar 2018 09:10:41 -0400
> 
> Evaluate the following from 'emacs -Q':
> 
>     (setq-local foo 1)
> 
>     ;; Simulate (debug-watch 'foo) + continue from *Backtrace*
>     (add-variable-watcher 'foo (lambda (symbol newval operation where)
>                                  (with-temp-buffer
>                                    (kill-all-local-variables))))
>     (fundamental-mode)
> 
> This results in
> 
> ../../src/data.c:98: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)
> 
> Backtrace attached.  I guess it has something to do with the recursive
> `kill-all-local-variables' call, although I'm not familiar enough with
> the local variable machinery to say more about it.

Do you mean that the inner call to kill-all-local-variables steps on
toes of the outer call, and thus corrupts the local values or
something?  If so, do you see any signs of such a corruption?  Because
otherwise maybe the assertion is wrong?

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Date: Sun, 18 Mar 2018 09:10:41 -0400
>> 
>> Evaluate the following from 'emacs -Q':
>> 
>>     (setq-local foo 1)
>> 
>>     ;; Simulate (debug-watch 'foo) + continue from *Backtrace*
>>     (add-variable-watcher 'foo (lambda (symbol newval operation where)
>>                                  (with-temp-buffer
>>                                    (kill-all-local-variables))))
>>     (fundamental-mode)
>> 
>> This results in
>> 
>> ../../src/data.c:98: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)
>> 
>> Backtrace attached.  I guess it has something to do with the recursive
>> `kill-all-local-variables' call, although I'm not familiar enough with
>> the local variable machinery to say more about it.
>
> Do you mean that the inner call to kill-all-local-variables steps on
> toes of the outer call, and thus corrupts the local values or
> something?  If so, do you see any signs of such a corruption?  Because
> otherwise maybe the assertion is wrong?

No, I haven't seen any signs of corrupted values, though I'm not sure
exactly where to look.  It's possible the assertion is wrong (or more
precisely, that the variable watcher breaks the assertion without
breaking anything else).  I don't really understand what the assertion
is testing, at a high level (that is, why does it expect 'defcell' and
'valcell' to have that relation).

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: 30846@debbugs.gnu.org
> Date: Sun, 18 Mar 2018 10:20:54 -0400
> 
> I don't really understand what the assertion is testing, at a high
> level (that is, why does it expect 'defcell' and 'valcell' to have
> that relation).

Yes, the 'found' member could use a better documentation.  (Btw, all
calls to set_blv_found seem to use the last argument of zero.)

I hope Stefan can comment on this.

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Cc: 30846@debbugs.gnu.org
>> Date: Sun, 18 Mar 2018 10:20:54 -0400
>> 
>> I don't really understand what the assertion is testing, at a high
>> level (that is, why does it expect 'defcell' and 'valcell' to have
>> that relation).
>
> Yes, the 'found' member could use a better documentation.  (Btw, all
> calls to set_blv_found seem to use the last argument of zero.)
>
> I hope Stefan can comment on this.

I'll add him to Cc then, I think he's not subscribed to the bug list.

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Stefan Monnier]

> I'll add him to Cc then, I think he's not subscribed to the bug list.

Indeed, I'm not, thanks.

I think there's a real bug in there (kill-local-variable may leave
valcell pointing to the old (now used) cell), let me take a closer look.


        Stefan

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Stefan Monnier]

> This results in
>
> ../../src/data.c:98: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)

OK, I found the culprit: in kill-all-local-variables, we first change
all the (relevant) symbols to point to their global value (with
swap_in_global_binding called from swap_out_buffer_local_variables),
then go and kill them one by one (in reset_buffer_local_variables).
This worked fine in the past, but now that we run watchers while we kill
the vars, the act of running the watchers may undo the effect of
swap_in_global_binding, so we can't kill them quite in the same way.

The patch below against emacs-26 seems to fix the problem (it mostly
merges the code of swap_out_buffer_local_variables into that of
reset_buffer_local_variables so that the swap_in_global_binding is done
just before we actually kill the var, with no opportunity for watchers
to undo the effect).

The patch doesn't only fix this problem, it also changes the time at
which we run the watcher: in the current emacs-26 code,
kill-all-local-variables runs the watcher *after* killing the
corresponding var, whereas usually the watchers are run *before* the var
is modified.  I can split the patch into two, if you want and/or only
apply the part that actually fixes this bug.

If you feel this is too risky for emacs-26, I wouldn't blame you (this
is pretty tricky code): while the assertion crashes Emacs, a normal
build without assertions will likely not notice the problem at all.
I came up with a test case that catches the problem, but I think that in
"real" life it's very unlikely to cause a problem.

It applies unchanged to master (and while looking at this bug I found
a whole bunch of other minor changes that I plan to install into master
anyway).


        Stefan


diff --git a/src/buffer.c b/src/buffer.c
index 9b54e4b778..b0cee71703 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -108,7 +108,6 @@ int last_per_buffer_idx;
 static void call_overlay_mod_hooks (Lisp_Object list, Lisp_Object overlay,
                                     bool after, Lisp_Object arg1,
                                     Lisp_Object arg2, Lisp_Object arg3);
-static void swap_out_buffer_local_variables (struct buffer *b);
 static void reset_buffer_local_variables (struct buffer *, bool);
 
 /* Alist of all buffer names vs the buffers.  This used to be
@@ -991,10 +990,29 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
   else
     {
       Lisp_Object tmp, last = Qnil;
+      Lisp_Object buffer;
+      XSETBUFFER (buffer, b);
+
       for (tmp = BVAR (b, local_var_alist); CONSP (tmp); tmp = XCDR (tmp))
         {
           Lisp_Object local_var = XCAR (XCAR (tmp));
           Lisp_Object prop = Fget (local_var, Qpermanent_local);
+          Lisp_Object sym = local_var;
+
+          /* Watchers are run *before* modifying the var.  */
+          if (XSYMBOL (local_var)->u.s.trapped_write == SYMBOL_TRAPPED_WRITE)
+            notify_variable_watchers (local_var, Qnil,
+                                      Qmakunbound, Fcurrent_buffer ());
+
+          eassert (XSYMBOL (sym)->u.s.redirect == SYMBOL_LOCALIZED);
+          /* Need not do anything if some other buffer's binding is
+	     now cached.  */
+          if (EQ (SYMBOL_BLV (XSYMBOL (sym))->where, buffer))
+	    {
+	      /* Symbol is set up for this buffer's old local value:
+	         swap it out!  */
+	      swap_in_global_binding (XSYMBOL (sym));
+	    }
 
           if (!NILP (prop))
             {
@@ -1034,10 +1052,6 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
             bset_local_var_alist (b, XCDR (tmp));
           else
             XSETCDR (last, XCDR (tmp));
-
-          if (XSYMBOL (local_var)->u.s.trapped_write == SYMBOL_TRAPPED_WRITE)
-            notify_variable_watchers (local_var, Qnil,
-                                      Qmakunbound, Fcurrent_buffer ());
         }
     }
 
@@ -1867,7 +1881,6 @@ cleaning up all windows currently displaying the buffer to be killed. */)
      won't be protected from GC.  They would be protected
      if they happened to remain cached in their symbols.
      This gets rid of them for certain.  */
-  swap_out_buffer_local_variables (b);
   reset_buffer_local_variables (b, 1);
 
   bset_name (b, Qnil);
@@ -2737,11 +2750,6 @@ the normal hook `change-major-mode-hook'.  */)
 {
   run_hook (Qchange_major_mode_hook);
 
-  /* Make sure none of the bindings in local_var_alist
-     remain swapped in, in their symbols.  */
-
-  swap_out_buffer_local_variables (current_buffer);
-
   /* Actually eliminate all local bindings of this buffer.  */
 
   reset_buffer_local_variables (current_buffer, 0);
@@ -2753,31 +2761,6 @@ the normal hook `change-major-mode-hook'.  */)
   return Qnil;
 }
 
-/* Make sure no local variables remain set up with buffer B
-   for their current values.  */
-
-static void
-swap_out_buffer_local_variables (struct buffer *b)
-{
-  Lisp_Object oalist, alist, buffer;
-
-  XSETBUFFER (buffer, b);
-  oalist = BVAR (b, local_var_alist);
-
-  for (alist = oalist; CONSP (alist); alist = XCDR (alist))
-    {
-      Lisp_Object sym = XCAR (XCAR (alist));
-      eassert (XSYMBOL (sym)->u.s.redirect == SYMBOL_LOCALIZED);
-      /* Need not do anything if some other buffer's binding is
-	 now cached.  */
-      if (EQ (SYMBOL_BLV (XSYMBOL (sym))->where, buffer))
-	{
-	  /* Symbol is set up for this buffer's old local value:
-	     swap it out!  */
-	  swap_in_global_binding (XSYMBOL (sym));
-	}
-    }
-}
 \f
 /* Find all the overlays in the current buffer that contain position POS.
    Return the number found, and store them in a vector in *VEC_PTR.
diff --git a/test/src/data-tests.el b/test/src/data-tests.el
index dda1278b6d..34637e4bfb 100644
--- a/test/src/data-tests.el
+++ b/test/src/data-tests.el
@@ -1,4 +1,4 @@
-;;; data-tests.el --- tests for src/data.c
+;;; data-tests.el --- tests for src/data.c  -*- lexical-binding:t -*-
 
 ;; Copyright (C) 2013-2018 Free Software Foundation, Inc.
 
@@ -484,3 +484,20 @@ binding-test-some-local
       (remove-variable-watcher 'data-tests-lvar collect-watch-data)
       (setq data-tests-lvar 6)
       (should (null watch-data)))))
+
+(ert-deftest data-tests-kill-all-local-variables () ;bug#30846
+  (with-temp-buffer
+    (setq-local data-tests-foo1 1)
+    (setq-local data-tests-foo2 2)
+    (setq-local data-tests-foo3 3)
+    (let ((oldfoo2 nil))
+      (add-variable-watcher 'data-tests-foo2
+                            (lambda (&rest _)
+                              (setq oldfoo2 (bound-and-true-p data-tests-foo2))))
+      (kill-all-local-variables)
+      (should (equal oldfoo2 '2)) ;Watcher is run before changing the var.
+      (should (not (or (bound-and-true-p data-tests-foo1)
+                       (bound-and-true-p data-tests-foo2)
+                       (bound-and-true-p data-tests-foo3)))))))
+
+;;; data-tests.el ends here

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Thu, 22 Mar 2018 11:45:32 -0400
> Cc: 30846@debbugs.gnu.org
> 
> If you feel this is too risky for emacs-26, I wouldn't blame you (this
> is pretty tricky code): while the assertion crashes Emacs, a normal
> build without assertions will likely not notice the problem at all.
> I came up with a test case that catches the problem, but I think that in
> "real" life it's very unlikely to cause a problem.
> 
> It applies unchanged to master (and while looking at this bug I found
> a whole bunch of other minor changes that I plan to install into master
> anyway).

I indeed prefer to install this on master, not on the release branch.

Thanks.

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Noam Postavsky]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> If you feel this is too risky for emacs-26, I wouldn't blame you (this
> is pretty tricky code): while the assertion crashes Emacs, a normal
> build without assertions will likely not notice the problem at all.
> I came up with a test case that catches the problem, but I think that in
> "real" life it's very unlikely to cause a problem.

Should we disable the assertion in emacs-26 then?

And would the diff below updating comments on struct
Lisp_Buffer_Local_Value be correct?

--- i/src/lisp.h
+++ w/src/lisp.h
@@ -2593,10 +2593,10 @@ XUSER_PTR (Lisp_Object a)
    variable, you must first make sure the right binding is loaded;
    then you can access the value in (or through) `realvalue'.
 
-   `buffer' and `frame' are the buffer and frame for which the loaded
-   binding was found.  If those have changed, to make sure the right
+   `where' is the buffer for which the loaded
+   binding was found.  If it has changed, to make sure the right
    binding is loaded it is necessary to find which binding goes with
-   the current buffer and selected frame, then load it.  To load it,
+   the current buffer, then load it.  To load it,
    first unload the previous binding, then copy the value of the new
    binding into `realvalue' (or through it).  Also update
    LOADED-BINDING to point to the newly loaded binding.
@@ -2615,14 +2615,14 @@ XUSER_PTR (Lisp_Object a)
     bool_bf found : 1;
     /* If non-NULL, a forwarding to the C var where it should also be set.  */
     union Lisp_Fwd *fwd;	/* Should never be (Buffer|Kboard)_Objfwd.  */
-    /* The buffer or frame for which the loaded binding was found.  */
+    /* The buffer for which the loaded binding was found.  */
     Lisp_Object where;
     /* A cons cell that holds the default value.  It has the form
        (SYMBOL . DEFAULT-VALUE).  */
     Lisp_Object defcell;
     /* The cons cell from `where's parameter alist.
        It always has the form (SYMBOL . VALUE)
-       Note that if `forward' is non-nil, VALUE may be out of date.
+       Note that if `fwd' is non-NULL, VALUE may be out of date.
        Also if the currently loaded binding is the default binding, then
        this is `eq'ual to defcell.  */
     Lisp_Object valcell;

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Stefan Monnier]

> Should we disable the assertion in emacs-26 then?

I don't have an opinion on this.

> And would the diff below updating comments on struct
> Lisp_Buffer_Local_Value be correct?

Yes.


        Stefan

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Noam Postavsky]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> And would the diff below updating comments on struct
>> Lisp_Buffer_Local_Value be correct?
>
> Yes.

Pushed to emacs-26 [1: b8ebf5fb64].

Eli Zaretskii <eliz@gnu.org> writes:

>> Should we disable the assertion in emacs-26 then?

> Do we believe people build a production version with --enable-checking?

Hmm, no, probably not.

[1: b8ebf5fb64]: 2018-03-23 08:19:42 -0400
  * src/lisp.h (struct Lisp_Buffer_Local_Value): Update commentary.
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b8ebf5fb64dbf261315bfdb281a8b0a119e7cc2b

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Thu, 22 Mar 2018 20:20:13 -0400
> Cc: 30846@debbugs.gnu.org
> 
> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> 
> > If you feel this is too risky for emacs-26, I wouldn't blame you (this
> > is pretty tricky code): while the assertion crashes Emacs, a normal
> > build without assertions will likely not notice the problem at all.
> > I came up with a test case that catches the problem, but I think that in
> > "real" life it's very unlikely to cause a problem.
> 
> Should we disable the assertion in emacs-26 then?

Do we believe people build a production version with --enable-checking?

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Stefan Monnier]

>> > If you feel this is too risky for emacs-26, I wouldn't blame you (this
>> > is pretty tricky code): while the assertion crashes Emacs, a normal
>> > build without assertions will likely not notice the problem at all.
>> > I came up with a test case that catches the problem, but I think that in
>> > "real" life it's very unlikely to cause a problem.
>> Should we disable the assertion in emacs-26 then?
> Do we believe people build a production version with --enable-checking?

Does that mean you'd rather not install my patch into emacs-26 or are
you still undecided?


        Stefan

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Noam Postavsky <npostavs@gmail.com>, 30846@debbugs.gnu.org
> Date: Fri, 23 Mar 2018 08:57:13 -0400
> 
> >> > If you feel this is too risky for emacs-26, I wouldn't blame you (this
> >> > is pretty tricky code): while the assertion crashes Emacs, a normal
> >> > build without assertions will likely not notice the problem at all.
> >> > I came up with a test case that catches the problem, but I think that in
> >> > "real" life it's very unlikely to cause a problem.
> >> Should we disable the assertion in emacs-26 then?
> > Do we believe people build a production version with --enable-checking?
> 
> Does that mean you'd rather not install my patch into emacs-26 or are
> you still undecided?

I'm quite sure we shouldn't put this on emacs-26, I was just replying
to Noam's suggestion to remove the assertion.

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Stefan Monnier]

>> Does that mean you'd rather not install my patch into emacs-26 or are
>> you still undecided?
> I'm quite sure we shouldn't put this on emacs-26, I was just replying
> to Noam's suggestion to remove the assertion.

OK, thanks, I'll prepare my patch for master, then.


        Stefan

bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Stefan Monnier]

Version: 27.1

> The patch doesn't only fix this problem, it also changes the time at
> which we run the watcher: in the current emacs-26 code,

Installed into master,


        Stefan