From: Pedro Alves <palves@redhat.com>
To: Alan Hayward <Alan.Hayward@arm.com>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Cc: nd <nd@arm.com>
Subject: Re: [PATCH v2] gdbserver: When attaching, add process before lwps
Date: Wed, 06 Feb 2019 18:02:00 -0000 [thread overview]
Message-ID: <a345cebe-6e8a-b139-da59-d29a97bc4152@redhat.com> (raw)
In-Reply-To: <20190206121859.81717-1-alan.hayward@arm.com>
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
prev parent reply other threads:[~2019-02-06 18:02 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-06 12:19 Alan Hayward
2019-02-06 18:02 ` Pedro Alves [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a345cebe-6e8a-b139-da59-d29a97bc4152@redhat.com \
--to=palves@redhat.com \
--cc=Alan.Hayward@arm.com \
--cc=gdb-patches@sourceware.org \
--cc=nd@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).