bug#36370: 27.0.50; XFIXNAT called on negative numbers

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

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

I just spent some time reviewing the use of XFIXNAT, after being
bitten by this code in fileio.c:

      if (FIXNUMP (tem))
    nextpos = XFIXNAT (tem);

In the following cases, negative numbers passed to APIs which aren't
strictly internal or low-level cause us to call XFIXNAT on a negative
number:

(indent-to -100 -100)
(set-text-properties -1 -1 nil "")
(make-network-process :name "name" :family 'ipv6 :remote [-1 0 0 0 0 0 0 0 0])
(end-kbd-macro -1 nil)
(previous-single-property-change (point) 'dummy nil -1)

I think those six cases are worth fixing, but more generally, I think
it would help avoid such errors if XFIXNAT were "always" paired with
FIXNATP, XFIXNUM with FIXNUMP, and XCHARACTER (which would be new)
with CHARACTERP.

So I'm going to be brave here and admit that in many cases, while
XFIXNUM was correct, it took me a long time to see that because the
check was performed somewhere else.

I understand there is a very slight performance hit on some platforms,
but is that really still an issue? Also, consistent pairing would
catch a few cases in which we call XFIXNUM but could get away with an
XFIXNAT, such as this code in self-insert-command:

  if (!CHARACTERP (c))
    bitch_at_user ();
  else {
    int character = translate_char (Vtranslation_table_for_input,
                    XFIXNUM (c));

I also think it would make sense to eassume (FIXNUMP (a)) in both
XFIXNAT and XFIXNUM (but not XHASH, or XUFIXNUM if that's what XHASH
uses). It might catch bugs like

    (set-text-properties nil nil nil "")

currently not throwing an error.

The patch I'm attaching fixes only things I'm reasonably sure are bugs.

[-- Attachment #2: xfixnat.diff --]
[-- Type: text/x-patch, Size: 8925 bytes --]

diff --git a/src/ccl.c b/src/ccl.c
index ec108e30d8..c853e61982 100644
--- a/src/ccl.c
+++ b/src/ccl.c
@@ -2062,7 +2062,7 @@ #define CCL_EXECUTE_BUF_SIZE 1024
       if (TYPE_RANGED_FIXNUMP (int, AREF (status, i)))
 	ccl.reg[i] = XFIXNUM (AREF (status, i));
     }
-  if (FIXNUMP (AREF (status, i)))
+  if (FIXNATP (AREF (status, i)))
     {
       i = XFIXNAT (AREF (status, 8));
       if (ccl.ic < i && i < ccl.size)
diff --git a/src/dosfns.c b/src/dosfns.c
index 47c545007a..fb5bcc9ad3 100644
--- a/src/dosfns.c
+++ b/src/dosfns.c
@@ -72,7 +72,7 @@ DEFUN ("int86", Fint86, Sint86, 2, 2, 0,
   if (no < 0 || no > 0xff || ASIZE (registers) != 8)
     return Qnil;
   for (i = 0; i < 8; i++)
-    CHECK_FIXNUM (AREF (registers, i));
+    CHECK_FIXNAT (AREF (registers, i));
 
   inregs.x.ax    = (unsigned long) XFIXNAT (AREF (registers, 0));
   inregs.x.bx    = (unsigned long) XFIXNAT (AREF (registers, 1));
@@ -139,7 +139,7 @@ DEFUN ("msdos-memput", Fdos_memput, Sdos_memput, 2, 2, 0,
 
   for (i = 0; i < len; i++)
     {
-      CHECK_FIXNUM (AREF (vector, i));
+      CHECK_FIXNAT (AREF (vector, i));
       buf[i] = (unsigned char) XFIXNAT (AREF (vector, i)) & 0xFF;
     }
 
diff --git a/src/fileio.c b/src/fileio.c
index 5dd14daacb..b2db6efd54 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4719,7 +4719,7 @@ because (1) it preserves some marker positions and (2) it puts less data
 		 the format conversion.  */
 	      Lisp_Object tem = XCAR (old_undo);
 	      if (CONSP (tem) && FIXNUMP (XCAR (tem))
-		  && FIXNUMP (XCDR (tem))
+		  && FIXNATP (XCDR (tem))
 		  && XFIXNAT (XCDR (tem)) == PT + old_inserted)
 		XSETCDR (tem, make_fixnum (PT + inserted));
 	    }
@@ -5391,7 +5391,7 @@ a_write (int desc, Lisp_Object string, ptrdiff_t pos,
     {
       tem = Fcar_safe (Fcar (*annot));
       nextpos = pos - 1;
-      if (FIXNUMP (tem))
+      if (FIXNATP (tem))
 	nextpos = XFIXNAT (tem);
 
       /* If there are no more annotations in this range,
@@ -5847,7 +5847,7 @@ DEFUN ("do-auto-save", Fdo_auto_save, Sdo_auto_save, 0, 2, "",
 
 	    set_buffer_internal (b);
 	    if (NILP (Vauto_save_include_big_deletions)
-		&& FIXNUMP (BVAR (b, save_length))
+		&& FIXNATP (BVAR (b, save_length))
 		/* A short file is likely to change a large fraction;
 		   spare the user annoying messages.  */
 		&& XFIXNAT (BVAR (b, save_length)) > 5000
diff --git a/src/image.c b/src/image.c
index 7b648c46ae..2ecea0893f 100644
--- a/src/image.c
+++ b/src/image.c
@@ -2385,7 +2385,7 @@ lookup_image (struct frame *f, Lisp_Object spec)
 #endif
 
 	  ascent = image_spec_value (spec, QCascent, NULL);
-	  if (FIXNUMP (ascent))
+	  if (FIXNATP (ascent))
 	    img->ascent = XFIXNAT (ascent);
 	  else if (EQ (ascent, Qcenter))
 	    img->ascent = CENTERED_IMAGE_ASCENT;
diff --git a/src/lisp.h b/src/lisp.h
index 77fc22d118..a1bd6d8f6b 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -2968,6 +2968,14 @@ #define CHECK_FIXNUM_COERCE_MARKER(x)					\
       CHECK_TYPE (FIXNUMP (x), Qinteger_or_marker_p, x);		\
   } while (false)
 
+#define CHECK_FIXNAT_COERCE_MARKER(x)					\
+  do {									\
+    if (MARKERP ((x)))							\
+      XSETFASTINT (x, marker_position (x));				\
+    else								\
+      CHECK_TYPE (FIXNATP (x), Qinteger_or_marker_p, x);		\
+  } while (false)
+
 INLINE double
 XFLOATINT (Lisp_Object n)
 {
diff --git a/src/process.c b/src/process.c
index 6717ccb418..6bc3c75190 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2673,7 +2673,7 @@ conv_lisp_to_sockaddr (int family, Lisp_Object address, struct sockaddr *sa, int
 	  hostport = XFIXNUM (p->contents[--len]);
 	  sin6->sin6_port = htons (hostport);
 	  for (i = 0; i < len; i++)
-	    if (FIXNUMP (p->contents[i]))
+	    if (FIXNATP (p->contents[i]))
 	      {
 		int j = XFIXNAT (p->contents[i]) & 0xffff;
 		ip6[i] = ntohs (j);
diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..6b20a34820 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -134,8 +134,8 @@ validate_interval_range (Lisp_Object object, Lisp_Object *begin,
   ptrdiff_t searchpos;
 
   CHECK_STRING_OR_BUFFER (object);
-  CHECK_FIXNUM_COERCE_MARKER (*begin);
-  CHECK_FIXNUM_COERCE_MARKER (*end);
+  CHECK_FIXNAT_COERCE_MARKER (*begin);
+  CHECK_FIXNAT_COERCE_MARKER (*end);
 
   /* If we are asked for a point, but from a subr which operates
      on a range, then return nothing.  */
@@ -790,14 +790,14 @@ DEFUN ("next-single-char-property-change", Fnext_single_char_property_change,
 	  Fset_buffer (object);
 	}
 
-      CHECK_FIXNUM_COERCE_MARKER (position);
+      CHECK_FIXNAT_COERCE_MARKER (position);
 
       initial_value = Fget_char_property (position, prop, object);
 
       if (NILP (limit))
 	XSETFASTINT (limit, ZV);
       else
-	CHECK_FIXNUM_COERCE_MARKER (limit);
+	CHECK_FIXNAT_COERCE_MARKER (limit);
 
       if (XFIXNAT (position) >= XFIXNAT (limit))
 	{
@@ -874,12 +874,12 @@ DEFUN ("previous-single-char-property-change",
 	  Fset_buffer (object);
 	}
 
-      CHECK_FIXNUM_COERCE_MARKER (position);
+      CHECK_FIXNAT_COERCE_MARKER (position);
 
       if (NILP (limit))
 	XSETFASTINT (limit, BEGV);
       else
-	CHECK_FIXNUM_COERCE_MARKER (limit);
+	CHECK_FIXNAT_COERCE_MARKER (limit);
 
       if (XFIXNAT (position) <= XFIXNAT (limit))
 	{
@@ -942,7 +942,7 @@ DEFUN ("next-property-change", Fnext_property_change,
     XSETBUFFER (object, current_buffer);
 
   if (!NILP (limit) && !EQ (limit, Qt))
-    CHECK_FIXNUM_COERCE_MARKER (limit);
+    CHECK_FIXNAT_COERCE_MARKER (limit);
 
   i = validate_interval_range (object, &position, &position, soft);
 
@@ -975,7 +975,7 @@ DEFUN ("next-property-change", Fnext_property_change,
 
   if (!next
       || (next->position
-	  >= (FIXNUMP (limit)
+	  >= (FIXNATP (limit)
 	      ? XFIXNAT (limit)
 	      : (STRINGP (object)
 		 ? SCHARS (object)
@@ -1009,7 +1009,7 @@ DEFUN ("next-single-property-change", Fnext_single_property_change,
     XSETBUFFER (object, current_buffer);
 
   if (!NILP (limit))
-    CHECK_FIXNUM_COERCE_MARKER (limit);
+    CHECK_FIXNAT_COERCE_MARKER (limit);
 
   i = validate_interval_range (object, &position, &position, soft);
   if (!i)
@@ -1024,7 +1024,7 @@ DEFUN ("next-single-property-change", Fnext_single_property_change,
 
   if (!next
       || (next->position
-	  >= (FIXNUMP (limit)
+	  >= (FIXNATP (limit)
 	      ? XFIXNAT (limit)
 	      : (STRINGP (object)
 		 ? SCHARS (object)
@@ -1056,7 +1056,7 @@ DEFUN ("previous-property-change", Fprevious_property_change,
     XSETBUFFER (object, current_buffer);
 
   if (!NILP (limit))
-    CHECK_FIXNUM_COERCE_MARKER (limit);
+    CHECK_FIXNAT_COERCE_MARKER (limit);
 
   i = validate_interval_range (object, &position, &position, soft);
   if (!i)
@@ -1074,7 +1074,7 @@ DEFUN ("previous-property-change", Fprevious_property_change,
 
   if (!previous
       || (previous->position + LENGTH (previous)
-	  <= (FIXNUMP (limit)
+	  <= (FIXNATP (limit)
 	      ? XFIXNAT (limit)
 	      : (STRINGP (object) ? 0 : BUF_BEGV (XBUFFER (object))))))
     return limit;
@@ -1106,7 +1106,7 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change,
     XSETBUFFER (object, current_buffer);
 
   if (!NILP (limit))
-    CHECK_FIXNUM_COERCE_MARKER (limit);
+    CHECK_FIXNAT_COERCE_MARKER (limit);
 
   i = validate_interval_range (object, &position, &position, soft);
 
@@ -1127,7 +1127,7 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change,
 
   if (!previous
       || (previous->position + LENGTH (previous)
-	  <= (FIXNUMP (limit)
+	  <= (FIXNATP (limit)
 	      ? XFIXNAT (limit)
 	      : (STRINGP (object) ? 0 : BUF_BEGV (XBUFFER (object))))))
     return limit;
@@ -1353,8 +1353,8 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
   /* If we want no properties for a whole string,
      get rid of its intervals.  */
   if (NILP (properties) && STRINGP (object)
-      && XFIXNAT (start) == 0
-      && XFIXNAT (end) == SCHARS (object))
+      && FIXNATP (start) && XFIXNAT (start) == 0
+      && FIXNATP (end) && XFIXNAT (end) == SCHARS (object))
     {
       if (!string_intervals (object))
 	return Qnil;
diff --git a/src/w32term.c b/src/w32term.c
index 5726124b0e..886fc6c751 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -2463,7 +2463,7 @@ w32_draw_glyph_string (struct glyph_string *s)
 		  Lisp_Object val
 		    = buffer_local_value (Qunderline_minimum_offset,
 					s->w->contents);
-		  if (FIXNUMP (val))
+		  if (FIXNATP (val))
 		    minimum_offset = XFIXNAT (val);
 		  else
 		    minimum_offset = 1;
diff --git a/src/xterm.c b/src/xterm.c
index 1acff2af0d..64467e0d92 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -3806,7 +3806,7 @@ x_draw_glyph_string (struct glyph_string *s)
 		  Lisp_Object val
 		    = buffer_local_value (Qunderline_minimum_offset,
 					  s->w->contents);
-		  if (FIXNUMP (val))
+		  if (FIXNATP (val))
 		    minimum_offset = XFIXNAT (val);
 		  else
 		    minimum_offset = 1;

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Paul Eggert]

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

Thanks for looking into this. Cleaning this up has been on my to-do list 
for a while, and something along these lines has been discussed on the 
mailing list. Some comments on your fixes:

* The ccl.c, fileio.c, image.c, and process.c fixes should use XFIXNUM, 
not XFIXNAT, since the code is range-checking after extraction anyway 
and the latter range-checking should do the right thing. Come to think 
of it, the ccl.c and image.c code is buggy regardless of the 
XFIXNUM/XFIXNAT business since the Emacs integer might not fit into int 
range, and that should be fixed too.

* Instead of adding a new macro CHECK_FIXNAT_COERCE_MARKER, the code 
should stick with CHECK_FIXNUM_COERCE_MARKER and do the usual 
range-checking on the results. This should yield more-precise 
diagnostics. Most of the range-checking is there already.

* The set_text_properties fix can be done more efficiently via EQ.

* I'm mildly inclined to think that w32term.c and xterm.c should treate 
negative minimum values as 0 when the actual value must be nonnegative. 
That is, a negative minimum should act like a minimum of 0, not as a 
minimum of 1 as in your proposed patch.

To help prevent similar problems in the future, XFIXNUM, XFIXNAT etc. 
should check that their arguments are of the proper type when 
ENABLE_CHECKING is nonzero.

A proposed patch is attached; it should do all the above.

PS. At some point we should also check that make_fixnum is invoked only 
on values in fixnum range, but that's a matter for a later patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Clean-up-use-of-XFIXNUM-etc.patch --]
[-- Type: text/x-patch; name="0001-Clean-up-use-of-XFIXNUM-etc.patch", Size: 15900 bytes --]

From bdec4f8e8d7278d68d3534b87c29362ffcbe23d2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 26 Jun 2019 17:52:06 -0700
Subject: [PATCH] Clean up use of XFIXNUM etc.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A few bits of the code were relying on the fact that XFIXNUM,
XFIXNAT, and XUFIXNUM do something even with arguments that
are not fixnums/fixnats.  Separate these rare uses out into
XFIXNUM_RAW and XUFIXNUM_RAW.
Problem and original patch reported by Pip Cet (Bug#36370).
* src/ccl.c (Fccl_execute_on_string):
* src/fileio.c (Finsert_file_contents, a_write)
(Fdo_auto_save):
* src/process.c (conv_lisp_to_sockaddr):
* src/textprop.c (Fnext_single_char_property_change)
(Fprevious_single_char_property_change)
(Fnext_property_change, Fnext_single_property_change)
(Fprevious_property_change)
(Fprevious_single_property_change):
Don’t assume fixnums are nonnegative.
* src/ccl.c (Fccl_execute_on_string):
Fix range-checking bug if AREF (status, i) is out of int range.
* src/data.c (arith_driver): Use XFIXNUM_RAW as we want
efficient garbage if the value is not a fixnum.
* src/dosfns.c (Fint86, Fdos_memput):
Check that args are nonnegative.
* src/image.c (lookup_image): Check that args are in range.
* src/lisp.h (lisp_h_XHASH): Use XUFIXNUM_RAW, since this
is for hashing.
(lisp_h_XFIXNAT, XFIXNAT) [USE_LSB_TAG]: Remove macros.
(lisp_h_XFIXNUM_RAW, XFIXNUM_RAW) [USE_LSB_TAG]: New macros, with
the semantics of the old macros without _RAW.
(XFIXNUM_RAW, XUFIXNUM_RAW): New inline functions, with the
semantics of the old functions without _RAW.
(FIXNUMP): Move definition up to avoid forward use.
(XFIXNUM, XFIXNAT, XUFIXNUM): Use eassume to add a runtime
check (when debugging) that the argument has the proper form.
(XFIXNUM, XFIXNAT): Now inline functions only, since they
refer to their arguments more than once now that they use eassume.
* src/textprop.c (Fprevious_single_char_property_change):
Avoid fixnum overflow with invalid input.
(set_text_properties): Fix unlikely failure
to validate arguments, by using EQ instead of XFIXNAT.
* src/w32term.c (w32_draw_glyph_string):
* src/xterm.c (x_draw_glyph_string):
Treat negative minimums as 0 rather than as garbage patterns.
---
 src/ccl.c      |  6 ++--
 src/data.c     |  2 +-
 src/dosfns.c   |  4 +--
 src/fileio.c   |  6 ++--
 src/image.c    |  4 +--
 src/lisp.h     | 75 +++++++++++++++++++++++++-------------------------
 src/process.c  |  2 +-
 src/textprop.c | 29 +++++++++----------
 src/w32term.c  |  2 +-
 src/xterm.c    |  2 +-
 10 files changed, 67 insertions(+), 65 deletions(-)

diff --git a/src/ccl.c b/src/ccl.c
index ec108e30d8..f1d4c28df1 100644
--- a/src/ccl.c
+++ b/src/ccl.c
@@ -2064,9 +2064,9 @@ #define CCL_EXECUTE_BUF_SIZE 1024
     }
   if (FIXNUMP (AREF (status, i)))
     {
-      i = XFIXNAT (AREF (status, 8));
-      if (ccl.ic < i && i < ccl.size)
-	ccl.ic = i;
+      EMACS_INT ic = XFIXNUM (AREF (status, i));
+      if (ccl.ic < ic && ic < ccl.size)
+	ccl.ic = ic;
     }
 
   buf_magnification = ccl.buf_magnification ? ccl.buf_magnification : 1;
diff --git a/src/data.c b/src/data.c
index c1699aeae7..46bd7e0e25 100644
--- a/src/data.c
+++ b/src/data.c
@@ -2928,7 +2928,7 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args,
   ptrdiff_t argnum = 0;
   /* Set ACCUM to VAL's value if it is a fixnum, otherwise to some
      ignored value to avoid using an uninitialized variable later.  */
-  intmax_t accum = XFIXNUM (val);
+  intmax_t accum = XFIXNUM_RAW (val);
 
   if (FIXNUMP (val))
     while (true)
diff --git a/src/dosfns.c b/src/dosfns.c
index 47c545007a..fb5bcc9ad3 100644
--- a/src/dosfns.c
+++ b/src/dosfns.c
@@ -72,7 +72,7 @@ DEFUN ("int86", Fint86, Sint86, 2, 2, 0,
   if (no < 0 || no > 0xff || ASIZE (registers) != 8)
     return Qnil;
   for (i = 0; i < 8; i++)
-    CHECK_FIXNUM (AREF (registers, i));
+    CHECK_FIXNAT (AREF (registers, i));
 
   inregs.x.ax    = (unsigned long) XFIXNAT (AREF (registers, 0));
   inregs.x.bx    = (unsigned long) XFIXNAT (AREF (registers, 1));
@@ -139,7 +139,7 @@ DEFUN ("msdos-memput", Fdos_memput, Sdos_memput, 2, 2, 0,
 
   for (i = 0; i < len; i++)
     {
-      CHECK_FIXNUM (AREF (vector, i));
+      CHECK_FIXNAT (AREF (vector, i));
       buf[i] = (unsigned char) XFIXNAT (AREF (vector, i)) & 0xFF;
     }
 
diff --git a/src/fileio.c b/src/fileio.c
index 0da9894a73..61e10dac47 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4720,7 +4720,7 @@ because (1) it preserves some marker positions and (2) it puts less data
 	      Lisp_Object tem = XCAR (old_undo);
 	      if (CONSP (tem) && FIXNUMP (XCAR (tem))
 		  && FIXNUMP (XCDR (tem))
-		  && XFIXNAT (XCDR (tem)) == PT + old_inserted)
+		  && XFIXNUM (XCDR (tem)) == PT + old_inserted)
 		XSETCDR (tem, make_fixnum (PT + inserted));
 	    }
 	}
@@ -5392,7 +5392,7 @@ a_write (int desc, Lisp_Object string, ptrdiff_t pos,
       tem = Fcar_safe (Fcar (*annot));
       nextpos = pos - 1;
       if (FIXNUMP (tem))
-	nextpos = XFIXNAT (tem);
+	nextpos = XFIXNUM (tem);
 
       /* If there are no more annotations in this range,
 	 output the rest of the range all at once.  */
@@ -5850,7 +5850,7 @@ DEFUN ("do-auto-save", Fdo_auto_save, Sdo_auto_save, 0, 2, "",
 		&& FIXNUMP (BVAR (b, save_length))
 		/* A short file is likely to change a large fraction;
 		   spare the user annoying messages.  */
-		&& XFIXNAT (BVAR (b, save_length)) > 5000
+		&& XFIXNUM (BVAR (b, save_length)) > 5000
 		&& (growth_factor * (BUF_Z (b) - BUF_BEG (b))
 		    < (growth_factor - 1) * XFIXNAT (BVAR (b, save_length)))
 		/* These messages are frequent and annoying for `*mail*'.  */
diff --git a/src/image.c b/src/image.c
index 7b648c46ae..d204749631 100644
--- a/src/image.c
+++ b/src/image.c
@@ -2385,13 +2385,13 @@ lookup_image (struct frame *f, Lisp_Object spec)
 #endif
 
 	  ascent = image_spec_value (spec, QCascent, NULL);
-	  if (FIXNUMP (ascent))
+	  if (RANGED_FIXNUMP (0, ascent, INT_MAX))
 	    img->ascent = XFIXNAT (ascent);
 	  else if (EQ (ascent, Qcenter))
 	    img->ascent = CENTERED_IMAGE_ASCENT;
 
 	  margin = image_spec_value (spec, QCmargin, NULL);
-	  if (FIXNUMP (margin))
+	  if (RANGED_FIXNUMP (0, margin, INT_MAX))
 	    img->vmargin = img->hmargin = XFIXNAT (margin);
 	  else if (CONSP (margin))
 	    {
diff --git a/src/lisp.h b/src/lisp.h
index 77fc22d118..077d236065 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -414,12 +414,11 @@ #define lisp_h_XCAR(c) XCONS (c)->u.s.car
 #define lisp_h_XCDR(c) XCONS (c)->u.s.u.cdr
 #define lisp_h_XCONS(a) \
    (eassert (CONSP (a)), XUNTAG (a, Lisp_Cons, struct Lisp_Cons))
-#define lisp_h_XHASH(a) XUFIXNUM (a)
+#define lisp_h_XHASH(a) XUFIXNUM_RAW (a)
 #if USE_LSB_TAG
 # define lisp_h_make_fixnum(n) \
     XIL ((EMACS_INT) (((EMACS_UINT) (n) << INTTYPEBITS) + Lisp_Int0))
-# define lisp_h_XFIXNAT(a) XFIXNUM (a)
-# define lisp_h_XFIXNUM(a) (XLI (a) >> INTTYPEBITS)
+# define lisp_h_XFIXNUM_RAW(a) (XLI (a) >> INTTYPEBITS)
 # define lisp_h_XTYPE(a) ((enum Lisp_Type) (XLI (a) & ~VALMASK))
 #endif
 
@@ -460,8 +459,7 @@ #define lisp_h_XHASH(a) XUFIXNUM (a)
 # define XHASH(a) lisp_h_XHASH (a)
 # if USE_LSB_TAG
 #  define make_fixnum(n) lisp_h_make_fixnum (n)
-#  define XFIXNAT(a) lisp_h_XFIXNAT (a)
-#  define XFIXNUM(a) lisp_h_XFIXNUM (a)
+#  define XFIXNUM_RAW(a) lisp_h_XFIXNUM_RAW (a)
 #  define XTYPE(a) lisp_h_XTYPE (a)
 # endif
 #endif
@@ -1141,17 +1139,9 @@ #define MOST_NEGATIVE_FIXNUM (-1 - MOST_POSITIVE_FIXNUM)
 }
 
 INLINE EMACS_INT
-(XFIXNUM) (Lisp_Object a)
+(XFIXNUM_RAW) (Lisp_Object a)
 {
-  return lisp_h_XFIXNUM (a);
-}
-
-INLINE EMACS_INT
-(XFIXNAT) (Lisp_Object a)
-{
-  EMACS_INT n = lisp_h_XFIXNAT (a);
-  eassume (0 <= n);
-  return n;
+  return lisp_h_XFIXNUM_RAW (a);
 }
 
 #else /* ! USE_LSB_TAG */
@@ -1179,9 +1169,11 @@ make_fixnum (EMACS_INT n)
   return XIL (n);
 }
 
-/* Extract A's value as a signed integer.  */
+/* Extract A's value as a signed integer.  Unlike XFIXNUM, this works
+   on any Lisp object, although the resulting integer is useful only
+   for things like hashing when A is not a fixnum.  */
 INLINE EMACS_INT
-XFIXNUM (Lisp_Object a)
+XFIXNUM_RAW (Lisp_Object a)
 {
   EMACS_INT i = XLI (a);
   if (! USE_LSB_TAG)
@@ -1192,31 +1184,36 @@ XFIXNUM (Lisp_Object a)
   return i >> INTTYPEBITS;
 }
 
-/* Like XFIXNUM (A), but may be faster.  A must be nonnegative.
-   If ! USE_LSB_TAG, this takes advantage of the fact that Lisp
-   integers have zero-bits in their tags.  */
-INLINE EMACS_INT
-XFIXNAT (Lisp_Object a)
+#endif /* ! USE_LSB_TAG */
+
+INLINE bool
+(FIXNUMP) (Lisp_Object x)
 {
-  EMACS_INT int0 = Lisp_Int0;
-  EMACS_INT n = USE_LSB_TAG ? XFIXNUM (a) : XLI (a) - (int0 << VALBITS);
-  eassume (0 <= n);
-  return n;
+  return lisp_h_FIXNUMP (x);
 }
 
-#endif /* ! USE_LSB_TAG */
+INLINE EMACS_INT
+XFIXNUM (Lisp_Object a)
+{
+  eassume (FIXNUMP (a));
+  return XFIXNUM_RAW (a);
+}
 
 /* Extract A's value as an unsigned integer in the range 0..INTMASK.  */
 INLINE EMACS_UINT
-XUFIXNUM (Lisp_Object a)
+XUFIXNUM_RAW (Lisp_Object a)
 {
   EMACS_UINT i = XLI (a);
   return USE_LSB_TAG ? i >> INTTYPEBITS : i & INTMASK;
 }
+INLINE EMACS_UINT
+XUFIXNUM (Lisp_Object a)
+{
+  eassume (FIXNUMP (a));
+  return XUFIXNUM_RAW (a);
+}
 
-/* Return A's hash, which is in the range 0..INTMASK.  Although XHASH (A) ==
-   XUFIXNUM (A) currently, XUFIXNUM should be applied only to fixnums.  */
-
+/* Return A's hash, which is in the range 0..INTMASK.  */
 INLINE EMACS_INT
 (XHASH) (Lisp_Object a)
 {
@@ -1261,12 +1258,6 @@ make_lisp_ptr (void *ptr, enum Lisp_Type type)
   return a;
 }
 
-INLINE bool
-(FIXNUMP) (Lisp_Object x)
-{
-  return lisp_h_FIXNUMP (x);
-}
-
 #define XSETINT(a, b) ((a) = make_fixnum (b))
 #define XSETFASTINT(a, b) ((a) = make_fixed_natnum (b))
 #define XSETCONS(a, b) ((a) = make_lisp_ptr (b, Lisp_Cons))
@@ -2832,6 +2823,16 @@ FIXNATP (Lisp_Object x)
 {
   return FIXNUMP (x) && 0 <= XFIXNUM (x);
 }
+
+/* Like XFIXNUM (A), but may be faster.  A must be nonnegative.  */
+INLINE EMACS_INT
+XFIXNAT (Lisp_Object a)
+{
+  eassume (FIXNATP (a));
+  EMACS_INT int0 = Lisp_Int0;
+  return USE_LSB_TAG ? XFIXNUM (a) : XLI (a) - (int0 << VALBITS);
+}
+
 INLINE bool
 NUMBERP (Lisp_Object x)
 {
diff --git a/src/process.c b/src/process.c
index 15d87cf601..cab390c10c 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2675,7 +2675,7 @@ conv_lisp_to_sockaddr (int family, Lisp_Object address, struct sockaddr *sa, int
 	  for (i = 0; i < len; i++)
 	    if (FIXNUMP (p->contents[i]))
 	      {
-		int j = XFIXNAT (p->contents[i]) & 0xffff;
+		int j = XFIXNUM (p->contents[i]) & 0xffff;
 		ip6[i] = ntohs (j);
 	      }
 	  sa->sa_family = family;
diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..3026ec7e99 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -799,7 +799,7 @@ DEFUN ("next-single-char-property-change", Fnext_single_char_property_change,
       else
 	CHECK_FIXNUM_COERCE_MARKER (limit);
 
-      if (XFIXNAT (position) >= XFIXNAT (limit))
+      if (XFIXNAT (position) >= XFIXNUM (limit))
 	{
 	  position = limit;
 	  if (XFIXNAT (position) > ZV)
@@ -881,16 +881,17 @@ DEFUN ("previous-single-char-property-change",
       else
 	CHECK_FIXNUM_COERCE_MARKER (limit);
 
-      if (XFIXNAT (position) <= XFIXNAT (limit))
+      if (XFIXNUM (position) <= XFIXNUM (limit))
 	{
 	  position = limit;
-	  if (XFIXNAT (position) < BEGV)
+	  if (XFIXNUM (position) < BEGV)
 	    XSETFASTINT (position, BEGV);
 	}
       else
 	{
 	  Lisp_Object initial_value
-	    = Fget_char_property (make_fixnum (XFIXNAT (position) - 1),
+	    = Fget_char_property (make_fixnum (XFIXNUM (position)
+					       - (0 <= XFIXNUM (position))),
 				  prop, object);
 
 	  while (true)
@@ -970,13 +971,13 @@ DEFUN ("next-property-change", Fnext_property_change,
   next = next_interval (i);
 
   while (next && intervals_equal (i, next)
-	 && (NILP (limit) || next->position < XFIXNAT (limit)))
+	 && (NILP (limit) || next->position < XFIXNUM (limit)))
     next = next_interval (next);
 
   if (!next
       || (next->position
 	  >= (FIXNUMP (limit)
-	      ? XFIXNAT (limit)
+	      ? XFIXNUM (limit)
 	      : (STRINGP (object)
 		 ? SCHARS (object)
 		 : BUF_ZV (XBUFFER (object))))))
@@ -1019,13 +1020,13 @@ DEFUN ("next-single-property-change", Fnext_single_property_change,
   next = next_interval (i);
   while (next
 	 && EQ (here_val, textget (next->plist, prop))
-	 && (NILP (limit) || next->position < XFIXNAT (limit)))
+	 && (NILP (limit) || next->position < XFIXNUM (limit)))
     next = next_interval (next);
 
   if (!next
       || (next->position
 	  >= (FIXNUMP (limit)
-	      ? XFIXNAT (limit)
+	      ? XFIXNUM (limit)
 	      : (STRINGP (object)
 		 ? SCHARS (object)
 		 : BUF_ZV (XBUFFER (object))))))
@@ -1069,13 +1070,13 @@ DEFUN ("previous-property-change", Fprevious_property_change,
   previous = previous_interval (i);
   while (previous && intervals_equal (previous, i)
 	 && (NILP (limit)
-	     || (previous->position + LENGTH (previous) > XFIXNAT (limit))))
+	     || (previous->position + LENGTH (previous) > XFIXNUM (limit))))
     previous = previous_interval (previous);
 
   if (!previous
       || (previous->position + LENGTH (previous)
 	  <= (FIXNUMP (limit)
-	      ? XFIXNAT (limit)
+	      ? XFIXNUM (limit)
 	      : (STRINGP (object) ? 0 : BUF_BEGV (XBUFFER (object))))))
     return limit;
   else
@@ -1122,13 +1123,13 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change,
   while (previous
 	 && EQ (here_val, textget (previous->plist, prop))
 	 && (NILP (limit)
-	     || (previous->position + LENGTH (previous) > XFIXNAT (limit))))
+	     || (previous->position + LENGTH (previous) > XFIXNUM (limit))))
     previous = previous_interval (previous);
 
   if (!previous
       || (previous->position + LENGTH (previous)
 	  <= (FIXNUMP (limit)
-	      ? XFIXNAT (limit)
+	      ? XFIXNUM (limit)
 	      : (STRINGP (object) ? 0 : BUF_BEGV (XBUFFER (object))))))
     return limit;
   else
@@ -1353,8 +1354,8 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
   /* If we want no properties for a whole string,
      get rid of its intervals.  */
   if (NILP (properties) && STRINGP (object)
-      && XFIXNAT (start) == 0
-      && XFIXNAT (end) == SCHARS (object))
+      && EQ (start, make_fixnum (0))
+      && EQ (end, make_fixnum (SCHARS (object))))
     {
       if (!string_intervals (object))
 	return Qnil;
diff --git a/src/w32term.c b/src/w32term.c
index 5726124b0e..97a5fc6389 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -2464,7 +2464,7 @@ w32_draw_glyph_string (struct glyph_string *s)
 		    = buffer_local_value (Qunderline_minimum_offset,
 					s->w->contents);
 		  if (FIXNUMP (val))
-		    minimum_offset = XFIXNAT (val);
+		    minimum_offset = max (0, XFIXNUM (val));
 		  else
 		    minimum_offset = 1;
 		  val = buffer_local_value (Qx_underline_at_descent_line,
diff --git a/src/xterm.c b/src/xterm.c
index 1acff2af0d..38bc17de97 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -3807,7 +3807,7 @@ x_draw_glyph_string (struct glyph_string *s)
 		    = buffer_local_value (Qunderline_minimum_offset,
 					  s->w->contents);
 		  if (FIXNUMP (val))
-		    minimum_offset = XFIXNAT (val);
+		    minimum_offset = max (0, XFIXNUM (val));
 		  else
 		    minimum_offset = 1;
 		  val = buffer_local_value (Qx_underline_at_descent_line,
-- 
2.21.0

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Thu, Jun 27, 2019 at 1:11 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
> Thanks for looking into this. Cleaning this up has been on my to-do list
> for a while, and something along these lines has been discussed on the
> mailing list. Some comments on your fixes:

I really think I should reiterate that readability is much much more
important than whatever performance tweaks XFIXNAT might allow. I'd
prefer if the code were written as though FIXNATs weren't FIXNUMs at
all: pair FIXNATP with XFIXNAT, and provide FIXNAT versions of
CHECK_RANGED_FIXNUM and CHECK_FIXNUM_COERCE_MARKER.

Hmm. I'm not sure it's a problem for you, but I'm finding it rather
hard to check in all cases whether XFIXNAT is safe. For example,

      ascent = image_spec_value (spec, QCascent, NULL);
      if (FIXNUMP (ascent))
        img->ascent = XFIXNAT (ascent);

in image.c is safe, because lookup_image is called only after valid_image_p.

   if (FIXNUMP (AREF (status, i)))
     {
-      i = XFIXNAT (AREF (status, 8));
-      if (ccl.ic < i && i < ccl.size)
-    ccl.ic = i;
+      EMACS_INT ic = XFIXNUM (AREF (status, i));
+      if (ccl.ic < ic && ic < ccl.size)
+    ccl.ic = ic;
     }

I'd prefer AREF (status, 8) here, for what readability the code gives us.

 # define XHASH(a) lisp_h_XHASH (a)
 # if USE_LSB_TAG
 #  define make_fixnum(n) lisp_h_make_fixnum (n)
-#  define XFIXNAT(a) lisp_h_XFIXNAT (a)
-#  define XFIXNUM(a) lisp_h_XFIXNUM (a)
+#  define XFIXNUM_RAW(a) lisp_h_XFIXNUM_RAW (a)
 #  define XTYPE(a) lisp_h_XTYPE (a)
 # endif
 #endif

I'd prefer leaving the macros for now; eassume (inline_function (x))
doesn't work very well on current GCC, while eassume (MACRO (x)) is
fine, so I'm sometimes building with DEFINE_KEY_OPS_AS_MACROS. (The
eassume/eassert situation is something else I've been planning to look
at in more detail, though debugging it will be easier when
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91005 is fixed).

> * The ccl.c, fileio.c, image.c, and process.c fixes should use XFIXNUM,
> not XFIXNAT, since the code is range-checking after extraction anyway
> and the latter range-checking should do the right thing. Come to think

That's an argument that either XFIXNUM or XFIXNAT are fine, and in
this case XFIXNAT seems, to me, much more readable.

> of it, the ccl.c and image.c code is buggy regardless of the
> XFIXNUM/XFIXNAT business since the Emacs integer might not fit into int
> range, and that should be fixed too.

I don't see how either ccl.c or image.c are buggy for out-of-range
integer arguments.

> * Instead of adding a new macro CHECK_FIXNAT_COERCE_MARKER, the code
> should stick with CHECK_FIXNUM_COERCE_MARKER and do the usual
> range-checking on the results. This should yield more-precise
> diagnostics. Most of the range-checking is there already.

I disagree, obviously. When you want a position, you want a FIXNAT,
not a negative number, so CHECK_FIXNAT_COERCE_MARKER everywhere would
catch those cases where someone passes an obviously invalid argument.

> * The set_text_properties fix can be done more efficiently via EQ.

I suspect your change results in a few more insns being emitted, but
I'll have to check.

> * I'm mildly inclined to think that w32term.c and xterm.c should treate
> negative minimum values as 0 when the actual value must be nonnegative.
> That is, a negative minimum should act like a minimum of 0, not as a
> minimum of 1 as in your proposed patch.

Doesn't matter, so I decided not to fiddle with this code.

> To help prevent similar problems in the future, XFIXNUM, XFIXNAT etc.
> should check that their arguments are of the proper type when
> ENABLE_CHECKING is nonzero.
>
> A proposed patch is attached; it should do all the above.

I'd prefer not touching the dosfns.c code, for the simple reason that
if anyone still uses it, they might rely on the broken behavior.

> PS. At some point we should also check that make_fixnum is invoked only
> on values in fixnum range, but that's a matter for a later patch.

Definitely.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Paul Eggert]

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

Pip Cet wrote:

> I'd prefer if the code were written as though FIXNATs weren't FIXNUMs at all.

I'm more of the opposite opinion. The original reason for XFIXNAT (under its old 
name) was performance, because long ago Emacs put the type tag in the most 
significant bits, and it was faster to mask it out than to smear the sign bit 
when you knew the fixnum was nonnegative. Nowadays that original reason is 
obsolete because the tags are in the least significant bits and XFIXNAT is just 
an alias for XFIXNUM, and if it were up to me we'd get rid of XFIXNAT entirely, 
and just use XFIXNUM. But old habits die hard....


>        ascent = image_spec_value (spec, QCascent, NULL);
>        if (FIXNUMP (ascent))
>          img->ascent = XFIXNAT (ascent);
> 
> in image.c is safe, because lookup_image is called only after valid_image_p.

Sorry, you've lost me. How does valid_image_p guarantee that 'ascent' (if a 
fixnum) is in the range 0..INT_MAX? Because if it's not in that range, the above 
code could trap.

> I'd prefer AREF (status, 8) here, for what readability the code gives us.

Sure, sounds good. See attached further patch.

> I'd prefer leaving the macros for now

I actually did it that way at first, but that failed miserably as the macros 
evaluated their arguments more than once when debugging , and lots of code 
expects them to behave like functions and evaluate their arguments exactly once. 
So functions it is.

> eassume (inline_function (x))
> doesn't work very well on current GCC, while eassume (MACRO (x)) is
> fine, so I'm sometimes building with DEFINE_KEY_OPS_AS_MACROS.

Hmm, I see I should have helped GCC more there. First, the two new eassumes 
should just be easserts as the assumptions are not helpful to the compiler. 
Second, I should restore the eassume that tells GCC that XFIXNAT returns a 
nonnegative integer.

> (The
> eassume/eassert situation is something else I've been planning to look
> at in more detail, though debugging it will be easier when
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91005 is fixed).

Sorry, I don't follow. That bug report seems to be about nested C functions; why 
is it relevant to Emacs, which doesn't use nested C functions?

>> * The ccl.c, fileio.c, image.c, and process.c fixes should use XFIXNUM,
>> not XFIXNAT, since the code is range-checking after extraction anyway
>> and the latter range-checking should do the right thing. Come to think
> 
> That's an argument that either XFIXNUM or XFIXNAT are fine, and in
> this case XFIXNAT seems, to me, much more readable.

No, as the range-checking is done after the XFIXNAT, which means the XFIXNAT 
could have an assertion failure.

>> of it, the ccl.c and image.c code is buggy regardless of the
>> XFIXNUM/XFIXNAT business since the Emacs integer might not fit into int
>> range, and that should be fixed too.
> 
> I don't see how either ccl.c or image.c are buggy for out-of-range
> integer arguments.

For example, ccl.c in master has this:

   if (FIXNUMP (AREF (status, i)))
     {
       i = XFIXNAT (AREF (status, 8));
       if (ccl.ic < i && i < ccl.size)
	ccl.ic = i;
     }

where i is a 32-bit int. If AREF (status, 8) is the 62-bit Lisp fixnum with 
value '2 * (EMACS_INT) INT_MAX + ccl.ic + 3', the assignment to i overflows, 
which can trap. Even if the assignment doesn't trap but merely wraps around, the 
next line won't do range checking correctly.

> When you want a position, you want a FIXNAT,
> not a negative number, so CHECK_FIXNAT_COERCE_MARKER everywhere would
> catch those cases where someone passes an obviously invalid argument.

But why stop there? 0 is an obviously invalid argument for buffers. :-) I think 
this is just an area where we disagree - I want a range check where you want a 
type check. It's safe either way and the range check is a tad faster (as the 
range check needs to be done anyway) and results in a crisper diagnostic when it 
fails.
> I'd prefer not touching the dosfns.c code, for the simple reason that
> if anyone still uses it, they might rely on the broken behavior.

We can't leave it alone, as XFIXNAT is now checking that its result is 
nonnegative when debugging is enabled. However, we could instead change the 
XFIXNATs to XFIXNUMs: this will generate exactly the same machine code on that 
platform when debugging is disabled, and will not trap when debugging is 
enabled. See attached patch.

[-- Attachment #2: 0001-Improve-XFIXNUM-cleanup-a-bit.txt --]
[-- Type: text/plain, Size: 4162 bytes --]

From 3f8090e91125de98798d4bfb5f91934bcac9626b Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 27 Jun 2019 01:14:53 -0700
Subject: [PATCH] Improve XFIXNUM cleanup a bit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Based on Pip Cet’s review (Bug#36370#13).
* src/ccl.c (Fccl_execute_on_string): Use clearer indexing.
* src/dosfns.c (Fint86, Fdos_memput):
Avoid runtime checks for negative fixnums when debugging.
This restores the earlier machine code.
* src/lisp.h (XFIXNUM, XUFIXNUM): Use eassert, not eassume.
(XFIXNAT): At the start, merely eassert FIXNUMP rather
than eassuming FIXNATP.  At the end, eassume that the
result is nonnegative.  This restores help to the compiler
that the previous patch mistakenly removed.
---
 src/ccl.c    |  4 ++--
 src/dosfns.c | 22 +++++++++++-----------
 src/lisp.h   | 10 ++++++----
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/src/ccl.c b/src/ccl.c
index f1d4c28df1..ff42c6f25f 100644
--- a/src/ccl.c
+++ b/src/ccl.c
@@ -2062,9 +2062,9 @@ #define CCL_EXECUTE_BUF_SIZE 1024
       if (TYPE_RANGED_FIXNUMP (int, AREF (status, i)))
 	ccl.reg[i] = XFIXNUM (AREF (status, i));
     }
-  if (FIXNUMP (AREF (status, i)))
+  if (FIXNUMP (AREF (status, 8)))
     {
-      EMACS_INT ic = XFIXNUM (AREF (status, i));
+      EMACS_INT ic = XFIXNUM (AREF (status, 8));
       if (ccl.ic < ic && ic < ccl.size)
 	ccl.ic = ic;
     }
diff --git a/src/dosfns.c b/src/dosfns.c
index fb5bcc9ad3..635f29bd65 100644
--- a/src/dosfns.c
+++ b/src/dosfns.c
@@ -72,16 +72,16 @@ DEFUN ("int86", Fint86, Sint86, 2, 2, 0,
   if (no < 0 || no > 0xff || ASIZE (registers) != 8)
     return Qnil;
   for (i = 0; i < 8; i++)
-    CHECK_FIXNAT (AREF (registers, i));
+    CHECK_FIXNUM (AREF (registers, i));
 
-  inregs.x.ax    = (unsigned long) XFIXNAT (AREF (registers, 0));
-  inregs.x.bx    = (unsigned long) XFIXNAT (AREF (registers, 1));
-  inregs.x.cx    = (unsigned long) XFIXNAT (AREF (registers, 2));
-  inregs.x.dx    = (unsigned long) XFIXNAT (AREF (registers, 3));
-  inregs.x.si    = (unsigned long) XFIXNAT (AREF (registers, 4));
-  inregs.x.di    = (unsigned long) XFIXNAT (AREF (registers, 5));
-  inregs.x.cflag = (unsigned long) XFIXNAT (AREF (registers, 6));
-  inregs.x.flags = (unsigned long) XFIXNAT (AREF (registers, 7));
+  inregs.x.ax    = (unsigned long) XFIXNUM (AREF (registers, 0));
+  inregs.x.bx    = (unsigned long) XFIXNUM (AREF (registers, 1));
+  inregs.x.cx    = (unsigned long) XFIXNUM (AREF (registers, 2));
+  inregs.x.dx    = (unsigned long) XFIXNUM (AREF (registers, 3));
+  inregs.x.si    = (unsigned long) XFIXNUM (AREF (registers, 4));
+  inregs.x.di    = (unsigned long) XFIXNUM (AREF (registers, 5));
+  inregs.x.cflag = (unsigned long) XFIXNUM (AREF (registers, 6));
+  inregs.x.flags = (unsigned long) XFIXNUM (AREF (registers, 7));
 
   int86 (no, &inregs, &outregs);
 
@@ -139,8 +139,8 @@ DEFUN ("msdos-memput", Fdos_memput, Sdos_memput, 2, 2, 0,
 
   for (i = 0; i < len; i++)
     {
-      CHECK_FIXNAT (AREF (vector, i));
-      buf[i] = (unsigned char) XFIXNAT (AREF (vector, i)) & 0xFF;
+      CHECK_FIXNUM (AREF (vector, i));
+      buf[i] = (unsigned char) XFIXNUM (AREF (vector, i)) & 0xFF;
     }
 
   dosmemput (buf, len, offs);
diff --git a/src/lisp.h b/src/lisp.h
index 077d236065..a0619e64f2 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1195,7 +1195,7 @@ XFIXNUM_RAW (Lisp_Object a)
 INLINE EMACS_INT
 XFIXNUM (Lisp_Object a)
 {
-  eassume (FIXNUMP (a));
+  eassert (FIXNUMP (a));
   return XFIXNUM_RAW (a);
 }
 
@@ -1209,7 +1209,7 @@ XUFIXNUM_RAW (Lisp_Object a)
 INLINE EMACS_UINT
 XUFIXNUM (Lisp_Object a)
 {
-  eassume (FIXNUMP (a));
+  eassert (FIXNUMP (a));
   return XUFIXNUM_RAW (a);
 }
 
@@ -2828,9 +2828,11 @@ FIXNATP (Lisp_Object x)
 INLINE EMACS_INT
 XFIXNAT (Lisp_Object a)
 {
-  eassume (FIXNATP (a));
+  eassert (FIXNUMP (a));
   EMACS_INT int0 = Lisp_Int0;
-  return USE_LSB_TAG ? XFIXNUM (a) : XLI (a) - (int0 << VALBITS);
+  EMACS_INT result = USE_LSB_TAG ? XFIXNUM (a) : XLI (a) - (int0 << VALBITS);
+  eassume (0 <= result);
+  return result;
 }
 
 INLINE bool
-- 
2.21.0

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Thu, Jun 27, 2019 at 8:28 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
> Pip Cet wrote:
> > I'd prefer if the code were written as though FIXNATs weren't FIXNUMs at all.
>
> I'm more of the opposite opinion. The original reason for XFIXNAT (under its old
> name) was performance, because long ago Emacs put the type tag in the most
> significant bits, and it was faster to mask it out than to smear the sign bit
> when you knew the fixnum was nonnegative. Nowadays that original reason is
> obsolete because the tags are in the least significant bits and XFIXNAT is just
> an alias for XFIXNUM, and if it were up to me we'd get rid of XFIXNAT entirely,
> and just use XFIXNUM. But old habits die hard....

I actually think that would be best, so we're only disagreeing about
what the second-best solution is :-)

> >        ascent = image_spec_value (spec, QCascent, NULL);
> >        if (FIXNUMP (ascent))
> >          img->ascent = XFIXNAT (ascent);
> >
> > in image.c is safe, because lookup_image is called only after valid_image_p.
>
> Sorry, you've lost me. How does valid_image_p guarantee that 'ascent' (if a
> fixnum) is in the range 0..INT_MAX? Because if it's not in that range, the above
> code could trap.

valid_image_p only returns true if ->valid_p returns true, which only
happens when parse_image_spec returns true, which only happens if
:ascent is not present, Qcenter, or a fixnum in the 0..100 range.

> > I'd prefer leaving the macros for now
>
> I actually did it that way at first, but that failed miserably as the macros
> evaluated their arguments more than once when debugging , and lots of code
> expects them to behave like functions and evaluate their arguments exactly once.
> So functions it is.

>
> > eassume (inline_function (x))
> > doesn't work very well on current GCC, while eassume (MACRO (x)) is
> > fine, so I'm sometimes building with DEFINE_KEY_OPS_AS_MACROS.
>
> Hmm, I see I should have helped GCC more there. First, the two new eassumes
> should just be easserts as the assumptions are not helpful to the compiler.
> Second, I should restore the eassume that tells GCC that XFIXNAT returns a
> nonnegative integer.
>
> > (The
> > eassume/eassert situation is something else I've been planning to look
> > at in more detail, though debugging it will be easier when
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91005 is fixed).
>
> Sorry, I don't follow. That bug report seems to be about nested C functions; why
> is it relevant to Emacs, which doesn't use nested C functions?

I don't think it's really relevant to this discussion, though there
are similarities:

----

Just like having both XFIXNUM and XFIXNAT is confusing, so is having
eassume and eassert. My idea is to define eassume as follows:

#define eassume(cond) (__builtin_constant_p (!(cond) == !(cond)) ?
((cond) ? 0 : __builtin_unreachable ()) : 0)

That would catch most (not all) cases in which eassume() causes code
to be emitted.  I wanted to test this, and in particular to catch
false negatives like this eassume in lisp.h:

  eassume (0 <= i && i < bool_vector_size (a));

(GCC isn't smart enough to derive any information from the RHS of the
&&, so __builtin_constant_p (!(cond) == !(cond)) is false, but we only
actually use the LHS of the &&...)

So I resorted to this old trick to see whether code was being emitted:

int c;
asm(".pushsection .text.bad");
c = (cond);
asm(".popsection");

but that hack no longer works with -g3. Instead, we can define an
inner function with the relevant code, put it into a magic section,
then confirm that only the trivial function actually ended up in the
magic section.
----

> > I don't see how either ccl.c or image.c are buggy for out-of-range
> > integer arguments.
>
> For example, ccl.c in master has this:
>
>    if (FIXNUMP (AREF (status, i)))
>      {
>        i = XFIXNAT (AREF (status, 8));
>        if (ccl.ic < i && i < ccl.size)
>         ccl.ic = i;
>      }
>
> where i is a 32-bit int. If AREF (status, 8) is the 62-bit Lisp fixnum with
> value '2 * (EMACS_INT) INT_MAX + ccl.ic + 3', the assignment to i overflows,
> which can trap. Even if the assignment doesn't trap but merely wraps around, the
> next line won't do range checking correctly.

Ah. Right. Thanks.

> > When you want a position, you want a FIXNAT,
> > not a negative number, so CHECK_FIXNAT_COERCE_MARKER everywhere would
> > catch those cases where someone passes an obviously invalid argument.
>
> But why stop there?

Because it's as far as we get only replacing characters in our code,
not typing in new ones.

> 0 is an obviously invalid argument for buffers. :-)

True, but then we'd have to add extra characters to distinguish the
buffer and string cases.

> I think
> this is just an area where we disagree - I want a range check where you want a
> type check. It's safe either way and the range check is a tad faster (as the
> range check needs to be done anyway) and results in a crisper diagnostic when it
> fails.

Okay, let's agree to disagree on what the second-best solution is here.

> > I'd prefer not touching the dosfns.c code, for the simple reason that
> > if anyone still uses it, they might rely on the broken behavior.
>
> We can't leave it alone, as XFIXNAT is now checking that its result is
> nonnegative when debugging is enabled.

Do DOS machines have enough memory to run Emacs with debugging
enabled? :-) You're absolutely right, of course.

Thanks for the changes! I think this code is a bit hard to read:

      margin = image_spec_value (spec, QCmargin, NULL);
      if (RANGED_FIXNUMP (0, margin, INT_MAX))
        img->vmargin = img->hmargin = XFIXNAT (margin);
      else if (CONSP (margin))
        {
          img->hmargin = XFIXNAT (XCAR (margin));
          img->vmargin = XFIXNAT (XCDR (margin));
        }
because range is checked for the :margin 3 case but not the :margin
'(3 4) case; I'm perfectly okay with taking a list and checking it
twice.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 27 Jun 2019 13:17:46 +0000
> Cc: 36370@debbugs.gnu.org
> 
> Do DOS machines have enough memory to run Emacs with debugging
> enabled? :-)

Yes.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Paul Eggert]

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

>> if it were up to me we'd get rid of XFIXNAT entirely,
>> and just use XFIXNUM. But old habits die hard....
> 
> I actually think that would be best, so we're only disagreeing about
> what the second-best solution is :-)

Removing XFIXNAT would be outside the scope of this patch. However, if 
we're already fixing the code for some other reason and if the XFIXNATs 
are confusing that code, we might as well replace them with XFIXNUMs. 
The attached patch does that.

> valid_image_p only returns true if ->valid_p returns true, which only
> happens when parse_image_spec returns true, which only happens if
> :ascent is not present, Qcenter, or a fixnum in the 0..100 range.

Thanks for explaining. The attached patch removes the unnecessary range 
checks that I proposed.

> My idea is to define eassume as follows:
> 
> #define eassume(cond) (__builtin_constant_p (!(cond) == !(cond)) ?
> ((cond) ? 0 : __builtin_unreachable ()) : 0)

That would generate worse code in some cases, since after (say) "eassume 
(i >= 0); return i/2;" where i is a variable, GCC would not be able to 
optimize i/2 into i>>1 because GCC would not know that i is nonnegative. 
The main point of eassume (as opposed to eassert) is to enable 
optimizations like that.

>  think this code is a bit hard to read:

The attached patch changes that to use XFIXNUM instead of XFIXNAT, along 
the lines discussed above.

Thanks for the review. In addition to the already-mentioned patches, I 
installed the attached and am closing the bug report.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Omit-a-few-minor-unnecessary-range-checks.patch --]
[-- Type: text/x-patch; name="0001-Omit-a-few-minor-unnecessary-range-checks.patch", Size: 2976 bytes --]

From 7219f991a94015e06851d54b77acb4b1161749a5 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 27 Jun 2019 12:31:27 -0700
Subject: [PATCH] Omit a few minor unnecessary range checks
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Based on Pip Cet’s review (Bug#36370#19).
* src/fileio.c (Fdo_auto_save):
* src/image.c (lookup_image):
* src/textprop.c (Fnext_single_char_property_change):
Prefer XFIXNUM to XFIXNAT for clarity and consistency with
neighboring code, and to avoid unnecessary range checks.
* src/image.c (lookup_image): Omit unnecessary range checks.
---
 src/fileio.c   |  2 +-
 src/image.c    | 12 ++++++------
 src/textprop.c |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/fileio.c b/src/fileio.c
index 61e10dac47..ed1d2aedf3 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -5852,7 +5852,7 @@ DEFUN ("do-auto-save", Fdo_auto_save, Sdo_auto_save, 0, 2, "",
 		   spare the user annoying messages.  */
 		&& XFIXNUM (BVAR (b, save_length)) > 5000
 		&& (growth_factor * (BUF_Z (b) - BUF_BEG (b))
-		    < (growth_factor - 1) * XFIXNAT (BVAR (b, save_length)))
+		    < (growth_factor - 1) * XFIXNUM (BVAR (b, save_length)))
 		/* These messages are frequent and annoying for `*mail*'.  */
 		&& !NILP (BVAR (b, filename))
 		&& NILP (no_message))
diff --git a/src/image.c b/src/image.c
index bbf25b4d58..f3d6508f46 100644
--- a/src/image.c
+++ b/src/image.c
@@ -2385,18 +2385,18 @@ lookup_image (struct frame *f, Lisp_Object spec)
 #endif
 
 	  ascent = image_spec_value (spec, QCascent, NULL);
-	  if (RANGED_FIXNUMP (0, ascent, INT_MAX))
-	    img->ascent = XFIXNAT (ascent);
+	  if (FIXNUMP (ascent))
+	    img->ascent = XFIXNUM (ascent);
 	  else if (EQ (ascent, Qcenter))
 	    img->ascent = CENTERED_IMAGE_ASCENT;
 
 	  margin = image_spec_value (spec, QCmargin, NULL);
-	  if (RANGED_FIXNUMP (0, margin, INT_MAX))
-	    img->vmargin = img->hmargin = XFIXNAT (margin);
+	  if (FIXNUMP (margin))
+	    img->vmargin = img->hmargin = XFIXNUM (margin);
 	  else if (CONSP (margin))
 	    {
-	      img->hmargin = XFIXNAT (XCAR (margin));
-	      img->vmargin = XFIXNAT (XCDR (margin));
+	      img->hmargin = XFIXNUM (XCAR (margin));
+	      img->vmargin = XFIXNUM (XCDR (margin));
 	    }
 
 	  relief = image_spec_value (spec, QCrelief, NULL);
diff --git a/src/textprop.c b/src/textprop.c
index 3026ec7e99..9023f4efa0 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -799,10 +799,10 @@ DEFUN ("next-single-char-property-change", Fnext_single_char_property_change,
       else
 	CHECK_FIXNUM_COERCE_MARKER (limit);
 
-      if (XFIXNAT (position) >= XFIXNUM (limit))
+      if (XFIXNUM (position) >= XFIXNUM (limit))
 	{
 	  position = limit;
-	  if (XFIXNAT (position) > ZV)
+	  if (XFIXNUM (position) > ZV)
 	    XSETFASTINT (position, ZV);
 	}
       else
-- 
2.21.0

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Thu, Jun 27, 2019 at 7:38 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> >> if it were up to me we'd get rid of XFIXNAT entirely,
> >> and just use XFIXNUM. But old habits die hard....
> >
> > I actually think that would be best, so we're only disagreeing about
> > what the second-best solution is :-)
>
> Removing XFIXNAT would be outside the scope of this patch. However, if
> we're already fixing the code for some other reason and if the XFIXNATs
> are confusing that code, we might as well replace them with XFIXNUMs.
> The attached patch does that.

Cool!

> > My idea is to define eassume as follows:
> >
> > #define eassume(cond) (__builtin_constant_p (!(cond) == !(cond)) ?
> > ((cond) ? 0 : __builtin_unreachable ()) : 0)
>
> That would generate worse code in some cases, since after (say) "eassume
> (i >= 0); return i/2;" where i is a variable, GCC would not be able to
> optimize i/2 into i>>1 because GCC would not know that i is nonnegative.

I'm confused, and I'm not sure what you're saying.

int f (int i)
{
  eassume (i >= 0);
  printf("%d\n", i & 0x80000000);
  return 0;
}

The eassume tells GCC i is nonnegative, since (!(i >= 0) == !(i >= 0))
is indeed a constant. So the code generated is:

        xorl    %esi, %esi
        movl    $.LC0, %edi
        xorl    %eax, %eax
        call    printf

which is as it should be. For your example, it is:

        movl    %edi, %eax
        sarl    %eax
        ret

with the eassume and

        movl    %edi, %eax
        shrl    $31, %eax
        addl    %edi, %eax
        sarl    %eax
        ret

without.

> The main point of eassume (as opposed to eassert) is to enable
> optimizations like that.

Yes, indeed. I don't see how they are disabled by my proposed definition.

> Thanks for the review. In addition to the already-mentioned patches, I
> installed the attached and am closing the bug report.

Thanks!

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Paul Eggert]

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

On 6/27/19 12:56 PM, Pip Cet wrote:

> The eassume tells GCC i is nonnegative, since (!(i >= 0) == !(i >= 0))
> is indeed a constant.

Ah! Thanks, I didn't catch that subtle point. Would the attached patch 
to verify.h address that problem? This patch is for Gnulib, but would 
propagate into Emacs.

I tried this out with Emacs master and although it did change the 
machine code subtly I didn't have the patience to see whether the 
changes were likely to improve performance. The changes did grow the 
Emacs text segment from 2556193 to 2557657 bytes (a 0.06% growth), which 
is not a good sign. This was on Fedora 30 x86-64 with a default Emacs build.

I'll CC: this to bug-gnulib since it's a Gnulib issue. I have not 
installed this patch into Gnulib on savannah.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-verify-tweak-assume-performance.patch --]
[-- Type: text/x-patch; name="0001-verify-tweak-assume-performance.patch", Size: 1822 bytes --]

From 9a5a83937544e7c127026fcf32030f7dbaa5766c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 27 Jun 2019 14:01:53 -0700
Subject: [PATCH] =?UTF-8?q?verify:=20tweak=20=E2=80=98assume=E2=80=99=20pe?=
 =?UTF-8?q?rformance?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Suggested by Pip Cet (Bug#36370#30).
* lib/verify.h (assume): Use __builtin_constant_p to generate
better code in recent GCC.
---
 ChangeLog    | 7 +++++++
 lib/verify.h | 6 ++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5ae108e25..7059f4f2b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2019-06-27  Paul Eggert  <eggert@cs.ucla.edu>
+
+	verify: tweak ‘assume’ performance
+	Suggested by Pip Cet (Bug#36370#30).
+	* lib/verify.h (assume): Use __builtin_constant_p to generate
+	better code in recent GCC.
+
 2019-06-26  Paul Eggert  <eggert@cs.ucla.edu>
 
 	strverscmp: sync from glibc
diff --git a/lib/verify.h b/lib/verify.h
index f8e4eff02..9b015c693 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -263,9 +263,11 @@ template <int w>
    accordingly.  R should not have side-effects; it may or may not be
    evaluated.  Behavior is undefined if R is false.  */
 
-#if (__has_builtin (__builtin_unreachable) \
+#if ((__has_builtin (__builtin_constant_p) \
+      && __has_builtin (__builtin_unreachable)) \
      || 4 < __GNUC__ + (5 <= __GNUC_MINOR__))
-# define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
+# define assume(R) (!__builtin_constant_p (!(R) == !(R)) || (R) \
+                    ? (void) 0 : __builtin_unreachable ())
 #elif 1200 <= _MSC_VER
 # define assume(R) __assume (R)
 #elif ((defined GCC_LINT || defined lint) \
-- 
2.21.0

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Thu, Jun 27, 2019 at 9:13 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 6/27/19 12:56 PM, Pip Cet wrote:
> > The eassume tells GCC i is nonnegative, since (!(i >= 0) == !(i >= 0))
> > is indeed a constant.
>
> Ah! Thanks, I didn't catch that subtle point. Would the attached patch
> to verify.h address that problem? This patch is for Gnulib, but would
> propagate into Emacs.

Eventually, I think that would be a good idea. But right now, I would
like to figure out why GCC isn't generating code that's as good as it
should be.

> I tried this out with Emacs master and although it did change the
> machine code subtly I didn't have the patience to see whether the
> changes were likely to improve performance. The changes did grow the
> Emacs text segment from 2556193 to 2557657 bytes (a 0.06% growth), which
> is not a good sign. This was on Fedora 30 x86-64 with a default Emacs build.

So far, what I've found is that

  eassume (0 <= i && i < bool_vector_size (a));

doesn't work with my patch. Tweaking that to

  eassume (0 <= i);
  eassume (i < bool_vector_size (a));

appears to help a little, but it's a good idea to avoid generating a
call to a function that might be out-lined by the compiler, in any
case.

One thing I've neglected to look at in detail, because Emacs isn't
affected by it, is how this works with pure functions.

> I'll CC: this to bug-gnulib since it's a Gnulib issue. I have not
> installed this patch into Gnulib on savannah.

I'd be extremely interested in any comments people might have!

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Bruno Haible]

Hi Paul,

> I'll CC: this to bug-gnulib since it's a Gnulib issue. I have not 
> installed this patch into Gnulib on savannah.

Can you please show an example code on which the change makes a difference?

I used this test program

==================================== foo.c ====================================
#include <stdio.h>

//#define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
#define assume(R) (!__builtin_constant_p (!(R) == !(R)) || (R) ? (void) 0 : __builtin_unreachable ())

int f_generic (int i)
{
  printf("%d\n", i & 0x80000000);
  return 0;
}

int f_condition (int i)
{
  if (i >= 0)
    printf("%d\n", i & 0x80000000);
  return 0;
}

int f_assume (int i)
{
  assume (i >= 0);
  printf("%d\n", i & 0x80000000);
  return 0;
}
===============================================================================

$ gcc -O2 -m32 -S foo.c && fgrep -v .cfi foo.s

and the code is the same, regardless of which definition of 'assume' I enable.

Bruno

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Paul Eggert]

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

On 6/27/19 4:45 PM, Bruno Haible wrote:
> Can you please show an example code on which the change makes a difference?

Not easily, I'm afraid. I grabbed the latest Emacs master 
<https://git.savannah.gnu.org/git/emacs.git>, ran './configure; make 
CFLAGS=-O2', edited the emacs lib/verify.h, and ran 'make CFLAGS=-O2' 
again. The biggest percentage change in text size was in 
src/regex-emacs.o. I'll attach the two respective .s files, compressed.

[-- Attachment #2: regex-emacs-old.s.gz --]
[-- Type: application/gzip, Size: 41374 bytes --]

[-- Attachment #3: regex-emacs-new.s.gz --]
[-- Type: application/gzip, Size: 41750 bytes --]

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Thu, Jun 27, 2019 at 11:45 PM Bruno Haible <bruno@clisp.org> wrote:
> Can you please show an example code on which the change makes a difference?

int main(void)
{
  eassume (printf("hi\n"));
  return 0;
}

Or, more realistically:

extern int potentially_inlined_function(int i);

int main(void)
{
  ...
  eassume(potentially_inlined_function(i));
  return i >= 0;
}

With the old gnulib eassume, the programmer has to know whether
potentially_inlined_function is inlined (in which case the eassume
appears to be treated as a nop) or not (in which case a potentially
expensive external function call is generated). With the new eassume,
these cases are distinguished by the compiler.

This makes it safe to use function expressions in eassume, whether the
function is inlined or not.

(That GCC doesn't actually do very much with this information is a
separate issue).

This approach does fail for certain compound expressions passed as
arguments to eassume:

eassume(i >= 0 && i < complicated_function ());

will not "split" the && expression, so it'll behave differently from

eassume(i >= 0);
eassume(i < complicated_function ());

But even in those cases, this approach is better than the old approach
of actually evaluating complicated_function.

At first, I thought it would be better to have a __builtin_assume
expression at the GCC level, but even that would have to have "either
evaluate the entire condition expression, or evaluate none of it"
semantics. We'll just have to get used to splitting our eassumes, I
think.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Bruno Haible]

Pip Cet wrote:
> Or, more realistically:
> 
> extern int potentially_inlined_function(int i);
> 
> int main(void)
> {
>   ...
>   eassume(potentially_inlined_function(i));
>   return i >= 0;
> }

OK, I see...

> This makes it safe to use function expressions in eassume, whether the
> function is inlined or not.

By "safe" you mean that you want the function call to not be evaluated.

You are mentioning a limitation:

> eassume(i >= 0 && i < complicated_function ());
> 
> will not "split" the && expression, so it'll behave differently from
> 
> eassume(i >= 0);
> eassume(i < complicated_function ());

And I would mention a regression: When -flto is in use and the expression
invokes an external potentially-inlined function, the old 'assume' would
work fine, i.e. do optimizations across compilation-unit boundaries.
Whereas the new 'assume' does not.

Test case:
================================ foo.c =================================
#include <stdio.h>

#define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
//#define assume(R) (!__builtin_constant_p (!(R) == !(R)) || (R) ? (void) 0 : __builtin_unreachable ())

extern int complicated (int i);
extern int nonnegative (int i);

int f_generic (int i)
{
  printf("%d\n", i & 0x80000000);
  return 0;
}

int f_condition (int i)
{
  if (complicated (i) && i >= 0)
    printf("%d\n", i & 0x80000000);
  return 0;
}

int f_assume (int i)
{
  assume (complicated (i) && i >= 0);
  printf("%d\n", i & 0x80000000);
  return 0;
}
================================= bar.c ================================
int complicated (int i) { return (i & 7) == 3; }
int nonnegative (int i) { return i >= 0; }
========================================================================
$ gcc -O2 -m32 -flto foo.c bar.c -shared -o libfoo.so && objdump --disassemble libfoo.so

With the old 'assume':

000005f0 <f_assume>:
 5f0:   83 ec 10                sub    $0x10,%esp
 5f3:   6a 00                   push   $0x0
 5f5:   68 74 06 00 00          push   $0x674
 5fa:   6a 01                   push   $0x1
 5fc:   e8 fc ff ff ff          call   5fd <f_assume+0xd>
 601:   31 c0                   xor    %eax,%eax
 603:   83 c4 1c                add    $0x1c,%esp
 606:   c3                      ret    
 607:   89 f6                   mov    %esi,%esi
 609:   8d bc 27 00 00 00 00    lea    0x0(%edi,%eiz,1),%edi

With the new 'assume':

00000610 <f_generic>:
 610:   83 ec 10                sub    $0x10,%esp
 613:   8b 44 24 14             mov    0x14(%esp),%eax
 617:   25 00 00 00 80          and    $0x80000000,%eax
 61c:   50                      push   %eax
 61d:   68 48 06 00 00          push   $0x648
 622:   6a 01                   push   $0x1
 624:   e8 fc ff ff ff          call   625 <f_generic+0x15>
 629:   31 c0                   xor    %eax,%eax
 62b:   83 c4 1c                add    $0x1c,%esp
 62e:   c3                      ret    
 62f:   90                      nop

00000630 <f_assume>:
 630:   eb de                   jmp    610 <f_generic>

> But even in those cases, this approach is better than the old approach
> of actually evaluating complicated_function.

I disagree that it is better:
  1. The new 'assume' is worse when -flto is in use.
  2. You recommend to users to split assume(A && B) into assume(A); assume(B);
     which is unnatural.

> At first, I thought it would be better to have a __builtin_assume
> expression at the GCC level, but even that would have to have "either
> evaluate the entire condition expression, or evaluate none of it"
> semantics.

No. At GCC level, it could have a "make the maximum of inferences - across
all optimization phases -, but evaluate none of it" semantics.

Bruno

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Bruno Haible]

Oops, the test case that I meant to show is this one:

================================ foo.c =================================
#include <stdio.h>

#define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
//#define assume(R) (!__builtin_constant_p (!(R) == !(R)) || (R) ? (void) 0 : __builtin_unreachable ())

extern int complicated (int i);
extern int nonnegative (int i);

int f_generic (int i)
{
  printf("%d\n", i & 0x80000000);
  return 0;
}

int f_condition (int i)
{
  if (complicated (i) && nonnegative (i))
    printf("%d\n", i & 0x80000000);
  return 0;
}

int f_assume (int i)
{
  assume (complicated (i) && nonnegative (i));
  printf("%d\n", i & 0x80000000);
  return 0;
}
================================ bar.c =================================
int complicated (int i) { return (i & 7) == 3; }
int nonnegative (int i) { return i >= 0; }
========================================================================
The results are as shown: the optimization in f_assume is performed
with the old 'assume' definition, but not with the new one.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Fri, Jun 28, 2019 at 12:14 PM Bruno Haible <bruno@clisp.org> wrote:
> Pip Cet wrote:
> > This makes it safe to use function expressions in eassume, whether the
> > function is inlined or not.
>
> By "safe" you mean that you want the function call to not be evaluated.

Sorry, sloppy wording there. You're right.

> You are mentioning a limitation:
>
> > eassume(i >= 0 && i < complicated_function ());
> >
> > will not "split" the && expression, so it'll behave differently from
> >
> > eassume(i >= 0);
> > eassume(i < complicated_function ());
>
> And I would mention a regression: When -flto is in use and the expression
> invokes an external potentially-inlined function, the old 'assume' would
> work fine, i.e. do optimizations across compilation-unit boundaries.

Sorry, can't reproduce that here. I'm sure the changes I need to make
are obvious once I've found them, but can you let me know your gcc
version?

> > But even in those cases, this approach is better than the old approach
> > of actually evaluating complicated_function.
>
> I disagree that it is better:

Sorry to be pedantic, but do you disagree that it is better in these
cases, or in general? The latter is a question that I'm trying to find
the answer to, but in these specific cases, it clearly is better.

(Just in the interest of full disclosure, I described the idea in a
different context; I think it's a neat hack, and I'm trying to figure
out whether it has practical applications, but if it doesn't then I
won't feel there's continuing disagreement).

>   1. The new 'assume' is worse when -flto is in use.

Maybe. Even if it is, though, that's a GCC limitation which I consider
likely to be fixable; your estimation of that may vary, of course.

>   2. You recommend to users to split assume(A && B) into assume(A); assume(B);
>      which is unnatural.

I make that recommendation independently of which assume is in use.

In practice, combining a complicated expression with a simple one in
an eassume is almost always not what you want to do. It's way too easy
to do something like

eassume(ptr->field >= 0 && f(ptr));

when what you mean is

eassume(ptr->field >= 0);
eassume(f(ptr));

(As an unusual special case, consider:

{
  printf("%d\n", i & 0x80000000);
  assume(i >= 0 && complicated_function());
}

which would generate different code from

{
  printf("%d\n", i & 0x80000000);
  assume(i >= 0);
  assume(complicated_function());
})

Combining two simple expressions and not getting the right result
appears, at this point, to run into a GCC limitation, but I'm not sure
where.

> > At first, I thought it would be better to have a __builtin_assume
> > expression at the GCC level, but even that would have to have "either
> > evaluate the entire condition expression, or evaluate none of it"
> > semantics.
>
> No. At GCC level, it could have a "make the maximum of inferences - across
> all optimization phases -, but evaluate none of it" semantics.

There's no contradiction there: I'm saying that the programmer is
allowed to assume that the expression passed to assume either has been
evaluated, or hasn't been, with no in-between interpretations allowed
to the compiler. That means assume (A && B) isn't equivalent, in
general, to assume (A); assume (B); My suspicion is that the latter is
almost always what is intended.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Paul Eggert]

Pip Cet wrote:
> It's way too easy
> to do something like
> 
> eassume(ptr->field >= 0 && f(ptr));
> 
> when what you mean is
> 
> eassume(ptr->field >= 0);
> eassume(f(ptr));

These mean the same thing. Both tell the compiler that a certain condition (A && 
B) is known to be true, and that behavior is undefined if (A && B) is false. The 
fact that Gnulib+GCC implements them differently is a quality-of-implementation 
issue, not a semantics issue.

> I'm saying that the programmer is
> allowed to assume that the expression passed to assume either has been
> evaluated, or hasn't been, with no in-between interpretations allowed
> to the compiler.

I don't see why that assumption is valid. It's OK if GCC partially evaluates the 
expression. As a silly example, eassume (0 * dump_core () + getchar ()) is not 
required to call dump_core, even if the compiler generates a call to getchar.

Perhaps we should change the comments in verify.h to make this point clearer.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Bruno Haible]

Pip Cet wrote:
> Sorry, can't reproduce that here. I'm sure the changes I need to make
> are obvious once I've found them, but can you let me know your gcc
> version?

I reproduce this with GCC versions 5.4.0, 6.5.0, 7.4.0, 8.3.0, and 9.1.0.
1. Take the files foo.c and bar.c from
   <https://lists.gnu.org/archive/html/bug-gnulib/2019-06/msg00096.html>
2. Compile and disassemble:
    gcc -O2 -m32 -flto foo.c bar.c -shared -o libfoo.so && objdump --disassemble 
libfoo.so
   Observe that f_assume pushes an immediate $0 argument on the stack for the
   function call.
3. Enable the second 'assume' definition instead of the first one.
4. Compile and disassemble:
    gcc -O2 -m32 -flto foo.c bar.c -shared -o libfoo.so && objdump --disassemble 
libfoo.so
   Observe that f_assume is now an alias of f_generic, that pushes a
   computed value as argument on the stack for the function call (push %eax).

> Sorry to be pedantic, but do you disagree that it is better in these
> cases, or in general?

I disagree that it is better in general.

You're apparently setting out a high goal for the 'assume' macro:
(1) that the programmer may call it with an expression that involves
    function calls,
(2) that the generated code will never include these function calls,
    because the generated code with the 'assume' invocation should be
    optimized at least as well as the generated code without the
    'assume' invocation.

I'm adding certain quality criteria:
  - It is not "good" if a construct behaves unpredictably, that is,
    if it hard to document precisely how it will behave. (*)
  - It is not "good" if the behaviour with no LTO is different from
    the behaviour with LTO.

The implementation with the __builtin_constant, while attaining the
goals (1) and (2), does not satisfy the two quality criteria.

I believe the only way to attain the goals and the quality criteria
is, as you suggested, to ask the GCC people to add a __builtin_assume
built-in.

> >   1. The new 'assume' is worse when -flto is in use.
> 
> Maybe. Even if it is, though, that's a GCC limitation which I consider
> likely to be fixable

Yes, *maybe* the GCC people can change the semantics of __builtin_constant_p
so that it is effectively computed at link-time, rather than when a single
compilation unit gets compiled. Or maybe not. I don't know...

> It's way too easy to do something like
> 
> eassume(ptr->field >= 0 && f(ptr));
> 
> when what you mean is
> 
> eassume(ptr->field >= 0);
> eassume(f(ptr));

I argue that it's unnatural if the two don't behave exactly the same.
Like everyone expects that
  x = foo ? yes : no;
is equivalent to
  if (foo) x = yes; else x = no;
And everyone expects that
  if (A && B) { ... }
is equivalent to
  if (A) if (B) { ... }

Bruno

(*) My favourite example of this principle is tail-recursion elimination.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Fri, Jun 28, 2019 at 5:46 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> Pip Cet wrote:
> > It's way too easy
> > to do something like
> >
> > eassume(ptr->field >= 0 && f(ptr));
> >
> > when what you mean is
> >
> > eassume(ptr->field >= 0);
> > eassume(f(ptr));
>
> These mean the same thing.

I'm really convinced they don't. Can you humor me and explain why
they're equivalent?

I'm considering this test case:
====
int global;

extern int f(void);

#define eassume0(cond) ((cond) ? (void) 0 : __builtin_unreachable ())
#ifdef ASSUME_GNULIB
#define eassume eassume0
#else
#define eassume(cond) (__builtin_constant_p (!(cond) == !(cond)) ?
eassume0(cond) : (void) 0)
#endif

int main(void)
{
#ifdef TWO_ASSUMES
  eassume (global == 0);
  eassume (f ());
#else
  eassume (global == 0 && f ());
#endif

  global++;
}
====
with this external function:
====
extern int global;

int f(void)
{
  return ++global;
}
====

I believe, and that is what my patch is based on, that the compiler
should be free to "use" the first eassume and ignore the second one,
resulting in this machine code:

    movl    $1, global(%rip)
    xorl    %eax, %eax
    ret

No call to f, it just sets global.

Without -DTWO_ASSUMES, the compiler cannot split the assumption.

> Both tell the compiler that a certain condition (A &&
> B) is known to be true, and that behavior is undefined if (A && B) is false.

Again, no. The split eassumes tell the compiler that A, B, and A && B
would all evaluate to true or fail to evaluate. The single eassume()
only covers the last of those three cases.

> The
> fact that Gnulib+GCC implements them differently is a quality-of-implementation
> issue, not a semantics issue.

Are you really saying that the single-assume case is equivalent to the
single-instruction program?

> > I'm saying that the programmer is
> > allowed to assume that the expression passed to assume either has been
> > evaluated, or hasn't been, with no in-between interpretations allowed
> > to the compiler.
>
> I don't see why that assumption is valid. It's OK if GCC partially evaluates the
> expression. As a silly example, eassume (0 * dump_core () + getchar ()) is not
> required to call dump_core, even if the compiler generates a call to getchar.

That's because && implies a sequence point, and * doesn't.

> Perhaps we should change the comments in verify.h to make this point clearer.

I'm sorry to be selfish, but I'd really rather understand where I've
gone wrong, first.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Bruno Haible]

On Freitag, 28. Juni 2019 19:15:06 CEST Pip Cet wrote:
> On Fri, Jun 28, 2019 at 5:46 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> > Pip Cet wrote:
> > > It's way too easy
> > > to do something like
> > >
> > > eassume(ptr->field >= 0 && f(ptr));
> > >
> > > when what you mean is
> > >
> > > eassume(ptr->field >= 0);
> > > eassume(f(ptr));
> >
> > These mean the same thing.
> 
> I'm really convinced they don't. Can you humor me and explain why
> they're equivalent?
> 
> I'm considering this test case:
> ====
> int global;
> 
> extern int f(void);
> 
> #define eassume0(cond) ((cond) ? (void) 0 : __builtin_unreachable ())
> #ifdef ASSUME_GNULIB
> #define eassume eassume0
> #else
> #define eassume(cond) (__builtin_constant_p (!(cond) == !(cond)) ?
> eassume0(cond) : (void) 0)
> #endif
> 
> int main(void)
> {
> #ifdef TWO_ASSUMES
>   eassume (global == 0);
>   eassume (f ());
> #else
>   eassume (global == 0 && f ());
> #endif
> 
>   global++;
> }
> ====
> with this external function:
> ====
> extern int global;
> 
> int f(void)
> {
>   return ++global;
> }
> ====
> 
> I believe, and that is what my patch is based on, that the compiler
> should be free to "use" the first eassume and ignore the second one,
> resulting in this machine code:
> 
>     movl    $1, global(%rip)
>     xorl    %eax, %eax
>     ret

For reference: This test case produces:
  Options                        Result
  none                           increment
  -DASSUME_GNULIB                increment
  -DTWO_ASSUMES                  single-store
  -DASSUME_GNULIB -DTWO_ASSUMES  increment

Bruno

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Fri, Jun 28, 2019 at 7:11 PM Bruno Haible <bruno@clisp.org> wrote:
> Pip Cet wrote:
> > Sorry, can't reproduce that here. I'm sure the changes I need to make
> > are obvious once I've found them, but can you let me know your gcc
> > version?
>
> I reproduce this with GCC versions 5.4.0, 6.5.0, 7.4.0, 8.3.0, and 9.1.0.
> 1. Take the files foo.c and bar.c from
>    <https://lists.gnu.org/archive/html/bug-gnulib/2019-06/msg00096.html>
> 2. Compile and disassemble:
>     gcc -O2 -m32 -flto foo.c bar.c -shared -o libfoo.so && objdump --disassemble
> libfoo.so
>    Observe that f_assume pushes an immediate $0 argument on the stack for the
>    function call.
> 3. Enable the second 'assume' definition instead of the first one.
> 4. Compile and disassemble:
>     gcc -O2 -m32 -flto foo.c bar.c -shared -o libfoo.so && objdump --disassemble
> libfoo.so
>    Observe that f_assume is now an alias of f_generic, that pushes a
>    computed value as argument on the stack for the function call (push %eax).

Thanks. I have no idea what I was doing wrong, but I can confirm that
this case indeed demonstrates a GCC weakness when there is a non-split
eassume (A && B), where A and B are both assumable (in the sense that
__builtin_constant_p(!A == !A).

> > Sorry to be pedantic, but do you disagree that it is better in these
> > cases, or in general?
>
> I disagree that it is better in general.

> You're apparently setting out a high goal for the 'assume' macro:
> (1) that the programmer may call it with an expression that involves
>     function calls,

That's the status quo in Emacs and many other projects that have
started believing the "an inline function is as fast as a macro"
mantra*, assuming you include inline functions with "function calls".

(* - as have I)

> (2) that the generated code will never include these function calls,
>     because the generated code with the 'assume' invocation should be
>     optimized at least as well as the generated code without the
>     'assume' invocation.

I think it should be the rarest of exceptions for an assume() to
result in slower code, yes. I believe that includes the case where
functions marked inline aren't inlined, because of compiler options,
for example.

> I'm adding certain quality criteria:
>   - It is not "good" if a construct behaves unpredictably, that is,
>     if it hard to document precisely how it will behave. (*)

Uhm, isn't the whole point of assume() to make the program behavior
undefined in a certain case?

int i = 1;
assume(i == 0);

will behave unpredictably. That's why we use the assume(), to speed up
execution by providing the compiler with an expression which "may or
may not be evaluated", and allowing it to do just anything if the
expression evaluates to false. That's the documented API.

If you want your program to behave predictably, in the strict sense,
you cannot ever use the current assume() API. If you want it to behave
predictably in some looser sense, the sense I suggest is that
assume(C) may or may not evaluate C. Again, that's what the
documentation says.

>   - It is not "good" if the behaviour with no LTO is different from
>     the behaviour with LTO.

I agree I should investigate the LTO example further, but please let's
keep in mind that I observed problematic behavior of assume(A && B) in
other contexts, so I'm not convinced LTO is the decisive factor here.

However, optimization can and will influence program behavior in many
ways. One of these ways is whether the argument to assume, which the
programmer knows "may or may not be evaluated", is evaluated or not.

> The implementation with the __builtin_constant, while attaining the
> goals (1) and (2), does not satisfy the two quality criteria.

For the record, my quality criteria are:

(1) implement the documented API, and don't change it
(2) when optimizing for speed, do not produce slower code with
eassume() than we would without it. Even when the programmer wrongly
guessed that a function would be inlined.
(3) when optimizing for size, do not produce larger code with
eassume() than we would without it. Even when inline functions are not
inlined.
(4) be at least as fast as gnulib assume()

> I believe the only way to attain the goals and the quality criteria
> is, as you suggested, to ask the GCC people to add a __builtin_assume
> built-in.

I think there's a significant probability that the GCC people would
agree to add such a built-in, but insist on its having "may or may not
evaluate its argument" semantics.

> > >   1. The new 'assume' is worse when -flto is in use.
> >
> > Maybe. Even if it is, though, that's a GCC limitation which I consider
> > likely to be fixable
>
> Yes, *maybe* the GCC people can change the semantics of __builtin_constant_p
> so that it is effectively computed at link-time, rather than when a single
> compilation unit gets compiled. Or maybe not. I don't know...

Hmm. My observations suggest that __builtin_constant_p is effectively
computed at link-time; all I have to do is to split the assume() in
your example.

> > It's way too easy to do something like
> >
> > eassume(ptr->field >= 0 && f(ptr));
> >
> > when what you mean is
> >
> > eassume(ptr->field >= 0);
> > eassume(f(ptr));
>
> I argue that it's unnatural if the two don't behave exactly the same.

But you're arguing for a __builtin_assume which doesn't evaluate its
argument, still? Because

{
  i++;
  __builtin_assume(i == 0);
  __builtin_assume(infloop());
  i++;
}

could validly be optimized to i = 1, while

{
  i++;
  __builtin_assume(i == 0 && infloop());
  i++;
}

could not be.

Or did I misunderstand the semantics you suggested for __builtin_assume?

> Like everyone expects that
>   x = foo ? yes : no;
> is equivalent to
>   if (foo) x = yes; else x = no;

It is.

> And everyone expects that
>   if (A && B) { ... }
> is equivalent to
>   if (A) if (B) { ... }

It is.

Sorry if I'm being a bit dense here.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

>   Options                        Result
>   none                           increment
>   -DASSUME_GNULIB                increment
>   -DTWO_ASSUMES                  single-store
>   -DASSUME_GNULIB -DTWO_ASSUMES  increment

Thanks, I should have included that. However, please note that in the
-DASSUME_GNULIB cases, global will have a final value of 2, as it's
incremented by f as well as by main; in the "none" case, it will have
a final value of 1.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Bruno Haible]

Pip Cet wrote:
> have started believing the "an inline function is as fast as a macro"
> mantra*, assuming you include inline functions with "function calls".

Ah, that's where the entire topic with the function calls inside assume()
comes from! I agree it's an important case (more important than the
functions defined in other compilation units). So let's test this:

==================================== foo.c ====================================
#include <stdio.h>

#define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
//#define assume(R) (!__builtin_constant_p (!(R) == !(R)) || (R) ? (void) 0 : __builtin_unreachable ())

#if USE_MACROS
# define complicated(i) (((i) & 7) == 3)
# define nonnegative(i) ((i) >= 0)
#else
static inline int complicated (int i) { return (i & 7) == 3; }
static inline int nonnegative (int i) { return i >= 0; }
#endif

#if COMPLEX_CONDITION
# define CONDITION complicated (i) && nonnegative (i)
#else
# define CONDITION nonnegative (i)
#endif

int f_generic (int i)
{
  printf("%d\n", i & 0x80000000);
  return 0;
}

int f_condition (int i)
{
  if (CONDITION)
    printf("%d\n", i & 0x80000000);
  return 0;
}

int f_assume (int i)
{
  assume (CONDITION);
  printf("%d\n", i & 0x80000000);
  return 0;
}
===============================================================================
$ gcc -O2 -m32 -S foo.c && fgrep -v .cfi foo.s

Results:
// old 'assume', !COMPLEX_CONDITION, USE_MACROS  -> f_assume optimized
// old 'assume', COMPLEX_CONDITION,  USE_MACROS  -> f_assume optimized
// old 'assume', !COMPLEX_CONDITION, !USE_MACROS -> f_assume optimized
// old 'assume', COMPLEX_CONDITION,  !USE_MACROS -> f_assume optimized
// new 'assume', !COMPLEX_CONDITION, USE_MACROS  -> f_assume optimized
// new 'assume', COMPLEX_CONDITION,  USE_MACROS  -> f_assume optimized
// new 'assume', !COMPLEX_CONDITION, !USE_MACROS -> f_assume not optimized
// new 'assume', COMPLEX_CONDITION,  !USE_MACROS -> f_assume not optimized

So, the main effect of the proposed new 'assume' is that it de-optimizes
the case where the CONDITION is defined using inline functions!

The other case - that the CONDITION calls functions defined in other
compilation units - is a fringe case. And the topic regarding the
COMPLEX_CONDITION versus simple condition is also less important.

Based on these results, I formally object against the proposed patch.

> > (2) that the generated code will never include these function calls,
> >     because the generated code with the 'assume' invocation should be
> >     optimized at least as well as the generated code without the
> >     'assume' invocation.
> 
> I think it should be the rarest of exceptions for an assume() to
> result in slower code, yes. I believe that includes the case where
> functions marked inline aren't inlined, because of compiler options,
> for example.

Then, I think we should change the documentation of 'assume' to say
that when it invokes functions, these functions should be marked
'__attribute__ ((__always_inline__))', otherwise performance will
be worse than without the 'assume', not better.

> (1) implement the documented API, and don't change it
> (2) when optimizing for speed, do not produce slower code with
> eassume() than we would without it. Even when the programmer wrongly
> guessed that a function would be inlined.
> (3) when optimizing for size, do not produce larger code with
> eassume() than we would without it. Even when inline functions are not
> inlined.
> (4) be at least as fast as gnulib assume()

You evidently have slightly different quality criteria than I do. :)

> > I believe the only way to attain the goals and the quality criteria
> > is, as you suggested, to ask the GCC people to add a __builtin_assume
> > built-in.
> 
> I think there's a significant probability that the GCC people would
> agree to add such a built-in, but insist on its having "may or may not
> evaluate its argument" semantics.

We can tell them that it would be important for us that is does not
evaluate its argument. Like sizeof (EXPRESSION) does not evaluate EXPRESSION.

> Sorry if I'm being a bit dense here.

No problem. I'm also often being dense.

Bruno

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Paul Eggert]

Bruno Haible wrote:
> I think we should change the documentation of 'assume' to say
> that when it invokes functions, these functions should be marked
> '__attribute__ ((__always_inline__))', otherwise performance will
> be worse than without the 'assume', not better.

I suggest something simpler and a little more general. The Emacs documentation 
for 'eassume' says this:

    This can improve performance in some cases,
    though it can degrade performance in others.  It's often suboptimal
    for COND to call external functions or access volatile storage.

and we could migrate that into the documentation for 'assume'.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Paul Eggert]

Pip Cet wrote:
>    eassume (global == 0);
>    eassume (f ());
> #else
>    eassume (global == 0 && f ());
> ...
> extern int global;
> 
> int f(void)
> {
>    return ++global;
> }


This is not a valid use of 'assume'. It's documented that assume's argument 
should be free of side effects.

It would be nice if 'assume (R)' reported an error if R has side effects, and 
generated a warning unless the compiler can verify that R is free of side 
effects. However, these niceties would require better support from the compiler.

> If you want your program to behave predictably, in the strict sense,
> you cannot ever use the current assume() API.

I'm not sure what you mean by "in the strict sense". It's true that programs can 
misuse 'assume' and get undefined behavior, but that's kind of the point....

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Fri, Jun 28, 2019 at 11:30 PM Bruno Haible <bruno@clisp.org> wrote:
> Pip Cet wrote:
> > have started believing the "an inline function is as fast as a macro"
> > mantra*, assuming you include inline functions with "function calls".
>
> Ah, that's where the entire topic with the function calls inside assume()
> comes from! I agree it's an important case (more important than the
> functions defined in other compilation units).

As I said earlier:

----
This makes it safe to use function expressions in eassume, whether the
function is inlined or not.

(That GCC doesn't actually do very much with this information is a
separate issue).
----

So we're talking about this separate issue now? I ask not for
rhetorical points, but because I genuinely think it's interesting if
the objection is based on GCC limitations rather than fundamentally
unfixable reasons.

> So, the main effect of the proposed new 'assume' is that it de-optimizes
> the case where the CONDITION is defined using inline functions!

(I don't think it's the "main" effect).

That certainly is something that appears to be happening in some
situations. It's a GCC limitation, and let's be clear: as long as this
limitation isn't lifted, an inline function is not as fast as a macro.

However, passing an inline function to assume() is problematic anyway,
unless it's marked as __attribute__((always_inline)), and marking it
as __attribute__((always_inline)) is problematic because it might
directly contradict what the programmer was trying to achieve by
passing -fno-inline.

> Based on these results, I formally object against the proposed patch.

I won't argue against that as long as I haven't found a way around the
GCC issue, but I would like to state that the current assume does
behave very badly when combined with -fno-inline-small-functions
-fno-inline.

> > > (2) that the generated code will never include these function calls,
> > >     because the generated code with the 'assume' invocation should be
> > >     optimized at least as well as the generated code without the
> > >     'assume' invocation.
> >
> > I think it should be the rarest of exceptions for an assume() to
> > result in slower code, yes. I believe that includes the case where
> > functions marked inline aren't inlined, because of compiler options,
> > for example.
>
> Then, I think we should change the documentation of 'assume' to say
> that when it invokes functions, these functions should be marked
> '__attribute__ ((__always_inline__))', otherwise performance will
> be worse than without the 'assume', not better.

I disagree. It's tedious, and people might just change their INLINE
macros (or whatever) to specify __attribute__((always_inline)), making
-fno-inline worthless...

> > I think there's a significant probability that the GCC people would
> > agree to add such a built-in, but insist on its having "may or may not
> > evaluate its argument" semantics.
>
> We can tell them that it would be important for us that is does not
> evaluate its argument. Like sizeof (EXPRESSION) does not evaluate EXPRESSION.

We can tell them that, but my suspicion is it'll be much, much harder
to implement that way.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Sat, Jun 29, 2019 at 5:41 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
> Pip Cet wrote:
> >    eassume (global == 0);
> >    eassume (f ());
> > #else
> >    eassume (global == 0 && f ());
> > ...
> > extern int global;
> >
> > int f(void)
> > {
> >    return ++global;
> > }
>
>
> This is not a valid use of 'assume'. It's documented that assume's argument
> should be free of side effects.

But the compiler makes no such assumption, so it cannot optimize
assume(i >= 0 && f()), (where i is a global or a non-const pointer to
i has potentially leaked) unless f happens to be available at
optimization time.

I think this is an interesting point: if GCC decided to add a
__builtin_assume() builtin, we could give it slightly different
semantics: that the expression passed to it evaluates to true, but
doesn't evaluate to false or fail to evaluate. Something like
__attribute__((does_return)) might do on a function.

However, if I'm not too confused, we're discussing whether
assume(SIMPLE_CONDITION && COMPLICATED_CONDITION) is ever a good idea.
With the old assume, it's harmful. With the new assume, it's
pointless. Is assume(SIMPLE_CONDITION); assume(COMPLICATED_CONDITION);
a good idea? With the old assume, it's harmful. With the new assume,
it's a more verbose way of simply assuming SIMPLE_CONDITION, so it
might be a good idea.

Also, "should" doesn't mean "must", does it? I'd prefer rewording that
sentence as "R may or may not be evaluated: it should not normally
have side-effects".

> It would be nice if 'assume (R)' reported an error if R has side effects, and
> generated a warning unless the compiler can verify that R is free of side
> effects. However, these niceties would require better support from the compiler.

But if we had that support from the compiler, wouldn't it be even
nicer to give up (most of) the distinction between assert and assume
and just tell people to use assume? That idea was my starting point,
and I'm still convinced it would result in better code overall. Except
someone would have to grep a little once in a while and replace most
eassume (A && B) expressions with eassume (A); eassume (B);

However, there are a few tricks we can use to verify this in special
debug builds.
------
Putting inner functions into sections:

#define assume(C) ({ auto void inner (void)
__attribute__((section("only_trivial_functions_go_here"), used)); void
inner(void) { (void)(C); } (void) 0; })

Then verify that the section contains only trivial function
definitions. (For the record, for Emacs, it does).

Another approach for detecting when (C) has "global" side effects
(such as calling an external function) is to do this:
-------
#include <stdio.h>

int global;

extern void dummy(void (*)(void)) __attribute__((warning ("assume
might have side effects")));

#define C printf("3")

int main(void)
{
  auto void inner(void) __attribute__((used));
  void inner(void)
  {
    if (global)
      __builtin_unreachable ();
    (void)(C);
    if (global)
      dummy(inner);
  }
}
-----

A third possibility is to use __builtin_constant_p(!(C)!=!(C)), as the
patch does. That doesn't state precisely that C has no side effects,
but it does come fairly close in practice ... except for the inline
function problem.

> > If you want your program to behave predictably, in the strict sense,
> > you cannot ever use the current assume() API.
>
> I'm not sure what you mean by "in the strict sense". It's true that programs can
> misuse 'assume' and get undefined behavior, but that's kind of the point....

Precisely what I meant.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Bruno Haible]

Pip Cet wrote:
> I would like to state that the current assume does
> behave very badly when combined with -fno-inline-small-functions
> -fno-inline.

Since we can't address this limitation through an acceptable change
to the 'assume' macro, we need to address it through documentation.

> marking it
> as __attribute__((always_inline)) is problematic because it might
> directly contradict what the programmer was trying to achieve by
> passing -fno-inline.

__attribute__((always_inline)) exists precisely to make a distinction
between functions where inlining is usually desirable vs. functions
where inlining is essential. We don't need to warn against the uses
of __attribute__((always_inline)) -- confusing behaviour in the debugger
etc. -- becauses these drawbacks are already well-known.

Paul Eggert wrote:
> I suggest something simpler and a little more general. The Emacs documentation 
> for 'eassume' says this:
>
>     This can improve performance in some cases,
>     though it can degrade performance in others.  It's often suboptimal
>     for COND to call external functions or access volatile storage.
>
> and we could migrate that into the documentation for 'assume'.

As a user of the 'assume' macro, I want a definitive statement about what
I need to provide so that the macro works in the sense of improved performance.
A statement about "often" is too vague.

How about this proposed patch?

diff --git a/lib/verify.h b/lib/verify.h
index f8e4eff..ed1ba19 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -261,7 +261,10 @@ template <int w>
 
 /* Assume that R always holds.  This lets the compiler optimize
    accordingly.  R should not have side-effects; it may or may not be
-   evaluated.  Behavior is undefined if R is false.  */
+   evaluated.  The behavior is undefined if R is false.
+   If you want the use of this macro to improve, not deteriorate,
+   performance, R should not contain function calls except to functions
+   that are declared 'inline __attribute__((__always_inline__))'.  */
 
 #if (__has_builtin (__builtin_unreachable) \
      || 4 < __GNUC__ + (5 <= __GNUC_MINOR__))

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Paul Eggert]

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

Bruno Haible wrote:

> +   If you want the use of this macro to improve, not deteriorate,
> +   performance, R should not contain function calls except to functions
> +   that are declared 'inline __attribute__((__always_inline__))'.  */

A reader of that might incorrectly conclude that using such functions will 
always improve performance, compared to using functions not declared that way. 
And I'm leery of putting such GCC-specific info into the documentation of a 
generic macro.

Also, given Pip Cet's misunderstanding it'd be helpful to add a word or two 
about the intent of 'assume (R)'.

How about the attached patch?

 > -   evaluated.  Behavior is undefined if R is false.  */
 > +   evaluated.  The behavior is undefined if R is false.

This is a nit, but to my ears the shorter version is better, and has a 
more-accurate connotation as "the behavior" connotes "the behavior of 'assume 
(R)'", whereas behavior is also undefined for code before or after a call to an 
incorrect 'assume (R)'.

[-- Attachment #2: verify.diff --]
[-- Type: text/x-patch, Size: 800 bytes --]

diff --git a/lib/verify.h b/lib/verify.h
index f8e4eff02..168a7afdb 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -259,9 +259,11 @@ template <int w>
 # define __has_builtin(x) 0
 #endif
 
-/* Assume that R always holds.  This lets the compiler optimize
-   accordingly.  R should not have side-effects; it may or may not be
-   evaluated.  Behavior is undefined if R is false.  */
+/* Assume that R always holds.  Behavior is undefined if R is false.
+   R should lack side effects, as it may be evaluated only partially.
+   Although 'assume (R)' is typically intended to help performance,
+   performance may degrade if R uses hard-to-optimize features
+   such as calls to non-inlined functions.  */
 
 #if (__has_builtin (__builtin_unreachable) \
      || 4 < __GNUC__ + (5 <= __GNUC_MINOR__))

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Paul Eggert]

Pip Cet wrote:

>> This is not a valid use of 'assume'. It's documented that assume's argument
>> should be free of side effects.
> 
> But the compiler makes no such assumption

Sure, but if the caller uses 'assume' contrary to its documentation, that's a 
problem with the caller's code, not with 'assume'. It's merely an implementation 
detail as to which pothole the problematic code runs into.
> if GCC decided to add a
> __builtin_assume() builtin, we could give it slightly different
> semantics: that the expression passed to it evaluates to true, but
> doesn't evaluate to false or fail to evaluate. Something like
> __attribute__((does_return)) might do on a function.

Yes, the expression should return true without side effects or looping. I don't 
see this as an significant difference in semantics. One should also not call 
Gnulib assume (R) with an expression that loops forever, as this defeats the 
intent of 'assume' which is to make code more efficient. If there's any real 
confusion about this issue, we can add it to the 'assume' documentation as well.

> Also, "should" doesn't mean "must", does it?

It's not the "should" of an Internet RFC. It's more the "should" of "you should 
do this, and if you don't you're on your own".

> I'd prefer rewording that
> sentence as "R may or may not be evaluated: it should not normally
> have side-effects".

Better to say that it should not have side effects at all. There's no "normally" 
about that. Side effects are trouble.

> wouldn't it be even
> nicer to give up (most of) the distinction between assert and assume
> and just tell people to use assume?

No, because 'assert (false)' has well-defined behavior, whereas behavior is 
undefined for programs that do 'assume (false)' . This is a fundamental 
difference between the two.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Bruno Haible]

Hi Paul,

> > +   If you want the use of this macro to improve, not deteriorate,
> > +   performance, R should not contain function calls except to functions
> > +   that are declared 'inline __attribute__((__always_inline__))'.  */
> 
> A reader of that might incorrectly conclude that using such functions will 
> always improve performance, compared to using functions not declared that way.

For functions not declared 'inline __attribute__((__always_inline__))', it
depends on the inlining heuristics of the compiler whether the 'assume (R)'
will be a performance improvement or the opposite.

> Also, given Pip Cet's misunderstanding it'd be helpful to add a word or two 
> about the intent of 'assume (R)'.
> 
> How about the attached patch?

That's a good compromise.

>  > -   evaluated.  Behavior is undefined if R is false.  */
>  > +   evaluated.  The behavior is undefined if R is false.
> 
> This is a nit, but to my ears the shorter version is better

OK, fine.

Bruno

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Sat, Jun 29, 2019 at 5:31 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> >> This is not a valid use of 'assume'. It's documented that assume's argument
> >> should be free of side effects.
> >
> > But the compiler makes no such assumption
>
> Sure, but if the caller uses 'assume' contrary to its documentation, that's a
> problem with the caller's code, not with 'assume'. It's merely an implementation
> detail as to which pothole the problematic code runs into.

I agree, for what it's worth, that the new documentation is clear on
what is and isn't allowed with assume(). It's just that it's a
significant API change from the previous state, and I dislike the API
change.

Again, this isn't about using `assume' contrary to its documentation,
it's about using it in a way that the compiler cannot prove obeys the
documented API, and thus handles suboptimally.

> > if GCC decided to add a
> > __builtin_assume() builtin, we could give it slightly different
> > semantics: that the expression passed to it evaluates to true, but
> > doesn't evaluate to false or fail to evaluate. Something like
> > __attribute__((does_return)) might do on a function.
>
> Yes, the expression should return true without side effects or looping. I don't
> see this as an significant difference in semantics.

It's significant to GCC. It optimizes function calls that are known to
return differently from ordinary function calls.

> One should also not call
> Gnulib assume (R) with an expression that loops forever, as this defeats the
> intent of 'assume' which is to make code more efficient.

No disagreement there. However, that doesn't mean the compiler is free
to assume that assume()'s argument doesn't loop; it needs to prove
that, whereas with the new __builtin_assume() builtin, it wouldn't
have to go to that trouble.

> If there's any real
> confusion about this issue, we can add it to the 'assume' documentation as well.

I think the new documentation is fine, in this regard.

> > Also, "should" doesn't mean "must", does it?
>
> It's not the "should" of an Internet RFC. It's more the "should" of "you should
> do this, and if you don't you're on your own".

So if I add a debug print statement to an inline function that happens
to be called in an eassume, I'm on my own? That's a significant API
change.

> > I'd prefer rewording that
> > sentence as "R may or may not be evaluated: it should not normally
> > have side-effects".
>
> Better to say that it should not have side effects at all. There's no "normally"
> about that. Side effects are trouble.

Except for debugging statements, other assert()s, abort()s, other
assume()s, divisions by zero that happen in the eassume() code rather
than after it...

Again, I think there are two issues here: you want to change the API
to be much more restrictive (my original patch intended to make it
more lenient), and you want to document the new API. Obviously, the
second point is fine.

If the new requirement is that R has no side effects, we don't need
the "R may or may not be evaluated". In fact, it might confuse others
in the same way I was confused.

I read the old documentation as:

As far as side effects go, each assume(R) may decide, independently
and unpredictably, whether side effects of R are visible or not: R may
or may not be evaluated. This unpredictable behavior can be confusing;
to avoid confusion, R RFC-SHOULD not have side effects.

the new documentation, I read as:

If R has any side effects, assume(R) is undefined. It can cause
compilation, linking, or run-time errors.

> > wouldn't it be even
> > nicer to give up (most of) the distinction between assert and assume
> > and just tell people to use assume?
>
> No, because 'assert (false)' has well-defined behavior, whereas behavior is
> undefined for programs that do 'assume (false)' . This is a fundamental
> difference between the two.

Thus the "most of", yes.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Sat, Jun 29, 2019 at 10:31 AM Bruno Haible <bruno@clisp.org> wrote:
> Pip Cet wrote:
> > I would like to state that the current assume does
> > behave very badly when combined with -fno-inline-small-functions
> > -fno-inline.
>
> Since we can't address this limitation through an acceptable change
> to the 'assume' macro, we need to address it through documentation.

I agree. There's a limitation bad enough for me to consider it a bug
in current GCC, and even if that's lifted tomorrow, it wouldn't be
backported so it would be quite a while until I'd ask you to
reconsider the matter.

> > marking it
> > as __attribute__((always_inline)) is problematic because it might
> > directly contradict what the programmer was trying to achieve by
> > passing -fno-inline.
>
> __attribute__((always_inline)) exists precisely to make a distinction
> between functions where inlining is usually desirable vs. functions
> where inlining is essential.

Indeed. In this case, it's usually desirable but not essential for the
correctness of the program, IMHO. I understand you disagree.

> We don't need to warn against the uses
> of __attribute__((always_inline)) -- confusing behaviour in the debugger
> etc. -- becauses these drawbacks are already well-known.

I disagree completely. There's no warning in the GCC documentation for
the attribute.

> As a user of the 'assume' macro, I want a definitive statement about what
> I need to provide so that the macro works in the sense of improved performance.
> A statement about "often" is too vague.

I agree.


> How about this proposed patch?
>
> diff --git a/lib/verify.h b/lib/verify.h
> index f8e4eff..ed1ba19 100644
> --- a/lib/verify.h
> +++ b/lib/verify.h
> @@ -261,7 +261,10 @@ template <int w>
>
>  /* Assume that R always holds.  This lets the compiler optimize
>     accordingly.  R should not have side-effects; it may or may not be
> -   evaluated.  Behavior is undefined if R is false.  */
> +   evaluated.  The behavior is undefined if R is false.
> +   If you want the use of this macro to improve, not deteriorate,
> +   performance, R should not contain function calls except to functions
> +   that are declared 'inline __attribute__((__always_inline__))'.  */

My suggestion would be "Assume that R always holds.  This lets the
compiler optimize accordingly.  Behavior is undefined if R is false,
fails to evaluate, or has side effects.  Performance will suffer if R
contains function calls that are not inlined at compile time."

That would describe the API as I understand you and Paul think it
always has been; I think it describes a drastically stricter API than
what the old comment did. As a reminder, my starting point was that I
wanted to make the API more lenient. So, obviously, I disagree with
the API change but it is more important that there's consensus on what
the API actually is than it is to have a good API.

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Bruno Haible]

Pip Cet wrote:
> My suggestion would be "Assume that R always holds.  This lets the
> compiler optimize accordingly.  Behavior is undefined if R is false,
> fails to evaluate, or has side effects.  Performance will suffer if R
> contains function calls that are not inlined at compile time."

I like this wording.

Bruno

bug#36370: 27.0.50; XFIXNAT called on negative numbers

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

  > I agree. There's a limitation bad enough for me to consider it a bug
  > in current GCC,

If you have encountered an apparent GCC bug, please report it.
The GCC developers need our help.

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

bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Paul Eggert]

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

Bruno Haible wrote:

> I like this wording.

OK, I merged that into the wording I proposed and installed the attached. One 
new point that I added is that I sometimes use 'assume (X)' to pacify GCC so 
that it does not issue a bogus warning in nearby code; this benefit of 'assume' 
is independent of the efficiency of the generated code.

[-- Attachment #2: 0001-verify-document-assume-better.txt --]
[-- Type: text/plain, Size: 1661 bytes --]

From c882996026931d9b11b0878a9bb032ca6b3941dd Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 2 Jul 2019 16:33:04 -0700
Subject: [PATCH] =?UTF-8?q?verify:=20document=20=E2=80=98assume=E2=80=99?=
 =?UTF-8?q?=20better?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lib/verify.h: Reword doc (Bug#36370).
---
 ChangeLog    | 5 +++++
 lib/verify.h | 8 +++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 77d8e9e6a..b89fd5d9e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2019-07-02  Paul Eggert  <eggert@cs.ucla.edu>
+
+	verify: document ‘assume’ better
+	* lib/verify.h: Reword doc (Bug#36370).
+
 2019-07-02  Bruno Haible  <bruno@clisp.org>
 
 	localcharset, nl_langinfo: Fix return value for UTF-8 locales on MSVC.
diff --git a/lib/verify.h b/lib/verify.h
index f8e4eff02..9b8e1ed20 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -259,9 +259,11 @@ template <int w>
 # define __has_builtin(x) 0
 #endif
 
-/* Assume that R always holds.  This lets the compiler optimize
-   accordingly.  R should not have side-effects; it may or may not be
-   evaluated.  Behavior is undefined if R is false.  */
+/* Assume that R always holds.  Behavior is undefined if R is false,
+   fails to evaluate, or has side effects.  Although assuming R can
+   help a compiler generate better code or diagnostics, performance
+   can suffer if R uses hard-to-optimize features such as function
+   calls not inlined by the compiler.  */
 
 #if (__has_builtin (__builtin_unreachable) \
      || 4 < __GNUC__ + (5 <= __GNUC_MINOR__))
-- 
2.17.1