[PATCH 1/2] Add conversions to and from struct timespec to module interface.

[PATCH 1/2] Add conversions to and from struct timespec to module interface.

[Philipp Stephani]

Time values are a fundamental data type, and such conversions are hard
to implement within modules because of the various forms of time
values in Emacs Lisp.  Adding dedicated conversion functions can
significantly simplify module code dealing with times.

This approach uses nanosecond precision.  While Emacs in theory has
support for higher-precision time values, in practice most languages
and standards, such as POSIX, C, Java, and Go, have settled on
nanosecond-precision integers to represent time.

* src/emacs-module.h.in: Add header for struct timespec.

* src/module-env-27.h: Add module functions for time conversion.

* src/emacs-module.c (module_extract_time, module_make_time): New
functions.
(initialize_environment): Use them.

* test/data/emacs-module/mod-test.c (Fmod_test_add_nanosecond): New
test function.
(emacs_module_init): Define it.

* test/src/emacs-module-tests.el (mod-test-add-nanosecond/valid)
(mod-test-add-nanosecond/invalid): New unit tests.

* doc/lispref/internals.texi (Module Values): Document time
conversion functions.
---
 doc/lispref/internals.texi        | 22 ++++++++++++++++++++++
 etc/NEWS                          |  3 +++
 src/emacs-module.c                | 20 ++++++++++++++++++++
 src/emacs-module.h.in             |  1 +
 src/module-env-27.h               |  6 ++++++
 test/data/emacs-module/mod-test.c | 13 +++++++++++++
 test/src/emacs-module-tests.el    | 25 +++++++++++++++++++++++++
 7 files changed, 90 insertions(+)

diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index 25892d4b57..c2969d2cd1 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -1387,6 +1387,15 @@ Module Values
 @var{arg}, as a C @code{double} value.
 @end deftypefn
 
+@deftypefn Function struct timespec extract_time (emacs_env *@var{env}, emacs_value @var{time})
+This function interprets @var{time} as an Emacs time value and returns
+the corresponding @code{struct timespec}.  @xref{Time of Day}.
+@var{time} may not be @code{nil}.  This function raises an error if
+@var{time} is out of range for @code{struct timespec}.  If @var{time}
+has higher precision than nanoseconds, then this function truncates it
+to nanosecond precision.
+@end deftypefn
+
 @deftypefn Function bool copy_string_contents (emacs_env *@var{env}, emacs_value @var{arg}, char *@var{buf}, ptrdiff_t *@var{len})
 This function stores the UTF-8 encoded text of a Lisp string specified
 by @var{arg} in the array of @code{char} pointed by @var{buf}, which
@@ -1452,6 +1461,19 @@ Module Values
 corresponding Emacs floating-point value.
 @end deftypefn
 
+@deftypefn Function emacs_value make_time (emacs_env *@var{env}, struct timespec @var{time})
+This function takes a @code{struct timespec} argument @var{time} and
+returns the corresponding Emacs timestamp.  @xref{Time of Day} for the
+possible return value formats.  It is not specified in which timestamp
+format the time is returned, but it is always a valid Emacs timestamp.
+The return value is exactly the same timestamp as @var{time}: all
+input values are representable, and there is never a loss of
+precision.  @code{time.tv_sec} and @code{time.tv_nsec} can be
+arbitrary values.  In particular, there's no requirement that @var{time}
+be normalized.  This means that @code{time.tv_nsec} doesn't have to be
+in the range [0, 999999999].
+@end deftypefn
+
 @deftypefn Function emacs_value make_string (emacs_env *@var{env}, const char *@var{str}, ptrdiff_t @var{strlen})
 This function creates an Emacs string from C text string pointed by
 @var{str} whose length in bytes, not including the terminating null
diff --git a/etc/NEWS b/etc/NEWS
index b13ab47768..2534262b62 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1910,6 +1910,9 @@ returns a regexp that never matches anything, which is an identity for
 this operation.  Previously, the empty string was returned in this
 case.
 
+** New module environment functions 'make_time' and 'extract_time' to
+convert between timespec structures and Emacs time values.
+
 \f
 * Changes in Emacs 27.1 on Non-Free Operating Systems
 
diff --git a/src/emacs-module.c b/src/emacs-module.c
index 20dcff2b67..fc5a912d85 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -74,6 +74,7 @@ To add a new module function, proceed as follows:
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <time.h>
 
 #include "lisp.h"
 #include "dynlib.h"
@@ -731,6 +732,22 @@ module_process_input (emacs_env *env)
   return emacs_process_input_continue;
 }
 
+static struct timespec
+module_extract_time (emacs_env *env, emacs_value value)
+{
+  MODULE_FUNCTION_BEGIN ((struct timespec) {0});
+  Lisp_Object time = value_to_lisp (value);
+  CHECK_TYPE (!NILP (time), Qtimep, time);
+  return lisp_time_argument (time);
+}
+
+static emacs_value
+module_make_time (emacs_env *env, struct timespec time)
+{
+  MODULE_FUNCTION_BEGIN (NULL);
+  return lisp_to_value (env, make_lisp_time (time));
+}
+
 \f
 /* Subroutines.  */
 
@@ -1134,6 +1151,8 @@ initialize_environment (emacs_env *env, struct emacs_env_private *priv)
   env->vec_size = module_vec_size;
   env->should_quit = module_should_quit;
   env->process_input = module_process_input;
+  env->extract_time = module_extract_time;
+  env->make_time = module_make_time;
   Vmodule_environments = Fcons (make_mint_ptr (env), Vmodule_environments);
   return env;
 }
@@ -1296,6 +1315,7 @@ syms_of_module (void)
         build_pure_c_string ("Invalid function arity"));
 
   DEFSYM (Qmodule_function_p, "module-function-p");
+  DEFSYM (Qtimep, "timep");
 
   defsubr (&Smodule_load);
 }
diff --git a/src/emacs-module.h.in b/src/emacs-module.h.in
index 009d1583fe..bfbe226dd9 100644
--- a/src/emacs-module.h.in
+++ b/src/emacs-module.h.in
@@ -22,6 +22,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 
 #include <stdint.h>
 #include <stddef.h>
+#include <time.h>
 
 #ifndef __cplusplus
 #include <stdbool.h>
diff --git a/src/module-env-27.h b/src/module-env-27.h
index b491b60fbb..e63843f8d6 100644
--- a/src/module-env-27.h
+++ b/src/module-env-27.h
@@ -2,3 +2,9 @@
      function should quit.  */
   enum emacs_process_input_result (*process_input) (emacs_env *env)
     EMACS_ATTRIBUTE_NONNULL (1);
+
+  struct timespec (*extract_time) (emacs_env *env, emacs_value value)
+    EMACS_ATTRIBUTE_NONNULL (1);
+
+  emacs_value (*make_time) (emacs_env *env, struct timespec time)
+    EMACS_ATTRIBUTE_NONNULL (1);
diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c
index a39e41afee..44a28fa18f 100644
--- a/test/data/emacs-module/mod-test.c
+++ b/test/data/emacs-module/mod-test.c
@@ -366,6 +366,18 @@ Fmod_test_sleep_until (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
   return env->intern (env, "finished");
 }
 
+static emacs_value
+Fmod_test_add_nanosecond (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
+                          void *data)
+{
+  assert (nargs == 1);
+  struct timespec time = env->extract_time (env, args[0]);
+  assert (time.tv_nsec >= 0);
+  assert (time.tv_nsec < 1000000000);
+  time.tv_nsec++;
+  return env->make_time (env, time);
+}
+
 /* Lisp utilities for easier readability (simple wrappers).  */
 
 /* Provide FEATURE to Emacs.  */
@@ -434,6 +446,7 @@ emacs_module_init (struct emacs_runtime *ert)
   DEFUN ("mod-test-invalid-finalizer", Fmod_test_invalid_finalizer, 0, 0,
          NULL, NULL);
   DEFUN ("mod-test-sleep-until", Fmod_test_sleep_until, 2, 2, NULL, NULL);
+  DEFUN ("mod-test-add-nanosecond", Fmod_test_add_nanosecond, 1, 1, NULL, NULL);
 
 #undef DEFUN
 
diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el
index 35aaaa64b6..6b986a96e2 100644
--- a/test/src/emacs-module-tests.el
+++ b/test/src/emacs-module-tests.el
@@ -310,4 +310,29 @@ module--test-assertion
                       'finished))
         (quit)))))
 
+(ert-deftest mod-test-add-nanosecond/valid ()
+  (dolist (input (list
+                  ;; Some realistic examples.
+                  (current-time) (time-to-seconds)
+                  (encode-time 12 34 5 6 7 2019 t)
+                  ;; Various legacy timestamp forms.
+                  '(123 456) '(123 456 789) '(123 456 789 6000)
+                  ;; Corner case: this will result in a nanosecond
+                  ;; value of 1000000000 after addition.  The module
+                  ;; code should handle this correctly.
+                  '(123 65535 999999 999000)
+                  ;; Seconds since the epoch.
+                  123 123.45
+                  ;; New (TICKS . HZ) format.
+                  '(123456789 . 1000000000)))
+    (ert-info ((format "input: %s" input))
+      (should (time-equal-p (mod-test-add-nanosecond input)
+                            (time-add input '(0 0 0 1000)))))))
+
+(ert-deftest mod-test-add-nanosecond/invalid ()
+  (dolist (input '(#x10000000000000000  ; out of range
+                   (123) (123.45 6 7) nil "foo" [1 2]))
+    (ert-info ((format "input: %s" input))
+      (should-error (mod-test-add-nanosecond input)))))
+
 ;;; emacs-module-tests.el ends here
-- 
2.20.1 (Apple Git-117)

[PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

* src/module-env-27.h: Add new module functions to convert big
integers.

* src/emacs-module.c (module_required_bytes)
(module_extract_big_integer, module_make_big_integer): New functions.
(initialize_environment): Use them.
(syms_of_module): Define needed symbols.

* test/data/emacs-module/mod-test.c (signal_memory_full): New helper
function.
(Fmod_test_double): New test function.
(emacs_module_init): Define it.

* test/src/emacs-module-tests.el (mod-test-double): New unit test.

* doc/lispref/internals.texi (Module Values): Document new functions.
---
 doc/lispref/internals.texi        |  26 +++++++
 etc/NEWS                          |   4 ++
 src/emacs-module.c                | 108 ++++++++++++++++++++++++++++++
 src/module-env-27.h               |   9 +++
 test/data/emacs-module/mod-test.c |  48 +++++++++++++
 test/src/emacs-module-tests.el    |   7 ++
 6 files changed, 202 insertions(+)

diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index c2969d2cd1..17f56907c5 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -1382,6 +1382,23 @@ Module Values
 @code{overflow-error}.
 @end deftypefn
 
+@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{size}, unsigned char *@var{magnitude})
+This function, which is available since Emacs 27, extracts the
+integral value of @var{arg}.  If @var{sign} is not @code{NULL}, it
+stores the sign of @var{arg} (-1, 0, or +1) into @code{*sign}.  The
+magnitude is stored into @var{magnitude} as follows.  If @var{size}
+and @var{magnitude} are bot non-@code{NULL}, then @var{magnitude} must
+point to an array of at least @code{*size} bytes.  If @var{magnitude}
+is large enough to hold the magnitude of @var{arg}, then this function
+writes the magnitude into the @var{magnitude} array in little-endian
+form, stores the number of bytes written into @code{*size}, and
+returns @code{true}.  If @var{magnitude} is not large enough, it
+stores the required size into @code{*size}, signals an error, and
+returns @code{false}.  If @var{size} is not @code{NULL} and
+@var{magnitude} is @code{NULL}, then the function stores the required
+size into @code{*size} and returns @code{true}.
+@end deftypefn
+
 @deftypefn Function double extract_float (emacs_env *@var{env}, emacs_value @var{arg})
 This function returns the value of a Lisp float specified by
 @var{arg}, as a C @code{double} value.
@@ -1456,6 +1473,15 @@ Module Values
 @code{most-positive-fixnum} (@pxref{Integer Basics}).
 @end deftypefn
 
+@deftypefn Function emacs_value make_big_integer (emacs_env *@var{env}, int sign, ptrdiff_t size, unsigned char *magnitude)
+This function, which is available since Emacs 27, takes an
+arbitrary-sized integer argument and returns a corresponding
+@code{emacs_value} object.  The @var{sign} argument gives the sign of
+the return value.  If @var{sign} is nonzero, then @var{magnitude} must
+point to an array of at least @var{size} bytes specifying the
+little-endian magnitude of the return value.
+@end deftypefn
+
 @deftypefn Function emacs_value make_float (emacs_env *@var{env}, double @var{d})
 This function takes a @code{double} argument @var{d} and returns the
 corresponding Emacs floating-point value.
diff --git a/etc/NEWS b/etc/NEWS
index 2534262b62..a0b764217f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1913,6 +1913,10 @@ case.
 ** New module environment functions 'make_time' and 'extract_time' to
 convert between timespec structures and Emacs time values.
 
+** New module environment functions 'make_big_integer' and
+'extract_big_integer' to create and extract arbitrary-size integer
+values.
+
 \f
 * Changes in Emacs 27.1 on Non-Free Operating Systems
 
diff --git a/src/emacs-module.c b/src/emacs-module.c
index fc5a912d85..bdc5370978 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -77,6 +77,7 @@ To add a new module function, proceed as follows:
 #include <time.h>
 
 #include "lisp.h"
+#include "bignum.h"
 #include "dynlib.h"
 #include "coding.h"
 #include "keyboard.h"
@@ -748,6 +749,108 @@ module_make_time (emacs_env *env, struct timespec time)
   return lisp_to_value (env, make_lisp_time (time));
 }
 
+/* Return the number of bytes needed to represent U.  */
+
+static ptrdiff_t
+module_required_bytes (EMACS_UINT u)
+{
+  if (u == 0)
+    return 0;
+  for (ptrdiff_t i = 0; i < sizeof u; ++i)
+    if (u <= 0xFFu << (8 * i))
+      return i + 1;
+  return sizeof u;
+}
+
+enum
+{
+  /* Constants for mpz_import and mpz_export.  */
+  module_bigint_order = -1,
+  module_bigint_size = 1,
+  module_bigint_endian = -1,
+  module_bigint_nails = 0,
+};
+
+static bool
+module_extract_big_integer (emacs_env *env, emacs_value value,
+                            int *sign, ptrdiff_t *size,
+                            unsigned char *magnitude)
+{
+  MODULE_FUNCTION_BEGIN (false);
+  Lisp_Object o = value_to_lisp (value);
+  CHECK_INTEGER (o);
+  if (size == NULL && magnitude != NULL)
+    wrong_type_argument (Qnull, Qmagnitude);
+  if (FIXNUMP (o))
+    {
+      EMACS_INT x = XFIXNUM (o);
+      if (sign != NULL)
+        *sign = (0 < x) - (x < 0);
+      if (size != NULL)
+        {
+          eassert (-EMACS_INT_MAX <= x);  /* Verify -x is defined. */
+          EMACS_UINT u = x < 0 ? -x : x;
+          verify (UCHAR_MAX == 0xFF);
+          verify (CHAR_BIT == 8);
+          ptrdiff_t required = module_required_bytes (u);
+          if (magnitude != NULL)
+            {
+              if (*size < required)
+                args_out_of_range_3 (make_int (*size), make_int (required),
+                                     make_int (PTRDIFF_MAX));
+              for (ptrdiff_t i = 0; i < required; ++i)
+                magnitude[i] = (u >> (8 * i)) & 0xFFu;
+            }
+          *size = required;
+        }
+    }
+  else
+    {
+      struct Lisp_Bignum *x = XBIGNUM (o);
+      if (sign != NULL)
+        *sign = mpz_sgn (x->value);
+      if (size != NULL)
+        {
+          /* See the remark at the end of the Info node
+             `(gmp) Integer Import and Export'.  */
+          ptrdiff_t required = (mpz_sizeinbase (x->value, 2) + 7) / 8;
+          if (magnitude != NULL)
+            {
+              if (*size < required)
+                args_out_of_range_3 (make_int (*size), make_int (required),
+                                     make_int (PTRDIFF_MAX));
+              verify (sizeof *magnitude == module_bigint_size);
+              size_t written;
+              mpz_export (magnitude, &written, module_bigint_order,
+                          module_bigint_size, module_bigint_endian,
+                          module_bigint_nails, x->value);
+              eassert (written == required);
+            }
+          *size = required;
+        }
+    }
+  return true;
+}
+
+static emacs_value
+module_make_big_integer (emacs_env *env, int sign, ptrdiff_t size,
+                         const unsigned char *magnitude)
+{
+  MODULE_FUNCTION_BEGIN (NULL);
+  if (sign != 0 && size == 0)
+    wrong_type_argument (Qnatnump, Qsize);
+  if (size != 0 && magnitude == NULL)
+    wrong_type_argument (Qarrayp, Qmagnitude);
+  if (sign == 0)
+    return lisp_to_value (env, make_fixed_natnum (0));
+  verify (sizeof *magnitude == module_bigint_size);
+  mpz_import (mpz[0], size, module_bigint_order, module_bigint_size,
+              module_bigint_endian, module_bigint_nails, magnitude);
+  if (sign < 0)
+    mpz_neg (mpz[0], mpz[0]);
+  return lisp_to_value (env, make_integer_mpz ());
+}
+
 \f
 /* Subroutines.  */
 
@@ -1153,6 +1256,8 @@ initialize_environment (emacs_env *env, struct emacs_env_private *priv)
   env->process_input = module_process_input;
   env->extract_time = module_extract_time;
   env->make_time = module_make_time;
+  env->extract_big_integer = module_extract_big_integer;
+  env->make_big_integer = module_make_big_integer;
   Vmodule_environments = Fcons (make_mint_ptr (env), Vmodule_environments);
   return env;
 }
@@ -1316,6 +1421,9 @@ syms_of_module (void)
 
   DEFSYM (Qmodule_function_p, "module-function-p");
   DEFSYM (Qtimep, "timep");
+  DEFSYM (Qnull, "null");
+
+  DEFSYM (Qmagnitude, "magnitude");
 
   defsubr (&Smodule_load);
 }
diff --git a/src/module-env-27.h b/src/module-env-27.h
index e63843f8d6..b4ecea2902 100644
--- a/src/module-env-27.h
+++ b/src/module-env-27.h
@@ -8,3 +8,12 @@
 
   emacs_value (*make_time) (emacs_env *env, struct timespec time)
     EMACS_ATTRIBUTE_NONNULL (1);
+
+  bool (*extract_big_integer) (emacs_env *env, emacs_value value,
+                               int *sign, ptrdiff_t *size,
+                               unsigned char *magnitude)
+    EMACS_ATTRIBUTE_NONNULL (1);
+
+  emacs_value (*make_big_integer) (emacs_env *env, int sign, ptrdiff_t size,
+                                   const unsigned char *magnitude)
+    EMACS_ATTRIBUTE_NONNULL (1);
diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c
index 44a28fa18f..b0c25717e8 100644
--- a/test/data/emacs-module/mod-test.c
+++ b/test/data/emacs-module/mod-test.c
@@ -329,6 +329,17 @@ signal_errno (emacs_env *env, const char *function)
   env->non_local_exit_signal (env, symbol, data);
 }
 
+static void
+signal_memory_full (emacs_env *env)
+{
+  emacs_value symbol = env->intern (env, "error");
+  const char *message = "Out of memory";
+  emacs_value message_value = env->make_string (env, message, strlen (message));
+  emacs_value data
+    = env->funcall (env, env->intern (env, "list"), 1, &message_value);
+  env->non_local_exit_signal (env, symbol, data);
+}
+
 /* A long-running operation that occasionally calls `should_quit' or
    `process_input'.  */
 
@@ -378,6 +389,42 @@ Fmod_test_add_nanosecond (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
   return env->make_time (env, time);
 }
 
+static emacs_value
+Fmod_test_double (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
+                  void *data)
+{
+  assert (nargs == 1);
+  emacs_value arg = args[0];
+  ptrdiff_t size;
+  if (!env->extract_big_integer (env, arg, NULL, &size, NULL))
+    return NULL;
+  unsigned char *magnitude = malloc (size + 1);
+  if (magnitude == NULL)
+    {
+      signal_memory_full (env);
+      return NULL;
+    }
+  int sign;
+  emacs_value result = NULL;
+  if (!env->extract_big_integer (env, arg, &sign, &size, magnitude))
+    goto out;
+  unsigned int carry = 0;
+  for (ptrdiff_t i = 0; i < size; ++i)
+    {
+      static_assert (UCHAR_MAX == 0xFF, "UCHAR_MAX != 0xFF");
+      static_assert (CHAR_BIT == 8, "CHAR_BIT != 8");
+      static_assert (UINT_MAX >= 2u * UCHAR_MAX, "unsigned int is too small");
+      unsigned int value = 2u * magnitude[i] + carry;
+      magnitude[i] = value & 0xFF;
+      carry = value >> 8;
+      assert (carry <= 1);
+    }
+  magnitude[size] = carry;
+  result = env->make_big_integer (env, sign, size + 1, magnitude);
+ out: free (magnitude);
+  return result;
+}
+
 /* Lisp utilities for easier readability (simple wrappers).  */
 
 /* Provide FEATURE to Emacs.  */
@@ -447,6 +494,7 @@ emacs_module_init (struct emacs_runtime *ert)
          NULL, NULL);
   DEFUN ("mod-test-sleep-until", Fmod_test_sleep_until, 2, 2, NULL, NULL);
   DEFUN ("mod-test-add-nanosecond", Fmod_test_add_nanosecond, 1, 1, NULL, NULL);
+  DEFUN ("mod-test-double", Fmod_test_double, 1, 1, NULL, NULL);
 
 #undef DEFUN
 
diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el
index 6b986a96e2..c160ae50b0 100644
--- a/test/src/emacs-module-tests.el
+++ b/test/src/emacs-module-tests.el
@@ -335,4 +335,11 @@ module--test-assertion
     (ert-info ((format "input: %s" input))
       (should-error (mod-test-add-nanosecond input)))))
 
+(ert-deftest mod-test-double ()
+  (dolist (input (list 0 1 2 -1 42 12345678901234567890
+                       most-positive-fixnum (1+ most-positive-fixnum)
+                       most-negative-fixnum (1- most-negative-fixnum)))
+    (ert-info ((format "input: %d" input))
+      (should (= (mod-test-double input) (* 2 input))))))
+
 ;;; emacs-module-tests.el ends here
-- 
2.20.1 (Apple Git-117)

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Tue, 23 Apr 2019 15:17:42 +0200
> Cc: Philipp Stephani <phst@google.com>
> 
> +@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{size}, unsigned char *@var{magnitude})
> +This function, which is available since Emacs 27, extracts the
> +integral value of @var{arg}.  If @var{sign} is not @code{NULL}, it
> +stores the sign of @var{arg} (-1, 0, or +1) into @code{*sign}.  The
> +magnitude is stored into @var{magnitude} as follows.  If @var{size}
> +and @var{magnitude} are bot non-@code{NULL}, then @var{magnitude} must
> +point to an array of at least @code{*size} bytes.  If @var{magnitude}
> +is large enough to hold the magnitude of @var{arg}, then this function
> +writes the magnitude into the @var{magnitude} array in little-endian
> +form, stores the number of bytes written into @code{*size}, and
> +returns @code{true}.  If @var{magnitude} is not large enough, it
> +stores the required size into @code{*size}, signals an error, and
> +returns @code{false}.  If @var{size} is not @code{NULL} and
> +@var{magnitude} is @code{NULL}, then the function stores the required
> +size into @code{*size} and returns @code{true}.
> +@end deftypefn

I think this text should tell more about how magnitude is put into the
MAGNITUDE array.  I needed to look at the implementation to find the
answer to that question.  Since the caller will have to make sure the
array is "large enough", I think the reader needs to know more about
this than this text reveals.

> +@deftypefn Function emacs_value make_big_integer (emacs_env *@var{env}, int sign, ptrdiff_t size, unsigned char *magnitude)
> +This function, which is available since Emacs 27, takes an
> +arbitrary-sized integer argument and returns a corresponding
> +@code{emacs_value} object.

Same here.  In particular, the text mentions "integer argument", but
it's unclear to what exactly that alludes, as there's no "argument" in
the function's signature as shown.

Thanks.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Paul Eggert]

On 4/23/19 6:17 AM, Philipp Stephani wrote:
> +@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{size}, unsigned char *@var{magnitude})y

This sounds reasonably inconvenient for authors of modules, plus it's a
pain to document. Why not just assume GMP instead? It's pretty unlikely
authors would use anything else for bignums. This should simplify the
code not only on the Emacs side but also on the module side; plus it
should improve performance by avoiding roundtrips through mpz_export and
mpz_import.

If a module author really wanted the array-of-unsigned-char
representation (which they probably wouldn't), you could supply a simple
conversion module to help them do that.

Similarly for make_big_integer.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

Am Di., 23. Apr. 2019 um 16:51 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 4/23/19 6:17 AM, Philipp Stephani wrote:
> > +@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{size}, unsigned char *@var{magnitude})y
>
> This sounds reasonably inconvenient for authors of modules, plus it's a
> pain to document. Why not just assume GMP instead? It's pretty unlikely
> authors would use anything else for bignums. This should simplify the
> code not only on the Emacs side but also on the module side; plus it
> should improve performance by avoiding roundtrips through mpz_export and
> mpz_import.

I considered using GMP. However, it has quite a few downsides:
- It would require making emacs-module.h dependent on GMP, even for
users that don't use big integers. Right now it only depends on
standard C, and I'd like to keep it that way.
- It's unclear how well GMP is supported in other languages. mpz_t is
a weird and unusual type, and other languages might not support it
well in their C interface.
- Other languages tend to have their own bigint support, so I don't
think the advantages of using GMP directly are that big.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Paul Eggert]

On 4/23/19 8:12 AM, Philipp Stephani wrote:
> I considered using GMP. However, it has quite a few downsides:
> - It would require making emacs-module.h dependent on GMP, even for
> users that don't use big integers. Right now it only depends on
> standard C, and I'd like to keep it that way.
> - It's unclear how well GMP is supported in other languages. mpz_t is
> a weird and unusual type, and other languages might not support it
> well in their C interface.
> - Other languages tend to have their own bigint support, so I don't
> think the advantages of using GMP directly are that big.

All true, though Emacs requires GMP anyway (one way or another) and it's
typically faster than the non-GMP approaches used in Python (and I
assume elsewhere).

Some of this depends on the importance of performance and convenience
when communicating between Emacs Lisp and GMP-using modules. If these
are unimportant then the current approach is OK. However, I'm thinking
that at least some users will view them as being important.

Could emacs-module.h expose to the user a GMP-style interface only if
the macro __GNU_MP_VERSION is defined? (Or we can choose our own macro
name if we don't want to require the user to include gmp.h before
emacs-module.h.) That way, users that don't use big integers won't need
to worry about GMP at all.

It might also be good (independently of having a GMP-style interface) to
expose just mp_limb_t to module code - we could give it another name and
put it into emacs-module.h as "typedef XXX bignum_limb_t;" where XXX is
either unsigned int, unsigned long int, or unsigned long long int, as
computed by 'configure'. This could avoid much of the overhead of
converting between GMP's representation and the unsigned char
representation.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

Am Di., 23. Apr. 2019 um 17:48 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 4/23/19 8:12 AM, Philipp Stephani wrote:
> > I considered using GMP. However, it has quite a few downsides:
> > - It would require making emacs-module.h dependent on GMP, even for
> > users that don't use big integers. Right now it only depends on
> > standard C, and I'd like to keep it that way.
> > - It's unclear how well GMP is supported in other languages. mpz_t is
> > a weird and unusual type, and other languages might not support it
> > well in their C interface.
> > - Other languages tend to have their own bigint support, so I don't
> > think the advantages of using GMP directly are that big.
>
> All true, though Emacs requires GMP anyway (one way or another) and it's
> typically faster than the non-GMP approaches used in Python (and I
> assume elsewhere).

Emacs does require GMP, but module authors might not.

>
> Some of this depends on the importance of performance and convenience
> when communicating between Emacs Lisp and GMP-using modules. If these
> are unimportant then the current approach is OK. However, I'm thinking
> that at least some users will view them as being important.

They are definitely not unimportant, though less important than
robustness and simplicity.
OTOH, the proposed approach isn't really simpler or more robust
either. It's just less dependent on third-party libraries.

>
> Could emacs-module.h expose to the user a GMP-style interface only if
> the macro __GNU_MP_VERSION is defined? (Or we can choose our own macro
> name if we don't want to require the user to include gmp.h before
> emacs-module.h.) That way, users that don't use big integers won't need
> to worry about GMP at all.

This is possible (and indeed required) since we couldn't require GMP
headers unconditionally.
My current approach would be to define a new macro EMACS_MODULE_GMP
and wrap the mpz_t type in a datatype like this:

#ifdef EMACS_MODULE_GMP
struct emacs_mpz { mpz_t value; };
#else
struct emacs_mpz;
#endif

We always need to expose the conversion function, that's required for
ABI compatibility. However, using the approach with a forward-declared
struct (that is never defined) would prevent users that don't have GMP
from using the API.

I'm generally fine with that approach as well.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

Am Di., 23. Apr. 2019 um 17:54 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Di., 23. Apr. 2019 um 17:48 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
> >
> > On 4/23/19 8:12 AM, Philipp Stephani wrote:
> > > I considered using GMP. However, it has quite a few downsides:
> > > - It would require making emacs-module.h dependent on GMP, even for
> > > users that don't use big integers. Right now it only depends on
> > > standard C, and I'd like to keep it that way.
> > > - It's unclear how well GMP is supported in other languages. mpz_t is
> > > a weird and unusual type, and other languages might not support it
> > > well in their C interface.
> > > - Other languages tend to have their own bigint support, so I don't
> > > think the advantages of using GMP directly are that big.
> >
> > All true, though Emacs requires GMP anyway (one way or another) and it's
> > typically faster than the non-GMP approaches used in Python (and I
> > assume elsewhere).
>
> Emacs does require GMP, but module authors might not.
>
> >
> > Some of this depends on the importance of performance and convenience
> > when communicating between Emacs Lisp and GMP-using modules. If these
> > are unimportant then the current approach is OK. However, I'm thinking
> > that at least some users will view them as being important.
>
> They are definitely not unimportant, though less important than
> robustness and simplicity.
> OTOH, the proposed approach isn't really simpler or more robust
> either. It's just less dependent on third-party libraries.

After gaining a bit of experience with the current implementation
(including writing bindings for Go), I'm now again convinced that we
should take the original approach (only relying on the C standard and
not introducing an optional dependency on GMP).

- It feels generally wrong and too heavyweight to introduce a
dependency on a third-party library just to represent what is
essentially a byte array plus a sign bit.

- The current implementation requires module authors to depend on
exactly the same GMP library as Emacs, because otherwise the
allocation functions aren't compatible. In particular, module authors
can't statically link GMP.

- Many programming languages such as Go, Java, or Python have big
integer support built into the language or the standard library. They
don't gain anything from using GMP in the module interface.

- The interface leaks an implementation detail of Emacs to the module
interface. Even if Emacs were to switch away from GMP at some point,
it would still have to support the GMP interface and link against GMP.

- Using conditional compilation means that the header isn't hermetic
any more. This can produce surprising results (compilation now depends
on whether some other header has already included emacs-module.h, with
potentially other preprocessor macros), and breaks things like
precompiled headers.

The primary advantages of using GMP in the interface are:

- C and C++ module authors that use GMP anyway benefit from a bit
easier interface. I don't think this argument is very strong, as
writing the necessary glue code using mpz_import and mpz_export isn't
too onerous and can easily be factored out.

- Not calling mpz_export/mpz_import is a bit faster. This seems like
premature optimization to me, but we can even avoid that overhead by
using an unsigned long array for the magnitude: then GMP will
special-case the export/import to a memcpy (in the typical case where
the GMP limb is itself an unsigned long).

Overall I now think the original approach (without mpz_t) is quite
superior, and unless I hear convincing counterarguments, I'll send a
patch to revert to that.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Stefan Monnier]

> Overall I now think the original approach (without mpz_t) is quite
> superior, and unless I hear convincing counterarguments, I'll send a
> patch to revert to that.

FWIW, I agree.  The main argument for me is the fact that it exposes an
internal implementation detail whereas the module interface makes a lot
of effort to hide all implementation details (even the representation
of Lisp_Object!).


        Stefan

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

Am So., 3. Nov. 2019 um 14:38 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> > Overall I now think the original approach (without mpz_t) is quite
> > superior, and unless I hear convincing counterarguments, I'll send a
> > patch to revert to that.
>
> FWIW, I agree.  The main argument for me is the fact that it exposes an
> internal implementation detail whereas the module interface makes a lot
> of effort to hide all implementation details (even the representation
> of Lisp_Object!).
>


SG, I'll send the patch once I find the time to do so ;-) Since this a
breaking change, I'd request not releasing any pretests until the
patch is in.

[PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

As described in the new comment added to emacs-module.c, using GMP
directly in the module interface has significant downsides: it couples
the module interface directly to the implementation and requires
module authors to link their module against the same GMP library as
Emacs itself, which is often difficult and an unnecessary burden.  By
picking a representation for the magnitude that often matches the one
used by GMP, we can avoid overhead when converting from and to GMP in
most cases.

Loading the test module in test/data/emacs-module and evaluating

(dotimes (_ 10000)
  (mod-test-double (* 2 most-negative-fixnum)))

under Callgrind shows that on my (GNU/Linux) machine Emacs only spends
10% of the CPU time of mod-test-double in mpz_import and mpz_export
combined, even though that function does little else.  (By contrast,
30% is spent in allocate_pseudovector.)

* src/emacs-module.h.in: Don't check EMACS_MODULE_GMP.  Don't include
gmp.h.  Remove emacs_mpz structure.

* src/module-env-27.h: Change interface of extract_big_integer and
make_big_integer to take a sign-magnitude representation instead of
mpz_t.

* src/emacs-module.c: Don't check EMACS_MODULE_GMP or
EMACS_MODULE_HAVE_MPZ_T.  Add a comment about the chosen
implementation.
(module_extract_big_integer, module_make_big_integer): Reimplement
without using mpz_t in the interface.

* doc/lispref/internals.texi (Module Values): Adapt function
documentation and example.  Stop mentioning GMP and EMACS_MODULE_GMP.

* test/data/emacs-module/mod-test.c: Don't define EMACS_MODULE_GMP or
EMACS_MODULE_HAVE_MPZ_T.
(memory_full, extract_big_integer, make_big_integer): New helper
functions, identical to example in the Info documentation.
(Fmod_test_nanoseconds, Fmod_test_double): Adapt to new interface.
---
 doc/lispref/internals.texi        | 192 ++++++++++++++++++++----------
 src/emacs-module.c                | 138 ++++++++++++++++++---
 src/emacs-module.h.in             |  10 --
 src/module-env-27.h               |  12 +-
 test/data/emacs-module/mod-test.c | 102 +++++++++++++---
 5 files changed, 350 insertions(+), 104 deletions(-)

diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index e870d6e06e..8e87c37ad4 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -1475,6 +1475,24 @@ Module Values
 @code{overflow-error}.
 @end deftypefn
 
+@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{count}, unsigned long *@var{magnitude})
+This function, which is available since Emacs 27, extracts the
+integral value of @var{arg}.  If @var{sign} is not @code{NULL}, it
+stores the sign of @var{arg} (-1, 0, or +1) into @code{*sign}.  The
+magnitude is stored into @var{magnitude} as follows.  If @var{count}
+and @var{magnitude} are bot non-@code{NULL}, then @var{magnitude} must
+point to an array of at least @code{*count} @code{unsigned long}
+elements.  If @var{magnitude} is large enough to hold the magnitude of
+@var{arg}, then this function writes the magnitude into the
+@var{magnitude} array in little-endian form, stores the number of
+array elements written into @code{*count}, and returns @code{true}.
+If @var{magnitude} is not large enough, it stores the required array
+size into @code{*count}, signals an error, and returns @code{false}.
+If @var{count} is not @code{NULL} and @var{magnitude} is @code{NULL},
+then the function stores the required array size into @code{*count}
+and returns @code{true}.
+@end deftypefn
+
 @deftypefn Function double extract_float (emacs_env *@var{env}, emacs_value @var{arg})
 This function returns the value of a Lisp float specified by
 @var{arg}, as a C @code{double} value.
@@ -1572,6 +1590,120 @@ Module Values
 @code{most-positive-fixnum} (@pxref{Integer Basics}).
 @end deftypefn
 
+@deftypefn Function emacs_value make_big_integer (emacs_env *@var{env}, int sign, ptrdiff_t count, const unsigned long *magnitude)
+This function, which is available since Emacs 27, takes an
+arbitrary-sized integer argument and returns a corresponding
+@code{emacs_value} object.  The @var{sign} argument gives the sign of
+the return value.  If @var{sign} is nonzero, then @var{magnitude} must
+point to an array of at least @var{count} elements specifying the
+little-endian magnitude of the return value.
+@end deftypefn
+
+The following example uses the GNU Multiprecision Library (GMP) to
+calculate the next probable prime after a given integer.
+@xref{Top,,,gmp} for a general overview of GMP, and @pxref{Integer
+Import and Export,,,gmp} for how to convert the @code{magnitude} array
+to and from GMP @code{mpz_t} values.
+
+@example
+#include <assert.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <gmp.h>
+
+#include <emacs-module.h>
+
+static void
+memory_full (emacs_env *env)
+@{
+  const char *message = "Memory exhausted";
+  emacs_value data = env->make_string (env, message, strlen (message));
+  env->non_local_exit_signal (env, env->intern (env, "error"),
+                              env->funcall (env, env->intern (env, "list"), 1,
+                                            &data));
+@}
+
+static bool
+extract_big_integer (emacs_env *env, emacs_value arg, mpz_t result)
+@{
+  int sign;
+  ptrdiff_t count;
+  bool success = env->extract_big_integer (env, value, &sign, &count, NULL);
+  if (!success)
+    return false;
+  if (sign == 0)
+    @{
+      mpz_set_ui (result, 0);
+      return true;
+    @}
+  enum @{ order = -1, size = sizeof (unsigned long), endian = 0, nails = 0 @};
+  assert (count > 0 && count <= SIZE_MAX);
+  unsigned long *magnitude = calloc (count, size);
+  if (magnitude == NULL)
+    @{
+      memory_full (env);
+      return false;
+    @}
+  success = env->extract_big_integer (env, arg, NULL, &count, magnitude);
+  assert (success);
+  mpz_import (value, count, order, size, endian, nails, magnitude);
+  free (magnitude);
+  if (sign < 0)
+    mpz_neg (result, result);
+  return true;
+@}
+
+static emacs_value
+make_big_integer (emacs_env *env, const mpz_t value)
+@{
+  if (mpz_sgn (value) == 0)
+    return env->make_integer (env, 0);
+  enum
+  @{
+    order = -1,
+    size = sizeof (unsigned long),
+    endian = 0,
+    nails = 0,
+    numb = 8 * size - nails
+  @};
+  size_t count = (mpz_sizeinbase (value, 2) + numb - 1) / numb;
+  if (count > PTRDIFF_MAX)
+    @{
+      memory_full (env);
+      return NULL;
+    @}
+  unsigned long *magnitude = calloc (count, size);
+  if (magnitude == NULL)
+    @{
+      memory_full (env);
+      return NULL;
+    @}
+  size_t written;
+  mpz_export (magnitude, &written, order, size, endian, nails, value);
+  assert (written == count);
+  emacs_value result = env->make_big_integer (env, mpz_sgn (value),
+                                              (ptrdiff_t) count, magnitude);
+  free (magnitude);
+  return result;
+@}
+
+static emacs_value
+next_prime (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
+            void *data)
+@{
+  assert (nargs == 1);
+  emacs_mpz p;
+  mpz_init (p);
+  extract_big_integer (env, args[0], p);
+  mpz_nextprime (p, p);
+  emacs_value result = make_big_integer (env, p);
+  mpz_clear (p);
+  return result;
+@}
+@end example
+
 @deftypefn Function emacs_value make_float (emacs_env *@var{env}, double @var{d})
 This function takes a @code{double} argument @var{d} and returns the
 corresponding Emacs floating-point value.
@@ -1601,66 +1733,6 @@ Module Values
 string.
 @end deftypefn
 
-If you define the preprocessor macro @code{EMACS_MODULE_GMP} before
-including the header @file{emacs-module.h}, you can also convert
-between Emacs integers and GMP @code{mpz_t} values.  @xref{GMP
-Basics,,,gmp}.  If @code{EMACS_MODULE_GMP} is defined,
-@file{emacs-module.h} wraps @code{mpz_t} in the following structure:
-
-@deftp struct emacs_mpz value
-struct emacs_mpz @{ mpz_t value; @};
-@end deftp
-
-@noindent
-Then you can use the following additional functions:
-
-@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, emacs_value @var{arg}, struct emacs_mpz *@var{result})
-This function, which is available since Emacs 27, extracts the
-integral value of @var{arg} into @var{result}.  @var{result} must not
-be @code{NULL}.  @code{@var{result}->value} must be an initialized
-@code{mpz_t} object.  @xref{Initializing Integers,,,gmp}.  If
-@var{arg} is an integer, Emacs will store its value into
-@code{@var{result}->value}.  After you have finished using
-@code{@var{result}->value}, you should free it using @code{mpz_clear}
-or similar.
-@end deftypefn
-
-@deftypefn Function emacs_value make_big_integer (emacs_env *@var{env}, const struct emacs_mpz *@var{value})
-This function, which is available since Emacs 27, takes an
-arbitrary-sized integer argument and returns a corresponding
-@code{emacs_value} object.  @var{value} must not be @code{NULL}.
-@code{@var{value}->value} must be an initialized @code{mpz_t} object.
-@xref{Initializing Integers,,,gmp}.  Emacs will return a corresponding
-integral object.  After you have finished using
-@code{@var{value}->value}, you should free it using @code{mpz_clear}
-or similar.
-@end deftypefn
-
-The following example uses GMP to calculate the next probable prime
-after a given integer:
-
-@example
-#include <assert.h>
-#include <gmp.h>
-
-#define EMACS_MODULE_GMP
-#include <emacs-module.h>
-
-static emacs_value
-next_prime (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
-            void *data)
-@{
-  assert (nargs == 1);
-  emacs_mpz p;
-  mpz_init (p.value);
-  env->extract_big_integer (env, args[0], &p);
-  mpz_nextprime (p.value, p.value);
-  emacs_value result = env->make_big_integer (env, &p);
-  mpz_clear (p.value);
-  return result;
-@}
-@end example
-
 The @acronym{API} does not provide functions to manipulate Lisp data
 structures, for example, create lists with @code{cons} and @code{list}
 (@pxref{Building Lists}), extract list members with @code{car} and
diff --git a/src/emacs-module.c b/src/emacs-module.c
index 4b991a1c74..4c560c5e9b 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -70,12 +70,6 @@ To add a new module function, proceed as follows:
 
 #include <config.h>
 
-#ifndef HAVE_GMP
-#include "mini-gmp.h"
-#define EMACS_MODULE_HAVE_MPZ_T
-#endif
-
-#define EMACS_MODULE_GMP
 #include "emacs-module.h"
 
 #include <stdarg.h>
@@ -772,21 +766,137 @@ module_make_time (emacs_env *env, struct timespec time)
   return lisp_to_value (env, timespec_to_lisp (time));
 }
 
-static void
-module_extract_big_integer (emacs_env *env, emacs_value value,
-                            struct emacs_mpz *result)
+/*
+Big integer support.
+
+There are two possible ways to support big integers in the module API
+that have been discussed:
+
+1. Exposing GMP numbers (mpz_t) directly in the API.
+
+2. Isolating the API from GMP by converting to/from a custom
+   sign-magnitude representation.
+
+Approach (1) has the advantage of being faster (no import/export
+required) and requiring less code in Emacs and in modules that would
+use GMP anyway.  However, (1) also couples big integer support
+directly to the current implementation in Emacs (GMP).  Also (1)
+requires each module author to ensure that their module is linked to
+the same GMP library as Emacs itself; in particular, module authors
+can't link GMP statically.  (1) also requires conditional compilation
+and workarounds to ensure the module interface still works if GMP
+isn't available while including emacs-module.h It also means that
+modules written in languages such as Go and Java that support big
+integers without GMP now have to carry an otherwise unnecessary GMP
+dependency.  Approach (2), on the other hand, neatly decouples the
+module interface from the GMP-based implementation.  It's not
+significantly more complex than (1) either: the additional code is
+mostly straightforward.  Over all, the benefits of (2) over (1) are
+large enough to prefer it here.
+
+We use a simple sign-magnitude representation for the big integers.
+For the magnitude we pick an unsigned long array instead of
+e.g. unsigned char.  This matches in most cases the representation of
+a GMP limb.  In such cases GMP picks an optimized algorithm for
+mpz_import and mpz_export that boils down to a single memcpy to
+convert the magnitude.  This way we largely avoid the import/export
+overhead on most platforms.
+*/
+
+static bool
+module_extract_big_integer (emacs_env *env, emacs_value arg, int *sign,
+                            ptrdiff_t *count, unsigned long *magnitude)
 {
-  MODULE_FUNCTION_BEGIN ();
-  Lisp_Object o = value_to_lisp (value);
+  MODULE_FUNCTION_BEGIN (false);
+  Lisp_Object o = value_to_lisp (arg);
   CHECK_INTEGER (o);
-  mpz_set_integer (result->value, o);
+  int dummy;
+  if (sign == NULL)
+    sign = &dummy;
+  /* See
+     https://gmplib.org/manual/Integer-Import-and-Export.html#index-Export. */
+  enum
+  {
+    order = -1,
+    size = sizeof *magnitude,
+    bits = size * CHAR_BIT,
+    endian = 0,
+    nails = 0,
+    numb = 8 * size - nails
+  };
+  if (FIXNUMP (o))
+    {
+      EMACS_INT x = XFIXNUM (o);
+      *sign = (0 < x) - (x < 0);
+      if (x == 0 || count == NULL)
+        return true;
+      /* As a simplification we don't check how many array elements
+         are exactly required, but use a reasonable static upper
+         bound.  For most architectures exactly one element should
+         suffice.  */
+      EMACS_UINT u;
+      enum { required = (sizeof u + size - 1) / size };
+      verify (0 < required);
+      if (magnitude == NULL)
+        {
+          *count = required;
+          return true;
+        }
+      if (*count < required)
+        {
+          ptrdiff_t actual = *count;
+          *count = required;
+          args_out_of_range_3 (INT_TO_INTEGER (actual),
+                               INT_TO_INTEGER (required),
+                               INT_TO_INTEGER (PTRDIFF_MAX));
+        }
+      /* Set u = abs(x).  See https://stackoverflow.com/a/17313717. */
+      if (0 < x)
+        u = (EMACS_UINT) x;
+      else
+        u = -(EMACS_UINT) x;
+      verify (required * bits < PTRDIFF_MAX);
+      for (ptrdiff_t i = 0; i < required; ++i)
+        magnitude[i] = (unsigned long) (u >> (i * bits));
+      return true;
+    }
+  const mpz_t *x = xbignum_val (o);
+  *sign = mpz_sgn (*x);
+  if (count == NULL)
+    return true;
+  size_t required_size = (mpz_sizeinbase (*x, 2) + numb - 1) / numb;
+  if (PTRDIFF_MAX < required_size)
+    overflow_error ();
+  ptrdiff_t required = (ptrdiff_t) required_size;
+  if (magnitude == NULL)
+    {
+      *count = required;
+      return true;
+    }
+  if (*count < required)
+    {
+      ptrdiff_t actual = *count;
+      *count = required;
+      args_out_of_range_3 (INT_TO_INTEGER (actual), INT_TO_INTEGER (required),
+                           INT_TO_INTEGER (PTRDIFF_MAX));
+    }
+  size_t written;
+  mpz_export (magnitude, &written, order, size, endian, nails, *x);
+  eassert (written == required_size);
+  return true;
 }
 
 static emacs_value
-module_make_big_integer (emacs_env *env, const struct emacs_mpz *value)
+module_make_big_integer (emacs_env *env, int sign,
+                         ptrdiff_t count, const unsigned long *magnitude)
 {
   MODULE_FUNCTION_BEGIN (NULL);
-  mpz_set (mpz[0], value->value);
+  if (sign == 0)
+    return lisp_to_value (env, make_fixed_natnum (0));
+  enum { order = -1, size = sizeof *magnitude, endian = 0, nails = 0 };
+  mpz_import (mpz[0], count, order, size, endian, nails, magnitude);
+  if (sign < 0)
+    mpz_neg (mpz[0], mpz[0]);
   return lisp_to_value (env, make_integer_mpz ());
 }
 
diff --git a/src/emacs-module.h.in b/src/emacs-module.h.in
index 9955e30eb7..314cf0f6f7 100644
--- a/src/emacs-module.h.in
+++ b/src/emacs-module.h.in
@@ -28,10 +28,6 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <stdbool.h>
 #endif
 
-#if defined EMACS_MODULE_GMP && !defined EMACS_MODULE_HAVE_MPZ_T
-#include <gmp.h>
-#endif
-
 #define EMACS_MAJOR_VERSION @emacs_major_version@
 
 #if defined __cplusplus && __cplusplus >= 201103L
@@ -100,12 +96,6 @@ enum emacs_process_input_result
   emacs_process_input_quit = 1
 };
 
-#ifdef EMACS_MODULE_GMP
-struct emacs_mpz { mpz_t value; };
-#else
-struct emacs_mpz;  /* no definition */
-#endif
-
 struct emacs_env_25
 {
 @module_env_snippet_25@
diff --git a/src/module-env-27.h b/src/module-env-27.h
index 00de300900..da8ac0e747 100644
--- a/src/module-env-27.h
+++ b/src/module-env-27.h
@@ -9,10 +9,10 @@
   emacs_value (*make_time) (emacs_env *env, struct timespec time)
     EMACS_ATTRIBUTE_NONNULL (1);
 
-  void (*extract_big_integer) (emacs_env *env, emacs_value value,
-                               struct emacs_mpz *result)
-    EMACS_ATTRIBUTE_NONNULL (1, 3);
+  bool (*extract_big_integer) (emacs_env *env, emacs_value arg, int *sign,
+                               ptrdiff_t *count, unsigned long *magnitude)
+    EMACS_ATTRIBUTE_NONNULL (1);
 
-  emacs_value (*make_big_integer) (emacs_env *env,
-                                   const struct emacs_mpz *value)
-    EMACS_ATTRIBUTE_NONNULL (1, 2);
+  emacs_value (*make_big_integer) (emacs_env *env, int sign, ptrdiff_t count,
+                                   const unsigned long *magnitude)
+    EMACS_ATTRIBUTE_NONNULL (1);
diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c
index 2891b73c1a..7521636b12 100644
--- a/test/data/emacs-module/mod-test.c
+++ b/test/data/emacs-module/mod-test.c
@@ -33,10 +33,8 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <gmp.h>
 #else
 #include "mini-gmp.h"
-#define EMACS_MODULE_HAVE_MPZ_T
 #endif
 
-#define EMACS_MODULE_GMP
 #include <emacs-module.h>
 
 #include "timespec.h"
@@ -372,23 +370,99 @@ Fmod_test_add_nanosecond (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
   return env->make_time (env, time);
 }
 
+static void
+memory_full (emacs_env *env)
+{
+  const char *message = "Memory exhausted";
+  emacs_value data = env->make_string (env, message, strlen (message));
+  env->non_local_exit_signal (env, env->intern (env, "error"),
+                              env->funcall (env, env->intern (env, "list"), 1,
+                                            &data));
+}
+
+static bool
+extract_big_integer (emacs_env *env, emacs_value arg, mpz_t result)
+{
+  int sign;
+  ptrdiff_t count;
+  bool success = env->extract_big_integer (env, arg, &sign, &count, NULL);
+  if (!success)
+    return false;
+  if (sign == 0)
+    {
+      mpz_set_ui (result, 0);
+      return true;
+    }
+  enum { order = -1, size = sizeof (unsigned long), endian = 0, nails = 0 };
+  assert (count > 0 && count <= SIZE_MAX);
+  unsigned long *magnitude = calloc (count, size);
+  if (magnitude == NULL)
+    {
+      memory_full (env);
+      return false;
+    }
+  success = env->extract_big_integer (env, arg, NULL, &count, magnitude);
+  assert (success);
+  mpz_import (result, count, order, size, endian, nails, magnitude);
+  free (magnitude);
+  if (sign < 0)
+    mpz_neg (result, result);
+  return true;
+}
+
+static emacs_value
+make_big_integer (emacs_env *env, const mpz_t value)
+{
+  if (mpz_sgn (value) == 0)
+    return env->make_integer (env, 0);
+  /* See
+     https://gmplib.org/manual/Integer-Import-and-Export.html#index-Export. */
+  enum
+  {
+    order = -1,
+    size = sizeof (unsigned long),
+    endian = 0,
+    nails = 0,
+    numb = 8 * size - nails
+  };
+  size_t count = (mpz_sizeinbase (value, 2) + numb - 1) / numb;
+  if (count > PTRDIFF_MAX)
+    {
+      memory_full (env);
+      return NULL;
+    }
+  unsigned long *magnitude = calloc (count, size);
+  if (magnitude == NULL)
+    {
+      memory_full (env);
+      return NULL;
+    }
+  size_t written;
+  mpz_export (magnitude, &written, order, size, endian, nails, value);
+  assert (written == count);
+  emacs_value result = env->make_big_integer (env, mpz_sgn (value),
+                                              (ptrdiff_t) count, magnitude);
+  free (magnitude);
+  return result;
+}
+
 static emacs_value
 Fmod_test_nanoseconds (emacs_env *env, ptrdiff_t nargs, emacs_value *args, void *data) {
   assert (nargs == 1);
   struct timespec time = env->extract_time (env, args[0]);
-  struct emacs_mpz nanoseconds;
+  mpz_t nanoseconds;
   assert (LONG_MIN <= time.tv_sec && time.tv_sec <= LONG_MAX);
-  mpz_init_set_si (nanoseconds.value, time.tv_sec);
+  mpz_init_set_si (nanoseconds, time.tv_sec);
 #ifdef __MINGW32__
   _Static_assert (1000000000 <= ULONG_MAX, "unsupported architecture");
 #else
   static_assert (1000000000 <= ULONG_MAX, "unsupported architecture");
 #endif
-  mpz_mul_ui (nanoseconds.value, nanoseconds.value, 1000000000);
+  mpz_mul_ui (nanoseconds, nanoseconds, 1000000000);
   assert (0 <= time.tv_nsec && time.tv_nsec <= ULONG_MAX);
-  mpz_add_ui (nanoseconds.value, nanoseconds.value, time.tv_nsec);
-  emacs_value result = env->make_big_integer (env, &nanoseconds);
-  mpz_clear (nanoseconds.value);
+  mpz_add_ui (nanoseconds, nanoseconds, time.tv_nsec);
+  emacs_value result = make_big_integer (env, nanoseconds);
+  mpz_clear (nanoseconds);
   return result;
 }
 
@@ -398,12 +472,12 @@ Fmod_test_double (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
 {
   assert (nargs == 1);
   emacs_value arg = args[0];
-  struct emacs_mpz value;
-  mpz_init (value.value);
-  env->extract_big_integer (env, arg, &value);
-  mpz_mul_ui (value.value, value.value, 2);
-  emacs_value result = env->make_big_integer (env, &value);
-  mpz_clear (value.value);
+  mpz_t value;
+  mpz_init (value);
+  extract_big_integer (env, arg, value);
+  mpz_mul_ui (value, value, 2);
+  emacs_value result = make_big_integer (env, value);
+  mpz_clear (value);
   return result;
 }
 
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

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

Thanks. I like the idea of decoupling Emacs modules from GMP.

The proposed approach will typically require copying bignum data twice 
to do a conversion, even if modules use GMP themselves. Shouldn't there 
be a shortcut for modules using GMP, so that they can use 
mpz_limbs_write and mpz_limbs_finish and bypass one of the copies? We 
can do that even if the module interface itself doesn't include gmp.h or 
assume GMP. For example, the Emacs module interface could define a type 
emacs_limb_t that happens to be the same size and representation as 
mpz_limb_t when Emacs is implemented via GMP, and the example function 
could look like the attached handwavy file "bigt.c". (It might be 
possible to avoid even the remaining copy in some cases; I didn't 
investigate this.)

Without such a shortcut, then we are to some extent giving up on 
efficiency, and in that case shouldn't we just ask people to convert to 
text and back, thus simplifying the interface?

On 11/17/19 10:38 AM, Philipp Stephani wrote:

 > +@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, 
emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{count}, unsigned 
long *@var{magnitude})
 > +This function, which is available since Emacs 27,

Should we be more systematic about saying which Emacs versions support 
which parts of the module API?

Uses of 'unsigned long' should be changed to a more-abstract integer 
type like 'emacs_limb_t', to give the implementation more leeway to 
change in future versions.

The extraction API looks a bit complicated. How about if we simplify it 
so that it (1) never signals an error and (2) it returns the limb count 
instead of always returning 'true' and storing the limb count via a 
pointer. Any too-small array can be reported merely by returning a limb 
count larger than the array size, without changing the array, as in the 
attached sample code.

 > extracts the
 > +integral value of @var{arg}.

Need to make it clearer that ARG can be any integer; it need not be a 
bignum.

 > it stores the required array
 > +size into @code{*count}

It should document that this size cannot exceed (min (PTRDIFF_MAX, 
SIZE_MAX) / sizeof (emacs_limb_t)). No matter what we do inside Emacs, 
it won't exceed that value for various reasons, and documenting this can 
simplify callers.

 > +  bool success = env->extract_big_integer (env, value, &sign, 
&count, NULL);

Shouldn't that 'value' be 'arg'?

 > +  mpz_import (value, count, order, size, endian, nails, magnitude);

Shouldn't that 'value' be 'result'?

At this point I stopped detailed review because I wanted your thoughts 
on the bigger questions.

[-- Attachment #2: bigt.c --]
[-- Type: text/x-csrc, Size: 880 bytes --]

  static bool
  extract_big_integer (emacs_env *env, emacs_value arg, mpz_t result)
  {
    int sign;
    unsigned long u;
    ptrdiff_t count = env->extract_big_integer (env, arg, &sign, 1, &u);
    if (count <= 1)
      mpz_set_ui (result, u);
    else
      {
	emacs_limb_t *magnitude
	  = _Generic (*mpz_limbs_write (result, count),
		      emacs_limb_t: mpz_limbs_write (result, count),
		      default: checked_malloc (count * sizeof *magnitude));
	env->extract_big_integer (env, arg, &sign, count, magnitude);
	if (_Generic (*mpz_limbs_write (result, count),
		      emacs_limb_t:
		        (mpz_limbs_finish (result, sign < 0 ? -count : count),
		         true),
		      default: false))
	  return true;
	mpz_import (result, count, -1, sizeof *magnitude, 0, 0, magnitude);
	free (magnitude);
      }

    if (sign < 0)
      mpz_neg (result, result);
    return true;
  }

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

Am Mo., 18. Nov. 2019 um 22:21 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> Thanks. I like the idea of decoupling Emacs modules from GMP.
>
> The proposed approach will typically require copying bignum data twice
> to do a conversion, even if modules use GMP themselves. Shouldn't there
> be a shortcut for modules using GMP, so that they can use
> mpz_limbs_write and mpz_limbs_finish and bypass one of the copies? We
> can do that even if the module interface itself doesn't include gmp.h or
> assume GMP. For example, the Emacs module interface could define a type
> emacs_limb_t that happens to be the same size and representation as
> mpz_limb_t when Emacs is implemented via GMP, and the example function
> could look like the attached handwavy file "bigt.c". (It might be
> possible to avoid even the remaining copy in some cases; I didn't
> investigate this.)

This sounds like a rather complex and probably premature optimization.
The current "unsigned long" type is identical to mpz_limb_t on most
platforms, and module authors can make use of that if they wish and
have profiled their code.

>
> Without such a shortcut, then we are to some extent giving up on
> efficiency, and in that case shouldn't we just ask people to convert to
> text and back, thus simplifying the interface?

With that argument you could argue that even fixed integer or float
conversions are unneeded because users could convert them to strings
as well. However, they are separate data types, so they deserve their
own interface.

>
> On 11/17/19 10:38 AM, Philipp Stephani wrote:
>
>  > +@deftypefn Function bool extract_big_integer (emacs_env *@var{env},
> emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{count}, unsigned
> long *@var{magnitude})
>  > +This function, which is available since Emacs 27,
>
> Should we be more systematic about saying which Emacs versions support
> which parts of the module API?

Sure, but that's for a different commit.

>
> Uses of 'unsigned long' should be changed to a more-abstract integer
> type like 'emacs_limb_t', to give the implementation more leeway to
> change in future versions.

In practice the implementation can't change, since that would break
either source or binary compatibility.
I'm not against a typedef to provide clearer semantics, but then we'd
also have to define EMACS_LIMB_MAX etc., and I see relatively little
benefit in that (since we can't realistically change it anyway later).

>
> The extraction API looks a bit complicated. How about if we simplify it
> so that it (1) never signals an error and (2) it returns the limb count
> instead of always returning 'true' and storing the limb count via a
> pointer. Any too-small array can be reported merely by returning a limb
> count larger than the array size, without changing the array, as in the
> attached sample code.

I'm not feeling strongly about the function in isolation, but the
current interface is modeled after the existing copy_string_contents
(which we can't change), and providing two different interfaces for
"copying an array" would be very confusing and unnecessarily
inconsistent.

>
>  > extracts the
>  > +integral value of @var{arg}.
>
> Need to make it clearer that ARG can be any integer; it need not be a
> bignum.

Ack, I'll make that change.

>
>  > it stores the required array
>  > +size into @code{*count}
>
> It should document that this size cannot exceed (min (PTRDIFF_MAX,
> SIZE_MAX) / sizeof (emacs_limb_t)). No matter what we do inside Emacs,
> it won't exceed that value for various reasons, and documenting this can
> simplify callers.

I'm not sure we should document such facts. It's true that this allows
callers to use malloc without checking for overflow, and it seems this
already follows from the C standard (can an array with total size
larger than SIZE_MAX exist?), but then I'm not convinced that this is
reason enough to add more complicated guarantees here.

>
>  > +  bool success = env->extract_big_integer (env, value, &sign,
> &count, NULL);
>
> Shouldn't that 'value' be 'arg'?

Yeah, that follows from changing the naming conventions in the middle ;)

>
>  > +  mpz_import (value, count, order, size, endian, nails, magnitude);
>
> Shouldn't that 'value' be 'result'?
>
> At this point I stopped detailed review because I wanted your thoughts
> on the bigger questions.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

On 11/19/19 1:12 PM, Philipp Stephani wrote:

> I'm not against a typedef to provide clearer semantics, but then we'd
> also have to define EMACS_LIMB_MAX etc., and I see relatively little
> benefit in that (since we can't realistically change it anyway later).

I can see a benefit, partly for the cleaner semantics, partly as it lets 
us arrange for Emacs limbs to be the same size as GMP limbs. If we can't 
realistically change this later, let's do it the better way the first 
time. It's easy enough to do; for example, we can define EMACS_LIMB_MAX 
to be the equivalent of ((emacs_limb_t) -1).

> The current "unsigned long" type is identical to mpz_limb_t on most
> platforms

unsigned long differs from mpz_limb_t on significant-enough platforms 
(MinGW, Cygwin, ...) that it's worth creating an emacs_limb_t to support 
those platforms better.

> I'm not feeling strongly about the function in isolation, but the
> current interface is modeled after the existing copy_string_contents

Why does copy_string_contents always return true when it returns? If 
there's no good reason, let's have the new function use a cleaner API. A 
flaw in an old function's API does not require us to have a similar flaw 
in the new one.

>> It should document that this size cannot exceed (min (PTRDIFF_MAX,
>> SIZE_MAX) / sizeof (emacs_limb_t))....
> 
> It's true that this allows
> callers to use malloc without checking for overflow, and it seems this
> already follows from the C standard (can an array with total size
> larger than SIZE_MAX exist?),
It does not follow from the C standard, as there's no requirement 
anywhere that Emacs internally must represent each integer as a 
contiguous array.

I found the guarantee useful in the first function I wrote that 
attempted to use the proposed API, so that's good evidence that it's 
worth documenting.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

Am Di., 19. Nov. 2019 um 22:54 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 11/19/19 1:12 PM, Philipp Stephani wrote:
>
> > I'm not against a typedef to provide clearer semantics, but then we'd
> > also have to define EMACS_LIMB_MAX etc., and I see relatively little
> > benefit in that (since we can't realistically change it anyway later).
>
> I can see a benefit, partly for the cleaner semantics, partly as it lets
> us arrange for Emacs limbs to be the same size as GMP limbs. If we can't
> realistically change this later, let's do it the better way the first
> time. It's easy enough to do; for example, we can define EMACS_LIMB_MAX
> to be the equivalent of ((emacs_limb_t) -1).

GMP seems to make a very complex series of choices when selecting the
limb datatype, which we can't and shouldn't replicate. I guess the
best we can do is to use unsigned long long if unsigned long is 4
bytes, and unsigned long otherwise.

>
> > The current "unsigned long" type is identical to mpz_limb_t on most
> > platforms
>
> unsigned long differs from mpz_limb_t on significant-enough platforms
> (MinGW, Cygwin, ...) that it's worth creating an emacs_limb_t to support
> those platforms better.

I really don't want to pollute the emacs-module header with lots of
platform-specific cruft just to support a very specific
micro-optimization.

>
> > I'm not feeling strongly about the function in isolation, but the
> > current interface is modeled after the existing copy_string_contents
>
> Why does copy_string_contents always return true when it returns? If
> there's no good reason, let's have the new function use a cleaner API. A
> flaw in an old function's API does not require us to have a similar flaw
> in the new one.

It's not a flaw, just a different design choice. copy_string_contents
returns false if and only if it has signaled an error, which is a
quite useful invariant.

>
> >> It should document that this size cannot exceed (min (PTRDIFF_MAX,
> >> SIZE_MAX) / sizeof (emacs_limb_t))....
> >
> > It's true that this allows
> > callers to use malloc without checking for overflow, and it seems this
> > already follows from the C standard (can an array with total size
> > larger than SIZE_MAX exist?),
> It does not follow from the C standard, as there's no requirement
> anywhere that Emacs internally must represent each integer as a
> contiguous array.
>
> I found the guarantee useful in the first function I wrote that
> attempted to use the proposed API, so that's good evidence that it's
> worth documenting.

I don't think so, it's a very narrow guarantee that we also don't give
for other functions. Each guarantee, however minor, comes with a cost:
we need to document it and stick to it forever.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

On 11/20/19 1:06 PM, Philipp Stephani wrote:
> GMP seems to make a very complex series of choices when selecting the
> limb datatype, which we can't and shouldn't replicate.

We don't need to replicate GMP's reasoning. We can use a 'configure' 
time test to use whatever type GMP uses. The emacs-module header could 
be quite simple, and just use that type (typically unsigned long, but 
not always) when typedeffing emacs_limb_t. I'll write the configure-time 
code, so you don't need to worry about implementing it (not that it's 
hard to write).

> copy_string_contents
> returns false if and only if it has signaled an error, which is a
> quite useful invariant.

No, copy_string_contents never returns false. If it signals an error, it 
does not return. If it returns, it always returns true. It's pretty 
weird, but there it is.

>> I found the guarantee useful in the first function I wrote that
>> attempted to use the proposed API, so that's good evidence that it's
>> worth documenting.
> 
> I don't think so, it's a very narrow guarantee that we also don't give
> for other functions.

I don't think any of the other functions need such a guarantee (but if 
they do, we should provide it).

> Each guarantee, however minor, comes with a cost:
> we need to document it and stick to it forever.

This particular guarantee has so little cost that the cost is not worth 
worrying about. It's a guarantee we'll never want to violate and there's 
essentially no cost to sticking to it "forever". I'll write the 
one-sentence documentation, if that helps.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

Am Mi., 20. Nov. 2019 um 22:24 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 11/20/19 1:06 PM, Philipp Stephani wrote:
> > GMP seems to make a very complex series of choices when selecting the
> > limb datatype, which we can't and shouldn't replicate.
>
> We don't need to replicate GMP's reasoning. We can use a 'configure'
> time test to use whatever type GMP uses. The emacs-module header could
> be quite simple, and just use that type (typically unsigned long, but
> not always) when typedeffing emacs_limb_t. I'll write the configure-time
> code, so you don't need to worry about implementing it (not that it's
> hard to write).

I don't think we can do that either. emacs-module.h has to be
completely independent from the rest of Emacs. GMP allows changing the
limb at configure time and depending on some architecture flags, we
can't allow that.

>
> > copy_string_contents
> > returns false if and only if it has signaled an error, which is a
> > quite useful invariant.
>
> No, copy_string_contents never returns false. If it signals an error, it
> does not return. If it returns, it always returns true. It's pretty
> weird, but there it is.

No, it returns false in case of an error. Module functions always return.

>
> >> I found the guarantee useful in the first function I wrote that
> >> attempted to use the proposed API, so that's good evidence that it's
> >> worth documenting.
> >
> > I don't think so, it's a very narrow guarantee that we also don't give
> > for other functions.
>
> I don't think any of the other functions need such a guarantee (but if
> they do, we should provide it).
>
> > Each guarantee, however minor, comes with a cost:
> > we need to document it and stick to it forever.
>
> This particular guarantee has so little cost that the cost is not worth
> worrying about. It's a guarantee we'll never want to violate and there's
> essentially no cost to sticking to it "forever". I'll write the
> one-sentence documentation, if that helps.

The guarantee itself is fine with me (we should put an assertion into
the codebase to document it). I just wonder whether it's important
enough to actually make.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

On 11/20/19 1:30 PM, Philipp Stephani wrote:

> I don't think we can do that either. emacs-module.h has to be
> completely independent from the rest of Emacs. GMP allows changing the
> limb at configure time and depending on some architecture flags, we
> can't allow that.

I don't see why not, as my proposal would continue to keep 
emacs-module.h independent of the rest of Emacs. emacs_limb_t would 
merely be a performance hint about GMP, and would not necessarily match 
the mp_limb_t that an Emacs module uses.

Let's take an unusual and extreme example. Suppose Emacs is built with a 
a "normal" GMP that uses 64-bit limbs on the current platform, so its 
emacs-module.h defines emacs_limb_t to be a 64-bit integer. And suppose 
the module author builds with an "unsual" GMP that is configured to use 
32-bit limbs on the current platform. Then emacs_limb_t might be 
'unsigned long long int' while mp_limb_t is 'unsigned int' within module 
code. This combination will work: there will be two copies of the GMP 
library linked into the executable (the "normal" one and the "unusual" 
one), each with its own API and names and calling conventions; Emacs 
internals will call one copy and module code will call the other, and 
the module code will note that mp_limb_t and emacs_limb_t are different 
types and will fall back on the slower mpz_export method to copy integers.

However, such an example will hardly ever happen. In practice, GMP is 
built one way for a particular platform, choosing a platform (which an 
Emacs builder needs to do anyway) will choose a particular GMP 
implementation, and Emacs's internal use of GMP will match the module's 
use so conversion will operate more efficiently.

>> opy_string_contents never returns false. If it signals an error, it
>> does not return. If it returns, it always returns true. It's pretty
>> weird, but there it is.
> 
> No, it returns false in case of an error. Module functions always return.

Ah, thanks, I see now; it's handled by MODULE_FUNCTION_BEGIN. OK, I'll 
withdraw this part of my comment; we can live with returning a bool.

> The guarantee itself is fine with me (we should put an assertion into
> the codebase to document it). I just wonder whether it's important
> enough to actually make.

Yes, I think so. (Admittedly I worry about integer overflow more than 
most people do. :-)

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

Am Do., 21. Nov. 2019 um 02:12 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 11/20/19 1:30 PM, Philipp Stephani wrote:
>
> > I don't think we can do that either. emacs-module.h has to be
> > completely independent from the rest of Emacs. GMP allows changing the
> > limb at configure time and depending on some architecture flags, we
> > can't allow that.
>
> I don't see why not, as my proposal would continue to keep
> emacs-module.h independent of the rest of Emacs. emacs_limb_t would
> merely be a performance hint about GMP, and would not necessarily match
> the mp_limb_t that an Emacs module uses.
>
> Let's take an unusual and extreme example. Suppose Emacs is built with a
> a "normal" GMP that uses 64-bit limbs on the current platform, so its
> emacs-module.h defines emacs_limb_t to be a 64-bit integer. And suppose
> the module author builds with an "unsual" GMP that is configured to use
> 32-bit limbs on the current platform. Then emacs_limb_t might be
> 'unsigned long long int' while mp_limb_t is 'unsigned int' within module
> code. This combination will work: there will be two copies of the GMP
> library linked into the executable (the "normal" one and the "unusual"
> one), each with its own API and names and calling conventions; Emacs
> internals will call one copy and module code will call the other, and
> the module code will note that mp_limb_t and emacs_limb_t are different
> types and will fall back on the slower mpz_export method to copy integers.
>
> However, such an example will hardly ever happen. In practice, GMP is
> built one way for a particular platform, choosing a platform (which an
> Emacs builder needs to do anyway) will choose a particular GMP
> implementation, and Emacs's internal use of GMP will match the module's
> use so conversion will operate more efficiently.

That is not the issue I have in mind. The problem is that
emacs-module.h needs to be token-by-token identical for all platforms
- it's expected that distributors and users copy it around at will. We
can't have it depend on the platform that Emacs was built, or any
configure flags used for building Emacs.
Even if we allowed emacs-module.h to differ between platforms (which
we shouldn't), we still couldn't allow emacs-module.h to contain data
derived from configure flags, as then users specifying different
configure options could produce wrong/incompatible variants of the
header. It's crucial that there's a single canonical emacs-module.h.


> > The guarantee itself is fine with me (we should put an assertion into
> > the codebase to document it). I just wonder whether it's important
> > enough to actually make.
>
> Yes, I think so. (Admittedly I worry about integer overflow more than
> most people do. :-)

I definitely worry about it, too, and everyone should. I'm OK with
documenting the guarantee since it can prevent users from running into
undefined behavior.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

On 11/21/19 12:31 PM, Philipp Stephani wrote:
> It's crucial that there's a single canonical emacs-module.h.

I don't see why it's crucial. Certainly emacs-module.h will differ for 
different Emacs versions, and people will deal with any mismatches as 
usual. And developers do not expect /usr/include/stdio.h to be the same 
on all platforms; why would they expect /usr/include/emacs-module.h to 
be the same? After all, modules themselves will not be portable among 
platforms, even if the module's sources and include files are 
byte-for-byte identical.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

Am Sa., 23. Nov. 2019 um 03:13 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 11/21/19 12:31 PM, Philipp Stephani wrote:
> > It's crucial that there's a single canonical emacs-module.h.
>
> I don't see why it's crucial. Certainly emacs-module.h will differ for
> different Emacs versions,

But only in very specific backwards-compatible ways.

> and people will deal with any mismatches as
> usual. And developers do not expect /usr/include/stdio.h to be the same
> on all platforms; why would they expect /usr/include/emacs-module.h to
> be the same?

Because stdio.h is part of the C implementation, but emacs-module.h is not.

> After all, modules themselves will not be portable among
> platforms, even if the module's sources and include files are
> byte-for-byte identical.

Yes, but we still at least need the header to remain the same across
time and across configuration options within each platform. Having the
header depend on configure makes it almost impossible to guarantee
that.

In the interest of not blocking the Emacs 27 release on this, and
unless Eli objects, I'll push my original patch to master within the
next few days, with the following minor modifications:

- Documenting the restriction on the number of magnitude array elements.
- Using a typedef emacs_limb_t for the magnitude array elements, with
the guarantee that's is a stable unsigned integer type. I will make
emacs_limb_t an alias of unsigned long long if ULONG_MAX <=
0xFFFFFFFF, and an alias of unsigned long otherwise.
- Fixing the example code.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

On 11/23/19 12:08 PM, Philipp Stephani wrote:

> stdio.h is part of the C implementation, but emacs-module.h is not.

>> > And developers do not expect /usr/include/stdio.h to be the same
>> > on all platforms; why would they expect /usr/include/emacs-module.h to
>> > be the same?
> 
> Because stdio.h is part of the C implementation, but emacs-module.h is not.

What difference does that make to the module developer? emacs-module.h includes 
<stdint.h> and other files that are implementation-dependent. Because of this, 
from the developer's point of view "#include <emacs-module.h>" is just as 
implementation-dependent as "#include <stdio.h>" is, which means developers must 
be cognizant of platform dependencies anyway.

So I still don't why it's crucial that emacs-module.h is byte-for-byte identical 
on all platforms.

> - Documenting the restriction on the number of magnitude array elements.
> - Using a typedef emacs_limb_t for the magnitude array elements, with
> the guarantee that's is a stable unsigned integer type. I will make
> emacs_limb_t an alias of unsigned long long if ULONG_MAX <=
> 0xFFFFFFFF, and an alias of unsigned long otherwise.

Thanks, that'll be an improvement.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Stefan Monnier]

>> It's crucial that there's a single canonical emacs-module.h.
> I don't see why it's crucial.

The module API was designed with the goal that a module compiled for
a given version of Emacs will keep working with other versions, as long
as they're more recent.  For example recompiling --with-wide-int or
--without-wide-int shouldn't affect the modules's binary compatibility.

I don't know if it's crucial, but so far we have decided that it is and
I don't think that bignums are a good reason to revert this decision.


        Stefan

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

On 11/23/19 12:46 PM, Stefan Monnier wrote:
> recompiling --with-wide-int or
> --without-wide-int shouldn't affect the modules's binary compatibility.

Sure, but recompiling with gcc -m32 or -m64 does affect binary compatibility. 
Choice of binary API can be affected not only by the CPU model, but also by a 
bunch of other stuff including compiler, compiler options and libraries. Even if 
some of these choices do not affect binary compatibility, other choices do and 
developers must take this into account anyway.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Andreas Schwab]

On Nov 23 2019, Paul Eggert wrote:

> Sure, but recompiling with gcc -m32 or -m64 does affect binary
> compatibility.

How is that relevant?  It's like compiling for a completely different
architecture.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

On 11/24/19 1:28 AM, Andreas Schwab wrote:
> It's like compiling for a completely different architecture.

Yes, and that's the point.

We agree that choice of 'configure' options can change the platform, and 
that one cannot assume that modules built for one platform are 
compatible with an Emacs built for another.

The disagreement is whether we should consider GMP to be "part of Emacs" 
(so GMP should not affect the module ABI) or "part of the platform" (so 
GMP can affect it). Both approaches are plausible; the former lets 
modules be more portable, and the latter gives modules better 
performance. Although I'm a fan of portability, here the portability 
advantage seems small and the performance advantage significant, so it's 
not clear that saying "GMP is part of Emacs" would be a win.

Since an Emacs 27 release is imminent, perhaps the best thing to do 
would be to remove the extract_big_integer and make_big_integer methods 
from src/emacs-module.h.in. These methods are not good in master because 
they depend on gmp.h, and the proposed replacements are not good because 
they're slow and awkward. Surely we can do better than either, but we 
can't do that in a rush.

Plus, quite possibly it will turn out that module authors don't much 
need a specialized API for bignums. After all, we've gotten along so far 
without a specialized API for bool-vectors, and to some extent bignums 
are just bool-vectors in disguise.

I can propose a simple patch along these lines, if there's interest.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

Am Mo., 25. Nov. 2019 um 22:03 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 11/24/19 1:28 AM, Andreas Schwab wrote:
> > It's like compiling for a completely different architecture.
>
> Yes, and that's the point.
>
> We agree that choice of 'configure' options can change the platform, and
> that one cannot assume that modules built for one platform are
> compatible with an Emacs built for another.

It cannot "change the platform" for usual definitions of "platform"
(combination of operating system and CPU architecture). Different
configuration options don't constitute "platforms."

>
> The disagreement is whether we should consider GMP to be "part of Emacs"
> (so GMP should not affect the module ABI) or "part of the platform" (so
> GMP can affect it). Both approaches are plausible; the former lets
> modules be more portable, and the latter gives modules better
> performance. Although I'm a fan of portability, here the portability
> advantage seems small and the performance advantage significant, so it's
> not clear that saying "GMP is part of Emacs" would be a win.

Unsurprisingly I completely disagree with that. As I've shown (!) in
the commit message, even in the case of a function that does nothing
except dealing with big integers, the overhead on GNU/Linux (and
presumably most Unix systems) is insignificant. (In fact, in an
earlier benchmark using unsigned char as magnitude element I couldn't
find any significant slowdown either, even though GMP always had to
pick the slow path during converting.) On the other hand, you haven't
shown any significant performance advantage of your approach at all,
making this a discussion about a purely hypothetical
micro-optimization. I won't compromise on stability on robustness
without any data point that would necessitate such a compromise.
To be totally clear, the primary goals of the module API are
stability, robustness, and simplicity. Performance is only a secondary
goal. Even so, in this case there's no performance disadvantage at
all.

>
> Since an Emacs 27 release is imminent, perhaps the best thing to do
> would be to remove the extract_big_integer and make_big_integer methods
> from src/emacs-module.h.in. These methods are not good in master because
> they depend on gmp.h, and the proposed replacements are not good because
> they're slow and awkward.

They are not slow and awkward (at least not more awkward than typical
C APIs), they are portable, stable, relatively clean, simple, and fast
enough.

> Plus, quite possibly it will turn out that module authors don't much
> need a specialized API for bignums. After all, we've gotten along so far
> without a specialized API for bool-vectors, and to some extent bignums
> are just bool-vectors in disguise.

Boolean vectors are an exotic data type that isn't common in Emacs
Lisp or other languages, unlike big integers, which are built in in
some languages (Python, Haskell), or readily available in the standard
library (Java, Go).
(I'm not opposed to adding functionality to extract and create Boolean
vectors, but they are clearly less important.)

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

Am Mo., 25. Nov. 2019 um 22:59 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Mo., 25. Nov. 2019 um 22:03 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
> >
> > On 11/24/19 1:28 AM, Andreas Schwab wrote:
> > > It's like compiling for a completely different architecture.
> >
> > Yes, and that's the point.
> >
> > We agree that choice of 'configure' options can change the platform, and
> > that one cannot assume that modules built for one platform are
> > compatible with an Emacs built for another.
>
> It cannot "change the platform" for usual definitions of "platform"
> (combination of operating system and CPU architecture). Different
> configuration options don't constitute "platforms."
>
> >
> > The disagreement is whether we should consider GMP to be "part of Emacs"
> > (so GMP should not affect the module ABI) or "part of the platform" (so
> > GMP can affect it). Both approaches are plausible; the former lets
> > modules be more portable, and the latter gives modules better
> > performance. Although I'm a fan of portability, here the portability
> > advantage seems small and the performance advantage significant, so it's
> > not clear that saying "GMP is part of Emacs" would be a win.
>
> Unsurprisingly I completely disagree with that. As I've shown (!) in
> the commit message, even in the case of a function that does nothing
> except dealing with big integers, the overhead on GNU/Linux (and
> presumably most Unix systems) is insignificant. (In fact, in an
> earlier benchmark using unsigned char as magnitude element I couldn't
> find any significant slowdown either, even though GMP always had to
> pick the slow path during converting.) On the other hand, you haven't
> shown any significant performance advantage of your approach at all,
> making this a discussion about a purely hypothetical
> micro-optimization. I won't compromise on stability on robustness
> without any data point that would necessitate such a compromise.
> To be totally clear, the primary goals of the module API are
> stability, robustness, and simplicity. Performance is only a secondary
> goal. Even so, in this case there's no performance disadvantage at
> all.
>
> >
> > Since an Emacs 27 release is imminent, perhaps the best thing to do
> > would be to remove the extract_big_integer and make_big_integer methods
> > from src/emacs-module.h.in. These methods are not good in master because
> > they depend on gmp.h, and the proposed replacements are not good because
> > they're slow and awkward.
>
> They are not slow and awkward (at least not more awkward than typical
> C APIs), they are portable, stable, relatively clean, simple, and fast
> enough.
>
> > Plus, quite possibly it will turn out that module authors don't much
> > need a specialized API for bignums. After all, we've gotten along so far
> > without a specialized API for bool-vectors, and to some extent bignums
> > are just bool-vectors in disguise.
>
> Boolean vectors are an exotic data type that isn't common in Emacs
> Lisp or other languages, unlike big integers, which are built in in
> some languages (Python, Haskell), or readily available in the standard
> library (Java, Go).
> (I'm not opposed to adding functionality to extract and create Boolean
> vectors, but they are clearly less important.)

Since there were no more comments or objections, and all other points
of discussion have been resolved, I've pushed a slightly modified
version of the patch as 096be9c454. Except for extremely exotic
architectures or custom GMP builds, the chosen limb should be the same
as the default GMP limb.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 4 Dec 2019 21:31:42 +0100
> Cc: Philipp Stephani <phst@google.com>, Andreas Schwab <schwab@linux-m68k.org>,
>  Stefan Monnier <monnier@iro.umontreal.ca>,
>  Emacs developers <emacs-devel@gnu.org>
> 
> Since there were no more comments or objections, and all other points
> of discussion have been resolved, I've pushed a slightly modified
> version of the patch as 096be9c454. Except for extremely exotic
> architectures or custom GMP builds, the chosen limb should be the same
> as the default GMP limb.

Thanks.

However, this:

  #if ULONG_MAX == 0xFFFFFFFF
  typedef unsigned long long emacs_limb_t;
  # define EMACS_LIMB_MAX ULLONG_MAX
  #else
  typedef unsigned long emacs_limb_t;
  # define EMACS_LIMB_MAX ULONG_MAX
  #endif

seems to punish every 32-bit build of Emacs (and on MS-Windows even
the 64-bit builds, AFAIU).  Is there a reason for doing this?  Testing
for the native size of an 'unsigned long' data type is not
significantly more complicated than the above, and GMP goes with that
when it determines the type of mp_limb_t, AFAIK.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

Am Do., 5. Dez. 2019 um 15:43 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Wed, 4 Dec 2019 21:31:42 +0100
> > Cc: Philipp Stephani <phst@google.com>, Andreas Schwab <schwab@linux-m68k.org>,
> >  Stefan Monnier <monnier@iro.umontreal.ca>,
> >  Emacs developers <emacs-devel@gnu.org>
> >
> > Since there were no more comments or objections, and all other points
> > of discussion have been resolved, I've pushed a slightly modified
> > version of the patch as 096be9c454. Except for extremely exotic
> > architectures or custom GMP builds, the chosen limb should be the same
> > as the default GMP limb.
>
> Thanks.
>
> However, this:
>
>   #if ULONG_MAX == 0xFFFFFFFF
>   typedef unsigned long long emacs_limb_t;
>   # define EMACS_LIMB_MAX ULLONG_MAX
>   #else
>   typedef unsigned long emacs_limb_t;
>   # define EMACS_LIMB_MAX ULONG_MAX
>   #endif
>
> seems to punish every 32-bit build of Emacs (and on MS-Windows even
> the 64-bit builds, AFAIU).  Is there a reason for doing this?  Testing
> for the native size of an 'unsigned long' data type is not
> significantly more complicated than the above, and GMP goes with that
> when it determines the type of mp_limb_t, AFAIK.

Not quite sure I understand. In what way does this selection punish
32-bit builds? Also the code already effectively tests the native size
of unsigned long, so I'm not sure what you'd like to change here.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 8 Dec 2019 21:28:12 +0100
> Cc: Paul Eggert <eggert@cs.ucla.edu>, Philipp Stephani <phst@google.com>, 
> 	Andreas Schwab <schwab@linux-m68k.org>, Stefan Monnier <monnier@iro.umontreal.ca>, 
> 	Emacs developers <emacs-devel@gnu.org>
> 
> Am Do., 5. Dez. 2019 um 15:43 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> >
> > However, this:
> >
> >   #if ULONG_MAX == 0xFFFFFFFF
> >   typedef unsigned long long emacs_limb_t;
> >   # define EMACS_LIMB_MAX ULLONG_MAX
> >   #else
> >   typedef unsigned long emacs_limb_t;
> >   # define EMACS_LIMB_MAX ULONG_MAX
> >   #endif
> >
> > seems to punish every 32-bit build of Emacs (and on MS-Windows even
> > the 64-bit builds, AFAIU).  Is there a reason for doing this?  Testing
> > for the native size of an 'unsigned long' data type is not
> > significantly more complicated than the above, and GMP goes with that
> > when it determines the type of mp_limb_t, AFAIK.
> 
> Not quite sure I understand. In what way does this selection punish
> 32-bit builds?

It makes emacs_limb_t be a 64-bit type, whereas mp_limb_t is a 32-bit
type in that case, and that makes the interface less efficient,
because GMP is optimized for the case where they match.

> Also the code already effectively tests the native size of unsigned
> long, so I'm not sure what you'd like to change here.

I'd like to make emacs_limb_t be an unsigned long in 32-bit builds, to
match mp_limb_t.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

On 12/8/19 7:26 PM, Eli Zaretskii wrote:
> I'd like to make emacs_limb_t be an unsigned long in 32-bit builds, to
> match mp_limb_t.

mp_limb_t is more complicated than that. On some 32-bit platforms, mp_limb_t
defaults to 64-bit 'unsigned long long', not to 32-bit 'unsigned long'. (The
libgmp builder can override this default, though I imagine most don't do that.)

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Eli Zaretskii]

> Cc: phst@google.com, schwab@linux-m68k.org, monnier@iro.umontreal.ca,
>  emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 8 Dec 2019 20:58:29 -0800
> 
> On 12/8/19 7:26 PM, Eli Zaretskii wrote:
> > I'd like to make emacs_limb_t be an unsigned long in 32-bit builds, to
> > match mp_limb_t.
> 
> mp_limb_t is more complicated than that. On some 32-bit platforms, mp_limb_t
> defaults to 64-bit 'unsigned long long', not to 32-bit 'unsigned long'. (The
> libgmp builder can override this default, though I imagine most don't do that.)

That's fine, but if we can ignore _all_ platform differences, we
surely can ignore a subset, i.e. platforms where such non-default
sizes were chosen for some reason, and just use 'unsigned long long'
for them.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

Am Mo., 9. Dez. 2019 um 04:27 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sun, 8 Dec 2019 21:28:12 +0100
> > Cc: Paul Eggert <eggert@cs.ucla.edu>, Philipp Stephani <phst@google.com>,
> >       Andreas Schwab <schwab@linux-m68k.org>, Stefan Monnier <monnier@iro.umontreal.ca>,
> >       Emacs developers <emacs-devel@gnu.org>
> >
> > Am Do., 5. Dez. 2019 um 15:43 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> > >
> > > However, this:
> > >
> > >   #if ULONG_MAX == 0xFFFFFFFF
> > >   typedef unsigned long long emacs_limb_t;
> > >   # define EMACS_LIMB_MAX ULLONG_MAX
> > >   #else
> > >   typedef unsigned long emacs_limb_t;
> > >   # define EMACS_LIMB_MAX ULONG_MAX
> > >   #endif
> > >
> > > seems to punish every 32-bit build of Emacs (and on MS-Windows even
> > > the 64-bit builds, AFAIU).  Is there a reason for doing this?  Testing
> > > for the native size of an 'unsigned long' data type is not
> > > significantly more complicated than the above, and GMP goes with that
> > > when it determines the type of mp_limb_t, AFAIK.
> >
> > Not quite sure I understand. In what way does this selection punish
> > 32-bit builds?
>
> It makes emacs_limb_t be a 64-bit type, whereas mp_limb_t is a 32-bit
> type in that case, and that makes the interface less efficient,
> because GMP is optimized for the case where they match.
>
> > Also the code already effectively tests the native size of unsigned
> > long, so I'm not sure what you'd like to change here.
>
> I'd like to make emacs_limb_t be an unsigned long in 32-bit builds, to
> match mp_limb_t.

In other words, you'd like it to be an alias for uintptr_t? I'd be
totally fine with that as well.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

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

On 12/9/19 3:15 PM, Philipp Stephani wrote:
> In other words, you'd like it to be an alias for uintptr_t? I'd be
> totally fine with that as well.

size_t would be a bit better, as it would serve the same function and 
it's more commonly used and better supported. Something like the 
attached patch, say.

[-- Attachment #2: 0001-Just-use-size_t-for-emacs_limb_t.patch --]
[-- Type: text/x-patch, Size: 1996 bytes --]

From fa7a36d9f2577fe843fd32beb8df771d7e485c99 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 9 Dec 2019 16:16:56 -0800
Subject: [PATCH] Just use size_t for emacs_limb_t

* src/emacs-module.h.in: Do not include limits.h; no longer needed.
(emacs_limb_t, EMACS_LIMB_MAX): Now size_t and SIZE_MAX.
---
 src/emacs-module.h.in | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/src/emacs-module.h.in b/src/emacs-module.h.in
index 800c0188ff..0891b1aa28 100644
--- a/src/emacs-module.h.in
+++ b/src/emacs-module.h.in
@@ -20,7 +20,6 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #ifndef EMACS_MODULE_H
 #define EMACS_MODULE_H
 
-#include <limits.h>
 #include <stdint.h>
 #include <stddef.h>
 #include <time.h>
@@ -97,22 +96,13 @@ enum emacs_process_input_result
   emacs_process_input_quit = 1
 };
 
-/*
-Implementation note: We define emacs_limb_t so that it is likely to
-match the GMP mp_limb_t type.  If the types match, GMP can use an
-optimization for mpz_import and mpz_export that boils down to a
-memcpy.  According to https://gmplib.org/manual/ABI-and-ISA.html GMP
-will prefer a 64-bit limb and will default to unsigned long if that is
-wide enough.  Note that this is an internal micro-optimization.  Users
-shouldn't rely on the exact size of emacs_limb_t.
-*/
-#if ULONG_MAX == 0xFFFFFFFF
-typedef unsigned long long emacs_limb_t;
-# define EMACS_LIMB_MAX ULLONG_MAX
-#else
-typedef unsigned long emacs_limb_t;
-# define EMACS_LIMB_MAX ULONG_MAX
-#endif
+/* Define emacs_limb_t so that it is likely to match GMP's mp_limb_t.
+   This micro-optimization can help modules that use mpz_export and
+   mpz_import, which operate more efficiently on mp_limb_t.  It's OK
+   (if perhaps a bit slower) if the two types do not match, and
+   modules shouldn't rely on the two types matching.  */
+typedef size_t emacs_limb_t;
+#define EMACS_LIMB_MAX SIZE_MAX
 
 struct emacs_env_25
 {
-- 
2.23.0

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

Am Di., 10. Dez. 2019 um 01:22 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 12/9/19 3:15 PM, Philipp Stephani wrote:
> > In other words, you'd like it to be an alias for uintptr_t? I'd be
> > totally fine with that as well.
>
> size_t would be a bit better, as it would serve the same function and
> it's more commonly used and better supported. Something like the
> attached patch, say.

Also fine with me. Feel free to install (unless Eli objects).

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Eli Zaretskii]

> Cc: Philipp Stephani <phst@google.com>, Andreas Schwab
>  <schwab@linux-m68k.org>, Stefan Monnier <monnier@iro.umontreal.ca>,
>  Emacs developers <emacs-devel@gnu.org>
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 9 Dec 2019 16:22:13 -0800
> 
> On 12/9/19 3:15 PM, Philipp Stephani wrote:
> > In other words, you'd like it to be an alias for uintptr_t? I'd be
> > totally fine with that as well.
> 
> size_t would be a bit better, as it would serve the same function and 
> it's more commonly used and better supported. Something like the 
> attached patch, say.

Fine with me, thanks.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Philipp Stephani]

Am Di., 10. Dez. 2019 um 16:58 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > Cc: Philipp Stephani <phst@google.com>, Andreas Schwab
> >  <schwab@linux-m68k.org>, Stefan Monnier <monnier@iro.umontreal.ca>,
> >  Emacs developers <emacs-devel@gnu.org>
> > From: Paul Eggert <eggert@cs.ucla.edu>
> > Date: Mon, 9 Dec 2019 16:22:13 -0800
> >
> > On 12/9/19 3:15 PM, Philipp Stephani wrote:
> > > In other words, you'd like it to be an alias for uintptr_t? I'd be
> > > totally fine with that as well.
> >
> > size_t would be a bit better, as it would serve the same function and
> > it's more commonly used and better supported. Something like the
> > attached patch, say.
>
> Fine with me, thanks.

Another random idea: What about adding parameters for the limb size,
order and endinanness instead, like mpz_export/import? It would make
the interface a bit more complicated, but would allow users to decide
which limb type to use.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

On 12/14/19 8:06 AM, Philipp Stephani wrote:
>> Fine with me, thanks.
> Another random idea: What about adding parameters for the limb size,
> order and endinanness instead, like mpz_export/import? It would make
> the interface a bit more complicated, but would allow users to decide
> which limb type to use.

I'd leave it alone for now, and see what users want. It's possible the current
interface is fine. It's also possible users will want something quite different
than a fancier import/export facility.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert]

On 12/5/19 6:43 AM, Eli Zaretskii wrote:
> this:
> 
>    #if ULONG_MAX == 0xFFFFFFFF
>    typedef unsigned long long emacs_limb_t;
>    # define EMACS_LIMB_MAX ULLONG_MAX
>    #else
>    typedef unsigned long emacs_limb_t;
>    # define EMACS_LIMB_MAX ULONG_MAX
>    #endif
> 
> seems to punish every 32-bit build of Emacs (and on MS-Windows even
> the 64-bit builds, AFAIU).  Is there a reason for doing this?  Testing
> for the native size of an 'unsigned long' data type is not
> significantly more complicated than the above, and GMP goes with that
> when it determines the type of mp_limb_t, AFAIK.

Actually GMP uses a more complicated way to determine the type of 
mp_limb_t. The builder of GMP can select 'unsigned long long' or 
'unsigned long', and although the default GMP limb type is typically 
'unsigned long' there are some notable exceptions (including 64-bit 
mingw and cygwin, 32-bit powerpc, etc.) that exploit the 64-bit 
registers of certain platforms.

Are your concerns about "punishing" based on choosing a different limb 
size from GMP's? But emacs-module.h doesn't attempt to mimic GMP's limb 
size. Instead, it is intended to be portable and reliable and "fast 
enough". It is not a high-performance API; for example, if you write a 
simple GMP-based module function to add 1 to a bignum, calling the 
function will copy all the integer's bits four times (in addition to the 
work that libgmp itself does to add 1). Since performance is not a big 
deal in this interface, choice of limb size is not a big deal either.

With that in mind, perhaps we should replace the above 7 lines with 
these 2 lines:

typedef unsigned long long emacs_limb_t;
#define EMACS_LIMB_MAX ULLONG_MAX

This should address the point you raise (for mingw and cygwin, anyway), 
and would be even simpler and more reliable than what we have now. For 
example, it'd let modules format limb values with "%llx" instead of 
requiring something more complicated (as the current master does).

As I mentioned in a previous email I'm not a fan of a module API for 
bignums that requires so much copying. If modules hardly ever use 
bignums then converting via text should be good enough; and if they use 
bignums a lot then the extra copying will make for poor performance for 
large bignums. But I suppose that if the latter becomes an issue, we can 
add a better API later and support the current API "forever" for older 
modules.

Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Eli Zaretskii]

> Cc: phst@google.com, schwab@linux-m68k.org, monnier@iro.umontreal.ca,
>  emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 8 Dec 2019 16:35:13 -0800
> 
> With that in mind, perhaps we should replace the above 7 lines with 
> these 2 lines:
> 
> typedef unsigned long long emacs_limb_t;
> #define EMACS_LIMB_MAX ULLONG_MAX

I don't mind doing this, but then we should remove this comment from
emacs-module.c, or reword it so that it says nothing about optimality
of matching emacs_limb_t and mp_limb_t:

  For the magnitude we pick an array of an unsigned integer type similar
  to mp_limb_t instead of e.g. unsigned char.  This matches in most
  cases the representation of a GMP limb.  In such cases GMP picks an
  optimized algorithm for mpz_import and mpz_export that boils down to a
  single memcpy to convert the magnitude.  This way we largely avoid the
  import/export overhead on most platforms.

because the suggestion means we don't really care about how optimal
this is.

Re: [PATCH 1/2] Add conversions to and from struct timespec to module interface.

[Paul Eggert]

On 4/23/19 6:17 AM, Philipp Stephani wrote:
> +@deftypefn Function struct timespec extract_time (emacs_env *@var{env}, emacs_value @var{time})
> +This function interprets @var{time} as an Emacs time value and returns
> +the corresponding @code{struct timespec}.  @xref{Time of Day}.
> +@var{time} may not be @code{nil}.

Why not have 'nil' mean the same thing here that it does elsewhere for
Emacs time values, namely, the current time? (That would make the code a
tiny bit faster. :-)

> This function raises an error if

"signals an error"

> +  assert (time.tv_nsec < 1000000000);

2 billion would be safer, to cater to (hypothetical and non-POSIX)
implementations that represent leap seconds with 1 billion <= tv_nsec <
2 billion. That's what timespec_cmp does, anyway.

> +  (dolist (input '(#x10000000000000000  ; out of range
This test will fail to be out of range on (hypothetical) machines where
time_t is 128 bits. How about 1.0e+INF instead? You can test -1.0e+INF
and 0.0e+NaN while you're at it.

[PATCH 1/2] Add conversions to and from struct timespec to module interface.

[Philipp Stephani]

Time values are a fundamental data type, and such conversions are hard
to implement within modules because of the various forms of time
values in Emacs Lisp.  Adding dedicated conversion functions can
significantly simplify module code dealing with times.

This approach uses nanosecond precision.  While Emacs in theory has
support for higher-precision time values, in practice most languages
and standards, such as POSIX, C, Java, and Go, have settled on
nanosecond-precision integers to represent time.

* src/emacs-module.h.in: Add header for struct timespec.

* src/module-env-27.h: Add module functions for time conversion.

* src/emacs-module.c (module_extract_time, module_make_time): New
functions.
(initialize_environment): Use them.

* test/data/emacs-module/mod-test.c (Fmod_test_add_nanosecond): New
test function.
(emacs_module_init): Define it.

* test/src/emacs-module-tests.el (mod-test-add-nanosecond/valid)
(mod-test-add-nanosecond/nil, mod-test-add-nanosecond/invalid): New
unit tests.

* doc/lispref/internals.texi (Module Values): Document time
conversion functions.
---
 doc/lispref/internals.texi        | 22 ++++++++++++++++++++++
 etc/NEWS                          |  3 +++
 src/emacs-module.c                | 17 +++++++++++++++++
 src/emacs-module.h.in             |  1 +
 src/module-env-27.h               |  6 ++++++
 test/data/emacs-module/mod-test.c | 13 +++++++++++++
 test/src/emacs-module-tests.el    | 28 ++++++++++++++++++++++++++++
 7 files changed, 90 insertions(+)

diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index 25892d4b57..fb838113bc 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -1387,6 +1387,15 @@ Module Values
 @var{arg}, as a C @code{double} value.
 @end deftypefn
 
+@deftypefn Function struct timespec extract_time (emacs_env *@var{env}, emacs_value @var{time})
+This function, which is available since Emacs 27, interprets
+@var{time} as an Emacs time value and returns the corresponding
+@code{struct timespec}.  @xref{Time of Day}.  This function signals an
+error if @var{time} is out of range for @code{struct timespec}.  If
+@var{time} has higher precision than nanoseconds, then this function
+truncates it to nanosecond precision.
+@end deftypefn
+
 @deftypefn Function bool copy_string_contents (emacs_env *@var{env}, emacs_value @var{arg}, char *@var{buf}, ptrdiff_t *@var{len})
 This function stores the UTF-8 encoded text of a Lisp string specified
 by @var{arg} in the array of @code{char} pointed by @var{buf}, which
@@ -1452,6 +1461,19 @@ Module Values
 corresponding Emacs floating-point value.
 @end deftypefn
 
+@deftypefn Function emacs_value make_time (emacs_env *@var{env}, struct timespec @var{time})
+This function, which is available since Emacs 27, takes a @code{struct
+timespec} argument @var{time} and returns the corresponding Emacs
+timestamp.  @xref{Time of Day} for the possible return value formats.
+It is not specified in which timestamp format the time is returned,
+but it is always a valid Emacs timestamp.  The return value is exactly
+the same timestamp as @var{time}: all input values are representable,
+and there is never a loss of precision.  @code{time.tv_sec} and
+@code{time.tv_nsec} can be arbitrary values.  In particular, there's
+no requirement that @var{time} be normalized.  This means that
+@code{time.tv_nsec} doesn't have to be in the range [0, 999999999].
+@end deftypefn
+
 @deftypefn Function emacs_value make_string (emacs_env *@var{env}, const char *@var{str}, ptrdiff_t @var{strlen})
 This function creates an Emacs string from C text string pointed by
 @var{str} whose length in bytes, not including the terminating null
diff --git a/etc/NEWS b/etc/NEWS
index b13ab47768..2534262b62 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1910,6 +1910,9 @@ returns a regexp that never matches anything, which is an identity for
 this operation.  Previously, the empty string was returned in this
 case.
 
+** New module environment functions 'make_time' and 'extract_time' to
+convert between timespec structures and Emacs time values.
+
 \f
 * Changes in Emacs 27.1 on Non-Free Operating Systems
 
diff --git a/src/emacs-module.c b/src/emacs-module.c
index d7704efcf6..b798789351 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -74,6 +74,7 @@ To add a new module function, proceed as follows:
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <time.h>
 
 #include "lisp.h"
 #include "dynlib.h"
@@ -734,6 +735,20 @@ module_process_input (emacs_env *env)
   return emacs_process_input_continue;
 }
 
+static struct timespec
+module_extract_time (emacs_env *env, emacs_value value)
+{
+  MODULE_FUNCTION_BEGIN ((struct timespec) {0});
+  return lisp_time_argument (value_to_lisp (value));
+}
+
+static emacs_value
+module_make_time (emacs_env *env, struct timespec time)
+{
+  MODULE_FUNCTION_BEGIN (NULL);
+  return lisp_to_value (env, make_lisp_time (time));
+}
+
 \f
 /* Subroutines.  */
 
@@ -1137,6 +1152,8 @@ initialize_environment (emacs_env *env, struct emacs_env_private *priv)
   env->vec_size = module_vec_size;
   env->should_quit = module_should_quit;
   env->process_input = module_process_input;
+  env->extract_time = module_extract_time;
+  env->make_time = module_make_time;
   Vmodule_environments = Fcons (make_mint_ptr (env), Vmodule_environments);
   return env;
 }
diff --git a/src/emacs-module.h.in b/src/emacs-module.h.in
index 009d1583fe..bfbe226dd9 100644
--- a/src/emacs-module.h.in
+++ b/src/emacs-module.h.in
@@ -22,6 +22,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 
 #include <stdint.h>
 #include <stddef.h>
+#include <time.h>
 
 #ifndef __cplusplus
 #include <stdbool.h>
diff --git a/src/module-env-27.h b/src/module-env-27.h
index b491b60fbb..e63843f8d6 100644
--- a/src/module-env-27.h
+++ b/src/module-env-27.h
@@ -2,3 +2,9 @@
      function should quit.  */
   enum emacs_process_input_result (*process_input) (emacs_env *env)
     EMACS_ATTRIBUTE_NONNULL (1);
+
+  struct timespec (*extract_time) (emacs_env *env, emacs_value value)
+    EMACS_ATTRIBUTE_NONNULL (1);
+
+  emacs_value (*make_time) (emacs_env *env, struct timespec time)
+    EMACS_ATTRIBUTE_NONNULL (1);
diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c
index a39e41afee..dbdbfecfe6 100644
--- a/test/data/emacs-module/mod-test.c
+++ b/test/data/emacs-module/mod-test.c
@@ -366,6 +366,18 @@ Fmod_test_sleep_until (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
   return env->intern (env, "finished");
 }
 
+static emacs_value
+Fmod_test_add_nanosecond (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
+                          void *data)
+{
+  assert (nargs == 1);
+  struct timespec time = env->extract_time (env, args[0]);
+  assert (time.tv_nsec >= 0);
+  assert (time.tv_nsec < 2000000000);  /* possible leap second */
+  time.tv_nsec++;
+  return env->make_time (env, time);
+}
+
 /* Lisp utilities for easier readability (simple wrappers).  */
 
 /* Provide FEATURE to Emacs.  */
@@ -434,6 +446,7 @@ emacs_module_init (struct emacs_runtime *ert)
   DEFUN ("mod-test-invalid-finalizer", Fmod_test_invalid_finalizer, 0, 0,
          NULL, NULL);
   DEFUN ("mod-test-sleep-until", Fmod_test_sleep_until, 2, 2, NULL, NULL);
+  DEFUN ("mod-test-add-nanosecond", Fmod_test_add_nanosecond, 1, 1, NULL, NULL);
 
 #undef DEFUN
 
diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el
index 35aaaa64b6..eea4c61165 100644
--- a/test/src/emacs-module-tests.el
+++ b/test/src/emacs-module-tests.el
@@ -310,4 +310,32 @@ module--test-assertion
                       'finished))
         (quit)))))
 
+(ert-deftest mod-test-add-nanosecond/valid ()
+  (dolist (input (list
+                  ;; Some realistic examples.
+                  (current-time) (time-to-seconds)
+                  (encode-time 12 34 5 6 7 2019 t)
+                  ;; Various legacy timestamp forms.
+                  '(123 456) '(123 456 789) '(123 456 789 6000)
+                  ;; Corner case: this will result in a nanosecond
+                  ;; value of 1000000000 after addition.  The module
+                  ;; code should handle this correctly.
+                  '(123 65535 999999 999000)
+                  ;; Seconds since the epoch.
+                  123 123.45
+                  ;; New (TICKS . HZ) format.
+                  '(123456789 . 1000000000)))
+    (ert-info ((format "input: %s" input))
+      (should (time-equal-p (mod-test-add-nanosecond input)
+                            (time-add input '(0 0 0 1000)))))))
+
+(ert-deftest mod-test-add-nanosecond/nil ()
+  (should (<= (float-time (mod-test-add-nanosecond nil))
+              (+ (float-time) 1e-9))))
+
+(ert-deftest mod-test-add-nanosecond/invalid ()
+  (dolist (input '(1.0e+INF 1.0e-INF 0.0e+NaN (123) (123.45 6 7) "foo" [1 2]))
+    (ert-info ((format "input: %s" input))
+      (should-error (mod-test-add-nanosecond input)))))
+
 ;;; emacs-module-tests.el ends here
-- 
2.20.1 (Apple Git-117)

Re: [PATCH 1/2] Add conversions to and from struct timespec to module interface.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Cc: Philipp Stephani <phst@google.com>
> Date: Tue, 23 Apr 2019 23:32:17 +0200
> 
> +@deftypefn Function struct timespec extract_time (emacs_env *@var{env}, emacs_value @var{time})
> +This function, which is available since Emacs 27, interprets
> +@var{time} as an Emacs time value and returns the corresponding
> +@code{struct timespec}.  @xref{Time of Day}.

I think we need to tell something about 'struct timespec' here.
Ideally, simply describe its members, unless they are too platform
dependent.  This text is for C programmers, who will need to look up
the struct to be able to use this function.

>                                            This function signals an
> +error if @var{time} is out of range for @code{struct timespec}.

Instead of "out of range for", I'd prefer saying "cannot be
represented by", perhaps with an example.  "Out of range" is a
complicated subject when talking about time values, and we shouldn't
assume the reader is an expert on time handling.

>                                                                    If
> +@var{time} has higher precision than nanoseconds, then this function
> +truncates it to nanosecond precision.

If we describe 'struct timespec', this sentence will be redundant, I
think.  (Btw, "truncates" or "rounds", and if the former, why not the
latter?)

> +This function, which is available since Emacs 27, takes a @code{struct
> +timespec} argument @var{time} and returns the corresponding Emacs
> +timestamp.  @xref{Time of Day} for the possible return value formats.
                                 ^
There should be a comma there, or makeinfo will produce a warning.

> +It is not specified in which timestamp format the time is returned,
> +but it is always a valid Emacs timestamp.

I question the wisdom of such an obscurity.  There are no secrets
here, as the source code is available, and I can envision use cases
where the programmer does really need to know the structure of the
returned timestamp.  Why should we make this "unspecified"?

>                                           The return value is exactly
> +the same timestamp as @var{time}: all input values are representable,
> +and there is never a loss of precision.  @code{time.tv_sec} and
> +@code{time.tv_nsec} can be arbitrary values.

Here you reference members of 'struct timespec' without describing the
struct first, which I think is confusing.  One more reason to describe
the struct explcitly

> +@code{time.tv_nsec} doesn't have to be in the range [0, 999999999].
                                                       ^^^^^^^^^^^^^^
This should be in @code, and I think using @dots{} instead of the
comma will look better in print.

> +** New module environment functions 'make_time' and 'extract_time' to
> +convert between timespec structures and Emacs time values.
                                           ^^^^^^^^^^^^^^^^^
"Emacs Lisp time values", I'd say.

Thanks.

Re: [PATCH 1/2] Add conversions to and from struct timespec to module interface.

[Philipp Stephani]

Thanks, I've addressed these comments. Since they were minor, I've
pushed the result to master as commit bffceab633.

Am Mi., 24. Apr. 2019 um 09:21 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Cc: Philipp Stephani <phst@google.com>
> > Date: Tue, 23 Apr 2019 23:32:17 +0200
> >
> > +@deftypefn Function struct timespec extract_time (emacs_env *@var{env}, emacs_value @var{time})
> > +This function, which is available since Emacs 27, interprets
> > +@var{time} as an Emacs time value and returns the corresponding
> > +@code{struct timespec}.  @xref{Time of Day}.
>
> I think we need to tell something about 'struct timespec' here.
> Ideally, simply describe its members, unless they are too platform
> dependent.  This text is for C programmers, who will need to look up
> the struct to be able to use this function.

I've documented the structure members and added a reference to the
libc manual. Fortunately the members are standardized by both C and
Posix.

> >                                                                    If
> > +@var{time} has higher precision than nanoseconds, then this function
> > +truncates it to nanosecond precision.
>
> If we describe 'struct timespec', this sentence will be redundant, I
> think.

Not really, there could be other options (e.g. signaling an error).

>  (Btw, "truncates" or "rounds", and if the former, why not the
> latter?)

I think that's just the behavior of lisp_time_argument. I'll see that
I can add further unit tests and improve the documentation.

>
> > +This function, which is available since Emacs 27, takes a @code{struct
> > +timespec} argument @var{time} and returns the corresponding Emacs
> > +timestamp.  @xref{Time of Day} for the possible return value formats.
>                                  ^
> There should be a comma there, or makeinfo will produce a warning.

I don't see any warning, and I think adding a comma there wouldn't be correct.

>
> > +It is not specified in which timestamp format the time is returned,
> > +but it is always a valid Emacs timestamp.
>
> I question the wisdom of such an obscurity.  There are no secrets
> here, as the source code is available, and I can envision use cases
> where the programmer does really need to know the structure of the
> returned timestamp.  Why should we make this "unspecified"?

To be able to change the format if we wish to do so later.
However, the current (TICKS . HZ) representation seems stable enough,
so I've documented it.

> > +@code{time.tv_nsec} doesn't have to be in the range [0, 999999999].
>                                                        ^^^^^^^^^^^^^^
> This should be in @code, and I think using @dots{} instead of the
> comma will look better in print.

This was meant to be a mathematical closed interval. Since people
might not be familiar with that notation, I've reworded it.

Re: [PATCH 1/2] Add conversions to and from struct timespec to module interface.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 24 Apr 2019 13:03:21 +0200
> Cc: Paul Eggert <eggert@cs.ucla.edu>, Emacs developers <emacs-devel@gnu.org>, 
> 	Philipp Stephani <phst@google.com>
> 
> > > +This function, which is available since Emacs 27, takes a @code{struct
> > > +timespec} argument @var{time} and returns the corresponding Emacs
> > > +timestamp.  @xref{Time of Day} for the possible return value formats.
> >                                  ^
> > There should be a comma there, or makeinfo will produce a warning.
> 
> I don't see any warning, and I think adding a comma there wouldn't be correct.

You don't see a warning because latest versions of Texinfo stopped
emitting them.

Thanks for the other fixes.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Paul Eggert]

Thanks, these two patches both look good to me. That was quick work!

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Cc: Philipp Stephani <phst@google.com>
> Date: Tue, 23 Apr 2019 23:32:18 +0200
> 
> * src/module-env-27.h: Add new module functions to convert big
> integers.
> 
> * src/emacs-module.h.in (emacs_mpz): Define if GMP is available.
> 
> * src/emacs-module.c (module_extract_big_integer)
> (module_make_big_integer): New functions.
> (initialize_environment): Use them.
> 
> * test/data/emacs-module/mod-test.c (Fmod_test_double): New test
> function.
> (emacs_module_init): Define it.
> 
> * test/src/emacs-module-tests.el (mod-test-double): New unit test.
> 
> * doc/lispref/internals.texi (Module Values): Document new functions.

These changes break the Emacs build on platforms where GMP is not
available and Emacs uses mini-gmp.c.  In particular, lisp.h amd
emacs-module.c unconditionally define EMACS_MODULE_GMP, which then
causes emacs-module.h include gmp.h, whose defionitions conflict with
mini-gmp.h.

It is also not clear whether the new emacs-module facilities are
compatible with mini-gmp, i.e. whether mini-gmp can support the above
interfaces in emacs-module.  because if they can, we have a problem:
emacs-module.h cannot include mini-gmp.h, AFAIU, where the relevant
structures and macros are defined instead of system-wide gmp.h.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

Am Mi., 24. Apr. 2019 um 18:03 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Cc: Philipp Stephani <phst@google.com>
> > Date: Tue, 23 Apr 2019 23:32:18 +0200
> >
> > * src/module-env-27.h: Add new module functions to convert big
> > integers.
> >
> > * src/emacs-module.h.in (emacs_mpz): Define if GMP is available.
> >
> > * src/emacs-module.c (module_extract_big_integer)
> > (module_make_big_integer): New functions.
> > (initialize_environment): Use them.
> >
> > * test/data/emacs-module/mod-test.c (Fmod_test_double): New test
> > function.
> > (emacs_module_init): Define it.
> >
> > * test/src/emacs-module-tests.el (mod-test-double): New unit test.
> >
> > * doc/lispref/internals.texi (Module Values): Document new functions.
>
> These changes break the Emacs build on platforms where GMP is not
> available and Emacs uses mini-gmp.c.  In particular, lisp.h amd
> emacs-module.c unconditionally define EMACS_MODULE_GMP, which then
> causes emacs-module.h include gmp.h, whose defionitions conflict with
> mini-gmp.h.
>
> It is also not clear whether the new emacs-module facilities are
> compatible with mini-gmp, i.e. whether mini-gmp can support the above
> interfaces in emacs-module.  because if they can, we have a problem:
> emacs-module.h cannot include mini-gmp.h, AFAIU, where the relevant
> structures and macros are defined instead of system-wide gmp.h.

Thanks, I'll see whether I can make this work.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 24 Apr 2019 18:37:53 +0200
> Cc: Philipp Stephani <phst@google.com>, Paul Eggert <eggert@cs.ucla.edu>,
> 	Emacs developers <emacs-devel@gnu.org>
> 
> Thanks, I'll see whether I can make this work.

Thanks.

In any case, I think the unconditional inclusion of gmp.h in
emacs-module.c should go, because emacs-module.h already does that,
conditionally.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

Am Mi., 24. Apr. 2019 um 18:37 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Mi., 24. Apr. 2019 um 18:03 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> >
> > > From: Philipp Stephani <p.stephani2@gmail.com>
> > > Cc: Philipp Stephani <phst@google.com>
> > > Date: Tue, 23 Apr 2019 23:32:18 +0200
> > >
> > > * src/module-env-27.h: Add new module functions to convert big
> > > integers.
> > >
> > > * src/emacs-module.h.in (emacs_mpz): Define if GMP is available.
> > >
> > > * src/emacs-module.c (module_extract_big_integer)
> > > (module_make_big_integer): New functions.
> > > (initialize_environment): Use them.
> > >
> > > * test/data/emacs-module/mod-test.c (Fmod_test_double): New test
> > > function.
> > > (emacs_module_init): Define it.
> > >
> > > * test/src/emacs-module-tests.el (mod-test-double): New unit test.
> > >
> > > * doc/lispref/internals.texi (Module Values): Document new functions.
> >
> > These changes break the Emacs build on platforms where GMP is not
> > available and Emacs uses mini-gmp.c.  In particular, lisp.h amd
> > emacs-module.c unconditionally define EMACS_MODULE_GMP, which then
> > causes emacs-module.h include gmp.h, whose defionitions conflict with
> > mini-gmp.h.
> >
> > It is also not clear whether the new emacs-module facilities are
> > compatible with mini-gmp, i.e. whether mini-gmp can support the above
> > interfaces in emacs-module.  because if they can, we have a problem:
> > emacs-module.h cannot include mini-gmp.h, AFAIU, where the relevant
> > structures and macros are defined instead of system-wide gmp.h.
>
> Thanks, I'll see whether I can make this work.

Right now I see 2 options:

1. Don't support these functions at all if GMP isn't available (i.e.
signal an error unconditionally)
2. Add another preprocessor macro that would stop emacs-module.h from
including gmp.h.

(1) seems a bit easier and cleaner, but means that we wouldn't support
these functions if Emacs isn't compiled with GMP support.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 24 Apr 2019 18:57:40 +0200
> Cc: Paul Eggert <eggert@cs.ucla.edu>, Emacs developers <emacs-devel@gnu.org>, 
> 	Philipp Stephani <phst@google.com>
> 
> Right now I see 2 options:
> 
> 1. Don't support these functions at all if GMP isn't available (i.e.
> signal an error unconditionally)
> 2. Add another preprocessor macro that would stop emacs-module.h from
> including gmp.h.
> 
> (1) seems a bit easier and cleaner, but means that we wouldn't support
> these functions if Emacs isn't compiled with GMP support.

I don't think I understand (2): how would you declare GMP-related data
types used in emacs-module.c, if you don't include gmp.h?

And there might be option (3): have emacs-module.h duplicate the
portion of mini-gmp.h that are needed when gmp.h isn't available.
It's a bit inelegant, and maintenance burden, but maybe not grave
enough to decide that wer cannot support bignums without a real GMP.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

Am Mi., 24. Apr. 2019 um 19:11 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Wed, 24 Apr 2019 18:57:40 +0200
> > Cc: Paul Eggert <eggert@cs.ucla.edu>, Emacs developers <emacs-devel@gnu.org>,
> >       Philipp Stephani <phst@google.com>
> >
> > Right now I see 2 options:
> >
> > 1. Don't support these functions at all if GMP isn't available (i.e.
> > signal an error unconditionally)
> > 2. Add another preprocessor macro that would stop emacs-module.h from
> > including gmp.h.
> >
> > (1) seems a bit easier and cleaner, but means that we wouldn't support
> > these functions if Emacs isn't compiled with GMP support.
>
> I don't think I understand (2): how would you declare GMP-related data
> types used in emacs-module.c, if you don't include gmp.h?

It would require the user (or rather, Emacs) to include gmp.h or
mini-gmp.h before including emacs-module.h. I.e.:

#ifndef EMACS_MODULE_DO_NOT_INCLUDE_GMP_H
#include <gmp.h>
#endif

>
> And there might be option (3): have emacs-module.h duplicate the
> portion of mini-gmp.h that are needed when gmp.h isn't available.
> It's a bit inelegant, and maintenance burden, but maybe not grave
> enough to decide that wer cannot support bignums without a real GMP.

I don't think we should replicate internal data structures of
third-party libraries. It's really ugly.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 24 Apr 2019 19:15:13 +0200
> Cc: Paul Eggert <eggert@cs.ucla.edu>, Emacs developers <emacs-devel@gnu.org>, 
> 	Philipp Stephani <phst@google.com>
> 
> > > 1. Don't support these functions at all if GMP isn't available (i.e.
> > > signal an error unconditionally)
> > > 2. Add another preprocessor macro that would stop emacs-module.h from
> > > including gmp.h.
> > >
> > > (1) seems a bit easier and cleaner, but means that we wouldn't support
> > > these functions if Emacs isn't compiled with GMP support.
> >
> > I don't think I understand (2): how would you declare GMP-related data
> > types used in emacs-module.c, if you don't include gmp.h?
> 
> It would require the user (or rather, Emacs) to include gmp.h or
> mini-gmp.h before including emacs-module.h. I.e.:
> 
> #ifndef EMACS_MODULE_DO_NOT_INCLUDE_GMP_H
> #include <gmp.h>
> #endif

Ah, okay.  But then what would emacs-module.c do with these 2 new APIs
if gmp.h was not included?

> > And there might be option (3): have emacs-module.h duplicate the
> > portion of mini-gmp.h that are needed when gmp.h isn't available.
> > It's a bit inelegant, and maintenance burden, but maybe not grave
> > enough to decide that wer cannot support bignums without a real GMP.
> 
> I don't think we should replicate internal data structures of
> third-party libraries. It's really ugly.

mini-gmp.h is not third-party, it's Emacs.

The problem I'm struggling with is how can we defend lack of support
of bignums in modules when Emacs itself does support them?

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

Am Mi., 24. Apr. 2019 um 19:23 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Wed, 24 Apr 2019 19:15:13 +0200
> > Cc: Paul Eggert <eggert@cs.ucla.edu>, Emacs developers <emacs-devel@gnu.org>,
> >       Philipp Stephani <phst@google.com>
> >
> > > > 1. Don't support these functions at all if GMP isn't available (i.e.
> > > > signal an error unconditionally)
> > > > 2. Add another preprocessor macro that would stop emacs-module.h from
> > > > including gmp.h.
> > > >
> > > > (1) seems a bit easier and cleaner, but means that we wouldn't support
> > > > these functions if Emacs isn't compiled with GMP support.
> > >
> > > I don't think I understand (2): how would you declare GMP-related data
> > > types used in emacs-module.c, if you don't include gmp.h?
> >
> > It would require the user (or rather, Emacs) to include gmp.h or
> > mini-gmp.h before including emacs-module.h. I.e.:
> >
> > #ifndef EMACS_MODULE_DO_NOT_INCLUDE_GMP_H
> > #include <gmp.h>
> > #endif
>
> Ah, okay.  But then what would emacs-module.c do with these 2 new APIs
> if gmp.h was not included?

It would include mini-gmp.h instead before including emacs-module.h.
I'm testing this right now, but I think it should work.

>
> > > And there might be option (3): have emacs-module.h duplicate the
> > > portion of mini-gmp.h that are needed when gmp.h isn't available.
> > > It's a bit inelegant, and maintenance burden, but maybe not grave
> > > enough to decide that wer cannot support bignums without a real GMP.
> >
> > I don't think we should replicate internal data structures of
> > third-party libraries. It's really ugly.
>
> mini-gmp.h is not third-party, it's Emacs.

Still, replicating internal data structures and making sure that they
stay ABI-compatible is too risky and too much of a burden.

>
> The problem I'm struggling with is how can we defend lack of support
> of bignums in modules when Emacs itself does support them?

I'd definitely prefer supporting bignums if possible. The question is
just whether it's possible.

[PATCH] Unbreak build when building without GMP support.

[Philipp Stephani]

Add support for a new preprocessor macro EMACS_MODULE_HAVE_MPZ_T to
emacs-module.h.  If this macro is defined, assume that mpz_t is
already defined and don’t include gmp.h.

Don’t document the new macro for now, as it’s unclear whether we want
to support this in modules outside the Emacs tree.

* src/emacs-module.h.in: Allow user to prevent inclusion of gmp.h.

* src/emacs-module.c: Use mini-gmp if GMP is unavailable.  Don’t
include gmp.h.

* src/lisp.h: Don’t require gmp.h.  It’s not needed for lisp.h.

* test/Makefile.in (GMP_LIB, GMP_OBJ): New variables.
($(test_module)): Use them.

* test/data/emacs-module/mod-test.c: Use mini-gmp if GMP is unavailable.
---
 src/emacs-module.c                | 7 +++++--
 src/emacs-module.h.in             | 2 +-
 src/lisp.h                        | 1 -
 test/Makefile.in                  | 4 +++-
 test/data/emacs-module/mod-test.c | 9 +++++++--
 5 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/emacs-module.c b/src/emacs-module.c
index 41ce9ef03e..65c25a0684 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -70,6 +70,11 @@ To add a new module function, proceed as follows:
 
 #include <config.h>
 
+#ifndef HAVE_GMP
+#include "mini-gmp.h"
+#define EMACS_MODULE_HAVE_MPZ_T
+#endif
+
 #define EMACS_MODULE_GMP
 #include "emacs-module.h"
 
@@ -80,8 +85,6 @@ To add a new module function, proceed as follows:
 #include <stdlib.h>
 #include <time.h>
 
-#include <gmp.h>
-
 #include "lisp.h"
 #include "bignum.h"
 #include "dynlib.h"
diff --git a/src/emacs-module.h.in b/src/emacs-module.h.in
index e61aadfc3a..fbc62a61ef 100644
--- a/src/emacs-module.h.in
+++ b/src/emacs-module.h.in
@@ -28,7 +28,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <stdbool.h>
 #endif
 
-#ifdef EMACS_MODULE_GMP
+#if defined EMACS_MODULE_GMP && !defined EMACS_MODULE_HAVE_MPZ_T
 #include <gmp.h>
 #endif
 
diff --git a/src/lisp.h b/src/lisp.h
index 703fe76d64..d803f16000 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4151,7 +4151,6 @@ extern void *unexec_realloc (void *, size_t);
 extern void unexec_free (void *);
 #endif
 
-#define EMACS_MODULE_GMP
 #include "emacs-module.h"
 
 /* Function prototype for the module Lisp functions.  */
diff --git a/test/Makefile.in b/test/Makefile.in
index ce6ce04b8b..6d812818f8 100644
--- a/test/Makefile.in
+++ b/test/Makefile.in
@@ -256,6 +256,8 @@ FPIC_CFLAGS =
 
 HYBRID_MALLOC = @HYBRID_MALLOC@
 LIBEGNU_ARCHIVE = ../lib/lib$(if $(HYBRID_MALLOC),e)gnu.a
+GMP_LIB = @GMP_LIB@
+GMP_OBJ = ../src/@GMP_OBJ@
 
 # Note: emacs-module.h is generated from emacs-module.h.in, hence we
 # look in ../src, not $(srcdir)/../src.
@@ -268,7 +270,7 @@ src/emacs-module-tests.log:
 $(test_module): $(test_module:${SO}=.c) ../src/emacs-module.h $(LIBEGNU_ARCHIVE)
 	$(AM_V_at)${MKDIR_P} $(dir $@)
 	$(AM_V_CCLD)$(CC) -shared $(CPPFLAGS) $(MODULE_CFLAGS) $(LDFLAGS) \
-	  -o $@ $< $(LIBEGNU_ARCHIVE)
+	  -o $@ $< $(LIBEGNU_ARCHIVE) $(GMP_LIB) $(GMP_OBJ)
 endif
 
 ## Check that there is no 'automated' subdirectory, which would
diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c
index 8ac08f7153..b7007bd80f 100644
--- a/test/data/emacs-module/mod-test.c
+++ b/test/data/emacs-module/mod-test.c
@@ -27,11 +27,16 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <string.h>
 #include <time.h>
 
+#ifdef HAVE_GMP
+#include <gmp.h>
+#else
+#include "mini-gmp.h"
+#define EMACS_MODULE_HAVE_MPZ_T
+#endif
+
 #define EMACS_MODULE_GMP
 #include <emacs-module.h>
 
-#include <gmp.h>
-
 #include "timespec.h"
 
 int plugin_is_GPL_compatible;
-- 
2.20.1 (Apple Git-117)

Re: [PATCH] Unbreak build when building without GMP support.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 24 Apr 2019 19:51:05 +0200
> Cc: Philipp Stephani <phst@google.com>
> 
> diff --git a/src/lisp.h b/src/lisp.h
> index 703fe76d64..d803f16000 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -4151,7 +4151,6 @@ extern void *unexec_realloc (void *, size_t);
>  extern void unexec_free (void *);
>  #endif
>  
> -#define EMACS_MODULE_GMP
>  #include "emacs-module.h"

Btw, why are we including emacs-module.h in lisp.h?  IME, this usually
causes trouble, so it is best to avoid that.

Re: [PATCH] Unbreak build when building without GMP support.

[Philipp Stephani]

Am Mi., 24. Apr. 2019 um 20:41 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Wed, 24 Apr 2019 19:51:05 +0200
> > Cc: Philipp Stephani <phst@google.com>
> >
> > diff --git a/src/lisp.h b/src/lisp.h
> > index 703fe76d64..d803f16000 100644
> > --- a/src/lisp.h
> > +++ b/src/lisp.h
> > @@ -4151,7 +4151,6 @@ extern void *unexec_realloc (void *, size_t);
> >  extern void unexec_free (void *);
> >  #endif
> >
> > -#define EMACS_MODULE_GMP
> >  #include "emacs-module.h"
>
> Btw, why are we including emacs-module.h in lisp.h?  IME, this usually
> causes trouble, so it is best to avoid that.

lisp.h needs types from emacs-module.h to define Lisp_Module_Function.

Re: [PATCH] Unbreak build when building without GMP support.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 24 Apr 2019 20:49:34 +0200
> Cc: Emacs developers <emacs-devel@gnu.org>, Philipp Stephani <phst@google.com>
> 
> > Btw, why are we including emacs-module.h in lisp.h?  IME, this usually
> > causes trouble, so it is best to avoid that.
> 
> lisp.h needs types from emacs-module.h to define Lisp_Module_Function.

A single data type?  I'd rather move XMODULE_FUNCTION to
emacs-module.c, and the data type definition and all the other related
stuff to emacs-module.h.  It isn't worth the hassle.

Re: [PATCH] Unbreak build when building without GMP support.

[Philipp Stephani]

Am Mi., 24. Apr. 2019 um 21:06 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Wed, 24 Apr 2019 20:49:34 +0200
> > Cc: Emacs developers <emacs-devel@gnu.org>, Philipp Stephani <phst@google.com>
> >
> > > Btw, why are we including emacs-module.h in lisp.h?  IME, this usually
> > > causes trouble, so it is best to avoid that.
> >
> > lisp.h needs types from emacs-module.h to define Lisp_Module_Function.
>
> A single data type?  I'd rather move XMODULE_FUNCTION to
> emacs-module.c, and the data type definition and all the other related
> stuff to emacs-module.h.  It isn't worth the hassle.

We shouldn't move anything to emacs-module.h because it's a public
header, to be used outside of Emacs.
What hassle are you talking about? There is no hassle. We have been
including emacs-module.h in lisp.h for years, and there was never any
problem.

Re: [PATCH] Unbreak build when building without GMP support.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 24 Apr 2019 21:19:54 +0200
> Cc: Emacs developers <emacs-devel@gnu.org>, Philipp Stephani <phst@google.com>
> 
> Am Mi., 24. Apr. 2019 um 21:06 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> >
> > > From: Philipp Stephani <p.stephani2@gmail.com>
> > > Date: Wed, 24 Apr 2019 20:49:34 +0200
> > > Cc: Emacs developers <emacs-devel@gnu.org>, Philipp Stephani <phst@google.com>
> > >
> > > > Btw, why are we including emacs-module.h in lisp.h?  IME, this usually
> > > > causes trouble, so it is best to avoid that.
> > >
> > > lisp.h needs types from emacs-module.h to define Lisp_Module_Function.
> >
> > A single data type?  I'd rather move XMODULE_FUNCTION to
> > emacs-module.c, and the data type definition and all the other related
> > stuff to emacs-module.h.  It isn't worth the hassle.
> 
> We shouldn't move anything to emacs-module.h because it's a public
> header, to be used outside of Emacs.

Then keep them in lisp.h, but make them self-sufficient.  Which types
does Lisp_Module_Function need that are defined by emacs-module.h?

> What hassle are you talking about? There is no hassle. We have been
> including emacs-module.h in lisp.h for years, and there was never any
> problem.

I had one just today.  And the patch you are now proposing includes a
time bomb: files that include lisp.h will have emacs-module.h with a
different definition of emacs_mpz than those which include
emacs-module.h directly, depending on whether they do or don't define
EMACS_MODULE_GMP etc.  It's confusing and error prone.  The only other
header lisp.h includes, thread.h, was also a source of many
compilation problems, and will be again if we change the thread
machinery.

Re: [PATCH] Unbreak build when building without GMP support.

[Philipp Stephani]

Am Mi., 24. Apr. 2019 um 21:31 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Wed, 24 Apr 2019 21:19:54 +0200
> > Cc: Emacs developers <emacs-devel@gnu.org>, Philipp Stephani <phst@google.com>
> >
> > Am Mi., 24. Apr. 2019 um 21:06 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> > >
> > > > From: Philipp Stephani <p.stephani2@gmail.com>
> > > > Date: Wed, 24 Apr 2019 20:49:34 +0200
> > > > Cc: Emacs developers <emacs-devel@gnu.org>, Philipp Stephani <phst@google.com>
> > > >
> > > > > Btw, why are we including emacs-module.h in lisp.h?  IME, this usually
> > > > > causes trouble, so it is best to avoid that.
> > > >
> > > > lisp.h needs types from emacs-module.h to define Lisp_Module_Function.
> > >
> > > A single data type?  I'd rather move XMODULE_FUNCTION to
> > > emacs-module.c, and the data type definition and all the other related
> > > stuff to emacs-module.h.  It isn't worth the hassle.
> >
> > We shouldn't move anything to emacs-module.h because it's a public
> > header, to be used outside of Emacs.
>
> Then keep them in lisp.h, but make them self-sufficient.  Which types
> does Lisp_Module_Function need that are defined by emacs-module.h?

Turns out that it's relatively straightforward to move the structure
type itself out of lisp.h, so I've just did that.

Re: [PATCH] Unbreak build when building without GMP support.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 24 Apr 2019 23:15:42 +0200
> Cc: Emacs developers <emacs-devel@gnu.org>, Philipp Stephani <phst@google.com>
> 
> Turns out that it's relatively straightforward to move the structure
> type itself out of lisp.h, so I've just did that.

Thanks, but can you point me to the commit which does that?  Or did
you not yet install it?

Re: [PATCH] Unbreak build when building without GMP support.

[Eli Zaretskii]

> Date: Thu, 25 Apr 2019 09:04:22 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: phst@google.com, emacs-devel@gnu.org
> 
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Wed, 24 Apr 2019 23:15:42 +0200
> > Cc: Emacs developers <emacs-devel@gnu.org>, Philipp Stephani <phst@google.com>
> > 
> > Turns out that it's relatively straightforward to move the structure
> > type itself out of lisp.h, so I've just did that.
> 
> Thanks, but can you point me to the commit which does that?  Or did
> you not yet install it?

Forget it, I found it.

Thanks again for this simplification.

Re: [PATCH] Unbreak build when building without GMP support.

[Philipp Stephani]

Am Do., 25. Apr. 2019 um 08:39 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > Date: Thu, 25 Apr 2019 09:04:22 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: phst@google.com, emacs-devel@gnu.org
> >
> > > From: Philipp Stephani <p.stephani2@gmail.com>
> > > Date: Wed, 24 Apr 2019 23:15:42 +0200
> > > Cc: Emacs developers <emacs-devel@gnu.org>, Philipp Stephani <phst@google.com>
> > >
> > > Turns out that it's relatively straightforward to move the structure
> > > type itself out of lisp.h, so I've just did that.
> >
> > Thanks, but can you point me to the commit which does that?  Or did
> > you not yet install it?
>
> Forget it, I found it.
>
> Thanks again for this simplification.

For reference, it's commit d2e1bac478.

Re: [PATCH] Unbreak build when building without GMP support.

[Philipp Stephani]

Am Mi., 24. Apr. 2019 um 19:50 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Add support for a new preprocessor macro EMACS_MODULE_HAVE_MPZ_T to
> emacs-module.h.  If this macro is defined, assume that mpz_t is
> already defined and don’t include gmp.h.

In the interest of unbreaking CI, I've installed this patch as 4eb7f9ef59.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Stefan Monnier]

I'm not sure I understand the details of the discussion, but if we
sometimes include mini-gmp.h (or copies of parts of it) and sometimes
gmp.h, isn't there a risk that a module built for "Emacs on
amd64" will not work on "all" versions of "Emacs on amd64" (depending
on whether it was built with GMP or with mini-gmp)?


        Stefan

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Paul Eggert]

On 4/24/19 12:44 PM, Stefan Monnier wrote:
> I'm not sure I understand the details of the discussion, but if we
> sometimes include mini-gmp.h (or copies of parts of it) and sometimes
> gmp.h, isn't there a risk that a module built for "Emacs on
> amd64" will not work on "all" versions of "Emacs on amd64" (depending
> on whether it was built with GMP or with mini-gmp)?

Yes, although the module fingerprint is supposed to catch problems like
that. Such problems can also be caused by compiling with -O2 vs -O3,
say. That is, pretty much any change to your build procedure will cause
the fingerprint to change so that Emacs won't load your old
already-compiled modules.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Stefan Monnier]

>> I'm not sure I understand the details of the discussion, but if we
>> sometimes include mini-gmp.h (or copies of parts of it) and sometimes
>> gmp.h, isn't there a risk that a module built for "Emacs on
>> amd64" will not work on "all" versions of "Emacs on amd64" (depending
>> on whether it was built with GMP or with mini-gmp)?
>
> Yes, although the module fingerprint is supposed to catch problems
> like that.  Such problems can also be caused by compiling with -O2
> vs -O3, say.

Really?  I thought our module system was designed so modules aren't so
tightly coupled to Emacs's internals [ I remember discussions
around exposing the size of Lisp_Object, for example.  ]


        Stefan

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

Am Mi., 24. Apr. 2019 um 22:58 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> >> I'm not sure I understand the details of the discussion, but if we
> >> sometimes include mini-gmp.h (or copies of parts of it) and sometimes
> >> gmp.h, isn't there a risk that a module built for "Emacs on
> >> amd64" will not work on "all" versions of "Emacs on amd64" (depending
> >> on whether it was built with GMP or with mini-gmp)?
> >
> > Yes, although the module fingerprint is supposed to catch problems
> > like that.  Such problems can also be caused by compiling with -O2
> > vs -O3, say.
>
> Really?  I thought our module system was designed so modules aren't so
> tightly coupled to Emacs's internals [ I remember discussions
> around exposing the size of Lisp_Object, for example.  ]

They are not coupled at all. They only depend on the basic machine ABI
(size and representation of arithmetic types and pointers, calling
conventions), but should otherwise be maximally compatible: If the
module author followed the recommendations from the manual, then any
version of Emacs can load any version of the module.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Paul Eggert]

On 4/24/19 2:17 PM, Philipp Stephani wrote:
> They are not coupled at all. They only depend on the basic machine ABI
> (size and representation of arithmetic types and pointers, calling
> conventions),

Ah, I mixed up module API with pdumper compatibility. Sorry about the
confusion.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

Am Mi., 24. Apr. 2019 um 21:57 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> I'm not sure I understand the details of the discussion, but if we
> sometimes include mini-gmp.h (or copies of parts of it) and sometimes
> gmp.h, isn't there a risk that a module built for "Emacs on
> amd64" will not work on "all" versions of "Emacs on amd64" (depending
> on whether it was built with GMP or with mini-gmp)?


I'd definitely hope so, otherwise we should reconsider the mini-gmp
support. Unfortunately I didn't find any statement about ABI
compatibility of mini-gmp.h.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Paul Eggert]

On 4/24/19 2:19 PM, Philipp Stephani wrote:
> I'd definitely hope so, otherwise we should reconsider the mini-gmp
> support. Unfortunately I didn't find any statement about ABI
> compatibility of mini-gmp.h.

There will be a compatibility problem if a module assumes functionality
of full GMP that mini-gmp does not provide, e.g., NAILS != 0. The full
list of functionality missing in mini-gmp is documented in the
gmp/mini-gmp/README. It's a short list. It would probably help to
mention this issue in our module documentation.

Also, the gmp.h and mini-gmp.h ABIs are incompatible on a few old Cray
machines. I don't think we need to worry about this nowadays.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 24 Apr 2019 17:00:08 -0700
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> On 4/24/19 2:19 PM, Philipp Stephani wrote:
> > I'd definitely hope so, otherwise we should reconsider the mini-gmp
> > support. Unfortunately I didn't find any statement about ABI
> > compatibility of mini-gmp.h.
> 
> There will be a compatibility problem if a module assumes functionality
> of full GMP that mini-gmp does not provide

Yes, and I think only in that case.  We should try to avoid such
features in the module API.

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

[Philipp Stephani]

Am Do., 25. Apr. 2019 um 07:33 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Paul Eggert <eggert@cs.ucla.edu>
> > Date: Wed, 24 Apr 2019 17:00:08 -0700
> > Cc: Emacs developers <emacs-devel@gnu.org>
> >
> > On 4/24/19 2:19 PM, Philipp Stephani wrote:
> > > I'd definitely hope so, otherwise we should reconsider the mini-gmp
> > > support. Unfortunately I didn't find any statement about ABI
> > > compatibility of mini-gmp.h.
> >
> > There will be a compatibility problem if a module assumes functionality
> > of full GMP that mini-gmp does not provide
>
> Yes, and I think only in that case.  We should try to avoid such
> features in the module API.

Thinking about it further, I think it might be simpler and cleaner to
just require full GMP unconditionally when building with module
support. It's not a large barrier, and I'd expect "serious" systems to
always build with GMP anyway for performance reasons.

[PATCH 1/2] Require full GMP when building module support.

[Philipp Stephani]

This allows mini-gmp to not stay binary-compatible with full GMP.

* configure.ac (HAVE_MODULES): Disable if full GMP is unavailable.
Move check below GMP check.

* src/emacs-module.h.in: Don’t check EMACS_MODULE_HAVE_MPZ_T macro.

* src/emacs-module.c:
* test/data/emacs-module/mod-test.c: Error out if full GMP is
unavailable.
---
 configure.ac                      | 109 ++++++++++++++++--------------
 src/emacs-module.c                |   6 +-
 src/emacs-module.h.in             |   2 +-
 test/data/emacs-module/mod-test.c |   3 +-
 4 files changed, 63 insertions(+), 57 deletions(-)

diff --git a/configure.ac b/configure.ac
index 810c3219e4..1f87fe0383 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3646,58 +3646,6 @@ AC_DEFUN
 fi
 AC_SUBST(LIBZ)
 
-### Dynamic modules support
-LIBMODULES=
-HAVE_MODULES=no
-MODULES_OBJ=
-case $opsys in
-  cygwin|mingw32) MODULES_SUFFIX=".dll" ;;
-  *) MODULES_SUFFIX=".so" ;;
-esac
-if test "${with_modules}" != "no"; then
-  case $opsys in
-    gnu|gnu-linux)
-      LIBMODULES="-ldl"
-      HAVE_MODULES=yes
-      ;;
-    cygwin|mingw32|darwin)
-      HAVE_MODULES=yes
-      ;;
-    *)
-      # BSD systems have dlopen in libc.
-      AC_CHECK_FUNC([dlopen], [HAVE_MODULES=yes])
-      ;;
-  esac
-
-  if test "${HAVE_MODULES}" = no; then
-    AC_MSG_ERROR([Dynamic modules are not supported on your system])
-  else
-    SAVE_LIBS=$LIBS
-    LIBS="$LIBS $LIBMODULES"
-    AC_CHECK_FUNCS([dladdr dlfunc])
-    LIBS=$SAVE_LIBS
-  fi
-fi
-
-if test "${HAVE_MODULES}" = yes; then
-   MODULES_OBJ="dynlib.o emacs-module.o"
-   AC_DEFINE(HAVE_MODULES, 1, [Define to 1 if dynamic modules are enabled])
-   AC_DEFINE_UNQUOTED(MODULES_SUFFIX, "$MODULES_SUFFIX",
-     [System extension for dynamic libraries])
-fi
-AC_SUBST(MODULES_OBJ)
-AC_SUBST(LIBMODULES)
-AC_SUBST(HAVE_MODULES)
-AC_SUBST(MODULES_SUFFIX)
-
-AC_CONFIG_FILES([src/emacs-module.h])
-AC_SUBST_FILE([module_env_snippet_25])
-AC_SUBST_FILE([module_env_snippet_26])
-AC_SUBST_FILE([module_env_snippet_27])
-module_env_snippet_25="$srcdir/src/module-env-25.h"
-module_env_snippet_26="$srcdir/src/module-env-26.h"
-module_env_snippet_27="$srcdir/src/module-env-27.h"
-
 ### Use -lpng if available, unless '--with-png=no'.
 HAVE_PNG=no
 LIBPNG=
@@ -4473,6 +4421,63 @@ AC_DEFUN
 AC_SUBST([GMP_LIB])
 AC_SUBST([GMP_OBJ])
 
+### Dynamic modules support
+LIBMODULES=
+HAVE_MODULES=no
+MODULES_OBJ=
+case $opsys in
+  cygwin|mingw32) MODULES_SUFFIX=".dll" ;;
+  *) MODULES_SUFFIX=".so" ;;
+esac
+if test "${with_modules}" != "no"; then
+  case $opsys in
+    gnu|gnu-linux)
+      LIBMODULES="-ldl"
+      HAVE_MODULES=yes
+      ;;
+    cygwin|mingw32|darwin)
+      HAVE_MODULES=yes
+      ;;
+    *)
+      # BSD systems have dlopen in libc.
+      AC_CHECK_FUNC([dlopen], [HAVE_MODULES=yes])
+      ;;
+  esac
+
+  if test "${HAVE_GMP}" != yes; then
+    # Modules require full GMP support.
+    HAVE_MODULES=no
+  fi
+
+  if test "${HAVE_MODULES}" = no; then
+    AC_MSG_ERROR([Dynamic modules are not supported on your system])
+  else
+    SAVE_LIBS=$LIBS
+    LIBS="$LIBS $LIBMODULES"
+    AC_CHECK_FUNCS([dladdr dlfunc])
+    LIBS=$SAVE_LIBS
+  fi
+fi
+
+if test "${HAVE_MODULES}" = yes; then
+   MODULES_OBJ="dynlib.o emacs-module.o"
+   AC_DEFINE(HAVE_MODULES, 1, [Define to 1 if dynamic modules are enabled])
+   AC_DEFINE_UNQUOTED(MODULES_SUFFIX, "$MODULES_SUFFIX",
+     [System extension for dynamic libraries])
+fi
+AC_SUBST(MODULES_OBJ)
+AC_SUBST(LIBMODULES)
+AC_SUBST(HAVE_MODULES)
+AC_SUBST(MODULES_SUFFIX)
+
+AC_CONFIG_FILES([src/emacs-module.h])
+AC_SUBST_FILE([module_env_snippet_25])
+AC_SUBST_FILE([module_env_snippet_26])
+AC_SUBST_FILE([module_env_snippet_27])
+module_env_snippet_25="$srcdir/src/module-env-25.h"
+module_env_snippet_26="$srcdir/src/module-env-26.h"
+module_env_snippet_27="$srcdir/src/module-env-27.h"
+
 AC_CHECK_HEADERS(valgrind/valgrind.h)
 
 AC_CHECK_MEMBERS([struct unipair.unicode], [], [], [[#include <linux/kd.h>]])
diff --git a/src/emacs-module.c b/src/emacs-module.c
index 0b7b3d6ffb..f60495844b 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -70,9 +70,11 @@ To add a new module function, proceed as follows:
 
 #include <config.h>
 
+/* We don't support mini-gmp for modules because it might not stay
+   binary-compatible with GMP proper.  */
+
 #ifndef HAVE_GMP
-#include "mini-gmp.h"
-#define EMACS_MODULE_HAVE_MPZ_T
+# error "Modules require full GMP support"
 #endif
 
 #define EMACS_MODULE_GMP
diff --git a/src/emacs-module.h.in b/src/emacs-module.h.in
index fbc62a61ef..e61aadfc3a 100644
--- a/src/emacs-module.h.in
+++ b/src/emacs-module.h.in
@@ -28,7 +28,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <stdbool.h>
 #endif
 
-#if defined EMACS_MODULE_GMP && !defined EMACS_MODULE_HAVE_MPZ_T
+#ifdef EMACS_MODULE_GMP
 #include <gmp.h>
 #endif
 
diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c
index b7007bd80f..73c8c52f7c 100644
--- a/test/data/emacs-module/mod-test.c
+++ b/test/data/emacs-module/mod-test.c
@@ -30,8 +30,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #ifdef HAVE_GMP
 #include <gmp.h>
 #else
-#include "mini-gmp.h"
-#define EMACS_MODULE_HAVE_MPZ_T
+# error "Modules require full GMP support"
 #endif
 
 #define EMACS_MODULE_GMP
-- 
2.20.1 (Apple Git-117)

[PATCH 2/2] Check for __attribute__ ((cleanup)) during configuration.

[Philipp Stephani]

It’s nicer to fail early if __attribute__ ((cleanup)) doesn’t work.

* configure.ac: Enable modules only if __attribute__ ((cleanup))
works.
---
 configure.ac | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/configure.ac b/configure.ac
index 1f87fe0383..7acd2cb6e4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4422,6 +4422,18 @@ AC_DEFUN
 AC_SUBST([GMP_OBJ])
 
 ### Dynamic modules support
+AC_CACHE_CHECK(
+  [for __attribute__ ((cleanup))],
+  [emacs_cv_have_attribute_cleanup],
+  [AC_RUN_IFELSE(
+     [AC_LANG_PROGRAM(
+       [static int status = 1;
+        static void cleanup (int *p) { status = 0; }],
+       [{ __attribute__ ((cleanup (cleanup))) int v; }
+        return status;])],
+     [emacs_cv_have_attribute_cleanup=yes],
+     [emacs_cv_have_attribute_cleanup=no])])
+
 LIBMODULES=
 HAVE_MODULES=no
 MODULES_OBJ=
@@ -4449,6 +4461,12 @@ AC_DEFUN
     HAVE_MODULES=no
   fi
 
+  if test "${emacs_cv_have_attribute_cleanup}" != yes; then
+    # The module implementation currently requires
+    # __attribute__((cleanup)).
+    HAVE_MODULES=no
+  fi
+
   if test "${HAVE_MODULES}" = no; then
     AC_MSG_ERROR([Dynamic modules are not supported on your system])
   else
-- 
2.20.1 (Apple Git-117)

Re: [PATCH 2/2] Check for __attribute__ ((cleanup)) during configuration.

[Paul Eggert]

On 4/25/19 6:46 AM, Philipp Stephani wrote:
> It’s nicer to fail early if __attribute__ ((cleanup)) doesn’t work.

Yes, but it's not nicer to make 'configure' slower than it already is.
As the slowness happens to every builder, whereas the "fail early"
problem happens only to the tiny minority of builders who configure
--with-modules but whose compilers don't support __attribute__
((cleanup)), it might be better to omit this unnecessary complication.

Re: [PATCH 2/2] Check for __attribute__ ((cleanup)) during configuration.

[Philipp Stephani]

Am Do., 25. Apr. 2019 um 23:18 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 4/25/19 6:46 AM, Philipp Stephani wrote:
> > It’s nicer to fail early if __attribute__ ((cleanup)) doesn’t work.
>
> Yes, but it's not nicer to make 'configure' slower than it already is.
> As the slowness happens to every builder, whereas the "fail early"
> problem happens only to the tiny minority of builders who configure
> --with-modules but whose compilers don't support __attribute__
> ((cleanup)), it might be better to omit this unnecessary complication.
>

Fine with me.

Re: [PATCH 1/2] Require full GMP when building module support.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Cc: emacs-devel@gnu.org,
> 	Philipp Stephani <phst@google.com>
> Date: Thu, 25 Apr 2019 15:46:08 +0200
> 
> This allows mini-gmp to not stay binary-compatible with full GMP.
> 
> * configure.ac (HAVE_MODULES): Disable if full GMP is unavailable.

Sounds too harsh to me.  After all, arbitrary-size numbers are needed
only in handful of situations.

Re: [PATCH 1/2] Require full GMP when building module support.

[Philipp Stephani]

Am Do., 25. Apr. 2019 um 16:38 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Cc: emacs-devel@gnu.org,
> >       Philipp Stephani <phst@google.com>
> > Date: Thu, 25 Apr 2019 15:46:08 +0200
> >
> > This allows mini-gmp to not stay binary-compatible with full GMP.
> >
> > * configure.ac (HAVE_MODULES): Disable if full GMP is unavailable.
>
> Sounds too harsh to me.  After all, arbitrary-size numbers are needed
> only in handful of situations.

I don't really see a good alternative. It would otherwise be
disastrous if any version of mini-gmp.h ever became
binary-incompatible with any version of GMP.

Re: [PATCH 1/2] Require full GMP when building module support.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Thu, 25 Apr 2019 17:06:28 +0200
> Cc: Paul Eggert <eggert@cs.ucla.edu>, Emacs developers <emacs-devel@gnu.org>, 
> 	Philipp Stephani <phst@google.com>
> 
> > > * configure.ac (HAVE_MODULES): Disable if full GMP is unavailable.
> >
> > Sounds too harsh to me.  After all, arbitrary-size numbers are needed
> > only in handful of situations.
> 
> I don't really see a good alternative.

Even not supporting bignums in modules at all sounds like a better
alternative to me.

[PATCH] Require full GMP for big integer module functions.

[Philipp Stephani]

This allows mini-gmp to not stay binary-compatible with full GMP.

* src/emacs-module.h.in: Don’t check EMACS_MODULE_HAVE_MPZ_T macro.

* src/emacs-module.c: Define EMACS_MODULE_GMP only if GMP is
available.
(module_extract_big_integer, module_make_big_integer):
Implement only if GMP is available.  Otherwise signal an error.
(syms_of_module): Define new error ‘unimplemented’.

* test/Makefile.in ($(test_module)): Remove support for mini-gmp.

* test/data/emacs-module/mod-test.c (EMACS_MODULE_GMP)
(Fmod_test_nanoseconds, Fmod_test_double): Define only if GMP is
available.

* test/src/emacs-module-tests.el (mod-test-nanoseconds)
(mod-test-double): Skip if GMP is unavailable.

* doc/lispref/internals.texi (Module Values): Document changed
behavior.
---
 doc/lispref/internals.texi        |  6 +++++-
 src/emacs-module.c                | 19 +++++++++++++++----
 src/emacs-module.h.in             |  2 +-
 test/Makefile.in                  |  3 +--
 test/data/emacs-module/mod-test.c | 11 +++++++----
 test/src/emacs-module-tests.el    | 10 ++++++++++
 6 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index 5ae71afbef..a8bf2ee31b 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -1519,7 +1519,8 @@ Module Values
 @end deftp
 
 @noindent
-Then you can use the following additional functions:
+If Emacs itself is compiled with GMP support, you can use the
+following additional functions:
 
 @deftypefn Function bool extract_big_integer (emacs_env *@var{env}, emacs_value @var{arg}, struct emacs_mpz *@var{result})
 This function, which is available since Emacs 27, extracts the
@@ -1543,6 +1544,9 @@ Module Values
 or similar.
 @end deftypefn
 
+If Emacs is compiled without GMP support, these functions will signal
+an error of type @code{unimplemented}.
+
 The following example uses GMP to calculate the next probable prime
 after a given integer:
 
diff --git a/src/emacs-module.c b/src/emacs-module.c
index 0b7b3d6ffb..3d9d9e5757 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -70,12 +70,10 @@ To add a new module function, proceed as follows:
 
 #include <config.h>
 
-#ifndef HAVE_GMP
-#include "mini-gmp.h"
-#define EMACS_MODULE_HAVE_MPZ_T
+#ifdef HAVE_GMP
+#define EMACS_MODULE_GMP
 #endif
 
-#define EMACS_MODULE_GMP
 #include "emacs-module.h"
 
 #include <stdarg.h>
@@ -788,20 +786,28 @@ module_extract_big_integer (emacs_env *env, emacs_value value,
                             struct emacs_mpz *result)
 {
   MODULE_FUNCTION_BEGIN ();
+#ifdef HAVE_GMP
   Lisp_Object o = value_to_lisp (value);
   CHECK_INTEGER (o);
   if (FIXNUMP (o))
     mpz_set_intmax (result->value, XFIXNUM (o));
   else
     mpz_set (result->value, XBIGNUM (o)->value);
+#else
+  xsignal0 (Qunimplemented);
+#endif
 }
 
 static emacs_value
 module_make_big_integer (emacs_env *env, const struct emacs_mpz *value)
 {
   MODULE_FUNCTION_BEGIN (NULL);
+#ifdef HAVE_GMP
   mpz_set (mpz[0], value->value);
   return lisp_to_value (env, make_integer_mpz ());
+#else
+  xsignal0 (Qunimplemented);
+#endif
 }
 
 \f
@@ -1384,6 +1390,11 @@ syms_of_module (void)
   Fput (Qinvalid_arity, Qerror_message,
         build_pure_c_string ("Invalid function arity"));
 
+  DEFSYM (Qunimplemented, "unimplemented");
+  Fput (Qunimplemented, Qerror_conditions, pure_list (Qunimplemented, Qerror));
+  Fput (Qunimplemented, Qerror_message,
+        build_pure_c_string ("Function not implemented"));
+
   DEFSYM (Qmodule_function_p, "module-function-p");
 
   defsubr (&Smodule_load);
diff --git a/src/emacs-module.h.in b/src/emacs-module.h.in
index fbc62a61ef..e61aadfc3a 100644
--- a/src/emacs-module.h.in
+++ b/src/emacs-module.h.in
@@ -28,7 +28,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <stdbool.h>
 #endif
 
-#if defined EMACS_MODULE_GMP && !defined EMACS_MODULE_HAVE_MPZ_T
+#ifdef EMACS_MODULE_GMP
 #include <gmp.h>
 #endif
 
diff --git a/test/Makefile.in b/test/Makefile.in
index ec20a427ba..2282ccd951 100644
--- a/test/Makefile.in
+++ b/test/Makefile.in
@@ -257,7 +257,6 @@ FPIC_CFLAGS =
 HYBRID_MALLOC = @HYBRID_MALLOC@
 LIBEGNU_ARCHIVE = ../lib/lib$(if $(HYBRID_MALLOC),e)gnu.a
 GMP_LIB = @GMP_LIB@
-GMP_OBJ = $(if @GMP_OBJ@, ../src/@GMP_OBJ@)
 
 # Note: emacs-module.h is generated from emacs-module.h.in, hence we
 # look in ../src, not $(srcdir)/../src.
@@ -270,7 +269,7 @@ src/emacs-module-tests.log:
 $(test_module): $(test_module:${SO}=.c) ../src/emacs-module.h $(LIBEGNU_ARCHIVE)
 	$(AM_V_at)${MKDIR_P} $(dir $@)
 	$(AM_V_CCLD)$(CC) -shared $(CPPFLAGS) $(MODULE_CFLAGS) $(LDFLAGS) \
-	  -o $@ $< $(LIBEGNU_ARCHIVE) $(GMP_LIB) $(GMP_OBJ)
+	  -o $@ $< $(LIBEGNU_ARCHIVE) $(GMP_LIB)
 endif
 
 ## Check that there is no 'automated' subdirectory, which would
diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c
index b7007bd80f..a264c0170f 100644
--- a/test/data/emacs-module/mod-test.c
+++ b/test/data/emacs-module/mod-test.c
@@ -29,12 +29,9 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 
 #ifdef HAVE_GMP
 #include <gmp.h>
-#else
-#include "mini-gmp.h"
-#define EMACS_MODULE_HAVE_MPZ_T
+#define EMACS_MODULE_GMP
 #endif
 
-#define EMACS_MODULE_GMP
 #include <emacs-module.h>
 
 #include "timespec.h"
@@ -386,6 +383,8 @@ Fmod_test_add_nanosecond (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
   return env->make_time (env, time);
 }
 
+#ifdef HAVE_GMP
+
 static emacs_value
 Fmod_test_nanoseconds (emacs_env *env, ptrdiff_t nargs, emacs_value *args, void *data) {
   assert (nargs == 1);
@@ -417,6 +416,8 @@ Fmod_test_double (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
   return result;
 }
 
+#endif /* HAVE_GMP */
+
 /* Lisp utilities for easier readability (simple wrappers).  */
 
 /* Provide FEATURE to Emacs.  */
@@ -486,8 +487,10 @@ emacs_module_init (struct emacs_runtime *ert)
          NULL, NULL);
   DEFUN ("mod-test-sleep-until", Fmod_test_sleep_until, 2, 2, NULL, NULL);
   DEFUN ("mod-test-add-nanosecond", Fmod_test_add_nanosecond, 1, 1, NULL, NULL);
+#ifdef HAVE_GMP
   DEFUN ("mod-test-nanoseconds", Fmod_test_nanoseconds, 1, 1, NULL, NULL);
   DEFUN ("mod-test-double", Fmod_test_double, 1, 1, NULL, NULL);
+#endif
 
 #undef DEFUN
 
diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el
index 173b63670f..fcd122626e 100644
--- a/test/src/emacs-module-tests.el
+++ b/test/src/emacs-module-tests.el
@@ -344,6 +344,9 @@ module--test-assertion
 
 (ert-deftest mod-test-nanoseconds ()
   "Test truncation when converting to `struct timespec'."
+  ;; `mod-test-nanoseconds' is not defined if the test module was
+  ;; compiled without GMP support.
+  (skip-unless (fboundp 'mod-test-nanoseconds))
   (dolist (test-case '((0 . 0)
                        (-1 . -1000000000)
                        ((1 . 1000000000) . 1)
@@ -362,10 +365,17 @@ module--test-assertion
         (should (eq (mod-test-nanoseconds input) expected))))))
 
 (ert-deftest mod-test-double ()
+  ;; `mod-test-double' is not defined if the test module was compiled
+  ;; without GMP support.
+  (skip-unless (fboundp 'mod-test-double))
   (dolist (input (list 0 1 2 -1 42 12345678901234567890
                        most-positive-fixnum (1+ most-positive-fixnum)
                        most-negative-fixnum (1- most-negative-fixnum)))
     (ert-info ((format "input: %d" input))
       (should (= (mod-test-double input) (* 2 input))))))
 
+;; Prevent compiler warnings for conditionally-defined functions.
+(declare-function mod-test-nanoseconds "mod-test.so" (arg))
+(declare-function mod-test-double "mod-test.so" (arg))
+
 ;;; emacs-module-tests.el ends here
-- 
2.20.1 (Apple Git-117)

Re: [PATCH] Require full GMP for big integer module functions.

[Paul Eggert]

On 4/25/19 10:09 AM, Philipp Stephani wrote:
> This allows mini-gmp to not stay binary-compatible with full GMP.

Isn't this overkill? I can see many practical scenarios where using
extract_big_integer etc. will work just fine even if Emacs is compiled
with mini-gmp and the module with full GMP (or vice versa). I can't see
many practical scenarios where these additional checks will cause more
good than harm.

Would it help to write to the GMP maintainer and ask how compatible
mini-gmp is intended to be? My impression is that compatiblity concerns
are pretty high on his priority list.

Re: [PATCH] Require full GMP for big integer module functions.

[Philipp Stephani]

Am Do., 25. Apr. 2019 um 23:00 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 4/25/19 10:09 AM, Philipp Stephani wrote:
> > This allows mini-gmp to not stay binary-compatible with full GMP.
>
> Isn't this overkill? I can see many practical scenarios where using
> extract_big_integer etc. will work just fine even if Emacs is compiled
> with mini-gmp and the module with full GMP (or vice versa). I can't see
> many practical scenarios where these additional checks will cause more
> good than harm.

The problem with binary compatibility is that if emacs-mini-gmp.h ever
becomes incompatible, the behavior will silently become undefined,
leading to extremely subtle bugs. So even a bit of harm here is too
much.

>
> Would it help to write to the GMP maintainer and ask how compatible
> mini-gmp is intended to be? My impression is that compatiblity concerns
> are pretty high on his priority list.

For full GMP definitely. My understanding is that emacs-mini-gmp.h is
maintained as part of Emacs, so *we* need to make sure to never become
incompatible if we want to continue to support this use case.

Re: [PATCH] Require full GMP for big integer module functions.

[Paul Eggert]

Philipp Stephani wrote:

> if emacs-mini-gmp.h ever
> becomes incompatible, the behavior will silently become undefined,

There is no emacs-mini-gmp.h so I assume you mean Emacs's src/mini-gmp.h.  That 
file is identical to GMP's mini-gmp.h, so isn't the Emacs-relevant 
incompatibility the same as the incompatiblity that the GMP maintainers are 
worried about?
> My understanding is that emacs-mini-gmp.h is
> maintained as part of Emacs

They're identical to the GMP mini-gmp.c and mini-gmp.h so they're primarily 
maintained as part of GMP. The only part relevant to Emacs maintenance is that 
when we update Emacs src/mini-gmp.h and src/mini-gmp.c from GMP sources, we 
should look for Emacs-relevant incompatibilities and if there are any (which 
typically there are not) we should delay these updates until the next Emacs 
release. But this is something we should do anyway - it's an ordinary part of 
release maintenance.

There's an analogy to Gnulib here. At some point after forking emacs-26 from the 
master branch, we stopped propagating Gnulib patches into emacs-26 because they 
were considered too likely to break what was intended to be a stable branch. If 
mini-gmp had been part of Emacs 26, I expect we would have taken the same 
approach to mini-gmp as well.

Which reminds me, now's as good time as any to sync Emacs master to the latest 
GMP copies of mini-gmp.h and mini-gmp.c, so I did that.

Re: [PATCH 1/2] Require full GMP when building module support.

[Philipp Stephani]

Am Do., 25. Apr. 2019 um 18:15 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Thu, 25 Apr 2019 17:06:28 +0200
> > Cc: Paul Eggert <eggert@cs.ucla.edu>, Emacs developers <emacs-devel@gnu.org>,
> >       Philipp Stephani <phst@google.com>
> >
> > > > * configure.ac (HAVE_MODULES): Disable if full GMP is unavailable.
> > >
> > > Sounds too harsh to me.  After all, arbitrary-size numbers are needed
> > > only in handful of situations.
> >
> > I don't really see a good alternative.
>
> Even not supporting bignums in modules at all sounds like a better
> alternative to me.

OK, I'll think about an alternative.