Lisp watchpoints (Was: [Emacs-diffs] master 19e09cf: Ensure redisplay after evaluation)

Lisp watchpoints (Was: [Emacs-diffs] master 19e09cf: Ensure redisplay after evaluation)

[Noam Postavsky]

On Fri, Nov 13, 2015 at 8:58 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
> Actually, I think it's not that complicated:
> - Look at the definition of SYMBOL_CONSTANT_P.

There is a comment on Lisp_Symbol's `constant' field saying "If the
value is 3, then the var can be changed, but only by `defconst'".
However, I can't find any code that uses this value. Is it safe to
ignore?

> - Change its name to SYMBOL_SLOWWRITE_P.
> - Change the field it tests from being a boolean to being a 3-valued
>   thingy, with values "fullspeed", "hooked", and "readonly".

Would it be sensible to implement "readonly" as "hooked" + some hook
that signals error on write?

> Maybe it'd be nice to make sure that "defvaralias" could be
> re-implemented on top of those hooks, even though I don't think such
> a reimplementation is desirable at this point.

To do that we'd have to trap reads as well as writes, right? Which
could be useful for debugging too.

Re: Lisp watchpoints

[Stefan Monnier]

>> Actually, I think it's not that complicated:
>> - Look at the definition of SYMBOL_CONSTANT_P.
> There is a comment on Lisp_Symbol's `constant' field saying "If the
> value is 3, then the var can be changed, but only by `defconst'".
> However, I can't find any code that uses this value.

It's a mistake (I had such a thing working locally at some point but
I dropped it before I installed the code in the repository): I just
forgot to update the comment.

> Is it safe to ignore?

Yes.

>> - Change its name to SYMBOL_SLOWWRITE_P.
>> - Change the field it tests from being a boolean to being a 3-valued
>> thingy, with values "fullspeed", "hooked", and "readonly".
> Would it be sensible to implement "readonly" as "hooked" + some hook
> that signals error on write?

Could be, but I think I'd rather not take the risk that some advice
could turn nil's value to something else than nil.
IOW those vars currently marked as "read-only" should *really*
be kept read-only.

OTOH if we make sure that the "read-only" hook can't be
overridden/changed/skipped by Elisp code, it might be fine.

>> Maybe it'd be nice to make sure that "defvaralias" could be
>> re-implemented on top of those hooks, even though I don't think such
>> a reimplementation is desirable at this point.
> To do that we'd have to trap reads as well as writes, right?

I don't think so.  We just need to keep the two vars "synchronized" so
any assignment to one also changes the other.

> Which could be useful for debugging too.

I think a hook on variable reads is not a good idea.  Fundamentally the
issue is similar to the one for assignments, but the trade-offs work
differently:
- we currently don't have a CONSTANT_P check for reads, so adding
  a HOOKED_P check would incur an additional cost.
- When debugging it's much less frequent to need to catch reads than writes.
- The risk involved in running arbitrary Lisp code for a variable-read
  seems higher.

So the cost and risks are higher whereas the expected benefits are lower.
If/when dynamic-binding is the exception, maybe this could be
reconsidered but there'd also need to be a compelling argument in favor.


        Stefan

Re: Lisp watchpoints

[Noam Postavsky]

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

Okay, I think I have something mostly working now. The first patch
adds watchpoints, then the following 2 implement lisp debug on
variable set, and ensure redisplay, respectively. The debugger part
isn't quite right, I'm seeing it trigger a 2nd time after continuing.

On Sat, Nov 14, 2015 at 5:29 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
> OTOH if we make sure that the "read-only" hook can't be
> overridden/changed/skipped by Elisp code, it might be fine.

After thinking about it, I think the simplest thing is not to
implement read-only with watchpoints, but rather keep it as a separate
Lisp_Symbol setting.

> - The risk involved in running arbitrary Lisp code for a variable-read
>   seems higher.

We could make `add-variable-watcher' callable only from C, and
manipulate some internal hash table instead of the symbol's plist. Of
course this would make it less flexible, but it avoids the whole
problem of running arbitrary Lisp code for variable-write (and read,
though I don't plan to implement read watchpoints for now).

[-- Attachment #2: 0001-Add-lisp-watchpoints.patch --]
[-- Type: text/x-diff, Size: 17493 bytes --]

From 462e260b5c95bbee07d6f855e462333e987a5e9c Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Thu, 19 Nov 2015 19:50:06 -0500
Subject: [PATCH 1/3] Add lisp watchpoints

This allows to call a function whenever a symbol-value is changed.

* src/lisp.h (lisp_h_SYMBOL_TRAPPED_WRITE_P): Rename from
  lisp_h_SYMBOL_CONSTANT_P.
  (SYMBOL_TRAPPED_WRITE_P): Rename from SYMBOL_CONSTANT_P.
  (enum symbol_trapped_write): New enumeration.
  (struct Lisp_Symbol): Rename field constant to trapped_write.
  (make_symbol_constant): New functions.

* src/data.c (Fadd_variable_watcher, Fremove_variable_watcher):
  (set_symbol_trapped_write, reset_symbol_trapped_write):
  (notify_variable_watchers): New functions.

* src/data.c (Fmakunbound, set_internal, Fset_default): Call
  notify_variable_watchers for trapped symbols.

* src/data.c (syms_of_data):
* src/data.c (syms_of_data):
* src/font.c (syms_of_font):
* src/lread.c (intern_sym, init_obarray):
* src/buffer.c (syms_of_buffer): Use make_symbol_constant.

* src/alloc.c (init_symbol):
* src/bytecode.c (exec_byte_code): Use SYMBOL_TRAPPED_WRITE_P.
* src/data.c (Fmake_variable_buffer_local, Fmake_local_variable):
  (Fmake_variable_frame_local):
* src/eval.c (Fdefvaralias, specbind): Refer to
  Lisp_Symbol's trapped_write instead of constant.
---
 src/alloc.c    |   2 +-
 src/buffer.c   |   2 +-
 src/bytecode.c |   2 +-
 src/data.c     | 153 +++++++++++++++++++++++++++++++++++++++++++++++----------
 src/eval.c     |   6 +--
 src/font.c     |   6 +--
 src/lisp.h     |  35 ++++++++-----
 src/lread.c    |   6 +--
 8 files changed, 163 insertions(+), 49 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index bee7cd1..56c3a55 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -3368,7 +3368,7 @@ init_symbol (Lisp_Object val, Lisp_Object name)
   set_symbol_next (val, NULL);
   p->gcmarkbit = false;
   p->interned = SYMBOL_UNINTERNED;
-  p->constant = 0;
+  p->trapped_write = SYMBOL_UNTRAPPED_WRITE;
   p->declared_special = false;
   p->pinned = false;
 }
diff --git a/src/buffer.c b/src/buffer.c
index ab91aaa..11da887 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -5689,7 +5689,7 @@ syms_of_buffer (void)
 This variable is buffer-local but you cannot set it directly;
 use the function `set-buffer-multibyte' to change a buffer's representation.
 See also Info node `(elisp)Text Representations'.  */);
-  XSYMBOL (intern_c_string ("enable-multibyte-characters"))->constant = 1;
+  make_symbol_constant (intern_c_string ("enable-multibyte-characters"));
 
   DEFVAR_PER_BUFFER ("buffer-file-coding-system",
 		     &BVAR (current_buffer, buffer_file_coding_system), Qnil,
diff --git a/src/bytecode.c b/src/bytecode.c
index 864db1a..02edb44 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -799,7 +799,7 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
 	    if (SYMBOLP (sym)
 		&& !EQ (val, Qunbound)
 		&& !XSYMBOL (sym)->redirect
-		&& !SYMBOL_CONSTANT_P (sym))
+		&& !SYMBOL_TRAPPED_WRITE_P (sym))
 	      SET_SYMBOL_VAL (XSYMBOL (sym), val);
 	    else
 	      {
diff --git a/src/data.c b/src/data.c
index 5154604..7c6fc2c 100644
--- a/src/data.c
+++ b/src/data.c
@@ -34,6 +34,11 @@
 #include "frame.h"
 #include "keymap.h"
 
+static void notify_variable_watchers (Lisp_Object operation,
+                                    Lisp_Object where,
+                                    Lisp_Object symbol,
+                                    Lisp_Object newval);
+
 static void swap_in_symval_forwarding (struct Lisp_Symbol *,
 				       struct Lisp_Buffer_Local_Value *);
 
@@ -629,9 +634,17 @@ DEFUN ("makunbound", Fmakunbound, Smakunbound, 1, 1, 0,
   (register Lisp_Object symbol)
 {
   CHECK_SYMBOL (symbol);
-  if (SYMBOL_CONSTANT_P (symbol))
+  switch (XSYMBOL (symbol)->trapped_write) {
+  case SYMBOL_NOWRITE:
     xsignal1 (Qsetting_constant, symbol);
-  Fset (symbol, Qunbound);
+
+  case SYMBOL_TRAPPED_WRITE:
+    notify_variable_watchers (Qunbind, Qnil, symbol, Qnil);
+    /* fallthrough */
+  case SYMBOL_UNTRAPPED_WRITE:
+  default:
+    Fset (symbol, Qunbound);
+  }
   return symbol;
 }
 
@@ -1230,18 +1243,24 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
       return; */
 
   CHECK_SYMBOL (symbol);
-  if (SYMBOL_CONSTANT_P (symbol))
-    {
-      if (NILP (Fkeywordp (symbol))
-	  || !EQ (newval, Fsymbol_value (symbol)))
-	xsignal1 (Qsetting_constant, symbol);
-      else
-	/* Allow setting keywords to their own value.  */
-	return;
-    }
+  sym = XSYMBOL (symbol);
+  switch (sym->trapped_write) {
+  case SYMBOL_NOWRITE:
+    if (NILP (Fkeywordp (symbol))
+        || !EQ (newval, Fsymbol_value (symbol)))
+      xsignal1 (Qsetting_constant, symbol);
+    else
+      /* Allow setting keywords to their own value.  */
+      return;
+
+  case SYMBOL_TRAPPED_WRITE:
+    notify_variable_watchers (bindflag? Qlet : Qset, where, symbol, newval);
+    /* fallthrough */
+  case SYMBOL_UNTRAPPED_WRITE:
+  default: ;
+  }
 
   maybe_set_redisplay (symbol);
-  sym = XSYMBOL (symbol);
 
  start:
   switch (sym->redirect)
@@ -1365,6 +1384,72 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
     }
   return;
 }
+
+/* A (SYMBOL . (FUNCTIONS...)) alist */
+static Lisp_Object sym_watchers_table;
+
+static void
+set_symbol_trapped_write (Lisp_Object symbol, enum symbol_trapped_write trap)
+{
+  struct Lisp_Symbol* sym = XSYMBOL (symbol);
+  if (sym->trapped_write == SYMBOL_NOWRITE)
+    xsignal1 (Qtrapping_constant, symbol);
+  sym->trapped_write = trap;
+}
+
+static void
+reset_symbol_trapped_write (Lisp_Object symbol)
+{
+  set_symbol_trapped_write (symbol, SYMBOL_TRAPPED_WRITE);
+}
+
+DEFUN ("add-variable-watcher", Fadd_variable_watcher, Sadd_variable_watcher,
+       2, 2, 0,
+       doc: /* Cause WATCH-FUNCTION to be called when SYMBOL is set. */)
+  (Lisp_Object symbol, Lisp_Object watch_function)
+{
+  set_symbol_trapped_write (symbol, SYMBOL_TRAPPED_WRITE);
+
+  Lisp_Object watchers = Fget (symbol, Qwatchers);
+  Lisp_Object member = Fmember (watch_function, watchers);
+  if (NILP (member))
+    Fput (symbol, Qwatchers, Fcons (watch_function, watchers));
+  return Qnil;
+}
+
+DEFUN ("remove-variable-watcher", Fremove_variable_watcher, Sremove_variable_watcher,
+       2, 2, 0,
+       doc: /* Cause WATCH-FUNCTION to be called when SYMBOL is set. */)
+  (Lisp_Object symbol, Lisp_Object watch_function)
+{
+  Lisp_Object watchers = Fget (symbol, Qwatchers);
+  watchers = Fdelete (watch_function, watchers);
+  if (NILP (watchers))
+    set_symbol_trapped_write (symbol, SYMBOL_UNTRAPPED_WRITE);
+  Fput (symbol, Qwatchers, watchers);
+  return Qnil;
+}
+
+static void
+notify_variable_watchers (Lisp_Object operation,
+                          Lisp_Object where,
+                          Lisp_Object symbol,
+                          Lisp_Object newval)
+{
+  Lisp_Object watchers = Fget (symbol, Qwatchers);
+
+  set_symbol_trapped_write (symbol, SYMBOL_UNTRAPPED_WRITE); /* avoid recursion */
+  ptrdiff_t count = SPECPDL_INDEX ();
+  record_unwind_protect (&reset_symbol_trapped_write, symbol);
+
+  while (!NILP (watchers)) {
+    CALLN (Ffuncall, XCAR (watchers), operation, where, symbol, newval);
+    watchers = XCDR (watchers);
+  }
+
+  unbind_to (count, Qnil);
+}
+
 \f
 /* Access or set a buffer-local symbol's default value.  */
 
@@ -1451,16 +1536,22 @@ DEFUN ("set-default", Fset_default, Sset_default, 2, 2, 0,
   struct Lisp_Symbol *sym;
 
   CHECK_SYMBOL (symbol);
-  if (SYMBOL_CONSTANT_P (symbol))
-    {
-      if (NILP (Fkeywordp (symbol))
-	  || !EQ (value, Fdefault_value (symbol)))
-	xsignal1 (Qsetting_constant, symbol);
-      else
-	/* Allow setting keywords to their own value.  */
-	return value;
-    }
   sym = XSYMBOL (symbol);
+  switch (sym->trapped_write) {
+  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 value;
+
+  case SYMBOL_TRAPPED_WRITE:
+    notify_variable_watchers (Qset_default, Qnil, symbol, value);
+    /* fallthrough */
+  case SYMBOL_UNTRAPPED_WRITE:
+  default: ;
+  }
 
  start:
   switch (sym->redirect)
@@ -1631,7 +1722,7 @@ DEFUN ("make-variable-buffer-local", Fmake_variable_buffer_local,
     default: emacs_abort ();
     }
 
-  if (sym->constant)
+  if (sym->trapped_write == SYMBOL_NOWRITE)
     error ("Symbol %s may not be buffer-local", SDATA (SYMBOL_NAME (variable)));
 
   if (!blv)
@@ -1706,7 +1797,7 @@ DEFUN ("make-local-variable", Fmake_local_variable, Smake_local_variable,
     default: emacs_abort ();
     }
 
-  if (sym->constant)
+  if (sym->trapped_write == SYMBOL_NOWRITE)
     error ("Symbol %s may not be buffer-local",
 	   SDATA (SYMBOL_NAME (variable)));
 
@@ -1900,7 +1991,7 @@ DEFUN ("make-variable-frame-local", Fmake_variable_frame_local, Smake_variable_f
     default: emacs_abort ();
     }
 
-  if (sym->constant)
+  if (sym->trapped_write == SYMBOL_NOWRITE)
     error ("Symbol %s may not be frame-local", SDATA (SYMBOL_NAME (variable)));
 
   blv = make_blv (sym, forwarded, valcontents);
@@ -3451,6 +3542,7 @@ syms_of_data (void)
   DEFSYM (Qcyclic_variable_indirection, "cyclic-variable-indirection");
   DEFSYM (Qvoid_variable, "void-variable");
   DEFSYM (Qsetting_constant, "setting-constant");
+  DEFSYM (Qtrapping_constant, "trapping-constant");
   DEFSYM (Qinvalid_read_syntax, "invalid-read-syntax");
 
   DEFSYM (Qinvalid_function, "invalid-function");
@@ -3526,6 +3618,8 @@ syms_of_data (void)
   PUT_ERROR (Qvoid_variable, error_tail, "Symbol's value as variable is void");
   PUT_ERROR (Qsetting_constant, error_tail,
 	     "Attempt to set a constant symbol");
+  PUT_ERROR (Qtrapping_constant, error_tail,
+             "Attempt to trap writes to a constant symbol");
   PUT_ERROR (Qinvalid_read_syntax, error_tail, "Invalid read syntax");
   PUT_ERROR (Qinvalid_function, error_tail, "Invalid function");
   PUT_ERROR (Qwrong_number_of_arguments, error_tail,
@@ -3698,10 +3792,17 @@ syms_of_data (void)
   DEFVAR_LISP ("most-positive-fixnum", Vmost_positive_fixnum,
 	       doc: /* The largest value that is representable in a Lisp integer.  */);
   Vmost_positive_fixnum = make_number (MOST_POSITIVE_FIXNUM);
-  XSYMBOL (intern_c_string ("most-positive-fixnum"))->constant = 1;
+  make_symbol_constant (intern_c_string ("most-positive-fixnum"));
 
   DEFVAR_LISP ("most-negative-fixnum", Vmost_negative_fixnum,
 	       doc: /* The smallest value that is representable in a Lisp integer.  */);
   Vmost_negative_fixnum = make_number (MOST_NEGATIVE_FIXNUM);
-  XSYMBOL (intern_c_string ("most-negative-fixnum"))->constant = 1;
+  make_symbol_constant (intern_c_string ("most-negative-fixnum"));
+
+  DEFSYM (Qwatchers, "watchers");
+  DEFSYM (Qunbind, "unbind");
+  DEFSYM (Qset, "set");
+  DEFSYM (Qset_default, "Qset_default");
+  defsubr (&Sadd_variable_watcher);
+  defsubr (&Sremove_variable_watcher);
 }
diff --git a/src/eval.c b/src/eval.c
index ac98ca1..3d17825 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -586,7 +586,7 @@ DEFUN ("defvaralias", Fdefvaralias, Sdefvaralias, 2, 3, 0,
 
   sym = XSYMBOL (new_alias);
 
-  if (sym->constant)
+  if (sym->trapped_write == SYMBOL_NOWRITE)
     /* Not sure why, but why not?  */
     error ("Cannot make a constant an alias");
 
@@ -623,7 +623,7 @@ DEFUN ("defvaralias", Fdefvaralias, Sdefvaralias, 2, 3, 0,
   XSYMBOL (base_variable)->declared_special = 1;
   sym->redirect = SYMBOL_VARALIAS;
   SET_SYMBOL_ALIAS (sym, XSYMBOL (base_variable));
-  sym->constant = SYMBOL_CONSTANT_P (base_variable);
+  sym->trapped_write = XSYMBOL (base_variable)->trapped_write;
   LOADHIST_ATTACH (new_alias);
   /* Even if docstring is nil: remove old docstring.  */
   Fput (new_alias, Qvariable_documentation, docstring);
@@ -2972,7 +2972,7 @@ specbind (Lisp_Object symbol, Lisp_Object value)
       specpdl_ptr->let.symbol = symbol;
       specpdl_ptr->let.old_value = SYMBOL_VAL (sym);
       grow_specpdl ();
-      if (!sym->constant)
+      if (!sym->trapped_write)
 	SET_SYMBOL_VAL (sym, value);
       else
 	set_internal (symbol, value, Qnil, 1);
diff --git a/src/font.c b/src/font.c
index 016b7e0..509d3cc 100644
--- a/src/font.c
+++ b/src/font.c
@@ -5388,19 +5388,19 @@ syms_of_font (void)
     [NUMERIC-VALUE SYMBOLIC-NAME ALIAS-NAME ...]
 NUMERIC-VALUE is an integer, and SYMBOLIC-NAME and ALIAS-NAME are symbols. */);
   Vfont_weight_table = BUILD_STYLE_TABLE (weight_table);
-  XSYMBOL (intern_c_string ("font-weight-table"))->constant = 1;
+  make_symbol_constant (intern_c_string ("font-weight-table"));
 
   DEFVAR_LISP_NOPRO ("font-slant-table", Vfont_slant_table,
 	       doc: /*  Vector of font slant symbols vs the corresponding numeric values.
 See `font-weight-table' for the format of the vector. */);
   Vfont_slant_table = BUILD_STYLE_TABLE (slant_table);
-  XSYMBOL (intern_c_string ("font-slant-table"))->constant = 1;
+  make_symbol_constant (intern_c_string ("font-slant-table"));
 
   DEFVAR_LISP_NOPRO ("font-width-table", Vfont_width_table,
 	       doc: /*  Alist of font width symbols vs the corresponding numeric values.
 See `font-weight-table' for the format of the vector. */);
   Vfont_width_table = BUILD_STYLE_TABLE (width_table);
-  XSYMBOL (intern_c_string ("font-width-table"))->constant = 1;
+  make_symbol_constant (intern_c_string ("font-width-table"));
 
   staticpro (&font_style_table);
   font_style_table = make_uninit_vector (3);
diff --git a/src/lisp.h b/src/lisp.h
index 3efa492..b8431ee 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -337,7 +337,7 @@ error !;
 #define lisp_h_NILP(x) EQ (x, Qnil)
 #define lisp_h_SET_SYMBOL_VAL(sym, v) \
    (eassert ((sym)->redirect == SYMBOL_PLAINVAL), (sym)->val.value = (v))
-#define lisp_h_SYMBOL_CONSTANT_P(sym) (XSYMBOL (sym)->constant)
+#define lisp_h_SYMBOL_TRAPPED_WRITE_P(sym) (XSYMBOL (sym)->trapped_write)
 #define lisp_h_SYMBOL_VAL(sym) \
    (eassert ((sym)->redirect == SYMBOL_PLAINVAL), (sym)->val.value)
 #define lisp_h_SYMBOLP(x) (XTYPE (x) == Lisp_Symbol)
@@ -383,7 +383,7 @@ error !;
 # define MISCP(x) lisp_h_MISCP (x)
 # define NILP(x) lisp_h_NILP (x)
 # define SET_SYMBOL_VAL(sym, v) lisp_h_SET_SYMBOL_VAL (sym, v)
-# define SYMBOL_CONSTANT_P(sym) lisp_h_SYMBOL_CONSTANT_P (sym)
+# define SYMBOL_TRAPPED_WRITE_P(sym) lisp_h_SYMBOL_TRAPPED_WRITE_P (sym)
 # define SYMBOL_VAL(sym) lisp_h_SYMBOL_VAL (sym)
 # define SYMBOLP(x) lisp_h_SYMBOLP (x)
 # define VECTORLIKEP(x) lisp_h_VECTORLIKEP (x)
@@ -630,6 +630,13 @@ enum symbol_redirect
   SYMBOL_FORWARDED = 3
 };
 
+enum symbol_trapped_write
+{
+  SYMBOL_UNTRAPPED_WRITE = 0,
+  SYMBOL_NOWRITE = 1,
+  SYMBOL_TRAPPED_WRITE = 2
+};
+
 struct Lisp_Symbol
 {
   bool_bf gcmarkbit : 1;
@@ -641,10 +648,10 @@ struct Lisp_Symbol
      3 : it's a forwarding variable, the value is in `forward'.  */
   ENUM_BF (symbol_redirect) redirect : 3;
 
-  /* Non-zero means symbol is constant, i.e. changing its value
-     should signal an error.  If the value is 3, then the var
-     can be changed, but only by `defconst'.  */
-  unsigned constant : 2;
+  /* 0 : normal case, just set the value
+     1 : constant, cannot set, e.g. nil, t, :keywords.
+     2 : trap the write, call watcher functions.  */
+  ENUM_BF (symbol_trapped_write) trapped_write : 2;
 
   /* Interned state of the symbol.  This is an enumerator from
      enum symbol_interned.  */
@@ -1829,14 +1836,14 @@ SYMBOL_INTERNED_IN_INITIAL_OBARRAY_P (Lisp_Object sym)
   return XSYMBOL (sym)->interned == SYMBOL_INTERNED_IN_INITIAL_OBARRAY;
 }
 
-/* Value is non-zero if symbol is considered a constant, i.e. its
-   value cannot be changed (there is an exception for keyword symbols,
-   whose value can be set to the keyword symbol itself).  */
+/* Value is non-zero if symbol cannot be changed through a simple set,
+ i.e. it's a constant (e.g. nil, t, :keywords), or it has some
+ watching functions.  */
 
 INLINE int
-(SYMBOL_CONSTANT_P) (Lisp_Object sym)
+(SYMBOL_TRAPPED_WRITE_P) (Lisp_Object sym)
 {
-  return lisp_h_SYMBOL_CONSTANT_P (sym);
+  return lisp_h_SYMBOL_TRAPPED_WRITE_P (sym);
 }
 
 /* Placeholder for make-docfile to process.  The actual symbol
@@ -3256,6 +3263,12 @@ set_symbol_next (Lisp_Object sym, struct Lisp_Symbol *next)
   XSYMBOL (sym)->next = next;
 }
 
+INLINE void
+make_symbol_constant (Lisp_Object sym)
+{
+  XSYMBOL (sym)->trapped_write = SYMBOL_NOWRITE;
+}
+
 /* Buffer-local (also frame-local) variable access functions.  */
 
 INLINE int
diff --git a/src/lread.c b/src/lread.c
index c4456f3..0852215 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -3721,7 +3721,7 @@ intern_sym (Lisp_Object sym, Lisp_Object obarray, Lisp_Object index)
 
   if (SREF (SYMBOL_NAME (sym), 0) == ':' && EQ (obarray, initial_obarray))
     {
-      XSYMBOL (sym)->constant = 1;
+      make_symbol_constant (sym);
       XSYMBOL (sym)->redirect = SYMBOL_PLAINVAL;
       SET_SYMBOL_VAL (XSYMBOL (sym), sym);
     }
@@ -4010,12 +4010,12 @@ init_obarray (void)
 
   DEFSYM (Qnil, "nil");
   SET_SYMBOL_VAL (XSYMBOL (Qnil), Qnil);
-  XSYMBOL (Qnil)->constant = 1;
+  make_symbol_constant (Qnil);
   XSYMBOL (Qnil)->declared_special = true;
 
   DEFSYM (Qt, "t");
   SET_SYMBOL_VAL (XSYMBOL (Qt), Qt);
-  XSYMBOL (Qt)->constant = 1;
+  make_symbol_constant (Qt);
   XSYMBOL (Qt)->declared_special = true;
 
   /* Qt is correct even if CANNOT_DUMP.  loadup.el will set to nil at end.  */
-- 
2.6.2


[-- Attachment #3: 0002-Add-function-to-trigger-debugger-on-variable-write.patch --]
[-- Type: text/x-diff, Size: 2289 bytes --]

From 8655f2690f589117ea8677f4e1dfb1bef78ac75b Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 21 Nov 2015 16:03:06 -0500
Subject: [PATCH 2/3] Add function to trigger debugger on variable write

* lisp/emacs-lisp/debug.el (debug-on-set):
(debug--variable-list):
(cancel-debug-on-set): New functions.
---
 lisp/emacs-lisp/debug.el | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/lisp/emacs-lisp/debug.el b/lisp/emacs-lisp/debug.el
index 0e307fa..a9f6978 100644
--- a/lisp/emacs-lisp/debug.el
+++ b/lisp/emacs-lisp/debug.el
@@ -765,6 +765,45 @@ (defun debug--implement-debug-on-entry (&rest _ignore)
       (funcall debugger 'debug))))
 
 ;;;###autoload
+(defun debug-on-set (variable)
+  (interactive
+   (let* ((var-at-point (variable-at-point))
+          (var (and (symbolp var-at-point) var-at-point))
+          (val (completing-read
+                (concat "Debug when setting variable"
+                        (if var (format " (default %s): " var) ": "))
+                obarray #'boundp
+                t nil nil (and var (symbol-name var)))))
+     (list (if (equal val "") var (intern val)))))
+  (add-variable-watcher variable #'debug--implement-debug-on-entry))
+
+
+(defun debug--variable-list ()
+  "List of variables currently set for debug on set."
+  (let ((vars '()))
+    (mapatoms
+     (lambda (s)
+       (when (and (boundp s) (memq #'debug--implement-debug-on-entry
+                                   (get s 'watchers)))
+         (push s vars))))
+    vars))
+
+;;;###autoload
+(defun cancel-debug-on-set (&optional variable)
+  (interactive
+   (list (let ((name
+                (completing-read
+                 "Cancel debug on set for variable (default all variables): "
+                 (mapcar #'symbol-name (debug--variable-list)) nil t)))
+           (when name
+             (unless (string= name "")
+               (intern name))))))
+  (if variable
+      (remove-variable-watcher variable #'debug--implement-debug-on-entry)
+    (message "Canceling debug-on-set for all variables")
+    (mapc #'cancel-debug-on-set (debug--variable-list))))
+
+;;;###autoload
 (defun debug-on-entry (function)
   "Request FUNCTION to invoke debugger each time it is called.
 
-- 
2.6.2


[-- Attachment #4: 0003-Ensure-redisplay-using-variable-watchers.patch --]
[-- Type: text/x-diff, Size: 3363 bytes --]

From f3e82ab6d29dc0b673704dafd7e8c8c636bf0407 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 21 Nov 2015 17:02:42 -0500
Subject: [PATCH 3/3] Ensure redisplay using variable watchers

Instead of looking up the variable name in redisplay--variables when
setting.

* lisp/frame.el: Replace redisplay--variables with add-variable-watcher
  calls.
* src/data.c (set_internal): Remove maybe_set_redisplay call.
* src/xdisp.c (Fset_redisplay): Rename from maybe_set_redisplay, set the
  redisplay flag unconditionally.
---
 lisp/frame.el |  3 +--
 src/data.c    |  2 --
 src/window.h  |  1 -
 src/xdisp.c   | 19 ++++++++-----------
 4 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/lisp/frame.el b/lisp/frame.el
index f024065..95c514f 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -2232,9 +2232,8 @@ (make-obsolete-variable
  'window-system-version "it does not give useful information." "24.3")
 
 ;; Variables which should trigger redisplay of the current buffer.
-(setq redisplay--variables (make-hash-table :test 'eq :size 10))
 (mapc (lambda (var)
-        (puthash var 1 redisplay--variables))
+        (add-variable-watcher var #'set-redisplay))
       '(line-spacing
         overline-margin
         line-prefix
diff --git a/src/data.c b/src/data.c
index 7c6fc2c..a411cf5 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1260,8 +1260,6 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
   default: ;
   }
 
-  maybe_set_redisplay (symbol);
-
  start:
   switch (sym->redirect)
     {
diff --git a/src/window.h b/src/window.h
index 135f5de..eaff57e 100644
--- a/src/window.h
+++ b/src/window.h
@@ -1056,7 +1056,6 @@ extern void wset_redisplay (struct window *w);
 extern void fset_redisplay (struct frame *f);
 extern void bset_redisplay (struct buffer *b);
 extern void bset_update_mode_line (struct buffer *b);
-extern void maybe_set_redisplay (Lisp_Object);
 /* Call this to tell redisplay to look for other windows than selected-window
    that need to be redisplayed.  Calling one of the *set_redisplay functions
    above already does it, so it's only needed in unusual cases.  */
diff --git a/src/xdisp.c b/src/xdisp.c
index 30dfac5..705f352 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -620,15 +620,14 @@ bset_update_mode_line (struct buffer *b)
   b->text->redisplay = true;
 }
 
-void
-maybe_set_redisplay (Lisp_Object symbol)
+DEFUN ("set-redisplay", Fset_redisplay, Sset_redisplay,
+       0, MANY, 0,
+       doc: /* Cause a full redisplay to happen.
+       usage: () */)
+  (ptrdiff_t nargs, Lisp_Object* rest)
 {
-  if (HASH_TABLE_P (Vredisplay__variables)
-      && hash_lookup (XHASH_TABLE (Vredisplay__variables), symbol, NULL) >= 0)
-    {
-      bset_update_mode_line (current_buffer);
-      current_buffer->prevent_redisplay_optimizations_p = true;
-    }
+  bset_update_mode_line (current_buffer);
+  current_buffer->prevent_redisplay_optimizations_p = true;
 }
 
 #ifdef GLYPH_DEBUG
@@ -31478,9 +31477,7 @@ or t (meaning all windows).  */);
 	       doc: /*  */);
   Vredisplay__mode_lines_cause = Fmake_hash_table (0, NULL);
 
-  DEFVAR_LISP ("redisplay--variables", Vredisplay__variables,
-     doc: /* A hash-table of variables changing which triggers a thorough redisplay.  */);
-  Vredisplay__variables = Qnil;
+  defsubr (&Sset_redisplay);
 }
 
 
-- 
2.6.2

Re: Lisp watchpoints

[Eli Zaretskii]

> Date: Sun, 22 Nov 2015 15:13:47 -0500
> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Cc: Eli Zaretskii <eliz@gnu.org>, John Wiegley <jwiegley@gmail.com>, emacs-devel@gnu.org
> 
> Okay, I think I have something mostly working now. The first patch
> adds watchpoints, then the following 2 implement lisp debug on
> variable set, and ensure redisplay, respectively. The debugger part
> isn't quite right, I'm seeing it trigger a 2nd time after continuing.

Thanks.

Is it possible to have watchers that don't require a Funcall, or avoid
exposing the watcher function to Lisp to begin with?  Funcall can GC,
so the usability of such watchers for internal purposes might be
limited in some situations.

How about allowing special, say, integer values for a watcher, which
will then cause a call to a C function from a table indexed by the
watcher value?  Or something else to that effect.

Another comment is: do we care about scalability of property lists for
storing watchers?

The debug command might be more mnemonic (at least for me ;-) if it
were called 'debug-watch', or maybe 'debug-set-watchpoint'.  A similar
command for Edebug would also be nice.

Thanks again for working on this.

P.S. We should decide whether we ad this to the emacs-25 branch or to
master.

Re: Lisp watchpoints

[Stefan Monnier]

> Another comment is: do we care about scalability of property lists for
> storing watchers?

As long as we only look up the property list for those few symbols that
have the "hooked" bit set, I think the slow down shouldn't be worrisome.
Also property lists are usually reasonably short (not much than 10
elements on those lists), so looking them up should be reasonably quick.

[ Reminds me: did I understand correctly that you found gethash to be
  faster than Fmemq on redisplay--variables?  For such a small number
  of elements, I find it really surprising, since I thought the
  generally expected "threshold" where gethash is faster than assq was
  somewhere in the order of a hundred elements.  ]


        Stefan

Re: Lisp watchpoints

[Noam Postavsky]

On Sun, Nov 22, 2015 at 3:41 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> Is it possible to have watchers that don't require a Funcall, or avoid
> exposing the watcher function to Lisp to begin with?  Funcall can GC,
> so the usability of such watchers for internal purposes might be
> limited in some situations.

Yes, avoiding exposure to Lisp is what I had in mind when I said

    We could make `add-variable-watcher' callable only from C, and
    manipulate some internal hash table instead of the symbol's plist. Of
    course this would make it less flexible, but it avoids the whole
    problem of running arbitrary Lisp code for variable-write (and read,
    though I don't plan to implement read watchpoints for now).

But I wasn't aware of the Funcall GC thing.

>
> How about allowing special, say, integer values for a watcher, which
> will then cause a call to a C function from a table indexed by the
> watcher value?  Or something else to that effect.

That could work.

>
> Another comment is: do we care about scalability of property lists for
> storing watchers?

I actually tried to use a hashtable first but it didn't work so I
switched to property lists because it's simpler. (I think my problem
with the hashtable was that I missed the note about staticpro).

>
> The debug command might be more mnemonic (at least for me ;-) if it
> were called 'debug-watch', or maybe 'debug-set-watchpoint'.  A similar
> command for Edebug would also be nice.

Sure, I have no particular attachment to the current names. When I
figure out how to do plain debug properly, I'll look at an Edebug
version. :)

> P.S. We should decide whether we ad this to the emacs-25 branch or to
> master.

Right, so I think the arguments for emacs-25 despite feature freeze would be

- watchpoints would be useful for catching all those bugs we have to
fix before release :)
- fixing a performance regression (I guess? I haven't made any
measurements though)

Re: Lisp watchpoints

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Noam Postavsky <npostavs@users.sourceforge.net>, jwiegley@gmail.com,
>         emacs-devel@gnu.org
> Date: Sun, 22 Nov 2015 16:25:40 -0500
> 
> [ Reminds me: did I understand correctly that you found gethash to be
>   faster than Fmemq on redisplay--variables?

Yes.

>   For such a small number of elements, I find it really surprising,
>   since I thought the generally expected "threshold" where gethash
>   is faster than assq was somewhere in the order of a hundred
>   elements.  ]

That's not what I saw.

Re: Lisp watchpoints

[Noam Postavsky]

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

Here is v2.

On 11/22/15, Eli Zaretskii <eliz@gnu.org> wrote:
> How about allowing special, say, integer values for a watcher, which
> will then cause a call to a C function from a table indexed by the
> watcher value?  Or something else to that effect.

I used this for the ensure redisplay.

> The debug command might be more mnemonic (at least for me ;-) if it
> were called 'debug-watch', or maybe 'debug-set-watchpoint'.  A similar
> command for Edebug would also be nice.

I changed to 'debug-watch'.


On Sun, 22 Nov 2015 21:31:53 -0500, I wrote:
> When I figure out how to do plain debug properly

Enlightenment arrived after reading (elisp) "Invoking the Debugger".
Though I'm not entirely if I got the point manipulation in
`debug-setup-buffer' right.

> I'll look at an Edebug version.

Um, actually, I'm not sure how to do the Edebug version at all,
wouldn't it have to magically instrument all the possible functions
that change the variable?

[-- Attachment #2: v2-0001-Add-lisp-watchpoints.patch --]
[-- Type: text/x-diff, Size: 18033 bytes --]

From c1e2e14097f4488384ea8ea3cab3cd51c41947eb Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Thu, 19 Nov 2015 19:50:06 -0500
Subject: [PATCH v2 1/3] Add lisp watchpoints

This allows to call a function whenever a symbol-value is changed.

* src/lisp.h (lisp_h_SYMBOL_TRAPPED_WRITE_P): Rename from
  lisp_h_SYMBOL_CONSTANT_P.
  (SYMBOL_TRAPPED_WRITE_P): Rename from SYMBOL_CONSTANT_P.
  (enum symbol_trapped_write): New enumeration.
  (struct Lisp_Symbol): Rename field constant to trapped_write.
  (make_symbol_constant): New function.

* src/data.c (Fadd_variable_watcher, Fremove_variable_watcher):
  (set_symbol_trapped_write, reset_symbol_trapped_write):
  (notify_variable_watchers): New functions.
  (watcher_table): New variable.

* src/data.c (Fmakunbound, set_internal, Fset_default): Call
  notify_variable_watchers for trapped symbols.

* src/data.c (syms_of_data):
* src/data.c (syms_of_data):
* src/font.c (syms_of_font):
* src/lread.c (intern_sym, init_obarray):
* src/buffer.c (syms_of_buffer): Use make_symbol_constant.

* src/alloc.c (init_symbol):
* src/bytecode.c (exec_byte_code): Use SYMBOL_TRAPPED_WRITE_P.
* src/data.c (Fmake_variable_buffer_local, Fmake_local_variable):
  (Fmake_variable_frame_local):
* src/eval.c (Fdefvaralias, specbind): Refer to
  Lisp_Symbol's trapped_write instead of constant.
---
 src/alloc.c    |   2 +-
 src/buffer.c   |   2 +-
 src/bytecode.c |   2 +-
 src/data.c     | 161 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 src/eval.c     |   6 +--
 src/font.c     |   6 +--
 src/lisp.h     |  35 +++++++++----
 src/lread.c    |   6 +--
 8 files changed, 177 insertions(+), 43 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index bee7cd1..56c3a55 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -3368,7 +3368,7 @@ init_symbol (Lisp_Object val, Lisp_Object name)
   set_symbol_next (val, NULL);
   p->gcmarkbit = false;
   p->interned = SYMBOL_UNINTERNED;
-  p->constant = 0;
+  p->trapped_write = SYMBOL_UNTRAPPED_WRITE;
   p->declared_special = false;
   p->pinned = false;
 }
diff --git a/src/buffer.c b/src/buffer.c
index ab91aaa..11da887 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -5689,7 +5689,7 @@ syms_of_buffer (void)
 This variable is buffer-local but you cannot set it directly;
 use the function `set-buffer-multibyte' to change a buffer's representation.
 See also Info node `(elisp)Text Representations'.  */);
-  XSYMBOL (intern_c_string ("enable-multibyte-characters"))->constant = 1;
+  make_symbol_constant (intern_c_string ("enable-multibyte-characters"));
 
   DEFVAR_PER_BUFFER ("buffer-file-coding-system",
 		     &BVAR (current_buffer, buffer_file_coding_system), Qnil,
diff --git a/src/bytecode.c b/src/bytecode.c
index 864db1a..02edb44 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -799,7 +799,7 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
 	    if (SYMBOLP (sym)
 		&& !EQ (val, Qunbound)
 		&& !XSYMBOL (sym)->redirect
-		&& !SYMBOL_CONSTANT_P (sym))
+		&& !SYMBOL_TRAPPED_WRITE_P (sym))
 	      SET_SYMBOL_VAL (XSYMBOL (sym), val);
 	    else
 	      {
diff --git a/src/data.c b/src/data.c
index 5154604..1576280 100644
--- a/src/data.c
+++ b/src/data.c
@@ -34,6 +34,11 @@
 #include "frame.h"
 #include "keymap.h"
 
+static void notify_variable_watchers (Lisp_Object symbol,
+                                      Lisp_Object newval,
+                                      Lisp_Object operation,
+                                      Lisp_Object where);
+
 static void swap_in_symval_forwarding (struct Lisp_Symbol *,
 				       struct Lisp_Buffer_Local_Value *);
 
@@ -629,9 +634,18 @@ DEFUN ("makunbound", Fmakunbound, Smakunbound, 1, 1, 0,
   (register Lisp_Object symbol)
 {
   CHECK_SYMBOL (symbol);
-  if (SYMBOL_CONSTANT_P (symbol))
-    xsignal1 (Qsetting_constant, symbol);
-  Fset (symbol, Qunbound);
+  switch (XSYMBOL (symbol)->trapped_write)
+    {
+    case SYMBOL_NOWRITE:
+      xsignal1 (Qsetting_constant, symbol);
+
+    case SYMBOL_TRAPPED_WRITE:
+      notify_variable_watchers (symbol, Qnil, Qunbind, Qnil);
+      /* fallthrough */
+    case SYMBOL_UNTRAPPED_WRITE:
+    default:
+      Fset (symbol, Qunbound);
+    }
   return symbol;
 }
 
@@ -1230,18 +1244,25 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
       return; */
 
   CHECK_SYMBOL (symbol);
-  if (SYMBOL_CONSTANT_P (symbol))
+  sym = XSYMBOL (symbol);
+  switch (sym->trapped_write)
     {
+    case SYMBOL_NOWRITE:
       if (NILP (Fkeywordp (symbol))
-	  || !EQ (newval, Fsymbol_value (symbol)))
-	xsignal1 (Qsetting_constant, symbol);
+          || !EQ (newval, Fsymbol_value (symbol)))
+        xsignal1 (Qsetting_constant, symbol);
       else
-	/* Allow setting keywords to their own value.  */
-	return;
+        /* Allow setting keywords to their own value.  */
+        return;
+
+    case SYMBOL_TRAPPED_WRITE:
+      notify_variable_watchers (symbol, newval, bindflag? Qlet : Qset, where);
+      /* fallthrough */
+    case SYMBOL_UNTRAPPED_WRITE:
+    default: ;
     }
 
   maybe_set_redisplay (symbol);
-  sym = XSYMBOL (symbol);
 
  start:
   switch (sym->redirect)
@@ -1365,6 +1386,89 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
     }
   return;
 }
+
+/* A (SYMBOL . (FUNCTIONS...)) alist */
+static Lisp_Object sym_watchers_table;
+
+static void
+set_symbol_trapped_write (Lisp_Object symbol, enum symbol_trapped_write trap)
+{
+  struct Lisp_Symbol* sym = XSYMBOL (symbol);
+  if (sym->trapped_write == SYMBOL_NOWRITE)
+    xsignal1 (Qtrapping_constant, symbol);
+  sym->trapped_write = trap;
+}
+
+static void
+reset_symbol_trapped_write (Lisp_Object symbol)
+{
+  set_symbol_trapped_write (symbol, SYMBOL_TRAPPED_WRITE);
+}
+
+DEFUN ("add-variable-watcher", Fadd_variable_watcher, Sadd_variable_watcher,
+       2, 2, 0,
+       doc: /* Cause WATCH-FUNCTION to be called when SYMBOL is set. */)
+  (Lisp_Object symbol, Lisp_Object watch_function)
+{
+  set_symbol_trapped_write (symbol, SYMBOL_TRAPPED_WRITE);
+
+  Lisp_Object watchers = Fget (symbol, Qwatchers);
+  Lisp_Object member = Fmember (watch_function, watchers);
+  if (NILP (member))
+    Fput (symbol, Qwatchers, Fcons (watch_function, watchers));
+  return Qnil;
+}
+
+DEFUN ("remove-variable-watcher", Fremove_variable_watcher, Sremove_variable_watcher,
+       2, 2, 0,
+       doc: /* Cause WATCH-FUNCTION to be called when SYMBOL is set. */)
+  (Lisp_Object symbol, Lisp_Object watch_function)
+{
+  Lisp_Object watchers = Fget (symbol, Qwatchers);
+  watchers = Fdelete (watch_function, watchers);
+  if (NILP (watchers))
+    set_symbol_trapped_write (symbol, SYMBOL_UNTRAPPED_WRITE);
+  Fput (symbol, Qwatchers, watchers);
+  return Qnil;
+}
+
+typedef void (*WATCHER_FUNCTION) (Lisp_Object, Lisp_Object, Lisp_Object, Lisp_Object);
+static const WATCHER_FUNCTION watcher_table[] =
+  {
+  };
+enum
+  {
+  };
+
+static void
+notify_variable_watchers (Lisp_Object symbol,
+                          Lisp_Object newval,
+                          Lisp_Object operation,
+                          Lisp_Object where)
+{
+  Lisp_Object watchers = Fget (symbol, Qwatchers);
+
+  set_symbol_trapped_write (symbol, SYMBOL_UNTRAPPED_WRITE); /* avoid recursion */
+  ptrdiff_t count = SPECPDL_INDEX ();
+  record_unwind_protect (&reset_symbol_trapped_write, symbol);
+
+  while (!NILP (watchers))
+    {
+      Lisp_Object watcher = XCAR (watchers);
+      if (INTEGERP (watcher))
+        {
+          EMACS_INT wnum = XINT (watcher);
+          if (wnum < ARRAYELTS (watcher_table))
+            watcher_table[wnum] (operation, where, symbol, newval);
+        }
+      else if (FUNCTIONP (watcher))
+        CALLN (Ffuncall, watcher, operation, where, symbol, newval);
+      watchers = XCDR (watchers);
+    }
+
+  unbind_to (count, Qnil);
+}
+
 \f
 /* Access or set a buffer-local symbol's default value.  */
 
@@ -1451,16 +1555,23 @@ DEFUN ("set-default", Fset_default, Sset_default, 2, 2, 0,
   struct Lisp_Symbol *sym;
 
   CHECK_SYMBOL (symbol);
-  if (SYMBOL_CONSTANT_P (symbol))
+  sym = XSYMBOL (symbol);
+  switch (sym->trapped_write)
     {
+    case SYMBOL_NOWRITE:
       if (NILP (Fkeywordp (symbol))
-	  || !EQ (value, Fdefault_value (symbol)))
-	xsignal1 (Qsetting_constant, symbol);
+          || !EQ (value, Fsymbol_value (symbol)))
+        xsignal1 (Qsetting_constant, symbol);
       else
-	/* Allow setting keywords to their own value.  */
-	return value;
+        /* Allow setting keywords to their own value.  */
+        return value;
+
+    case SYMBOL_TRAPPED_WRITE:
+      notify_variable_watchers (symbol, value, Qset_default, Qnil);
+      /* fallthrough */
+    case SYMBOL_UNTRAPPED_WRITE:
+    default: ;
     }
-  sym = XSYMBOL (symbol);
 
  start:
   switch (sym->redirect)
@@ -1631,7 +1742,7 @@ DEFUN ("make-variable-buffer-local", Fmake_variable_buffer_local,
     default: emacs_abort ();
     }
 
-  if (sym->constant)
+  if (sym->trapped_write == SYMBOL_NOWRITE)
     error ("Symbol %s may not be buffer-local", SDATA (SYMBOL_NAME (variable)));
 
   if (!blv)
@@ -1706,7 +1817,7 @@ DEFUN ("make-local-variable", Fmake_local_variable, Smake_local_variable,
     default: emacs_abort ();
     }
 
-  if (sym->constant)
+  if (sym->trapped_write == SYMBOL_NOWRITE)
     error ("Symbol %s may not be buffer-local",
 	   SDATA (SYMBOL_NAME (variable)));
 
@@ -1900,7 +2011,7 @@ DEFUN ("make-variable-frame-local", Fmake_variable_frame_local, Smake_variable_f
     default: emacs_abort ();
     }
 
-  if (sym->constant)
+  if (sym->trapped_write == SYMBOL_NOWRITE)
     error ("Symbol %s may not be frame-local", SDATA (SYMBOL_NAME (variable)));
 
   blv = make_blv (sym, forwarded, valcontents);
@@ -3451,6 +3562,7 @@ syms_of_data (void)
   DEFSYM (Qcyclic_variable_indirection, "cyclic-variable-indirection");
   DEFSYM (Qvoid_variable, "void-variable");
   DEFSYM (Qsetting_constant, "setting-constant");
+  DEFSYM (Qtrapping_constant, "trapping-constant");
   DEFSYM (Qinvalid_read_syntax, "invalid-read-syntax");
 
   DEFSYM (Qinvalid_function, "invalid-function");
@@ -3526,6 +3638,8 @@ syms_of_data (void)
   PUT_ERROR (Qvoid_variable, error_tail, "Symbol's value as variable is void");
   PUT_ERROR (Qsetting_constant, error_tail,
 	     "Attempt to set a constant symbol");
+  PUT_ERROR (Qtrapping_constant, error_tail,
+             "Attempt to trap writes to a constant symbol");
   PUT_ERROR (Qinvalid_read_syntax, error_tail, "Invalid read syntax");
   PUT_ERROR (Qinvalid_function, error_tail, "Invalid function");
   PUT_ERROR (Qwrong_number_of_arguments, error_tail,
@@ -3698,10 +3812,17 @@ syms_of_data (void)
   DEFVAR_LISP ("most-positive-fixnum", Vmost_positive_fixnum,
 	       doc: /* The largest value that is representable in a Lisp integer.  */);
   Vmost_positive_fixnum = make_number (MOST_POSITIVE_FIXNUM);
-  XSYMBOL (intern_c_string ("most-positive-fixnum"))->constant = 1;
+  make_symbol_constant (intern_c_string ("most-positive-fixnum"));
 
   DEFVAR_LISP ("most-negative-fixnum", Vmost_negative_fixnum,
 	       doc: /* The smallest value that is representable in a Lisp integer.  */);
   Vmost_negative_fixnum = make_number (MOST_NEGATIVE_FIXNUM);
-  XSYMBOL (intern_c_string ("most-negative-fixnum"))->constant = 1;
+  make_symbol_constant (intern_c_string ("most-negative-fixnum"));
+
+  DEFSYM (Qwatchers, "watchers");
+  DEFSYM (Qunbind, "unbind");
+  DEFSYM (Qset, "set");
+  DEFSYM (Qset_default, "Qset_default");
+  defsubr (&Sadd_variable_watcher);
+  defsubr (&Sremove_variable_watcher);
 }
diff --git a/src/eval.c b/src/eval.c
index ac98ca1..3d17825 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -586,7 +586,7 @@ DEFUN ("defvaralias", Fdefvaralias, Sdefvaralias, 2, 3, 0,
 
   sym = XSYMBOL (new_alias);
 
-  if (sym->constant)
+  if (sym->trapped_write == SYMBOL_NOWRITE)
     /* Not sure why, but why not?  */
     error ("Cannot make a constant an alias");
 
@@ -623,7 +623,7 @@ DEFUN ("defvaralias", Fdefvaralias, Sdefvaralias, 2, 3, 0,
   XSYMBOL (base_variable)->declared_special = 1;
   sym->redirect = SYMBOL_VARALIAS;
   SET_SYMBOL_ALIAS (sym, XSYMBOL (base_variable));
-  sym->constant = SYMBOL_CONSTANT_P (base_variable);
+  sym->trapped_write = XSYMBOL (base_variable)->trapped_write;
   LOADHIST_ATTACH (new_alias);
   /* Even if docstring is nil: remove old docstring.  */
   Fput (new_alias, Qvariable_documentation, docstring);
@@ -2972,7 +2972,7 @@ specbind (Lisp_Object symbol, Lisp_Object value)
       specpdl_ptr->let.symbol = symbol;
       specpdl_ptr->let.old_value = SYMBOL_VAL (sym);
       grow_specpdl ();
-      if (!sym->constant)
+      if (!sym->trapped_write)
 	SET_SYMBOL_VAL (sym, value);
       else
 	set_internal (symbol, value, Qnil, 1);
diff --git a/src/font.c b/src/font.c
index 016b7e0..509d3cc 100644
--- a/src/font.c
+++ b/src/font.c
@@ -5388,19 +5388,19 @@ syms_of_font (void)
     [NUMERIC-VALUE SYMBOLIC-NAME ALIAS-NAME ...]
 NUMERIC-VALUE is an integer, and SYMBOLIC-NAME and ALIAS-NAME are symbols. */);
   Vfont_weight_table = BUILD_STYLE_TABLE (weight_table);
-  XSYMBOL (intern_c_string ("font-weight-table"))->constant = 1;
+  make_symbol_constant (intern_c_string ("font-weight-table"));
 
   DEFVAR_LISP_NOPRO ("font-slant-table", Vfont_slant_table,
 	       doc: /*  Vector of font slant symbols vs the corresponding numeric values.
 See `font-weight-table' for the format of the vector. */);
   Vfont_slant_table = BUILD_STYLE_TABLE (slant_table);
-  XSYMBOL (intern_c_string ("font-slant-table"))->constant = 1;
+  make_symbol_constant (intern_c_string ("font-slant-table"));
 
   DEFVAR_LISP_NOPRO ("font-width-table", Vfont_width_table,
 	       doc: /*  Alist of font width symbols vs the corresponding numeric values.
 See `font-weight-table' for the format of the vector. */);
   Vfont_width_table = BUILD_STYLE_TABLE (width_table);
-  XSYMBOL (intern_c_string ("font-width-table"))->constant = 1;
+  make_symbol_constant (intern_c_string ("font-width-table"));
 
   staticpro (&font_style_table);
   font_style_table = make_uninit_vector (3);
diff --git a/src/lisp.h b/src/lisp.h
index 3efa492..b8431ee 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -337,7 +337,7 @@ error !;
 #define lisp_h_NILP(x) EQ (x, Qnil)
 #define lisp_h_SET_SYMBOL_VAL(sym, v) \
    (eassert ((sym)->redirect == SYMBOL_PLAINVAL), (sym)->val.value = (v))
-#define lisp_h_SYMBOL_CONSTANT_P(sym) (XSYMBOL (sym)->constant)
+#define lisp_h_SYMBOL_TRAPPED_WRITE_P(sym) (XSYMBOL (sym)->trapped_write)
 #define lisp_h_SYMBOL_VAL(sym) \
    (eassert ((sym)->redirect == SYMBOL_PLAINVAL), (sym)->val.value)
 #define lisp_h_SYMBOLP(x) (XTYPE (x) == Lisp_Symbol)
@@ -383,7 +383,7 @@ error !;
 # define MISCP(x) lisp_h_MISCP (x)
 # define NILP(x) lisp_h_NILP (x)
 # define SET_SYMBOL_VAL(sym, v) lisp_h_SET_SYMBOL_VAL (sym, v)
-# define SYMBOL_CONSTANT_P(sym) lisp_h_SYMBOL_CONSTANT_P (sym)
+# define SYMBOL_TRAPPED_WRITE_P(sym) lisp_h_SYMBOL_TRAPPED_WRITE_P (sym)
 # define SYMBOL_VAL(sym) lisp_h_SYMBOL_VAL (sym)
 # define SYMBOLP(x) lisp_h_SYMBOLP (x)
 # define VECTORLIKEP(x) lisp_h_VECTORLIKEP (x)
@@ -630,6 +630,13 @@ enum symbol_redirect
   SYMBOL_FORWARDED = 3
 };
 
+enum symbol_trapped_write
+{
+  SYMBOL_UNTRAPPED_WRITE = 0,
+  SYMBOL_NOWRITE = 1,
+  SYMBOL_TRAPPED_WRITE = 2
+};
+
 struct Lisp_Symbol
 {
   bool_bf gcmarkbit : 1;
@@ -641,10 +648,10 @@ struct Lisp_Symbol
      3 : it's a forwarding variable, the value is in `forward'.  */
   ENUM_BF (symbol_redirect) redirect : 3;
 
-  /* Non-zero means symbol is constant, i.e. changing its value
-     should signal an error.  If the value is 3, then the var
-     can be changed, but only by `defconst'.  */
-  unsigned constant : 2;
+  /* 0 : normal case, just set the value
+     1 : constant, cannot set, e.g. nil, t, :keywords.
+     2 : trap the write, call watcher functions.  */
+  ENUM_BF (symbol_trapped_write) trapped_write : 2;
 
   /* Interned state of the symbol.  This is an enumerator from
      enum symbol_interned.  */
@@ -1829,14 +1836,14 @@ SYMBOL_INTERNED_IN_INITIAL_OBARRAY_P (Lisp_Object sym)
   return XSYMBOL (sym)->interned == SYMBOL_INTERNED_IN_INITIAL_OBARRAY;
 }
 
-/* Value is non-zero if symbol is considered a constant, i.e. its
-   value cannot be changed (there is an exception for keyword symbols,
-   whose value can be set to the keyword symbol itself).  */
+/* Value is non-zero if symbol cannot be changed through a simple set,
+ i.e. it's a constant (e.g. nil, t, :keywords), or it has some
+ watching functions.  */
 
 INLINE int
-(SYMBOL_CONSTANT_P) (Lisp_Object sym)
+(SYMBOL_TRAPPED_WRITE_P) (Lisp_Object sym)
 {
-  return lisp_h_SYMBOL_CONSTANT_P (sym);
+  return lisp_h_SYMBOL_TRAPPED_WRITE_P (sym);
 }
 
 /* Placeholder for make-docfile to process.  The actual symbol
@@ -3256,6 +3263,12 @@ set_symbol_next (Lisp_Object sym, struct Lisp_Symbol *next)
   XSYMBOL (sym)->next = next;
 }
 
+INLINE void
+make_symbol_constant (Lisp_Object sym)
+{
+  XSYMBOL (sym)->trapped_write = SYMBOL_NOWRITE;
+}
+
 /* Buffer-local (also frame-local) variable access functions.  */
 
 INLINE int
diff --git a/src/lread.c b/src/lread.c
index c4456f3..0852215 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -3721,7 +3721,7 @@ intern_sym (Lisp_Object sym, Lisp_Object obarray, Lisp_Object index)
 
   if (SREF (SYMBOL_NAME (sym), 0) == ':' && EQ (obarray, initial_obarray))
     {
-      XSYMBOL (sym)->constant = 1;
+      make_symbol_constant (sym);
       XSYMBOL (sym)->redirect = SYMBOL_PLAINVAL;
       SET_SYMBOL_VAL (XSYMBOL (sym), sym);
     }
@@ -4010,12 +4010,12 @@ init_obarray (void)
 
   DEFSYM (Qnil, "nil");
   SET_SYMBOL_VAL (XSYMBOL (Qnil), Qnil);
-  XSYMBOL (Qnil)->constant = 1;
+  make_symbol_constant (Qnil);
   XSYMBOL (Qnil)->declared_special = true;
 
   DEFSYM (Qt, "t");
   SET_SYMBOL_VAL (XSYMBOL (Qt), Qt);
-  XSYMBOL (Qt)->constant = 1;
+  make_symbol_constant (Qt);
   XSYMBOL (Qt)->declared_special = true;
 
   /* Qt is correct even if CANNOT_DUMP.  loadup.el will set to nil at end.  */
-- 
2.6.2


[-- Attachment #3: v2-0002-Add-function-to-trigger-debugger-on-variable-writ.patch --]
[-- Type: text/x-diff, Size: 3630 bytes --]

From eafd2867e15a5cfafa75868ae9bce239b9fdfa3a Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 21 Nov 2015 16:03:06 -0500
Subject: [PATCH v2 2/3] Add function to trigger debugger on variable write

* lisp/emacs-lisp/debug.el (debug-watchpoint):
  (debug--variable-list):
  (cancel-debug-watchpoint): New functions.
  (debugger-setup-buffer): Add watchpoint clause.
---
 lisp/emacs-lisp/debug.el | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/lisp/emacs-lisp/debug.el b/lisp/emacs-lisp/debug.el
index 0e307fa..68be115 100644
--- a/lisp/emacs-lisp/debug.el
+++ b/lisp/emacs-lisp/debug.el
@@ -304,6 +304,22 @@ (defun debugger-setup-buffer (args)
        (delete-char 1)
        (insert ? )
        (beginning-of-line))
+      ;; Watchpoint triggered.
+      ((and `watchpoint (let `(,symbol ,newval . ,details) (cdr args)))
+       (insert
+        "--"
+        (pcase details
+          (`(unbind ,_) (format "unbinding %s" symbol))
+          (`(let ,_) (format "let-binding %s to %S" symbol newval))
+          (`(set nil) (format "setting %s to %S" symbol newval))
+          (`(set ,buffer) (format "setting %s in %s to %S"
+                                  symbol buffer newval))
+          (`(set-default ,_) (format "setting %s's default value to %S"
+                                     symbol newval))
+          (_ (format "watchpoint triggered %S" (cdr args))))
+        ": ")
+       (setq pos (point))
+       (insert ?\n))
       ;; Debugger entered for an error.
       (`error
        (insert "--Lisp error: ")
@@ -848,6 +864,53 @@ (defun debugger-list-functions ()
           (princ "Note: if you have redefined a function, then it may no longer\n")
           (princ "be set to debug on entry, even if it is in the list."))))))
 
+(defun debug--implement-debug-watch (op where symbol newval)
+  "Conditionally call the debugger.
+This function is called when SYMBOL's value is modified."
+  (if (or inhibit-debug-on-entry debugger-jumping-flag)
+      nil
+    (let ((inhibit-debug-on-entry t))
+      (funcall debugger 'watchpoint symbol newval op where))))
+
+;;;###autoload
+(defun debug-watch (variable)
+  (interactive
+   (let* ((var-at-point (variable-at-point))
+          (var (and (symbolp var-at-point) var-at-point))
+          (val (completing-read
+                (concat "Debug when setting variable"
+                        (if var (format " (default %s): " var) ": "))
+                obarray #'boundp
+                t nil nil (and var (symbol-name var)))))
+     (list (if (equal val "") var (intern val)))))
+  (add-variable-watcher variable #'debug--implement-debug-watch))
+
+
+(defun debug--variable-list ()
+  "List of variables currently set for debug on set."
+  (let ((vars '()))
+    (mapatoms
+     (lambda (s)
+       (when (memq #'debug--implement-debug-watch
+                   (get s 'watchers))
+         (push s vars))))
+    vars))
+
+;;;###autoload
+(defun cancel-debug-watch (&optional variable)
+  (interactive
+   (list (let ((name
+                (completing-read
+                 "Cancel debug on set for variable (default all variables): "
+                 (mapcar #'symbol-name (debug--variable-list)) nil t)))
+           (when name
+             (unless (string= name "")
+               (intern name))))))
+  (if variable
+      (remove-variable-watcher variable #'debug--implement-debug-watch)
+    (message "Canceling debug-watch for all variables")
+    (mapc #'cancel-debug-watchpoint (debug--variable-list))))
+
 (provide 'debug)
 
 ;;; debug.el ends here
-- 
2.6.2


[-- Attachment #4: v2-0003-Ensure-redisplay-using-variable-watcher.patch --]
[-- Type: text/x-diff, Size: 5311 bytes --]

From 06df77af4794cc94e534aa695da06ff064e47e76 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 21 Nov 2015 17:02:42 -0500
Subject: [PATCH v2 3/3] Ensure redisplay using variable watcher

Instead of looking up the variable name in redisplay--variables when
setting.

* lisp/frame.el: Replace redisplay--variables with add-variable-watcher
  calls.
* src/xdisp.c (set_redisplay): Rename from maybe_set_redisplay, set the
  redisplay flag unconditionally.
* src/data.c (watcher_table): Add set_resdisplay.
  (syms_of_data): New var set-redisplay-internal-watcher-number.
  (set_internal): Remove maybe_set_redisplay call.
---
 lisp/emacs-lisp/debug.el |  2 +-
 lisp/frame.el            |  3 +--
 src/data.c               | 11 +++++++++--
 src/window.h             |  2 +-
 src/xdisp.c              | 15 ++++-----------
 5 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/lisp/emacs-lisp/debug.el b/lisp/emacs-lisp/debug.el
index 68be115..e20654e 100644
--- a/lisp/emacs-lisp/debug.el
+++ b/lisp/emacs-lisp/debug.el
@@ -864,7 +864,7 @@ (defun debugger-list-functions ()
           (princ "Note: if you have redefined a function, then it may no longer\n")
           (princ "be set to debug on entry, even if it is in the list."))))))
 
-(defun debug--implement-debug-watch (op where symbol newval)
+(defun debug--implement-debug-watch (symbol newval op where)
   "Conditionally call the debugger.
 This function is called when SYMBOL's value is modified."
   (if (or inhibit-debug-on-entry debugger-jumping-flag)
diff --git a/lisp/frame.el b/lisp/frame.el
index f024065..79c954a 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -2232,9 +2232,8 @@ (make-obsolete-variable
  'window-system-version "it does not give useful information." "24.3")
 
 ;; Variables which should trigger redisplay of the current buffer.
-(setq redisplay--variables (make-hash-table :test 'eq :size 10))
 (mapc (lambda (var)
-        (puthash var 1 redisplay--variables))
+        (add-variable-watcher var set-redisplay-internal-watcher-number))
       '(line-spacing
         overline-margin
         line-prefix
diff --git a/src/data.c b/src/data.c
index 1576280..a1ceeec 100644
--- a/src/data.c
+++ b/src/data.c
@@ -33,6 +33,7 @@
 #include "keyboard.h"
 #include "frame.h"
 #include "keymap.h"
+#include "window.h"
 
 static void notify_variable_watchers (Lisp_Object symbol,
                                       Lisp_Object newval,
@@ -1262,8 +1263,6 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
     default: ;
     }
 
-  maybe_set_redisplay (symbol);
-
  start:
   switch (sym->redirect)
     {
@@ -1435,9 +1434,11 @@ DEFUN ("remove-variable-watcher", Fremove_variable_watcher, Sremove_variable_wat
 typedef void (*WATCHER_FUNCTION) (Lisp_Object, Lisp_Object, Lisp_Object, Lisp_Object);
 static const WATCHER_FUNCTION watcher_table[] =
   {
+    &set_redisplay
   };
 enum
   {
+    WATCHER_NUMBER_SET_REDISPLAY
   };
 
 static void
@@ -3825,4 +3826,10 @@ syms_of_data (void)
   DEFSYM (Qset_default, "Qset_default");
   defsubr (&Sadd_variable_watcher);
   defsubr (&Sremove_variable_watcher);
+
+  DEFVAR_INT ("set-redisplay-internal-watcher-number",
+              Vset_redisplay_internal_watcher_number,
+              doc: /* Internal watch function constant.  */);
+  Vset_redisplay_internal_watcher_number = WATCHER_NUMBER_SET_REDISPLAY;
+  make_symbol_constant (intern_c_string ("set-redisplay-internal-watcher-number"));
 }
diff --git a/src/window.h b/src/window.h
index 135f5de..ebf5ef7 100644
--- a/src/window.h
+++ b/src/window.h
@@ -1056,7 +1056,7 @@ extern void wset_redisplay (struct window *w);
 extern void fset_redisplay (struct frame *f);
 extern void bset_redisplay (struct buffer *b);
 extern void bset_update_mode_line (struct buffer *b);
-extern void maybe_set_redisplay (Lisp_Object);
+extern void set_redisplay (Lisp_Object, Lisp_Object, Lisp_Object, Lisp_Object);
 /* Call this to tell redisplay to look for other windows than selected-window
    that need to be redisplayed.  Calling one of the *set_redisplay functions
    above already does it, so it's only needed in unusual cases.  */
diff --git a/src/xdisp.c b/src/xdisp.c
index 30dfac5..5c5c4d2 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -621,14 +621,11 @@ bset_update_mode_line (struct buffer *b)
 }
 
 void
-maybe_set_redisplay (Lisp_Object symbol)
+set_redisplay (Lisp_Object symbol, Lisp_Object newval,
+               Lisp_Object op, Lisp_Object where)
 {
-  if (HASH_TABLE_P (Vredisplay__variables)
-      && hash_lookup (XHASH_TABLE (Vredisplay__variables), symbol, NULL) >= 0)
-    {
-      bset_update_mode_line (current_buffer);
-      current_buffer->prevent_redisplay_optimizations_p = true;
-    }
+  bset_update_mode_line (current_buffer);
+  current_buffer->prevent_redisplay_optimizations_p = true;
 }
 
 #ifdef GLYPH_DEBUG
@@ -31477,10 +31474,6 @@ or t (meaning all windows).  */);
   DEFVAR_LISP ("redisplay--mode-lines-cause", Vredisplay__mode_lines_cause,
 	       doc: /*  */);
   Vredisplay__mode_lines_cause = Fmake_hash_table (0, NULL);
-
-  DEFVAR_LISP ("redisplay--variables", Vredisplay__variables,
-     doc: /* A hash-table of variables changing which triggers a thorough redisplay.  */);
-  Vredisplay__variables = Qnil;
 }
 
 
-- 
2.6.2

Re: Lisp watchpoints

[Stefan Monnier]

> @@ -629,9 +634,18 @@ DEFUN ("makunbound", Fmakunbound, Smakunbound, 1, 1, 0,
>    (register Lisp_Object symbol)
>  {
>    CHECK_SYMBOL (symbol);
> -  if (SYMBOL_CONSTANT_P (symbol))
> -    xsignal1 (Qsetting_constant, symbol);
> -  Fset (symbol, Qunbound);
> +  switch (XSYMBOL (symbol)->trapped_write)
> +    {
> +    case SYMBOL_NOWRITE:
> +      xsignal1 (Qsetting_constant, symbol);
> +
> +    case SYMBOL_TRAPPED_WRITE:
> +      notify_variable_watchers (symbol, Qnil, Qunbind, Qnil);
> +      /* fallthrough */
> +    case SYMBOL_UNTRAPPED_WRITE:
> +    default:
> +      Fset (symbol, Qunbound);
> +    }
>    return symbol;
>  }

Hmm... `unbind' is the term usually used when exiting a let binding
rather than when setting a var to Qunbound.  So I think it'd be better
here to use Qmakunbound to avoid confusion.

> +  set_symbol_trapped_write (symbol, SYMBOL_UNTRAPPED_WRITE); /* avoid recursion */

Please capitalize and punctaute your comments.  Also, try not to exceed
80 columns.

> +  ptrdiff_t count = SPECPDL_INDEX ();
> +  record_unwind_protect (&reset_symbol_trapped_write, symbol);
                           ^^^
I think this "&" is unneeded.

> +  while (!NILP (watchers))
> +    {
> +      Lisp_Object watcher = XCAR (watchers);

This will lead to a segmentation fault after something like (put <var>
'watchers [1]).  While very unlikely, it's very easy to avoid this
problem: just use CONSP instead of !NILP.

> +      else if (FUNCTIONP (watcher))
> +        CALLN (Ffuncall, watcher, operation, where, symbol, newval);

I don't think you need to test "FUNCTIONP (watcher)".
And you don't need Ffuncall: what you wrote is similar to writing

   (funcall #'funcall watcher operation where symbol newval)

> -  if (sym->constant)
> +  if (sym->trapped_write == SYMBOL_NOWRITE)
>      error ("Symbol %s may not be frame-local", SDATA (SYMBOL_NAME (variable)));

Actually, trapped writes won't work reliably for frame-local vars
(because you can set them with things like `put' or `setcar' rather than
let/setq/...), so better disallow combining the two (frame-local vars
are obsolete anyway and should be removed "soonish", so it's fine not
to provide support for new features for them).


        Stefan

Re: Lisp watchpoints

[Noam Postavsky]

On Sat, Nov 28, 2015 at 11:44 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> +  while (!NILP (watchers))
>> +    {
>> +      Lisp_Object watcher = XCAR (watchers);
>
> This will lead to a segmentation fault after something like (put <var>
> 'watchers [1]).  While very unlikely, it's very easy to avoid this
> problem: just use CONSP instead of !NILP.
>
>> +      else if (FUNCTIONP (watcher))
>> +        CALLN (Ffuncall, watcher, operation, where, symbol, newval);
>
> I don't think you need to test "FUNCTIONP (watcher)".

Isn't the FUNCTIONP test needed for basically the same reason as using
CONSP over !NILP? i.e. to avoid segfault in case of (put <var>
'watchers '([1])).

Re: Lisp watchpoints

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +      else if (FUNCTIONP (watcher))
>> +        CALLN (Ffuncall, watcher, operation, where, symbol, newval);
>
> I don't think you need to test "FUNCTIONP (watcher)".
> And you don't need Ffuncall: what you wrote is similar to writing
>
>    (funcall #'funcall watcher operation where symbol newval)

The first argument to CALLN is a C function implementing a Lisp
function, like Ffuncall, but watcher isn't.  So the Ffuncall is needed
and it's equivalent to (funcall watcher operation where symbol newval).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Lisp watchpoints

[Andreas Schwab]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> On Sat, Nov 28, 2015 at 11:44 PM, Stefan Monnier
> <monnier@iro.umontreal.ca> wrote:
>>> +  while (!NILP (watchers))
>>> +    {
>>> +      Lisp_Object watcher = XCAR (watchers);
>>
>> This will lead to a segmentation fault after something like (put <var>
>> 'watchers [1]).  While very unlikely, it's very easy to avoid this
>> problem: just use CONSP instead of !NILP.
>>
>>> +      else if (FUNCTIONP (watcher))
>>> +        CALLN (Ffuncall, watcher, operation, where, symbol, newval);
>>
>> I don't think you need to test "FUNCTIONP (watcher)".
>
> Isn't the FUNCTIONP test needed for basically the same reason as using
> CONSP over !NILP? i.e. to avoid segfault in case of (put <var>
> 'watchers '([1])).

Ffuncall does all necessary checks.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Lisp watchpoints

[Noam Postavsky]

On Sun, Nov 29, 2015 at 6:28 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>> Isn't the FUNCTIONP test needed for basically the same reason as using
>> CONSP over !NILP? i.e. to avoid segfault in case of (put <var>
>> 'watchers '([1])).
>
> Ffuncall does all necessary checks.

It would signal in case of error right? Currently the code is ignoring
bad watcher elements (including out of range integers), but perhaps
it's actually better to signal an error.

Re: Lisp watchpoints

[Stefan Monnier]

> It would signal in case of error right? Currently the code is ignoring
> bad watcher elements (including out of range integers), but perhaps
> it's actually better to signal an error.

It's marginally better to signal an error, I think.  But the difference
is not tremendously important.


        Stefan

Re: Lisp watchpoints

[Stefan Monnier]

>> I don't think you need to test "FUNCTIONP (watcher)".
>> And you don't need Ffuncall: what you wrote is similar to writing
>> 
>> (funcall #'funcall watcher operation where symbol newval)

> The first argument to CALLN is a C function implementing a Lisp
> function, like Ffuncall, but watcher isn't.  So the Ffuncall is needed
> and it's equivalent to (funcall watcher operation where symbol newval).

Duh, indeed, sorry!


        Stefan

Re: Lisp watchpoints

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  John Wiegley <jwiegley@gmail.com>,  emacs-devel@gnu.org
> Date: Sat, 28 Nov 2015 23:44:19 -0500
> 
> Actually, trapped writes won't work reliably for frame-local vars

Will they work for let-bound "local" variables?  What happens when
they go "out of scope"?

Re: Lisp watchpoints

[Noam Postavsky]

On Sun, Nov 29, 2015 at 10:45 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  John Wiegley <jwiegley@gmail.com>,  emacs-devel@gnu.org
>> Date: Sat, 28 Nov 2015 23:44:19 -0500
>>
>> Actually, trapped writes won't work reliably for frame-local vars
>
> Will they work for let-bound "local" variables?  What happens when
> they go "out of scope"?

The let-binding is currently trapped, but not the corresponding
unbinding. Is it better to skip both? As far as I can tell, it's not
feasible to trap the unbinding.

Re: Lisp watchpoints

[Noam Postavsky]

On Sun, Nov 29, 2015 at 11:01 AM, Noam Postavsky
<npostavs@users.sourceforge.net> wrote:
> On Sun, Nov 29, 2015 at 10:45 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>>> Cc: Eli Zaretskii <eliz@gnu.org>,  John Wiegley <jwiegley@gmail.com>,  emacs-devel@gnu.org
>>> Date: Sat, 28 Nov 2015 23:44:19 -0500
>>>
>>> Actually, trapped writes won't work reliably for frame-local vars
>>
>> Will they work for let-bound "local" variables?  What happens when
>> they go "out of scope"?
>
> The let-binding is currently trapped, but not the corresponding
> unbinding. Is it better to skip both? As far as I can tell, it's not
> feasible to trap the unbinding.

Um, actually, what did you mean by "local" exactly? I should clarify
that this only applies to dynamically-bound variables, lexical
variables are never trapped.

Re: Lisp watchpoints

[Stefan Monnier]

>> Actually, trapped writes won't work reliably for frame-local vars
> Will they work for let-bound "local" variables?  What happens when
> they go "out of scope"?

There's no technical reason I can think of why they wouldn't, but
I haven't checked the proposed patch to see if it's handled right.


        Stefan

Re: Lisp watchpoints

[Eli Zaretskii]

> Date: Sun, 29 Nov 2015 09:12:34 -0500
> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Cc: John Wiegley <jwiegley@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> > Ffuncall does all necessary checks.
> 
> It would signal in case of error right? Currently the code is ignoring
> bad watcher elements (including out of range integers), but perhaps
> it's actually better to signal an error.

No, I think it would be better to report an error, and then continue,
perhaps after removing the watch.

Re: Lisp watchpoints

[Eli Zaretskii]

> Date: Sat, 28 Nov 2015 21:04:25 -0500
> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, John Wiegley <jwiegley@gmail.com>, emacs-devel@gnu.org
> 
> From c1e2e14097f4488384ea8ea3cab3cd51c41947eb Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Thu, 19 Nov 2015 19:50:06 -0500
> Subject: [PATCH v2 1/3] Add lisp watchpoints

Thanks.

> +DEFUN ("remove-variable-watcher", Fremove_variable_watcher, Sremove_variable_watcher,
> +       2, 2, 0,
> +       doc: /* Cause WATCH-FUNCTION to be called when SYMBOL is set. */)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy/paste mistake?

> +static void
> +notify_variable_watchers (Lisp_Object symbol,
> +                          Lisp_Object newval,
> +                          Lisp_Object operation,
> +                          Lisp_Object where)
> +{
> +  Lisp_Object watchers = Fget (symbol, Qwatchers);
> +
> +  set_symbol_trapped_write (symbol, SYMBOL_UNTRAPPED_WRITE); /* avoid recursion */
> +  ptrdiff_t count = SPECPDL_INDEX ();
> +  record_unwind_protect (&reset_symbol_trapped_write, symbol);
> +
> +  while (!NILP (watchers))
> +    {
> +      Lisp_Object watcher = XCAR (watchers);
> +      if (INTEGERP (watcher))
> +        {
> +          EMACS_INT wnum = XINT (watcher);
> +          if (wnum < ARRAYELTS (watcher_table))
> +            watcher_table[wnum] (operation, where, symbol, newval);
> +        }
> +      else if (FUNCTIONP (watcher))
> +        CALLN (Ffuncall, watcher, operation, where, symbol, newval);
> +      watchers = XCDR (watchers);
> +    }

The call to ARRAYELTS should be outside of the loop (for those poor
souls who run unoptimized builds most of the time).

>  static const WATCHER_FUNCTION watcher_table[] =
>    {
> +    &set_redisplay
>    };
>  enum
>    {
> +    WATCHER_NUMBER_SET_REDISPLAY
>    };

Shouldn't we make sure WATCHER_NUMBER_SET_REDISPLAY's value is zero?

> +  DEFVAR_INT ("set-redisplay-internal-watcher-number",
> +              Vset_redisplay_internal_watcher_number,
> +              doc: /* Internal watch function constant.  */);
> +  Vset_redisplay_internal_watcher_number = WATCHER_NUMBER_SET_REDISPLAY;
> +  make_symbol_constant (intern_c_string ("set-redisplay-internal-watcher-number"));

I'd prefer if all this were moved to window.c.  data.c has no business
with display-related issues.

Please also add a short notice for etc/NEWS.  Bonus points for adding
some tests.

Other than that, and after the comments by others are taken care of, I
think this is good to go into master.

Thanks again for working on this.

Re: Lisp watchpoints

[Eli Zaretskii]

> Date: Sun, 29 Nov 2015 11:01:32 -0500
> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, John Wiegley <jwiegley@gmail.com>, emacs-devel@gnu.org
> 
> >> Actually, trapped writes won't work reliably for frame-local vars
> >
> > Will they work for let-bound "local" variables?  What happens when
> > they go "out of scope"?
> 
> The let-binding is currently trapped, but not the corresponding
> unbinding. Is it better to skip both? As far as I can tell, it's not
> feasible to trap the unbinding.

If unbinding will cause the watch to silently be disabled, that is
good enough.

Thanks.

Re: Lisp watchpoints

[Eli Zaretskii]

> Date: Sun, 29 Nov 2015 11:04:23 -0500
> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, John Wiegley <jwiegley@gmail.com>, emacs-devel@gnu.org
> 
> >>> Actually, trapped writes won't work reliably for frame-local vars
> >>
> >> Will they work for let-bound "local" variables?  What happens when
> >> they go "out of scope"?
> >
> > The let-binding is currently trapped, but not the corresponding
> > unbinding. Is it better to skip both? As far as I can tell, it's not
> > feasible to trap the unbinding.
> 
> Um, actually, what did you mean by "local" exactly? I should clarify
> that this only applies to dynamically-bound variables, lexical
> variables are never trapped.

Yes, I also meant dynamically-bound variables.  Sorry for being
unclear.

Re: Lisp watchpoints

[Noam Postavsky]

On Sun, Nov 29, 2015 at 11:43 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> +DEFUN ("remove-variable-watcher", Fremove_variable_watcher, Sremove_variable_watcher,
>> +       2, 2, 0,
>> +       doc: /* Cause WATCH-FUNCTION to be called when SYMBOL is set. */)
>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Copy/paste mistake?

Oops.

>
> The call to ARRAYELTS should be outside of the loop (for those poor
> souls who run unoptimized builds most of the time).

Compiling the following shows gcc folds the constants even with -O0
(also, notify_variable_watchers() should only be called for a few
selected symbols so I don't think it would really slow things down
very much).

    int array[] = { 1, 2, 3, 4 };

    /* Number of elements in an array.  */
    #define ARRAYELTS(arr) (sizeof (arr) / sizeof (arr)[0])

    int foo(int x)
    {
        if (x < ARRAYELTS(array))
            return array[x];
        return 0;
    }


>
>>  static const WATCHER_FUNCTION watcher_table[] =
>>    {
>> +    &set_redisplay
>>    };
>>  enum
>>    {
>> +    WATCHER_NUMBER_SET_REDISPLAY
>>    };
>
> Shouldn't we make sure WATCHER_NUMBER_SET_REDISPLAY's value is zero?

Probably better to be explicit, yes.

>
>> +  DEFVAR_INT ("set-redisplay-internal-watcher-number",
>> +              Vset_redisplay_internal_watcher_number,
>> +              doc: /* Internal watch function constant.  */);
>> +  Vset_redisplay_internal_watcher_number = WATCHER_NUMBER_SET_REDISPLAY;
>> +  make_symbol_constant (intern_c_string ("set-redisplay-internal-watcher-number"));
>
> I'd prefer if all this were moved to window.c.  data.c has no business
> with display-related issues.

Hmm, then how should we make the connection between watcher numbers
and C function pointers? I think data.c should be in charge of that.

>
> Please also add a short notice for etc/NEWS.  Bonus points for adding
> some tests.

Will do.

Re: Lisp watchpoints

[Eli Zaretskii]

> Date: Sun, 29 Nov 2015 14:40:57 -0500
> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, John Wiegley <jwiegley@gmail.com>, emacs-devel@gnu.org
> 
> > The call to ARRAYELTS should be outside of the loop (for those poor
> > souls who run unoptimized builds most of the time).
> 
> Compiling the following shows gcc folds the constants even with -O0
> (also, notify_variable_watchers() should only be called for a few
> selected symbols so I don't think it would really slow things down
> very much).

That depends om your GCC version, and then there are -fFOO switches
that can control each specific optimizations.  And seeing that in the
code simply looks wrong, regardless.  So let's move it out, please.

> >> +  DEFVAR_INT ("set-redisplay-internal-watcher-number",
> >> +              Vset_redisplay_internal_watcher_number,
> >> +              doc: /* Internal watch function constant.  */);
> >> +  Vset_redisplay_internal_watcher_number = WATCHER_NUMBER_SET_REDISPLAY;
> >> +  make_symbol_constant (intern_c_string ("set-redisplay-internal-watcher-number"));
> >
> > I'd prefer if all this were moved to window.c.  data.c has no business
> > with display-related issues.
> 
> Hmm, then how should we make the connection between watcher numbers
> and C function pointers? I think data.c should be in charge of that.

What connection?  I'm afraid I'm not following.

> > > Please also add a short notice for etc/NEWS.  Bonus points for adding
> > some tests.
> 
> Will do.

Thanks.

Re: Lisp watchpoints

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> The call to ARRAYELTS should be outside of the loop (for those poor
> souls who run unoptimized builds most of the time).

ARRAYELTS is a compile-time constant.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Lisp watchpoints

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Date: Sun, 29 Nov 2015 21:00:59 +0100
> Cc: jwiegley@gmail.com, emacs-devel@gnu.org, monnier@iro.umontreal.ca,
> 	Noam Postavsky <npostavs@users.sourceforge.net>
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The call to ARRAYELTS should be outside of the loop (for those poor
> > souls who run unoptimized builds most of the time).
> 
> ARRAYELTS is a compile-time constant.

Yes.

Re: Lisp watchpoints

[Noam Postavsky]

On Sun, Nov 29, 2015 at 2:54 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Sun, 29 Nov 2015 14:40:57 -0500
>> From: Noam Postavsky <npostavs@users.sourceforge.net>
>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, John Wiegley <jwiegley@gmail.com>, emacs-devel@gnu.org
>>
>> > The call to ARRAYELTS should be outside of the loop (for those poor
>> > souls who run unoptimized builds most of the time).
>>
>> Compiling the following shows gcc folds the constants even with -O0
>> (also, notify_variable_watchers() should only be called for a few
>> selected symbols so I don't think it would really slow things down
>> very much).
>
> That depends om your GCC version, and then there are -fFOO switches
> that can control each specific optimizations.  And seeing that in the
> code simply looks wrong, regardless.  So let's move it out, please.

Since ARRAYELTS is a compile-time constant, to me it looks completely
redundant to move it out of the loop. Would it help if I added a
WATCHER_NUMBER_LIMIT to the enum after WATCHER_NUMBER_SET_REDISPLAY
and used that instead?

>
>> >> +  DEFVAR_INT ("set-redisplay-internal-watcher-number",
>> >> +              Vset_redisplay_internal_watcher_number,
>> >> +              doc: /* Internal watch function constant.  */);
>> >> +  Vset_redisplay_internal_watcher_number = WATCHER_NUMBER_SET_REDISPLAY;
>> >> +  make_symbol_constant (intern_c_string ("set-redisplay-internal-watcher-number"));
>> >
>> > I'd prefer if all this were moved to window.c.  data.c has no business
>> > with display-related issues.
>>
>> Hmm, then how should we make the connection between watcher numbers
>> and C function pointers? I think data.c should be in charge of that.
>
> What connection?  I'm afraid I'm not following.

The watchers_table holds all of the C functions that can be used as
watchers. I think it makes sense to have this table in data.c. Lisp
code that wants to add a watcher to a variable needs to know which
number to use (0 for set_redisplay, because it's the 1st (and only)
entry in the table). Since data.c has the table, I figured it makes
sense that it would be in charge of exporting the number->Cpointer
mapping to lisp. Hence, set-redisplay-internal-watcher-number is
defined in data.c.

If window.c would be in charge of defining
set-redisplay-internal-watcher-number, then it would need to know the
right number, which would probably mean moving the
WATCHER_NUMBER_SET_REDISPLAY definition to a header file away from the
watcher_table definition, which would be suboptimal, I think.

Or I guess we could just hard code the number 0 with a comment to look
in data.c; is that too dirty?

Re: Lisp watchpoints

[Eli Zaretskii]

> Date: Sun, 29 Nov 2015 15:35:24 -0500
> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, John Wiegley <jwiegley@gmail.com>, emacs-devel@gnu.org
> 
> Would it help if I added a WATCHER_NUMBER_LIMIT to the enum after
> WATCHER_NUMBER_SET_REDISPLAY and used that instead?

Yes, thanks.  (WATCHER_NUMBER_LAST sounds a better name to me, but
that's nitpicking.)

> If window.c would be in charge of defining
> set-redisplay-internal-watcher-number, then it would need to know the
> right number, which would probably mean moving the
> WATCHER_NUMBER_SET_REDISPLAY definition to a header file away from the
> watcher_table definition

Yes, that's what I had in mind.

> which would be suboptimal, I think.

But then data.c will forever be doomed to export all the other watcher
numbers, utterly unrelated to it.  I think this would be worse.  Code
should live where its natural home is, even if that means to have some
declarations in a header.

> Or I guess we could just hard code the number 0 with a comment to look
> in data.c; is that too dirty?

Probably.  At least if we believe there will be other numbers.

Thanks.

Re: Lisp watchpoints

[Noam Postavsky]

On Sun, Nov 29, 2015 at 3:45 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Sun, 29 Nov 2015 15:35:24 -0500
>> From: Noam Postavsky <npostavs@users.sourceforge.net>
>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, John Wiegley <jwiegley@gmail.com>, emacs-devel@gnu.org
>>
>> Would it help if I added a WATCHER_NUMBER_LIMIT to the enum after
>> WATCHER_NUMBER_SET_REDISPLAY and used that instead?
>
> Yes, thanks.  (WATCHER_NUMBER_LAST sounds a better name to me, but
> that's nitpicking.)

The thing I don't like about LAST (or MAX) is that it implies it's a
valid number, when it isn't.

>
>> If window.c would be in charge of defining
>> set-redisplay-internal-watcher-number, then it would need to know the
>> right number, which would probably mean moving the
>> WATCHER_NUMBER_SET_REDISPLAY definition to a header file away from the
>> watcher_table definition
>
> Yes, that's what I had in mind.
>
>> which would be suboptimal, I think.
>
> But then data.c will forever be doomed to export all the other watcher
> numbers, utterly unrelated to it.  I think this would be worse.  Code
> should live where its natural home is, even if that means to have some
> declarations in a header.

Hmm, okay. Header it is then.

Re: Lisp watchpoints

[Stefan Monnier]

>> The let-binding is currently trapped, but not the corresponding
>> unbinding.  Is it better to skip both?  As far as I can tell, it's not
>> feasible to trap the unbinding.

The unbinding currently doesn't check CONSTANT_P, but that's just an
optimization that becomes invalid for trapped variables.  I don't see
why we couldn't trap the unbinding just like we trap the binding (at
the cost of an extra check of the `trapped' attribute).

In terms of debugging, I think trapping let-binding and let-unbinding is
definitely useful, so I think it's worth the effort to try and catch
both cases.

> I should clarify that this only applies to dynamically-bound
> variables, lexical variables are never trapped.

Yes, that's normal and right.  Fundamentally, lexical variables are
anonymous, anyway.


        Stefan

Re: Lisp watchpoints

[Noam Postavsky]

On Sun, Nov 29, 2015 at 11:10 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Sun, 29 Nov 2015 09:12:34 -0500
>> From: Noam Postavsky <npostavs@users.sourceforge.net>
>> Cc: John Wiegley <jwiegley@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>>       Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
>>
>> > Ffuncall does all necessary checks.
>>
>> It would signal in case of error right? Currently the code is ignoring
>> bad watcher elements (including out of range integers), but perhaps
>> it's actually better to signal an error.
>
> No, I think it would be better to report an error, and then continue,
> perhaps after removing the watch.

I'm thinking to just call display-warning then (removing the element
looks like too much work to be worthwhile, having to modify the list
while iterating over it is kind of annoying).

I noticed that display_malloc_warning passes intern ("emergency") as
the LEVEL, but it should be intern (":emergency") (with a colon),
right?

Re: Lisp watchpoints

[Stefan Monnier]

>>> > Ffuncall does all necessary checks.
>>> 
>>> It would signal in case of error right? Currently the code is ignoring
>>> bad watcher elements (including out of range integers), but perhaps
>>> it's actually better to signal an error.
>> 
>> No, I think it would be better to report an error, and then continue,
>> perhaps after removing the watch.

> I'm thinking to just call display-warning then (removing the element
> looks like too much work to be worthwhile, having to modify the list
> while iterating over it is kind of annoying).

I don't think we should do anything different for the case where
Ffuncall signals an error because  the object is not a function than for
the case where the object *is* a function but its execution signals some
kind of error.

And the function may want to signal `setting-constant', so I think we're
better off just not trying to catch an error there at all.


        Stefan

Re: Lisp watchpoints

[Eli Zaretskii]

> Date: Sun, 29 Nov 2015 19:57:05 -0500
> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Cc: Andreas Schwab <schwab@linux-m68k.org>, John Wiegley <jwiegley@gmail.com>, 
> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> I noticed that display_malloc_warning passes intern ("emergency") as
> the LEVEL, but it should be intern (":emergency") (with a colon),
> right?

Both will work (see warning-level-aliases), but it's indeed better to
use :emergency, as the other kind is only for XEmacs compatibility.

Lisp watchpoints

[Kaushal Modi]

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

Hi Noam,

This is awesome! Thanks! I will be trying this new feature out soon!

http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=88fefc3291060f18503738aaa4e81b98f1970a55

+++
+** New function `add-variable-watcher' can be used to call a function
+when a symbol's value is changed.  This is used to implement the new
+debugger command `debug-on-variable-change'.
+
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 743 bytes --]