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.