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

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Philipp Stephani

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))



Reply | Threaded
Open this post in threaded view
|

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

Alan Mackenzie
Hello, Philipp.

In article <[hidden email]> 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).




Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>:

>
> Hello, Philipp.
>
> In article <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>:

> > In article <[hidden email]> 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).



Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>:

>
> 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 <[hidden email]>:
>
> > > In article <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>:

> > 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 <[hidden email]>:

[ .... ]

> > > 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).



Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>:

>
> 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 <[hidden email]>:
>
> > > 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 <[hidden email]>:
>
> [ .... ]
>
> > > > 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.



Reply | Threaded
Open this post in threaded view
|

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

Philipp Stephani
In reply to this post by Philipp Stephani
Am Di., 2. März 2021 um 16:59 Uhr schrieb Stefan Monnier
<[hidden email]>:

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



Reply | Threaded
Open this post in threaded view
|

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
<[hidden email]>:

>
> Am Di., 2. März 2021 um 16:59 Uhr schrieb Stefan Monnier
> <[hidden email]>:
> >
> > > 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.



Reply | Threaded
Open this post in threaded view
|

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
<[hidden email]>:

>
> Am Di., 2. März 2021 um 18:28 Uhr schrieb Philipp Stephani
> <[hidden email]>:
> >
> > Am Di., 2. März 2021 um 16:59 Uhr schrieb Stefan Monnier
> > <[hidden email]>:
> > >
> > > > 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?



Reply | Threaded
Open this post in threaded view
|

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




Reply | Threaded
Open this post in threaded view
|

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
<[hidden email]>:
>
> >> - 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.



Reply | Threaded
Open this post in threaded view
|

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
<[hidden email]>:

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



Reply | Threaded
Open this post in threaded view
|

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

Philipp Stephani


> Am 05.04.2021 um 16:32 schrieb Philipp Stephani <[hidden email]>:
>
>>>>> 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.

0001-Edebug-Disable-backtracking-when-hitting-a-define-ke.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Philipp Stephani


> Am 10.04.2021 um 17:51 schrieb Stefan Monnier <[hidden email]>:
>
>> 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



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Philipp Stephani
> From: Philipp <[hidden email]>
> Date: Sat, 10 Apr 2021 17:07:48 +0200
> Cc: [hidden email]
>
> 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.



Reply | Threaded
Open this post in threaded view
|

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




Reply | Threaded
Open this post in threaded view
|

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

Philipp Stephani
In reply to this post by Eli Zaretskii


> Am 10.04.2021 um 19:29 schrieb Eli Zaretskii <[hidden email]>:
>
>> From: Philipp <[hidden email]>
>> Date: Sat, 10 Apr 2021 17:07:48 +0200
>> Cc: [hidden email]
>>
>> 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.