Re: [PATCH] Fix/Update misc comments

[Luis Machado <luis.machado@linaro.org>]

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!