From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28791 invoked by alias); 11 Sep 2015 08:34:58 -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 28777 invoked by uid 89); 11 Sep 2015 08:34:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 11 Sep 2015 08:34:56 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 2F8428B136; Fri, 11 Sep 2015 08:34:55 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t8B8Yqnt008220; Fri, 11 Sep 2015 04:34:53 -0400 Message-ID: <55F2922C.9060306@redhat.com> Date: Fri, 11 Sep 2015 08:34:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Don Breazeal , "gdb-patches@sourceware.org" Subject: Re: [PATCH v3 1/4] Extended-remote follow exec References: <1438298360-29594-1-git-send-email-donb@codesourcery.com> <1441839937-22251-1-git-send-email-donb@codesourcery.com> <1441839937-22251-2-git-send-email-donb@codesourcery.com> <55F17AFA.5080102@redhat.com> <55F20AA7.8050301@codesourcery.com> In-Reply-To: <55F20AA7.8050301@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-09/txt/msg00209.txt.bz2 Hi Don, Other than the nits below, it LGTM. Fix those and you're good to go. Please push. On 09/10/2015 11:56 PM, Don Breazeal wrote: > On 9/10/2015 5:43 AM, Pedro Alves wrote: >> On 09/10/2015 12:05 AM, Don Breazeal wrote: >>> Hi Pedro, >>> >>> This is an updated version of the patch previously submitted here: >>> https://sourceware.org/ml/gdb-patches/2015-07/msg00924.html. Changes >>> from the previous version include: >>> >>> * In gdbserver, when an exec event occurs, gdbserver deletes all >>> of the data (inferior, lwps, threads) associated with the execing >>> process and replaces it with a new set of data. >>> >>> * In GDB, the remote exec-file is now stored per-inferior in the >>> inferior's program space as a REGISTRY field. >>> >>> * In GDB, a new target hook, target_follow_exec, is used to enable >>> storing the remote exec-file as per-inferior data. >>> >>> * In GDB, follow_exec now calls add_inferior_with_spaces for mode >>> "new" in place of add_inferior and the calls to set up the program >>> and address spaces. >>> >>> Some of the things that were part of the previous patchset were >>> eliminated as a result of these changes, including: >>> >>> * Deleting "vanished" lwps in gdbserver/linux-low.c:send_sigstop. >>> >>> * Fiddling with the regcache and r_debug in >>> gdbserver/linux-low.c:handle_extended_wait. >>> >>> * Fiddling with the inferior's architecture in >>> remote.c:remote_parse_stop_reply. >>> >>> A couple of your questions about the previous version of the patch still >>> apply, in spite of the rework. Regarding the handling of the exec event >>> in linux-low.c:handle_extended_wait: >>> >>>>> + /* Mark the exec status as pending. */ >>>>> + event_lwp->stopped = 1; >>>>> + event_lwp->status_pending_p = 1; >>>>> + event_lwp->status_pending = wstat; >>>>> + event_thr->last_resume_kind = resume_stop; >>>> >>>> Shouldn't this be resume_continue? >>> >>> My thinking here is that as far as gdbserver is concerned, we *do* want >>> to use resume_stop, so that we stop and report the event to GDB. It will >>> be up to GDB whether to continue from this point. Does that make sense? >> >> Not really -- putting exec events out of the picture, consider: >> >> If you simply continue a thread (vCont;c) and it hits a breakpoint, it'll >> have last_resume_kind==resume_continue, and we still report the event >> to gdb, and it's still up to GDB whether to continue past the breakpoint. >> >> So if you set last_resume_kind to resume_continue, and drop this hunk: >> >>> @@ -3373,7 +3463,8 @@ linux_wait_1 (ptid_t ptid, >>> ourstatus->value.sig = GDB_SIGNAL_0; >>> } >>> else if (current_thread->last_resume_kind == resume_stop >>> - && WSTOPSIG (w) != SIGSTOP) >>> + && WSTOPSIG (w) != SIGSTOP >>> + && ourstatus->kind != TARGET_WAITKIND_EXECD) >>> { >>> /* A thread that has been requested to stop by GDB with vCont;t, >>> but, it stopped for other reasons. */ >>> @@ -5801,6 +5892,14 @@ linux_supports_vfork_events (void) >>> return linux_supports_tracefork (); >>> } >> >> ... what doesn't work? > > That works just fine. I've made that change. > > Clearly I didn't understand the purpose of resume_stop. Is that only > used when GDB requests a stop, and/or when an inferior is just starting > up or being attached? As opposed to when the inferior is stopped by an > event? Yeah, lots of different state flags, and several layers of state machines involved. gdb's, core gdbserver's, and linux-low's. The flags we have today have come into being through code evolution, rather than design... Probably, we could probably merge/simplify them, but it'd require lots of careful analysis. Anyway, from a high level, thread->last_resume_kind indicates the last resume state from _gdb_'s perspective. So resume_stop is used: - when GDB requests an explicit stop with vCont;t. The thread gets set to resume_stop even if it is still running. - when a thread that was last continued/stepped (resume_continue/resume_step) hits an event and _after_ gdbserver reports the stop to gdb, _then_ it's last resume kind is set to resume_stop. > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -75,6 +75,14 @@ > static char *target_buf; > static long target_buf_size; > > +/* Per-program-space data key. */ > +static const struct program_space_data *remote_pspace_data; > + > +/* The variable registered as the control variable used by the > + remote exec-file commands. Used by the set/show machinery > + as the location of the remote exec-file value. */ > +static char *remote_exec_file_var; I think we should mention the per-program-space aspect. Something like: /* The variable registered as the control variable used by the remote exec-file commands. While the remote exec-file setting is per-program-space, the set/show machinery uses this as location of the remote exec-file value. */ > +/* Fetch the remote exec-file from the current program space. */ > + > +static const char * > +get_remote_exec_file (void) > +{ > + char *remote_exec_file; > + > + remote_exec_file = program_space_data (current_program_space, > + remote_pspace_data); > + if (remote_exec_file == NULL) > + return ""; > + > + return remote_exec_file; > +} > + > +/* Set the remote exec file for the current program space. */ /* Set the remote exec file for PSPACE. */ > + > +static void > +set_remote_exec_file_1 (struct program_space *pspace, > + char *remote_exec_file) I think we can now rename this for clarity. E.g., set_program_space_remote_exec_file / set_pspace_remote_exec_file. > +{ > + char *old_file = program_space_data (pspace, remote_pspace_data); > + > + xfree (old_file); > + set_program_space_data (pspace, remote_pspace_data, > + xstrdup (remote_exec_file)); > +} Thanks, Pedro Alves