Re: Enabling --enable-check-lisp-object-type by default on x86 and AMD64

[Paul Eggert <eggert@cs.ucla.edu>]

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

Originally, the Lisp_Object type was a struct containing bitfields. This 
was significantly slower than using integer shifting and masking on many 
platforms, so long ago the default Lisp_Object type was changed to be an 
integer. Nowadays Lisp_Object no longer contains bitfields even when 
--enable-check-lisp-object-type is used, so this old motivation is no 
longer relevant.

For x86-64 there should be little difference nowadays when 
--enable-check-lisp-object-type is used. For other platforms (e.g., 
x86), however, I suppose there can still be a significant (though small) 
difference. So when compiling emacs for production, it makes sense to 
omit --enable-check-lisp-object-type, for the benefit of x86 and similar 
platforms.

When developing, the advantages of --enable-check-lisp-object-type 
outweigh the small increase in runtime cost, particularly considering 
that x86-64 is the most common development platform nowadays and there 
the runtime cost is insignificant.  So it makes sense to default 
--enable-check-lisp-object-type to "yes" if --enable-gcc-warnings is 
also enabled (which it is by default, in developer builds). I did this 
by installing the attached patch; comments welcome.

As a result, if you build master from a developer (Git-based) directory, 
--enable-check-lisp-object-type should now be the default. If you don't 
build from Git, I suggest configuring with --enable-gcc-warnings.

[-- Attachment #2: 0001-Check-list-object-type-if-enable-gcc-warnings.txt --]
[-- Type: text/plain, Size: 8434 bytes --]

From f1550f70a1e139022e6238c3c5c22cb7bd6923a1 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 2 May 2017 14:52:21 -0700
Subject: [PATCH] Check list object type if --enable-gcc-warnings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* configure.ac (--enable-check-lisp-object-type):
Default to "yes" if --enable-gcc-warnings is not "no".
* etc/NEWS: Mention this.
* src/eval.c (internal_lisp_condition_case): Fix some glitches
with 'volatile' uncovered by the above: in particular, 'clauses'
should be a pointer to volatile storage on the stack, and need not
be volatile itself.  Use an int, not ptrdiff_t, to count clauses.
Don’t bother gathering binding count if VAR is nil.  Use
more-specific local names to try to clarify what’s going on.
---
 configure.ac |  25 +++++++-------
 etc/NEWS     |   5 ++-
 src/eval.c   | 105 ++++++++++++++++++++++++++++++-----------------------------
 3 files changed, 69 insertions(+), 66 deletions(-)

diff --git a/configure.ac b/configure.ac
index 45cfdfc..d6b8001 100644
--- a/configure.ac
+++ b/configure.ac
@@ -507,16 +507,6 @@ AC_DEFUN
 [Define this to enable glyphs debugging code.])
 fi
 
-AC_ARG_ENABLE(check-lisp-object-type,
-[AS_HELP_STRING([--enable-check-lisp-object-type],
-                [enable compile time checks for the Lisp_Object data type.
-		This is useful for development for catching certain types of bugs.])],
-if test "${enableval}" != "no"; then
-   AC_DEFINE(CHECK_LISP_OBJECT_TYPE, 1,
-   [Define this to enable compile time checks for the Lisp_Object data type.])
-fi)
-
-
 dnl The name of this option is unfortunate.  It predates, and has no
 dnl relation to, the "sampling-based elisp profiler" added in 24.3.
 dnl Actually, it stops it working.
@@ -877,9 +867,18 @@ AC_DEFUN
    # just a release imported into Git for patch management.
    gl_gcc_warnings=no
    if test -e "$srcdir"/.git && test ! -f "$srcdir"/.tarball-version; then
-     gl_GCC_VERSION_IFELSE([5], [3], [gl_gcc_warnings=warn-only])]
-   fi
-)
+     gl_GCC_VERSION_IFELSE([5], [3], [gl_gcc_warnings=warn-only])
+   fi])
+
+AC_ARG_ENABLE([check-lisp-object-type],
+  [AS_HELP_STRING([--enable-check-lisp-object-type],
+     [Enable compile-time checks for the Lisp_Object data type,
+      which can catch some bugs during development.
+      The default is "no" if --enable-gcc-warnings is "no".])])
+if test "${enable_check_lisp_object_type-$gl_gcc_warnings}" != "no"; then
+  AC_DEFINE([CHECK_LISP_OBJECT_TYPE], 1,
+    [Define to enable compile-time checks for the Lisp_Object data type.])
+fi
 
 # clang is unduly picky about some things.
 AC_CACHE_CHECK([whether the compiler is clang], [emacs_cv_clang],
diff --git a/etc/NEWS b/etc/NEWS
index 173c4e4..410e681 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -42,6 +42,9 @@ now the default in developer builds.  As before, use
 '--disable-gcc-warnings' to suppress GCC's warnings, and
 '--enable-gcc-warnings' to stop the build if GCC issues warnings.
 
+** When GCC warnings are enabled, '--enable-check-lisp-object-type' is
+now enabled by default when configuring.
+
 +++
 ** The Emacs server now has socket-launching support.  This allows
 socket based activation, where an external process like systemd can
@@ -393,7 +396,7 @@ procedure and therefore obeys saving hooks.
 * Changes in Specialized Modes and Packages in Emacs 26.1
 
 *** Info menu and index completion uses substring completion by default.
-This can be customized via the `info-menu` category in
+This can be customized via the info-menu category in
 completion-category-override.
 
 +++
diff --git a/src/eval.c b/src/eval.c
index af0912f..0030271 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1225,18 +1225,17 @@ usage: (condition-case VAR BODYFORM &rest HANDLERS)  */)
    rather than passed in a list.  Used by Fbyte_code.  */
 
 Lisp_Object
-internal_lisp_condition_case (volatile Lisp_Object var, Lisp_Object bodyform,
+internal_lisp_condition_case (Lisp_Object var, Lisp_Object bodyform,
 			      Lisp_Object handlers)
 {
-  Lisp_Object val;
   struct handler *oldhandlerlist = handlerlist;
-  int clausenb = 0;
+  ptrdiff_t clausenb = 0;
 
   CHECK_SYMBOL (var);
 
-  for (val = handlers; CONSP (val); val = XCDR (val))
+  for (Lisp_Object tail = handlers; CONSP (tail); tail = XCDR (tail))
     {
-      Lisp_Object tem = XCAR (val);
+      Lisp_Object tem = XCAR (tail);
       clausenb++;
       if (! (NILP (tem)
 	     || (CONSP (tem)
@@ -1246,55 +1245,57 @@ internal_lisp_condition_case (volatile Lisp_Object var, Lisp_Object bodyform,
 	       SDATA (Fprin1_to_string (tem, Qt)));
     }
 
-  { /* The first clause is the one that should be checked first, so it should
-       be added to handlerlist last.  So we build in `clauses' a table that
-       contains `handlers' but in reverse order.  SAFE_ALLOCA won't work
-       here due to the setjmp, so impose a MAX_ALLOCA limit.  */
-    if (MAX_ALLOCA / word_size < clausenb)
-      memory_full (SIZE_MAX);
-    Lisp_Object *clauses = alloca (clausenb * sizeof *clauses);
-    Lisp_Object *volatile clauses_volatile = clauses;
-    int i = clausenb;
-    for (val = handlers; CONSP (val); val = XCDR (val))
-      clauses[--i] = XCAR (val);
-    for (i = 0; i < clausenb; i++)
-      {
-	Lisp_Object clause = clauses[i];
-	Lisp_Object condition = CONSP (clause) ? XCAR (clause) : Qnil;
-	if (!CONSP (condition))
-	  condition = Fcons (condition, Qnil);
-	struct handler *c = push_handler (condition, CONDITION_CASE);
-	if (sys_setjmp (c->jmp))
-	  {
-	    ptrdiff_t count = SPECPDL_INDEX ();
-	    Lisp_Object val = handlerlist->val;
-	    Lisp_Object *chosen_clause = clauses_volatile;
-	    for (c = handlerlist->next; c != oldhandlerlist; c = c->next)
-	      chosen_clause++;
-	    handlerlist = oldhandlerlist;
-	    if (!NILP (var))
-	      {
-		if (!NILP (Vinternal_interpreter_environment))
-		  specbind (Qinternal_interpreter_environment,
-			    Fcons (Fcons (var, val),
-				   Vinternal_interpreter_environment));
-		else
-		  specbind (var, val);
-	      }
-	    val = Fprogn (XCDR (*chosen_clause));
-	    /* Note that this just undoes the binding of var; whoever
-	       longjumped to us unwound the stack to c.pdlcount before
-	       throwing.  */
-	    if (!NILP (var))
-	      unbind_to (count, Qnil);
-	    return val;
-	  }
-      }
-  }
+  /* The first clause is the one that should be checked first, so it
+     should be added to handlerlist last.  So build in CLAUSES a table
+     that contains HANDLERS but in reverse order.  CLAUSES is pointer
+     to volatile to avoid issues with setjmp and local storage.
+     SAFE_ALLOCA won't work here due to the setjmp, so impose a
+     MAX_ALLOCA limit.  */
+  if (MAX_ALLOCA / word_size < clausenb)
+    memory_full (SIZE_MAX);
+  Lisp_Object volatile *clauses = alloca (clausenb * sizeof *clauses);
+  clauses += clausenb;
+  for (Lisp_Object tail = handlers; CONSP (tail); tail = XCDR (tail))
+    *--clauses = XCAR (tail);
+  for (ptrdiff_t i = 0; i < clausenb; i++)
+    {
+      Lisp_Object clause = clauses[i];
+      Lisp_Object condition = CONSP (clause) ? XCAR (clause) : Qnil;
+      if (!CONSP (condition))
+	condition = list1 (condition);
+      struct handler *c = push_handler (condition, CONDITION_CASE);
+      if (sys_setjmp (c->jmp))
+	{
+	  Lisp_Object val = handlerlist->val;
+	  Lisp_Object volatile *chosen_clause = clauses;
+	  for (struct handler *h = handlerlist->next; h != oldhandlerlist;
+	       h = h->next)
+	    chosen_clause++;
+	  Lisp_Object handler_body = XCDR (*chosen_clause);
+	  handlerlist = oldhandlerlist;
+
+	  if (NILP (var))
+	    return Fprogn (handler_body);
+
+	  if (!NILP (Vinternal_interpreter_environment))
+	    {
+	      val = Fcons (Fcons (var, val),
+			   Vinternal_interpreter_environment);
+	      var = Qinternal_interpreter_environment;
+	    }
+
+	  /* Bind VAR to VAL while evaluating HANDLER_BODY.  The
+	     unbind_to just undoes VAR's binding; whoever longjumped
+	     to us unwound the stack to C->pdlcount before throwing.  */
+	  ptrdiff_t count = SPECPDL_INDEX ();
+	  specbind (var, val);
+	  return unbind_to (count, Fprogn (handler_body));
+	}
+    }
 
-  val = eval_sub (bodyform);
+  Lisp_Object result = eval_sub (bodyform);
   handlerlist = oldhandlerlist;
-  return val;
+  return result;
 }
 
 /* Call the function BFUN with no arguments, catching errors within it
-- 
2.9.3