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

[Philipp Stephani <p.stephani2@gmail.com>]

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.