From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.bugs Subject: bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)' Date: Thu, 22 Mar 2018 11:45:32 -0400 Message-ID: References: <87sh8xttpq.fsf@gmail.com> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1521733453 24298 195.159.176.226 (22 Mar 2018 15:44:13 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Thu, 22 Mar 2018 15:44:13 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) Cc: 30846@debbugs.gnu.org To: Noam Postavsky Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu Mar 22 16:44:09 2018 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ez2Ns-0006D3-In for geb-bug-gnu-emacs@m.gmane.org; Thu, 22 Mar 2018 16:44:08 +0100 Original-Received: from localhost ([::1]:33281 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ez2Pv-0004rc-WE for geb-bug-gnu-emacs@m.gmane.org; Thu, 22 Mar 2018 11:46:16 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:38638) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ez2Po-0004rB-C9 for bug-gnu-emacs@gnu.org; Thu, 22 Mar 2018 11:46:09 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ez2Pi-0000Tz-5O for bug-gnu-emacs@gnu.org; Thu, 22 Mar 2018 11:46:08 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:40136) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1ez2Pi-0000TR-1o for bug-gnu-emacs@gnu.org; Thu, 22 Mar 2018 11:46:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1ez2Ph-0004rr-Ol for bug-gnu-emacs@gnu.org; Thu, 22 Mar 2018 11:46:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 22 Mar 2018 15:46:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 30846 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 30846-submit@debbugs.gnu.org id=B30846.152173353818680 (code B ref 30846); Thu, 22 Mar 2018 15:46:01 +0000 Original-Received: (at 30846) by debbugs.gnu.org; 22 Mar 2018 15:45:38 +0000 Original-Received: from localhost ([127.0.0.1]:48033 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ez2PJ-0004rE-Dt for submit@debbugs.gnu.org; Thu, 22 Mar 2018 11:45:37 -0400 Original-Received: from chene.dit.umontreal.ca ([132.204.246.20]:55406) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ez2PG-0004r4-9G for 30846@debbugs.gnu.org; Thu, 22 Mar 2018 11:45:35 -0400 Original-Received: from pastel.home (lechon.iro.umontreal.ca [132.204.27.242]) by chene.dit.umontreal.ca (8.14.7/8.14.1) with ESMTP id w2MFjWu2006466; Thu, 22 Mar 2018 11:45:32 -0400 Original-Received: by pastel.home (Postfix, from userid 20848) id 1BAB5604C8; Thu, 22 Mar 2018 11:45:32 -0400 (EDT) In-Reply-To: <87sh8xttpq.fsf@gmail.com> (Noam Postavsky's message of "Sun, 18 Mar 2018 09:10:41 -0400") X-NAI-Spam-Flag: NO X-NAI-Spam-Level: X-NAI-Spam-Threshold: 5 X-NAI-Spam-Score: 0.2 X-NAI-Spam-Rules: 3 Rules triggered LNG_SB_1=0.2, EDT_SA_DN_PASS=0, RV6248=0 X-NAI-Spam-Version: 2.3.0.9418 : core <6248> : inlines <6514> : streams <1781946> : uri <2612879> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:144529 Archived-At: > 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)); - } - } -} /* 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