GNU bug report logs - #25316
26.0.50; Bugs in testcover-reinstrument

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 25316 in the body.
You can then email your comments to 25316 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.0.50; Bugs in testcover-reinstrument
Date: Sat, 31 Dec 2016 16:39:38 -0800

I'm working on tests for testcover.el and a refactoring of
testcover-reinstrument to use pcase. In the process I've found these
bugs. The refactoring will fix these as well as make
testcover-reinstrument easier to read and modify.

Since testcover-start requires the code it works on to be saved in a
file, the instructions to reproduce the bugs below all assume that you
have saved the code snippet given in a file called bug.el. If you are
running Emacs 25 and testcover-start doesn't work, load it with this: 
M-: (require 'testcover) RET

I've reproduced all of these using emacs -Q.


1. Wrapping forms in 1value suppresses more splotches than it should.

(defun my-bug (num)
  (1value (- num num (- num 3) (* num -1) 3)))

To reproduce:
M-x testcover-start RET bug.el RET
M-: (my-bug 5) RET
M-x testcover-mark-all RET RET

Result: The form (- num 3) does not have the tan splotch that it should
have. (* num -1) does get a splotch.


2. Testcover splotches backquoted expressions containing only constants.

(defconst my-const "apples")
(defun my-bug ()
  `(,my-const "oranges"))

To reproduce:
M-x testcover-start RET bug.el RET
M-: (my-bug) RET
M-x testcover-mark-all RET RET

Result: The form `(,my-const "oranges") has a tan splotch. But
since it will always evaluate to ("apples" "oranges"), Testcover should
mark it as 1value and not splotch it.


3. Testcover fails to reinstrument inside vectors and backquoted vectors.

(defun my-bug (a b c)
  `[,a ,(list b c)])
  
(defmacro my-nth-case (arg vec)
  (declare (indent 1)
           (debug (form (vector &rest form))))
  `(eval (aref ,vec ,arg)))

(defun my-bug-2 (choice val)
  (my-nth-case choice
    [(+ 1 val)
     (- 1 val)
     (* 7 val)
     (/ 4 val)]))

To reproduce:
M-x testcover-start RET bug.el RET

Followed by either one of these:
M-: (my-bug 1 2 3) RET
M-: (my-bug-2 2 6) RET

Result in both cases: void-variable edebug-freq-count or
wrong-type-argument consp nil depending on if you are running the
current master or not. The errors happen in an edebug-after that did not
get replaced by a testcover-after.


4. Testcover incorrectly parses quoted forms within backquotes.

(defun my-make-list ()
  (list 'defun 'defvar))

(defmacro my-bq-macro (fun)
  (declare (debug (symbolp)))
  `(memq ,fun '(defconst ,@(my-make-list))))

(defun my-use-bq-macro (fun)
  (my-bq-macro fun))

To reproduce:
M-x testcover-start RET bug.el RET

Result: void-variable edebug-freq-count or wrong-type-argument consp
nil, another error from an edebug-after not replaced during
reinstrumentation. C-h v testcover-module-constants RET will show that
testcover was trying to reinstrument the list beginning with defconst as
a top-level form.


5. Testcover doesn't correctly reinstrument code matching an Edebug spec
containing a quote. See c-make-font-lock-search-function for an example
of an Edebug spec containing a quote in the Emacs sources.

(defun my-make-function (forms)
  `(lambda (flag) (if flag 0 ,@forms)))

(def-edebug-spec my-make-function
  (("quote" (&rest def-form))))

(defun my-thing ()
  (my-make-function '((+ 1 (+ 2 (+ 3 (+ 4 5)))))))

(defun my-use-thing ()
  (funcall (my-thing) nil))

To reproduce:
M-x testcover-start RET bug.el RET
M-: (my-use-thing) RET

Result: Emacs will give you the debugger prompt inside the definition of
my-thing, because an edebug-enter didn't get changed to a
testcover-enter in the instrumentation.

Message #8 received at 25316 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: 25316 <at> debbugs.gnu.org
Subject: Patch for bug#25316: 26.0.50; Bugs in testcover-reinstrument
Date: Mon, 25 Sep 2017 15:04:58 -0700

[Message part 1 (text/plain, inline)]

The attached patches fix the 5 bugs in this bug report, as well as
11307, 24509, 24688, 24743 and 25326.

testcover-reinstrument is a code walker which rewrites Edebug's
instrumented code replacing Edebug's functions edebug-enter,
edebug-before and edebug-after with Testcover's functions (some with
different call signatures), and at the same time determining which
forms might only return a single value and treating them
differently. Since it uses car and cdr to parse and rewrite the code,
it is consequently difficult to read and modify.

My original goal was to change testcover-reinstrument to use pcase,
but along the way I realized that I could simplify it and not only fix
all the bugs where one of Edebug's functions gets left in the
instrumented code, but remove the possibility of undiscovered or
future ones by not doing code rewriting and instead adding a hook
mechanism to make Edebug's functions call Testcover's. So in these
patches testcover-reinstrument has become testcover-analyze-coverage,
which uses pcase to walk Edebug's instrumented code and save the
results of its analysis of single-value forms in Edebug's code
coverage vector.

[0001-Allow-Edebug-s-instrumentation-to-be-used-for-other-.patch (text/plain, attachment)]

[0002-Rewrite-Testcover-s-internals-fixing-several-bugs.patch (text/plain, attachment)]

Message #13 received at 25316 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 25316 <at> debbugs.gnu.org
Subject: Re: bug#25316: Patch for bug#25316: 26.0.50;
 Bugs in testcover-reinstrument
Date: Fri, 29 Sep 2017 13:05:13 +0300

> From: Gemini Lasswell <gazally <at> runbox.com>
> Date: Mon, 25 Sep 2017 15:04:58 -0700
> 
> The attached patches fix the 5 bugs in this bug report, as well as
> 11307, 24509, 24688, 24743 and 25326.
> 
> testcover-reinstrument is a code walker which rewrites Edebug's
> instrumented code replacing Edebug's functions edebug-enter,
> edebug-before and edebug-after with Testcover's functions (some with
> different call signatures), and at the same time determining which
> forms might only return a single value and treating them
> differently. Since it uses car and cdr to parse and rewrite the code,
> it is consequently difficult to read and modify.

Thanks, I have a few comments below.  Please let others to comment for
a week or so before installing on master.

> 
> My original goal was to change testcover-reinstrument to use pcase,
> but along the way I realized that I could simplify it and not only fix
> all the bugs where one of Edebug's functions gets left in the
> instrumented code, but remove the possibility of undiscovered or
> future ones by not doing code rewriting and instead adding a hook
> mechanism to make Edebug's functions call Testcover's. So in these
> patches testcover-reinstrument has become testcover-analyze-coverage,
> which uses pcase to walk Edebug's instrumented code and save the
> results of its analysis of single-value forms in Edebug's code
> coverage vector.
> 
> 
> [2:text/plain Hide Save:0001-Allow-Edebug-s-instrumentation-to-be-used-for-other-.patch (10kB)]
> 
> >From c0e8293a9b4919b22e3d409acddc9bde49021bc8 Mon Sep 17 00:00:00 2001
> From: Gemini Lasswell <gazally <at> runbox.com>
> Date: Sat, 16 Sep 2017 14:23:35 -0700
> Subject: [PATCH 1/2] Allow Edebug's instrumentation to be used for other
>  purposes
> 
> * lisp/emacs-lisp/edebug.el:
> (edebug-after-instrumentation-functions)
> (edebug-new-definition-functions): New hook variables.
> (edebug-behavior-alist): New variable.
> (edebug-read-and-maybe-wrap-form): Run a hook after a form is
> wrapped.
> (edebug-make-form-wrapper): Run a hook after a definition is
> wrapped.
> (edebug-default-enter): New name for edebug-enter.
> (edebug-enter): New function which changes behavior of Edebug based
> on symbol property 'edebug-behavior and edebug-behavior-alist.
> (edebug-run-slow, edebug-run-fast): Modify edebug-behavior-alist.
> ---
>  lisp/emacs-lisp/edebug.el | 154 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 100 insertions(+), 54 deletions(-)
> 
> diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
> index dbc56e272f..ca1f55cb6a 100644
> --- a/lisp/emacs-lisp/edebug.el
> +++ b/lisp/emacs-lisp/edebug.el
> @@ -1065,6 +1065,29 @@ edebug-old-def-name
>  (defvar edebug-error-point nil)
>  (defvar edebug-best-error nil)
>  
> +;; Hooks which may be used to extend Edebug's functionality.  See
> +;; Testcover for an example.
> +(defvar edebug-after-instrumentation-functions nil
> +  "Abnormal hook run on code after instrumentation for debugging.
> +Each function is called with one argument, a form which has just
> +been instrumented for Edebugging.")
> +(defvar edebug-new-definition-functions '(edebug-announce-definition)
> +  "Abnormal hook run after Edebug wraps a new definition.
> +After Edebug has initialized its own data, each hook function is
> +called with one argument, the symbol associated with the
> +definition, which may be the actual symbol defined or one
> +generated by Edebug.")

These hooks should be documented in the ELisp manual and in NEWS.

Also, please have an empty line between the defvar's.

> @@ -1330,10 +1354,7 @@ edebug-make-form-wrapper
>  	 form-data-entry edebug-def-name ;; in case name is changed
>  	 form-begin form-end))
>  
> -      ;;    (message "defining: %s" edebug-def-name) (sit-for 2)
>        (edebug-make-top-form-data-entry form-data-entry)
> -      (message "Edebug: %s" edebug-def-name)
> -      ;;(debug edebug-def-name)

Why are you removing this?  It looks like debugging code that could be
useful to someone, no?

> +(defun edebug-announce-definition (def-name)
> +  "Announce Edebug's processing of DEF-NAME."
> +  (message "Edebug: %s" def-name))

This new function is not mentioned in the commit log message.

> -(defun edebug-enter (function args body)
> +(defun edebug-enter (func args body)

This function is indicated as "new" in the commit log, but it isn't
new.

> +(defalias 'edebug-before nil
> +  "Function called by Edebug before a form is evaluated.
> +See `edebug-behavior-alist' for implementations.")
> +(defalias 'edebug-after nil
> +  "Function called by Edebug after a form is evaluated.
> +See `edebug-behavior-alist' for implementations.")

These are not in the commit log.

> +** Testcover
> +
> +---
> +*** If you've tried to use Testcover before and were blocked by
> +unhelpful error messages, give it another try.  Many bugs have been
> +fixed.

Please make this a more informative entry.  I would remove the 1st
sentence, and instead describe what new features are available and
what could users do with them.  Since this is not documented in the
manuals (and you suggest to leave it at that with the "---" marker),
the NEWS entry should do a better job describing the changes.  (You
don't need to describe which bugs were fixed, only the new and changed
behavior.)

> +;;; Coverage Analysis
> +
> +;; The top level function for initializing code coverage is
> +;; `testcover-analyze-coverage', which recursively walks the form it is
> +;; passed, which should have already been instrumented by
> +;; edebug-read-and-maybe-wrap-form, and initializes the associated
> +;; code coverage vectors, which should have already been created by
> +;; `edebug-clear-coverage'.
> +;;
> +;; The purpose of the analysis is to identify forms which can only
> +;; ever return a single value. These forms can be considered to have
> +;; adequate code coverage even if only executed once. In addition,
> +;; forms which will never return, such as error signals, can be
> +;; identified and treated correctly.

This text uses one space between sentences, whereas our convention is
to use 2.

Thanks again for working on this.

Message #16 received at 25316 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 25316 <at> debbugs.gnu.org
Subject: Re: bug#25316: Patch for bug#25316: 26.0.50;
 Bugs in testcover-reinstrument
Date: Sat, 30 Sep 2017 10:40:09 -0400

Gemini Lasswell <gazally <at> runbox.com> writes:

> (edebug-enter): New function which changes behavior of Edebug based
> on symbol property 'edebug-behavior and edebug-behavior-alist.

Those should be wrapped in quotes as in `edebug-behavior' and
`edebug-behavior-alist'.

> -(defun edebug-enter (function args body)
> +(defun edebug-enter (func args body)
> +  "Enter Edebug for a function.
> +FUNC should be the symbol with the Edebug information, ARGS is
> +the list of arguments and BODY is the code.
> +
> +Look up the `edebug-behavior' for FUNC in `edebug-behavior-alist'
> +and run its entry function, and set up `edebug-before' and
> +`edebug-after'."
> +  (cl-letf* ((behavior (get func 'edebug-behavior))
> +             (functions (cdr (assoc behavior edebug-behavior-alist)))
> +             ((symbol-function #'edebug-before) (nth 1 functions))
> +             ((symbol-function #'edebug-after) (nth 2 functions)))
> +    (funcall (nth 0 functions) func args body)))

Overriding the function-definition of edebug-before and edebug-after
doesn't seem very clean.  It would be better to have an
edebug-before-function which is `funcall'ed I think (I know your patch
didn't introduce this, it just makes it more obvious.  Perhaps it could
be addressed later).

>  (defun edebug-run-slow ()
> -  (defalias 'edebug-before 'edebug-slow-before)
> -  (defalias 'edebug-after 'edebug-slow-after))
> +  "Set up Edebug's normal behavior."
> +  (setf (cdr (assq 'edebug edebug-behavior-alist))
> +        '(edebug-default-enter edebug-slow-before edebug-slow-after)))


> +(defun testcover-analyze-coverage (form)

> +  (pcase form

> +    (`(quote . ,_)
> +     ;; A quoted form is 1value. Edebug could have instrumented
> +     ;; something inside the form if an Edebug spec contained a quote.
> +     ;; It's also possible that the quoted form is a circular object.
> +     ;; To avoid infinite recursion, don't examine quoted objects.
> +     ;; This will cause the coverage marks on an instrumented quoted
> +     ;; form to look odd. See bug#25316.
> +     '1value)

Missed double spacing again.

> +    ((or `(\` ,bq-form) `(\` . ,bq-form))

Isn't only the first of these is needed?  (read "`foo") ;=> (\` foo)

> +(defun testcover-analyze-coverage-wrapped-form (form)

> +  (pcase form

> +    ((or `(\` ,bq-form) `(\` . ,bq-form))

As above.

Message #19 received at 25316 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 25316 <at> debbugs.gnu.org
Subject: Re: bug#25316: Patch for bug#25316: 26.0.50;
 Bugs in testcover-reinstrument
Date: Tue, 03 Oct 2017 10:32:55 -0700

Noam Postavsky writes:

> Overriding the function-definition of edebug-before and edebug-after
> doesn't seem very clean.  It would be better to have an
> edebug-before-function which is `funcall'ed I think (I know your patch
> didn't introduce this, it just makes it more obvious.  Perhaps it could
> be addressed later).

An advantage to using an overridden function-definition is that it makes
backtraces of instrumented code easier to read.

>> +    ((or `(\` ,bq-form) `(\` . ,bq-form))
>
> Isn't only the first of these is needed?  (read "`foo") ;=> (\` foo)

Yes, only the first is needed, so I'll fix this in both places.

Message #22 received at 25316 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 25316 <at> debbugs.gnu.org
Subject: Re: bug#25316: Patch for bug#25316: 26.0.50;
 Bugs in testcover-reinstrument
Date: Tue, 03 Oct 2017 12:17:50 -0700

[Message part 1 (text/plain, inline)]

Eli Zaretskii writes:

> Thanks, I have a few comments below.  Please let others to comment for
> a week or so before installing on master.
>
Here are revised patches with your comments incorporated. I've removed
the NEWS entry for Testcover because there aren't any new features to
document, just old features that are more likely to work correctly.

[0001-Allow-Edebug-s-instrumentation-to-be-used-for-other-.patch (text/plain, attachment)]

[0002-Rewrite-Testcover-s-internals-fixing-several-bugs.patch (text/plain, attachment)]

Message #25 received at 25316 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 25316 <at> debbugs.gnu.org
Subject: Re: bug#25316: Patch for bug#25316: 26.0.50;
 Bugs in testcover-reinstrument
Date: Wed, 04 Oct 2017 08:55:32 +0300

> From: Gemini Lasswell <gazally <at> runbox.com>
> Cc: 25316 <at> debbugs.gnu.org
> Date: Tue, 03 Oct 2017 12:17:50 -0700
> 
> Eli Zaretskii writes:
> 
> > Thanks, I have a few comments below.  Please let others to comment for
> > a week or so before installing on master.
> >
> Here are revised patches with your comments incorporated. I've removed
> the NEWS entry for Testcover because there aren't any new features to
> document, just old features that are more likely to work correctly.

Thanks, this LGTM.

Message #30 received at 25316-done <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: 25316-done <at> debbugs.gnu.org
Subject: Re: bug#25316: Patch for bug#25316: 26.0.50;
 Bugs in testcover-reinstrument
Date: Sun, 08 Oct 2017 16:38:22 -0700

I've pushed these patches to master.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.