[PATCH 0/4] PR14161: a partial fix

[PATCH 0/4] PR14161: a partial fix

[Yao Qi]

Hi,
PR14161 reports that agent socket '/tmp/gdb_ustXXXX' is not removed when
program is done.  This patch series is to address this problem partially
only in GDBserver side (in patch 3/4) and kfail it temporarily in GDB
side (in patch 4/4) due to some limitations on threads control.
Patch 1/4 is a new test case to show this problem.

[PATCH 2/4] Remove socket file at exit.

[Yao Qi]

This patch is to add code to call 'unlink' to make sure agent can remove socket
when inferior (after detached from GDB/GDBserver) exits.  It fixes the fail
below:

  FAIL: gdb.trace/strace.exp: remove_socket_after_detach: socket file removed

gdb/gdbserver:

2012-06-09  Yao Qi  <yao@codesourcery.com>

	PR gdb/14161.
	* tracepoint.c (gdb_agent_socket_init): Removed.
	(gdb_agent_remove_socket): New.
	(gdb_agent_helper_thread): Call gdb_agent_remove_socket at
	exit.
	Inline gdb_agent_socket_init here.
---
 gdb/gdbserver/tracepoint.c |   56 +++++++++++++++++++++----------------------
 1 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index f103dfc..4e6c3ec 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -6748,29 +6748,6 @@ init_named_socket (const char *name)
   return fd;
 }
 
-static int
-gdb_agent_socket_init (void)
-{
-  int result, fd;
-  char name[UNIX_PATH_MAX];
-
-  result = xsnprintf (name, UNIX_PATH_MAX, "%s/gdb_ust%d",
-		      SOCK_DIR, getpid ());
-  if (result >= UNIX_PATH_MAX)
-    {
-      trace_debug ("string overflow allocating socket name");
-      return -1;
-    }
-
-  fd = init_named_socket (name);
-  if (fd < 0)
-    warning ("Error initializing named socket (%s) for communication with the "
-	     "ust helper thread. Check that directory exists and that it "
-	     "is writable.", name);
-
-  return fd;
-}
-
 #ifdef HAVE_UST
 
 /* The next marker to be returned on a qTsSTM command.  */
@@ -6995,27 +6972,48 @@ gdb_ust_init (void)
 #endif /* HAVE_UST */
 
 #include <sys/syscall.h>
+#include <stdlib.h>
+
+static char agent_socket_name[UNIX_PATH_MAX];
+
+static void
+gdb_agent_remove_socket (void)
+{
+  unlink (agent_socket_name);
+}
 
 /* Helper thread of agent.  */
 
 static void *
 gdb_agent_helper_thread (void *arg)
 {
-  int listen_fd;
+  atexit (gdb_agent_remove_socket);
 
   while (1)
     {
-      listen_fd = gdb_agent_socket_init ();
+      int listen_fd, result;
 
-      if (helper_thread_id == 0)
-	helper_thread_id = syscall (SYS_gettid);
+      result = xsnprintf (agent_socket_name, UNIX_PATH_MAX, "%s/gdb_ust%d",
+			  SOCK_DIR, getpid ());
+      if (result >= UNIX_PATH_MAX)
+	{
+	  trace_debug ("string overflow allocating socket name");
+	  break;
+	}
 
-      if (listen_fd == -1)
+      listen_fd = init_named_socket (agent_socket_name);
+      if (listen_fd < 0)
 	{
-	  warning ("could not create sync socket\n");
+	  warning ("Error initializing named socket (%s) for communication "
+		   "with the ust helper thread. Check that directory exists"
+		   " and that it is writable.", agent_socket_name);
 	  break;
+
 	}
 
+      if (helper_thread_id == 0)
+	helper_thread_id = syscall (SYS_gettid);
+
       while (1)
 	{
 	  socklen_t tmp;
-- 
1.7.0.4

Re: [PATCH 2/4] Remove socket file at exit.

[Pedro Alves]

On 06/09/2012 01:46 PM, Yao Qi wrote:

> This patch is to add code to call 'unlink' to make sure agent can remove socket
> when inferior (after detached from GDB/GDBserver) exits.  It fixes the fail
> below:
> 
>   FAIL: gdb.trace/strace.exp: remove_socket_after_detach: socket file removed
> 


(Just as reminder, ideally, at check in time, the patches would be sequenced
to not introduce any FAILs along the way.)

> gdb/gdbserver:
> 
> 2012-06-09  Yao Qi  <yao@codesourcery.com>
> 
> 	PR gdb/14161.
> 	* tracepoint.c (gdb_agent_socket_init): Removed.


"Remove".

> 	(gdb_agent_remove_socket): New.
> 	(gdb_agent_helper_thread): Call gdb_agent_remove_socket at
> 	exit.
> 	Inline gdb_agent_socket_init here.


But why inline?  We should just need to move the "name" local
of gdb_agent_socket_init a global?  (btw, the 'agent_socket_name'
global is missing from the ChangeLog.)

> ---
>  gdb/gdbserver/tracepoint.c |   56 +++++++++++++++++++++----------------------
>  1 files changed, 27 insertions(+), 29 deletions(-)
> 
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index f103dfc..4e6c3ec 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -6748,29 +6748,6 @@ init_named_socket (const char *name)
>    return fd;
>  }
>  
> -static int
> -gdb_agent_socket_init (void)
> -{
> -  int result, fd;
> -  char name[UNIX_PATH_MAX];
> -
> -  result = xsnprintf (name, UNIX_PATH_MAX, "%s/gdb_ust%d",
> -		      SOCK_DIR, getpid ());
> -  if (result >= UNIX_PATH_MAX)
> -    {
> -      trace_debug ("string overflow allocating socket name");
> -      return -1;
> -    }
> -
> -  fd = init_named_socket (name);
> -  if (fd < 0)
> -    warning ("Error initializing named socket (%s) for communication with the "
> -	     "ust helper thread. Check that directory exists and that it "
> -	     "is writable.", name);
> -
> -  return fd;
> -}
> -
>  #ifdef HAVE_UST
>  
>  /* The next marker to be returned on a qTsSTM command.  */
> @@ -6995,27 +6972,48 @@ gdb_ust_init (void)
>  #endif /* HAVE_UST */
>  
>  #include <sys/syscall.h>
> +#include <stdlib.h>
> +
> +static char agent_socket_name[UNIX_PATH_MAX];
> +
> +static void
> +gdb_agent_remove_socket (void)
> +{
> +  unlink (agent_socket_name);
> +}
>  
>  /* Helper thread of agent.  */
>  
>  static void *
>  gdb_agent_helper_thread (void *arg)
>  {
> -  int listen_fd;
> +  atexit (gdb_agent_remove_socket);
>  
>    while (1)
>      {
> -      listen_fd = gdb_agent_socket_init ();
> +      int listen_fd, result;
>  
> -      if (helper_thread_id == 0)
> -	helper_thread_id = syscall (SYS_gettid);
> +      result = xsnprintf (agent_socket_name, UNIX_PATH_MAX, "%s/gdb_ust%d",
> +			  SOCK_DIR, getpid ());
> +      if (result >= UNIX_PATH_MAX)
> +	{
> +	  trace_debug ("string overflow allocating socket name");
> +	  break;
> +	}
>  
> -      if (listen_fd == -1)
> +      listen_fd = init_named_socket (agent_socket_name);
> +      if (listen_fd < 0)
>  	{
> -	  warning ("could not create sync socket\n");
> +	  warning ("Error initializing named socket (%s) for communication "
> +		   "with the ust helper thread. Check that directory exists"
> +		   " and that it is writable.", agent_socket_name);
>  	  break;
> +
>  	}
>  
> +      if (helper_thread_id == 0)
> +	helper_thread_id = syscall (SYS_gettid);
> +
>        while (1)
>  	{
>  	  socklen_t tmp;



-- 
Pedro Alves

Re: [PATCH 2/4] Remove socket file at exit.

[Yao Qi]

On Tuesday 12 June 2012 23:14:32 Pedro Alves wrote:
> >       (gdb_agent_remove_socket): New.
> >       (gdb_agent_helper_thread): Call gdb_agent_remove_socket at
> >       exit.
> >       Inline gdb_agent_socket_init here.
> 
> But why inline?  We should just need to move the "name" local
> of gdb_agent_socket_init a global?  (btw, the 'agent_socket_name'
> global is missing from the ChangeLog.)

At first, I tried to avoid introducing a global variable, so I inlined that
function.  Forgot to revert change back.  Fixed.

gdb/gdbserver:

2012-06-14  Yao Qi  <yao@codesourcery.com>

	* tracepoint.c (agent_socket_name): New static variable.
	(gdb_agent_socket_init): Replace local variable 'name' with
	'agent_socket_name'.
	Include 'stdlib.h'.
	(gdb_agent_remove_socket): New.
	(gdb_agent_helper_thread): Install gdb_agent_remove_socket to
	atexit hook.
---
 gdb/gdbserver/tracepoint.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index f103dfc..8fcaa46 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -6748,13 +6748,14 @@ init_named_socket (const char *name)
   return fd;
 }
 
+static char agent_socket_name[UNIX_PATH_MAX];
+
 static int
 gdb_agent_socket_init (void)
 {
   int result, fd;
-  char name[UNIX_PATH_MAX];
 
-  result = xsnprintf (name, UNIX_PATH_MAX, "%s/gdb_ust%d",
+  result = xsnprintf (agent_socket_name, UNIX_PATH_MAX, "%s/gdb_ust%d",
 		      SOCK_DIR, getpid ());
   if (result >= UNIX_PATH_MAX)
     {
@@ -6762,11 +6763,11 @@ gdb_agent_socket_init (void)
       return -1;
     }
 
-  fd = init_named_socket (name);
+  fd = init_named_socket (agent_socket_name);
   if (fd < 0)
     warning ("Error initializing named socket (%s) for communication with the "
 	     "ust helper thread. Check that directory exists and that it "
-	     "is writable.", name);
+	     "is writable.", agent_socket_name);
 
   return fd;
 }
@@ -6995,6 +6996,13 @@ gdb_ust_init (void)
 #endif /* HAVE_UST */
 
 #include <sys/syscall.h>
+#include <stdlib.h>
+
+static void
+gdb_agent_remove_socket (void)
+{
+  unlink (agent_socket_name);
+}
 
 /* Helper thread of agent.  */
 
@@ -7003,6 +7011,8 @@ gdb_agent_helper_thread (void *arg)
 {
   int listen_fd;
 
+  atexit (gdb_agent_remove_socket);
+
   while (1)
     {
       listen_fd = gdb_agent_socket_init ();
-- 
1.7.0.4

Re: [PATCH 2/4] Remove socket file at exit.

[Pedro Alves]

On 06/14/2012 03:43 PM, Yao Qi wrote:

> At first, I tried to avoid introducing a global variable, so I inlined that
> function.  Forgot to revert change back.  Fixed.


Thanks.

> 
> gdb/gdbserver:
> 
> 2012-06-14  Yao Qi  <yao@codesourcery.com>
> 
> 	* tracepoint.c (agent_socket_name): New static variable.
> 	(gdb_agent_socket_init): Replace local variable 'name' with
> 	'agent_socket_name'.
> 	Include 'stdlib.h'.
> 	(gdb_agent_remove_socket): New.
> 	(gdb_agent_helper_thread): Install gdb_agent_remove_socket to
> 	atexit hook.


This is okay.

-- 
Pedro Alves

[PATCH 1/4] New test for removing socket file in gdb.trace/strace.exp.

[Yao Qi]

This new test can expose the problem that socket file is not removed
under some different situations,

We'll see three fails in both native and gdbserver testing,

 FAIL: gdb.trace/strace.exp: remove_socket_after_quit: socket file removed
 FAIL: gdb.trace/strace.exp: remove_socket_after_detach: socket file removed
 FAIL: gdb.trace/strace.exp: remove_socket_after_continue: socket file removed

gdb/testsuite:

2012-06-09  Yao Qi  <yao@codesourcery.com>

	PR gdb/14161.
	* gdb.trace/strace.exp (strace_remove_socket): New proc.
---
 gdb/testsuite/gdb.trace/strace.exp |   69 ++++++++++++++++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
index 51da92b..f141e9b 100644
--- a/gdb/testsuite/gdb.trace/strace.exp
+++ b/gdb/testsuite/gdb.trace/strace.exp
@@ -39,6 +39,68 @@ if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags
     return -1
 }
 
+# Test socket file is removed when GDB quit, detach or resume inferior until it
+# exits.
+
+proc strace_remove_socket { action } {
+    with_test_prefix "remove_socket_after_${action}" {
+
+    global executable
+    global gdb_prompt
+
+    # Restart with a fresh gdb.
+    clean_restart $executable
+    if ![runto_main] {
+	fail "Can't run to main"
+	return -1
+    }
+
+    # List the markers in program.
+    gdb_test "info static-tracepoint-markers" \
+	".*ust/bar\[\t \]+n\[\t \]+.*ust/bar2\[\t \]+n\[\t \]+.*"
+
+    set pid ""
+    set test "collect pid"
+    gdb_test_multiple "info inferiors" $test {
+	-re "process (\[-0-9a-fx\]+) \[^\n\]*\n.*${gdb_prompt} $" {
+	    set pid $expect_out(1,string)
+	    pass $test
+	}
+	-re ".*${gdb_prompt} $" {
+	    fail $test
+	}
+    }
+
+    set test "socket file exists"
+    set socket_file "/tmp/gdb_ust${pid}"
+    if { [file exists $socket_file] } {
+	pass $test
+    } else {
+	fail $test
+    }
+
+    send_gdb "${action}\n"
+    gdb_expect {
+	-re "A debugging session is active.\r\n.*\r\nQuit anyway\\? \\(y or n\\) $" {
+	    send_gdb "y\n"
+	}
+	-re "Detaching .*, process .*$" {
+	}
+	-re "Continuing.*$" {
+	}
+    }
+
+    sleep 2
+
+    set test "socket file removed"
+    if { [file exists $socket_file] } {
+	fail $test
+    } else {
+	pass $test
+    }
+
+}}
+
 proc strace_info_marker { } { with_test_prefix "info_marker" {
     global executable
     global gdb_prompt
@@ -253,6 +315,9 @@ if ![runto_main] {
 if { ![is_remote target]
      && ([istarget "x86_64-*-linux*"] || [istarget "i\[34567\]86-*-linux*"]) } {
     strace_info_marker
+    strace_remove_socket "quit"
+    strace_remove_socket "detach"
+    strace_remove_socket "continue"
     return
 }
 
@@ -263,6 +328,10 @@ if { ![gdb_target_supports_trace] } then {
 
 gdb_load_shlibs $libipa
 
+strace_remove_socket "quit"
+strace_remove_socket "detach"
+strace_remove_socket "continue"
+
 strace_info_marker
 strace_probe_marker
 
-- 
1.7.0.4

Re: [PATCH 1/4] New test for removing socket file in gdb.trace/strace.exp.

[Pedro Alves]

On 06/09/2012 01:46 PM, Yao Qi wrote:

> This new test can expose the problem that socket file is not removed
> under some different situations,
> 
> We'll see three fails in both native and gdbserver testing,
> 
>  FAIL: gdb.trace/strace.exp: remove_socket_after_quit: socket file removed
>  FAIL: gdb.trace/strace.exp: remove_socket_after_detach: socket file removed
>  FAIL: gdb.trace/strace.exp: remove_socket_after_continue: socket file removed
> 
> gdb/testsuite:
> 
> 2012-06-09  Yao Qi  <yao@codesourcery.com>
> 
> 	PR gdb/14161.
> 	* gdb.trace/strace.exp (strace_remove_socket): New proc.


Would be more complete if said something about the new uses too.

> ---
>  gdb/testsuite/gdb.trace/strace.exp |   69 ++++++++++++++++++++++++++++++++++++
>  1 files changed, 69 insertions(+), 0 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
> index 51da92b..f141e9b 100644
> --- a/gdb/testsuite/gdb.trace/strace.exp
> +++ b/gdb/testsuite/gdb.trace/strace.exp
> @@ -39,6 +39,68 @@ if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags
>      return -1
>  }
>  
> +# Test socket file is removed when GDB quit, detach or resume inferior until it
> +# exits.


I think you meant:

# Test that the socket file is removed when GDB quits, detaches or
# resumes the inferior until it exits.


> +
> +proc strace_remove_socket { action } {
> +    with_test_prefix "remove_socket_after_${action}" {
> +
> +    global executable
> +    global gdb_prompt
> +
> +    # Restart with a fresh gdb.
> +    clean_restart $executable
> +    if ![runto_main] {
> +	fail "Can't run to main"
> +	return -1
> +    }
> +
> +    # List the markers in program.
> +    gdb_test "info static-tracepoint-markers" \
> +	".*ust/bar\[\t \]+n\[\t \]+.*ust/bar2\[\t \]+n\[\t \]+.*"
> +
> +    set pid ""
> +    set test "collect pid"
> +    gdb_test_multiple "info inferiors" $test {
> +	-re "process (\[-0-9a-fx\]+) \[^\n\]*\n.*${gdb_prompt} $" {
> +	    set pid $expect_out(1,string)
> +	    pass $test
> +	}
> +	-re ".*${gdb_prompt} $" {
> +	    fail $test
> +	}
> +    }
> +
> +    set test "socket file exists"
> +    set socket_file "/tmp/gdb_ust${pid}"
> +    if { [file exists $socket_file] } {
> +	pass $test
> +    } else {
> +	fail $test
> +    }


This won't work with remote host testing.  This file is really a
file on the target.  Why not use "remote_file target exists" ?

> +
> +    send_gdb "${action}\n"
> +    gdb_expect {
> +	-re "A debugging session is active.\r\n.*\r\nQuit anyway\\? \\(y or n\\) $" {
> +	    send_gdb "y\n"
> +	}
> +	-re "Detaching .*, process .*$" {
> +	}
> +	-re "Continuing.*$" {
> +	}
> +    }


gdb_test_multiple ?

> +
> +    sleep 2
> +
> +    set test "socket file removed"
> +    if { [file exists $socket_file] } {
> +	fail $test
> +    } else {
> +	pass $test
> +    }

> +

> +}}
> +
>  proc strace_info_marker { } { with_test_prefix "info_marker" {
>      global executable
>      global gdb_prompt
> @@ -253,6 +315,9 @@ if ![runto_main] {
>  if { ![is_remote target]
>       && ([istarget "x86_64-*-linux*"] || [istarget "i\[34567\]86-*-linux*"]) } {
>      strace_info_marker
> +    strace_remove_socket "quit"
> +    strace_remove_socket "detach"
> +    strace_remove_socket "continue"
>      return
>  }
>  
> @@ -263,6 +328,10 @@ if { ![gdb_target_supports_trace] } then {
>  
>  gdb_load_shlibs $libipa
>  
> +strace_remove_socket "quit"
> +strace_remove_socket "detach"
> +strace_remove_socket "continue"
> +
>  strace_info_marker
>  strace_probe_marker


-- 
Pedro Alves

Re: [PATCH 1/4] New test for removing socket file in gdb.trace/strace.exp.

[Yao Qi]

On Tuesday 12 June 2012 22:50:49 Pedro Alves wrote:
> > +    set test "socket file exists"
> > +    set socket_file "/tmp/gdb_ust${pid}"
> > +    if { [file exists $socket_file] } {
> > +     pass $test
> > +    } else {
> > +     fail $test
> > +    }
> 
> This won't work with remote host testing.  This file is really a
> file on the target.  Why not use "remote_file target exists" ?
> 

Yes, we should use "remote_file XXX exits", however, I find "remote_file XXX
 exists" always return 0 for socket file because it uses "[ -f $file ]" to check
 whether file exists.  I work around this limitation in this way,

    set exists ""
    if [is_remote target] {
       set status [remote_exec target "sh -c \"exit `\\\[ -S $socket_file `\""]
       set exists [expr [lindex $status 0] == 0]
    } else {
       set exists [file exists $socket_file]
    }

b.t.w, the quote on tcl and bash makes crazy :)

> > +
> > +    send_gdb "${action}\n"
> > +    gdb_expect {
> > +     -re "A debugging session is active.\r\n.*\r\nQuit anyway\\? \\(y or
> > n\\) $" { +         send_gdb "y\n"
> > +     }
> > +     -re "Detaching .*, process .*$" {
> > +     }
> > +     -re "Continuing.*$" {
> > +     }
> > +    }
> 
> gdb_test_multiple ?

When I tried gdb_test_multiple, I'll get an extra fail, so I use
 send_gdb/gdb_expect,

  FAIL: gdb.trace/strace.exp: remove_socket_after_quit: quit (got interactive prompt)

This new test is tested on native on local host, native-gdbserver, and native
 on remote host.  I saw some problems when running test on remote host.  They are
 not related to this patch series, so I'd like to defer them to follow up
fix later.

gdb/testsuite:

2012-06-14  Yao Qi  <yao@codesourcery.com>

	PR gdb/14161.
	* gdb.trace/strace.exp (strace_remove_socket): New proc to test
	the socket file is removed.
---
 gdb/testsuite/gdb.trace/strace.exp |   83 ++++++++++++++++++++++++++++++++++++
 1 files changed, 83 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
index 51da92b..b96ab41 100644
--- a/gdb/testsuite/gdb.trace/strace.exp
+++ b/gdb/testsuite/gdb.trace/strace.exp
@@ -39,6 +39,82 @@ if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags
     return -1
 }
 
+# Test that the socket file is removed when GDB quits, detaches or
+# resumes the inferior until it exits.
+
+proc strace_remove_socket { action } {
+    with_test_prefix "remove_socket_after_${action}" {
+
+    global executable
+    global gdb_prompt
+
+    # Restart with a fresh gdb.
+    clean_restart $executable
+    if ![runto_main] {
+	fail "Can't run to main"
+	return -1
+    }
+
+    # List the markers in program.
+    gdb_test "info static-tracepoint-markers" \
+	".*ust/bar\[\t \]+n\[\t \]+.*ust/bar2\[\t \]+n\[\t \]+.*"
+
+    set pid ""
+    set test "collect pid"
+    gdb_test_multiple "info inferiors" $test {
+	-re "process (\[-0-9a-fx\]+) \[^\n\]*\n.*${gdb_prompt} $" {
+	    set pid $expect_out(1,string)
+	    pass $test
+	}
+	-re ".*${gdb_prompt} $" {
+	    fail $test
+	}
+    }
+
+    set test "socket file exists"
+    set socket_file "/tmp/gdb_ust${pid}"
+    set exists ""
+    if [is_remote target] {
+	set status [remote_exec target "sh -c \"exit `\\\[ -S $socket_file ]`\""]
+	set exists [expr [lindex $status 0] == 0]
+    } else {
+	set exists [file exists $socket_file]
+    }
+
+    if { $exists } {
+	pass $test
+    } else {
+	fail $test
+    }
+
+    send_gdb "${action}\n"
+    gdb_expect {
+	-re "A debugging session is active.\r\n.*\r\nQuit anyway\\? \\(y or n\\) $" {
+	    send_gdb "y\n"
+	}
+	-re "Detaching .*, process .*$" {
+	}
+	-re "Continuing.*$" {
+	}
+    }
+
+    sleep 5
+
+    set test "socket file removed"
+    if [is_remote target] {
+	set status [remote_exec target "sh -c \"exit `\\\[ -S $socket_file ]`\""]
+	set exists [expr [lindex $status 0] == 0]
+    } else {
+	set exists [file exists $socket_file]
+    }
+
+    if { $exists } {
+	fail $test
+    } else {
+	pass $test
+    }
+}}
+
 proc strace_info_marker { } { with_test_prefix "info_marker" {
     global executable
     global gdb_prompt
@@ -253,6 +329,9 @@ if ![runto_main] {
 if { ![is_remote target]
      && ([istarget "x86_64-*-linux*"] || [istarget "i\[34567\]86-*-linux*"]) } {
     strace_info_marker
+    strace_remove_socket "quit"
+    strace_remove_socket "detach"
+    strace_remove_socket "continue"
     return
 }
 
@@ -263,6 +342,10 @@ if { ![gdb_target_supports_trace] } then {
 
 gdb_load_shlibs $libipa
 
+strace_remove_socket "quit"
+strace_remove_socket "detach"
+strace_remove_socket "continue"
+
 strace_info_marker
 strace_probe_marker
 
-- 
1.7.0.4

Re: [PATCH 1/4] New test for removing socket file in gdb.trace/strace.exp.

[Pedro Alves]

On 06/14/2012 03:39 PM, Yao Qi wrote:

> On Tuesday 12 June 2012 22:50:49 Pedro Alves wrote:
>>> +    set test "socket file exists"
>>> +    set socket_file "/tmp/gdb_ust${pid}"
>>> +    if { [file exists $socket_file] } {
>>> +     pass $test
>>> +    } else {
>>> +     fail $test
>>> +    }
>>
>> This won't work with remote host testing.  This file is really a
>> file on the target.  Why not use "remote_file target exists" ?
>>
> 
> Yes, we should use "remote_file XXX exits", however, I find "remote_file XXX
>  exists" always return 0 for socket file because it uses "[ -f $file ]" to check
>  whether file exists.  I work around this limitation in this way,
> 


Bummer.  :-(

>     set exists ""
>     if [is_remote target] {
>        set status [remote_exec target "sh -c \"exit `\\\[ -S $socket_file `\""]
>        set exists [expr [lindex $status 0] == 0]
>     } else {
>        set exists [file exists $socket_file]
>     }
> 
> b.t.w, the quote on tcl and bash makes crazy :)


:-)

In general, there are plenty of targets where we can't assume
a posix shell on the target.  But in this case, since this functionality
is implemented on GNU/Linux, we can go with it.  I think -S is a
bash extension, but again, it may be okay in this case.  I tried ksh,
and dash, and both accept it.

I don't understand how the else branch works for you with remote host
native testing, as you're checking for a file on the build machine, not
the host (where GDB runs) (unless you're sharing /tmp between
build and host machines, but you're probably not.)

Is there anything preventing using "remote_exec target" bits always,
getting rid of the else branch?

> 
>>> +
>>> +    send_gdb "${action}\n"
>>> +    gdb_expect {
>>> +     -re "A debugging session is active.\r\n.*\r\nQuit anyway\\? \\(y or
>>> n\\) $" { +         send_gdb "y\n"
>>> +     }
>>> +     -re "Detaching .*, process .*$" {
>>> +     }
>>> +     -re "Continuing.*$" {
>>> +     }
>>> +    }
>>
>> gdb_test_multiple ?
> 
> When I tried gdb_test_multiple, I'll get an extra fail, so I use
>  send_gdb/gdb_expect,
> 
>   FAIL: gdb.trace/strace.exp: remove_socket_after_quit: quit (got interactive prompt)
> 
> This new test is tested on native on local host, native-gdbserver, and native
>  on remote host.  


> I saw some problems when running test on remote host.  They are

> not related to this patch series, so I'd like to defer them to follow up
> fix later.


Thanks, that mode tends to rot a bit.

> 
> gdb/testsuite:
> 
> 2012-06-14  Yao Qi  <yao@codesourcery.com>
> 
> 	PR gdb/14161.
> 	* gdb.trace/strace.exp (strace_remove_socket): New proc to test
> 	the socket file is removed.


The uses of the new function should be mentioned as well.

> +proc strace_remove_socket { action } {


> +    sleep 5


We have 3 calls to this function, adding up to 15 seconds.

We could do things a bit differently to try to avoid always
sleeping, or sleeping so much.  For instance, with something like this
pseudo code:

 detach
 $exists=true
 for i in 0..4 {
     if ! socket exists
        $exists=false
        break
     sleep 1
 }
 if $exists
   fail $test
 else
   pass $test

IOW, check if the socket exists or not immediately after detaching.  Only
sleep if we find it still exists.  With luck, expect will be slow enough, and
we'll find the socket is gone on the first iteration, avoiding the sleep.
If not, we'll wait one second, then re-check, etc.

-- 
Pedro Alves

Re: [PATCH 1/4] New test for removing socket file in gdb.trace/strace.exp.

[Yao Qi]

On 06/16/2012 02:59 AM, Pedro Alves wrote:
> I don't understand how the else branch works for you with remote host
> native testing, as you're checking for a file on the build machine, not
> the host (where GDB runs) (unless you're sharing /tmp between
> build and host machines, but you're probably not.)
> 
> Is there anything preventing using "remote_exec target" bits always,
> getting rid of the else branch?
> 

The exit code of `exit FOO' is tricky on local and remote situation.  In this new version, I replace 'exit [ -S $socket_file]' with '[ -S $socket_file]', and use "remote_exec target" stuff in a unique way.

>> > 
>> > gdb/testsuite:
>> > 
>> > 2012-06-14  Yao Qi  <yao@codesourcery.com>
>> > 
>> > 	PR gdb/14161.
>> > 	* gdb.trace/strace.exp (strace_remove_socket): New proc to test
>> > 	the socket file is removed.
> 
> The uses of the new function should be mentioned as well.
> 
>> > +proc strace_remove_socket { action } {
> 
>> > +    sleep 5
> 
> We have 3 calls to this function, adding up to 15 seconds.
> 
> We could do things a bit differently to try to avoid always
> sleeping, or sleeping so much.  For instance, with something like this
> pseudo code:
> 
>  detach
>  $exists=true
>  for i in 0..4 {
>      if ! socket exists
>         $exists=false
>         break
>      sleep 1
>  }
>  if $exists
>    fail $test
>  else
>    pass $test
> 
> IOW, check if the socket exists or not immediately after detaching.  Only
> sleep if we find it still exists.  With luck, expect will be slow enough, and
> we'll find the socket is gone on the first iteration, avoiding the sleep.
> If not, we'll wait one second, then re-check, etc.

It is done in my new patch.  Beside these changes in this new patch, we unset gdb_spawn_id in 'strace_remove_socket', to avoid some error messages during test.

-- 
Yao (齐尧) 

gdb/testsuite:

2012-06-20  Yao Qi  <yao@codesourcery.com>

	PR gdb/14161.
	* gdb.trace/strace.exp (strace_remove_socket): New proc to test
	the socket file is removed.
	Invoke proc strace_remove_socket without checking
	'is_remote target'.
---
 gdb/testsuite/gdb.trace/strace.exp |  108 ++++++++++++++++++++++++++++++++----
 1 file changed, 98 insertions(+), 10 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
index 51da92b..d45c66d 100644
--- a/gdb/testsuite/gdb.trace/strace.exp
+++ b/gdb/testsuite/gdb.trace/strace.exp
@@ -39,6 +39,90 @@ if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags
     return -1
 }
 
+# Test that the socket file is removed when GDB quits, detaches or
+# resumes the inferior until it exits.
+
+proc strace_remove_socket { action } {
+    with_test_prefix "remove_socket_after_${action}" {
+
+    global executable
+    global gdb_prompt
+
+    # Restart with a fresh gdb.
+    clean_restart $executable
+    if ![runto_main] {
+	fail "Can't run to main"
+	return -1
+    }
+
+    # List the markers in program.
+    gdb_test "info static-tracepoint-markers" \
+	".*ust/bar\[\t \]+n\[\t \]+.*ust/bar2\[\t \]+n\[\t \]+.*"
+
+    set pid ""
+    set test "collect pid"
+    gdb_test_multiple "info inferiors" $test {
+	-re "process (\[-0-9a-fx\]+) \[^\n\]*\n.*${gdb_prompt} $" {
+	    set pid $expect_out(1,string)
+	    pass $test
+	}
+	-re ".*${gdb_prompt} $" {
+	    fail $test
+	}
+    }
+
+    set test "socket file exists"
+    set socket_file "/tmp/gdb_ust${pid}"
+    set status [remote_exec target "sh -c { \[ -S $socket_file \] }"]
+
+    if { [lindex $status 0] == 0 } {
+	pass $test
+    } else {
+	fail $test
+    }
+
+    send_gdb "${action}\n"
+    gdb_expect {
+	-re "A debugging session is active.\r\n.*\r\nQuit anyway\\? \\(y or n\\) $" {
+	    send_gdb "y\n"
+	}
+	-re "Detaching .*, process .*$" {
+	}
+	-re "Continuing.*$" {
+	}
+    }
+
+    set exists 1
+
+    for {set i 1} {$i <= 5} {incr i} {
+	set status [remote_exec target "sh -c { \[ -S $socket_file \] }"]
+	if { [lindex $status 0] != 0 } {
+	    set exists 0
+	    break
+	}
+	sleep 1
+    }
+
+    if { ![is_remote target] && ![string equal $action "detach"] } {
+	setup_kfail gdb/14161 *-*-*
+    }
+
+    set test "socket file removed"
+
+    if { $exists } {
+	fail $test
+    } else {
+	pass $test
+    }
+
+    if { [string equal $action "quit"] && [is_remote host] } {
+	global gdb_spawn_id
+	# unset gdb_spawn_id here to avoid sending command 'quit' to GDB
+	# later in default_gdb_exit.
+	unset gdb_spawn_id
+    }
+}}
+
 proc strace_info_marker { } { with_test_prefix "info_marker" {
     global executable
     global gdb_prompt
@@ -242,28 +326,32 @@ proc strace_trace_on_diff_addr { } { with_test_prefix "trace_diff_addr" {
     gdb_test "tfind" "Target failed to find requested trace frame\\..*"
 }}
 
-clean_restart $executable
+# Run it on x86/x86_64 linux.
+if { [istarget "x86_64-*-linux*"] || [istarget "i\[34567\]86-*-linux*"] } {
+    strace_info_marker
+    strace_remove_socket "quit"
+    strace_remove_socket "detach"
+    strace_remove_socket "continue"
+}
 
+clean_restart $executable
 if ![runto_main] {
     fail "Can't run to main to check for trace support"
     return -1
 }
-
-# Run it on native x86/x86_64 linux.
-if { ![is_remote target]
-     && ([istarget "x86_64-*-linux*"] || [istarget "i\[34567\]86-*-linux*"]) } {
-    strace_info_marker
-    return
-}
-
 if { ![gdb_target_supports_trace] } then {
+    # GDB detaches inferior so that the socket file can be removed.
+    gdb_test_multiple "detach" "detach" {
+	-re "Detaching .*, process .*${gdb_prompt} $" {
+	    pass "detach"
+	}
+    }
     unsupported "Current target does not support trace"
     return -1;
 }
 
 gdb_load_shlibs $libipa
 
-strace_info_marker
 strace_probe_marker
 
 strace_trace_on_same_addr "trace"
-- 
1.7.9.5

Re: [PATCH 1/4] New test for removing socket file in gdb.trace/strace.exp.

[Pedro Alves]

On 06/20/2012 02:46 PM, Yao Qi wrote:

> gdb/testsuite:
> 
> 2012-06-20  Yao Qi  <yao@codesourcery.com>
> 
> 	PR gdb/14161.
> 	* gdb.trace/strace.exp (strace_remove_socket): New proc to test
> 	the socket file is removed.
> 	Invoke proc strace_remove_socket without checking
> 	'is_remote target'.
> ---
>  gdb/testsuite/gdb.trace/strace.exp |  108 ++++++++++++++++++++++++++++++++----
>  1 file changed, 98 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
> index 51da92b..d45c66d 100644
> --- a/gdb/testsuite/gdb.trace/strace.exp
> +++ b/gdb/testsuite/gdb.trace/strace.exp
> @@ -39,6 +39,90 @@ if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags
>      return -1
>  }
>  
> +# Test that the socket file is removed when GDB quits, detaches or
> +# resumes the inferior until it exits.
> +
> +proc strace_remove_socket { action } {
> +    with_test_prefix "remove_socket_after_${action}" {
> +
> +    global executable
> +    global gdb_prompt
> +
> +    # Restart with a fresh gdb.
> +    clean_restart $executable
> +    if ![runto_main] {
> +	fail "Can't run to main"
> +	return -1
> +    }
> +
> +    # List the markers in program.
> +    gdb_test "info static-tracepoint-markers" \
> +	".*ust/bar\[\t \]+n\[\t \]+.*ust/bar2\[\t \]+n\[\t \]+.*"
> +
> +    set pid ""
> +    set test "collect pid"
> +    gdb_test_multiple "info inferiors" $test {
> +	-re "process (\[-0-9a-fx\]+) \[^\n\]*\n.*${gdb_prompt} $" {
> +	    set pid $expect_out(1,string)
> +	    pass $test
> +	}
> +	-re ".*${gdb_prompt} $" {
> +	    fail $test
> +	}
> +    }
> +
> +    set test "socket file exists"
> +    set socket_file "/tmp/gdb_ust${pid}"
> +    set status [remote_exec target "sh -c { \[ -S $socket_file \] }"]
> +
> +    if { [lindex $status 0] == 0 } {
> +	pass $test
> +    } else {
> +	fail $test
> +    }
> +
> +    send_gdb "${action}\n"
> +    gdb_expect {
> +	-re "A debugging session is active.\r\n.*\r\nQuit anyway\\? \\(y or n\\) $" {
> +	    send_gdb "y\n"
> +	}
> +	-re "Detaching .*, process .*$" {
> +	}
> +	-re "Continuing.*$" {
> +	}
> +    }
> +
> +    set exists 1
> +
> +    for {set i 1} {$i <= 5} {incr i} {
> +	set status [remote_exec target "sh -c { \[ -S $socket_file \] }"]
> +	if { [lindex $status 0] != 0 } {
> +	    set exists 0
> +	    break
> +	}
> +	sleep 1
> +    }
> +
> +    if { ![is_remote target] && ![string equal $action "detach"] } {
> +	setup_kfail gdb/14161 *-*-*
> +    }
> +
> +    set test "socket file removed"
> +
> +    if { $exists } {
> +	fail $test
> +    } else {
> +	pass $test
> +    }
> +
> +    if { [string equal $action "quit"] && [is_remote host] } {
> +	global gdb_spawn_id
> +	# unset gdb_spawn_id here to avoid sending command 'quit' to GDB
> +	# later in default_gdb_exit.
> +	unset gdb_spawn_id
> +    }
> +}}
> +
>  proc strace_info_marker { } { with_test_prefix "info_marker" {
>      global executable
>      global gdb_prompt
> @@ -242,28 +326,32 @@ proc strace_trace_on_diff_addr { } { with_test_prefix "trace_diff_addr" {
>      gdb_test "tfind" "Target failed to find requested trace frame\\..*"
>  }}
>  
> -clean_restart $executable
> +# Run it on x86/x86_64 linux.
> +if { [istarget "x86_64-*-linux*"] || [istarget "i\[34567\]86-*-linux*"] } {
> +    strace_info_marker
> +    strace_remove_socket "quit"
> +    strace_remove_socket "detach"
> +    strace_remove_socket "continue"
> +}
>  
> +clean_restart $executable
>  if ![runto_main] {
>      fail "Can't run to main to check for trace support"
>      return -1
>  }
> -
> -# Run it on native x86/x86_64 linux.
> -if { ![is_remote target]
> -     && ([istarget "x86_64-*-linux*"] || [istarget "i\[34567\]86-*-linux*"]) } {
> -    strace_info_marker
> -    return
> -}
> -
>  if { ![gdb_target_supports_trace] } then {
> +    # GDB detaches inferior so that the socket file can be removed.
> +    gdb_test_multiple "detach" "detach" {
> +	-re "Detaching .*, process .*${gdb_prompt} $" {
> +	    pass "detach"
> +	}
> +    }


This could just be gdb_test ?  But I'm confused on why "detach"
is needed at all here, going from that comment.  Wouldn't not
doing anything (and therefore gdb quitting and killing the inferior)
have the same effect?

>      unsupported "Current target does not support trace"
>      return -1;
>  }
>  
>  gdb_load_shlibs $libipa


If we now have tests earlier that run against remote targets,
it seems this should be moved earlier too.

>  
> -strace_info_marker
>  strace_probe_marker
>  
>  strace_trace_on_same_addr "trace"



-- 
Pedro Alves

Re: [PATCH 1/4] New test for removing socket file in gdb.trace/strace.exp.

[Yao Qi]

On 06/21/2012 11:56 PM, Pedro Alves wrote:
>> >  if { ![gdb_target_supports_trace] } then {
>> > +    # GDB detaches inferior so that the socket file can be removed.
>> > +    gdb_test_multiple "detach" "detach" {
>> > +	-re "Detaching .*, process .*${gdb_prompt} $" {
>> > +	    pass "detach"
>> > +	}
>> > +    }
> 
> This could just be gdb_test ?  But I'm confused on why "detach"
> is needed at all here, going from that comment.  Wouldn't not
> doing anything (and therefore gdb quitting and killing the inferior)
> have the same effect?

Yes, gdb_test is better.  The reason GDB detaches here is that socket 
file has been created at this point, and only GDB detach inferior can 
remove the socket file.  I add some comments more here.

> 
>> >      unsupported "Current target does not support trace"
>> >      return -1;
>> >  }
>> >  
>> >  gdb_load_shlibs $libipa
> 
> If we now have tests earlier that run against remote targets,
> it seems this should be moved earlier too.
> 

I noticed this problem when run it on remote-host, and planed to put 
the fix in next series.  I cherry-pick'ed the fix into this patch.  In 
short, 'gdb_load_shlibs $libipa' is invoked after each clean_restart.

-- 
Yao (齐尧) 

gdb/testsuite:

2012-06-27  Yao Qi  <yao@codesourcery.com>

	PR gdb/14161.
	* gdb.trace/strace.exp (strace_remove_socket): New proc to test
	the socket file is removed.
	Invoke proc strace_remove_socket without checking
	'is_remote target'.
	(strace_remove_socket, strace_probe_marker): Call proc gdb_load_shlibs.
	(strace_trace_on_same_addr, strace_trace_on_diff_addr): Likewise.
---
 gdb/testsuite/gdb.trace/strace.exp |  120 ++++++++++++++++++++++++++++++++----
 1 file changed, 108 insertions(+), 12 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
index d9e90b6..33700c2 100644
--- a/gdb/testsuite/gdb.trace/strace.exp
+++ b/gdb/testsuite/gdb.trace/strace.exp
@@ -37,12 +37,100 @@ if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags
     return -1
 }
 
+# Test that the socket file is removed when GDB quits, detaches or
+# resumes the inferior until it exits.
+
+proc strace_remove_socket { action } {
+    with_test_prefix "remove_socket_after_${action}" {
+
+    global executable
+    global gdb_prompt
+    global libipa
+
+    # Restart with a fresh gdb.
+    clean_restart $executable
+    gdb_load_shlibs $libipa
+    if ![runto_main] {
+	fail "Can't run to main"
+	return -1
+    }
+
+    # List the markers in program.
+    gdb_test "info static-tracepoint-markers" \
+	".*ust/bar\[\t \]+n\[\t \]+.*ust/bar2\[\t \]+n\[\t \]+.*"
+
+    set pid ""
+    set test "collect pid"
+    gdb_test_multiple "info inferiors" $test {
+	-re "process (\[-0-9a-fx\]+) \[^\n\]*\n.*${gdb_prompt} $" {
+	    set pid $expect_out(1,string)
+	    pass $test
+	}
+	-re ".*${gdb_prompt} $" {
+	    fail $test
+	}
+    }
+
+    set test "socket file exists"
+    set socket_file "/tmp/gdb_ust${pid}"
+    set status [remote_exec target "sh -c { \[ -S $socket_file \] }"]
+
+    if { [lindex $status 0] == 0 } {
+	pass $test
+    } else {
+	fail $test
+    }
+
+    send_gdb "${action}\n"
+    gdb_expect {
+	-re "A debugging session is active.\r\n.*\r\nQuit anyway\\? \\(y or n\\) $" {
+	    send_gdb "y\n"
+	}
+	-re "Detaching .*, process .*$" {
+	}
+	-re "Continuing.*$" {
+	}
+    }
+
+    set exists 1
+
+    for {set i 1} {$i <= 5} {incr i} {
+	set status [remote_exec target "sh -c { \[ -S $socket_file \] }"]
+	if { [lindex $status 0] != 0 } {
+	    set exists 0
+	    break
+	}
+	sleep 1
+    }
+
+    if { ![is_remote target] && ![string equal $action "detach"] } {
+	setup_kfail gdb/14161 *-*-*
+    }
+
+    set test "socket file removed"
+
+    if { $exists } {
+	fail $test
+    } else {
+	pass $test
+    }
+
+    if { [string equal $action "quit"] && [is_remote host] } {
+	global gdb_spawn_id
+	# unset gdb_spawn_id here to avoid sending command 'quit' to GDB
+	# later in default_gdb_exit.
+	unset gdb_spawn_id
+    }
+}}
+
 proc strace_info_marker { } { with_test_prefix "info_marker" {
     global executable
     global gdb_prompt
+    global libipa
 
     # Restart with a fresh gdb.
     clean_restart $executable
+    gdb_load_shlibs $libipa
     if ![runto_main] {
 	fail "Can't run to main"
 	return -1
@@ -66,9 +154,11 @@ proc strace_probe_marker { } { with_test_prefix "probe_marker" {
     global expect_out
     global gdb_prompt
     global hex
+    global libipa
 
     # Restart with a fresh gdb.
     clean_restart $executable
+    gdb_load_shlibs $libipa
     if ![runto_main] {
 	fail "Can't run to main"
 	return -1
@@ -97,9 +187,11 @@ with_test_prefix "trace_same_addr $type" {
     global expect_out
     global gdb_prompt
     global hex
+    global libipa
 
     # Restart with a fresh gdb.
     clean_restart $executable
+    gdb_load_shlibs $libipa
     if ![runto_main] {
 	fail "Can't run to main"
 	return -1
@@ -192,9 +284,11 @@ proc strace_trace_on_diff_addr { } { with_test_prefix "trace_diff_addr" {
     global expect_out
     global gdb_prompt
     global hex
+    global libipa
 
     # Restart with a fresh gdb.
     clean_restart $executable
+    gdb_load_shlibs $libipa
     if ![runto_main] {
 	fail "Can't run to main"
 	return -1
@@ -240,28 +334,30 @@ proc strace_trace_on_diff_addr { } { with_test_prefix "trace_diff_addr" {
     gdb_test "tfind" "Target failed to find requested trace frame\\..*"
 }}
 
-clean_restart $executable
+# Run it on x86/x86_64 linux.
+if { [istarget "x86_64-*-linux*"] || [istarget "i\[34567\]86-*-linux*"] } {
+    strace_info_marker
+    strace_remove_socket "quit"
+    strace_remove_socket "detach"
+    strace_remove_socket "continue"
+}
 
+clean_restart $executable
+gdb_load_shlibs $libipa
 if ![runto_main] {
     fail "Can't run to main to check for trace support"
     return -1
 }
-
-# Run it on native x86/x86_64 linux.
-if { ![is_remote target]
-     && ([istarget "x86_64-*-linux*"] || [istarget "i\[34567\]86-*-linux*"]) } {
-    strace_info_marker
-    return
-}
-
 if { ![gdb_target_supports_trace] } then {
+    # At this point, the socket file has been created.  We must make sure it is
+    # removed when we return here.  GDB detaches inferior so that the socket
+    # file can be removed.  Note that GDB simply kill inferior doesn't remove
+    # the socket file.
+    gdb_test "detach" "Detaching .*, process .*"
     unsupported "Current target does not support trace"
     return -1;
 }
 
-gdb_load_shlibs $libipa
-
-strace_info_marker
 strace_probe_marker
 
 strace_trace_on_same_addr "trace"
-- 
1.7.9.5

[PATCH 4/4] gdb: kfail for PR14161

[Yao Qi]

GDB doesn't have something equivalent to pause_all/unpause_all in GDBserver,
so GDB is unable to control the threads correctly after sending commands
to agent.  KFAIL this first.

gdb/testsuite:

2012-06-09  Yao Qi  <yao@codesourcery.com>

	KFAIL for PR14161.
	* gdb.trace/strace.exp (strace_remove_socket): kfail for native.
	Cleanup socket files.
---
 gdb/testsuite/gdb.trace/strace.exp |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
index f141e9b..00a3fad 100644
--- a/gdb/testsuite/gdb.trace/strace.exp
+++ b/gdb/testsuite/gdb.trace/strace.exp
@@ -92,6 +92,10 @@ proc strace_remove_socket { action } {
 
     sleep 2
 
+    if { ![is_remote target] && ! [string equal $action "detach"] } {
+	setup_kfail gdb/14161 *-*-*
+    }
+
     set test "socket file removed"
     if { [file exists $socket_file] } {
 	fail $test
@@ -318,6 +322,13 @@ if { ![is_remote target]
     strace_remove_socket "quit"
     strace_remove_socket "detach"
     strace_remove_socket "continue"
+
+    # Due to PR gdb/14161, sockets files are not removed when agent exists.
+    # However this problem only affects native gdb, so we don't have to bother
+    # 'remote_file target delete'.  Simple tcl command 'file delete' should
+    # be OK.
+    eval file delete [glob "/tmp/gdb_ust*"]
+
     return
 }
 
-- 
1.7.0.4

Re: [PATCH 4/4] gdb: kfail for PR14161

[Pedro Alves]

On 06/09/2012 01:46 PM, Yao Qi wrote:

> GDB doesn't have something equivalent to pause_all/unpause_all in GDBserver,
> so GDB is unable to control the threads correctly after sending commands
> to agent.


Yeah, my ideal would be to merge gdb/gdbserver's backends, but you've been
distracting me from that goal.  ;-)

>  KFAIL this first.



> gdb/testsuite:
> 
> 2012-06-09  Yao Qi  <yao@codesourcery.com>
> 
> 	KFAIL for PR14161.
> 	* gdb.trace/strace.exp (strace_remove_socket): kfail for native.
> 	Cleanup socket files.
> ---
>  gdb/testsuite/gdb.trace/strace.exp |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
> index f141e9b..00a3fad 100644
> --- a/gdb/testsuite/gdb.trace/strace.exp
> +++ b/gdb/testsuite/gdb.trace/strace.exp
> @@ -92,6 +92,10 @@ proc strace_remove_socket { action } {
>  
>      sleep 2
>  
> +    if { ![is_remote target] && ! [string equal $action "detach"] } {
> +	setup_kfail gdb/14161 *-*-*
> +    }
> +
>      set test "socket file removed"
>      if { [file exists $socket_file] } {
>  	fail $test
> @@ -318,6 +322,13 @@ if { ![is_remote target]
>      strace_remove_socket "quit"
>      strace_remove_socket "detach"
>      strace_remove_socket "continue"
> +
> +    # Due to PR gdb/14161, sockets files are not removed when agent exists.

> +    # However this problem only affects native gdb, so we don't have to bother

> +    # 'remote_file target delete'.  Simple tcl command 'file delete' should
> +    # be OK.
> +    eval file delete [glob "/tmp/gdb_ust*"]
> +


"bother with".  But note this doesn't work with remote host testing.

>      return
>  }
>  



-- 
Pedro Alves

Re: [PATCH 4/4] gdb: kfail for PR14161

[Yao Qi]

On Wednesday 13 June 2012 00:20:30 Pedro Alves wrote:
> > +    # Due to PR gdb/14161, sockets files are not removed when agent
> > exists.
> > 
> > +    # However this problem only affects native gdb, so we don't have to
> > bother
> > 
> > +    # 'remote_file target delete'.  Simple tcl command 'file delete'
> > should +    # be OK.
> > +    eval file delete [glob "/tmp/gdb_ust*"]
> > +
> 
> "bother with".  But note this doesn't work with remote host testing.

I use "remote_file target delete" in the new patch when "is_remote target" is
 true, otherwise use tcl builtin command to remove files.

gdb/testsuite:

2012-06-14  Yao Qi  <yao@codesourcery.com>

	KFAIL for PR14161.
	* gdb.trace/strace.exp (strace_remove_socket): kfail for native.
	Cleanup socket files.
---
 gdb/testsuite/gdb.trace/strace.exp |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
index b96ab41..1520055 100644
--- a/gdb/testsuite/gdb.trace/strace.exp
+++ b/gdb/testsuite/gdb.trace/strace.exp
@@ -100,6 +100,10 @@ proc strace_remove_socket { action } {
 
     sleep 5
 
+    if { ![is_remote target] && ! [string equal $action "detach"] } {
+	setup_kfail gdb/14161 *-*-*
+    }
+
     set test "socket file removed"
     if [is_remote target] {
 	set status [remote_exec target "sh -c \"exit `\\\[ -S $socket_file ]`\""]
@@ -332,7 +336,15 @@ if { ![is_remote target]
     strace_remove_socket "quit"
     strace_remove_socket "detach"
     strace_remove_socket "continue"
+
+    # Due to PR gdb/14161, sockets files are not removed when agent exists.
+    if [is_remote target] {
+	foreach match [glob -nocomplain "/tmp/gdb_ust*"] {
+	    remote_file target delete $match
+	}
+    } else {
+	eval file delete [glob "/tmp/gdb_ust*"]
+    }
    return
 }
 
 if { ![gdb_target_supports_trace] } then {
-- 
1.7.0.4

Re: [PATCH 4/4] gdb: kfail for PR14161

[Pedro Alves]

On 06/14/2012 04:01 PM, Yao Qi wrote:

> On Wednesday 13 June 2012 00:20:30 Pedro Alves wrote:
>>> > > +    # Due to PR gdb/14161, sockets files are not removed when agent
>>> > > exists.
>>> > > 
>>> > > +    # However this problem only affects native gdb, so we don't have to
>>> > > bother
>>> > > 
>>> > > +    # 'remote_file target delete'.  Simple tcl command 'file delete'
>>> > > should +    # be OK.
>>> > > +    eval file delete [glob "/tmp/gdb_ust*"]
>>> > > +
>> > 
>> > "bother with".  But note this doesn't work with remote host testing.
> I use "remote_file target delete" in the new patch when "is_remote target" is
>  true, otherwise use tcl builtin command to remove files.
> 
> gdb/testsuite:
> 
> 2012-06-14  Yao Qi  <yao@codesourcery.com>
> 
> 	KFAIL for PR14161.
> 	* gdb.trace/strace.exp (strace_remove_socket): kfail for native.
> 	Cleanup socket files.
> ---
>  gdb/testsuite/gdb.trace/strace.exp |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
> index b96ab41..1520055 100644
> --- a/gdb/testsuite/gdb.trace/strace.exp
> +++ b/gdb/testsuite/gdb.trace/strace.exp
> @@ -100,6 +100,10 @@ proc strace_remove_socket { action } {
>  
>      sleep 5
>  
> +    if { ![is_remote target] && ! [string equal $action "detach"] } {
> +	setup_kfail gdb/14161 *-*-*
> +    }
> +
>      set test "socket file removed"
>      if [is_remote target] {
>  	set status [remote_exec target "sh -c \"exit `\\\[ -S $socket_file ]`\""]
> @@ -332,7 +336,15 @@ if { ![is_remote target]
>      strace_remove_socket "quit"
>      strace_remove_socket "detach"
>      strace_remove_socket "continue"
> +
> +    # Due to PR gdb/14161, sockets files are not removed when agent exists.
> +    if [is_remote target] {
> +	foreach match [glob -nocomplain "/tmp/gdb_ust*"] {
> +	    remote_file target delete $match


That's unfortunately not good, because while the "delete" runs on
the "target", the "glob" is running on the "build" machine (which is usually
the "host", unless you're doing remote-host testing).

> +	}
> +    } else {
> +	eval file delete [glob "/tmp/gdb_ust*"]

> +    }

This else doesn't work for remote-host testing either.  With remote-
-host testing, you have expect running on machine A (build), GDB
running on machine B (host), and that GDB connecting to machine
C (target).  Usually, either A and B are the same, or B and C are
the same, though A != B != C is possible.

Maybe we can avoid the need for globing in the first place, by making
strace_remove_socket (and whatever other function in the file, if
necessary), remove the socket file before returning, while they
still know the inferior's PID?

-- 
Pedro Alves

Re: [PATCH 4/4] gdb: kfail for PR14161

[Yao Qi]

On 06/16/2012 03:33 AM, Pedro Alves wrote:
>> > +    # Due to PR gdb/14161, sockets files are not removed when agent exists.
>> > +    if [is_remote target] {
>> > +	foreach match [glob -nocomplain "/tmp/gdb_ust*"] {
>> > +	    remote_file target delete $match
> 
> That's unfortunately not good, because while the "delete" runs on
> the "target", the "glob" is running on the "build" machine (which is usually
> the "host", unless you're doing remote-host testing).
> 
>> > +	}
>> > +    } else {
>> > +	eval file delete [glob "/tmp/gdb_ust*"]
>> > +    }
> This else doesn't work for remote-host testing either.  With remote-
> -host testing, you have expect running on machine A (build), GDB
> running on machine B (host), and that GDB connecting to machine
> C (target).  Usually, either A and B are the same, or B and C are
> the same, though A != B != C is possible.
> 

I am using 'remote_exec target "sh -c \"rm -r $socket_file\""' to
delete them in a unique way.  The reason is explained in the comments.

> Maybe we can avoid the need for globing in the first place, by making
> strace_remove_socket (and whatever other function in the file, if
> necessary), remove the socket file before returning, while they
> still know the inferior's PID?

Yes, at the end of strace_remove_socket, we can delete socket file if
it still exists.  In proc strace_info_marker, I simply force GDB to
detach inferior, so the socket file can be removed as well.

-- 
Yao (齐尧) 

gdb/testsuite:

2012-06-20  Yao Qi  <yao@codesourcery.com>

	KFAIL for PR14161.
	* gdb.trace/strace.exp (strace_remove_socket): kfail for native.
	Cleanup socket files.
	(strace_info_marker): Detach inferior.
---
 gdb/testsuite/gdb.trace/strace.exp |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
index d45c66d..587d9b3 100644
--- a/gdb/testsuite/gdb.trace/strace.exp
+++ b/gdb/testsuite/gdb.trace/strace.exp
@@ -111,6 +111,9 @@ proc strace_remove_socket { action } {
 
     if { $exists } {
 	fail $test
+	# Since $socket_file is a socket file instead of a regular file, we
+	# can't use 'remote_file target delete $socket_file' here.
+	remote_exec target "sh -c \"rm -r $socket_file\""
     } else {
 	pass $test
     }
@@ -145,6 +148,13 @@ proc strace_info_marker { } { with_test_prefix "info_marker" {
 	    pass "info threads"
 	}
     }
+
+    # GDB detaches inferior so that the socket file can be removed.
+    gdb_test_multiple "detach" "detach" {
+	-re "Detaching .*, process .*${gdb_prompt} $" {
+	    pass "detach"
+	}
+    }
 }}
 
 proc strace_probe_marker { } { with_test_prefix "probe_marker" {
-- 
1.7.9.5

[PATCH 3/4] New agent command 'kill' and used by gdbserver

[Yao Qi]

When GDB or GDBserver terminated inferior, agent has no chance to
remove socket file, even in atexit hook.  This patch adds a call
to send command 'kill' to agent in function kill_inferior, in
order to tell agent that inferior will be killed, and then agent
can do cleanup.

Of course, new agent command 'kill' is added and documented.

When GDB or GDBserver sends 'kill' command to an older agent, in
which 'kill is not known, and it will be ignored.

gdb/gdbserver:

2012-06-09  Yao Qi  <yao@codesourcery.com>

	* server.h: Declare run_inferior_command.
	* target.c (kill_inferior): New.  Send command 'kill'.
	* target.h (kill_inferior): Removed macro.
	* tracepoint.c (run_inferior_command): Remove static.
	Handle command 'kill'.

gdb/doc:

2012-06-09  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (IPA Protocol Commands): Document new command
	'kill'.
---
 gdb/doc/gdb.texinfo        |    4 ++++
 gdb/gdbserver/server.h     |    1 +
 gdb/gdbserver/target.c     |    8 ++++++++
 gdb/gdbserver/target.h     |    5 ++---
 gdb/gdbserver/tracepoint.c |   17 +++++++++++++++--
 5 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ad227a4..09339e5 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33585,6 +33585,10 @@ for an error
 
 @end table
 
+@item kill
+Kills in-process agent.  Usually, this command is sent when @value{GDBN}
+or GDBserver is about to kill inferiors.
+
 @item qTfSTM
 @xref{qTfSTM}.
 @item qTsSTM
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 02dfa29..452e400 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -558,4 +558,5 @@ int emit_error;
 extern const char version[];
 extern const char host_name[];
 
+int run_inferior_command (char *cmd, int len);
 #endif /* SERVER_H */
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 7539476..35aaecd 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -182,3 +182,11 @@ target_waitstatus_to_string (const struct target_waitstatus *ws)
 
   return buf;
 }
+
+int
+kill_inferior(int pid)
+{
+  run_inferior_command ("kill", strlen ("kill"));
+
+  return (*the_target->kill) (pid);
+}
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 0cf5687..101c3c4 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -409,9 +409,6 @@ void set_target_ops (struct target_ops *);
 #define myattach(pid) \
   (*the_target->attach) (pid)
 
-#define kill_inferior(pid) \
-  (*the_target->kill) (pid)
-
 #define detach_inferior(pid) \
   (*the_target->detach) (pid)
 
@@ -555,4 +552,6 @@ const char *target_pid_to_str (ptid_t);
 
 const char *target_waitstatus_to_string (const struct target_waitstatus *);
 
+int kill_inferior (int);
+
 #endif /* TARGET_H */
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 4e6c3ec..bb2df8f 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -412,7 +412,7 @@ static int flush_trace_buffer_handler (CORE_ADDR);
 static void download_trace_state_variables (void);
 static void upload_fast_traceframes (void);
 
-static int run_inferior_command (char *cmd, int len);
+int run_inferior_command (char *cmd, int len);
 
 static int
 read_inferior_integer (CORE_ADDR symaddr, int *val)
@@ -6662,7 +6662,7 @@ static struct ltt_available_probe gdb_ust_probe =
    thread by means of direct memory xfering, and a socket for
    synchronization.  */
 
-static int
+int
 run_inferior_command (char *cmd, int len)
 {
   int err = -1;
@@ -7053,6 +7053,17 @@ gdb_agent_helper_thread (void *arg)
 
 	  if (cmd_buf[0])
 	    {
+	      if (strncmp ("kill", cmd_buf, 4) == 0)
+		{
+		  ret = write (fd, buf, 1);
+		  close (fd);
+
+		  close (listen_fd);
+		  unlink (agent_socket_name);
+
+		  return NULL;
+		}
+	      else
 #ifdef HAVE_UST
 	      if (strcmp ("qTfSTM", cmd_buf) == 0)
 		{
@@ -7080,7 +7091,9 @@ gdb_agent_helper_thread (void *arg)
 		{
 		  cmd_qtstmat (cmd_buf);
 		}
+	      else
 #endif /* HAVE_UST */
+		{}
 	    }
 
 	  /* Fix compiler's warning: ignoring return value of 'write'.  */
-- 
1.7.0.4

Re: [PATCH 3/4] New agent command 'kill' and used by gdbserver

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Sat, 9 Jun 2012 20:46:41 +0800
> 
> 2012-06-09  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.texinfo (IPA Protocol Commands): Document new command
> 	'kill'.

This part is OK.  Thanks.

Re: [PATCH 3/4] New agent command 'kill' and used by gdbserver

[Pedro Alves]

On 06/09/2012 01:46 PM, Yao Qi wrote:

> When GDB or GDBserver terminated inferior, agent has no chance to
> remove socket file, even in atexit hook.  This patch adds a call
> to send command 'kill' to agent in function kill_inferior, in
> order to tell agent that inferior will be killed, and then agent
> can do cleanup.
> 
> Of course, new agent command 'kill' is added and documented.
> 
> When GDB or GDBserver sends 'kill' command to an older agent, in
> which 'kill is not known, and it will be ignored.
> 
> gdb/gdbserver:
> 
> 2012-06-09  Yao Qi  <yao@codesourcery.com>
> 
> 	* server.h: Declare run_inferior_command.
> 	* target.c (kill_inferior): New.  Send command 'kill'.
> 	* target.h (kill_inferior): Removed macro.
> 	* tracepoint.c (run_inferior_command): Remove static.


> 	Handle command 'kill'.


Missing context function for this sentence?

> 
> gdb/doc:
> 
> 2012-06-09  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.texinfo (IPA Protocol Commands): Document new command
> 	'kill'.
> ---
>  gdb/doc/gdb.texinfo        |    4 ++++
>  gdb/gdbserver/server.h     |    1 +
>  gdb/gdbserver/target.c     |    8 ++++++++
>  gdb/gdbserver/target.h     |    5 ++---
>  gdb/gdbserver/tracepoint.c |   17 +++++++++++++++--
>  5 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index ad227a4..09339e5 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -33585,6 +33585,10 @@ for an error
>  
>  @end table
>  
> +@item kill
> +Kills in-process agent.  


That's not really true.  The command doesn't kill the in-process agent.
That'd be the same as killing the inferior, given that the agent is
in-process.  Maybe calling the command "close" or "closecon" would
be better.

> Usually, this command is sent when @value{GDBN}
> +or GDBserver is about to kill inferiors.


Drop the "usually".  There's only one use for this presently, so
let's be specific.

> +
>  @item qTfSTM
>  @xref{qTfSTM}.
>  @item qTsSTM
> diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
> index 02dfa29..452e400 100644
> --- a/gdb/gdbserver/server.h
> +++ b/gdb/gdbserver/server.h
> @@ -558,4 +558,5 @@ int emit_error;
>  extern const char version[];
>  extern const char host_name[];
>  
> +int run_inferior_command (char *cmd, int len);
>  #endif /* SERVER_H */
> diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
> index 7539476..35aaecd 100644
> --- a/gdb/gdbserver/target.c
> +++ b/gdb/gdbserver/target.c
> @@ -182,3 +182,11 @@ target_waitstatus_to_string (const struct target_waitstatus *ws)
>  
>    return buf;
>  }
> +
> +int
> +kill_inferior(int pid)

> +{

> +  run_inferior_command ("kill", strlen ("kill"));


- Missing space before parens.

- What happens if there's no IPA to talk to?

- Also, all callers of run_inferior_command command currently
  pass "cmd, strlen (cmd) + 1".

- The kill_inferior function takes a PID as argument.  It's not
  right to assume that PID is the current process.  IOW, in a
  multi-process scenario, you may well run the inferior command
  on the wrong inferior as is.

> +
> +  return (*the_target->kill) (pid);
> +}
> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
> index 0cf5687..101c3c4 100644
> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
> @@ -409,9 +409,6 @@ void set_target_ops (struct target_ops *);
>  #define myattach(pid) \
>    (*the_target->attach) (pid)
>  
> -#define kill_inferior(pid) \
> -  (*the_target->kill) (pid)
> -
>  #define detach_inferior(pid) \
>    (*the_target->detach) (pid)
>  
> @@ -555,4 +552,6 @@ const char *target_pid_to_str (ptid_t);
>  
>  const char *target_waitstatus_to_string (const struct target_waitstatus *);
>  
> +int kill_inferior (int);


I'd prefer leaving this declaration where the old macro is, close
to the cousins attach, detach, etc.

> +
>  #endif /* TARGET_H */
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index 4e6c3ec..bb2df8f 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -412,7 +412,7 @@ static int flush_trace_buffer_handler (CORE_ADDR);
>  static void download_trace_state_variables (void);
>  static void upload_fast_traceframes (void);
>  
> -static int run_inferior_command (char *cmd, int len);
> +int run_inferior_command (char *cmd, int len);


This is now declared in server.h, so this declaration should
be removed, not adjusted.

>  
>  static int
>  read_inferior_integer (CORE_ADDR symaddr, int *val)
> @@ -6662,7 +6662,7 @@ static struct ltt_available_probe gdb_ust_probe =
>     thread by means of direct memory xfering, and a socket for
>     synchronization.  */
>  
> -static int
> +int
>  run_inferior_command (char *cmd, int len)
>  {
>    int err = -1;
> @@ -7053,6 +7053,17 @@ gdb_agent_helper_thread (void *arg)
>  
>  	  if (cmd_buf[0])
>  	    {
> +	      if (strncmp ("kill", cmd_buf, 4) == 0)
> +		{
> +		  ret = write (fd, buf, 1);
> +		  close (fd);
> +
> +		  close (listen_fd);
> +		  unlink (agent_socket_name);
> +
> +		  return NULL;
> +		}
> +	      else
>  #ifdef HAVE_UST
>  	      if (strcmp ("qTfSTM", cmd_buf) == 0)
>  		{
> @@ -7080,7 +7091,9 @@ gdb_agent_helper_thread (void *arg)
>  		{
>  		  cmd_qtstmat (cmd_buf);
>  		}
> +	      else
>  #endif /* HAVE_UST */
> +		{}


Why do we need these "else" and "{}" lines?  AFAICS, you can
just remove them.  You could do:

-  	      if (strcmp ("qTfSTM", cmd_buf) == 0)
+  	      else if (strcmp ("qTfSTM", cmd_buf) == 0)

to be extra neat.

>  	    }
>  
>  	  /* Fix compiler's warning: ignoring return value of 'write'.  */



-- 
Pedro Alves

Re: [PATCH 3/4] New agent command 'kill' and used by gdbserver

[Yao Qi]

On Wednesday 13 June 2012 00:14:19 Pedro Alves wrote:
> > +  run_inferior_command ("kill", strlen ("kill"));
> 
> - Missing space before parens.
> 
> - What happens if there's no IPA to talk to?
> 
> - Also, all callers of run_inferior_command command currently
>   pass "cmd, strlen (cmd) + 1".
> 
> - The kill_inferior function takes a PID as argument.  It's not
>   right to assume that PID is the current process.  IOW, in a
>   multi-process scenario, you may well run the inferior command
>   on the wrong inferior as is.

This new patch addresses all your comments except this one above.  IMO, the
 whole agent work in GDBserver doesn't consider much about multi-process, so
 this problem shall be addressed as a whole, when we start to support agent in
 multi-process.  I leave a FIXME in comment there.  WDYT?

gdb/gdbserver:

2012-06-14  Yao Qi  <yao@codesourcery.com>

	* server.h: Declare run_inferior_command and
	maybe_write_ipa_not_loaded.
	* target.c (kill_inferior): Include "agent.h".
	New.  Send command 'kill'.
	* target.h (kill_inferior): Removed macro.
	* tracepoint.c (run_inferior_command): Remove 'static'.
	(kill_inferior): New.
	(maybe_write_ipa_not_loaded): Remove 'static'.
	(gdb_agent_helper_thread): Handle command 'close'.
	Wait endlessly until the inferior stops.

gdb/doc:

2012-06-14  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (IPA Protocol Commands): Document new command
	'close'.
---
 gdb/doc/gdb.texinfo        |    4 ++++
 gdb/gdbserver/server.h     |    3 +++
 gdb/gdbserver/target.c     |   18 ++++++++++++++++++
 gdb/gdbserver/target.h     |    3 +--
 gdb/gdbserver/tracepoint.c |   27 ++++++++++++++++++++++-----
 5 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ad227a4..8dd1bf7 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33585,6 +33585,10 @@ for an error
 
 @end table
 
+@item close
+Closes the in-process agent.  This command is sent when @value{GDBN} or GDBserver
+is about to kill inferiors.
+
 @item qTfSTM
 @xref{qTfSTM}.
 @item qTsSTM
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 02dfa29..d5fa867 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -467,6 +467,8 @@ void stop_tracing (void);
 
 int claim_trampoline_space (ULONGEST used, CORE_ADDR *trampoline);
 int have_fast_tracepoint_trampoline_buffer (char *msgbuf);
+
+int maybe_write_ipa_not_loaded (char *buffer);
 #endif
 
 struct traceframe;
@@ -558,4 +560,5 @@ int emit_error;
 extern const char version[];
 extern const char host_name[];
 
+int run_inferior_command (char *cmd, int len);
 #endif /* SERVER_H */
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 7539476..6148e1c 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -20,6 +20,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "server.h"
+#include "agent.h"
 
 struct target_ops *the_target;
 
@@ -182,3 +183,20 @@ target_waitstatus_to_string (const struct target_waitstatus *ws)
 
   return buf;
 }
+
+int
+kill_inferior (int pid)
+{
+  char buf[IPA_CMD_BUF_SIZE];
+
+  if (!maybe_write_ipa_not_loaded (buf))
+    {
+      strcpy (buf, "close");
+      /* FIXME: It is wrong to assume PID is the current process.  In
+	 a multiple-process scenario, we may run inferior command on
+	 the wrong process.  */
+      run_inferior_command (buf, strlen (buf) + 1);
+    }
+
+  return (*the_target->kill) (pid);
+}
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 0cf5687..9f96e04 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -409,8 +409,7 @@ void set_target_ops (struct target_ops *);
 #define myattach(pid) \
   (*the_target->attach) (pid)
 
-#define kill_inferior(pid) \
-  (*the_target->kill) (pid)
+int kill_inferior (int);
 
 #define detach_inferior(pid) \
   (*the_target->detach) (pid)
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 8fcaa46..df69508 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -281,7 +281,7 @@ write_e_ust_not_loaded (char *buffer)
 /* If the in-process agent library isn't loaded in the inferior, write
    an error to BUFFER, and return 1.  Otherwise, return 0.  */
 
-static int
+int
 maybe_write_ipa_not_loaded (char *buffer)
 {
   if (!agent_loaded_p ())
@@ -412,8 +412,6 @@ static int flush_trace_buffer_handler (CORE_ADDR);
 static void download_trace_state_variables (void);
 static void upload_fast_traceframes (void);
 
-static int run_inferior_command (char *cmd, int len);
-
 static int
 read_inferior_integer (CORE_ADDR symaddr, int *val)
 {
@@ -6662,7 +6660,7 @@ static struct ltt_available_probe gdb_ust_probe =
    thread by means of direct memory xfering, and a socket for
    synchronization.  */
 
-static int
+int
 run_inferior_command (char *cmd, int len)
 {
   int err = -1;
@@ -7033,6 +7031,7 @@ gdb_agent_helper_thread (void *arg)
 	  int fd;
 	  char buf[1];
 	  int ret;
+	  int stop_loop = 0;
 
 	  tmp = sizeof (sockaddr);
 
@@ -7065,8 +7064,12 @@ gdb_agent_helper_thread (void *arg)
 
 	  if (cmd_buf[0])
 	    {
+	      if (strncmp ("close", cmd_buf, 5) == 0)
+		{
+		  stop_loop = 1;
+		}
 #ifdef HAVE_UST
-	      if (strcmp ("qTfSTM", cmd_buf) == 0)
+	      else if (strcmp ("qTfSTM", cmd_buf) == 0)
 		{
 		  cmd_qtfstm (cmd_buf);
 		}
@@ -7098,6 +7101,20 @@ gdb_agent_helper_thread (void *arg)
 	  /* Fix compiler's warning: ignoring return value of 'write'.  */
 	  ret = write (fd, buf, 1);
 	  close (fd);
+
+	  if (stop_loop)
+	    {
+	      close (listen_fd);
+	      unlink (agent_socket_name);
+
+	      /* Sleep endlessly to wait the whole inferior stops.  This
+		 thread can not exit because GDB or GDBserver may still need
+		 'current_inferior' (representing this thread) to access
+		 inferior memory.  Otherwise, this thread exits earlier than
+		 other threads, and 'current_inferior' is set to NULL.  */
+	      while (1)
+		sleep (10);
+	    }
 	}
     }
 
-- 
1.7.0.4

Re: [PATCH 3/4] New agent command 'kill' and used by gdbserver

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Thu, 14 Jun 2012 22:50:01 +0800
> CC: <gdb-patches@sourceware.org>
> 
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -33585,6 +33585,10 @@ for an error
>  
>  @end table
>  
> +@item close
> +Closes the in-process agent.  This command is sent when @value{GDBN} or GDBserver
> +is about to kill inferiors.
> +

Shouldn't there be a "Reply" section, as with other commands?

If not, this part is OK with me.

Thanks.

Re: [PATCH 3/4] New agent command 'kill' and used by gdbserver

[Pedro Alves]

On 06/14/2012 03:50 PM, Yao Qi wrote:

> On Wednesday 13 June 2012 00:14:19 Pedro Alves wrote:
>>> > > +  run_inferior_command ("kill", strlen ("kill"));
>> > 
>> > - Missing space before parens.
>> > 
>> > - What happens if there's no IPA to talk to?
>> > 
>> > - Also, all callers of run_inferior_command command currently
>> >   pass "cmd, strlen (cmd) + 1".
>> > 
>> > - The kill_inferior function takes a PID as argument.  It's not
>> >   right to assume that PID is the current process.  IOW, in a
>> >   multi-process scenario, you may well run the inferior command
>> >   on the wrong inferior as is.
> This new patch addresses all your comments except this one above.  IMO, the
>  whole agent work in GDBserver doesn't consider much about multi-process, so
>  this problem shall be addressed as a whole, when we start to support agent in
>  multi-process.  I leave a FIXME in comment there.  WDYT?


I'd really rather see that fixed now.  Even with single process,
you may get here without a selected inferior at all.  We just need
to lookup a thread of PID, and select it as current_inferior for the
duration of run_inferior_command, I think?  (wrapped with the
save_inferior dance).

> +int
> +kill_inferior (int pid)
> +{


> +  char buf[IPA_CMD_BUF_SIZE];
> +
> +  if (!maybe_write_ipa_not_loaded (buf))
> +    {
> +      strcpy (buf, "close");
> +      /* FIXME: It is wrong to assume PID is the current process.  In
> +	 a multiple-process scenario, we may run inferior command on
> +	 the wrong process.  */
> +      run_inferior_command (buf, strlen (buf) + 1);
> +    }


I'd rather have this whole block be factored out to a function
in tracepoint.c (tracepoint_about_to_kill for example).  More so
with a fix to set current_inferior around run_inferior_command.
Then we wouldn't have to export details of the IPA, such as
maybe_write_ipa_not_loaded or run_inferior_command.

-- 
Pedro Alves

Re: [PATCH 3/4] New agent command 'kill' and used by gdbserver

[Yao Qi]

On 06/16/2012 03:24 AM, Pedro Alves wrote:
> 
> I'd really rather see that fixed now.  Even with single process,
> you may get here without a selected inferior at all.  We just need
> to lookup a thread of PID, and select it as current_inferior for the
> duration of run_inferior_command, I think?  (wrapped with the
> save_inferior dance).
> 
>> > +int
>> > +kill_inferior (int pid)
>> > +{
> 
>> > +  char buf[IPA_CMD_BUF_SIZE];
>> > +
>> > +  if (!maybe_write_ipa_not_loaded (buf))
>> > +    {
>> > +      strcpy (buf, "close");
>> > +      /* FIXME: It is wrong to assume PID is the current process.  In
>> > +	 a multiple-process scenario, we may run inferior command on
>> > +	 the wrong process.  */
>> > +      run_inferior_command (buf, strlen (buf) + 1);
>> > +    }
> 
> I'd rather have this whole block be factored out to a function
> in tracepoint.c (tracepoint_about_to_kill for example).  More so
> with a fix to set current_inferior around run_inferior_command.
> Then we wouldn't have to export details of the IPA, such as
> maybe_write_ipa_not_loaded or run_inferior_command.

Here is the new one.

-- 
Yao (齐尧) 

gdb/gdbserver:

2012-06-14  Yao Qi  <yao@codesourcery.com>

	* server.h: Declare gdb_agent_about_to_close.
	* target.c (kill_inferior): Include "agent.h".
	New.  Send command 'kill'.
	* target.h (kill_inferior): Removed macro.
	* tracepoint.c (gdb_agent_about_to_close): New.
	(gdb_agent_helper_thread): Handle command 'close'.
	Wait endlessly until the inferior stops.
---
 gdb/gdbserver/server.h     |    1 +
 gdb/gdbserver/target.c     |    8 +++++++
 gdb/gdbserver/target.h     |    3 +--
 gdb/gdbserver/tracepoint.c |   53 +++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 02dfa29..752bc3b 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -467,6 +467,7 @@ void stop_tracing (void);
 
 int claim_trampoline_space (ULONGEST used, CORE_ADDR *trampoline);
 int have_fast_tracepoint_trampoline_buffer (char *msgbuf);
+void gdb_agent_about_to_close (int pid);
 #endif
 
 struct traceframe;
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 7539476..e8b2b08 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -182,3 +182,11 @@ target_waitstatus_to_string (const struct target_waitstatus *ws)
 
   return buf;
 }
+
+int
+kill_inferior (int pid)
+{
+  gdb_agent_about_to_close (pid);
+
+  return (*the_target->kill) (pid);
+}
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 0cf5687..9f96e04 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -409,8 +409,7 @@ void set_target_ops (struct target_ops *);
 #define myattach(pid) \
   (*the_target->attach) (pid)
 
-#define kill_inferior(pid) \
-  (*the_target->kill) (pid)
+int kill_inferior (int);
 
 #define detach_inferior(pid) \
   (*the_target->detach) (pid)
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 8fcaa46..7ad77f0 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -3923,6 +3923,38 @@ cmd_qtstmat (char *packet)
     run_inferior_command (packet, strlen (packet) + 1);
 }
 
+/* Sent the agent a command to close it.  */
+
+void
+gdb_agent_about_to_close (int pid)
+{
+  char buf[IPA_CMD_BUF_SIZE];
+
+  if (!maybe_write_ipa_not_loaded (buf))
+    {
+      struct thread_info *save_inferior;
+      struct inferior_list_entry *inf = all_threads.head;
+
+      save_inferior = current_inferior;
+
+      /* Find a certain thread which belongs to process PID.  */
+      while (inf != NULL)
+	{
+	  if (ptid_get_pid (inf->id) == pid)
+	    break;
+	  inf = inf->next;
+	}
+
+      current_inferior = (struct thread_info *) inf;
+
+      strcpy (buf, "close");
+
+      run_inferior_command (buf, strlen (buf) + 1);
+
+      current_inferior = save_inferior;
+    }
+}
+
 /* Return the minimum instruction size needed for fast tracepoints as a
    hexadecimal number.  */
 
@@ -7033,6 +7065,7 @@ gdb_agent_helper_thread (void *arg)
 	  int fd;
 	  char buf[1];
 	  int ret;
+	  int stop_loop = 0;
 
 	  tmp = sizeof (sockaddr);
 
@@ -7065,8 +7098,12 @@ gdb_agent_helper_thread (void *arg)
 
 	  if (cmd_buf[0])
 	    {
+	      if (strncmp ("close", cmd_buf, 5) == 0)
+		{
+		  stop_loop = 1;
+		}
 #ifdef HAVE_UST
-	      if (strcmp ("qTfSTM", cmd_buf) == 0)
+	      else if (strcmp ("qTfSTM", cmd_buf) == 0)
 		{
 		  cmd_qtfstm (cmd_buf);
 		}
@@ -7098,6 +7135,20 @@ gdb_agent_helper_thread (void *arg)
 	  /* Fix compiler's warning: ignoring return value of 'write'.  */
 	  ret = write (fd, buf, 1);
 	  close (fd);
+
+	  if (stop_loop)
+	    {
+	      close (listen_fd);
+	      unlink (agent_socket_name);
+
+	      /* Sleep endlessly to wait the whole inferior stops.  This
+		 thread can not exit because GDB or GDBserver may still need
+		 'current_inferior' (representing this thread) to access
+		 inferior memory.  Otherwise, this thread exits earlier than
+		 other threads, and 'current_inferior' is set to NULL.  */
+	      while (1)
+		sleep (10);
+	    }
 	}
     }
 
-- 
1.7.9.5

Re: [PATCH 3/4] New agent command 'kill' and used by gdbserver

[Pedro Alves]

On 06/20/2012 02:49 PM, Yao Qi wrote:

> +
> +int
> +kill_inferior (int pid)
> +{
> +  gdb_agent_about_to_close (pid);


"about to kill" was meant to indicate that the function does something
just before the caller kills.  "about to close" sounds like the function
is called to react to the fact that the the caller is about to close
something else, but that is not what is happening, as the "about to close"
function is what really closes.  gdb_agent_about_to_kill or gdb_agent_close
would sound right, but about_to_close doesn't.

> +/* Sent the agent a command to close it.  */

> +
> +void
> +gdb_agent_about_to_close (int pid)
> +{
> +  char buf[IPA_CMD_BUF_SIZE];
> +
> +  if (!maybe_write_ipa_not_loaded (buf))
> +    {
> +      struct thread_info *save_inferior;
> +      struct inferior_list_entry *inf = all_threads.head;
> +
> +      save_inferior = current_inferior;
> +
> +      /* Find a certain thread which belongs to process PID.  */
> +      while (inf != NULL)
> +	{
> +	  if (ptid_get_pid (inf->id) == pid)
> +	    break;
> +	  inf = inf->next;
> +	}


This is a little simpler if written as a for loop:

      struct inferior_list_entry *inf;

      /* Find a certain thread which belongs to process PID.  */
      for (inf = all_threads.head; inf != NULL; inf = inf->next)
        if (ptid_get_pid (inf->id) == pid)
	  break;

Okay with those changes.

-- 
Pedro Alves

[committed] : [PATCH 0/4] PR14161: a partial fix

[Yao Qi]

On Saturday, June 09, 2012 08:46:38 PM Yao Qi wrote:
> Hi,
> PR14161 reports that agent socket '/tmp/gdb_ustXXXX' is not removed when
> program is done.  This patch series is to address this problem partially
> only in GDBserver side (in patch 3/4) and kfail it temporarily in GDB
> side (in patch 4/4) due to some limitations on threads control.
> Patch 1/4 is a new test case to show this problem.

This is what I applied to HEAD.

-- 
Yao (齐尧)

gdb/gdbserver/
	PR remote/14161.

	* server.h: Declare gdb_agent_about_to_close.
	* target.c (kill_inferior): Include "agent.h".
	New.  Send command 'kill'.
    	* target.h (kill_inferior): Removed macro.
    	* tracepoint.c (gdb_agent_about_to_close): New.
    	(gdb_agent_helper_thread): Handle command 'close'.
    	Wait endlessly until the inferior stops.
    	Install gdb_agent_remove_socket to atexit hook.
    	(agent_socket_name): New static variable.
    	(gdb_agent_socket_init): Replace local variable 'name' with
    	'agent_socket_name'.
    	(gdb_agent_remove_socket): New.

gdb/doc/
    	* gdb.texinfo (IPA Protocol Commands): Document new command
    	'close'.

gdb/testsuite/
	KFAIL for PR remote/14161.
	* gdb.trace/strace.exp (strace_remove_socket): kfail for native.
	Cleanup socket files.
	(strace_info_marker): Detach inferior.

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index facca8f..a4503bf 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33673,6 +33673,10 @@ for an error
 
 @end table
 
+@item close
+Closes the in-process agent.  This command is sent when @value{GDBN} or GDBserver
+is about to kill inferiors.
+
 @item qTfSTM
 @xref{qTfSTM}.
 @item qTsSTM
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 02dfa29..752bc3b 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -467,6 +467,7 @@ void stop_tracing (void);
 
 int claim_trampoline_space (ULONGEST used, CORE_ADDR *trampoline);
 int have_fast_tracepoint_trampoline_buffer (char *msgbuf);
+void gdb_agent_about_to_close (int pid);
 #endif
 
 struct traceframe;
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 7539476..e8b2b08 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -182,3 +182,11 @@ target_waitstatus_to_string (const struct target_waitstatus *ws)
 
   return buf;
 }
+
+int
+kill_inferior (int pid)
+{
+  gdb_agent_about_to_close (pid);
+
+  return (*the_target->kill) (pid);
+}
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 0cf5687..9f96e04 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -409,8 +409,7 @@ void set_target_ops (struct target_ops *);
 #define myattach(pid) \
   (*the_target->attach) (pid)
 
-#define kill_inferior(pid) \
-  (*the_target->kill) (pid)
+int kill_inferior (int);
 
 #define detach_inferior(pid) \
   (*the_target->detach) (pid)
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index f103dfc..7ad77f0 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -3923,6 +3923,38 @@ cmd_qtstmat (char *packet)
     run_inferior_command (packet, strlen (packet) + 1);
 }
 
+/* Sent the agent a command to close it.  */
+
+void
+gdb_agent_about_to_close (int pid)
+{
+  char buf[IPA_CMD_BUF_SIZE];
+
+  if (!maybe_write_ipa_not_loaded (buf))
+    {
+      struct thread_info *save_inferior;
+      struct inferior_list_entry *inf = all_threads.head;
+
+      save_inferior = current_inferior;
+
+      /* Find a certain thread which belongs to process PID.  */
+      while (inf != NULL)
+	{
+	  if (ptid_get_pid (inf->id) == pid)
+	    break;
+	  inf = inf->next;
+	}
+
+      current_inferior = (struct thread_info *) inf;
+
+      strcpy (buf, "close");
+
+      run_inferior_command (buf, strlen (buf) + 1);
+
+      current_inferior = save_inferior;
+    }
+}
+
 /* Return the minimum instruction size needed for fast tracepoints as a
    hexadecimal number.  */
 
@@ -6748,13 +6780,14 @@ init_named_socket (const char *name)
   return fd;
 }
 
+static char agent_socket_name[UNIX_PATH_MAX];
+
 static int
 gdb_agent_socket_init (void)
 {
   int result, fd;
-  char name[UNIX_PATH_MAX];
 
-  result = xsnprintf (name, UNIX_PATH_MAX, "%s/gdb_ust%d",
+  result = xsnprintf (agent_socket_name, UNIX_PATH_MAX, "%s/gdb_ust%d",
 		      SOCK_DIR, getpid ());
   if (result >= UNIX_PATH_MAX)
     {
@@ -6762,11 +6795,11 @@ gdb_agent_socket_init (void)
       return -1;
     }
 
-  fd = init_named_socket (name);
+  fd = init_named_socket (agent_socket_name);
   if (fd < 0)
     warning ("Error initializing named socket (%s) for communication with the "
 	     "ust helper thread. Check that directory exists and that it "
-	     "is writable.", name);
+	     "is writable.", agent_socket_name);
 
   return fd;
 }
@@ -6995,6 +7028,13 @@ gdb_ust_init (void)
 #endif /* HAVE_UST */
 
 #include <sys/syscall.h>
+#include <stdlib.h>
+
+static void
+gdb_agent_remove_socket (void)
+{
+  unlink (agent_socket_name);
+}
 
 /* Helper thread of agent.  */
 
@@ -7003,6 +7043,8 @@ gdb_agent_helper_thread (void *arg)
 {
   int listen_fd;
 
+  atexit (gdb_agent_remove_socket);
+
   while (1)
     {
       listen_fd = gdb_agent_socket_init ();
@@ -7023,6 +7065,7 @@ gdb_agent_helper_thread (void *arg)
 	  int fd;
 	  char buf[1];
 	  int ret;
+	  int stop_loop = 0;
 
 	  tmp = sizeof (sockaddr);
 
@@ -7055,8 +7098,12 @@ gdb_agent_helper_thread (void *arg)
 
 	  if (cmd_buf[0])
 	    {
+	      if (strncmp ("close", cmd_buf, 5) == 0)
+		{
+		  stop_loop = 1;
+		}
 #ifdef HAVE_UST
-	      if (strcmp ("qTfSTM", cmd_buf) == 0)
+	      else if (strcmp ("qTfSTM", cmd_buf) == 0)
 		{
 		  cmd_qtfstm (cmd_buf);
 		}
@@ -7088,6 +7135,20 @@ gdb_agent_helper_thread (void *arg)
 	  /* Fix compiler's warning: ignoring return value of 'write'.  */
 	  ret = write (fd, buf, 1);
 	  close (fd);
+
+	  if (stop_loop)
+	    {
+	      close (listen_fd);
+	      unlink (agent_socket_name);
+
+	      /* Sleep endlessly to wait the whole inferior stops.  This
+		 thread can not exit because GDB or GDBserver may still need
+		 'current_inferior' (representing this thread) to access
+		 inferior memory.  Otherwise, this thread exits earlier than
+		 other threads, and 'current_inferior' is set to NULL.  */
+	      while (1)
+		sleep (10);
+	    }
 	}
     }
 
diff --git a/gdb/testsuite/gdb.trace/strace.exp b/gdb/testsuite/gdb.trace/strace.exp
index d9e90b6..f09eeb3 100644
--- a/gdb/testsuite/gdb.trace/strace.exp
+++ b/gdb/testsuite/gdb.trace/strace.exp
@@ -37,12 +37,103 @@ if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags
     return -1
 }
 
+# Test that the socket file is removed when GDB quits, detaches or
+# resumes the inferior until it exits.
+
+proc strace_remove_socket { action } {
+    with_test_prefix "remove_socket_after_${action}" {
+
+    global executable
+    global gdb_prompt
+    global libipa
+
+    # Restart with a fresh gdb.
+    clean_restart $executable
+    gdb_load_shlibs $libipa
+    if ![runto_main] {
+	fail "Can't run to main"
+	return -1
+    }
+
+    # List the markers in program.
+    gdb_test "info static-tracepoint-markers" \
+	".*ust/bar\[\t \]+n\[\t \]+.*ust/bar2\[\t \]+n\[\t \]+.*"
+
+    set pid ""
+    set test "collect pid"
+    gdb_test_multiple "info inferiors" $test {
+	-re "process (\[-0-9a-fx\]+) \[^\n\]*\n.*${gdb_prompt} $" {
+	    set pid $expect_out(1,string)
+	    pass $test
+	}
+	-re ".*${gdb_prompt} $" {
+	    fail $test
+	}
+    }
+
+    set test "socket file exists"
+    set socket_file "/tmp/gdb_ust${pid}"
+    set status [remote_exec target "sh -c { \[ -S $socket_file \] }"]
+
+    if { [lindex $status 0] == 0 } {
+	pass $test
+    } else {
+	fail $test
+    }
+
+    send_gdb "${action}\n"
+    gdb_expect {
+	-re "A debugging session is active.\r\n.*\r\nQuit anyway\\? \\(y or n\\) $" {
+	    send_gdb "y\n"
+	}
+	-re "Detaching .*, process .*$" {
+	}
+	-re "Continuing.*$" {
+	}
+    }
+
+    set exists 1
+
+    for {set i 1} {$i <= 5} {incr i} {
+	set status [remote_exec target "sh -c { \[ -S $socket_file \] }"]
+	if { [lindex $status 0] != 0 } {
+	    set exists 0
+	    break
+	}
+	sleep 1
+    }
+
+    if { ![is_remote target] && ![string equal $action "detach"] } {
+	setup_kfail gdb/14161 *-*-*
+    }
+
+    set test "socket file removed"
+
+    if { $exists } {
+	fail $test
+	# Since $socket_file is a socket file instead of a regular file, we
+	# can't use 'remote_file target delete $socket_file' here.
+	remote_exec target "sh -c \"rm -r $socket_file\""
+    } else {
+	pass $test
+    }
+
+    if { [string equal $action "quit"] && [is_remote host] } {
+	global gdb_spawn_id
+	# unset gdb_spawn_id here to avoid sending command 'quit' to GDB
+	# later in default_gdb_exit.
+	unset gdb_spawn_id
+    }
+}}
+
 proc strace_info_marker { } { with_test_prefix "info_marker" {
     global executable
     global gdb_prompt
+    global libipa
 
     # Restart with a fresh gdb.
     clean_restart $executable
+    gdb_load_shlibs $libipa
     if ![runto_main] {
 	fail "Can't run to main"
 	return -1
@@ -59,6 +150,13 @@ proc strace_info_marker { } { with_test_prefix "info_marker" {
 	    pass "info threads"
 	}
     }
+
+    # GDB detaches inferior so that the socket file can be removed.
+    gdb_test_multiple "detach" "detach" {
+	-re "Detaching .*, process .*${gdb_prompt} $" {
+	    pass "detach"
+	}
+    }
 }}
 
 proc strace_probe_marker { } { with_test_prefix "probe_marker" {
@@ -66,9 +164,11 @@ proc strace_probe_marker { } { with_test_prefix "probe_marker" {
     global expect_out
     global gdb_prompt
     global hex
+    global libipa
 
     # Restart with a fresh gdb.
     clean_restart $executable
+    gdb_load_shlibs $libipa
     if ![runto_main] {
 	fail "Can't run to main"
 	return -1
@@ -97,9 +197,11 @@ with_test_prefix "trace_same_addr $type" {
     global expect_out
     global gdb_prompt
     global hex
+    global libipa
 
     # Restart with a fresh gdb.
     clean_restart $executable
+    gdb_load_shlibs $libipa
     if ![runto_main] {
 	fail "Can't run to main"
 	return -1
@@ -192,9 +294,11 @@ proc strace_trace_on_diff_addr { } { with_test_prefix "trace_diff_addr" {
     global expect_out
     global gdb_prompt
     global hex
+    global libipa
 
     # Restart with a fresh gdb.
     clean_restart $executable
+    gdb_load_shlibs $libipa
     if ![runto_main] {
 	fail "Can't run to main"
 	return -1
@@ -240,28 +344,30 @@ proc strace_trace_on_diff_addr { } { with_test_prefix "trace_diff_addr" {
     gdb_test "tfind" "Target failed to find requested trace frame\\..*"
 }}
 
-clean_restart $executable
+# Run it on x86/x86_64 linux.
+if { [istarget "x86_64-*-linux*"] || [istarget "i\[34567\]86-*-linux*"] } {
+    strace_info_marker
+    strace_remove_socket "quit"
+    strace_remove_socket "detach"
+    strace_remove_socket "continue"
+}
 
+clean_restart $executable
+gdb_load_shlibs $libipa
 if ![runto_main] {
     fail "Can't run to main to check for trace support"
     return -1
 }
-
-# Run it on native x86/x86_64 linux.
-if { ![is_remote target]
-     && ([istarget "x86_64-*-linux*"] || [istarget "i\[34567\]86-*-linux*"]) } {
-    strace_info_marker
-    return
-}
-
 if { ![gdb_target_supports_trace] } then {
+    # At this point, the socket file has been created.  We must make sure it is
+    # removed when we return here.  GDB detaches inferior so that the socket
+    # file can be removed.  Note that GDB simply kill inferior doesn't remove
+    # the socket file.
+    gdb_test "detach" "Detaching .*, process .*"
     unsupported "Current target does not support trace"
     return -1;
 }
 
-gdb_load_shlibs $libipa
-
-strace_info_marker
 strace_probe_marker
 
 strace_trace_on_same_addr "trace"