Make buffer- and frame-locals a misc object

Make buffer- and frame-locals a misc object

[Dmitry Antipov]

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

This patch converts Lisp_Buffer_Local_Value to misc object (for the sake of
GC-aware management) and provides simple inline access functions (for the sake
of further GC development).

Dmitry

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

=== modified file 'src/alloc.c'
--- src/alloc.c	2012-08-14 21:38:06 +0000
+++ src/alloc.c	2012-08-15 10:45:33 +0000
@@ -3697,7 +3697,21 @@
   free_misc (marker);
 }
 
-\f
+/* Return partially initialized buffer- or frame-local value.  */
+
+struct Lisp_Buffer_Local_Value *
+make_buffer_local_value (void)
+{
+  struct Lisp_Buffer_Local_Value *blv;
+
+  blv = XBLV (allocate_misc (Lisp_Misc_Buffer_Local_Value));
+  blv->frame_local = 0;
+  blv->local_if_set = 0;
+  set_blv_where (blv, Qnil);
+  set_blv_found (blv, 0);
+  return blv;
+}
+
 /* Return a newly created vector or string with specified arguments as
    elements.  If all the arguments are characters that can fit
    in a string of events, make a string; otherwise, make a vector.
@@ -6070,14 +6084,13 @@
 	    }
 	  case SYMBOL_LOCALIZED:
 	    {
-	      struct Lisp_Buffer_Local_Value *blv = SYMBOL_BLV (ptr);
+	      Lisp_Object tem;
+	      XSETMISC (tem, SYMBOL_BLV (ptr));
 	      /* If the value is forwarded to a buffer or keyboard field,
 		 these are marked when we see the corresponding object.
 		 And if it's forwarded to a C variable, either it's not
 		 a Lisp_Object var, or it's staticpro'd already.  */
-	      mark_object (blv->where);
-	      mark_object (blv->valcell);
-	      mark_object (blv->defcell);
+	      mark_object (tem);
 	      break;
 	    }
 	  case SYMBOL_FORWARDED:
@@ -6139,6 +6152,13 @@
 	  mark_overlay (XOVERLAY (obj));
 	  break;
 
+	case Lisp_Misc_Buffer_Local_Value:
+	  XMISCANY (obj)->gcmarkbit = 1;
+	  mark_object (XBLV (obj)->where);
+	  mark_object (XBLV (obj)->defcell);
+	  mark_object (XBLV (obj)->valcell);
+	  break;
+
 	default:
 	  abort ();
 	}
@@ -6460,8 +6480,6 @@
 
 	    if (!sym->s.gcmarkbit && !pure_p)
 	      {
-		if (sym->s.redirect == SYMBOL_LOCALIZED)
-		  xfree (SYMBOL_BLV (&sym->s));
 		sym->s.next = symbol_free_list;
 		symbol_free_list = &sym->s;
 #if GC_MARK_STACK

=== modified file 'src/data.c'
--- src/data.c	2012-08-14 17:10:38 +0000
+++ src/data.c	2012-08-15 10:39:00 +0000
@@ -543,7 +543,7 @@
 	else
 	  {
 	    swap_in_symval_forwarding (sym, blv);
-	    valcontents = BLV_VALUE (blv);
+	    valcontents = get_blv_value (blv);
 	  }
 	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, get_blv_value (blv), 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) : get_blv_value (blv);
       }
       /* 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)
 	  {
@@ -1465,26 +1465,21 @@
 static struct Lisp_Buffer_Local_Value *
 make_blv (struct Lisp_Symbol *sym, int forwarded, union Lisp_Val_Fwd valcontents)
 {
-  struct Lisp_Buffer_Local_Value *blv = xmalloc (sizeof *blv);
-  Lisp_Object symbol;
-  Lisp_Object tem;
+  struct Lisp_Buffer_Local_Value *blv = make_buffer_local_value ();
+  Lisp_Object symbol, tem;
 
- XSETSYMBOL (symbol, sym);
- tem = Fcons (symbol, (forwarded
-                       ? do_symval_forwarding (valcontents.fwd)
-                       : valcontents.value));
+  XSETSYMBOL (symbol, sym);
+  tem = Fcons (symbol, (forwarded
+			? do_symval_forwarding (valcontents.fwd)
+			: valcontents.value));
 
   /* Buffer_Local_Values cannot have as realval a buffer-local
      or keyboard-local forwarding.  */
   eassert (!(forwarded && BUFFER_OBJFWDP (valcontents.fwd)));
   eassert (!(forwarded && KBOARD_OBJFWDP (valcontents.fwd)));
   blv->fwd = forwarded ? valcontents.fwd : NULL;
-  blv->where = 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);
   return blv;
 }
 
@@ -1660,10 +1655,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 +1726,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 +1848,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 +1942,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-08 19:53:44 +0000
+++ src/eval.c	2012-08-15 10:11:49 +0000
@@ -3157,12 +3157,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-15 10:11:49 +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-14 17:45:25 +0000
+++ src/lisp.h	2012-08-15 10:43:09 +0000
@@ -280,6 +280,7 @@
     /* Currently floats are not a misc type,
        but let's define this in case we want to change that.  */
     Lisp_Misc_Float,
+    Lisp_Misc_Buffer_Local_Value,
     /* This is not a type code.  It is for range checking.  */
     Lisp_Misc_Limit
   };
@@ -515,10 +516,11 @@
 
 #define XMISC(a)	((union Lisp_Misc *) XUNTAG (a, Lisp_Misc))
 #define XMISCANY(a)	(eassert (MISCP (a)), &(XMISC (a)->u_any))
-#define XMISCTYPE(a)   (XMISCANY (a)->type)
+#define XMISCTYPE(a)	(XMISCANY (a)->type)
 #define XMARKER(a)	(eassert (MARKERP (a)), &(XMISC (a)->u_marker))
 #define XOVERLAY(a)	(eassert (OVERLAYP (a)), &(XMISC (a)->u_overlay))
 #define XSAVE_VALUE(a)	(eassert (SAVE_VALUEP (a)), &(XMISC (a)->u_save_value))
+#define XBLV(a) 	(eassert (BLVP (a)), &(XMISC (a)->u_blv))
 
 /* Forwarding object types.  */
 
@@ -1342,6 +1344,59 @@
     Lisp_Object plist;
   };
 
+/* struct Lisp_Buffer_Local_Value is used in a symbol value cell when
+   the symbol has buffer-local or frame-local bindings.  (Exception:
+   some buffer-local variables are built-in, with their values stored
+   in the buffer structure itself.  They are handled differently,
+   using struct Lisp_Buffer_Objfwd.)
+
+   The `realvalue' slot holds the variable's current value, or a
+   forwarding pointer to where that value is kept.  This value is the
+   one that corresponds to the loaded binding.  To read or set the
+   variable, you must first make sure the right binding is loaded;
+   then you can access the value in (or through) `realvalue'.
+
+   `buffer' and `frame' are the buffer and frame for which the loaded
+   binding was found.  If those have changed, to make sure the right
+   binding is loaded it is necessary to find which binding goes with
+   the current buffer and selected frame, then load it.  To load it,
+   first unload the previous binding, then copy the value of the new
+   binding into `realvalue' (or through it).  Also update
+   LOADED-BINDING to point to the newly loaded binding.
+
+   `local_if_set' indicates that merely setting the variable creates a
+   local binding for the current buffer.  Otherwise the latter, setting
+   the variable does not do that; only make-local-variable does that.  */
+
+struct Lisp_Buffer_Local_Value
+  {
+    ENUM_BF (Lisp_Misc_Type) type : 16; /* = Lisp_Misc_Buffer_Local_Value */
+    unsigned gcmarkbit : 1;
+    int spacer : 12;
+    /* 1 means that merely setting the variable creates a local
+       binding for the current buffer.  */
+    unsigned int local_if_set : 1;
+    /* 1 means this variable can have frame-local bindings, otherwise, it is
+       can have buffer-local bindings.  The two cannot be combined.  */
+    unsigned int frame_local : 1;
+    /* 1 means that the binding now loaded was found.
+       Presumably equivalent to (defcell!=valcell).  */
+    unsigned int found : 1;
+    /* If non-NULL, a forwarding to the C var where it should also be set.  */
+    union Lisp_Fwd *fwd;	/* Should never be (Buffer|Kboard)_Objfwd.  */
+    /* The buffer or frame for which the loaded binding was found.  */
+    Lisp_Object where;
+    /* A cons cell that holds the default value.  It has the form
+       (SYMBOL . DEFAULT-VALUE).  */
+    Lisp_Object defcell;
+    /* The cons cell from `where's parameter alist.
+       It always has the form (SYMBOL . VALUE)
+       Note that if `forward' is non-nil, VALUE may be out of date.
+       Also if the currently loaded binding is the default binding, then
+       this is `eq'ual to defcell.  */
+    Lisp_Object valcell;
+  };
+
 /* Hold a C pointer for later use.
    This type of object is used in the arg to record_unwind_protect.  */
 struct Lisp_Save_Value
@@ -1375,6 +1430,7 @@
     struct Lisp_Free u_free;
     struct Lisp_Marker u_marker;
     struct Lisp_Overlay u_overlay;
+    struct Lisp_Buffer_Local_Value u_blv;
     struct Lisp_Save_Value u_save_value;
   };
 
@@ -1417,64 +1473,6 @@
     Lisp_Object slottype; /* Qnil, Lisp_Int, Lisp_Symbol, or Lisp_String.  */
   };
 
-/* struct Lisp_Buffer_Local_Value is used in a symbol value cell when
-   the symbol has buffer-local or frame-local bindings.  (Exception:
-   some buffer-local variables are built-in, with their values stored
-   in the buffer structure itself.  They are handled differently,
-   using struct Lisp_Buffer_Objfwd.)
-
-   The `realvalue' slot holds the variable's current value, or a
-   forwarding pointer to where that value is kept.  This value is the
-   one that corresponds to the loaded binding.  To read or set the
-   variable, you must first make sure the right binding is loaded;
-   then you can access the value in (or through) `realvalue'.
-
-   `buffer' and `frame' are the buffer and frame for which the loaded
-   binding was found.  If those have changed, to make sure the right
-   binding is loaded it is necessary to find which binding goes with
-   the current buffer and selected frame, then load it.  To load it,
-   first unload the previous binding, then copy the value of the new
-   binding into `realvalue' (or through it).  Also update
-   LOADED-BINDING to point to the newly loaded binding.
-
-   `local_if_set' indicates that merely setting the variable creates a
-   local binding for the current buffer.  Otherwise the latter, setting
-   the variable does not do that; only make-local-variable does that.  */
-
-struct Lisp_Buffer_Local_Value
-  {
-    /* 1 means that merely setting the variable creates a local
-       binding for the current buffer.  */
-    unsigned int local_if_set : 1;
-    /* 1 means this variable can have frame-local bindings, otherwise, it is
-       can have buffer-local bindings.  The two cannot be combined.  */
-    unsigned int frame_local : 1;
-    /* 1 means that the binding now loaded was found.
-       Presumably equivalent to (defcell!=valcell).  */
-    unsigned int found : 1;
-    /* If non-NULL, a forwarding to the C var where it should also be set.  */
-    union Lisp_Fwd *fwd;	/* Should never be (Buffer|Kboard)_Objfwd.  */
-    /* The buffer or frame for which the loaded binding was found.  */
-    Lisp_Object where;
-    /* A cons cell that holds the default value.  It has the form
-       (SYMBOL . DEFAULT-VALUE).  */
-    Lisp_Object defcell;
-    /* The cons cell from `where's parameter alist.
-       It always has the form (SYMBOL . VALUE)
-       Note that if `forward' is non-nil, VALUE may be out of date.
-       Also if the currently loaded binding is the default binding, then
-       this is `eq'ual to defcell.  */
-    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
@@ -1664,6 +1662,7 @@
 #define OVERLAYP(x) (MISCP (x) && XMISCTYPE (x) == Lisp_Misc_Overlay)
 #define MARKERP(x) (MISCP (x) && XMISCTYPE (x) == Lisp_Misc_Marker)
 #define SAVE_VALUEP(x) (MISCP (x) && XMISCTYPE (x) == Lisp_Misc_Save_Value)
+#define BLVP(x) (MISCP (x) && XMISCTYPE (x) == Lisp_Misc_Buffer_Local_Value)
 
 #define INTFWDP(x) (XFWDTYPE (x) == Lisp_Fwd_Int)
 #define BOOLFWDP(x) (XFWDTYPE (x) == Lisp_Fwd_Bool)
@@ -2407,6 +2406,53 @@
   XSYMBOL (sym)->next = next;
 }
 
+/* Use these functions to get and set Lisp_Object
+   slots of struct Lisp_Buffer_Local_Value.  */
+
+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 Lisp_Object
+get_blv_value (struct Lisp_Buffer_Local_Value *blv)
+{
+  return XCDR (blv->valcell);
+}
+
+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
@@ -2802,6 +2848,7 @@
 extern void display_malloc_warning (void);
 extern ptrdiff_t inhibit_garbage_collection (void);
 extern Lisp_Object make_save_value (void *, ptrdiff_t);
+extern struct Lisp_Buffer_Local_Value * make_buffer_local_value (void);
 extern Lisp_Object build_overlay (Lisp_Object, Lisp_Object, Lisp_Object);
 extern void free_marker (Lisp_Object);
 extern void free_cons (struct Lisp_Cons *);

Re: Make buffer- and frame-locals a misc object

[Stefan Monnier]

> This patch converts Lisp_Buffer_Local_Value to misc object (for the
> sake of GC-aware management) and provides simple inline access
> functions (for the sake of further GC development).

No, that's going backward.
Please explain why you think you need that, so we can find alternatives.


        Stefan

Re: Make buffer- and frame-locals a misc object

[Dmitry Antipov]

On 08/15/2012 06:20 PM, Stefan Monnier wrote:

>> This patch converts Lisp_Buffer_Local_Value to misc object (for the
>> sake of GC-aware management) and provides simple inline access
>> functions (for the sake of further GC development).
>
> No, that's going backward.
> Please explain why you think you need that, so we can find alternatives.

This patch 1) converts Lisp_Buffer_Local_Value to misc object (for the
sake of GC-aware management) and 2) provides simple inline access
functions (for the sake of further GC development).

What's going backward - 1) or 2) or both?

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.

For 2), the usual purpose is to prepare the hooks for the write barrier.

Dmitry

Re: Make buffer- and frame-locals a misc object

[Stefan Monnier]

>>> This patch converts Lisp_Buffer_Local_Value to misc object (for the
>>> sake of GC-aware management) and provides simple inline access
>>> functions (for the sake of further GC development).
>> No, that's going backward.
>> Please explain why you think you need that, so we can find alternatives.

> This patch 1) converts Lisp_Buffer_Local_Value to misc object (for the
> sake of GC-aware management) and 2) provides simple inline access
> functions (for the sake of further GC development).

> What's going backward - 1) or 2) or both?

1) is going backward.

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

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.

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

I do wonder, tho: do we need those write-barriers in the object-creation
function (e.g. make_buffer_local_value)?  After all, if GC happens
between the malloc and the end of the initialization, we're in trouble
anyway, right?


        Stefan

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

[Dmitry Antipov]

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

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

[Stefan Monnier]

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

So, the problem is not that the Lisp_Excursion used malloc/free, but
that the marker used malloc/free (implicitly by being embedded in the
Lisp_Excursion) although it can get captured by other pointers.

> OK. So please consider attached patch for the trunk.

See comments below.

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

Another reason is that until they're initialized, the fields contain
invalid values, so if the GC sees them we're in trouble, right?

-	    valcontents = BLV_VALUE (blv);
+	    valcontents = XCDR (blv->valcell);

Please don't: BLV_VALUE is more clear and abstract (same applies to
other places where you replace BLV_VALUE with XCDR (blv->valcell)).

-#define BLV_FOUND(blv) \
+get_blv_found (struct Lisp_Buffer_Local_Value *blv)

Why add a "get_" prefix?
Elisp and Emacs generally uses "<type>-<field>" for accessors and
"set-<type>-<field>" for setters.


        Stefan

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

[Dmitry Antipov]

On 08/17/2012 05:20 PM, Stefan Monnier wrote:

> Another reason is that until they're initialized, the fields contain
> invalid values, so if the GC sees them we're in trouble, right?

Not quite.  In our terms, the core thing for simple generational GC is:

struct Lisp_Foo
{
   Lisp_Object bar;
   Lisp_Object baz;
};

void
foo_set_bar (Lisp_Object foo, Lisp_Object bar)
{
   if (object_generation (foo) == OLD && object_generation (bar) == NEW)
     /* This function should record all pointer-type Lisp_Objects
        from new generation which are accessible from old generation.
        Such objects will be considered as a part of the root set at
        the next generational collection.  */
     record_intergenerational_object (bar);
   XFOO (foo)->bar = bar;
}

/* Likewise for foo_set_baz.  */

Lisp_Object
make_foo (Lisp_Object bar, Lisp_Object baz)
{
   Lisp_Object foo = allocate_foo ();    /* At this point, object_generation (foo) is always NEW.  */
   foo_set_bar (foo, bar);               /* Since foo is NEW, barrier action isn't raised whatever bar is;
                                            don't care about old (uninitialized) XFOO (foo)->bar here.  */
   foo_set_baz (foo, baz);               /* Likewise.  */
   return foo;
}

>
> -	    valcontents = BLV_VALUE (blv);
> +	    valcontents = XCDR (blv->valcell);
>
> Please don't: BLV_VALUE is more clear and abstract (same applies to
> other places where you replace BLV_VALUE with XCDR (blv->valcell)).
>
> -#define BLV_FOUND(blv) \
> +get_blv_found (struct Lisp_Buffer_Local_Value *blv)
>
> Why add a "get_" prefix?
> Elisp and Emacs generally uses "<type>-<field>" for accessors and
> "set-<type>-<field>" for setters.

Fixed and installed as 109660.

Dmitry

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

[Paul Eggert]

On 08/17/2012 06:20 AM, Stefan Monnier wrote:

> Elisp and Emacs generally uses "<type>-<field>" for accessors and
> "set-<type>-<field>" for setters.

As Dmitry didn't know that, he recently added several C functions
that didn't follow that naming convention.  I just now changed
three of their names to use the convention, in trunk bzr 109663.
(e.g., sub_char_table_set_contents -> set_sub_char_table_contents).
I assume there's no objection if the other names are changed similarly.

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

[Stefan Monnier]

>> Another reason is that until they're initialized, the fields contain
>> invalid values, so if the GC sees them we're in trouble, right?
> Not quite.  In our terms, the core thing for simple generational GC is:

I didn't mean to say that a write-barrier during initialization is
a bug, only that it can't be necessary since the object is not "GC
ready" yet.


        Stefan