From patchwork Wed Feb 6 18:02:04 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 31334 Received: (qmail 96955 invoked by alias); 6 Feb 2019 18:02:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 96947 invoked by uid 89); 6 Feb 2019 18:02:09 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=pedro X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 06 Feb 2019 18:02:07 +0000 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E3C848E58D; Wed, 6 Feb 2019 18:02:05 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id E3D76166A6; Wed, 6 Feb 2019 18:02:04 +0000 (UTC) Subject: Re: [PATCH v2] gdbserver: When attaching, add process before lwps To: Alan Hayward , "gdb-patches@sourceware.org" References: <20190206121859.81717-1-alan.hayward@arm.com> Cc: nd From: Pedro Alves Message-ID: Date: Wed, 6 Feb 2019 18:02:04 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 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 > > * 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 > > * 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 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"