[PATCH 0/8] Fix various issues in --wrapper in GDBserver

[PATCH 0/8] Fix various issues in --wrapper in GDBserver

[Yao Qi]

The original goal of this patch series is to fix GDBserver bug that
it creates target description too early.  The real fix and details
can be found in patch #7.  Patch #5 and #6 are refactor patch, and
do preparations for patch #7.  Patch #8 is a cleanup patch.

When I fix this target description issue, I see some other problems,
so I fix them together within this patch series.  Patch #1 - #4 are
not strictly related to this target description creation issue.
Patch #1 lets GDBserver to complain when --attach and --wrapper are
used together.  Patch #2 adds a test for --wrapper in extended mode.
Patch #3 adds a test about restarting process, and includes a fix to
a problem exposed by the test.  Patch #4 adds a test for --wrapper
to restart process.  With these tests added, I can make sure my
following changes/patches don't break anything.

The whole series are tested on 86_64-linux both native and gdbserver.
OK for mainline and 7.10 branch?

*** BLURB HERE ***

Yao Qi (8):
  Disallow using --attach and --wrapper together.
  Test --wrapper in extended-remote
  Set general_thread after restart
  Test --wrapper when restarting process.
  Refactor start_inferior
  Set proc->priv->new_inferior out of linux_add_process
  Initialise target descrption after skipping extra traps for --wrapper
  Remove proc->priv->new_inferior

 gdb/gdbserver/linux-low.c                | 77 +++++++++++++++++++++++++-------
 gdb/gdbserver/linux-low.h                |  5 ---
 gdb/gdbserver/lynx-low.c                 |  1 +
 gdb/gdbserver/nto-low.c                  |  1 +
 gdb/gdbserver/server.c                   | 40 +++++++++++------
 gdb/gdbserver/spu-low.c                  |  1 +
 gdb/gdbserver/target.h                   | 10 +++++
 gdb/gdbserver/win32-low.c                |  1 +
 gdb/testsuite/gdb.server/ext-restart.exp | 65 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.server/ext-wrapper.exp | 72 +++++++++++++++++++++++++++++
 gdb/testsuite/lib/gdbserver-support.exp  | 16 ++++---
 11 files changed, 251 insertions(+), 38 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/ext-restart.exp
 create mode 100644 gdb/testsuite/gdb.server/ext-wrapper.exp

-- 
1.9.1

[PATCH 4/8] Test --wrapper when restarting process.

[Yao Qi]

My patch series will affect the code starting inferior in GDBserver
(callees of start_inferior), so we need tests to cover how
start_inferior is used in different cases.

In server.c:process_serial_event, start_inferior is used when
GBDserver receives 'R' packet, and this patch is to add a test
for this path, and see how --wrapper option works when the process
is restarted.

gdb/testsuite:

2015-07-17  Yao Qi  <yao.qi@linaro.org>

	* gdb.server/ext-wrapper.exp: Test --wrapper option when
	restarting process.
---
 gdb/testsuite/gdb.server/ext-wrapper.exp | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/gdb/testsuite/gdb.server/ext-wrapper.exp b/gdb/testsuite/gdb.server/ext-wrapper.exp
index 2ddd3a8..ac5f14a 100644
--- a/gdb/testsuite/gdb.server/ext-wrapper.exp
+++ b/gdb/testsuite/gdb.server/ext-wrapper.exp
@@ -47,6 +47,26 @@ gdb_test "run" "Breakpoint.* marker .*" "run to marker"
 
 gdb_test "print d" "\\$${decimal} = ${hex} \"1\".*"
 
+# Restart the process.
+with_test_prefix "restart" {
+    # Disable vRun packet and clear remote exec-file, so that GDB will
+    # use R packet to restart the process.
+    gdb_test_no_output "set remote run-packet off"
+    gdb_test_no_output "set remote exec-file"
+    set test "run to marker"
+    gdb_test_multiple "run" $test {
+	-re {Start it from the beginning\? \(y or n\) $} {
+	    send_gdb "y\n"
+	    exp_continue
+	}
+	-re "Breakpoint.* marker .*\r\n$gdb_prompt $" {
+	    pass $test
+	}
+    }
+
+    gdb_test "print d" "\\$${decimal} = ${hex} \"1\".*"
+}
+
 gdb_test "kill" "" "kill" "Kill the program being debugged.*" "y"
 
 gdb_test_no_output "monitor exit"
-- 
1.9.1

Re: [PATCH 4/8] Test --wrapper when restarting process.

[Pedro Alves]

On 07/20/2015 12:35 PM, Yao Qi wrote:
> My patch series will affect the code starting inferior in GDBserver
> (callees of start_inferior), so we need tests to cover how
> start_inferior is used in different cases.
> 
> In server.c:process_serial_event, start_inferior is used when
> GBDserver receives 'R' packet, and this patch is to add a test
> for this path, and see how --wrapper option works when the process
> is restarted.
> 

LGTM.

Thanks,
Pedro Alves

[PATCH 1/8] Disallow using --attach and --wrapper together.

[Yao Qi]

gdb/gdbserver:

2015-07-15  Yao Qi  <yao.qi@linaro.org>

	* server.c (captured_main): Call gdbserver_usage and exit if
	attach is true and wrapper_argv isn't NULL.
---
 gdb/gdbserver/server.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 7e388dd..ce3ffb5 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -3481,6 +3481,15 @@ captured_main (int argc, char *argv[])
       exit (1);
     }
 
+  if (attach && wrapper_argv != NULL)
+    {
+      /* Option --wrapper is used to start a new program, while option
+	 --attach is used to attach to an existing process.  Emit error
+	 and quit when they are used together.  */
+      gdbserver_usage (stderr);
+      exit (1);
+    }
+
   initialize_async_io ();
   initialize_low ();
   initialize_event_loop ();
-- 
1.9.1

Re: [PATCH 1/8] Disallow using --attach and --wrapper together.

[Pedro Alves]

On 07/20/2015 12:35 PM, Yao Qi wrote:
> gdb/gdbserver:
> 
> 2015-07-15  Yao Qi  <yao.qi@linaro.org>
> 
> 	* server.c (captured_main): Call gdbserver_usage and exit if
> 	attach is true and wrapper_argv isn't NULL.

Really not sure about this.  It's reasonable to do e.g.,
alias gs="gdbserver --wrapper=/whatever/wrapper --"
(or the equivalent wrapper shell script that execs gdbserver)
and then always start that instead of gdbserver:

sometimes:

 $ gs :9999 PROGRAM

othertimes:

 $ gs --attach :9999 $pid

but after the patch, the latter errors out.

Plus, one can combine --attach and --multi, which means
that the wrapper would still apply to processes spawned
after connecting.

Thanks,
Pedro Alves

Re: [PATCH 1/8] Disallow using --attach and --wrapper together.

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Really not sure about this.  It's reasonable to do e.g.,
> alias gs="gdbserver --wrapper=/whatever/wrapper --"
> (or the equivalent wrapper shell script that execs gdbserver)
> and then always start that instead of gdbserver:
>
> sometimes:
>
>  $ gs :9999 PROGRAM
>
> othertimes:
>
>  $ gs --attach :9999 $pid
>
> but after the patch, the latter errors out.

IMO, it makes sense to error out in the latter case, because --wrapper
is useless if GDBserver attaches to a process.

>
> Plus, one can combine --attach and --multi, which means
> that the wrapper would still apply to processes spawned
> after connecting.

I agree on this case.  I withdraw this patch.

-- 
Yao (齐尧)

Re: [PATCH 1/8] Disallow using --attach and --wrapper together.

[Pedro Alves]

On 07/24/2015 09:44 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Really not sure about this.  It's reasonable to do e.g.,
>> alias gs="gdbserver --wrapper=/whatever/wrapper --"
>> (or the equivalent wrapper shell script that execs gdbserver)
>> and then always start that instead of gdbserver:
>>
>> sometimes:
>>
>>  $ gs :9999 PROGRAM
>>
>> othertimes:
>>
>>  $ gs --attach :9999 $pid
>>
>> but after the patch, the latter errors out.
> 
> IMO, it makes sense to error out in the latter case, because --wrapper
> is useless if GDBserver attaches to a process.

The point was that before you could just forget about --wrapper,
set it once for your target/environment, having it hidden in the
script|alias-that-wraps-gdbserver.  I see it the same as putting
this in ~/.gdbinit:

 set exec-wrapper /whatever/wrapper

and then doing "$ gdb -p PID", and having GDB complain
about "set exec-wrapper" and "-p" being incompatible.

> 
>>
>> Plus, one can combine --attach and --multi, which means
>> that the wrapper would still apply to processes spawned
>> after connecting.
> 
> I agree on this case.  I withdraw this patch.
> 


Thanks,
Pedro Alves

[PATCH 2/8] Test --wrapper in extended-remote

[Yao Qi]

We didn't test --wrapper option in extended-remote before, this patch
is to add a test case for it.  In order to pass option --wrapper to
gdbserver in extended-remote, I add arg in gdbserver_start_extended,
and its default value is "", so that other places use
gdbserver_start_extended don't have to be updated.

gdb/testsuite:

2015-07-17  Yao Qi  <yao.qi@linaro.org>

	* lib/gdbserver-support.exp (gdbserver_start_extended): Add
	argument options.
	* gdb.server/ext-wrapper.exp: New file.
---
 gdb/testsuite/gdb.server/ext-wrapper.exp | 52 ++++++++++++++++++++++++++++++++
 gdb/testsuite/lib/gdbserver-support.exp  | 16 +++++++---
 2 files changed, 63 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/ext-wrapper.exp

diff --git a/gdb/testsuite/gdb.server/ext-wrapper.exp b/gdb/testsuite/gdb.server/ext-wrapper.exp
new file mode 100644
index 0000000..2ddd3a8
--- /dev/null
+++ b/gdb/testsuite/gdb.server/ext-wrapper.exp
@@ -0,0 +1,52 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test running programs using extended-remote.
+
+load_lib gdbserver-support.exp
+
+standard_testfile wrapper.c
+
+if { [skip_gdbserver_tests] } {
+    return 0
+}
+
+if {[build_executable $testfile.exp $testfile $srcfile debug] == -1} {
+    return -1
+}
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+    return -1
+}
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+set target_exec [gdbserver_download_current_prog]
+gdbserver_start_extended "--wrapper env TEST=1 --"
+
+gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
+
+gdb_breakpoint marker
+gdb_test "run" "Breakpoint.* marker .*" "run to marker"
+
+gdb_test "print d" "\\$${decimal} = ${hex} \"1\".*"
+
+gdb_test "kill" "" "kill" "Kill the program being debugged.*" "y"
+
+gdb_test_no_output "monitor exit"
diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index 6c3401c..e6d8f2c 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -434,15 +434,21 @@ proc gdbserver_reconnect { } {
     return [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
 }
 
-# Start and connect to a gdbserver in extended mode.  Note this frobs
-# $gdbserver_protocol, so should be used only from a board that
-# usually connects in target remote mode.
-proc gdbserver_start_extended { } {
+# Start gdbserver in extended mode with OPTIONS and connect to it.  Note
+# this frobs $gdbserver_protocol, so should be used only from a board
+# that usually connects in target remote mode.
+proc gdbserver_start_extended { {options ""} } {
     global gdbserver_protocol
     global gdbserver_gdbport
     global use_gdb_stub
 
-    if { [catch { gdbserver_start "--multi" "" } res] == 1 } {
+    set gdbserver_options "--multi"
+
+    if { $options != "" } {
+	append gdbserver_options " $options"
+    }
+
+    if { [catch { gdbserver_start $gdbserver_options "" } res] == 1 } {
 	perror $res
 	return 2
     }
-- 
1.9.1

Re: [PATCH 2/8] Test --wrapper in extended-remote

[Pedro Alves]

On 07/20/2015 12:35 PM, Yao Qi wrote:
> We didn't test --wrapper option in extended-remote before, this patch
> is to add a test case for it.  In order to pass option --wrapper to
> gdbserver in extended-remote, I add arg in gdbserver_start_extended,
> and its default value is "", so that other places use
> gdbserver_start_extended don't have to be updated.
> 

LGTM.

Thanks,
Pedro Alves

[PATCH 6/8] Set proc->priv->new_inferior out of linux_add_process

[Yao Qi]

Nowadays, we set proc->priv->new_inferior to 1 inside linux_add_process,
and new_inferior is used as a flag to initialise target description later.
linux_add_process is used for the three cases, fork/vfork event
(handle_extended_wait), run the program (linux_create_inferior), and
attach to the process (linux_attach).  In the first case, the child's
target description is copied from parent's, so we don't need to initialise
target description again later, which means we don't need to set
proc->priv->new_inferior to 1 in this case.  For the rest of two cases,
we need this flag.

This patch move the code setting proc->priv->new_inferior to 1 inside
linux_add_process to linux_create_inferior and linux_attach.  No
functionality is changed.

gdb/gdbserver:

2015-07-16  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_add_process): Don't set
	proc->priv->new_inferior.
	(linux_create_inferior): Set proc->priv->new_inferior to 1.
	(linux_attach): Likewise.
---
 gdb/gdbserver/linux-low.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 2dafb03..fa9dc29 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -405,9 +405,6 @@ linux_add_process (int pid, int attached)
   proc = add_process (pid, attached);
   proc->priv = xcalloc (1, sizeof (*proc->priv));
 
-  /* Set the arch when the first LWP stops.  */
-  proc->priv->new_inferior = 1;
-
   if (the_low_target.new_process != NULL)
     proc->priv->arch_private = the_low_target.new_process ();
 
@@ -766,6 +763,7 @@ linux_create_inferior (char *program, char **allargs)
   ptid_t ptid;
   struct cleanup *restore_personality
     = maybe_disable_address_space_randomization (disable_randomization);
+  struct process_info *proc;
 
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)
   pid = vfork ();
@@ -813,7 +811,9 @@ linux_create_inferior (char *program, char **allargs)
 
   do_cleanups (restore_personality);
 
-  linux_add_process (pid, 0);
+  proc = linux_add_process (pid, 0);
+  /* Set the arch when the first LWP stops.  */
+  proc->priv->new_inferior = 1;
 
   ptid = ptid_build (pid, pid, 0);
   new_lwp = add_lwp (ptid);
@@ -959,6 +959,7 @@ linux_attach (unsigned long pid)
 {
   ptid_t ptid = ptid_build (pid, pid, 0);
   int err;
+  struct process_info *proc;
 
   /* Attach to PID.  We will check for other threads
      soon.  */
@@ -967,7 +968,9 @@ linux_attach (unsigned long pid)
     error ("Cannot attach to process %ld: %s",
 	   pid, linux_ptrace_attach_fail_reason_string (ptid, err));
 
-  linux_add_process (pid, 1);
+  proc = linux_add_process (pid, 1);
+  /* Set the arch when the first LWP stops.  */
+  proc->priv->new_inferior = 1;
 
   if (!non_stop)
     {
-- 
1.9.1

Re: [PATCH 6/8] Set proc->priv->new_inferior out of linux_add_process

[Pedro Alves]

On 07/20/2015 12:35 PM, Yao Qi wrote:
> Nowadays, we set proc->priv->new_inferior to 1 inside linux_add_process,
> and new_inferior is used as a flag to initialise target description later.
> linux_add_process is used for the three cases, fork/vfork event
> (handle_extended_wait), run the program (linux_create_inferior), and
> attach to the process (linux_attach).  In the first case, the child's
> target description is copied from parent's, so we don't need to initialise
> target description again later, which means we don't need to set
> proc->priv->new_inferior to 1 in this case.  For the rest of two cases,
> we need this flag.
> 
> This patch move the code setting proc->priv->new_inferior to 1 inside
> linux_add_process to linux_create_inferior and linux_attach.  No
> functionality is changed.

LGTM.

Thanks,
Pedro Alves

[PATCH 5/8] Refactor start_inferior

[Yao Qi]

This patch is to refactor function start_inferior that signal_pid
is returned in one place.

gdb/gdbserver:

2015-07-16  Yao Qi  <yao.qi@linaro.org>

	* server.c (start_inferior): Code refactor.
---
 gdb/gdbserver/server.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 53befc7..497c5fe 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -257,22 +257,21 @@ start_inferior (char **argv)
 
       last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
 
-      if (last_status.kind != TARGET_WAITKIND_STOPPED)
-	return signal_pid;
-
-      do
+      if (last_status.kind == TARGET_WAITKIND_STOPPED)
 	{
-	  (*the_target->resume) (&resume_info, 1);
+	  do
+	    {
+	      (*the_target->resume) (&resume_info, 1);
 
- 	  last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
-	  if (last_status.kind != TARGET_WAITKIND_STOPPED)
-	    return signal_pid;
+	      last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
+	      if (last_status.kind != TARGET_WAITKIND_STOPPED)
+		break;
 
-	  current_thread->last_resume_kind = resume_stop;
-	  current_thread->last_status = last_status;
+	      current_thread->last_resume_kind = resume_stop;
+	      current_thread->last_status = last_status;
+	    }
+	  while (last_status.value.sig != GDB_SIGNAL_TRAP);
 	}
-      while (last_status.value.sig != GDB_SIGNAL_TRAP);
-
       return signal_pid;
     }
 
-- 
1.9.1

Re: [PATCH 5/8] Refactor start_inferior

[Pedro Alves]

On 07/20/2015 12:35 PM, Yao Qi wrote:
> This patch is to refactor function start_inferior that signal_pid
> is returned in one place.
> 

LGTM.

> gdb/gdbserver:
> 
> 2015-07-16  Yao Qi  <yao.qi@linaro.org>
> 
> 	* server.c (start_inferior): Code refactor.

Thanks,
Pedro Alves

[PATCH 8/8] Remove proc->priv->new_inferior

[Yao Qi]

As the result of the previous patch, new_inferior is no longer used.
This patch is to remove it.

gdb/gdbserver:

2015-07-16  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_create_inferior): Remove setting to
	proc->priv->new_inferior.
	(linux_attach): Likewise.
	(linux_low_filter_event): Likewise.
	* linux-low.h (struct process_info_private) <new_inferior>: Remove.
---
 gdb/gdbserver/linux-low.c | 12 ++----------
 gdb/gdbserver/linux-low.h |  5 -----
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 10942c8..3275576 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -763,7 +763,6 @@ linux_create_inferior (char *program, char **allargs)
   ptid_t ptid;
   struct cleanup *restore_personality
     = maybe_disable_address_space_randomization (disable_randomization);
-  struct process_info *proc;
 
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)
   pid = vfork ();
@@ -811,9 +810,7 @@ linux_create_inferior (char *program, char **allargs)
 
   do_cleanups (restore_personality);
 
-  proc = linux_add_process (pid, 0);
-  /* Set the arch when the first LWP stops.  */
-  proc->priv->new_inferior = 1;
+  linux_add_process (pid, 0);
 
   ptid = ptid_build (pid, pid, 0);
   new_lwp = add_lwp (ptid);
@@ -967,7 +964,6 @@ linux_attach (unsigned long pid)
 {
   ptid_t ptid = ptid_build (pid, pid, 0);
   int err;
-  struct process_info *proc;
 
   /* Attach to PID.  We will check for other threads
      soon.  */
@@ -976,9 +972,7 @@ linux_attach (unsigned long pid)
     error ("Cannot attach to process %ld: %s",
 	   pid, linux_ptrace_attach_fail_reason_string (ptid, err));
 
-  proc = linux_add_process (pid, 1);
-  /* Set the arch when the first LWP stops.  */
-  proc->priv->new_inferior = 1;
+  linux_add_process (pid, 1);
 
   if (!non_stop)
     {
@@ -2130,8 +2124,6 @@ linux_low_filter_event (int lwpid, int wstat)
 	      the_low_target.arch_setup ();
 
 	      current_thread = saved_thread;
-
-	      proc->priv->new_inferior = 0;
 	    }
 	  else
 	    {
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 3300da9..5a3697b 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -115,11 +115,6 @@ struct process_info_private
 
   /* &_r_debug.  0 if not yet determined.  -1 if no PT_DYNAMIC in Phdrs.  */
   CORE_ADDR r_debug;
-
-  /* This flag is true iff we've just created or attached to the first
-     LWP of this process but it has not stopped yet.  As soon as it
-     does, we need to call the low target's arch_setup callback.  */
-  int new_inferior;
 };
 
 struct lwp_info;
-- 
1.9.1

Re: [PATCH 8/8] Remove proc->priv->new_inferior

[Pedro Alves]

On 07/20/2015 12:35 PM, Yao Qi wrote:
> As the result of the previous patch, new_inferior is no longer used.
> This patch is to remove it.
> 
> gdb/gdbserver:
> 
> 2015-07-16  Yao Qi  <yao.qi@linaro.org>
> 
> 	* linux-low.c (linux_create_inferior): Remove setting to
> 	proc->priv->new_inferior.
> 	(linux_attach): Likewise.
> 	(linux_low_filter_event): Likewise.
> 	* linux-low.h (struct process_info_private) <new_inferior>: Remove.

LGTM.

Thanks,
Pedro Alves

[PATCH 7/8] Initialise target descrption after skipping extra traps for --wrapper

[Yao Qi]

Nowadays, when --wrapper is used, GDBserver skips extra traps/stops
in the wrapper program, and stops at the first instruction of the
program to be debugged.  However, GDBserver created target description
in the first stop of inferior, and the executable of the inferior
is the wrapper program rather than the program to be debugged.  In
this way, the target description can be wrong if the architectures
of wrapper program and program to be debugged are different.  This
is shown by some fails in gdb.server/wrapper.exp on buildbot.

We are testing i686-linux GDB (Fedora-i686) on an x86_64-linux box
(fedora-x86-64-4) in buildbot, such configuration causes fails in
gdb.server/wrapper.exp like this:

spawn /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/../../gdb/gdbserver/gdbserver --once --wrapper env TEST=1 -- :2346 /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/outputs/gdb.server/wrapper/wrapper
Process /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/outputs/gdb.server/wrapper/wrapper created; pid = 8795
Can't debug 64-bit process with 32-bit GDBserver
Exiting
target remote localhost:2346
localhost:2346: Connection timed out.
(gdb) FAIL: gdb.server/wrapper.exp: setting breakpoint at marker

See https://sourceware.org/ml/gdb-testers/2015-q3/msg01541.html

In this case, program to be debugged ("wrapper") is 32-bit but wrapper
program ("/usr/bin/env") is 64-bit, so GDBserver gets the 64-bit
target description instead of 32-bit.

The root cause of this problem is that GDBserver creates target
description too early, and the rationale of fix could be creating
target description once the GDBserver skips extra traps and inferior
stops at the first instruction of the program we want to debug.  IOW,
when GDBserver skips extra traps, the inferior's tdesc is NULL, and
mywait and its callees shouldn't use inferior's tdesc, so in this
patch, we do the shortcut if proc->tdesc == NULL, see changes in
linux_resume_one_lwp_throw and need_step_over_p.

In linux_low_filter_event, if target description isn't initialised and
GDBserver attached the process, we create target description immediately,
because GDBserver don't have to skip extra traps for attach, IOW, it
makes no sense to use --attach and --wrapper together.  Otherwise, the
process is launched by GDBserver, we keep the status pending, and return.

After GDBserver skipped extra traps in start_inferior, we call a
target_ops hook arch_setup to initialise target description there.

gdb/gdbserver:

2015-07-16  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_arch_setup): New function.
	(linux_low_filter_event): If proc->tdesc is NULL and
	proc->attached is true, call the_low_target.arch_setup.
	Otherwise, keep status pending, and return.
	(linux_resume_one_lwp_throw): if proc->tdesc is NULL
	resume the inferior.
	(need_step_over_p): Return 0 if proc->tdesc is NULL.
	(linux_target_ops): Install arch_setup.
	* server.c (start_inferior): Call the_target->arch_setup.
	* target.h (struct target_ops) <arch_setup>: New field.
	(target_arch_setup): New marco.
	* lynx-low.c (lynx_target_ops): Update.
	* nto-low.c (nto_target_ops): Update.
	* spu-low.c (spu_target_ops): Update.
	* win32-low.c (win32_target_ops): Update.
---
 gdb/gdbserver/linux-low.c | 74 ++++++++++++++++++++++++++++++++++++++++-------
 gdb/gdbserver/lynx-low.c  |  1 +
 gdb/gdbserver/nto-low.c   |  1 +
 gdb/gdbserver/server.c    |  3 ++
 gdb/gdbserver/spu-low.c   |  1 +
 gdb/gdbserver/target.h    | 10 +++++++
 gdb/gdbserver/win32-low.c |  1 +
 7 files changed, 80 insertions(+), 11 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index fa9dc29..10942c8 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -822,6 +822,14 @@ linux_create_inferior (char *program, char **allargs)
   return pid;
 }
 
+/* Implement the arch_setup target_ops method.  */
+
+static void
+linux_arch_setup (void)
+{
+  the_low_target.arch_setup ();
+}
+
 /* Attach to an inferior process.  Returns 0 on success, ERRNO on
    error.  */
 
@@ -2105,23 +2113,35 @@ linux_low_filter_event (int lwpid, int wstat)
     {
       struct process_info *proc;
 
-      /* Architecture-specific setup after inferior is running.  This
-	 needs to happen after we have attached to the inferior and it
-	 is stopped for the first time, but before we access any
-	 inferior registers.  */
+      /* Architecture-specific setup after inferior is running.  */
       proc = find_process_pid (pid_of (thread));
-      if (proc->priv->new_inferior)
+      if (proc->tdesc == NULL)
 	{
-	  struct thread_info *saved_thread;
+	  if (proc->attached)
+	    {
+	      struct thread_info *saved_thread;
 
-	  saved_thread = current_thread;
-	  current_thread = thread;
+	      /* This needs to happen after we have attached to the
+		 inferior and it is stopped for the first time, but
+		 before we access any inferior registers.  */
+	      saved_thread = current_thread;
+	      current_thread = thread;
 
-	  the_low_target.arch_setup ();
+	      the_low_target.arch_setup ();
 
-	  current_thread = saved_thread;
+	      current_thread = saved_thread;
 
-	  proc->priv->new_inferior = 0;
+	      proc->priv->new_inferior = 0;
+	    }
+	  else
+	    {
+	      /* The process is started, but GDBserver will do
+		 architecture-specific setup after the program stops at
+		 the first instruction.  */
+	      child->status_pending_p = 1;
+	      child->status_pending = wstat;
+	      return child;
+	    }
 	}
     }
 
@@ -3651,6 +3671,31 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
   struct thread_info *thread = get_lwp_thread (lwp);
   struct thread_info *saved_thread;
   int fast_tp_collecting;
+  struct process_info *proc = get_thread_process (thread);
+
+  if (proc->tdesc == NULL)
+    {
+      /* Target description isn't initialised because the program hasn't
+	 stop at the first instruction.  It means GDBserver skips the
+	 extra traps from the wrapper program (see option --wrapper),
+	 and GDBserver just simply resumes the program without accessing
+	 inferior registers, because target description is unavailable
+	 at this point.  */
+      errno = 0;
+      lwp->stepping = step;
+      ptrace (step ? PTRACE_SINGLESTEP : PTRACE_CONT, lwpid_of (thread),
+	      (PTRACE_TYPE_ARG3) 0,
+	      /* Coerce to a uintptr_t first to avoid potential gcc warning
+		 of coercing an 8 byte integer to a 4 byte pointer.  */
+	      (PTRACE_TYPE_ARG4) (uintptr_t) signal);
+
+      if (errno)
+	perror_with_name ("resuming thread");
+
+      lwp->stopped = 0;
+      lwp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
+      return;
+    }
 
   if (lwp->stopped == 0)
     return;
@@ -4014,6 +4059,12 @@ need_step_over_p (struct inferior_list_entry *entry, void *dummy)
   struct lwp_info *lwp = get_thread_lwp (thread);
   struct thread_info *saved_thread;
   CORE_ADDR pc;
+  struct process_info *proc = get_thread_process (thread);
+
+  /* GDBserver is skipping the extra traps from the wrapper program,
+     don't have to do step over.  */
+  if (proc->tdesc == NULL)
+    return 0;
 
   /* LWPs which will not be resumed are not interesting, because we
      might not wait for them next time through linux_wait.  */
@@ -6635,6 +6686,7 @@ current_lwp_ptid (void)
 
 static struct target_ops linux_target_ops = {
   linux_create_inferior,
+  linux_arch_setup,
   linux_attach,
   linux_kill,
   linux_detach,
diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index ee7b28a..5cf03be 100644
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -722,6 +722,7 @@ lynx_request_interrupt (void)
 
 static struct target_ops lynx_target_ops = {
   lynx_create_inferior,
+  NULL,  /* arch_setup */
   lynx_attach,
   lynx_kill,
   lynx_detach,
diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
index 9276736..19f492f 100644
--- a/gdb/gdbserver/nto-low.c
+++ b/gdb/gdbserver/nto-low.c
@@ -925,6 +925,7 @@ nto_supports_non_stop (void)
 
 static struct target_ops nto_target_ops = {
   nto_create_inferior,
+  NULL,  /* arch_setup */
   nto_attach,
   nto_kill,
   nto_detach,
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 497c5fe..1af7389 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -272,6 +272,7 @@ start_inferior (char **argv)
 	    }
 	  while (last_status.value.sig != GDB_SIGNAL_TRAP);
 	}
+      target_arch_setup ();
       return signal_pid;
     }
 
@@ -279,6 +280,8 @@ start_inferior (char **argv)
      (assuming success).  */
   last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
 
+  target_arch_setup ();
+
   if (last_status.kind != TARGET_WAITKIND_EXITED
       && last_status.kind != TARGET_WAITKIND_SIGNALLED)
     {
diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
index a56a889..cbee960 100644
--- a/gdb/gdbserver/spu-low.c
+++ b/gdb/gdbserver/spu-low.c
@@ -638,6 +638,7 @@ spu_request_interrupt (void)
 
 static struct target_ops spu_target_ops = {
   spu_create_inferior,
+  NULL,  /* arch_setup */
   spu_attach,
   spu_kill,
   spu_detach,
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 9a40867..fefd8d1 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -74,6 +74,9 @@ struct target_ops
 
   int (*create_inferior) (char *program, char **args);
 
+  /* Architecture-specific setup.  */
+  void (*arch_setup) (void);
+
   /* Attach to a running process.
 
      PID is the process ID to attach to, specified by the user
@@ -445,6 +448,13 @@ void set_target_ops (struct target_ops *);
 #define create_inferior(program, args) \
   (*the_target->create_inferior) (program, args)
 
+#define target_arch_setup()			 \
+  do						 \
+    {						 \
+      if (the_target->arch_setup != NULL)	 \
+	(*the_target->arch_setup) ();		 \
+    } while (0)
+
 #define myattach(pid) \
   (*the_target->attach) (pid)
 
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 64caf24..7ccb3dd 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -1785,6 +1785,7 @@ win32_get_tib_address (ptid_t ptid, CORE_ADDR *addr)
 
 static struct target_ops win32_target_ops = {
   win32_create_inferior,
+  NULL,  /* arch_setup */
   win32_attach,
   win32_kill,
   win32_detach,
-- 
1.9.1

Re: [PATCH 7/8] Initialise target descrption after skipping extra traps for --wrapper

[Pedro Alves]

On 07/20/2015 12:35 PM, Yao Qi wrote:
> Nowadays, when --wrapper is used, GDBserver skips extra traps/stops
> in the wrapper program, and stops at the first instruction of the
> program to be debugged.  However, GDBserver created target description
> in the first stop of inferior, and the executable of the inferior
> is the wrapper program rather than the program to be debugged.  In
> this way, the target description can be wrong if the architectures
> of wrapper program and program to be debugged are different.  This
> is shown by some fails in gdb.server/wrapper.exp on buildbot.
> 
> We are testing i686-linux GDB (Fedora-i686) on an x86_64-linux box
> (fedora-x86-64-4) in buildbot, such configuration causes fails in
> gdb.server/wrapper.exp like this:
> 
> spawn /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/../../gdb/gdbserver/gdbserver --once --wrapper env TEST=1 -- :2346 /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/outputs/gdb.server/wrapper/wrapper
> Process /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/outputs/gdb.server/wrapper/wrapper created; pid = 8795
> Can't debug 64-bit process with 32-bit GDBserver
> Exiting
> target remote localhost:2346
> localhost:2346: Connection timed out.
> (gdb) FAIL: gdb.server/wrapper.exp: setting breakpoint at marker
> 
> See https://sourceware.org/ml/gdb-testers/2015-q3/msg01541.html
> 
> In this case, program to be debugged ("wrapper") is 32-bit but wrapper
> program ("/usr/bin/env") is 64-bit, so GDBserver gets the 64-bit
> target description instead of 32-bit.
> 
> The root cause of this problem is that GDBserver creates target
> description too early, and the rationale of fix could be creating
> target description once the GDBserver skips extra traps and inferior
> stops at the first instruction of the program we want to debug.  IOW,
> when GDBserver skips extra traps, the inferior's tdesc is NULL, and
> mywait and its callees shouldn't use inferior's tdesc, so in this
> patch, we do the shortcut if proc->tdesc == NULL, see changes in
> linux_resume_one_lwp_throw and need_step_over_p.
> 
> In linux_low_filter_event, if target description isn't initialised and
> GDBserver attached the process, we create target description immediately,
> because GDBserver don't have to skip extra traps for attach, IOW, it
> makes no sense to use --attach and --wrapper together.  Otherwise, the
> process is launched by GDBserver, we keep the status pending, and return.
> 
> After GDBserver skipped extra traps in start_inferior, we call a
> target_ops hook arch_setup to initialise target description there.
> 

Great, thanks for doing this.  Looks generally good to me.  One
comment below.

> @@ -3651,6 +3671,31 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>    struct thread_info *thread = get_lwp_thread (lwp);
>    struct thread_info *saved_thread;
>    int fast_tp_collecting;
> +  struct process_info *proc = get_thread_process (thread);
> +
> +  if (proc->tdesc == NULL)
> +    {
> +      /* Target description isn't initialised because the program hasn't
> +	 stop at the first instruction.  It means GDBserver skips the

"hasn't stopped" ... "first instruction yet".

> +	 extra traps from the wrapper program (see option --wrapper),
> +	 and GDBserver just simply resumes the program without accessing
> +	 inferior registers, because target description is unavailable
> +	 at this point.  */
> +      errno = 0;
> +      lwp->stepping = step;
> +      ptrace (step ? PTRACE_SINGLESTEP : PTRACE_CONT, lwpid_of (thread),
> +	      (PTRACE_TYPE_ARG3) 0,
> +	      /* Coerce to a uintptr_t first to avoid potential gcc warning
> +		 of coercing an 8 byte integer to a 4 byte pointer.  */
> +	      (PTRACE_TYPE_ARG4) (uintptr_t) signal);
> +
> +      if (errno)
> +	perror_with_name ("resuming thread");
> +
> +      lwp->stopped = 0;
> +      lwp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
> +      return;

Instead of this whole code block, I think we should be able to skip
the bits in the function that require accessing registers.  E.g.,
this:

 /* Cancel actions that rely on GDB not changing the PC (e.g., the
     user used the "jump" command, or "set $pc = foo").  */
  if (lwp->stop_pc != get_pc (lwp))
    {
      /* Collecting 'while-stepping' actions doesn't make sense
	 anymore.  */
      release_while_stepping_state_list (thread);
    }

Could be guarded by:

  if (thread->while_stepping != NULL)

And this:

  if (the_low_target.get_pc != NULL)
    {
      struct regcache *regcache = get_thread_regcache (current_thread, 1);

      lwp->stop_pc = (*the_low_target.get_pc) (regcache);

      if (debug_threads)
	{
	  debug_printf ("  %s from pc 0x%lx\n", step ? "step" : "continue",
			(long) lwp->stop_pc);
	}
    }

could be guarded by:

  if (proc->tdesc == NULL)

Did you try that?

> +    }
>  
>    if (lwp->stopped == 0)
>      return;

Thanks,
Pedro Alves

Re: [PATCH 7/8] Initialise target descrption after skipping extra traps for --wrapper

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Instead of this whole code block, I think we should be able to skip
> the bits in the function that require accessing registers.  E.g.,
> this:
>
>  /* Cancel actions that rely on GDB not changing the PC (e.g., the
>      user used the "jump" command, or "set $pc = foo").  */
>   if (lwp->stop_pc != get_pc (lwp))
>     {
>       /* Collecting 'while-stepping' actions doesn't make sense
> 	 anymore.  */
>       release_while_stepping_state_list (thread);
>     }
>
> Could be guarded by:
>
>   if (thread->while_stepping != NULL)
>
> And this:
>
>   if (the_low_target.get_pc != NULL)
>     {
>       struct regcache *regcache = get_thread_regcache (current_thread, 1);
>
>       lwp->stop_pc = (*the_low_target.get_pc) (regcache);
>
>       if (debug_threads)
> 	{
> 	  debug_printf ("  %s from pc 0x%lx\n", step ? "step" : "continue",
> 			(long) lwp->stop_pc);
> 	}
>     }
>
> could be guarded by:
>
>   if (proc->tdesc == NULL)
>
> Did you try that?

To make sure I understand you correctly, is the change below what you suggested?
If yes, I thought about this approach before, but I didn't try that
because I was worried that we should check every piece of code in
linux_resume_one_lwp_throw and its callees that don't access registers
when target description isn't initialised yet.  Especially for
the_low_target.prepare_to_resume, the implementation of this hook should
be aware of that target description may not be available.  Nowadays,
prepare_to_resume is only used to update HW debug registers, and
should be OK because GDBserver shouldn't update HW debug registers
before the inferior stops at the first instruction of the program.
However, in nat/x86-linux.c:lwp_debug_registers_changed, I saw such
comments

  struct arch_lwp_info *info = lwp_arch_private_info (lwp);

  /* NULL means either that this is the main thread still going
     through the shell, or that no watchpoint has been set yet.
     The debug registers are unchanged in either case.  */

I was wondering all the implementations of prepare_to_resume of
different targets should be aware that "thread still going through the
shell"?  I'll test this patch on targets other than x86 (such as arm and
aarch64) and see if it causes fails.

-- 
Yao (齐尧)

@@ -3651,6 +3671,14 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
   struct thread_info *thread = get_lwp_thread (lwp);
   struct thread_info *saved_thread;
   int fast_tp_collecting;
+  struct process_info *proc = get_thread_process (thread);
+
+  /* Note that target description may not be initialised
+     (proc->tdesc == NULL) at this point because the program hasn't
+     stopped at the first instruction yet.  It means GDBserver skips
+     the extra traps from the wrapper program (see option --wrapper).
+     Code in this function that requires register access should be
+     guarded by proc->tdesc == NULL or something else.  */
 
   if (lwp->stopped == 0)
     return;
@@ -3661,7 +3689,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 
   /* Cancel actions that rely on GDB not changing the PC (e.g., the
      user used the "jump" command, or "set $pc = foo").  */
-  if (lwp->stop_pc != get_pc (lwp))
+  if (thread->while_stepping != NULL && lwp->stop_pc != get_pc (lwp))
     {
       /* Collecting 'while-stepping' actions doesn't make sense
         anymore.  */
@@ -3787,7 +3815,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
       step = 1;
     }
 
-  if (the_low_target.get_pc != NULL)
+  if (proc->tdesc != NULL && the_low_target.get_pc != NULL)
     {
       struct regcache *regcache = get_thread_regcache (current_thread, 1);
 

Re: [PATCH 7/8] Initialise target descrption after skipping extra traps for --wrapper

[Pedro Alves]

On 07/24/2015 12:11 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Instead of this whole code block, I think we should be able to skip
>> the bits in the function that require accessing registers.  E.g.,
>> this:
>>
>>  /* Cancel actions that rely on GDB not changing the PC (e.g., the
>>      user used the "jump" command, or "set $pc = foo").  */
>>   if (lwp->stop_pc != get_pc (lwp))
>>     {
>>       /* Collecting 'while-stepping' actions doesn't make sense
>> 	 anymore.  */
>>       release_while_stepping_state_list (thread);
>>     }
>>
>> Could be guarded by:
>>
>>   if (thread->while_stepping != NULL)
>>
>> And this:
>>
>>   if (the_low_target.get_pc != NULL)
>>     {
>>       struct regcache *regcache = get_thread_regcache (current_thread, 1);
>>
>>       lwp->stop_pc = (*the_low_target.get_pc) (regcache);
>>
>>       if (debug_threads)
>> 	{
>> 	  debug_printf ("  %s from pc 0x%lx\n", step ? "step" : "continue",
>> 			(long) lwp->stop_pc);
>> 	}
>>     }
>>
>> could be guarded by:
>>
>>   if (proc->tdesc == NULL)
>>
>> Did you try that?
> 
> To make sure I understand you correctly, is the change below what you suggested?

Yes.

> If yes, I thought about this approach before, but I didn't try that
> because I was worried that we should check every piece of code in
> linux_resume_one_lwp_throw and its callees that don't access registers
> when target description isn't initialised yet.  Especially for
> the_low_target.prepare_to_resume, the implementation of this hook should
> be aware of that target description may not be available.  Nowadays,
> prepare_to_resume is only used to update HW debug registers, and
> should be OK because GDBserver shouldn't update HW debug registers
> before the inferior stops at the first instruction of the program.
> However, in nat/x86-linux.c:lwp_debug_registers_changed, I saw such
> comments
> 
>   struct arch_lwp_info *info = lwp_arch_private_info (lwp);
> 
>   /* NULL means either that this is the main thread still going
>      through the shell, or that no watchpoint has been set yet.
>      The debug registers are unchanged in either case.  */
> 
> I was wondering all the implementations of prepare_to_resume of
> different targets should be aware that "thread still going through the
> shell"?  

Yes, I think they should.  It's what the GDB side used to do already
when that code was x86 gdb only (and hence that shell comment, which
gdbserver doesn't use yet), and then other archs followed suit.
"Going through the shell" is the exact same as going through
the exec wrapper -- we're not interested in debugging the shell,
and it may well have a different bitness/architecture of the target
program we want to debug.

In practice that hook's implementation should want to avoid work if some
flag is not set, to avoid unnecessary ptrace syscalls and thus ends up
not doing anything when going through the shell/exec-wrapper.

> I'll test this patch on targets other than x86 (such as arm and
> aarch64) and see if it causes fails.

Thanks,
Pedro Alves

Re: [PATCH 7/8] Initialise target descrption after skipping extra traps for --wrapper

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Yes, I think they should.  It's what the GDB side used to do already
> when that code was x86 gdb only (and hence that shell comment, which
> gdbserver doesn't use yet), and then other archs followed suit.
> "Going through the shell" is the exact same as going through
> the exec wrapper -- we're not interested in debugging the shell,
> and it may well have a different bitness/architecture of the target
> program we want to debug.
>
> In practice that hook's implementation should want to avoid work if some
> flag is not set, to avoid unnecessary ptrace syscalls and thus ends up
> not doing anything when going through the shell/exec-wrapper.

OK.

>
>> I'll test this patch on targets other than x86 (such as arm and
>> aarch64) and see if it causes fails.

I tested the patch on aarch64-linux, and no regressions are found.  I'll
push the following patch in.

-- 
Yao (齐尧)

From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 16 Jul 2015 16:35:46 +0100
Subject: [PATCH] Initialise target descrption after skipping extra traps for --wrapper

Nowadays, when --wrapper is used, GDBserver skips extra traps/stops
in the wrapper program, and stops at the first instruction of the
program to be debugged.  However, GDBserver created target description
in the first stop of inferior, and the executable of the inferior
is the wrapper program rather than the program to be debugged.  In
this way, the target description can be wrong if the architectures
of wrapper program and program to be debugged are different.  This
is shown by some fails in gdb.server/wrapper.exp on buildbot.

We are testing i686-linux GDB (Fedora-i686) on an x86_64-linux box
(fedora-x86-64-4) in buildbot, such configuration causes fails in
gdb.server/wrapper.exp like this:

spawn /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/../../gdb/gdbserver/gdbserver --once --wrapper env TEST=1 -- :2346 /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/outputs/gdb.server/wrapper/wrapper
Process /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/outputs/gdb.server/wrapper/wrapper created; pid = 8795
Can't debug 64-bit process with 32-bit GDBserver
Exiting
target remote localhost:2346
localhost:2346: Connection timed out.
(gdb) FAIL: gdb.server/wrapper.exp: setting breakpoint at marker

See https://sourceware.org/ml/gdb-testers/2015-q3/msg01541.html

In this case, program to be debugged ("wrapper") is 32-bit but wrapper
program ("/usr/bin/env") is 64-bit, so GDBserver gets the 64-bit
target description instead of 32-bit.

The root cause of this problem is that GDBserver creates target
description too early, and the rationale of fix could be creating
target description once the GDBserver skips extra traps and inferior
stops at the first instruction of the program we want to debug.  IOW,
when GDBserver skips extra traps, the inferior's tdesc is NULL, and
mywait and its callees shouldn't use inferior's tdesc, so in this
patch, we skip code that requires register access, see changes in
linux_resume_one_lwp_throw and need_step_over_p.

In linux_low_filter_event, if target description isn't initialised and
GDBserver attached the process, we create target description immediately,
because GDBserver don't have to skip extra traps for attach, IOW, it
makes no sense to use --attach and --wrapper together.  Otherwise, the
process is launched by GDBserver, we keep the status pending, and return.

After GDBserver skipped extra traps in start_inferior, we call a
target_ops hook arch_setup to initialise target description there.

gdb/gdbserver:

2015-07-24  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_arch_setup): New function.
	(linux_low_filter_event): If proc->tdesc is NULL and
	proc->attached is true, call the_low_target.arch_setup.
	Otherwise, keep status pending, and return.
	(linux_resume_one_lwp_throw): Don't call get_pc if
	thread->while_stepping isn't NULL.  Don't call
	get_thread_regcache if proc->tdesc is NULL.
	(need_step_over_p): Return 0 if proc->tdesc is NULL.
	(linux_target_ops): Install arch_setup.
	* server.c (start_inferior): Call the_target->arch_setup.
	* target.h (struct target_ops) <arch_setup>: New field.
	(target_arch_setup): New marco.
	* lynx-low.c (lynx_target_ops): Update.
	* nto-low.c (nto_target_ops): Update.
	* spu-low.c (spu_target_ops): Update.
	* win32-low.c (win32_target_ops): Update.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index fa9dc29..ac1ad6f 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -822,6 +822,14 @@ linux_create_inferior (char *program, char **allargs)
   return pid;
 }
 
+/* Implement the arch_setup target_ops method.  */
+
+static void
+linux_arch_setup (void)
+{
+  the_low_target.arch_setup ();
+}
+
 /* Attach to an inferior process.  Returns 0 on success, ERRNO on
    error.  */
 
@@ -2105,23 +2113,35 @@ linux_low_filter_event (int lwpid, int wstat)
     {
       struct process_info *proc;
 
-      /* Architecture-specific setup after inferior is running.  This
-	 needs to happen after we have attached to the inferior and it
-	 is stopped for the first time, but before we access any
-	 inferior registers.  */
+      /* Architecture-specific setup after inferior is running.  */
       proc = find_process_pid (pid_of (thread));
-      if (proc->priv->new_inferior)
+      if (proc->tdesc == NULL)
 	{
-	  struct thread_info *saved_thread;
+	  if (proc->attached)
+	    {
+	      struct thread_info *saved_thread;
 
-	  saved_thread = current_thread;
-	  current_thread = thread;
+	      /* This needs to happen after we have attached to the
+		 inferior and it is stopped for the first time, but
+		 before we access any inferior registers.  */
+	      saved_thread = current_thread;
+	      current_thread = thread;
 
-	  the_low_target.arch_setup ();
+	      the_low_target.arch_setup ();
 
-	  current_thread = saved_thread;
+	      current_thread = saved_thread;
 
-	  proc->priv->new_inferior = 0;
+	      proc->priv->new_inferior = 0;
+	    }
+	  else
+	    {
+	      /* The process is started, but GDBserver will do
+		 architecture-specific setup after the program stops at
+		 the first instruction.  */
+	      child->status_pending_p = 1;
+	      child->status_pending = wstat;
+	      return child;
+	    }
 	}
     }
 
@@ -3651,6 +3671,14 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
   struct thread_info *thread = get_lwp_thread (lwp);
   struct thread_info *saved_thread;
   int fast_tp_collecting;
+  struct process_info *proc = get_thread_process (thread);
+
+  /* Note that target description may not be initialised
+     (proc->tdesc == NULL) at this point because the program hasn't
+     stopped at the first instruction yet.  It means GDBserver skips
+     the extra traps from the wrapper program (see option --wrapper).
+     Code in this function that requires register access should be
+     guarded by proc->tdesc == NULL or something else.  */
 
   if (lwp->stopped == 0)
     return;
@@ -3661,7 +3689,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 
   /* Cancel actions that rely on GDB not changing the PC (e.g., the
      user used the "jump" command, or "set $pc = foo").  */
-  if (lwp->stop_pc != get_pc (lwp))
+  if (thread->while_stepping != NULL && lwp->stop_pc != get_pc (lwp))
     {
       /* Collecting 'while-stepping' actions doesn't make sense
 	 anymore.  */
@@ -3787,7 +3815,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
       step = 1;
     }
 
-  if (the_low_target.get_pc != NULL)
+  if (proc->tdesc != NULL && the_low_target.get_pc != NULL)
     {
       struct regcache *regcache = get_thread_regcache (current_thread, 1);
 
@@ -4014,6 +4042,12 @@ need_step_over_p (struct inferior_list_entry *entry, void *dummy)
   struct lwp_info *lwp = get_thread_lwp (thread);
   struct thread_info *saved_thread;
   CORE_ADDR pc;
+  struct process_info *proc = get_thread_process (thread);
+
+  /* GDBserver is skipping the extra traps from the wrapper program,
+     don't have to do step over.  */
+  if (proc->tdesc == NULL)
+    return 0;
 
   /* LWPs which will not be resumed are not interesting, because we
      might not wait for them next time through linux_wait.  */
@@ -6635,6 +6669,7 @@ current_lwp_ptid (void)
 
 static struct target_ops linux_target_ops = {
   linux_create_inferior,
+  linux_arch_setup,
   linux_attach,
   linux_kill,
   linux_detach,
diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index ee7b28a..5cf03be 100644
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -722,6 +722,7 @@ lynx_request_interrupt (void)
 
 static struct target_ops lynx_target_ops = {
   lynx_create_inferior,
+  NULL,  /* arch_setup */
   lynx_attach,
   lynx_kill,
   lynx_detach,
diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
index 9276736..19f492f 100644
--- a/gdb/gdbserver/nto-low.c
+++ b/gdb/gdbserver/nto-low.c
@@ -925,6 +925,7 @@ nto_supports_non_stop (void)
 
 static struct target_ops nto_target_ops = {
   nto_create_inferior,
+  NULL,  /* arch_setup */
   nto_attach,
   nto_kill,
   nto_detach,
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index df514f6..6b8617c 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -272,6 +272,7 @@ start_inferior (char **argv)
 	    }
 	  while (last_status.value.sig != GDB_SIGNAL_TRAP);
 	}
+      target_arch_setup ();
       return signal_pid;
     }
 
@@ -279,6 +280,8 @@ start_inferior (char **argv)
      (assuming success).  */
   last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
 
+  target_arch_setup ();
+
   if (last_status.kind != TARGET_WAITKIND_EXITED
       && last_status.kind != TARGET_WAITKIND_SIGNALLED)
     {
diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
index a56a889..cbee960 100644
--- a/gdb/gdbserver/spu-low.c
+++ b/gdb/gdbserver/spu-low.c
@@ -638,6 +638,7 @@ spu_request_interrupt (void)
 
 static struct target_ops spu_target_ops = {
   spu_create_inferior,
+  NULL,  /* arch_setup */
   spu_attach,
   spu_kill,
   spu_detach,
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 9a40867..fefd8d1 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -74,6 +74,9 @@ struct target_ops
 
   int (*create_inferior) (char *program, char **args);
 
+  /* Architecture-specific setup.  */
+  void (*arch_setup) (void);
+
   /* Attach to a running process.
 
      PID is the process ID to attach to, specified by the user
@@ -445,6 +448,13 @@ void set_target_ops (struct target_ops *);
 #define create_inferior(program, args) \
   (*the_target->create_inferior) (program, args)
 
+#define target_arch_setup()			 \
+  do						 \
+    {						 \
+      if (the_target->arch_setup != NULL)	 \
+	(*the_target->arch_setup) ();		 \
+    } while (0)
+
 #define myattach(pid) \
   (*the_target->attach) (pid)
 
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 64caf24..7ccb3dd 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -1785,6 +1785,7 @@ win32_get_tib_address (ptid_t ptid, CORE_ADDR *addr)
 
 static struct target_ops win32_target_ops = {
   win32_create_inferior,
+  NULL,  /* arch_setup */
   win32_attach,
   win32_kill,
   win32_detach,

Re: [PATCH 7/8] Initialise target descrption after skipping extra traps for --wrapper

[Pedro Alves]

On 07/24/2015 02:08 PM, Yao Qi wrote:

>>> I'll test this patch on targets other than x86 (such as arm and
>>> aarch64) and see if it causes fails.
> 
> I tested the patch on aarch64-linux, and no regressions are found.  I'll
> push the following patch in.
> 

Great, looks good to me.

Thanks,
Pedro Alves

[PATCH 3/8] Set general_thread after restart

[Yao Qi]

When I run gdb.server/ext-restart.exp, I get the following GDB internal
error,

run^M
The program being debugged has been started already.^M
Start it from the beginning? (y or n) y^M
Sending packet: $vKill;53c5#3d...Packet received: OK^M
Packet vKill (kill) is supported^M
Sending packet: $vFile:close:6#b6...Packet received: F0^M
Sending packet: $vFile:close:3#b3...Packet received: F0^M
Starting program: /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/gdb.server/ext-restart ^M
Sending packet: $QDisableRandomization:1#cf...Packet received: OK^M
Sending packet: $R0#82...Sending packet: $qC#b4...Packet received: QCp53c5.53c5^M  <-- [1]
Sending packet: $qAttached:53c5#c9...Packet received: E01^M
warning: Remote failure reply: E01^M
....
0x00002aaaaaaac2d0 in ?? () from target:/lib64/ld-linux-x86-64.so.2^M
/home/yao/SourceCode/gnu/gdb/git/gdb/thread.c:88: internal-error: inferior_thread: Assertion `tp' failed.^M
A problem internal to GDB has been detected,^M
further debugging may prove unreliable.^M
Quit this debugging session? (y or n) FAIL: gdb.server/ext-restart.exp: run to main (GDB internal error)
Resyncing due to internal error.

the test is to restart the program, to make sure GDBserver handles
packet 'R' correctly.  From the GDBserver output, we can see,

 Remote debugging from host 127.0.0.1^M
 Process /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/gdb.server/ext-restart created; pid = 21445^M
 GDBserver restarting^M
 Process /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/gdb.server/ext-restart created; pid = 21446^M
 Killing process(es): 21446

we first start process 21445(0x53c5), kill it and restart a new process
21446.  However, in the gdb output above [1], we can see that the reply
of qC is still the old process id rather than the new one.  Looks
general_thread isn't up to date after GDBserver receives R packet.
This patch is to update general_thread after call start_inferior.

gdb/gdbserver:

2015-07-16  Yao Qi  <yao.qi@linaro.org>

	* server.c (process_serial_event): Set general_thread.

gdb/testsuite:

2015-07-16  Yao Qi  <yao.qi@linaro.org>

	* gdb.server/ext-restart.exp: New file.
---
 gdb/gdbserver/server.c                   |  5 ++-
 gdb/testsuite/gdb.server/ext-restart.exp | 65 ++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.server/ext-restart.exp

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index ce3ffb5..53befc7 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -4121,7 +4121,10 @@ process_serial_event (void)
 
 	  /* Wait till we are at 1st instruction in prog.  */
 	  if (program_argv != NULL)
-	    start_inferior (program_argv);
+	    {
+	      start_inferior (program_argv);
+	      general_thread = last_ptid;
+	    }
 	  else
 	    {
 	      last_status.kind = TARGET_WAITKIND_EXITED;
diff --git a/gdb/testsuite/gdb.server/ext-restart.exp b/gdb/testsuite/gdb.server/ext-restart.exp
new file mode 100644
index 0000000..32a6a12
--- /dev/null
+++ b/gdb/testsuite/gdb.server/ext-restart.exp
@@ -0,0 +1,65 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test running programs using extended-remote.
+
+load_lib gdbserver-support.exp
+
+standard_testfile server.c
+
+if { [skip_gdbserver_tests] } {
+    return 0
+}
+
+if { [prepare_for_testing $testfile.exp $testfile $srcfile debug] } {
+    return -1
+}
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+set target_exec [gdbserver_download_current_prog]
+gdbserver_start_extended
+
+gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
+
+gdb_breakpoint main
+gdb_test "run" "Breakpoint.* main .*" "run to main"
+
+# Restart the process.
+with_test_prefix "restart" {
+    # Disable vRun packet and clear remote exec-file, so that GDB will
+    # use R packet to restart the process.
+    gdb_test_no_output "set remote run-packet off"
+    gdb_test_no_output "set remote exec-file"
+
+    set test "run to main"
+    gdb_test_multiple "run" $test {
+	-re {Start it from the beginning\? \(y or n\) $} {
+	    send_gdb "y\n"
+	    exp_continue
+	}
+	-re "Breakpoint.* main .*\r\n$gdb_prompt $" {
+	    pass $test
+	}
+    }
+}
+
+gdb_test "kill" "" "kill" "Kill the program being debugged.*" "y"
+
+gdb_test_no_output "monitor exit"
-- 
1.9.1

Re: [PATCH 3/8] Set general_thread after restart

[Pedro Alves]

On 07/20/2015 12:35 PM, Yao Qi wrote:
> When I run gdb.server/ext-restart.exp, I get the following GDB internal
> error,
> 
> run^M
> The program being debugged has been started already.^M
> Start it from the beginning? (y or n) y^M
> Sending packet: $vKill;53c5#3d...Packet received: OK^M
> Packet vKill (kill) is supported^M
> Sending packet: $vFile:close:6#b6...Packet received: F0^M
> Sending packet: $vFile:close:3#b3...Packet received: F0^M
> Starting program: /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/gdb.server/ext-restart ^M
> Sending packet: $QDisableRandomization:1#cf...Packet received: OK^M
> Sending packet: $R0#82...Sending packet: $qC#b4...Packet received: QCp53c5.53c5^M  <-- [1]
> Sending packet: $qAttached:53c5#c9...Packet received: E01^M
> warning: Remote failure reply: E01^M

Curious.  Kevin's original patch to handle bogus qC replies would
have hidden this bug.

Plus, today I wrote a patch that exposed a bunch of stale
general_thread issues (but not this one).

Funny how sometimes we all end up staring at the same things
at the same time.

>  
>  	  /* Wait till we are at 1st instruction in prog.  */
>  	  if (program_argv != NULL)
> -	    start_inferior (program_argv);
> +	    {
> +	      start_inferior (program_argv);
> +	      general_thread = last_ptid;
> +	    }

I think this should be:

  if (last_status.kind == TARGET_WAITKIND_STOPPED)
    {
      /* Stopped at the first instruction of the target process.  */
      general_thread = last_ptid;
    }
  else
    {
      /* Something went wrong.  */
      general_thread = null_ptid;
    }

> +# Test running programs using extended-remote.

Comment looks stale.  Looks like I missed pointing out the same
in patch #2.

Otherwise looks good to me.

(I think it's likely we have lots of stale-data bugs on the
gdb side after R, as we don't resync much.  It previously crossed my mind
that immediately after sending R, gdb should refresh all its
remote state anew, like if it had just disconnected and then reconnected.
That is, do most of what remote_start_remote does, except the
connection-specific details (qSupported, etc.)
Hard to justify the effort though -- I don't think I ever worked with
a stub that relies on R.)

Thanks,
Pedro Alves

Re: [PATCH 3/8] Set general_thread after restart

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> I think this should be:
>
>   if (last_status.kind == TARGET_WAITKIND_STOPPED)
>     {
>       /* Stopped at the first instruction of the target process.  */
>       general_thread = last_ptid;
>     }
>   else
>     {
>       /* Something went wrong.  */
>       general_thread = null_ptid;
>     }
>

OK, I'll fix it in the commit ready to push.

>> +# Test running programs using extended-remote.
>
> Comment looks stale.  Looks like I missed pointing out the same
> in patch #2.
>

I'll remove it.

> Otherwise looks good to me.
>
> (I think it's likely we have lots of stale-data bugs on the
> gdb side after R, as we don't resync much.  It previously crossed my mind
> that immediately after sending R, gdb should refresh all its
> remote state anew, like if it had just disconnected and then reconnected.
> That is, do most of what remote_start_remote does, except the
> connection-specific details (qSupported, etc.)
> Hard to justify the effort though -- I don't think I ever worked with
> a stub that relies on R.)

Even GDB refreshes all its state after sending R packet, we still need
some way to test GDB and GDBserver with R packet used.  Otherwise, it
will be bit-rotten in the future.

GDBserver has already had an option --disable-packet, so that we can
extend it to force GDBserver/GDB use R packet.  However, I don't think
we use --disable-packet much in our testing, at least I don't.  Probably
we can hack native-gdbserver.exp to run tests in a loop and pass
different --disable-packet=FOO to GDBserver in each iteration.

-- 
Yao (齐尧)

Re: [PATCH 3/8] Set general_thread after restart

[Pedro Alves]

On 07/24/2015 10:33 AM, Yao Qi wrote:

>>> +# Test running programs using extended-remote.
>>
>> Comment looks stale.  Looks like I missed pointing out the same
>> in patch #2.
>>
> 
> I'll remove it.

I find these descriptions useful.  Could you instead write something
like:

 "Test restarting programs with the R packet."

?

> 
>> Otherwise looks good to me.
>>
>> (I think it's likely we have lots of stale-data bugs on the
>> gdb side after R, as we don't resync much.  It previously crossed my mind
>> that immediately after sending R, gdb should refresh all its
>> remote state anew, like if it had just disconnected and then reconnected.
>> That is, do most of what remote_start_remote does, except the
>> connection-specific details (qSupported, etc.)
>> Hard to justify the effort though -- I don't think I ever worked with
>> a stub that relies on R.)
> 
> Even GDB refreshes all its state after sending R packet, we still need
> some way to test GDB and GDBserver with R packet used.  Otherwise, it
> will be bit-rotten in the future.

Sounds like we're talking past each other.
Not sure what I said that made it sounds like that
idea would obviate the need for the test -- I think your new
test is great.

I meant something like gdb itself, around extended_remote_restart, calling
into a new function factored out from remote_start_remote.
This is because the R packet is documented as having no reply, like
'k', no doubt because it assumes the remote target can really hard reset
after the R packet.  But let's forget it; hardly worth it to spend time
on it right now.

> 
> GDBserver has already had an option --disable-packet, so that we can
> extend it to force GDBserver/GDB use R packet.  However, I don't think
> we use --disable-packet much in our testing, at least I don't.  Probably
> we can hack native-gdbserver.exp to run tests in a loop and pass
> different --disable-packet=FOO to GDBserver in each iteration.

Thanks,
Pedro Alves

Re: [PATCH 3/8] Set general_thread after restart

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> I find these descriptions useful.  Could you instead write something
> like:
>
>  "Test restarting programs with the R packet."
>
> ?

OK.

>
>> 
>>> Otherwise looks good to me.
>>>
>>> (I think it's likely we have lots of stale-data bugs on the
>>> gdb side after R, as we don't resync much.  It previously crossed my mind
>>> that immediately after sending R, gdb should refresh all its
>>> remote state anew, like if it had just disconnected and then reconnected.
>>> That is, do most of what remote_start_remote does, except the
>>> connection-specific details (qSupported, etc.)
>>> Hard to justify the effort though -- I don't think I ever worked with
>>> a stub that relies on R.)
>> 
>> Even GDB refreshes all its state after sending R packet, we still need
>> some way to test GDB and GDBserver with R packet used.  Otherwise, it
>> will be bit-rotten in the future.
>
> Sounds like we're talking past each other.
> Not sure what I said that made it sounds like that
> idea would obviate the need for the test -- I think your new
> test is great.

No :)  I have a habit that think about how to test the change before I
start to change.  This leads me there.

>
> I meant something like gdb itself, around extended_remote_restart, calling
> into a new function factored out from remote_start_remote.
> This is because the R packet is documented as having no reply, like
> 'k', no doubt because it assumes the remote target can really hard reset
> after the R packet.  But let's forget it; hardly worth it to spend time
> on it right now.

Yes, that is a good idea, and you are right that it is hard to justify
the effort now.

-- 
Yao (齐尧)

Re: [PATCH 0/8] Fix various issues in --wrapper in GDBserver

[Yao Qi]

Yao Qi <qiyaoltc@gmail.com> writes:

> The whole series are tested on 86_64-linux both native and gdbserver.
> OK for mainline and 7.10 branch?

I withdraw patch #1 from this series, and push the rest of them in to
mainline.  I want to see the buildbot test results first, and decide
whether we need them in 7.10 branch or not later.

-- 
Yao (齐尧)