inlinable functions instead of macros

inlinable functions instead of macros

[Stefan Monnier]

While inlinable functions are much cleaner than macros, they have
a very serious downside: you just end up with

lisp.h:2416: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)

I.e. the file&line info is always the same rather than giving the
file&line where the inlinable function was called.


        Stefan

Re: inlinable functions instead of macros

[Paul Eggert]

On 08/17/2012 04:48 PM, Stefan Monnier wrote:
> While inlinable functions are much cleaner than macros, they have
> a very serious downside: you just end up with
> 
> lisp.h:2416: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)
> 
> I.e. the file&line info is always the same rather than giving the
> file&line where the inlinable function was called.

On systems that use glibc we could adjust eassert so that it
also prints a backtrace, using glibc's 'backtrace' function.  See
<http://www.gnu.org/software/libc/manual/html_node/Backtraces.html>.
This would not be quite the same thing, as it would print function
names, insn offsets, and return addresses; but it would recapture
some of the ground lost here, and the backtrace info would in some
cases be more useful than what we have now.

Re: inlinable functions instead of macros

[Daniel Colascione]

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

On 8/17/12 4:59 PM, Paul Eggert wrote:
> On 08/17/2012 04:48 PM, Stefan Monnier wrote:
>> While inlinable functions are much cleaner than macros, they have
>> a very serious downside: you just end up with
>>
>> lisp.h:2416: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)
>>
>> I.e. the file&line info is always the same rather than giving the
>> file&line where the inlinable function was called.
> 
> On systems that use glibc we could adjust eassert so that it
> also prints a backtrace, using glibc's 'backtrace' function.  See
> <http://www.gnu.org/software/libc/manual/html_node/Backtraces.html>.
> This would not be quite the same thing, as it would print function
> names, insn offsets, and return addresses; but it would recapture
> some of the ground lost here, and the backtrace info would in some
> cases be more useful than what we have now.

On Windows, we solved this problem a long time ago by teaching the
operating system itself how to walk stack frames. When a program
crashes, the OS captures a minidump containing the faulting stack,
instruction pointer, register contents, and (optionally) things like
data segments. If you have one of these minidumps, you don't need
particularly good assertion-failure output: you have everything you
need in the minidump, which you can load into the debugger and examine.

I hear Google's breakpad provides similar facilities for other systems.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: inlinable functions instead of macros

[John Yates]

On Sat, Aug 18, 2012 at 6:15 PM, Daniel Colascione <dancol@dancol.org> wrote:
>
> On Windows, we solved this problem a long time ago by teaching the
> operating system itself how to walk stack frames. When a program
> crashes, the OS captures a minidump containing the faulting stack,
> instruction pointer, register contents, and (optionally) things like
> data segments. If you have one of these minidumps, you don't need
> particularly good assertion-failure output: you have everything you
> need in the minidump, which you can load into the debugger and examine.

And even before that Vax/VMS solved the problem by capturing a stack
trace every time a signal was propagated (including of course ACCVIO
or access violation, the equivalent of SEGV).  A shell setting
controlled whether the trace was displayed by default of not.  Even if
one choose not to display it by default it was always available by
explicit request. VMS coding conventions encouraged failure
notification via signalling and required a signal handler to use a
subsystem specific status when propagating a signal.  This lead to
very informative stack traces consisting of call sites interspersed
with local statuses showing how failure was interpreted and reported
as it propagated up the stack.

/john

Re: inlinable functions instead of macros

[Richard Stallman]

I think the Unix solution for this was to dump core so you could
examine everything in a debugger.

--
Dr Richard Stallman
President, Free Software Foundation
51 Franklin St
Boston MA 02110
USA
www.fsf.org  www.gnu.org
Skype: No way! That's nonfree (freedom-denying) software.
  Use Ekiga or an ordinary phone call

Re: inlinable functions instead of macros

[Stefan Monnier]

Could some please check our inlinable functions and move any easserts in
them to a macro (i.e. rename the function, replace the old name with
a macro that does the assert and calls the inlinable function)?

If someone wants to *additionally* provide a patch to use glibc's
`backtrace' in our `eassert' that's fine, but the two are
separate issues.


        Stefan

C backtraces for Emacs

[Paul Eggert]

On 08/21/2012 10:50 AM, Stefan Monnier wrote:
> If someone wants to *additionally* provide a patch to use glibc's
> `backtrace' in our `eassert' that's fine

I've prepared a patch to do this additional work, enclosed below.  The
key part is at the end; the rest is boilerplate for non-GNUish
systems.  I've tested this on Fedora 15, Solaris 11, and Solaris 10.

I'll hold off installing this to give Eli and Juanma a heads-up, as it
creates a new substitute header <execinfo.h> on non-GNUish systems,
and the Windows build will need to do something similar.  If I understand
things correctly Windows already does backtraces in a different way, so
its execinfo.h can just be a copy of execinfo.in.h.

=== modified file 'ChangeLog'
--- ChangeLog	2012-08-22 06:55:44 +0000
+++ ChangeLog	2012-08-22 08:59:10 +0000
@@ -1,3 +1,11 @@
+2012-08-22  Paul Eggert  <eggert@cs.ucla.edu>
+
+	On assertion failure, print backtrace if available.
+	Merge from gnulib, incorporating:
+	2012-08-22 execinfo: new module
+	* lib/gnulib.mk, m4/gnulib-comp.m4: Regenerate.
+	* lib/execinfo.c, lib/execinfo.in.h, m4/execinfo.m4: New files.
+
 2012-08-22  Glenn Morris  <rgm@gnu.org>
 
 	* Makefile.in (install-arch-dep): If NO_BIN_LINK is non-null,

=== modified file 'admin/ChangeLog'
--- admin/ChangeLog	2012-08-16 21:58:44 +0000
+++ admin/ChangeLog	2012-08-22 08:59:10 +0000
@@ -1,3 +1,8 @@
+2012-08-22  Paul Eggert  <eggert@cs.ucla.edu>
+
+	On assertion failure, print backtrace if available.
+	* merge-gnulib (GNULIB_MODULES): Add execinfo.
+
 2012-08-16  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Use ASCII tests for character types.

=== modified file 'admin/merge-gnulib'
--- admin/merge-gnulib	2012-08-16 21:58:44 +0000
+++ admin/merge-gnulib	2012-08-22 08:59:10 +0000
@@ -28,7 +28,7 @@
 GNULIB_MODULES='
   alloca-opt c-ctype c-strcase
   careadlinkat crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512
-  dtoastr dtotimespec dup2 environ
+  dtoastr dtotimespec dup2 environ execinfo
   filemode getloadavg getopt-gnu gettime gettimeofday
   ignore-value intprops largefile lstat
   manywarnings mktime pselect pthread_sigmask readlink

=== added file 'lib/execinfo.c'
--- lib/execinfo.c	1970-01-01 00:00:00 +0000
+++ lib/execinfo.c	2012-08-22 08:59:10 +0000
@@ -0,0 +1,3 @@
+#include <config.h>
+#define _GL_EXECINFO_INLINE _GL_EXTERN_INLINE
+#include "execinfo.h"

=== added file 'lib/execinfo.in.h'
--- lib/execinfo.in.h	1970-01-01 00:00:00 +0000
+++ lib/execinfo.in.h	2012-08-22 08:59:10 +0000
@@ -0,0 +1,54 @@
+/* Information about executables.
+
+   Copyright (C) 2012 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Written by Paul Eggert.  */
+
+#ifndef _GL_EXECINFO_H
+#define _GL_EXECINFO_H
+
+_GL_INLINE_HEADER_BEGIN
+#ifndef _GL_EXECINFO_INLINE
+# define _GL_EXECINFO_INLINE _GL_INLINE
+#endif
+
+_GL_EXECINFO_INLINE int
+backtrace (void **buffer, int size)
+{
+  (void) buffer;
+  (void) size;
+  return 0;
+}
+
+_GL_EXECINFO_INLINE char **
+backtrace_symbols (void *const *buffer, int size)
+{
+  (void) buffer;
+  (void) size;
+  return 0;
+}
+
+_GL_EXECINFO_INLINE void
+backtrace_symbols_fd (void *const *buffer, int size, int fd)
+{
+  (void) buffer;
+  (void) size;
+  (void) fd;
+}
+
+_GL_INLINE_HEADER_END
+
+#endif

=== modified file 'lib/gnulib.mk'
--- lib/gnulib.mk	2012-08-19 23:31:24 +0000
+++ lib/gnulib.mk	2012-08-22 08:59:10 +0000
@@ -21,7 +21,7 @@
 # the same distribution terms as the rest of that program.
 #
 # Generated by gnulib-tool.
-# Reproduce by: gnulib-tool --import --dir=. --lib=libgnu --source-base=lib --m4-base=m4 --doc-base=doc --tests-base=tests --aux-dir=build-aux --avoid=errno --avoid=fcntl --avoid=fcntl-h --avoid=fstat --avoid=msvc-inval --avoid=msvc-nothrow --avoid=raise --avoid=select --avoid=sigprocmask --avoid=sys_types --avoid=threadlib --makefile-name=gnulib.mk --conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca-opt c-ctype c-strcase careadlinkat crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512 dtoastr dtotimespec dup2 environ filemode getloadavg getopt-gnu gettime gettimeofday ignore-value intprops largefile lstat manywarnings mktime pselect pthread_sigmask readlink socklen stat-time stdalign stdarg stdbool stdio strftime strtoimax strtoumax symlink sys_stat sys_time tim
 e timespec-add timespec-sub utimens warnings
+# Reproduce by: gnulib-tool --import --dir=. --lib=libgnu --source-base=lib --m4-base=m4 --doc-base=doc --tests-base=tests --aux-dir=build-aux --avoid=errno --avoid=fcntl --avoid=fcntl-h --avoid=fstat --avoid=msvc-inval --avoid=msvc-nothrow --avoid=raise --avoid=select --avoid=sigprocmask --avoid=sys_types --avoid=threadlib --makefile-name=gnulib.mk --conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca-opt c-ctype c-strcase careadlinkat crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512 dtoastr dtotimespec dup2 environ execinfo filemode getloadavg getopt-gnu gettime gettimeofday ignore-value intprops largefile lstat manywarnings mktime pselect pthread_sigmask readlink socklen stat-time stdalign stdarg stdbool stdio strftime strtoimax strtoumax symlink sys_stat sys
 _time time timespec-add timespec-sub utimens warnings
 
 
 MOSTLYCLEANFILES += core *.stackdump
@@ -150,6 +150,31 @@
 
 ## end   gnulib module dup2
 
+## begin gnulib module execinfo
+
+BUILT_SOURCES += $(EXECINFO_H)
+
+# We need the following in order to create <execinfo.h> when the system
+# doesn't have one that works.
+if GL_GENERATE_EXECINFO_H
+execinfo.h: execinfo.in.h $(top_builddir)/config.status
+	$(AM_V_GEN)rm -f $@-t $@ && \
+	{ echo '/* DO NOT EDIT! GENERATED AUTOMATICALLY! */'; \
+	  cat $(srcdir)/execinfo.in.h; \
+	} > $@-t && \
+	mv $@-t $@
+else
+execinfo.h: $(top_builddir)/config.status
+	rm -f $@
+endif
+MOSTLYCLEANFILES += execinfo.h execinfo.h-t
+
+EXTRA_DIST += execinfo.c execinfo.in.h
+
+EXTRA_libgnu_a_SOURCES += execinfo.c
+
+## end   gnulib module execinfo
+
 ## begin gnulib module filemode
 
 libgnu_a_SOURCES += filemode.c

=== added file 'm4/execinfo.m4'
--- m4/execinfo.m4	1970-01-01 00:00:00 +0000
+++ m4/execinfo.m4	2012-08-22 08:59:10 +0000
@@ -0,0 +1,21 @@
+# Check for GNU-style execinfo.h.
+
+dnl Copyright 2012 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+AC_DEFUN([gl_EXECINFO_H],
+[
+  AC_CHECK_HEADERS_ONCE([execinfo.h])
+
+  if test $ac_cv_header_execinfo_h = yes; then
+    EXECINFO_H=''
+  else
+    EXECINFO_H='execinfo.h'
+    AC_LIBOBJ([execinfo])
+  fi
+
+  AC_SUBST([EXECINFO_H])
+  AM_CONDITIONAL([GL_GENERATE_EXECINFO_H], [test -n "$EXECINFO_H"])
+])

=== modified file 'm4/gnulib-comp.m4'
--- m4/gnulib-comp.m4	2012-08-19 23:31:24 +0000
+++ m4/gnulib-comp.m4	2012-08-22 08:59:10 +0000
@@ -53,6 +53,7 @@
   # Code from module dtotimespec:
   # Code from module dup2:
   # Code from module environ:
+  # Code from module execinfo:
   # Code from module extensions:
   AC_REQUIRE([gl_USE_SYSTEM_EXTENSIONS])
   # Code from module extern-inline:
@@ -152,6 +153,7 @@
   gl_UNISTD_MODULE_INDICATOR([dup2])
   gl_ENVIRON
   gl_UNISTD_MODULE_INDICATOR([environ])
+  gl_EXECINFO_H
   AC_REQUIRE([gl_EXTERN_INLINE])
   gl_FILEMODE
   gl_GETLOADAVG
@@ -534,6 +536,8 @@
   lib/dtoastr.c
   lib/dtotimespec.c
   lib/dup2.c
+  lib/execinfo.c
+  lib/execinfo.in.h
   lib/filemode.c
   lib/filemode.h
   lib/ftoastr.c
@@ -605,6 +609,7 @@
   m4/clock_time.m4
   m4/dup2.m4
   m4/environ.m4
+  m4/execinfo.m4
   m4/extensions.m4
   m4/extern-inline.m4
   m4/filemode.m4

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-08-22 07:20:42 +0000
+++ src/ChangeLog	2012-08-22 08:59:10 +0000
@@ -1,5 +1,9 @@
 2012-08-22  Paul Eggert  <eggert@cs.ucla.edu>
 
+	On assertion failure, print backtrace if available.
+	* alloc.c [ENABLE_CHECKING]: Include <execinfo.h>.
+	(die) [ENABLE_CHECKING]: Print a backtrace if available.
+
 	* fontset.c (FONTSET_ADD): Return void, not Lisp_Object.
 	Otherwise, the compiler complains about (A?B:C) where B is void
 	and C is Lisp_Object.  This fixes an incompatibility with Sun C	5.12.

=== modified file 'src/alloc.c'
--- src/alloc.c	2012-08-21 23:39:56 +0000
+++ src/alloc.c	2012-08-22 08:59:10 +0000
@@ -6681,13 +6681,21 @@
 }
 
 #ifdef ENABLE_CHECKING
+
+# include <execinfo.h>
+
 bool suppress_checking;
 
 void
 die (const char *msg, const char *file, int line)
 {
+  enum { NPOINTERS_MAX = 500 };
+  void *buffer[NPOINTERS_MAX];
+  int npointers;
   fprintf (stderr, "\r\n%s:%d: Emacs fatal error: %s\r\n",
 	   file, line, msg);
+  npointers = backtrace (buffer, NPOINTERS_MAX);
+  backtrace_symbols_fd (buffer, npointers, STDERR_FILENO);
   abort ();
 }
 #endif

Re: C backtraces for Emacs

[Eli Zaretskii]

> Date: Wed, 22 Aug 2012 02:24:31 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: Juanma Barranquero <lekktu@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
> 	emacs-devel@gnu.org
> 
> I'll hold off installing this to give Eli and Juanma a heads-up, as it
> creates a new substitute header <execinfo.h> on non-GNUish systems,
> and the Windows build will need to do something similar.  If I understand
> things correctly Windows already does backtraces in a different way, so
> its execinfo.h can just be a copy of execinfo.in.h.

I'm not sure what you mean.  An assertion violation on Windows calls a
function that displays an abort dialogue, giving the user a chance to
attach a debugger.  If the user does attach a debugger, she can then
display a backtrace from the debugger.  There's no other facility to
display a backtrace on Windows that I'm aware of.

Re: C backtraces for Emacs

[Paul Eggert]

On 08/22/2012 09:01 AM, Eli Zaretskii wrote:
> I'm not sure what you mean.  An assertion violation on Windows calls a
> function that displays an abort dialogue, giving the user a chance to
> attach a debugger.  If the user does attach a debugger, she can then
> display a backtrace from the debugger.  There's no other facility to
> display a backtrace on Windows that I'm aware of.

Yes, sorry, that's what I meant.  The idea is that the Windows build
can just copy execinfo.in.h to execinfo.h, which means Emacs
will continue to behave as before when eassert fails.

Re: C backtraces for Emacs

[Daniel Colascione]

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

On 8/22/2012 9:01 AM, Eli Zaretskii wrote:
>> Date: Wed, 22 Aug 2012 02:24:31 -0700
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Cc: Juanma Barranquero <lekktu@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>> 	emacs-devel@gnu.org
>>
>> I'll hold off installing this to give Eli and Juanma a heads-up, as it
>> creates a new substitute header <execinfo.h> on non-GNUish systems,
>> and the Windows build will need to do something similar.  If I understand
>> things correctly Windows already does backtraces in a different way, so
>> its execinfo.h can just be a copy of execinfo.in.h.
> 
> I'm not sure what you mean.  An assertion violation on Windows calls a
> function that displays an abort dialogue, giving the user a chance to
> attach a debugger.  If the user does attach a debugger, she can then
> display a backtrace from the debugger.  There's no other facility to
> display a backtrace on Windows that I'm aware of.

You can use the StackWalk64 function from dbghelp.dll.

"The StackWalk64 function provides a portable method for obtaining a stack
trace. Using the StackWalk64 function is recommended over writing your own
function because of all the complexities associated with stack walking on
platforms. In addition, there are compiler options that cause the stack to
appear differently, depending on how the module is compiled. By using this
function, your application has a portable stack trace that continues to work as
the compiler and operating system change."

Despite its name, StackWalk64 works on 32-bit platforms too. Windows actually
has very good support for this kind of thing.

By the way: when we assert, we should kill Emacs using RaiseFailFastException.
This function produces a nice WER (Windows Error Reporting) dialog and bypasses
a lot of layers of exception handling machinery that would ordinarily run in
userspace --- that is, with RaiseFailFastException, fewer things can go wrong.
abort() under Windows actually tried to run global destructors, send messages to
loaded DLLs about pending process termination, and a bunch of other things. RFFE
is much more reliable. On systems without RFFE, plain old RaiseException is fine
too, and RaiseException is present on all supported Windows versions.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: C backtraces for Emacs

[Eli Zaretskii]

> Date: Wed, 22 Aug 2012 17:40:08 -0700
> From: Daniel Colascione <dancol@dancol.org>
> CC: Paul Eggert <eggert@cs.ucla.edu>, lekktu@gmail.com, 
>  monnier@IRO.UMontreal.CA, emacs-devel@gnu.org
> 
> You can use the StackWalk64 function from dbghelp.dll.

Will that work with GCC-compiled programs and DLLs?  Isn't dbghelp.dll
VC-specific?  If it doesn't produce addresses that can be plugged into
GDB, then it's not good, because any meaningful debugging after the
assertion violation will have to be with GDB.

Also, dbghelp.dll seems to need to be installed separately, and MinGW
doesn't provide its import library, so we cannot link against it by
default.  Did I miss something?

Anyway, if this is workable, patches to use it are welcome.

> By the way: when we assert, we should kill Emacs using RaiseFailFastException.
> This function produces a nice WER (Windows Error Reporting) dialog and bypasses
> a lot of layers of exception handling machinery that would ordinarily run in
> userspace --- that is, with RaiseFailFastException, fewer things can go wrong.
> abort() under Windows actually tried to run global destructors, send messages to
> loaded DLLs about pending process termination, and a bunch of other things. RFFE
> is much more reliable. On systems without RFFE, plain old RaiseException is fine
> too, and RaiseException is present on all supported Windows versions.

Emacs overrides 'abort' with a custom function that calls our own
abort dialog.  So I don't think the problems with the standard 'abort'
function are relevant to Emacs.

Re: C backtraces for Emacs

[Paul Eggert]

Bastien Roucariès mentioned that the 'backtrace' function and
friends need the -lexecinfo linker flag on FreeBSD, so here's an
updated version of the patch that does this.  This affects
only 'configure' and 'make', not Emacs itself.  The NT and
DOS builds are affected (a new makefile variable
LIB_EXECINFO, which can be empty there) so I'm sending this
to Eli and Juanma as a heads-up.

=== modified file 'ChangeLog'
--- ChangeLog	2012-08-22 06:55:44 +0000
+++ ChangeLog	2012-08-24 10:43:12 +0000
@@ -1,3 +1,12 @@
+2012-08-24  Paul Eggert  <eggert@cs.ucla.edu>
+
+	On assertion failure, print backtrace if available.
+	Merge from gnulib, incorporating:
+	2012-08-24 execinfo: port to FreeBSD
+	2012-08-22 execinfo: new module
+	* lib/gnulib.mk, m4/gnulib-comp.m4: Regenerate.
+	* lib/execinfo.c, lib/execinfo.in.h, m4/execinfo.m4: New files.
+
 2012-08-22  Glenn Morris  <rgm@gnu.org>
 
 	* Makefile.in (install-arch-dep): If NO_BIN_LINK is non-null,

=== modified file 'admin/ChangeLog'
--- admin/ChangeLog	2012-08-16 21:58:44 +0000
+++ admin/ChangeLog	2012-08-24 10:43:12 +0000
@@ -1,3 +1,8 @@
+2012-08-24  Paul Eggert  <eggert@cs.ucla.edu>
+
+	On assertion failure, print backtrace if available.
+	* merge-gnulib (GNULIB_MODULES): Add execinfo.
+
 2012-08-16  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Use ASCII tests for character types.

=== modified file 'admin/merge-gnulib'
--- admin/merge-gnulib	2012-08-16 21:58:44 +0000
+++ admin/merge-gnulib	2012-08-22 08:59:10 +0000
@@ -28,7 +28,7 @@
 GNULIB_MODULES='
   alloca-opt c-ctype c-strcase
   careadlinkat crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512
-  dtoastr dtotimespec dup2 environ
+  dtoastr dtotimespec dup2 environ execinfo
   filemode getloadavg getopt-gnu gettime gettimeofday
   ignore-value intprops largefile lstat
   manywarnings mktime pselect pthread_sigmask readlink

=== added file 'lib/execinfo.c'
--- lib/execinfo.c	1970-01-01 00:00:00 +0000
+++ lib/execinfo.c	2012-08-22 08:59:10 +0000
@@ -0,0 +1,3 @@
+#include <config.h>
+#define _GL_EXECINFO_INLINE _GL_EXTERN_INLINE
+#include "execinfo.h"

=== added file 'lib/execinfo.in.h'
--- lib/execinfo.in.h	1970-01-01 00:00:00 +0000
+++ lib/execinfo.in.h	2012-08-22 08:59:10 +0000
@@ -0,0 +1,54 @@
+/* Information about executables.
+
+   Copyright (C) 2012 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Written by Paul Eggert.  */
+
+#ifndef _GL_EXECINFO_H
+#define _GL_EXECINFO_H
+
+_GL_INLINE_HEADER_BEGIN
+#ifndef _GL_EXECINFO_INLINE
+# define _GL_EXECINFO_INLINE _GL_INLINE
+#endif
+
+_GL_EXECINFO_INLINE int
+backtrace (void **buffer, int size)
+{
+  (void) buffer;
+  (void) size;
+  return 0;
+}
+
+_GL_EXECINFO_INLINE char **
+backtrace_symbols (void *const *buffer, int size)
+{
+  (void) buffer;
+  (void) size;
+  return 0;
+}
+
+_GL_EXECINFO_INLINE void
+backtrace_symbols_fd (void *const *buffer, int size, int fd)
+{
+  (void) buffer;
+  (void) size;
+  (void) fd;
+}
+
+_GL_INLINE_HEADER_END
+
+#endif

=== modified file 'lib/gnulib.mk'
--- lib/gnulib.mk	2012-08-19 23:31:24 +0000
+++ lib/gnulib.mk	2012-08-22 08:59:10 +0000
@@ -21,7 +21,7 @@
 # the same distribution terms as the rest of that program.
 #
 # Generated by gnulib-tool.
-# Reproduce by: gnulib-tool --import --dir=. --lib=libgnu --source-base=lib --m4-base=m4 --doc-base=doc --tests-base=tests --aux-dir=build-aux --avoid=errno --avoid=fcntl --avoid=fcntl-h --avoid=fstat --avoid=msvc-inval --avoid=msvc-nothrow --avoid=raise --avoid=select --avoid=sigprocmask --avoid=sys_types --avoid=threadlib --makefile-name=gnulib.mk --conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca-opt c-ctype c-strcase careadlinkat crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512 dtoastr dtotimespec dup2 environ filemode getloadavg getopt-gnu gettime gettimeofday ignore-value intprops largefile lstat manywarnings mktime pselect pthread_sigmask readlink socklen stat-time stdalign stdarg stdbool stdio strftime strtoimax strtoumax symlink sys_stat sys_time time timespec-add timespec-sub utimens warnings
+# Reproduce by: gnulib-tool --import --dir=. --lib=libgnu --source-base=lib --m4-base=m4 --doc-base=doc --tests-base=tests --aux-dir=build-aux --avoid=errno --avoid=fcntl --avoid=fcntl-h --avoid=fstat --avoid=msvc-inval --avoid=msvc-nothrow --avoid=raise --avoid=select --avoid=sigprocmask --avoid=sys_types --avoid=threadlib --makefile-name=gnulib.mk --conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca-opt c-ctype c-strcase careadlinkat crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512 dtoastr dtotimespec dup2 environ execinfo filemode getloadavg getopt-gnu gettime gettimeofday ignore-value intprops largefile lstat manywarnings mktime pselect pthread_sigmask readlink socklen stat-time stdalign stdarg stdbool stdio strftime strtoimax strtoumax symlink sys_stat sys_time time timespec-add timespec-sub utimens warnings
 
 
 MOSTLYCLEANFILES += core *.stackdump
@@ -150,6 +150,31 @@
 
 ## end   gnulib module dup2
 
+## begin gnulib module execinfo
+
+BUILT_SOURCES += $(EXECINFO_H)
+
+# We need the following in order to create <execinfo.h> when the system
+# doesn't have one that works.
+if GL_GENERATE_EXECINFO_H
+execinfo.h: execinfo.in.h $(top_builddir)/config.status
+	$(AM_V_GEN)rm -f $@-t $@ && \
+	{ echo '/* DO NOT EDIT! GENERATED AUTOMATICALLY! */'; \
+	  cat $(srcdir)/execinfo.in.h; \
+	} > $@-t && \
+	mv $@-t $@
+else
+execinfo.h: $(top_builddir)/config.status
+	rm -f $@
+endif
+MOSTLYCLEANFILES += execinfo.h execinfo.h-t
+
+EXTRA_DIST += execinfo.c execinfo.in.h
+
+EXTRA_libgnu_a_SOURCES += execinfo.c
+
+## end   gnulib module execinfo
+
 ## begin gnulib module filemode
 
 libgnu_a_SOURCES += filemode.c

=== added file 'm4/execinfo.m4'
--- m4/execinfo.m4	1970-01-01 00:00:00 +0000
+++ m4/execinfo.m4	2012-08-24 10:43:12 +0000
@@ -0,0 +1,31 @@
+# Check for GNU-style execinfo.h.
+
+dnl Copyright 2012 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+AC_DEFUN([gl_EXECINFO_H],
+[
+  AC_CHECK_HEADERS_ONCE([execinfo.h])
+
+  LIB_EXECINFO=''
+  EXECINFO_H='execinfo.h'
+
+  if test $ac_cv_header_execinfo_h = yes; then
+    gl_saved_libs=$LIBS
+      AC_SEARCH_LIBS([backtrace_symbols_fd], [execinfo],
+        [test "$ac_cv_search_backtrace_symbols_fd" = "none required" ||
+         LIB_EXECINFO=$ac_cv_search_backtrace_symbols_fd])
+    LIBS=$gl_saved_libs
+    test "$ac_cv_search_backtrace_symbols_fd" = no || EXECINFO_H=''
+  fi
+
+  if test -n "$EXECINFO_H"; then
+    AC_LIBOBJ([execinfo])
+  fi
+
+  AC_SUBST([EXECINFO_H])
+  AC_SUBST([LIB_EXECINFO])
+  AM_CONDITIONAL([GL_GENERATE_EXECINFO_H], [test -n "$EXECINFO_H"])
+])

=== modified file 'm4/gnulib-comp.m4'
--- m4/gnulib-comp.m4	2012-08-19 23:31:24 +0000
+++ m4/gnulib-comp.m4	2012-08-22 08:59:10 +0000
@@ -53,6 +53,7 @@
   # Code from module dtotimespec:
   # Code from module dup2:
   # Code from module environ:
+  # Code from module execinfo:
   # Code from module extensions:
   AC_REQUIRE([gl_USE_SYSTEM_EXTENSIONS])
   # Code from module extern-inline:
@@ -152,6 +153,7 @@
   gl_UNISTD_MODULE_INDICATOR([dup2])
   gl_ENVIRON
   gl_UNISTD_MODULE_INDICATOR([environ])
+  gl_EXECINFO_H
   AC_REQUIRE([gl_EXTERN_INLINE])
   gl_FILEMODE
   gl_GETLOADAVG
@@ -534,6 +536,8 @@
   lib/dtoastr.c
   lib/dtotimespec.c
   lib/dup2.c
+  lib/execinfo.c
+  lib/execinfo.in.h
   lib/filemode.c
   lib/filemode.h
   lib/ftoastr.c
@@ -605,6 +609,7 @@
   m4/clock_time.m4
   m4/dup2.m4
   m4/environ.m4
+  m4/execinfo.m4
   m4/extensions.m4
   m4/extern-inline.m4
   m4/filemode.m4

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-08-24 04:37:57 +0000
+++ src/ChangeLog	2012-08-24 10:43:12 +0000
@@ -1,5 +1,11 @@
 2012-08-24  Paul Eggert  <eggert@cs.ucla.edu>
 
+	On assertion failure, print backtrace if available.
+	* alloc.c [ENABLE_CHECKING]: Include <execinfo.h>.
+	(die) [ENABLE_CHECKING]: Print a backtrace if available.
+	* Makefile.in (LIB_EXECINFO): New macro.
+	(LIBES): Use it.
+
 	* buffer.c, buffer.h: Use bool for boolean.
 	* buffer.c (reset_buffer_local_variables)
 	(buffer_lisp_local_variables, Fset_buffer_modified_p)

=== modified file 'src/Makefile.in'
--- src/Makefile.in	2012-08-20 23:57:35 +0000
+++ src/Makefile.in	2012-08-24 10:43:12 +0000
@@ -159,6 +159,8 @@
 ## dbusbind.o if HAVE_DBUS, else empty.
 DBUS_OBJ = @DBUS_OBJ@
 
+LIB_EXECINFO=@LIB_EXECINFO@
+
 SETTINGS_CFLAGS = @SETTINGS_CFLAGS@
 SETTINGS_LIBS = @SETTINGS_LIBS@
 
@@ -383,6 +385,7 @@
 ## with GCC, we might need LIB_GCC again after them.
 LIBES = $(LIBS) $(LIBX_BASE) $(LIBX_OTHER) $(LIBSOUND) \
    $(RSVG_LIBS) $(IMAGEMAGICK_LIBS) $(LIB_CLOCK_GETTIME) $(DBUS_LIBS) \
+   $(LIB_EXECINFO) \
    $(LIBXML2_LIBS) $(LIBGPM) $(LIBRESOLV) $(LIBS_SYSTEM) \
    $(LIBS_TERMCAP) $(GETLOADAVG_LIBS) $(SETTINGS_LIBS) $(LIBSELINUX_LIBS) \
    $(FREETYPE_LIBS) $(FONTCONFIG_LIBS) $(LIBOTF_LIBS) $(M17N_FLT_LIBS) \

=== modified file 'src/alloc.c'
--- src/alloc.c	2012-08-21 23:39:56 +0000
+++ src/alloc.c	2012-08-22 08:59:10 +0000
@@ -6681,13 +6681,21 @@
 }
 
 #ifdef ENABLE_CHECKING
+
+# include <execinfo.h>
+
 bool suppress_checking;
 
 void
 die (const char *msg, const char *file, int line)
 {
+  enum { NPOINTERS_MAX = 500 };
+  void *buffer[NPOINTERS_MAX];
+  int npointers;
   fprintf (stderr, "\r\n%s:%d: Emacs fatal error: %s\r\n",
 	   file, line, msg);
+  npointers = backtrace (buffer, NPOINTERS_MAX);
+  backtrace_symbols_fd (buffer, npointers, STDERR_FILENO);
   abort ();
 }
 #endif

Re: C backtraces for Emacs

[Paul Eggert]

I've checked in that change (trunk bzr 109765 and 109766)
and so the Windows builds probably need tweaking now
along the lines previously discussed.

One other thing I just now noticed -- the Windows substitute
for unistd.h may need something like this:

#define STDIN_FILENO 0
#define STDOUT_FILENO 1
#define STDERR_FILENO 2

if Windows doesn't already have the equivalent.  The backtrace
code uses STDERR_FILENO rather than fileno (stderr)
as it's better not to rely on stdio's data structures
after an assertion has failed.

Re: C backtraces for Emacs

[Eli Zaretskii]

> Date: Fri, 24 Aug 2012 21:41:03 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org, Juanma Barranquero <lekktu@gmail.com>
> 
> I've checked in that change (trunk bzr 109765 and 109766)
> and so the Windows builds probably need tweaking now
> along the lines previously discussed.

Done.

> One other thing I just now noticed -- the Windows substitute
> for unistd.h may need something like this:
> 
> #define STDIN_FILENO 0
> #define STDOUT_FILENO 1
> #define STDERR_FILENO 2
> 
> if Windows doesn't already have the equivalent.

They are defined on stdio.h, so no changes are needed.

Re: inlinable functions instead of macros

[Paul Eggert]

On 08/21/2012 10:50 AM, Stefan Monnier wrote:

> Could some please check our inlinable functions and move any easserts in
> them to a macro (i.e. rename the function, replace the old name with
> a macro that does the assert and calls the inlinable function)?

I took a look at doing this, but it's problematic.

I assume we want to do this only with the "small" functions -- where
we're not interested in the function itself, but in the function's
caller.  Doing it with all inlinable functions would not be useful,
since so many functions are inlinable these days.

Unfortunately wrapping "small" functions with macros breaks down in
the common case where the caller is another "small" function.  For
example, set_hash_key_slot is a "small" function, and it calls another
"small" function gc_aset, which invokes eassert.  If both functions
are wrapped, an assertion failure in the inner reports the outer's
location, instead of what's wanted (the outer's caller's location).

How about if we try the following instead?  It's easier to implement
and is less intrusive to the code.  The idea is to use the <execinfo.h>
change that I mentioned in my previous message (a change we
should do independently anyway).  With that change, when an assertion
fails, it will generate output that looks like this:

alloc.c:800: Emacs fatal error: assertion failed: 0 <= nitems && 0 < item_size
./temacs[0x5bbdaf]
./temacs[0x5bf5e2]
./temacs[0x549cbd]
./temacs[0x417681]
/lib64/libc.so.6(__libc_start_main+0xed)[0x3b3082135d]
./temacs[0x418d85]

You can then find out the source locations corresponding to the
backtrace by giving those addresses to the debugger, like this:

$ gdb temacs
...
(gdb) b *0x5bbdaf
Breakpoint 2 at 0x5bbdaf: file alloc.c, line 6698.
(gdb) b *0x5bf5e2
Breakpoint 3 at 0x5bf5e2: file alloc.c, line 802.
(gdb) b *0x549cbd
Breakpoint 4 at 0x549cbd: file emacs.c, line 1799.

The top of the stack is 'die' itself; the second slot is the "small"
function xnmalloc; the third slot is the "big" function sort_args,
where it calls xnmalloc.

This gives all the information that the current scheme does, plus
more, because you have a full backtrace.  Once we have this, we
shouldn't need to create extra macro wrappers and address the extra
hassles that they'd cause.

Re: inlinable functions instead of macros

[Andreas Schwab]

Paul Eggert <eggert@cs.ucla.edu> writes:

> alloc.c:800: Emacs fatal error: assertion failed: 0 <= nitems && 0 < item_size
> ./temacs[0x5bbdaf]
> ./temacs[0x5bf5e2]
> ./temacs[0x549cbd]
> ./temacs[0x417681]
> /lib64/libc.so.6(__libc_start_main+0xed)[0x3b3082135d]
> ./temacs[0x418d85]
>
> You can then find out the source locations corresponding to the
> backtrace by giving those addresses to the debugger, like this:

Or use addr2line(1).

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

Re: inlinable functions instead of macros

[Stefan Monnier]

>> Could some please check our inlinable functions and move any easserts in
>> them to a macro (i.e. rename the function, replace the old name with
>> a macro that does the assert and calls the inlinable function)?
> I took a look at doing this, but it's problematic.

No it's not: we had that a few weeks ago.
I don't want to rely on execinfo for that.

It was working simply and effectively and want it to work again simply
and effectively, using plain C without relying on any special support
in glibc.


        Stefan

Re: inlinable functions instead of macros

[Paul Eggert]

On 08/22/2012 07:31 AM, Stefan Monnier wrote:
> It was working simply and effectively

It's simple to put easserts into macros, but as you know this has
problems that make the macros hard to use.  They cannot be invoked
from GDB, which complicates debugging.  More important, callers
sometimes cannot specify arguments with side effects, and this
restriction makes callers error-prone.  We've tried to address some of
the side-effect issues with this hack:

   /* The IDX==IDX tries to detect when the macro argument is side-effecting.  */
   #define ASET(ARRAY, IDX, VAL)	\
	 (eassert ((IDX) == (IDX)),				\
	  eassert ((IDX) >= 0 && (IDX) < ASIZE (ARRAY)),	\
	  XVECTOR (ARRAY)->contents[IDX] = (VAL))

but even this hack doesn't catch the side-effect problem reliably,
and anyway it's better to not introduce further problems like this.

Here's how I'd like to address this issue:

 1. Revert the setters and getters (e.g., bset_filename, BVAR)
    and go back to ordinary field accesses, as described in
    <http://bugs.gnu.org/12215#61>.  This will simplify the problem
    by reducing the number of functions/macros affected.
    We're still working on the details here but it's clear that
    most of these functions will go.

 2. Revert the easserts recently added to Emacs that are contributing
    to this problem.  We went without these easserts for years, and
    there's no particular reason to add them now if they cause problems.

 3. Assuming (1) and (2), only two problematic easserts remain: in
    blv_found and set_blv_found.  Address them with the simple solution
    that Sam Steingold proposed in
    <http://lists.gnu.org/archive/html/emacs-devel/2012-08/msg00641.html>,
    combined with an externally-visible wrapper for GDB's sake.

 4. After 24.3, look into fixing the side-effect problem for ASET and
    other macros too, as described in Bug#11935.  That's a bigger
    project, but it will have other benefits too.

Here's a patch to implement (2) and (3); it assumes the patch
mentioned in (1).  This patch makes Emacs a tiny bit smaller with
the default build, so it should be OK on efficiency grounds.

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-08-24 04:43:52 +0000
+++ src/ChangeLog	2012-08-24 21:24:45 +0000
@@ -1,5 +1,17 @@
 2012-08-24  Paul Eggert  <eggert@cs.ucla.edu>
 
+	Report better line numbers when assertions fail (Bug#12215).
+	* alloc.c, lisp.h (LISP_INLINE_EXTERNALLY_VISIBLE): New macro.
+	* buffer.h (buffer_intervals, set_buffer_intervals):
+	* lisp.h (gc_aset, vcopy): Remove recently-introduced easserts, as
+	they're not that useful in practice and they emit bogus line numbers.
+	* lisp.h (eassert, blv_found, set_blv_found):
+	Reimplement in terms of eassert_f, blv_found_f, set_blv_found_f.
+	(eassert_f, blv_found_f, set_blv_found_f): New macros or functions,
+	containing the guts of the old, but which let the caller specify
+	file and line number.
+	(blv_found, set_blv_found): Also define as a function, for GDB.
+
 	Remove setters like bset_keymap and getters like BVAR (Bug#12215).
 	They make the code harder to read, and for now they don't
 	bring anything to the table.  Open-code them instead.

=== modified file 'src/alloc.c'
--- src/alloc.c	2012-08-23 06:49:09 +0000
+++ src/alloc.c	2012-08-24 21:24:45 +0000
@@ -21,6 +21,7 @@
 #include <config.h>
 
 #define LISP_INLINE EXTERN_INLINE
+#define LISP_INLINE_EXTERNALLY_VISIBLE EXTERN_INLINE EXTERNALLY_VISIBLE
 
 #include <stdio.h>
 #include <limits.h>		/* For CHAR_BIT.  */

=== modified file 'src/buffer.h'
--- src/buffer.h	2012-08-24 04:43:52 +0000
+++ src/buffer.h	2012-08-24 21:24:45 +0000
@@ -948,7 +948,6 @@
 BUFFER_INLINE INTERVAL
 buffer_intervals (struct buffer *b)
 {
-  eassert (b->text != NULL);
   return b->text->intervals;
 }
 
@@ -957,7 +956,6 @@
 BUFFER_INLINE void
 set_buffer_intervals (struct buffer *b, INTERVAL i)
 {
-  eassert (b->text != NULL);
   b->text->intervals = i;
 }
 

=== modified file 'src/lisp.h'
--- src/lisp.h	2012-08-24 04:43:52 +0000
+++ src/lisp.h	2012-08-24 21:24:45 +0000
@@ -32,6 +32,7 @@
 INLINE_HEADER_BEGIN
 #ifndef LISP_INLINE
 # define LISP_INLINE INLINE
+# define LISP_INLINE_EXTERNALLY_VISIBLE INLINE
 #endif
 
 /* The ubiquitous max and min macros.  */
@@ -110,8 +111,13 @@
 /* Define an Emacs version of 'assert (COND)', since some
    system-defined 'assert's are flaky.  COND should be free of side
    effects; it may or may not be evaluated.  */
+
+#define eassert(cond) eassert_f (cond, __FILE__, __LINE__)
+
 #ifndef ENABLE_CHECKING
-# define eassert(X) ((void) (0 && (X))) /* Check that X compiles.  */
+  /* Do nothing, but check that args compile. */
+# define eassert_f(cond, file, line) \
+    ((void) (0 && (cond) && (file)[(line) >> 31]))
 #else /* ENABLE_CHECKING */
 
 extern _Noreturn void die (const char *, const char *, int);
@@ -126,10 +132,10 @@
    testing that STRINGP (x) is true, making the test redundant.  */
 extern bool suppress_checking EXTERNALLY_VISIBLE;
 
-# define eassert(cond)						\
+# define eassert_f(cond, file, line)				\
    ((cond) || suppress_checking					\
     ? (void) 0							\
-    : die ("assertion failed: " # cond, __FILE__, __LINE__))
+    : die ("assertion failed: " # cond, file, line))
 #endif /* ENABLE_CHECKING */
 \f
 /* Use the configure flag --enable-check-lisp-object-type to make
@@ -2335,7 +2341,6 @@
 {
   /* Like ASET, but also can be used in the garbage collector:
      sweep_weak_table calls set_hash_key etc. while the table is marked.  */
-  eassert (0 <= idx && idx < (ASIZE (array) & ~ARRAY_MARK_FLAG));
   XVECTOR (array)->contents[idx] = val;
 }
 
@@ -2344,7 +2349,6 @@
 LISP_INLINE void
 vcopy (Lisp_Object v, ptrdiff_t offset, Lisp_Object *args, ptrdiff_t count)
 {
-  eassert (0 <= offset && 0 <= count && offset + count <= ASIZE (v));
   memcpy (XVECTOR (v)->contents + offset, args, count * sizeof *args);
 }
 
@@ -2383,18 +2387,33 @@
 /* Buffer-local (also frame-local) variable access functions.  */
 
 LISP_INLINE int
-blv_found (struct Lisp_Buffer_Local_Value *blv)
+blv_found_f (struct Lisp_Buffer_Local_Value *blv, char const *file, int line)
 {
-  eassert (blv->found == !EQ (blv->defcell, blv->valcell));
+  eassert_f (blv->found == !EQ (blv->defcell, blv->valcell), file, line);
   return blv->found;
 }
+#define blv_found(blv) \
+     blv_found_f (blv, __FILE__, __LINE__)
+LISP_INLINE_EXTERNALLY_VISIBLE int
+(blv_found) (struct Lisp_Buffer_Local_Value *blv)
+{
+  return blv_found (blv);
+}
 
 LISP_INLINE void
-set_blv_found (struct Lisp_Buffer_Local_Value *blv, int found)
+set_blv_found_f (struct Lisp_Buffer_Local_Value *blv, int found,
+		 char const *file, int line)
 {
-  eassert (found == !EQ (blv->defcell, blv->valcell));
+  eassert_f (found == !EQ (blv->defcell, blv->valcell), file, line);
   blv->found = found;
 }
+#define set_blv_found(blv, found) \
+     set_blv_found_f (blv, found, __FILE__, __LINE__)
+LISP_INLINE_EXTERNALLY_VISIBLE void
+(set_blv_found) (struct Lisp_Buffer_Local_Value *blv, int found)
+{
+  set_blv_found (blv, found);
+}
 
 LISP_INLINE Lisp_Object
 blv_value (struct Lisp_Buffer_Local_Value *blv)

Re: inlinable functions instead of macros

[Tom Tromey]

>>>>> "Paul" == Paul Eggert <eggert@cs.ucla.edu> writes:

Paul> It's simple to put easserts into macros, but as you know this has
Paul> problems that make the macros hard to use.  They cannot be invoked
Paul> from GDB, which complicates debugging.

Why is that?  I haven't been following this, but I wonder if there is a
gdb deficiency we can easily address.

Paul> More important, callers sometimes cannot specify arguments with
Paul> side effects, and this restriction makes callers error-prone.
Paul> We've tried to address some of the side-effect issues with this
Paul> hack:

Paul>    /* The IDX==IDX tries to detect when the macro argument is side-effecting.  */
Paul>    #define ASET(ARRAY, IDX, VAL)	\
Paul> 	 (eassert ((IDX) == (IDX)),				\

I've often wished that GCC had a __builtin_assert_no_side_effects for
use in macros.

Tom

Re: inlinable functions instead of macros

[Paul Eggert]

On 08/24/2012 06:38 PM, Tom Tromey wrote:
> I wonder if there is a gdb deficiency we can easily address.

I looked into this and it turns out that it can be addressed,
on the platforms that support "gcc -g3".  -g3 is not the default,
which means random Emacs builds are unlikely to work with GDB and
macros.  Still, gcc -g3 works on my main platform, which removes
much of that issue with macros.

Macros' main problems are their gotchas with side effects and scoping,
alas, which gcc -g3 doesn't fix.

Re: inlinable functions instead of macros

[Eli Zaretskii]

> Date: Fri, 24 Aug 2012 19:58:10 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: emacs-devel@gnu.org
> 
> On 08/24/2012 06:38 PM, Tom Tromey wrote:
> > I wonder if there is a gdb deficiency we can easily address.
> 
> I looked into this and it turns out that it can be addressed,
> on the platforms that support "gcc -g3".  -g3 is not the default,
> which means random Emacs builds are unlikely to work with GDB and
> macros.

FWIW, I always wondered why we don't use "-ggdb -g3" by default, when
GCC is the compiler.

Re: inlinable functions instead of macros

[Tom Tromey]

Eli> FWIW, I always wondered why we don't use "-ggdb -g3" by default, when
Eli> GCC is the compiler.

Just '-g3' is enough (I don't think -ggdb has done anything important in
a long time).  I think it would be a good change.  I build Emacs (and
gdb and gcc) with -g3 routinely, as it makes debugging much simpler.

Tom

Re: inlinable functions instead of macros

[Paul Eggert]

On 08/25/2012 01:13 PM, Tom Tromey wrote:
> Just '-g3' is enough (I don't think -ggdb has done anything important in
> a long time).  I think it would be a good change.

OK, thanks, I gave it a shot by having 'configure' default to -g3
instead of plain -g, if -g3 works.  This affects the build only when
the user of 'configure' does not specify CFLAGS.  I installed this as
trunk bzr 109779.

Re: inlinable functions instead of macros

[Stefan Monnier]

> problems that make the macros hard to use.  They cannot be invoked
> from GDB, which complicates debugging.  More important, callers

Most of those macros don't need to be used from GDB (e.g. I've never
needed to use ASET from GDB).

> restriction makes callers error-prone.  We've tried to address some of
> the side-effect issues with this hack:

>    /* The IDX==IDX tries to detect when the macro argument is side-effecting.  */
>    #define ASET(ARRAY, IDX, VAL)	\
> 	 (eassert ((IDX) == (IDX)),				\

This should be removed indeed.  I added it temporarily and in any case
it doesn't work (e.g. "i++ == i++" will always return true in my
experience).

> +	Reimplement in terms of eassert_f, blv_found_f, set_blv_found_f.
> +	(eassert_f, blv_found_f, set_blv_found_f): New macros or functions,
> +	containing the guts of the old, but which let the caller specify
> +	file and line number.

I'd rather avoid these kinds of efforts,


        Stefan

Re: inlinable functions instead of macros

[Paul Eggert]

On 08/24/2012 07:05 PM, Stefan Monnier wrote:
> I've never needed to use ASET from GDB

I guess our debugging styles differ.  But this issue is moot
now that -g3 is the default.

>>    /* The IDX==IDX tries to detect when the macro argument is side-effecting.  */
>>    #define ASET(ARRAY, IDX, VAL)	\
>> 	 (eassert ((IDX) == (IDX)),				\
> 
> This should be removed indeed.

That's easy, and I did that in trunk bzr 109780.

>> +	Reimplement in terms of eassert_f,...
> 
> I'd rather avoid these kinds of efforts,

Me too.  I'd rather rely on the new execinfo stuff.
Perhaps after you've had a chance to give execinfo a try,
we can see whether we can improve matters further
in this area.

Re: inlinable functions instead of macros

[Eli Zaretskii]

> Date: Wed, 22 Aug 2012 02:28:52 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: emacs-devel@gnu.org
> 
> alloc.c:800: Emacs fatal error: assertion failed: 0 <= nitems && 0 < item_size
> ./temacs[0x5bbdaf]
> ./temacs[0x5bf5e2]
> ./temacs[0x549cbd]
> ./temacs[0x417681]
> /lib64/libc.so.6(__libc_start_main+0xed)[0x3b3082135d]
> ./temacs[0x418d85]
> 
> You can then find out the source locations corresponding to the
> backtrace by giving those addresses to the debugger, like this:
> 
> $ gdb temacs
> ...
> (gdb) b *0x5bbdaf
> Breakpoint 2 at 0x5bbdaf: file alloc.c, line 6698.
> (gdb) b *0x5bf5e2
> Breakpoint 3 at 0x5bf5e2: file alloc.c, line 802.
> (gdb) b *0x549cbd
> Breakpoint 4 at 0x549cbd: file emacs.c, line 1799.

Also, "l *0x5bbdaf" etc. will show source lines around the offending
line (6698 etc.).

Re: inlinable functions instead of macros

[Richard Stallman]

    lisp.h:2416: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)

    I.e. the file&line info is always the same rather than giving the
    file&line where the inlinable function was called.

Could a GCC improvement be a good fix for this?  For instance, if it
allowed the assertion failure to find and mention the source location
of the function's caller?

--
Dr Richard Stallman
President, Free Software Foundation
51 Franklin St
Boston MA 02110
USA
www.fsf.org  www.gnu.org
Skype: No way! That's nonfree (freedom-denying) software.
  Use Ekiga or an ordinary phone call

Re: inlinable functions instead of macros

[Paul Eggert]

On 08/18/2012 12:08 PM, Richard Stallman wrote:
> Could a GCC improvement be a good fix for this?  For instance, if it
> allowed the assertion failure to find and mention the source location
> of the function's caller?

That could be done via GCC.  It should also be doable without
changing the compiler, by enhancing glibc's 'backtrace' function.
GNU binutils contains code to do this, in its addr2line command,
and presumably we could use similar code inside Emacs.  Emacs
could link to libbfd on systems that have it, or it could
bundle libbfd, or it could invoke the command-line tool
addr2line to get the relevant info.  Whichever's easiest,
presumably.

Re: inlinable functions instead of macros

[Eli Zaretskii]

> Date: Sat, 18 Aug 2012 13:57:58 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: Stefan Monnier <monnier@IRO.UMontreal.CA>, emacs-devel@gnu.org
> 
> On 08/18/2012 12:08 PM, Richard Stallman wrote:
> > Could a GCC improvement be a good fix for this?  For instance, if it
> > allowed the assertion failure to find and mention the source location
> > of the function's caller?
> 
> That could be done via GCC.

What do __FUNCTION__ and __func__ hold for an inlined function?

Re: inlinable functions instead of macros

[Paul Eggert]

On 08/18/2012 02:03 PM, Eli Zaretskii wrote:
> What do __FUNCTION__ and __func__ hold for an inlined function?

The name of the inlined function itself, which is not what's
wanted here.

Re: inlinable functions instead of macros

[Eli Zaretskii]

> Date: Sat, 18 Aug 2012 14:31:13 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> On 08/18/2012 02:03 PM, Eli Zaretskii wrote:
> > What do __FUNCTION__ and __func__ hold for an inlined function?
> 
> The name of the inlined function itself, which is not what's
> wanted here.

Sounds like this could be a starting point for a GCC feature that is
wanted here.

Re: inlinable functions instead of macros

[Óscar Fuentes]

Richard Stallman <rms@gnu.org> writes:

>     lisp.h:2416: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)
>
>     I.e. the file&line info is always the same rather than giving the
>     file&line where the inlinable function was called.
>
> Could a GCC improvement be a good fix for this?  For instance, if it
> allowed the assertion failure to find and mention the source location
> of the function's caller?

It looks quite difficult to even give a sound specification for such
feature. And I bet that the implementation, if possible at all, would be
very complex and extensive.

Either print a backtrace or revert to #define's.

Re: inlinable functions instead of macros

[Florian Weimer]

* Stefan Monnier:

> While inlinable functions are much cleaner than macros, they have
> a very serious downside: you just end up with
>
> lisp.h:2416: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)
>
> I.e. the file&line info is always the same rather than giving the
> file&line where the inlinable function was called.

Coincidentally, there's a related discussion on the gcc mailing list:
<http://gcc.gnu.org/ml/gcc/2012-08/msg00172.html>

The approach we settled for is C++-specific, though.

Re: inlinable functions instead of macros

[Sam Steingold]

> * Stefan Monnier <zbaavre@VEB.HZbagerny.PN> [2012-08-17 19:48:50 -0400]:
>
> While inlinable functions are much cleaner than macros, they have
> a very serious downside: you just end up with
>
> lisp.h:2416: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)
>
> I.e. the file&line info is always the same rather than giving the
> file&line where the inlinable function was called.

a simple solution is this:

inline void my_inlinable_function (....., char* file, int line) {....}
#deinfe my_inlinable_function(.....)  (my_inlinable_function)(.....,FILE,LINE)

-- 
Sam Steingold (http://sds.podval.org/) on Ubuntu 12.04 (precise) X 11.0.11103000
http://www.childpsy.net/ http://pmw.org.il
http://www.PetitionOnline.com/tap12009/ http://dhimmi.com http://jihadwatch.org
A man paints with his brains and not with his hands.