bug#70183: [PATCH] Fix + ert for multiple watcher notifications (2 of 9)

bug#70183: [PATCH] Fix + ert for multiple watcher notifications (2 of 9)

[Robert Burks]

[-- 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 --]

bug#70183: [PATCH] Fix + ert for multiple watcher notifications (2 of 9)

[Eli Zaretskii]

> From: Robert Burks <rburksdev@gmail.com>
> Date: Thu, 4 Apr 2024 04:44:41 -0400
> 
> 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))

Stefan, any comments about this?  I'm not sure what should watchers
do with aliases.  Also, please look at the patches (which seem to have
been updated in bug#70189, sigh...)

bug#70183: [PATCH] Fix + ert for multiple watcher notifications (2 of 9)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Stefan, any comments about this?  I'm not sure what should watchers
> do with aliases.

I think watchers should not be allowed on aliases, or should
automatically be redirected to the end of the aliasing chain.


        Stefan

bug#70183: [PATCH] Fix + ert for multiple watcher notifications (2 of 9)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>   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.

Maybe a better option here is to signal an error if passed a non-bare symbol.
At the very least it'd be useful to know how a sympos arrived here, in
order to decide whether we want to tolerate them here or whether we should
better fix the caller(s).

> Subject: [PATCH 02/31] Fix multiple watcher calls in set_default_internal
>  (bug#00001)
>
> * src/data.c (set_default_internal):
> Variable watchers will now execute one time if called using an alias.
> (bug#00001)
>
> ---
>  src/data.c | 43 ++++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/src/data.c b/src/data.c
> index c42424497ad..48da5cee429 100644
> --- a/src/data.c
> +++ b/src/data.c
> @@ -1986,31 +1986,21 @@ set_default_internal (Lisp_Object symbol, Lisp_Object value,
>  {
>    CHECK_SYMBOL (symbol);
>    struct Lisp_Symbol *sym = XSYMBOL (symbol);
> -  switch (sym->u.s.trapped_write)
> +  /* Update for aliasing and sanitize the input to ensure only a bare
> +     symbol is stored.  It may be accessed with positions disabled.  */
> + start:
> +  XSETSYMBOL (symbol, sym);
> +
> +  if (sym->u.s.trapped_write == SYMBOL_NOWRITE)
>      {
> -    case SYMBOL_NOWRITE:
>        if (NILP (Fkeywordp (symbol))
>            || !EQ (value, Fsymbol_value (symbol)))
>          xsignal1 (Qsetting_constant, symbol);
>        else
>          /* Allow setting keywords to their own value.  */
> -        return;
> -
> -    case SYMBOL_TRAPPED_WRITE:
> -      /* Don't notify here if we're going to call Fset anyway.  */
> -      if (sym->u.s.redirect != SYMBOL_PLAINVAL
> -          /* Setting due to thread switching doesn't count.  */
> -          && bindflag != SET_INTERNAL_THREAD_SWITCH)
> -        notify_variable_watchers (symbol, value, Qset_default, Qnil);
> -      break;
> -
> -    case SYMBOL_UNTRAPPED_WRITE:
> -      break;
> -
> -    default: emacs_abort ();
> +	return;
>      }

I think we should check `sym->u.s.trapped_write`
only after following the alias redirection.


        Stefan