bug#3892: corrupting load-in-progress value with "let"

bug#3892: corrupting load-in-progress value with "let"

[Ken Raeburn]

With these source files:

==> test.el <==
(message "test:  load-in-progress %S" load-in-progress)
(let ((load-path (cons "." load-path)))
   (load "test2"))
(message "test:  load-in-progress %S" load-in-progress)

==> test2.el <==
(message "test2: load-in-progress %S" load-in-progress)
(load "test3")
(message "test2: load-in-progress %S" load-in-progress)

==> test3.el <==
(message "test3: load-in-progress %S" load-in-progress)

and test.el and test2.el byte-compiled but test3.el only present in  
source form, the resulting output shows that load-in-progress is  
inconsistent:

% ./b/Inst/bin/emacs --batch -l test.elc
test:  load-in-progress t
Loading test2...
test2: load-in-progress t
Loading /tmp/emacs-23.0.96/test3.el (source)...
test3: load-in-progress t
test2: load-in-progress t
test:  load-in-progress nil
%

I'm working with the CVS sources and 23.0.96 prerelease code, but can  
reproduce the behavior in 22.1 as shipped with Mac OS X.

The variable load-in-progress is defined in the C code with  
DEFVAR_BOOL applied to an int variable.  Since file loading can be  
invoked recursively, the int value is incremented and decremented when  
loading begins and ends, and should in theory be zero only when no  
file is being loaded.

However, if we're loading a .el file from source, the C code in  
lread.c:Fload calls out to the load-source-file-function, which winds  
up calling code in mule.el, which uses "let" on load-in-progress.  The  
let/unwind support doesn't save and restore the integer value for a  
Lisp_Misc_Boolfwd variable, just the boolean state.  So after loading  
of test3.el finishes above, load-in-progress is restored from its old  
*boolean* value, and gets the value 1 instead of 2 as it should have.   
When test2.elc is done loading, it drops to zero and it looks like  
we're not currently loading any files, even though we're in the middle  
of loading test.elc still.

There is code (in C mode, among others) which checks whether a file is  
being loaded, so this can have a behavioral impact under certain  
conditions.  I haven't actually triggered the problem in my normal  
usage patterns though.

On the assumption that DEFVAR_BOOL variables really ought to just be  
used as booleans (I haven't checked other boolean variables though),  
and we don't want to change the Lisp-visible binding to an integer (or  
"if load-in-progress" would stop working right), I'm working on a  
patch to make load-in-progress an actual boolean, and put the file- 
loading depth counter elsewhere, inaccessible to Lisp (since it's  
inaccessible now).

Ken

bug#3892: corrupting load-in-progress value with "let"

[Stefan Monnier]

> The variable load-in-progress is defined in the C code with DEFVAR_BOOL
> applied to an int variable.  Since file loading can be  invoked recursively,
> the int value is incremented and decremented when  loading begins and ends,
> and should in theory be zero only when no  file is being loaded.

> However, if we're loading a .el file from source, the C code in
> lread.c:Fload calls out to the load-source-file-function, which winds  up
> calling code in mule.el, which uses "let" on load-in-progress.
> The  let/unwind support doesn't save and restore the integer value for
> a  Lisp_Misc_Boolfwd variable, just the boolean state.  So after loading  of
> test3.el finishes above, load-in-progress is restored from its old
> *boolean* value, and gets the value 1 instead of 2 as it should have.
> When test2.elc is done loading, it drops to zero and it looks like  we're
> not currently loading any files, even though we're in the middle  of loading
> test.elc still.

Good catch.

> On the assumption that DEFVAR_BOOL variables really ought to just be used as
> booleans (I haven't checked other boolean variables though),  and we don't
> want to change the Lisp-visible binding to an integer (or "if
> load-in-progress" would stop working right), I'm working on a  patch to make
> load-in-progress an actual boolean, and put the file-
> loading depth counter elsewhere, inaccessible to Lisp (since it's
> inaccessible now).

I think we should drop the counter altogether, and use specbind instead.


        Stefan

bug#3892: corrupting load-in-progress value with "let"

[Ken Raeburn]

On Jul 21, 2009, at 12:27, Stefan Monnier wrote:
> I think we should drop the counter altogether, and use specbind  
> instead.

Much simpler, yes, and it looks like nothing in the C code cares about  
the numeric value either.
Thanks for the suggestion; I'll revise my patch.

Ken

bug#3892: corrupting load-in-progress value with "let"

[Ken Raeburn]

On Jul 21, 2009, at 14:44, Ken Raeburn wrote:
> On Jul 21, 2009, at 12:27, Stefan Monnier wrote:
>> I think we should drop the counter altogether, and use specbind  
>> instead.
>
> Much simpler, yes, and it looks like nothing in the C code cares  
> about the numeric value either.
> Thanks for the suggestion; I'll revise my patch.

I'm working on the revised patch, but it occurs to me that this way,  
some broken bit of code could set load-in-progress to t at some point  
when nothing is being loaded, and it will never become nil again  
unless explicitly reset.  I'm not sure that's right either.  I think I  
kind of like the behavior that while code can temporarily override it,  
the correct state will be restored from the C code any time it changes  
(file loading starts or completes).

A related issue: If we're changing the way it's set, should we retain  
the constraint that load-in-progress can only (appear to) hold boolean  
values, or let it hold any Lisp value instead?

Ken

bug#3892: corrupting load-in-progress value with "let"

[Stefan Monnier]

>>> I think we should drop the counter altogether, and use specbind instead.
>> Much simpler, yes, and it looks like nothing in the C code cares about the
>> numeric value either.
>> Thanks for the suggestion; I'll revise my patch.

> I'm working on the revised patch, but it occurs to me that this way, some
> broken bit of code could set load-in-progress to t at some point  when
> nothing is being loaded, and it will never become nil again  unless
> explicitly reset.

Isn't that a problem that already exists?
In any case such setting would be a bug, so I wouldn't worry about it.

> A related issue: If we're changing the way it's set, should we retain the
> constraint that load-in-progress can only (appear to) hold boolean  values,
> or let it hold any Lisp value instead?

A boolean seems sufficient, I see no need to change it for now,


        Stefan

bug#3892: corrupting load-in-progress value with "let"

[Ken Raeburn]

On Jul 23, 2009, at 21:14, Stefan Monnier wrote:
>> I'm working on the revised patch, but it occurs to me that this  
>> way, some
>> broken bit of code could set load-in-progress to t at some point   
>> when
>> nothing is being loaded, and it will never become nil again  unless
>> explicitly reset.
>
> Isn't that a problem that already exists?

Not exactly... in the released code (and trunk before my first  
change), it can explicitly be set to t at the top level, but it'll be  
reset to nil after loading another file.  And, the bug I was aiming to  
fix initially, the counter (unseen by Lisp) can be set to 1 from some  
higher number during nested loading, causing it to be set to nil when  
returning back to enclosing load calls, but once you get back to the  
top level the nil value is correct, and when the next load starts from  
the top level it'll correctly be set to t (temporarily) again.

So the specbind version would make the first bug a little worse,  
though it fixes the second.

> In any case such setting would be a bug, so I wouldn't worry about it.

Yeah, that's true enough...

>> A related issue: If we're changing the way it's set, should we  
>> retain the
>> constraint that load-in-progress can only (appear to) hold boolean   
>> values,
>> or let it hold any Lisp value instead?
>
> A boolean seems sufficient, I see no need to change it for now,

Okay, it doesn't seem like a big deal to me either way.

Ken

bug#3892: marked as done (corrupting load-in-progress value with "let")

[Emacs bug Tracking System]

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]


Your message dated Sat, 15 Aug 2009 19:10:34 -0400
with message-id <87fxbslllh.fsf@cyd.mit.edu>
and subject line Re: bug#3892: corrupting load-in-progress value with "let"
has caused the Emacs bug report #3892,
regarding corrupting load-in-progress value with "let"
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@emacsbugs.donarmstrong.com
immediately.)


-- 
3892: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=3892
Emacs Bug Tracking System
Contact owner@emacsbugs.donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 4752 bytes --]

From: Ken Raeburn <raeburn@raeburn.org>
To: bug-gnu-emacs@gnu.org
Subject: corrupting load-in-progress value with "let"
Date: Tue, 21 Jul 2009 00:01:12 -0400
Message-ID: <47712C91-B427-4C1C-8731-F8BE9463A4A3@raeburn.org>

With these source files:

==> test.el <==
(message "test:  load-in-progress %S" load-in-progress)
(let ((load-path (cons "." load-path)))
   (load "test2"))
(message "test:  load-in-progress %S" load-in-progress)

==> test2.el <==
(message "test2: load-in-progress %S" load-in-progress)
(load "test3")
(message "test2: load-in-progress %S" load-in-progress)

==> test3.el <==
(message "test3: load-in-progress %S" load-in-progress)

and test.el and test2.el byte-compiled but test3.el only present in  
source form, the resulting output shows that load-in-progress is  
inconsistent:

% ./b/Inst/bin/emacs --batch -l test.elc
test:  load-in-progress t
Loading test2...
test2: load-in-progress t
Loading /tmp/emacs-23.0.96/test3.el (source)...
test3: load-in-progress t
test2: load-in-progress t
test:  load-in-progress nil
%

I'm working with the CVS sources and 23.0.96 prerelease code, but can  
reproduce the behavior in 22.1 as shipped with Mac OS X.

The variable load-in-progress is defined in the C code with  
DEFVAR_BOOL applied to an int variable.  Since file loading can be  
invoked recursively, the int value is incremented and decremented when  
loading begins and ends, and should in theory be zero only when no  
file is being loaded.

However, if we're loading a .el file from source, the C code in  
lread.c:Fload calls out to the load-source-file-function, which winds  
up calling code in mule.el, which uses "let" on load-in-progress.  The  
let/unwind support doesn't save and restore the integer value for a  
Lisp_Misc_Boolfwd variable, just the boolean state.  So after loading  
of test3.el finishes above, load-in-progress is restored from its old  
*boolean* value, and gets the value 1 instead of 2 as it should have.   
When test2.elc is done loading, it drops to zero and it looks like  
we're not currently loading any files, even though we're in the middle  
of loading test.elc still.

There is code (in C mode, among others) which checks whether a file is  
being loaded, so this can have a behavioral impact under certain  
conditions.  I haven't actually triggered the problem in my normal  
usage patterns though.

On the assumption that DEFVAR_BOOL variables really ought to just be  
used as booleans (I haven't checked other boolean variables though),  
and we don't want to change the Lisp-visible binding to an integer (or  
"if load-in-progress" would stop working right), I'm working on a  
patch to make load-in-progress an actual boolean, and put the file- 
loading depth counter elsewhere, inaccessible to Lisp (since it's  
inaccessible now).

Ken



[-- Attachment #3: Type: message/rfc822, Size: 1103 bytes --]

From: Chong Yidong <cyd@stupidchicken.com>
To: 3892-done@emacsbugs.donarmstrong.com
Subject: Re: bug#3892: corrupting load-in-progress value with "let"
Date: Sat, 15 Aug 2009 19:10:34 -0400
Message-ID: <87fxbslllh.fsf@cyd.mit.edu>

Closing the bug (see 2009-07-25 checkin to trunk by Ken Raeburn).