[gdb/testsuite] Fix hang in fork-running-state.c

[gdb/testsuite] Fix hang in fork-running-state.c

[Tom de Vries]

Hi,

When I run make check:
...
$ cd build/gdb
$ make check 2>&1 | tee ../CHECKLOG.gdb
...
I see after ~30m the summary of the test run printed, but make still hangs.

This seems to be due to some sleeping processes:
...
$ ps  fx | grep fork-run
 6475 ?        S      0:00 gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
 6451 ?        S      0:00 gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
 6427 ?        S      0:00 gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
...

Killing the sleeping processes like this:
...
kill -9 $(ps -A  | grep fork-running-st | awk '{print $1}')
...
allows make to finish.

If I isolate one debug session from fork-running-state.exp that causes one of
these sleeping processes, we get:
...
(gdb) set non-stop on
(gdb) break main
Breakpoint 1 at 0x400665: file src/gdb/testsuite/gdb.base/fork-running-state.c, line 52.
(gdb) run 
Starting program: build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state 

Breakpoint 1, main () at src/gdb/testsuite/gdb.base/fork-running-state.c:52
52        save_parent = getpid ();
(gdb) set detach-on-fork on
(gdb) set follow-fork parent
(gdb) continue &
Continuing.
[Detaching after fork from child process 18797]
(gdb) info threads
  Id   Target Id         Frame 
* 1    process 18793 "fork-running-st" (running)
(gdb) set print inferior-events off
(gdb) kill inferior 1
...
So, AFAIU, the hanging process is the child process that gdb detaches from.

There's an alarm set in main before the fork, but alarms are not preserved in
the fork child:
...
$ man alarm
   ...
NOTES
       Alarms created by alarm() are preserved across execve(2) and are not inherited by children created via fork(2).
...
So, AFAIU, once the parent is killed, there's no alarm to terminate the child.

The patch fixes this by moving the setting of the alarm into the
fork_main/fork_child functions, making sure that an alarm will trigger for
the child.

Tested with make check on x86_64.

OK for trunk?

Thanks,
- Tom

[gdb/testsuite] Fix hang in fork-running-state.c

2018-06-12  Tom de Vries  <tdevries@suse.de>

	PR testsuite/23269
	* gdb.base/fork-running-state.c (main): Move setting of alarm ...
	(fork_child): ... here, and ...
	(fork_parent): ... here.

---
 gdb/testsuite/gdb.base/fork-running-state.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.base/fork-running-state.c b/gdb/testsuite/gdb.base/fork-running-state.c
index 8ea4739609..65ca942ea0 100644
--- a/gdb/testsuite/gdb.base/fork-running-state.c
+++ b/gdb/testsuite/gdb.base/fork-running-state.c
@@ -27,6 +27,9 @@ int save_parent;
 static int
 fork_child (void)
 {
+  /* Don't run forever.  */
+  alarm (180);
+
   while (1)
     pause ();
 
@@ -38,6 +41,9 @@ fork_child (void)
 static int
 fork_parent (void)
 {
+  /* Don't run forever.  */
+  alarm (180);
+
   while (1)
     pause ();
 
@@ -51,9 +57,6 @@ main (void)
 
   save_parent = getpid ();
 
-  /* Don't run forever.  */
-  alarm (180);
-
   /* The parent and child should basically run forever without
      tripping on any debug event.  We want to check that GDB updates
      the parent and child running states correctly right after the

Re: [gdb/testsuite] Fix hang in fork-running-state.c

[Pedro Alves]

On 06/12/2018 04:51 PM, Tom de Vries wrote:

> ...
> So, AFAIU, the hanging process is the child process that gdb detaches from.
> 
> There's an alarm set in main before the fork, but alarms are not preserved in
> the fork child:
> ...
> $ man alarm
>    ...
> NOTES
>        Alarms created by alarm() are preserved across execve(2) and are not inherited by children created via fork(2).

Whoops.

> ...
> So, AFAIU, once the parent is killed, there's no alarm to terminate the child.

Indeed.

Your patch is definitely a good idea.  Please push.

However, I think that still leaves an unnecessary delay until
the detached child/parent terminate.  They used to exit themselves,
but that caused a race, so they no longer do, see here:

 https://sourceware.org/ml/gdb-patches/2018-03/msg00588.html

Sounds like the best would be to restore the self-killing,
but make it controlled by a variable that gdb sets, depending
on whether gdb is staying attached to the child/parent.

Thanks,
Pedro Alves

[PATCH] Avoid gdb.base/fork-running-state.exp lingering processes (Re: [gdb/testsuite] Fix hang in fork-running-state.c)

[Pedro Alves]

On 06/13/2018 01:10 PM, Pedro Alves wrote:

> Your patch is definitely a good idea.  Please push.
> 
> However, I think that still leaves an unnecessary delay until
> the detached child/parent terminate.  They used to exit themselves,
> but that caused a race, so they no longer do, see here:
> 
>  https://sourceware.org/ml/gdb-patches/2018-03/msg00588.html
> 
> Sounds like the best would be to restore the self-killing,
> but make it controlled by a variable that gdb sets, depending
> on whether gdb is staying attached to the child/parent.

Like so?

From 1d7047ff526a4bbd6e2f4336c046b3438048808f Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 14 Jun 2018 13:00:32 +0100
Subject: [PATCH] Avoid gdb.base/fork-running-state.exp lingering processes

Currently, the gdb.base/fork-running-state.exp testcase leaves a few
processes lingering until a 3 minutes alarm kills them:

 pedro    28308     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
 pedro    28340     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
 pedro    28372     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
 pedro    28400     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
 pedro    28431     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
 pedro    28463     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state

Those processes used to kill themselves, but that was changed by
commit f50d8a2eaea0 ("Fix gdb.base/fork-running-state.exp race").

This commit restores the self-killing, but only in the cases gdb won't
try killing the processes, thus avoiding the old race.

(The restored code in fork_parent isn't exactly the same as it was.
In this version, we're exiting immediately when 'wait' returns
success, while in the old version we'd loop again and end up in the
perror call.  The output from that perror call is not expected by the
"kill inferior" tests, and would result in a test FAIL.)

gdb/testsuite/ChangeLog:
2018-06-14  Pedro Alves  <palves@redhat.com>

	* gdb.base/fork-running-state.c: Include <errno.h>.
	(exit_if_relative_exits): New.
	(fork_child): If 'exit_if_relative_exits' is true, exit if the parent
	exits.
	(fork_parent): If 'exit_if_relative_exits' is true, exit if the
	child exits.
---
 gdb/testsuite/gdb.base/fork-running-state.c   | 39 +++++++++++++++++++++++++--
 gdb/testsuite/gdb.base/fork-running-state.exp |  7 +++++
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.base/fork-running-state.c b/gdb/testsuite/gdb.base/fork-running-state.c
index 65ca942ea02..0037cb5a5e1 100644
--- a/gdb/testsuite/gdb.base/fork-running-state.c
+++ b/gdb/testsuite/gdb.base/fork-running-state.c
@@ -19,9 +19,15 @@
 #include <stdio.h>
 #include <sys/types.h>
 #include <sys/wait.h>
+#include <errno.h>
 
 int save_parent;
 
+/* Variable set by GDB.  If true, then a fork child (or parent) exits
+   if its parent (or child) exits.  Otherwise the process waits
+   forever until either GDB or the alarm kills it.  */
+volatile int exit_if_relative_exits = 0;
+
 /* The fork child.  Just runs forever.  */
 
 static int
@@ -31,7 +37,20 @@ fork_child (void)
   alarm (180);
 
   while (1)
-    pause ();
+    {
+      if (exit_if_relative_exits)
+	{
+	  sleep (1);
+
+	  /* Exit if GDB kills the parent.  */
+	  if (getppid () != save_parent)
+	    break;
+	  if (kill (getppid (), 0) != 0)
+	    break;
+	}
+      else
+	pause ();
+    }
 
   return 0;
 }
@@ -45,7 +64,23 @@ fork_parent (void)
   alarm (180);
 
   while (1)
-    pause ();
+    {
+      if (exit_if_relative_exits)
+	{
+	  int res = wait (NULL);
+	  if (res == -1 && errno == EINTR)
+	    continue;
+	  else if (res == -1)
+	    {
+	      perror ("wait");
+	      return 1;
+	    }
+	  else
+	    return 0;
+	}
+      else
+	pause ();
+    }
 
   return 0;
 }
diff --git a/gdb/testsuite/gdb.base/fork-running-state.exp b/gdb/testsuite/gdb.base/fork-running-state.exp
index 27ed8a43e93..c15bc53f7a0 100644
--- a/gdb/testsuite/gdb.base/fork-running-state.exp
+++ b/gdb/testsuite/gdb.base/fork-running-state.exp
@@ -70,6 +70,13 @@ proc do_test { detach_on_fork follow_fork non_stop schedule_multiple } {
 	gdb_test_no_output "set schedule-multiple $schedule_multiple"
     }
 
+    # If we're detaching from the parent (or child), then tell it to
+    # exit itself when its child (or parent) exits.  If we stay
+    # attached, we take care of killing it.
+    if {$detach_on_fork == "on"} {
+	gdb_test "print exit_if_relative_exits = 1" " = 1"
+    }
+
     set test "continue &"
     gdb_test_multiple $test $test {
 	-re "$gdb_prompt " {
-- 
2.14.3

Re: [PATCH] Avoid gdb.base/fork-running-state.exp lingering processes (Re: [gdb/testsuite] Fix hang in fork-running-state.c)

[Tom de Vries]

On 06/14/2018 02:59 PM, Pedro Alves wrote:
> On 06/13/2018 01:10 PM, Pedro Alves wrote:
> 
>> Your patch is definitely a good idea.  Please push.
>>
>> However, I think that still leaves an unnecessary delay until
>> the detached child/parent terminate.  They used to exit themselves,
>> but that caused a race, so they no longer do, see here:
>>
>>  https://sourceware.org/ml/gdb-patches/2018-03/msg00588.html
>>
>> Sounds like the best would be to restore the self-killing,
>> but make it controlled by a variable that gdb sets, depending
>> on whether gdb is staying attached to the child/parent.
> 
> Like so?
> 

I tried it out, and it works for me.

The approach looks ok to me.

I guess the alarms (180) calls are no longer necessary?

Thanks,
- Tom

> From 1d7047ff526a4bbd6e2f4336c046b3438048808f Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 14 Jun 2018 13:00:32 +0100
> Subject: [PATCH] Avoid gdb.base/fork-running-state.exp lingering processes
> 
> Currently, the gdb.base/fork-running-state.exp testcase leaves a few
> processes lingering until a 3 minutes alarm kills them:
> 
>  pedro    28308     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
>  pedro    28340     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
>  pedro    28372     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
>  pedro    28400     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
>  pedro    28431     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
>  pedro    28463     1  0 13:55 ?        00:00:00 /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/fork-running-state/fork-running-state
> 
> Those processes used to kill themselves, but that was changed by
> commit f50d8a2eaea0 ("Fix gdb.base/fork-running-state.exp race").
> 
> This commit restores the self-killing, but only in the cases gdb won't
> try killing the processes, thus avoiding the old race.
> 
> (The restored code in fork_parent isn't exactly the same as it was.
> In this version, we're exiting immediately when 'wait' returns
> success, while in the old version we'd loop again and end up in the
> perror call.  The output from that perror call is not expected by the
> "kill inferior" tests, and would result in a test FAIL.)
> 
> gdb/testsuite/ChangeLog:
> 2018-06-14  Pedro Alves  <palves@redhat.com>
> 
> 	* gdb.base/fork-running-state.c: Include <errno.h>.
> 	(exit_if_relative_exits): New.
> 	(fork_child): If 'exit_if_relative_exits' is true, exit if the parent
> 	exits.
> 	(fork_parent): If 'exit_if_relative_exits' is true, exit if the
> 	child exits.
> ---
>  gdb/testsuite/gdb.base/fork-running-state.c   | 39 +++++++++++++++++++++++++--
>  gdb/testsuite/gdb.base/fork-running-state.exp |  7 +++++
>  2 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/fork-running-state.c b/gdb/testsuite/gdb.base/fork-running-state.c
> index 65ca942ea02..0037cb5a5e1 100644
> --- a/gdb/testsuite/gdb.base/fork-running-state.c
> +++ b/gdb/testsuite/gdb.base/fork-running-state.c
> @@ -19,9 +19,15 @@
>  #include <stdio.h>
>  #include <sys/types.h>
>  #include <sys/wait.h>
> +#include <errno.h>
>  
>  int save_parent;
>  
> +/* Variable set by GDB.  If true, then a fork child (or parent) exits
> +   if its parent (or child) exits.  Otherwise the process waits
> +   forever until either GDB or the alarm kills it.  */
> +volatile int exit_if_relative_exits = 0;
> +
>  /* The fork child.  Just runs forever.  */
>  
>  static int
> @@ -31,7 +37,20 @@ fork_child (void)
>    alarm (180);
>  
>    while (1)
> -    pause ();
> +    {
> +      if (exit_if_relative_exits)
> +	{
> +	  sleep (1);
> +
> +	  /* Exit if GDB kills the parent.  */
> +	  if (getppid () != save_parent)
> +	    break;
> +	  if (kill (getppid (), 0) != 0)
> +	    break;
> +	}
> +      else
> +	pause ();
> +    }
>  
>    return 0;
>  }
> @@ -45,7 +64,23 @@ fork_parent (void)
>    alarm (180);
>  
>    while (1)
> -    pause ();
> +    {
> +      if (exit_if_relative_exits)
> +	{
> +	  int res = wait (NULL);
> +	  if (res == -1 && errno == EINTR)
> +	    continue;
> +	  else if (res == -1)
> +	    {
> +	      perror ("wait");
> +	      return 1;
> +	    }
> +	  else
> +	    return 0;
> +	}
> +      else
> +	pause ();
> +    }
>  
>    return 0;
>  }
> diff --git a/gdb/testsuite/gdb.base/fork-running-state.exp b/gdb/testsuite/gdb.base/fork-running-state.exp
> index 27ed8a43e93..c15bc53f7a0 100644
> --- a/gdb/testsuite/gdb.base/fork-running-state.exp
> +++ b/gdb/testsuite/gdb.base/fork-running-state.exp
> @@ -70,6 +70,13 @@ proc do_test { detach_on_fork follow_fork non_stop schedule_multiple } {
>  	gdb_test_no_output "set schedule-multiple $schedule_multiple"
>      }
>  
> +    # If we're detaching from the parent (or child), then tell it to
> +    # exit itself when its child (or parent) exits.  If we stay
> +    # attached, we take care of killing it.
> +    if {$detach_on_fork == "on"} {
> +	gdb_test "print exit_if_relative_exits = 1" " = 1"
> +    }
> +
>      set test "continue &"
>      gdb_test_multiple $test $test {
>  	-re "$gdb_prompt " {
> 

Re: [PATCH] Avoid gdb.base/fork-running-state.exp lingering processes (Re: [gdb/testsuite] Fix hang in fork-running-state.c)

[Pedro Alves]

On 06/14/2018 05:06 PM, Tom de Vries wrote:
> On 06/14/2018 02:59 PM, Pedro Alves wrote:

>> Like so?
>>
> 
> I tried it out, and it works for me.
> 
> The approach looks ok to me.
> 

Great, I'll push it in in a bit.

> I guess the alarms (180) calls are no longer necessary?

It's better to still have them, in case something goes wrong,
gdb crashes or the test fails, etc.

Thanks,
Pedro Alves