* [PATCH v2] gdbserver: When attaching, add process before lwps
@ 2019-02-06 12:19 Alan Hayward
2019-02-06 18:02 ` Pedro Alves
0 siblings, 1 reply; 2+ messages in thread
From: Alan Hayward @ 2019-02-06 12:19 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
(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(-)
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"
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2] gdbserver: When attaching, add process before lwps
2019-02-06 12:19 [PATCH v2] gdbserver: When attaching, add process before lwps Alan Hayward
@ 2019-02-06 18:02 ` Pedro Alves
0 siblings, 0 replies; 2+ messages in thread
From: Pedro Alves @ 2019-02-06 18:02 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-02-06 18:02 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-06 12:19 [PATCH v2] gdbserver: When attaching, add process before lwps Alan Hayward
2019-02-06 18:02 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).