GNU bug report logs - #26837
Wrong file in "autoloading failed to define" error

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 26837 in the body.
You can then email your comments to 26837 AT debbugs.gnu.org in the normal way.

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

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

From: Glenn Morris <rgm <at> gnu.org>
To: submit <at> debbugs.gnu.org
Subject: Wrong file in "autoloading failed to define" error
Date: Mon, 08 May 2017 15:14:44 -0400

Package: emacs
Version: 26.0.50
Severity: minor

cat <<EOF > bar.el
(provide 'bar)
EOF

emacs -Q -L $PWD
(autoload 'bar "bar")
(bar)

 -> (error "Autoloading file /path/to/bar.el failed to define function bar")

which is correct and good.

Now:
C-]   ; exit debugger
(bar) ; repeat

 -> (error "Autoloading file /path/to/help-mode.elc failed to define function bar") 

Ie, the wrong file name is reported after the first time round.


Ref: http://lists.gnu.org/archive/html/emacs-devel/2016-10/msg00668.html

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

From: Glenn Morris <rgm <at> gnu.org>
To: 26837 <at> debbugs.gnu.org
Cc: agrambot <at> gmail.com
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Mon, 08 May 2017 20:04:56 -0400

Am I mistaken in the following patch to fix this, or has load not been
moving an already present file to the start of load-history since c 2005?

--- a/src/lread.c
+++ b/src/lread.c
@@ -1885,7 +1885,7 @@ PREDICATE can also be an integer to pass to the faccessat(2) function,
       /* On the first cycle, we can easily test here
 	 whether we are reading the whole buffer.  */
       if (b && first_sexp)
-	whole_buffer = (PT == BEG && ZV == Z);
+	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
 
       instream = stream;
     read_next:

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

From: Alex <agrambot <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 26837 <at> debbugs.gnu.org
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Tue, 09 May 2017 01:26:59 -0600

Glenn Morris <rgm <at> gnu.org> writes:

> Am I mistaken in the following patch to fix this, or has load not been
> moving an already present file to the start of load-history since c 2005?
>
> --- a/src/lread.c
> +++ b/src/lread.c
> @@ -1885,7 +1885,7 @@ PREDICATE can also be an integer to pass to the faccessat(2) function,
>        /* On the first cycle, we can easily test here
>  	 whether we are reading the whole buffer.  */
>        if (b && first_sexp)
> -	whole_buffer = (PT == BEG && ZV == Z);
> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
>  
>        instream = stream;
>      read_next:

I can't claim to know what is going on within that function, but your
patch seems to only work for some files (e.g. your bar.el). If bar
starts with a comment, then bar isn't moved to the top after evaluating
(load "bar").

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

From: Glenn Morris <rgm <at> gnu.org>
To: Alex <agrambot <at> gmail.com>
Cc: 26837 <at> debbugs.gnu.org
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Tue, 09 May 2017 12:33:42 -0400

Alex wrote:

>> -	whole_buffer = (PT == BEG && ZV == Z);
>> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
[...]
> I can't claim to know what is going on within that function, but your
> patch seems to only work for some files (e.g. your bar.el). If bar
> starts with a comment, then bar isn't moved to the top after evaluating
> (load "bar").

The intent of the code seems to be to only adjust an existing
load-history element for some file if the entire file is being loaded.
I guess in the leading comment case, something else has already moved
point looking for a significant leading comment (eg lexbind, script,
prop-line). Hmmm.

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

From: Glenn Morris <rgm <at> gnu.org>
To: Alex <agrambot <at> gmail.com>
Cc: 26837 <at> debbugs.gnu.org
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Tue, 09 May 2017 14:39:24 -0400

Glenn Morris wrote:

>> I can't claim to know what is going on within that function, but your
>> patch seems to only work for some files (e.g. your bar.el). If bar
>> starts with a comment, then bar isn't moved to the top after evaluating
>> (load "bar").
>
> The intent of the code seems to be to only adjust an existing
> load-history element for some file if the entire file is being loaded.
> I guess in the leading comment case, something else has already moved
> point looking for a significant leading comment (eg lexbind, script,
> prop-line). Hmmm.

I think it was the lexical-binding thing. Following seems to work:

--- i/src/lread.c
+++ w/src/lread.c
@@ -1885,7 +1885,7 @@ PREDICATE can also be an integer to pass to the faccessat(2) function,
       /* On the first cycle, we can easily test here
 	 whether we are reading the whole buffer.  */
       if (b && first_sexp)
-	whole_buffer = (PT == BEG && ZV == Z);
+	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
 
       instream = stream;
     read_next:
@@ -2008,6 +2008,7 @@ BUFFER is the buffer to evaluate (nil means use current buffer),
   record_unwind_protect (save_excursion_restore, save_excursion_save ());
   BUF_TEMP_SET_PT (XBUFFER (buf), BUF_BEGV (XBUFFER (buf)));
   specbind (Qlexical_binding, lisp_file_lexically_bound_p (buf) ? Qt : Qnil);
+  BUF_TEMP_SET_PT (XBUFFER (buf), BUF_BEGV (XBUFFER (buf)));
   readevalloop (buf, 0, filename,
 		!NILP (printflag), unibyte, Qnil, Qnil, Qnil);
   unbind_to (count, Qnil);

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 26837 <at> debbugs.gnu.org, agrambot <at> gmail.com
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Tue, 09 May 2017 22:12:48 +0300

> From: Glenn Morris <rgm <at> gnu.org>
> Date: Tue, 09 May 2017 14:39:24 -0400
> Cc: 26837 <at> debbugs.gnu.org
> 
> I think it was the lexical-binding thing. Following seems to work:
> 
> --- i/src/lread.c
> +++ w/src/lread.c
> @@ -1885,7 +1885,7 @@ PREDICATE can also be an integer to pass to the faccessat(2) function,
>        /* On the first cycle, we can easily test here
>  	 whether we are reading the whole buffer.  */
>        if (b && first_sexp)
> -	whole_buffer = (PT == BEG && ZV == Z);
> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));

You are saying that set_buffer_internal didn't do its job?  Of that
the call was bypassed?

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

From: Glenn Morris <rgm <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 26837 <at> debbugs.gnu.org, agrambot <at> gmail.com
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Tue, 09 May 2017 19:49:17 -0400

Eli Zaretskii wrote:

>> -	whole_buffer = (PT == BEG && ZV == Z);
>> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
>
> You are saying that set_buffer_internal didn't do its job?  Of that
> the call was bypassed?

When START = nil (eg when called from eval-buffer), set_buffer_internal
is never called.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 26837 <at> debbugs.gnu.org, agrambot <at> gmail.com
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Wed, 10 May 2017 05:43:20 +0300

> From: Glenn Morris <rgm <at> gnu.org>
> Cc: agrambot <at> gmail.com,  26837 <at> debbugs.gnu.org
> Date: Tue, 09 May 2017 19:49:17 -0400
> 
> Eli Zaretskii wrote:
> 
> >> -	whole_buffer = (PT == BEG && ZV == Z);
> >> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
> >
> > You are saying that set_buffer_internal didn't do its job?  Of that
> > the call was bypassed?
> 
> When START = nil (eg when called from eval-buffer), set_buffer_internal
> is never called.

What's the use case when this goes together with b not being the
current buffer?

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

From: Glenn Morris <rgm <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 26837 <at> debbugs.gnu.org, agrambot <at> gmail.com
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Wed, 10 May 2017 02:23:28 -0400

Eli Zaretskii wrote:

> What's the use case when this goes together with b not being the
> current buffer?

(load "file.el")

-> load-source-file-function
-> load-with-code-conversion
-> (eval-buffer buffer ...)

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

From: Alex <agrambot <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 26837 <at> debbugs.gnu.org, eliz <at> gnu.org
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Thu, 18 May 2017 14:11:00 -0600

Glenn Morris <rgm <at> gnu.org> writes:

> Glenn Morris wrote:
>
> I think it was the lexical-binding thing. Following seems to work:
>
> --- i/src/lread.c
> +++ w/src/lread.c
> @@ -1885,7 +1885,7 @@ PREDICATE can also be an integer to pass to the faccessat(2) function,
>        /* On the first cycle, we can easily test here
>  	 whether we are reading the whole buffer.  */
>        if (b && first_sexp)
> -	whole_buffer = (PT == BEG && ZV == Z);
> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
>  
>        instream = stream;
>      read_next:
> @@ -2008,6 +2008,7 @@ BUFFER is the buffer to evaluate (nil means use current buffer),
>    record_unwind_protect (save_excursion_restore, save_excursion_save ());
>    BUF_TEMP_SET_PT (XBUFFER (buf), BUF_BEGV (XBUFFER (buf)));
>    specbind (Qlexical_binding, lisp_file_lexically_bound_p (buf) ? Qt : Qnil);
> +  BUF_TEMP_SET_PT (XBUFFER (buf), BUF_BEGV (XBUFFER (buf)));
>    readevalloop (buf, 0, filename,
>  		!NILP (printflag), unibyte, Qnil, Qnil, Qnil);
>    unbind_to (count, Qnil);

That seems to work for me as well. Should it be documented that
reevaluations of a file update the position of the file in load-history?
I assumed this was already the case when making my autoload patch
because of the "history" in load-history, but documentation would help
to make that clear.

PS: I realized that I was a bit careless in using SDATA without checking
that the argument is necessarily a string. I don't think it matters much
if your above patch is applied, but it's probably not a good idea to
leave it in, just in case there's a bug somewhere else in the future.

What do you think about the following diff? This removes the dependency
on load-history altogether:

--- a/src/eval.c
+++ b/src/eval.c
@@ -2021,7 +2021,10 @@ it defines a macro.  */)
 
       if (!NILP (Fequal (fun, fundef)))
        error ("Autoloading file %s failed to define function %s",
-              SDATA (Fcar (Fcar (Vload_history))),
+              SDATA (Flocate_file_internal (Fcar (Fcdr (fundef)),
+                                            Vload_path,
+                                            Vload_suffixes,
+                                            Qnil)),
               SDATA (SYMBOL_NAME (funname)));
       else
        return fun;

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

From: Glenn Morris <rgm <at> gnu.org>
To: Alex <agrambot <at> gmail.com>
Cc: 26837 <at> debbugs.gnu.org, eliz <at> gnu.org
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Sat, 20 May 2017 19:17:30 -0400

Alex wrote:

> That seems to work for me as well. Should it be documented that
> reevaluations of a file update the position of the file in load-history?

I think it's mentioned in the doc of load:

  Loading a file records its definitions, and its `provide' and
  `require' calls, in an element of `load-history' whose
  car is the file name loaded.  See `load-history'.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^
but that did seem to be the only mention I found in a quick search.

> PS: I realized that I was a bit careless in using SDATA without checking
> that the argument is necessarily a string. I don't think it matters much
> if your above patch is applied, but it's probably not a good idea to
> leave it in, just in case there's a bug somewhere else in the future.

In 58326f0 I check if the car is nil (though I don't see how it could be).
Could add the same thing to eval.c.

> What do you think about the following diff? This removes the dependency
> on load-history altogether:
>
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -2021,7 +2021,10 @@ it defines a macro.  */)
>  
>        if (!NILP (Fequal (fun, fundef)))
>         error ("Autoloading file %s failed to define function %s",
> -              SDATA (Fcar (Fcar (Vload_history))),
> +              SDATA (Flocate_file_internal (Fcar (Fcdr (fundef)),
> +                                            Vload_path,
> +                                            Vload_suffixes,
> +                                            Qnil)),
>                SDATA (SYMBOL_NAME (funname)));
>        else
>         return fun;

I don't have an informed opinion right now.

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

From: Glenn Morris <rgm <at> gnu.org>
To: Alex <agrambot <at> gmail.com>
Cc: 26837 <at> debbugs.gnu.org
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Sat, 20 May 2017 19:24:38 -0400

Glenn Morris wrote:

> I think it's mentioned in the doc of load:
>
>   Loading a file records its definitions, and its `provide' and
>   `require' calls, in an element of `load-history' whose
>   car is the file name loaded.  See `load-history'.
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Duh, re-reading that I see it doesn't say what I thought it said.
So I guess it isn't documented, though it was clearly the intent of the
code. I don't know if it should be mentioned explicitly.

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

From: Alex <agrambot <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 26837 <at> debbugs.gnu.org
Subject: Re: bug#26837: Wrong file in "autoloading failed to define" error
Date: Sun, 21 May 2017 15:59:08 -0600

Glenn Morris <rgm <at> gnu.org> writes:

> In 58326f0 I check if the car is nil (though I don't see how it could be).
> Could add the same thing to eval.c.

According to the docstring of load-history:

 As an exception, one of the alist elements may have FILE-NAME nil,
 for symbols and features not associated with any file.

With your proposed patch I don't think it's really an issue anymore, but
previously it could have triggered a segfault under that rare condition.

> I don't know if it should be mentioned explicitly.

If the intention is that someone can expect the position of files in
load-history to be updated so that it provides a history of some sort
(i.e. use (caar load-history) to get the last file evaluated), then I
would think that there should be a line mentioning it somewhere.

Previous Next

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