about the byte-opt.el patch

about the byte-opt.el patch

[Feng Li]

Hi guys,

I built emacs from the cvs trunk today and it crashed at me on startup while
loading recentf-mode. Commenting out recentf-mode in my .emacs would get emacs
up and run, but then windmove.el would crash it when I try to use it. After some
investigation I found that if I revert the recently installed byte-opt.el patch
then everything works just fine. My platform is windows xp sp2, mingw gcc 3.4.5.

Here's my stack trace on crash moment. To make it crash, I just started emacs
with -q, then type "M-x windmove-down".

Microsoft Windows XP [Version 5.1.2600]
(C) Copyright 1985-2001 Microsoft Corp.

C:\projects\emacs\bin>gdb emacs
GNU gdb 6.3
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i686-pc-mingw32"...(no debugging symbols found)

(gdb) r -q
Starting program: C:\projects\emacs\bin/emacs.exe -q
warning: Hooking
warning: C:\PROJECTS\EMACS\BIN\EMACS.EXE
warning: RPH:Injecting code at start up

warning: done...

---Type <return> to continue, or q <return> to quit---

Program received signal SIGTRAP, Trace/breakpoint trap.
0x7c901231 in ntdll!DbgUiConnectToDbg () from ntdll.dll
(gdb) bt
#0  0x7c901231 in ntdll!DbgUiConnectToDbg () from ntdll.dll
#1  0x0113ee5a in w32_abort ()
#2  0x0111505b in Fbyte_code ()
#3  0x0100c324 in funcall_lambda ()
#4  0x0100c776 in Ffuncall ()
#5  0x0111507f in Fbyte_code ()
#6  0x0100c324 in funcall_lambda ()
#7  0x0100c776 in Ffuncall ()
#8  0x0111507f in Fbyte_code ()
#9  0x0100c324 in funcall_lambda ()
#10 0x0100c776 in Ffuncall ()
#11 0x0111507f in Fbyte_code ()
#12 0x0100c324 in funcall_lambda ()
#13 0x0100c776 in Ffuncall ()
#14 0x01113ccf in Fcall_interactively ()
#15 0x01057f86 in Fcommand_execute ()
#16 0x01058190 in Fexecute_extended_command ()
#17 0x0100c995 in Ffuncall ()
#18 0x01113ccf in Fcall_interactively ()
#19 0x01057f86 in Fcommand_execute ()
#20 0x0105ee9d in command_loop_1 ()
#21 0x0100aab4 in internal_condition_case ()
#22 0x0105295e in command_loop_2 ()
---Type <return> to continue, or q <return> to quit---
#23 0x0100a9d9 in internal_catch ()
#24 0x0105276e in command_loop ()
#25 0x0105280f in recursive_edit_1 ()
#26 0x0105290c in Frecursive_edit ()
#27 0x01002aba in main ()
(gdb)

Re: about the byte-opt.el patch

[Richard Stallman]

Would someone please revert that byte-opt patch?
Then please ack.

Re: about the byte-opt.el patch

[Thien-Thi Nguyen]

() Feng Li <fengli@gmail.com>
() Thu, 5 Jul 2007 00:04:30 +0000 (UTC)

   I built emacs from the cvs trunk today 

what commands did you use, precisely?

   and it crashed at me on startup while
   loading recentf-mode

with env var "CVSREAD" set to "1" and
using "cvs update
       && mv /tmp/byte-opt.el-1.94 lisp/emacs-lisp/byte-opt.el
       && ./configure --prefix ~/local
       && make clean
       && rm -f lisp/loaddefs.el
       && make bootstrap"
(these steps take into account the request by rms (in another message) to
revert byte-opt.el), i see a different error:

[...]
Generating autoloads for ps-bdf.el...
Generating autoloads for ps-bdf.el...done
Generating autoloads for progmodes/ps-mode.el...
Generating autoloads for progmodes/ps-mode.el...done
Generating autoloads for ps-mule.el...
Autoloads file /home/ttn/build/GNU/emacs/lisp/ps-print.el is not writable
make[2]: *** [autoloads] Error 255
make[2]: Leaving directory `/home/ttn/build/GNU/emacs/lisp'

at the time of error, there is not even a single .elc file in the tree,
so i surmise that if byte-opt.el is a problem, it is only the tail end
of a series of other problems.  thus (to rms), i don't think reverting
it will be useful.

the message "Autoloads file .../ps-print.el is not writable" is strange;
the "autoloads file" should be loaddefs.el.  sounds like some kind of
internal corruption.

can you do a fresh cvs update and build using the above steps?  i'd like
to know if the error i see is reproducible.  if not, could you describe
what you see?

thi

Re: about the byte-opt.el patch

[Stefan Monnier]

> with env var "CVSREAD" set to "1" and

That's the problem.

> the message "Autoloads file .../ps-print.el is not writable" is strange;
> the "autoloads file" should be loaddefs.el.

Not any more.  Autoload entries can now be redirected to some other file, so
ps-mule.el's autoloads end up in ps-print.el (where they used to be written
by hand instead), and similarly for cl-*.el which end up in cl-loaddefs.el.


        Stefan

Re: about the byte-opt.el patch

[Thien-Thi Nguyen]

() Stefan Monnier <monnier@iro.umontreal.ca>
() Thu, 05 Jul 2007 13:17:10 -0400

   > with env var "CVSREAD" set to "1" and

   That's the problem. [...] Autoload entries can now be redirected to
   some other file, so ps-mule.el's autoloads end up in ps-print.el
   (where they used to be written by hand instead), and similarly for
   cl-*.el which end up in cl-loaddefs.el.

grumble.

ok, now w/ the latest Makefile.in (which does chmod +w on those files),
i am able to build from "make bootstrap" and:

 - not reproduce the crash w/ byte-opt.el pre-change
 - yes reproduce the crash w/ byte-opt.el post-change

i have reverted byte-opt.el.  to OP: sorry for the noise.  please
disregard previous requests for additional info (but feel free to
share any insights you have on the problem, anyway :-).

thi

Re: about the byte-opt.el patch

[Eli Zaretskii]

> From: Thien-Thi Nguyen <ttn@gnuvola.org>
> Date: Thu, 05 Jul 2007 22:46:46 +0200
> Cc: Feng Li <fengli@gmail.com>, emacs-devel@gnu.org
> 
> the latest Makefile.in (which does chmod +w on those files),

Is this change really a good idea?  It defeats the purpose of CVSREAD,
doesn't it?  Perhaps it is better to change autoload.el so that it can
update even read-only files, and preserve the read-only attribute of a
file after it's finished updating its autoload cookies.  WDYT?

File-specific autoloads (was: about the byte-opt.el patch)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 05 Jul 2007 13:17:10 -0400
> Cc: Feng Li <fengli@gmail.com>, emacs-devel@gnu.org
> 
> Autoload entries can now be redirected to some other file, so
> ps-mule.el's autoloads end up in ps-print.el (where they used to be written
> by hand instead), and similarly for cl-*.el which end up in cl-loaddefs.el.

Because of this change, "cvs up" now shows ps-print.el and
cl-loaddefs.el as modified, which might cause trouble if, say,
ps-print.el is ever modified in the repository.

Re: about the byte-opt.el patch

[Thien-Thi Nguyen]

() Eli Zaretskii <eliz@gnu.org>
() Fri, 06 Jul 2007 12:50:05 +0300

   Is this change really a good idea?  It defeats the purpose of CVSREAD,
   doesn't it?

for a handful of files, yes.  and only yes, partially.  i use CVSREAD=1
primarily to be able to categorically determine that some file has not
been munged by me, accidentally.  in the case of "make bootstrap", file
munging is intentional...

   Perhaps it is better to change autoload.el so that it can
   update even read-only files, and preserve the read-only attribute of a
   file after it's finished updating its autoload cookies.  WDYT?

...and furthermore, if it is munged, i don't want it to appear (on quick
scan) unmunged.  now, if the munging produces no change in the text;
then that would be a good reason to chmod it back to read-only.  if such
"move-if-changed" checking can be added to autoload then i would support
having autoload deal w/ read-only files.

re-reading what i wrote above, it doesn't seem so clear (even to me)
what my thoughts are.  so let's just let the brain rest and give the
lazy soul some floor: why don't we leave it like so for now and revisit
this later if someone complains.

thi

Re: about the byte-opt.el patch

[Stefan Monnier]

> Is this change really a good idea?  It defeats the purpose of CVSREAD,
> doesn't it?

Actually, no it doesn't.  "chmod +w" in such a context is more or less like
a "checkout" in RCS, so it's about as close to the right thing as we
can get.  Unless of course the file is *not* changed.

> Perhaps it is better to change autoload.el so that it can
> update even read-only files, and preserve the read-only attribute of a
> file after it's finished updating its autoload cookies.  WDYT?

I think that would be the wrong thing to do since you'd end up with a file
still read-only but modified.


        Stefan

Re: File-specific autoloads

[Thien-Thi Nguyen]

() Eli Zaretskii <eliz@gnu.org>
() Fri, 06 Jul 2007 13:53:33 +0300

   Because of this change, "cvs up" now shows ps-print.el and
   cl-loaddefs.el as modified, which might cause trouble if, say,
   ps-print.el is ever modified in the repository.

brief experiments w/ chmod +w; touch; rewrite (random mod plus undo
plus save) -- all do not result in "cvs up" displaying "M filename".
this is w/ CVSREAD=1 and cvs --version 1.11.22.  ymmv.

but regardless of cvs version quirks, let's look at the nature of the
changes: autoload processing changes a specified region; programmers
should not change that region manually.  i think possible conflict (as
evidenced by a "C" line on "cvs up") is unlikely.  any such conflict
would be good to know about.

thi

Re: File-specific autoloads

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Thien-Thi Nguyen <ttn@gnuvola.org>
> Date: Fri, 06 Jul 2007 16:02:59 +0200
> 
> () Eli Zaretskii <eliz@gnu.org>
> () Fri, 06 Jul 2007 13:53:33 +0300
> 
>    Because of this change, "cvs up" now shows ps-print.el and
>    cl-loaddefs.el as modified, which might cause trouble if, say,
>    ps-print.el is ever modified in the repository.
> 
> brief experiments w/ chmod +w; touch; rewrite (random mod plus undo
> plus save) -- all do not result in "cvs up" displaying "M filename".

That's an incorrect emulation of what "make autoloads" does: it _does_
modify the file.  At least in my case, it did (I verified that with
"cvs diff").

> but regardless of cvs version quirks, let's look at the nature of the
> changes: autoload processing changes a specified region; programmers
> should not change that region manually.

And we want to rely on that?

Anyway, seeing those "M ps-print.el" lines in the output of "cvs up"
is extremely annoying, because I'm used to take that as a sign that I
have uncommitted changes in my sandbox.  It also breaks the principle
that files that are rewritten locally as part of the build process are
not kept in CVS.

So I think this change in its current incarnation is for the worse.
Maybe if the autoloads were written into files that are not in CVS I'd
be happier.

Re: File-specific autoloads

[Thien-Thi Nguyen]

() Eli Zaretskii <eliz@gnu.org>
() Fri, 06 Jul 2007 19:04:53 +0300

   That's an incorrect emulation of what "make autoloads" does: it _does_
   modify the file.  At least in my case, it did (I verified that with
   "cvs diff").

that's fine (no argument).  we discuss that case below:

   > but regardless of cvs version quirks, let's look at the nature of the
   > changes: autoload processing changes a specified region; programmers
   > should not change that region manually.

   And we want to rely on that?

we want to rely on it inasmuch as we designed it that way.  if our design
changes so that mixing autoload-processing regions and programmer regions is
encouraged, then that would influence our downstream reliance.  however, i see
from cvs annotate autoload.el:

1.58         (rms      05-Aug-97): (defvar generated-autoload-file "loaddefs.el"
1.58         (rms      05-Aug-97):    "*File \\[update-file-autoloads] puts autoloads into.
1.58         (rms      05-Aug-97): A `.el' file can set this in its local variables section to make its
1.71         (kwzh     27-Jun-99): autoloads go somewhere else.  The autoload file is assumed to contain a
1.71         (kwzh     27-Jun-99): trailer starting with a FormFeed character.")

which leads me to believe that autoload processing is not prone to any radical
changes since it has worked for quite a while now (i could be wrong).

   Anyway, seeing those "M ps-print.el" lines in the output of "cvs up"
   is extremely annoying, because I'm used to take that as a sign that I
   have uncommitted changes in my sandbox.  It also breaks the principle
   that files that are rewritten locally as part of the build process are
   not kept in CVS.

these are separate issues that should be addressed separately.  one
approach to resolve the M status would be to request maintainers to
regenerate and check in their newest ps-print.el or cl-loaddefs.el
(regenerated) whenever any of the files that they serve as the autoload
file for are changed.  this is similar to the convention of checking in
configure (regenerated) as well as configure.in.

   So I think this change in its current incarnation is for the worse.
   Maybe if the autoloads were written into files that are not in CVS
   I'd be happier.

cvs already contains one generated file (configure script) and possibly
others.  i personally dislike that practice as well, but since we have
already done the head-scratching for that file, we might as well apply
the lessons to this situation.

thi

Re: File-specific autoloads

[Stefan Monnier]

> Anyway, seeing those "M ps-print.el" lines in the output of "cvs up"
> is extremely annoying, because I'm used to take that as a sign that I
> have uncommitted changes in my sandbox.  It also breaks the principle
> that files that are rewritten locally as part of the build process are
> not kept in CVS.

> So I think this change in its current incarnation is for the worse.
> Maybe if the autoloads were written into files that are not in CVS I'd
> be happier.

Indeed, there's a problem with the current situation.  I have an idea to
solve this problem, I'll install a patch shortly,


        Stefan

Re: File-specific autoloads

[Stefan Monnier]

> So I think this change in its current incarnation is for the worse.
> Maybe if the autoloads were written into files that are not in CVS I'd
> be happier.

I believe the patch below which I've just installed will fix those problems
by using MD5 checkums rather than file time-stamps in
ps-print.el's autoloads.


        Stefan


--- autoload.el	26 jun 2007 15:44:58 -0400	1.126
+++ autoload.el	07 jui 2007 00:48:13 -0400	
@@ -409,6 +409,9 @@
                   (forward-line 1))))))
 
           (when output-start
+            (let ((secondary-autoloads-file-buf
+                   (if (local-variable-p 'generated-autoload-file)
+                       (current-buffer))))
             (with-current-buffer outbuf
               (save-excursion
                 ;; Insert the section-header line which lists the file name
@@ -416,9 +419,23 @@
                 (goto-char output-start)
                 (autoload-insert-section-header
                  outbuf autoloads-done load-name relfile
-                 (nth 5 (file-attributes relfile)))
+                   (if secondary-autoloads-file-buf
+                       ;; MD5 checksums are much better because they do not
+                       ;; change unless the file changes (so they'll be
+                       ;; equal on two different systems and will change
+                       ;; less often than time-stamps, thus leading to fewer
+                       ;; unneeded changes causing spurious conflicts), but
+                       ;; using time-stamps is a very useful optimization,
+                       ;; so we use time-stamps for the main autoloads file
+                       ;; (loaddefs.el) where we have special ways to
+                       ;; circumvent the "random change problem", and MD5
+                       ;; checksum in secondary autoload files where we do
+                       ;; not need the time-stamp optimization because it is
+                       ;; already provided by the primary autoloads file.
+                       (md5 secondary-autoloads-file-buf nil nil 'emacs-mule)
+                     (nth 5 (file-attributes relfile))))
                 (insert ";;; Generated autoloads from " relfile "\n"))
-              (insert generate-autoload-section-trailer)))
+                (insert generate-autoload-section-trailer))))
           (message "Generating autoloads for %s...done" file))
         (or visited
             ;; We created this buffer, so we should kill it.
@@ -454,13 +471,11 @@
 FILE is the file name of the current buffer.
 Returns a buffer whose point is placed at the requested location.
 Returns nil if the file's autoloads are uptodate, otherwise
-removes any prior now out-of-date autoload entries.
-The current buffer only matters if it is visiting a file or if it has a buffer-local
-value for some variables such as `generated-autoload-file', so it's OK
-to call it from a dummy buffer if FILE is not currently visited."
+removes any prior now out-of-date autoload entries."
   (catch 'up-to-date
-    (let ((load-name (autoload-file-load-name file))
-          (existing-buffer (if buffer-file-name (current-buffer)))
+    (let* ((load-name (autoload-file-load-name file))
+           (buf (current-buffer))
+           (existing-buffer (if buffer-file-name buf))
           (found nil))
       (with-current-buffer
           ;; We must read/write the file without any code conversion,
@@ -489,8 +504,16 @@
                          (file-time (nth 5 (file-attributes file))))
                      (if (and (or (null existing-buffer)
                                   (not (buffer-modified-p existing-buffer)))
-                              (listp last-time) (= (length last-time) 2)
+                              (or
+                               ;; last-time is the time-stamp (specifying
+                               ;; the last time we looked at the file) and
+                               ;; the file hasn't been changed since.
+                               (and (listp last-time) (= (length last-time) 2)
                               (not (time-less-p last-time file-time)))
+                               ;; last-time is an MD5 checksum instead.
+                               (and (stringp last-time)
+                                    (equal last-time
+                                           (md5 buf nil nil 'emacs-mule)))))
                          (throw 'up-to-date nil)
                        (autoload-remove-section begin)
                        (setq found t))))

Re: File-specific autoloads

[Eli Zaretskii]

> Cc: Thien-Thi Nguyen <ttn@gnuvola.org>,  emacs-devel@gnu.org
> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 06 Jul 2007 21:32:23 -0400
> 
> Indeed, there's a problem with the current situation.  I have an idea to
> solve this problem, I'll install a patch shortly,

Thank you.

Re: File-specific autoloads

[Eli Zaretskii]

> Cc: Thien-Thi Nguyen <ttn@gnuvola.org>,  emacs-devel@gnu.org
> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 07 Jul 2007 00:55:43 -0400
> 
> I believe the patch below which I've just installed will fix those problems
> by using MD5 checkums rather than file time-stamps in
> ps-print.el's autoloads.

It works for me, thanks.