This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/5] gdb: Clean up remote.c:remote_resume


On 02/17/2016 10:32 AM, Pedro Alves wrote:
On 02/17/2016 11:45 AM, Luis Machado wrote:
Just nits.

On 02/17/2016 12:44 AM, Pedro Alves wrote:
Just some refactoring / TLC.  Mainly split the old c/s/C/S packet
handling to a separate function.

gdb/ChangeLog:
2016-02-09  Pedro Alves  <palves@redhat.com>

	* remote.c (remote_resume_with_hc): New function, factored out
	from ...
	(remote_resume): ... this.  Always try vCont first.
	(remote_vcont_resume): Rename to ...
	(remote_resume_with_vcont): ... this.  Bail out if execution
	direction is reverse.
---
   gdb/remote.c | 113 ++++++++++++++++++++++++++++++++---------------------------
   1 file changed, 62 insertions(+), 51 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index fa97e1e..60e2dda 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5460,6 +5460,58 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid)
     return p;
   }

+/* Set the target running, using the packets that use Hc
+   (c/s/C/S).  */
+
+static void
+remote_resume_with_hc (struct target_ops *ops,
+		       ptid_t ptid, int step, enum gdb_signal siggnal)
+{
+  struct remote_state *rs = get_remote_state ();
+  struct thread_info *thread;
+  char *buf;
+
+  rs->last_sent_signal = siggnal;
+  rs->last_sent_step = step;
+
+  /* The c/s/C/S resume packets use Hc, so set the continue
+     thread.  */
+  if (ptid_equal (ptid, minus_one_ptid))
+    set_continue_thread (any_thread_ptid);
+  else
+    set_continue_thread (ptid);
+
+  ALL_NON_EXITED_THREADS (thread)
+    resume_clear_thread_private_info (thread);
+
+  buf = rs->buf;
+  if (execution_direction == EXEC_REVERSE)
+    {
+      /* We don't pass signals to the target in reverse exec mode.  */
+      if (info_verbose && siggnal != GDB_SIGNAL_0)
+	warning (_(" - Can't pass signal %d to target in reverse: ignored."),
+		 siggnal);
+

Even though it is existing code, this reads a bit odd.

(Also, I have no idea what that unusual leading " - " is there.)


Should we update it to "... in reverse execution: ..." maybe?

Hmm, it'd still sound like a word is missing after execution,
to me.

I did 'grep reverse * | grep "\""' and found:

  reverse.c:    error (_("Already in reverse mode.  Use '%s' or 'set exec-dir forward'."),
  infcall.c:    error (_("Cannot call functions in reverse mode."));

So maybe

   "... in reverse mode: ..."
   "... in reverse execution mode: ..."

?

I'd rather leave it be in this patch though, since it's
just a refactor with no UI change intended.


"... in reverse mode: ..." sounds good. I'm fine with leaving this be though.

   static int
-remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal)
+remote_resume_with_vcont (ptid_t ptid, int step, enum gdb_signal siggnal)
   {
     struct remote_state *rs = get_remote_state ();
     char *p;
     char *endp;

+  /* No reverse support (yet) for vCont.  */
+  if (execution_direction == EXEC_REVERSE)
+    return 0;
+

Same case as above. Also, do we need "(yet)"?

How about:

   /* There are no vCont reverse-execution actions defined.  */
   if (execution_direction == EXEC_REVERSE)
     return 0;

?

That's good.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]