bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp]

As an example, edebug-instrument (C-u C-M-x) the following definition:

(defun bar ()
  (cl-flet ((foo () 1))
    (foo)))

The *Messages* buffer now says

Edebug: foo [2 times]
Edebug: bar

Note the '[2 times]'.  I believe this is because `edebug-match-&define'
calls `edebug-make-form-wrapper' unconditionally.  The Edebug spec for
`cl-flet' has two `&or' branches that both use `&define', so if the
first one doesn't match it will still create a definition using
`edebug-make-form-wrapper'.  Probably `edebug-match-&define' should only
invoke `edebug-make-form-wrapper' if the specification actually matches.


In GNU Emacs 28.0.50 (build 55, x86_64-apple-darwin19.4.0, NS appkit-1894.50 Version 10.15.5 (Build 19F101))
 of 2020-06-21
Repository revision: a4d3897d8f0caa54be1e1d081651ed6640b7f25e
Repository branch: master
Windowing system distributor 'Apple', version 10.3.1894
System Description:  Mac OS X 10.15.5

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
C-c C-c is undefined

Configured using:
 'configure --with-modules --without-xml2 --without-pop --with-mailutils
 --enable-gcc-warnings=warn-only --enable-checking=all
 --enable-check-lisp-object-type 'CFLAGS=-ggdb3 -O0''

Configured features:
JPEG TIFF GIF PNG NOTIFY KQUEUE ACL GNUTLS ZLIB TOOLKIT_SCROLL_BARS NS
MODULES THREADS JSON PDUMPER LCMS2 GMP

Important settings:
  value of $LANG: de_DE.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc dired dired-loaddefs rfc822
mml easymenu mml-sec epa epg epg-config gnus-util rmail rmail-loaddefs
text-property-search time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils phst skeleton derived edmacro
kmacro pcase ffap thingatpt url url-proxy url-privacy url-expand
url-methods url-history url-cookie url-domsuf url-util url-parse
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json map url-vars mailcap subr-x rx gnutls puny seq
byte-opt gv bytecomp byte-compile cconv dbus xml compile comint
ansi-color ring cl-loaddefs cl-lib tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/ns-win ns-win
ucs-normalize mule-util term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
timer select scroll-bar mouse jit-lock font-lock syntax facemenu
font-core term/tty-colors frame minibuffer cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads kqueue cocoa ns
lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 69651 5774)
 (symbols 48 8651 1)
 (strings 32 23551 1900)
 (string-bytes 1 768689)
 (vectors 16 14140)
 (vector-slots 8 172499 9631)
 (floats 8 25 30)
 (intervals 56 206 0)
 (buffers 992 10))

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Alan Mackenzie]

Hello, Philipp.

In article <mailman.222.1592758804.2574.bug-gnu-emacs@gnu.org> you wrote:

> As an example, edebug-instrument (C-u C-M-x) the following definition:

> (defun bar ()
>   (cl-flet ((foo () 1))
>     (foo)))

> The *Messages* buffer now says

> Edebug: foo [2 times]
> Edebug: bar

> Note the '[2 times]'.  I believe this is because `edebug-match-&define'
> calls `edebug-make-form-wrapper' unconditionally.  The Edebug spec for
> `cl-flet' has two `&or' branches that both use `&define', so if the
> first one doesn't match it will still create a definition using
> `edebug-make-form-wrapper'.  Probably `edebug-match-&define' should only
> invoke `edebug-make-form-wrapper' if the specification actually matches.

I don't understand why this is a bug.  What precisely is wrong with the
messages displayed in *Messages*?  Or is it something else which is
wrong?

After instrumenting bar, can you actually step through it with edebug?
(I can't try it out myself, since I can't discern from the documentation
what, precisely, cl-flet is supposed to do.)

> In GNU Emacs 28.0.50 (build 55, x86_64-apple-darwin19.4.0, NS appkit-1894.50 Version 10.15.5 (Build 19F101))
>  of 2020-06-21
> Repository revision: a4d3897d8f0caa54be1e1d081651ed6640b7f25e
> Repository branch: master
> Windowing system distributor 'Apple', version 10.3.1894
> System Description:  Mac OS X 10.15.5

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp Stephani]

Am Mo., 22. Juni 2020 um 01:48 Uhr schrieb Alan Mackenzie <acm@muc.de>:
>
> Hello, Philipp.
>
> In article <mailman.222.1592758804.2574.bug-gnu-emacs@gnu.org> you wrote:
>
> > As an example, edebug-instrument (C-u C-M-x) the following definition:
>
> > (defun bar ()
> >   (cl-flet ((foo () 1))
> >     (foo)))
>
> > The *Messages* buffer now says
>
> > Edebug: foo [2 times]
> > Edebug: bar
>
> > Note the '[2 times]'.  I believe this is because `edebug-match-&define'
> > calls `edebug-make-form-wrapper' unconditionally.  The Edebug spec for
> > `cl-flet' has two `&or' branches that both use `&define', so if the
> > first one doesn't match it will still create a definition using
> > `edebug-make-form-wrapper'.  Probably `edebug-match-&define' should only
> > invoke `edebug-make-form-wrapper' if the specification actually matches.
>
> I don't understand why this is a bug.  What precisely is wrong with the
> messages displayed in *Messages*?  Or is it something else which is
> wrong?
>
> After instrumenting bar, can you actually step through it with edebug?
> (I can't try it out myself, since I can't discern from the documentation
> what, precisely, cl-flet is supposed to do.)
>

So this is somewhat subtle, so let me try to give some context. The
message is merely a symptom of defining a symbol twice (via
edebug-make-form-wrapper). That's a problem when using Edebug for
coverage instrumentation (in batch mode), as the coverage information
is attached to properties of the symbol that Edebug
generates/instruments. Instrumenting a symbol with two different
definitions can lead to very subtle bugs because the frequency vector
and the form offset vector are out of sync, see e.g.
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853. Therefore it's
important to prevent such duplicate instrumentation, typically by
changing the Edebug symbol in some way (appending a unique suffix,
etc.). Edebug does this already in many cases (ERT tests, CL methods,
...), but not always.
For some more context, see the coverage instrumentation in my Bazel
rules for ELisp (https://github.com/phst/rules_elisp).
https://github.com/phst/rules_elisp/blob/master/elisp/ert/runner.el
contains the ERT and coverage integration. In
https://github.com/phst/rules_elisp/blob/0b24aa1660af2f6c668899bdd78aaba383d7ac18/elisp/ert/runner.el#L133-L134
I explicitly check for duplicate instrumentation. It is hard to
predict in general whether a specific instance of duplicate
instrumentation will lead to bugs like
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 or not, thus I'm
treating every duplicate instrumentation as a bug.

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Alan Mackenzie]

Hello, Philipp.

I must admit, I'm having difficulty understanding this problem.

On Sat, Aug 08, 2020 at 13:01:50 +0200, Philipp Stephani wrote:
> Am Mo., 22. Juni 2020 um 01:48 Uhr schrieb Alan Mackenzie <acm@muc.de>:

> > In article <mailman.222.1592758804.2574.bug-gnu-emacs@gnu.org> you wrote:

> > > As an example, edebug-instrument (C-u C-M-x) the following
> > > definition:

> > > (defun bar ()
> > >   (cl-flet ((foo () 1))
> > >     (foo)))

> > > The *Messages* buffer now says

> > > Edebug: foo [2 times]
> > > Edebug: bar

> > > Note the '[2 times]'.  I believe this is because
> > > `edebug-match-&define' calls `edebug-make-form-wrapper'
> > > unconditionally.  The Edebug spec for `cl-flet' has two `&or'
> > > branches that both use `&define', so if the first one doesn't match
> > > it will still create a definition using `edebug-make-form-wrapper'.
> > > Probably `edebug-match-&define' should only invoke
> > > `edebug-make-form-wrapper' if the specification actually matches.

> > I don't understand why this is a bug.  What precisely is wrong with
> > the messages displayed in *Messages*?  Or is it something else which
> > is wrong?

> > After instrumenting bar, can you actually step through it with
> > edebug?  (I can't try it out myself, since I can't discern from the
> > documentation what, precisely, cl-flet is supposed to do.)


> So this is somewhat subtle, so let me try to give some context. The
> message is merely a symptom of defining a symbol twice (via
> edebug-make-form-wrapper). That's a problem when using Edebug for
> coverage instrumentation (in batch mode), as the coverage information
> is attached to properties of the symbol that Edebug
> generates/instruments.

I'm trying to see what, exactly, this problem is.  Edebug is defining a
symbol twice, once for each of two arms of a &or form in the edebug spec.
The first of these surely does nothing; it will eventually end up in the
garbage collector.  The second will form the function slot of the symbol,
fulfilling all the Edebug things.  What am I missing?

> Instrumenting a symbol with two different definitions can lead to very
> subtle bugs because the frequency vector and the form offset vector are
> out of sync, ....

The picture you seem to be painting is of two distinct definitions being
assigned to the same symbol, and both of them being live.  Do you have
any evidence that this is happening?

> .... see e.g.  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853.
> Therefore it's important to prevent such duplicate instrumentation,
> typically by changing the Edebug symbol in some way (appending a unique
> suffix, etc.). Edebug does this already in many cases (ERT tests, CL
> methods, ...), but not always.  For some more context, see the coverage
> instrumentation in my Bazel rules for ELisp
> (https://github.com/phst/rules_elisp).
> https://github.com/phst/rules_elisp/blob/master/elisp/ert/runner.el
> contains the ERT and coverage integration. In
> https://github.com/phst/rules_elisp/blob/0b24aa1660af2f6c668899bdd78aaba383d7ac18/elisp/ert/runner.el#L133-L134
> I explicitly check for duplicate instrumentation. It is hard to predict
> in general whether a specific instance of duplicate instrumentation
> will lead to bugs like
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 or not, thus I'm
> treating every duplicate instrumentation as a bug.

What exactly do you mean by "duplicate instrumentation"?  If a symbol
gets defined twice, once for each arm of an &or in the edebug spec, does
that count as a duplicate instrumentation?

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp Stephani]

Am Sa., 8. Aug. 2020 um 16:59 Uhr schrieb Alan Mackenzie <acm@muc.de>:
>
> Hello, Philipp.
>
> I must admit, I'm having difficulty understanding this problem.
>
> On Sat, Aug 08, 2020 at 13:01:50 +0200, Philipp Stephani wrote:
> > Am Mo., 22. Juni 2020 um 01:48 Uhr schrieb Alan Mackenzie <acm@muc.de>:
>
> > > In article <mailman.222.1592758804.2574.bug-gnu-emacs@gnu.org> you wrote:
>
> > > > As an example, edebug-instrument (C-u C-M-x) the following
> > > > definition:
>
> > > > (defun bar ()
> > > >   (cl-flet ((foo () 1))
> > > >     (foo)))
>
> > > > The *Messages* buffer now says
>
> > > > Edebug: foo [2 times]
> > > > Edebug: bar
>
> > > > Note the '[2 times]'.  I believe this is because
> > > > `edebug-match-&define' calls `edebug-make-form-wrapper'
> > > > unconditionally.  The Edebug spec for `cl-flet' has two `&or'
> > > > branches that both use `&define', so if the first one doesn't match
> > > > it will still create a definition using `edebug-make-form-wrapper'.
> > > > Probably `edebug-match-&define' should only invoke
> > > > `edebug-make-form-wrapper' if the specification actually matches.
>
> > > I don't understand why this is a bug.  What precisely is wrong with
> > > the messages displayed in *Messages*?  Or is it something else which
> > > is wrong?
>
> > > After instrumenting bar, can you actually step through it with
> > > edebug?  (I can't try it out myself, since I can't discern from the
> > > documentation what, precisely, cl-flet is supposed to do.)
>
>
> > So this is somewhat subtle, so let me try to give some context. The
> > message is merely a symptom of defining a symbol twice (via
> > edebug-make-form-wrapper). That's a problem when using Edebug for
> > coverage instrumentation (in batch mode), as the coverage information
> > is attached to properties of the symbol that Edebug
> > generates/instruments.
>
> I'm trying to see what, exactly, this problem is.  Edebug is defining a
> symbol twice, once for each of two arms of a &or form in the edebug spec.
> The first of these surely does nothing; it will eventually end up in the
> garbage collector.  The second will form the function slot of the symbol,
> fulfilling all the Edebug things.  What am I missing?

The problem is that Edebug not only generates objects that would later
be garbage-collected (and therefore not observable), but also modifies
observable global state. This starts at
https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1418
and continues for the rest of the edebug-make-form-wrapper function.
In particular, https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1444
sets the `edebug' symbol property of the symbol being generated. None
of these mutations are undone when backtracking.

>
> > Instrumenting a symbol with two different definitions can lead to very
> > subtle bugs because the frequency vector and the form offset vector are
> > out of sync, ....
>
> The picture you seem to be painting is of two distinct definitions being
> assigned to the same symbol, and both of them being live.  Do you have
> any evidence that this is happening?

Let's say it's rather an incompatible mixture of two definitions.
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 is a symptom of
this. Another piece of evidence is the implementation of
`edebug-make-form-wrapper': that function clearly modifies buffer
contents and symbol properties even in branches that would later be
discarded as part of backtracking.
My (not well evidenced) assumption is that
https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1427
regenerates the offset vector, but there's no regeneration of the
frequency vector, which is the immediate trigger of
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853, since now the
frequency and offset vectors might be incompatible with each other.
But I'd also assume the problem runs deeper: edebug-make-form-wrapper
performs multiple mutations, and it's not really clear which of those
are "safe" w.r.t. multiple definitions in not-taken branches.

>
> > .... see e.g.  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853.
> > Therefore it's important to prevent such duplicate instrumentation,
> > typically by changing the Edebug symbol in some way (appending a unique
> > suffix, etc.). Edebug does this already in many cases (ERT tests, CL
> > methods, ...), but not always.  For some more context, see the coverage
> > instrumentation in my Bazel rules for ELisp
> > (https://github.com/phst/rules_elisp).
> > https://github.com/phst/rules_elisp/blob/master/elisp/ert/runner.el
> > contains the ERT and coverage integration. In
> > https://github.com/phst/rules_elisp/blob/0b24aa1660af2f6c668899bdd78aaba383d7ac18/elisp/ert/runner.el#L133-L134
> > I explicitly check for duplicate instrumentation. It is hard to predict
> > in general whether a specific instance of duplicate instrumentation
> > will lead to bugs like
> > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 or not, thus I'm
> > treating every duplicate instrumentation as a bug.
>
> What exactly do you mean by "duplicate instrumentation"?  If a symbol
> gets defined twice, once for each arm of an &or in the edebug spec, does
> that count as a duplicate instrumentation?

What I mean concretely is evaluating `edebug-make-form-wrapper' (and
therefore, mutating symbol properties and buffer contents) once for
each branch of an &or construct.

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Alan Mackenzie]

Hello, Philipp.

On Sun, Aug 09, 2020 at 13:33:53 +0200, Philipp Stephani wrote:
> Am Sa., 8. Aug. 2020 um 16:59 Uhr schrieb Alan Mackenzie <acm@muc.de>:

> > I must admit, I'm having difficulty understanding this problem.

> > On Sat, Aug 08, 2020 at 13:01:50 +0200, Philipp Stephani wrote:
> > > Am Mo., 22. Juni 2020 um 01:48 Uhr schrieb Alan Mackenzie <acm@muc.de>:

[ .... ]

> > > So this is somewhat subtle, so let me try to give some context. The
> > > message is merely a symptom of defining a symbol twice (via
> > > edebug-make-form-wrapper). That's a problem when using Edebug for
> > > coverage instrumentation (in batch mode), as the coverage
> > > information is attached to properties of the symbol that Edebug
> > > generates/instruments.

> > I'm trying to see what, exactly, this problem is.  Edebug is defining
> > a symbol twice, once for each of two arms of a &or form in the edebug
> > spec.  The first of these surely does nothing; it will eventually end
> > up in the garbage collector.  The second will form the function slot
> > of the symbol, fulfilling all the Edebug things.  What am I missing?

> The problem is that Edebug not only generates objects that would later
> be garbage-collected (and therefore not observable), but also modifies
> observable global state. This starts at
> https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1418
> and continues for the rest of the edebug-make-form-wrapper function.
> In particular,
> https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1444
> sets the `edebug' symbol property of the symbol being generated. None
> of these mutations are undone when backtracking.

Ah, I think I see it, now.  edebug-form-data contains structures
referring to functions, and could well have two entries with the same
function name.  (I see that at the moment in a file where I instrumented
alternately an old version and a new version of a function.)  The
property list on the symbol then contains a messy combination of details
for the two functions.

> > > Instrumenting a symbol with two different definitions can lead to
> > > very subtle bugs because the frequency vector and the form offset
> > > vector are out of sync, ....

> > The picture you seem to be painting is of two distinct definitions
> > being assigned to the same symbol, and both of them being live.  Do
> > you have any evidence that this is happening?

> Let's say it's rather an incompatible mixture of two definitions.
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 is a symptom of
> this. Another piece of evidence is the implementation of
> `edebug-make-form-wrapper': that function clearly modifies buffer
> contents and symbol properties even in branches that would later be
> discarded as part of backtracking.
> My (not well evidenced) assumption is that
> https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1427
> regenerates the offset vector, but there's no regeneration of the
> frequency vector, which is the immediate trigger of
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853, since now the
> frequency and offset vectors might be incompatible with each other.
> But I'd also assume the problem runs deeper: edebug-make-form-wrapper
> performs multiple mutations, and it's not really clear which of those
> are "safe" w.r.t. multiple definitions in not-taken branches.

How about, instead of having symbol properties, we institute non-symbol
property lists contained in each entry in edebug-form-data?  This list
could be rapidly searched, with repeated memq, for the pertinent entry.
It would mean, however, that all gathered data would be discarded on each
fresh instrumentation of a function.  Apologies if you've already
suggested this and I missed it.

> > > .... see e.g.  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853.
> > > Therefore it's important to prevent such duplicate instrumentation,
> > > typically by changing the Edebug symbol in some way (appending a unique
> > > suffix, etc.). Edebug does this already in many cases (ERT tests, CL
> > > methods, ...), but not always.  For some more context, see the coverage
> > > instrumentation in my Bazel rules for ELisp
> > > (https://github.com/phst/rules_elisp).
> > > https://github.com/phst/rules_elisp/blob/master/elisp/ert/runner.el
> > > contains the ERT and coverage integration. In
> > > https://github.com/phst/rules_elisp/blob/0b24aa1660af2f6c668899bdd78aaba383d7ac18/elisp/ert/runner.el#L133-L134
> > > I explicitly check for duplicate instrumentation. It is hard to predict
> > > in general whether a specific instance of duplicate instrumentation
> > > will lead to bugs like
> > > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 or not, thus I'm
> > > treating every duplicate instrumentation as a bug.

> > What exactly do you mean by "duplicate instrumentation"?  If a symbol
> > gets defined twice, once for each arm of an &or in the edebug spec, does
> > that count as a duplicate instrumentation?

> What I mean concretely is evaluating `edebug-make-form-wrapper' (and
> therefore, mutating symbol properties and buffer contents) once for
> each branch of an &or construct.

OK, thanks.  How does my above plan, for reinitilising the function's
properties at each instrumentation, sound?

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp Stephani]

Am So., 9. Aug. 2020 um 18:35 Uhr schrieb Alan Mackenzie <acm@muc.de>:
>
> Hello, Philipp.
>
> On Sun, Aug 09, 2020 at 13:33:53 +0200, Philipp Stephani wrote:
> > Am Sa., 8. Aug. 2020 um 16:59 Uhr schrieb Alan Mackenzie <acm@muc.de>:
>
> > > I must admit, I'm having difficulty understanding this problem.
>
> > > On Sat, Aug 08, 2020 at 13:01:50 +0200, Philipp Stephani wrote:
> > > > Am Mo., 22. Juni 2020 um 01:48 Uhr schrieb Alan Mackenzie <acm@muc.de>:
>
> [ .... ]
>
> > > > So this is somewhat subtle, so let me try to give some context. The
> > > > message is merely a symptom of defining a symbol twice (via
> > > > edebug-make-form-wrapper). That's a problem when using Edebug for
> > > > coverage instrumentation (in batch mode), as the coverage
> > > > information is attached to properties of the symbol that Edebug
> > > > generates/instruments.
>
> > > I'm trying to see what, exactly, this problem is.  Edebug is defining
> > > a symbol twice, once for each of two arms of a &or form in the edebug
> > > spec.  The first of these surely does nothing; it will eventually end
> > > up in the garbage collector.  The second will form the function slot
> > > of the symbol, fulfilling all the Edebug things.  What am I missing?
>
> > The problem is that Edebug not only generates objects that would later
> > be garbage-collected (and therefore not observable), but also modifies
> > observable global state. This starts at
> > https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1418
> > and continues for the rest of the edebug-make-form-wrapper function.
> > In particular,
> > https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1444
> > sets the `edebug' symbol property of the symbol being generated. None
> > of these mutations are undone when backtracking.
>
> Ah, I think I see it, now.  edebug-form-data contains structures
> referring to functions, and could well have two entries with the same
> function name.  (I see that at the moment in a file where I instrumented
> alternately an old version and a new version of a function.)  The
> property list on the symbol then contains a messy combination of details
> for the two functions.

Yes.

>
> > > > Instrumenting a symbol with two different definitions can lead to
> > > > very subtle bugs because the frequency vector and the form offset
> > > > vector are out of sync, ....
>
> > > The picture you seem to be painting is of two distinct definitions
> > > being assigned to the same symbol, and both of them being live.  Do
> > > you have any evidence that this is happening?
>
> > Let's say it's rather an incompatible mixture of two definitions.
> > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 is a symptom of
> > this. Another piece of evidence is the implementation of
> > `edebug-make-form-wrapper': that function clearly modifies buffer
> > contents and symbol properties even in branches that would later be
> > discarded as part of backtracking.
> > My (not well evidenced) assumption is that
> > https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1427
> > regenerates the offset vector, but there's no regeneration of the
> > frequency vector, which is the immediate trigger of
> > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853, since now the
> > frequency and offset vectors might be incompatible with each other.
> > But I'd also assume the problem runs deeper: edebug-make-form-wrapper
> > performs multiple mutations, and it's not really clear which of those
> > are "safe" w.r.t. multiple definitions in not-taken branches.
>
> How about, instead of having symbol properties, we institute non-symbol
> property lists contained in each entry in edebug-form-data?  This list
> could be rapidly searched, with repeated memq, for the pertinent entry.
> It would mean, however, that all gathered data would be discarded on each
> fresh instrumentation of a function.  Apologies if you've already
> suggested this and I missed it.

That seems roughly what the documentation of `edebug-form-data' itself
suggests: "In the future (haha!), the symbol will be irrelevant and
edebug data will be stored in the definitions themselves rather than
in the property list of a symbol."
Though I don't know what "the definitions themselves" is supposed to mean here.
In any case, I'd agree that attaching the data (in an undocumented
manner) to some symbol that then carefully needs to be constructed to
be unique is too brittle. However, the current implementation is used
in quite a few places (see e.g. the interactive specification of
`edebug-remove-instrumentation'), so changing it isn't trivial.

>
> > > > .... see e.g.  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853.
> > > > Therefore it's important to prevent such duplicate instrumentation,
> > > > typically by changing the Edebug symbol in some way (appending a unique
> > > > suffix, etc.). Edebug does this already in many cases (ERT tests, CL
> > > > methods, ...), but not always.  For some more context, see the coverage
> > > > instrumentation in my Bazel rules for ELisp
> > > > (https://github.com/phst/rules_elisp).
> > > > https://github.com/phst/rules_elisp/blob/master/elisp/ert/runner.el
> > > > contains the ERT and coverage integration. In
> > > > https://github.com/phst/rules_elisp/blob/0b24aa1660af2f6c668899bdd78aaba383d7ac18/elisp/ert/runner.el#L133-L134
> > > > I explicitly check for duplicate instrumentation. It is hard to predict
> > > > in general whether a specific instance of duplicate instrumentation
> > > > will lead to bugs like
> > > > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 or not, thus I'm
> > > > treating every duplicate instrumentation as a bug.
>
> > > What exactly do you mean by "duplicate instrumentation"?  If a symbol
> > > gets defined twice, once for each arm of an &or in the edebug spec, does
> > > that count as a duplicate instrumentation?
>
> > What I mean concretely is evaluating `edebug-make-form-wrapper' (and
> > therefore, mutating symbol properties and buffer contents) once for
> > each branch of an &or construct.
>
> OK, thanks.  How does my above plan, for reinitilising the function's
> properties at each instrumentation, sound?

It would definitely be an improvement I think.

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Stefan Monnier]

> As an example, edebug-instrument (C-u C-M-x) the following definition:
>
> (defun bar ()
>   (cl-flet ((foo () 1))
>     (foo)))
>
> The *Messages* buffer now says
>
> Edebug: foo [2 times]
> Edebug: bar

I believe this is now fixed in `master`.
At least we only get a total of 2 messages rather than "3 collapsed to 2".
But I don't know enough about the code coverage to be sure that the
underlying problem you saw is also fixed.  Can you confirm?


        Stefan

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp Stephani]

Am Di., 2. März 2021 um 16:59 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> > As an example, edebug-instrument (C-u C-M-x) the following definition:
> >
> > (defun bar ()
> >   (cl-flet ((foo () 1))
> >     (foo)))
> >
> > The *Messages* buffer now says
> >
> > Edebug: foo [2 times]
> > Edebug: bar
>
> I believe this is now fixed in `master`.
> At least we only get a total of 2 messages rather than "3 collapsed to 2".
> But I don't know enough about the code coverage to be sure that the
> underlying problem you saw is also fixed.  Can you confirm?

Thanks! I'll check (hopefully in the next few days).
For context, the tests that should now be fixed are
https://github.com/phst/rules_elisp/blob/6050b4ecc35e1a450320420f3a652f2551fc22b1/tests/ert_test.go#L215
and https://github.com/phst/rules_elisp/blob/6050b4ecc35e1a450320420f3a652f2551fc22b1/elisp/ert/runner.el#L138.
But these run under Emacs 27 by default. I'm going to check whether I
can make them run under "master" as well.

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp Stephani]

Am Di., 2. März 2021 um 18:28 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Di., 2. März 2021 um 16:59 Uhr schrieb Stefan Monnier
> <monnier@iro.umontreal.ca>:
> >
> > > As an example, edebug-instrument (C-u C-M-x) the following definition:
> > >
> > > (defun bar ()
> > >   (cl-flet ((foo () 1))
> > >     (foo)))
> > >
> > > The *Messages* buffer now says
> > >
> > > Edebug: foo [2 times]
> > > Edebug: bar
> >
> > I believe this is now fixed in `master`.
> > At least we only get a total of 2 messages rather than "3 collapsed to 2".
> > But I don't know enough about the code coverage to be sure that the
> > underlying problem you saw is also fixed.  Can you confirm?
>
> Thanks! I'll check (hopefully in the next few days).

Confirmed that the original issue is indeed fixed.

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp Stephani]

Am Mo., 8. März 2021 um 17:33 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Di., 2. März 2021 um 18:28 Uhr schrieb Philipp Stephani
> <p.stephani2@gmail.com>:
> >
> > Am Di., 2. März 2021 um 16:59 Uhr schrieb Stefan Monnier
> > <monnier@iro.umontreal.ca>:
> > >
> > > > As an example, edebug-instrument (C-u C-M-x) the following definition:
> > > >
> > > > (defun bar ()
> > > >   (cl-flet ((foo () 1))
> > > >     (foo)))
> > > >
> > > > The *Messages* buffer now says
> > > >
> > > > Edebug: foo [2 times]
> > > > Edebug: bar
> > >
> > > I believe this is now fixed in `master`.
> > > At least we only get a total of 2 messages rather than "3 collapsed to 2".
> > > But I don't know enough about the code coverage to be sure that the
> > > underlying problem you saw is also fixed.  Can you confirm?
> >
> > Thanks! I'll check (hopefully in the next few days).
>
> Confirmed that the original issue is indeed fixed.

Hmm, maybe I spoke too soon. The original issue with cl-flet is indeed
fixed, but I think one likely explanation for that is that it now only
has a single &define form. What about other macros that still have
multiple &define forms in an &or form? Are they fixed as well?

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Stefan Monnier]

> Hmm, maybe I spoke too soon. The original issue with cl-flet is indeed
> fixed, but I think one likely explanation for that is that it now only
> has a single &define form. What about other macros that still have
> multiple &define forms in an &or form? Are they fixed as well?

No, I don't think they'd be fixed either.


        Stefan

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp]

> Am 08.03.2021 um 18:41 schrieb Stefan Monnier <monnier@IRO.UMontreal.CA>:
> 
>> Hmm, maybe I spoke too soon. The original issue with cl-flet is indeed
>> fixed, but I think one likely explanation for that is that it now only
>> has a single &define form. What about other macros that still have
>> multiple &define forms in an &or form? Are they fixed as well?
> 
> No, I don't think they'd be fixed either.
> 

OK, then maybe it would make sense to reopen this bug.
Do you know of a similar example that still reproduces the bug?

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Stefan Monnier]

>>> Hmm, maybe I spoke too soon. The original issue with cl-flet is indeed
>>> fixed, but I think one likely explanation for that is that it now only
>>> has a single &define form. What about other macros that still have
>>> multiple &define forms in an &or form? Are they fixed as well?
>> No, I don't think they'd be fixed either.
> OK, then maybe it would make sense to reopen this bug.

It might be easier to declare this as a known limitation.

> Do you know of a similar example that still reproduces the bug?

No.  And I'd hope the problem can be avoided in all cases.
I guess we could try and make it more "official" by imposing some kind of "cut"
such that after passing a `&define` we can't backtrack.


        Stefan

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp]

> Am 14.03.2021 um 18:38 schrieb Stefan Monnier <monnier@iro.umontreal.ca>:
> 
>>>> Hmm, maybe I spoke too soon. The original issue with cl-flet is indeed
>>>> fixed, but I think one likely explanation for that is that it now only
>>>> has a single &define form. What about other macros that still have
>>>> multiple &define forms in an &or form? Are they fixed as well?
>>> No, I don't think they'd be fixed either.
>> OK, then maybe it would make sense to reopen this bug.
> 
> It might be easier to declare this as a known limitation.
> 
>> Do you know of a similar example that still reproduces the bug?
> 
> No.  And I'd hope the problem can be avoided in all cases.
> I guess we could try and make it more "official" by imposing some kind of "cut"
> such that after passing a `&define` we can't backtrack.
> 

From looking at the code, would it be possible to achieve this by setting edebug-gate to non-nil in the right places?
If so, then this seems to be only a matter of finding the right places ;-)

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Stefan Monnier]

>> No.  And I'd hope the problem can be avoided in all cases.
>> I guess we could try and make it more "official" by imposing some kind of "cut"
>> such that after passing a `&define` we can't backtrack.
> From looking at the code, would it be possible to achieve this by setting
> edebug-gate to non-nil in the right places?
> If so, then this seems to be only a matter of finding the right places ;-)

It's possible, but:
- I don't understand enough the way backtracking works in Edebug to know
  what `edebug-gate` does, really.
- The old spec of `cl-flet` would be broken by such a change, so if we
  want to make such a change, we'd probably want to arrange so that it
  emits a clear warning.

I'm not sure it's worth the trouble: the pain seems higher than the gain.


        Stefan

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp]

> Am 18.03.2021 um 15:01 schrieb Stefan Monnier <monnier@iro.umontreal.ca>:
> 
>>> No.  And I'd hope the problem can be avoided in all cases.
>>> I guess we could try and make it more "official" by imposing some kind of "cut"
>>> such that after passing a `&define` we can't backtrack.
>> From looking at the code, would it be possible to achieve this by setting
>> edebug-gate to non-nil in the right places?
>> If so, then this seems to be only a matter of finding the right places ;-)
> 
> It's possible, but:
> - I don't understand enough the way backtracking works in Edebug to know
>  what `edebug-gate` does, really.

It looks like you can set edebug-gate to t in edebug--match-&-spec-op (the &define branch).  That should have the same effect as a [gate ...] construct around each &define form.

> - The old spec of `cl-flet` would be broken by such a change, so if we
>  want to make such a change, we'd probably want to arrange so that it
>  emits a clear warning.

Where?  When setting the debug specification (byte-run--set-debug), or in some other place?

> 
> I'm not sure it's worth the trouble: the pain seems higher than the gain.
> 

This bug is rather nasty when it's hit (it took me quite a while to debug/hunt down), so I think it would be reasonable to prevent.  We already disable backtracking for literal symbols, and I think forms that require multiple &define forms with backtracking should be exceedingly rare and can be rewritten as you did with cl-flet.

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Stefan Monnier]

>> - The old spec of `cl-flet` would be broken by such a change, so if we
>>  want to make such a change, we'd probably want to arrange so that it
>>  emits a clear warning.
> Where?  When setting the debug specification (byte-run--set-debug), or in some other place?

The old spec was of the form (or (&define name form) (&define name sexp
body)), so if we put a gate at `&define`, this will probably fail to even match
the (&define name sexp body) part.

[ Disclaimer: I don't understand the precise semantics of `gate`, tho
  I do remember using it once via trial-and-error.  So maybe it wouldn't
  prevent it, but if doesn't prevent it, then it doesn't likely "fix"
  our problem ;-)  ]

>> I'm not sure it's worth the trouble: the pain seems higher than the gain.
> This bug is rather nasty when it's hit (it took me quite a while to
> debug/hunt down),

Could you remind me what was this nasty outcome?  In the context of
Edebug the effect is negligible (it just emits a spurious message about
defining an extra something, but other than that debugging works just fine).

> so I think it would be reasonable to prevent.  We already
> disable backtracking for literal symbols, and I think forms that require
> multiple &define forms with backtracking should be exceedingly rare and can
> be rewritten as you did with cl-flet.

Emitting a warning would be much more helpful than just silently
"cut"ting the backtracking.


        Stefan

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp Stephani]

Am So., 21. März 2021 um 15:37 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> >> - The old spec of `cl-flet` would be broken by such a change, so if we
> >>  want to make such a change, we'd probably want to arrange so that it
> >>  emits a clear warning.
> > Where?  When setting the debug specification (byte-run--set-debug), or in some other place?
>
> The old spec was of the form (or (&define name form) (&define name sexp
> body)), so if we put a gate at `&define`, this will probably fail to even match
> the (&define name sexp body) part.

Correct, this would turn a mismatch into a hard error.

>
> [ Disclaimer: I don't understand the precise semantics of `gate`, tho
>   I do remember using it once via trial-and-error.  So maybe it wouldn't
>   prevent it, but if doesn't prevent it, then it doesn't likely "fix"
>   our problem ;-)  ]

AIUI the semantics of "gate" aren't that complex, it just means "don't
backtrack beyond this point."

>
> >> I'm not sure it's worth the trouble: the pain seems higher than the gain.
> > This bug is rather nasty when it's hit (it took me quite a while to
> > debug/hunt down),
>
> Could you remind me what was this nasty outcome?

The original bug report was
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 (extremely subtle
bug due to mismatch between frequency and offset vector). I don't know
whether that problem would appear here as well, but it's nasty enough
that I'd like to prevent duplicate definitions altogether.

> In the context of
> Edebug the effect is negligible (it just emits a spurious message about
> defining an extra something, but other than that debugging works just fine).

That's only true if edebug-new-definition-function handles the
"duplicate definition" case well. For example, to prevent bug#41853 I
bail out on duplicate definitions in
https://github.com/phst/rules_elisp/blob/b67339e66fed8117dc26d4d2fb2ad321c66a3632/elisp/ert/runner.el#L127-L136;
that doesn't work if this bug isn't fixed.

>
> > so I think it would be reasonable to prevent.  We already
> > disable backtracking for literal symbols, and I think forms that require
> > multiple &define forms with backtracking should be exceedingly rare and can
> > be rewritten as you did with cl-flet.
>
> Emitting a warning would be much more helpful than just silently
> "cut"ting the backtracking.

A gate isn't silent, it would cause a hard error in this case.

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Stefan Monnier]

>> [ Disclaimer: I don't understand the precise semantics of `gate`, tho
>>   I do remember using it once via trial-and-error.  So maybe it wouldn't
>>   prevent it, but if doesn't prevent it, then it doesn't likely "fix"
>>   our problem ;-)  ]
> AIUI the semantics of "gate" aren't that complex, it just means "don't
> backtrack beyond this point."

[ Yes, that's the part I understand.  But it's not clear where
  backtracking is possible and where it's not.  At least, the code that
  I saw in edebug.el didn't match my expectations back when I looked at
  it, hence my not feeling quite sure what the semantics are (and/or
  should be).
  IIRC the issue was that the scope of that effect wasn't clear: if you
  think of Prolog's cut, its effect is local to a particular definition,
  whereas I think the scope of `gate` is not nearly as clear because
  there isn't such a notion of "definition".  ]

>> >> I'm not sure it's worth the trouble: the pain seems higher than the gain.
>> > This bug is rather nasty when it's hit (it took me quite a while to
>> > debug/hunt down),
>> Could you remind me what was this nasty outcome?
> The original bug report was
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 (extremely subtle
> bug due to mismatch between frequency and offset vector).

Thanks, that's worst than I thought indeed.

>> > so I think it would be reasonable to prevent.  We already
>> > disable backtracking for literal symbols, and I think forms that require
>> > multiple &define forms with backtracking should be exceedingly rare and can
>> > be rewritten as you did with cl-flet.
>> Emitting a warning would be much more helpful than just silently
>> "cut"ting the backtracking.
> A gate isn't silent, it would cause a hard error in this case.

What I meant is that a gate would just make the old cl-flet spec fail in
most cases, with no explanation why that spec now fails even though it
worked in the past.


        Stefan

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp Stephani]

Am So., 4. Apr. 2021 um 22:16 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> >> [ Disclaimer: I don't understand the precise semantics of `gate`, tho
> >>   I do remember using it once via trial-and-error.  So maybe it wouldn't
> >>   prevent it, but if doesn't prevent it, then it doesn't likely "fix"
> >>   our problem ;-)  ]
> > AIUI the semantics of "gate" aren't that complex, it just means "don't
> > backtrack beyond this point."
>
> [ Yes, that's the part I understand.  But it's not clear where
>   backtracking is possible and where it's not.  At least, the code that
>   I saw in edebug.el didn't match my expectations back when I looked at
>   it, hence my not feeling quite sure what the semantics are (and/or
>   should be).
>   IIRC the issue was that the scope of that effect wasn't clear: if you
>   think of Prolog's cut, its effect is local to a particular definition,
>   whereas I think the scope of `gate` is not nearly as clear because
>   there isn't such a notion of "definition".  ]

I agree that the code isn't terribly clear and probably has some bugs.
However, the "prevent backtracking through gates" seems to work at
least somewhat (experimentally, in this case I can turn the double
definition into an error).

> >> > so I think it would be reasonable to prevent.  We already
> >> > disable backtracking for literal symbols, and I think forms that require
> >> > multiple &define forms with backtracking should be exceedingly rare and can
> >> > be rewritten as you did with cl-flet.
> >> Emitting a warning would be much more helpful than just silently
> >> "cut"ting the backtracking.
> > A gate isn't silent, it would cause a hard error in this case.
>
> What I meant is that a gate would just make the old cl-flet spec fail in
> most cases, with no explanation why that spec now fails even though it
> worked in the past.

Yes, we'd need to at least announce the change in NEWS as an
incompatible Lisp change. However, I think overall that's still better
than the current situation: with your fix to cl-flet the problematic
constructs shouldn't occur any more in the Emacs codebase, and I
wouldn't expect such constructs to be very frequent "in the wild", so
the overall breakage would be very small, and if it can avoid
similarly subtle bugs then I think it's warranted.

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp]

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



> Am 05.04.2021 um 16:32 schrieb Philipp Stephani <p.stephani2@gmail.com>:
> 
>>>>> so I think it would be reasonable to prevent.  We already
>>>>> disable backtracking for literal symbols, and I think forms that require
>>>>> multiple &define forms with backtracking should be exceedingly rare and can
>>>>> be rewritten as you did with cl-flet.
>>>> Emitting a warning would be much more helpful than just silently
>>>> "cut"ting the backtracking.
>>> A gate isn't silent, it would cause a hard error in this case.
>> 
>> What I meant is that a gate would just make the old cl-flet spec fail in
>> most cases, with no explanation why that spec now fails even though it
>> worked in the past.
> 
> Yes, we'd need to at least announce the change in NEWS as an
> incompatible Lisp change. However, I think overall that's still better
> than the current situation: with your fix to cl-flet the problematic
> constructs shouldn't occur any more in the Emacs codebase, and I
> wouldn't expect such constructs to be very frequent "in the wild", so
> the overall breakage would be very small, and if it can avoid
> similarly subtle bugs then I think it's warranted.

Here's a patch.

[-- Attachment #2: 0001-Edebug-Disable-backtracking-when-hitting-a-define-ke.patch --]
[-- Type: application/octet-stream, Size: 5544 bytes --]

From 9e2183edea41adf275f057a75232ea0b2c51e726 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Thu, 18 Mar 2021 12:40:08 +0100
Subject: [PATCH] Edebug: Disable backtracking when hitting a &define keyword.

Edebug doesn't deal well with backtracking out of definitions, see
Bug#41988.  Rather than trying to support this rare situation (e.g. by
implementing a multipass parser), prevent it by adding an implicit
gate.

* lisp/emacs-lisp/edebug.el (edebug--match-&-spec-op): Disable
backtracking when hitting a &define keyword.

* test/lisp/emacs-lisp/edebug-tests.el
(edebug-tests-duplicate-&define): New unit test.
(edebug-tests--duplicate-&define): New helper macro.

* doc/lispref/edebug.texi (Backtracking): Mention &define in the list
of constructs that disable backtracking.

* etc/NEWS: Document new behavior.
---
 doc/lispref/edebug.texi              | 10 +++++-----
 etc/NEWS                             |  3 +++
 lisp/emacs-lisp/edebug.el            | 18 ++++++++++--------
 test/lisp/emacs-lisp/edebug-tests.el | 25 +++++++++++++++++++++++++
 4 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/doc/lispref/edebug.texi b/doc/lispref/edebug.texi
index 8942f55aff..323130f237 100644
--- a/doc/lispref/edebug.texi
+++ b/doc/lispref/edebug.texi
@@ -1510,11 +1510,11 @@ Backtracking
 must be in the form itself rather than at a higher level.
 
 Backtracking is also disabled after successfully matching a quoted
-symbol or string specification, since this usually indicates a
-recognized construct.  But if you have a set of alternative constructs that
-all begin with the same symbol, you can usually work around this
-constraint by factoring the symbol out of the alternatives, e.g.,
-@code{["foo" &or [first case] [second case] ...]}.
+symbol, string specification, or @code{&define} keyword, since this
+usually indicates a recognized construct.  But if you have a set of
+alternative constructs that all begin with the same symbol, you can
+usually work around this constraint by factoring the symbol out of the
+alternatives, e.g., @code{["foo" &or [first case] [second case] ...]}.
 
 Most needs are satisfied by these two ways that backtracking is
 automatically disabled, but occasionally it is useful to explicitly
diff --git a/etc/NEWS b/etc/NEWS
index a0f05d8cf1..9ae3740482 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2524,6 +2524,9 @@ back in Emacs 23.1.  The affected functions are: 'make-obsolete',
 
 ** The 'values' variable is now obsolete.
 
+** The '&define' keyword in an Edebug specification now disables
+backtracking.
+
 \f
 * Lisp Changes in Emacs 28.1
 
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index f1455ffe73..365bc74874 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1942,14 +1942,16 @@ edebug--match-&-spec-op
   ;; Normally, &define is interpreted specially other places.
   ;; This should only be called inside of a spec list to match the remainder
   ;; of the current list.  e.g. ("lambda" &define args def-body)
-   (edebug-make-form-wrapper
-    cursor
-    (edebug-before-offset cursor)
-    ;; Find the last offset in the list.
-    (let ((offsets (edebug-cursor-offsets cursor)))
-      (while (consp offsets) (setq offsets (cdr offsets)))
-      offsets)
-    specs))
+  (prog1 (edebug-make-form-wrapper
+          cursor
+          (edebug-before-offset cursor)
+          ;; Find the last offset in the list.
+          (let ((offsets (edebug-cursor-offsets cursor)))
+            (while (consp offsets) (setq offsets (cdr offsets)))
+            offsets)
+          specs)
+    ;; Stop backtracking here (Bug#41988).
+    (setq edebug-gate t)))
 
 (cl-defmethod edebug--match-&-spec-op ((_ (eql &name)) cursor specs)
   "Compute the name for `&name SPEC FUN` spec operator.
diff --git a/test/lisp/emacs-lisp/edebug-tests.el b/test/lisp/emacs-lisp/edebug-tests.el
index dcb261c2eb..7d45432e57 100644
--- a/test/lisp/emacs-lisp/edebug-tests.el
+++ b/test/lisp/emacs-lisp/edebug-tests.el
@@ -1061,5 +1061,30 @@ edebug-tests-duplicate-symbol-backtrack
                        "edebug-anon10001"
                        "edebug-tests-duplicate-symbol-backtrack"))))))
 
+(defmacro edebug-tests--duplicate-&define (_arg)
+  "Helper macro for the ERT test `edebug-tests-duplicate-&define'.
+The Edebug specification is similar to the one used by `cl-flet'
+previously; see Bug#41988."
+  (declare (debug (&or (&define name function-form) (defun)))))
+
+(ert-deftest edebug-tests-duplicate-&define ()
+  "Check that Edebug doesn't backtrack out of `&define' forms.
+This avoids potential duplicate definitions (Bug#41988)."
+  (with-temp-buffer
+    (print '(defun edebug-tests-duplicate-&define ()
+              (edebug-tests--duplicate-&define
+               (edebug-tests-duplicate-&define-inner () nil)))
+           (current-buffer))
+    (let* ((edebug-all-defs t)
+           (edebug-initial-mode 'Go-nonstop)
+           (instrumented-names ())
+           (edebug-new-definition-function
+            (lambda (name)
+              (when (memq name instrumented-names)
+                (error "Duplicate definition of `%s'" name))
+              (push name instrumented-names)
+              (edebug-new-definition name))))
+      (should-error (eval-buffer) :type 'invalid-read-syntax))))
+
 (provide 'edebug-tests)
 ;;; edebug-tests.el ends here
-- 
2.24.3 (Apple Git-128)

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Stefan Monnier]

> Here's a patch.

LGTM, as far as I'm concerned you can push it to `master` and we'll just
cross our fingers ;-)


        Stefan


> From 9e2183edea41adf275f057a75232ea0b2c51e726 Mon Sep 17 00:00:00 2001
> From: Philipp Stephani <phst@google.com>
> Date: Thu, 18 Mar 2021 12:40:08 +0100
> Subject: [PATCH] Edebug: Disable backtracking when hitting a &define keyword.
>
> Edebug doesn't deal well with backtracking out of definitions, see
> Bug#41988.  Rather than trying to support this rare situation (e.g. by
> implementing a multipass parser), prevent it by adding an implicit
> gate.
>
> * lisp/emacs-lisp/edebug.el (edebug--match-&-spec-op): Disable
> backtracking when hitting a &define keyword.
>
> * test/lisp/emacs-lisp/edebug-tests.el
> (edebug-tests-duplicate-&define): New unit test.
> (edebug-tests--duplicate-&define): New helper macro.
>
> * doc/lispref/edebug.texi (Backtracking): Mention &define in the list
> of constructs that disable backtracking.
>
> * etc/NEWS: Document new behavior.
> ---
>  doc/lispref/edebug.texi              | 10 +++++-----
>  etc/NEWS                             |  3 +++
>  lisp/emacs-lisp/edebug.el            | 18 ++++++++++--------
>  test/lisp/emacs-lisp/edebug-tests.el | 25 +++++++++++++++++++++++++
>  4 files changed, 43 insertions(+), 13 deletions(-)
>
> diff --git a/doc/lispref/edebug.texi b/doc/lispref/edebug.texi
> index 8942f55aff..323130f237 100644
> --- a/doc/lispref/edebug.texi
> +++ b/doc/lispref/edebug.texi
> @@ -1510,11 +1510,11 @@ Backtracking
>  must be in the form itself rather than at a higher level.
>  
>  Backtracking is also disabled after successfully matching a quoted
> -symbol or string specification, since this usually indicates a
> -recognized construct.  But if you have a set of alternative constructs that
> -all begin with the same symbol, you can usually work around this
> -constraint by factoring the symbol out of the alternatives, e.g.,
> -@code{["foo" &or [first case] [second case] ...]}.
> +symbol, string specification, or @code{&define} keyword, since this
> +usually indicates a recognized construct.  But if you have a set of
> +alternative constructs that all begin with the same symbol, you can
> +usually work around this constraint by factoring the symbol out of the
> +alternatives, e.g., @code{["foo" &or [first case] [second case] ...]}.
>  
>  Most needs are satisfied by these two ways that backtracking is
>  automatically disabled, but occasionally it is useful to explicitly
> diff --git a/etc/NEWS b/etc/NEWS
> index a0f05d8cf1..9ae3740482 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -2524,6 +2524,9 @@ back in Emacs 23.1.  The affected functions are: 'make-obsolete',
>  
>  ** The 'values' variable is now obsolete.
>  
> +** The '&define' keyword in an Edebug specification now disables
> +backtracking.
> +
>  \f
>  * Lisp Changes in Emacs 28.1
>  
> diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
> index f1455ffe73..365bc74874 100644
> --- a/lisp/emacs-lisp/edebug.el
> +++ b/lisp/emacs-lisp/edebug.el
> @@ -1942,14 +1942,16 @@ edebug--match-&-spec-op
>    ;; Normally, &define is interpreted specially other places.
>    ;; This should only be called inside of a spec list to match the remainder
>    ;; of the current list.  e.g. ("lambda" &define args def-body)
> -   (edebug-make-form-wrapper
> -    cursor
> -    (edebug-before-offset cursor)
> -    ;; Find the last offset in the list.
> -    (let ((offsets (edebug-cursor-offsets cursor)))
> -      (while (consp offsets) (setq offsets (cdr offsets)))
> -      offsets)
> -    specs))
> +  (prog1 (edebug-make-form-wrapper
> +          cursor
> +          (edebug-before-offset cursor)
> +          ;; Find the last offset in the list.
> +          (let ((offsets (edebug-cursor-offsets cursor)))
> +            (while (consp offsets) (setq offsets (cdr offsets)))
> +            offsets)
> +          specs)
> +    ;; Stop backtracking here (Bug#41988).
> +    (setq edebug-gate t)))
>  
>  (cl-defmethod edebug--match-&-spec-op ((_ (eql &name)) cursor specs)
>    "Compute the name for `&name SPEC FUN` spec operator.
> diff --git a/test/lisp/emacs-lisp/edebug-tests.el b/test/lisp/emacs-lisp/edebug-tests.el
> index dcb261c2eb..7d45432e57 100644
> --- a/test/lisp/emacs-lisp/edebug-tests.el
> +++ b/test/lisp/emacs-lisp/edebug-tests.el
> @@ -1061,5 +1061,30 @@ edebug-tests-duplicate-symbol-backtrack
>                         "edebug-anon10001"
>                         "edebug-tests-duplicate-symbol-backtrack"))))))
>  
> +(defmacro edebug-tests--duplicate-&define (_arg)
> +  "Helper macro for the ERT test `edebug-tests-duplicate-&define'.
> +The Edebug specification is similar to the one used by `cl-flet'
> +previously; see Bug#41988."
> +  (declare (debug (&or (&define name function-form) (defun)))))
> +
> +(ert-deftest edebug-tests-duplicate-&define ()
> +  "Check that Edebug doesn't backtrack out of `&define' forms.
> +This avoids potential duplicate definitions (Bug#41988)."
> +  (with-temp-buffer
> +    (print '(defun edebug-tests-duplicate-&define ()
> +              (edebug-tests--duplicate-&define
> +               (edebug-tests-duplicate-&define-inner () nil)))
> +           (current-buffer))
> +    (let* ((edebug-all-defs t)
> +           (edebug-initial-mode 'Go-nonstop)
> +           (instrumented-names ())
> +           (edebug-new-definition-function
> +            (lambda (name)
> +              (when (memq name instrumented-names)
> +                (error "Duplicate definition of `%s'" name))
> +              (push name instrumented-names)
> +              (edebug-new-definition name))))
> +      (should-error (eval-buffer) :type 'invalid-read-syntax))))
> +
>  (provide 'edebug-tests)
>  ;;; edebug-tests.el ends here

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp]

> Am 10.04.2021 um 17:51 schrieb Stefan Monnier <monnier@iro.umontreal.ca>:
> 
>> Here's a patch.
> 
> LGTM, as far as I'm concerned you can push it to `master` and we'll just
> cross our fingers ;-)
> 

Done

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Eli Zaretskii]

> From: Philipp <p.stephani2@gmail.com>
> Date: Sat, 10 Apr 2021 17:07:48 +0200
> Cc: 41988@debbugs.gnu.org
> 
> diff --git a/etc/NEWS b/etc/NEWS
> index a0f05d8cf1..9ae3740482 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -2524,6 +2524,9 @@ back in Emacs 23.1.  The affected functions are: 'make-obsolete',
>  
>  ** The 'values' variable is now obsolete.
>  
> +** The '&define' keyword in an Edebug specification now disables
> +backtracking.

Please add here a pointer to the ELisp manual where this is described,
as the text otherwise is too terse to speak for itself.

Also, the heading line should be a single complete sentence, and the
NEWS entry should be marked "+++", since the manual is being updated
by the same changeset.

(I have no opinion on the change itself, although it strikes me as
unusual to delete a feature rather than fix it.)

Thanks.

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Stefan Monnier]

>> +** The '&define' keyword in an Edebug specification now disables
>> +backtracking.
>
> Please add here a pointer to the ELisp manual where this is described,
> as the text otherwise is too terse to speak for itself.
>
> Also, the heading line should be a single complete sentence, and the
> NEWS entry should be marked "+++", since the manual is being updated
> by the same changeset.
>
> (I have no opinion on the change itself, although it strikes me as
> unusual to delete a feature rather than fix it.)

It's a "feature" that's been broken for ever, that noone knows how to
fix, and for which we know no current user.

Also the new `&name` can be useful to avoid the need for that feature,
so in a sense we did provide a kind of fix in the form of a new
alternative solution.


        Stefan

bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs

[Philipp]

> Am 10.04.2021 um 19:29 schrieb Eli Zaretskii <eliz@gnu.org>:
> 
>> From: Philipp <p.stephani2@gmail.com>
>> Date: Sat, 10 Apr 2021 17:07:48 +0200
>> Cc: 41988@debbugs.gnu.org
>> 
>> diff --git a/etc/NEWS b/etc/NEWS
>> index a0f05d8cf1..9ae3740482 100644
>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> @@ -2524,6 +2524,9 @@ back in Emacs 23.1.  The affected functions are: 'make-obsolete',
>> 
>> ** The 'values' variable is now obsolete.
>> 
>> +** The '&define' keyword in an Edebug specification now disables
>> +backtracking.
> 
> Please add here a pointer to the ELisp manual where this is described,
> as the text otherwise is too terse to speak for itself.
> 
> Also, the heading line should be a single complete sentence, and the
> NEWS entry should be marked "+++", since the manual is being updated
> by the same changeset.

Done in commit 1d474ad69d19d01b047012734530fb4c5eb82144.