bignum branch

bignum branch

[Tom Tromey]

I've pushed the bignum branch to emacs git, as feature/bignum.

Please read through it and try it out, and reply to this message with
your comments.

thanks,
Tom

Re: bignum branch

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Date: Thu, 12 Jul 2018 22:26:38 -0600
> 
> I've pushed the bignum branch to emacs git, as feature/bignum.

Thanks for your work on this.  (I didn't yet have time to have a look,
but will do so soon.)

Re: bignum branch

[Robert Pluim]

Tom Tromey <tom@tromey.com> writes:

> I've pushed the bignum branch to emacs git, as feature/bignum.
>
> Please read through it and try it out, and reply to this message with
> your comments.

Iʼve not tried it it yet, but I see that GMP will basically always be
available because mini-gmp.o is used when gmp is not found by
configure. Would it make sense to add GMP to emacs_config_features so
we can detect that case?

Regards

Robert

Re: bignum branch

[Robert Pluim]

Robert Pluim <rpluim@gmail.com> writes:

> Tom Tromey <tom@tromey.com> writes:
>
>> I've pushed the bignum branch to emacs git, as feature/bignum.
>>
>> Please read through it and try it out, and reply to this message with
>> your comments.
>
> Iʼve not tried it it yet, but I see that GMP will basically always be
> available because mini-gmp.o is used when gmp is not found by
> configure. Would it make sense to add GMP to emacs_config_features so
> we can detect that case?

And now that I have tried it:

        ELC      obsolete/pgg-pgp.elc

    In toplevel form:
    obsolete/pgg-pgp.el:30:1:Error: Error in CCL program

This is with gmp 6.1.2

If I force usage of mini-gmp.o I get the same error.

Regards

Robert

Re: bignum branch

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Fri, 13 Jul 2018 10:45:34 +0200
> Cc: emacs-devel@gnu.org
> 
> Would it make sense to add GMP to emacs_config_features so we can
> detect that case?

Yes, I think so.

Re: bignum branch

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Fri, 13 Jul 2018 10:45:34 +0200
>> Cc: emacs-devel@gnu.org
>> 
>> Would it make sense to add GMP to emacs_config_features so we can
>> detect that case?
>
> Yes, I think so.

OK. Tom, shall I push such a change to the feature/bignum branch?

Robert

Re: bignum branch

[Tom Tromey]

>>>>> "Robert" == Robert Pluim <rpluim@gmail.com> writes:

Robert> Eli Zaretskii <eliz@gnu.org> writes:
>>> From: Robert Pluim <rpluim@gmail.com>
>>> Date: Fri, 13 Jul 2018 10:45:34 +0200
>>> Cc: emacs-devel@gnu.org
>>> 
>>> Would it make sense to add GMP to emacs_config_features so we can
>>> detect that case?
>> 
>> Yes, I think so.

Robert> OK. Tom, shall I push such a change to the feature/bignum branch?

Sure, thanks.

Tom

Re: bignum branch

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Fri, 13 Jul 2018 11:51:43 +0200
> Cc: emacs-devel@gnu.org
> 
>         ELC      obsolete/pgg-pgp.elc
> 
>     In toplevel form:
>     obsolete/pgg-pgp.el:30:1:Error: Error in CCL program

The line number makes no sense.  Does the problem happen when loading
'cl'?  I mean, what CCL program is involved here?

Re: bignum branch

[Eli Zaretskii]

> Date: Fri, 13 Jul 2018 15:04:15 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > From: Robert Pluim <rpluim@gmail.com>
> > Date: Fri, 13 Jul 2018 11:51:43 +0200
> > Cc: emacs-devel@gnu.org
> > 
> >         ELC      obsolete/pgg-pgp.elc
> > 
> >     In toplevel form:
> >     obsolete/pgg-pgp.el:30:1:Error: Error in CCL program
> 
> The line number makes no sense.  Does the problem happen when loading
> 'cl'?  I mean, what CCL program is involved here?

Sorry, I looked in the wrong branch.  Line 30 is actually about
loading pgg.  Does the same problem happen when pgg.el is loaded
and/or compiled?

Re: bignum branch

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Fri, 13 Jul 2018 15:04:15 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: emacs-devel@gnu.org
>> 
>> > From: Robert Pluim <rpluim@gmail.com>
>> > Date: Fri, 13 Jul 2018 11:51:43 +0200
>> > Cc: emacs-devel@gnu.org
>> > 
>> >         ELC      obsolete/pgg-pgp.elc
>> > 
>> >     In toplevel form:
>> >     obsolete/pgg-pgp.el:30:1:Error: Error in CCL program
>> 
>> The line number makes no sense.  Does the problem happen when loading
>> 'cl'?  I mean, what CCL program is involved here?
>
> Sorry, I looked in the wrong branch.  Line 30 is actually about
> loading pgg.  Does the same problem happen when pgg.el is loaded
> and/or compiled?

emacs -Q
(require 'pgg)

⇒

Debugger entered--Lisp error: (error "Error in CCL program")
  register-ccl-program(pgg-parse-crc24 [1 30 14 114744 114775 0 161 131127 1 148217 15 82167 1 1848 131159 1 1595 5 256 114743 390 114775 19707 1467 16 7 183 1 4611686018427382276 4611686018427380740 22])
  byte-code("\301\302!\203\33\0\303\30\304\10!\210\305\306\307\310\306\10\"#\210)\311\312\313\"\210\301\207" [prog fboundp define-ccl-program [1 30 14 114744 114775 0 161 131127 1 148217 15 82167 1 1848 131159 1 1595 5 256 114743 390 114775 19707 1467 16 7 183 1 4611686018427382276 4611686018427380740 22] (lambda (def-tmp-var) (defconst pgg-parse-crc24 def-tmp-var nil)) put pgg-parse-crc24 ccl-program-idx register-ccl-program defalias pgg-parse-crc24-string #f(compiled-function (string) #<bytecode 0x5033e9>)] 6)
  require(pgg-parse)
  eval-buffer(#<buffer  *load*> nil "/home/rpluim/repos/emacs-bignum/lisp/obsolete/pgg.el" nil t)  ; Reading at buffer position 1024
  load-with-code-conversion("/home/rpluim/repos/emacs-bignum/lisp/obsolete/pgg.el" "/home/rpluim/repos/emacs-bignum/lisp/obsolete/pgg.el" nil t)
  require(pgg)
  eval((require 'pgg) nil)
  elisp--eval-last-sexp(nil)
  eval-last-sexp(nil)
  funcall-interactively(eval-last-sexp nil)
  call-interactively(eval-last-sexp nil nil)
  command-execute(eval-last-sexp)


Which confirms what I found using gdb. That 4611686018427382276 in the
backtrace is bigger than 'most-positive-fixnum' on this machine, so
the error makes sense. I know nothing about how to fix CCL though.

Robert

Re: bignum branch

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Fri, 13 Jul 2018 15:02:01 +0200
> 
> emacs -Q
> (require 'pgg)
> 
> ⇒
> 
> Debugger entered--Lisp error: (error "Error in CCL program")
>   register-ccl-program(pgg-parse-crc24 [1 30 14 114744 114775 0 161 131127 1 148217 15 82167 1 1848 131159 1 1595 5 256 114743 390 114775 19707 1467 16 7 183 1 4611686018427382276 4611686018427380740 22])

Thanks.

I guess the operations in pgg-parse-crc24 give birth to these numbers,
so ccl.c should be fixed to avoid that, for backward compatibility
with existing CCL programs.  (I'm guessing that ccl.c relies on
integer wrap-around, and bignums violate that.)

Re: bignum branch

[Andy Moreton]

On Fri 13 Jul 2018, Eli Zaretskii wrote:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: emacs-devel@gnu.org
>> Date: Fri, 13 Jul 2018 15:02:01 +0200
>> 
>> emacs -Q
>> (require 'pgg)
>> 
>> ⇒
>> 
>> Debugger entered--Lisp error: (error "Error in CCL program")
>>   register-ccl-program(pgg-parse-crc24 [1 30 14 114744 114775 0 161 131127 1
>> 148217 15 82167 1 1848 131159 1 1595 5 256 114743 390 114775 19707 1467 16 7
>> 183 1 4611686018427382276 4611686018427380740 22])
>
> Thanks.
>
> I guess the operations in pgg-parse-crc24 give birth to these numbers,
> so ccl.c should be fixed to avoid that, for backward compatibility
> with existing CCL programs.  (I'm guessing that ccl.c relies on
> integer wrap-around, and bignums violate that.)

If I bootstrap a 32bit build from the bignum branch, this error does not
occur and the build completes successfully. However if I bootstrap a
64bit build and then build a 32bit emacs from the same source tree, I
see this error.

This suggests that the problem is (at least partly) in the compiler for
CCL programs in ccl.el, rather than the interpreter in ccl.c.

    AndyM

Re: bignum branch

[Robert Pluim]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> On Fri 13 Jul 2018, Eli Zaretskii wrote:
>
>>> From: Robert Pluim <rpluim@gmail.com>
>>> Cc: emacs-devel@gnu.org
>>> Date: Fri, 13 Jul 2018 15:02:01 +0200
>>> 
>>> emacs -Q
>>> (require 'pgg)
>>> 
>>> ⇒
>>> 
>>> Debugger entered--Lisp error: (error "Error in CCL program")
>>>   register-ccl-program(pgg-parse-crc24 [1 30 14 114744 114775 0 161 131127 1
>>> 148217 15 82167 1 1848 131159 1 1595 5 256 114743 390 114775 19707 1467 16 7
>>> 183 1 4611686018427382276 4611686018427380740 22])
>>
>> Thanks.
>>
>> I guess the operations in pgg-parse-crc24 give birth to these numbers,
>> so ccl.c should be fixed to avoid that, for backward compatibility
>> with existing CCL programs.  (I'm guessing that ccl.c relies on
>> integer wrap-around, and bignums violate that.)
>
> If I bootstrap a 32bit build from the bignum branch, this error does not
> occur and the build completes successfully. However if I bootstrap a
> 64bit build and then build a 32bit emacs from the same source tree, I
> see this error.
>
> This suggests that the problem is (at least partly) in the compiler for
> CCL programs in ccl.el, rather than the interpreter in ccl.c.

The interpreter is fine. ccl.el assumes that 'ash' will truncate its
result, which is no longer true when using bignums. Truncating all ash
operations to 28 bits in ccl.el fixes this particular error for me,
but the resulting CCL programs are not identical:

emacs-master:

[1 30 14 114744 114775 0 161 131127 1 148217 15 82167 1 1848 131159 1
1595 5 256 114743 390 114775 19707 1467 16 7 183 1 -5628 -7164 22]

emacs-bignum:

[1 31 14 114744 114775 0 162 0 131127 1 148217 15 82167 1 1848 131159
1 1595 5 256 114743 390 114775 19707 1467 16 7 183 1 268429828
268428036 22]

Someone who know ccl should take a look (or we punt and remove pgg,
since itʼs obsolete anyway).

Robert

Re: bignum branch

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Tue, 17 Jul 2018 20:10:39 +0200
> Cc: emacs-devel@gnu.org
> 
> The interpreter is fine. ccl.el assumes that 'ash' will truncate its
> result, which is no longer true when using bignums. Truncating all ash
> operations to 28 bits in ccl.el fixes this particular error for me,
> but the resulting CCL programs are not identical:

Why is it important that the CCL programs be identical?

Or maybe we should have a variant of 'ash' that does truncate, since
other callers might expect the same?

Re: bignum branch

[Eli Zaretskii]

> Date: Tue, 17 Jul 2018 21:24:31 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> Or maybe we should have a variant of 'ash' that does truncate, since
> other callers might expect the same?

More generally, do we want to have a knob that would disable automatic
extension into bignums while some Lisp code which needs that?

Re: bignum branch

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Tue, 17 Jul 2018 20:10:39 +0200
>> Cc: emacs-devel@gnu.org
>> 
>> The interpreter is fine. ccl.el assumes that 'ash' will truncate its
>> result, which is no longer true when using bignums. Truncating all ash
>> operations to 28 bits in ccl.el fixes this particular error for me,
>> but the resulting CCL programs are not identical:
>
> Why is it important that the CCL programs be identical?
>

The source is the same, so I assumed that any difference in the output
is a bug. I donʼt think ccl has changed much.

> Or maybe we should have a variant of 'ash' that does truncate, since
> other callers might expect the same?

It an obvious assumption to make in a system that only has
fixnums. The issue is probably not confined to 'ash'.

Robert

Re: bignum branch

[Clément Pit-Claudel]

On 2018-07-17 16:00, Robert Pluim wrote:>> Or maybe we should have a variant of 'ash' that does truncate, since>> other callers might expect the same?>
> It an obvious assumption to make in a system that only has
> fixnums. The issue is probably not confined to 'ash'.

I wonder if a file- or dir-local variable to tell the interpreter and the byte-compiler whether to use fixnums would make sense.  It would be akin to lexical-binding: t.

Re: bignum branch

[Stefan Monnier]

> I wonder if a file- or dir-local variable to tell the interpreter and the
> byte-compiler whether to use fixnums would make sense.  It would be akin to
> lexical-binding: t.

I think this would be quite expensive to implement compared to the
expected benefit.


        Stefan

Re: bignum branch

[Andy Moreton]

On Tue 17 Jul 2018, Robert Pluim wrote:
> The interpreter is fine. ccl.el assumes that 'ash' will truncate its
> result, which is no longer true when using bignums. Truncating all ash
> operations to 28 bits in ccl.el fixes this particular error for me,
> but the resulting CCL programs are not identical:

I tried something similar, by truncating the resulting CCL codewords to 28bits.
That fixes compiling the CCL program, but fails in the interpreter. You
can check by running some simple tests:

master (64bit mingw64 MSYS2):
    ELISP> (require 'pgg)
    pgg
    ELISP> (seq-map (lambda (x) (format "#x%x" x)) pgg-parse-crc24)
    ("#x1" "#x1e" "#xe" "#x1c038" "#x1c057" "#x0" "#xa1" "#x20037"
     "#x1" "#x242f9" "#xf" "#x140f7" "#x1" "#x738" "#x20057" "#x1"
     "#x63b" "#x5" "#x100" "#x1c037" "#x186" "#x1c057" "#x4cfb"
     "#x5bb" "#x10" "#x7" "#xb7" "#x1" "#x3fffffffffffea04"
     "#x3fffffffffffe404" "#x16")

    ELISP> (ccl-dump pgg-parse-crc24)
    nil
    ELISP> Out-buffer must be as large as in-buffer.
    Main-body:
        2:[read-register] read r0 (0 remaining)
        3:[set-assign-expr-register] r1 ^= r0
        4:[set-assign-expr-const] r2 ^= 0
        6:[set-short-const] r5 = 0
        7:[set-assign-expr-const] r1 <<= 1
        9:[set-expr-const] r7 = r2 >> 15
       11:[set-assign-expr-const] r7 &= 1
       13:[set-assign-expr-register] r1 += r7
       14:[set-assign-expr-const] r2 <<= 1
       16:[jump-cond-expr-const] if !(r1 & 256), jump to 23(+7)
       19:[set-assign-expr-const] r1 ^= 390
       21:[set-assign-expr-const] r2 ^= 19707
       23:[jump-cond-expr-const] if !(r5 < 7), jump to 29(+6)
       26:[set-assign-expr-const] r5 += 1
       28:[jump] jump to 7(-21)
       29:[jump] jump to 2(-27)
    At EOF:
       30:[end] end

    ELISP> (dolist (s '("foo" "bar" "baz"))
      (let ((crc (pgg-parse-crc24-string s)))
        (insert "\n" s " --> "
         (mapconcat (lambda (b) (format "%02x" b)) crc ""))))
    nil
    ELISP> 
    foo --> 4fc255
    bar --> 51d953
    baz --> f0586a


bignum (64bit mingw64 MSYS2) with patch for `ash':
    ELISP> (require 'pgg)
    pgg
    ELISP> (seq-map (lambda (x) (format "#x%x" x)) pgg-parse-crc24)
    ("#x1" "#x1f" "#xe" "#x1c038" "#x1c057" "#x0" "#xa2" "#x0" "#x20037"
     "#x1" "#x242f9" "#xf" "#x140f7" "#x1" "#x738" "#x20057" "#x1"
     "#x63b" "#x5" "#x100" "#x1c037" "#x186" "#x1c057" "#x4cfb" "#x5bb"
     "#x10" "#x7" "#xb7" "#x1" "#xfffea04" "#xfffe304" "#x16")

    ELISP> (ccl-dump pgg-parse-crc24)
    nil
    ELISP> Out-buffer must be as large as in-buffer.
    Main-body:
        2:[read-register] read r0 (0 remaining)
        3:[set-assign-expr-register] r1 ^= r0
        4:[set-assign-expr-const] r2 ^= 0
        6:[set-const] r5 = 0
        8:[set-assign-expr-const] r1 <<= 1
       10:[set-expr-const] r7 = r2 >> 15
       12:[set-assign-expr-const] r7 &= 1
       14:[set-assign-expr-register] r1 += r7
       15:[set-assign-expr-const] r2 <<= 1
       17:[jump-cond-expr-const] if !(r1 & 256), jump to 24(+7)
       20:[set-assign-expr-const] r1 ^= 390
       22:[set-assign-expr-const] r2 ^= 19707
       24:[jump-cond-expr-const] if !(r5 < 7), jump to 30(+6)
       27:[set-assign-expr-const] r5 += 1
       29:[jump] jump to 1048584(+1048555)
       30:[jump] jump to 1048578(+1048548)
    At EOF:
       31:[end] end

    ELISP> (dolist (s '("foo" "bar" "baz"))
          (let ((crc (pgg-parse-crc24-string s)))
            (insert "\n" s " --> "
             (mapconcat (lambda (b) (format "%02x" b)) crc ""))))
    *** Eval error ***  Error in CCL program at 30th code

> Someone who know ccl should take a look (or we punt and remove pgg,
> since itʼs obsolete anyway).

If pgg is removed, the CCL interpreter and ccl.el should also be
removed.

    AndyM

Re: bignum branch

[Robert Pluim]

Andy Moreton <andrewjmoreton@gmail.com> writes:

>> Someone who know ccl should take a look (or we punt and remove pgg,
>> since itʼs obsolete anyway).
>
> If pgg is removed, the CCL interpreter and ccl.el should also be
> removed.

lisp/language/ethiopic.el still calls define-ccl-program, but as far
as I can tell the result is not actually used anywhere. Removing pgg
and ccl completely and removing the ccl from lisp/language/ethiopic.el
doesnʼt seem to have had any adverse effect on my emacs.

midi-kbd is the only external package Iʼve found that uses ccl.

Iʼm all for removing unused features, but Iʼm not 100% sure thatʼs
true for ccl. Probably weʼd need to deprecate it first.

Robert

Re: bignum branch

[Stefan Monnier]

> Iʼm all for removing unused features, but Iʼm not 100% sure thatʼs
> true for ccl.  Probably weʼd need to deprecate it first.

Indeed!


        Stefan

Re: bignum branch

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Wed, 18 Jul 2018 15:21:33 +0200
> Cc: emacs-devel@gnu.org
> 
> lisp/language/ethiopic.el still calls define-ccl-program, but as far
> as I can tell the result is not actually used anywhere.

Unless I'm missing something, it is used right there: we put it into
font-ccl-encoder-alist, whose doc string should explain why.

> Removing pgg and ccl completely and removing the ccl from
> lisp/language/ethiopic.el doesnʼt seem to have had any adverse
> effect on my emacs.

Probably because you didn't use a font whose name matches "ethiopic".

Re: bignum branch

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Wed, 18 Jul 2018 15:21:33 +0200
>> Cc: emacs-devel@gnu.org
>> 
>> lisp/language/ethiopic.el still calls define-ccl-program, but as far
>> as I can tell the result is not actually used anywhere.
>
> Unless I'm missing something, it is used right there: we put it into
> font-ccl-encoder-alist, whose doc string should explain why.

I donʼt see any other code referencing font-ccl-encoder-alist, nor
Vfont_ccl_encoder_alist.

>> Removing pgg and ccl completely and removing the ccl from
>> lisp/language/ethiopic.el doesnʼt seem to have had any adverse
>> effect on my emacs.
>
> Probably because you didn't use a font whose name matches
> "ethiopic".

I donʼt have any of those. Iʼll see if I can find one.

Robert

Re: bignum branch

[Eli Zaretskii]

> X-Spam-Status: No, score=-0.5 required=5.0 tests=BAYES_05,FREEMAIL_FROM,
> 	T_DKIM_INVALID autolearn=disabled version=3.3.2
> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Wed, 18 Jul 2018 18:21:03 +0200
> 
> I donʼt see any other code referencing font-ccl-encoder-alist, nor
> Vfont_ccl_encoder_alist.

Ah, good point.  The code which uses that was removed when we moved to
Unicode in Emacs 23.

Re: bignum branch

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Fri, 13 Jul 2018 11:51:43 +0200
>> Cc: emacs-devel@gnu.org
>> 
>>         ELC      obsolete/pgg-pgp.elc
>> 
>>     In toplevel form:
>>     obsolete/pgg-pgp.el:30:1:Error: Error in CCL program
>
> The line number makes no sense.  Does the problem happen when loading
> 'cl'?  I mean, what CCL program is involved here?

I donʼt know. I tried replacing the (require) on that line with the
contents of the require'd library, and the error went away. Iʼve
tracked it down to resolve_symbol_ccl_program returning nil, but the
only thing Tom changed in that function was replacing INTERGP with
FIXNUMP.

Robert

Re: bignum branch

[Andy Moreton]

On Thu 12 Jul 2018, Tom Tromey wrote:

> I've pushed the bignum branch to emacs git, as feature/bignum.
>
> Please read through it and try it out, and reply to this message with
> your comments.
>
> thanks,
> Tom

I've bootstrapped this with 64bit mingw64 (MSYS2) on Windows without
problems. There were a few compile warnings:

C:/emacs/git/emacs/bignum/src/floatfns.c: In function 'Fabs':
C:/emacs/git/emacs/bignum/src/floatfns.c:291:29: warning: overflow in implicit constant conversion [-Woverflow]
       mpz_init_set_si (val, - MOST_NEGATIVE_FIXNUM);
                             ^

In toplevel form:
../../../lisp/calc/calc-aent.el:28:1:Error: Arithmetic range error: "truncate", -1.0e+INF
make[2]: *** [Makefile:301: calc/calc-aent.elc] Error 1

In toplevel form:
../../../lisp/calc/calc-alg.el:28:1:Error: Arithmetic range error: "truncate", -1.0e+INF
make[2]: *** [Makefile:301: calc/calc-alg.elc] Error 1


All of the other warnings are the same as the master branch.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 13 Jul 2018 15:28:27 +0100
> 
> I've bootstrapped this with 64bit mingw64 (MSYS2) on Windows without
> problems.

Thanks.

> All of the other warnings are the same as the master branch.

If those warnings were never reported before, please report them (in a
separate thread, preferably a bug report).  Emacs should ideally
compile cleanly, without any warnings, as long as you use a recent
enough version of GCC.

Re: bignum branch

[Andy Moreton]

On Fri 13 Jul 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 13 Jul 2018 15:28:27 +0100
>> 
>> I've bootstrapped this with 64bit mingw64 (MSYS2) on Windows without
>> problems.
>
> Thanks.
>
>> All of the other warnings are the same as the master branch.
>
> If those warnings were never reported before, please report them (in a
> separate thread, preferably a bug report).  Emacs should ideally
> compile cleanly, without any warnings, as long as you use a recent
> enough version of GCC.

The warnings on the master branch are from byte compiling lisp, not
from GCC.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 13 Jul 2018 15:53:15 +0100
> 
> The warnings on the master branch are from byte compiling lisp, not
> from GCC.

Ah, okay.

Re: bignum branch

[Andy Moreton]

On Fri 13 Jul 2018, Andy Moreton wrote:

> On Thu 12 Jul 2018, Tom Tromey wrote:
>
>> I've pushed the bignum branch to emacs git, as feature/bignum.
>>
>> Please read through it and try it out, and reply to this message with
>> your comments.
>>
>> thanks,
>> Tom
>
> I've bootstrapped this with 64bit mingw64 (MSYS2) on Windows without
> problems. There were a few compile warnings:
>
> C:/emacs/git/emacs/bignum/src/floatfns.c: In function 'Fabs':
> C:/emacs/git/emacs/bignum/src/floatfns.c:291:29: warning: overflow in implicit constant conversion [-Woverflow]
>        mpz_init_set_si (val, - MOST_NEGATIVE_FIXNUM);

There is also odd behaviour on the bignum branch:
ELISP> most-negative-fixnum
-2305843009213693952 (#o200000000000000000000, #x2000000000000000)
ELISP> (abs most-negative-fixnum)
0 (#o0, #x0, ?\C-@)

On master, this behaves as expected:
ELISP> most-negative-fixnum
-2305843009213693952 (#o200000000000000000000, #x2000000000000000)
ELISP> (abs most-negative-fixnum)
-2305843009213693952 (#o200000000000000000000, #x2000000000000000)

Re: bignum branch

[Andy Moreton]

On Fri 13 Jul 2018, Andy Moreton wrote:

> On Fri 13 Jul 2018, Andy Moreton wrote:
>
>> On Thu 12 Jul 2018, Tom Tromey wrote:
>> I've bootstrapped this with 64bit mingw64 (MSYS2) on Windows without
>> problems.

It seems that there are problems after all.

The mpz_* API from libgmp uses "long" as the widest type for native
integers, which does not work on 64bit mingw64 on Windows, where "long" is
32bit and "long long" (used for EMACS_INT) is 64bit.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 13 Jul 2018 20:35:18 +0100
> 
> The mpz_* API from libgmp uses "long" as the widest type for native
> integers, which does not work on 64bit mingw64 on Windows, where "long" is
> 32bit and "long long" (used for EMACS_INT) is 64bit.

Isn't that a problem in how GMP was configured for MinGW64?  I see in
the GMP sources that it does check for "long long", so why didn't that
work during the build?

In the file gmp-h.in (from which gmp.h is generated during the build),
I see this:

  @DEFN_LONG_LONG_LIMB@
  ...
  #ifdef __GMP_SHORT_LIMB
  typedef unsigned int		mp_limb_t;
  typedef int			mp_limb_signed_t;
  #else
  #ifdef _LONG_LONG_LIMB
  typedef unsigned long long int	mp_limb_t;
  typedef long long int		mp_limb_signed_t;
  #else
  typedef unsigned long int	mp_limb_t;
  typedef long int		mp_limb_signed_t;
  #endif
  #endif

This seems to indicate that if the configure script defines
_LONG_LONG_LIMB, then mp_limb_t will be a 64-bit signed integer type,
which is what you want.  And looking at configure.ac, I seem to
understand that MinGW64 should have caused _LONG_LONG_LIMB to be
defined.  (I cannot test all those because I don't have MinGW64
installed.)  Can you see why all this didn't work for you?

Re: bignum branch

[Andy Moreton]

On Sat 14 Jul 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 13 Jul 2018 20:35:18 +0100
>> 
>> The mpz_* API from libgmp uses "long" as the widest type for native
>> integers, which does not work on 64bit mingw64 on Windows, where "long" is
>> 32bit and "long long" (used for EMACS_INT) is 64bit.
>
> Isn't that a problem in how GMP was configured for MinGW64?  I see in
> the GMP sources that it does check for "long long", so why didn't that
> work during the build?
>
> In the file gmp-h.in (from which gmp.h is generated during the build),
> I see this:
[snipped]

The choice of word size for the limbs internal to GMP is irrelevant
here.

The problem is that if EMACS_INT is "long long", then the calls to
mpz_*_si() API routines will truncate any values passed to GMP. This
means that emacs cannot use any the mpz_*_si() routines directly.

Alternatives include:
a) Build 64bit emacs on Windows using 32bit long for EMACS_INT. This
seems undesirable as all values larger than 32bits are then bignums.

b) Use 64bit long long for EMACS_INT, and do extra work to pass native
values into GMP in two halves, i.e. something like:
    EMACS_INT i;
    mpz_t val, tmp;
    mpz_init_set_si(val, (i >> 32));
    mpz_mul_2exp(val, val, 32);
    mpz_init_set_si(tmp, (i & 0xffffffff);
    void mpz_ior(val, val, tmp);
    mpz_clear(tmp);

GMP does not yet support C99 types, so will result in awkward and
inefficient handling of 64bit vlaues on LLP64 systems.

    AndyM

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> b) Use 64bit long long for EMACS_INT, and do extra work to pass native
Andy> values into GMP in two halves, i.e. something like:
Andy>     EMACS_INT i;
Andy>     mpz_t val, tmp;
Andy>     mpz_init_set_si(val, (i >> 32));
Andy>     mpz_mul_2exp(val, val, 32);
Andy>     mpz_init_set_si(tmp, (i & 0xffffffff);
Andy>     void mpz_ior(val, val, tmp);
Andy>     mpz_clear(tmp);

I was thinking this is what I' have emacs do when
sizeof(EMACS_INT) > sizeof(long).

Tom

Re: bignum branch

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Date: Sun, 15 Jul 2018 07:46:39 -0600
> Cc: emacs-devel@gnu.org
> 
> Andy> b) Use 64bit long long for EMACS_INT, and do extra work to pass native
> Andy> values into GMP in two halves, i.e. something like:
> Andy>     EMACS_INT i;
> Andy>     mpz_t val, tmp;
> Andy>     mpz_init_set_si(val, (i >> 32));
> Andy>     mpz_mul_2exp(val, val, 32);
> Andy>     mpz_init_set_si(tmp, (i & 0xffffffff);
> Andy>     void mpz_ior(val, val, tmp);
> Andy>     mpz_clear(tmp);
> 
> I was thinking this is what I' have emacs do when
> sizeof(EMACS_INT) > sizeof(long).

Yes, we need such wrappers in those cases.  Another use case is a
32-bit build --with-wide-int.

Thanks.

Re: bignum branch

[Stefan Monnier]

> Yes, we need such wrappers in those cases.  Another use case is a
> 32-bit build --with-wide-int.

Tho, eventually, bignums should make --with-wide-int redundant.


        Stefan

Re: bignum branch

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 16 Jul 2018 08:19:07 -0400
> 
> > Yes, we need such wrappers in those cases.  Another use case is a
> > 32-bit build --with-wide-int.
> 
> Tho, eventually, bignums should make --with-wide-int redundant.

Only if we allow buffer and string text be referenced by a bignum, and
if the performance is comparable with --with-wide-int.

Is it reasonable to expect a comparable performance from native 32-bit
code calculating 64-bit values vs function calls?  I think I'd be
surprised.

Re: bignum branch

[Stefan Monnier]

>> > Yes, we need such wrappers in those cases.  Another use case is a
>> > 32-bit build --with-wide-int.
>> Tho, eventually, bignums should make --with-wide-int redundant.
> Only if we allow buffer and string text be referenced by a bignum,

I'd have said "when" rather than "if", but otherwise, yes.

> and if the performance is comparable with --with-wide-int.

Not sure what kind of performance you have in mind: a plain old 32bit
build with bignums will almost inevitably be more efficient than
one --with-wide-int when dealing with buffers <512MB, but it will just
as inevitably be less efficient when dealing with buffers between 512MB
and 2GB.

> Is it reasonable to expect a comparable performance from native 32-bit
> code calculating 64-bit values vs function calls?  I think I'd be
> surprised.

Operations on (small) bignums will be significantly slower than
operations of "long long" 64bit integers, yes.  How this will impact the
overall performance when dealing with buffers between 512MB and 2GB
I don't know: these already tend to suffer from various other
performance problems and I don't know if one will dwarf the other or if
they will make each other more painful.


        Stefan

Re: bignum branch

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Mon, 16 Jul 2018 12:09:41 -0400
> 
> > and if the performance is comparable with --with-wide-int.
> 
> Not sure what kind of performance you have in mind: a plain old 32bit
> build with bignums will almost inevitably be more efficient than
> one --with-wide-int when dealing with buffers <512MB, but it will just
> as inevitably be less efficient when dealing with buffers between 512MB
> and 2GB.

Between 512MB and 2GB is what I had in mind.

> > Is it reasonable to expect a comparable performance from native 32-bit
> > code calculating 64-bit values vs function calls?  I think I'd be
> > surprised.
> 
> Operations on (small) bignums will be significantly slower than
> operations of "long long" 64bit integers, yes.  How this will impact the
> overall performance when dealing with buffers between 512MB and 2GB
> I don't know: these already tend to suffer from various other
> performance problems and I don't know if one will dwarf the other or if
> they will make each other more painful.

That's what I'd expect as well, and so I don't think --with-wide-int
will die too quickly.  I personally need to use buffers larger than
0.5GB all the time on my daytime job.

Re: bignum branch

[Stefan Monnier]

> That's what I'd expect as well, and so I don't think --with-wide-int
> will die too quickly.

Not in the near future, no.  For that reason I just said "eventually".

> I personally need to use buffers larger than
> 0.5GB all the time on my daytime job.

Is that on 32bit systems (i.e. using wide-int)?
Do these rarely exceed the 2GB limit?
By "use" I assume you mean also "edit", right?


        Stefan

Re: bignum branch

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 16 Jul 2018 14:32:18 -0400
> 
> > I personally need to use buffers larger than
> > 0.5GB all the time on my daytime job.
> 
> Is that on 32bit systems (i.e. using wide-int)?

Emacs is compiled with a 32-bit compiler.

> Do these rarely exceed the 2GB limit?

Yes, almost never.

> By "use" I assume you mean also "edit", right?

Yes.

Re: bignum branch

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I was thinking this is what I' have emacs do when
Tom> sizeof(EMACS_INT) > sizeof(long).

Please try this patch.

Unfortunately I don't know how I can test it locally

Tom

diff --git a/src/alloc.c b/src/alloc.c
index b775948fd9..1dc1bbb031 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -3824,6 +3824,36 @@ make_number (mpz_t value)
   return obj;
 }
 
+void
+mpz_set_intmax_slow (mpz_t result, intmax_t v)
+{
+  /* If long is larger then a faster path is taken.  */
+  eassert (sizeof (intmax_t) > sizeof (long));
+
+  bool negate = false;
+  if (v < 0)
+    {
+      v = -v;
+      negate = true;
+    }
+  mpz_set_uintmax_slow (result, (uintmax_t) v);
+  if (negate)
+    mpz_neg (result, result);
+}
+
+void
+mpz_set_uintmax_slow (mpz_t result, uintmax_t v)
+{
+  /* If long is larger then a faster path is taken.  */
+  eassert (sizeof (uintmax_t) > sizeof (unsigned long));
+  /* This restriction could be lifted if needed.  */
+  eassert (sizeof (uintmax_t) <= 2 * sizeof (unsigned long));
+
+  mpz_set_ui (result, v >> (CHAR_BIT * sizeof (unsigned long)));
+  mpz_mul_2exp (result, result, CHAR_BIT * sizeof (unsigned long));
+  mpz_add_ui (result, result, v & -1ul);
+}
+
 \f
 /* Return a newly created vector or string with specified arguments as
    elements.  If all the arguments are characters that can fit
diff --git a/src/data.c b/src/data.c
index 862381229d..0deebdca1a 100644
--- a/src/data.c
+++ b/src/data.c
@@ -2882,7 +2882,7 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args)
 	      if (BIGNUMP (val))
 		mpz_set (accum, XBIGNUM (val)->value);
 	      else
-		mpz_set_si (accum, XINT (val));
+		mpz_set_intmax (accum, XINT (val));
 	      if (nargs == 1)
 		mpz_neg (accum, accum);
 	    }
@@ -2905,7 +2905,7 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args)
 	      if (BIGNUMP (val))
 		mpz_set (accum, XBIGNUM (val)->value);
 	      else
-		mpz_set_si (accum, XINT (val));
+		mpz_set_intmax (accum, XINT (val));
 	    }
 	  else
 	    {
@@ -2933,7 +2933,8 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args)
 	  else
 	    {
 	      mpz_t tem;
-	      mpz_init_set_ui (tem, XUINT (val));
+	      mpz_init (tem);
+	      mpz_set_uintmax (tem, XUINT (val));
 	      mpz_and (accum, accum, tem);
 	      mpz_clear (tem);
 	    }
@@ -2944,7 +2945,8 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args)
 	  else
 	    {
 	      mpz_t tem;
-	      mpz_init_set_ui (tem, XUINT (val));
+	      mpz_init (tem);
+	      mpz_set_uintmax (tem, XUINT (val));
 	      mpz_ior (accum, accum, tem);
 	      mpz_clear (tem);
 	    }
@@ -2955,7 +2957,8 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args)
 	  else
 	    {
 	      mpz_t tem;
-	      mpz_init_set_ui (tem, XUINT (val));
+	      mpz_init (tem);
+	      mpz_set_uintmax (tem, XUINT (val));
 	      mpz_xor (accum, accum, tem);
 	      mpz_clear (tem);
 	    }
@@ -3092,7 +3095,8 @@ Both must be integers or markers.  */)
 	xmp = &XBIGNUM (x)->value;
       else
 	{
-	  mpz_init_set_si (xm, XINT (x));
+	  mpz_init (xm);
+	  mpz_set_intmax (xm, XINT (x));
 	  xmp = &xm;
 	}
 
@@ -3100,7 +3104,8 @@ Both must be integers or markers.  */)
 	ymp = &XBIGNUM (y)->value;
       else
 	{
-	  mpz_init_set_si (ym, XINT (y));
+	  mpz_init (ym);
+	  mpz_set_intmax (ym, XINT (y));
 	  ymp = &ym;
 	}
 
@@ -3163,7 +3168,8 @@ Both X and Y must be numbers or markers.  */)
 	xmp = &XBIGNUM (x)->value;
       else
 	{
-	  mpz_init_set_si (xm, XINT (x));
+	  mpz_init (xm);
+	  mpz_set_intmax (xm, XINT (x));
 	  xmp = &xm;
 	}
 
@@ -3171,7 +3177,8 @@ Both X and Y must be numbers or markers.  */)
 	ymp = &XBIGNUM (y)->value;
       else
 	{
-	  mpz_init_set_si (ym, XINT (y));
+	  mpz_init (ym);
+	  mpz_set_intmax (ym, XINT (y));
 	  ymp = &ym;
 	}
 
@@ -3317,10 +3324,11 @@ ash_lsh_impl (Lisp_Object value, Lisp_Object count, bool lsh)
       /* Just do the work as bignums to make the code simpler.  */
       mpz_t result;
       eassume (FIXNUMP (value));
+      mpz_init (result);
       if (lsh)
-	mpz_init_set_ui (result, XUINT (value));
+	mpz_set_uintmax (result, XUINT (value));
       else
-	mpz_init_set_si (result, XINT (value));
+	mpz_set_intmax (result, XINT (value));
       if (XINT (count) >= 0)
 	mpz_mul_2exp (result, result, XINT (count));
       else
@@ -3376,7 +3384,8 @@ Markers are converted to integers.  */)
       else
 	{
 	  mpz_t num;
-	  mpz_init_set_si (num, XINT (number) + 1);
+	  mpz_init (num);
+	  mpz_set_intmax (num, XINT (number) + 1);
 	  number = make_number (num);
 	  mpz_clear (num);
 	}
@@ -3410,7 +3419,8 @@ Markers are converted to integers.  */)
       else
 	{
 	  mpz_t num;
-	  mpz_init_set_si (num, XINT (number) - 1);
+	  mpz_init (num);
+	  mpz_set_intmax (num, XINT (number) - 1);
 	  number = make_number (num);
 	  mpz_clear (num);
 	}
diff --git a/src/emacs-module.c b/src/emacs-module.c
index 7709eeca94..83eccae1f7 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -536,7 +536,8 @@ module_make_integer (emacs_env *env, intmax_t n)
   if (FIXNUM_OVERFLOW_P (n))
     {
       mpz_t val;
-      mpz_init_set_si (val, n);
+      mpz_init (val);
+      mpz_set_uintmax (val, n);
       obj = make_number (val);
       mpz_clear (val);
     }
diff --git a/src/floatfns.c b/src/floatfns.c
index 9a5f0a3ad2..563c65f827 100644
--- a/src/floatfns.c
+++ b/src/floatfns.c
@@ -288,7 +288,8 @@ DEFUN ("abs", Fabs, Sabs, 1, 1, 0,
   else if (FIXNUMP (arg) && XINT (arg) == MOST_NEGATIVE_FIXNUM)
     {
       mpz_t val;
-      mpz_init_set_si (val, - MOST_NEGATIVE_FIXNUM);
+      mpz_init (val);
+      mpz_set_intmax (val, - MOST_NEGATIVE_FIXNUM);
       arg = make_number (val);
       mpz_clear (val);
     }
diff --git a/src/lisp.h b/src/lisp.h
index e046429c1b..4208634fa9 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3655,6 +3655,32 @@ extern Lisp_Object listn (enum constype, ptrdiff_t, Lisp_Object, ...);
 
 extern Lisp_Object make_bignum_str (const char *num, int base);
 extern Lisp_Object make_number (mpz_t value);
+extern void mpz_set_intmax_slow (mpz_t result, intmax_t v);
+extern void mpz_set_uintmax_slow (mpz_t result, uintmax_t v);
+
+INLINE void
+mpz_set_intmax (mpz_t result, intmax_t v)
+{
+  /* mpz_set_si works in terms of long, but Emacs may use a wider
+     integer type, and so sometimes will have to construct the mpz_t
+     by hand.  */
+  if (sizeof (intmax_t) > sizeof (long) && (long) v != v)
+    mpz_set_intmax_slow (result, v);
+  else
+    mpz_set_si (result, v);
+}
+
+INLINE void
+mpz_set_uintmax (mpz_t result, uintmax_t v)
+{
+  /* mpz_set_ui works in terms of unsigned long, but Emacs may use a
+     wider integer type, and so sometimes will have to construct the
+     mpz_t by hand.  */
+  if (sizeof (uintmax_t) > sizeof (unsigned long) && (unsigned long) v != v)
+    mpz_set_uintmax_slow (result, v);
+  else
+    mpz_set_ui (result, v);
+}
 
 /* Build a frequently used 2/3/4-integer lists.  */
 

Re: bignum branch

[Andy Moreton]

On Mon 16 Jul 2018, Tom Tromey wrote:

>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
>
> Tom> I was thinking this is what I' have emacs do when
> Tom> sizeof(EMACS_INT) > sizeof(long).
>
> Please try this patch.
>
> Unfortunately I don't know how I can test it locally

This builds ok on Windows with 64bit mingw64 (MSYS2), but still has a
few issues and some odd behaviour:
 - Some GCC warnings
 - A selection of marker related issues
 - A problem with the ccl.el compiler for CCL programs
 - Some test failures
 - eassert() failures in make_bignum_str() during tests.
   Possible issues are that its caller, string_to_number() does not 
   appear to handle the S2N_IGNORE_TRAILING flag for arguments that
   end up as bignums.
   It is also possible that string_to_number() has slightly different
   semantics for its `base' argument from mpz_set_str().
  


1) Some GCC warnings:

C:/emacs/git/emacs/bignum/src/search.c: In function 'Freplace_match':
C:/emacs/git/emacs/bignum/src/search.c:2651:15: warning: argument 1 value '2305843009213693951' exceeds maximum object size 2147483647 [-Walloc-size-larger-than=]
       substed = xmalloc (substed_alloc_size);
       ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from C:/emacs/git/emacs/bignum/src/search.c:24:0:
C:/emacs/git/emacs/bignum/src/lisp.h:4548:14: note: in a call to allocation function 'xmalloc' declared here
 extern void *xmalloc (size_t) ATTRIBUTE_MALLOC_SIZE ((1));
              ^~~~~~~

2) A selection of marker related issues:

In toplevel form:
../../../lisp/cedet/semantic/wisent/javat-wy.el:265:17:Error: Wrong type argument: number-or-marker-p, 1152921504606846975
In toplevel form:
../../../lisp/cedet/semantic/wisent/js-wy.el:185:17:Error: Wrong type argument: number-or-marker-p, 1152921504606846975
In toplevel form:
../../../lisp/cedet/semantic/wisent/python-wy.el:237:17:Error: Wrong type argument: number-or-marker-p, 1152921504606846975

Eager macro-expansion failure: (wrong-type-argument number-or-marker-p 1152921504606846975)
In toplevel form:
../../../lisp/cedet/semantic/wisent/python.el:38:1:Error: Wrong type argument: number-or-marker-p, 1152921504606846975


3) A problem with the ccl.el compiler for CCL programs. I think that
   this can be fixed by truncating the CCL code words output by the
   compiler in ccl.el to ensure that they doe not extend beyond the
   expected 28bit code word. The interpreter in ccl.c appears to range
   check the code words properly.

In toplevel form:
../../../lisp/obsolete/pgg.el:29:1:Error: Error in CCL program
In toplevel form:
../../../lisp/obsolete/pgg-gpg.el:32:1:Error: Error in CCL program
In toplevel form:
../../../lisp/obsolete/pgg-pgp.el:30:1:Error: Error in CCL program
In toplevel form:
../../../lisp/obsolete/pgg-pgp5.el:30:1:Error: Error in CCL program


4) Some test failures. I have omitted conditions for tests with similar
complaints about number-or-markerp.

Test test-calc-23889 condition:
    (wrong-type-argument number-or-marker-p 3568982627)
   FAILED  1/5  test-calc-23889 (0.084549 sec)

Test test-calc-convert-units condition:
    (ert-test-failed
     ((should
       (calc-tests-equal
	(calc-tests-simple ... "-1 m" nil "cm")
	'...))
      :form
      (calc-tests-equal nil
			(* -100
			   (var cm var-cm)))
      :value nil))
   FAILED  2/5  test-calc-convert-units (0.018381 sec)

Test test-calc-extract-units condition:
    (ert-test-failed
     ((should
       (calc-tests-equal
	(calc-tests-simple ... "-1 m")
	'...))
      :form
      (calc-tests-equal nil
			(var m var-m))
      :value nil))
   FAILED  3/5  test-calc-extract-units (0.001062 sec)

Test test-calc-remove-units condition:
    (ert-test-failed
     ((should
       (calc-tests-equal
	(calc-tests-simple ... "-1 m")
	-1))
      :form
      (calc-tests-equal nil -1)
      :value nil))
   FAILED  4/5  test-calc-remove-units (0.023494 sec)

Test test-math-bignum condition:
    (wrong-type-argument number-or-marker-p 2305843009)
   FAILED  5/5  test-math-bignum (0.000160 sec)

Test dired-test-bug25609 condition:
    (wrong-type-argument numberp #<marker in no buffer>)
   FAILED   3/11  dired-test-bug25609 (0.028504 sec)

Test viper-test-undo-2 condition:
    (error "Wrong type argument: numberp, #<marker at 3 in *viper-test-buffer*>")
   FAILED  4/6  viper-test-undo-2 (0.053162 sec)

Test eshell-test/command-running-p condition:
    (wrong-type-argument numberp #<marker at 29 in *eshell*>)
   FAILED   1/29  eshell-test/command-running-p (0.124705 sec)

Test number-sequence-test condition:
    (ert-test-failed
     ((should
       (=
	(length ...)
	2))
      :form
      (= 1 2)
      :value nil))
   FAILED   2/15  number-sequence-test (0.000085 sec)

Test simple-transpose-subr condition:
    (wrong-type-argument numberp #<marker in no buffer>)

Test bignum-abs condition:
    (ert-test-failed
     ((should
       (= most-positive-fixnum
	  (- ... 1)))
      :form
      (= 2305843009213693951 2305843009213693951)
      :value nil))

Test data-tests-/ condition:
    (ert-test-failed
     ((should
       (= most-positive-fixnum
	  (/ x 8)))
      :form
      (= 2305843009213693951 -1)
      :value nil))
   FAILED  22/41  data-tests-/ (0.000130 sec)

Test data-tests-1+ condition:
    (ert-test-failed
     ((should
       (fixnump
	(1+ ...)))
      :form
      (fixnump -2305843009213693952)
      :value nil))
   FAILED  23/41  data-tests-1+ (0.000151 sec)

Test data-tests-1- condition:
    (ert-test-failed
     ((should
       (fixnump
	(1- ...)))
      :form
      (fixnump 2305843009213693951)
      :value nil))
   FAILED  24/41  data-tests-1- (0.000166 sec)

Test data-tests-ash-lsh condition:
    (ert-test-failed
     ((should
       (=
	(ash most-negative-fixnum 1)
	(* most-negative-fixnum 2)))
      :form
      (= -4611686018427387904 0)
      :value nil))
   FAILED  30/41  data-tests-ash-lsh (0.000129 sec)


5) Assertion failures in tests. I've omitted the backtrace as it isn;t
much use without decoding it, and this post is long enough already. I
assume that the failing test is the next test after the previously
reported passing test.

   passed  33/41  data-tests-local-variable-watchers (0.000995 sec)
Backtrace:
...
make[2]: *** [Makefile:182: src/data-tests.log] Error 3


   passed  13/20  format-with-field (0.000145 sec)
Backtrace:
...
C:/emacs/git/emacs/bignum/src/alloc.c:3795: Emacs fatal error: assertion failed: check == 0
make[2]: *** [Makefile:182: src/editfns-tests.log] Error 3

Re: bignum branch

[Andy Moreton]

On Mon 16 Jul 2018, Andy Moreton wrote:

> On Mon 16 Jul 2018, Tom Tromey wrote:
>
>>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
>>
>> Tom> I was thinking this is what I' have emacs do when
>> Tom> sizeof(EMACS_INT) > sizeof(long).
>>
>> Please try this patch.
>>
>> Unfortunately I don't know how I can test it locally
>
> This builds ok on Windows with 64bit mingw64 (MSYS2), but still has a
> few issues and some odd behaviour:
>
> 3) A problem with the ccl.el compiler for CCL programs. I think that
>    this can be fixed by truncating the CCL code words output by the
>    compiler in ccl.el to ensure that they doe not extend beyond the
>    expected 28bit code word. The interpreter in ccl.c appears to range
>    check the code words properly.
>
> In toplevel form:
> ../../../lisp/obsolete/pgg.el:29:1:Error: Error in CCL program
> In toplevel form:
> ../../../lisp/obsolete/pgg-gpg.el:32:1:Error: Error in CCL program
> In toplevel form:
> ../../../lisp/obsolete/pgg-pgp.el:30:1:Error: Error in CCL program
> In toplevel form:
> ../../../lisp/obsolete/pgg-pgp5.el:30:1:Error: Error in CCL program

I think the following patch fixes the issues in CCL (compiler,
interpreter and disassembler). It truncates the compiled CCL code words
to 28 bits and then sign extends to ensure that the interpreter sees
signed values of constants (stored at the upper end of the code word).

I've done some simple testing with:

    ;; check compiler output
    (require 'pgg)
    pgg-parse-crc24
    (seq-map (lambda (x) (format "#x%x" x)) pgg-parse-crc24)

    ;; check disassembler
    (ccl-dump pgg-parse-crc24)

    ;; check interpreter
    (dolist (s '("foo" "bar" "baz"))
      (let ((crc (pgg-parse-crc24-string s)))
        (insert "\n" s " --> "
                (mapconcat (lambda (b) (format "%02x" b)) crc ""))))

The patch has been tested this on Windows 32bit and 64bit emacs builds
(mingw64 MSYS2) on master and the feature/bignum branch.

    AndyM

Fix CCL compiler and interpreter reliance on fixnum behaviour

	* lisp/international/ccl.el (ccl-embed-data)
        (ccl-embed-current-address): Truncate code word to 28 bits to
        ensure fixnum values in CCL compiler output.
        (ccl-get-next-code): Truncate code word to 28 bits and sign
	extend to fix output from ccl-dump.
	* src/ccl.c (GET_CCL_RANGE): Truncate code word to 28 bits and
	sign extend to ensure fixnum values in CCL interpreter.

diff --git a/lisp/international/ccl.el b/lisp/international/ccl.el
index d2f490d59c..e08d9fc55e 100644
--- a/lisp/international/ccl.el
+++ b/lisp/international/ccl.el
@@ -187,6 +187,7 @@ ccl-current-ic
 (defun ccl-embed-data (data &optional ic)
   "Embed integer DATA in `ccl-program-vector' at `ccl-current-ic' and
 increment it.  If IC is specified, embed DATA at IC."
+  (setq data (logand #x0fffffff data))
   (if ic
       (aset ccl-program-vector ic data)
     (let ((len (length ccl-program-vector)))
@@ -230,7 +231,8 @@ ccl-embed-current-address
 `ccl-program-vector' at IC without altering the other bit field."
   (let ((relative (- ccl-current-ic (1+ ic))))
     (aset ccl-program-vector ic
-	  (logior (aref ccl-program-vector ic) (ash relative 8)))))
+	  (logior (aref ccl-program-vector ic)
+                  (logand #x0fffffff (ash relative 8))))))
 
 (defun ccl-embed-code (op reg data &optional reg2)
   "Embed CCL code for the operation OP and arguments REG and DATA in
@@ -986,7 +988,9 @@ ccl-dump
 (defun ccl-get-next-code ()
   "Return a CCL code in `ccl-code' at `ccl-current-ic'."
   (prog1
-      (aref ccl-code ccl-current-ic)
+      (let ((code (aref ccl-code ccl-current-ic)))
+        ;; Truncate to 28 bit code word and sign extend
+        (- (logxor (logand #xfffffff code) #x8000000) #x8000000))
     (setq ccl-current-ic (1+ ccl-current-ic))))
 
 (defun ccl-dump-1 ()
diff --git a/src/ccl.c b/src/ccl.c
index 529b302ed9..80e9bcd98a 100644
--- a/src/ccl.c
+++ b/src/ccl.c
@@ -737,6 +737,9 @@ while (0)
   do								\
     {								\
       EMACS_INT prog_word = XINT ((ccl_prog)[ic]);		\
+      /* Truncate to 28 bit code word and sign extend */	\
+      prog_word = prog_word & 0x0fffffff;			\
+      prog_word = (prog_word ^ 0x08000000) - 0x08000000;	\
       if (! ASCENDING_ORDER (lo, prog_word, hi))		\
 	CCL_INVALID_CMD;					\
       (var) = prog_word;					\

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> I think the following patch fixes the issues in CCL (compiler,
Andy> interpreter and disassembler). It truncates the compiled CCL code words
Andy> to 28 bits and then sign extends to ensure that the interpreter sees
Andy> signed values of constants (stored at the upper end of the code word).

Would you mind pushing this to the bignum branch?
If it's inconvenient, let me know and I will do it.

thanks,
Tom

Re: bignum branch

[Andy Moreton]

On Sun 22 Jul 2018, Tom Tromey wrote:

>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>
> Andy> I think the following patch fixes the issues in CCL (compiler,
> Andy> interpreter and disassembler). It truncates the compiled CCL code words
> Andy> to 28 bits and then sign extends to ensure that the interpreter sees
> Andy> signed values of constants (stored at the upper end of the code word).
>
> Would you mind pushing this to the bignum branch?
> If it's inconvenient, let me know and I will do it.

I've done some more testing with the CCl program in the midi-kbd ELPA
package, and that shows that the patch is not correct.

I'll report back when I get it working on master and on the bignum
branch. The problems I reported with markers make debugging awkward on
the bignum branch.

    AndyM

Re: bignum branch

[Andy Moreton]

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

On Sun 22 Jul 2018, Andy Moreton wrote:

> On Sun 22 Jul 2018, Tom Tromey wrote:
>
>>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>>
>> Andy> I think the following patch fixes the issues in CCL (compiler,
>> Andy> interpreter and disassembler). It truncates the compiled CCL code words
>> Andy> to 28 bits and then sign extends to ensure that the interpreter sees
>> Andy> signed values of constants (stored at the upper end of the code word).
>>
>> Would you mind pushing this to the bignum branch?
>> If it's inconvenient, let me know and I will do it.
>
> I've done some more testing with the CCl program in the midi-kbd ELPA
> package, and that shows that the patch is not correct.
>
> I'll report back when I get it working on master and on the bignum
> branch. The problems I reported with markers make debugging awkward on
> the bignum branch.

After a lot more testing, I have a somewhat scruffy patch that works in
the following emacs builds on unpatched master, and on patched bignum branch:
 - cygwin 64bit (LP64 model)
 - mingw64 msys2 32bit
 - mingw64 msys2 64bit (LLP64 model)

The patch contains changes for:
 - fix CCL to ensure it uses 28biut codewords properly in bignum build
 - ensure make_number creates fixnums in LLP64 builds
 - fix bugnumcompare for LLP64 builds
 - fix arith_driver to handle markers correctly
 - fix arith_driver operations for LLP64 builds (more testing needed)
 - fix float_arith_driver to allow bignums
 - fix ash_lsh_impl to produce correct results in bignum build
 - fix NUMBERP to remove redundant BIGNUMP test (ensured by INTEGERP)

The patch has been tested with the attached ccl-tests.el ERT tests to
check that ash/lsh shifts behave properly, and that the CCL machinery
uses its 28bit codewords correctly in a bignum build.

Please check this works for you, and feel free to commit it to the
bignum branch.

    AndyM



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Bignum support fixes --]
[-- Type: text/x-patch, Size: 9886 bytes --]

diff --git a/lisp/international/ccl.el b/lisp/international/ccl.el
index d2f490d59c..3529dd9c30 100644
--- a/lisp/international/ccl.el
+++ b/lisp/international/ccl.el
@@ -184,11 +184,15 @@ ccl-program-vector
 (defvar ccl-current-ic 0
   "The current index for `ccl-program-vector'.")
 
+(defun ccl-fixnum (code)
+  "Convert a CCL code word to a fixnum value."
+  (- (logxor (logand code #x0fffffff) #x08000000) #x08000000))
+
 (defun ccl-embed-data (data &optional ic)
   "Embed integer DATA in `ccl-program-vector' at `ccl-current-ic' and
 increment it.  If IC is specified, embed DATA at IC."
   (if ic
-      (aset ccl-program-vector ic data)
+      (aset ccl-program-vector ic (ccl-fixnum data))
     (let ((len (length ccl-program-vector)))
       (if (>= ccl-current-ic len)
 	  (let ((new (make-vector (* len 2) nil)))
@@ -196,7 +200,7 @@ ccl-embed-data
 	      (setq len (1- len))
 	      (aset new len (aref ccl-program-vector len)))
 	    (setq ccl-program-vector new))))
-    (aset ccl-program-vector ccl-current-ic data)
+    (aset ccl-program-vector ccl-current-ic (ccl-fixnum data))
     (setq ccl-current-ic (1+ ccl-current-ic))))
 
 (defun ccl-embed-symbol (symbol prop)
@@ -230,7 +234,8 @@ ccl-embed-current-address
 `ccl-program-vector' at IC without altering the other bit field."
   (let ((relative (- ccl-current-ic (1+ ic))))
     (aset ccl-program-vector ic
-	  (logior (aref ccl-program-vector ic) (ash relative 8)))))
+	  (logior (aref ccl-program-vector ic)
+                  (ccl-fixnum (ash relative 8))))))
 
 (defun ccl-embed-code (op reg data &optional reg2)
   "Embed CCL code for the operation OP and arguments REG and DATA in
@@ -986,7 +991,8 @@ ccl-dump
 (defun ccl-get-next-code ()
   "Return a CCL code in `ccl-code' at `ccl-current-ic'."
   (prog1
-      (aref ccl-code ccl-current-ic)
+      (let ((code (aref ccl-code ccl-current-ic)))
+        (if (numberp code) (ccl-fixnum code) code))
     (setq ccl-current-ic (1+ ccl-current-ic))))
 
 (defun ccl-dump-1 ()
diff --git a/src/alloc.c b/src/alloc.c
index 1dc1bbb031..4c794048be 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -3815,6 +3815,34 @@ make_number (mpz_t value)
 	}
     }
 
+  /* Check if fixnum can be larger than long.  */
+  if (sizeof (EMACS_INT) > sizeof (long))
+    {
+      size_t bits = mpz_sizeinbase (value, 2);
+      int sign = mpz_sgn (value);
+
+      if (bits < FIXNUM_BITS + (sign < 0))
+        {
+          EMACS_INT v = 0;
+          size_t limbs = mpz_size(value);
+          mp_size_t i;
+
+          for (i = 0; i < limbs; i++)
+            {
+              mp_limb_t limb = mpz_getlimbn (value, i);
+              v |= (EMACS_INT) ((EMACS_UINT) limb << (i * GMP_NUMB_BITS));
+            }
+          if (sign < 0)
+            v = -v;
+
+          if (!FIXNUM_OVERFLOW_P (v))
+            {
+              XSETINT (obj, v);
+              return obj;
+            }
+        }
+    }
+
   obj = allocate_misc (Lisp_Misc_Bignum);
   b = XBIGNUM (obj);
   /* We could mpz_init + mpz_swap here, to avoid a copy, but the
diff --git a/src/data.c b/src/data.c
index 0deebdca1a..6ca868a938 100644
--- a/src/data.c
+++ b/src/data.c
@@ -2409,7 +2409,18 @@ bignumcompare (Lisp_Object num1, Lisp_Object num2,
       if (FLOATP (num2))
 	cmp = mpz_cmp_d (XBIGNUM (num1)->value, XFLOAT_DATA (num2));
       else if (FIXNUMP (num2))
-	cmp = mpz_cmp_si (XBIGNUM (num1)->value, XINT (num2));
+        {
+          if (sizeof (EMACS_INT) > sizeof(long) && XINT (num2) > LONG_MAX)
+            {
+              mpz_t tem;
+              mpz_init (tem);
+              mpz_set_intmax (tem, XINT (num2));
+              cmp = mpz_cmp (XBIGNUM (num1)->value, tem);
+              mpz_clear(tem);
+            }
+          else
+            cmp = mpz_cmp_si (XBIGNUM (num1)->value, XINT (num2));
+        }
       else
 	{
 	  eassume (BIGNUMP (num2));
@@ -2422,10 +2433,18 @@ bignumcompare (Lisp_Object num1, Lisp_Object num2,
       if (FLOATP (num1))
 	cmp = - mpz_cmp_d (XBIGNUM (num2)->value, XFLOAT_DATA (num1));
       else
-	{
-	  eassume (FIXNUMP (num1));
-	  cmp = - mpz_cmp_si (XBIGNUM (num2)->value, XINT (num1));
-	}
+        {
+          if (sizeof (EMACS_INT) > sizeof(long) && XINT (num1) > LONG_MAX)
+            {
+              mpz_t tem;
+              mpz_init (tem);
+              mpz_set_intmax (tem, XINT (num1));
+              cmp = - mpz_cmp (XBIGNUM (num2)->value, tem);
+              mpz_clear(tem);
+            }
+          else
+            cmp = - mpz_cmp_si (XBIGNUM (num2)->value, XINT (num1));
+        }
     }
 
   switch (comparison)
@@ -2860,7 +2879,7 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args)
     {
       /* Using args[argnum] as argument to CHECK_NUMBER... */
       val = args[argnum];
-      CHECK_NUMBER (val);
+      CHECK_NUMBER_COERCE_MARKER (val);
 
       if (FLOATP (val))
 	return unbind_to (count,
@@ -2871,7 +2890,15 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args)
 	case Aadd:
 	  if (BIGNUMP (val))
 	    mpz_add (accum, accum, XBIGNUM (val)->value);
-	  else if (XINT (val) < 0)
+	  else if (sizeof (EMACS_INT) > sizeof (long))
+            {
+	      mpz_t tem;
+	      mpz_init (tem);
+	      mpz_set_intmax (tem, XINT (val));
+	      mpz_add (accum, accum, tem);
+	      mpz_clear (tem);
+            }
+          else if (XINT (val) < 0)
 	    mpz_sub_ui (accum, accum, - XINT (val));
 	  else
 	    mpz_add_ui (accum, accum, XINT (val));
@@ -2888,6 +2915,14 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args)
 	    }
 	  else if (BIGNUMP (val))
 	    mpz_sub (accum, accum, XBIGNUM (val)->value);
+	  else if (sizeof (EMACS_INT) > sizeof (long))
+            {
+	      mpz_t tem;
+	      mpz_init (tem);
+	      mpz_set_intmax (tem, XINT (val));
+	      mpz_sub (accum, accum, tem);
+	      mpz_clear (tem);
+            }
 	  else if (XINT (val) < 0)
 	    mpz_add_ui (accum, accum, - XINT (val));
 	  else
@@ -2896,6 +2931,14 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args)
 	case Amult:
 	  if (BIGNUMP (val))
 	    mpz_mul (accum, accum, XBIGNUM (val)->value);
+	  else if (sizeof (EMACS_INT) > sizeof (long))
+            {
+	      mpz_t tem;
+	      mpz_init (tem);
+	      mpz_set_intmax (tem, XINT (val));
+	      mpz_mul (accum, accum, tem);
+	      mpz_clear (tem);
+            }
 	  else
 	    mpz_mul_si (accum, accum, XINT (val));
 	  break;
@@ -2915,6 +2958,14 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args)
 		xsignal0 (Qarith_error);
 	      if (BIGNUMP (val))
 		mpz_tdiv_q (accum, accum, XBIGNUM (val)->value);
+              else if (sizeof (EMACS_INT) > sizeof (long))
+                {
+                  mpz_t tem;
+                  mpz_init (tem);
+                  mpz_set_intmax (tem, XINT (val));
+                  mpz_tdiv_q (accum, accum, tem);
+                  mpz_clear (tem);
+                }
 	      else
 		{
 		  EMACS_INT value = XINT (val);
@@ -2982,8 +3033,9 @@ float_arith_driver (double accum, ptrdiff_t argnum, enum arithop code,
 
   for (; argnum < nargs; argnum++)
     {
-      val = args[argnum];    /* using args[argnum] as argument to CHECK_FIXNUM_... */
-      CHECK_FIXNUM_OR_FLOAT_COERCE_MARKER (val);
+      /* using args[argnum] as argument to CHECK_NUMBER_... */
+      val = args[argnum];
+      CHECK_NUMBER_COERCE_MARKER (val);
 
       if (FLOATP (val))
 	{
@@ -3277,7 +3329,7 @@ representation.  */)
 
   if (BIGNUMP (value))
     {
-      if (mpz_cmp_si (XBIGNUM (value)->value, 0) >= 0)
+      if (mpz_sgn (XBIGNUM (value)->value) >= 0)
 	return make_fixnum (mpz_popcount (XBIGNUM (value)->value));
       mpz_t tem;
       mpz_init (tem);
@@ -3314,8 +3366,10 @@ ash_lsh_impl (Lisp_Object value, Lisp_Object count, bool lsh)
       mpz_init (result);
       if (XINT (count) >= 0)
 	mpz_mul_2exp (result, XBIGNUM (value)->value, XINT (count));
-      else
+      else if (lsh)
 	mpz_tdiv_q_2exp (result, XBIGNUM (value)->value, - XINT (count));
+      else
+	mpz_fdiv_q_2exp (result, XBIGNUM (value)->value, - XINT (count));
       val = make_number (result);
       mpz_clear (result);
     }
@@ -3325,14 +3379,19 @@ ash_lsh_impl (Lisp_Object value, Lisp_Object count, bool lsh)
       mpz_t result;
       eassume (FIXNUMP (value));
       mpz_init (result);
-      if (lsh)
-	mpz_set_uintmax (result, XUINT (value));
-      else
-	mpz_set_intmax (result, XINT (value));
+
+      mpz_set_intmax (result, XINT (value));
+
       if (XINT (count) >= 0)
 	mpz_mul_2exp (result, result, XINT (count));
-      else
-	mpz_tdiv_q_2exp (result, result, - XINT (count));
+      else if (lsh)
+        if (mpz_sgn (result) > 0)
+          mpz_fdiv_q_2exp (result, result, - XINT (count));
+        else
+          mpz_fdiv_q_2exp (result, result, - XINT (count));
+      else /* ash */
+	mpz_fdiv_q_2exp (result, result, - XINT (count));
+
       val = make_number (result);
       mpz_clear (result);
     }
@@ -3414,7 +3473,7 @@ Markers are converted to integers.  */)
   else
     {
       eassume (FIXNUMP (number));
-      if (XINT (number) > MOST_POSITIVE_FIXNUM)
+      if (XINT (number) > MOST_NEGATIVE_FIXNUM)
 	XSETINT (number, XINT (number) - 1);
       else
 	{
diff --git a/src/lisp.h b/src/lisp.h
index 4208634fa9..b404f9d89a 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -2778,7 +2778,7 @@ NATNUMP (Lisp_Object x)
 INLINE bool
 NUMBERP (Lisp_Object x)
 {
-  return INTEGERP (x) || FLOATP (x) || BIGNUMP (x);
+  return INTEGERP (x) || FLOATP (x);
 }
 
 INLINE bool
@@ -2947,7 +2947,7 @@ CHECK_INTEGER (Lisp_Object x)
     if (MARKERP (x))							\
       XSETFASTINT (x, marker_position (x));				\
     else								\
-      CHECK_TYPE (FIXED_OR_FLOATP (x), Qnumber_or_marker_p, x);			\
+      CHECK_TYPE (FIXED_OR_FLOATP (x), Qnumber_or_marker_p, x);		\
   } while (false)
 
 #define CHECK_NUMBER_COERCE_MARKER(x)					\

[-- Attachment #3: Tests for bignum support and CCL fixes --]
[-- Type: application/emacs-lisp, Size: 7683 bytes --]

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 03 Aug 2018 01:43:17 +0100
> 
> After a lot more testing, I have a somewhat scruffy patch that works in
> the following emacs builds on unpatched master, and on patched bignum branch:
>  - cygwin 64bit (LP64 model)
>  - mingw64 msys2 32bit
>  - mingw64 msys2 64bit (LLP64 model)

I believe your changes also cover the 32-bit MinGW build with wide ints.

> The patch contains changes for:
>  - fix CCL to ensure it uses 28biut codewords properly in bignum build
>  - ensure make_number creates fixnums in LLP64 builds
>  - fix bugnumcompare for LLP64 builds
>  - fix arith_driver to handle markers correctly
>  - fix arith_driver operations for LLP64 builds (more testing needed)
>  - fix float_arith_driver to allow bignums
>  - fix ash_lsh_impl to produce correct results in bignum build
>  - fix NUMBERP to remove redundant BIGNUMP test (ensured by INTEGERP)

Can you tell what is the following hunk about?

> @@ -3414,7 +3473,7 @@ Markers are converted to integers.  */)
>    else
>      {
>        eassume (FIXNUMP (number));
> -      if (XINT (number) > MOST_POSITIVE_FIXNUM)
> +      if (XINT (number) > MOST_NEGATIVE_FIXNUM)
>  	XSETINT (number, XINT (number) - 1);
>        else
>  	{

Also, this:

> +(defun ccl-fixnum (code)
> +  "Convert a CCL code word to a fixnum value."
> +  (- (logxor (logand code #x0fffffff) #x08000000) #x08000000))

should have a comment explaining why this function is needed.

Btw, isn't it confusing that INTEGERP allows fixnums and bignums, but
XINT, XFASTINT, and XSETINT work only with fixnums?  I envision quite
a few of potential bugs due to this semantic discrepancy, like the one
you just fixed:

>  - fix NUMBERP to remove redundant BIGNUMP test (ensured by INTEGERP)

Should we have a different name for what is now INTEGERP?  Like
WHOLENUMP, for example?

Thanks again for working on this.

Re: bignum branch

[Andy Moreton]

On Fri 03 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 03 Aug 2018 01:43:17 +0100
>> 
>> After a lot more testing, I have a somewhat scruffy patch that works in
>> the following emacs builds on unpatched master, and on patched bignum branch:
>>  - cygwin 64bit (LP64 model)
>>  - mingw64 msys2 32bit
>>  - mingw64 msys2 64bit (LLP64 model)

I have now also tested
    - mingw64 msys2 32bit (wide int)

> I believe your changes also cover the 32-bit MinGW build with wide ints.

I expect so, but the build fails for 32bit MinGW builds on bignum branch
with gettimeofday problems. I have a feeling that this has already been
fixed on the master branch some time ago.

>> The patch contains changes for:
>>  - fix CCL to ensure it uses 28biut codewords properly in bignum build
>>  - ensure make_number creates fixnums in LLP64 builds
>>  - fix bugnumcompare for LLP64 builds
>>  - fix arith_driver to handle markers correctly
>>  - fix arith_driver operations for LLP64 builds (more testing needed)
>>  - fix float_arith_driver to allow bignums
>>  - fix ash_lsh_impl to produce correct results in bignum build
>>  - fix NUMBERP to remove redundant BIGNUMP test (ensured by INTEGERP)
>
> Can you tell what is the following hunk about?
>
>> @@ -3414,7 +3473,7 @@ Markers are converted to integers.  */)
>>    else
>>      {
>>        eassume (FIXNUMP (number));
>> -      if (XINT (number) > MOST_POSITIVE_FIXNUM)
>> +      if (XINT (number) > MOST_NEGATIVE_FIXNUM)
>>  	XSETINT (number, XINT (number) - 1);
>>        else
>>  	{

The updated Fadd1 checks if the old value is MOST_POSITIVE_FIXNUM before
incrementing, as that would need a bignum result.

This hunk in Fsub1 should check if the old value is MOST_NEGATIVE_FIXNUM
before decrementing, as that would need a bignum result.


> Also, this:
>
>> +(defun ccl-fixnum (code)
>> +  "Convert a CCL code word to a fixnum value."
>> +  (- (logxor (logand code #x0fffffff) #x08000000) #x08000000))

The CCL compiled codewords are 28bits, but the CCL implementation
assumes that the codewords are sign-extended, so that data constants in
the upper part of the codeword are signed. This function truncates a
codeword to 28bits, and then sign extends the result to a fixnum.

This ensures that the CCL compiler output is the same on master and
bignum branches.

> Btw, isn't it confusing that INTEGERP allows fixnums and bignums, but
> XINT, XFASTINT, and XSETINT work only with fixnums?  I envision quite
> a few of potential bugs due to this semantic discrepancy, like the one
> you just fixed:
>
>>  - fix NUMBERP to remove redundant BIGNUMP test (ensured by INTEGERP)

Indeed. NUMBERP could include rationals if emacs lisp ever acquires a full
scheme-like numeric tower.

> Should we have a different name for what is now INTEGERP?  Like
> WHOLENUMP, for example?

INTEGERP seems natural enough, and WHOLENUMP would perhaps be confused
with NATNUMP and FIXNATP.

If anything, perhaps XINT, XFASTINT and XSETINT should be XFIXNUM,
XFASTFIXNUM and XSETFIXNUM.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 03 Aug 2018 10:01:59 +0100
> 
> > I believe your changes also cover the 32-bit MinGW build with wide ints.
> 
> I expect so, but the build fails for 32bit MinGW builds on bignum branch
> with gettimeofday problems. I have a feeling that this has already been
> fixed on the master branch some time ago.

Could be, but can you show the error messages?

> > Can you tell what is the following hunk about?
> >
> >> @@ -3414,7 +3473,7 @@ Markers are converted to integers.  */)
> >>    else
> >>      {
> >>        eassume (FIXNUMP (number));
> >> -      if (XINT (number) > MOST_POSITIVE_FIXNUM)
> >> +      if (XINT (number) > MOST_NEGATIVE_FIXNUM)
> >>  	XSETINT (number, XINT (number) - 1);
> >>        else
> >>  	{
> 
> The updated Fadd1 checks if the old value is MOST_POSITIVE_FIXNUM before
> incrementing, as that would need a bignum result.
> 
> This hunk in Fsub1 should check if the old value is MOST_NEGATIVE_FIXNUM
> before decrementing, as that would need a bignum result.

So the previous code in Fsub1 had a bug?

> > Also, this:
> >
> >> +(defun ccl-fixnum (code)
> >> +  "Convert a CCL code word to a fixnum value."
> >> +  (- (logxor (logand code #x0fffffff) #x08000000) #x08000000))
> 
> The CCL compiled codewords are 28bits, but the CCL implementation
> assumes that the codewords are sign-extended, so that data constants in
> the upper part of the codeword are signed. This function truncates a
> codeword to 28bits, and then sign extends the result to a fixnum.
> 
> This ensures that the CCL compiler output is the same on master and
> bignum branches.

Yes, I know.  I was asking for a comment to that effect, so that
others won't wonder what this is about.

Re: bignum branch

[Andy Moreton]

On Fri 03 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 03 Aug 2018 10:01:59 +0100
>> 
>> > I believe your changes also cover the 32-bit MinGW build with wide ints.
>> 
>> I expect so, but the build fails for 32bit MinGW builds on bignum branch
>> with gettimeofday problems. I have a feeling that this has already been
>> fixed on the master branch some time ago.
>
> Could be, but can you show the error messages?

On the bignum ranch for 32bit MinGW I get:

make -C lib all
make[1]: Entering directory `/c/emacs/git/emacs/bignum/build/mingw32/lib'
  CC       gettimeofday.o
In file included from c:/emacs/git/emacs/bignum/lib/gettimeofday.c:23:0:
c:/emacs/git/emacs/bignum/nt/inc/sys/time.h:11:19: error: field 'it_interval' has incomplete type
   struct  timeval it_interval; /* timer interval */
                   ^~~~~~~~~~~
c:/emacs/git/emacs/bignum/nt/inc/sys/time.h:12:19: error: field 'it_value' has incomplete type
   struct  timeval it_value; /* current value */
                   ^~~~~~~~
c:/emacs/git/emacs/bignum/lib/gettimeofday.c:64:1: error: conflicting types for 'gettimeofday'
 gettimeofday (struct timeval *restrict tv, void *restrict tz)
 ^~~~~~~~~~~~
In file included from c:/emacs/git/emacs/bignum/nt/inc/sys/time.h:4:0,
                 from c:/emacs/git/emacs/bignum/lib/gettimeofday.c:23:
c:\mingw\include\sys\time.h:108:29: note: previous declaration of 'gettimeofday' was here
 int __cdecl __MINGW_NOTHROW gettimeofday
                             ^~~~~~~~~~~~
c:/emacs/git/emacs/bignum/lib/gettimeofday.c: In function 'gettimeofday':
c:/emacs/git/emacs/bignum/lib/gettimeofday.c:100:5: error: dereferencing pointer to incomplete type 'struct timeval'
   tv->tv_sec = microseconds_since_1970 / (ULONGLONG) 1000000;
     ^~
make[1]: *** [gettimeofday.o] Error 1
make[1]: Leaving directory `/c/emacs/git/emacs/bignum/build/mingw32/lib'
make: *** [lib] Error 2


>> > Can you tell what is the following hunk about?
>> >
>> >> @@ -3414,7 +3473,7 @@ Markers are converted to integers.  */)
>> >>    else
>> >>      {
>> >>        eassume (FIXNUMP (number));
>> >> -      if (XINT (number) > MOST_POSITIVE_FIXNUM)
>> >> +      if (XINT (number) > MOST_NEGATIVE_FIXNUM)
>> >>  	XSETINT (number, XINT (number) - 1);
>> >>        else
>> >>  	{
>> 
>> The updated Fadd1 checks if the old value is MOST_POSITIVE_FIXNUM before
>> incrementing, as that would need a bignum result.
>> 
>> This hunk in Fsub1 should check if the old value is MOST_NEGATIVE_FIXNUM
>> before decrementing, as that would need a bignum result.
>
> So the previous code in Fsub1 had a bug?

The code on master is fine, but there was a bug in the conversion to
bignums. It looks like the code in Fsub1 was copy-pasted from Fadd1
but not all of the differences got fixed up.

>
>> > Also, this:
>> >
>> >> +(defun ccl-fixnum (code)
>> >> +  "Convert a CCL code word to a fixnum value."
>> >> +  (- (logxor (logand code #x0fffffff) #x08000000) #x08000000))
>> 
>> The CCL compiled codewords are 28bits, but the CCL implementation
>> assumes that the codewords are sign-extended, so that data constants in
>> the upper part of the codeword are signed. This function truncates a
>> codeword to 28bits, and then sign extends the result to a fixnum.
>> 
>> This ensures that the CCL compiler output is the same on master and
>> bignum branches.
>
> Yes, I know.  I was asking for a comment to that effect, so that
> others won't wonder what this is about.

I'll update the patch to add a comment.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 03 Aug 2018 11:07:06 +0100
> 
> On the bignum ranch for 32bit MinGW I get:
> 
> make -C lib all
> make[1]: Entering directory `/c/emacs/git/emacs/bignum/build/mingw32/lib'
>   CC       gettimeofday.o
> In file included from c:/emacs/git/emacs/bignum/lib/gettimeofday.c:23:0:
> c:/emacs/git/emacs/bignum/nt/inc/sys/time.h:11:19: error: field 'it_interval' has incomplete type
>    struct  timeval it_interval; /* timer interval */
>                    ^~~~~~~~~~~
> c:/emacs/git/emacs/bignum/nt/inc/sys/time.h:12:19: error: field 'it_value' has incomplete type
>    struct  timeval it_value; /* current value */
>                    ^~~~~~~~
> c:/emacs/git/emacs/bignum/lib/gettimeofday.c:64:1: error: conflicting types for 'gettimeofday'
>  gettimeofday (struct timeval *restrict tv, void *restrict tz)
>  ^~~~~~~~~~~~
> In file included from c:/emacs/git/emacs/bignum/nt/inc/sys/time.h:4:0,
>                  from c:/emacs/git/emacs/bignum/lib/gettimeofday.c:23:
> c:\mingw\include\sys\time.h:108:29: note: previous declaration of 'gettimeofday' was here
>  int __cdecl __MINGW_NOTHROW gettimeofday
>                              ^~~~~~~~~~~~
> c:/emacs/git/emacs/bignum/lib/gettimeofday.c: In function 'gettimeofday':
> c:/emacs/git/emacs/bignum/lib/gettimeofday.c:100:5: error: dereferencing pointer to incomplete type 'struct timeval'
>    tv->tv_sec = microseconds_since_1970 / (ULONGLONG) 1000000;
>      ^~
> make[1]: *** [gettimeofday.o] Error 1
> make[1]: Leaving directory `/c/emacs/git/emacs/bignum/build/mingw32/lib'
> make: *** [lib] Error 2

Any idea how come these errors don't happen in the 64-bit MinGW64
build?  They use the same headers, so what is different that triggers
the errors only in the 32-bit build?

Re: bignum branch

[Andy Moreton]

On Fri 03 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 03 Aug 2018 11:07:06 +0100
>> 
>> On the bignum ranch for 32bit MinGW I get:
>> 
>> make -C lib all
>> make[1]: Entering directory `/c/emacs/git/emacs/bignum/build/mingw32/lib'
>>   CC       gettimeofday.o
>> In file included from c:/emacs/git/emacs/bignum/lib/gettimeofday.c:23:0:
>> c:/emacs/git/emacs/bignum/nt/inc/sys/time.h:11:19: error: field 'it_interval' has incomplete type
>>    struct  timeval it_interval; /* timer interval */
>>                    ^~~~~~~~~~~
>> c:/emacs/git/emacs/bignum/nt/inc/sys/time.h:12:19: error: field 'it_value' has incomplete type
>>    struct  timeval it_value; /* current value */
>>                    ^~~~~~~~
>> c:/emacs/git/emacs/bignum/lib/gettimeofday.c:64:1: error: conflicting types for 'gettimeofday'
>>  gettimeofday (struct timeval *restrict tv, void *restrict tz)
>>  ^~~~~~~~~~~~
>> In file included from c:/emacs/git/emacs/bignum/nt/inc/sys/time.h:4:0,
>>                  from c:/emacs/git/emacs/bignum/lib/gettimeofday.c:23:
>> c:\mingw\include\sys\time.h:108:29: note: previous declaration of 'gettimeofday' was here
>>  int __cdecl __MINGW_NOTHROW gettimeofday
>>                              ^~~~~~~~~~~~
>> c:/emacs/git/emacs/bignum/lib/gettimeofday.c: In function 'gettimeofday':
>> c:/emacs/git/emacs/bignum/lib/gettimeofday.c:100:5: error: dereferencing pointer to incomplete type 'struct timeval'
>>    tv->tv_sec = microseconds_since_1970 / (ULONGLONG) 1000000;
>>      ^~
>> make[1]: *** [gettimeofday.o] Error 1
>> make[1]: Leaving directory `/c/emacs/git/emacs/bignum/build/mingw32/lib'
>> make: *** [lib] Error 2
>
> Any idea how come these errors don't happen in the 64-bit MinGW64
> build?  They use the same headers, so what is different that triggers
> the errors only in the 32-bit build?

They don't happen in the 32bit MinGW (mingw.org) build on master either.

I assume that something got fixed on the master branch after the bignum
branch was created, and that the bignum branch has not been rebased
since then.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 03 Aug 2018 15:05:56 +0100
> 
> > Any idea how come these errors don't happen in the 64-bit MinGW64
> > build?  They use the same headers, so what is different that triggers
> > the errors only in the 32-bit build?
> 
> They don't happen in the 32bit MinGW (mingw.org) build on master either.
> 
> I assume that something got fixed on the master branch after the bignum
> branch was created, and that the bignum branch has not been rebased
> since then.

OK, let's wait until bignum is merged, and take it from there.

Thanks.

Re: bignum branch

[Andy Moreton]

On Fri 03 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 03 Aug 2018 15:05:56 +0100
>> 
>> > Any idea how come these errors don't happen in the 64-bit MinGW64
>> > build?  They use the same headers, so what is different that triggers
>> > the errors only in the 32-bit build?
>> 
>> They don't happen in the 32bit MinGW (mingw.org) build on master either.
>> 
>> I assume that something got fixed on the master branch after the bignum
>> branch was created, and that the bignum branch has not been rebased
>> since then.

If I apply the following from master to the bignum branch then a 32bit
MinGW build succeeds:

024d20f81e ("Fix compilation with mingw.org's MinGW 5.x headers")
bd52f37cae ("Fix last change: only MinGW runtime 5.0.2 and later needs
that.")

> OK, let's wait until bignum is merged, and take it from there.

Yes, that will fix it.

   AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 03 Aug 2018 20:54:34 +0100
> 
> If I apply the following from master to the bignum branch then a 32bit
> MinGW build succeeds:
> 
> 024d20f81e ("Fix compilation with mingw.org's MinGW 5.x headers")
> bd52f37cae ("Fix last change: only MinGW runtime 5.0.2 and later needs
> that.")

That's a surprise: I thought MinGW64 uses its own headers for 32-bit
builds, whereas the above commits fix problems specific to mingw.org's
headers.  At the time, I looked at the MinGW64 headers, and my
conclusion was that this particular problem doesn't exist there.

And the fix is conditioned on __MINGW32_VERSION, which AFAIK doesn't
exist in the MinGW64 headers.

You did use MinGW64 for the 32-bit build, right?  If so, can you help
me understand what I am missing here?

Re: bignum branch

[Andy Moreton]

On Sat 04 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 03 Aug 2018 20:54:34 +0100
>> 
>> If I apply the following from master to the bignum branch then a 32bit
>> MinGW build succeeds:
>> 
>> 024d20f81e ("Fix compilation with mingw.org's MinGW 5.x headers")
>> bd52f37cae ("Fix last change: only MinGW runtime 5.0.2 and later needs
>> that.")
>
> That's a surprise: I thought MinGW64 uses its own headers for 32-bit
> builds, whereas the above commits fix problems specific to mingw.org's
> headers.  At the time, I looked at the MinGW64 headers, and my
> conclusion was that this particular problem doesn't exist there.

We are miscommunicating.

The MSYS2 mingw64 builds (i686 and x86_64) all work correctly.
The problem is with the MinGW (mingw.org, i686 only) builds.

This would all be a lot less confusing if the two unreleated projects
had chosen more distinctive names.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 04 Aug 2018 12:14:02 +0100
> 
> On Sat 04 Aug 2018, Eli Zaretskii wrote:
> 
> >> From: Andy Moreton <andrewjmoreton@gmail.com>
> >> Date: Fri, 03 Aug 2018 20:54:34 +0100
> >> 
> >> If I apply the following from master to the bignum branch then a 32bit
> >> MinGW build succeeds:
> >> 
> >> 024d20f81e ("Fix compilation with mingw.org's MinGW 5.x headers")
> >> bd52f37cae ("Fix last change: only MinGW runtime 5.0.2 and later needs
> >> that.")
> >
> > That's a surprise: I thought MinGW64 uses its own headers for 32-bit
> > builds, whereas the above commits fix problems specific to mingw.org's
> > headers.  At the time, I looked at the MinGW64 headers, and my
> > conclusion was that this particular problem doesn't exist there.
> 
> We are miscommunicating.
> 
> The MSYS2 mingw64 builds (i686 and x86_64) all work correctly.
> The problem is with the MinGW (mingw.org, i686 only) builds.

Sorry for my misunderstanding, now everything is clear.

> This would all be a lot less confusing if the two unreleated projects
> had chosen more distinctive names.

Or didn't diverge in their headers.  Agreed.

Re: bignum branch

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> OK, let's wait until bignum is merged, and take it from there.

Two things still on my to-do list are to make sure the hash table eq
code is working correctly (I didn't update this and learned later that I
should); and that comparisons against NaN work properly (the GMP docs
say this is undefined, so we will need a special case).

I'm not sure what the semantics of NaN comparison are in Emacs.  In
particular, this doesn't really make sense to me:

    (> 0 0.0e+NaN) => nil
    (< 0 0.0e+NaN) => nil
    (min 0 0.0e+NaN) => 0.0e+NaN
    (min 0.0e+NaN 0) => 0.0e+NaN
    (max 0 0.0e+NaN) => 0.0e+NaN
    (max 0.0e+NaN 0) => 0.0e+NaN

Tom

Re: bignum branch

[Paul Eggert]

On 08/03/2018 01:17 PM, Tom Tromey wrote:
> I'm not sure what the semantics of NaN comparison are in Emacs.  In
> particular, this doesn't really make sense to me:
>
>     (> 0 0.0e+NaN) => nil
>     (< 0 0.0e+NaN) => nil
>     (min 0 0.0e+NaN) => 0.0e+NaN
>     (min 0.0e+NaN 0) => 0.0e+NaN
>     (max 0 0.0e+NaN) => 0.0e+NaN
>     (max 0.0e+NaN 0) => 0.0e+NaN

NaNs never compare numerically equal to, less than, or greater than any
other floating-point value, even other NaNs. This is part of the IEEE
standard.

min and max propagate any NaNs they find.

By the way, we've changed master so that eql now looks at NaN's
significands. That is, (eql x y) now returns t if x and y are NaNs
containing identical significands.

Re: bignum branch

[Tom Tromey]

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

Paul> NaNs never compare numerically equal to, less than, or greater than any
Paul> other floating-point value, even other NaNs. This is part of the IEEE
Paul> standard.

This part I get.

Paul> min and max propagate any NaNs they find.

This part I don't understand, since my mental mode of min/max is that
they are iteratively applying < or >

Anyway, it's fine with me, thanks for answering.  Maybe the min/max
behavior should be documented.

Tom

Re: bignum branch

[Paul Eggert]

Tom Tromey wrote:
> Paul> min and max propagate any NaNs they find.
> 
> This part I don't understand, since my mental mode of min/max is that
> they are iteratively applying < or >

The goal is more to have useful max and min functions than to have a particular 
implementation tactic. Returning a NaN is more useful, since it warns the caller 
that the min or max expression doesn't have a reasonable numeric interpretation.

Re: bignum branch

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 3 Aug 2018 18:22:10 -0700
> Cc: Eli Zaretskii <eliz@gnu.org>, Andy Moreton <andrewjmoreton@gmail.com>,
> 	emacs-devel@gnu.org
> 
> Tom Tromey wrote:
> > Paul> min and max propagate any NaNs they find.
> > 
> > This part I don't understand, since my mental mode of min/max is that
> > they are iteratively applying < or >
> 
> The goal is more to have useful max and min functions than to have a particular 
> implementation tactic. Returning a NaN is more useful, since it warns the caller 
> that the min or max expression doesn't have a reasonable numeric interpretation.

There's a certain tension here between people who are used to do IEEE
compliant FP math in other languages, and the rest of us.  The former
will want the IEEE semantics of NaNs, which is what surprised Tom; the
latter will probably be surprised like Tom was.

I don't see how we can fix this dilemma better than we already did,
with making sure eql compares NaNs as equal.  I do think we should
document the special behavior of NaNs, because many Emacs users will
not be aware of these subtleties.

Thanks.

Re: bignum branch

[Achim Gratz]

Eli Zaretskii writes:
> There's a certain tension here between people who are used to do IEEE
> compliant FP math in other languages, and the rest of us.  The former
> will want the IEEE semantics of NaNs, which is what surprised Tom; the
> latter will probably be surprised like Tom was.

The semantics of NaN have not much to do with IEEE754 and a lot with how
you do error handling, which shouldn't be a surprise to any programmer.

> I don't see how we can fix this dilemma better than we already did,
> with making sure eql compares NaNs as equal.  I do think we should
> document the special behavior of NaNs, because many Emacs users will
> not be aware of these subtleties.

Again, comparing the representations of an NaN (binary or otherwise) is
fair game.  The NaN itself, as long as it propagates through a chain of
numerical computations, needs to be preserved; otherwise it'd be an
exercise in futility to produce them in the first place.  If you don't
want to deal with NaN at all, there are other methods of handling
numerical domain errors, but they are usually worse (and often much more
so) than the alternative.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves

Re: bignum branch

[Eli Zaretskii]

> From: Achim Gratz <Stromeko@nexgo.de>
> Date: Sat, 04 Aug 2018 12:49:24 +0200
> 
> Eli Zaretskii writes:
> > There's a certain tension here between people who are used to do IEEE
> > compliant FP math in other languages, and the rest of us.  The former
> > will want the IEEE semantics of NaNs, which is what surprised Tom; the
> > latter will probably be surprised like Tom was.
> 
> The semantics of NaN have not much to do with IEEE754 and a lot with how
> you do error handling, which shouldn't be a surprise to any programmer.

It won't surprise those of us who had to deal with FP calculations.
I'm not so sure about others, because the semantics of FP exceptions
is not necessarily known to them.

> > I don't see how we can fix this dilemma better than we already did,
> > with making sure eql compares NaNs as equal.  I do think we should
> > document the special behavior of NaNs, because many Emacs users will
> > not be aware of these subtleties.
> 
> Again, comparing the representations of an NaN (binary or otherwise) is
> fair game.  The NaN itself, as long as it propagates through a chain of
> numerical computations, needs to be preserved; otherwise it'd be an
> exercise in futility to produce them in the first place.  If you don't
> want to deal with NaN at all, there are other methods of handling
> numerical domain errors, but they are usually worse (and often much more
> so) than the alternative.

Yes, I know.  Others may not be aware of all those subtleties, which
is exactly what I was saying in my original message.

Re: bignum branch

[Achim Gratz]

Tom Tromey writes:
> Paul> min and max propagate any NaNs they find.
>
> This part I don't understand, since my mental mode of min/max is that
> they are iteratively applying < or >

The point of having NaN at all is to set aside a tiny bit of the symbol
space for floating point numbers to encode certain error conditions and
propagate them in-band.  Otherwise they'd need to be signalled via side
channels like traps and exceptions, which usually significantly slow
down the error-free cases also or complicate the code even further.
Getting an NaN indicates that your computation has left the domain it
was defined in.  So no matter what you do, after you get an NaN this
indication must be preserved, hence all results must propagate NaN until
you get to the point where you do error handling.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: bignum branch

[Tom Tromey]

>>>>> "Achim" == Achim Gratz <Stromeko@nexgo.de> writes:

Achim> Tom Tromey writes:
Paul> min and max propagate any NaNs they find.
>> 
>> This part I don't understand, since my mental mode of min/max is that
>> they are iteratively applying < or >

Achim> The point of having NaN at all is to set aside a tiny bit of the symbol
Achim> space for floating point numbers to encode certain error conditions and
Achim> propagate them in-band.  Otherwise they'd need to be signalled via side
Achim> channels like traps and exceptions, which usually significantly slow
Achim> down the error-free cases also or complicate the code even further.
Achim> Getting an NaN indicates that your computation has left the domain it
Achim> was defined in.  So no matter what you do, after you get an NaN this
Achim> indication must be preserved, hence all results must propagate NaN until
Achim> you get to the point where you do error handling.

Yes, I understand.  What specifically is weird about this situation, for
me, is that min and max aren't like arithemetic operations like + or -.
And, the naive approach to writing min or max would not always
(depending on argument ordering) preserve NaN, because min and max,
presumably, are defined in terms of comparisons -- which always return
false with NaN.

Anyway, no big deal, it's easy to preserve these semantics with bignum
as well.

Tom

Re: bignum branch

[Achim Gratz]

Tom Tromey writes:
> Yes, I understand.  What specifically is weird about this situation, for
> me, is that min and max aren't like arithemetic operations like + or
> -.

As numerical comparison, they are defined to yield valid results over
some domain.  If one of the comparison partners is outside that domain,
then you can't give back a result.  It would be akin to declaring that
"yellow is heavier than an apple".

> And, the naive approach to writing min or max would not always
> (depending on argument ordering) preserve NaN, because min and max,
> presumably, are defined in terms of comparisons -- which always return
> false with NaN.

Yes, that needs to be special cased unless you do the sort in a way
that the NaN always ends up at the appropriate end, i.e. whatever your
comparison operator, it must preserve the NaN if it gives you back
"false".  One of the reasons that isnan() is a thing you'll find in
libraries…


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: bignum branch

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 3 Aug 2018 14:02:04 -0700
> Cc: Andy Moreton <andrewjmoreton@gmail.com>, emacs-devel@gnu.org
> 
> >     (> 0 0.0e+NaN) => nil
> >     (< 0 0.0e+NaN) => nil
> >     (min 0 0.0e+NaN) => 0.0e+NaN
> >     (min 0.0e+NaN 0) => 0.0e+NaN
> >     (max 0 0.0e+NaN) => 0.0e+NaN
> >     (max 0.0e+NaN 0) => 0.0e+NaN
> 
> NaNs never compare numerically equal to, less than, or greater than any
> other floating-point value, even other NaNs. This is part of the IEEE
> standard.

I find it easier to memorize the following rule: "Any comparison with
NaN yields FALSE."  YMMV.

Re: bignum branch

[Andy Moreton]

On Fri 03 Aug 2018, Tom Tromey wrote:

>>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
>
> Eli> OK, let's wait until bignum is merged, and take it from there.
>
> Two things still on my to-do list are to make sure the hash table eq
> code is working correctly (I didn't update this and learned later that I
> should); and that comparisons against NaN work properly (the GMP docs
> say this is undefined, so we will need a special case).

purecopy also needs updating tosupport bignums, and also the macros in
.gdbinit.

    AndyM

Re: bignum branch

[Tom Tromey]

Andy> purecopy also needs updating tosupport bignums

Yeah, I haven't really considered this.  Maybe the portable dumper could
land first :)

Tom

Re: bignum branch

[Robert Pluim]

Tom Tromey <tom@tromey.com> writes:

> Andy> purecopy also needs updating tosupport bignums
>
> Yeah, I haven't really considered this.  Maybe the portable dumper could
> land first :)

The portable dumper appears stalled (and it doesnʼt merge cleanly to
master anymore last time I checked),

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> purecopy also needs updating tosupport bignums

Did you have a use for this?  It seems like there must not be any
bignums being dumped currently, since if there were, surely something
would fail.

Anyway, if you do have a use, could you try the appended?


Andy> and also the macros in .gdbinit.

This doesn't seem like a must-have to me; but if it is, I think it would
be best to start by writing pretty-printers to submit to GMP.

Tom

diff --git a/src/alloc.c b/src/alloc.c
index 367bb73fc1..dba90e7eb2 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -5535,6 +5535,28 @@ make_pure_float (double num)
   return new;
 }
 
+/* Value is a bignum object with value VALUE allocated from pure
+   space.  */
+
+static Lisp_Object
+make_pure_bignum (struct Lisp_Bignum *value)
+{
+  Lisp_Object new;
+  size_t nbytes = value->value[0]._mp_alloc * sizeof (mp_limb_t);
+
+  struct Lisp_Bignum *b = pure_alloc (sizeof (struct Lisp_Bignum), Lisp_Misc);
+  b->type = Lisp_Misc_Bignum;
+
+  /* An mpz_t is an array of one element, so this is the correct way
+     to copy the contents.  */
+  b->value[0] = value->value[0];
+
+  b->value[0]._mp_d = pure_alloc (nbytes, -1);
+  memcpy (b->value[0]._mp_d, value->value[0]._mp_d, nbytes);
+
+  XSETMISC (new, b);
+  return new;
+}
 
 /* Return a vector with room for LEN Lisp_Objects allocated from
    pure space.  */
@@ -5676,6 +5698,8 @@ purecopy (Lisp_Object obj)
       /* Don't hash-cons it.  */
       return obj;
     }
+  else if (BIGNUMP (obj))
+    obj = make_pure_bignum (XBIGNUM (obj));
   else
     {
       AUTO_STRING (fmt, "Don't know how to purify: %S");

Re: bignum branch

[Andy Moreton]

On Mon 06 Aug 2018, Tom Tromey wrote:

>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>
> Andy> purecopy also needs updating tosupport bignums
>
> Did you have a use for this?  It seems like there must not be any
> bignums being dumped currently, since if there were, surely something
> would fail.
>
> Anyway, if you do have a use, could you try the appended?

I don't have a use for this yet - more a case of thing that should be
done before this branch is merged to master.

> Andy> and also the macros in .gdbinit.
>
> This doesn't seem like a must-have to me; but if it is, I think it would
> be best to start by writing pretty-printers to submit to GMP.

GMP already has a gmp_*printf family of functions which should be
suitable for formatted output. Fixing up .gdbinit is nice to have, but
not essential before getting the bignum branch merged into master.

> diff --git a/src/alloc.c b/src/alloc.c
> index 367bb73fc1..dba90e7eb2 100644
> --- a/src/alloc.c
> +++ b/src/alloc.c
> @@ -5535,6 +5535,28 @@ make_pure_float (double num)
>    return new;
>  }
>  
> +/* Value is a bignum object with value VALUE allocated from pure
> +   space.  */
> +
> +static Lisp_Object
> +make_pure_bignum (struct Lisp_Bignum *value)
> +{
> +  Lisp_Object new;
> +  size_t nbytes = value->value[0]._mp_alloc * sizeof (mp_limb_t);

This should use mpz_size.
> +
> +  struct Lisp_Bignum *b = pure_alloc (sizeof (struct Lisp_Bignum), Lisp_Misc);
> +  b->type = Lisp_Misc_Bignum;
> +
> +  /* An mpz_t is an array of one element, so this is the correct way
> +     to copy the contents.  */
> +  b->value[0] = value->value[0];
> +
> +  b->value[0]._mp_d = pure_alloc (nbytes, -1);
> +  memcpy (b->value[0]._mp_d, value->value[0]._mp_d, nbytes);

Use a loop with mpz_getlimbn or mpz_limbs_read to do this with the API.
mpz_roinit_n may be useful instead, using the low level APIs.
See (info "(gmp) Integer Special Functions").

> +
> +  XSETMISC (new, b);
> +  return new;
> +}
>  
>  /* Return a vector with room for LEN Lisp_Objects allocated from
>     pure space.  */
> @@ -5676,6 +5698,8 @@ purecopy (Lisp_Object obj)
>        /* Don't hash-cons it.  */
>        return obj;
>      }
> +  else if (BIGNUMP (obj))
> +    obj = make_pure_bignum (XBIGNUM (obj));
>    else
>      {
>        AUTO_STRING (fmt, "Don't know how to purify: %S");

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> This should use mpz_size.
[...]
Andy> Use a loop with mpz_getlimbn or mpz_limbs_read to do this with the API.
Andy> mpz_roinit_n may be useful instead, using the low level APIs.
Andy> See (info "(gmp) Integer Special Functions").

What do you think of this version?

I tested it by hacking a bignum defconst into subr.el and then
re-building.

Tom

diff --git a/src/alloc.c b/src/alloc.c
index 512fdadfb2..edfb87e5cd 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -5535,6 +5535,34 @@ make_pure_float (double num)
   return new;
 }
 
+/* Value is a bignum object with value VALUE allocated from pure
+   space.  */
+
+static Lisp_Object
+make_pure_bignum (struct Lisp_Bignum *value)
+{
+  Lisp_Object new;
+  size_t i, nlimbs = mpz_size (value->value);
+  size_t nbytes = nlimbs * sizeof (mp_limb_t);
+  mp_limb_t *pure_limbs;
+  mp_size_t new_size;
+
+  struct Lisp_Bignum *b = pure_alloc (sizeof (struct Lisp_Bignum), Lisp_Misc);
+  b->type = Lisp_Misc_Bignum;
+
+  pure_limbs = pure_alloc (nbytes, -1);
+  for (i = 0; i < nlimbs; ++i)
+    pure_limbs[i] = mpz_getlimbn (value->value, i);
+
+  new_size = nlimbs;
+  if (mpz_sgn (value->value) < 0)
+    new_size = -new_size;
+
+  mpz_roinit_n (b->value, pure_limbs, new_size);
+
+  XSETMISC (new, b);
+  return new;
+}
 
 /* Return a vector with room for LEN Lisp_Objects allocated from
    pure space.  */
@@ -5676,6 +5704,8 @@ purecopy (Lisp_Object obj)
       /* Don't hash-cons it.  */
       return obj;
     }
+  else if (BIGNUMP (obj))
+    obj = make_pure_bignum (XBIGNUM (obj));
   else
     {
       AUTO_STRING (fmt, "Don't know how to purify: %S");

Re: bignum branch

[Tom Tromey]

Tom> Two things still on my to-do list are to make sure the hash table eq
Tom> code is working correctly (I didn't update this and learned later that I
Tom> should); and that comparisons against NaN work properly (the GMP docs
Tom> say this is undefined, so we will need a special case).

I've implemented both of these and pushed them to the branch.

Tom

Re: bignum branch

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> Btw, isn't it confusing that INTEGERP allows fixnums and bignums, but
Eli> XINT, XFASTINT, and XSETINT work only with fixnums?  I envision quite
Eli> a few of potential bugs due to this semantic discrepancy, like the one
Eli> you just fixed:

>> - fix NUMBERP to remove redundant BIGNUMP test (ensured by INTEGERP)

Eli> Should we have a different name for what is now INTEGERP?  Like
Eli> WHOLENUMP, for example?

Paul suggested this and it made sense to me, as a bignum is an integer.
Perhaps one fix would be to take some of the renaming further and use
XFIXNUM, XFASTFIXNUM, and XSETFIXNUM.

Tom

Re: bignum branch

[Andy Moreton]

On Fri 03 Aug 2018, Tom Tromey wrote:

>>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
>
> Eli> Btw, isn't it confusing that INTEGERP allows fixnums and bignums, but
> Eli> XINT, XFASTINT, and XSETINT work only with fixnums?  I envision quite
> Eli> a few of potential bugs due to this semantic discrepancy, like the one
> Eli> you just fixed:
>
>>> - fix NUMBERP to remove redundant BIGNUMP test (ensured by INTEGERP)
>
> Eli> Should we have a different name for what is now INTEGERP?  Like
> Eli> WHOLENUMP, for example?
>
> Paul suggested this and it made sense to me, as a bignum is an integer.
> Perhaps one fix would be to take some of the renaming further and use
> XFIXNUM, XFASTFIXNUM, and XSETFIXNUM.

Yes. Also perhaps change XBIGNUM to:

    INLINE struct Lisp_Bignum *
    XBIGNUM (Lisp_Object a)
    {
      eassert (BIGNUMP (a));
      return XUNTAG (a, Lisp_Misc, struct Lisp_Bignum)->value;
    }

That allows "XBIGNUM(value)->value" to be replaced with "XBIGNUM(value)"
in all callers.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 03 Aug 2018 20:16:05 +0100
> 
> Yes. Also perhaps change XBIGNUM to:
> 
>     INLINE struct Lisp_Bignum *
>     XBIGNUM (Lisp_Object a)
>     {
>       eassert (BIGNUMP (a));
>       return XUNTAG (a, Lisp_Misc, struct Lisp_Bignum)->value;
>     }
> 
> That allows "XBIGNUM(value)->value" to be replaced with "XBIGNUM(value)"
> in all callers.

That would go against the convention with all the other Xfoo macros.

However, I see your point, and so perhaps an additional macro,
XINTEGER, could call either XINT or XBIGNUM()->value, depending on the
argument type?

Re: bignum branch

[Andy Moreton]

On Sat 04 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 03 Aug 2018 20:16:05 +0100
>> 
>> Yes. Also perhaps change XBIGNUM to:
>> 
>>     INLINE struct Lisp_Bignum *
>>     XBIGNUM (Lisp_Object a)
>>     {
>>       eassert (BIGNUMP (a));
>>       return XUNTAG (a, Lisp_Misc, struct Lisp_Bignum)->value;
>>     }
>> 
>> That allows "XBIGNUM(value)->value" to be replaced with "XBIGNUM(value)"
>> in all callers.
>
> That would go against the convention with all the other Xfoo macros.

True, the only thing with similar behaviour being xmint_pointer. While
inconsistent with the other Xfoo macros, it does reduce visual clutter
in the callers.

> However, I see your point, and so perhaps an additional macro,
> XINTEGER, could call either XINT or XBIGNUM()->value, depending on the
> argument type?

I'm not sure that would help, as callers still need to know if the
result is a bignum or fixnum to handle it correctly.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sun, 05 Aug 2018 12:36:13 +0100
> 
> >> That allows "XBIGNUM(value)->value" to be replaced with "XBIGNUM(value)"
> >> in all callers.
> >
> > That would go against the convention with all the other Xfoo macros.
> 
> True, the only thing with similar behaviour being xmint_pointer. While
> inconsistent with the other Xfoo macros, it does reduce visual clutter
> in the callers.

Yes, but then how do you access the C structure represented by a Lisp
bignum objects?  The usual way is XBIGNUM(bignum), from which you can
access members other than 'value'.  If XBIGNUM expands into the value,
we will need something else to do what XBIGNUM does now.

> > However, I see your point, and so perhaps an additional macro,
> > XINTEGER, could call either XINT or XBIGNUM()->value, depending on the
> > argument type?
> 
> I'm not sure that would help, as callers still need to know if the
> result is a bignum or fixnum to handle it correctly.

That's fine, we have a lot of code like

  if (NATNUMP (foo))
    x = XFASTINT (foo);

And sometimes the test is redundant, as its result is known in
advance.

Re: bignum branch

[Andy Moreton]

On Sun 05 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sun, 05 Aug 2018 12:36:13 +0100
>> 
>> >> That allows "XBIGNUM(value)->value" to be replaced with "XBIGNUM(value)"
>> >> in all callers.
>> >
>> > That would go against the convention with all the other Xfoo macros.
>> 
>> True, the only thing with similar behaviour being xmint_pointer. While
>> inconsistent with the other Xfoo macros, it does reduce visual clutter
>> in the callers.
>
> Yes, but then how do you access the C structure represented by a Lisp
> bignum objects?  The usual way is XBIGNUM(bignum), from which you can
> access members other than 'value'.  If XBIGNUM expands into the value,
> we will need something else to do what XBIGNUM does now.
>
>> > However, I see your point, and so perhaps an additional macro,
>> > XINTEGER, could call either XINT or XBIGNUM()->value, depending on the
>> > argument type?
>> 
>> I'm not sure that would help, as callers still need to know if the
>> result is a bignum or fixnum to handle it correctly.
>
> That's fine, we have a lot of code like
>
>   if (NATNUMP (foo))
>     x = XFASTINT (foo);

ok, then maybe XINTEGER would still be useful.
On the bignum branch that code would now be:

    if (FIXNATP (foo))
      x = XFASTINT (foo);

Perhaps XFASTINT should be renamed XFIXNAT to be consistent.

    AndyM

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> ok, then maybe XINTEGER would still be useful.
Andy> On the bignum branch that code would now be:
Andy>     if (FIXNATP (foo))
Andy>       x = XFASTINT (foo);
Andy> Perhaps XFASTINT should be renamed XFIXNAT to be consistent.

It's easy to do these kinds of renamings, since it's just a simple
"sed -i".

So, if Eli or Paul can let me know what they want, I can do it.

The main difficulty with these things is that they make the merges more
complicated.  One idea to simplify this would be to start a new branch
just before the merge to trunk, do the renamings there, then cherry-pick
the rest of the patches off the current branch.  Other ways might also
not be too hard; I suppose it depends on what kind of merges are
considered ok in Emacs development (I don't know).

As far as I know the branch is ready.  I have two uncommitted patches:
one for dumping (just sent) and one to use mpz_import (that I can't
test).  The second one seems like a minor improvement and could probably
be dropped.

Tom

Re: bignum branch

[Paul Eggert]

Tom Tromey wrote:
> So, if Eli or Paul can let me know what they want, I can do it.

I prefer consistent names in the new version, even if they don't match the old 
names. I suggest C names like INTEGERP, XINTEGER, FIXNUMP, XFIXNUM, BIGNUMP, and 
XBIGNUM, where 'integer' is the disjoint union of 'fixnum' and 'bignum'. 
Although this consistency will cost us a bit in the short run (due to renaming) 
and even in the long run (due to 'git diff' output being longer), it's worth it 
to keep the code readable.

Re: bignum branch

[Tom Tromey]

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

Paul> Tom Tromey wrote:
>> So, if Eli or Paul can let me know what they want, I can do it.

Paul> I prefer consistent names in the new version, even if they don't match
Paul> the old names. I suggest C names like INTEGERP, XINTEGER, FIXNUMP,
Paul> XFIXNUM, BIGNUMP, and XBIGNUM, where 'integer' is the disjoint union
Paul> of 'fixnum' and 'bignum'. Although this consistency will cost us a bit
Paul> in the short run (due to renaming) and even in the long run (due to
Paul> 'git diff' output being longer), it's worth it to keep the code
Paul> readable.

Seems reasonable to me.  The branch is partway there already (as noted);
but maybe all that's left is to rename XINT and XFASTINT.  So, XFIXNUM
and XFASTFIXNUM?

Also, are there any I'm missing?

Tom

Re: bignum branch

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> Also, are there any I'm missing?

... then I found XUINT as well.  I guess => XUFIXNUM.

Tom

Re: bignum branch

[Andy Moreton]

On Mon 06 Aug 2018, Tom Tromey wrote:

>>>>>> "Paul" == Paul Eggert <eggert@cs.ucla.edu> writes:
>
> Paul> Tom Tromey wrote:
>>> So, if Eli or Paul can let me know what they want, I can do it.
>
> Paul> I prefer consistent names in the new version, even if they don't match
> Paul> the old names. I suggest C names like INTEGERP, XINTEGER, FIXNUMP,
> Paul> XFIXNUM, BIGNUMP, and XBIGNUM, where 'integer' is the disjoint union
> Paul> of 'fixnum' and 'bignum'. Although this consistency will cost us a bit
> Paul> in the short run (due to renaming) and even in the long run (due to
> Paul> 'git diff' output being longer), it's worth it to keep the code
> Paul> readable.
>
> Seems reasonable to me.  The branch is partway there already (as noted);
> but maybe all that's left is to rename XINT and XFASTINT.  So, XFIXNUM
> and XFASTFIXNUM?

XFASTINT should be XFIXNAT, as the matching predicate is FIXNATP.

    AndyM

Re: bignum branch

[Paul Eggert]

Tom Tromey wrote:
> So, XFIXNUM
> and XFASTFIXNUM?

I'd get rid of XFASTINT. Its main reason for existing was faster extraction with 
MSB tagging, but we don't do MSB any more (except for --with-wide-int, where the 
faster extraction is not that much faster anyway).

Re: bignum branch

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 7 Aug 2018 09:53:19 -0700
> Cc: Andy Moreton <andrewjmoreton@gmail.com>, emacs-devel@gnu.org
> 
> Tom Tromey wrote:
> > So, XFIXNUM
> > and XFASTFIXNUM?
> 
> I'd get rid of XFASTINT. Its main reason for existing was faster extraction with 
> MSB tagging, but we don't do MSB any more (except for --with-wide-int, where the 
> faster extraction is not that much faster anyway).

I see no reason yet to get rid of XFASTINT.  Certainly not because we
need to come up with one more macro name.

Re: bignum branch

[Paul Eggert]

Eli Zaretskii wrote:
> I see no reason yet to get rid of XFASTINT.  Certainly not because we
> need to come up with one more macro name.

The name is just the straw that broke the camel's back. We haven't needed 
XFASTINT for years, the distinction between XINT and XFASTINT is more hassle 
than it's worth, and maintaining the distinction with XFASTFIXNUM and 
XFASTINTEGER and XFASTBIGNUM would compound the hassle.

The main reason to keep XFASTINT was inertia. As long as we're redoing XFASTINT 
calls anyway we might as well change them to XINTEGER or XFIXNUM (whichever is 
applicable), and clean out the XFAST... cruft.

Re: bignum branch

[Tom Tromey]

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

Paul> Eli Zaretskii wrote:
>> I see no reason yet to get rid of XFASTINT.  Certainly not because we
>> need to come up with one more macro name.

Paul> The name is just the straw that broke the camel's back. We haven't
Paul> needed XFASTINT for years, the distinction between XINT and XFASTINT
Paul> is more hassle than it's worth, and maintaining the distinction with
Paul> XFASTFIXNUM and XFASTINTEGER and XFASTBIGNUM would compound the
Paul> hassle.

Paul> The main reason to keep XFASTINT was inertia. As long as we're redoing
Paul> XFASTINT calls anyway we might as well change them to XINTEGER or
Paul> XFIXNUM (whichever is applicable), and clean out the XFAST... cruft.

While I tend to agree that it makes sense to remove XFASTINT, I think I
would rather it be a separate patch from the bignum branch.  So, I used
the name Andy suggested.

Tom

Re: bignum branch

[Andy Moreton]

On Mon 06 Aug 2018, Tom Tromey wrote:

>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>
> Andy> ok, then maybe XINTEGER would still be useful.
> Andy> On the bignum branch that code would now be:
> Andy>     if (FIXNATP (foo))
> Andy>       x = XFASTINT (foo);
> Andy> Perhaps XFASTINT should be renamed XFIXNAT to be consistent.
>
> It's easy to do these kinds of renamings, since it's just a simple
> "sed -i".
>
> So, if Eli or Paul can let me know what they want, I can do it.

From other comments in this thread, it seems there is agreement that
ending up with consistent naming is desirable. 

> As far as I know the branch is ready.  I have two uncommitted patches:
> one for dumping (just sent) and one to use mpz_import (that I can't
> test).  The second one seems like a minor improvement and could probably
> be dropped.

The mpz_import patch [1] results in appears to cause a problem in the CCL
tests. I'll report back when I have time to look into it further.

I haven't yet tested the purecopy patch [2], as nothing needs it yet.
The patch also uses the underlying representation rather than using the
low level API, so I think the patch still needs some work. However it
should not block merging to master.

    AndyM

[1] mpz_import patch:
http://lists.gnu.org/archive/html/emacs-devel/2018-08/msg00103.html

[2] purecopy patch:
http://lists.gnu.org/archive/html/emacs-devel/2018-08/msg00182.html

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> The mpz_import patch [1] results in appears to cause a problem in the CCL
Andy> tests. I'll report back when I have time to look into it further.

Thanks.  This isn't a major issue, but the current code does cause a
compiler warning in alloc.c on my machine -- the code in question won't
ever be run here, but gcc complains about an overlong shift.  The
mpz_import approach would avoid this.

Tom

Re: bignum branch

[Andy Moreton]

On Tue 07 Aug 2018, Tom Tromey wrote:

>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>
> Andy> The mpz_import patch [1] results in appears to cause a problem in the CCL
> Andy> tests. I'll report back when I have time to look into it further.
>
> Thanks.  This isn't a major issue, but the current code does cause a
> compiler warning in alloc.c on my machine -- the code in question won't
> ever be run here, but gcc complains about an overlong shift.  The
> mpz_import approach would avoid this.

I think the problem was pilot error. I have now run the CCL tests with
the mpz_import patch without problems on the following Windows builds:
1) mingw64 x86_64 (MSYS2)
2) mingw64 i686   (MSYS2)
3) mingw32 i686 (mingw.org)
   This works after also applying commit 024d20f81e from master, needed
   to fix build problems (i.e. the problem will be fixed by merging into
   master, or rebasing the bignum branch).

I have not tried a 32bit wide int build, but I would expect that they
should work given that (1) works.

    AndyM

Re: bignum branch

[Andy Moreton]

On Mon 06 Aug 2018, Tom Tromey wrote:
> As far as I know the branch is ready.  I have two uncommitted patches:
> one for dumping (just sent) and one to use mpz_import (that I can't
> test).  The second one seems like a minor improvement and could probably
> be dropped.

Have you run "make check" on the bignum branch ?

I see various test failures (many of which are also failures on master),
and three crashes:

1) A crash in the json tests
   This is bug#32381, fixed on master in commit 3eac378c96.

2) An eassert in make_bignum_str called from string_to_number.
   This appears to be caused by differences in the syntax accepted by
   emacs lisp and by mpz_set_str.  To reproduce:

     ELISP> (format "+%S" (- most-negative-fixnum))
     "+2305843009213693952"
     ELISP> (string-to-number "+2305843009213693952")
     ### crash ###

   It also appears that make_bignum_str in alloc.c is only used in 
   string_to_number, and so should be open coded in its caller.

3) Another crash which I have not yet had time to analyse with gdb.

    AndyM

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> Have you run "make check" on the bignum branch ?

No, but I'll do that.

Andy> I see various test failures (many of which are also failures on master),

Still wondering about git merge -vs- git rebase for this branch.

Andy>    It also appears that make_bignum_str in alloc.c is only used in 
Andy>    string_to_number, and so should be open coded in its caller.

That would require exposing allocate_misc, which I would rather not do.

Tom

Re: bignum branch

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Date: Wed, 08 Aug 2018 17:14:25 -0600
> Cc: emacs-devel@gnu.org
> 
> Still wondering about git merge -vs- git rebase for this branch.

If you are wondering because you are not sure what are our policy,
then you should know we don't have one.  Some people merge, others
rebase, still others do a mix.  So it's up to you, I think.

Re: bignum branch

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Still wondering about git merge -vs- git rebase for this branch.
>
> If you are wondering because you are not sure what are our policy,
> then you should know we don't have one.  Some people merge, others
> rebase, still others do a mix.  So it's up to you, I think.

emacswiki recommends "git merge", see
<https://www.emacswiki.org/emacs/GitForEmacsDevs#toc13>.

Maybe we shall add a similar section (Working in a Task Branch) to
admin/notes/git-workflow, be it "git merge" or "git rebase" based. Or
simply link to emacswiki.

Best regards, Michael.

Re: bignum branch

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Tom Tromey <tom@tromey.com>,  andrewjmoreton@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 09 Aug 2018 09:59:56 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Still wondering about git merge -vs- git rebase for this branch.
> >
> > If you are wondering because you are not sure what are our policy,
> > then you should know we don't have one.  Some people merge, others
> > rebase, still others do a mix.  So it's up to you, I think.
> 
> emacswiki recommends "git merge", see
> <https://www.emacswiki.org/emacs/GitForEmacsDevs#toc13>.

That's a recommendation, not a mandatory policy.  People have been
doing things differently right and left.

> Maybe we shall add a similar section (Working in a Task Branch) to
> admin/notes/git-workflow, be it "git merge" or "git rebase" based. Or
> simply link to emacswiki.

We already say that in CONTRIBUTE.  (I don't really understand why
git-workflow exists, but at the time people objected to delete it.)

Re: bignum branch

[Paul Eggert]

Eli Zaretskii wrote:
>> emacswiki recommends "git merge", see
>> <https://www.emacswiki.org/emacs/GitForEmacsDevs#toc13>.
> That's a recommendation, not a mandatory policy.  People have been
> doing things differently right and left.

For what it's worth, I almost always rebase rather than merge, as this makes it 
easier for later maintainers to understand the changes.

There's a tension here between maintaining metadata (that is, info about who 
made a change and when and in what context), versus maintaining data (that is, 
the software itself). Merging prioritizes metadata, whereas rebasing prioritizes 
data. When I'm spelunking through development history I'm almost always more 
interested in data, so I prefer changes to be rebased.

Re: bignum branch

[Eli Zaretskii]

> Cc: tom@tromey.com, andrewjmoreton@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 9 Aug 2018 10:31:41 -0700
> 
> For what it's worth, I almost always rebase rather than merge, as this makes it 
> easier for later maintainers to understand the changes.
> 
> There's a tension here between maintaining metadata (that is, info about who 
> made a change and when and in what context), versus maintaining data (that is, 
> the software itself). Merging prioritizes metadata, whereas rebasing prioritizes 
> data. When I'm spelunking through development history I'm almost always more 
> interested in data, so I prefer changes to be rebased.

To each their own.  Here's an opposite data point: a week or so ago, a
question asked on reddit required me to recall why I made a certain
minor change while developing the native line numbers.  Fortunately, I
merged my feature branch in its entirety, and so I could still see the
commit on the branch which introduced the change together with its log
message which explained why I did that.

Re: bignum branch

[Stefan Monnier]

> For what it's worth, I almost always rebase rather than merge, as this makes
> it easier for later maintainers to understand the changes.

Of course, the right answer is that our tools shouldn't make us choose.


        Stefan

Re: bignum branch

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> From: Tom Tromey <tom@tromey.com>
>> Date: Wed, 08 Aug 2018 17:14:25 -0600
>> Cc: emacs-devel@gnu.org
>> 
>> Still wondering about git merge -vs- git rebase for this branch.

Eli> If you are wondering because you are not sure what are our policy,
Eli> then you should know we don't have one.  Some people merge, others
Eli> rebase, still others do a mix.  So it's up to you, I think.

Ok, thanks.

Any preferences about keeping the feature branch?  Should I remove it
once everything is in?

I am somewhat inclined to pursue the approach I mentioned earlier: make
a new branch, run the renaming commands there, then cherry-pick the rest
of the bignum changes on top.  Then, the final branch could be merged to
master.  This would avoid a messy merge from trunk to the branch and
then back.

Perhaps if the lisp-misc removal goes in earlier a bit more work will be
required in here.

I could either push a new branch or push -f to the existing branch if
someone wants to do a final review.

Tom

Re: bignum branch

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>,  andrewjmoreton@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 09 Aug 2018 10:34:59 -0600
> 
> Eli> If you are wondering because you are not sure what are our policy,
> Eli> then you should know we don't have one.  Some people merge, others
> Eli> rebase, still others do a mix.  So it's up to you, I think.
> 
> Ok, thanks.
> 
> Any preferences about keeping the feature branch?  Should I remove it
> once everything is in?

I think feature branches are removed once the feature lands on master,
yes.

> I am somewhat inclined to pursue the approach I mentioned earlier: make
> a new branch, run the renaming commands there, then cherry-pick the rest
> of the bignum changes on top.  Then, the final branch could be merged to
> master.  This would avoid a messy merge from trunk to the branch and
> then back.

That'd be fine.

> I could either push a new branch or push -f to the existing branch if
> someone wants to do a final review.

I think pushing a new branch is slightly better.

Re: bignum branch

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I am somewhat inclined to pursue the approach I mentioned earlier: make
Tom> a new branch, run the renaming commands there, then cherry-pick the rest
Tom> of the bignum changes on top.  Then, the final branch could be merged to
Tom> master.  This would avoid a messy merge from trunk to the branch and
Tom> then back.

I see there was a push to the feature branch to fix up the *.m files for
the big renamings -- something I forgot to do.  So maybe a merge from
master and then back again is preferable after all, to try a bit harder
to avoid regressing anything.

Tom

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> Have you run "make check" on the bignum branch ?

I did this and fixed the problems I saw.

Andy> 1) A crash in the json tests
Andy>    This is bug#32381, fixed on master in commit 3eac378c96.

I didn't see this, but since it is unrelated to the branch, I wouldn't
have anyway.

I just ran "make check" at the top level.  Is there something else I
should do?

Andy> 2) An eassert in make_bignum_str called from string_to_number.
Andy>    This appears to be caused by differences in the syntax accepted by
Andy>    emacs lisp and by mpz_set_str.  To reproduce:
ELISP> (format "+%S" (- most-negative-fixnum))
Andy>      "+2305843009213693952"
ELISP> (string-to-number "+2305843009213693952")
Andy>      ### crash ###

I fixed this.

Andy> 3) Another crash which I have not yet had time to analyse with gdb.

I didn't see this.  However, emacs-module-tests.el had a test failure;
I've updated the test.

I've pushed all the patches I have.

Tom

Re: bignum branch

[Andy Moreton]

On Wed 08 Aug 2018, Tom Tromey wrote:

>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>
> Andy> Have you run "make check" on the bignum branch ?
>
> I did this and fixed the problems I saw.
>
> Andy> 1) A crash in the json tests
> Andy>    This is bug#32381, fixed on master in commit 3eac378c96.
>
> I didn't see this, but since it is unrelated to the branch, I wouldn't
> have anyway.

Tis only happens if you configure with --with-json to use the jansson
library for JSON parsing.

> Andy> 2) An eassert in make_bignum_str called from string_to_number.
> Andy>    This appears to be caused by differences in the syntax accepted by
> Andy>    emacs lisp and by mpz_set_str.  To reproduce:
> ELISP> (format "+%S" (- most-negative-fixnum))
> Andy>      "+2305843009213693952"
> ELISP> (string-to-number "+2305843009213693952")
> Andy>      ### crash ###
>
> I fixed this.

Thanks - retesting on Windows mingw64 x86_64 shows this is fixed.

> Andy> 3) Another crash which I have not yet had time to analyse with gdb.

This crashes in data-tests-logcount from test/src/data-tests.el, so there is
a problem in logcount.

> I didn't see this.  However, emacs-module-tests.el had a test failure;
> I've updated the test.
>
> I've pushed all the patches I have.

Once the logcount problem is fixed, this is looking ready to merge.

    AndyM

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> This crashes in data-tests-logcount from test/src/data-tests.el, so there is
Andy> a problem in logcount.

I couldn't reproduce this, so I would appreciate it if you could look
into it.  Thanks.

Tom

Re: bignum branch

[Andy Moreton]

On Wed 08 Aug 2018, Tom Tromey wrote:

>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>
> Andy> This crashes in data-tests-logcount from test/src/data-tests.el, so there is
> Andy> a problem in logcount.
>
> I couldn't reproduce this, so I would appreciate it if you could look
> into it.  Thanks.

Sure - I was reporting what I have so far.
 - cygwin x86_64 (LP64 model) is ok
 - mingw64 x86_64 (LLP64 model) fails
 - mingw64 i686 is ok

The problem is in mpz_popcount in the GMP library from MSYS2 x86_64,
which segfaults when calling mpn_popcount. I'll report back when I have
more time to investigate further.

    AndyM

Re: bignum branch

[Andy Moreton]

On Wed 08 Aug 2018, Tom Tromey wrote:

>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>
> Andy> This crashes in data-tests-logcount from test/src/data-tests.el, so there is
> Andy> a problem in logcount.
>
> I couldn't reproduce this, so I would appreciate it if you could look
> into it.  Thanks.

Testing on several builds:
 - cygwin x86_64 is ok
 - mingw64 x86_64 crashes in logcount
 - mingw64 i686 is ok

Looking further into this, I think it is an MSYS2 packaging bug. The
MSYS2 mingw-w64-x86_64-gmp package is built using this script:

https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-gmp/PKGBUILD

That script builds both a static library and a shared library, but only
installs one of the gmp.h headers from the builds.

The GMP static and shared library builds generate slightly different
gmp.h headers, so both should be installed in separate locations.

c:/msys64/mingw64/include/gmp.h has this (package version 6.1.2-1):

    /* Instantiated by configure. */
    #if ! defined (__GMP_WITHIN_CONFIGURE)
    #define _LONG_LONG_LIMB 1
    #define __GMP_LIBGMP_DLL  0
    #endif

That is suitable for linking to a static library. If I change it to look
like this:

    /* Instantiated by configure. */
    #if ! defined (__GMP_WITHIN_CONFIGURE)
    #define _LONG_LONG_LIMB 1
    #define __GMP_LIBGMP_DLL  1
    #endif

After rebuilding emacs, logcount works, and data-tests passes.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Thu, 09 Aug 2018 21:49:46 +0100
> 
> c:/msys64/mingw64/include/gmp.h has this (package version 6.1.2-1):
> 
>     /* Instantiated by configure. */
>     #if ! defined (__GMP_WITHIN_CONFIGURE)
>     #define _LONG_LONG_LIMB 1
>     #define __GMP_LIBGMP_DLL  0
>     #endif
> 
> That is suitable for linking to a static library. If I change it to look
> like this:
> 
>     /* Instantiated by configure. */
>     #if ! defined (__GMP_WITHIN_CONFIGURE)
>     #define _LONG_LONG_LIMB 1
>     #define __GMP_LIBGMP_DLL  1
>     #endif
> 
> After rebuilding emacs, logcount works, and data-tests passes.

I'd actually prefer us to link against GMP statically, at least on
Windows, because GMP usually gets replaced when you install a new
version of GCC.  So linking statically runs a lower risk of a "DLL
hell".  It also makes the Emacs binary self-contained and more easily
movable.

Why is it a problem with linking statically against GMP?  Could it be
tat your libgmp.a is from a different GMP version, i.e. incompatible
with the GMP headers you have installed?  Or maybe some other optional
library depends on GMP and causes conflicts?  If not, I don't
understand why the results should depend on how we linked the library.

Thanks.

Re: bignum branch

[Andy Moreton]

On Fri 10 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Thu, 09 Aug 2018 21:49:46 +0100
>> 
>> c:/msys64/mingw64/include/gmp.h has this (package version 6.1.2-1):
>> 
>>     /* Instantiated by configure. */
>>     #if ! defined (__GMP_WITHIN_CONFIGURE)
>>     #define _LONG_LONG_LIMB 1
>>     #define __GMP_LIBGMP_DLL  0
>>     #endif
>> 
>> That is suitable for linking to a static library. If I change it to look
>> like this:
>> 
>>     /* Instantiated by configure. */
>>     #if ! defined (__GMP_WITHIN_CONFIGURE)
>>     #define _LONG_LONG_LIMB 1
>>     #define __GMP_LIBGMP_DLL  1
>>     #endif
>> 
>> After rebuilding emacs, logcount works, and data-tests passes.
>
> I'd actually prefer us to link against GMP statically, at least on
> Windows, because GMP usually gets replaced when you install a new
> version of GCC.  So linking statically runs a lower risk of a "DLL
> hell".  It also makes the Emacs binary self-contained and more easily
> movable.

Yes. I don't know how to fix the configury and makefiles to ensure it
links against a static library if it is available.

> Why is it a problem with linking statically against GMP?  Could it be
> tat your libgmp.a is from a different GMP version, i.e. incompatible
> with the GMP headers you have installed?  Or maybe some other optional
> library depends on GMP and causes conflicts?  If not, I don't
> understand why the results should depend on how we linked the library.

Building the GMP library builds *different* gmp.h headers when building
for static library vs. a shared library.

The only difference in the headers preduced by the static libary and
shared libary builds is the hunk shown above. The shared library version
(the second hunk above) ensures that APIs get a __dllimport__
decoration for APIs on Windows, for linking to the shared library.

The MSYS2 GMP package includes a single gmp.h header for the static
library build, installed as "c:/msys64/mingw64/include/gmp.h".

Emacs currently links against the shared library on MSYS2 64bit, and has
a dependency on "c:/msys64/mingw64/bin/libgmp-10.dll".

Using the gmp.h header without __dllimport__ API decorations probably
results in incorrect runtime linking to the DLL.

    AndyM

Re: bignum branch

[Paul Eggert]

Andy Moreton wrote:
> I don't know how to fix the configury and makefiles to ensure it
> links against a static library if it is available.

Please keep in mind that we shouldn't prefer static linking on GNU/Linux 
platforms, even if a static library is available. These systems generally have a 
libgmp managed by standard tools like dnf or apt, and so the advantages static 
linking has on MS-Windows don't apply.

Re: bignum branch

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 10 Aug 2018 00:59:25 -0700
> 
> Andy Moreton wrote:
> > I don't know how to fix the configury and makefiles to ensure it
> > links against a static library if it is available.
> 
> Please keep in mind that we shouldn't prefer static linking on GNU/Linux 
> platforms, even if a static library is available. These systems generally have a 
> libgmp managed by standard tools like dnf or apt, and so the advantages static 
> linking has on MS-Windows don't apply.

I don't see why managing the library matters here, since linking
statically makes sure Emacs always uses the same library against which
it was linked.

Other projects link statically against libraries without any issues.
See GDB, for example.  Why should Emacs prefer dynamic linking?

Re: bignum branch

[Paul Eggert]

Eli Zaretskii wrote:

> I don't see why managing the library matters here, since linking
> statically makes sure Emacs always uses the same library against which
> it was linked.

That's exactly what we don't want, at least not on a well-managed system like 
Ubuntu, Red Hat, etc. where we will want the standard libgmp that everybody 
uses. These GNUish systems are very good about maintaining compatibility when 
libraries like libgmp change.

> Other projects link statically against libraries without any issues.
> See GDB, for example.

No, on the systems that I'm talking about, GDB links dynamically against libgmp. 
As does GCC and every other GNU package I know. There are good reasons for this, 
and Emacs shouldn't try to fight them. If MS-Windows has a different tradition 
then fine, Emacs can use static linking on MS-Windows. But Emacs should not use 
static linking on GNU/Linux distributions, or on other POSIXish systems.

Re: bignum branch

[Eli Zaretskii]

> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 10 Aug 2018 13:58:20 -0700
> 
> Eli Zaretskii wrote:
> 
> > I don't see why managing the library matters here, since linking
> > statically makes sure Emacs always uses the same library against which
> > it was linked.
> 
> That's exactly what we don't want, at least not on a well-managed system like 
> Ubuntu, Red Hat, etc. where we will want the standard libgmp that everybody 
> uses.

You say that, but don't provide any arguments for it, except library
management (which I'm not sure is very relevant here).  What are the
arguments _for_ using "the standard libgmp that everybody uses", not
arguments _against_not_ using it?

My main reason for linking GMP statically is that the way we integrate
it into Emacs is different from all the other libraries we use in
Emacs: the other libraries support optional features, whereas GMP
supports a feature that is part of the Emacs Lisp language.  So the
GMP code will always be present in Emacs, and is exactly like the code
in floatfns.c.  Thus, it is IMO important to have it exactly like at
build time, to make sure we don't suffer regressions in ELisp due to
some issue with the system-wide GMP library, because if that happens,
Emacs users will be screwed.

> > Other projects link statically against libraries without any issues.
> > See GDB, for example.
> 
> No, on the systems that I'm talking about, GDB links dynamically against libgmp. 

I wasn't talking about GMP, I was talking about other libraries GDB
links in.  libexpat, liblzma, even libintl -- all of those are linked
statically here.  Maybe they, too, are linked dynamically on
GNU/Linux, but the point is, it isn't something outlandish.  And the
main point is being an integral part of the language, see above.

Re: bignum branch

[Paul Eggert]

Eli Zaretskii wrote:

> What are the
> arguments _for_ using "the standard libgmp that everybody uses"

It's what Emacs does for the other libraries that it uses, some of which are 
also essential for Lisp operation, and there is no good reason for treating 
libgmp differently. libgmp is a common dynamic library used by many other 
libraries and programs, Emacs is already dynamically linked to libgmp on 
GNU/Linux distributions with few if any reports of dynamic linking hell, so 
there is no good reason to change on these platforms.

> My main reason for linking GMP statically is that the way we integrate
> it into Emacs is different from all the other libraries we use in
> Emacs: the other libraries support optional features, whereas GMP
> supports a feature that is part of the Emacs Lisp language.

First, that's not true for Emacs. libc and libm are dynamically linked and are 
essential for Emacs Lisp.

Second, libgmp is essential for other GNU programs like GCC, and they work just 
fine dynamically linked. Emacs is not special here.

> I wasn't talking about GMP, I was talking about other libraries GDB
> links in.  libexpat, liblzma, even libintl -- all of those are linked
> statically here.  Maybe they, too, are linked dynamically on
> GNU/Linux

Yes, these libraries are linked dynamically on GNU/Linux.

> it isn't something outlandish.

Sorry, but static linking *is* outlandish on GNU/Linux. Again, maybe things are 
different on MS-Windows, but let's not let the tail wag the dog here. I'm not 
saying static linking never happens on GNU/Linux -- it does -- but it's 
definitely oddball nowadays.

Much of this discussion appears to have been based on a misunderstanding of how 
things commonly work on GNU/Linux. Static linking is no longer routinely used by 
applications on these platforms. Some GNU/Linux distributions don't even fully 
support static linking any more, for security reasons. It would be a mistake for 
Emacs to insist on it.

Re: bignum branch

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 11 Aug 2018 01:02:01 -0700
> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
> 
> Eli Zaretskii wrote:
> 
> > What are the
> > arguments _for_ using "the standard libgmp that everybody uses"
> 
> It's what Emacs does for the other libraries that it uses, some of which are 
> also essential for Lisp operation

Which of the other libraries are essential for Lisp?  I don't see how
an optional library can be essential, that's a contradiction in my
book.

> libc and libm are dynamically linked and are essential for Emacs
> Lisp.

That's a strawman I didn't expect to see from you.

> Some GNU/Linux distributions don't even fully support static linking
> any more, for security reasons.

Really?  But we do link to Gnulib statically, so it sounds like the
platforms we care about do still support static linking, and probably
will for the observable future, right?

Re: bignum branch

[Stefan Monnier]

>> libc and libm are dynamically linked and are essential for Emacs Lisp.
> That's a strawman I didn't expect to see from you.

I personally don't see why libc/libm is not a good example for libgmp.

>> Some GNU/Linux distributions don't even fully support static linking
>> any more, for security reasons.
> Really?  But we do link to Gnulib statically, so it sounds like the
> platforms we care about do still support static linking, and probably
> will for the observable future, right?

I don't think any GNU/Linux distribution prevents anyone from using
static linking directly.  I think what Paul meant is that some GNU/Linux
distributions don't provide library packages with the .a files any more:
you only get the .h and the .so.  But if you compile the library on your
own (as we do for gnulib and lwlib), then the tools fully support static
linking of course.

So under GNU/Linux we can statically link to a libgmp if we distribute
that libgmp's source code with Emacs, but otherwise we may be forced to
use dynamic linking.


        Stefan

Re: bignum branch

[Paul Eggert]

Eli Zaretskii wrote:

>> libc and libm are dynamically linked and are essential for Emacs
>> Lisp.
> 
> That's a strawman I didn't expect to see from you.

It's not a strawman at all. It's close to what libgmp is for the bignum branch. 
The bignum branch requires libgmp functionality, and arranges for a substitute 
in Emacs itself if libgmp isn't supplied by the system. Similarly, Emacs 
requires C and math functionality, and arranges for substitutes in Emacs itself 
if some of the required functions do not exist in the current platform's libraries.

>> Some GNU/Linux distributions don't even fully support static linking
>> any more, for security reasons.
> 
> Really?  But we do link to Gnulib statically, so it sounds like the
> platforms we care about do still support static linking, and probably
> will for the observable future, right?

These platforms allow you to build and use your own .a files, since that's 
basically the same as building and using your own .o files. But they don't 
supply .a files for standard libraries, so they won't let you statically link to 
their libgmp. That is what I meant by "not fully support". Static linking is 
generally discouraged on these platforms, and we shouldn't try to fight that.

Re: bignum branch

[Andy Moreton]

On Fri 10 Aug 2018, Paul Eggert wrote:

> Andy Moreton wrote:
>> I don't know how to fix the configury and makefiles to ensure it
>> links against a static library if it is available.
>
> Please keep in mind that we shouldn't prefer static linking on GNU/Linux
> platforms, even if a static library is available. These systems generally have
> a libgmp managed by standard tools like dnf or apt, and so the advantages
> static linking has on MS-Windows don't apply.

Both static and dynamic linking work on Windows if the right headers are
installed to declare functions properly. The fact that the GMP project
cannot organise its public headers properly is an irritation.

Performance may be faster on any platform using static linking
(depending on the details of the ABI and runtime linker).

GCC links GMP statically. Platform package manager updates to shared
libraries are more likely to cause breakage than static linking.

    AndyM

Re: bignum branch

[Andreas Schwab]

On Aug 10 2018, Andy Moreton <andrewjmoreton@gmail.com> wrote:

> GCC links GMP statically.

It does?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: bignum branch

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Date: Fri, 10 Aug 2018 13:56:07 +0200
> Cc: emacs-devel@gnu.org
> 
> On Aug 10 2018, Andy Moreton <andrewjmoreton@gmail.com> wrote:
> 
> > GCC links GMP statically.
> 
> It does?

Indeed, it doesn't, at least not on my box: look at all those
executables under libexec/.  In my case, at least cc1.exe,
cc1plus.exe, f951.exe, cc1obj.exe, cc1objplus.exe, and gnat1.exe are
linked against GMP dynamically.

Re: bignum branch

[Andy Moreton]

On Fri 10 Aug 2018, Andreas Schwab wrote:

> On Aug 10 2018, Andy Moreton <andrewjmoreton@gmail.com> wrote:
>
>> GCC links GMP statically.
>
> It does?

Cygwin 64bit:

/home/ajm> uname -a
CYGWIN_NT-10.0 ajm-desktop2 2.10.0(0.325/5/3) 2018-02-02 15:16 x86_64 Cygwin
/home/ajm> cygcheck /usr/bin/gcc
C:\cygwin\bin\gcc.exe
  C:\cygwin\bin\cygwin1.dll
    C:\WINDOWS\system32\KERNEL32.dll
  C:\cygwin\bin\cygiconv-2.dll
  C:\cygwin\bin\cygintl-8.dll

MSYS2 64bit:

ajm@ajm-desktop2 /c/home/ajm> uname -a
MINGW64_NT-10.0 ajm-desktop2 2.10.0(0.325/5/3) 2018-02-09 15:25 x86_64 Msys
ajm@ajm-desktop2 /c/home/ajm> which gcc
/mingw64/bin/gcc
ajm@ajm-desktop2 /c/home/ajm> cygcheck /mingw64/bin/gcc
C:\msys64\mingw64\bin\gcc.exe
  C:\WINDOWS\system32\KERNEL32.dll
    C:\WINDOWS\system32\ntdll.dll
    C:\WINDOWS\system32\KERNELBASE.dll
  C:\WINDOWS\system32\msvcrt.dll
  C:\msys64\mingw64\bin\libwinpthread-1.dll

Maybe not everywhere, but cetainly on some platforms.

    AndyM

Re: bignum branch

[Achim Gratz]

Andy Moreton writes:
> On Fri 10 Aug 2018, Andreas Schwab wrote:
>
>> On Aug 10 2018, Andy Moreton <andrewjmoreton@gmail.com> wrote:
>>
>>> GCC links GMP statically.
>>
>> It does?
>
> Cygwin 64bit:
>
> /home/ajm> uname -a
> CYGWIN_NT-10.0 ajm-desktop2 2.10.0(0.325/5/3) 2018-02-02 15:16 x86_64 Cygwin
> /home/ajm> cygcheck /usr/bin/gcc
> C:\cygwin\bin\gcc.exe
>   C:\cygwin\bin\cygwin1.dll
>     C:\WINDOWS\system32\KERNEL32.dll
>   C:\cygwin\bin\cygiconv-2.dll
>   C:\cygwin\bin\cygintl-8.dll

Looking for that library being referenced in the driver program rather
than the actual compiler executable is futile on any platform.

~ gcc -print-libgcc-file-name
/usr/lib/gcc/x86_64-pc-cygwin/7.3.0/libgcc.a
~ cygcheck /usr/lib/gcc/x86_64-pc-cygwin/7.3.0/cc1
D:\Freeware\Cygwin64\lib\gcc\x86_64-pc-cygwin\7.3.0\cc1.exe
  D:\Freeware\Cygwin64\bin\cygwin1.dll
    C:\Windows\system32\KERNEL32.dll
      C:\Windows\system32\api-ms-win-core-rtlsupport-l1-2-0.dll
      C:\Windows\system32\ntdll.dll
      C:\Windows\system32\KERNELBASE.dll
        C:\Windows\system32\api-ms-win-core-apiquery-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-processthreads-l1-1-2.dll
      C:\Windows\system32\api-ms-win-core-registry-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-heap-l1-2-0.dll
      C:\Windows\system32\api-ms-win-core-memory-l1-1-2.dll
      C:\Windows\system32\api-ms-win-core-handle-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-synch-l1-2-0.dll
      C:\Windows\system32\api-ms-win-core-file-l1-2-1.dll
      C:\Windows\system32\api-ms-win-core-delayload-l1-1-1.dll
      C:\Windows\system32\api-ms-win-core-io-l1-1-1.dll
      C:\Windows\system32\api-ms-win-core-job-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-threadpool-legacy-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-threadpool-private-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-libraryloader-l1-2-0.dll
      C:\Windows\system32\api-ms-win-core-namedpipe-l1-2-0.dll
      C:\Windows\system32\api-ms-win-core-datetime-l1-1-1.dll
      C:\Windows\system32\api-ms-win-core-sysinfo-l1-2-1.dll
      C:\Windows\system32\api-ms-win-core-timezone-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-localization-l1-2-1.dll
      C:\Windows\system32\api-ms-win-core-processenvironment-l1-2-0.dll
      C:\Windows\system32\api-ms-win-core-string-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-debug-l1-1-1.dll
      C:\Windows\system32\api-ms-win-core-errorhandling-l1-1-1.dll
      C:\Windows\system32\api-ms-win-core-fibers-l1-1-1.dll
      C:\Windows\system32\api-ms-win-core-util-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-profile-l1-1-0.dll
      C:\Windows\system32\api-ms-win-security-base-l1-2-0.dll
      C:\Windows\system32\api-ms-win-security-appcontainer-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-comm-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-realtime-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-wow64-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-systemtopology-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-processtopology-l1-2-0.dll
      C:\Windows\system32\api-ms-win-core-namespace-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-file-l2-1-1.dll
      C:\Windows\system32\api-ms-win-core-xstate-l2-1-0.dll
      C:\Windows\system32\api-ms-win-core-localization-l2-1-0.dll
      C:\Windows\system32\api-ms-win-core-normalization-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-localization-private-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-sidebyside-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-appcompat-l1-1-1.dll
      C:\Windows\system32\api-ms-win-core-windowserrorreporting-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-console-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-console-l2-1-0.dll
      C:\Windows\system32\api-ms-win-core-psapi-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-psapi-ansi-l1-1-0.dll
      C:\Windows\system32\api-ms-win-core-psapi-obsolete-l1-1-0.dll
  D:\Freeware\Cygwin64\bin\cyggmp-10.dll
  D:\Freeware\Cygwin64\bin\cygiconv-2.dll
  D:\Freeware\Cygwin64\bin\cygintl-8.dll
  D:\Freeware\Cygwin64\bin\cygisl-15.dll
  D:\Freeware\Cygwin64\bin\cygmpc-3.dll
    D:\Freeware\Cygwin64\bin\cygmpfr-6.dll
      D:\Freeware\Cygwin64\bin\cyggcc_s-seh-1.dll
  D:\Freeware\Cygwin64\bin\cygz.dll

There are officially no statically linked libraries on Cygwin, although
you can force it in certain circumstances.  It's unsupported and
fragile.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 10 Aug 2018 12:18:02 +0100
> 
> Both static and dynamic linking work on Windows if the right headers are
> installed to declare functions properly. The fact that the GMP project
> cannot organise its public headers properly is an irritation.

I think the actual problem is that you cannot request static or
dynamic linking easily, not even by using a -Dsomething cpp option,
not without some jumping through hoops.

Re: bignum branch

[Andy Moreton]

On Fri 10 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 10 Aug 2018 12:18:02 +0100
>> 
>> Both static and dynamic linking work on Windows if the right headers are
>> installed to declare functions properly. The fact that the GMP project
>> cannot organise its public headers properly is an irritation.
>
> I think the actual problem is that you cannot request static or
> dynamic linking easily, not even by using a -Dsomething cpp option,
> not without some jumping through hoops.

True, but a -Dsomething cpp option at least allows you to have a single
header file (that can add the dllimport decoration if required) based on
a choice made in the build script, even if there are complications in
ensuring that the choice made for the headers matches the linker
options.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 10 Aug 2018 08:43:38 +0100
> 
> Building the GMP library builds *different* gmp.h headers when building
> for static library vs. a shared library.
> 
> The only difference in the headers preduced by the static libary and
> shared libary builds is the hunk shown above. The shared library version
> (the second hunk above) ensures that APIs get a __dllimport__
> decoration for APIs on Windows, for linking to the shared library.
> 
> The MSYS2 GMP package includes a single gmp.h header for the static
> library build, installed as "c:/msys64/mingw64/include/gmp.h".

Yes, I know.  I would like to try to use the gmp.h header without
changes, if possible, since that makes it easier for others to build
Emacs.

> Emacs currently links against the shared library on MSYS2 64bit, and has
> a dependency on "c:/msys64/mingw64/bin/libgmp-10.dll".

You mean, on the bignum branch or on master/emacs-26?  If the latter,
is the dependency on libgmp recorded in the binary, or do you see it
in a running session?  In either case, is the dependency direct in
Emacs or indirect via some other library, and if so, which library
causes the dependency?

> Using the gmp.h header without __dllimport__ API decorations probably
> results in incorrect runtime linking to the DLL.

That's what I don't understand, yet.

Re: bignum branch

[Andy Moreton]

On Fri 10 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 10 Aug 2018 08:43:38 +0100
>> 
>> Building the GMP library builds *different* gmp.h headers when building
>> for static library vs. a shared library.
>> 
>> The only difference in the headers preduced by the static libary and
>> shared libary builds is the hunk shown above. The shared library version
>> (the second hunk above) ensures that APIs get a __dllimport__
>> decoration for APIs on Windows, for linking to the shared library.
>> 
>> The MSYS2 GMP package includes a single gmp.h header for the static
>> library build, installed as "c:/msys64/mingw64/include/gmp.h".
>
> Yes, I know.  I would like to try to use the gmp.h header without
> changes, if possible, since that makes it easier for others to build
> Emacs.

Agreed - modifying the header is about problem diagnosis, not a long
term solution.

>> Emacs currently links against the shared library on MSYS2 64bit, and has
>> a dependency on "c:/msys64/mingw64/bin/libgmp-10.dll".
>
> You mean, on the bignum branch or on master/emacs-26?  If the latter,

This entire thread is about the bignum branch. GMP has never been linked
in on master. Why would you think otherwise ? Of course this is about
the bignum branch.

> is the dependency on libgmp recorded in the binary, or do you see it
> in a running session?  In either case, is the dependency direct in
> Emacs or indirect via some other library, and if so, which library
> causes the dependency?

I used the MSYS2 version of cygcheck to report the dependencies, and
also the Dependency Walker tool. Both show libgmp-10.dll as a direct
dependency of emacs.exe.

>> Using the gmp.h header without __dllimport__ API decorations probably
>> results in incorrect runtime linking to the DLL.
>
> That's what I don't understand, yet.

The only thing I changed was the dllimport decorations, and that changed
the behaviour from a crash to working properly. It is possible that some
other change in output after recompilation fixed the bad behaviour, but
improper runtime linking is a reasonable cause for this problem. Further
diagnosis from someone more expert than me is required to confirm if
that is the cause.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 10 Aug 2018 12:39:33 +0100
> 
> >> Emacs currently links against the shared library on MSYS2 64bit, and has
> >> a dependency on "c:/msys64/mingw64/bin/libgmp-10.dll".
> >
> > You mean, on the bignum branch or on master/emacs-26?  If the latter,
> 
> This entire thread is about the bignum branch. GMP has never been linked
> in on master. Why would you think otherwise ?

Because "currently" is ambiguous, and because you evidently changed
the static/dynamic linking as part of trying to investigate this
problem, so it wasn't clear whether "currently" refers to what was
before or after the change.  Sorry for my slow mind.

> I used the MSYS2 version of cygcheck to report the dependencies, and
> also the Dependency Walker tool. Both show libgmp-10.dll as a direct
> dependency of emacs.exe.

And that happens no matter what version of the gmp.h header, the
original one or the one you modified. you use during compilation?

> The only thing I changed was the dllimport decorations, and that changed
> the behaviour from a crash to working properly.

What happens if you restore the original gmp.h, rename libgmp.dll.a to
something the linker won't recognize, and rebuild Emacs?  (This should
cause Emacs to be linked statically against libgmp.a.)  Does the
problem with the test suite happen then?

Thanks.

Re: bignum branch

[Andy Moreton]

On Fri 10 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 10 Aug 2018 12:39:33 +0100
>> 
>> >> Emacs currently links against the shared library on MSYS2 64bit, and has
>> >> a dependency on "c:/msys64/mingw64/bin/libgmp-10.dll".
>> >
>> I used the MSYS2 version of cygcheck to report the dependencies, and
>> also the Dependency Walker tool. Both show libgmp-10.dll as a direct
>> dependency of emacs.exe.
>
> And that happens no matter what version of the gmp.h header, the
> original one or the one you modified. you use during compilation?

Yes, which is expected. The API decoration with dllimport only ensures
that the symbols are linked correctly - the choice of libary is set by
the linker options.

>> The only thing I changed was the dllimport decorations, and that changed
>> the behaviour from a crash to working properly.
>
> What happens if you restore the original gmp.h, rename libgmp.dll.a to
> something the linker won't recognize, and rebuild Emacs?  (This should
> cause Emacs to be linked statically against libgmp.a.)  Does the
> problem with the test suite happen then?

Good idea. I reinstalledthe GMP package, and checked that gmp.h is
unmodified, and then tested each time as follows:
 - touch globals.h
 - rebuild emacs
 - "ldd emacs.exe | grep -i gmp" to check dependencies
 - "make -C test data-tests" to run logcount test

|   | __GMP_LIBGMP_DLL | libgmp.dll.a | dependencies  | data-tests |
|---+------------------+--------------+---------------+------------|
| 1 |                0 | libgmp.dll.a | libgmp-10.dll | crash      |
|---+------------------+--------------+---------------+------------|
| 2 |                0 | (removed)    | (none)        | pass       |
|---+------------------+--------------+---------------+------------|
| 3 |                1 | libgmp.dll.a | libgmp-10.dll | pass       |
|---+------------------+--------------+---------------+------------|
| 4 |                1 | (removed)    | (link fails)  | n/a        |
|---+------------------+--------------+---------------+------------|

Row (1) is the original bignum build with the problem.
Row (2) results in using the static lib correctly.
Row (3) with modified gmp.h shows the shared lib working correctly.
Row (4) shows that asking for dllimport APIs without the import library
fails to link:

  CCLD     temacs.exe
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/8.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: frame.o: in function `XFLOATINT':
C:/emacs/git/emacs/bignum/src/lisp.h:2923: undefined reference to
`__imp___gmpz_get_d'
[many similar lines omitted]

I think this clearly shows that the problem is nmismatched calling
convention and library usage.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 10 Aug 2018 15:05:19 +0100
> 
> |   | __GMP_LIBGMP_DLL | libgmp.dll.a | dependencies  | data-tests |
> |---+------------------+--------------+---------------+------------|
> | 1 |                0 | libgmp.dll.a | libgmp-10.dll | crash      |
> |---+------------------+--------------+---------------+------------|
> | 2 |                0 | (removed)    | (none)        | pass       |
> |---+------------------+--------------+---------------+------------|
> | 3 |                1 | libgmp.dll.a | libgmp-10.dll | pass       |
> |---+------------------+--------------+---------------+------------|
> | 4 |                1 | (removed)    | (link fails)  | n/a        |
> |---+------------------+--------------+---------------+------------|
> 
> Row (1) is the original bignum build with the problem.

Can you show a C backtrace from the crash?

I'm asking because there's something weird about this crash.  What row
(1) does is use gmp.h where exported functions are not declared
__declspec(dllexport).  But that shouldn't prevent a valid dynamic
link against the DLL; in fact, you should see that most other DLLs we
use in Emacs don't have __declspec(dllexport) in their header files,
because that's the old MS convention that at least GNU tools tossed a
long time ago (although they still support it for backward
compatibility).

And since your problem AFAIU is with a single GMP function, I think
there's something special in that single function or maybe in the way
it is declared in gmp.h.  Or something similarly unique.

> Row (4) shows that asking for dllimport APIs without the import library
> fails to link:
> 
>   CCLD     temacs.exe
> C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/8.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: frame.o: in function `XFLOATINT':
> C:/emacs/git/emacs/bignum/src/lisp.h:2923: undefined reference to
> `__imp___gmpz_get_d'
> [many similar lines omitted]

That's expected, if gmp.h uses __declspec(dllexport) for function
prototypes.

> I think this clearly shows that the problem is nmismatched calling
> convention and library usage.

See above: there's still some mystery here.

Re: bignum branch

[Andy Moreton]

On Fri 10 Aug 2018, Eli Zaretskii wrote:

> Can you show a C backtrace from the crash?

(gdb) bt full
#0  0x00007ff83c63c903 in KERNELBASE!DebugBreak () from C:\WINDOWS\System32\KernelBase.dll
No symbol table info available.
#1  0x000000040021d84d in emacs_abort () at C:/emacs/git/emacs/bignum/src/w32fns.c:10775
        button = <optimized out>
#2  0x00000004000eff91 in terminate_due_to_signal (sig=0xb, backtrace_limit=<optimized out>) at C:/emacs/git/emacs/bignum/src/emacs.c:399
No locals.
#3  0x0000000400110620 in handle_fatal_signal (sig=0xd60000, sig@entry=0xb) at C:/emacs/git/emacs/bignum/src/sysdep.c:1769
No locals.
#4  0x00000004001102c8 in deliver_thread_signal (sig=0xb, handler=0x400110612 <handle_fatal_signal>) at C:/emacs/git/emacs/bignum/src/sysdep.c:1743
        old_errno = 0x16
#5  0x00000004001102e5 in deliver_fatal_thread_signal (sig=0xd60000) at C:/emacs/git/emacs/bignum/src/sysdep.c:1781
No locals.
#6  0x000000040028610c in _gnu_exception_handler (exception_data=0xbfb890) at C:/repo/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/crt_handler.c:223
        old_handler = <optimized out>
        action = 0x0
        reset_fpu = 0x0
#7  0x00007ff83d0d7c58 in msvcrt!__C_specific_handler () from C:\WINDOWS\System32\msvcrt.dll
No symbol table info available.
#8  0x00007ff83f86eced in ntdll!.chkstk () from C:\WINDOWS\SYSTEM32\ntdll.dll
No symbol table info available.
#9  0x00007ff83f7d6c86 in ntdll!RtlWalkFrameChain () from C:\WINDOWS\SYSTEM32\ntdll.dll
No symbol table info available.
#10 0x00007ff83f86dc1e in ntdll!KiUserExceptionDispatcher () from C:\WINDOWS\SYSTEM32\ntdll.dll
No symbol table info available.
#11 0x000000046ace5dc0 in ?? ()
No symbol table info available.
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

Lisp Backtrace:
"logcount" (0xbfc7e0)
"list" (0xbfc928)
"let" (0xbfcad8)
"condition-case" (0xbfcc78)
"let*" (0xbfcdd8)
0x457bcf0 Lisp type 3
"ert--run-test-internal" (0xbfd2c0)
"ert-run-test" (0xbfd598)
"ert-run-or-rerun-test" (0xbfd880)
"ert-run-tests" (0xbfdb30)
"ert-run-tests-batch" (0xbfdd78)
"ert-run-tests-batch-and-exit" (0xbfdf30)
"eval" (0xbfe288)
"command-line-1" (0xbfe930)
"command-line" (0xbff1c8)
"normal-top-level" (0xbff550)

> I'm asking because there's something weird about this crash.

The crashing build used "-Og". If I rebuild with "-O0" the build works
properly.

Let me know if there is anything I should look for in gdb to try to
debug this.

    AndyM

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> On Fri 10 Aug 2018, Eli Zaretskii wrote:
>> Can you show a C backtrace from the crash?

Andy> (gdb) bt full

[...]

Should I wait for this to be resolved before merging the bignum branch
to master?

Tom

Re: bignum branch

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Date: Sat, 11 Aug 2018 09:25:52 -0600
> Cc: emacs-devel@gnu.org
> 
> >>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
> 
> Andy> (gdb) bt full
> 
> [...]
> 
> Should I wait for this to be resolved before merging the bignum branch
> to master?

No, please go ahead.

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 11 Aug 2018 16:21:42 +0100
> 
> (gdb) bt full
> #0  0x00007ff83c63c903 in KERNELBASE!DebugBreak () from C:\WINDOWS\System32\KernelBase.dll
> No symbol table info available.
> #1  0x000000040021d84d in emacs_abort () at C:/emacs/git/emacs/bignum/src/w32fns.c:10775
>         button = <optimized out>
> #2  0x00000004000eff91 in terminate_due_to_signal (sig=0xb, backtrace_limit=<optimized out>) at C:/emacs/git/emacs/bignum/src/emacs.c:399
> No locals.
> #3  0x0000000400110620 in handle_fatal_signal (sig=0xd60000, sig@entry=0xb) at C:/emacs/git/emacs/bignum/src/sysdep.c:1769
> No locals.
> #4  0x00000004001102c8 in deliver_thread_signal (sig=0xb, handler=0x400110612 <handle_fatal_signal>) at C:/emacs/git/emacs/bignum/src/sysdep.c:1743
>         old_errno = 0x16
> #5  0x00000004001102e5 in deliver_fatal_thread_signal (sig=0xd60000) at C:/emacs/git/emacs/bignum/src/sysdep.c:1781
> No locals.
> #6  0x000000040028610c in _gnu_exception_handler (exception_data=0xbfb890) at C:/repo/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/crt_handler.c:223
>         old_handler = <optimized out>
>         action = 0x0
>         reset_fpu = 0x0
> #7  0x00007ff83d0d7c58 in msvcrt!__C_specific_handler () from C:\WINDOWS\System32\msvcrt.dll
> No symbol table info available.
> #8  0x00007ff83f86eced in ntdll!.chkstk () from C:\WINDOWS\SYSTEM32\ntdll.dll
> No symbol table info available.
> #9  0x00007ff83f7d6c86 in ntdll!RtlWalkFrameChain () from C:\WINDOWS\SYSTEM32\ntdll.dll
> No symbol table info available.
> #10 0x00007ff83f86dc1e in ntdll!KiUserExceptionDispatcher () from C:\WINDOWS\SYSTEM32\ntdll.dll
> No symbol table info available.
> #11 0x000000046ace5dc0 in ?? ()
> No symbol table info available.
> Backtrace stopped: previous frame inner to this frame (corrupt stack?)
> 
> Lisp Backtrace:
> "logcount" (0xbfc7e0)
> "list" (0xbfc928)

Strange that it shows nothing before the exception handler.  Did you
run the test under GDB to begin with, or only attached it after the
abort dialog popped up?  If the latter, please run the test from GDB
to begin with (it is easier to do that if you run the test
interactively).

> Let me know if there is anything I should look for in gdb to try to
> debug this.

In case you already run the test from GDB: Is it true that logcount
always crashes in this build?  If so, can you step through logcount
and tell where exactly it crashes?  Once you determine the C line in
which it crashes, please re-run the test with a breakpoint inside
logcount, step through the function until you get to the crashing
line, and then use "stepi" to step into the function that crashes.
Please show the transcript of this stepping.

Thanks.

Re: bignum branch

[Andy Moreton]

On Sat 11 Aug 2018, Eli Zaretskii wrote:
> Strange that it shows nothing before the exception handler.  Did you
> run the test under GDB to begin with, or only attached it after the
> abort dialog popped up?  If the latter, please run the test from GDB
> to begin with (it is easier to do that if you run the test
> interactively).

That dump was from attachig after the crtash: running it under gdb to
start with produces the same unhelpful backtrace.

>> Let me know if there is anything I should look for in gdb to try to
>> debug this.
>
> In case you already run the test from GDB: Is it true that logcount
> always crashes in this build?  If so, can you step through logcount
> and tell where exactly it crashes?  Once you determine the C line in
> which it crashes, please re-run the test with a breakpoint inside
> logcount, step through the function until you get to the crashing
> line, and then use "stepi" to step into the function that crashes.
> Please show the transcript of this stepping.

This always crashes when handling a bignum, in mpz_popcount at the call
to mpn_popcount.

In the gdb session below, I evaluated this in the scratch buffer:
    (logcount (1+ most-positive-fixnum))

(gdb) b Flogcount
Breakpoint 4 at 0x40016e2de: file C:/emacs/git/emacs/bignum/src/data.c, line 3340.
(gdb) run -Q
Starting program: C:\emacs\git\emacs\bignum\build\mingw64-x86_64\src\emacs.exe -Q
[New Thread 7140.0x73c]
[New Thread 7140.0x1124]
[New Thread 7140.0x770]
[New Thread 7140.0x2160]
[New Thread 7140.0x9e0]
[New Thread 7140.0x1a48]
[Thread 7140.0x770 exited with code 0]
[Thread 7140.0x2160 exited with code 0]
[Thread 7140.0x1124 exited with code 0]

Thread 1 hit Breakpoint 4, Flogcount (value=XIL(0x4c5ec51)) at C:/emacs/git/emacs/bignum/src/data.c:3340
3340    {
(gdb) n
3341      CHECK_INTEGER (value);
(gdb)
3343      if (BIGNUMP (value))
(gdb)
3345          if (mpz_sgn (XBIGNUM (value)->value) >= 0)
(gdb)
3346            return make_fixnum (mpz_popcount (XBIGNUM (value)->value));
(gdb) s
__gmpz_popcount (__gmp_u=0x4c5ec60) at C:/msys64/mingw64/include/gmp.h:1844
1844      if (__GMP_LIKELY (__gmp_usize > 0))
(gdb) s
1845        __gmp_result =  mpn_popcount (__gmp_u->_mp_d, __gmp_usize);
(gdb) stepi
0x000000040016e405      1845        __gmp_result =  mpn_popcount (__gmp_u->_mp_d, __gmp_usize);
(gdb)
0x000000046ace5dc0 in ?? ()
(gdb)

Thread 1 received signal SIGSEGV, Segmentation fault.
0x000000046ace5dc0 in ?? ()
(gdb)

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 11 Aug 2018 17:54:42 +0100
> 
> 3346            return make_fixnum (mpz_popcount (XBIGNUM (value)->value));
> (gdb) s
> __gmpz_popcount (__gmp_u=0x4c5ec60) at C:/msys64/mingw64/include/gmp.h:1844
> 1844      if (__GMP_LIKELY (__gmp_usize > 0))
> (gdb) s
> 1845        __gmp_result =  mpn_popcount (__gmp_u->_mp_d, __gmp_usize);
> (gdb) stepi
> 0x000000040016e405      1845        __gmp_result =  mpn_popcount (__gmp_u->_mp_d, __gmp_usize);
> (gdb)
> 0x000000046ace5dc0 in ?? ()
> (gdb)
> 
> Thread 1 received signal SIGSEGV, Segmentation fault.
> 0x000000046ace5dc0 in ?? ()

What do the following GDB commands show, in the crashing build?

  (gdb) disassemble __gmpz_popcount
  (gdb) ptype __gmpz_popcount
  (gdb) ptype __gmpn_popcount

Re: bignum branch

[Andy Moreton]

On Sat 11 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sat, 11 Aug 2018 17:54:42 +0100
>> 
>> 3346            return make_fixnum (mpz_popcount (XBIGNUM (value)->value));
>> (gdb) s
>> __gmpz_popcount (__gmp_u=0x4c5ec60) at C:/msys64/mingw64/include/gmp.h:1844
>> 1844      if (__GMP_LIKELY (__gmp_usize > 0))
>> (gdb) s
>> 1845        __gmp_result =  mpn_popcount (__gmp_u->_mp_d, __gmp_usize);
>> (gdb) stepi
>> 0x000000040016e405      1845        __gmp_result =  mpn_popcount (__gmp_u->_mp_d, __gmp_usize);
>> (gdb)
>> 0x000000046ace5dc0 in ?? ()
>> (gdb)
>> 
>> Thread 1 received signal SIGSEGV, Segmentation fault.
>> 0x000000046ace5dc0 in ?? ()
>
> What do the following GDB commands show, in the crashing build?
>
>   (gdb) disassemble __gmpz_popcount
>   (gdb) ptype __gmpz_popcount
>   (gdb) ptype __gmpn_popcount

Nothing useful :-(

Thread 1 hit Breakpoint 4, Flogcount (value=XIL(0x4bf8631)) at C:/emacs/git/emacs/bignum/src/data.c:3340
3340    {
(gdb) disassemble __gmpz_popcount
No symbol "__gmpz_popcount" in current context.
(gdb) ptype __gmpz_popcount
No symbol "__gmpz_popcount" in current context.
(gdb) ptype __gmpn_popcount
type = <unknown return type> ()
(gdb)

Dependency Walker shows __gmpn_popcount at ordinal 306 in the export
table of libgmp-10.dll, and __gmpz_popcount at ordinal 561, so gdb
should be able to find them.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 11 Aug 2018 18:56:15 +0100
> 
> > What do the following GDB commands show, in the crashing build?
> >
> >   (gdb) disassemble __gmpz_popcount
> >   (gdb) ptype __gmpz_popcount
> >   (gdb) ptype __gmpn_popcount
> 
> Nothing useful :-(

What about

  (gdb) disassemble Flogcount

?

Re: bignum branch

[Andy Moreton]

On Sat 11 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sat, 11 Aug 2018 18:56:15 +0100
>> 
>> > What do the following GDB commands show, in the crashing build?
>> >
>> >   (gdb) disassemble __gmpz_popcount
>> >   (gdb) ptype __gmpz_popcount
>> >   (gdb) ptype __gmpn_popcount
>> 
>> Nothing useful :-(
>
> What about
>
>   (gdb) disassemble Flogcount

(gdb) disas Flogcount
Dump of assembler code for function Flogcount:
   0x000000040016e2de <+0>:	push   %rsi
   0x000000040016e2df <+1>:	push   %rbx
   0x000000040016e2e0 <+2>:	sub    $0x38,%rsp
   0x000000040016e2e4 <+6>:	mov    %rcx,%rbx
   0x000000040016e2e7 <+9>:	mov    0x53d252(%rip),%rax        # 0x4006ab540 <.refptr.suppress_checking>
   0x000000040016e2ee <+16>:	cmpb   $0x0,(%rax)
   0x000000040016e2f1 <+19>:	je     0x40016e317 <Flogcount+57>
   0x000000040016e2f3 <+21>:	mov    %ebx,%eax
   0x000000040016e2f5 <+23>:	and    $0x3,%eax
   0x000000040016e2f8 <+26>:	cmp    $0x2,%eax
   0x000000040016e2fb <+29>:	je     0x40016e36a <Flogcount+140>
   0x000000040016e2fd <+31>:	mov    %ebx,%eax
   0x000000040016e2ff <+33>:	and    $0x7,%eax
   0x000000040016e302 <+36>:	cmp    $0x1,%eax
   0x000000040016e305 <+39>:	je     0x40016e34d <Flogcount+111>
   0x000000040016e307 <+41>:	mov    $0x0,%eax
   0x000000040016e30c <+46>:	test   %eax,%eax
   0x000000040016e30e <+48>:	je     0x40016e36f <Flogcount+145>
   0x000000040016e310 <+50>:	mov    $0x1,%eax
   0x000000040016e315 <+55>:	jmp    0x40016e36f <Flogcount+145>
   0x000000040016e317 <+57>:	mov    $0xc010,%ecx
   0x000000040016e31c <+62>:	callq  0x4000e6466 <XSYMBOL>
   0x000000040016e321 <+67>:	mov    0x53cd78(%rip),%rsi        # 0x4006ab0a0 <.refptr.lispsym>
   0x000000040016e328 <+74>:	lea    0xc010(%rsi),%rdx
   0x000000040016e32f <+81>:	cmp    %rdx,%rax
   0x000000040016e332 <+84>:	je     0x40016e2f3 <Flogcount+21>
   0x000000040016e334 <+86>:	mov    $0x3ad,%r8d
   0x000000040016e33a <+92>:	lea    0x51905f(%rip),%rdx        # 0x4006873a0 <gdb_make_enums_visible+408>
   0x000000040016e341 <+99>:	lea    0x519144(%rip),%rcx        # 0x40068748c <gdb_make_enums_visible+644>
   0x000000040016e348 <+106>:	callq  0x400160fd4 <die>
   0x000000040016e34d <+111>:	mov    %rbx,%rcx
   0x000000040016e350 <+114>:	callq  0x4000e9651 <XMISCANY>
   0x000000040016e355 <+119>:	cmpw   $0x5eb1,(%rax)
   0x000000040016e35a <+124>:	je     0x40016e363 <Flogcount+133>
   0x000000040016e35c <+126>:	mov    $0x0,%eax
   0x000000040016e361 <+131>:	jmp    0x40016e30c <Flogcount+46>
   0x000000040016e363 <+133>:	mov    $0x1,%eax
   0x000000040016e368 <+138>:	jmp    0x40016e30c <Flogcount+46>
   0x000000040016e36a <+140>:	mov    $0x1,%eax
   0x000000040016e36f <+145>:	test   %eax,%eax
   0x000000040016e371 <+147>:	je     0x40016e3c6 <Flogcount+232>
   0x000000040016e373 <+149>:	mov    %ebx,%eax
   0x000000040016e375 <+151>:	and    $0x7,%eax
   0x000000040016e378 <+154>:	cmp    $0x1,%eax
   0x000000040016e37b <+157>:	je     0x40016e3d3 <Flogcount+245>
   0x000000040016e37d <+159>:	mov    $0x0,%eax
   0x000000040016e382 <+164>:	test   %eax,%eax
   0x000000040016e384 <+166>:	jne    0x40016e3f0 <Flogcount+274>
   0x000000040016e386 <+168>:	mov    0x53d1b3(%rip),%rax        # 0x4006ab540 <.refptr.suppress_checking>
   0x000000040016e38d <+175>:	cmpb   $0x0,(%rax)
   0x000000040016e390 <+178>:	jne    0x40016e3a0 <Flogcount+194>
   0x000000040016e392 <+180>:	mov    %ebx,%eax
   0x000000040016e394 <+182>:	and    $0x3,%eax
   0x000000040016e397 <+185>:	cmp    $0x2,%eax
   0x000000040016e39a <+188>:	jne    0x40016e49d <Flogcount+447>
   0x000000040016e3a0 <+194>:	mov    %rbx,%rcx
   0x000000040016e3a3 <+197>:	sar    $0x2,%rcx
   0x000000040016e3a7 <+201>:	js     0x40016e4b6 <Flogcount+472>
   0x000000040016e3ad <+207>:	callq  0x400286940 <__popcountdi2>
   0x000000040016e3b2 <+212>:	cltq   
   0x000000040016e3b4 <+214>:	lea    0x2(,%rax,4),%rbx
   0x000000040016e3bc <+222>:	mov    %rbx,%rax
   0x000000040016e3bf <+225>:	add    $0x38,%rsp
   0x000000040016e3c3 <+229>:	pop    %rbx
   0x000000040016e3c4 <+230>:	pop    %rsi
   0x000000040016e3c5 <+231>:	retq   
   0x000000040016e3c6 <+232>:	mov    %rbx,%rdx
   0x000000040016e3c9 <+235>:	mov    $0xc010,%ecx
   0x000000040016e3ce <+240>:	callq  0x40016c439 <wrong_type_argument>
   0x000000040016e3d3 <+245>:	mov    %rbx,%rcx
   0x000000040016e3d6 <+248>:	callq  0x4000e9651 <XMISCANY>
   0x000000040016e3db <+253>:	cmpw   $0x5eb1,(%rax)
   0x000000040016e3e0 <+258>:	je     0x40016e3e9 <Flogcount+267>
   0x000000040016e3e2 <+260>:	mov    $0x0,%eax
   0x000000040016e3e7 <+265>:	jmp    0x40016e382 <Flogcount+164>
   0x000000040016e3e9 <+267>:	mov    $0x1,%eax
   0x000000040016e3ee <+272>:	jmp    0x40016e382 <Flogcount+164>
   0x000000040016e3f0 <+274>:	mov    %rbx,%rcx
   0x000000040016e3f3 <+277>:	callq  0x4000e9c29 <XBIGNUM>
   0x000000040016e3f8 <+282>:	mov    0x14(%rax),%edx
   0x000000040016e3fb <+285>:	test   %edx,%edx
   0x000000040016e3fd <+287>:	js     0x40016e41d <Flogcount+319>
   0x000000040016e3ff <+289>:	jle    0x40016e416 <Flogcount+312>
   0x000000040016e401 <+291>:	mov    0x18(%rax),%rcx
   0x000000040016e405 <+295>:	callq  0x401e60494 <__imp___gmpn_popcount>
   0x000000040016e40a <+300>:	mov    %eax,%eax
   0x000000040016e40c <+302>:	lea    0x2(,%rax,4),%rbx
   0x000000040016e414 <+310>:	jmp    0x40016e3bc <Flogcount+222>
   0x000000040016e416 <+312>:	mov    $0x0,%eax
   0x000000040016e41b <+317>:	jmp    0x40016e40a <Flogcount+300>
   0x000000040016e41d <+319>:	lea    0x20(%rsp),%rsi
   0x000000040016e422 <+324>:	mov    %rsi,%rcx
   0x000000040016e425 <+327>:	callq  0x400285308 <__gmpz_init>
   0x000000040016e42a <+332>:	mov    %rbx,%rcx
   0x000000040016e42d <+335>:	callq  0x4000e9c29 <XBIGNUM>
   0x000000040016e432 <+340>:	lea    0x10(%rax),%rdx
   0x000000040016e436 <+344>:	cmp    %rsi,%rdx
   0x000000040016e439 <+347>:	je     0x40016e445 <Flogcount+359>
   0x000000040016e43b <+349>:	lea    0x20(%rsp),%rcx
   0x000000040016e440 <+354>:	callq  0x4002852d0 <__gmpz_set>
   0x000000040016e445 <+359>:	mov    0x24(%rsp),%eax
   0x000000040016e449 <+363>:	neg    %eax
   0x000000040016e44b <+365>:	mov    %eax,0x24(%rsp)
   0x000000040016e44f <+369>:	lea    0x20(%rsp),%rcx
   0x000000040016e454 <+374>:	mov    $0x1,%r8d
   0x000000040016e45a <+380>:	mov    %rcx,%rdx
   0x000000040016e45d <+383>:	callq  0x4002852a0 <__gmpz_sub_ui>
   0x000000040016e462 <+388>:	mov    0x24(%rsp),%edx
   0x000000040016e466 <+392>:	test   %edx,%edx
   0x000000040016e468 <+394>:	js     0x40016e496 <Flogcount+440>
   0x000000040016e46a <+396>:	mov    $0x0,%eax
   0x000000040016e46f <+401>:	test   %edx,%edx
   0x000000040016e471 <+403>:	jle    0x40016e47d <Flogcount+415>
   0x000000040016e473 <+405>:	mov    0x28(%rsp),%rcx
   0x000000040016e478 <+410>:	callq  0x401e60494 <__imp___gmpn_popcount>
   0x000000040016e47d <+415>:	mov    %eax,%eax
   0x000000040016e47f <+417>:	lea    0x2(,%rax,4),%rbx
   0x000000040016e487 <+425>:	lea    0x20(%rsp),%rcx
   0x000000040016e48c <+430>:	callq  0x400285360 <__gmpz_clear>
   0x000000040016e491 <+435>:	jmpq   0x40016e3bc <Flogcount+222>
   0x000000040016e496 <+440>:	mov    $0xffffffff,%eax
   0x000000040016e49b <+445>:	jmp    0x40016e46f <Flogcount+401>
   0x000000040016e49d <+447>:	mov    $0xd1c,%r8d
   0x000000040016e4a3 <+453>:	lea    0x518e66(%rip),%rdx        # 0x400687310 <gdb_make_enums_visible+264>
   0x000000040016e4aa <+460>:	lea    0x51911f(%rip),%rcx        # 0x4006875d0 <gdb_make_enums_visible+968>
   0x000000040016e4b1 <+467>:	callq  0x400160fd4 <die>
   0x000000040016e4b6 <+472>:	not    %rcx
   0x000000040016e4b9 <+475>:	jmpq   0x40016e3ad <Flogcount+207>
End of assembler dump.

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 11 Aug 2018 19:15:08 +0100
> 
>    0x000000040016e3f3 <+277>:	callq  0x4000e9c29 <XBIGNUM>
>    0x000000040016e3f8 <+282>:	mov    0x14(%rax),%edx
>    0x000000040016e3fb <+285>:	test   %edx,%edx
>    0x000000040016e3fd <+287>:	js     0x40016e41d <Flogcount+319>
>    0x000000040016e3ff <+289>:	jle    0x40016e416 <Flogcount+312>
>    0x000000040016e401 <+291>:	mov    0x18(%rax),%rcx
>    0x000000040016e405 <+295>:	callq  0x401e60494 <__imp___gmpn_popcount>
>    0x000000040016e40a <+300>:	mov    %eax,%eax
>    0x000000040016e40c <+302>:	lea    0x2(,%rax,4),%rbx
>    0x000000040016e414 <+310>:	jmp    0x40016e3bc <Flogcount+222>
>    0x000000040016e416 <+312>:	mov    $0x0,%eax
>    0x000000040016e41b <+317>:	jmp    0x40016e40a <Flogcount+300>
>    0x000000040016e41d <+319>:	lea    0x20(%rsp),%rsi
>    0x000000040016e422 <+324>:	mov    %rsi,%rcx
>    0x000000040016e425 <+327>:	callq  0x400285308 <__gmpz_init>
>    0x000000040016e42a <+332>:	mov    %rbx,%rcx
>    0x000000040016e42d <+335>:	callq  0x4000e9c29 <XBIGNUM>
>    0x000000040016e432 <+340>:	lea    0x10(%rax),%rdx
>    0x000000040016e436 <+344>:	cmp    %rsi,%rdx
>    0x000000040016e439 <+347>:	je     0x40016e445 <Flogcount+359>
>    0x000000040016e43b <+349>:	lea    0x20(%rsp),%rcx
>    0x000000040016e440 <+354>:	callq  0x4002852d0 <__gmpz_set>
>    0x000000040016e445 <+359>:	mov    0x24(%rsp),%eax
>    0x000000040016e449 <+363>:	neg    %eax
>    0x000000040016e44b <+365>:	mov    %eax,0x24(%rsp)
>    0x000000040016e44f <+369>:	lea    0x20(%rsp),%rcx
>    0x000000040016e454 <+374>:	mov    $0x1,%r8d
>    0x000000040016e45a <+380>:	mov    %rcx,%rdx
>    0x000000040016e45d <+383>:	callq  0x4002852a0 <__gmpz_sub_ui>
>    0x000000040016e462 <+388>:	mov    0x24(%rsp),%edx
>    0x000000040016e466 <+392>:	test   %edx,%edx
>    0x000000040016e468 <+394>:	js     0x40016e496 <Flogcount+440>
>    0x000000040016e46a <+396>:	mov    $0x0,%eax
>    0x000000040016e46f <+401>:	test   %edx,%edx
>    0x000000040016e471 <+403>:	jle    0x40016e47d <Flogcount+415>
>    0x000000040016e473 <+405>:	mov    0x28(%rsp),%rcx
>    0x000000040016e478 <+410>:	callq  0x401e60494 <__imp___gmpn_popcount>

Interesting: all the other GMP functions are referenced by their name,
and only __gmpn_popcount is referenced through the DLL import...

In the dependency walker display, which functions are shown in the
upper-right window (the "parent import function list"), the one that
shows "C" icons with green background, when you click on libgmp-10.dll
in the top-left window, the one that shows the dependency DLLs?

Re: bignum branch

[Andy Moreton]

On Sat 11 Aug 2018, Eli Zaretskii wrote:
> Interesting: all the other GMP functions are referenced by their name,
> and only __gmpn_popcount is referenced through the DLL import...

Indeed. As building with "-O0" works and building with "-Og" fails, it
seems that inlining could be involved. mpn_popcount is only called from
the inlined mpz_popcount.

> In the dependency walker display, which functions are shown in the
> upper-right window (the "parent import function list"), the one that
> shows "C" icons with green background, when you click on libgmp-10.dll
> in the top-left window, the one that shows the dependency DLLs?

As far as I can tell, all of the GMP exports used in emacs are listed
there, including __gmpn_popcount.

As Tom has completed merging to master, I have switched to the master
branch and rebuilt from a clean tree (after "git clean -Xdf").

Stepping through the code in gdb, I see:

(gdb) stepi
0x000000040016ebcb      1845        __gmp_result =  mpn_popcount (__gmp_u->_mp_d, __gmp_usize);
(gdb)
0x000000046ace5dc0 in ?? ()
(gdb)

Thread 1 received signal SIGSEGV, Segmentation fault.
0x000000046ace5dc0 in ?? ()
(gdb) p mpn_popcount
$5 = {<text variable, no debug info>} 0x401e61484 <__imp___gmpn_popcount>
(gdb) x/xg mpn_popcount
0x401e61484 <__imp___gmpn_popcount>:    0x000000006ace5dc0
(gdb) disas 0x000000006ace5dc0,+0x80
Dump of assembler code from 0x6ace5dc0 to 0x6ace5e40:
   0x000000006ace5dc0:  push   %rdi
   0x000000006ace5dc1:  push   %rsi
   0x000000006ace5dc2:  mov    %rcx,%rdi
   0x000000006ace5dc5:  mov    %rdx,%rsi
   0x000000006ace5dc8:  push   %r12
   0x000000006ace5dca:  push   %r13
   0x000000006ace5dcc:  movabs $0x5555555555555555,%r10
   0x000000006ace5dd6:  movabs $0x3333333333333333,%r11
   0x000000006ace5de0:  movabs $0xf0f0f0f0f0f0f0f,%rcx
   0x000000006ace5dea:  movabs $0x101010101010101,%rdx
   0x000000006ace5df4:  lea    (%rdi,%rsi,8),%rdi
   0x000000006ace5df8:  neg    %rsi
   0x000000006ace5dfb:  xor    %eax,%eax
   0x000000006ace5dfd:  bt     $0x0,%esi
   0x000000006ace5e01:  jae    0x6ace5e50
   0x000000006ace5e03:  mov    (%rdi,%rsi,8),%r8
   0x000000006ace5e07:  mov    %r8,%r9
   0x000000006ace5e0a:  shr    %r8
   0x000000006ace5e0d:  and    %r10,%r8
   0x000000006ace5e10:  sub    %r8,%r9
   0x000000006ace5e13:  mov    %r9,%r8
   0x000000006ace5e16:  shr    $0x2,%r9
   0x000000006ace5e1a:  and    %r11,%r8
   0x000000006ace5e1d:  and    %r11,%r9
   0x000000006ace5e20:  add    %r8,%r9
   0x000000006ace5e23:  mov    %r9,%r8
   0x000000006ace5e26:  shr    $0x4,%r9
   0x000000006ace5e2a:  and    %rcx,%r8
   0x000000006ace5e2d:  and    %rcx,%r9
   0x000000006ace5e30:  add    %r8,%r9
   0x000000006ace5e33:  imul   %rdx,%r9
   0x000000006ace5e37:  shr    $0x38,%r9
   0x000000006ace5e3b:  mov    %r9,%rax
   0x000000006ace5e3e:  add    $0x1,%rsi
End of assembler dump.

The disassembly above matches the start of mpn_popcount in the GMP
sources in gmp-6.1.2/mpn/x86_64/popham.asm.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 11 Aug 2018 23:15:28 +0100
> 
> As Tom has completed merging to master, I have switched to the master
> branch and rebuilt from a clean tree (after "git clean -Xdf").
> 
> Stepping through the code in gdb, I see:
> 
> (gdb) stepi
> 0x000000040016ebcb      1845        __gmp_result =  mpn_popcount (__gmp_u->_mp_d, __gmp_usize);
> (gdb)
> 0x000000046ace5dc0 in ?? ()
> (gdb)
> 
> Thread 1 received signal SIGSEGV, Segmentation fault.
> 0x000000046ace5dc0 in ?? ()

I don't see this here, with mingw.org's GMP library.

If you step through the code after typing

  (gdb) set debugexceptions on

what Windows exception is reported that leads to this SIGSEGV?

Also, could you try compiling and running the small program attached
below.  It is a slightly modified code of Flogcount, and I'm curious
to know whether it crashes in the same way if you compile it like the
crashing Emacs: with the -Og switch and with gmp.h set up for static
linking.  (It didn't crash for me here.)  Also, do you see there the
same call to __imp___gmpn_popcount as in the Emacs case.

After compiling this, you can run it with different arguments to get
the bit counts, and the result can be displayed as "echo %ERRORLEVEL%"
in cmd.exe or "echo $?"  in Bash.

#include <stdlib.h>
#include <gmp.h>

int
main (int argc, char *argv[])
{
  int value = argc > 1 ? atoi (argv[1]) : 42;
  mpz_t tem, tem1;
  int retval;
  mpz_init (tem);
  mpz_add_ui (tem, tem, value);
  if (mpz_sgn (tem) >= 0)
    retval = mpz_popcount (tem);
  else
    {
      mpz_init (tem1);
      mpz_neg (tem1, tem);
      mpz_sub_ui (tem1, tem1, 1);
      retval = mpz_popcount (tem1);
      mpz_clear (tem1);
    }
  return retval;
}

Re: bignum branch

[Andy Moreton]

On Sun 12 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sat, 11 Aug 2018 23:15:28 +0100
>> 
>> As Tom has completed merging to master, I have switched to the master
>> branch and rebuilt from a clean tree (after "git clean -Xdf").
>> 
>> Stepping through the code in gdb, I see:
>> 
>> (gdb) stepi
>> 0x000000040016ebcb      1845        __gmp_result =  mpn_popcount (__gmp_u->_mp_d, __gmp_usize);
>> (gdb)
>> 0x000000046ace5dc0 in ?? ()
>> (gdb)
>> 
>> Thread 1 received signal SIGSEGV, Segmentation fault.
>> 0x000000046ace5dc0 in ?? ()
>
> I don't see this here, with mingw.org's GMP library.
>
> If you step through the code after typing
>
>   (gdb) set debugexceptions on
>
> what Windows exception is reported that leads to this SIGSEGV?

(gdb) n
gdb: Target exception EXCEPTION_SINGLE_STEP at 0x40016c2ee
gdb: Target exception EXCEPTION_SINGLE_STEP at 0x4000e9446
gdb: Target exception EXCEPTION_BREAKPOINT at 0x40016c2f3
gdb: Target exception EXCEPTION_SINGLE_STEP at 0x40016c2f6
gdb: Target exception EXCEPTION_SINGLE_STEP at 0x40016c2f8
gdb: Target exception EXCEPTION_SINGLE_STEP at 0x40016c2fa
3335            return make_fixnum (mpz_popcount (XBIGNUM (value)->value));
(gdb) s
__gmpz_popcount (__gmp_u=0x400c0a768 <dumped_data+4928520>) at C:/msys64/mingw64/include/gmp.h:1844
1844      if (__GMP_LIKELY (__gmp_usize > 0))
(gdb)
[New Thread 836.0x888]
gdb: Target exception EXCEPTION_SINGLE_STEP at 0x40016c300
1845        __gmp_result =  mpn_popcount (__gmp_u->_mp_d, __gmp_usize);
(gdb)
gdb: Target exception EXCEPTION_SINGLE_STEP at 0x40016c304
gdb: Target exception EXCEPTION_SINGLE_STEP at 0x46ace5dc0
0x000000046ace5dc0 in ?? ()
(gdb)
Cannot find bounds of current function
(gdb) stepi
gdb: Target exception EXCEPTION_ACCESS_VIOLATION at 0x46ace5dc0

Thread 1 received signal SIGSEGV, Segmentation fault.
0x000000046ace5dc0 in ?? ()
(gdb)

> Also, could you try compiling and running the small program attached
> below.  It is a slightly modified code of Flogcount, and I'm curious
> to know whether it crashes in the same way if you compile it like the
> crashing Emacs: with the -Og switch and with gmp.h set up for static
> linking.  (It didn't crash for me here.)  Also, do you see there the
> same call to __imp___gmpn_popcount as in the Emacs case.

I don't see a crash. Your program only accepts non-negative numbers that
are small enough to use only a single limb, so may not be representative
as a cut down test case.

I saved the code in foo.c and built with "gcc -Og -o foo.exe foo.c -lgmp".
Dumping in gdb, I see the same call to __imp___gmpn_popcount:

(gdb) disas main
Dump of assembler code for function main:
   0x0000000000401560 <+0>:     push   %rsi
   0x0000000000401561 <+1>:     push   %rbx
   0x0000000000401562 <+2>:     sub    $0x48,%rsp
   0x0000000000401566 <+6>:     mov    %ecx,%ebx
   0x0000000000401568 <+8>:     mov    %rdx,%rsi
   0x000000000040156b <+11>:    callq  0x4016f0 <__main>
   0x0000000000401570 <+16>:    cmp    $0x1,%ebx
   0x0000000000401573 <+19>:    jg     0x4015b4 <main+84>
   0x0000000000401575 <+21>:    mov    $0x2a,%esi
   0x000000000040157a <+26>:    lea    0x30(%rsp),%rbx
   0x000000000040157f <+31>:    mov    %rbx,%rcx
   0x0000000000401582 <+34>:    callq  0x401640 <__gmpz_init>
   0x0000000000401587 <+39>:    mov    %esi,%r8d
   0x000000000040158a <+42>:    mov    %rbx,%rdx
   0x000000000040158d <+45>:    mov    %rbx,%rcx
   0x0000000000401590 <+48>:    callq  0x401650 <__gmpz_add_ui>
   0x0000000000401595 <+53>:    mov    0x34(%rsp),%edx
   0x0000000000401599 <+57>:    test   %edx,%edx
   0x000000000040159b <+59>:    js     0x4015c8 <main+104>
   0x000000000040159d <+61>:    jle    0x4015c1 <main+97>
   0x000000000040159f <+63>:    mov    0x38(%rsp),%rcx
   0x00000000004015a4 <+68>:    callq  0x408220 <__imp___gmpn_popcount>
   0x00000000004015a9 <+73>:    mov    %eax,%ebx
   0x00000000004015ab <+75>:    mov    %ebx,%eax
   0x00000000004015ad <+77>:    add    $0x48,%rsp
   0x00000000004015b1 <+81>:    pop    %rbx
   0x00000000004015b2 <+82>:    pop    %rsi
   0x00000000004015b3 <+83>:    retq
   0x00000000004015b4 <+84>:    mov    0x8(%rsi),%rcx
   0x00000000004015b8 <+88>:    callq  0x402c68 <atoi>
   0x00000000004015bd <+93>:    mov    %eax,%esi
   0x00000000004015bf <+95>:    jmp    0x40157a <main+26>
   0x00000000004015c1 <+97>:    mov    $0x0,%eax
   0x00000000004015c6 <+102>:   jmp    0x4015a9 <main+73>
   0x00000000004015c8 <+104>:   lea    0x20(%rsp),%rbx
   0x00000000004015cd <+109>:   mov    %rbx,%rcx
   0x00000000004015d0 <+112>:   callq  0x401640 <__gmpz_init>
   0x00000000004015d5 <+117>:   lea    0x30(%rsp),%rdx
   0x00000000004015da <+122>:   mov    %rbx,%rcx
   0x00000000004015dd <+125>:   callq  0x401638 <__gmpz_set>
   0x00000000004015e2 <+130>:   mov    0x24(%rsp),%eax
   0x00000000004015e6 <+134>:   neg    %eax
   0x00000000004015e8 <+136>:   mov    %eax,0x24(%rsp)
   0x00000000004015ec <+140>:   mov    $0x1,%r8d
   0x00000000004015f2 <+146>:   mov    %rbx,%rdx
   0x00000000004015f5 <+149>:   mov    %rbx,%rcx
   0x00000000004015f8 <+152>:   callq  0x401630 <__gmpz_sub_ui>
   0x00000000004015fd <+157>:   mov    0x24(%rsp),%edx
   0x0000000000401601 <+161>:   test   %edx,%edx
   0x0000000000401603 <+163>:   js     0x401626 <main+198>
   0x0000000000401605 <+165>:   mov    $0x0,%eax
   0x000000000040160a <+170>:   test   %edx,%edx
   0x000000000040160c <+172>:   jle    0x401618 <main+184>
   0x000000000040160e <+174>:   mov    0x28(%rsp),%rcx
   0x0000000000401613 <+179>:   callq  0x408220 <__imp___gmpn_popcount>
   0x0000000000401618 <+184>:   mov    %eax,%ebx
   0x000000000040161a <+186>:   lea    0x20(%rsp),%rcx
   0x000000000040161f <+191>:   callq  0x401648 <__gmpz_clear>
   0x0000000000401624 <+196>:   jmp    0x4015ab <main+75>
   0x0000000000401626 <+198>:   mov    $0xffffffff,%eax
   0x000000000040162b <+203>:   jmp    0x40160a <main+170>
   0x000000000040162d <+205>:   nop
   0x000000000040162e <+206>:   nop
   0x000000000040162f <+207>:   nop
End of assembler dump.
(gdb)

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sun, 12 Aug 2018 20:44:03 +0100
> 
> gdb: Target exception EXCEPTION_SINGLE_STEP at 0x40016c304
> gdb: Target exception EXCEPTION_SINGLE_STEP at 0x46ace5dc0
> 0x000000046ace5dc0 in ?? ()
> (gdb)
> Cannot find bounds of current function
> (gdb) stepi
> gdb: Target exception EXCEPTION_ACCESS_VIOLATION at 0x46ace5dc0
> 
> Thread 1 received signal SIGSEGV, Segmentation fault.
> 0x000000046ace5dc0 in ?? ()

Hmm... and what does this say?

  (gdb) x/i 0x000000046ace5dc0

This address looks bogus to me.  Earlier, you reported:

> Thread 1 received signal SIGSEGV, Segmentation fault.
> 0x000000046ace5dc0 in ?? ()
> (gdb) p mpn_popcount
> $5 = {<text variable, no debug info>} 0x401e61484 <__imp___gmpn_popcount>
> (gdb) x/xg mpn_popcount
> 0x401e61484 <__imp___gmpn_popcount>:    0x000000006ace5dc0
> (gdb) disas 0x000000006ace5dc0,+0x80

I think your disassembly used the wrong address, it should have used
this:

  (gdb) disas 0x401e61484,+0x80

I'd expect to see an indirect jump there.

And notice how 0x000000006ace5dc0, the value at __imp___gmpn_popcount,
and 0x000000046ace5dc0, where Emacs crashed, are the same value, up to
the 0x0000000400000000 bit.  Hmm...

> I don't see a crash. Your program only accepts non-negative numbers that
> are small enough to use only a single limb, so may not be representative
> as a cut down test case.

Feel free to change the program as you see fit.  I hoped that we will
have a small enough test case to report to GCC and GMP developers.  If
not, maybe it's worth to report to the GMP list anyway, they could
have some ideas.

It may also be a good idea to report the problem with gmp.h to the
MSYS2 forum, they should fix the header anyway.  (Mingw.org already
fixed theirs, as I reported related problems, not about Emacs, a few
months ago.)

Thanks.

Re: bignum branch

[Andy Moreton]

On Mon 13 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sun, 12 Aug 2018 20:44:03 +0100
>> 
>> gdb: Target exception EXCEPTION_SINGLE_STEP at 0x40016c304
>> gdb: Target exception EXCEPTION_SINGLE_STEP at 0x46ace5dc0
>> 0x000000046ace5dc0 in ?? ()
>> (gdb)
>> Cannot find bounds of current function
>> (gdb) stepi
>> gdb: Target exception EXCEPTION_ACCESS_VIOLATION at 0x46ace5dc0
>> 
>> Thread 1 received signal SIGSEGV, Segmentation fault.
>> 0x000000046ace5dc0 in ?? ()
>
> Hmm... and what does this say?
>
>   (gdb) x/i 0x000000046ace5dc0

This is a different binary (master rebuilt from commit eb787d749f28),
but eh address are the same:

(gdb) s
__gmpz_popcount (__gmp_u=0x400c2e688 <dumped_data+5075752>) at C:/msys64/mingw64/include/gmp.h:1844
1844      if (__GMP_LIKELY (__gmp_usize > 0))
(gdb)
1845        __gmp_result =  mpn_popcount (__gmp_u->_mp_d, __gmp_usize);
(gdb)
0x000000046ace5dc0 in ?? ()
(gdb) x/i 0x000000046ace5dc0
=> 0x46ace5dc0: Cannot access memory at address 0x46ace5dc0
(gdb)
Undefined command: "".  Try "help".
(gdb) x/i 0x000000006ace5dc0
   0x6ace5dc0:  push   %rdi
(gdb)

>
> This address looks bogus to me.  Earlier, you reported:
>
>> Thread 1 received signal SIGSEGV, Segmentation fault.
>> 0x000000046ace5dc0 in ?? ()
>> (gdb) p mpn_popcount
>> $5 = {<text variable, no debug info>} 0x401e61484 <__imp___gmpn_popcount>
>> (gdb) x/xg mpn_popcount
>> 0x401e61484 <__imp___gmpn_popcount>:    0x000000006ace5dc0
>> (gdb) disas 0x000000006ace5dc0,+0x80
>
> I think your disassembly used the wrong address, it should have used
> this:
>
>   (gdb) disas 0x401e61484,+0x80
>
> I'd expect to see an indirect jump there.

Agreed.

> And notice how 0x000000006ace5dc0, the value at __imp___gmpn_popcount,
> and 0x000000046ace5dc0, where Emacs crashed, are the same value, up to
> the 0x0000000400000000 bit.  Hmm...

And notice that 0x0000000400000000 is the image base of emacs.exe:

..mingw64-x86_64/src> objdump -x emacs.exe | grep ^ImageBase
ImageBase               0000000400000000

This is almost as if it has assumed that __gmpn_popcount is a function
from the text of emacs.exe rather than from libgmp-10.dll.

> Feel free to change the program as you see fit.  I hoped that we will
> have a small enough test case to report to GCC and GMP developers.  If
> not, maybe it's worth to report to the GMP list anyway, they could
> have some ideas.

I have made some attempts to reproduce that bad behaviour, but so far
without success. I'll report back if I make more progress.

> It may also be a good idea to report the problem with gmp.h to the
> MSYS2 forum, they should fix the header anyway.  (Mingw.org already
> fixed theirs, as I reported related problems, not about Emacs, a few
> months ago.)

Looking at the MSYS2 on github, there was a pull request to fix the
broken header install problem for GMP, but the maintainers dropped it:
<https://github.com/Alexpux/MINGW-packages/pull/3941>

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Tue, 14 Aug 2018 00:13:54 +0100
> 
> > It may also be a good idea to report the problem with gmp.h to the
> > MSYS2 forum, they should fix the header anyway.  (Mingw.org already
> > fixed theirs, as I reported related problems, not about Emacs, a few
> > months ago.)
> 
> Looking at the MSYS2 on github, there was a pull request to fix the
> broken header install problem for GMP, but the maintainers dropped it:
> <https://github.com/Alexpux/MINGW-packages/pull/3941>

It sounds like they want GMP and MPFR to be available only for static
linking, "for simplicity".  That is OK, and is their prerogative, but
then the package should come without the libgmp.dll.a import library
(and maybe even without the DLL), because the GNU linker will prefer
linking against the DLL if it sees the import library.  IOW, their
decision seems to be inconsistent with the package contents.
(Assuming that the import library you have is not a left-over from
some old installation, that is.)

Re: bignum branch

[Andy Moreton]

On Tue 14 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Tue, 14 Aug 2018 00:13:54 +0100
>> 
>> > It may also be a good idea to report the problem with gmp.h to the
>> > MSYS2 forum, they should fix the header anyway.  (Mingw.org already
>> > fixed theirs, as I reported related problems, not about Emacs, a few
>> > months ago.)
>> 
>> Looking at the MSYS2 on github, there was a pull request to fix the
>> broken header install problem for GMP, but the maintainers dropped it:
>> <https://github.com/Alexpux/MINGW-packages/pull/3941>
>
> It sounds like they want GMP and MPFR to be available only for static
> linking, "for simplicity".  That is OK, and is their prerogative, but
> then the package should come without the libgmp.dll.a import library
> (and maybe even without the DLL), because the GNU linker will prefer
> linking against the DLL if it sees the import library.  IOW, their
> decision seems to be inconsistent with the package contents.

Yes, that was my conclusion too.

> (Assuming that the import library you have is not a left-over from
> some old installation, that is.)

No, the lib is cleanly installed from the package. My earlier
experiments with removing the import library before linking and then
reinstalling the package elicited a warning from the package manager
about the missing file, which was then reinstalled (all as expected).

It seems the answer is patch the header manually , or avoid -Og builds.
I have not yet tried an -O2 build to see if that works.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Tue, 14 Aug 2018 16:11:36 +0100
> 
> It seems the answer is patch the header manually , or avoid -Og builds.

I think that could be dangerous of the MSYS2 maintainers want GMP to
be linked statically.  A better course of action would be to remove or
rename libgmp.dll.a.  (I understand the annoyance with the warning
from pacman, but wouldn't it also whine if you modify the header
file?)

> I have not yet tried an -O2 build to see if that works.

Even if it does, I think one cannot rely on that.

Re: bignum branch

[Andy Moreton]

On Tue 14 Aug 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Tue, 14 Aug 2018 16:11:36 +0100
>> 
>> It seems the answer is patch the header manually , or avoid -Og builds.
>
> I think that could be dangerous of the MSYS2 maintainers want GMP to
> be linked statically.  A better course of action would be to remove or
> rename libgmp.dll.a.  (I understand the annoyance with the warning
> from pacman, but wouldn't it also whine if you modify the header
> file?)
>
>> I have not yet tried an -O2 build to see if that works.
>
> Even if it does, I think one cannot rely on that.

Another approach is to force static linking. I tried setting GMP_LIB on
the configure command line, but that did not seem to propagate to the
resulting Makefile. However, hacking <builddir>/src/Makefile to have
this works:

    GMP_LIB = -Wl,-Bstatic -lgmp -Wl,-Bdynamic

That resulted in a binary with statically linked GMP, and where
evaluating "(logcount #xfffffffffffffffffffffffffffff)" does not crash.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Tue, 14 Aug 2018 17:16:46 +0100
> 
> Another approach is to force static linking.

Since my suggestion to the same effect up-thread was voted down, I
think using static linking on a single platform would be a mistake,
from the maintenance POV.

How about reopening that closed ticket on the MSYS2 issue tracker, and
asking them to make the distribution consistent with how they want
users to link against GMP?

Re: bignum branch

[Andy Moreton]

On Sat 11 Aug 2018, Eli Zaretskii wrote:
> In case you already run the test from GDB: Is it true that logcount
> always crashes in this build?

It always crashes for bignum inputs, but fixnums are ok:
  (logcount most-positive-fixnum)
  => 61
  (logcount most-negative-fixnum)
  => 61
  (logcount (1+ most-positive-fixnum))
  => CRASH
  (logcount (1- most-negative-fixnum))
  => CRASH

Re: bignum branch

[Stefan Monnier]

>> >> Emacs currently links against the shared library on MSYS2 64bit, and has
>> >> a dependency on "c:/msys64/mingw64/bin/libgmp-10.dll".
>> > You mean, on the bignum branch or on master/emacs-26?  If the latter,

Under GNU/Linux, Emacs (at least both the 25 and 26 versions I have
here) link dynamically with libgmp.  This is probably happening
via libgnutls.


        Stefan

Re: bignum branch

[Andy Moreton]

On Fri 10 Aug 2018, Stefan Monnier wrote:

>>> >> Emacs currently links against the shared library on MSYS2 64bit, and has
>>> >> a dependency on "c:/msys64/mingw64/bin/libgmp-10.dll".
>>> > You mean, on the bignum branch or on master/emacs-26?  If the latter,
>
> Under GNU/Linux, Emacs (at least both the 25 and 26 versions I have
> here) link dynamically with libgmp.  This is probably happening
> via libgnutls.

Good point. On Windows, emacs loads gnutls (and many other libraries)
explicitly using LoadLibrary/GetProcAddress (similar to dlopen/dlsym).
The Process Explorer utility can be used to show the loaded DLLs.

The master branch mingw64 x86_64 build does not load libgmp-10.dll when run
as "emacs -Q". However when run normally to use the package manage and
gnus, Process Explorer shows libgmp-10.dll is loaded.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 10 Aug 2018 11:25:24 -0400
> 
> Under GNU/Linux, Emacs (at least both the 25 and 26 versions I have
> here) link dynamically with libgmp.  This is probably happening
> via libgnutls.

Yes, but that doesn't happen on Windows; and in any case, the fact
that GnuTLS loads the GMP shared library shouldn't have any effect on
the statically-linked libgmp, because the calls to the shared library
are all inside GnuTLS, not inside Emacs's own sources.

Re: bignum branch

[Stefan Monnier]

>> Andy> 3) Another crash which I have not yet had time to analyse with gdb.
> This crashes in data-tests-logcount from test/src/data-tests.el, so there is
> a problem in logcount.

Might be related to https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29865


        Stefan

Re: bignum branch

[Andy Moreton]

On Wed 08 Aug 2018, Stefan Monnier wrote:

>>> Andy> 3) Another crash which I have not yet had time to analyse with gdb.
>> This crashes in data-tests-logcount from test/src/data-tests.el, so there is
>> a problem in logcount.
>
> Might be related to https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29865

Unlikely, as the code has already checked it is a non-negative bignum at
this point.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Date: Wed, 08 Aug 2018 17:37:27 -0600
> Cc: emacs-devel@gnu.org
> 
> Andy> 1) A crash in the json tests
> Andy>    This is bug#32381, fixed on master in commit 3eac378c96.
> 
> I didn't see this, but since it is unrelated to the branch, I wouldn't
> have anyway.

That crash was Windows-specific, so you are unlikely to see it on
other platforms.

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> Please check this works for you, and feel free to commit it to the
Andy> bignum branch.

I haven't read it in great detail yet but I do see you fixed a number of
bugs in there.  Thank you.

Tom

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> The patch has been tested with the attached ccl-tests.el ERT tests to
Andy> check that ash/lsh shifts behave properly, and that the CCL machinery
Andy> uses its 28bit codewords correctly in a bignum build.

Andy> Please check this works for you, and feel free to commit it to the
Andy> bignum branch.

I've applied the patch but the new test fails for me.
I've appended the log.

I'm still going to push your patch to the branch in a minute, because I
read through it and I agree with all the changes.  Thanks again for
doing this.

I fixed a couple of trivial formatting issues in your patch, and I also
added a comment by ccl-fixnum, but otherwise it's what you wrote.

Tom


Running 7 tests (2018-08-04 10:37:28-0600, selector `(not (tag :unstable))')
   passed  1/7  ccl-compile-midi (0.000601 sec)
   passed  2/7  ccl-compile-pgg (0.000303 sec)
Test ccl-dump-midi backtrace:
  signal(ert-test-failed (((should (equal (buffer-string) prog-midi-du
  ert-fail(((should (equal (buffer-string) prog-midi-dump)) :form (equ
  (if (unwind-protect (setq value-92 (apply fn-90 args-91)) (setq form
  (let (form-description-94) (if (unwind-protect (setq value-92 (apply
  (let ((value-92 'ert-form-evaluation-aborted-93)) (let (form-descrip
  (let* ((fn-90 #'equal) (args-91 (condition-case err (let ((signal-ho
  (progn (ccl-dump prog-midi-code) (let* ((fn-90 #'equal) (args-91 (co
  (unwind-protect (progn (ccl-dump prog-midi-code) (let* ((fn-90 #'equ
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*"))) (save-current-b
  (lambda nil (let ((temp-buffer (generate-new-buffer " *temp*"))) (sa
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name ccl-dump-midi :documentation nil :bod
  ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable)) 
  ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
  ert-run-tests-batch((not (tag :unstable)))
  ert-run-tests-batch-and-exit((not (tag :unstable)))
  eval((ert-run-tests-batch-and-exit '(not (tag :unstable))))
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/international/ccl-te
  command-line()
  normal-top-level()
Test ccl-dump-midi condition:
    (ert-test-failed
     ((should
       (equal
	(buffer-string)
	prog-midi-dump))
      :form
      (equal "Out-buffer must be 2 times bigger than in-buffer.
Main-body:
    2:[read-jump-cond-expr-const] read r0, if !(r0 < 128), jump to 22(+20)
    5:[branch] jump to array[r3] of length 4
	11 12 15 18 22 
   11:[jump] jump to 2(-9)
   12:[set-register] r1 = r0
   13:[set-register] r0 = r4
   14:[jump] jump to 41(+27)
   15:[set-register] r1 = r0
   16:[set-short-const] r3 = 3
   17:[jump] jump to 2(-15)
   18:[set-register] r2 = r0
   19:[set-short-const] r3 = 2
   20:[set-register] r0 = r4
   21:[jump] jump to 41(+20)
   22:[jump-cond-expr-const] if !(r0 >= 248), jump to 26(+4)
   25:[jump] jump to 41(+16)
   26:[jump-cond-expr-const] if !(r0 < 240), jump to 39(+13)
   29:[set-register] r4 = r0
   30:[set-expr-const] r7 = r0 & 224
   32:[jump-cond-expr-const] if !(r7 == 192), jump to 37(+5)
   35:[set-short-const] r3 = 1
   36:[jump] jump to 38(+2)
   37:[set-short-const] r3 = 2
   38:[jump] jump to 2(-36)
   39:[set-short-const] r3 = 0
   40:[jump] jump to 2(-38)
   41:[set-expr-const] r7 = r0 & 240
   43:[jump-cond-expr-const] if !(r7 == 144), jump to 59(+16)
   46:[jump-cond-expr-const] if !(r2 == 0), jump to 52(+6)
   49:[set-assign-expr-const] r0 -= 16
   51:[jump] jump to 59(+8)
   52:[set-assign-expr-const] r0 &= 15
   54:[write-const-string] write char \".\"
   55:[write-register] write r0 (2 remaining)
   56:[write-register] write r1 (1 remaining)
   57:[write-register] write r2 (0 remaining)
   58:[jump] jump to 2(-56)
   59:[set-expr-const] r7 = r0 & 240
   61:[jump-cond-expr-const] if !(r7 == 128), jump to 71(+10)
   64:[set-assign-expr-const] r0 &= 15
   66:[write-const-string] write char \".\"
   67:[write-register] write r0 (2 remaining)
   68:[write-register] write r1 (1 remaining)
   69:[write-register] write r2 (0 remaining)
   70:[jump] jump to 2(-68)
   71:[jump] jump to 2(-69)
At EOF:
   72:[end] end
" "Out-buffer must be 2 times bigger than in-buffer.
Main-body:
    2:[read-jump-cond-expr-const] read r0, if !(r0 < 128), jump to 22(+20)
    5:[branch] jump to array[r3] of length 4
	11 12 15 18 22
   11:[jump] jump to 2(-9)
   12:[set-register] r1 = r0
   13:[set-register] r0 = r4
   14:[jump] jump to 41(+27)
   15:[set-register] r1 = r0
   16:[set-short-const] r3 = 3
   17:[jump] jump to 2(-15)
   18:[set-register] r2 = r0
   19:[set-short-const] r3 = 2
   20:[set-register] r0 = r4
   21:[jump] jump to 41(+20)
   22:[jump-cond-expr-const] if !(r0 >= 248), jump to 26(+4)
   25:[jump] jump to 41(+16)
   26:[jump-cond-expr-const] if !(r0 < 240), jump to 39(+13)
   29:[set-register] r4 = r0
   30:[set-expr-const] r7 = r0 & 224
   32:[jump-cond-expr-const] if !(r7 == 192), jump to 37(+5)
   35:[set-short-const] r3 = 1
   36:[jump] jump to 38(+2)
   37:[set-short-const] r3 = 2
   38:[jump] jump to 2(-36)
   39:[set-short-const] r3 = 0
   40:[jump] jump to 2(-38)
   41:[set-expr-const] r7 = r0 & 240
   43:[jump-cond-expr-const] if !(r7 == 144), jump to 59(+16)
   46:[jump-cond-expr-const] if !(r2 == 0), jump to 52(+6)
   49:[set-assign-expr-const] r0 -= 16
   51:[jump] jump to 59(+8)
   52:[set-assign-expr-const] r0 &= 15
   54:[write-const-string] write char \".\"
   55:[write-register] write r0 (2 remaining)
   56:[write-register] write r1 (1 remaining)
   57:[write-register] write r2 (0 remaining)
   58:[jump] jump to 2(-56)
   59:[set-expr-const] r7 = r0 & 240
   61:[jump-cond-expr-const] if !(r7 == 128), jump to 71(+10)
   64:[set-assign-expr-const] r0 &= 15
   66:[write-const-string] write char \".\"
   67:[write-register] write r0 (2 remaining)
   68:[write-register] write r1 (1 remaining)
   69:[write-register] write r2 (0 remaining)
   70:[jump] jump to 2(-68)
   71:[jump] jump to 2(-69)
At EOF:
   72:[end] end
")
      :value nil :explanation
      (arrays-of-different-length 1843 1842 "Out-buffer must be 2 times bigger than in-buffer.
Main-body:
    2:[read-jump-cond-expr-const] read r0, if !(r0 < 128), jump to 22(+20)
    5:[branch] jump to array[r3] of length 4
	11 12 15 18 22 
   11:[jump] jump to 2(-9)
   12:[set-register] r1 = r0
   13:[set-register] r0 = r4
   14:[jump] jump to 41(+27)
   15:[set-register] r1 = r0
   16:[set-short-const] r3 = 3
   17:[jump] jump to 2(-15)
   18:[set-register] r2 = r0
   19:[set-short-const] r3 = 2
   20:[set-register] r0 = r4
   21:[jump] jump to 41(+20)
   22:[jump-cond-expr-const] if !(r0 >= 248), jump to 26(+4)
   25:[jump] jump to 41(+16)
   26:[jump-cond-expr-const] if !(r0 < 240), jump to 39(+13)
   29:[set-register] r4 = r0
   30:[set-expr-const] r7 = r0 & 224
   32:[jump-cond-expr-const] if !(r7 == 192), jump to 37(+5)
   35:[set-short-const] r3 = 1
   36:[jump] jump to 38(+2)
   37:[set-short-const] r3 = 2
   38:[jump] jump to 2(-36)
   39:[set-short-const] r3 = 0
   40:[jump] jump to 2(-38)
   41:[set-expr-const] r7 = r0 & 240
   43:[jump-cond-expr-const] if !(r7 == 144), jump to 59(+16)
   46:[jump-cond-expr-const] if !(r2 == 0), jump to 52(+6)
   49:[set-assign-expr-const] r0 -= 16
   51:[jump] jump to 59(+8)
   52:[set-assign-expr-const] r0 &= 15
   54:[write-const-string] write char \".\"
   55:[write-register] write r0 (2 remaining)
   56:[write-register] write r1 (1 remaining)
   57:[write-register] write r2 (0 remaining)
   58:[jump] jump to 2(-56)
   59:[set-expr-const] r7 = r0 & 240
   61:[jump-cond-expr-const] if !(r7 == 128), jump to 71(+10)
   64:[set-assign-expr-const] r0 &= 15
   66:[write-const-string] write char \".\"
   67:[write-register] write r0 (2 remaining)
   68:[write-register] write r1 (1 remaining)
   69:[write-register] write r2 (0 remaining)
   70:[jump] jump to 2(-68)
   71:[jump] jump to 2(-69)
At EOF:
   72:[end] end
" "Out-buffer must be 2 times bigger than in-buffer.
Main-body:
    2:[read-jump-cond-expr-const] read r0, if !(r0 < 128), jump to 22(+20)
    5:[branch] jump to array[r3] of length 4
	11 12 15 18 22
   11:[jump] jump to 2(-9)
   12:[set-register] r1 = r0
   13:[set-register] r0 = r4
   14:[jump] jump to 41(+27)
   15:[set-register] r1 = r0
   16:[set-short-const] r3 = 3
   17:[jump] jump to 2(-15)
   18:[set-register] r2 = r0
   19:[set-short-const] r3 = 2
   20:[set-register] r0 = r4
   21:[jump] jump to 41(+20)
   22:[jump-cond-expr-const] if !(r0 >= 248), jump to 26(+4)
   25:[jump] jump to 41(+16)
   26:[jump-cond-expr-const] if !(r0 < 240), jump to 39(+13)
   29:[set-register] r4 = r0
   30:[set-expr-const] r7 = r0 & 224
   32:[jump-cond-expr-const] if !(r7 == 192), jump to 37(+5)
   35:[set-short-const] r3 = 1
   36:[jump] jump to 38(+2)
   37:[set-short-const] r3 = 2
   38:[jump] jump to 2(-36)
   39:[set-short-const] r3 = 0
   40:[jump] jump to 2(-38)
   41:[set-expr-const] r7 = r0 & 240
   43:[jump-cond-expr-const] if !(r7 == 144), jump to 59(+16)
   46:[jump-cond-expr-const] if !(r2 == 0), jump to 52(+6)
   49:[set-assign-expr-const] r0 -= 16
   51:[jump] jump to 59(+8)
   52:[set-assign-expr-const] r0 &= 15
   54:[write-const-string] write char \".\"
   55:[write-register] write r0 (2 remaining)
   56:[write-register] write r1 (1 remaining)
   57:[write-register] write r2 (0 remaining)
   58:[jump] jump to 2(-56)
   59:[set-expr-const] r7 = r0 & 240
   61:[jump-cond-expr-const] if !(r7 == 128), jump to 71(+10)
   64:[set-assign-expr-const] r0 &= 15
   66:[write-const-string] write char \".\"
   67:[write-register] write r0 (2 remaining)
   68:[write-register] write r1 (1 remaining)
   69:[write-register] write r2 (0 remaining)
   70:[jump] jump to 2(-68)
   71:[jump] jump to 2(-69)
At EOF:
   72:[end] end
" first-mismatch-at 196)))
   FAILED  3/7  ccl-dump-midi (0.001107 sec)
   passed  4/7  ccl-dump-pgg (0.000431 sec)
   passed  5/7  pgg-parse-crc24 (0.009028 sec)
   passed  6/7  pgg-parse-crc24-dump (0.000449 sec)
   passed  7/7  shift (0.000209 sec)

Ran 7 tests, 6 results as expected, 1 unexpected (2018-08-04 10:37:28-0600, 0.093933 sec)

1 unexpected results:
   FAILED  ccl-dump-midi

Re: bignum branch

[Tom Tromey]

Andy> Please check this works for you, and feel free to commit it to the
Andy> bignum branch.

Hi.  Today I found out about the mpz_import function, which would make
the slow case of uintmax_t conversion more robust.  I was wondering if
you could try the appended?  I can't readily try it since I don't think
this code path is ever used on my machine.

thanks,
Tom

diff --git a/src/alloc.c b/src/alloc.c
index 367bb73fc1..c9c3cfb496 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -3874,12 +3874,12 @@ mpz_set_uintmax_slow (mpz_t result, uintmax_t v)
 {
   /* If long is larger then a faster path is taken.  */
   eassert (sizeof (uintmax_t) > sizeof (unsigned long));
-  /* This restriction could be lifted if needed.  */
-  eassert (sizeof (uintmax_t) <= 2 * sizeof (unsigned long));
 
-  mpz_set_ui (result, v >> (CHAR_BIT * sizeof (unsigned long)));
-  mpz_mul_2exp (result, result, CHAR_BIT * sizeof (unsigned long));
-  mpz_add_ui (result, result, v & -1ul);
+  /* COUNT = 1 means just a single word of the given size.  ORDER = -1
+     is arbitrary since there's only a single word.  ENDIAN = 0 means
+     use the native endian-ness.  NAILS = 0 means use the whole
+     word.  */
+  mpz_import (result, 1, -1, sizeof (uintmax_t), 0, 0, &v);
 }
 
 \f

Re: bignum branch

[Andy Moreton]

On Sat 04 Aug 2018, Tom Tromey wrote:

>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>
> Andy> The patch has been tested with the attached ccl-tests.el ERT tests to
> Andy> check that ash/lsh shifts behave properly, and that the CCL machinery
> Andy> uses its 28bit codewords correctly in a bignum build.
>
> Andy> Please check this works for you, and feel free to commit it to the
> Andy> bignum branch.
>
> I've applied the patch but the new test fails for me.
> I've appended the log.

The test failure is caused by a missing space in the expected CCL dump
output. The following (add a single space) works for me:

diff --git a/test/lisp/international/ccl-tests.el b/test/lisp/international/ccl-tests.el
index d0c254ce91..ba6d2040e8 100644
--- a/test/lisp/international/ccl-tests.el
+++ b/test/lisp/international/ccl-tests.el
@@ -162,7 +162,7 @@ prog-midi-dump
 Main-body:
     2:[read-jump-cond-expr-const] read r0, if !(r0 < 128), jump to 22(+20)
     5:[branch] jump to array[r3] of length 4
-	11 12 15 18 22
+	11 12 15 18 22 
    11:[jump] jump to 2(-9)
    12:[set-register] r1 = r0
    13:[set-register] r0 = r4

> I fixed a couple of trivial formatting issues in your patch, and I also
> added a comment by ccl-fixnum, but otherwise it's what you wrote.

Thanks. I was going to add the explanation I gave to Eli earlier in this
thread:

;; The CCL compiled codewords are 28bits, but the CCL implementation
;; assumes that the codewords are sign-extended, so that data constants in
;; the upper part of the codeword are signed. This function truncates a
;; codeword to 28bits, and then sign extends the result to a fixnum.

Perhaps you could add that to give a more detailed explanation.
Hopefully nobody else will need to do anything further to CCL until it
is deprecated and removed. :-)

    AndyM

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> The test failure is caused by a missing space in the expected CCL dump
Andy> output. The following (add a single space) works for me:

Thanks, this was my fault, since I removed the space when git commit
complained about it.

Andy> Thanks. I was going to add the explanation I gave to Eli earlier in this
Andy> thread:

I changed ccl.el to use this comment instead.

I've pushed this to the branch.

Tom

Re: bignum branch

[Andy Moreton]

On Sun 05 Aug 2018, Tom Tromey wrote:

>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>
> Andy> The test failure is caused by a missing space in the expected CCL dump
> Andy> output. The following (add a single space) works for me:
>
> Thanks, this was my fault, since I removed the space when git commit
> complained about it.
>
> Andy> Thanks. I was going to add the explanation I gave to Eli earlier in this
> Andy> thread:
>
> I changed ccl.el to use this comment instead.
>
> I've pushed this to the branch.

Thanks Tom.

The Windows 64bit build of the bignum branch now seems stable enough for
me to use for everyday work without obvious issues in normal usage.

    AndyM

Re: bignum branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 14 Jul 2018 21:04:38 +0100
> 
> The problem is that if EMACS_INT is "long long", then the calls to
> mpz_*_si() API routines will truncate any values passed to GMP. This
> means that emacs cannot use any the mpz_*_si() routines directly.

Right, I see what you mean now: all those APIs accept 'long' or
'unsigned long' as the integral type.  So this is a limitation of GMP
as of now.  This message:

  https://gmplib.org/list-archives/gmp-discuss/2016-March/005966.html

indicates that GMP developers plan to address this in version 7, but
the current master branch of GMP is still on version 6, so I guess it
will be a while.

Btw, I had a cursory look at the GMP sources, and it seemed to me that
changing GMP to lift this limitation should not be too hard.  The
patch shown in this message:

  https://gmplib.org/list-archives/gmp-discuss/2016-March/005965.html

seems to confirm that.  So maybe someone could build GMP with MinGW64
after applying that patch, and if it works, submit the patches to
MSYS2 guys so that they could release a fixed library.  Then Emacs
won't need to jump through these hoops on 64-bit Windows systems.

Re: bignum branch

[Paul Eggert]

Eli Zaretskii wrote:
> Btw, I had a cursory look at the GMP sources, and it seemed to me that
> changing GMP to lift this limitation should not be too hard.  The
> patch shown in this message:
> 
>    https://gmplib.org/list-archives/gmp-discuss/2016-March/005965.html
> 
> seems to confirm that.  So maybe someone could build GMP with MinGW64
> after applying that patch, and if it works, submit the patches to
> MSYS2 guys so that they could release a fixed library.  Then Emacs
> won't need to jump through these hoops on 64-bit Windows systems.

It'd still have to jump through the hoops for 32-bit systems --with-wide-int, 
though. Instead, how about fixing our GMP substitute source code to work for 
this situation, and fall back on it when GMP proper doesn't handle long long? 
That should work for both 32-bit --with-wide-int and MinGW64, and won't require 
us to wait for any action on the part of MSYS2 or the GMP maintainers.

Re: bignum branch

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 15 Jul 2018 10:31:04 -0700
> 
> Eli Zaretskii wrote:
> > Btw, I had a cursory look at the GMP sources, and it seemed to me that
> > changing GMP to lift this limitation should not be too hard.  The
> > patch shown in this message:
> > 
> >    https://gmplib.org/list-archives/gmp-discuss/2016-March/005965.html
> > 
> > seems to confirm that.  So maybe someone could build GMP with MinGW64
> > after applying that patch, and if it works, submit the patches to
> > MSYS2 guys so that they could release a fixed library.  Then Emacs
> > won't need to jump through these hoops on 64-bit Windows systems.
> 
> It'd still have to jump through the hoops for 32-bit systems --with-wide-int, 
> though.

Yes.  And maybe also for other platforms.

> Instead, how about fixing our GMP substitute source code to work for 
> this situation, and fall back on it when GMP proper doesn't handle long long? 

Not sure what you mean by "our GMP substitute source code".  Do you
mean mini-gmp?  I didn't yet look at it, and so I don't know what that
means in practice.  (Someone said it's less efficient than GMP?)

> That should work for both 32-bit --with-wide-int and MinGW64, and won't require 
> us to wait for any action on the part of MSYS2 or the GMP maintainers.

There's no need to wait anyway.  We should condition use of GMP on
EMACS_INT being not wider than the appropriate GMP type (I guess
mp_bitcnt_t?), and then if and when GMP learns to support 64-bit
Windows, things will "just work".

Re: bignum branch

[Paul Eggert]

Eli Zaretskii wrote:
> Not sure what you mean by "our GMP substitute source code".  Do you
> mean mini-gmp?  I didn't yet look at it, and so I don't know what that
> means in practice.  (Someone said it's less efficient than GMP?)

Yes, I meant mini-gmp. And yes, it'd be less efficient than real GMP, something 
we'd have to live with on 32-bit-long hosts until GMP gets its act together 
(which I hope wouldn't take that long). I don't think the efficiency loss would 
be major, as bignums aren't commonly used.

> There's no need to wait anyway.  We should condition use of GMP on
> EMACS_INT being not wider than the appropriate GMP type (I guess
> mp_bitcnt_t?), and then if and when GMP learns to support 64-bit
> Windows, things will "just work".

Tom's idea is to just assume GMP in almost all the source code (and to compile 
and use mini-gmp as a substitute if libgmp is not available), as this simplifies 
a lot of things. For example, Elisp code won't have to worry about integer 
overflow, which is a significant win. If this approach performs well enough, I'd 
prefer its simplicity.

Re: bignum branch

[Eli Zaretskii]

> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 16 Jul 2018 12:02:34 -0700
> 
> Eli Zaretskii wrote:
> > Not sure what you mean by "our GMP substitute source code".  Do you
> > mean mini-gmp?  I didn't yet look at it, and so I don't know what that
> > means in practice.  (Someone said it's less efficient than GMP?)
> 
> Yes, I meant mini-gmp.

The functions in mini-gmp have the same problem, so I'm unsure why
this solution is better.

> And yes, it'd be less efficient than real GMP, something 
> we'd have to live with on 32-bit-long hosts until GMP gets its act together 
> (which I hope wouldn't take that long). I don't think the efficiency loss would 
> be major, as bignums aren't commonly used.

To, just published a patch that should work with mini-gmp and GMP
alike, so I think we already have a way ahead.

Re: bignum branch

[Paul Eggert]

On 07/16/2018 07:42 PM, Eli Zaretskii wrote:
>> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Mon, 16 Jul 2018 12:02:34 -0700
>>
>> Eli Zaretskii wrote:
>>> Not sure what you mean by "our GMP substitute source code".  Do you
>>> mean mini-gmp?  I didn't yet look at it, and so I don't know what that
>>> means in practice.  (Someone said it's less efficient than GMP?)
>> Yes, I meant mini-gmp.
> The functions in mini-gmp have the same problem, so I'm unsure why
> this solution is better.

The idea is to fix mini-gmp ourselves so that they do not have the same 
problem. This should be easy, and it should be more efficient than the 
patch that you mentioned.

Re: bignum branch

[Eli Zaretskii]

> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 17 Jul 2018 08:53:17 -0700
> 
> On 07/16/2018 07:42 PM, Eli Zaretskii wrote:
> >> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
> >> From: Paul Eggert <eggert@cs.ucla.edu>
> >> Date: Mon, 16 Jul 2018 12:02:34 -0700
> >>
> >> Eli Zaretskii wrote:
> >>> Not sure what you mean by "our GMP substitute source code".  Do you
> >>> mean mini-gmp?  I didn't yet look at it, and so I don't know what that
> >>> means in practice.  (Someone said it's less efficient than GMP?)
> >> Yes, I meant mini-gmp.
> > The functions in mini-gmp have the same problem, so I'm unsure why
> > this solution is better.
> 
> The idea is to fix mini-gmp ourselves so that they do not have the same 
> problem.

And then maintain local patches, and apply them every time we sync
with upstream GMP?  I'd rather we avoided that burden.

Re: bignum branch

[Paul Eggert]

On 07/17/2018 10:03 AM, Eli Zaretskii wrote:
> And then maintain local patches, and apply them every time we sync
> with upstream GMP?  I'd rather we avoided that burden.

That's OK, we can put it into Gnulib and then let the Gnulib maintainers 
worry about it. They can work to get the patches merged upstream into 
GMP. They already do this for lots of other code. The main question is 
whether it'd be an overall win. If the required patches are small and 
easily maintained, it will be.

Re: bignum branch

[Eli Zaretskii]

> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 17 Jul 2018 10:24:51 -0700
> 
> On 07/17/2018 10:03 AM, Eli Zaretskii wrote:
> > And then maintain local patches, and apply them every time we sync
> > with upstream GMP?  I'd rather we avoided that burden.
> 
> That's OK, we can put it into Gnulib and then let the Gnulib maintainers 
> worry about it.

Then we'd need to coordinate with Gnulib, which further complicates
maintenance for us.

I don't understand why this is needed, given that Tom published a
proposed solution for the issue.  If you are bothered by possible
run-time penalty for LP64 systems, we can use compile-time checks
instead of run-time checks.  So I really don't see any justification
for more elaborate schemes.

Re: bignum branch

[Paul Eggert]

On 07/17/2018 10:38 AM, Eli Zaretskii wrote:
> I don't understand why this is needed, given that Tom published a
> proposed solution for the issue.  If you are bothered by possible
> run-time penalty for LP64 systems, we can use compile-time checks
> instead of run-time checks.

It's mostly a performance concern on 32-bit platforms. But if nobody 
else cares about performance on 32-bit platforms, I'll stop worrying 
too. I haven't used them in years, except for porting tests.

Re: bignum branch

[Eli Zaretskii]

> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 17 Jul 2018 10:41:43 -0700
> 
> On 07/17/2018 10:38 AM, Eli Zaretskii wrote:
> > I don't understand why this is needed, given that Tom published a
> > proposed solution for the issue.  If you are bothered by possible
> > run-time penalty for LP64 systems, we can use compile-time checks
> > instead of run-time checks.
> 
> It's mostly a performance concern on 32-bit platforms.

Only on those built --with-wide-int, right?  Because otherwise, a
32-bit 'long' is just fine with GMP.

> But if nobody else cares about performance on 32-bit platforms, I'll
> stop worrying too.

I see no reason to worry about slightly less efficient bignums on some
platforms, until GMP folks get their act together.  Maybe someone
should ask the GMP developers to make those changes sooner rather than
later, to expedite the process.  After all, they should care about
Emacs, I think.

Re: bignum branch

[Paul Eggert]

Eli Zaretskii wrote:
>> It's mostly a performance concern on 32-bit platforms.
> Only on those built --with-wide-int, right?  Because otherwise, a
> 32-bit 'long' is just fine with GMP.

It could also be a problem with integer types other than EMACS_INT, which would 
affect 32-bit platforms even if --with-wide-int is not used. As I recall, Tom's 
patch works around the problems only when converting Emacs integers, so there 
could be a correctness issue there as well as an efficiency issue, since the 
problem would remain unfixed for the other integer types.

Re: bignum branch

[Eli Zaretskii]

> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 17 Jul 2018 11:55:48 -0700
> 
> Eli Zaretskii wrote:
> >> It's mostly a performance concern on 32-bit platforms.
> > Only on those built --with-wide-int, right?  Because otherwise, a
> > 32-bit 'long' is just fine with GMP.
> 
> It could also be a problem with integer types other than EMACS_INT

But that problem will affect all platforms, wouldn't it?  Because GMP
can only work with 'long', and I guess we don't want to rely on stuff
like 'prtdiff_t' and 'intmax_t' being no wider than 'long'?  We'd need
to convert the other types explicitly.

Re: bignum branch

[Paul Eggert]

Eli Zaretskii wrote:
> But that problem will affect all platforms, wouldn't it?  Because GMP
> can only work with 'long', and I guess we don't want to rely on stuff
> like 'prtdiff_t' and 'intmax_t' being no wider than 'long'?  We'd need
> to convert the other types explicitly.

Yes, of course. However, the more of these types are wider than a GMP limb, the 
more problems we'll have.

There's another thing about debugging. This stuff is still uncertain. That is, 
there are no doubt bugs in the bignum branch in this area, and we don't know yet 
how significant these correctness problems will be. With that in mind, it would 
be useful to have avoid-libgmp options with our own GMP replacement that uses 
uintmax_t limbs (or 'unsigned int' limbs, for that matter) for debugging 
purposes, to help us debug the inevitable glitches that will arise due to 
libgmp's limbs being wrong for some types.

I looked into libgmp, and it appears that all we would need is a 2-line change 
to mini-gmp.h. This should not be hard to maintain ourselves, with the idea of 
propagating it upstream when we can.

Re: bignum branch

[Eli Zaretskii]

> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 17 Jul 2018 15:39:02 -0700
> 
> There's another thing about debugging. This stuff is still uncertain. That is, 
> there are no doubt bugs in the bignum branch in this area, and we don't know yet 
> how significant these correctness problems will be. With that in mind, it would 
> be useful to have avoid-libgmp options with our own GMP replacement that uses 
> uintmax_t limbs (or 'unsigned int' limbs, for that matter) for debugging 
> purposes, to help us debug the inevitable glitches that will arise due to 
> libgmp's limbs being wrong for some types.
> 
> I looked into libgmp, and it appears that all we would need is a 2-line change 
> to mini-gmp.h. This should not be hard to maintain ourselves, with the idea of 
> propagating it upstream when we can.

Could you please elaborate and perhaps show an example or two?  I
don't think I understand what change you are proposing to make, and
how will that help debugging.

Thanks.

Re: bignum branch

[Paul Eggert]

Eli Zaretskii wrote:
> Could you please elaborate and perhaps show an example or two?  I
> don't think I understand what change you are proposing to make, and
> how will that help debugging.

The patch to mini-gmp.h is simple:

--- a/mini-gmp/mini-gmp.h	Tue Jul 03 11:16:06 2018 +0200
+++ b/mini-gmp/mini-gmp.h	Wed Jul 18 00:35:27 2018 -0700
@@ -53,9 +53,11 @@
  			      void *(**) (void *, size_t, size_t),
  			      void (**) (void *, size_t));

+#ifndef mp_limb_t
  typedef unsigned long mp_limb_t;
  typedef long mp_size_t;
  typedef unsigned long mp_bitcnt_t;
+#endif

  typedef mp_limb_t *mp_ptr;
  typedef const mp_limb_t *mp_srcptr;


When debugging, Emacs would #define mp_limb_t, mp_size_t, and mp_bitcnt_t to 
larger and/or smaller types than typical for the current platform before 
including mini-gmp.h, to check for portability gotchas on other platforms, and 
to improve performance on platforms lacking libgmp.

Re: bignum branch

[Andy Moreton]

On Wed 18 Jul 2018, Paul Eggert wrote:

> Stefan Monnier wrote:
>> I recently posted a patch which makes EQ behave like `eql` attached.
>> I just tried to see its impact on the ELisp compilation time
>> (i.e. I did `rm **/*.elc; make`).
>
> I tried to reproduce this on my machine, and ran into some trouble (the code
> had warnings that caused compilation to fail). Although your patch is
> evidently intended only as a quick benchmark and not as an actual change, I
> worry that the benchmark isn't realistic enough, as some usage of EQ in C code
> will need to change to Feql or equivalent. Also, the C code will need to
> change how hashing works since XHASH etc. must be consistent with eq.
>
> Looking into this a bit more, I discovered that eql currently operates
> incorrectly on NaNs, as it's inconsistent with how hash tables work. I
> installed the attached patch into master to fix that.

Your added testcase has:

+;; Test that equality predicates work correctly on NaNs when combined
+;; with hash tables based on those predicates.  This was not the case
+;; for eql in Emacs 26.

Shouldn't something similar be added to NEWS ?

    AndyM

Re: bignum branch

[Paul Eggert]

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

Andy Moreton wrote:
> Shouldn't something similar be added to NEWS ?

Good point, I installed the attached.

[-- Attachment #2: 0001-etc-NEWS-Mention-eql-etc.-NaN-fix.patch --]
[-- Type: text/x-patch, Size: 1048 bytes --]

From af4133fd16a8e811fb0aad170c3a6c8eb4e6857a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 18 Jul 2018 04:55:34 -0700
Subject: [PATCH] * etc/NEWS: Mention eql etc. NaN fix.

---
 etc/NEWS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index c0f3806..5648dd0 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -822,6 +822,13 @@ changes and the change hooks are time consuming.
 remote systems, which support this check.
 
 +++
+** 'eql', 'make-hash-table', etc. now treat NaNs consistently.
+Formerly, some of these functions ignored signs and significands of
+NaNs.  Now, all these functions treat NaN signs and significands as
+significant.  For example, (eql 0.0e+NaN -0.0e+NaN) now returns t
+because the two NaNs have different signs; formerly it returned nil.
+
++++
 ** The function 'make-string' accepts an additional optional argument.
 If the optional third argument is non-nil, 'make-string' will produce
 a multibyte string even if its second argument is an ASCII character.
-- 
2.7.4

Re: bignum branch

[Clément Pit-Claudel]

On 2018-07-18 07:57, Paul Eggert wrote:
> For example, (eql 0.0e+NaN -0.0e+NaN) now returns t
> because the two NaNs have different signs; formerly it returned nil.

Should this be the other way around ?

Re: bignum branch

[Stefan Monnier]

>> For example, (eql 0.0e+NaN -0.0e+NaN) now returns t
>> because the two NaNs have different signs; formerly it returned nil.
> Should this be the other way around ?

You mean it's (eql -0.0e+NaN 0.0e+NaN) which should return t?
Shouldn't `eql` be commutative?


        Stefan "confusion is power"

Re: bignum branch

[Clément Pit-Claudel]

On 2018-07-18 09:18, Stefan Monnier wrote:
>>> For example, (eql 0.0e+NaN -0.0e+NaN) now returns t
>>> because the two NaNs have different signs; formerly it returned nil.
>> Should this be the other way around ?
> You mean it's (eql -0.0e+NaN 0.0e+NaN) which should return t?
> Shouldn't `eql` be commutative?

No, I mean that Paul wrote 't' where he meant 'nil' and 'nil' where he meant 't'.

Re: bignum branch

[Andy Moreton]

On Wed 18 Jul 2018, Clément Pit-Claudel wrote:

> On 2018-07-18 09:18, Stefan Monnier wrote:
>>>> For example, (eql 0.0e+NaN -0.0e+NaN) now returns t
>>>> because the two NaNs have different signs; formerly it returned nil.
>>> Should this be the other way around ?
>> You mean it's (eql -0.0e+NaN 0.0e+NaN) which should return t?
>> Shouldn't `eql` be commutative?
>
> No, I mean that Paul wrote 't' where he meant 'nil' and 'nil' where he meant 't'.

I agree:

    ELISP> emacs-repository-version
    "c70d22f70b77b053d01c7380122d166ecb728610"
    ELISP> (eql 0.0e+NaN 0.0e+NaN)
    t
    ELISP> (eql -0.0e+NaN -0.0e+NaN)
    t
    ELISP> (eql 0.0e+NaN -0.0e+NaN)
    nil
    ELISP> (eql -0.0e+NaN 0.0e+NaN)
    nil

Re: bignum branch

[Achim Gratz]

Andy Moreton writes:
> I agree:
>
>     ELISP> emacs-repository-version
>     "c70d22f70b77b053d01c7380122d166ecb728610"
>     ELISP> (eql 0.0e+NaN 0.0e+NaN)
>     t

NaN (specifically of the IEEE754 variety) is supposed to compare
non-equal even when compared to itself.  I recognize that eql is used in
the above, but that would probably still create two LISP objects that
happen to have the same value, but the established FP math says these
two values should not compare equal anyway.  If you stray from that
convention there should be a really good reason for that and it needs to
be prominently documented.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: bignum branch

[Stefan Monnier]

> NaN (specifically of the IEEE754 variety) is supposed to compare
> non-equal even when compared to itself.

There's "compare" and there's "compare".
I think we always want (eql x x) to return t, so the above behavior
makes a lot of sense.  The rule that NaN is not equal to itself should
only be applied when doing a numerical comparison (i.e. (= x x) may
return nil).

> I recognize that eql is used in the above, but that would probably
> still create two LISP objects that happen to have the same value, but
> the established FP math says these two values should not compare equal
> anyway.  If you stray from that convention there should be a really
> good reason for that and it needs to be prominently documented.

NaNs are a minefield.  I understand there are good reasons for this
minefield, but it's good practice to try and confine the pain to those
places where it is indispensable.


        Stefan

Re: bignum branch

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 18 Jul 2018 16:41:14 -0400
> 
> > NaN (specifically of the IEEE754 variety) is supposed to compare
> > non-equal even when compared to itself.
> 
> There's "compare" and there's "compare".
> I think we always want (eql x x) to return t, so the above behavior
> makes a lot of sense.  The rule that NaN is not equal to itself should
> only be applied when doing a numerical comparison (i.e. (= x x) may
> return nil).

100% agreement.  We are testing equality of two objects, not their
numerical equality.

bignum branch

[Paul Eggert]

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

On 07/18/2018 12:25 PM, Achim Gratz wrote:
> the established FP math says these
> two values should not compare equal anyway.

Sure, and = still does that. = is about numeric comparison; eql is about value 
comparison. Neither dominates the other: for example, they disagree about 0.0 vs 
-0.0 (= says they're equal, but eql says they're distinguishable values and so 
are not equal), and conversely they disagree about 0.0e+NaN versus 0.0e+NaN (= 
says they're not equal, whereas eql says they're indistinguishable values are so 
are equal).

> If you stray from that
> convention there should be a really good reason for that and it needs to
> be prominently documented.

There is good reason: eq, eql, and equal are about distinguishing objects (vital 
for hash tables, e.g.) whereas = is about numeric comparison. These are 
different things, and the numeric-comparison rule for NaNs is inconsistent with 
how hash tables should work. I gave a shot at documenting this by installing the 
attached patch.


[-- Attachment #2: 0001-Improve-doc-for-floating-point-vs-eql.txt --]
[-- Type: text/plain, Size: 3844 bytes --]

From 96d77f9eb882b68e994e187ed9c2156a23e3279d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 19 Jul 2018 13:29:28 -0700
Subject: [PATCH] =?UTF-8?q?Improve=20doc=20for=20floating=20point=20?=
 =?UTF-8?q?=E2=80=98=3D=E2=80=99=20vs=20=E2=80=98eql=E2=80=99?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* doc/lispref/numbers.texi (Float Basics, Comparison of Numbers):
Improve documentation of ‘=’ vs ‘eq’, ‘eql’ and ‘equal’
when NaNs and signed zeros are involved.
---
 doc/lispref/numbers.texi | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/doc/lispref/numbers.texi b/doc/lispref/numbers.texi
index 6c51b84..70bb103 100644
--- a/doc/lispref/numbers.texi
+++ b/doc/lispref/numbers.texi
@@ -223,7 +223,7 @@ Float Basics
 @samp{1500.} is an integer, not a floating-point number.
 
   Emacs Lisp treats @code{-0.0} as numerically equal to ordinary zero
-with respect to @code{equal} and @code{=}.  This follows the
+with respect to numeric comparisons like @code{=}.  This follows the
 @acronym{IEEE} floating-point standard, which says @code{-0.0} and
 @code{0.0} are numerically equal even though other operations can
 distinguish them.
@@ -232,19 +232,26 @@ Float Basics
 @cindex negative infinity
 @cindex infinity
 @cindex NaN
-@findex eql
-@findex sxhash-eql
   The @acronym{IEEE} floating-point standard supports positive
 infinity and negative infinity as floating-point values.  It also
 provides for a class of values called NaN, or ``not a number'';
 numerical functions return such values in cases where there is no
 correct answer.  For example, @code{(/ 0.0 0.0)} returns a NaN@.
 A NaN is never numerically equal to any value, not even to itself.
-NaNs carry a sign and a significand, and non-numeric functions like
-@code{eql} and @code{sxhash-eql} treat two NaNs as equal when their
+NaNs carry a sign and a significand, and non-numeric functions treat
+two NaNs as equal when their
 signs and significands agree.  Significands of NaNs are
 machine-dependent and are not directly visible to Emacs Lisp.
 
+  When NaNs and signed zeros are involved, non-numeric functions like
+@code{eql}, @code{equal}, @code{sxhash-eql}, @code{sxhash-equal} and
+@code{gethash} determine whether values are indistinguishable, not
+whether they are numerically equal.  For example, when @var{x} and
+@var{y} are the same NaN, @code{(equal x y)} returns @code{t} whereas
+@code{(= x y)} uses numeric comparison and returns @code{nil};
+conversely, @code{(equal 0.0 -0.0)} returns @code{nil} whereas
+@code{(= 0.0 -0.0)} returns @code{t}.
+
 Here are read syntaxes for these special floating-point values:
 
 @table @asis
@@ -359,11 +366,15 @@ Comparison of Numbers
 @cindex comparing numbers
 
   To test numbers for numerical equality, you should normally use
-@code{=}, not @code{eq}.  There can be many distinct floating-point
-objects with the same numeric value.  If you use @code{eq} to
-compare them, then you test whether two values are the same
-@emph{object}.  By contrast, @code{=} compares only the numeric values
-of the objects.
+@code{=} instead of non-numeric comparison predicates like @code{eq},
+@code{eql} and @code{equal}.  Distinct floating-point objects can be
+numerically equal.  If you use @code{eq} to compare them, you test
+whether they are the same @emph{object}; if you use @code{eql} or
+@code{equal}, you test whether their values are
+@emph{indistinguishable}.  In contrast, @code{=} uses numeric
+comparison, and sometimes returns @code{t} when a non-numeric
+comparison would return @code{nil} and vice versa.  @xref{Float
+Basics}.
 
   In Emacs Lisp, each integer is a unique Lisp object.
 Therefore, @code{eq} is equivalent to @code{=} where integers are
-- 
2.7.4

Re: bignum branch

[Achim Gratz]

Paul Eggert writes:
> On 07/18/2018 12:25 PM, Achim Gratz wrote:
>> the established FP math says these
>> two values should not compare equal anyway.
>
> Sure, and = still does that. = is about numeric comparison; eql is
> about value comparison. Neither dominates the other: for example, they
> disagree about 0.0 vs -0.0 (= says they're equal, but eql says they're
> distinguishable values and so are not equal), and conversely they
> disagree about 0.0e+NaN versus 0.0e+NaN (= says they're not equal,
> whereas eql says they're indistinguishable values are so are equal).

I was obliquely hinting at this part of the documentation:

--8<---------------cut here---------------start------------->8---
eql is a built-in function in ‘C source code’.

(eql OBJ1 OBJ2)

Return t if the two args are the same Lisp object.
Floating-point numbers of equal value are ‘eql’, but they may not be ‘eq’.
--8<---------------cut here---------------end--------------->8---

This seems to say that eql returns a predicate for same-objectness,
except for FP numbers where it compares values instead.  Your
documentation clarification (I think) is about different ways of
comparing FP numeric values, so maybe the doc string for eql should
directly indicate that the representations of the FP numbers are
compared bit-wise, which is distinct from their numerical values as
prescribed by IEEE754 (and that comparison is done via =)?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptations for KORG EX-800 and Poly-800MkII V0.9:
http://Synth.Stromeko.net/Downloads.html#KorgSDada

Re: bignum branch

[Paul Eggert]

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

On 07/20/2018 01:02 PM, Achim Gratz wrote:
> maybe the doc string for eql should
> directly indicate that the representations of the FP numbers are
> compared bit-wise, which is distinct from their numerical values as
> prescribed by IEEE754 (and that comparison is done via =)?

Sure, I took a stab at that and installed the attached.


[-- Attachment #2: 0001-src-fns.c-Feql-Fequal-Improve-floating-point-doc.patch --]
[-- Type: text/x-patch, Size: 1602 bytes --]

From 64a168519df8c3f842df11dab01eabbfc35a42c4 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@Penguin.CS.UCLA.EDU>
Date: Fri, 20 Jul 2018 13:55:12 -0700
Subject: [PATCH] * src/fns.c (Feql, Fequal): Improve floating-point doc.

---
 src/fns.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index 7d120a90f7..e7424c3471 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2193,8 +2193,10 @@ The PLIST is modified by side effects.  */)
 }
 \f
 DEFUN ("eql", Feql, Seql, 2, 2, 0,
-       doc: /* Return t if the two args are the same Lisp object.
-Floating-point numbers of equal value are `eql', but they may not be `eq'.  */)
+       doc: /* Return t if the two args are `eq' or are indistinguishable numbers.
+Floating-point values with the same sign, exponent and fraction are `eql'.
+This differs from numeric comparison: (eql 0.0 -0.0) returns nil and
+\(eql 0.0e+NaN 0.0e+NaN) returns t, whereas `=' does the opposite.  */)
   (Lisp_Object obj1, Lisp_Object obj2)
 {
   if (FLOATP (obj1))
@@ -2208,8 +2210,8 @@ DEFUN ("equal", Fequal, Sequal, 2, 2, 0,
 They must have the same data type.
 Conses are compared by comparing the cars and the cdrs.
 Vectors and strings are compared element by element.
-Numbers are compared by value, but integers cannot equal floats.
- (Use `=' if you want integers and floats to be able to be equal.)
+Numbers are compared via `eql', so integers do not equal floats.
+\(Use `=' if you want integers and floats to be able to be equal.)
 Symbols must match exactly.  */)
   (Lisp_Object o1, Lisp_Object o2)
 {
-- 
2.17.1

Re: bignum branch

[Stefan Monnier]

> -       doc: /* Return t if the two args are the same Lisp object.
> -Floating-point numbers of equal value are `eql', but they may not be `eq'.  */)
> +       doc: /* Return t if the two args are `eq' or are indistinguishable numbers.
> +Floating-point values with the same sign, exponent and fraction are `eql'.
> +This differs from numeric comparison: (eql 0.0 -0.0) returns nil and
> +\(eql 0.0e+NaN 0.0e+NaN) returns t, whereas `=' does the opposite.  */)

Interestingly, if we change EQ to do the comparison currently performed
by `eql`, then `eql` is easier to document since "indistinguishable"
becomes closer to the truth (currently you can distinguish two `eql`
numbers by resorting to `eq`).


        Stefan

Re: bignum branch

[Achim Gratz]

Paul Eggert writes:
> On 07/20/2018 01:02 PM, Achim Gratz wrote:
>> maybe the doc string for eql should
>> directly indicate that the representations of the FP numbers are
>> compared bit-wise, which is distinct from their numerical values as
>> prescribed by IEEE754 (and that comparison is done via =)?
>
> Sure, I took a stab at that and installed the attached.

Thank you.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: bignum branch

[Paul Eggert]

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

Clément Pit-Claudel wrote:
>> For example, (eql 0.0e+NaN -0.0e+NaN) now returns t
>> because the two NaNs have different signs; formerly it returned nil.
> Should this be the other way around ?

Sorry, that's what I get for writing etc/NEWS patches at 5am. I installed the 
attached to fix this.

[-- Attachment #2: 0001-etc-NEWS-Fix-eql-typo-in-previous-change.patch --]
[-- Type: text/x-patch, Size: 1005 bytes --]

From 5934122c1f3371a07b9f041aec693d762e9d8767 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 18 Jul 2018 11:27:06 -0700
Subject: [PATCH] * etc/NEWS: Fix eql typo in previous change.

---
 etc/NEWS | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 861520b..f30ab69 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -829,8 +829,8 @@ remote systems, which support this check.
 ** 'eql', 'make-hash-table', etc. now treat NaNs consistently.
 Formerly, some of these functions ignored signs and significands of
 NaNs.  Now, all these functions treat NaN signs and significands as
-significant.  For example, (eql 0.0e+NaN -0.0e+NaN) now returns t
-because the two NaNs have different signs; formerly it returned nil.
+significant.  For example, (eql 0.0e+NaN -0.0e+NaN) now returns nil
+because the two NaNs have different signs; formerly it returned t.
 
 +++
 ** The function 'make-string' accepts an additional optional argument.
-- 
2.7.4

Re: bignum branch

[Andy Moreton]

On Tue 17 Jul 2018, Paul Eggert wrote:

> Eli Zaretskii wrote:
>> But that problem will affect all platforms, wouldn't it?  Because GMP
>> can only work with 'long', and I guess we don't want to rely on stuff
>> like 'prtdiff_t' and 'intmax_t' being no wider than 'long'?  We'd need
>> to convert the other types explicitly.
>
> Yes, of course. However, the more of these types are wider than a GMP limb,
> the more problems we'll have.

The size of a GMP limb is irrelevant - it is the API for getting native
integer types into mpz_t values that cannot handle interger types wider
than long.

Wider mp_limb_t types improve efficiency, but do nothing about
correctness on LLP64 systems.

> There's another thing about debugging. This stuff is still uncertain. That is,
> there are no doubt bugs in the bignum branch in this area, and we don't know
> yet how significant these correctness problems will be. With that in mind, it
> would be useful to have avoid-libgmp options with our own GMP replacement that
> uses uintmax_t limbs (or 'unsigned int' limbs, for that matter) for debugging
> purposes, to help us debug the inevitable glitches that will arise due to
> libgmp's limbs being wrong for some types.

Again, this is unrelated to the API problem.

    AndyM

Re: bignum branch

[Paul Eggert]

Andy Moreton wrote:
> The size of a GMP limb is irrelevant - it is the API for getting native
> integer types into mpz_t values that cannot handle interger types wider
> than long.
> 
> Wider mp_limb_t types improve efficiency, but do nothing about
> correctness on LLP64 systems.

You're right, of course. It would be helpful to have that two-line change 
anyway, for performance reasons on 32-bit systems; however, Eli seems to be 
thinking that we shouldn't worry much about performance on these obsolescent 
systems and I'm becoming more inclined to agree.

Re: bignum branch

[Andy Moreton]

On Sun 15 Jul 2018, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sat, 14 Jul 2018 21:04:38 +0100
>> 
>> The problem is that if EMACS_INT is "long long", then the calls to
>> mpz_*_si() API routines will truncate any values passed to GMP. This
>> means that emacs cannot use any the mpz_*_si() routines directly.
>
> Right, I see what you mean now: all those APIs accept 'long' or
> 'unsigned long' as the integral type.  So this is a limitation of GMP
> as of now.  This message:
>
>   https://gmplib.org/list-archives/gmp-discuss/2016-March/005966.html
>
> indicates that GMP developers plan to address this in version 7, but
> the current master branch of GMP is still on version 6, so I guess it
> will be a while.

Indeed - I don't see this being addressed anytime soon.

> Btw, I had a cursory look at the GMP sources, and it seemed to me that
> changing GMP to lift this limitation should not be too hard.  The
> patch shown in this message:
>
>   https://gmplib.org/list-archives/gmp-discuss/2016-March/005965.html
>
> seems to confirm that.  So maybe someone could build GMP with MinGW64
> after applying that patch, and if it works, submit the patches to
> MSYS2 guys so that they could release a fixed library.  Then Emacs
> won't need to jump through these hoops on 64-bit Windows systems.

A quick look at that patch shows that it does not address all of the
issues. It fixes problems with large bitwidth numbers, but does not
handle other problems where the code assumes that long is not smaller
than mp_limb_t.

I think we will need to work around this in emacs until a better
upstream solution is available.

    AndyM

Re: bignum branch

[Charles A. Roelli]

> From: Tom Tromey <tom@tromey.com>
> Date: Thu, 12 Jul 2018 22:26:38 -0600
> 
> I've pushed the bignum branch to emacs git, as feature/bignum.
> 
> Please read through it and try it out, and reply to this message with
> your comments.
> 
> thanks,
> Tom

Thanks.  It now works on macOS too (with GMP 6.1.1 installed), after
replacing the old macros used in NS-related files.

However, when building without GMP installed, I get this:

  CC       alloc.o
alloc.c: In function ‘make_number’:
alloc.c:3833: error: ‘GMP_NUMB_BITS’ undeclared (first use in this function)
alloc.c:3833: error: (Each undeclared identifier is reported only once
alloc.c:3833: error: for each function it appears in.)

Where should that symbol be defined?

Re: bignum branch

[Andy Moreton]

On Thu 09 Aug 2018, Charles A. Roelli wrote:

>> From: Tom Tromey <tom@tromey.com>
>> Date: Thu, 12 Jul 2018 22:26:38 -0600
>> 
>> I've pushed the bignum branch to emacs git, as feature/bignum.
>> 
>> Please read through it and try it out, and reply to this message with
>> your comments.
>> 
>> thanks,
>> Tom
>
> Thanks.  It now works on macOS too (with GMP 6.1.1 installed), after
> replacing the old macros used in NS-related files.
>
> However, when building without GMP installed, I get this:
>
>   CC       alloc.o
> alloc.c: In function ‘make_number’:
> alloc.c:3833: error: ‘GMP_NUMB_BITS’ undeclared (first use in this function)
> alloc.c:3833: error: (Each undeclared identifier is reported only once
> alloc.c:3833: error: for each function it appears in.)
>
> Where should that symbol be defined?

It's defined in gmp.h from the GMP library. If you are building without
GMP installed, then emacs uses an internal copy of the GMP library's
simplified version from src/mini-gmp.[ch].

It appears that mini-gmp does not define GMP_NUMB_BITS, so does this
patch work for you instead ?

Thanks for testing,

    AndyM

diff --git a/src/alloc.c b/src/alloc.c
index 1504d7912b..a8bc55beb4 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -3830,7 +3830,7 @@ make_number (mpz_t value)
           for (i = 0; i < limbs; i++)
             {
               mp_limb_t limb = mpz_getlimbn (value, i);
-              v |= (EMACS_INT) ((EMACS_UINT) limb << (i * GMP_NUMB_BITS));
+              v |= (EMACS_INT) ((EMACS_UINT) limb << (i * mp_bits_per_limb));
             }
           if (sign < 0)
             v = -v;

Re: bignum branch

[Charles A. Roelli]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Thu, 09 Aug 2018 16:17:18 +0100
> 
> > Where should that symbol be defined?
> 
> It's defined in gmp.h from the GMP library. If you are building without
> GMP installed, then emacs uses an internal copy of the GMP library's
> simplified version from src/mini-gmp.[ch].
> 
> It appears that mini-gmp does not define GMP_NUMB_BITS, so does this
> patch work for you instead ?
> 
> Thanks for testing,
> 
>     AndyM
> 
> diff --git a/src/alloc.c b/src/alloc.c
> index 1504d7912b..a8bc55beb4 100644
> --- a/src/alloc.c
> +++ b/src/alloc.c
> @@ -3830,7 +3830,7 @@ make_number (mpz_t value)
>            for (i = 0; i < limbs; i++)
>              {
>                mp_limb_t limb = mpz_getlimbn (value, i);
> -              v |= (EMACS_INT) ((EMACS_UINT) limb << (i * GMP_NUMB_BITS));
> +              v |= (EMACS_INT) ((EMACS_UINT) limb << (i * mp_bits_per_limb));
>              }
>            if (sign < 0)
>              v = -v;

Thanks, Emacs now builds and runs with this patch installed locally.
I also ran "make check" in this configuration (with GMP uninstalled)
and it seems that none of the bignum-related tests have failed.

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> It appears that mini-gmp does not define GMP_NUMB_BITS, so does this
Andy> patch work for you instead ?

Let me know if you want me to push this to the branch for you.

Tom

Re: bignum branch

[Andy Moreton]

On Thu 09 Aug 2018, Tom Tromey wrote:

>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>
> Andy> It appears that mini-gmp does not define GMP_NUMB_BITS, so does this
> Andy> patch work for you instead ?
>
> Let me know if you want me to push this to the branch for you.

Yes please.

As a minor speedup, you could also change various places that use
"mpz_cmp_si (foo, 0) >= 0" to use "mpz_sgn (foo) >= 0".

    AndyM

Re: bignum branch

[Tom Tromey]

>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:

Andy> On Thu 09 Aug 2018, Tom Tromey wrote:
>>>>>>> "Andy" == Andy Moreton <andrewjmoreton@gmail.com> writes:
>> 
Andy> It appears that mini-gmp does not define GMP_NUMB_BITS, so does this
Andy> patch work for you instead ?
>> 
>> Let me know if you want me to push this to the branch for you.

Andy> Yes please.

Andy> As a minor speedup, you could also change various places that use
Andy> "mpz_cmp_si (foo, 0) >= 0" to use "mpz_sgn (foo) >= 0".

I've pushed your patch and the one you suggested here.  I re-ran the
test suite first.

Thanks for all your help on this project.

Tom