Buffer-/frame-local variables [Was: Re: Make buffer- and frame-locals a misc object]

[Dmitry Antipov <dmantipov@yandex.ru>]

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

On 08/15/2012 08:02 PM, Stefan Monnier wrote:

>> For 1), my previous (and inglorious) attempt to hack around
>> save-excursion shows that mixing explicitly allocated/freed objects
>> with GC-managed objects is poor idea, so getting rid of xmalloc/xfree
>> makes the things more predictable.
>
> Fear of the unknown is not a good motivation for a change ;-)
> Have you found out what was the problem?

The problem was that the marker embedded into struct Lisp_Excursion
can be linked to buffer's undo list; when struct Lisp_Excursion gets
xfree'd, the marker pointer from undo list becomes dangling.

> But in any case the current use of xmalloc/xfree for Lisp_Buffer_Local_Value
> doesn't show any sign of suffering from a similar problem.

OK

>> For 2), the usual purpose is to prepare the hooks for the write barrier.
>
> Why have get_blv_value, since it's not needed for a write barrier?
> The set_blv_* are OK, OTOH.

OK. So please consider attached patch for the trunk.

> I do wonder, tho: do we need those write-barriers in the object-creation
> function (e.g. make_buffer_local_value)?

In general, no, since all just allocated objects are new by definition,
and write barrier action is raised only if the pointer to new object is
stored into an old one. So, the write barrier is installed but never
raised; I remember about this, and, of course, such a cases will be
optimized away.

Dmitry

[-- Attachment #2: blv.patch --]
[-- Type: text/plain, Size: 8001 bytes --]

=== modified file 'src/data.c'
--- src/data.c	2012-08-14 17:10:38 +0000
+++ src/data.c	2012-08-16 03:47:46 +0000
@@ -543,7 +543,7 @@
 	else
 	  {
 	    swap_in_symval_forwarding (sym, blv);
-	    valcontents = BLV_VALUE (blv);
+	    valcontents = XCDR (blv->valcell);
 	  }
 	break;
       }
@@ -964,16 +964,16 @@
 
   /* Unload the previously loaded binding.  */
   if (blv->fwd)
-    SET_BLV_VALUE (blv, do_symval_forwarding (blv->fwd));
+    set_blv_value (blv, do_symval_forwarding (blv->fwd));
 
   /* Select the global binding in the symbol.  */
-  blv->valcell = blv->defcell;
+  set_blv_valcell (blv, blv->defcell);
   if (blv->fwd)
     store_symval_forwarding (blv->fwd, XCDR (blv->defcell), NULL);
 
   /* Indicate that the global binding is set up now.  */
-  blv->where = Qnil;
-  SET_BLV_FOUND (blv, 0);
+  set_blv_where (blv, Qnil);
+  set_blv_found (blv, 0);
 }
 
 /* Set up the buffer-local symbol SYMBOL for validity in the current buffer.
@@ -1001,7 +1001,7 @@
       /* Unload the previously loaded binding.  */
       tem1 = blv->valcell;
       if (blv->fwd)
-	SET_BLV_VALUE (blv, do_symval_forwarding (blv->fwd));
+	set_blv_value (blv, do_symval_forwarding (blv->fwd));
       /* Choose the new binding.  */
       {
 	Lisp_Object var;
@@ -1009,7 +1009,7 @@
 	if (blv->frame_local)
 	  {
 	    tem1 = assq_no_quit (var, XFRAME (selected_frame)->param_alist);
-	    blv->where = selected_frame;
+	    set_blv_where (blv, selected_frame);
 	  }
 	else
 	  {
@@ -1021,9 +1021,9 @@
 	tem1 = blv->defcell;
 
       /* Load the new binding.  */
-      blv->valcell = tem1;
+      set_blv_valcell (blv, tem1);
       if (blv->fwd)
-	store_symval_forwarding (blv->fwd, BLV_VALUE (blv), NULL);
+	store_symval_forwarding (blv->fwd, XCDR (blv->valcell), NULL);
     }
 }
 \f
@@ -1050,7 +1050,7 @@
       {
 	struct Lisp_Buffer_Local_Value *blv = SYMBOL_BLV (sym);
 	swap_in_symval_forwarding (sym, blv);
-	return blv->fwd ? do_symval_forwarding (blv->fwd) : BLV_VALUE (blv);
+	return blv->fwd ? do_symval_forwarding (blv->fwd) : XCDR (blv->valcell);
       }
       /* FALLTHROUGH */
     case SYMBOL_FORWARDED:
@@ -1175,7 +1175,7 @@
 
 	    /* Write out `realvalue' to the old loaded binding.  */
 	    if (blv->fwd)
-	      SET_BLV_VALUE (blv, do_symval_forwarding (blv->fwd));
+	      set_blv_value (blv, do_symval_forwarding (blv->fwd));
 
 	    /* Find the new binding.  */
 	    XSETSYMBOL (symbol, sym); /* May have changed via aliasing.  */
@@ -1183,8 +1183,8 @@
 			  (blv->frame_local
 			   ? XFRAME (where)->param_alist
 			   : BVAR (XBUFFER (where), local_var_alist)));
-	    blv->where = where;
-	    blv->found = 1;
+	    set_blv_where (blv, where);
+	    set_blv_found (blv, 1);
 
 	    if (NILP (tem1))
 	      {
@@ -1199,7 +1199,7 @@
 		if (bindflag || !blv->local_if_set
 		    || let_shadows_buffer_binding_p (sym))
 		  {
-		    blv->found = 0;
+		    set_blv_found (blv, 0);
 		    tem1 = blv->defcell;
 		  }
 		/* If it's a local_if_set, being set not bound,
@@ -1219,11 +1219,11 @@
 	      }
 
 	    /* Record which binding is now loaded.  */
-	    blv->valcell = tem1;
+	    set_blv_valcell (blv, tem1);
 	  }
 
 	/* Store the new value in the cons cell.  */
-	SET_BLV_VALUE (blv, newval);
+	set_blv_value (blv, newval);
 
 	if (blv->fwd)
 	  {
@@ -1479,12 +1479,12 @@
   eassert (!(forwarded && BUFFER_OBJFWDP (valcontents.fwd)));
   eassert (!(forwarded && KBOARD_OBJFWDP (valcontents.fwd)));
   blv->fwd = forwarded ? valcontents.fwd : NULL;
-  blv->where = Qnil;
+  set_blv_where (blv, Qnil);
   blv->frame_local = 0;
   blv->local_if_set = 0;
-  blv->defcell = tem;
-  blv->valcell = tem;
-  SET_BLV_FOUND (blv, 0);
+  set_blv_defcell (blv, tem);
+  set_blv_valcell (blv, tem);
+  set_blv_found (blv, 0);
   return blv;
 }
 
@@ -1660,10 +1660,8 @@
       /* Make sure symbol does not think it is set up for this buffer;
 	 force it to look once again for this buffer's value.  */
       if (current_buffer == XBUFFER (blv->where))
-	blv->where = Qnil;
-      /* blv->valcell = blv->defcell;
-       * SET_BLV_FOUND (blv, 0); */
-      blv->found = 0;
+	set_blv_where (blv, Qnil);
+      set_blv_found (blv, 0);
     }
 
   /* If the symbol forwards into a C variable, then load the binding
@@ -1733,10 +1731,8 @@
     Lisp_Object buf; XSETBUFFER (buf, current_buffer);
     if (EQ (buf, blv->where))
       {
-	blv->where = Qnil;
-	/* blv->valcell = blv->defcell;
-	 * SET_BLV_FOUND (blv, 0); */
-	blv->found = 0;
+	set_blv_where (blv, Qnil);
+	set_blv_found (blv, 0);
 	find_symbol_value (variable);
       }
   }
@@ -1857,11 +1853,11 @@
 	    if (EQ (variable, XCAR (elt)))
 	      {
 		eassert (!blv->frame_local);
-		eassert (BLV_FOUND (blv) || !EQ (blv->where, tmp));
+		eassert (get_blv_found (blv) || !EQ (blv->where, tmp));
 		return Qt;
 	      }
 	  }
-	eassert (!BLV_FOUND (blv) || !EQ (blv->where, tmp));
+	eassert (!get_blv_found (blv) || !EQ (blv->where, tmp));
 	return Qnil;
       }
     case SYMBOL_FORWARDED:
@@ -1951,7 +1947,7 @@
       if (!NILP (Flocal_variable_p (variable, Qnil)))
 	return Fcurrent_buffer ();
       else if (sym->redirect == SYMBOL_LOCALIZED
-	       && BLV_FOUND (SYMBOL_BLV (sym)))
+	       && get_blv_found (SYMBOL_BLV (sym)))
 	return SYMBOL_BLV (sym)->where;
       else
 	return Qnil;

=== modified file 'src/eval.c'
--- src/eval.c	2012-08-16 01:18:07 +0000
+++ src/eval.c	2012-08-16 03:45:47 +0000
@@ -3169,12 +3169,12 @@
 	    if (!NILP (Flocal_variable_p (symbol, Qnil)))
 	      {
 		eassert (sym->redirect != SYMBOL_LOCALIZED
-			 || (BLV_FOUND (SYMBOL_BLV (sym))
+			 || (get_blv_found (SYMBOL_BLV (sym))
 			     && EQ (cur_buf, SYMBOL_BLV (sym)->where)));
 		where = cur_buf;
 	      }
 	    else if (sym->redirect == SYMBOL_LOCALIZED
-		     && BLV_FOUND (SYMBOL_BLV (sym)))
+		     && get_blv_found (SYMBOL_BLV (sym)))
 	      where = SYMBOL_BLV (sym)->where;
 	    else
 	      where = Qnil;

=== modified file 'src/frame.c'
--- src/frame.c	2012-08-14 08:44:24 +0000
+++ src/frame.c	2012-08-16 03:45:47 +0000
@@ -2096,7 +2096,7 @@
 	case SYMBOL_PLAINVAL: case SYMBOL_FORWARDED: break;
 	case SYMBOL_LOCALIZED:
 	  { struct Lisp_Buffer_Local_Value *blv = sym->val.blv;
-	    if (blv->frame_local && BLV_FOUND (blv) && XFRAME (blv->where) == f)
+	    if (blv->frame_local && get_blv_found (blv) && XFRAME (blv->where) == f)
 	      swap_in_global_binding (sym);
 	    break;
 	  }

=== modified file 'src/lisp.h'
--- src/lisp.h	2012-08-16 03:13:44 +0000
+++ src/lisp.h	2012-08-16 03:48:23 +0000
@@ -1472,14 +1472,6 @@
     Lisp_Object valcell;
   };
 
-#define BLV_FOUND(blv) \
-  (eassert ((blv)->found == !EQ ((blv)->defcell, (blv)->valcell)), (blv)->found)
-#define SET_BLV_FOUND(blv, v) \
-  (eassert ((v) == !EQ ((blv)->defcell, (blv)->valcell)), (blv)->found = (v))
-
-#define BLV_VALUE(blv) (XCDR ((blv)->valcell))
-#define SET_BLV_VALUE(blv, v) (XSETCDR ((blv)->valcell, v))
-
 /* Like Lisp_Objfwd except that value lives in a slot in the
    current kboard.  */
 struct Lisp_Kboard_Objfwd
@@ -2412,6 +2404,46 @@
   XSYMBOL (sym)->next = next;
 }
 
+/* Buffer-local (also frame-local) variable access functions.  */
+
+LISP_INLINE int
+get_blv_found (struct Lisp_Buffer_Local_Value *blv)
+{
+  eassert (blv->found == !EQ (blv->defcell, blv->valcell));
+  return blv->found;
+}
+
+LISP_INLINE void
+set_blv_found (struct Lisp_Buffer_Local_Value *blv, int found)
+{
+  eassert (found == !EQ (blv->defcell, blv->valcell));
+  blv->found = found;
+}
+
+LISP_INLINE void
+set_blv_value (struct Lisp_Buffer_Local_Value *blv, Lisp_Object val)
+{
+  XSETCDR (blv->valcell, val);
+}
+
+LISP_INLINE void
+set_blv_where (struct Lisp_Buffer_Local_Value *blv, Lisp_Object val)
+{
+  blv->where = val;
+}
+
+LISP_INLINE void
+set_blv_defcell (struct Lisp_Buffer_Local_Value *blv, Lisp_Object val)
+{
+  blv->defcell = val;
+}
+
+LISP_INLINE void
+set_blv_valcell (struct Lisp_Buffer_Local_Value *blv, Lisp_Object val)
+{
+  blv->valcell = val;
+}
+
 /* Set overlay's property list.  */
 
 LISP_INLINE void