Subject: revised patch for Bug #11604

From: Paul Eggert <[email-masked]>
Date: Sat, 02 Jun 2012 23:54:35 -0700

Here is a revised patch for Bug#11604.
It attempts to address all the comments raised.
Thanks to everybody who reviewed the earlier patch.

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-06-02 21:01:07 +0000
+++ src/ChangeLog	2012-06-03 06:39:29 +0000
@@ -1,3 +1,26 @@
+2012-06-03  Paul Eggert  <[email-masked]>
+
+	USE_LISP_UNION_TYPE + USE_LSB_TAG cleanup (Bug#11604)
+	* alloc.c (make_number) [!defined make_number]:
+	Remove, as lisp.h always defines this now.
+	(mark_maybe_pointer): Simplify since USE_LSB_TAG is always defined now.
+	* data.c (Fmake_variable_buffer_local, Fmake_local_variable):
+	* ftfont.c (ftfont_driver): Use LISP_INITIALLY_ZERO.
+	* lisp.h (USE_LSB_TAG): Allow the builder to compile with
+	-DUSE_LSB_TAG=0, to override the automatically-selected default.
+	USE_LSB_TAG now is always defined to be either 0 or 1.
+	All uses changed.
+	(union Lisp_Object): Don't worry about WORDS_BIGENDIAN; the
+	code works fine either way, and efficiency is not a concern here,
+	as the union type is for debugging, not for production.
+	(LISP_MAKE_RVALUE, make_number) [USE_LISP_UNION_TYPE]:
+	Use an inline function on all platforms when using the union type,
+	since this is simpler and 'static inline' can be used portably
+	within Emacs now.
+	(LISP_INITIALLY_ZERO): New macro.
+	(XFASTINT, XSETFASTINT) [USE_LISP_UNION_TYPE]: Remove.
+	(XSET) [USE_LISP_UNION_TYPE]: Don't overparenthesize.
+
 2012-06-02  Paul Eggert  <[email-masked]>
 
 	* sysdep.c (system_process_attributes): Improve comment.

=== modified file 'src/alloc.c'
--- src/alloc.c	2012-06-02 08:52:27 +0000
+++ src/alloc.c	2012-06-02 15:19:06 +0000
@@ -525,7 +525,7 @@
       char c;						\
     },							\
     c)
-#ifdef USE_LSB_TAG
+#if USE_LSB_TAG
 /* A common multiple of the positive integers A and B.  Ideally this
    would be the least common multiple, but there's no way to do that
    as a constant expression in C, so do the best that we can easily do.  */
@@ -890,8 +890,8 @@
    number of bytes to allocate, TYPE describes the intended use of the
    allocated memory block (for strings, for conses, ...).  */
 
-#ifndef USE_LSB_TAG
-static void *lisp_malloc_loser;
+#if ! USE_LSB_TAG
+void *lisp_malloc_loser EXTERNALLY_VISIBLE;
 #endif
 
 static void *
@@ -907,7 +907,7 @@
 
   val = (void *) malloc (nbytes);
 
-#ifndef USE_LSB_TAG
+#if ! USE_LSB_TAG
   /* If the memory just allocated cannot be addressed thru a Lisp
      object's pointer, and it needs to be,
      that's equivalent to running out of memory.  */
@@ -1088,7 +1088,7 @@
       mallopt (M_MMAP_MAX, MMAP_MAX_AREAS);
 #endif
 
-#ifndef USE_LSB_TAG
+#if ! USE_LSB_TAG
       /* If the memory just allocated cannot be addressed thru a Lisp
 	 object's pointer, and it needs to be, that's equivalent to
 	 running out of memory.  */
@@ -1581,20 +1581,6 @@
    if (! NULL_INTERVAL_P (i))				\
      (i) = balance_intervals (i);			\
   } while (0)
-
-
-/* Number support.  If USE_LISP_UNION_TYPE is in effect, we
-   can't create number objects in macros.  */
-#ifndef make_number
-Lisp_Object
-make_number (EMACS_INT n)
-{
-  Lisp_Object obj;
-  obj.s.val = n;
-  obj.s.type = Lisp_Int;
-  return obj;
-}
-#endif
 
 /* Convert the pointer-sized word P to EMACS_INT while preserving its
    type and ptr fields.  */
@@ -3158,7 +3144,7 @@
 union aligned_Lisp_Symbol
 {
   struct Lisp_Symbol s;
-#ifdef USE_LSB_TAG
+#if USE_LSB_TAG
   unsigned char c[(sizeof (struct Lisp_Symbol) + (1 << GCTYPEBITS) - 1)
 		  & -(1 << GCTYPEBITS)];
 #endif
@@ -3264,7 +3250,7 @@
 union aligned_Lisp_Misc
 {
   union Lisp_Misc m;
-#ifdef USE_LSB_TAG
+#if USE_LSB_TAG
   unsigned char c[(sizeof (union Lisp_Misc) + (1 << GCTYPEBITS) - 1)
 		  & -(1 << GCTYPEBITS)];
 #endif
@@ -4219,14 +4205,10 @@
 {
   struct mem_node *m;
 
-  /* Quickly rule out some values which can't point to Lisp data.  */
-  if ((intptr_t) p %
-#ifdef USE_LSB_TAG
-      8 /* USE_LSB_TAG needs Lisp data to be aligned on multiples of 8.  */
-#else
-      2 /* We assume that Lisp data is aligned on even addresses.  */
-#endif
-      )
+  /* Quickly rule out some values which can't point to Lisp data.
+     USE_LSB_TAG needs Lisp data to be aligned on multiples of 8.
+     Otherwise, assume that Lisp data is aligned on even addresses.  */
+  if ((intptr_t) p % (USE_LSB_TAG ? 8 : 2))
     return;
 
   m = mem_find (p);
@@ -4301,8 +4283,8 @@
    wider than a pointer might allocate a Lisp_Object in non-adjacent halves.
    If USE_LSB_TAG, the bottom half is not a valid pointer, but it should
    suffice to widen it to to a Lisp_Object and check it that way.  */
-#if defined USE_LSB_TAG || VAL_MAX < UINTPTR_MAX
-# if !defined USE_LSB_TAG && VAL_MAX < UINTPTR_MAX >> GCTYPEBITS
+#if USE_LSB_TAG || VAL_MAX < UINTPTR_MAX
+# if !USE_LSB_TAG && VAL_MAX < UINTPTR_MAX >> GCTYPEBITS
   /* If tag bits straddle pointer-word boundaries, neither mark_maybe_pointer
      nor mark_maybe_object can follow the pointers.  This should not occur on
      any practical porting target.  */
@@ -4730,7 +4712,7 @@
 pure_alloc (size_t size, int type)
 {
   void *result;
-#ifdef USE_LSB_TAG
+#if USE_LSB_TAG
   size_t alignment = (1 << GCTYPEBITS);
 #else
   size_t alignment = sizeof (EMACS_INT);

=== modified file 'src/data.c'
--- src/data.c	2012-05-30 03:59:42 +0000
+++ src/data.c	2012-06-01 23:34:07 +0000
@@ -1500,7 +1500,7 @@
 {
   struct Lisp_Symbol *sym;
   struct Lisp_Buffer_Local_Value *blv = NULL;
-  union Lisp_Val_Fwd valcontents IF_LINT (= {0});
+  union Lisp_Val_Fwd valcontents IF_LINT (= {LISP_INITIALLY_ZERO});
   int forwarded IF_LINT (= 0);
 
   CHECK_SYMBOL (variable);
@@ -1577,7 +1577,7 @@
 {
   register Lisp_Object tem;
   int forwarded IF_LINT (= 0);
-  union Lisp_Val_Fwd valcontents IF_LINT (= {0});
+  union Lisp_Val_Fwd valcontents IF_LINT (= {LISP_INITIALLY_ZERO});
   struct Lisp_Symbol *sym;
   struct Lisp_Buffer_Local_Value *blv = NULL;

=== modified file 'src/emacs.c'
--- src/emacs.c	2012-05-30 19:23:37 +0000
+++ src/emacs.c	2012-06-01 23:34:07 +0000
@@ -104,7 +104,7 @@
 
 /* Make these values available in GDB, which doesn't see macros.  */
 
-#ifdef USE_LSB_TAG
+#if USE_LSB_TAG
 int gdb_use_lsb EXTERNALLY_VISIBLE = 1;
 #else
 int gdb_use_lsb EXTERNALLY_VISIBLE = 0;
@@ -116,7 +116,7 @@
 #endif
 int gdb_valbits EXTERNALLY_VISIBLE = VALBITS;
 int gdb_gctypebits EXTERNALLY_VISIBLE = GCTYPEBITS;
-#if defined (DATA_SEG_BITS) && ! defined (USE_LSB_TAG)
+#if defined DATA_SEG_BITS && !USE_LSB_TAG
 uintptr_t gdb_data_seg_bits EXTERNALLY_VISIBLE = DATA_SEG_BITS;
 #else
 uintptr_t gdb_data_seg_bits EXTERNALLY_VISIBLE = 0;

=== modified file 'src/frame.c'
--- src/frame.c	2012-06-01 03:41:03 +0000
+++ src/frame.c	2012-06-01 23:34:07 +0000
@@ -1152,10 +1152,6 @@
   described for Fdelete_frame.  */
 Lisp_Object
 delete_frame (Lisp_Object frame, Lisp_Object force)
-     /* If we use `register' here, gcc-4.0.2 on amd64 using
-	-DUSE_LISP_UNION_TYPE complains further down that we're getting the
-	address of `force'.  Go figure.  */
-
 {
   struct frame *f;
   struct frame *sf = SELECTED_FRAME ();

=== modified file 'src/ftfont.c'
--- src/ftfont.c	2012-04-09 22:54:59 +0000
+++ src/ftfont.c	2012-06-01 23:34:07 +0000
@@ -525,7 +525,7 @@
 
 struct font_driver ftfont_driver =
   {
-    0,				/* Qfreetype */
+    LISP_INITIALLY_ZERO,	/* Qfreetype */
     0,				/* case insensitive */
     ftfont_get_cache,
     ftfont_list,

=== modified file 'src/lisp.h'
--- src/lisp.h	2012-05-30 19:23:37 +0000
+++ src/lisp.h	2012-06-03 06:39:29 +0000
@@ -161,10 +161,8 @@
      always 0, and we can thus use them to hold tag bits, without
      restricting our addressing space.
 
-   If USE_LSB_TAG is not set, then we use the top 3 bits for tagging, thus
-   restricting our possible address range.  Currently USE_LSB_TAG is not
-   allowed together with a union.  This is not due to any fundamental
-   technical (or political ;-) problem: nobody wrote the code to do it yet.
+   If ! USE_LSB_TAG, then use the top 3 bits for tagging, thus
+   restricting our possible address range.
 
    USE_LSB_TAG not only requires the least 3 bits of pointers returned by
    malloc to be 0 but also needs to be able to impose a mult-of-8 alignment
@@ -201,25 +199,31 @@
 # endif
 #endif
 
-/* Let's USE_LSB_TAG on systems where we know malloc returns mult-of-8.  */
-#if (defined GNU_MALLOC || defined DOUG_LEA_MALLOC || defined __GLIBC__ \
-     || defined DARWIN_OS || defined __sun)
-/* We also need to be able to specify mult-of-8 alignment on static vars.  */
-# if defined DECL_ALIGN
-/* On hosts where pointers-as-ints do not exceed VAL_MAX,
-   USE_LSB_TAG is:
+/* Unless otherwise specified, use USE_LSB_TAG on systems where:  */
+#ifndef USE_LSB_TAG
+/* 1.  We know malloc returns a multiple of 8.  */
+# if (defined GNU_MALLOC || defined DOUG_LEA_MALLOC || defined __GLIBC__ \
+      || defined DARWIN_OS || defined __sun)
+/* 2.  We can specify multiple-of-8 alignment on static variables.  */
+#  ifdef DECL_ALIGN
+/* 3.  Pointers-as-ints exceed VAL_MAX.
+   On hosts where pointers-as-ints do not exceed VAL_MAX, USE_LSB_TAG is:
     a. unnecessary, because the top bits of an EMACS_INT are unused, and
     b. slower, because it typically requires extra masking.
-   So, define USE_LSB_TAG only on hosts where it might be useful.  */
-#  if VAL_MAX < UINTPTR_MAX
-#   define USE_LSB_TAG
+   So, default USE_LSB_TAG to 1 only on hosts where it might be useful.  */
+#   if VAL_MAX < UINTPTR_MAX
+#    define USE_LSB_TAG 1
+#   endif
 #  endif
 # endif
 #endif
+#ifndef USE_LSB_TAG
+# define USE_LSB_TAG 0
+#endif
 
 /* If we cannot use 8-byte alignment, make DECL_ALIGN a no-op.  */
 #ifndef DECL_ALIGN
-# ifdef USE_LSB_TAG
+# if USE_LSB_TAG
 #  error "USE_LSB_TAG used without defining DECL_ALIGN"
 # endif
 # define DECL_ALIGN(type, var) type var
@@ -248,7 +252,7 @@
 #else
 # define LISP_INT_TAG Lisp_Int0
 # define case_Lisp_Int case Lisp_Int0: case Lisp_Int1
-# ifdef USE_LSB_TAG
+# if USE_LSB_TAG
 #  define LISP_INT1_TAG 4
 #  define LISP_STRING_TAG 1
 #  define LISP_INT_TAG_P(x) (((x) & 3) == 0)
@@ -333,70 +337,35 @@
 
 #ifdef USE_LISP_UNION_TYPE
 
-#ifndef WORDS_BIGENDIAN
-
-/* Definition of Lisp_Object for little-endian machines.  */
-
-typedef
-union Lisp_Object
-  {
-    /* Used for comparing two Lisp_Objects;
-       also, positive integers can be accessed fast this way.  */
-    EMACS_INT i;
-
-    struct
-      {
-	/* Use explicit signed, the signedness of a bit-field of type
-	   int is implementation defined.  */
-	signed EMACS_INT val  : VALBITS;
-	ENUM_BF (Lisp_Type) type : GCTYPEBITS;
-      } s;
-    struct
-      {
-	EMACS_UINT val : VALBITS;
-	ENUM_BF (Lisp_Type) type : GCTYPEBITS;
-      } u;
-  }
-Lisp_Object;
-
-#else /* If WORDS_BIGENDIAN */
-
-typedef
-union Lisp_Object
-  {
-    /* Used for comparing two Lisp_Objects;
-       also, positive integers can be accessed fast this way.  */
-    EMACS_INT i;
-
-    struct
-      {
-	ENUM_BF (Lisp_Type) type : GCTYPEBITS;
-	/* Use explicit signed, the signedness of a bit-field of type
-	   int is implementation defined.  */
-	signed EMACS_INT val  : VALBITS;
-      } s;
-    struct
-      {
-	ENUM_BF (Lisp_Type) type : GCTYPEBITS;
-	EMACS_UINT val : VALBITS;
-      } u;
-  }
-Lisp_Object;
-
-#endif /* WORDS_BIGENDIAN */
-
-#ifdef __GNUC__
+typedef
+union Lisp_Object
+  {
+    /* Used for comparing two Lisp_Objects;
+       also, positive integers can be accessed fast this way.  */
+    EMACS_INT i;
+
+    struct
+      {
+	/* Use explicit signed, the signedness of a bit-field of type
+	   int is implementation defined.  */
+	signed EMACS_INT val  : VALBITS;
+	ENUM_BF (Lisp_Type) type : GCTYPEBITS;
+      } s;
+    struct
+      {
+	EMACS_UINT val : VALBITS;
+	ENUM_BF (Lisp_Type) type : GCTYPEBITS;
+      } u;
+  }
+Lisp_Object;
+
 static inline Lisp_Object
 LISP_MAKE_RVALUE (Lisp_Object o)
 {
     return o;
 }
-#else
-/* This is more portable to pre-C99 non-GCC compilers, but for
-   backwards compatibility GCC still accepts an old GNU extension
-   which caused this to only generate a warning.  */
-#define LISP_MAKE_RVALUE(o) (0 ? (o) : (o))
-#endif
+
+#define LISP_INITIALLY_ZERO {0}
 
 #else /* USE_LISP_UNION_TYPE */
 
@@ -404,6 +373,7 @@
 
 typedef EMACS_INT Lisp_Object;
 #define LISP_MAKE_RVALUE(o) (0+(o))
+#define LISP_INITIALLY_ZERO 0
 #endif /* USE_LISP_UNION_TYPE */
 
 /* In the size word of a vector, this bit means the vector has been marked.  */
@@ -467,7 +437,7 @@
 /* Return a perfect hash of the Lisp_Object representation.  */
 #define XHASH(a) (a)
 
-#ifdef USE_LSB_TAG
+#if USE_LSB_TAG
 
 #define TYPEMASK ((((EMACS_INT) 1) << GCTYPEBITS) - 1)
 #define XTYPE(a) ((enum Lisp_Type) ((a) & TYPEMASK))
@@ -542,12 +512,12 @@
 #define XINT(a) ((EMACS_INT) (a).s.val)
 #define XUINT(a) ((EMACS_UINT) (a).u.val)
 
-#ifdef USE_LSB_TAG
+#if USE_LSB_TAG
 
 # define XSET(var, vartype, ptr) \
-  (eassert ((((uintptr_t) (ptr)) & ((1 << GCTYPEBITS) - 1)) == 0),	\
-   (var).u.val = ((uintptr_t) (ptr)) >> GCTYPEBITS,			\
-   (var).u.type = ((char) (vartype)))
+  (eassert (((uintptr_t) (ptr) & ((1 << GCTYPEBITS) - 1)) == 0),	\
+   (var).u.val = (uintptr_t) (ptr) >> GCTYPEBITS,			\
+   (var).u.type = (vartype))
 
 /* Some versions of gcc seem to consider the bitfield width when issuing
    the "cast to pointer from integer of different size" warning, so the
@@ -556,14 +526,8 @@
 
 #else  /* !USE_LSB_TAG */
 
-/* For integers known to be positive, XFASTINT provides fast retrieval
-   and XSETFASTINT provides fast storage.  This takes advantage of the
-   fact that Lisp_Int is 0.  */
-# define XFASTINT(a) ((a).i + 0)
-# define XSETFASTINT(a, b) ((a).i = (b))
-
 # define XSET(var, vartype, ptr) \
-   (((var).s.val = ((intptr_t) (ptr))), ((var).s.type = ((char) (vartype))))
+   ((var).s.val = (intptr_t) (ptr), (var).s.type = (vartype))
 
 #ifdef DATA_SEG_BITS
 /* DATA_SEG_BITS forces extra bits to be or'd in with any pointers
@@ -575,12 +539,14 @@
 
 #endif	/* !USE_LSB_TAG */
 
-#if __GNUC__ >= 2 && defined (__OPTIMIZE__)
-#define make_number(N) \
-  (__extension__ ({ Lisp_Object _l; _l.s.val = (N); _l.s.type = Lisp_Int; _l; }))
-#else
-extern Lisp_Object make_number (EMACS_INT);
-#endif
+static inline Lisp_Object
+make_number (EMACS_INT n)
+{
+  Lisp_Object o;
+  o.s.val = n;
+  o.s.type = Lisp_Int;
+  return o;
+}
 
 #endif /* USE_LISP_UNION_TYPE */

=== modified file 'src/mem-limits.h'
--- src/mem-limits.h	2012-05-22 16:20:27 +0000
+++ src/mem-limits.h	2012-06-01 23:34:07 +0000
@@ -34,7 +34,7 @@
 #endif
 
 extern char *start_of_data (void);
-#if defined USE_LSB_TAG || UINTPTR_MAX <= VAL_MAX
+#if USE_LSB_TAG || UINTPTR_MAX <= VAL_MAX
 #define EXCEEDS_LISP_PTR(ptr) 0
 #elif defined DATA_SEG_BITS
 #define EXCEEDS_LISP_PTR(ptr) \

=== modified file 'src/xfont.c'
--- src/xfont.c	2012-02-10 18:58:48 +0000
+++ src/xfont.c	2012-06-01 23:34:07 +0000
@@ -132,7 +132,7 @@
 
 struct font_driver xfont_driver =
   {
-    0,				/* Qx */
+    LISP_INITIALLY_ZERO,	/* Qx */
     0,				/* case insensitive */
     xfont_get_cache,
     xfont_list,