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

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(-)