bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Alex]

Evaluate the following in emacs -Q:

(make-thread
 (lambda ()
   (message "hello")
   (sit-for 2)
   (message "there")))

Executing C-g before this finishes crashes Emacs. Here's a backtrace:

#0  0x00007fffefa8bfcf in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007fffefa8d3fa in __GI_abort () at abort.c:89
#2  0x00007fffefac9bd0 in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x7fffefbbd2c5 "*** %s ***: %s terminated\n") at ../sysdeps/posix/libc_fatal.c:175
#3  0x00007fffefb521b7 in __GI___fortify_fail (msg=0x7fffefbbd284 <longjmp_msg> "longjmp causes uninitialized stack frame") at fortify_fail.c:30
#4  0x00007fffefb520ed in ____longjmp_chk () at ../sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S:100
#5  0x00007fffefb5204b in __longjmp_chk (env=env@entry=0xbbd960 <getcjmp>, val=val@entry=1) at ../setjmp/longjmp.c:38
#6  0x00000000004f3007 in quit_throw_to_read_char (from_signal=from_signal@entry=false) at keyboard.c:10537
#7  0x0000000000501a4c in set_waiting_for_input (time_to_clear=time_to_clear@entry=0x7fffffffd940) at keyboard.c:10311
#8  0x00000000005a5d6d in wait_reading_process_output (time_limit=time_limit@entry=30, nsecs=nsecs@entry=0, read_kbd=read_kbd@entry=-1, do_display=do_display@entry=true, wait_for_cell=..., 
    wait_for_cell@entry=..., wait_proc=wait_proc@entry=0x0, just_wait_proc=0) at process.c:5172
#9  0x0000000000424000 in sit_for (timeout=..., reading=reading@entry=true, display_option=display_option@entry=1)
    at dispnew.c:5764
#10 0x00000000004fdeef in read_char (commandflag=commandflag@entry=1, map=..., 
    map@entry=..., prev_event=..., used_mouse_menu=used_mouse_menu@entry=0x7fffffffe31b, end_time=end_time@entry=0x0)
    at keyboard.c:2724
#11 0x00000000004feb2c in read_key_sequence (keybuf=keybuf@entry=0x7fffffffe420, prompt=..., 
    prompt@entry=..., dont_downcase_last=dont_downcase_last@entry=false, can_return_switch_frame=can_return_switch_frame@entry=true, fix_current_buffer=fix_current_buffer@entry=true, prevent_redisplay=prevent_redisplay@entry=false, bufsize=30) at keyboard.c:9154
#12 0x0000000000500576 in command_loop_1 () at keyboard.c:1372
#13 0x000000000056495e in internal_condition_case (bfun=bfun@entry=0x500350 <command_loop_1>, handlers=..., 
    handlers@entry=..., hfun=hfun@entry=0x4f6eb0 <cmd_error>) at eval.c:1332
#14 0x00000000004f2444 in command_loop_2 (ignore=..., ignore@entry=...) at keyboard.c:1114
#15 0x00000000005648cd in internal_catch (tag=..., tag@entry=..., func=func@entry=0x4f2420 <command_loop_2>, arg=..., 
    arg@entry=...) at eval.c:1097
#16 0x00000000004f23db in command_loop () at keyboard.c:1093
#17 0x00000000004f6ac3 in recursive_edit_1 () at keyboard.c:699
#18 0x00000000004f6de6 in Frecursive_edit () at keyboard.c:770
#19 0x000000000041a3f1 in main (argc=<optimized out>,
#argv=0x7fffffffe7a8) at emacs.c:1713

In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.18)
 of 2017-09-28 built on lylat
Repository revision: 1f02ae39310f15bf683642b9aee1cf162bd391e6
Windowing system distributor 'The X.Org Foundation', version 11.0.11903000
System Description:	Debian GNU/Linux testing (buster)

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS NOTIFY
ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 LCMS2

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Alex]

Alex <agrambot@gmail.com> writes:

> Evaluate the following in emacs -Q:
>
> (make-thread
>  (lambda ()
>    (message "hello")
>    (sit-for 2)
>    (message "there")))
>
> Executing C-g before this finishes crashes Emacs. Here's a backtrace:
> [...]

Should this be labelled a blocker for Emacs 26? This appears to be a
related to bug 25214, since I just tried to hit C-g when running the
example there and it indeed crashes Emacs.

It should be noted that `keyboard-escape-quit' (ESC ESC ESC) doesn't
crash Emacs.

Also, replacing (sit-for 2) with (read-char) or its family and hitting
C-g will also crash Emacs.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Alan Third]

On Thu, Sep 28, 2017 at 01:22:58AM -0600, Alex wrote:
> Evaluate the following in emacs -Q:
> 
> (make-thread
>  (lambda ()
>    (message "hello")
>    (sit-for 2)
>    (message "there")))
> 
> Executing C-g before this finishes crashes Emacs.

I can reproduce this crash on GNU/Linux, but not when built --with-ns.
-- 
Alan Third

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Cc: eliz@gnu.org, Tom Tromey <tom@tromey.com>
> Date: Sat, 30 Sep 2017 21:18:08 -0600
> 
> Should this be labelled a blocker for Emacs 26?

Maybe I'm not sure.  It doesn't sound like threads are used much, if
at all, which would make blocking on it silly.

> This appears to be a related to bug 25214, since I just tried to hit
> C-g when running the example there and it indeed crashes Emacs.

Could be, but the problem in that bug is not a crash, it's the way the
code was written to work.

> It should be noted that `keyboard-escape-quit' (ESC ESC ESC) doesn't
> crash Emacs.

Most probably because it doesn't longjmp.

> Also, replacing (sit-for 2) with (read-char) or its family and hitting
> C-g will also crash Emacs.

Is your Emacs built with --enable-checking?  Because I get an
assertion violation in that case, not a crash.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> Date: Sun, 1 Oct 2017 11:08:01 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: 28630@debbugs.gnu.org
> 
> I can reproduce this crash on GNU/Linux, but not when built --with-ns.

AFAIR, the NS build uses a very different code for both input and
thread switch.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Alex]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex <agrambot@gmail.com>
>> Cc: eliz@gnu.org, Tom Tromey <tom@tromey.com>
>> Date: Sat, 30 Sep 2017 21:18:08 -0600
>> 
>> Should this be labelled a blocker for Emacs 26?
>
> Maybe I'm not sure.  It doesn't sound like threads are used much, if
> at all, which would make blocking on it silly.

It's understandable that bug 25214 isn't a blocker for this reason, but
I think crashing is severe enough to be a blocker, especially since it's
somewhat trivial to trigger.

>> Also, replacing (sit-for 2) with (read-char) or its family and hitting
>> C-g will also crash Emacs.
>
> Is your Emacs built with --enable-checking?  Because I get an
> assertion violation in that case, not a crash.

I just tried building with "./configure --enable-checking" and it still
crashes for me.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Cc: 28630@debbugs.gnu.org,  tom@tromey.com
> Date: Sun, 01 Oct 2017 12:10:40 -0600
> 
> >> Should this be labelled a blocker for Emacs 26?
> >
> > Maybe I'm not sure.  It doesn't sound like threads are used much, if
> > at all, which would make blocking on it silly.
> 
> It's understandable that bug 25214 isn't a blocker for this reason, but
> I think crashing is severe enough to be a blocker, especially since it's
> somewhat trivial to trigger.

If this trigger never happens in real life, why should we block on it?

More generally, if no one besides my sorry self is interested in using
this feature, let alone fixing its bugs, maybe we should simply remove
it from Emacs.

> > Is your Emacs built with --enable-checking?  Because I get an
> > assertion violation in that case, not a crash.
> 
> I just tried building with "./configure --enable-checking" and it still
> crashes for me.

Strange, that's not what I see.

I hope to have some time soon to look into this.  I think I understand
the root cause: we are longjmp-ing in a thread other than the one
which called setjmp.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 01 Oct 2017 21:32:22 +0300, Eli Zaretskii <eliz@gnu.org> said:

> I hope to have some time soon to look into this.  I think I
> understand the root cause: we are longjmp-ing in a thread other than
> the one which called setjmp.

The secondary thread seems to be still blocking in a pselect call when
crash occurs.  Maybe SIGIO (and SIGALRM) should be unmasked in the
secondary thread that calls pselect for keyboard input, and the main
thread should forward the delivery of these signals to the secondary
(or mask them) so that the pselect call in the secondary thread can be
interrupted.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eric Abrahamsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex <agrambot@gmail.com>
>> Cc: 28630@debbugs.gnu.org,  tom@tromey.com
>> Date: Sun, 01 Oct 2017 12:10:40 -0600
>> 
>> >> Should this be labelled a blocker for Emacs 26?
>> >
>> > Maybe I'm not sure.  It doesn't sound like threads are used much, if
>> > at all, which would make blocking on it silly.
>> 
>> It's understandable that bug 25214 isn't a blocker for this reason, but
>> I think crashing is severe enough to be a blocker, especially since it's
>> somewhat trivial to trigger.
>
> If this trigger never happens in real life, why should we block on it?
>
> More generally, if no one besides my sorry self is interested in using
> this feature, let alone fixing its bugs, maybe we should simply remove
> it from Emacs.

Don't remove it! I am (extraordinarily slowly) working to make Gnus use
threading.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[YAMAMOTO Mitsuharu]

>>>>> On Mon, 02 Oct 2017 13:30:44 +0900, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> said:

> The secondary thread seems to be still blocking in a pselect call
> when crash occurs.  Maybe SIGIO (and SIGALRM) should be unmasked in
> the secondary thread that calls pselect for keyboard input, and the
> main thread should forward the delivery of these signals to the
> secondary (or mask them) so that the pselect call in the secondary
> thread can be interrupted.

Sorry.  The stacks seem to be already clobbered by (bad) longjmp.  So
the above analysis would be bogus.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> From: Eric Abrahamsen <eric@ericabrahamsen.net>
> Date: Sun, 01 Oct 2017 22:11:24 -0700
> 
> Don't remove it! I am (extraordinarily slowly) working to make Gnus use
> threading.

You've convinced me.

(I'd really like to see a thread-using Gnus, and I'm sure so are
people who actually use Gnus.)

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> Date: Mon, 02 Oct 2017 21:43:52 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: 28630@debbugs.gnu.org,
> 	tom@tromey.com,
> 	Alex <agrambot@gmail.com>
> 
> Sorry.  The stacks seem to be already clobbered by (bad) longjmp.  So
> the above analysis would be bogus.

I think the second thread calls setjmp and clobbers the values set by
the main thread, when the main thread called setjmp.  We need to
eliminate this possibility of threads stepping on each other's toes.

Caveat: I didn't yet take a good look at the involved code, so what I
say might make no sense.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Dmitry Gutov]

On 10/1/17 8:32 PM, Eli Zaretskii wrote:

> More generally, if no one besides my sorry self is interested in using
> this feature, let alone fixing its bugs, maybe we should simply remove
> it from Emacs.

I'm also interested! Just slow.

The core dev aside, the community would be disappointed, I'm sure.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> Cc: 28630@debbugs.gnu.org, tom@tromey.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 3 Oct 2017 11:35:04 +0200
> 
> The core dev aside, the community would be disappointed, I'm sure.

Does anyone know of any packages out there which use this feature?
Because if not, I'd have hard time understanding the disappointment.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Di., 3. Okt. 2017 um 16:40 Uhr:

> > Cc: 28630@debbugs.gnu.org, tom@tromey.com
> > From: Dmitry Gutov <dgutov@yandex.ru>
> > Date: Tue, 3 Oct 2017 11:35:04 +0200
> >
> > The core dev aside, the community would be disappointed, I'm sure.
>
> Does anyone know of any packages out there which use this feature?
> Because if not, I'd have hard time understanding the disappointment.
>
>
>
>
Since no released version of Emacs has thread support, I'm not surprised
that they aren't widely used yet. Let's wait a couple more years to gain
some experience.

[-- Attachment #2: Type: text/html, Size: 1097 bytes --]

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Tue, 03 Oct 2017 14:50:13 +0000
> Cc: 28630@debbugs.gnu.org, tom@tromey.com, agrambot@gmail.com
> 
>  Does anyone know of any packages out there which use this feature?
>  Because if not, I'd have hard time understanding the disappointment.
> 
> Since no released version of Emacs has thread support, I'm not surprised that they aren't widely used yet.

Well, I am, because Reddit, for example, is full of people who say
they use Emacs 26.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Di., 3. Okt. 2017 um 17:46 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Tue, 03 Oct 2017 14:50:13 +0000
> > Cc: 28630@debbugs.gnu.org, tom@tromey.com, agrambot@gmail.com
> >
> >  Does anyone know of any packages out there which use this feature?
> >  Because if not, I'd have hard time understanding the disappointment.
> >
> > Since no released version of Emacs has thread support, I'm not surprised
> that they aren't widely used yet.
>
> Well, I am, because Reddit, for example, is full of people who say
> they use Emacs 26.
>

I guess there's a huge selection bias at play. Most users typically use
whatever version their OS distributor provides, and that's often not even
Emacs 25. Libraries tend to support 24.x as well, and therefore can't use
threads until they drop compatibility with both 24.x and 25.x.

[-- Attachment #2: Type: text/html, Size: 1454 bytes --]

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Tue, 03 Oct 2017 16:22:56 +0000
> Cc: dgutov@yandex.ru, 28630@debbugs.gnu.org, tom@tromey.com, 
> 	agrambot@gmail.com
> 
>  > Since no released version of Emacs has thread support, I'm not surprised that they aren't widely used
>  yet.
> 
>  Well, I am, because Reddit, for example, is full of people who say
>  they use Emacs 26.
> 
> I guess there's a huge selection bias at play. Most users typically use whatever version their OS distributor
> provides, and that's often not even Emacs 25. Libraries tend to support 24.x as well, and therefore can't use
> threads until they drop compatibility with both 24.x and 25.x. 

We are talking about developers, not users.  And newer versions can be
supported by using the relevant version-comparison stuff.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Di., 3. Okt. 2017 um 18:37 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Tue, 03 Oct 2017 16:22:56 +0000
> > Cc: dgutov@yandex.ru, 28630@debbugs.gnu.org, tom@tromey.com,
> >       agrambot@gmail.com
> >
> >  > Since no released version of Emacs has thread support, I'm not
> surprised that they aren't widely used
> >  yet.
> >
> >  Well, I am, because Reddit, for example, is full of people who say
> >  they use Emacs 26.
> >
> > I guess there's a huge selection bias at play. Most users typically use
> whatever version their OS distributor
> > provides, and that's often not even Emacs 25. Libraries tend to support
> 24.x as well, and therefore can't use
> > threads until they drop compatibility with both 24.x and 25.x.
>
> We are talking about developers, not users.  And newer versions can be
> supported by using the relevant version-comparison stuff.
>

Sure, but it's already a lot of work to use threads unconditionally, and
they are a feature that can't be easily polyfilled. So I can understand
that library developers are hesitant.

[-- Attachment #2: Type: text/html, Size: 1786 bytes --]

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> Date: Mon, 02 Oct 2017 19:08:59 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 28630@debbugs.gnu.org, tom@tromey.com, agrambot@gmail.com
> 
> I think the second thread calls setjmp and clobbers the values set by
> the main thread, when the main thread called setjmp.

That guess was correct.

I hope I fixed this now, but please test this in a GUI session on X,
as I couldn't test that configuration.  If there are still crashes,
please post a backtrace.

Thanks.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[YAMAMOTO Mitsuharu]

>>>>> On Mon, 02 Oct 2017 19:08:59 +0300, Eli Zaretskii <eliz@gnu.org> said:

> I think the second thread calls setjmp and clobbers the values set
> by the main thread, when the main thread called setjmp.  We need to
> eliminate this possibility of threads stepping on each other's toes.

Make the global variable `getcjmp' thread-local?

			     YAMAMOTO Mitsuharu
			mituharu@math.s.chiba-u.ac.jp

diff --git a/src/keyboard.c b/src/keyboard.c
index e8701b8870..42d51a2ee8 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -145,10 +145,6 @@ static Lisp_Object recover_top_level_message;
 /* Message normally displayed by Vtop_level.  */
 static Lisp_Object regular_top_level_message;
 
-/* For longjmp to where kbd input is being done.  */
-
-static sys_jmp_buf getcjmp;
-
 /* True while displaying for echoing.   Delays C-g throwing.  */
 
 static bool echoing;
@@ -3325,13 +3321,13 @@ record_char (Lisp_Object c)
 static void
 save_getcjmp (sys_jmp_buf temp)
 {
-  memcpy (temp, getcjmp, sizeof getcjmp);
+  memcpy (temp, current_thread->getcjmp, sizeof current_thread->getcjmp);
 }
 
 static void
 restore_getcjmp (sys_jmp_buf temp)
 {
-  memcpy (getcjmp, temp, sizeof getcjmp);
+  memcpy (current_thread->getcjmp, temp, sizeof current_thread->getcjmp);
 }
 \f
 /* Low level keyboard/mouse input.
@@ -10534,7 +10530,7 @@ quit_throw_to_read_char (bool from_signal)
     do_switch_frame (make_lispy_switch_frame (internal_last_event_frame),
 		     0, 0, Qnil);
 
-  sys_longjmp (getcjmp, 1);
+  sys_longjmp (current_thread->getcjmp, 1);
 }
 \f
 DEFUN ("set-input-interrupt-mode", Fset_input_interrupt_mode,
diff --git a/src/lisp.h b/src/lisp.h
index 680c25d4c4..ed2423596e 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1865,8 +1865,6 @@ verify (offsetof (struct Lisp_Sub_Char_Table, contents)
 	== (offsetof (struct Lisp_Vector, contents)
 	    + SUB_CHAR_TABLE_OFFSET * sizeof (Lisp_Object)));
 
-#include "thread.h"
-
 /***********************************************************************
 			       Symbols
  ***********************************************************************/
@@ -3096,6 +3094,8 @@ union specbinding
     } bt;
   };
 
+#include "thread.h"
+
 /* These 3 are defined as macros in thread.h.  */
 /* extern union specbinding *specpdl; */
 /* extern union specbinding *specpdl_ptr; */
diff --git a/src/thread.h b/src/thread.h
index 7fce8674f0..125ca23b55 100644
--- a/src/thread.h
+++ b/src/thread.h
@@ -177,6 +177,9 @@ struct thread_state
      It must do so ASAP.  */
   int not_holding_lock;
 
+  /* For longjmp to where kbd input is being done.  */
+  sys_jmp_buf getcjmp;
+
   /* Threads are kept on a linked list.  */
   struct thread_state *next_thread;
 };

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[YAMAMOTO Mitsuharu]

>>>>> On Wed, 04 Oct 2017 10:39:05 +0300, Eli Zaretskii <eliz@gnu.org> said:

>> I think the second thread calls setjmp and clobbers the values set
>> by the main thread, when the main thread called setjmp.

> That guess was correct.

> I hope I fixed this now, but please test this in a GUI session on X,
> as I couldn't test that configuration.  If there are still crashes,
> please post a backtrace.

My patch was a bit late :-)

I confirmed the original test case no longer crashes with your patch
on X11 (and also on my internal version of the Mac port for Emacs 26).
Thanks.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> Date: Wed, 04 Oct 2017 16:39:27 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: 	28630@debbugs.gnu.org,
> 	tom@tromey.com,
> 	agrambot@gmail.com
> 
> >>>>> On Mon, 02 Oct 2017 19:08:59 +0300, Eli Zaretskii <eliz@gnu.org> said:
> 
> > I think the second thread calls setjmp and clobbers the values set
> > by the main thread, when the main thread called setjmp.  We need to
> > eliminate this possibility of threads stepping on each other's toes.
> 
> Make the global variable `getcjmp' thread-local?

That indeed was the central part of the solution I installed.  But it
isn't enough, because in a TTY session C-g triggers a SIGINT, which is
delivered to the main thread.  So we need also to make sure that when
the signal handler calls quit_throw_to_read_char, current_thread
points to main_thread, we have the global lock held by the main
thread, and all our Lisp thread machinery is aware that it's the main
thread that's running.  Otherwise, we are asking for trouble.

Note that this means the effect of C-g while I/O APIs are called from
non-main threads is inherently unportable: the effect will be
different in TTY and GUI sessions on Unix, and also on Windows (where
there are no signals, and therefore the original recipe didn't crash
at all, but instead interrupted the sit-for on the non-main thread and
caused that thread's early demise).

After the change, sit-for on non-main thread is effectively a
sleep-for, at least on Unix TTY frames (and I think on GUI frames as
well).  But on MS-Windows and I think also NS it still works as
sit-for on the non-main thread.

Thanks.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> Date: Wed, 04 Oct 2017 16:49:22 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: 	agrambot@gmail.com,
> 	28630@debbugs.gnu.org,
> 	tom@tromey.com
> 
> I confirmed the original test case no longer crashes with your patch
> on X11 (and also on my internal version of the Mac port for Emacs 26).
> Thanks.

Thanks for testing, I will close the bug.

OOC, could you please see whether on X the sit-for call is interrupted
or not?  That is, do you see the "there" message after typing C-g?

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[YAMAMOTO Mitsuharu]

>>>>> On Wed, 04 Oct 2017 11:16:50 +0300, Eli Zaretskii <eliz@gnu.org> said:

>> I confirmed the original test case no longer crashes with your
>> patch on X11 (and also on my internal version of the Mac port for
>> Emacs 26).  Thanks.

> Thanks for testing, I will close the bug.

> OOC, could you please see whether on X the sit-for call is
> interrupted or not?  That is, do you see the "there" message after
> typing C-g?

Not interrupted (I see "there") on GTK+3, GTK+2, Athena, no toolkit,
and (my internal version of) the Mac port.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> Date: Wed, 04 Oct 2017 17:32:40 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: agrambot@gmail.com,
> 	28630@debbugs.gnu.org,
> 	tom@tromey.com
> 
> > OOC, could you please see whether on X the sit-for call is
> > interrupted or not?  That is, do you see the "there" message after
> > typing C-g?
> 
> Not interrupted (I see "there") on GTK+3, GTK+2, Athena, no toolkit,
> and (my internal version of) the Mac port.

That's what I thought should happen, thanks for confirming.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[YAMAMOTO Mitsuharu]

>>>>> On Wed, 04 Oct 2017 12:23:19 +0300, Eli Zaretskii <eliz@gnu.org> said:

>> > OOC, could you please see whether on X the sit-for call is
>> > interrupted or not?  That is, do you see the "there" message
>> > after typing C-g?
>> 
>> Not interrupted (I see "there") on GTK+3, GTK+2, Athena, no
>> toolkit, and (my internal version of) the Mac port.

> That's what I thought should happen, thanks for confirming.

Same for NS.  Interestingly, some variants of the original recipe
behave differently with respect to C-g depending on the toolkit.

Example 1:

(thread-join
 (make-thread
  (lambda ()
    (message "hello")
    (sit-for 2)
    (message "there"))))

Interrupted: GTK+, Athena, no toolkit, Mac
Not interrupted: NS


Example 2:

(thread-join
 (make-thread
  (lambda ()
    (message "hello")
    (sleep-for 2)
    (message "there"))))

Interrupted: GTK+
Not interrupted: Athena, no toolkit, Mac, NS

Probably it has something to do with xg_select.

			     YAMAMOTO Mitsuharu
			mituharu@math.s.chiba-u.ac.jp

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> Date: Thu, 05 Oct 2017 12:27:28 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: agrambot@gmail.com,
> 	28630-done@debbugs.gnu.org,
> 	tom@tromey.com
> 
> >>>>> On Wed, 04 Oct 2017 12:23:19 +0300, Eli Zaretskii <eliz@gnu.org> said:
> 
> >> > OOC, could you please see whether on X the sit-for call is
> >> > interrupted or not?  That is, do you see the "there" message
> >> > after typing C-g?
> Same for NS.  Interestingly, some variants of the original recipe
> behave differently with respect to C-g depending on the toolkit.
> 
> Example 1:
> 
> (thread-join
>  (make-thread
>   (lambda ()
>     (message "hello")
>     (sit-for 2)
>     (message "there"))))
> 
> Interrupted: GTK+, Athena, no toolkit, Mac
> Not interrupted: NS
> 
> 
> Example 2:
> 
> (thread-join
>  (make-thread
>   (lambda ()
>     (message "hello")
>     (sleep-for 2)
>     (message "there"))))
> 
> Interrupted: GTK+
> Not interrupted: Athena, no toolkit, Mac, NS

For the record, on MS-Windows, both are interrupted.

> Probably it has something to do with xg_select.

Could be.

The upshot of this is that doing keyboard I/O in non-main threads is
inherently unportable.  Should we mention this in the manual?

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Do., 5. Okt. 2017 um 09:25 Uhr:

> > Date: Thu, 05 Oct 2017 12:27:28 +0900
> > From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> > Cc: agrambot@gmail.com,
> >       28630-done@debbugs.gnu.org,
> >       tom@tromey.com
> >
> > >>>>> On Wed, 04 Oct 2017 12:23:19 +0300, Eli Zaretskii <eliz@gnu.org>
> said:
> >
> > >> > OOC, could you please see whether on X the sit-for call is
> > >> > interrupted or not?  That is, do you see the "there" message
> > >> > after typing C-g?
> > Same for NS.  Interestingly, some variants of the original recipe
> > behave differently with respect to C-g depending on the toolkit.
> >
> > Example 1:
> >
> > (thread-join
> >  (make-thread
> >   (lambda ()
> >     (message "hello")
> >     (sit-for 2)
> >     (message "there"))))
> >
> > Interrupted: GTK+, Athena, no toolkit, Mac
> > Not interrupted: NS
> >
> >
> > Example 2:
> >
> > (thread-join
> >  (make-thread
> >   (lambda ()
> >     (message "hello")
> >     (sleep-for 2)
> >     (message "there"))))
> >
> > Interrupted: GTK+
> > Not interrupted: Athena, no toolkit, Mac, NS
>
> For the record, on MS-Windows, both are interrupted.
>
> > Probably it has something to do with xg_select.
>
> Could be.
>
> The upshot of this is that doing keyboard I/O in non-main threads is
> inherently unportable.  Should we mention this in the manual?
>
>
>
>

I'd rather see this as a bug and document it in etc/PROBLEMS.
Also, if the behavior between main and non-main threads differ, there
should probably a `in-main-thread-p' function.

[-- Attachment #2: Type: text/html, Size: 2564 bytes --]

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 07 Oct 2017 13:53:43 +0000
> Cc: 28630@debbugs.gnu.org, tom@tromey.com, agrambot@gmail.com
> 
> I'd rather see this as a bug and document it in etc/PROBLEMS.

PROBLEMS is not about Emacs bugs, it's about bugs in other software
that affect Emacs users and uses.  Emacs bugs are "documented" in the
bug tracker.

In any case, this is not a bug, this is how the feature was designed
and coded.  It's just that the consequences of this design were not
immediately apparent.

> Also, if the behavior between main and non-main threads differ, there should probably a `in-main-thread-p'
> function. 

I'd rather not add primitives before there are real-life applications
that need them and can describe the needs and the requirements.  (We
have such a function on the C level, because it was actually needed.)

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> Date: Sat, 07 Oct 2017 17:24:59 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 28630@debbugs.gnu.org, tom@tromey.com, agrambot@gmail.com
> 
> > Also, if the behavior between main and non-main threads differ, there should probably a `in-main-thread-p'
> > function. 
> 
> I'd rather not add primitives before there are real-life applications
> that need them and can describe the needs and the requirements.

And, btw, I think you can already check whether the current thread is
the main thread by using the existing primitive 'current-thread'.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[YAMAMOTO Mitsuharu]

>>>>> On Thu, 05 Oct 2017 10:24:11 +0300, Eli Zaretskii <eliz@gnu.org> said:

>> Interestingly, some variants of the original recipe behave
>> differently with respect to C-g depending on the toolkit.

> For the record, on MS-Windows, both are interrupted.

I could make the Mac port interruptible also for Example 2 (with some
delay because it uses polling with SIGALRM rather than SIGIO).  I also
confirmed that Motif behaved like Athena.  Below is updated one:

Example 1:

(thread-join
 (make-thread
  (lambda ()
    (message "hello")
    (sit-for 2)
    (message "there"))))

Interrupted: GTK+, Athena, Motif, no toolkit, MS-Windows, Mac
Not interrupted: NS


Example 2:

(thread-join
 (make-thread
  (lambda ()
    (message "hello")
    (sleep-for 2)
    (message "there"))))

Interrupted: GTK+, MS-Windows, Mac
Not interrupted: Athena, Motif, no toolkit, NS

> The upshot of this is that doing keyboard I/O in non-main threads is
> inherently unportable.  Should we mention this in the manual?

In principle, the method I used for the Mac port is also applicable to
X11 (but probably not for NS where secondary threads cannot read
keyboard input).  Each pselect call not involving keyboard input
additionally monitors one side of a socket pair, and the SIGALRM
handler writes to the other side.  This way, a pselect call in a
secondary thread is unblocked when the signal arrived, and then it
tries to see if keyboard input is available when none of other threads
is monitoring the keyboard input channel.

An alternative way would be to forward the signal to all the Lisp
threads.  But it looked too much for many cases.

Having said that, I don't think we need the change like this urgently.
For now, we can see if the above method really works for more
complicated situations using the Mac port.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> Date: Mon, 09 Oct 2017 17:21:59 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>, agrambot@gmail.com,
>  28630@debbugs.gnu.org, tom@tromey.com
> 
> > For the record, on MS-Windows, both are interrupted.
> 
> I could make the Mac port interruptible also for Example 2 (with some
> delay because it uses polling with SIGALRM rather than SIGIO).  I also
> confirmed that Motif behaved like Athena.  Below is updated one:

Thanks, more commonality in behavior is good, I think.

> In principle, the method I used for the Mac port is also applicable to
> X11 (but probably not for NS where secondary threads cannot read
> keyboard input).  Each pselect call not involving keyboard input
> additionally monitors one side of a socket pair, and the SIGALRM
> handler writes to the other side.  This way, a pselect call in a
> secondary thread is unblocked when the signal arrived, and then it
> tries to see if keyboard input is available when none of other threads
> is monitoring the keyboard input channel.

But the problematic part in these examples is that the main thread
always waits for keyboard input, because the non-main thread starts
running only when the main thread becomes idle, and becoming idle
means calling pselect.  And since the main thread is always the first
one to call pselect, it is the thread which finds the keyboard
descriptor unmonitored, and installs itself as its monitoring thread.

So with your proposal, more often than not, the "none of other threads
is monitoring the keyboard input channel" part will never be true,
except for the main thread.  Or am I missing something?

> Having said that, I don't think we need the change like this urgently.
> For now, we can see if the above method really works for more
> complicated situations using the Mac port.

I agree.  So far, I've only seen toy programs, and it isn't clear to
me how serious those problems are in real life.

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[mituharu]

>> In principle, the method I used for the Mac port is also applicable to
>> X11 (but probably not for NS where secondary threads cannot read
>> keyboard input).  Each pselect call not involving keyboard input
>> additionally monitors one side of a socket pair, and the SIGALRM
>> handler writes to the other side.  This way, a pselect call in a
>> secondary thread is unblocked when the signal arrived, and then it
>> tries to see if keyboard input is available when none of other threads
>> is monitoring the keyboard input channel.
>
> But the problematic part in these examples is that the main thread
> always waits for keyboard input, because the non-main thread starts
> running only when the main thread becomes idle, and becoming idle
> means calling pselect.  And since the main thread is always the first
> one to call pselect, it is the thread which finds the keyboard
> descriptor unmonitored, and installs itself as its monitoring thread.
>
> So with your proposal, more often than not, the "none of other threads
> is monitoring the keyboard input channel" part will never be true,
> except for the main thread.  Or am I missing something?

In both Examples, the main thread is not blocking at pselect, but
thread-join (sys_cond_wait internally), i.e., without monitoring
the keyboard descriptor. The secondary one is blocking at pselect
in sit-for (Example 1) or sleep-for (Example 2).  So "none of
other threads is monitoring the keyboard input channel" part
becomes true for the secondary thread after returning from
pselect.

In Example 2, no thread is monitoring the keyboard while the
secondary thread is blocking at pselect, and that is why the Mac
port could not be interrupted with C-g previously.  I think this
basically also applies to X11.  For GTK+ cases, my guess is the
additionally monitored descriptors obtained by
g_main_context_query has something to do with its peculiar
behavior.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#28630: 27.0.50; C-g while a non-main thread is sitting crashes Emacs

[Eli Zaretskii]

> Date: Mon, 9 Oct 2017 19:34:28 +0900
> From: mituharu@math.s.chiba-u.ac.jp
> Cc: agrambot@gmail.com,
>  28630@debbugs.gnu.org,
>  tom@tromey.com
> 
> In both Examples, the main thread is not blocking at pselect, but
> thread-join (sys_cond_wait internally), i.e., without monitoring
> the keyboard descriptor. The secondary one is blocking at pselect
> in sit-for (Example 1) or sleep-for (Example 2).  So "none of
> other threads is monitoring the keyboard input channel" part
> becomes true for the secondary thread after returning from
> pselect.

OK, with thread-join this is so.  Sorry for missing that part.

In any case, on TTY frames C-g triggers SIGINT, and we always deliver
signals to the main thread.  If we want to be able to handle keyboard
input in non-main threads, we should change how signals are handled in
Emacs.