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] Fix/Update misc comments


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!


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