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