bug#46722: test-map-into fails

bug#46722: test-map-into fails

[Glenn Morris]

Package: emacs
Version: 28.0.50

Something between 8d5dfaf and efe42c2 (inclusive) causes test-map-into
to reliably fail on hydra.nixos.org. We can't be more specific since
intermediate revisions don't build. So far I could not reproduce the issue.

Ref eg https://hydra.nixos.org/build/137474120

(cl-no-applicable-method map-into ((a . 1) (b . 2)) (hash-table :test equal))

bug#46722: test-map-into fails

[Basil L. Contovounesios]

Glenn Morris <rgm@gnu.org> writes:

> Something between 8d5dfaf and efe42c2 (inclusive) causes test-map-into
> to reliably fail on hydra.nixos.org. We can't be more specific since
> intermediate revisions don't build. So far I could not reproduce the issue.
>
> Ref eg https://hydra.nixos.org/build/137474120
>
> (cl-no-applicable-method map-into ((a . 1) (b . 2)) (hash-table :test equal))

FWIW, I've been working on map.el and map-tests.el since yesterday, and
I occasionally run into this heisenerror locally maybe once in every 50
runs of 'make test/map-tests RET'.

-- 
Basil

bug#46722: test-map-into fails

[Glenn Morris]

Thanks. Locally I got 3/100 such failures.

(I wish the Emacs test suite was less flaky.
Eg in https://hydra.nixos.org/build/137475263
we have dired-test-bug27243-02 failing as well, apropos of nothing.)

bug#46722: test-map-into fails

[Stefan Monnier]

>> Something between 8d5dfaf and efe42c2 (inclusive) causes test-map-into
>> to reliably fail on hydra.nixos.org. We can't be more specific since
>> intermediate revisions don't build. So far I could not reproduce the issue.
>>
>> Ref eg https://hydra.nixos.org/build/137474120
>>
>> (cl-no-applicable-method map-into ((a . 1) (b . 2)) (hash-table :test equal))
>
> FWIW, I've been working on map.el and map-tests.el since yesterday, and
> I occasionally run into this heisenerror locally maybe once in every 50
> runs of 'make test/map-tests RET'.

This kind of code is supposed to be fully deterministic, so it's quite
worrisome.  It sounds/smells like memory corruption or some other bug in
the memory management.

If you could come up with a reasonably simple code (in the sense that it
doesn't take too long to run) to reproduce this problem with a high
probability, maybe we can start adding some tracing to try and figure
out what's going on.

IOW, try and write a loop that runs the problematic code a couple
hundred times, so that one of those should fail.  Then try and cut down
the code as much as possible, verifying along the way that the problem
didn't disappear.


        Stefan

bug#46722: test-map-into fails

[Basil L. Contovounesios]

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

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

> This kind of code is supposed to be fully deterministic, so it's quite
> worrisome.  It sounds/smells like memory corruption or some other bug in
> the memory management.
>
> If you could come up with a reasonably simple code (in the sense that it
> doesn't take too long to run) to reproduce this problem with a high
> probability, maybe we can start adding some tracing to try and figure
> out what's going on.
>
> IOW, try and write a loop that runs the problematic code a couple
> hundred times, so that one of those should fail.  Then try and cut down
> the code as much as possible, verifying along the way that the problem
> didn't disappear.

Here's the simplest repro I could find:

  ;;; foo.el -*- lexical-binding: t -*-
  (require 'map)
  (map-into () '(hash-table))
  ;;; foo.el ends here

followed by:

  emacs -Q -batch -f batch-byte-compile foo.el
  while emacs -Q -batch -l foo.elc; do :; done

which barfs the attached after a few seconds.

The same happens with:

  (map-into () '(hash-table :test eq))
  (map-into () '(hash-table :test eql))
  (map-into () '(hash-table :test equal))

but not with:

  (map-into () 'hash-table)

so maybe the heisenbug lies with cl-defmethod's treatment of 'head'?

Let me know if you'd like me to run anything else.

Thanks,

-- 
Basil


[-- Attachment #2: backtrace.el --]
[-- Type: application/emacs-lisp, Size: 4890 bytes --]

bug#46722: test-map-into fails

[Basil L. Contovounesios]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Here's the simplest repro I could find:
>
>   ;;; foo.el -*- lexical-binding: t -*-
>   (require 'map)
>   (map-into () '(hash-table))
>   ;;; foo.el ends here
>
> followed by:
>
>   emacs -Q -batch -f batch-byte-compile foo.el
>   while emacs -Q -batch -l foo.elc; do :; done
>
> which barfs the attached after a few seconds.
>
> The same happens with:
>
>   (map-into () '(hash-table :test eq))
>   (map-into () '(hash-table :test eql))
>   (map-into () '(hash-table :test equal))
>
> but not with:
>
>   (map-into () 'hash-table)
>
> so maybe the heisenbug lies with cl-defmethod's treatment of 'head'?

For some reason looping in Lisp doesn't fail though:

  ;;; foo.el -*- lexical-binding: t -*-
  (require 'map)
  (while (map-into () '(hash-table)))
  ;;; foo.el ends here

when run as:

  emacs -Q -batch -f batch-byte-compile foo.el
  emacs -Q -batch -l foo.elc

I actually forgot I left this running in the background for over an
hour because compilation-finish-functions never ran.

The same happens if I change '-batch -l' to '-script'.

-- 
Basil

bug#46722: test-map-into fails

[Glenn Morris]

I've also now seen this failure on the emacs-27 branch, so my initial
comments about revisions are incorrect.

bug#46722: test-map-into fails

[Stefan Kangas]

found 46722 29.0.50
thanks

Glenn Morris <rgm@gnu.org> writes:

> Package: emacs
> Version: 28.0.50
>
> Something between 8d5dfaf and efe42c2 (inclusive) causes test-map-into
> to reliably fail on hydra.nixos.org. We can't be more specific since
> intermediate revisions don't build. So far I could not reproduce the issue.
>
> Ref eg https://hydra.nixos.org/build/137474120
>
> (cl-no-applicable-method map-into ((a . 1) (b . 2)) (hash-table :test equal))

Here's a similar intermittent failure.  I got it once (this morning),
while running the automatic merges:

Test test-map-merge backtrace:
  signal(cl-no-applicable-method (map-into (a 1) (hash-table :test equ
  apply(signal (cl-no-applicable-method (map-into (a 1) (hash-table :t
  #f(compiled-function () #<bytecode -0x1be1af7ee09701a0>)()
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name test-map-merge :documentation "Test `
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emacs-lisp/map-tests
  command-line()
  normal-top-level()
Test test-map-merge condition:
    (cl-no-applicable-method map-into
                             (a 1)
                             (hash-table :test equal))
   FAILED  28/51  test-map-merge (0.000101 sec)
Test test-map-merge-empty backtrace:
  signal(cl-no-applicable-method (map-into nil (hash-table :test equal
  cl-no-applicable-method(#s(cl--generic :name map-into :dispatches ((
  apply(cl-no-applicable-method #s(cl--generic :name map-into :dispatc
  #f(compiled-function (&rest args) #<bytecode 0x9923a443d6f3319>)(nil
  apply(#f(compiled-function (&rest args) #<bytecode 0x9923a443d6f3319
  map-into(nil (hash-table :test equal))
  map--merge(#f(compiled-function (result key value) #<bytecode 0x6e5c
  apply(map--merge #f(compiled-function (result key value) #<bytecode
  map-merge(list)
  apply(map-merge list)
  #f(compiled-function () #<bytecode -0x15c1e766ae726ce0>)()
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name test-map-merge-empty :documentation "
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emacs-lisp/map-tests
  command-line()
  normal-top-level()
Test test-map-merge-empty condition:
    (cl-no-applicable-method map-into nil
                             (hash-table :test equal))
   FAILED  29/51  test-map-merge-empty (0.000099 sec)
Test test-map-merge-with backtrace:
  signal(cl-no-applicable-method (map-into ((1 . 2)) (hash-table :test
  apply(signal (cl-no-applicable-method (map-into ((1 . 2)) (hash-tabl
  #f(compiled-function () #<bytecode -0xace723ac53d8a75>)()
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name test-map-merge-with :documentation ni
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emacs-lisp/map-tests
  command-line()
  normal-top-level()
Test test-map-merge-with condition:
    (cl-no-applicable-method map-into
                             ((1 . 2))
                             (hash-table :test equal))
   FAILED  30/51  test-map-merge-with (0.000090 sec)

bug#46722: test-map-into fails

[Glenn Morris]

It occurred for 8 builds in a row from
https://hydra.nixos.org/build/159053152
to
https://hydra.nixos.org/build/159054038

bug#46722: test-map-into fails

[Lars Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> It occurred for 8 builds in a row from
> https://hydra.nixos.org/build/159053152
> to
> https://hydra.nixos.org/build/159054038

Hm...  very odd:

Test test-map-into backtrace:
  signal(cl-no-applicable-method (map-into ((a . 1) (b . 2)) (hash-table :test equal)))
  cl-no-applicable-method(#s(cl--generic :name map-into :dispatches ((1 #s(cl--generic-generalizer :name cl--generic-eql-generalizer :priority 100 :ta
  apply(cl-no-applicable-method #s(cl--generic :name map-into :dispatches ((1 #s(cl--generic-generalizer :name cl--generic-eql-generalizer :priority 1
  #f(compiled-function (&rest args) #<bytecode -0xd5991ab961a5f21>)(((a . 1) (b . 2)) (hash-table :test equal))
  apply(#f(compiled-function (&rest args) #<bytecode -0xd5991ab961a5f21>) ((a . 1) (b . 2)) (hash-table :test equal) nil)
  map-into(((a . 1) (b . 2)) (hash-table :test equal))

Is there anything particular with the Emacs build on Hydra?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46722: test-map-into fails

[Glenn Morris]

Lars Ingebrigtsen wrote:

> Is there anything particular with the Emacs build on Hydra?

Not really. It's a Nix build that anyone can click the button to
reproduce, with some set-up effort. (Actions -> Reproduce locally).

(And 3 people have in this report said they sometimes see the same
problems on non-Hydra systems.)

bug#46722: test-map-into fails

[dick]

You can't do,

(funcall
 (funcall
  (backquote
   (lambda (arg)
     (lambda () arg)))
  t))

which is what `cl--generic-get-dispatcher` tries to do.  You need an
apply-partially somewhere in there.

Also in same function, `method-cache` buys you nothing, so getting rid of
it along with `with-memoization` and `(require subr-x)` is a not
insubstantial win.

Fixing may or may not put the kibosh on the heisen-flap.

bug#46722: test-map-into fails

[dick]

For unclear reasons, the hash-table `cl--generic-dispatchers` keyed off
raw, unburnished defstructs returns values for non-existent keys.

I normalized its key to a signature string, and,

for i in {1..300} ; do src/emacs -Q -batch -f batch-byte-compile foo.el
; src/emacs -Q -batch -l foo.elc ;  done ;

where foo.el is (map-into nil '(hash-table)) ran without incident.

bug#46722: test-map-into fails

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Dick,

dick [2022-01-05 09:01:29] wrote:
> For unclear reasons, the hash-table `cl--generic-dispatchers` keyed off
> raw, unburnished defstructs returns values for non-existent keys.

Hmm... I don't understand what this means concretely.
I'm aware that `cl--generic-dispatchers` can be fiddly (IIRC because
there can be functions in the key), but I still can't figure out what
scenario you're describing nor how it can happen to return values for
non-existent keys.

Could you say a bit more, ideally with something like a trace or
a test case?

[...time passes...]

> I normalized its key to a signature string, and,
>
> for i in {1..300} ; do src/emacs -Q -batch -f batch-byte-compile foo.el
> ; src/emacs -Q -batch -l foo.elc ;  done ;

Oh I think I see what you mean.  I can now reproduce the problem locally
as well, and with the foo.el below I get two different traces:

    (trace-function 'cl--generic-get-dispatcher)
    
    (require 'map)
    (condition-case nil
        (progn
          (map-into nil '(hash-table))
          (with-current-buffer "*trace-output*"
            (write-region nil nil "~/tmp/trace-good.eld")))
      (error
       (with-current-buffer "*trace-output*"
         (write-region nil nil "~/tmp/trace-bad.eld")
         (message "ERROR!!"))))

the two backtraces only differ on the last line.  The good trace
(slightly cleaned up for legibility) ends with:

======================================================================
1 -> (cl--generic-get-dispatcher
      (1 #s(cl--generic-generalizer cl--generic-eql-generalizer 100
            #[385 "<bytecode>" [gethash (cl--generic-eql-used)] 5]
            #[385 "<bytecode>" [eql] 4])
         #s(cl--generic-generalizer cl--generic-t-generalizer 0
            #[385 #2="\300\207" [nil] 3 #1#]
            #[385 #2# [(t)] 3])))
1 <- cl--generic-get-dispatcher:
     #[771 "<bytecode>" [make-hash-table :test eql make-closure
                         #[642 "<bytecode>"
                           [V0 V1 V2 V3 cl--generic-eql-used apply gethash
                            cl--generic-cache-miss 1 append
                            #[385 "<bytecode>" [eql] 4]
                            #[385 "\300\207" [(t)] 3]
                            nil puthash]
                           15]]
                        10]
======================================================================
1 -> (cl--generic-get-dispatcher
      (1 #s(cl--generic-generalizer cl--generic-eql-generalizer 100
            #[385 "<bytecode>" [gethash (cl--generic-eql-used)] 5]
            #[385 "<bytecode>" [eql] 4])
         #s(cl--generic-generalizer cl--generic-head-generalizer 80
            #[385 "<bytecode>" [gethash car-safe (cl--generic-head-used)] 5]
            #[385 "<bytecode>" [head] 4])
         #s(cl--generic-generalizer cl--generic-t-generalizer 0
            #[385 #3="\300\207" [nil] 3]
            #[385 #3# [(t)] 3])))
1 <- cl--generic-get-dispatcher:
     #[771 "<bytecode>" [make-hash-table :test eql make-closure
                         #[642 "<bytecode>"
                           [V0 V1 V2 V3 cl--generic-eql-used
                            cl--generic-head-used apply gethash
                            cl--generic-cache-miss 1 append
                            #[385 "<bytecode>" [eql] 4]
                            #[385 "<bytecode>" [head] 4]
                            #[385 "\300\207" [(t)] 3]
                            nil puthash]
                           16]]
                        10]

whereas in the bad trace, the last returned value is identical to the
previous one (i.e. somehow the `head` dispatcher is absent from the
returned function).

Hmm....


        Stefan

bug#46722: test-map-into fails

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

I think I tracked it down.

The patch below should fix it.  Boy do I hate side effects!
Could others confirm that it fixes the problem for them as well?

Eli, do you think this is appropriate for `emacs-28`?
OT1H this bug probably has been with us since Emacs-25, but OTOH the
patch should be very safe.


        Stefan


PS: We don't need the `copy-sequence` when we do the actual `gethash`,
we only need it if that `gethash` returns nil and we end up putting
a new entry in the hash-table, but avoiding those extra `copy-sequence`
requires manually macroexpanding `with-memoization`, so I favored
keeping the patch as short as possible.


diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
index 983694204df..5090e060372 100644
--- a/lisp/emacs-lisp/cl-generic.el
+++ b/lisp/emacs-lisp/cl-generic.el
@@ -604,7 +604,9 @@ cl--generic-dispatchers
 
 (defun cl--generic-get-dispatcher (dispatch)
   (with-memoization
-      (gethash dispatch cl--generic-dispatchers)
+      ;; We need `copy-sequence` here because this `dispatch' object might be
+      ;; modified by side-effect in `cl-generic-define-method' (bug#46722).
+      (gethash (copy-sequence dispatch) cl--generic-dispatchers)
     ;; (message "cl--generic-get-dispatcher (%S)" dispatch)
     (let* ((dispatch-arg (car dispatch))
            (generalizers (cdr dispatch))

bug#46722: test-map-into fails

[Eli Zaretskii]

> Cc: contovob@tcd.ie, Glenn Morris <rgm@gnu.org>,
>  Lars Ingebrigtsen <larsi@gnus.org>, Stefan Kangas <stefan@marxist.se>,
>  46722@debbugs.gnu.org
> Date: Wed, 05 Jan 2022 12:51:09 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I think I tracked it down.
> 
> The patch below should fix it.  Boy do I hate side effects!
> Could others confirm that it fixes the problem for them as well?
> 
> Eli, do you think this is appropriate for `emacs-28`?
> OT1H this bug probably has been with us since Emacs-25, but OTOH the
> patch should be very safe.

Is it? how so?  copy-sequence is not a trivial operation, at least not
with arbitrary data structures.  And we nowadays use generics a lot,
so this could potentially affect gobs of code.  So I'd rather we
didn't install this on the release branch.

bug#46722: test-map-into fails

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Is it? how so?  copy-sequence is not a trivial operation, at least not
> with arbitrary data structures.

This is used on the `key` argument of `gethash/puthash` on a hash-table
defined to use `:test #'equal`.  And it is used on a value which is
defined to be a list.

For this reason, I think it is very safe.

The reason why the current code only misfires "rarely" is that in most
cases, when we change by side-effect an object that's a key in an
`equal` hashtable, the result is a hashtable where this entry can't be
found (because its hash doesn't match any more), so we end up
luckily not making use of this (now broken) entry.

So the underlying bug only appears if either the hash-table is rehashed
or the new `sxhash-equal` value happens to the same as the old one.

> And we nowadays use generics a lot, so this could potentially affect
> gobs of code.  So I'd rather we didn't install this on the
> release branch.

Fine by me,


        Stefan

bug#46722: test-map-into fails

[Philipp Stephani]

Am Mi., 5. Jan. 2022 um 19:25 Uhr schrieb Stefan Monnier via Bug
reports for GNU Emacs, the Swiss army knife of text editors
<bug-gnu-emacs@gnu.org>:
> > And we nowadays use generics a lot, so this could potentially affect
> > gobs of code.  So I'd rather we didn't install this on the
> > release branch.
>
> Fine by me,

Should the affected tests (these map tests, but I think I have a
similar case with the bindat tests) be marked as unstable in the
meantime?

bug#46722: test-map-into fails

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philipp Stephani [2022-01-13 20:23:28] wrote:
> Am Mi., 5. Jan. 2022 um 19:25 Uhr schrieb Stefan Monnier via Bug
> reports for GNU Emacs, the Swiss army knife of text editors
> <bug-gnu-emacs@gnu.org>:
>> > And we nowadays use generics a lot, so this could potentially affect
>> > gobs of code.  So I'd rather we didn't install this on the
>> > release branch.
>> Fine by me,
> Should the affected tests (these map tests, but I think I have a
> similar case with the bindat tests) be marked as unstable in the
> meantime?

By "in the meantime" I assume you mean something like "on the release
branch", right (since the problem is supposedly fixed in `master`)?


        Stefan

bug#46722: test-map-into fails

[Philipp Stephani]

Am Do., 13. Jan. 2022 um 21:40 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> Philipp Stephani [2022-01-13 20:23:28] wrote:
> > Am Mi., 5. Jan. 2022 um 19:25 Uhr schrieb Stefan Monnier via Bug
> > reports for GNU Emacs, the Swiss army knife of text editors
> > <bug-gnu-emacs@gnu.org>:
> >> > And we nowadays use generics a lot, so this could potentially affect
> >> > gobs of code.  So I'd rather we didn't install this on the
> >> > release branch.
> >> Fine by me,
> > Should the affected tests (these map tests, but I think I have a
> > similar case with the bindat tests) be marked as unstable in the
> > meantime?
>
> By "in the meantime" I assume you mean something like "on the release
> branch", right (since the problem is supposedly fixed in `master`)?
>

Yes

bug#46722: test-map-into fails

[Lars Ingebrigtsen]

Philipp Stephani <p.stephani2@gmail.com> writes:

>> By "in the meantime" I assume you mean something like "on the release
>> branch", right (since the problem is supposedly fixed in `master`)?
>
> Yes

Now done.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46722: test-map-into fails

[Philipp Stephani]

Am Fr., 14. Jan. 2022 um 08:40 Uhr schrieb Lars Ingebrigtsen <larsi@gnus.org>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> >> By "in the meantime" I assume you mean something like "on the release
> >> branch", right (since the problem is supposedly fixed in `master`)?
> >
> > Yes
>
> Now done.
>

Thanks. In my case I also got flaky results for test-map-merge-empty,
test-map-into-hash-test, test-map-merge-with, and test-map-merge, as
well as the bindat tests (they already fail at
loading/byte-compilation time with (cl-no-applicable-method
bindat--type unpack (dest-ip ip) (src-ip ip) (dest-port uint 16)
(src-port uint 16))); any objection to marking them (or even all
affected tests) as unstable, too?

bug#46722: test-map-into fails

[Lars Ingebrigtsen]

Philipp Stephani <p.stephani2@gmail.com> writes:

> Thanks. In my case I also got flaky results for test-map-merge-empty,
> test-map-into-hash-test, test-map-merge-with, and test-map-merge, as
> well as the bindat tests (they already fail at
> loading/byte-compilation time with (cl-no-applicable-method
> bindat--type unpack (dest-ip ip) (src-ip ip) (dest-port uint 16)
> (src-port uint 16))); any objection to marking them (or even all
> affected tests) as unstable, too?

Marking the other test-map* functions as unstable (on the release
branch) should be fine.

Are the bindat tests also failing because of the map problems, or is
that unrelated? 

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46722: test-map-into fails

[Philipp Stephani]

Am Sa., 15. Jan. 2022 um 09:01 Uhr schrieb Lars Ingebrigtsen <larsi@gnus.org>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> > Thanks. In my case I also got flaky results for test-map-merge-empty,
> > test-map-into-hash-test, test-map-merge-with, and test-map-merge, as
> > well as the bindat tests (they already fail at
> > loading/byte-compilation time with (cl-no-applicable-method
> > bindat--type unpack (dest-ip ip) (src-ip ip) (dest-port uint 16)
> > (src-port uint 16))); any objection to marking them (or even all
> > affected tests) as unstable, too?
>
> Marking the other test-map* functions as unstable (on the release
> branch) should be fine.

OK, I've now done that with commit
2dcb1bc47485791177917bfbd8fba95d69454135. At least in my experiments
only the four ones that I marked are unstable.

>
> Are the bindat tests also failing because of the map problems, or is
> that unrelated?

It's at least the same symptom ("No applicable method", but only
sometimes), so I guess it's also the same root cause (the cl-generic
cache thingy discussed in this thread). In this case the problem
already appears when expanding the bindat-type macro (i.e. when
loading or byte-compiling the file), so we'd need to exclude
bindat-tests from the test suite altogether on the release branch.

bug#46722: test-map-into fails

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

FWIW, I think installing the cl-generic patch into `emacs-28` would be
a much better option.  In my view it's "super safe" and it fixes
a nasty bug that can affect a lot of code (as we're witnessing here
with those test problems).

It admittedly doesn't fix a regression.

For reference the patch is just:

    diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
    index 983694204df..5090e060372 100644
    --- a/lisp/emacs-lisp/cl-generic.el
    +++ b/lisp/emacs-lisp/cl-generic.el
    @@ -604,7 +604,9 @@ cl--generic-dispatchers
     
     (defun cl--generic-get-dispatcher (dispatch)
       (with-memoization
    -      (gethash dispatch cl--generic-dispatchers)
    +      ;; We need `copy-sequence` here because this `dispatch' object might be
    +      ;; modified by side-effect in `cl-generic-define-method' (bug#46722).
    +      (gethash (copy-sequence dispatch) cl--generic-dispatchers)
         ;; (message "cl--generic-get-dispatcher (%S)" dispatch)
         (let* ((dispatch-arg (car dispatch))
                (generalizers (cdr dispatch))

It's already the second time I say it, so I promise it's the last time
I insist here.  I can live with either choice, but I'm sure Emacs-28 will
be better off with the patch than without.


        Stefan

bug#46722: test-map-into fails

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  Eli Zaretskii <eliz@gnu.org>,
>   Glenn Morris <rgm@gnu.org>,  Stefan Kangas <stefan@marxist.se>,  "Basil
>  L. Contovounesios" <contovob@tcd.ie>,  dick.r.chiang@gmail.com,
>   46722@debbugs.gnu.org
> Date: Sat, 15 Jan 2022 10:16:48 -0500
> 
> It's already the second time I say it, so I promise it's the last time
> I insist here.  I can live with either choice, but I'm sure Emacs-28 will
> be better off with the patch than without.

My opinion is up-thread, so I guess it's up to Lars now.

bug#46722: test-map-into fails

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> It's already the second time I say it, so I promise it's the last time
>> I insist here.  I can live with either choice, but I'm sure Emacs-28 will
>> be better off with the patch than without.
>
> My opinion is up-thread, so I guess it's up to Lars now.

I agree with Eli.

And since this is fixed in Emacs 29 (and the tests marked in Emacs 28),
I'm therefore closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46722: test-map-into fails

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> You can't do,
>
> (funcall
>  (funcall
>   (backquote
>    (lambda (arg)
>      (lambda () arg)))
>   t))
>
> which is what `cl--generic-get-dispatcher` tries to do.

Is it?  Where does `cl--generic-get-dispatcher` do that?

Oh, you mean we don't guarantee that we use static scoping?
I think I see what mean.
Does the patch below help?


        Stefan


diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
index 983694204df..686a2375971 100644
--- a/lisp/emacs-lisp/cl-generic.el
+++ b/lisp/emacs-lisp/cl-generic.el
@@ -644,7 +644,8 @@ cl--generic-get-dispatcher
       ;; FIXME: For generic functions with a single method (or with 2 methods,
       ;; one of which always matches), using a tagcode + hash-table is
       ;; overkill: better just use a `cl-typep' test.
-      (byte-compile
+      (funcall
+       (lambda (exp) (let ((lexical-binding t)) (byte-compile exp)))
        `(lambda (generic dispatches-left methods)
           ;; FIXME: We should find a way to expand `with-memoize' once
           ;; and forall so we don't need `subr-x' when we get here.

bug#46722: test-map-into fails

[dick]

> Does the patch below help?

I don't see why it would if this doesn't work:

(let ((lexical-binding t))
  (funcall
   (funcall
    (backquote
     (lambda (arg)
       (lambda () arg)))
    t)))

But clearly `byte-compile` allows things that the interpreter doesn't,
otherwise it'd never have worked for so long.

While I remain leery of the fast and loose scoping here, it was but a red
herring.  The problem lies with the hashing behavior of
`cl--generic-dispatchers`.

bug#46722: test-map-into fails

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Does the patch below help?

No, it doesn't because I see now that we already fixed this bug by
binding `lexical-binding` (just a bit further above).

> I don't see why it would if this doesn't work:
>
> (let ((lexical-binding t))
>   (funcall
>    (funcall
>     (backquote
>      (lambda (arg)
>        (lambda () arg)))
>     t)))

The effect of `lexical-binding` is different from what you think.
It applies to the moment a sexp passes from being "data" to the moment
it is labeled as "code", not to the moment the code is executed
(otherwise we couldn't compile the code of functions ahead of time, not
knowing which value of `lexical-binding` would be active when the code
will be executed).

> While I remain leery of the fast and loose scoping here, it was but a red
> herring.

Indeed,


        Stefan

bug#46722: test-map-into fails

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Also in same function, `method-cache` buys you nothing, so getting rid of
> it along with `with-memoization` and `(require subr-x)` is a not
> insubstantial win.

The benefit of the `cl--generic-dispatchers` cache is probably
negligible in terms of performance, indeed.  Its main purpose is to try
and avoid loading the byte-compiler (e.g. most crucially, it saves us
from *pre*loading the byte-compiler).

The benefit of the `method-cache` OTOH should be very significant since
without it we'd have to recompute the effective method at every call.
This said, I have not measured it.


        Stefan

bug#46722: test-map-into fails

[dick]

> So the underlying bug only appears if either the hash-table is rehashed
> or the new `sxhash-equal` value happens to the same as the old one.

I can't say I understand this, but I'm contented if map-tests.el stops flapping.

> The benefit of the `method-cache` OTOH should be very significant
> since without it we'd have to recompute the effective method at every
> call.

I don't get it.  This assumes `method-cache` persists between successive
calls to the byte-compiled closure... but `method-cache` is clearly let'ted.

bug#46722: test-map-into fails

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> So the underlying bug only appears if either the hash-table is rehashed
>> or the new `sxhash-equal` value happens to the same as the old one.
> I can't say I understand this, but I'm contented if map-tests.el stops flapping.

Basically, here's what happens:

On one call, `dispatch` is of the form (1 #<eq-generalizer> #<t-generalizer>)
because the arg n°1 has methods that use `eql` and `t`.
So an entry for it is placed in bucket 7 of the hash-table (say) with
the corresponding "eq + t dispatch function".

Now, this very `dispatch` object is also stored in the generic
function metadata.  And later on, some new `cl-defmethod` is executed
which means the arg n°1 now also has methods that use `head`, so this
object is modified (by setcdr)
to (1 #<eq-generalizer> #<head-generalizer> #<t-generalizer>).
Since it's the very same object, it means that the hash table now has
a mapping from (1 #<eq-generalizer> #<head-generalizer> #<t-generalizer>)
to the "eq + t dispatch function", which is wrong and the core of problem.

Luckily for us, this problem has not bitten us very often because, the
hash of (1 #<eq-generalizer> #<head-generalizer> #<t-generalizer>) is
not the same as the hash of (1 #<eq-generalizer> #<t-generalizer>) so
our (now incorrect) entry (still placed in bucket 7) will simply never be
found by `gethash` because gethash will look for it in another bucket.

>> The benefit of the `method-cache` OTOH should be very significant
>> since without it we'd have to recompute the effective method at every
>> call.
>
> I don't get it.  This assumes `method-cache` persists between successive
> calls to the byte-compiled closure... but `method-cache` is clearly let'ted.

It does persist: the byte-compiled closure is a "dispatcher builder".
The function we store in the `symbol-function` slot of generic
functions, is the (lambda (,@fixedargs &rest args) ...) function
*returned* by this function.  This inner function has the method cache
in its captured environment which is why it persists between calls.


        Stefan