Re: [PATCH] Fix/Update misc comments

Luis Machado
2020-01-09T23:26:18Z

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!