From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66331 invoked by alias); 9 Jan 2020 19:48:38 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 66212 invoked by uid 89); 9 Jan 2020 19:48:21 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=ancient, child's, childs X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (207.211.31.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 09 Jan 2020 19:48:18 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578599296; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wuvk1uMFnywvVUq318jqrg7nekhs467VkbnPaHJzkBM=; b=A5q8WSNdG0DGPGezBqdtdcWLWefysHKyi8HarDG/92zbfjMFenp/iKKXdnw4TN1fiPtsVs dLXrQI/Y0iuebygroiEF1xDCxJ61jDFWljWXdvBkXtaPICCISKPxSc/j9E1i47Dz6Eryex 1QJreD4sw6KoacjDRv6v5PnXMbZjx+A= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-149-m2dM0CdzNCqCDyp2FmGaDg-1; Thu, 09 Jan 2020 14:48:14 -0500 Received: by mail-wm1-f71.google.com with SMTP id o24so740236wmh.0 for ; Thu, 09 Jan 2020 11:48:14 -0800 (PST) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id z11sm9288950wrt.82.2020.01.09.11.48.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 09 Jan 2020 11:48:12 -0800 (PST) Subject: Re: [PATCH] Fix/Update misc comments To: Luis Machado , gdb-patches@sourceware.org References: <20200106143620.2881-1-luis.machado@linaro.org> From: Pedro Alves Message-ID: <75d91766-e9bd-0ab2-60fa-3234a4dd82fc@redhat.com> Date: Thu, 09 Jan 2020 19:48:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20200106143620.2881-1-luis.machado@linaro.org> X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2020-01/txt/msg00219.txt.bz2 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