[v2] gdbserver: When attaching, add process before lwps

Message ID 20190206121859.81717-1-alan.hayward@arm.com
State New
Headers show
Series
  • [v2] gdbserver: When attaching, add process before lwps
Related show

Commit Message

Alan Hayward Feb. 6, 2019, 12:19 p.m.
(V2 adds error checks and test case.
 Tested with native-extended-gdbserver on x64.
 Difficult to test that on AArch64 due to random testsuite hangs
 when running native-extended-gdbserver on HEAD (On my list to
 fix soon, especially as it causes buildbot to timeout))

The recent BP/WP changes for AArch64 swapping the order in add_lwp()
so that the process was added before the lwp. This was due to the lwp
creation requiring the process data.

This also needs changing in linux_attach().

Also add additional checks to make sure cannot attach to the same
process twice. Add test case for this - do this by splitting
attach.exp into distinct pass and error case sections.

Fixes gdb.server/ext-attach.exp on Aarch64.

gdb/gdbserver/ChangeLog:

2019-02-06  Alan Hayward  <alan.hayward@arm.com>

	* linux-low.c (linux_attach): Add process before lwp.
	* server.c (attach_inferior): Check if already attached.

2019-02-06  Alan Hayward  <alan.hayward@arm.com>

	* gdb.base/attach.exp: Add double attach test.
---
 gdb/gdbserver/linux-low.c         |   7 +-
 gdb/gdbserver/server.c            |   3 +
 gdb/testsuite/gdb.base/attach.exp | 102 ++++++++++++++++++++++++------
 3 files changed, 90 insertions(+), 22 deletions(-)

-- 
2.17.2 (Apple Git-113)

Comments

Pedro Alves Feb. 6, 2019, 6:02 p.m. | #1
On 02/06/2019 12:19 PM, Alan Hayward wrote:
> (V2 adds error checks and test case.

>  Tested with native-extended-gdbserver on x64.

>  Difficult to test that on AArch64 due to random testsuite hangs

>  when running native-extended-gdbserver on HEAD (On my list to

>  fix soon, especially as it causes buildbot to timeout))

> 

> The recent BP/WP changes for AArch64 swapping the order in add_lwp()

> so that the process was added before the lwp. This was due to the lwp

> creation requiring the process data.

> 

> This also needs changing in linux_attach().

> 

> Also add additional checks to make sure cannot attach to the same

> process twice. Add test case for this - do this by splitting

> attach.exp into distinct pass and error case sections.

> 

> Fixes gdb.server/ext-attach.exp on Aarch64.

> 

> gdb/gdbserver/ChangeLog:

> 

> 2019-02-06  Alan Hayward  <alan.hayward@arm.com>

> 

> 	* linux-low.c (linux_attach): Add process before lwp.

> 	* server.c (attach_inferior): Check if already attached.

> 


Add:

gdb/testsuite/ChangeLog:

> 2019-02-06  Alan Hayward  <alan.hayward@arm.com>

> 

> 	* gdb.base/attach.exp: Add double attach test.

> ---

>  gdb/gdbserver/linux-low.c         |   7 +-

>  gdb/gdbserver/server.c            |   3 +

>  gdb/testsuite/gdb.base/attach.exp | 102 ++++++++++++++++++++++++------

>  3 files changed, 90 insertions(+), 22 deletions(-)

> 

> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c

> index 44016d2310..8c5a51f23c 100644

> --- a/gdb/gdbserver/linux-low.c

> +++ b/gdb/gdbserver/linux-low.c

> @@ -1188,18 +1188,19 @@ linux_attach (unsigned long pid)

>    ptid_t ptid = ptid_t (pid, pid, 0);

>    int err;

>  

> +  proc = linux_add_process (pid, 1);

> +

>    /* Attach to PID.  We will check for other threads

>       soon.  */

>    err = linux_attach_lwp (ptid);

>    if (err != 0)

>      {

> -      std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);

> +      remove_process (proc);

>  

> +      std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);

>        error ("Cannot attach to process %ld: %s", pid, reason.c_str ());

>      }

>  

> -  proc = linux_add_process (pid, 1);

> -

>    /* Don't ignore the initial SIGSTOP if we just attached to this

>       process.  It will be collected by wait shortly.  */

>    initial_thread = find_thread_ptid (ptid_t (pid, pid, 0));

> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c

> index f9bfdd7307..e960c10d40 100644

> --- a/gdb/gdbserver/server.c

> +++ b/gdb/gdbserver/server.c

> @@ -295,6 +295,9 @@ attach_inferior (int pid)

>    /* myattach should return -1 if attaching is unsupported,

>       0 if it succeeded, and call error() otherwise.  */

>  

> +  if (find_process_pid (pid) != nullptr)

> +    error ("Already attached to process %d\n", pid);

> +

>    if (myattach (pid) != 0)

>      return -1;

>  

> diff --git a/gdb/testsuite/gdb.base/attach.exp b/gdb/testsuite/gdb.base/attach.exp

> index 2343354dda..ea1da7098f 100644

> --- a/gdb/testsuite/gdb.base/attach.exp

> +++ b/gdb/testsuite/gdb.base/attach.exp

> @@ -45,7 +45,9 @@ if [get_compiler_info] {

>      return -1

>  }

>  

> -proc do_attach_tests {} {

> +# This is a test of the error cases for gdb's ability to attach to a running process.


Wrap this long line.

> +

> +proc do_attach_failure_tests {} {

>      global gdb_prompt

>      global binfile

>      global escapedbinfile

> @@ -55,6 +57,8 @@ proc do_attach_tests {} {

>      global timeout

>      global decimal

>      

> +    gdb_load ${binfile}

> +

>      # Figure out a regular expression that will match the sysroot,

>      # noting that the default sysroot is "target:", and also noting

>      # that GDB will strip "target:" from the start of filenames when

> @@ -154,6 +158,74 @@ proc do_attach_tests {} {

>  	}

>      }

>      

> +    # Verify that we can't double attach to the process

> +


Add missing period.

> +    set test "first attach"

> +    gdb_test_multiple "attach $testpid" "$test" {

> +	-re "Attaching to program.*`?$escapedbinfile'?, process $testpid.*main.*at .*$srcfile:.*$gdb_prompt $" {

> +	    pass "$test"

> +	}

> +	-re "Attaching to program.*`?$escapedbinfile\.exe'?, process $testpid.*\[Switching to thread $testpid\..*\].*$gdb_prompt $" {

> +	    # Response expected on Cygwin


Period.

> +	    pass "$test"

> +	}

> +    }

> +

> +    gdb_test "add-inferior" "Added inferior 2.*" "add empty inferior 2"

> +    gdb_test "inferior 2" "Switching to inferior 2.*" "switch to inferior 2"

> +

> +    set test "fail to attach again"

> +    gdb_test_multiple "attach $testpid" "$test" {

> +	-re "Attaching to process $testpid.*warning: process .* is already traced by process .*$gdb_prompt $" {

> +	    pass "$test"

> +	}

> +	-re "Attaching to process .* failed.*$gdb_prompt $" {

> +	    # Response expected when using gdbserver


Period.

> +	    pass "$test"

> +	}

> +    }

> +

> +    set test "exit after attach failures"

> +    gdb_test "kill" \

> +	"" \

> +	"$test" \

> +	"Kill the program being debugged.*y or n. $" \

> +	"y"


This isn't really killing anything.  If you mean to
kill the process with GDB then you need to switch to
inferior 1 first.

> +

> +    # Another "don't leave a process around"

> +    kill_wait_spawned_process $test_spawn_id

> +}

> +

> +# This is a test of gdb's ability to attach to a running process.

> +

> +proc do_attach_tests {} {

> +    global gdb_prompt

> +    global binfile

> +    global escapedbinfile

> +    global srcfile

> +    global testfile

> +    global subdir

> +    global timeout

> +    global decimal

> +

> +    gdb_load ${binfile}

> +

> +    # Figure out a regular expression that will match the sysroot,

> +    # noting that the default sysroot is "target:", and also noting

> +    # that GDB will strip "target:" from the start of filenames when

> +    # operating on the local filesystem.  However the default sysroot

> +    # can be set via configure option --with-sysroot, which can be "/".

> +    # If $binfile is a absolute path, so pattern

> +    # "$sysroot$escapedbinfile" below is wrong.  Use [^\r\n]* to make

> +    # $sysroot simple.

> +    set sysroot "\[^\r\n\]*"

> +

> +    # Start the program running and then wait for a bit, to be sure

> +    # that it can be attached to.

> +

> +    set test_spawn_id [spawn_wait_for_attach $binfile]

> +    set testpid [spawn_id_get_pid $test_spawn_id]

> +

>      # Verify that we can attach to the process by first giving its

>      # executable name via the file command, and using attach with the

>      # process ID.

> @@ -313,6 +385,8 @@ proc do_attach_tests {} {

>      kill_wait_spawned_process $test_spawn_id

>  }

>  

> +# Test attaching when the target is inside a system call


Add period.

> +

>  proc do_call_attach_tests {} {

>      global gdb_prompt

>      global binfile2

> @@ -435,24 +509,14 @@ proc test_command_line_attach_run {} {

>      }

>  }

>  

> -# Start with a fresh gdb

> -

> -gdb_exit

> -gdb_start

> -gdb_reinitialize_dir $srcdir/$subdir

> -gdb_load ${binfile}

> -

> -# This is a test of gdb's ability to attach to a running process.

> -

> -do_attach_tests

> -

> -# Test attaching when the target is inside a system call

> -

> -gdb_exit

> -gdb_start

> -

> -gdb_reinitialize_dir $srcdir/$subdir

> -do_call_attach_tests

> +foreach_with_prefix test { "do_attach_tests"

> +			   "do_attach_failure_tests"

> +			   "do_call_attach_tests" } {

> +    gdb_exit

> +    gdb_start

> +    gdb_reinitialize_dir $srcdir/$subdir

> +    $test

> +}


This seems like a bit of an odd pattern.  We can use
proc_with_prefix instead, and, I'd replace those
gdb_exit/gdb_start/gdb_reinitialize_dir's with
a clean_restart call in the procedures.  That gets
rid of the loop.  Like the patchlet below.

OK with these issues addressed.

From 196cae26007a9748f11ef66f7ac4965231aebec9 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Wed, 6 Feb 2019 17:48:04 +0000
Subject: [PATCH] diff

---
 gdb/testsuite/gdb.base/attach.exp | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/gdb/testsuite/gdb.base/attach.exp b/gdb/testsuite/gdb.base/attach.exp
index ea1da7098f..a4ea5cd2b8 100644
--- a/gdb/testsuite/gdb.base/attach.exp
+++ b/gdb/testsuite/gdb.base/attach.exp
@@ -47,7 +47,7 @@ if [get_compiler_info] {
 
 # This is a test of the error cases for gdb's ability to attach to a running process.
 
-proc do_attach_failure_tests {} {
+proc_with_prefix do_attach_failure_tests {} {
     global gdb_prompt
     global binfile
     global escapedbinfile
@@ -56,8 +56,8 @@ proc do_attach_failure_tests {} {
     global subdir
     global timeout
     global decimal
-    
-    gdb_load ${binfile}
+
+    clean_restart $binfile
 
     # Figure out a regular expression that will match the sysroot,
     # noting that the default sysroot is "target:", and also noting
@@ -198,7 +198,7 @@ proc do_attach_failure_tests {} {
 
 # This is a test of gdb's ability to attach to a running process.
 
-proc do_attach_tests {} {
+proc_with_prefix do_attach_tests {} {
     global gdb_prompt
     global binfile
     global escapedbinfile
@@ -208,7 +208,7 @@ proc do_attach_tests {} {
     global timeout
     global decimal
 
-    gdb_load ${binfile}
+    clean_restart $binfile
 
     # Figure out a regular expression that will match the sysroot,
     # noting that the default sysroot is "target:", and also noting
@@ -387,10 +387,12 @@ proc do_attach_tests {} {
 
 # Test attaching when the target is inside a system call
 
-proc do_call_attach_tests {} {
+proc_with_prefix do_call_attach_tests {} {
     global gdb_prompt
     global binfile2
-    
+
+    clean_restart
+
     set test_spawn_id [spawn_wait_for_attach $binfile2]
     set testpid [spawn_id_get_pid $test_spawn_id]
 
@@ -432,7 +434,7 @@ proc do_call_attach_tests {} {
     kill_wait_spawned_process $test_spawn_id
 }
 
-proc do_command_attach_tests {} {
+proc_with_prefix do_command_attach_tests {} {
     global gdb_prompt
     global binfile
     global verbose
@@ -509,14 +511,9 @@ proc test_command_line_attach_run {} {
     }
 }
 
-foreach_with_prefix test { "do_attach_tests"
-			   "do_attach_failure_tests"
-			   "do_call_attach_tests" } {
-    gdb_exit
-    gdb_start
-    gdb_reinitialize_dir $srcdir/$subdir
-    $test
-}
+do_attach_tests
+do_attach_failure_tests
+do_call_attach_tests
 
 # Test "gdb --pid"
 
-- 
2.14.4

Patch

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 44016d2310..8c5a51f23c 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -1188,18 +1188,19 @@  linux_attach (unsigned long pid)
   ptid_t ptid = ptid_t (pid, pid, 0);
   int err;
 
+  proc = linux_add_process (pid, 1);
+
   /* Attach to PID.  We will check for other threads
      soon.  */
   err = linux_attach_lwp (ptid);
   if (err != 0)
     {
-      std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);
+      remove_process (proc);
 
+      std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);
       error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
     }
 
-  proc = linux_add_process (pid, 1);
-
   /* Don't ignore the initial SIGSTOP if we just attached to this
      process.  It will be collected by wait shortly.  */
   initial_thread = find_thread_ptid (ptid_t (pid, pid, 0));
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index f9bfdd7307..e960c10d40 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -295,6 +295,9 @@  attach_inferior (int pid)
   /* myattach should return -1 if attaching is unsupported,
      0 if it succeeded, and call error() otherwise.  */
 
+  if (find_process_pid (pid) != nullptr)
+    error ("Already attached to process %d\n", pid);
+
   if (myattach (pid) != 0)
     return -1;
 
diff --git a/gdb/testsuite/gdb.base/attach.exp b/gdb/testsuite/gdb.base/attach.exp
index 2343354dda..ea1da7098f 100644
--- a/gdb/testsuite/gdb.base/attach.exp
+++ b/gdb/testsuite/gdb.base/attach.exp
@@ -45,7 +45,9 @@  if [get_compiler_info] {
     return -1
 }
 
-proc do_attach_tests {} {
+# This is a test of the error cases for gdb's ability to attach to a running process.
+
+proc do_attach_failure_tests {} {
     global gdb_prompt
     global binfile
     global escapedbinfile
@@ -55,6 +57,8 @@  proc do_attach_tests {} {
     global timeout
     global decimal
     
+    gdb_load ${binfile}
+
     # Figure out a regular expression that will match the sysroot,
     # noting that the default sysroot is "target:", and also noting
     # that GDB will strip "target:" from the start of filenames when
@@ -154,6 +158,74 @@  proc do_attach_tests {} {
 	}
     }
     
+    # Verify that we can't double attach to the process
+
+    set test "first attach"
+    gdb_test_multiple "attach $testpid" "$test" {
+	-re "Attaching to program.*`?$escapedbinfile'?, process $testpid.*main.*at .*$srcfile:.*$gdb_prompt $" {
+	    pass "$test"
+	}
+	-re "Attaching to program.*`?$escapedbinfile\.exe'?, process $testpid.*\[Switching to thread $testpid\..*\].*$gdb_prompt $" {
+	    # Response expected on Cygwin
+	    pass "$test"
+	}
+    }
+
+    gdb_test "add-inferior" "Added inferior 2.*" "add empty inferior 2"
+    gdb_test "inferior 2" "Switching to inferior 2.*" "switch to inferior 2"
+
+    set test "fail to attach again"
+    gdb_test_multiple "attach $testpid" "$test" {
+	-re "Attaching to process $testpid.*warning: process .* is already traced by process .*$gdb_prompt $" {
+	    pass "$test"
+	}
+	-re "Attaching to process .* failed.*$gdb_prompt $" {
+	    # Response expected when using gdbserver
+	    pass "$test"
+	}
+    }
+
+    set test "exit after attach failures"
+    gdb_test "kill" \
+	"" \
+	"$test" \
+	"Kill the program being debugged.*y or n. $" \
+	"y"
+
+    # Another "don't leave a process around"
+    kill_wait_spawned_process $test_spawn_id
+}
+
+# This is a test of gdb's ability to attach to a running process.
+
+proc do_attach_tests {} {
+    global gdb_prompt
+    global binfile
+    global escapedbinfile
+    global srcfile
+    global testfile
+    global subdir
+    global timeout
+    global decimal
+
+    gdb_load ${binfile}
+
+    # Figure out a regular expression that will match the sysroot,
+    # noting that the default sysroot is "target:", and also noting
+    # that GDB will strip "target:" from the start of filenames when
+    # operating on the local filesystem.  However the default sysroot
+    # can be set via configure option --with-sysroot, which can be "/".
+    # If $binfile is a absolute path, so pattern
+    # "$sysroot$escapedbinfile" below is wrong.  Use [^\r\n]* to make
+    # $sysroot simple.
+    set sysroot "\[^\r\n\]*"
+
+    # Start the program running and then wait for a bit, to be sure
+    # that it can be attached to.
+
+    set test_spawn_id [spawn_wait_for_attach $binfile]
+    set testpid [spawn_id_get_pid $test_spawn_id]
+
     # Verify that we can attach to the process by first giving its
     # executable name via the file command, and using attach with the
     # process ID.
@@ -313,6 +385,8 @@  proc do_attach_tests {} {
     kill_wait_spawned_process $test_spawn_id
 }
 
+# Test attaching when the target is inside a system call
+
 proc do_call_attach_tests {} {
     global gdb_prompt
     global binfile2
@@ -435,24 +509,14 @@  proc test_command_line_attach_run {} {
     }
 }
 
-# Start with a fresh gdb
-
-gdb_exit
-gdb_start
-gdb_reinitialize_dir $srcdir/$subdir
-gdb_load ${binfile}
-
-# This is a test of gdb's ability to attach to a running process.
-
-do_attach_tests
-
-# Test attaching when the target is inside a system call
-
-gdb_exit
-gdb_start
-
-gdb_reinitialize_dir $srcdir/$subdir
-do_call_attach_tests
+foreach_with_prefix test { "do_attach_tests"
+			   "do_attach_failure_tests"
+			   "do_call_attach_tests" } {
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+    $test
+}
 
 # Test "gdb --pid"