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

[Luis Machado <lgustavo@codesourcery.com>]

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.