Re: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch

Re: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch

[Pip Cet]

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

On Sun, Jul 21, 2019 at 7:32 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>  static void
> -allow_garbage_collection (void *ptr)
> +allow_garbage_collection (intmax_t consing)
>  {
> -  object_ct *p = ptr;
> -  consing_until_gc = *p;
> +  consing_until_gc = consing;

Shouldn't we count the allocations that happened while GC was
inhibited and subtract them from consing_until_gc afterwards?

Also, should garbage_collect_1 use inhibit_garbage_collection rather
than fiddling with consing_until_gc directly?

Attaching a patch that does both.

[-- Attachment #2: 0001-Trigger-GC-earlier-after-garbage-collection-was-inhi.patch --]
[-- Type: text/x-patch, Size: 1199 bytes --]

From 7da1122b9bf4f0ab8da6958bca36c9bc0726564e Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Mon, 22 Jul 2019 04:09:29 +0000
Subject: [PATCH] Trigger GC earlier after garbage collection was inhibited

* src/alloc.c (allow_garbage_collection): Don't ignore allocations
that happen while garbage collection is inhibited.
(garbage_collect_1): use `inhibit-garbage-collection'.
---
 src/alloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index aa9200f2eb..a32302f6e2 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -5507,7 +5507,7 @@ staticpro (Lisp_Object const *varaddress)
 static void
 allow_garbage_collection (intmax_t consing)
 {
-  consing_until_gc = consing;
+  consing_until_gc += consing - OBJECT_CT_MAX;
   garbage_collection_inhibited--;
 }
 
@@ -5823,7 +5823,7 @@ garbage_collect_1 (struct gcstat *gcst)
 
   /* In case user calls debug_print during GC,
      don't let that cause a recursive GC.  */
-  consing_until_gc = OBJECT_CT_MAX;
+  inhibit_garbage_collection ();
 
   /* Save what's currently displayed in the echo area.  Don't do that
      if we are GC'ing because we've run out of memory, since
-- 
2.22.0

Re: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch

[Stefan Monnier]

> Shouldn't we count the allocations that happened while GC was
> inhibited and subtract them from consing_until_gc afterwards?

I don't see why: they are counted already in consing_since_gc, no?


        Stefan

Re: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch

[Paul Eggert]

Stefan Monnier wrote:
>> Shouldn't we count the allocations that happened while GC was
>> inhibited and subtract them from consing_until_gc afterwards?
> 
> I don't see why: they are counted already in consing_since_gc, no?

Yes and no. The code keeps subtracting from consing_since_gc while GC is 
inhibited (and consing_since_gc is enormous); but when allow_garbage_collection 
is called, all those subtractions are lost.

I installed the first part of Pip Cet's patch (rewritten slightly to I hope make 
it clearer) - thanks, Pip. The second part I didn't see the need for, as it made 
GC a tiny bit slower and arguably made GC harder to follow.

Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Eli Zaretskii]

Please be aware that these changes might potentially affect the
schedule of the Emacs 27 release.

Emacs 27 has accrued many new features and important improvements, and
has proven to be generally very stable.  My intentions were to start
its pretest in the September-October time-frame, with the goal of
releasing Emacs 27.1 a few months later.

Your changes in GC, which is a very delicate part of Emacs, might
affect its stability and force us to analyze and fix hard-to-debug
crashes, and thus postpone the beginning of the pretest and the
subsequent release.  I think we should try to avoid postponing the
27.1 release, what with the many new features it will give our users.

The same goes for removing the pure space, IMO: another core feature
whose existence and traits many parts of Emacs came to take for
granted.

I'm all for improving GC and simplifying our memory management, but
please keep the above in your minds when you play with this stuff.
Especially as, judging by the changes you are making, the details and
indeed some of the aspects of the idea of the changes, are not yet
sufficiently clear/finalized.

An alternative would be to make these changes on a branch, and merge
that branch when it is sufficiently stable and mature.  Please
consider this possibility.  After all, these two issues are not
terribly urgent to fix (unlike, say, the unexec thingy).

TIA

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Paul Eggert]

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

Eli Zaretskii wrote:
> After all, these two issues are not
> terribly urgent to fix (unlike, say, the unexec thingy).

As it happens, the issue with hash tables is bound up with the unexec thingy, in 
that portable dumping currently can screw up hash tables with user-defined 
tests. I discovered this over the weekend while looking into Pip Cet's proposed 
changes to how portable dumping treats hash tables, and have a contrived test 
case (see attached) that causes gethash to fail with "Lisp nesting exceeds 
‘max-lisp-eval-depth’" in an Emacs started via a portable dump, even though the 
same gethash works fine before dumping. I plan to install a workaround for this 
soon. So all in all I think the hash-table changes should improve Emacs 
stability even if we were planning to split off a new release branch next week, 
much less September.

I take your point that getting rid of pure space might be a bridge too far now - 
unless someone can show that pure space also runs afoul of portable dumping.

[-- Attachment #2: trouble.sh --]
[-- Type: application/x-shellscript, Size: 663 bytes --]

Re: Changes in GC and in pure space

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 22 Jul 2019 10:47:27 -0700
> 
> > After all, these two issues are not
> > terribly urgent to fix (unlike, say, the unexec thingy).
> 
> As it happens, the issue with hash tables is bound up with the unexec thingy, in 
> that portable dumping currently can screw up hash tables with user-defined 
> tests.

I wasn't talking about the hash tables; that bug needs to be fixed,
indeed.  I was talking about making more thorough changes in GC (and
elsewhere) than are strictly needed for fixing the hash-table bug.
(AFAIR, the fix for the hash tables didn't involve GC, last time I
looked.)

Re: Changes in GC and in pure space

[Stefan Monnier]

>> As it happens, the issue with hash tables is bound up with the unexec thingy, in 
>> that portable dumping currently can screw up hash tables with user-defined 
>> tests.

Note that AFAIK we currently don't dump any such hash-tables, so if
there's a bug there it only affects the case of the user *re*dumping
with a special config, which AFAIK we do not intend to officially
support yet for 27.1.

> I wasn't talking about the hash tables; that bug needs to be fixed,
> indeed.

Not sure which bug you're referring to, because the one discussed
recently (linked to the pdumper's need to rehash tables) should be fixed
already (Pip Cet provided both that simple fix I installed as well as
a subsequent better but more intrusive fix).


        Stefan

Re: Changes in GC and in pure space

[Paul Eggert]

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

Stefan Monnier wrote:
>>> As it happens, the issue with hash tables is bound up with the unexec thingy, in
>>> that portable dumping currently can screw up hash tables with user-defined
>>> tests.
> 
> Note that AFAIK we currently don't dump any such hash-tables, so if
> there's a bug there it only affects the case of the user *re*dumping
> with a special config, which AFAIK we do not intend to officially
> support yet for 27.1.

Although I vaguely recall an email conversation to that effect, I don't see it 
in the Emacs documentation. And since unexec is now disabled by default, if 
people want to use any dumping method at all then by default they'll be 
redumping in the way that you suggest. If we really intend to not support that, 
we should be clearer about it in NEWS and documentation.

At any rate the simplest workaround is to avoid pdumping the problematic hash 
tables, and I installed the attached patch to do that. We can add support for 
pdumping those hash tables later, if anybody cares about it.

[-- Attachment #2: 0001-Do-not-pdump-user-defined-hashtabs.patch --]
[-- Type: text/x-patch, Size: 2119 bytes --]

From 3f4a9a5a3b267fbc13a8bebc4295bbfadd6ff03e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 22 Jul 2019 18:33:39 -0700
Subject: [PATCH] Do not pdump user-defined hashtabs

* src/pdumper.c (dump_hash_table_stable_p):
Signal an error if a hash table has user-defined tests (Bug#36769).
* src/fns.c (hashfn_user_defined): Now extern.
---
 src/fns.c     | 2 +-
 src/lisp.h    | 1 +
 src/pdumper.c | 2 ++
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/fns.c b/src/fns.c
index 8eefa7c72b..734a2e253c 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -4017,7 +4017,7 @@ hashfn_eql (Lisp_Object key, struct Lisp_Hash_Table *h)
 /* Given HT, return a hash code for KEY which uses a user-defined
    function to compare keys.  */
 
-static Lisp_Object
+Lisp_Object
 hashfn_user_defined (Lisp_Object key, struct Lisp_Hash_Table *h)
 {
   Lisp_Object args[] = { h->test.user_hash_function, key };
diff --git a/src/lisp.h b/src/lisp.h
index 9d37629bc4..e96fcfe94d 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3606,6 +3606,7 @@ EMACS_UINT hash_string (char const *, ptrdiff_t);
 EMACS_UINT sxhash (Lisp_Object, int);
 Lisp_Object hashfn_eql (Lisp_Object, struct Lisp_Hash_Table *);
 Lisp_Object hashfn_equal (Lisp_Object, struct Lisp_Hash_Table *);
+Lisp_Object hashfn_user_defined (Lisp_Object, struct Lisp_Hash_Table *);
 Lisp_Object make_hash_table (struct hash_table_test, EMACS_INT, float, float,
                              Lisp_Object, bool);
 ptrdiff_t hash_lookup (struct Lisp_Hash_Table *, Lisp_Object, Lisp_Object *);
diff --git a/src/pdumper.c b/src/pdumper.c
index 2abac80a37..cda8b40be4 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -2628,6 +2628,8 @@ dump_vectorlike_generic (struct dump_context *ctx,
 static bool
 dump_hash_table_stable_p (const struct Lisp_Hash_Table *hash)
 {
+  if (hash->test.hashfn == hashfn_user_defined)
+    error ("cannot dump hash tables with user-defined tests");  /* Bug#36769 */
   bool is_eql = hash->test.hashfn == hashfn_eql;
   bool is_equal = hash->test.hashfn == hashfn_equal;
   ptrdiff_t size = HASH_TABLE_SIZE (hash);
-- 
2.17.1

Re: Changes in GC and in pure space

[Eli Zaretskii]

> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 22 Jul 2019 18:43:45 -0700
> 
> > Note that AFAIK we currently don't dump any such hash-tables, so if
> > there's a bug there it only affects the case of the user *re*dumping
> > with a special config, which AFAIK we do not intend to officially
> > support yet for 27.1.
> 
> Although I vaguely recall an email conversation to that effect, I don't see it 
> in the Emacs documentation. And since unexec is now disabled by default, if 
> people want to use any dumping method at all then by default they'll be 
> redumping in the way that you suggest. If we really intend to not support that, 
> we should be clearer about it in NEWS and documentation.

We do want to support re-dumping eventually, but we are clearly not
there yet.  If you tried that, you probably saw a few quirks very
quickly.  It will take time to rectify all of them, and I agree we
shouldn't aim for that milestone in Emacs 27.1.

Re: Changes in GC and in pure space

[Paul Eggert]

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

>> we should be clearer about it in NEWS and documentation.
> 
> We do want to support re-dumping eventually, but we are clearly not
> there yet.

OK, I added something to that effect to the NEWS file and to internals.texi. 
While in the neighborhood, I also put in a statement that unexec is deprecated 
and rewrote instances of "portable dump file" as the dump files themselves are 
no more portable than an Emacs executable is. See attached patch.

[-- Attachment #2: 0001-Improve-pdumper-doc-say-unexec-is-deprecated.patch --]
[-- Type: text/x-patch, Size: 15372 bytes --]

From 8dd5b6ea56c38669bc98104ee2d6b31496624d28 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 23 Jul 2019 09:19:09 -0700
Subject: [PATCH] Improve pdumper doc; say unexec is deprecated

Say that pdumping cannot redump unless -batch is used.  Say that
the traditional unexec dumping method is by default not available,
and is deprecated.  Don't call dump files "portable", as dump files
are not any more portable than the Emacs executables themselves.
Just call them "dump files".  Similar, prefer "portable dumper"
(since the dumper code is portable) to "portable dumping" (since
the dump file is not).  Be more systematic about calling them
"dump files" instead of "dumped images" or whatnot.
---
 doc/lispref/internals.texi | 34 ++++++++++++++++++++--------------
 etc/NEWS                   | 14 +++++++++-----
 etc/TODO                   |  6 +-----
 lib/fingerprint.h          |  3 +--
 lisp/startup.el            |  4 ++--
 src/alloc.c                |  4 ++--
 src/emacs.c                |  6 +++---
 src/minibuf.c              |  2 +-
 src/pdumper.c              | 10 +++++-----
 src/pdumper.h              |  4 ++--
 src/unexaix.c              |  2 +-
 11 files changed, 47 insertions(+), 42 deletions(-)

diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index 72066d34f4..f85c266ede 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -61,10 +61,10 @@ Building Emacs
 
 @table @samp
 @item pdump
-@cindex portable dump file
-Record the preloaded Lisp data in a @dfn{portable dump} file.  This
+@cindex dump file
+Record the preloaded Lisp data in a @dfn{dump file}.  This
 method produces an additional data file which Emacs will load at
-startup.  The portable dump file is usually called @file{emacs.pdmp},
+startup.  The produced dump file is usually called @file{emacs.pdmp},
 and is installed in the Emacs @code{exec-directory} (@pxref{Help
 Functions}).  This method is the most preferred one, as it does not
 require Emacs to employ any special techniques of memory allocation,
@@ -75,7 +75,7 @@ Building Emacs
 @cindex bootstrapping Emacs
 Like @samp{pdump}, but used while @dfn{bootstrapping} Emacs, when no
 previous Emacs binary and no @file{*.elc} byte-compiled Lisp files are
-available.  The produced portable dump file is usually named
+available.  The produced dump file is usually named
 @file{bootstrap-emacs.pdmp} in this case.
 
 @item dump
@@ -88,6 +88,8 @@ Building Emacs
 dumped Emacs.)  This method is also known as @dfn{unexec}, because it
 produces a program file from a running process, and thus is in some
 sense the opposite of executing a program to start a process.
+Although this method was the way that Emacs traditionally saved its
+state, it is now deprecated.
 
 @item bootstrap
 Like @samp{dump}, but used when bootstrapping Emacs with the
@@ -97,11 +99,11 @@ Building Emacs
 @cindex preloaded Lisp files
 @vindex preloaded-file-list
   The dumped @file{emacs} executable (also called a @dfn{pure} Emacs)
-is the one which is installed.  If the portable dumping was used to
+is the one which is installed.  If the portable dumper was used to
 build Emacs, the @file{emacs} executable is actually an exact copy of
 @file{temacs}, and the corresponding @file{emacs.pdmp} file is
 installed as well.  The variable @code{preloaded-file-list} stores a
-list of the preloaded Lisp files recorded in the portable dump file or
+list of the preloaded Lisp files recorded in the dump file or
 in the dumped Emacs executable.  If you port Emacs to a new operating
 system, and are not able to implement dumping of any kind, then Emacs
 must load @file{loadup.el} each time it starts.
@@ -201,15 +203,19 @@ Building Emacs
 @code{before-init-hook} (@pxref{Startup Summary}).
 
 @defun dump-emacs-portable to-file &optional track-referrers
-This function dumps the current state of Emacs into a portable dump
+This function dumps the current state of Emacs into a dump
 file @var{to-file}, using the @code{pdump} method.  Normally, the
-portable dump file is called @file{@var{emacs-name}.dmp}, where
+dump file is called @file{@var{emacs-name}.dmp}, where
 @var{emacs-name} is the name of the Emacs executable file.  The
 optional argument @var{track-referrers}, if non-@code{nil}, causes the
-portable dumping process keep additional information to help track
+portable dumper to keep additional information to help track
 down the provenance of object types that are not yet supported by the
 @code{pdump} method.
 
+Although the portable dumper code can run on many platforms, the dump
+files that it produces are not portable---they can be loaded only by
+the Emacs executable that dumped them.
+
 If you want to use this function in an Emacs that was already dumped,
 you must run Emacs with the @samp{-batch} option.
 @end defun
@@ -220,20 +226,20 @@ Building Emacs
 @var{to-file}, using the @code{unexec} method.  It takes symbols from
 @var{from-file} (this is normally the executable file @file{temacs}).
 
-This function cannot be used in an Emacs that was already dumped.  If
-Emacs was built without @code{unexec} support, this function will not
-be available.
+This function cannot be used in an Emacs that was already dumped.
+This function is deprecated, and by default Emacs is built without
+@code{unexec} support so this function is not available.
 @end defun
 
 @defun pdumper-stats
-If the current Emacs session restored its state from a portable dump
+If the current Emacs session restored its state from a dump
 file, this function returns information about the dump file and the
 time it took to restore the Emacs state.  The value is an alist
 @w{@code{((dumped-with-pdumper . t) (load-time . @var{time})
 (dump-file-name . @var{file}))}},
 where @var{file} is the name of the dump file, and @var{time} is the
 time in seconds it took to restore the state from the dump file.
-If the current session was not restored from a portable dump file, the
+If the current session was not restored from a dump file, the
 value is nil.
 @end defun
 
diff --git a/etc/NEWS b/etc/NEWS
index 5378e56bca..7fd2214582 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -92,11 +92,6 @@ and in particular better supports the Address Space Layout
 Randomization (ASLR) feature, a security technique used by most modern
 operating systems.
 
-Portable dumping can be disabled at configure time via the configure
-option '--with-dumping=unexec' (but we don't recommend that, unless
-the portable dumping doesn't work on your system for some
-reason---please report such systems to the Emacs developers as bugs).
-
 When built with the portable dumping support (which is the default),
 Emacs looks for the 'emacs.pdmp' file, generated during the build, in
 its data directory at startup, and loads the dumped state from there.
@@ -104,6 +99,15 @@ The new command-line argument '--dump-file=FILE' allows to specify a
 non-default '.pdmp' file to load the state from; see the node "Initial
 Options" in the Emacs manual for more information.
 
+An Emacs started via a dump file can create a new dump file only if it
+was invoked with the -batch option.
+
+Although the portable dumper has been tested, it may have a bug on
+unusual platforms.  If you require traditional unexec dumping you can
+use the configure-time option '--with-dumping=unexec'; however, please
+file a bug report describing the situation, as unexec dumping is
+deprecated.
+
 +++
 ** The new configure option '--enable-checking=structs' attempts to
 check that the portable dumper code has been updated to match the last
diff --git a/etc/TODO b/etc/TODO
index b2446b0d91..a065763933 100644
--- a/etc/TODO
+++ b/etc/TODO
@@ -297,11 +297,7 @@ One way of doing this is to start with fx's dynamic loading, and use it
 to implement things like auto-loaded buffer parsers and database
 access in cases which need more than Lisp.
 
-** Replace unexec with a more portable form of dumping
-See eg https://lists.gnu.org/r/emacs-devel/2014-01/msg01034.html
-       https://lists.gnu.org/r/emacs-devel/2014-06/msg00452.html
-
-One way is to provide portable undumping using mmap (per gerd design).
+** Fix portable dumping so that you can redump without using -batch.
 
 ** Imenu could be extended into a file-structure browsing mechanism
 using code like that of customize-groups.
diff --git a/lib/fingerprint.h b/lib/fingerprint.h
index ba2e740cd9..7d2160c988 100644
--- a/lib/fingerprint.h
+++ b/lib/fingerprint.h
@@ -22,8 +22,7 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 
 /* We generate fingerprint.c and fingerprint.o from all the sources in
    Emacs.  This way, we have a unique value that we can use to pair
-   data files (like a portable dump image) with a specific build of
-   Emacs.  */
+   data files (like a dump file) with a specific build of Emacs.  */
 extern volatile unsigned char fingerprint[32];
 
 #endif
diff --git a/lisp/startup.el b/lisp/startup.el
index 7759ed5aed..564428580b 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -354,8 +354,8 @@ site-run-file
   "File containing site-wide run-time initializations.
 This file is loaded at run-time before `~/.emacs'.  It contains inits
 that need to be in place for the entire site, but which, due to their
-higher incidence of change, don't make sense to load into Emacs's
-dumped image.  Thus, the run-time load order is: 1. file described in
+higher incidence of change, don't make sense to put into Emacs's
+dump file.  Thus, the run-time load order is: 1. file described in
 this variable, if non-nil; 2. `~/.emacs'; 3. `default.el'.
 
 Don't use the `site-start.el' file for things some users may not like.
diff --git a/src/alloc.c b/src/alloc.c
index 5d8003ffb5..f256ff71b0 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4531,9 +4531,9 @@ mark_maybe_object (Lisp_Object obj)
 
   void *po = XPNTR (obj);
 
-  /* If the pointer is in the dumped image and the dump has a record
+  /* If the pointer is in the dump image and the dump has a record
      of the object starting at the place where the pointer points, we
-     definitely have an object.  If the pointer is in the dumped image
+     definitely have an object.  If the pointer is in the dump image
      and the dump has no idea what the pointer is pointing at, we
      definitely _don't_ have an object.  */
   if (pdumper_object_p (po))
diff --git a/src/emacs.c b/src/emacs.c
index ad661a081b..cc5818393a 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -686,7 +686,7 @@ dump_error_to_string (enum pdumper_load_result result)
 }
 
 /* Find a path (absolute or relative) to the Emacs executable.
-   Called early in initialization by portable dump loading code, so we
+   Called early in initialization by portable dumper loading code, so we
    can't use lisp and associated machinery.  On success, *EXENAME is
    set to a heap-allocated string giving a path to the Emacs
    executable or to NULL if we can't determine the path immediately.
@@ -801,12 +801,12 @@ load_pdump (int argc, char **argv)
     ;
 
   /* TODO: maybe more thoroughly scrub process environment in order to
-     make this use case (loading a pdumper image in an unexeced emacs)
+     make this use case (loading a dump file in an unexeced emacs)
      possible?  Right now, we assume that things we don't touch are
      zero-initialized, and in an unexeced Emacs, this assumption
      doesn't hold.  */
   if (initialized)
-    fatal ("cannot load pdumper image in unexeced Emacs");
+    fatal ("cannot load dump file in unexeced Emacs");
 
   /* Look for an explicitly-specified dump file.  */
   const char *path_exec = PATH_EXEC;
diff --git a/src/minibuf.c b/src/minibuf.c
index d9a6e15b05..8920f37827 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -1885,7 +1885,7 @@ init_minibuf_once_for_pdumper (void)
   PDUMPER_IGNORE (minibuf_prompt_width);
 
   /* We run this function on first initialization and whenever we
-     restore from a pdumper image.  pdumper doesn't try to preserve
+     restore from a dump file.  pdumper doesn't try to preserve
      frames, windows, and so on, so reset everything related here.  */
   Vminibuffer_list = Qnil;
   minibuf_level = 0;
diff --git a/src/pdumper.c b/src/pdumper.c
index cda8b40be4..84147353e8 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -333,8 +333,8 @@ dump_fingerprint (char const *label,
   fprintf (stderr, "%s: %.*s\n", label, hexbuf_size, hexbuf);
 }
 
-/* Format of an Emacs portable dump file.  All offsets are relative to
-   the beginning of the file.  An Emacs portable dump file is coupled
+/* Format of an Emacs dump file.  All offsets are relative to
+   the beginning of the file.  An Emacs dump file is coupled
    to exactly the Emacs binary that produced it, so details of
    alignment and endianness are unimportant.
 
@@ -3990,7 +3990,7 @@ dump_drain_deferred_symbols (struct dump_context *ctx)
 DEFUN ("dump-emacs-portable",
        Fdump_emacs_portable, Sdump_emacs_portable,
        1, 2, 0,
-       doc: /* Dump current state of Emacs into portable dump file FILENAME.
+       doc: /* Dump current state of Emacs into dump file FILENAME.
 If TRACK-REFERRERS is non-nil, keep additional debugging information
 that can help track down the provenance of unsupported object
 types.  */)
@@ -5470,14 +5470,14 @@ pdumper_record_wd (const char *wd)
 
 DEFUN ("pdumper-stats", Fpdumper_stats, Spdumper_stats, 0, 0, 0,
        doc: /* Return statistics about portable dumping used by this session.
-If this Emacs sesion was started from a portable dump file,
+If this Emacs session was started from a dump file,
 the return value is an alist of the form:
 
   ((dumped-with-pdumper . t) (load-time . TIME) (dump-file-name . FILE))
 
 where TIME is the time in seconds it took to restore Emacs state
 from the dump file, and FILE is the name of the dump file.
-Value is nil if this session was not started using a portable dump file.*/)
+Value is nil if this session was not started using a dump file.*/)
      (void)
 {
   if (!dumped_with_pdumper_p ())
diff --git a/src/pdumper.h b/src/pdumper.h
index ab2f426c1e..5d1e9c3aea 100644
--- a/src/pdumper.h
+++ b/src/pdumper.h
@@ -35,7 +35,7 @@ INLINE_HEADER_BEGIN
    variables to which the Lisp heap points.  It doesn't know anything
    about other C variables.  The functions below allow code from other
    parts of Emacs to tell the portable dumper about other bits of
-   information to preserve in dumped images.
+   information to preserve in dump files.
 
    These memory-records are themselves preserved in the dump, so call
    the functions below only on the !initialized init path, just
@@ -44,7 +44,7 @@ INLINE_HEADER_BEGIN
    There are no special functions to preserve a global Lisp_Object.
    You should just staticpro these.  */
 
-/* Remember the value of THING in dumped images.  THING must not
+/* Remember the value of THING in dump files.  THING must not
    contain any pointers or Lisp_Object variables: these values are not
    valid across dump and load.  */
 #define PDUMPER_REMEMBER_SCALAR(thing)                  \
diff --git a/src/unexaix.c b/src/unexaix.c
index 349d365383..c95486cf72 100644
--- a/src/unexaix.c
+++ b/src/unexaix.c
@@ -1,4 +1,4 @@
-/* Dump an executable image.
+/* Dump an executable file.
    Copyright (C) 1985-1988, 1999, 2001-2019 Free Software Foundation,
    Inc.
 
-- 
2.17.1

Re: Changes in GC and in pure space

[Eli Zaretskii]

> Cc: monnier@iro.umontreal.ca, pipcet@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 23 Jul 2019 09:27:32 -0700
> 
> While in the neighborhood, I also put in a statement that unexec is deprecated 
> and rewrote instances of "portable dump file" as the dump files themselves are 
> no more portable than an Emacs executable is. See attached patch.

Thanks.

Not to start another bikeshedding thread, but just a thought: I wonder
whether using "dump file" for the emacs.pdmp file is the best idea.
Recall that building with unexec says "Dumping under the name...", so
"dumping" is not necessarily alien to unexec.  If "portable dump file"
is deemed too confusing, then perhaps we should use "pdump file"?

Re: Changes in GC and in pure space

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  pipcet@gmail.com,  emacs-devel@gnu.org
> Date: Mon, 22 Jul 2019 15:58:27 -0400
> 
> > I wasn't talking about the hash tables; that bug needs to be fixed,
> > indeed.
> 
> Not sure which bug you're referring to, because the one discussed
> recently (linked to the pdumper's need to rehash tables) should be fixed
> already (Pip Cet provided both that simple fix I installed as well as
> a subsequent better but more intrusive fix).

That one.

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Pip Cet]

On Mon, Jul 22, 2019 at 3:01 PM Eli Zaretskii <eliz@gnu.org> wrote:
> Please be aware that these changes might potentially affect the
> schedule of the Emacs 27 release.

Thank you for keeping that in mind. I don't think we should delay the
Emacs 27 release, and I understand completely if we have to delay
improvements until Emacs 28.

> Emacs 27 has accrued many new features and important improvements, and
> has proven to be generally very stable.  My intentions were to start
> its pretest in the September-October time-frame, with the goal of
> releasing Emacs 27.1 a few months later.

That sounds like a good schedule to me.

> Your changes in GC, which is a very delicate part of Emacs, might
> affect its stability and force us to analyze and fix hard-to-debug
> crashes, and thus postpone the beginning of the pretest and the
> subsequent release.  I think we should try to avoid postponing the
> 27.1 release, what with the many new features it will give our users.

I think this is mostly directed at Paul, but I agree major GC changes
should wait until Emacs 28. I can't think of a really good example,
but I think the bug that triggered this discussion will have to do:
had it remained undiscovered, we would have seen very strange and
unpredictable behavior indeed; some Emacs sessions that fail to GC and
run out of memory, and some sessions that GC at every maybe_gc call.

> The same goes for removing the pure space, IMO: another core feature
> whose existence and traits many parts of Emacs came to take for
> granted.

Let's not remove pure space for Emacs 27.

(The traits of pure space changed significantly, with the introduction
of pdumper: CHECK_IMPURE is now a nop rather than a valuable debugging
aid, and PURE_P is always false. That is something we should mention
somewhere, along with all the good NEWS, because it will make Emacs 27
harder to maintain.  But that's all we should do, I think, document
the odd state we're in now and resolve to change it when it is more
appropriate to do so.)

> I'm all for improving GC and simplifying our memory management, but
> please keep the above in your minds when you play with this stuff.
> Especially as, judging by the changes you are making, the details and
> indeed some of the aspects of the idea of the changes, are not yet
> sufficiently clear/finalized.

I'm forced to agree, as far as my ideas are concerned.

Eager rehashing of hash tables: needs to be timed precisely right for
user-defined dumped hash tables to work, as Paul apparently wants them
to, and my current proposal isn't (but let's fix Fclrhash to work on
non-rehashed hash tables).

Hash tables without internal free vectors: change the interpretation
of the hash table API (:size has to be reinterpreted to remain
meaningful), and some trade-offs about when to use hash tables.

Four tag bits for "annotated" (e.g., immutable) objects: very far from
ready, and problematic on 32-bit machines (perhaps this is no longer a
concern for Emacs 28...)

Turning pseudovectors into their own tag type, as miscellaneous
objects: not convinced it's worth the change, yet.

> An alternative would be to make these changes on a branch, and merge
> that branch when it is sufficiently stable and mature.  Please
> consider this possibility.  After all, these two issues are not
> terribly urgent to fix (unlike, say, the unexec thingy).

I'm not quite sure which "unexec thingy" you're referring to.

Here are things I would consider urgent enough to warrant looking at
for Emacs 27:
- remove code like we have in `Ffset' to detect GC bugs that
presumably have been fixed by now
- make_fixnum called for numbers outside the fixnum range
- various places that should detect circular lists but don't (lread.c
reading hash tables, for example)
- doubly-evaluated macro arguments (the one I can think of offhand is
DUMP_SET_REFERRER, which is harmless, but others might not be)
- since we appear to be stuck with lazy rehashing for now, the various
bugs this causes (I mentioned Fclrhash above)

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Mon, 22 Jul 2019 19:05:43 +0000
> Cc: eggert@cs.ucla.edu, emacs-devel@gnu.org
> 
> > The same goes for removing the pure space, IMO: another core feature
> > whose existence and traits many parts of Emacs came to take for
> > granted.
> 
> Let's not remove pure space for Emacs 27.
> 
> (The traits of pure space changed significantly, with the introduction
> of pdumper: CHECK_IMPURE is now a nop rather than a valuable debugging
> aid, and PURE_P is always false. That is something we should mention
> somewhere, along with all the good NEWS, because it will make Emacs 27
> harder to maintain.  But that's all we should do, I think, document
> the odd state we're in now and resolve to change it when it is more
> appropriate to do so.)
> 
> > I'm all for improving GC and simplifying our memory management, but
> > please keep the above in your minds when you play with this stuff.
> > Especially as, judging by the changes you are making, the details and
> > indeed some of the aspects of the idea of the changes, are not yet
> > sufficiently clear/finalized.
> 
> I'm forced to agree, as far as my ideas are concerned.
> 
> Eager rehashing of hash tables: needs to be timed precisely right for
> user-defined dumped hash tables to work, as Paul apparently wants them
> to, and my current proposal isn't (but let's fix Fclrhash to work on
> non-rehashed hash tables).
> 
> Hash tables without internal free vectors: change the interpretation
> of the hash table API (:size has to be reinterpreted to remain
> meaningful), and some trade-offs about when to use hash tables.
> 
> Four tag bits for "annotated" (e.g., immutable) objects: very far from
> ready, and problematic on 32-bit machines (perhaps this is no longer a
> concern for Emacs 28...)
> 
> Turning pseudovectors into their own tag type, as miscellaneous
> objects: not convinced it's worth the change, yet.

We can still have these on a branch, or on several branches.

> > An alternative would be to make these changes on a branch, and merge
> > that branch when it is sufficiently stable and mature.  Please
> > consider this possibility.  After all, these two issues are not
> > terribly urgent to fix (unlike, say, the unexec thingy).
> 
> I'm not quite sure which "unexec thingy" you're referring to.

A.k.a. "pdumper".

Re: Changes in GC and in pure space

[Stefan Monnier]

> Four tag bits for "annotated" (e.g., immutable) objects: very far from
> ready, and problematic on 32-bit machines (perhaps this is no longer a
> concern for Emacs 28...)

I'm still using 32bit Debian on pretty much all my machines (except for
one).  I could change to 64bit OSes on most of them, but not on the
Thinkpad X60 I use for my in-class presentations nor on the two BananaPis
I use as servers.

So, AFAIC, 32bit is not something we can drop yet:
It's OK to support it less-well, but it's not OK not to support it.

> Turning pseudovectors into their own tag type, as miscellaneous
> objects: not convinced it's worth the change, yet.

I'm not sure what this is referring to.

But in this area, I'd welcome a change which makes symbols and strings
(and intervals) use the same representation as pseudovectors (just like
buffer and misc has been merged many years ago).

They could still use their own "enum Lisp_Type" tag (some benchmarking
might be needed to decide if it's worthwhile), but the idea would
be to drop their *_blocks from alloc.c.


        Stefan

Re: Changes in GC and in pure space

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Some of the machines that we use Libreboot on are 32-bit machines.
We need to keep supporting 32-bit mode for years
unless we get some unforeseen good news.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Paul Eggert]

Pip Cet wrote:

> Here are things I would consider urgent enough to warrant looking at
> for Emacs 27:
> - remove code like we have in `Ffset' to detect GC bugs that
> presumably have been fixed by now

Are you talking about just the code introduced in commit 
2014-04-03T00:18:08Z!dancol@dancol.org 
(01ae0fbf30b74e2490144fceabbf5bc5d96f1ba7), or is some other code involved? I'll 
CC this to dancol to see if he has thoughts.

> - make_fixnum called for numbers outside the fixnum range

I installed a patch to master just now to do that.

> - various places that should detect circular lists but don't (lread.c
> reading hash tables, for example)

Can you give an example of that problem?

> - doubly-evaluated macro arguments (the one I can think of offhand is
> DUMP_SET_REFERRER, which is harmless, but others might not be)

Do you mean evaluated as in macro expansion, or evaluated as in execution at 
runtime? If the latter, I don't get the problem there. Many other macros assume 
that arguments are side effect free, and some of these cannot be turned into 
inline functions easily.

> - since we appear to be stuck with lazy rehashing for now, the various
> bugs this causes (I mentioned Fclrhash above)

Sorry, what's the Fclrhash bug?

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Pip Cet]

On Thu, Aug 15, 2019 at 9:34 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
> Pip Cet wrote:
>
> > Here are things I would consider urgent enough to warrant looking at
> > for Emacs 27:
> > - remove code like we have in `Ffset' to detect GC bugs that
> > presumably have been fixed by now
>
> Are you talking about just the code introduced in commit
> 2014-04-03T00:18:08Z!dancol@dancol.org
> (01ae0fbf30b74e2490144fceabbf5bc5d96f1ba7), or is some other code involved? I'll
> CC this to dancol to see if he has thoughts.

There's also reference to a GC bug in describe_vector.

> > - make_fixnum called for numbers outside the fixnum range
>
> I installed a patch to master just now to do that.

Thanks!

> > - various places that should detect circular lists but don't (lread.c
> > reading hash tables, for example)
>
> Can you give an example of that problem?

Try reading #s(hash-table data #0=(#0# . #0#))

> > - doubly-evaluated macro arguments (the one I can think of offhand is
> > DUMP_SET_REFERRER, which is harmless, but others might not be)
>
> Do you mean evaluated as in macro expansion, or evaluated as in execution at
> runtime? If the latter, I don't get the problem there. Many other macros assume
> that arguments are side effect free, and some of these cannot be turned into
> inline functions easily.

I think it's likely we still have some places that assume their
arguments are side-effect-free, but whose callers pass arguments with
side effects.

(At least, that's what I'd be looking for once I'm done looking over
the recent major hash table/GC changes to find segfaults like the
below)...

> > - since we appear to be stuck with lazy rehashing for now, the various
> > bugs this causes (I mentioned Fclrhash above)
>
> Sorry, what's the Fclrhash bug?

Calling clrhash on a dumped hash table that hasn't been accessed used
to leave it untouched; now it produces a segfault.

I haven't quite been able to follow how Eli's request went
uncontradicted, but then we ended up making major changes like the
hash table thing, anyway. That leads to this segfault, but I'm not at
all convinced it won't also lead to subtle bugs somewhere else.

(To reproduce the promised segfault, add

(defvar custom-dummy (make-hash-table :test 'eq))
(puthash custom-dummy custom-dummy custom-dummy)

to custom.el, then rebuild and run (clrhash custom-dummy) in Emacs -Q.
Trivial to fix, but apparently not obvious enough to have been caught
in the original changes).

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Paul Eggert]

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

Pip Cet wrote:
> Try reading #s(hash-table data #0=(#0# . #0#))

Thanks for mentioning that. I installed the attached to fix the bug, along with 
some similar bugs I noticed. Are there more instances of the problem?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-hard-loop-on-cycles-in-read-etc.patch --]
[-- Type: text/x-patch; name="0001-Don-t-hard-loop-on-cycles-in-read-etc.patch", Size: 7611 bytes --]

From 951ea375d52891f79b89794fbb9dca86ab8cd5a8 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 21 Aug 2019 17:18:33 -0700
Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20hard-loop=20on=20cycles=20in=20?=
 =?UTF-8?q?=E2=80=98read=E2=80=99=20etc.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem for ‘read’ reported by Pip Cet in:
https://lists.gnu.org/r/emacs-devel/2019-08/msg00316.html
* src/fns.c (Frequire): Protect against circular current-load-list.
* src/lread.c (Fget_load_suffixes):
Protect against circular load-suffixes or load-file-rep-suffixes.
(Fload): Protect against circular loads-in-progress.
(openp): Protect against circular PATH and SUFFIXES.
(build_load_history): Protect against circular load-history or
current-load-list.
(readevalloop_eager_expand_eval): Protect against circular SUBFORMS.
(read1): Protect against circular data.
* test/src/lread-tests.el (lread-circular-hash): New test.
---
 src/fns.c               |  9 ++++--
 src/lread.c             | 68 ++++++++++++++++++++---------------------
 test/src/lread-tests.el |  3 ++
 3 files changed, 43 insertions(+), 37 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index 6c47b3e5b1..b606d6299c 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2950,9 +2950,12 @@ DEFUN ("require", Frequire, Srequire, 1, 3, 0,
      But not more than once in any file,
      and not when we aren't loading or reading from a file.  */
   if (!from_file)
-    for (tem = Vcurrent_load_list; CONSP (tem); tem = XCDR (tem))
-      if (NILP (XCDR (tem)) && STRINGP (XCAR (tem)))
-	from_file = 1;
+    {
+      Lisp_Object tail = Vcurrent_load_list;
+      FOR_EACH_TAIL_SAFE (tail)
+	if (NILP (XCDR (tail)) && STRINGP (XCAR (tail)))
+	  from_file = true;
+    }
 
   if (from_file)
     {
diff --git a/src/lread.c b/src/lread.c
index 1bfbf5aa86..e444830c99 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1064,18 +1064,13 @@ DEFUN ("get-load-suffixes", Fget_load_suffixes, Sget_load_suffixes, 0, 0, 0,
 This uses the variables `load-suffixes' and `load-file-rep-suffixes'.  */)
   (void)
 {
-  Lisp_Object lst = Qnil, suffixes = Vload_suffixes, suffix, ext;
-  while (CONSP (suffixes))
+  Lisp_Object lst = Qnil, suffixes = Vload_suffixes;
+  FOR_EACH_TAIL (suffixes)
     {
       Lisp_Object exts = Vload_file_rep_suffixes;
-      suffix = XCAR (suffixes);
-      suffixes = XCDR (suffixes);
-      while (CONSP (exts))
-	{
-	  ext = XCAR (exts);
-	  exts = XCDR (exts);
-	  lst = Fcons (concat2 (suffix, ext), lst);
-	}
+      Lisp_Object suffix = XCAR (suffixes);
+      FOR_EACH_TAIL (exts)
+	lst = Fcons (concat2 (suffix, XCAR (exts)), lst);
     }
   return Fnreverse (lst);
 }
@@ -1290,8 +1285,8 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
      the general case; the second load may do something different.  */
   {
     int load_count = 0;
-    Lisp_Object tem;
-    for (tem = Vloads_in_progress; CONSP (tem); tem = XCDR (tem))
+    Lisp_Object tem = Vloads_in_progress;
+    FOR_EACH_TAIL_SAFE (tem)
       if (!NILP (Fequal (found, XCAR (tem))) && (++load_count > 3))
 	signal_error ("Recursive load", Fcons (found, Vloads_in_progress));
     record_unwind_protect (record_load_unwind, Vloads_in_progress);
@@ -1611,7 +1606,8 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
 
   CHECK_STRING (str);
 
-  for (tail = suffixes; CONSP (tail); tail = XCDR (tail))
+  tail = suffixes;
+  FOR_EACH_TAIL_SAFE (tail)
     {
       CHECK_STRING_CAR (tail);
       max_suffix_len = max (max_suffix_len,
@@ -1625,12 +1621,17 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
 
   absolute = complete_filename_p (str);
 
+  AUTO_LIST1 (just_use_str, Qnil);
+  if (NILP (path))
+    path = just_use_str;
+
   /* Go through all entries in the path and see whether we find the
      executable. */
-  do {
+  FOR_EACH_TAIL_SAFE (path)
+   {
     ptrdiff_t baselen, prefixlen;
 
-    if (NILP (path))
+    if (EQ (path, just_use_str))
       filename = str;
     else
       filename = Fexpand_file_name (str, XCAR (path));
@@ -1663,8 +1664,9 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
     memcpy (fn, SDATA (filename) + prefixlen, baselen);
 
     /* Loop over suffixes.  */
-    for (tail = NILP (suffixes) ? list1 (empty_unibyte_string) : suffixes;
-	 CONSP (tail); tail = XCDR (tail))
+    AUTO_LIST1 (empty_string_only, empty_unibyte_string);
+    tail = NILP (suffixes) ? empty_string_only : suffixes;
+    FOR_EACH_TAIL_SAFE (tail)
       {
 	Lisp_Object suffix = XCAR (tail);
 	ptrdiff_t fnlen, lsuffix = SBYTES (suffix);
@@ -1808,10 +1810,9 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
 	      }
 	  }
       }
-    if (absolute || NILP (path))
+    if (absolute)
       break;
-    path = XCDR (path);
-  } while (CONSP (path));
+   }
 
   SAFE_FREE ();
   errno = last_errno;
@@ -1838,7 +1839,7 @@ build_load_history (Lisp_Object filename, bool entire)
   tail = Vload_history;
   prev = Qnil;
 
-  while (CONSP (tail))
+  FOR_EACH_TAIL (tail)
     {
       tem = XCAR (tail);
 
@@ -1861,22 +1862,19 @@ build_load_history (Lisp_Object filename, bool entire)
 	    {
 	      tem2 = Vcurrent_load_list;
 
-	      while (CONSP (tem2))
+	      FOR_EACH_TAIL (tem2)
 		{
 		  newelt = XCAR (tem2);
 
 		  if (NILP (Fmember (newelt, tem)))
 		    Fsetcar (tail, Fcons (XCAR (tem),
 		     			  Fcons (newelt, XCDR (tem))));
-
-		  tem2 = XCDR (tem2);
 		  maybe_quit ();
 		}
 	    }
 	}
       else
 	prev = tail;
-      tail = XCDR (tail);
       maybe_quit ();
     }
 
@@ -1918,10 +1916,9 @@ readevalloop_eager_expand_eval (Lisp_Object val, Lisp_Object macroexpand)
   if (EQ (CAR_SAFE (val), Qprogn))
     {
       Lisp_Object subforms = XCDR (val);
-
-      for (val = Qnil; CONSP (subforms); subforms = XCDR (subforms))
-          val = readevalloop_eager_expand_eval (XCAR (subforms),
-                                                macroexpand);
+      val = Qnil;
+      FOR_EACH_TAIL (subforms)
+	val = readevalloop_eager_expand_eval (XCAR (subforms), macroexpand);
     }
   else
       val = eval_sub (call2 (macroexpand, val, Qt));
@@ -2861,16 +2858,19 @@ read1 (Lisp_Object readcharfun, int *pch, bool first_in_list)
 	      /* Now use params to make a new hash table and fill it.  */
 	      ht = Fmake_hash_table (param_count, params);
 
-	      while (CONSP (data))
-	      	{
+	      Lisp_Object last = data;
+	      FOR_EACH_TAIL_SAFE (data)
+		{
 	      	  key = XCAR (data);
 	      	  data = XCDR (data);
 	      	  if (!CONSP (data))
-		    error ("Odd number of elements in hash table data");
+		    break;
 	      	  val = XCAR (data);
-	      	  data = XCDR (data);
+		  last = XCDR (data);
 	      	  Fputhash (key, val, ht);
-	      	}
+		}
+	      if (!NILP (last))
+		error ("Hash table data is not a list of even length");
 
 	      return ht;
 	    }
diff --git a/test/src/lread-tests.el b/test/src/lread-tests.el
index 82b75b195c..ba5bfe0145 100644
--- a/test/src/lread-tests.el
+++ b/test/src/lread-tests.el
@@ -220,4 +220,7 @@ lread-string-to-number-trailing-dot
                    (* most-positive-fixnum most-positive-fixnum)))
     (should (= n (string-to-number (format "%d." n))))))
 
+(ert-deftest lread-circular-hash ()
+  (should-error (read "#s(hash-table data #0=(#0# . #0#))")))
+
 ;;; lread-tests.el ends here
-- 
2.17.1

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Paul Eggert]

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

Pip Cet wrote:

> (To reproduce the promised segfault, add
> 
> (defvar custom-dummy (make-hash-table :test 'eq))
> (puthash custom-dummy custom-dummy custom-dummy)
> 
> to custom.el, then rebuild and run (clrhash custom-dummy) in Emacs -Q.
> Trivial to fix, but apparently not obvious enough to have been caught
> in the original changes).

Thanks for reporting that. I installed the attached patch, which should fix that 
bug and clean up a few nearby glitches. This patch is a bit less trivial but 
should be a smidge faster than the trivial patch would have been.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-clrhash-bug-when-hash-table-needs-rehashing.patch --]
[-- Type: text/x-patch; name="0001-Fix-clrhash-bug-when-hash-table-needs-rehashing.patch", Size: 4348 bytes --]

From ac71011bc1726bd767446407500c20cbbdca74a2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 21 Aug 2019 18:54:08 -0700
Subject: [PATCH] Fix clrhash bug when hash table needs rehashing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Pip Cet in:
https://lists.gnu.org/r/emacs-devel/2019-08/msg00316.html
* src/fns.c (maybe_resize_hash_table): Prefer ASET to gc_aset
where either will do.  Simplify appending of Qunbound values.
Put index_size calculation closer to where it’s needed.
(hash_clear): If hash_rehash_needed_p (h), don’t clear the
nonexistent hash vector.  Use memclear to speed up clearing.
* src/lisp.h (HASH_TABLE_SIZE): Check that the size is positive,
and tell that to the compiler.
---
 src/fns.c  | 28 ++++++++++++----------------
 src/lisp.h |  9 +++++----
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index b606d6299c..8ca0953fe8 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -4198,21 +4198,20 @@ maybe_resize_hash_table (struct Lisp_Hash_Table *h)
 					  new_size);
       ptrdiff_t next_size = ASIZE (next);
       for (ptrdiff_t i = old_size; i < next_size - 1; i++)
-	gc_aset (next, i, make_fixnum (i + 1));
-      gc_aset (next, next_size - 1, make_fixnum (-1));
-      ptrdiff_t index_size = hash_index_size (h, next_size);
+	ASET (next, i, make_fixnum (i + 1));
+      ASET (next, next_size - 1, make_fixnum (-1));
 
       /* Build the new&larger key_and_value vector, making sure the new
          fields are initialized to `unbound`.  */
       Lisp_Object key_and_value
 	= larger_vecalloc (h->key_and_value, 2 * (next_size - old_size),
 			   2 * next_size);
-      for (ptrdiff_t i = ASIZE (h->key_and_value);
-            i < ASIZE (key_and_value); i++)
+      for (ptrdiff_t i = 2 * old_size; i < 2 * next_size; i++)
         ASET (key_and_value, i, Qunbound);
 
       Lisp_Object hash = larger_vector (h->hash, next_size - old_size,
 					next_size);
+      ptrdiff_t index_size = hash_index_size (h, next_size);
       h->index = make_vector (index_size, make_fixnum (-1));
       h->key_and_value = key_and_value;
       h->hash = hash;
@@ -4404,17 +4403,14 @@ hash_clear (struct Lisp_Hash_Table *h)
 {
   if (h->count > 0)
     {
-      ptrdiff_t i, size = HASH_TABLE_SIZE (h);
-
-      for (i = 0; i < size; ++i)
-	{
-	  set_hash_next_slot (h, i, i < size - 1 ? i + 1 : -1);
-	  set_hash_key_slot (h, i, Qunbound);
-	  set_hash_value_slot (h, i, Qnil);
-	  set_hash_hash_slot (h, i, Qnil);
-	}
-
-      for (i = 0; i < ASIZE (h->index); ++i)
+      ptrdiff_t size = HASH_TABLE_SIZE (h);
+      if (!hash_rehash_needed_p (h))
+	memclear (XVECTOR (h->hash)->contents, size * word_size);
+      memclear (XVECTOR (h->key_and_value)->contents, size * 2 * word_size);
+      for (ptrdiff_t i = 0; i < size; i++)
+	set_hash_next_slot (h, i, i < size - 1 ? i + 1 : -1);
+
+      for (ptrdiff_t i = 0; i < ASIZE (h->index); i++)
 	ASET (h->index, i, make_fixnum (-1));
 
       h->next_free = 0;
diff --git a/src/lisp.h b/src/lisp.h
index 56ad99b8e3..ae5a81e7b5 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -2307,7 +2307,7 @@ #define DEFSYM(sym, name) /* empty */
      weakness of the table.  */
   Lisp_Object weak;
 
-  /* Vector of hash codes.
+  /* Vector of hash codes, or nil if the table needs rehashing.
      If the I-th entry is unused, then hash[I] should be nil.  */
   Lisp_Object hash;
 
@@ -2327,8 +2327,7 @@ #define DEFSYM(sym, name) /* empty */
      'index' are special and are either ignored by the GC or traced in
      a special way (e.g. because of weakness).  */
 
-  /* Number of key/value entries in the table.  This number is
-     negated if the table needs rehashing.  */
+  /* Number of key/value entries in the table.  */
   ptrdiff_t count;
 
   /* Index of first free entry in free list, or -1 if none.  */
@@ -2413,7 +2412,9 @@ HASH_HASH (const struct Lisp_Hash_Table *h, ptrdiff_t idx)
 INLINE ptrdiff_t
 HASH_TABLE_SIZE (const struct Lisp_Hash_Table *h)
 {
-  return ASIZE (h->next);
+  ptrdiff_t size = ASIZE (h->next);
+  eassume (0 < size);
+  return size;
 }
 
 void hash_table_rehash (struct Lisp_Hash_Table *h);
-- 
2.17.1

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Paul Eggert]

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

>>> Here are things I would consider urgent enough to warrant looking at
>>> for Emacs 27:
>>> - remove code like we have in `Ffset' to detect GC bugs that
>>> presumably have been fixed by now
>>
>> Are you talking about just the code introduced in commit
>> 2014-04-03T00:18:08Z!dancol@dancol.org
>> (01ae0fbf30b74e2490144fceabbf5bc5d96f1ba7), or is some other code involved? I'll
>> CC this to dancol to see if he has thoughts.
> 
> There's also reference to a GC bug in describe_vector.

Thanks for mentioning these two items. I installed the attached two patches into 
master. The first arranges for SUSPICIOUS_OBJECT_CHECKING to not be defined 
unless one compiles with ENABLE_CHECKING, which should remove the Ffset hack in 
production code; the second removes that 25-year-old GC workaround that hasn't 
been needed for quite some time.

Actually, all that SUSPICIOUS_OBJECT_CHECKING code can be removed. But one step 
at a time.

[-- Attachment #2: 0001-Don-t-debug-fset-by-default.patch --]
[-- Type: text/x-patch, Size: 2567 bytes --]

From 093515ae0db64058b0948dac5a42e9f72e06bcaa Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 21 Aug 2019 22:19:03 -0700
Subject: [PATCH 1/2] =?UTF-8?q?Don=E2=80=99t=20debug=20fset=20by=20default?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This GC bug seems to have been fixed, so the check is no longer
needed in production code.  From a suggestion by Pip Cet in:
https://lists.gnu.org/r/emacs-devel/2019-08/msg00316.html
* src/alloc.c (SUSPICIOUS_OBJECT_CHECKING) [!ENABLE_CHECKING]:
Do not define.
(find_suspicious_object_in_range, detect_suspicious_free):
Expand to proper dummy expressions if !SUSPICIOUS_OBJECT_CHECKING.
* src/data.c (Ffset): Convert test to an eassert.
---
 src/alloc.c | 12 ++++--------
 src/data.c  |  5 +----
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 53af7325f4..39964c4b29 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -302,15 +302,11 @@ #define PUREBEG (char *) pure
 
 const char *pending_malloc_warning;
 
-#if 0 /* Normally, pointer sanity only on request... */
+/* Pointer sanity only on request.  FIXME: Code depending on
+   SUSPICIOUS_OBJECT_CHECKING is obsolete; remove it entirely.  */
 #ifdef ENABLE_CHECKING
 #define SUSPICIOUS_OBJECT_CHECKING 1
 #endif
-#endif
-
-/* ... but unconditionally use SUSPICIOUS_OBJECT_CHECKING while the GC
-   bug is unresolved.  */
-#define SUSPICIOUS_OBJECT_CHECKING 1
 
 #ifdef SUSPICIOUS_OBJECT_CHECKING
 struct suspicious_free_record
@@ -327,8 +323,8 @@ #define SUSPICIOUS_OBJECT_CHECKING 1
 static void *find_suspicious_object_in_range (void *begin, void *end);
 static void detect_suspicious_free (void *ptr);
 #else
-# define find_suspicious_object_in_range(begin, end) NULL
-# define detect_suspicious_free(ptr) (void)
+# define find_suspicious_object_in_range(begin, end) ((void *) NULL)
+# define detect_suspicious_free(ptr) ((void) 0)
 #endif
 
 /* Maximum amount of C stack to save when a GC happens.  */
diff --git a/src/data.c b/src/data.c
index 8344bfdd3d..2797adfcdc 100644
--- a/src/data.c
+++ b/src/data.c
@@ -771,10 +771,7 @@ DEFUN ("fset", Ffset, Sfset, 2, 2, 0,
   if (AUTOLOADP (function))
     Fput (symbol, Qautoload, XCDR (function));
 
-  /* Convert to eassert or remove after GC bug is found.  In the
-     meantime, check unconditionally, at a slight perf hit.  */
-  if (! valid_lisp_object_p (definition))
-    emacs_abort ();
+  eassert (valid_lisp_object_p (definition));
 
   set_symbol_function (symbol, definition);
 
-- 
2.17.1


[-- Attachment #3: 0002-Remove-no-longer-needed-workaround-for-GC-bug.patch --]
[-- Type: text/x-patch, Size: 1174 bytes --]

From 6c5f753c28a024d8808323d08c8466f9d8e86e68 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 21 Aug 2019 22:29:35 -0700
Subject: [PATCH 2/2] Remove no-longer-needed workaround for GC bug

* src/keymap.c (describe_vector): Remove old workaround for GC bug.
This workaround, introduced in 1993-02-19T05:43:54Z!rms@gnu.org,
has not been needed for some time.  Problem reported by Pip Cet in:
https://lists.gnu.org/r/emacs-devel/2019-08/msg00316.html
---
 src/keymap.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/keymap.c b/src/keymap.c
index 6762915f70..b1e09a92f2 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -3371,12 +3371,10 @@ describe_vector (Lisp_Object vector, Lisp_Object prefix, Lisp_Object args,
 
   if (!keymap_p)
     {
-      /* Call Fkey_description first, to avoid GC bug for the other string.  */
       if (!NILP (prefix) && XFIXNAT (Flength (prefix)) > 0)
 	{
-	  Lisp_Object tem = Fkey_description (prefix, Qnil);
 	  AUTO_STRING (space, " ");
-	  elt_prefix = concat2 (tem, space);
+	  elt_prefix = concat2 (Fkey_description (prefix, Qnil), space);
 	}
       prefix = Qnil;
     }
-- 
2.17.1

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Paul Eggert]

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

Pip Cet wrote:

>>> - doubly-evaluated macro arguments (the one I can think of offhand is
>>> DUMP_SET_REFERRER, which is harmless, but others might not be)
>>
>> Do you mean evaluated as in macro expansion, or evaluated as in execution at
>> runtime? If the latter, I don't get the problem there. Many other macros assume
>> that arguments are side effect free, and some of these cannot be turned into
>> inline functions easily.
> 
> I think it's likely we still have some places that assume their
> arguments are side-effect-free, but whose callers pass arguments with
> side effects.

The DUMP_SET_REFERRER macro was indeed confusing. But weren't you talking about 
how that macro evaluates its OBJECT arg either zero or one time, vs evaluating 
arguments more than once? Anyway, it's not that hard to adjust the code so that 
it can be defined as a function rather than a macro, so I did that and installed 
the attached first patch.

More generally, I share your concern about confusion about how many times a C 
macro argument is evaluated. The Emacs C source has a lot of macros, though. 
Rather than continue to audit all uses of all macros, I've found it more 
effective to replace many/most of these macros with functions, so that we don't 
have to worry about evaluation. Similarly for object-like macros and constants. 
See, for example, the attached second patch, which I just installed (I was 
partly inspired by your email). This sort of improvement gives us fewer macro 
calls to audit. And as a side effect it typically improves Emacs performance 
when optimized (though Emacs can get slower if optimization is disabled).

We can't easily remove all C macros from the Emacs source code, but when it's 
relatively easy (as is the case here) it's worth doing. I did this sort of thing 
to lisp.h in 2013 and that worked out reasonably well, and if someone gets the 
time it would be good to do it to other parts of the Emacs source code.

[-- Attachment #2: 0001-Avoid-macros-in-pdumper.c-when-it-s-easy.patch --]
[-- Type: text/x-patch, Size: 14341 bytes --]

From 9117a667908064a0b4ae6ec6c8f3674d95ad6225 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 3 Sep 2019 21:53:36 -0700
Subject: [PATCH] =?UTF-8?q?Avoid=20macros=20in=20pdumper.c=20when=20it?=
 =?UTF-8?q?=E2=80=99s=20easy?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem with DUMP_SET_REFERRER mentioned by Pip Cet at end of:
https://lists.gnu.org/archive/html/emacs-devel/2019-07/msg00548.html
* src/pdumper.c (DANGEROUS, EMACS_RELOC_TYPE_BITS)
(EMACS_RELOC_LENGTH_BITS, DUMP_RELOC_TYPE_BITS)
(DUMP_RELOC_ALIGNMENT_BITS, DUMP_RELOC_OFFSET_BITS)
(DUMP_RELOCATION_ALIGNMENT, DUMP_ALIGNMENT)
(WEIGHT_NONE, WEIGHT_NORMAL, WEIGHT_STRONG)
(PDUMPER_MAX_OBJECT_SIZE):
Now a constant, not a macro.
(divide_round_up): Now a function, not a macro DIVIDE_ROUND_UP.
All uses changed.
(enum link_weight_enum, WEIGHT_NONE_VALUE)
(WEIGHT_NORMAL_VALUE, WEIGHT_STRONG_VALUE): Remove.
(struct link_weight): Just use an int.
(dump_set_referrer): New function, replacing DUMP_SET_REFERRER
macro with a different API.  All uses changed.
(dump_clear_referrer): Rename from DUMP_CLEAR_REFERRER.
All uses changed.
(DEFINE_FROMLISP_FUNC, DEFINE_TOLISP_FUNC): Remove.
(intmax_t_from_lisp, intmax_t_to_lisp, dump_off_from_lisp)
(dump_off_to_lisp): Define without using macros,
(dump_off_from_lisp): Add an eassert range check.
(DUMP_FIELD_COPY): Simplify.
---
 src/pdumper.c | 196 +++++++++++++++++++++++++-------------------------
 1 file changed, 98 insertions(+), 98 deletions(-)

diff --git a/src/pdumper.c b/src/pdumper.c
index f9c31d125a..306a70396e 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -105,8 +105,6 @@ #define VM_MS_WINDOWS 2
 # define VM_SUPPORTED 0
 #endif
 
-#define DANGEROUS 0
-
 /* PDUMPER_CHECK_REHASHING being true causes the portable dumper to
    check, for each hash table it dumps, that the hash table means the
    same thing after rehashing.  */
@@ -129,7 +127,11 @@ verify (sizeof (ptrdiff_t) <= sizeof (Lisp_Object));
 verify (sizeof (ptrdiff_t) <= sizeof (EMACS_INT));
 verify (CHAR_BIT == 8);
 
-#define DIVIDE_ROUND_UP(x, y) (((x) + (y) - 1) / (y))
+static size_t
+divide_round_up (size_t x, size_t y)
+{
+  return (x + y - 1) / y;
+}
 
 static const char dump_magic[16] = {
   'D', 'U', 'M', 'P', 'E', 'D',
@@ -235,9 +237,12 @@ #define dump_offsetof(type, member)                             \
     RELOC_EMACS_EMACS_LV,
   };
 
-#define EMACS_RELOC_TYPE_BITS 3
-#define EMACS_RELOC_LENGTH_BITS                         \
-  (sizeof (dump_off) * CHAR_BIT - EMACS_RELOC_TYPE_BITS)
+enum
+  {
+   EMACS_RELOC_TYPE_BITS = 3,
+   EMACS_RELOC_LENGTH_BITS = (sizeof (dump_off) * CHAR_BIT
+			      - EMACS_RELOC_TYPE_BITS)
+  };
 
 struct emacs_reloc
 {
@@ -274,19 +279,22 @@ emacs_reloc_set_type (struct emacs_reloc *reloc,
   dump_off nr_entries;
 };
 
-#define DUMP_RELOC_TYPE_BITS 5
-verify (RELOC_DUMP_TO_EMACS_LV + 8 < (1 << DUMP_RELOC_TYPE_BITS));
+enum
+  {
+   DUMP_RELOC_TYPE_BITS = 5,
+   DUMP_RELOC_ALIGNMENT_BITS = 2,
+
+   /* Minimum alignment required by dump file format.  */
+   DUMP_RELOCATION_ALIGNMENT = 1 << DUMP_RELOC_ALIGNMENT_BITS,
 
-#define DUMP_RELOC_ALIGNMENT_BITS 2
-#define DUMP_RELOC_OFFSET_BITS                          \
-  (sizeof (dump_off) * CHAR_BIT - DUMP_RELOC_TYPE_BITS)
+   /* The alignment granularity (in bytes) for objects we store in the
+      dump.  Always suitable for heap objects; may be more aligned.  */
+   DUMP_ALIGNMENT = max (GCALIGNMENT, DUMP_RELOCATION_ALIGNMENT),
 
-/* Minimum alignment required by dump file format.  */
-#define DUMP_RELOCATION_ALIGNMENT (1<<DUMP_RELOC_ALIGNMENT_BITS)
+   DUMP_RELOC_OFFSET_BITS = sizeof (dump_off) * CHAR_BIT - DUMP_RELOC_TYPE_BITS
+  };
 
-/* The alignment granularity (in bytes) for objects we store in the
-   dump.  Always suitable for heap objects; may be more aligned.  */
-#define DUMP_ALIGNMENT (max (GCALIGNMENT, DUMP_RELOCATION_ALIGNMENT))
+verify (RELOC_DUMP_TO_EMACS_LV + 8 < (1 << DUMP_RELOC_TYPE_BITS));
 verify (DUMP_ALIGNMENT >= GCALIGNMENT);
 
 struct dump_reloc
@@ -572,23 +580,17 @@ dump_fingerprint (char const *label,
   };
 
 /* Weights for score scores for object non-locality.  */
-enum link_weight_enum
-  {
-    WEIGHT_NONE_VALUE = 0,
-    WEIGHT_NORMAL_VALUE = 1000,
-    WEIGHT_STRONG_VALUE = 1200,
-  };
 
 struct link_weight
 {
   /* Wrapped in a struct to break unwanted implicit conversion.  */
-  enum link_weight_enum value;
+  int value;
 };
 
-#define LINK_WEIGHT_LITERAL(x) ((struct link_weight){.value=(x)})
-#define WEIGHT_NONE LINK_WEIGHT_LITERAL (WEIGHT_NONE_VALUE)
-#define WEIGHT_NORMAL LINK_WEIGHT_LITERAL (WEIGHT_NORMAL_VALUE)
-#define WEIGHT_STRONG LINK_WEIGHT_LITERAL (WEIGHT_STRONG_VALUE)
+static struct link_weight const
+  WEIGHT_NONE = { .value = 0 },
+  WEIGHT_NORMAL = { .value = 1000 },
+  WEIGHT_STRONG = { .value = 1200 };
 
 \f
 /* Dump file creation */
@@ -628,35 +630,27 @@ dump_set_have_current_referrer (struct dump_context *ctx, bool have)
 #endif
 }
 
-/* Remember the reason objects are enqueued.
+/* Return true if if objects should be enqueued in CTX to refer to an
+   object that the caller should store into CTX->current_referrer.
 
-   Until DUMP_CLEAR_REFERRER is called, any objects enqueued are being
-   enqueued because OBJECT refers to them.  It is not legal to enqueue
-   objects without a referer set.  We check this constraint
+   Until dump_clear_referrer is called, any objects enqueued are being
+   enqueued because the object refers to them.  It is not valid to
+   enqueue objects without a referrer set.  We check this constraint
    at runtime.
 
-   It is illegal to call DUMP_SET_REFERRER twice without an
-   intervening call to DUMP_CLEAR_REFERRER.
-
-   Define as a macro so we can avoid evaluating OBJECT
-   if we dont want referrer tracking.  */
-#define DUMP_SET_REFERRER(ctx, object)                   \
-  do                                                     \
-    {                                                    \
-      struct dump_context *_ctx = (ctx);                 \
-      eassert (!_ctx->have_current_referrer);            \
-      dump_set_have_current_referrer (_ctx, true);       \
-      if (dump_tracking_referrers_p (_ctx))              \
-        ctx->current_referrer = (object);                \
-    }                                                    \
-  while (0)
-
-/* Unset the referer that DUMP_SET_REFERRER set.
-
-   Named with upper-case letters for symmetry with
-   DUMP_SET_REFERRER.  */
+   It is invalid to call dump_set_referrer twice without an
+   intervening call to dump_clear_referrer.  */
+static bool
+dump_set_referrer (struct dump_context *ctx)
+{
+  eassert (!ctx->have_current_referrer);
+  dump_set_have_current_referrer (ctx, true);
+  return dump_tracking_referrers_p (ctx);
+}
+
+/* Unset the referrer that dump_set_referrer prepared for.  */
 static void
-DUMP_CLEAR_REFERRER (struct dump_context *ctx)
+dump_clear_referrer (struct dump_context *ctx)
 {
   eassert (ctx->have_current_referrer);
   dump_set_have_current_referrer (ctx, false);
@@ -732,34 +726,36 @@ dump_object_self_representing_p (Lisp_Object object)
   return FIXNUMP (object) || dump_builtin_symbol_p (object);
 }
 
-#define DEFINE_FROMLISP_FUNC(fn, type)          \
-  static type                                   \
-  fn (Lisp_Object value)                        \
-  {                                             \
-    ALLOW_IMPLICIT_CONVERSION;                  \
-    if (FIXNUMP (value))                        \
-      return XFIXNUM (value);                   \
-    eassert (BIGNUMP (value));                  \
-    type result;				\
-    if (TYPE_SIGNED (type))			\
-      result = bignum_to_intmax (value);	\
-    else					\
-      result = bignum_to_uintmax (value);	\
-    DISALLOW_IMPLICIT_CONVERSION;               \
-    return result;				\
-  }
+static intmax_t
+intmax_t_from_lisp (Lisp_Object value)
+{
+  intmax_t n;
+  bool ok = integer_to_intmax (value, &n);
+  eassert (ok);
+  return n;
+}
 
-#define DEFINE_TOLISP_FUNC(fn, type) \
-  static Lisp_Object                 \
-  fn (type value)                    \
-  {                                  \
-    return INT_TO_INTEGER (value);   \
-  }
+static Lisp_Object
+intmax_t_to_lisp (intmax_t value)
+{
+  return INT_TO_INTEGER (value);
+}
+
+static dump_off
+dump_off_from_lisp (Lisp_Object value)
+{
+  intmax_t n = intmax_t_from_lisp (value);
+  eassert (DUMP_OFF_MIN <= n && n <= DUMP_OFF_MAX);
+  ALLOW_IMPLICIT_CONVERSION;
+  return n;
+  DISALLOW_IMPLICIT_CONVERSION;
+}
 
-DEFINE_FROMLISP_FUNC (intmax_t_from_lisp, intmax_t)
-DEFINE_TOLISP_FUNC (intmax_t_to_lisp, intmax_t)
-DEFINE_FROMLISP_FUNC (dump_off_from_lisp, dump_off)
-DEFINE_TOLISP_FUNC (dump_off_to_lisp, dump_off)
+static Lisp_Object
+dump_off_to_lisp (dump_off value)
+{
+  return INT_TO_INTEGER (value);
+}
 
 static void
 dump_write (struct dump_context *ctx, const void *buf, dump_off nbyte)
@@ -1731,9 +1727,10 @@ dump_root_visitor (Lisp_Object const *root_ptr, enum gc_root_type type,
       eassert (dump_builtin_symbol_p (value));
       /* Remember to dump the object itself later along with all the
          rest of the copied-to-Emacs objects.  */
-      DUMP_SET_REFERRER (ctx, build_string ("built-in symbol list"));
+      if (dump_set_referrer (ctx))
+	ctx->current_referrer = build_string ("built-in symbol list");
       dump_enqueue_object (ctx, value, WEIGHT_NONE);
-      DUMP_CLEAR_REFERRER (ctx);
+      dump_clear_referrer (ctx);
     }
   else
     {
@@ -1743,9 +1740,11 @@ dump_root_visitor (Lisp_Object const *root_ptr, enum gc_root_type type,
                   ctx->staticpro_table);
       if (root_ptr != &Vinternal_interpreter_environment)
         {
-          DUMP_SET_REFERRER (ctx, dump_ptr_referrer ("emacs root", root_ptr));
+	  if (dump_set_referrer (ctx))
+	    ctx->current_referrer
+	      = dump_ptr_referrer ("emacs root", root_ptr);
           dump_emacs_reloc_to_lv (ctx, root_ptr, *root_ptr);
-          DUMP_CLEAR_REFERRER (ctx);
+          dump_clear_referrer (ctx);
         }
     }
 }
@@ -1759,7 +1758,7 @@ dump_roots (struct dump_context *ctx)
   visit_static_gc_roots (visitor);
 }
 
-#define PDUMPER_MAX_OBJECT_SIZE 2048
+enum { PDUMPER_MAX_OBJECT_SIZE = 2048 };
 
 static dump_off
 field_relpos (const void *in_start, const void *in_field)
@@ -1788,11 +1787,7 @@ cpyptr (void *out, const void *in)
 
 /* Convenience macro for regular assignment.  */
 #define DUMP_FIELD_COPY(out, in, name) \
-  do                                   \
-    {                                  \
-      (out)->name = (in)->name;        \
-    }                                  \
-  while (0)
+  ((out)->name = (in)->name)
 
 static void
 dump_field_lv_or_rawptr (struct dump_context *ctx,
@@ -1848,6 +1843,7 @@ dump_field_lv_or_rawptr (struct dump_context *ctx,
   intptr_t out_value;
   dump_off out_field_offset = ctx->obj_offset + relpos;
   dump_off target_offset = dump_recall_object (ctx, value);
+  enum { DANGEROUS = false };
   if (DANGEROUS
       && target_offset > 0 && dump_object_emacs_ptr (value) == NULL)
     {
@@ -2408,7 +2404,8 @@ dump_pre_dump_symbol (struct dump_context *ctx, struct Lisp_Symbol *symbol)
 {
   Lisp_Object symbol_lv = make_lisp_symbol (symbol);
   eassert (!dump_recall_symbol_aux (ctx, symbol_lv));
-  DUMP_SET_REFERRER (ctx, symbol_lv);
+  if (dump_set_referrer (ctx))
+    ctx->current_referrer = symbol_lv;
   switch (symbol->u.s.redirect)
     {
     case SYMBOL_LOCALIZED:
@@ -2422,7 +2419,7 @@ dump_pre_dump_symbol (struct dump_context *ctx, struct Lisp_Symbol *symbol)
     default:
       break;
     }
-  DUMP_CLEAR_REFERRER (ctx);
+  dump_clear_referrer (ctx);
 }
 
 static dump_off
@@ -2443,13 +2440,14 @@ dump_symbol (struct dump_context *ctx,
         {
 	  eassert (offset == DUMP_OBJECT_ON_NORMAL_QUEUE
 		   || offset == DUMP_OBJECT_NOT_SEEN);
-          DUMP_CLEAR_REFERRER (ctx);
+	  dump_clear_referrer (ctx);
           struct dump_flags old_flags = ctx->flags;
           ctx->flags.dump_object_contents = false;
           ctx->flags.defer_symbols = false;
           dump_object (ctx, object);
           ctx->flags = old_flags;
-          DUMP_SET_REFERRER (ctx, object);
+	  if (dump_set_referrer (ctx))
+	    ctx->current_referrer = object;
 
           offset = DUMP_OBJECT_ON_SYMBOL_QUEUE;
           dump_remember_object (ctx, object, offset);
@@ -3118,7 +3116,8 @@ dump_object (struct dump_context *ctx, Lisp_Object object)
     }
 
   /* Object needs to be dumped.  */
-  DUMP_SET_REFERRER (ctx, object);
+  if (dump_set_referrer (ctx))
+    ctx->current_referrer = object;
   switch (XTYPE (object))
     {
     case Lisp_String:
@@ -3142,7 +3141,7 @@ dump_object (struct dump_context *ctx, Lisp_Object object)
     default:
       emacs_abort ();
     }
-  DUMP_CLEAR_REFERRER (ctx);
+  dump_clear_referrer (ctx);
 
   /* offset can be < 0 if we've deferred an object.  */
   if (ctx->flags.dump_object_contents && offset > DUMP_OBJECT_NOT_SEEN)
@@ -3507,9 +3506,10 @@ dump_drain_user_remembered_data_hot (struct dump_context *ctx)
           read_ptr_raw_and_lv (mem, type, &value, &lv);
           if (value != NULL)
             {
-              DUMP_SET_REFERRER (ctx, dump_ptr_referrer ("user data", mem));
+	      if (dump_set_referrer (ctx))
+		ctx->current_referrer = dump_ptr_referrer ("user data", mem);
               dump_enqueue_object (ctx, lv, WEIGHT_NONE);
-              DUMP_CLEAR_REFERRER (ctx);
+	      dump_clear_referrer (ctx);
             }
         }
     }
@@ -4877,7 +4877,7 @@ dump_bitset_init (struct dump_bitset *bitset, size_t number_bits)
 {
   int xword_size = sizeof (bitset->bits[0]);
   int bits_per_word = xword_size * CHAR_BIT;
-  ptrdiff_t words_needed = DIVIDE_ROUND_UP (number_bits, bits_per_word);
+  ptrdiff_t words_needed = divide_round_up (number_bits, bits_per_word);
   bitset->number_words = words_needed;
   bitset->bits = calloc (words_needed, xword_size);
   return bitset->bits != NULL;
@@ -5420,7 +5420,7 @@ pdumper_load (const char *dump_filename)
 
   err = PDUMPER_LOAD_ERROR;
   mark_bits_needed =
-    DIVIDE_ROUND_UP (header->discardable_start, DUMP_ALIGNMENT);
+    divide_round_up (header->discardable_start, DUMP_ALIGNMENT);
   if (!dump_bitset_init (&mark_bits, mark_bits_needed))
     goto out;
 
-- 
2.17.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Prefer-functions-to-macros-in-buffer.h.patch --]
[-- Type: text/x-patch; name="0001-Prefer-functions-to-macros-in-buffer.h.patch", Size: 28681 bytes --]

From d84b4f83a55e3b1a12e399ad199cc36cd013dcb3 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 3 Sep 2019 21:54:58 -0700
Subject: [PATCH] Prefer functions to macros in buffer.h
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In buffer.h, prefer inline functions to function-like macros
when either will do.  This helps avoid confusion about how
many times an arg is evaluated.  On my platform, this patch
improves performance of ‘make compile-always’ by 5.7%.
Also, prefer enum constants to object-like macros
when either will do.
* src/buffer.h (BEG, BEG_BYTE, GAP_BYTES_DFL, GAP_BYTES_MIN)
(MAX_PER_BUFFER_VARS, NONEXISTENT_MODTIME_NSECS)
(UNKNOWN_MODTIME_NSECS, BUFFER_LISP_SIZE, BUFFER_REST_SIZE):
Now enum constants, instead of macros.
(BUFFER_CEILING_OF, BUFFER_FLOOR_OF, BUF_BEG, BUF_BEG_BYTE)
(BUF_BEGV, BUF_BEGV_BYTE, BUF_PT, BUF_PT_BYTE, BUF_ZV)
(BUF_ZV_BYTE, BUF_GPT_ADDR, BUF_Z_ADDR, BUF_GAP_END_ADDR)
(BUF_COMPUTE_UNCHANGED, SET_PT, TEMP_SET_PT, SET_PT_BOTH)
(TEMP_SET_PT_BOTH, BUF_TEMP_SET_PT, SET_BUF_BEGV, SET_BUF_ZV)
(SET_BUF_BEGV_BOTH, SET_BUF_ZV_BOTH, SET_BUF_PT_BOTH)
(BYTE_POS_ADDR, CHAR_POS_ADDR, CHAR_TO_BYTE, BYTE_TO_CHAR)
(PTR_BYTE_POS, FETCH_CHAR, FETCH_CHAR_AS_MULTIBYTE)
(BUF_BYTE_ADDRESS, BUF_CHAR_ADDRESS, BUF_PTR_BYTE_POS)
(BUF_FETCH_CHAR, BUF_FETCH_CHAR_AS_MULTIBYTE, BUF_FETCH_BYTE)
(BUFFER_PVEC_INIT, BUFFER_LIVE_P, BUFFER_HIDDEN_P)
(BUFFER_CHECK_INDIRECTION, OVERLAY_POSITION, PER_BUFFER_VALUE_P)
(SET_PER_BUFFER_VALUE_P, PER_BUFFER_IDX):
Now inline functions instead of macros.
---
 src/buffer.h | 661 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 395 insertions(+), 266 deletions(-)

diff --git a/src/buffer.h b/src/buffer.h
index 14de70c648..82d9350bfc 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -31,12 +31,11 @@ #define EMACS_BUFFER_H
 
 /* Accessing the parameters of the current buffer.  */
 
-/* These macros come in pairs, one for the char position
+/* These constants and macros come in pairs, one for the char position
    and one for the byte position.  */
 
 /* Position of beginning of buffer.  */
-#define BEG (1)
-#define BEG_BYTE (BEG)
+enum { BEG = 1, BEG_BYTE = BEG };
 
 /* Position of beginning of accessible range of buffer.  */
 #define BEGV (current_buffer->begv)
@@ -96,59 +95,7 @@ #define OVERLAY_MODIFF (current_buffer->text->overlay_modiff)
 
 /* Modification count as of last visit or save.  */
 #define SAVE_MODIFF (current_buffer->text->save_modiff)
-
-/* BUFFER_CEILING_OF (resp. BUFFER_FLOOR_OF), when applied to n, return
-   the max (resp. min) p such that
-
-   BYTE_POS_ADDR (p) - BYTE_POS_ADDR (n) == p - n       */
-
-#define BUFFER_CEILING_OF(BYTEPOS) \
-  (((BYTEPOS) < GPT_BYTE && GPT < ZV ? GPT_BYTE : ZV_BYTE) - 1)
-#define BUFFER_FLOOR_OF(BYTEPOS) \
-  (BEGV <= GPT && GPT_BYTE <= (BYTEPOS) ? GPT_BYTE : BEGV_BYTE)
 \f
-/* Similar macros to operate on a specified buffer.
-   Note that many of these evaluate the buffer argument more than once.  */
-
-/* Position of beginning of buffer.  */
-#define BUF_BEG(buf) (BEG)
-#define BUF_BEG_BYTE(buf) (BEG_BYTE)
-
-/* The BUF_BEGV[_BYTE], BUF_ZV[_BYTE], and BUF_PT[_BYTE] macros cannot
-   be used for assignment; use SET_BUF_* macros below for that.  */
-
-/* Position of beginning of accessible range of buffer.  */
-#define BUF_BEGV(buf)					\
-   (buf == current_buffer ? BEGV			\
-    : NILP (BVAR (buf, begv_marker)) ? buf->begv	\
-    : marker_position (BVAR (buf, begv_marker)))
-
-#define BUF_BEGV_BYTE(buf)				\
-   (buf == current_buffer ? BEGV_BYTE			\
-    : NILP (BVAR (buf, begv_marker)) ? buf->begv_byte	\
-    : marker_byte_position (BVAR (buf, begv_marker)))
-
-/* Position of point in buffer.  */
-#define BUF_PT(buf)					\
-   (buf == current_buffer ? PT				\
-    : NILP (BVAR (buf, pt_marker)) ? buf->pt		\
-    : marker_position (BVAR (buf, pt_marker)))
-
-#define BUF_PT_BYTE(buf)				\
-   (buf == current_buffer ? PT_BYTE			\
-    : NILP (BVAR (buf, pt_marker)) ? buf->pt_byte	\
-    : marker_byte_position (BVAR (buf, pt_marker)))
-
-/* Position of end of accessible range of buffer.  */
-#define BUF_ZV(buf)					\
-   (buf == current_buffer ? ZV				\
-    : NILP (BVAR (buf, zv_marker)) ? buf->zv		\
-    : marker_position (BVAR (buf, zv_marker)))
-
-#define BUF_ZV_BYTE(buf)				\
-   (buf == current_buffer ? ZV_BYTE			\
-    : NILP (BVAR (buf, zv_marker)) ? buf->zv_byte	\
-    : marker_byte_position (BVAR (buf, zv_marker)))
 
 /* Position of gap in buffer.  */
 #define BUF_GPT(buf) ((buf)->text->gpt)
@@ -161,15 +108,6 @@ #define BUF_Z_BYTE(buf) ((buf)->text->z_byte)
 /* Address of beginning of buffer.  */
 #define BUF_BEG_ADDR(buf) ((buf)->text->beg)
 
-/* Address of beginning of gap of buffer.  */
-#define BUF_GPT_ADDR(buf) ((buf)->text->beg + (buf)->text->gpt_byte - BEG_BYTE)
-
-/* Address of end of buffer.  */
-#define BUF_Z_ADDR(buf) ((buf)->text->beg + (buf)->text->gap_size + (buf)->text->z_byte - BEG_BYTE)
-
-/* Address of end of gap in buffer.  */
-#define BUF_GAP_END_ADDR(buf) ((buf)->text->beg + (buf)->text->gpt_byte + (buf)->text->gap_size - BEG_BYTE)
-
 /* Size of gap.  */
 #define BUF_GAP_SIZE(buf) ((buf)->text->gap_size)
 
@@ -209,43 +147,8 @@ #define OVERLAY_UNCHANGED_MODIFIED \
   BUF_OVERLAY_UNCHANGED_MODIFIED (current_buffer)
 #define BEG_UNCHANGED BUF_BEG_UNCHANGED (current_buffer)
 #define END_UNCHANGED BUF_END_UNCHANGED (current_buffer)
-
-/* Compute how many characters at the top and bottom of BUF are
-   unchanged when the range START..END is modified.  This computation
-   must be done each time BUF is modified.  */
-
-#define BUF_COMPUTE_UNCHANGED(buf, start, end)				\
-  do									\
-    {									\
-      if (BUF_UNCHANGED_MODIFIED (buf) == BUF_MODIFF (buf)		\
-	  && (BUF_OVERLAY_UNCHANGED_MODIFIED (buf)			\
-	      == BUF_OVERLAY_MODIFF (buf)))				\
-	{								\
-	  BUF_BEG_UNCHANGED (buf) = (start) - BUF_BEG (buf);		\
-	  BUF_END_UNCHANGED (buf) = BUF_Z (buf) - (end);		\
-	}								\
-      else								\
-	{								\
-	  if (BUF_Z (buf) - (end) < BUF_END_UNCHANGED (buf))		\
-	    BUF_END_UNCHANGED (buf) = BUF_Z (buf) - (end);		\
-	  if ((start) - BUF_BEG (buf) < BUF_BEG_UNCHANGED (buf))	\
-	    BUF_BEG_UNCHANGED (buf) = (start) - BUF_BEG (buf);		\
-	}								\
-    }									\
-  while (false)
-
 \f
-/* Macros to set PT in the current buffer, or another buffer.  */
-
-#define SET_PT(position) (set_point (position))
-#define TEMP_SET_PT(position) (temp_set_point (current_buffer, (position)))
-
-#define SET_PT_BOTH(position, byte) (set_point_both (position, byte))
-#define TEMP_SET_PT_BOTH(position, byte) \
-  (temp_set_point_both (current_buffer, (position), (byte)))
-
-#define BUF_TEMP_SET_PT(buffer, position) \
-  (temp_set_point ((buffer), (position)))
+/* Functions to set PT in the current buffer, or another buffer.  */
 
 extern void set_point (ptrdiff_t);
 extern void temp_set_point (struct buffer *, ptrdiff_t);
@@ -255,39 +158,32 @@ #define BUF_TEMP_SET_PT(buffer, position) \
 extern void set_point_from_marker (Lisp_Object);
 extern void enlarge_buffer_text (struct buffer *, ptrdiff_t);
 
+INLINE void
+SET_PT (ptrdiff_t position)
+{
+  set_point (position);
+}
+INLINE void
+TEMP_SET_PT (ptrdiff_t position)
+{
+  temp_set_point (current_buffer, position);
+}
+INLINE void
+SET_PT_BOTH (ptrdiff_t position, ptrdiff_t byte)
+{
+  set_point_both (position, byte);
+}
+INLINE void
+TEMP_SET_PT_BOTH (ptrdiff_t position, ptrdiff_t byte)
+{
+  temp_set_point_both (current_buffer, position, byte);
+}
+INLINE void
+BUF_TEMP_SET_PT (struct buffer *buffer, ptrdiff_t position)
+{
+  temp_set_point (buffer, position);
+}
 \f
-/* Macros for setting the BEGV, ZV or PT of a given buffer.
-
-   The ..._BOTH macros take both a charpos and a bytepos,
-   which must correspond to each other.
-
-   The macros without ..._BOTH take just a charpos,
-   and compute the bytepos from it.  */
-
-#define SET_BUF_BEGV(buf, charpos)				 \
-  ((buf)->begv_byte = buf_charpos_to_bytepos ((buf), (charpos)), \
-   (buf)->begv = (charpos))
-
-#define SET_BUF_ZV(buf, charpos)				\
-  ((buf)->zv_byte = buf_charpos_to_bytepos ((buf), (charpos)),	\
-   (buf)->zv = (charpos))
-
-#define SET_BUF_BEGV_BOTH(buf, charpos, byte)		\
-  ((buf)->begv = (charpos),				\
-   (buf)->begv_byte = (byte))
-
-#define SET_BUF_ZV_BOTH(buf, charpos, byte)		\
-  ((buf)->zv = (charpos),				\
-   (buf)->zv_byte = (byte))
-
-#define SET_BUF_PT_BOTH(buf, charpos, byte)		\
-  ((buf)->pt = (charpos),				\
-   (buf)->pt_byte = (byte))
-\f
-/* Macros to access a character or byte in the current buffer,
-   or convert between a byte position and an address.
-   These macros do not check that the position is in range.  */
-
 /* Maximum number of bytes in a buffer.
    A buffer cannot contain more bytes than a 1-origin fixnum can represent,
    nor can it be so large that C pointer arithmetic stops working.
@@ -298,115 +194,21 @@ #define BUF_BYTES_MAX \
 /* Maximum gap size after compact_buffer, in bytes.  Also
    used in make_gap_larger to get some extra reserved space.  */
 
-#define GAP_BYTES_DFL 2000
+enum { GAP_BYTES_DFL = 2000 };
 
 /* Minimum gap size after compact_buffer, in bytes.  Also
    used in make_gap_smaller to avoid too small gap size.  */
 
-#define GAP_BYTES_MIN 20
-
-/* Return the address of byte position N in current buffer.  */
-
-#define BYTE_POS_ADDR(n) \
-  (((n) >= GPT_BYTE ? GAP_SIZE : 0) + (n) + BEG_ADDR - BEG_BYTE)
-
-/* Return the address of char position N.  */
-
-#define CHAR_POS_ADDR(n)			\
-  (((n) >= GPT ? GAP_SIZE : 0)			\
-   + buf_charpos_to_bytepos (current_buffer, n)	\
-   + BEG_ADDR - BEG_BYTE)
-
-/* Convert a character position to a byte position.  */
-
-#define CHAR_TO_BYTE(charpos)			\
-  (buf_charpos_to_bytepos (current_buffer, charpos))
-
-/* Convert a byte position to a character position.  */
-
-#define BYTE_TO_CHAR(bytepos)			\
-  (buf_bytepos_to_charpos (current_buffer, bytepos))
+enum { GAP_BYTES_MIN = 20 };
 
 /* For those very rare cases where you may have a "random" pointer into
    the middle of a multibyte char, this moves to the next boundary.  */
 extern ptrdiff_t advance_to_char_boundary (ptrdiff_t byte_pos);
 
-/* Convert PTR, the address of a byte in the buffer, into a byte position.  */
-
-#define PTR_BYTE_POS(ptr) \
-  ((ptr) - (current_buffer)->text->beg					    \
-   - (ptr - (current_buffer)->text->beg <= GPT_BYTE - BEG_BYTE ? 0 : GAP_SIZE) \
-   + BEG_BYTE)
-
-/* Return character at byte position POS.  See the caveat WARNING for
-   FETCH_MULTIBYTE_CHAR below.  */
-
-#define FETCH_CHAR(pos)				      	\
-  (!NILP (BVAR (current_buffer, enable_multibyte_characters))	\
-   ? FETCH_MULTIBYTE_CHAR ((pos))		      	\
-   : FETCH_BYTE ((pos)))
-
-/* Return the byte at byte position N.  */
+/* Return the byte at byte position N.
+   Do not check that the position is in range.  */
 
 #define FETCH_BYTE(n) *(BYTE_POS_ADDR ((n)))
-
-/* Return character at byte position POS.  If the current buffer is unibyte
-   and the character is not ASCII, make the returning character
-   multibyte.  */
-
-#define FETCH_CHAR_AS_MULTIBYTE(pos)			\
-  (!NILP (BVAR (current_buffer, enable_multibyte_characters))	\
-   ? FETCH_MULTIBYTE_CHAR ((pos))			\
-   : UNIBYTE_TO_CHAR (FETCH_BYTE ((pos))))
-
-\f
-/* Macros for accessing a character or byte,
-   or converting between byte positions and addresses,
-   in a specified buffer.  */
-
-/* Return the address of character at byte position POS in buffer BUF.
-   Note that both arguments can be computed more than once.  */
-
-#define BUF_BYTE_ADDRESS(buf, pos) \
-  ((buf)->text->beg + (pos) - BEG_BYTE \
-   + ((pos) >= (buf)->text->gpt_byte ? (buf)->text->gap_size : 0))
-
-/* Return the address of character at char position POS in buffer BUF.
-   Note that both arguments can be computed more than once.  */
-
-#define BUF_CHAR_ADDRESS(buf, pos) \
-  ((buf)->text->beg + buf_charpos_to_bytepos ((buf), (pos)) - BEG_BYTE	\
-   + ((pos) >= (buf)->text->gpt ? (buf)->text->gap_size : 0))
-
-/* Convert PTR, the address of a char in buffer BUF,
-   into a character position.  */
-
-#define BUF_PTR_BYTE_POS(buf, ptr)				\
-  ((ptr) - (buf)->text->beg					\
-   - (ptr - (buf)->text->beg <= BUF_GPT_BYTE (buf) - BEG_BYTE	\
-      ? 0 : BUF_GAP_SIZE ((buf)))				\
-   + BEG_BYTE)
-
-/* Return the character at byte position POS in buffer BUF.   */
-
-#define BUF_FETCH_CHAR(buf, pos)	      	\
-  (!NILP (buf->enable_multibyte_characters)	\
-   ? BUF_FETCH_MULTIBYTE_CHAR ((buf), (pos))    \
-   : BUF_FETCH_BYTE ((buf), (pos)))
-
-/* Return character at byte position POS in buffer BUF.  If BUF is
-   unibyte and the character is not ASCII, make the returning
-   character multibyte.  */
-
-#define BUF_FETCH_CHAR_AS_MULTIBYTE(buf, pos)           \
-  (! NILP (BVAR ((buf), enable_multibyte_characters))   \
-   ? BUF_FETCH_MULTIBYTE_CHAR ((buf), (pos))            \
-   : UNIBYTE_TO_CHAR (BUF_FETCH_BYTE ((buf), (pos))))
-
-/* Return the byte at byte position N in buffer BUF.   */
-
-#define BUF_FETCH_BYTE(buf, n) \
-  *(BUF_BYTE_ADDRESS ((buf), (n)))
 \f
 /* Define the actual buffer data structures.  */
 
@@ -482,6 +284,13 @@ #define BUF_FETCH_BYTE(buf, n) \
 
 #define BVAR(buf, field) ((buf)->field ## _)
 
+/* Max number of builtin per-buffer variables.  */
+enum { MAX_PER_BUFFER_VARS = 50 };
+
+/* Special values for struct buffer.modtime.  */
+enum { NONEXISTENT_MODTIME_NSECS = -1 };
+enum { UNKNOWN_MODTIME_NSECS = -2 };
+
 /* This is the structure that the buffer Lisp object points to.  */
 
 struct buffer
@@ -796,7 +605,6 @@ #define BVAR(buf, field) ((buf)->field ## _)
      for a buffer-local variable is stored in that variable's slot
      in buffer_local_flags as a Lisp integer.  If the index is -1,
      this means the variable is always local in all buffers.  */
-#define MAX_PER_BUFFER_VARS 50
   char local_flags[MAX_PER_BUFFER_VARS];
 
   /* Set to the modtime of the visited file when read or written.
@@ -804,8 +612,6 @@ #define MAX_PER_BUFFER_VARS 50
      visited file was nonexistent.  modtime.tv_nsec ==
      UNKNOWN_MODTIME_NSECS means visited file modtime unknown;
      in no case complain about any mismatch on next save attempt.  */
-#define NONEXISTENT_MODTIME_NSECS (-1)
-#define UNKNOWN_MODTIME_NSECS (-2)
   struct timespec modtime;
 
   /* Size of the file when modtime was set.  This is used to detect the
@@ -1018,49 +824,281 @@ bset_width_table (struct buffer *b, Lisp_Object val)
   b->width_table_ = val;
 }
 
+/* BUFFER_CEILING_OF (resp. BUFFER_FLOOR_OF), when applied to n, return
+   the max (resp. min) p such that
+
+   BYTE_POS_ADDR (p) - BYTE_POS_ADDR (n) == p - n       */
+
+INLINE ptrdiff_t
+BUFFER_CEILING_OF (ptrdiff_t bytepos)
+{
+  return (bytepos < GPT_BYTE && GPT < ZV ? GPT_BYTE : ZV_BYTE) - 1;
+}
+
+INLINE ptrdiff_t
+BUFFER_FLOOR_OF (ptrdiff_t bytepos)
+{
+  return BEGV <= GPT && GPT_BYTE <= bytepos ? GPT_BYTE : BEGV_BYTE;
+}
+
+/* The BUF_BEGV[_BYTE], BUF_ZV[_BYTE], and BUF_PT[_BYTE] functions cannot
+   be used for assignment; use SET_BUF_* functions below for that.  */
+
+/* Position of beginning of accessible range of buffer.  */
+INLINE ptrdiff_t
+BUF_BEGV (struct buffer *buf)
+{
+  return (buf == current_buffer ? BEGV
+	  : NILP (BVAR (buf, begv_marker)) ? buf->begv
+	  : marker_position (BVAR (buf, begv_marker)));
+}
+
+INLINE ptrdiff_t
+BUF_BEGV_BYTE (struct buffer *buf)
+{
+  return (buf == current_buffer ? BEGV_BYTE
+	  : NILP (BVAR (buf, begv_marker)) ? buf->begv_byte
+	  : marker_byte_position (BVAR (buf, begv_marker)));
+}
+
+/* Position of point in buffer.  */
+INLINE ptrdiff_t
+BUF_PT (struct buffer *buf)
+{
+  return (buf == current_buffer ? PT
+	  : NILP (BVAR (buf, pt_marker)) ? buf->pt
+	  : marker_position (BVAR (buf, pt_marker)));
+}
+
+INLINE ptrdiff_t
+BUF_PT_BYTE (struct buffer *buf)
+{
+  return (buf == current_buffer ? PT_BYTE
+	  : NILP (BVAR (buf, pt_marker)) ? buf->pt_byte
+	  : marker_byte_position (BVAR (buf, pt_marker)));
+}
+
+/* Position of end of accessible range of buffer.  */
+INLINE ptrdiff_t
+BUF_ZV (struct buffer *buf)
+{
+  return (buf == current_buffer ? ZV
+	  : NILP (BVAR (buf, zv_marker)) ? buf->zv
+	  : marker_position (BVAR (buf, zv_marker)));
+}
+
+INLINE ptrdiff_t
+BUF_ZV_BYTE (struct buffer *buf)
+{
+  return (buf == current_buffer ? ZV_BYTE
+	  : NILP (BVAR (buf, zv_marker)) ? buf->zv_byte
+	  : marker_byte_position (BVAR (buf, zv_marker)));
+}
+
+/* Similar functions to operate on a specified buffer.  */
+
+/* Position of beginning of buffer.  */
+INLINE ptrdiff_t
+BUF_BEG (struct buffer *buf)
+{
+  return BEG;
+}
+
+INLINE ptrdiff_t
+BUF_BEG_BYTE (struct buffer *buf)
+{
+  return BEG_BYTE;
+}
+
+/* Address of beginning of gap of buffer.  */
+INLINE unsigned char *
+BUF_GPT_ADDR (struct buffer *buf)
+{
+  return buf->text->beg + buf->text->gpt_byte - BEG_BYTE;
+}
+
+/* Address of end of buffer.  */
+INLINE unsigned char *
+BUF_Z_ADDR (struct buffer *buf)
+{
+  return buf->text->beg + buf->text->gap_size + buf->text->z_byte - BEG_BYTE;
+}
+
+/* Address of end of gap in buffer.  */
+INLINE unsigned char *
+BUF_GAP_END_ADDR (struct buffer *buf)
+{
+  return buf->text->beg + buf->text->gpt_byte + buf->text->gap_size - BEG_BYTE;
+}
+
+/* Compute how many characters at the top and bottom of BUF are
+   unchanged when the range START..END is modified.  This computation
+   must be done each time BUF is modified.  */
+
+INLINE void
+BUF_COMPUTE_UNCHANGED (struct buffer *buf, ptrdiff_t start, ptrdiff_t end)
+{
+  if (BUF_UNCHANGED_MODIFIED (buf) == BUF_MODIFF (buf)
+      && (BUF_OVERLAY_UNCHANGED_MODIFIED (buf)
+	  == BUF_OVERLAY_MODIFF (buf)))
+    {
+      buf->text->beg_unchanged = start - BUF_BEG (buf);
+      buf->text->end_unchanged = BUF_Z (buf) - (end);
+    }
+  else
+    {
+      if (BUF_Z (buf) - end < BUF_END_UNCHANGED (buf))
+	buf->text->end_unchanged = BUF_Z (buf) - end;
+      if (start - BUF_BEG (buf) < BUF_BEG_UNCHANGED (buf))
+	buf->text->beg_unchanged = start - BUF_BEG (buf);
+    }
+}
+
+/* Functions for setting the BEGV, ZV or PT of a given buffer.
+
+   The ..._BOTH functions take both a charpos and a bytepos,
+   which must correspond to each other.
+
+   The functions without ..._BOTH take just a charpos,
+   and compute the bytepos from it.  */
+
+INLINE void
+SET_BUF_BEGV (struct buffer *buf, ptrdiff_t charpos)
+{
+  buf->begv_byte = buf_charpos_to_bytepos (buf, charpos);
+  buf->begv = charpos;
+}
+
+INLINE void
+SET_BUF_ZV (struct buffer *buf, ptrdiff_t charpos)
+{
+  buf->zv_byte = buf_charpos_to_bytepos (buf, charpos);
+  buf->zv = charpos;
+}
+
+INLINE void
+SET_BUF_BEGV_BOTH (struct buffer *buf, ptrdiff_t charpos, ptrdiff_t byte)
+{
+  buf->begv = charpos;
+  buf->begv_byte = byte;
+}
+
+INLINE void
+SET_BUF_ZV_BOTH (struct buffer *buf, ptrdiff_t charpos, ptrdiff_t byte)
+{
+  buf->zv = charpos;
+  buf->zv_byte = byte;
+}
+
+INLINE void
+SET_BUF_PT_BOTH (struct buffer *buf, ptrdiff_t charpos, ptrdiff_t byte)
+{
+  buf->pt = charpos;
+  buf->pt_byte = byte;
+}
+
+/* Functions to access a character or byte in the current buffer,
+   or convert between a byte position and an address.
+   These functions do not check that the position is in range.  */
+
+/* Return the address of byte position N in current buffer.  */
+
+INLINE unsigned char *
+BYTE_POS_ADDR (ptrdiff_t n)
+{
+  return (n < GPT_BYTE ? 0 : GAP_SIZE) + n + BEG_ADDR - BEG_BYTE;
+}
+
+/* Return the address of char position N.  */
+
+INLINE unsigned char *
+CHAR_POS_ADDR (ptrdiff_t n)
+{
+  return ((n < GPT ? 0 : GAP_SIZE)
+	  + buf_charpos_to_bytepos (current_buffer, n)
+	  + BEG_ADDR - BEG_BYTE);
+}
+
+/* Convert a character position to a byte position.  */
+
+INLINE ptrdiff_t
+CHAR_TO_BYTE (ptrdiff_t charpos)
+{
+  return buf_charpos_to_bytepos (current_buffer, charpos);
+}
+
+/* Convert a byte position to a character position.  */
+
+INLINE ptrdiff_t
+BYTE_TO_CHAR (ptrdiff_t bytepos)
+{
+  return buf_bytepos_to_charpos (current_buffer, bytepos);
+}
+
+/* Convert PTR, the address of a byte in the buffer, into a byte position.  */
+
+INLINE ptrdiff_t
+PTR_BYTE_POS (unsigned char const *ptr)
+{
+  ptrdiff_t byte = ptr - current_buffer->text->beg;
+  return byte - (byte <= GPT_BYTE - BEG_BYTE ? 0 : GAP_SIZE) + BEG_BYTE;
+}
+
 /* Number of Lisp_Objects at the beginning of struct buffer.
    If you add, remove, or reorder Lisp_Objects within buffer
    structure, make sure that this is still correct.  */
 
-#define BUFFER_LISP_SIZE						\
-  PSEUDOVECSIZE (struct buffer, cursor_in_non_selected_windows_)
+enum { BUFFER_LISP_SIZE = PSEUDOVECSIZE (struct buffer,
+					 cursor_in_non_selected_windows_) };
 
 /* Allocated size of the struct buffer part beyond leading
    Lisp_Objects, in word_size units.  */
 
-#define BUFFER_REST_SIZE (VECSIZE (struct buffer) - BUFFER_LISP_SIZE)
+enum { BUFFER_REST_SIZE = VECSIZE (struct buffer) - BUFFER_LISP_SIZE };
 
 /* Initialize the pseudovector header of buffer object.  BUFFER_LISP_SIZE
    is required for GC, but BUFFER_REST_SIZE is set up just to be consistent
    with other pseudovectors.  */
 
-#define BUFFER_PVEC_INIT(b)                                    \
-  XSETPVECTYPESIZE (b, PVEC_BUFFER, BUFFER_LISP_SIZE, BUFFER_REST_SIZE)
+INLINE void
+BUFFER_PVEC_INIT (struct buffer *b)
+{
+  XSETPVECTYPESIZE (b, PVEC_BUFFER, BUFFER_LISP_SIZE, BUFFER_REST_SIZE);
+}
 
 /* Convenient check whether buffer B is live.  */
 
-#define BUFFER_LIVE_P(b) (!NILP (BVAR (b, name)))
+INLINE bool
+BUFFER_LIVE_P (struct buffer *b)
+{
+  return !NILP (BVAR (b, name));
+}
 
 /* Convenient check whether buffer B is hidden (i.e. its name
    starts with a space).  Caller must ensure that B is live.  */
 
-#define BUFFER_HIDDEN_P(b) (SREF (BVAR (b, name), 0) == ' ')
+INLINE bool
+BUFFER_HIDDEN_P (struct buffer *b)
+{
+  return SREF (BVAR (b, name), 0) == ' ';
+}
 
 /* Verify indirection counters.  */
 
-#define BUFFER_CHECK_INDIRECTION(b)			\
-  do {							\
-    if (BUFFER_LIVE_P (b))				\
-      {							\
-	if (b->base_buffer)				\
-	  {						\
-	    eassert (b->indirections == -1);		\
-	    eassert (b->base_buffer->indirections > 0);	\
-	  }						\
-	else						\
-	  eassert (b->indirections >= 0);		\
-      }							\
-  } while (false)
+INLINE void
+BUFFER_CHECK_INDIRECTION (struct buffer *b)
+{
+  if (BUFFER_LIVE_P (b))
+    {
+      if (b->base_buffer)
+	{
+	  eassert (b->indirections == -1);
+	  eassert (b->base_buffer->indirections > 0);
+	}
+      else
+	eassert (b->indirections >= 0);
+    }
+}
 
 /* Chain of all buffers, including killed ones.  */
 
@@ -1157,7 +1195,9 @@ record_unwind_current_buffer (void)
 
 /* Get overlays at POSN into array OVERLAYS with NOVERLAYS elements.
    If NEXTP is non-NULL, return next overlay there.
-   See overlay_at arg CHANGE_REQ for meaning of CHRQ arg.  */
+   See overlay_at arg CHANGE_REQ for meaning of CHRQ arg.
+   This macro might evaluate its args multiple times,
+   and it treat some args as lvalues.  */
 
 #define GET_OVERLAYS_AT(posn, overlays, noverlays, nextp, chrq)		\
   do {									\
@@ -1207,6 +1247,10 @@ buffer_has_overlays (void)
 {
   return current_buffer->overlays_before || current_buffer->overlays_after;
 }
+\f
+/* Functions for accessing a character or byte,
+   or converting between byte positions and addresses,
+   in a specified buffer.  */
 
 /* Return character code of multi-byte form at byte position POS.  If POS
    doesn't point the head of valid multi-byte form, only the byte at
@@ -1232,6 +1276,80 @@ BUF_FETCH_MULTIBYTE_CHAR (struct buffer *buf, ptrdiff_t pos)
   return STRING_CHAR (p);
 }
 
+/* Return character at byte position POS.
+   If the current buffer is unibyte and the character is not ASCII,
+   make the returning character multibyte.  */
+
+INLINE int
+FETCH_CHAR_AS_MULTIBYTE (ptrdiff_t pos)
+{
+  return (!NILP (BVAR (current_buffer, enable_multibyte_characters))
+	  ? FETCH_MULTIBYTE_CHAR (pos)
+	  : UNIBYTE_TO_CHAR (FETCH_BYTE (pos)));
+}
+
+/* Return character at byte position POS.
+   See the caveat WARNING for FETCH_MULTIBYTE_CHAR above.  */
+
+INLINE int
+FETCH_CHAR (ptrdiff_t pos)
+{
+  return (!NILP (BVAR (current_buffer, enable_multibyte_characters))
+	  ? FETCH_MULTIBYTE_CHAR (pos)
+	  : FETCH_BYTE (pos));
+}
+
+/* Return the address of character at byte position POS in buffer BUF.
+   Note that both arguments can be computed more than once.  */
+
+INLINE unsigned char *
+BUF_BYTE_ADDRESS (struct buffer *buf, ptrdiff_t pos)
+{
+  return (buf->text->beg + pos - BEG_BYTE
+	  + (pos < buf->text->gpt_byte ? 0 : buf->text->gap_size));
+}
+
+/* Return the address of character at char position POS in buffer BUF.
+   Note that both arguments can be computed more than once.  */
+
+INLINE unsigned char *
+BUF_CHAR_ADDRESS (struct buffer *buf, ptrdiff_t pos)
+{
+  return (buf->text->beg + buf_charpos_to_bytepos (buf, pos) - BEG_BYTE
+	  + (pos < buf->text->gpt ? 0 : buf->text->gap_size));
+}
+
+/* Convert PTR, the address of a char in buffer BUF,
+   into a character position.  */
+
+INLINE ptrdiff_t
+BUF_PTR_BYTE_POS (struct buffer *buf, unsigned char *ptr)
+{
+  ptrdiff_t byte = ptr - buf->text->beg;
+  return (byte - (byte <= BUF_GPT_BYTE (buf) - BEG_BYTE ? 0 : BUF_GAP_SIZE (buf))
+	  + BEG_BYTE);
+}
+
+/* Return the byte at byte position N in buffer BUF.   */
+
+INLINE unsigned char
+BUF_FETCH_BYTE (struct buffer *buf, ptrdiff_t n)
+{
+  return *BUF_BYTE_ADDRESS (buf, n);
+}
+
+/* Return character at byte position POS in buffer BUF.  If BUF is
+   unibyte and the character is not ASCII, make the returning
+   character multibyte.  */
+
+INLINE int
+BUF_FETCH_CHAR_AS_MULTIBYTE (struct buffer *buf, ptrdiff_t pos)
+{
+  return (! NILP (BVAR (buf, enable_multibyte_characters))
+	  ? BUF_FETCH_MULTIBYTE_CHAR (buf, pos)
+	  : UNIBYTE_TO_CHAR (BUF_FETCH_BYTE (buf, pos)));
+}
+
 /* Return number of windows showing B.  */
 
 INLINE int
@@ -1260,8 +1378,11 @@ #define OVERLAY_PLIST(OV) XOVERLAY (OV)->plist
 /* Return the actual buffer position for the marker P.
    We assume you know which buffer it's pointing into.  */
 
-#define OVERLAY_POSITION(P) \
- (MARKERP (P) ? marker_position (P) : (emacs_abort (), 0))
+INLINE ptrdiff_t
+OVERLAY_POSITION (Lisp_Object p)
+{
+  return marker_position (p);
+}
 
 \f
 /***********************************************************************
@@ -1297,16 +1418,22 @@ #define PER_BUFFER_VAR_IDX(VAR) \
 /* Value is true if the variable with index IDX has a local value
    in buffer B.  */
 
-#define PER_BUFFER_VALUE_P(B, IDX)		\
-  (eassert (valid_per_buffer_idx (IDX)),	\
-   (B)->local_flags[IDX])
+INLINE bool
+PER_BUFFER_VALUE_P (struct buffer *b, int idx)
+{
+  eassert (valid_per_buffer_idx (idx));
+  return b->local_flags[idx];
+}
 
 /* Set whether per-buffer variable with index IDX has a buffer-local
    value in buffer B.  VAL zero means it hasn't.  */
 
-#define SET_PER_BUFFER_VALUE_P(B, IDX, VAL)	\
-  (eassert (valid_per_buffer_idx (IDX)),	\
-   (B)->local_flags[IDX] = (VAL))
+INLINE void
+SET_PER_BUFFER_VALUE_P (struct buffer *b, int idx, bool val)
+{
+  eassert (valid_per_buffer_idx (idx));
+  b->local_flags[idx] = val;
+}
 
 /* Return the index value of the per-buffer variable at offset OFFSET
    in the buffer structure.
@@ -1326,11 +1453,13 @@ #define SET_PER_BUFFER_VALUE_P(B, IDX, VAL)	\
    new buffer.
 
    If a slot in this structure corresponding to a DEFVAR_PER_BUFFER is
-   zero, that is a bug */
+   zero, that is a bug.  */
 
-
-#define PER_BUFFER_IDX(OFFSET) \
-      XFIXNUM (*(Lisp_Object *)((OFFSET) + (char *) &buffer_local_flags))
+INLINE int
+PER_BUFFER_IDX (ptrdiff_t offset)
+{
+  return XFIXNUM (*(Lisp_Object *) (offset + (char *) &buffer_local_flags));
+}
 
 /* Functions to get and set default value of the per-buffer
    variable at offset OFFSET in the buffer structure.  */
-- 
2.17.1

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 3 Sep 2019 23:05:00 -0700
> Cc: Eli Zaretskii <eliz@gnu.org>, Daniel Colascione <dancol@dancol.org>,
>  emacs-devel@gnu.org
> 
> In buffer.h, prefer inline functions to function-like macros
> when either will do.  This helps avoid confusion about how
> many times an arg is evaluated.  On my platform, this patch
> improves performance of ‘make compile-always’ by 5.7%.

How portable is "INLINE" (and if it's portable enough, why do we use a
macro for it)?  If some platforms don't support it, and these macros
become non-inline functions, those platforms will be punished by this
kind of changes.

I FWIW, personally find the issue of confusion about macro argument
evaluation to be a very weak one as justification to get rid of macros
that needed approximately zero maintenance for many years.  But that's
me.

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Paul Eggert]

Eli Zaretskii wrote:

> How portable is "INLINE"

Quite portable, as its portability issues were shaken out years ago. It's been 
used in Emacs since 2013, and it's designed for C99-and-later so future C 
compilers should support it well.

> (and if it's portable enough, why do we use a jmacro for it)?

First, for convenience so that it's easier to follow the C rule that exactly one 
compilation unit must declare a function to be 'extern inline' if any 
compilation unit declares the function to be plain 'inline'. Second, for 
portability to C compilers that still do not support C99-style 'extern inline' 
(are these still significant? I don't offhand know).

> I FWIW, personally find the issue of confusion about macro argument
> evaluation to be a very weak one as justification to get rid of macros

There are other reasons to avoid macros. Aside from the performance issue I 
mentioned in my buffer.h patch, debugging Emacs can be easier when C macros are 
avoided. On the Ubuntu 18.04.3 platform where I'm typing this message, when I'm 
using GDB to debug /usr/bin/emacs I can print expressions involving functions or 
constants but not expressions involving macros; this is because Ubuntu builds 
/usr/bin/emacs with -g instead of -g3. So avoiding macros helps make it easier 
to debug Emacs in my situation, which is common on GNUish platforms.

Of course functions and constants cannot replace some C macros, such as INLINE. 
But many of Emacs's C macros ought to be functions or constants now, and this is 
not simply to avoid confusion.

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Daniel Colascione]

>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Tue, 3 Sep 2019 23:05:00 -0700
>> Cc: Eli Zaretskii <eliz@gnu.org>, Daniel Colascione <dancol@dancol.org>,
>>  emacs-devel@gnu.org
>>
>> In buffer.h, prefer inline functions to function-like macros
>> when either will do.  This helps avoid confusion about how
>> many times an arg is evaluated.  On my platform, this patch
>> improves performance of ‘make compile-always’ by 5.7%.
>
> How portable is "INLINE" (and if it's portable enough, why do we use a
> macro for it)?  If some platforms don't support it, and these macros
> become non-inline functions, those platforms will be punished by this
> kind of changes.
>
> I FWIW, personally find the issue of confusion about macro argument
> evaluation to be a very weak one as justification to get rid of macros
> that needed approximately zero maintenance for many years.  But that's
> me.

Using a bare 'inline' is fine these days. We already require C99. Macros
are awkward in many ways, e.g., being invisible in debugger traces. These
days, there are a lot of reasons to avoid macros and very few reasons to
use them, so let's just switch to inline functions like the rest of the
world has.

Re: Changes in GC and in pure space

[Stefan Monnier]

> How portable is "INLINE" (and if it's portable enough, why do we use a
> macro for it)?  If some platforms don't support it, and these macros
> become non-inline functions, those platforms will be punished by this
> kind of changes.

I'm not sure if replacing INLINE with nothing at all would lead to much
worse code.  Obviously, in some cases it will, but the optimizer should
generally still inline them anyway.

> I FWIW, personally find the issue of confusion about macro argument
> evaluation to be a very weak one as justification to get rid of macros
> that needed approximately zero maintenance for many years.

I don't find it terribly compelling either, although I have been bitten
a few times.  OTOH as functions, they tend to behave better in GDB.
So overall, I think it's good to use functions rather than macros where
both are applicable (just like in Elisp).


        Stefan

Re: Changes in GC and in pure space

[Óscar Fuentes]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> How portable is "INLINE" (and if it's portable enough, why do we use a
>> macro for it)?  If some platforms don't support it, and these macros
>> become non-inline functions, those platforms will be punished by this
>> kind of changes.
>
> I'm not sure if replacing INLINE with nothing at all would lead to much
> worse code.  Obviously, in some cases it will, but the optimizer should
> generally still inline them anyway.

At least in the C++ world, `inline' is used for putting function
definitions on headers, not for inlining.

There is a consensus from many years ago that the compiler is expected
to do the right thing wrt inlining irrespectively of the presence or
ausence of the keyword, the same way `register` is no longer used for
overriding the compiler's register allocation system.

Re: Changes in GC and in pure space

[Paul Eggert]

Óscar Fuentes wrote:
> At least in the C++ world, `inline' is used for putting function
> definitions on headers, not for inlining.
> 
> There is a consensus from many years ago that the compiler is expected
> to do the right thing wrt inlining irrespectively of the presence or
> ausence of the keyword

Things are a bit different in C. In C, the consensus you mention holds for 
'static inline' (where the 'inline' is typically just a noise word nowadays), 
but it does not hold for the plain 'inline' functions that we're talking about 
because C requires that if one compilation unit defines a plain 'inline' 
function, then exactly one other compilation unit must define that function as 
'extern inline'.

Emacs rarely uses 'static inline', due to the consensus that you mention. In the 
few exceptions where Emacs does use 'static inline', I vaguely recall there 
being performance advantages when compiled by GCC in typical platforms, as GCC 
does not ignore the 'inline' in these exceptional situations.

Changes in GC and in pure space

[Paul Eggert]

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

Stefan Monnier wrote:
> I'm not sure if replacing INLINE with nothing at all would lead to much
> worse code.

I assume you meant "'static'" instead of "nothing at all" - we couldn't replace 
INLINE with nothing at all, as that would cause duplicate function definitions.

I did try 'static' (actually, 'static ATTRIBUTE_UNUSED' to pacify gcc 
-Wunused-function) and found that it improved performance of 'make 
compile-always' by 8% on my old work desktop and by 6% on a newer machine, so I 
installed the attached patch. Using 'static' also shrank the text size of the 
executable by 4.5%, for what that's worth. 'static' also has the virtue of being 
simpler. At some point we can remove the EMACS_EXTERN_INLINE code as I expect it 
won't be needed.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-plain-static-for-Emacs-C-inline-functions.patch --]
[-- Type: text/x-patch; name="0001-Use-plain-static-for-Emacs-C-inline-functions.patch", Size: 3549 bytes --]

From 365dad197bac5deec9244fd9c189d23c46c99b31 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 4 Sep 2019 23:13:54 -0700
Subject: [PATCH] =?UTF-8?q?Use=20plain=20=E2=80=98static=E2=80=99=20for=20?=
 =?UTF-8?q?Emacs=20C=20inline=20functions?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This improved performance of ‘make compile-always’ by 8.2%
on my platform (AMD Phenom II X4 910e, Fedora 30 x86-64).
* src/conf_post.h (INLINE, EXTERN_INLINE, INLINE_HEADER_BEGIN)
(INLINE_HEADER_END) [!EMACS_EXTERN_INLINE]: Use plain ‘static’.
---
 src/conf_post.h | 43 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/src/conf_post.h b/src/conf_post.h
index 4af1ba9331..43f98620a4 100644
--- a/src/conf_post.h
+++ b/src/conf_post.h
@@ -373,8 +373,13 @@ #define ATTRIBUTE_MALLOC_SIZE(args) ATTRIBUTE_MALLOC ATTRIBUTE_ALLOC_SIZE (args)
 #undef noinline
 #endif
 
-/* Use Gnulib's extern-inline module for extern inline functions.
-   An include file foo.h should prepend FOO_INLINE to function
+/* INLINE marks functions defined in Emacs-internal C headers.
+   INLINE is implemented via C99-style 'extern inline' if Emacs is built
+   with -DEMACS_EXTERN_INLINE; otherwise it is implemented via 'static'.
+   EMACS_EXTERN_INLINE is no longer the default, as 'static' seems to
+   have better performance with GCC.
+
+   An include file foo.h should prepend INLINE to function
    definitions, with the following overall pattern:
 
       [#include any other .h files first.]
@@ -399,20 +404,40 @@ #define ATTRIBUTE_MALLOC_SIZE(args) ATTRIBUTE_MALLOC ATTRIBUTE_ALLOC_SIZE (args)
    For Emacs, this is done by having emacs.c first '#define INLINE
    EXTERN_INLINE' and then include every .h file that uses INLINE.
 
-   The INLINE_HEADER_BEGIN and INLINE_HEADER_END suppress bogus
-   warnings in some GCC versions; see ../m4/extern-inline.m4.
+   The INLINE_HEADER_BEGIN and INLINE_HEADER_END macros suppress bogus
+   warnings in some GCC versions; see ../m4/extern-inline.m4.  */
+
+#ifdef EMACS_EXTERN_INLINE
+
+/* Use Gnulib's extern-inline module for extern inline functions.
 
    C99 compilers compile functions like 'incr' as C99-style extern
    inline functions.  Buggy GCC implementations do something similar with
    GNU-specific keywords.  Buggy non-GCC compilers use static
    functions, which bloats the code but is good enough.  */
 
-#ifndef INLINE
-# define INLINE _GL_INLINE
+# ifndef INLINE
+#  define INLINE _GL_INLINE
+# endif
+# define EXTERN_INLINE _GL_EXTERN_INLINE
+# define INLINE_HEADER_BEGIN _GL_INLINE_HEADER_BEGIN
+# define INLINE_HEADER_END _GL_INLINE_HEADER_END
+
+#else
+
+/* Use 'static' instead of 'extern inline' because 'static' typically
+   has better performance for Emacs.  Do not use the 'inline' keyword,
+   as modern compilers inline automatically.  ATTRIBUTE_UNUSED
+   pacifies gcc -Wunused-function.  */
+
+# ifndef INLINE
+#  define INLINE EXTERN_INLINE
+# endif
+# define EXTERN_INLINE static ATTRIBUTE_UNUSED
+# define INLINE_HEADER_BEGIN
+# define INLINE_HEADER_END
+
 #endif
-#define EXTERN_INLINE _GL_EXTERN_INLINE
-#define INLINE_HEADER_BEGIN _GL_INLINE_HEADER_BEGIN
-#define INLINE_HEADER_END _GL_INLINE_HEADER_END
 
 /* 'int x UNINIT;' is equivalent to 'int x;', except it cajoles GCC
    into not warning incorrectly about use of an uninitialized variable.  */
-- 
2.17.1

Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Your reasoning seems wise to me -- so let's simply decide to put the
GC changes into a branch for now, and integrate them after version 27.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Changes in GC and in pure space

[Angelo Graziosi]

Eli Zaretskii wrote
>
> If some platforms don't support it, and these macros
> become non-inline functions, those platforms will be punished by this
> kind of changes
>
> [...] But that's me.
>

Me too! 

When I was a kid I was taught that macros are preferable to functions if these are short, don't use loop etc., and this to save on the cost of the function call.. also old FORTRAN has 'statement functions'. If I remember correctly, even if language as C++ has 'inline' statement, this does not mean that the function will be really 'inlined', is the compiler that decide about the fate of functions declared 'inline'..

Maybe all I learned from Borland manuals was wrong... and all programs I wrote up to 2004 have to be rewritten..... Gulp!

Re: Changes in GC and in pure space

[Paul Eggert]

Angelo Graziosi wrote:
> Maybe all I learned from Borland manuals was wrong.

It is indeed wrong, at least for the buffer.h macros that I recently turned into 
INLINE functions, as that sped up my usual nontrivial benchmark ('cd lisp; make 
compile-always') by 5.7% on my platform.

Stefan made a suggestion in this thread that I think can get us another ~5% 
speedup on the same benchmark. I'm looking into that. It also involves using 
functions not macros.

Re: Changes in GC and in pure space

[Stefan Monnier]

> When I was a kid I was taught that macros are preferable to functions if
> these are short, don't use loop etc., and this to save on the cost of the
> function call.. also old FORTRAN has 'statement functions'.
[...]
> Maybe all I learned from Borland manuals was wrong...

I don't think we can expect Emacs will be compiled by a compiler
comparable to Borland's (these were focused on generating code as fast
as possible).

Nowadays compilers work hard to encourage you to write readable code and
to focus your optimization efforts on algorithmic changes rather than
low-level micro-optimization (since these may just as well end up being
pessimizations unless you really know what's going on).

This said, I'd expect that using macros to perform "manual inlining" can
still be beneficial for the case where you compile with -O0 (which is
why I asked Paul to still use such macros in a few specific places where
it seemed worthwhile: I use -O0 most of the time).


        Stefan