Re: Bisecting display bugs

[Eli Zaretskii]

> From: Clément Pit--Claudel <address@hidden>
> Date: Mon, 11 Jul 2016 01:55:31 +0200
> 
> I've written up notes while bisecting a display bug (#23938) on master.  
> Since 
> it didn't seem easy to identify the problem automatically from Elisp, I used 
> a 
> small `git bisect run' script that automatically captured, cropped, and 
> compared screenshots of Emacs against a known-good reference.  All in all, 
> things worked nicely: git bisect took 4 hours to complete, but it didn't 
> require any interaction after starting.
> 
> I attached my notes.  Is there interest in including them in admin/notes/?

Thank you for doing this.  I'm okay with adding this kind of notes,
but I have some comments about the contents.

> +For general information about bisecting, use ‘M-x man git-bisect’.
                                                 ^^^^^^^^^^^^^^^^^^
This doesn't work on Windows (the Windows port of Git doesn't install
man pages), suggest using "git help bisect" instead.

> +* Bisecting Emacs bugs

This section doesn't say anything "git help bisect" doesn't.  It is
not Emacs-specific.  I'm not sure we should repeat the text that is
quite clear in the Git documentation, and also includes useful
examples.

> +it's a new bug, a not-too-recent revision could do; otherwise,
> +checking for the bug previous releases of Emacs works well.
                      ^^^
"in" missing here.

> +** Automating ‘git bisect’
> +
> +You can also write ‘git bisect run <some-shell-script>’ to automate
> +the process.  ‘<some-shell-script>’ should build Emacs, run your bug
> +reproduction test, and return 0 if the current revision is good, and 1
> +otherwise.  Returning 125 is equivalent to doing ‘git bisect skip’.

This part is also described on the Git bisect man page.

> +Concretely, ‘<some-shell-script>’ usually looks like this:
> +
> +    #!/usr/bin/env bash
> +
> +    # Remove leftovers from previous runs
> +    git clean -xfd > /dev/null
> +    # Build Emacs and skip commit if build fails
> +    (./autogen.sh && ./configure --cache-file=/tmp/emacs.config.cache && 
> make -j4) || exit 125
> +
> +    # Reproduce the bug, writing output somewhere
> +    src/emacs -Q -l "../reproduce-the-bug.el" || exit 125
> +    # Remove leftovers again
> +    git clean -xfd > /dev/null
> +    # Analyze output and produce exit code
> +    cmp ../reference-output current-output

IMO, this template could benefit from more generality.  The result
doesn't have to come from 'cmp', it can come from 'diff' or some other
program, like 'cat' or 'ls'.  It can also come from the exit code
produced by Emacs itself (which is probably a whole lot more
convenient when possible).  Finally, what about bugs that cause Emacs
to crash?  I think the template or the notes that come with it should
include some advice on that.

> +Some caveats:
> +
> +- This script cleans Emacs' source directory with ‘git clean -xfd’, so
> +  make sure your uncommitted changes are saved somewhere else.

While bootstrapping after each step of bisect is safe, it makes the
run much longer, so I'd try bisecting without a bootstrap first,
especially if the range of commits to bisect is relatively small.

> +** Using ‘git bisect’ to find display-related bugs
> +
> +Most bugs that manifest graphically can be checked for by
> +programmatically inspecting text properties

Not just text properties: also the position of point, the text itself,
the window start position, and much more.  I find the lack of details
in this part to be quite a gap, since familiarity with the respective
facilities is an important part of being efficient in debugging
display problems.

Specifically, I'd like to see at least the following facilities
mentioned here:

 . window-start and window-end
 . posn-at-point
 . pos-visible-in-window-p
 . frame-geometry
 . window--dump-frame
 . frame--size-history
 . display-monitor-attributes-list
 . C-u C-x =
 . trace-redisplay and trace-to-stderr
 . dump-glyph-matrix and dump-frame-glyph-matrix

Each one of these should be accompanied with a short (1-2 lines)
description of what it does and when is it useful.

>                                              but some are only
> +apparent through visual inspection.  Since building Emacs takes a long
> +time, it can be a pain to debug these manually.

I don't follow this logic: building is an automated process, while
visual inspection is usually very fast; and the automated comparison
you are about to suggest doesn't decrease the build time.  So how does
the conclusion follow?

> +Fortunately, it's relatively easy to bisect such bugs automatically.

This is too optimistic, IMO.  I would reword this in a more cautious
way, something like:

  If the display bug has a clear manifestation in a screenshot of a
  particular portion of Emacs display, and you have a program, like
  'xwd', that can capture the content of the Emacs frame, and also
  have ImageMagick installed, you can automate the comparison of the
  redisplay results to make the bisection process fully automatic.

This technique, while valuable in certain situations, has quite a few
caveats that make it inappropriate in other situations, so we cannot
tell users "here's your magic wand".  One important caveat is that the
size of an Emacs frame or window might not be constant across
revisions, so cropping the images correctly is not that trivial.
Another example of a caveat is when we deliberately change the visual
appearance of a part of the Emacs display (such as the mode line or
the tool bar) -- in these cases the baseline screenshot produced from
a "good" build will no longer be useful.  And there are more caveats,
so being excessively optimistic when recommending this technique might
be a disservice to the reader.

> +      (let ((window-id (frame-parameter nil 'outer-window-id)))
> +        (call-process "xwd" nil nil nil "-silent" "-id" window-id "-out" 
> fname))
> +      (call-process "mogrify" nil nil nil fname "-crop" (format 
> "%dx%d+%d+%d" w h x y))
> +      (kill-emacs))
> +
> +    (defun main ()
> +      ;; Reproduce your bug here
> +      …
> +      ;; Force a redisplay
> +      (redisplay t)
> +      ;; Insert rough X, Y, W, H values below
> +      (run-with-timer 0 nil #'take-screenshot-and-exit "screenshot.xwd" … … 
> … …))
> +
> +    (main)
> +
> +This script makes a screenshot of Emacs after reproducing your bug (if
> +‘xwd’ isn't available, you can use ImageMagick's ‘import’ tool,
> +passing it a ‘-window’ argument where ‘xwd’ wants ‘id’).  Cropping the
> +image is useful to weed out unrelated display changes; try to include
> +only a small portion of the screen containing your bug.
> +
> +Then to produce the right exit code use ImageMagick's ‘identify’ tool:
> +
> +    cmp <(identify -quiet -format "%#" "../screenshot.xwd") <(identify 
> -quiet -format "%#" "../good.xwd")

If you're OK with invoking external programs from within Emacs, why
not compare the results using Emacs capabilities, and produce the
output via the exit code (kill-emacs can instruct Emacs to return an
exit code to the parent shell)?  This has 2 advantages: (1) it doesn't
require the reader to be too proficient in Posix shell scripting, and
(2) the resulting script will be much more portable.

> --- a/admin/notes/repo
> +++ b/admin/notes/repo
> @@ -114,8 +114,7 @@ again.
 
>  * Bisecting
>  
> -This is a semi-automated way to find the revision that introduced a bug.
> -Browse 'git help bisect' for technical instructions.
> +See admin/notes/bisecting.

I wouldn't delete those 2 lines, they are useful for the reader to
decide whether she wants to read the details about bisecting.

Thanks again for working on this.