all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#70183: [PATCH] Fix + ert for multiple watcher notifications (2 of 9)
@ 2024-04-04  8:44 Robert Burks
  2024-04-06  7:29 ` Eli Zaretskii
  2024-04-07  3:17 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 2 replies; 4+ messages in thread
From: Robert Burks @ 2024-04-04  8:44 UTC (permalink / raw)
  To: 70183


[-- Attachment #1.1: Type: text/plain, Size: 5985 bytes --]

(2 of 9)

Bug#00001a (Using set-default with an alias double notifies)
Bug#00001a (Using set-default on the alias of an unbound forwarded symbol)
Bug#00001b (Setting a let bound forwarded symbol in buffer with no blv)

** Bug recreation is at the end
(These are extremely specific paths derived from digging at the c code
to make bugs happen, they are in no way intentional execution paths.)

I am calling all of these the same bug because they arise from the
functions set_internal and set_default_internal each calling the other
along specific paths.  There exists a way to achieve triple notification;
however, that involves combining with another bug that I will cover later.

Attached are patches to fix and testing for the bugs shown below.
(Please note the BUG# placeholder in twelve(12) places will need to be
updated.)

I divided the fix into a commit for each function.  These functions are
tangled together so both are required to fix the problems. I added the
testing as a separate commit.

Also, there is reasoning behind checking for the constant at every
step of redirection that will play out along the rest of my submissions.
It isn't so much about checking at every step as much as it is about the
end of the chain. At the top in the goto start loop is the easiest place to
make that happen.

  Additionally, I moved the XSETSYMBOL to the top from inside the
localized to point out that while redirection was important, the notes
there didn't express how important it was to convert the symbol to bare.
If handled only during cases of redirection there will occur
times when byte compiling in which set_internal will be passed a symbol
with position and then store it as a blv. Then kill-buffer will choke
on encountering a symbol with position in a blv cons cell when it is
accessed with positions disabled.

e.g. auto-composition-mode was one I encountered.  Simply moving the
XSETSYMBOL to only happen after variable alias redirection caused an issue.
That variable is a DEFVAR_LISP but is made buffer local in an .el file.
Everything would build fine but byte compilation would die on that file
because kill-buffer would pull all blv's and encounter that one put on
with 'position' while not being in a 'positions-enabled' environment.

Bug Recreations -------------------------------------------------------

Bug#00001a (Using set-default with an alias double notifies)
emacs -Q---------------------------------------------------------------

(defvar test 5)
test

(defvar result nil)
result

(defvaralias 'test-alias 'test)
test

(add-variable-watcher 'test (lambda (&rest args)
                                (push args result)))
nil

(set-default 'test-alias 100)
100

result
((test 100 set nil) (test 100 set nil))
;; there should only be one result here!!!!!!!!!!
;; This bug arises from set_default_internal making notification
;; one time for the alias and another time when it calls set_internal.

Bug#00001a (Using set-default on the alias of an unbound forwarded symbol)
emacs -Q---------------------------------------------------------------

(defvar results nil)
results

(add-variable-watcher 'right-margin-width (lambda (&rest args) (push args
results)))
nil

(defvaralias 'testa 'right-margin-width)
right-margin-width

(makunbound 'right-margin-width)
right-margin-width

(set 'results nil)
nil

(set-default 'testa 2000)
2000

results
((right-margin-width 2000 set nil) (right-margin-width 2000 set nil))
;; only one set occurred!!!!!!!!
;; Calling set-default with the alias of an unbound forwarded symbol
;; causes watchers to be notified once in set_default_internal and another
;; time in set_internal.
;; This is the same bug as the previous example and occurs because
set_internal converts
;; forwarded symbol into a plain value when it is unbound.


Bug#00001b (Setting a let bound forwarded symbol in a buffer with no blv)
emacs -Q---------------------------------------------------------------

(defvar results nil)
results

(add-variable-watcher 'right-margin-width (lambda (&rest args) (push args
results)))
nil

(let ((right-margin-width 150))
  (set 'right-margin-width 2000))
2000

results
((right-margin-width 0 set nil) (right-margin-width 2000 set nil)
(right-margin-width 2000 set #<buffer *scratch*>) (right-margin-width 150
set nil))
;;                                                   ^
             ^
;; These are double
_________________________________|__________________________________|
;; Notification is being sent once in set_internal and again in
;; set_default_internal as a result of being a let bound forwarded symbol.
;; Also, it is not sending the proper 'operation'.  While these should be
;; acting on the default value just as a normal blv that is shadowing
default
;; does, with blv the notification still reflects the proper operation used.
;; That being said, in this scenario they should not have a buffer name as
it is
;; acting globally but it should still have the 'let' and 'unlet' like a
blv would.
;; The (right-margin-width 2000 set #<buffer *scratch*>) being sent by
;; set_internal should not be there in this scenario.

**********************************************************************
;; I spent way to much time in gdb finding these last two paths.  I knew
;; they existed just by looking at the code but couldn't trigger them
;; in lisp for the longest time.


-----an example that looks like my testing------
;; As long as the uninterned alias is added after the watcher uninterned
symbols
;; work because the trapped write flag is copied and does not need to be
"harmonized".
;; I will cover this in more depth later.  This makes for clean and
repeatable tests.
;; I have an alternate version of most of the tests without uninterned
aliases.
(let* ((r1 nil)
       (a1 (gensym))
       (v1 (gensym))
       (f1 (lambda (&rest args) (push args r1))))
  (set v1 5)
  (add-variable-watcher v1 f1)
  (defvaralias a1 v1)
  (set-default a1 100)
  r1)
((g1 100 set nil) (g1 100 set nil))

[-- Attachment #1.2: Type: text/html, Size: 6723 bytes --]

[-- Attachment #2: 0002-Fix-multiple-watcher-calls-in-set_default_internal-b.patch --]
[-- Type: application/x-patch, Size: 3138 bytes --]

[-- Attachment #3: 0003-Fix-multiple-watcher-notification-in-set_internal-bu.patch --]
[-- Type: application/x-patch, Size: 5333 bytes --]

[-- Attachment #4: 0004-Add-ert-for-variable-watchers-bug-00001.patch --]
[-- Type: application/x-patch, Size: 2310 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-04-07  3:17 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-04  8:44 bug#70183: [PATCH] Fix + ert for multiple watcher notifications (2 of 9) Robert Burks
2024-04-06  7:29 ` Eli Zaretskii
2024-04-07  0:26   ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-07  3:17 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.