bug#26597: 25.1; Compilation error on master with --enable-check-lisp-object-type

bug#26597: 25.1; Compilation error on master with --enable-check-lisp-object-type

[Philipp Stephani]

When compiling current master with --enable-check-lisp-object-type,
compilation fails:

/Applications/Xcode.app/Contents/Developer/usr/bin/make -C src VCSWITNESS='$(srcdir)/../.git/logs/HEAD' all
gcc -c  -Demacs  -I. -I. -I../lib -I../lib   -I/usr/X11/include   -D_REENTRANT -I/usr/local/Cellar/librsvg/2.40.16_1/include/librsvg-2.0 -I/usr/local/Cellar/gdk-pixbuf/2.36.2/include/gdk-pixbuf-2.0 -I/usr/local/Cellar/libpng/1.6.29/include/libpng16 -I/usr/local/Cellar/cairo/1.14.8/include/cairo -I/usr/local/Cellar/glib/2.52.0/include/glib-2.0 -I/usr/local/Cellar/glib/2.52.0/lib/glib-2.0/include -I/usr/local/opt/gettext/include -I/usr/local/Cellar/pcre/8.40/include -I/usr/local/Cellar/pixman/0.34.0/include/pixman-1 -I/usr/local/Cellar/fontconfig/2.12.1_2/include -I/usr/local/opt/freetype/include/freetype2 -I/usr/local/Cellar/libpng/1.6.29/include/libpng16    -I/usr/local/Cellar/dbus/1.10.16/include/dbus-1.0 -I/usr/local/Cellar/dbus/1.10.16/lib/dbus-1.0/include           -MMD -MF deps/nsterm.d -MP  -I/usr/local/Cellar/gnutls/3.5.10/include -I/usr/local/Cellar/nettle/3.3/include -I/usr/local/Cellar/libtasn1/4.10/include -I/usr/local/Cellar/p11-kit/0.23.5/include/p11-kit-1    -Wno-switch -Wno-tautological-constant-out-of-range-compare -Wno-pointer-sign -Wno-string-plus-int -Wno-unknown-attributes -ggdb3 -O0   nsterm.m
nsterm.m:6969:8: error: member reference base type 'enum z_group' is not a structure or union
  if (!NILP (FRAME_Z_GROUP (f)))
       ^~~~~~~~~~~~~~~~~~~~~~~~

FRAME_Z_GROUP is indeed an enum, and NILP shouldn't be applied to it.  I
guess this works by accident if --enable-check-lisp-object-type is not
set because then Lisp_Object is a plain integer.

(Tangentially, is there any reason not to define Lisp_Object as struct
unconditionally, to avoid such coding errors?)



In GNU Emacs 25.1.1 (x86_64-apple-darwin15.6.0, NS appkit-1404.47 Version 10.11.6 (Build 15G1004))
 of 2016-09-22 built on p
Windowing system distributor 'Apple', version 10.3.1504
Configured using:
 'configure --disable-dependency-tracking --disable-silent-rules
 --enable-locallisppath=/usr/local/share/emacs/site-lisp
 --infodir=/usr/local/Cellar/emacs/25.1/share/info/emacs
 --prefix=/usr/local/Cellar/emacs/25.1 --without-x --with-xml2
 --without-dbus --without-gnutls --with-rsvg --with-ns
 --disable-ns-self-contained'

Configured features:
JPEG RSVG NOTIFY ACL LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS

Important settings:
  value of $LANG: de_DE.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Load-path shadows:
/usr/local/share/emacs/site-lisp/f/f hides /usr/local/share/emacs/site-lisp/f-emacs/f
/usr/local/share/emacs/site-lisp/let-alist/let-alist hides /usr/local/Cellar/emacs/25.1/share/emacs/25.1/lisp/emacs-lisp/let-alist

Features:
(shadow sort mail-extr emacsbug message dired format-spec rfc822 mml
mml-sec password-cache epg epg-config gnus-util mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util help-fns help-mode easymenu
cl-loaddefs pcase cl-lib mail-prsvr mail-utils time-date mule-util
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel ns-win ucs-normalize term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list newcomment elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow timer select scroll-bar
mouse jit-lock font-lock syntax facemenu font-core frame cl-generic cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese charscript case-table epa-hook
jka-cmpr-hook help simple abbrev minibuffer cl-preloaded nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote kqueue cocoa ns multi-tty
make-network-process emacs)

Memory information:
((conses 16 196364 9526)
 (symbols 48 19634 0)
 (miscs 40 77 241)
 (strings 32 15041 5419)
 (string-bytes 1 436541)
 (vectors 16 32909)
 (vector-slots 8 654057 5600)
 (floats 8 158 96)
 (intervals 56 208 0)
 (buffers 976 19))

bug#26597: 25.1; Compilation error on master with --enable-check-lisp-object-type

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Fri, 21 Apr 2017 22:05:58 +0200
> 
> nsterm.m:6969:8: error: member reference base type 'enum z_group' is not a structure or union
>   if (!NILP (FRAME_Z_GROUP (f)))
>        ^~~~~~~~~~~~~~~~~~~~~~~~

It should compare against z_group_none instead.

> (Tangentially, is there any reason not to define Lisp_Object as struct
> unconditionally, to avoid such coding errors?)

Yes, it produces slower code.

bug#26597: 25.1; Compilation error on master with --enable-check-lisp-object-type

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 22. Apr. 2017 um 09:42 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Fri, 21 Apr 2017 22:05:58 +0200
> >
> > nsterm.m:6969:8: error: member reference base type 'enum z_group' is not
> a structure or union
> >   if (!NILP (FRAME_Z_GROUP (f)))
> >        ^~~~~~~~~~~~~~~~~~~~~~~~
>
> It should compare against z_group_none instead.
>

Pushed eb52828a43 to fix.


>
> > (Tangentially, is there any reason not to define Lisp_Object as struct
> > unconditionally, to avoid such coding errors?)
>
> Yes, it produces slower code.
>

Are you sure that's still the case? I've just diffed the assembly output
for editfns.c with and without --enable-check-lisp-object-type, they seem
identical (apart from minor diffs due to different register allocation and
instruction ordering). Replacing a primitive value with a struct containing
such a value should never degrade performance; that would be a compiler
bug.

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

bug#26597: 25.1; Compilation error on master with --enable-check-lisp-object-type

[Andreas Schwab]

On Apr 22 2017, Philipp Stephani <p.stephani2@gmail.com> wrote:

> Are you sure that's still the case?

There are architectures where passing a struct is less efficient than
passing an integer, even if they are of the same size.

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

bug#26597: 25.1; Compilation error on master with --enable-check-lisp-object-type

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 22 Apr 2017 11:57:11 +0000
> 
>  > (Tangentially, is there any reason not to define Lisp_Object as struct
>  > unconditionally, to avoid such coding errors?)
> 
>  Yes, it produces slower code.
> 
> Are you sure that's still the case? I've just diffed the assembly output for editfns.c with and without
> --enable-check-lisp-object-type, they seem identical (apart from minor diffs due to different register allocation
> and instruction ordering). Replacing a primitive value with a struct containing such a value should never
> degrade performance; that would be a compiler bug. 

On what OS/architecture, and with what compiler options?

in any case, if you want to raise this issue, please post to
emacs-devel, not here.

bug#26597: 25.1; Compilation error on master with --enable-check-lisp-object-type

[Philipp Stephani]

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

Andreas Schwab <schwab@linux-m68k.org> schrieb am Sa., 22. Apr. 2017 um
15:35 Uhr:

> On Apr 22 2017, Philipp Stephani <p.stephani2@gmail.com> wrote:
>
> > Are you sure that's still the case?
>
> There are architectures where passing a struct is less efficient than
> passing an integer, even if they are of the same size.
>
>
That might be the case, but not on x86/AMD64.

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

Enabling --enable-check-lisp-object-type by default on x86 and AMD64 (was: Re: bug#26597: 25.1; Compilation error on master with --enable-check-lisp-object-type)

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 22. Apr. 2017 um 15:35 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sat, 22 Apr 2017 11:57:11 +0000
> >
> >  > (Tangentially, is there any reason not to define Lisp_Object as struct
> >  > unconditionally, to avoid such coding errors?)
> >
> >  Yes, it produces slower code.
> >
> > Are you sure that's still the case? I've just diffed the assembly output
> for editfns.c with and without
> > --enable-check-lisp-object-type, they seem identical (apart from minor
> diffs due to different register allocation
> > and instruction ordering). Replacing a primitive value with a struct
> containing such a value should never
> > degrade performance; that would be a compiler bug.
>
> On what OS/architecture, and with what compiler options?
>

On macOS, with configure called e.g. as

./configure  '--without-xml2' '--with-modules' '--without-pop'
'--with-mailutils' --enable-check-lisp-object-type 'CFLAGS=-O2 -save-temps'
'MAKEINFO=/usr/local/opt/texinfo/bin/makeinfo'


>
> in any case, if you want to raise this issue, please post to
> emacs-devel, not here.
>

Moved to emacs-devel.

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

bug#26597: 25.1; Compilation error on master with --enable-check-lisp-object-type

[Stefan Monnier]

> Replacing a primitive value with a struct containing such a value
> should never degrade performance; that would be a compiler bug.

FWIW, No, compilers do not guarantee that semantically-equivalent
programs will be compiled to code with equivalent performance, so it
wouldn't be a bug.  It's only a "quality of implementation" issue.


        Stefan

bug#26597: 25.1; Compilation error on master with --enable-check-lisp-object-type

[Philipp Stephani]

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

Stefan Monnier <monnier@iro.umontreal.ca> schrieb am Mo., 1. Mai 2017 um
15:01 Uhr:

> > Replacing a primitive value with a struct containing such a value
> > should never degrade performance; that would be a compiler bug.
>
> FWIW, No, compilers do not guarantee that semantically-equivalent
> programs will be compiled to code with equivalent performance, so it
> wouldn't be a bug.  It's only a "quality of implementation" issue.
>

Yes, I mean "bug" in a wider sense that includes QoI issues.

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

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

[Paul Eggert]

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

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

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 2 May 2017 15:14:17 -0700
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> 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.

One difficulty this adds in developing is that backtraces are much
less readable, as Lisp_Object values are not shown.

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

[Paul Eggert]

Eli Zaretskii wrote:
> One difficulty this adds in developing is that backtraces are much
> less readable, as Lisp_Object values are not shown.

Do you mean the C backtrace? Would it help to add a line "set print 
frame-arguments all" to src/.gdbinit?

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

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 2 May 2017 20:24:18 -0700
> 
> Eli Zaretskii wrote:
> > One difficulty this adds in developing is that backtraces are much
> > less readable, as Lisp_Object values are not shown.
> 
> Do you mean the C backtrace?

Yes.

> Would it help to add a line "set print frame-arguments all" to
> src/.gdbinit?

Only partially.  People who report backtraces seldom bother to start
GDB from the Emacs's src directory, let alone source .gdbinit by hand.
Moreover, latest versions of GDB refuse to auto-load .gdbinit files
from random directories, unless you put some magic in your ~/.gdbinit
(which most people don't).  Finally, this setting doesn't affect only
Lisp_Object, it affects any non-scalar arguments, so it has
non-trivial downsides of its own (which I guess is why it is not the
default).

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

[Paul Eggert]

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

On 05/03/2017 07:48 AM, Eli Zaretskii wrote:
>> Would it help to add a line "set print frame-arguments all" to
>> src/.gdbinit?
> Only partially.  People who report backtraces seldom bother to start
> GDB from the Emacs's src directory, let alone source .gdbinit by hand.
> Moreover, latest versions of GDB refuse to auto-load .gdbinit files
> from random directories, unless you put some magic in your ~/.gdbinit
> (which most people don't).

This problem is not new. It should be OK to continue ask developers to 
source .gdbinit, to compile with -O0, etc. If we can't assume that, the 
backtrace is already problematic, and the recent change doesn't make it 
much worse.

> this setting doesn't affect only
> Lisp_Object, it affects any non-scalar arguments, so it has
> non-trivial downsides of its own (which I guess is why it is not the
> default).

True. How about using GDB pretty-printers instead?  E.g., add something 
like the attached file as src/gdb-pretty.py, and add the line "source 
gdb-pretty.py" to src/.gdbinit. That way, only Lisp_Object values are 
affected, rather than all structs.


[-- Attachment #2: gdb-pretty.py --]
[-- Type: text/x-python, Size: 524 bytes --]

import gdb
import gdb.printing

class Lisp_Object_Printer:
    def __init__(self, val):
        self.val = val

    def to_string(self):
        return str (self.val["i"]) + "L"

def build_pretty_printer ():
    pp = gdb.printing.RegexpCollectionPrettyPrinter ("Emacs")
    pp.add_printer ('Lisp_Object', '^Lisp_Object$', Lisp_Object_Printer)
    return pp

gdb.printing.register_pretty_printer (gdb.current_objfile (),
                                      build_pretty_printer (),
                                      1)

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

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 3 May 2017 11:08:23 -0700
> 
> > Only partially.  People who report backtraces seldom bother to start
> > GDB from the Emacs's src directory, let alone source .gdbinit by hand.
> > Moreover, latest versions of GDB refuse to auto-load .gdbinit files
> > from random directories, unless you put some magic in your ~/.gdbinit
> > (which most people don't).
> 
> This problem is not new.

It isn't new, but previously it only affected displaying Lisp data in
human-readable form.  Now it will also affect backtraces.

> How about using GDB pretty-printers instead?

This is better, but how sure we are people's GDB is built with Python
support?

> E.g., add something like the attached file as src/gdb-pretty.py, and
> add the line "source gdb-pretty.py" to src/.gdbinit.

Why not make this part of .gdbinit itself?

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

[Eli Zaretskii]

> Date: Wed, 03 May 2017 21:22:09 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: p.stephani2@gmail.com, emacs-devel@gnu.org
> 
> > How about using GDB pretty-printers instead?
> 
> This is better, but how sure we are people's GDB is built with Python
> support?

Also, the Python script should be a bit smarter, to support also the
compilation without --enable-check-lisp-object-type.  When I try this
script in such a build, GDB crashes when displaying a backtrace.

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

[Paul Eggert]

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

On 05/03/2017 11:22 AM, Eli Zaretskii wrote:
> This is better, but how sure we are people's GDB is built with Python
> support?

We can ask developers to upgrade. GDB pretty-printing has worked with 
embedded Python for several years (since GDB 7.3 in 2011, I believe). We 
are not talking about general Emacs users here, only developers who are 
building bleeding-edge Emacs from Git. It's reasonable to expect these 
folks to have reasonably modern tools, especially since the only problem 
here is that the backtrace will be a bit harder to read if they use an 
old GDB.

> Why not make this part of .gdbinit itself? 

Sure, we can do that.

> Also, the Python script should be a bit smarter, to support also the
> compilation without --enable-check-lisp-object-type.  When I try this
> script in such a build, GDB crashes when displaying a backtrace.
Also doable.

I installed the attached, which I hope addresses the above issues well 
enough.


[-- Attachment #2: 0001-Pretty-print-Lisp_Object-values-in-GDB.patch --]
[-- Type: text/x-patch, Size: 2878 bytes --]

From f46a91a7811c359fae9c1c2a7f9374f9c5303209 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 5 May 2017 15:59:24 -0700
Subject: [PATCH] Pretty-print Lisp_Object values in GDB

* src/.gdbinit: Add a pretty-printer for Lisp_Object values.  Now,
GDB displays them as "XIL(0xXXX)" rather than displaying them
as "..." when CHECK_LISP_OBJECT_TYPE is in effect and as "DDDDD"
otherwise.
---
 src/.gdbinit | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/src/.gdbinit b/src/.gdbinit
index 6d7476d..0596188 100644
--- a/src/.gdbinit
+++ b/src/.gdbinit
@@ -1264,3 +1264,60 @@ commands
   end
   continue
 end
+
+
+# Put the Python code at the end of .gdbinit so that if GDB does not
+# support Python, GDB will do all the above initializations before
+# reporting an error.
+
+python
+
+# Omit pretty-printing in older (pre-7.3) GDBs that lack it.
+if hasattr(gdb, 'printing'):
+
+  class Emacs_Pretty_Printers (gdb.printing.RegexpCollectionPrettyPrinter):
+    """A collection of pretty-printers.  This is like GDB's
+       RegexpCollectionPrettyPrinter except when printing Lisp_Object."""
+    def __call__ (self, val):
+      """Look up the pretty-printer for the provided value."""
+      type = val.type
+      typename = type.tag or type.name
+      basic_type = gdb.types.get_basic_type (type)
+      basic_typename = basic_type.tag or basic_type.name
+      for printer in self.subprinters:
+        if (printer.enabled
+            and ((printer.regexp == '^Lisp_Object$'
+                  and typename == 'Lisp_Object')
+                 or (basic_typename
+                     and printer.compiled_re.search (basic_typename)))):
+          return printer.gen_printer (val)
+      return None
+
+  class Lisp_Object_Printer:
+    "A printer for Lisp_Object values."
+    def __init__ (self, val):
+      self.val = val
+
+    def to_string (self):
+      "Yield a string that can be fed back into GDB."
+      val = self.val
+      basic_type = gdb.types.get_basic_type (val.type)
+      if (basic_type.code == gdb.TYPE_CODE_STRUCT
+          and gdb.types.has_field (basic_type, "i")):
+        val = val["i"]
+      # Yield "XIL(N)", where N is a C integer.  This helps humans
+      # distinguish Lisp_Object values from ordinary integers even
+      # when Lisp_Object is an integer.  Perhaps some day the
+      # pretty-printing could be fancier.
+      if not val:
+        return "XIL(0)" # Easier to read than "XIL(0x0)".
+      return "XIL(0x%x)" % val
+
+  def build_pretty_printer ():
+    pp = Emacs_Pretty_Printers ("Emacs")
+    pp.add_printer ('Lisp_Object', '^Lisp_Object$', Lisp_Object_Printer)
+    return pp
+
+  gdb.printing.register_pretty_printer (gdb.current_objfile (),
+                                        build_pretty_printer (), True)
+end
-- 
2.9.3

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

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 5 May 2017 16:23:52 -0700
> 
> On 05/03/2017 11:22 AM, Eli Zaretskii wrote:
> > This is better, but how sure we are people's GDB is built with Python
> > support?
> 
> We can ask developers to upgrade. GDB pretty-printing has worked with 
> embedded Python for several years (since GDB 7.3 in 2011, I believe). We 
> are not talking about general Emacs users here, only developers who are 
> building bleeding-edge Emacs from Git.

We are talking about anyone who could submit backtraces from the Git
master builds.  I don't know who that might be (e.g., aren't there
snapshots of master provided by the various GNU/Linux distros?), but
even if we only consider people who are on this list, I'm not sure how
many of them have GDB built with Python support.  Maybe we should poll
this list about this, it cannot be that hard.

> It's reasonable to expect these folks to have reasonably modern
> tools

The GDB version is not the issue here, because even the latest GDB can
be built without Python.  It's a configure-time option.

> > Why not make this part of .gdbinit itself? 
> 
> Sure, we can do that.
> 
> > Also, the Python script should be a bit smarter, to support also the
> > compilation without --enable-check-lisp-object-type.  When I try this
> > script in such a build, GDB crashes when displaying a backtrace.
> Also doable.

Thanks.

> +    def to_string (self):
> +      "Yield a string that can be fed back into GDB."
> +      val = self.val
> +      basic_type = gdb.types.get_basic_type (val.type)
> +      if (basic_type.code == gdb.TYPE_CODE_STRUCT
> +          and gdb.types.has_field (basic_type, "i")):
> +        val = val["i"]
> +      # Yield "XIL(N)", where N is a C integer.  This helps humans
> +      # distinguish Lisp_Object values from ordinary integers even
> +      # when Lisp_Object is an integer.  Perhaps some day the
> +      # pretty-printing could be fancier.
> +      if not val:
> +        return "XIL(0)" # Easier to read than "XIL(0x0)".
> +      return "XIL(0x%x)" % val

I'm not sure I like this XIL(something) feature.  For starters, it
deviates from the format I'm used to, and takes precious screen
estate.  It also might confuse people who know GDB but won't
immediately grasp what "XIL" is.  It also introduces certain
inconsistencies, e.g. (in a build without --enable-check-lisp-object-type):

  (gdb) fr 28
  #28 0x011586e2 in command_loop_2 (ignore=XIL(0)) at keyboard.c:1112
  1112        val = internal_condition_case (command_loop_1, Qerror, cmd_error);
  (gdb) p ignore == Qnil
  $1 = 1
  (gdb) p Qnil
  $2 = 0
  (gdb) p ignore
  $3 = XIL(0)

So something whose value is zero is numerically equal to something
whose value is XIL(0).

As for "some day the pretty-printing could be fancier": what exactly
did you want to implement?  Guile has a pretty elaborate GDB support
for printing Scheme values, perhaps you could look there for
inspiration.

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

[Paul Eggert]

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

Eli Zaretskii wrote:

> Maybe we should poll this list about this

In effect we've already probed about it in this thread. Others are welcome to 
chime in.

> even the latest GDB can be built without Python. It's a configure-time option.

The configure-time option defaults to "yes" if Python is installed, which it 
typically is nowadays. It's been "yes" in the major GNU/Linux distributions for 
years, and even MinGW now ships a Python-enabled GDB. So this should not be a 
major problem for developers in practice.

> I'm not sure I like this XIL(something) feature.  For starters, it
> deviates from the format I'm used to, and takes precious screen
> estate.

One gets used to it, and it has the advantage of easily distinguishing 
Lisp_Object from integer values when debugging, which is worth some screen real 
estate.

> It also might confuse people who know GDB but won't
> immediately grasp what "XIL" is.

The confusion won't last long, and any confusion is outweighed by the clarity 
caused by GDB's now distinguishing Lisp_Object values from C integers. I don't 
see this as turning into a real problem, but if I'm wrong I suppose we can 
rename XIL to something more mnemonic.

>   (gdb) p Qnil
>   $2 = 0
>   (gdb) p ignore
>   $3 = XIL(0)

Ah, I didn't think about pretty-printing constants. I fixed that by installing 
the first attached patch. I also tried out a somewhat-older GDB implementation 
(Ubuntu LTS) and fixed some other glitches in the second attached patch. 
However, we shouldn't need to spend a lot of time worrying about glitches with 
ancient debuggers, as we can expect developers to be reasonably up-to-date.

> something whose value is zero is numerically equal to something
> whose value is XIL(0).

Sure, because XIL(0) == 0 when --enable-lisp-object-types=no. Pretty-printing 
doesn't change that.

> As for "some day the pretty-printing could be fancier": what exactly
> did you want to implement?

I wasn't planning anything exactly. The general principle is that the GDB 
pretty-printer should output a string that you can paste back into GDB. (In 
contrast, the long-established "pp" command should output a string that you can 
paste into Elisp.) GDB could pretty-print the Lisp integer 27 as 
"make_number(27)", for example.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Pretty-print-const-Lisp_Objects-in-.gdbinit.patch --]
[-- Type: text/x-diff; name="0001-Pretty-print-const-Lisp_Objects-in-.gdbinit.patch", Size: 1233 bytes --]

From 7cd7f5b4032092389a00e23af3ab435628febed3 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 6 May 2017 14:24:12 -0700
Subject: [PATCH 1/2] Pretty-print const Lisp_Objects in .gdbinit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/.gdbinit (Emacs_Pretty_Printers.__call__):
Compare unqualified type to Lisp_Object, to do the right thing
when the expression has type ‘Lisp_Object const’.
Problem reported by Eli Zaretskii in:
http://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00138.html
---
 src/.gdbinit | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/.gdbinit b/src/.gdbinit
index 0596188..29689e2 100644
--- a/src/.gdbinit
+++ b/src/.gdbinit
@@ -1280,7 +1280,7 @@ if hasattr(gdb, 'printing'):
        RegexpCollectionPrettyPrinter except when printing Lisp_Object."""
     def __call__ (self, val):
       """Look up the pretty-printer for the provided value."""
-      type = val.type
+      type = val.type.unqualified ()
       typename = type.tag or type.name
       basic_type = gdb.types.get_basic_type (type)
       basic_typename = basic_type.tag or basic_type.name
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Port-.gdbinit-to-GDB-7.11.1-Python-2.7.12.patch --]
[-- Type: text/x-diff; name="0002-Port-.gdbinit-to-GDB-7.11.1-Python-2.7.12.patch", Size: 1395 bytes --]

From f31689c803a13836ef3528d6e2b4c98c767c42c7 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 6 May 2017 15:29:16 -0700
Subject: [PATCH 2/2] Port .gdbinit to GDB 7.11.1 + Python 2.7.12

* src/.gdbinit (Lisp_Object_Printer.to_string):
Explicitly convert integer val to 'int', so that
older GDBs do not complain about the conversion.
* src/lisp.h (Lisp_Object) [CHECK_LISP_OBJECT_TYPE]:
Give the struct a tag, so that older GDB pretty-printers have a
tag to hang their hat on.
---
 src/.gdbinit | 2 +-
 src/lisp.h   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/.gdbinit b/src/.gdbinit
index 29689e2..80aa95b 100644
--- a/src/.gdbinit
+++ b/src/.gdbinit
@@ -1311,7 +1311,7 @@ if hasattr(gdb, 'printing'):
       # pretty-printing could be fancier.
       if not val:
         return "XIL(0)" # Easier to read than "XIL(0x0)".
-      return "XIL(0x%x)" % val
+      return "XIL(0x%x)" % int(val)
 
   def build_pretty_printer ():
     pp = Emacs_Pretty_Printers ("Emacs")
diff --git a/src/lisp.h b/src/lisp.h
index 5d4c64a..de3a548 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -546,7 +546,7 @@ enum Lisp_Fwd_Type
 
 #ifdef CHECK_LISP_OBJECT_TYPE
 
-typedef struct { EMACS_INT i; } Lisp_Object;
+typedef struct Lisp_Object { EMACS_INT i; } Lisp_Object;
 
 #define LISP_INITIALLY(i) {i}
 
-- 
2.7.4