C-g crash redux

C-g crash redux

[Romain Francoise]

I can crash Emacs reliably with current (Aug 2nd) sources by hitting C-g
while it's preparing a partial-completion-mode prompt.  I just did it
three times in a row.  The Lisp backtrace shows that it's indeed related
to sit-for:

| (gdb) xbacktrace
| "sit-for" (0x10)
| "PC-temp-minibuffer-message" (0x874d3fb)
| "PC-do-completion" (0x837f8c9)
| "PC-complete" (0x837f8c9)
| "call-interactively" (0x85eaac9)
| "completing-read" (0x889ea6b)
| "setq" (0x874320d)
| "let" (0x87431fd)
| "call-interactively" (0x8447709)
| (gdb)

In GNU Emacs 22.0.50.22 (i686-pc-linux-gnu, GTK+ Version 2.8.18)
 of 2006-08-02 on pacem
X server distributor `The X.Org Foundation', version 11.0.70000000
configured using `configure '--with-gtk' 'CFLAGS=-O0 -g''

Unfortunately, the regular backtrace doesn't make much sense to me.
Can someone suggest what to do to get more info?
(I'm pretty sure I bootstrapped this Emacs at least once since the
sit-for changes were installed, I can do that again if it helps.
I usually build with `make bootfast'.)

| (gdb) bt
| #0  0xffffe410 in __kernel_vsyscall ()
| #1  0xa7653cc6 in kill () from /lib/tls/i686/cmov/libc.so.6
| #2  0x08124fb4 in fatal_error_signal (sig=6) at emacs.c:430
| #3  <signal handler called>
| #4  0xffffe410 in __kernel_vsyscall ()
| #5  0xa7653cc6 in kill () from /lib/tls/i686/cmov/libc.so.6
| #6  0x08124fd4 in abort () at emacs.c:464
| #7  0x081e70fd in Fbyte_code (bytestr=136529099, vector=136529116, maxdepth=40)
|     at bytecode.c:1666
| #8  0x081ad855 in funcall_lambda (fun=136529036, nargs=1, 
|     arg_vector=0xafda15a4) at eval.c:3169
| #9  0x081ad315 in Ffuncall (nargs=2, args=0xafda15a0) at eval.c:3028
| #10 0x081e46a2 in Fbyte_code (bytestr=141874683, vector=141478564, maxdepth=16)
|     at bytecode.c:679
| #11 0x081ad855 in funcall_lambda (fun=141478732, nargs=1, 
|     arg_vector=0xafda18a4) at eval.c:3169
| #12 0x081ad315 in Ffuncall (nargs=2, args=0xafda18a0) at eval.c:3028
| #13 0x081e46a2 in Fbyte_code (bytestr=141871139, vector=141522028, maxdepth=56)
|     at bytecode.c:679
| #14 0x081ad855 in funcall_lambda (fun=141522612, nargs=1, 
|     arg_vector=0xafda1bb4) at eval.c:3169
| #15 0x081ad315 in Ffuncall (nargs=2, args=0xafda1bb0) at eval.c:3028
| #16 0x081e46a2 in Fbyte_code (bytestr=141870627, vector=141503852, maxdepth=40)
|     at bytecode.c:679
| #17 0x081ad855 in funcall_lambda (fun=141504020, nargs=0, 
|     arg_vector=0xafda1ef4) at eval.c:3169
| #18 0x081ad315 in Ffuncall (nargs=1, args=0xafda1ef0) at eval.c:3028
| #19 0x081acbf9 in apply1 (fn=140421833, arg=137885897) at eval.c:2723
| #20 0x081a7102 in Fcall_interactively (function=140421833, 
|     record_flag=137885897, keys=137926396) at callint.c:408
| #21 0x081378be in Fcommand_execute (cmd=140421833, record_flag=137885897, 
|     keys=137885897, special=137885897) at keyboard.c:9782
| #22 0x08129cb8 in command_loop_1 () at keyboard.c:1790
| #23 0x081aacba in internal_condition_case (bfun=0x81288ea <command_loop_1>, 
|     handlers=137930561, hfun=0x81283b6 <cmd_error>) at eval.c:1469
| #24 0x08128761 in command_loop_2 () at keyboard.c:1326
| #25 0x081aa775 in internal_catch (tag=137953105, 
|     func=0x812873e <command_loop_2>, arg=137885897) at eval.c:1210
| #26 0x081286bf in command_loop () at keyboard.c:1293
| #27 0x08128134 in recursive_edit_1 () at keyboard.c:1003
| #28 0x08157a53 in read_minibuf (map=137873781, initial=137885897, 
|     prompt=143256171, backup_n=0, expflag=0, histvar=138030145, histpos=0, 
|     defalt=137885897, allow_props=0, inherit_input_method=0) at minibuf.c:725
| #29 0x08159776 in Fcompleting_read (prompt=143256171, table=137886948, 
|     predicate=141832861, require_match=137885945, initial_input=137885897, 
|     hist=137885897, def=137885897, inherit_input_method=137885897)
|     at minibuf.c:1771
| #30 0x081ac41d in Feval (form=141832725) at eval.c:2341
| #31 0x081a9526 in Fsetq (args=141832717) at eval.c:537
| #32 0x081ac068 in Feval (form=141832709) at eval.c:2260
| #33 0x081a93b8 in Fprogn (args=141549117) at eval.c:435
| #34 0x081aa3fc in Flet (args=141832701) at eval.c:1052
| #35 0x081ac068 in Feval (form=141832629) at eval.c:2260
| #36 0x081a6ff9 in Fcall_interactively (function=138704649, 
|     record_flag=137885897, keys=137926396) at callint.c:380
| #37 0x081378be in Fcommand_execute (cmd=138704649, record_flag=137885897, 
|     keys=137885897, special=137885897) at keyboard.c:9782
| #38 0x08129cb8 in command_loop_1 () at keyboard.c:1790
| #39 0x081aacba in internal_condition_case (bfun=0x81288ea <command_loop_1>, 
|     handlers=137930561, hfun=0x81283b6 <cmd_error>) at eval.c:1469
| #40 0x08128761 in command_loop_2 () at keyboard.c:1326
| #41 0x081aa775 in internal_catch (tag=137926769, 
|     func=0x812873e <command_loop_2>, arg=137885897) at eval.c:1210
| #42 0x08128717 in command_loop () at keyboard.c:1305
| #43 0x08128134 in recursive_edit_1 () at keyboard.c:1003
| #44 0x08128276 in Frecursive_edit () at keyboard.c:1064
| #45 0x08126b36 in main (argc=1, argv=0xafda35b4) at emacs.c:1794

...

| (gdb) bt full
| #0  0xffffe410 in __kernel_vsyscall ()
| No symbol table info available.
| #1  0xa7653cc6 in kill () from /lib/tls/i686/cmov/libc.so.6
| No symbol table info available.
| #2  0x08124fb4 in fatal_error_signal (sig=6) at emacs.c:430
| No locals.
| #3  <signal handler called>
| No symbol table info available.
| #4  0xffffe410 in __kernel_vsyscall ()
| No symbol table info available.
| #5  0xa7653cc6 in kill () from /lib/tls/i686/cmov/libc.so.6
| No symbol table info available.
| #6  0x08124fd4 in abort () at emacs.c:464
| No locals.
| #7  0x081e70fd in Fbyte_code (bytestr=136529099, vector=136529116, maxdepth=40)
|     at bytecode.c:1666
|         count = 43
|         op = 135
|         vectorp = (Lisp_Object *) 0x82344e0
|         bytestr_length = 58
|         stack = {
|   pc = 0x8329d59 "", 
|   top = 0x0, 
|   bottom = 0xafda1290, 
|   byte_string = 136529099, 
|   byte_string_start = 0x8329d1f "\b\204\t", 
|   constants = 136529116, 
|   next = 0xafda1638
| }
|         top = (Lisp_Object *) 0xafda128c
|         result = 137885897
| #8  0x081ad855 in funcall_lambda (fun=136529036, nargs=1, 
|     arg_vector=0xafda15a4) at eval.c:3169
|         val = 142854916
|         syms_left = 137885897
|         next = 138193681
|         count = 40
|         i = 1
|         optional = 1
|         rest = 0
| #9  0x081ad315 in Ffuncall (nargs=2, args=0xafda15a0) at eval.c:3028
|         fun = 136529036
|         original_fun = 138193489
|         funcar = -1344662136
|         numargs = 1
|         lisp_numargs = 137885897
|         val = 137953722
|         backtrace = {
|   next = 0xafda184c, 
|   function = 0xafda15a0, 
|   args = 0xafda15a4, 
|   nargs = 1, 
|   evalargs = 0 '\0', 
|   debug_on_exit = 0 '\0'
| }
|         internal_args = (Lisp_Object *) 0x83901ba
|         i = -1344662136
| #10 0x081e46a2 in Fbyte_code (bytestr=141874683, vector=141478564, maxdepth=16)
|     at bytecode.c:679
|         count = 38
|         op = 1
|         vectorp = (Lisp_Object *) 0x86ecaa8
|         bytestr_length = 57
|         stack = {
|   pc = 0x887063d "\210\nd|\210\f\2057", 
|   top = 0xafda15a4, 
|   bottom = 0xafda15a0, 
|   byte_string = 141874683, 
|   byte_string_start = 0x8870614 "\b\203\020", 
|   constants = 141478564, 
|   next = 0xafda1948
| }
|         top = (Lisp_Object *) 0xafda15a0
|         result = -1344661880
| #11 0x081ad855 in funcall_lambda (fun=141478732, nargs=1, 
|     arg_vector=0xafda18a4) at eval.c:3169
|         val = -1344661368
|         syms_left = 137885897
|         next = 138001313
|         count = 37
|         i = 1
|         optional = 0
|         rest = 0
| #12 0x081ad315 in Ffuncall (nargs=2, args=0xafda18a0) at eval.c:3028
|         fun = 141478732
|         original_fun = 140422241
|         funcar = 10
|         numargs = 1
|         lisp_numargs = 0
|         val = 137885897
|         backtrace = {
|   next = 0xafda1b5c, 
|   function = 0xafda18a0, 
|   args = 0xafda18a4, 
|   nargs = 1, 
|   evalargs = 0 '\0', 
|   debug_on_exit = 0 '\0'
| }
|         internal_args = (Lisp_Object *) 0xafda1800
|         i = 1
| #13 0x081e46a2 in Fbyte_code (bytestr=141871139, vector=141522028, maxdepth=56)
|     at bytecode.c:679
|         count = 20
|         op = 1
|         vectorp = (Lisp_Object *) 0x86f7470
|         bytestr_length = 1652
|         stack = {
|   pc = 0x874d02d "\210\307\202q\006\016M\211\026NA\204\266\003\016Q\373>\203/\006\016A\203\024\004\016Q\370=\204\024\004\016M\036a\016b\016c\232\204\334\003\374\375\344\016b\211\026c\376#\377Q\026d\307\026K\016a\203\n\004\311\016d\016a@\"\204\001\004\311\201p", 
|   top = 0xafda18a4, 
|   bottom = 0xafda18a0, 
|   byte_string = 141871139, 
|   byte_string_start = 0x874cc8c "\b\204\a", 
|   constants = 141522028, 
|   next = 0xafda1c58
| }
|         top = (Lisp_Object *) 0xafda18a0
|         result = 137885897
| #14 0x081ad855 in funcall_lambda (fun=141522612, nargs=1, 
|     arg_vector=0xafda1bb4) at eval.c:3169
|         val = 137885897
|         syms_left = 137885897
|         next = 138140825
|         count = 17
|         i = 1
|         optional = 1
|         rest = 0
| #15 0x081ad315 in Ffuncall (nargs=2, args=0xafda1bb0) at eval.c:3028
|         fun = 141522612
|         original_fun = 140422145
|         funcar = 138045474
|         numargs = 1
|         lisp_numargs = 0
|         val = 137885921
|         backtrace = {
|   next = 0xafda1e6c, 
|   function = 0xafda1bb0, 
|   args = 0xafda1bb4, 
|   nargs = 1, 
|   evalargs = 0 '\0', 
|   debug_on_exit = 0 '\0'
| }
|         internal_args = (Lisp_Object *) 0x2
|         i = 138045474
| #16 0x081e46a2 in Fbyte_code (bytestr=141870627, vector=141503852, maxdepth=40)
|     at bytecode.c:679
|         count = 16
|         op = 1

-- 
Romain Francoise <romain@orebokech.com> | The sea! the sea! the open
it's a miracle -- http://orebokech.com/ | sea! The blue, the fresh, the
                                        | ever free! --Bryan W. Procter

Re: C-g crash redux

[Nick Roberts]

Romain Francoise writes:
 > I can crash Emacs reliably with current (Aug 2nd) sources by hitting C-g
 > while it's preparing a partial-completion-mode prompt.  I just did it
 > three times in a row.  The Lisp backtrace shows that it's indeed related
 > to sit-for:

Emacs can be made to crash simply by evaluating (sit-for 4) in the scratch
buffer and typing C-g before four seconds elapse.

It's due to this change:

2006-08-01  Kim F. Storm  <storm@cua.dk>

	* process.c (wait_reading_process_output_unwind): New function.
	Restores waiting_for_user_input_p to saved value.
	(wait_reading_process_output): Unwind protect waiting_for_user_input_p
	instead of save/restore old value on stack.

Presumably wait_reading_process_output is interrupted before it can do:

  unbind_to (count, Qnil);

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: C-g crash redux

[Kim F. Storm]

Nick Roberts <nickrob@snap.net.nz> writes:

> Romain Francoise writes:
>  > I can crash Emacs reliably with current (Aug 2nd) sources by hitting C-g
>  > while it's preparing a partial-completion-mode prompt.  I just did it
>  > three times in a row.  The Lisp backtrace shows that it's indeed related
>  > to sit-for:
>
> Emacs can be made to crash simply by evaluating (sit-for 4) in the scratch
> buffer and typing C-g before four seconds elapse.
>
> It's due to this change:
>
> 2006-08-01  Kim F. Storm  <storm@cua.dk>
>
> 	* process.c (wait_reading_process_output_unwind): New function.
> 	Restores waiting_for_user_input_p to saved value.
> 	(wait_reading_process_output): Unwind protect waiting_for_user_input_p
> 	instead of save/restore old value on stack.
>
> Presumably wait_reading_process_output is interrupted before it can do:
>
>   unbind_to (count, Qnil);

Huh?  Is that really possible?

What kind of interrupt can cause unwind_protect forms NOT to be run?

I don't have time to look at this right now.

Could someone pls. investigate this.  (It is trivial to
reproduce as Nick describes above).

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: C-g crash redux

[Nick Roberts]

 > > Presumably wait_reading_process_output is interrupted before it can do:
 > >
 > >   unbind_to (count, Qnil);
 > 
 > Huh?  Is that really possible?
 >
 > What kind of interrupt can cause unwind_protect forms NOT to be run?

Oh I see, unbind_to gets called through Fsignal.  Perhaps it's a byte compiler
problem then.  I just prefer to assume something simple to start with.


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: C-g crash redux

[Chong Yidong]

Nick Roberts <nickrob@snap.net.nz> writes:

> Romain Francoise writes:
>  > I can crash Emacs reliably with current (Aug 2nd) sources by hitting C-g
>  > while it's preparing a partial-completion-mode prompt.  I just did it
>  > three times in a row.  The Lisp backtrace shows that it's indeed related
>  > to sit-for:
>
> Emacs can be made to crash simply by evaluating (sit-for 4) in the scratch
> buffer and typing C-g before four seconds elapse.

A strange observation: if you evaluate the sit-for defun, no crash
happens.  The byte-compiled version of sit-for, however, causes a
crash due to a SPECL_INDEX imbalance.

Re: C-g crash redux

[Nick Roberts]

 > A strange observation: if you evaluate the sit-for defun, no crash
 > happens.  The byte-compiled version of sit-for, however, causes a
 > crash due to a SPECL_INDEX imbalance.

It's not that strange seeing that abort is called after testing for the
SPECL_INDEX imbalance in Fbyte_code (unless there is an analagous call in
somewhere like Feval).  In fact if BYTE_CODE_SAFE is defined, it appears Emacs
will just generate an error rather than crash.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: C-g crash redux

[Romain Francoise]

Nick Roberts <nickrob@snap.net.nz> writes:

> In fact if BYTE_CODE_SAFE is defined, it appears Emacs will just
> generate an error rather than crash.

For those following along at home:

|   /* Binds and unbinds are supposed to be compiled balanced.  */
|   if (SPECPDL_INDEX () != count)
| #ifdef BYTE_CODE_SAFE
|     error ("binding stack not balanced (serious byte compiler bug)");
| #else
|     abort ();
| #endif

I'm curious to know: if the error is recoverable, why abort?
The BYTE_CODE_SAFE branch certainly seems to suggest that it is
recoverable...

-- 
Romain Francoise <romain@orebokech.com> | The sea! the sea! the open
it's a miracle -- http://orebokech.com/ | sea! The blue, the fresh, the
                                        | ever free! --Bryan W. Procter

Re: C-g crash redux

[Stefan Monnier]

> |   /* Binds and unbinds are supposed to be compiled balanced.  */
> |   if (SPECPDL_INDEX () != count)
> | #ifdef BYTE_CODE_SAFE
> |     error ("binding stack not balanced (serious byte compiler bug)");
> | #else
> |     abort ();
> | #endif

> I'm curious to know: if the error is recoverable, why abort?
> The BYTE_CODE_SAFE branch certainly seems to suggest that it is
> recoverable...

Grep for BYTE_CODE_SAFE and you'll see that in order for the error to be
recoverable, we need to do extra work.


        Stefan

Re: C-g crash redux

[Chong Yidong]

>> Emacs can be made to crash simply by evaluating (sit-for 4) in the scratch
>> buffer and typing C-g before four seconds elapse.
>>
>> It's due to this change:
>>
>> 2006-08-01  Kim F. Storm  <storm@cua.dk>
>>
>> 	* process.c (wait_reading_process_output_unwind): New function.
>> 	Restores waiting_for_user_input_p to saved value.
>> 	(wait_reading_process_output): Unwind protect waiting_for_user_input_p
>> 	instead of save/restore old value on stack.
>>
>> Presumably wait_reading_process_output is interrupted before it can do:
>>
>>   unbind_to (count, Qnil);
>
> Huh?  Is that really possible?
>
> What kind of interrupt can cause unwind_protect forms NOT to be run?

Typing C-g, which runs interrupt_signal, runs quit_throw_to_read_char
instead of Fsignal during when waiting_for_input is set (as it is
during wait_reading_process_output).  If I understand correctly, the
purpose is to enable read-char to return a `C-g' character; and that's
why it's both undesirable and unnecesssary to use a
record_unwind_protect inside wait_reading_process_output.

I'll go ahead and revert the 2006-08-01 change.

Re: C-g crash redux

[Richard Stallman]

    | #6  0x08124fd4 in abort () at emacs.c:464
    | #7  0x081e70fd in Fbyte_code (bytestr=136529099, vector=136529116, maxdepth=40)
    |     at bytecode.c:1666
    | #8  0x081ad855 in funcall_lambda (fun=136529036, nargs=1, 
    |     arg_vector=0xafda15a4) at eval.c:3169
    | #9  0x081ad315 in Ffuncall (nargs=2, args=0xafda15a0) at eval.c:3028

Please do

f 9
p *args
xpr

so we can see what Lisp function is running.

If you run with a breakpoint at abort, then you will catch this
before it really tries to kill itself.  Then you will be able
to use the GDB command `pr' to print Lisp objects nicely.

.gdbinit in the Emacs source directory normally puts a breakpoint at
abort.  Why don't you have one?  Did you somehow avoid running .gdbinit?

Anyway, the next step after we know which function
is to try disassembling it and see how it could fail to clear
its specpdl entries.  Maybe it was compiled incorrectly;
someone should study its bytecode to see that it really
corresponds to the Lisp code and is consistent.

Re: C-g crash redux

[Richard Stallman]

    A strange observation: if you evaluate the sit-for defun, no crash
    happens.  The byte-compiled version of sit-for, however, causes a
    crash due to a SPECL_INDEX imbalance.

Please disassemble it using M-x disassemble
and see if it properly corresponds to the source code
and whether you can see why there is this imbalance.

Re: C-g crash redux

[Richard Stallman]

    I'm curious to know: if the error is recoverable, why abort?

I am not sure if there is a good reason.
However, the original reason is that it should never get there.
The code produced by the byte compiler should always be such
as to pop the specpdl by the time the end of the function is reached.

So either sit-for has been compiled wrong, or there is a bug at the C level
causing correct byte-code to execute wrong.

Re: C-g crash redux

[Kim F. Storm]

Chong Yidong <cyd@stupidchicken.com> writes:

>>> Emacs can be made to crash simply by evaluating (sit-for 4) in the scratch
>>> buffer and typing C-g before four seconds elapse.
>>>
>>> It's due to this change:
>>>
>>> 2006-08-01  Kim F. Storm  <storm@cua.dk>
>>>
>>> 	* process.c (wait_reading_process_output_unwind): New function.
>>> 	Restores waiting_for_user_input_p to saved value.
>>> 	(wait_reading_process_output): Unwind protect waiting_for_user_input_p
>>> 	instead of save/restore old value on stack.
>>>
>>> Presumably wait_reading_process_output is interrupted before it can do:
>>>
>>>   unbind_to (count, Qnil);
>>
>> Huh?  Is that really possible?
>>
>> What kind of interrupt can cause unwind_protect forms NOT to be run?
>
> Typing C-g, which runs interrupt_signal, runs quit_throw_to_read_char
> instead of Fsignal during when waiting_for_input is set (as it is
> during wait_reading_process_output).  If I understand correctly, the
> purpose is to enable read-char to return a `C-g' character; and that's
> why it's both undesirable and unnecesssary to use a
> record_unwind_protect inside wait_reading_process_output.

Ok, I see.


>
> I'll go ahead and revert the 2006-08-01 change.

Thanks.

But how do we ensure that waiting_for_user_input_p is restored
correctly on C-g?

Or isn't that necessary?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: C-g crash redux

[Chong Yidong]

>> Typing C-g, which runs interrupt_signal, runs quit_throw_to_read_char
>> instead of Fsignal during when waiting_for_input is set (as it is
>> during wait_reading_process_output).  If I understand correctly, the
>> purpose is to enable read-char to return a `C-g' character; and that's
>> why it's both undesirable and unnecesssary to use a
>> record_unwind_protect inside wait_reading_process_output.
>
> Ok, I see.
>
> But how do we ensure that waiting_for_user_input_p is restored
> correctly on C-g?
>
> Or isn't that necessary?

That's not necessary, since quit_throw_to_read_char calls
clear_waiting_for_input, which resets waiting_for_input to 0.

I guess someone worked all this out at some time in the past.  I added
a comment to the code to clarify this.

Re: C-g crash redux

[Nick Roberts]

 > Typing C-g, which runs interrupt_signal, runs quit_throw_to_read_char
 > instead of Fsignal during when waiting_for_input is set (as it is
 > during wait_reading_process_output).

keyboard-quit goes through interrupt_signal -> quit_throw_to_read_char
and then calls Fsignal i.e it's not necessarily an alternative.

 >                                       If I understand correctly, the
 > purpose is to enable read-char to return a `C-g' character; and that's
 > why it's both undesirable and unnecesssary to use a
 > record_unwind_protect inside wait_reading_process_output.

I don't follow.

 > I'll go ahead and revert the 2006-08-01 change.

I think that cures the symptom not the cause.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: C-g crash redux

[Nick Roberts]

Chong Yidong writes:
 > >> Typing C-g, which runs interrupt_signal, runs quit_throw_to_read_char
 > >> instead of Fsignal during when waiting_for_input is set (as it is
 > >> during wait_reading_process_output).  If I understand correctly, the
 > >> purpose is to enable read-char to return a `C-g' character; and that's
 > >> why it's both undesirable and unnecesssary to use a
 > >> record_unwind_protect inside wait_reading_process_output.
 > >
 > > Ok, I see.
 > >
 > > But how do we ensure that waiting_for_user_input_p is restored
 > > correctly on C-g?
 > >
 > > Or isn't that necessary?
 > 
 > That's not necessary, since quit_throw_to_read_char calls
 > clear_waiting_for_input, which resets waiting_for_input to 0.

But that might not have been the original value.

 > I guess someone worked all this out at some time in the past.  I added
 > a comment to the code to clarify this.

I'm not convinced that your analysis is correct, but it's in keeping with
the "all bugs must be fixed" policy.

I think that Fsignal should get called (perhaps with QUIT) before Emacs gets
back to the command loop (or at least before further expressions/bytecode is
evaluated) so that unbind_to gets called and the binding stack stays balanced.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: C-g crash redux

[Kim F. Storm]

Chong Yidong <cyd@stupidchicken.com> writes:

>> But how do we ensure that waiting_for_user_input_p is restored
>> correctly on C-g?
>>
>> Or isn't that necessary?
>
> That's not necessary, since quit_throw_to_read_char calls
> clear_waiting_for_input, which resets waiting_for_input to 0.

But it doesn't restore waiting_for_user_input_p to its
previous/original value.

Pls. see the comment about waiting_for_user_input_p in process.c.

>
> I guess someone worked all this out at some time in the past.  I added
> a comment to the code to clarify this.

I'm not sure someone worked this out originally.  

IIRC, the save/restore of waiting_for_user_input_p was added by
myself, and I doubt it was a concious decision at that time to use the
stack rather than unwind protect.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: C-g crash redux

[Chong Yidong]

storm@cua.dk (Kim F. Storm) writes:

>> That's not necessary, since quit_throw_to_read_char calls
>> clear_waiting_for_input, which resets waiting_for_input to 0.
>
> But it doesn't restore waiting_for_user_input_p to its
> previous/original value.
>
> Pls. see the comment about waiting_for_user_input_p in process.c.

Ah, sorry.  I got confused between waiting_for_user_input_p and
waiting_for_input.


Nick Roberts <nickrob@snap.net.nz> writes:

> I think that Fsignal should get called (perhaps with QUIT) before
> Emacs gets back to the command loop (or at least before further
> expressions/bytecode is evaluated) so that unbind_to gets called and
> the binding stack stays balanced.

I am hesitant to do this.  The code in question looks delicate.

Another approach, which may be less risky, is to protect the portions
of code in read_char where getcjmp is active.  We should then be able
to safely use unwind protects inside wait_reading_process_output.
Something like this:

      save_getcjmp (save_jump);
      restore_getcjmp (local_getcjmp);
      timer_start_idle ();
      c = kbd_buffer_get_event (&kb, used_mouse_menu, end_time);
      restore_getcjmp (save_jump);

 -->

      count = SPECPDL_INDEX ();
      save_getcjmp (save_jump);
      restore_getcjmp (local_getcjmp);
      timer_start_idle ();
      c = kbd_buffer_get_event (&kb, used_mouse_menu, end_time);
      restore_getcjmp (save_jump);
      unbind_to (count);

What do you think?

Re: C-g crash redux

[Chong Yidong]

Chong Yidong <cyd@stupidchicken.com> writes:

> Another approach, which may be less risky, is to protect the portions
> of code in read_char where getcjmp is active.  We should then be able
> to safely use unwind protects inside wait_reading_process_output.

The previous code suggestion I gave was not well thought out.  The
correct way to implement this is as follows.  I've checked that with
this change, the addition of wait_reading_process_output_unwind does
not cause a crash when C-g is done during sit-for.

Do you think this is the right approach?

*** emacs/src/keyboard.c.~1.862.~	2006-08-01 11:00:25.000000000 -0400
--- emacs/src/keyboard.c	2006-08-04 12:00:29.000000000 -0400
***************
*** 2413,2418 ****
--- 2413,2419 ----
    volatile int reread;
    struct gcpro gcpro1, gcpro2;
    int polling_stopped_here = 0;
+   int jmpcount;
  
    also_record = Qnil;
  
***************
*** 2629,2639 ****
--- 2630,2642 ----
       around any call to sit_for or kbd_buffer_get_event;
       it *must not* be in effect when we call redisplay.  */
  
+   jmpcount = SPECPDL_INDEX ();
    if (_setjmp (local_getcjmp))
      {
        /* We must have saved the outer value of getcjmp here,
  	 so restore it now.  */
        restore_getcjmp (save_jump);
+       unbind_to (jmpcount, Qnil);
        XSETINT (c, quit_char);
        internal_last_event_frame = selected_frame;
        Vlast_event_frame = internal_last_event_frame;

Re: C-g crash redux

[Richard Stallman]

This approach looks pretty safe to me.

Re: C-g crash redux

[Nick Roberts]

 > The previous code suggestion I gave was not well thought out.  The
 > correct way to implement this is as follows.  I've checked that with
 > this change, the addition of wait_reading_process_output_unwind does
 > not cause a crash when C-g is done during sit-for.
 > 
 > Do you think this is the right approach?
 >... 

It looks good to my inexperienced eye, but Emacs seems to go for a long time
before hitting Fsignal.  I wonder if Fsignal resets other things that might not
add up in the interim period but I don't know what the rules are for placing
the QUIT macro or whether one could be placed closer to read_char.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: C-g crash redux

[Kim F. Storm]

Nick Roberts <nickrob@snap.net.nz> writes:

>  > The previous code suggestion I gave was not well thought out.  The
>  > correct way to implement this is as follows.  I've checked that with
>  > this change, the addition of wait_reading_process_output_unwind does
>  > not cause a crash when C-g is done during sit-for.
>  > 
>  > Do you think this is the right approach?

Clever!  Yes.  


>  >... 
>
> It looks good to my inexperienced eye, but Emacs seems to go for a long time
> before hitting Fsignal.  I wonder if Fsignal resets other things that might not
> add up in the interim period but I don't know what the rules are for placing
> the QUIT macro or whether one could be placed closer to read_char.

I don't follow this argument.  

Since the purpose of setting waiting_for_input is for emacs to produce
C-g, it depends on the key binding whether Fsignal is ever called.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: C-g crash redux

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> Nick Roberts <nickrob@snap.net.nz> writes:
>
>>  > The previous code suggestion I gave was not well thought out.  The
>>  > correct way to implement this is as follows.  I've checked that with
>>  > this change, the addition of wait_reading_process_output_unwind does
>>  > not cause a crash when C-g is done during sit-for.
>>  > 
>>  > Do you think this is the right approach?
>
> Clever!  Yes.  
>
>
>>  >... 
>>
>> It looks good to my inexperienced eye, but Emacs seems to go for a long time
>> before hitting Fsignal.  I wonder if Fsignal resets other things that might not
>> add up in the interim period but I don't know what the rules are for placing
>> the QUIT macro or whether one could be placed closer to read_char.
>
> I don't follow this argument.  
>
> Since the purpose of setting waiting_for_input is for emacs to produce
> C-g, it depends on the key binding whether Fsignal is ever called.

There is no situation on a tty or on any platform where this is done
by signal directly?  And it does not depend on SYNC_INPUT?

While I have no clue about the situation, those are two keywords that
pop up in my head.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: C-g crash redux

[Richard Stallman]

Can someone tell me what the issue is, here?

>> It looks good to my inexperienced eye, but Emacs seems to go for a long time
>> before hitting Fsignal.  I wonder if Fsignal resets other things that might not
>> add up in the interim period but I don't know what the rules are for placing
>> the QUIT macro or whether one could be placed closer to read_char.

Why does it matter when, or whether, Emacs would ever call Fsignal?

Re: C-g crash redux

[Nick Roberts]

 > Can someone tell me what the issue is, here?
 > 
 > >> It looks good to my inexperienced eye, but Emacs seems to go for a long
 > >> time before hitting Fsignal.  I wonder if Fsignal resets other things
 > >> that might not add up in the interim period but I don't know what the
 > >> rules are for placing the QUIT macro or whether one could be placed
 > >> closer to read_char.
 > 
 > Why does it matter when, or whether, Emacs would ever call Fsignal?

I'm not sure that it does matter, I was surmising.  The problem with the
SPECL_INDEX imbalance arose because Emacs got back to Fbyte_code and checked
for an imbalance before reaching a call to unbind_to from Fsignal (from QUIT in
Ffuncall).  However Fsignal also appears to clean up a lot of other things.
Maybe some of these could cause a problem just as the binding imbalance did.

-- 
Nick                                           http://www.inet.net.nz/~nickrob