[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#41988: 28.0.50; Edebug unconditionally instruments definitions with
bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs
Sun, 9 Aug 2020 16:35:23 +0000
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 <email@example.com>:
> > 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 <firstname.lastname@example.org>:
[ .... ]
> > > 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
> and continues for the rest of the edebug-make-form-wrapper function.
> In particular,
> 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
> 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).