Patchwork Fix/Update misc comments

login
register
mail settings
Submitter Luis Machado
Date Jan. 6, 2020, 2:36 p.m.
Message ID <20200106143620.2881-1-luis.machado@linaro.org>
Download mbox | patch
Permalink /patch/37212/
State New
Headers show

Comments

Luis Machado - Jan. 6, 2020, 2:36 p.m.
While doing some investigation of mine, i noticed a few typos,
inaccuracies and missing information.

I went ahead and updated/improved those.

How does it look?

Regards,
Luis

gdb/ChangeLog:

2020-01-06  Luis Machado  <luis.machado@linaro.org>

	* inf-ptrace.c (inf_ptrace_target::resume): Update comments
	* infrun.c (resume_1): Likewise.
	(handle_inferior_event): Likewise.
	* linux-nat.c (linux_nat_target::resume): Likewise.
	(save_stop_reason): Likewise.
	(linux_nat_filter_event): Likewise.
	* linux-nat.h (struct lwp_info) <stop_pc>, <stop_reason>: Likewise.
---
 gdb/inf-ptrace.c |  4 ++--
 gdb/infrun.c     | 11 ++++++-----
 gdb/linux-nat.c  | 16 +++++++++++-----
 gdb/linux-nat.h  |  4 ++--
 4 files changed, 21 insertions(+), 14 deletions(-)
Tom Tromey - Jan. 9, 2020, 3:34 p.m.
>>>>> "Luis" == Luis Machado <luis.machado@linaro.org> writes:

Luis> How does it look?

It seems reasonable to me.  Probably Pedro should review it though.

Luis>        /* If this system does not support PT_STEP, a higher level
Luis> -         function will have called single_step() to transmute the step
Luis> -         request into a continue request (by setting breakpoints on
Luis> +         function will have called the appropriate functions to transmute the
Luis> +	 step request into a continue request (by setting breakpoints on

Is single_step not actually the function in question here?

Tom
Luis Machado - Jan. 9, 2020, 3:38 p.m.
On 1/9/20 12:34 PM, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado <luis.machado@linaro.org> writes:
> 
> Luis> How does it look?
> 
> It seems reasonable to me.  Probably Pedro should review it though.
> 

Thanks.

> Luis>        /* If this system does not support PT_STEP, a higher level
> Luis> -         function will have called single_step() to transmute the step
> Luis> -         request into a continue request (by setting breakpoints on
> Luis> +         function will have called the appropriate functions to transmute the
> Luis> +	 step request into a continue request (by setting breakpoints on
> 
> Is single_step not actually the function in question here?
> 

That's the confusing part. There is a single_step (...) function for 
gdbserver linux-low, but i don't see such a function for gdb.

So this reference looks stale to me.

> Tom
>
Tom Tromey - Jan. 9, 2020, 6:54 p.m.
Luis> That's the confusing part. There is a single_step (...) function for
Luis> gdbserver linux-low, but i don't see such a function for gdb.

Luis> So this reference looks stale to me.

Hah, I jumped to the tag and saw the function existed, but forgot to
check the directory.

The patch seems fine to me.  You can check it in.
If Pedro has some suggestions, we can always put in another patch.

Tom
Pedro Alves - Jan. 9, 2020, 7:48 p.m.
On 1/6/20 2:36 PM, Luis Machado wrote:

> @@ -354,8 +354,8 @@ inf_ptrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
>    if (step)
>      {
>        /* If this system does not support PT_STEP, a higher level
> -         function will have called single_step() to transmute the step
> -         request into a continue request (by setting breakpoints on
> +         function will have called the appropriate functions to transmute the
> +	 step request into a continue request (by setting breakpoints on

tabs vs spaces mixup.

>           all possible successor instructions), so we don't have to
>           worry about that here.  */
>        request = PT_STEP;
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 7ddd21dd09..14c1e76ac1 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2388,8 +2388,8 @@ resume_1 (enum gdb_signal sig)
>    if (tp->control.trap_expected || bpstat_should_step ())
>      tp->control.may_range_step = 0;
>  
> -  /* If enabled, step over breakpoints by executing a copy of the
> -     instruction at a different address.
> +  /* If displaced stepping is enabled, step over breakpoints by executing a
> +     copy of the instruction at a different address.
>  
>       We can't use displaced stepping when we have a signal to deliver;
>       the comments for displaced_step_prepare explain why.  The
> @@ -2477,7 +2477,7 @@ resume_1 (enum gdb_signal sig)
>        && step_over_info_valid_p ())
>      {
>        /* If we have nested signals or a pending signal is delivered
> -	 immediately after a handler returns, might might already have
> +	 immediately after a handler returns, might already have
>  	 a step-resume breakpoint set on the earlier handler.  We cannot
>  	 set another step-resume breakpoint; just continue on until the
>  	 original breakpoint is hit.  */
> @@ -4928,8 +4928,9 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
>        stop_waiting (ecs);
>        return;
>  
> -      /* The following are the only cases in which we keep going;
> -         the above cases end in a continue or goto.  */
> +      /* The following and TARGET_WAITKIND_THREAD_CREATED are the only
> +	 cases in which we keep going. The other cases end in a continue or
> +	 goto.  */

Double space after period after "keep going".  But if you're changing this,
better to update it to current reality -- the "continue" or "goto" this is
referring to is loooooong gone.  I think this is referring to the ancient
giant loop that wait_for_inferior used to be, and then was gradually over
the years broken down into separate functions.  "continue" and "goto" are
probably the modern stop_waiting and prepare_to_wait functions
(guessing here).

I'm just not seeing much value in the whole comment anymore.  How about
just removing it?


> @@ -2976,7 +2977,12 @@ linux_nat_filter_event (int lwpid, int status)
>    /* Make sure we don't report an event for the exit of an LWP not in
>       our list, i.e. not part of the current process.  This can happen
>       if we detach from a program we originally forked and then it
> -     exits.  */
> +     exits.
> +
> +     Note the forked children exiting may generate a SIGCHLD to the parent
> +     process.  We are still interested in that signal since the parent may
> +     have handlers for it, so we don't ignore it.  */

I'm not sure about this comment -- it seems distracting to me, in the
sense that I've read it a number of times to try to understand what is
it is that saying that is special, because in my view, we're interested in
that SIGCHLD signal simply if it is sent to a process that we're debugging,
just like all other signals.  Maybe I didn't understand it and I'm missing
the special case here.

Note that gdbserver has equivalent code in linux-low.c:

  /* If we didn't find a process, one of two things presumably happened:
     - A process we started and then detached from has exited.  Ignore it.
     - A process we are controlling has forked and the new child's stop
     was reported to us by the kernel.  Save its PID.  */
  if (child == NULL && WIFSTOPPED (wstat))
    {
      add_to_pid_list (&stopped_pids, lwpid, wstat);
      return NULL;
    }
  else if (child == NULL)
    return NULL;

> +
>    if (!WIFSTOPPED (status) && !lp)
>      return NULL;
>  


> --- a/gdb/linux-nat.h
> +++ b/gdb/linux-nat.h
> @@ -228,7 +228,7 @@ struct lwp_info
>  
>    /* When 'stopped' is set, this is where the lwp last stopped, with
>       decr_pc_after_break already accounted for.  If the LWP is
> -     running, and stepping, this is the address at which the lwp was
> +     running and stepping, this is the address at which the lwp was
>       resumed (that is, it's the previous stop PC).  If the LWP is
>       running and not stepping, this is 0.  */
>    CORE_ADDR stop_pc;
> @@ -237,7 +237,7 @@ struct lwp_info
>    int step;
>  
>    /* The reason the LWP last stopped, if we need to track it
> -     (breakpoint, watchpoint, etc.)  */
> +     (breakpoint, watchpoint, etc).  */

AFAIK, "etc." is the correct abbreviation of "et cetera".
So I think this should be:

     (breakpoint, watchpoint, etc.).  */

Thanks,
Pedro Alves
Luis Machado - Jan. 9, 2020, 11:26 p.m.
On 1/9/20 4:48 PM, Pedro Alves wrote:
> On 1/6/20 2:36 PM, Luis Machado wrote:
> 
>> @@ -354,8 +354,8 @@ inf_ptrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
>>     if (step)
>>       {
>>         /* If this system does not support PT_STEP, a higher level
>> -         function will have called single_step() to transmute the step
>> -         request into a continue request (by setting breakpoints on
>> +         function will have called the appropriate functions to transmute the
>> +	 step request into a continue request (by setting breakpoints on
> 
> tabs vs spaces mixup.
> 

Fixed the whole block now. It had spaces.

>> @@ -4928,8 +4928,9 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
>>         stop_waiting (ecs);
>>         return;
>>   
>> -      /* The following are the only cases in which we keep going;
>> -         the above cases end in a continue or goto.  */
>> +      /* The following and TARGET_WAITKIND_THREAD_CREATED are the only
>> +	 cases in which we keep going. The other cases end in a continue or
>> +	 goto.  */
> 
> Double space after period after "keep going".  But if you're changing this,
> better to update it to current reality -- the "continue" or "goto" this is
> referring to is loooooong gone.  I think this is referring to the ancient
> giant loop that wait_for_inferior used to be, and then was gradually over
> the years broken down into separate functions.  "continue" and "goto" are
> probably the modern stop_waiting and prepare_to_wait functions
> (guessing here).
> 
> I'm just not seeing much value in the whole comment anymore.  How about
> just removing it?

Yeah, i remember that old loop.

The removal sounds OK to me. I've now dropped this block of comments.

> 
> 
>> @@ -2976,7 +2977,12 @@ linux_nat_filter_event (int lwpid, int status)
>>     /* Make sure we don't report an event for the exit of an LWP not in
>>        our list, i.e. not part of the current process.  This can happen
>>        if we detach from a program we originally forked and then it
>> -     exits.  */
>> +     exits.
>> +
>> +     Note the forked children exiting may generate a SIGCHLD to the parent
>> +     process.  We are still interested in that signal since the parent may
>> +     have handlers for it, so we don't ignore it.  */
> 
> I'm not sure about this comment -- it seems distracting to me, in the
> sense that I've read it a number of times to try to understand what is
> it is that saying that is special, because in my view, we're interested in
> that SIGCHLD signal simply if it is sent to a process that we're debugging,
> just like all other signals.  Maybe I didn't understand it and I'm missing
> the special case here.
> 

While trying to understand this bug 
(https://sourceware.org/ml/gdb-patches/2019-12/msg01071.html), i think i 
misunderstood the comment a little and ended up saying the same, sorry. 
I've now dropped this hunk.

> Note that gdbserver has equivalent code in linux-low.c:

Indeed. The signal gets passed on to the inferior, but shouldn't cause a 
visible stop.

> 
>    /* If we didn't find a process, one of two things presumably happened:
>       - A process we started and then detached from has exited.  Ignore it.
>       - A process we are controlling has forked and the new child's stop
>       was reported to us by the kernel.  Save its PID.  */
>    if (child == NULL && WIFSTOPPED (wstat))
>      {
>        add_to_pid_list (&stopped_pids, lwpid, wstat);
>        return NULL;
>      }
>    else if (child == NULL)
>      return NULL;
> 
>> +
>>     if (!WIFSTOPPED (status) && !lp)
>>       return NULL;
>>   
> 
> 
>> --- a/gdb/linux-nat.h
>> +++ b/gdb/linux-nat.h
>> @@ -228,7 +228,7 @@ struct lwp_info
>>   
>>     /* When 'stopped' is set, this is where the lwp last stopped, with
>>        decr_pc_after_break already accounted for.  If the LWP is
>> -     running, and stepping, this is the address at which the lwp was
>> +     running and stepping, this is the address at which the lwp was
>>        resumed (that is, it's the previous stop PC).  If the LWP is
>>        running and not stepping, this is 0.  */
>>     CORE_ADDR stop_pc;
>> @@ -237,7 +237,7 @@ struct lwp_info
>>     int step;
>>   
>>     /* The reason the LWP last stopped, if we need to track it
>> -     (breakpoint, watchpoint, etc.)  */
>> +     (breakpoint, watchpoint, etc).  */
> 
> AFAIK, "etc." is the correct abbreviation of "et cetera".
> So I think this should be:

heh... it is true. Now i know! And i see there are quite a few offenders 
throughout the code. Something for a separate patch.

> 
>       (breakpoint, watchpoint, etc.).  */

Fixed now. Thanks!

Patch

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index fd18146efe..e424b9668b 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -354,8 +354,8 @@  inf_ptrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
   if (step)
     {
       /* If this system does not support PT_STEP, a higher level
-         function will have called single_step() to transmute the step
-         request into a continue request (by setting breakpoints on
+         function will have called the appropriate functions to transmute the
+	 step request into a continue request (by setting breakpoints on
          all possible successor instructions), so we don't have to
          worry about that here.  */
       request = PT_STEP;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 7ddd21dd09..14c1e76ac1 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2388,8 +2388,8 @@  resume_1 (enum gdb_signal sig)
   if (tp->control.trap_expected || bpstat_should_step ())
     tp->control.may_range_step = 0;
 
-  /* If enabled, step over breakpoints by executing a copy of the
-     instruction at a different address.
+  /* If displaced stepping is enabled, step over breakpoints by executing a
+     copy of the instruction at a different address.
 
      We can't use displaced stepping when we have a signal to deliver;
      the comments for displaced_step_prepare explain why.  The
@@ -2477,7 +2477,7 @@  resume_1 (enum gdb_signal sig)
       && step_over_info_valid_p ())
     {
       /* If we have nested signals or a pending signal is delivered
-	 immediately after a handler returns, might might already have
+	 immediately after a handler returns, might already have
 	 a step-resume breakpoint set on the earlier handler.  We cannot
 	 set another step-resume breakpoint; just continue on until the
 	 original breakpoint is hit.  */
@@ -4928,8 +4928,9 @@  Cannot fill $_exitsignal with the correct signal number.\n"));
       stop_waiting (ecs);
       return;
 
-      /* The following are the only cases in which we keep going;
-         the above cases end in a continue or goto.  */
+      /* The following and TARGET_WAITKIND_THREAD_CREATED are the only
+	 cases in which we keep going. The other cases end in a continue or
+	 goto.  */
     case TARGET_WAITKIND_FORKED:
     case TARGET_WAITKIND_VFORKED:
       /* Check whether the inferior is displaced stepping.  */
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 465b2acd94..53256d4edb 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1696,7 +1696,8 @@  linux_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
   resume_many = (minus_one_ptid == ptid
 		 || ptid.is_pid ());
 
-  /* Mark the lwps we're resuming as resumed.  */
+  /* Mark the lwps we're resuming as resumed and update their
+     last_resume_kind to resume_continue.  */
   iterate_over_lwps (ptid, resume_set_callback);
 
   /* See if it's the current inferior that should be handled
@@ -2721,7 +2722,7 @@  save_stop_reason (struct lwp_info *lp)
 	    {
 	      /* If we determine the LWP stopped for a SW breakpoint,
 		 trust it.  Particularly don't check watchpoint
-		 registers, because at least on s390, we'd find
+		 registers, because, at least on s390, we'd find
 		 stopped-by-watchpoint as long as there's a watchpoint
 		 set.  */
 	      lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
@@ -2925,7 +2926,7 @@  resumed_callback (struct lwp_info *lp)
 }
 
 /* Check if we should go on and pass this event to common code.
-   Return the affected lwp if we are, or NULL otherwise.  */
+   Return the affected lwp if we should, or NULL otherwise.  */
 
 static struct lwp_info *
 linux_nat_filter_event (int lwpid, int status)
@@ -2976,7 +2977,12 @@  linux_nat_filter_event (int lwpid, int status)
   /* Make sure we don't report an event for the exit of an LWP not in
      our list, i.e. not part of the current process.  This can happen
      if we detach from a program we originally forked and then it
-     exits.  */
+     exits.
+
+     Note the forked children exiting may generate a SIGCHLD to the parent
+     process.  We are still interested in that signal since the parent may
+     have handlers for it, so we don't ignore it.  */
+
   if (!WIFSTOPPED (status) && !lp)
     return NULL;
 
@@ -3118,7 +3124,7 @@  linux_nat_filter_event (int lwpid, int status)
 
   /* Don't report signals that GDB isn't interested in, such as
      signals that are neither printed nor stopped upon.  Stopping all
-     threads can be a bit time-consuming so if we want decent
+     threads can be a bit time-consuming, so if we want decent
      performance with heavily multi-threaded programs, especially when
      they're using a high frequency timer, we'd better avoid it if we
      can.  */
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index 0c1695ad10..2f0eeaf362 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -228,7 +228,7 @@  struct lwp_info
 
   /* When 'stopped' is set, this is where the lwp last stopped, with
      decr_pc_after_break already accounted for.  If the LWP is
-     running, and stepping, this is the address at which the lwp was
+     running and stepping, this is the address at which the lwp was
      resumed (that is, it's the previous stop PC).  If the LWP is
      running and not stepping, this is 0.  */
   CORE_ADDR stop_pc;
@@ -237,7 +237,7 @@  struct lwp_info
   int step;
 
   /* The reason the LWP last stopped, if we need to track it
-     (breakpoint, watchpoint, etc.)  */
+     (breakpoint, watchpoint, etc).  */
   enum target_stop_reason stop_reason;
 
   /* On architectures where it is possible to know the data address of