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

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

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