From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Paul Eggert Newsgroups: gmane.emacs.devel Subject: Re: inlinable functions instead of macros Date: Fri, 24 Aug 2012 14:37:21 -0700 Organization: UCLA Computer Science Department Message-ID: <5037F411.4010905@cs.ucla.edu> References: <502EDAF3.6030005@cs.ucla.edu> <5034A654.2080909@cs.ucla.edu> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1345844264 27914 80.91.229.3 (24 Aug 2012 21:37:44 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 24 Aug 2012 21:37:44 +0000 (UTC) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Aug 24 23:37:44 2012 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1T51ZV-0003ip-49 for ged-emacs-devel@m.gmane.org; Fri, 24 Aug 2012 23:37:41 +0200 Original-Received: from localhost ([::1]:34012 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T51ZT-0007HY-4d for ged-emacs-devel@m.gmane.org; Fri, 24 Aug 2012 17:37:39 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:49252) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T51ZK-0007HF-Lf for emacs-devel@gnu.org; Fri, 24 Aug 2012 17:37:37 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T51ZD-0008Ka-EX for emacs-devel@gnu.org; Fri, 24 Aug 2012 17:37:30 -0400 Original-Received: from smtp.cs.ucla.edu ([131.179.128.62]:34346) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T51ZD-0008KO-2g for emacs-devel@gnu.org; Fri, 24 Aug 2012 17:37:23 -0400 Original-Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id 600CB39E8019; Fri, 24 Aug 2012 14:37:21 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Original-Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 5cv2+0rlC0g1; Fri, 24 Aug 2012 14:37:20 -0700 (PDT) Original-Received: from [192.168.1.3] (pool-108-23-119-2.lsanca.fios.verizon.net [108.23.119.2]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id EC1E539E800D; Fri, 24 Aug 2012 14:37:19 -0700 (PDT) User-Agent: Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20120714 Thunderbird/14.0 In-Reply-To: X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 131.179.128.62 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:152819 Archived-At: 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 . 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 , 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 + 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 #define LISP_INLINE EXTERN_INLINE +#define LISP_INLINE_EXTERNALLY_VISIBLE EXTERN_INLINE EXTERNALLY_VISIBLE #include #include /* 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 */ /* 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)