[PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Doug Evans]

Hi.

This patch fixes PR 17247.

Basically, current Guile (git) starts an internal thread
(the "finalizer" thread), and libgc as of 7.4 now starts several
marker threads by default (before 7.4.0 one needed to configure
libgc with --enable-parallel-mark).

When other threads are running, and they haven't blocked SIGCHLD,
then the kernel may send SIGCHLD to these threads, leaving gdb
hung in the sigsuspend calls in linux-nat.c.

P.S. I have a tentative patch for PR 17314, which also fixes 17247,
but for a bit of robustness I'm applying both.

2014-08-31  Doug Evans  <xdje42@gmail.com>

	PR 17247
	* guile.c: #include <signal.h>.
	(_initialize_guile): Block SIGCHLD while initializing Guile.

	Replaces the following, which is reverted.

	2014-07-26  Doug Evans  <xdje42@gmail.com>

	PR 17185
	* configure.ac: Add check for header gc/gc.h.
	Add check for function setenv.
	* configure: Regenerate.
	* config.in: Regenerate.
	* guile/guile.c (_initialize_guile): Add workaround for libgc 7.4.0.

diff --git a/gdb/configure.ac b/gdb/configure.ac
index 26c8ecf..ab7b1c2 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1233,11 +1233,6 @@ AC_SUBST(GUILE_CPPFLAGS)
 AC_SUBST(GUILE_LIBS)
 AM_CONDITIONAL(HAVE_GUILE, test "${have_libguile}" != no)
 
-# PR 17185, see if we can get the libgc version to see if we need
-# to apply the workaround.
-AC_CHECK_HEADERS(gc/gc.h)
-AC_CHECK_FUNCS([setenv])
-
 # --------------------- #
 # Check for libmcheck.  #
 # --------------------- #
diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
index 575bb6c..40208d2 100644
--- a/gdb/guile/guile.c
+++ b/gdb/guile/guile.c
@@ -34,10 +34,8 @@
 #ifdef HAVE_GUILE
 #include "guile.h"
 #include "guile-internal.h"
-#ifdef HAVE_GC_GC_H
-#include <gc/gc.h> /* PR 17185 */
-#endif
 #endif
+#include <signal.h>
 
 /* The Guile version we're using.
    We *could* use the macros in libguile/version.h but that would preclude
@@ -833,7 +831,9 @@ extern initialize_file_ftype _initialize_guile;
 void
 _initialize_guile (void)
 {
-  char *msg;
+#ifdef HAVE_SIGPROCMASK
+  sigset_t sigchld_mask, prev_mask;
+#endif
 
   install_gdb_commands ();
 
@@ -842,16 +842,13 @@ _initialize_guile (void)
      side to define module "gdb" which imports "_gdb".  There is evidently no
      similar convention in Guile so we skip this.  */
 
-  /* PR 17185 There are problems with using libgc 7.4.0.
-     Copy over the workaround Guile uses (Guile is working around a different
-     problem, but the workaround is the same).  */
-#if (GC_VERSION_MAJOR == 7 && GC_VERSION_MINOR == 4 && GC_VERSION_MICRO == 0)
-  /* The bug is only known to appear with pthreads.  We assume any system
-     using pthreads also uses setenv (and not putenv).  That is why we don't
-     have a similar call to putenv here.  */
-#if defined (HAVE_SETENV)
-  setenv ("GC_MARKERS", "1", 1);
-#endif
+#ifdef HAVE_SIGPROCMASK
+  /* Before we initialize Guile, block SIGCHLD.
+     This is done so that all threads created during Guile initialization
+     have SIGCHLD blocked.  PR 17247.  */
+  sigemptyset (&sigchld_mask);
+  sigaddset (&sigchld_mask, SIGCHLD);
+  sigprocmask (SIG_BLOCK, &sigchld_mask, &prev_mask);
 #endif
 
   /* scm_with_guile is the most portable way to initialize Guile.
@@ -859,6 +856,10 @@ _initialize_guile (void)
      (e.g., called from within a call to scm_with_guile).  */
   scm_with_guile (call_initialize_gdb_module, NULL);
 
+#ifdef HAVE_SIGPROCMASK
+  sigprocmask (SIG_SETMASK, &prev_mask, NULL);
+#endif
+
   /* Set Guile's backtrace to match the "set guile print-stack" default.
      [N.B. The two settings are still separate.]
      But only do this after we've initialized Guile, it's nice to see a

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Eli Zaretskii]

> From: Doug Evans <xdje42@gmail.com>
> Date: Sun, 31 Aug 2014 12:07:58 -0700
> 
> Basically, current Guile (git) starts an internal thread
> (the "finalizer" thread), and libgc as of 7.4 now starts several
> marker threads by default (before 7.4.0 one needed to configure
> libgc with --enable-parallel-mark).
> 
> When other threads are running, and they haven't blocked SIGCHLD,
> then the kernel may send SIGCHLD to these threads, leaving gdb
> hung in the sigsuspend calls in linux-nat.c.

A heretic thought: is it at all a good idea to have Guile (and GC)
start threads when they run under GDB?  GDB is a single-threaded
program, so having it linked against libraries that start threads
whenever they like is IME a source of subtle problems (like this one)
and a lot of pain down the road.  Anything GDB does that affects the
global environment of the whole program (e.g., I/O redirection) will
also affect those threads, with who knows what consequences.

So maybe The Right Way of fixing these problems is configure Guile and
GC so that they never start any additional threads?

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Doug Evans]

[+ guile-devel]

On Sun, Aug 31, 2014 at 12:36 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Doug Evans <xdje42@gmail.com>
>> Date: Sun, 31 Aug 2014 12:07:58 -0700
>>
>> Basically, current Guile (git) starts an internal thread
>> (the "finalizer" thread), and libgc as of 7.4 now starts several
>> marker threads by default (before 7.4.0 one needed to configure
>> libgc with --enable-parallel-mark).
>>
>> When other threads are running, and they haven't blocked SIGCHLD,
>> then the kernel may send SIGCHLD to these threads, leaving gdb
>> hung in the sigsuspend calls in linux-nat.c.
>
> A heretic thought: is it at all a good idea to have Guile (and GC)
> start threads when they run under GDB?  GDB is a single-threaded
> program, so having it linked against libraries that start threads
> whenever they like is IME a source of subtle problems (like this one)
> and a lot of pain down the road.  Anything GDB does that affects the
> global environment of the whole program (e.g., I/O redirection) will
> also affect those threads, with who knows what consequences.
>
> So maybe The Right Way of fixing these problems is configure Guile and
> GC so that they never start any additional threads?

Users are going to want to start threads.
I've seen that already.
I think we should not shy away from them.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Eli Zaretskii]

> Date: Sun, 31 Aug 2014 13:20:48 -0700
> From: Doug Evans <xdje42@gmail.com>
> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, guile-devel <guile-devel@gnu.org>
> 
> Users are going to want to start threads.

If users want threads in Guile, they can use Guile.

In GDB, Guile is just an extension language.  I see no catastrophe in
saying there are some restrictions to what a Guile extension in GDB
can and cannot do.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Ludovic Courtès]

Eli Zaretskii <eliz@gnu.org> skribis:

>> Date: Sun, 31 Aug 2014 13:20:48 -0700
>> From: Doug Evans <xdje42@gmail.com>
>> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, guile-devel <guile-devel@gnu.org>
>> 
>> Users are going to want to start threads.
>
> If users want threads in Guile, they can use Guile.
>
> In GDB, Guile is just an extension language.  I see no catastrophe in
> saying there are some restrictions to what a Guile extension in GDB
> can and cannot do.

That can be acceptable, but requiring users to re-build Guile and GC
without thread support is not, IMO.

Ludo’.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Eli Zaretskii]

> From: ludo@gnu.org (Ludovic Courtès)
> Date: Mon, 01 Sep 2014 12:11:19 +0200
> Cc: guile-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> skribis:
> 
> > In GDB, Guile is just an extension language.  I see no catastrophe in
> > saying there are some restrictions to what a Guile extension in GDB
> > can and cannot do.
> 
> That can be acceptable, but requiring users to re-build Guile and GC
> without thread support is not, IMO.

Perhaps we should request GC and Guile to provide capabilities to
disable threads at run time, then.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Ludovic Courtès]

Eli Zaretskii <eliz@gnu.org> skribis:

>> From: ludo@gnu.org (Ludovic Courtès)
>> Date: Mon, 01 Sep 2014 12:11:19 +0200
>> Cc: guile-devel@gnu.org
>> 
>> Eli Zaretskii <eliz@gnu.org> skribis:
>> 
>> > In GDB, Guile is just an extension language.  I see no catastrophe in
>> > saying there are some restrictions to what a Guile extension in GDB
>> > can and cannot do.
>> 
>> That can be acceptable, but requiring users to re-build Guile and GC
>> without thread support is not, IMO.
>
> Perhaps we should request GC and Guile to provide capabilities to
> disable threads at run time, then.

I don’t think we need to go this far: reading the recent discussions, it
seems Doug found a way to make sure Guile’s and libgc’s internal threads
don’t receive signals that GDB is interested in, which should be enough
for practical purposes.

Ludo’.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Eli Zaretskii]

> From: ludo@gnu.org (Ludovic Courtès)
> Cc: gdb-patches@sourceware.org,  guile-devel@gnu.org
> Date: Mon, 01 Sep 2014 18:18:45 +0200
> 
> Eli Zaretskii <eliz@gnu.org> skribis:
> 
> > Perhaps we should request GC and Guile to provide capabilities to
> > disable threads at run time, then.
> 
> I don’t think we need to go this far: reading the recent discussions, it
> seems Doug found a way to make sure Guile’s and libgc’s internal threads
> don’t receive signals that GDB is interested in, which should be enough
> for practical purposes.

That's just the tip of an iceberg, I'm afraid.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Doug Evans]

On Mon, Sep 1, 2014 at 10:10 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: ludo@gnu.org (Ludovic Courtès)
>> Cc: gdb-patches@sourceware.org,  guile-devel@gnu.org
>> Date: Mon, 01 Sep 2014 18:18:45 +0200
>>
>> Eli Zaretskii <eliz@gnu.org> skribis:
>>
>> > Perhaps we should request GC and Guile to provide capabilities to
>> > disable threads at run time, then.
>>
>> I don’t think we need to go this far: reading the recent discussions, it
>> seems Doug found a way to make sure Guile’s and libgc’s internal threads
>> don’t receive signals that GDB is interested in, which should be enough
>> for practical purposes.
>
> That's just the tip of an iceberg, I'm afraid.

In the interests of seeking clarification early and clearly, can you elaborate?

I'm not saying there aren't issues, I'm just wondering if you have
anything specific that you are thinking of.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Eli Zaretskii]

> Date: Mon, 1 Sep 2014 15:04:16 -0700
> From: Doug Evans <xdje42@gmail.com>
> Cc: Ludovic Courtès <ludo@gnu.org>, 
> 	guile-devel <guile-devel@gnu.org>, 
> 	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
>> Eli Zaretskii skribis:
>>
>> > Perhaps we should request GC and Guile to provide capabilities to
>> > disable threads at run time, then.
>>
>> I don’t think we need to go this far: reading the recent discussions, it
>> seems Doug found a way to make sure Guile’s and libgc’s internal threads
>> don’t receive signals that GDB is interested in, which should be enough
>> for practical purposes.
>
> That's just the tip of an iceberg, I'm afraid.
> 
> In the interests of seeking clarification early and clearly, can you elaborate?
> 
> I'm not saying there aren't issues, I'm just wondering if you have
> anything specific that you are thinking of.

OK, but if no one except myself sees the danger, then that's about the
last time I'm writing about this, and will hold my peace thereafter.

With that out of my way...

Basically, anything you can do in Guile that somehow causes
process-wide changes would present a problem, if Guile threads are
allowed in GDB.  Here are a few examples that come to mind:

 . redirection of standard descriptors (I already mentioned this in my
   initial message in this discussion)

 . changing environment variables with setenv/unsetenv

 . changing the current directory with chdir

 . changing the process's umask

 . calling setuid/seteuid or setsid

 . changing the priority or affinity of the process

 . setting or changing signal handlers, or raising signals

 . setting or canceling interval timers

 . changing resource limits with setrlimit

The full list is almost infinite, since Guile supports foreign
libraries via libffi and libltdl.  So a Guile extension can load any
code that might call any API.

Since GDB is itself a program that uses at least some of the low-level
OS interfaces that underly the above Guile functionalities, the danger
of core GDB and threads running Guile extensions stomping onto each
other's feet is not just theoretical, IMO.

Moreover, the proposed solutions only address the issues we know about
at this time.  Both GC and Guile are being actively maintained, so
there's a real danger they will introduce more features whose use from
a separate thread will present problems beyond those known today.  I
predict this to be a constant source of maintainers' headache.  We
will probably see more incidents like this one, where we released a
seriously flawed GDB.

We had our share of similar problems in Emacs compiled with GTK, where
it turned out that GTK launches threads at will, and some of those
threads manipulate signals and even start subprocesses(!).  There are
several horror stories in the Emacs bug tracker about related
hard-to-debug crashes caused, e.g., by GTK usurping SIGCHLD while
Emacs was waiting for a subprocess to terminate.

In a single-threaded program, a well-behaved Guile extension can at
least minimize the damage by restoring the global state immediately
after doing whatever it needed the change in that state for.  But if
the extension code runs in a separate thread, there's no way of doing
that safely and reliably.

We could introduce some synchronization mechanism, whereby, for
example, GDB's main thread could cause all other threads stop in their
tracks while it is accessing some global resource, and then resumes
them.  But that means added complexity, and doesn't entirely fix the
problem (e.g., a chdir call that doesn't return to the original
directory could still cause harm to core GDB).

So I suggest to think about this now, rather than bearing the
maintenance burden in the years to come.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Doug Evans]

Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Mon, 1 Sep 2014 15:04:16 -0700
>> From: Doug Evans <xdje42@gmail.com>
>> Cc: Ludovic Courtès <ludo@gnu.org>, 
>> 	guile-devel <guile-devel@gnu.org>, 
>> 	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
>> 
>>> Eli Zaretskii skribis:
>>>
>>> > Perhaps we should request GC and Guile to provide capabilities to
>>> > disable threads at run time, then.
>>>
>>> I don’t think we need to go this far: reading the recent discussions, it
>>> seems Doug found a way to make sure Guile’s and libgc’s internal threads
>>> don’t receive signals that GDB is interested in, which should be enough
>>> for practical purposes.
>>
>> That's just the tip of an iceberg, I'm afraid.
>> 
>> In the interests of seeking clarification early and clearly, can you elaborate?
>> 
>> I'm not saying there aren't issues, I'm just wondering if you have
>> anything specific that you are thinking of.
>
> OK, but if no one except myself sees the danger, then that's about the
> last time I'm writing about this, and will hold my peace thereafter.
>
> With that out of my way...
>
> Basically, anything you can do in Guile that somehow causes
> process-wide changes would present a problem, if Guile threads are
> allowed in GDB.  Here are a few examples that come to mind:
>
>  . redirection of standard descriptors (I already mentioned this in my
>    initial message in this discussion)
>
>  . changing environment variables with setenv/unsetenv
>
>  . changing the current directory with chdir
>
>  . changing the process's umask
>
>  . calling setuid/seteuid or setsid
>
>  . changing the priority or affinity of the process
>
>  . setting or changing signal handlers, or raising signals
>
>  . setting or canceling interval timers
>
>  . changing resource limits with setrlimit
>
> The full list is almost infinite, since Guile supports foreign
> libraries via libffi and libltdl.  So a Guile extension can load any
> code that might call any API.
>
> Since GDB is itself a program that uses at least some of the low-level
> OS interfaces that underly the above Guile functionalities, the danger
> of core GDB and threads running Guile extensions stomping onto each
> other's feet is not just theoretical, IMO.
>
> Moreover, the proposed solutions only address the issues we know about
> at this time.  Both GC and Guile are being actively maintained, so
> there's a real danger they will introduce more features whose use from
> a separate thread will present problems beyond those known today.  I
> predict this to be a constant source of maintainers' headache.  We
> will probably see more incidents like this one, where we released a
> seriously flawed GDB.
>
> We had our share of similar problems in Emacs compiled with GTK, where
> it turned out that GTK launches threads at will, and some of those
> threads manipulate signals and even start subprocesses(!).  There are
> several horror stories in the Emacs bug tracker about related
> hard-to-debug crashes caused, e.g., by GTK usurping SIGCHLD while
> Emacs was waiting for a subprocess to terminate.
>
> In a single-threaded program, a well-behaved Guile extension can at
> least minimize the damage by restoring the global state immediately
> after doing whatever it needed the change in that state for.  But if
> the extension code runs in a separate thread, there's no way of doing
> that safely and reliably.
>
> We could introduce some synchronization mechanism, whereby, for
> example, GDB's main thread could cause all other threads stop in their
> tracks while it is accessing some global resource, and then resumes
> them.  But that means added complexity, and doesn't entirely fix the
> problem (e.g., a chdir call that doesn't return to the original
> directory could still cause harm to core GDB).
>
> So I suggest to think about this now, rather than bearing the
> maintenance burden in the years to come.

I'm happy to leave users to their own devices,
we can't physically prevent them from starting threads.
We pretty much leave them that way already given the myriad
of things they can do to mess up gdb without threads.

The python side of things is too silent on whether threads are supported
there.
https://sourceware.org/gdb/current/onlinedocs/gdb/Basic-Python.html#Basic-Python
The only thing it says with regards to threads is:
"GDB takes care to mark its internal file descriptors as
close-on-exec.  However, this cannot be done in a thread-safe way on
all platforms.  Your Python programs should be aware of this and
should both create new file descriptors with the close-on-exec flag
set and arrange to close unneeded file descriptors before starting a
child process."

At any rate, the task at hand is getting gdb to work well with Guile,
regardless of whether the user starts any threads,
and that's the intent of the patch.
I am happy to make the default value of GC_MARKERS be 1 (regardless
of libgc version, and if not already set by the user) when initializing Guile,
that will disable libgc marker threads.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Eli Zaretskii]

> From: Doug Evans <xdje42@gmail.com>
> Cc: ludo@gnu.org,  guile-devel@gnu.org,  gdb-patches@sourceware.org
> Date: Fri, 05 Sep 2014 01:26:28 -0700
> 
> we can't physically prevent [users] from starting threads.

Of course we can: if Guile gives us a way to disable threads, any user
extension that attempts to start a thread will simply fail.

> We pretty much leave them that way already given the myriad
> of things they can do to mess up gdb without threads.

?? The rest of GDB is compiled C code which users cannot change
without recompiling.  And it's a single-threaded code.  And it's under
our close scrutiny.  So I see no problems here.

> The python side of things is too silent on whether threads are supported
> there.
> https://sourceware.org/gdb/current/onlinedocs/gdb/Basic-Python.html#Basic-Python

I simply don't know enough about Python to discuss this; I do know
about Guile.  If Python extensions can start threads, then the same
considerations apply there.  And if the Python side didn't disallow
that until now, it doesn't sound like a good excuse to add insult to
injury on the Guile side.

> At any rate, the task at hand is getting gdb to work well with Guile,
> regardless of whether the user starts any threads,
> and that's the intent of the patch.

If we disable threads, this patch is unneeded, as are all the future
patches I envision that will have to deal with these issues.  We kill
the problem cleanly and once and for all.  From where I stand, it's a
no-brainer.

> I am happy to make the default value of GC_MARKERS be 1 (regardless
> of libgc version, and if not already set by the user) when initializing Guile,
> that will disable libgc marker threads.

That'd be fine with me, thanks.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Pedro Alves]

On 09/05/2014 09:48 AM, Eli Zaretskii wrote:
>> From: Doug Evans <xdje42@gmail.com>
>> Cc: ludo@gnu.org,  guile-devel@gnu.org,  gdb-patches@sourceware.org
>> Date: Fri, 05 Sep 2014 01:26:28 -0700
>>
>> we can't physically prevent [users] from starting threads.
> 
> Of course we can: if Guile gives us a way to disable threads, any user
> extension that attempts to start a thread will simply fail.
> 
>> We pretty much leave them that way already given the myriad
>> of things they can do to mess up gdb without threads.
> 
> ?? The rest of GDB is compiled C code which users cannot change
> without recompiling.  And it's a single-threaded code.  And it's under
> our close scrutiny.  So I see no problems here.
> 
>> The python side of things is too silent on whether threads are supported
>> there.
>> https://sourceware.org/gdb/current/onlinedocs/gdb/Basic-Python.html#Basic-Python

I'd be strongly against preventing extensions from using threads.  As an
example, tromey's wip/prototype gdb frontend written as a python extension to
gdb uses threads:

 https://gitorious.org/gdb-gui

Even GDB itself isn't really strictly single-threaded -- e.g., on
Windows, we spawn threads to handle I/O:

  ser-mingw.c:  state->thread = CreateThread (NULL, 0, thread_fn, scb, 0, &threadId);

Just last night I was debugging something in non-stop mode
where a ton of events happen behind the scenes without causing
a user-visible stop (a bunch of parallel single-steps), and
noticing how the cli/prompt becomes so unresponsive, because the event
loop handles either target events or input events in sequence, not
in parallel, and thinking that probably to completely fix this we'd
need to move stdin/readline handling to a separate thread.

Thanks,
Pedro Alves

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Eli Zaretskii]

> Date: Fri, 05 Sep 2014 11:51:00 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: ludo@gnu.org, guile-devel@gnu.org, gdb-patches@sourceware.org
> 
> I'd be strongly against preventing extensions from using threads.

Then how do you propose to deal with the difficulties I listed in one
of my previous messages?

> As an example, tromey's wip/prototype gdb frontend written as a
> python extension to gdb uses threads:

You don't need to convince me that forbidding threads takes away some
significant functionality.  This is a question of finding the right
balance, not whether threads are useful.

> Even GDB itself isn't really strictly single-threaded -- e.g., on
> Windows, we spawn threads to handle I/O:

That just means we already take some risk, where no other solution was
possible, or reasonably practical.  It does not mean we should from
now on be casual about adding more of that.  Moreover, this is _us_
doing threads, not users on whose code we have no control.

> Just last night I was debugging something in non-stop mode
> where a ton of events happen behind the scenes without causing
> a user-visible stop (a bunch of parallel single-steps), and
> noticing how the cli/prompt becomes so unresponsive, because the event
> loop handles either target events or input events in sequence, not
> in parallel, and thinking that probably to completely fix this we'd
> need to move stdin/readline handling to a separate thread.

It's fine with me to redesign GDB to be a multi-threaded program.  But
you know better than I do how deeply single-threaded is the current
GDB design.  I'm talking about allowing threads with arbitrary code
into our back-door, while GDB currently doesn't and cannot handle that
very well.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Pedro Alves]

On 09/05/2014 12:51 PM, Eli Zaretskii wrote:
>> Date: Fri, 05 Sep 2014 11:51:00 +0100
>> From: Pedro Alves <palves@redhat.com>
>> CC: ludo@gnu.org, guile-devel@gnu.org, gdb-patches@sourceware.org
>>
>> I'd be strongly against preventing extensions from using threads.
> 
> Then how do you propose to deal with the difficulties I listed in one
> of my previous messages?

As you said, both Guile and Python support loading foreign
libraries, so those difficulties aren't really specific
to multi-threading.  Even without loading foreign
libraries, it seems to me that a single-threaded extension
script can just as well mess up gdb, but doing some of the things
you list, like e.g., messing with signal handlers and timers.

So I think we should say that you mustn't change global
environment behind gdb's feet, and if you do so, you're in
undefined territory.

I thikn we also need to make clear that you can _only_ interact
with GDB through the main thread.  You can't have a random
thread call into GDB's APIs, as there's no locking.

> 
>> As an example, tromey's wip/prototype gdb frontend written as a
>> python extension to gdb uses threads:
> 
> You don't need to convince me that forbidding threads takes away some
> significant functionality.  This is a question of finding the right
> balance, not whether threads are useful.
> 
>> Even GDB itself isn't really strictly single-threaded -- e.g., on
>> Windows, we spawn threads to handle I/O:
> 
> That just means we already take some risk, where no other solution was
> possible, or reasonably practical.  It does not mean we should from
> now on be casual about adding more of that.  Moreover, this is _us_
> doing threads, not users on whose code we have no control.
> 
>> Just last night I was debugging something in non-stop mode
>> where a ton of events happen behind the scenes without causing
>> a user-visible stop (a bunch of parallel single-steps), and
>> noticing how the cli/prompt becomes so unresponsive, because the event
>> loop handles either target events or input events in sequence, not
>> in parallel, and thinking that probably to completely fix this we'd
>> need to move stdin/readline handling to a separate thread.
> 
> It's fine with me to redesign GDB to be a multi-threaded program.  But
> you know better than I do how deeply single-threaded is the current
> GDB design.  I'm talking about allowing threads with arbitrary code
> into our back-door, while GDB currently doesn't and cannot handle that
> very well.

Ack.

Thanks,
Pedro Alves

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Ludovic Courtès]

Eli Zaretskii <eliz@gnu.org> skribis:

>> From: Doug Evans <xdje42@gmail.com>
>> Cc: ludo@gnu.org,  guile-devel@gnu.org,  gdb-patches@sourceware.org
>> Date: Fri, 05 Sep 2014 01:26:28 -0700
>> 
>> we can't physically prevent [users] from starting threads.
>
> Of course we can: if Guile gives us a way to disable threads, any user
> extension that attempts to start a thread will simply fail.

What Guile provides is a configure-time switch to disable threads (the
default is to enable threads.)  However, I would find it unacceptable to
require GDB users to have a specially-configured Guile.

What I would suggest is to strongly warn against the use of threads in
the manual.  If users ignore that warning, I think it’s their problem.

We might be able to annihilate thread functionality, for instance by
‘set!’ing the relevant bindings in Guile when GDB starts up.  But that’s
fragile and it misses the point: that users are free to run whatever
code they want anyway, and have plenty of other ways to mess up with GDB
(the same applies to Python extensions.)

(Besides, I agree with Pedro that the long-term vision should be to
eventually permit multi-threaded extensions, although I understand that
it won’t happen overnight.)

Thanks,
Ludo’.

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Gary Benson]

Doug Evans wrote:
> On Sun, Aug 31, 2014 at 12:36 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> > > From: Doug Evans <xdje42@gmail.com>
> > > Date: Sun, 31 Aug 2014 12:07:58 -0700
> > >
> > > Basically, current Guile (git) starts an internal thread (the
> > > "finalizer" thread), and libgc as of 7.4 now starts several
> > > marker threads by default (before 7.4.0 one needed to configure
> > > libgc with --enable-parallel-mark).
> > >
> > > When other threads are running, and they haven't blocked
> > > SIGCHLD, then the kernel may send SIGCHLD to these threads,
> > > leaving gdb hung in the sigsuspend calls in linux-nat.c.
> >
> > A heretic thought: is it at all a good idea to have Guile (and GC)
> > start threads when they run under GDB?  GDB is a single-threaded
> > program, so having it linked against libraries that start threads
> > whenever they like is IME a source of subtle problems (like this
> > one) and a lot of pain down the road.  Anything GDB does that
> > affects the global environment of the whole program (e.g., I/O
> > redirection) will also affect those threads, with who knows what
> > consequences.
> >
> > So maybe The Right Way of fixing these problems is configure Guile
> > and GC so that they never start any additional threads?
> 
> Users are going to want to start threads.
> I've seen that already.
> I think we should not shy away from them.

This patch ensures the internal threads are created with SIGCHLD
blocked.  Does something do this for other (user started?) threads?

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Doug Evans]

On Mon, Sep 1, 2014 at 5:48 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
>> On Sun, Aug 31, 2014 at 12:36 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> > > From: Doug Evans <xdje42@gmail.com>
>> > > Date: Sun, 31 Aug 2014 12:07:58 -0700
>> > >
>> > > Basically, current Guile (git) starts an internal thread (the
>> > > "finalizer" thread), and libgc as of 7.4 now starts several
>> > > marker threads by default (before 7.4.0 one needed to configure
>> > > libgc with --enable-parallel-mark).
>> > >
>> > > When other threads are running, and they haven't blocked
>> > > SIGCHLD, then the kernel may send SIGCHLD to these threads,
>> > > leaving gdb hung in the sigsuspend calls in linux-nat.c.
>> >
>> > A heretic thought: is it at all a good idea to have Guile (and GC)
>> > start threads when they run under GDB?  GDB is a single-threaded
>> > program, so having it linked against libraries that start threads
>> > whenever they like is IME a source of subtle problems (like this
>> > one) and a lot of pain down the road.  Anything GDB does that
>> > affects the global environment of the whole program (e.g., I/O
>> > redirection) will also affect those threads, with who knows what
>> > consequences.
>> >
>> > So maybe The Right Way of fixing these problems is configure Guile
>> > and GC so that they never start any additional threads?
>>
>> Users are going to want to start threads.
>> I've seen that already.
>> I think we should not shy away from them.
>
> This patch ensures the internal threads are created with SIGCHLD
> blocked.  Does something do this for other (user started?) threads?

What to do for user-started threads is an open question, both for
Python and Guile.

For reference sake, see PR 17314.
[Still working on a testcase.  What I added to the PR is just a
snippet to get thoughts down.]

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Doug Evans]

Doug Evans <xdje42@gmail.com> writes:
> Hi.
>
> This patch fixes PR 17247.
>
> Basically, current Guile (git) starts an internal thread
> (the "finalizer" thread), and libgc as of 7.4 now starts several
> marker threads by default (before 7.4.0 one needed to configure
> libgc with --enable-parallel-mark).
>
> When other threads are running, and they haven't blocked SIGCHLD,
> then the kernel may send SIGCHLD to these threads, leaving gdb
> hung in the sigsuspend calls in linux-nat.c.
>
> P.S. I have a tentative patch for PR 17314, which also fixes 17247,
> but for a bit of robustness I'm applying both.
>
> 2014-08-31  Doug Evans  <xdje42@gmail.com>
>
> 	PR 17247
> 	* guile.c: #include <signal.h>.
> 	(_initialize_guile): Block SIGCHLD while initializing Guile.
>
> 	Replaces the following, which is reverted.
>
> 	2014-07-26  Doug Evans  <xdje42@gmail.com>
>
> 	PR 17185
> 	* configure.ac: Add check for header gc/gc.h.
> 	Add check for function setenv.
> 	* configure: Regenerate.
> 	* config.in: Regenerate.
> 	* guile/guile.c (_initialize_guile): Add workaround for libgc 7.4.0.

Hi.
I have checked in this slight variation to trunk,
and will commit it to the 7.8 branch tomorrow.

commit 92d8d229d9a310ebfcfc13bf4a75a286c1add1ac
Author: Doug Evans <xdje42@gmail.com>
Date:   Mon Sep 8 22:45:34 2014 -0700

    Fix for PR 17247: Block SIGCHLD while initializing Guile.
    
    The problem here is that if a thread other than gdb's main thread
    gets a SIGCHLD (it's an asynchronous signal so the kernel will
    essentially pick a random thread) then gdb will hang if it is
    in sigsuspend when the SIGCHLD is delivered.  The other thread
    will see the signal and the sigsuspend won't "wake up".
    
    Guile and libgc should be blocking SIGCHLD in their threads,
    but we need to work with Guile 2.0 and libgc 7.4.
    The problem first shows up in libgc 7.4 because it is the first
    release that enables multiple marker threads by default.
    
    gdb/ChangeLog:
    
    	PR 17247
    	* guile.c: #include <signal.h>.
    	(_initialize_guile): Block SIGCHLD while initializing Guile.
    
    	Replaces the following, which is reverted.
    
    	2014-07-26  Doug Evans  <xdje42@gmail.com>
    
    	PR 17185
    	* configure.ac: Add check for header gc/gc.h.
    	Add check for function setenv.
    	* configure: Regenerate.
    	* config.in: Regenerate.
    	* guile/guile.c (_initialize_guile): Add workaround for libgc 7.4.0.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ad5ef6f..9d3f392 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,22 @@
 2014-09-08  Doug Evans  <xdje42@gmail.com>
 
+	PR 17247
+	* guile.c: #include <signal.h>.
+	(_initialize_guile): Block SIGCHLD while initializing Guile.
+
+	Replaces the following, which is reverted.
+
+	2014-07-26  Doug Evans  <xdje42@gmail.com>
+
+	PR 17185
+	* configure.ac: Add check for header gc/gc.h.
+	Add check for function setenv.
+	* configure: Regenerate.
+	* config.in: Regenerate.
+	* guile/guile.c (_initialize_guile): Add workaround for libgc 7.4.0.
+
+2014-09-08  Doug Evans  <xdje42@gmail.com>
+
 	* guile/scm-cmd.c (gdbscm_parse_command_name): Replace magic number
 	with named constant.  Fix style of pointer comparison.
 	* python/py-cmd.c (gdbpy_parse_command_name): Ditto.
diff --git a/gdb/config.in b/gdb/config.in
index b853412..f1593bd 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -141,9 +141,6 @@
 /* Define if <sys/procfs.h> has fpregset_t. */
 #undef HAVE_FPREGSET_T
 
-/* Define to 1 if you have the <gc/gc.h> header file. */
-#undef HAVE_GC_GC_H
-
 /* Define to 1 if you have the `getgid' function. */
 #undef HAVE_GETGID
 
@@ -348,9 +345,6 @@
 /* Define to 1 if you have the `scm_new_smob' function. */
 #undef HAVE_SCM_NEW_SMOB
 
-/* Define to 1 if you have the `setenv' function. */
-#undef HAVE_SETENV
-
 /* Define to 1 if you have the `setlocale' function. */
 #undef HAVE_SETLOCALE
 
diff --git a/gdb/configure b/gdb/configure
index bf141f1..350ec0c 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -9276,32 +9276,6 @@ else
 fi
 
 
-# PR 17185, see if we can get the libgc version to see if we need
-# to apply the workaround.
-for ac_header in gc/gc.h
-do :
-  ac_fn_c_check_header_mongrel "$LINENO" "gc/gc.h" "ac_cv_header_gc_gc_h" "$ac_includes_default"
-if test "x$ac_cv_header_gc_gc_h" = x""yes; then :
-  cat >>confdefs.h <<_ACEOF
-#define HAVE_GC_GC_H 1
-_ACEOF
-
-fi
-
-done
-
-for ac_func in setenv
-do :
-  ac_fn_c_check_func "$LINENO" "setenv" "ac_cv_func_setenv"
-if test "x$ac_cv_func_setenv" = x""yes; then :
-  cat >>confdefs.h <<_ACEOF
-#define HAVE_SETENV 1
-_ACEOF
-
-fi
-done
-
-
 # --------------------- #
 # Check for libmcheck.  #
 # --------------------- #
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 26c8ecf..ab7b1c2 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1233,11 +1233,6 @@ AC_SUBST(GUILE_CPPFLAGS)
 AC_SUBST(GUILE_LIBS)
 AM_CONDITIONAL(HAVE_GUILE, test "${have_libguile}" != no)
 
-# PR 17185, see if we can get the libgc version to see if we need
-# to apply the workaround.
-AC_CHECK_HEADERS(gc/gc.h)
-AC_CHECK_FUNCS([setenv])
-
 # --------------------- #
 # Check for libmcheck.  #
 # --------------------- #
diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
index 575bb6c..97da042 100644
--- a/gdb/guile/guile.c
+++ b/gdb/guile/guile.c
@@ -34,10 +34,8 @@
 #ifdef HAVE_GUILE
 #include "guile.h"
 #include "guile-internal.h"
-#ifdef HAVE_GC_GC_H
-#include <gc/gc.h> /* PR 17185 */
-#endif
 #endif
+#include <signal.h>
 
 /* The Guile version we're using.
    We *could* use the macros in libguile/version.h but that would preclude
@@ -833,39 +831,46 @@ extern initialize_file_ftype _initialize_guile;
 void
 _initialize_guile (void)
 {
-  char *msg;
-
   install_gdb_commands ();
 
 #if HAVE_GUILE
-  /* The Python support puts the C side in module "_gdb", leaving the Python
-     side to define module "gdb" which imports "_gdb".  There is evidently no
-     similar convention in Guile so we skip this.  */
-
-  /* PR 17185 There are problems with using libgc 7.4.0.
-     Copy over the workaround Guile uses (Guile is working around a different
-     problem, but the workaround is the same).  */
-#if (GC_VERSION_MAJOR == 7 && GC_VERSION_MINOR == 4 && GC_VERSION_MICRO == 0)
-  /* The bug is only known to appear with pthreads.  We assume any system
-     using pthreads also uses setenv (and not putenv).  That is why we don't
-     have a similar call to putenv here.  */
-#if defined (HAVE_SETENV)
-  setenv ("GC_MARKERS", "1", 1);
+  {
+#ifdef HAVE_SIGPROCMASK
+    sigset_t sigchld_mask, prev_mask;
 #endif
+
+    /* The Python support puts the C side in module "_gdb", leaving the Python
+       side to define module "gdb" which imports "_gdb".  There is evidently no
+       similar convention in Guile so we skip this.  */
+
+#ifdef HAVE_SIGPROCMASK
+    /* Before we initialize Guile, block SIGCHLD.
+       This is done so that all threads created during Guile initialization
+       have SIGCHLD blocked.  PR 17247.
+       Really libgc and Guile should do this, but we need to work with
+       libgc 7.4.x.  */
+    sigemptyset (&sigchld_mask);
+    sigaddset (&sigchld_mask, SIGCHLD);
+    sigprocmask (SIG_BLOCK, &sigchld_mask, &prev_mask);
+#endif
+
+    /* scm_with_guile is the most portable way to initialize Guile.
+       Plus we need to initialize the Guile support while in Guile mode
+       (e.g., called from within a call to scm_with_guile).  */
+    scm_with_guile (call_initialize_gdb_module, NULL);
+
+#ifdef HAVE_SIGPROCMASK
+    sigprocmask (SIG_SETMASK, &prev_mask, NULL);
 #endif
 
-  /* scm_with_guile is the most portable way to initialize Guile.
-     Plus we need to initialize the Guile support while in Guile mode
-     (e.g., called from within a call to scm_with_guile).  */
-  scm_with_guile (call_initialize_gdb_module, NULL);
-
-  /* Set Guile's backtrace to match the "set guile print-stack" default.
-     [N.B. The two settings are still separate.]
-     But only do this after we've initialized Guile, it's nice to see a
-     backtrace if there's an error during initialization.
-     OTOH, if the error is that gdb/init.scm wasn't found because gdb is being
-     run from the build tree, the backtrace is more noise than signal.
-     Sigh.  */
-  gdbscm_set_backtrace (0);
+    /* Set Guile's backtrace to match the "set guile print-stack" default.
+       [N.B. The two settings are still separate.]
+       But only do this after we've initialized Guile, it's nice to see a
+       backtrace if there's an error during initialization.
+       OTOH, if the error is that gdb/init.scm wasn't found because gdb is
+       being run from the build tree, the backtrace is more noise than signal.
+       Sigh.  */
+    gdbscm_set_backtrace (0);
+  }
 #endif
 }

Re: [PATCH][PR guile/17247] Block SIGCHLD while initializing Guile

[Doug Evans]

Doug Evans <xdje42@gmail.com> writes:
> Doug Evans <xdje42@gmail.com> writes:
>> Hi.
>>
>> This patch fixes PR 17247.
>>
>> Basically, current Guile (git) starts an internal thread
>> (the "finalizer" thread), and libgc as of 7.4 now starts several
>> marker threads by default (before 7.4.0 one needed to configure
>> libgc with --enable-parallel-mark).
>>
>> When other threads are running, and they haven't blocked SIGCHLD,
>> then the kernel may send SIGCHLD to these threads, leaving gdb
>> hung in the sigsuspend calls in linux-nat.c.
>>
>> P.S. I have a tentative patch for PR 17314, which also fixes 17247,
>> but for a bit of robustness I'm applying both.
>>
>> 2014-08-31  Doug Evans  <xdje42@gmail.com>
>>
>> 	PR 17247
>> 	* guile.c: #include <signal.h>.
>> 	(_initialize_guile): Block SIGCHLD while initializing Guile.
>>
>> 	Replaces the following, which is reverted.
>>
>> 	2014-07-26  Doug Evans  <xdje42@gmail.com>
>>
>> 	PR 17185
>> 	* configure.ac: Add check for header gc/gc.h.
>> 	Add check for function setenv.
>> 	* configure: Regenerate.
>> 	* config.in: Regenerate.
>> 	* guile/guile.c (_initialize_guile): Add workaround for libgc 7.4.0.
>
> Hi.
> I have checked in this slight variation to trunk,
> and will commit it to the 7.8 branch tomorrow.
>
> [...]
>     gdb/ChangeLog:
>     
>     	PR 17247
>     	* guile.c: #include <signal.h>.
>     	(_initialize_guile): Block SIGCHLD while initializing Guile.
>     
>     	Replaces the following, which is reverted.
>     
>     	2014-07-26  Doug Evans  <xdje42@gmail.com>
>     
>     	PR 17185
>     	* configure.ac: Add check for header gc/gc.h.
>     	Add check for function setenv.
>     	* configure: Regenerate.
>     	* config.in: Regenerate.
>     	* guile/guile.c (_initialize_guile): Add workaround for libgc 7.4.0.

Pushed to 7.8 branch as well.