From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2699 invoked by alias); 12 Sep 2014 11:53:33 -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 2688 invoked by uid 89); 12 Sep 2014 11:53:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham 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, 12 Sep 2014 11:53:31 +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 (8.14.4/8.14.4) with ESMTP id s8CBrS7B008369 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 12 Sep 2014 07:53:28 -0400 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 s8CBrQ2s025237; Fri, 12 Sep 2014 07:53:26 -0400 Message-ID: <5412DEB5.6020706@redhat.com> Date: Fri, 12 Sep 2014 11:53:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.7.0 MIME-Version: 1.0 To: Gary Benson , Doug Evans CC: gdb-patches@sourceware.org Subject: Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid References: <1409320299-6812-1-git-send-email-gbenson@redhat.com> <1409320299-6812-4-git-send-email-gbenson@redhat.com> <21520.36381.756875.963606@ruffy2.mtv.corp.google.com> <20140911102659.GA17472@blade.nx> In-Reply-To: <20140911102659.GA17472@blade.nx> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-09/txt/msg00393.txt.bz2 On 09/11/2014 11:26 AM, Gary Benson wrote: > Doug Evans wrote: >> Gary Benson writes: >>> This commit introduces two new functions to stop and restart >>> target processes that shared code can use and that clients must >>> implement. It also changes some shared code to use these >>> functions. >> [...] >>> +/* See target/target.h. */ >>> + >>> +void >>> +target_continue_ptid (ptid_t ptid) >>> +{ >>> + target_resume (ptid, 0, GDB_SIGNAL_0); >>> +} >> >> How come GDB_SIGNAL_0 is used here? >> Maybe it's correct, but it's not immediately clear. >> >> The reason I ask is because there are two ways to "continue" >> the inferior: >> 1) resume it where it left off, and if it stopped because >> of a signal then forward on that signal (assuming the >> signal is not "nopass") (GDB_SIGNAL_DEFAULT). >> 2) Either inject a new signal (GDB_SIGNAL_FOO) or cancel out >> a previously queued signal (GDB_SIGNAL_0). >> >> GDB_SIGNAL_0 is used to resume the target and discarding >> any signal that it may have stopped for. >> GDB_SIGNAL_DEFAULT is used for (1). >> >> I realize the comments for target_resume say to not pass >> GDB_SIGNAL_DEFAULT to it. But the name "target_continue_ptid" >> with no option to choose between (1) and (2) >> says to me "do what GDB_SIGNAL_DEFAULT" does. > > I don't know the answer to this, I just moved the code from one > place to the next :) Possibly it's because (I think) under the > hood target_stop_ptid sends a SIGSTOP to the inferior, so you > don't want to restart it with that signal queued. No, that's not it. The SIGSTOP is an internal detail of the target implementation. It's reported to the target_wait caller as GDB_SIGNAL_0 (meaning, the thread stopped for no reason other than that GDB wanted it to stop). > The comment for target_continue_ptid says: > >>> +/* Restart a target that was previously stopped by target_stop_ptid. >>> + This function must be provided by the client. */ > > That implies to me "don't use this for targets not previously > stopped by target_stop_ptid". > > Maybe someone more familiar with this code could elaborate? I guess that'd be me... In the case that target_stop_ptid returns anything else other than GDB_SIGNAL_0, resuming with GDB_SIGNAL_0, GDB_SIGNAL_DEFAULT or the signal that the thread had stopped for would all be wrong. If we got a synchronous SIGSEGV, for instance, we'd want to abort the agent call and return error. We'd likely disable access to the agent from there on, as it's messed up. If instead we got an asynchronous signal, we'd want to hold on to it, and requeue it later, once the agent command is done with. [As an example showing this same complication being handled similarly, see See enqueue_one_deferred_signal and collecting_fast_tracepoint in gdbserver/linux-low.c. This handles the case of getting signals when they arrive while a thread is in a fast tracepoint jump pad. That's a lower level though.] But, we have to look at where/how this is presently being used to understand why the current code got away from handling that complication. The thread that we're interacting with blocks all signals. See gdbserver/tracepoint.c: static void gdb_agent_init (void) { int res; pthread_t thread; sigset_t new_mask; sigset_t orig_mask; /* We want the helper thread to be as transparent as possible, so have it inherit an all-signals-blocked mask. */ sigfillset (&new_mask); res = pthread_sigmask (SIG_SETMASK, &new_mask, &orig_mask); if (res) and, we run commands with all breakpoints lifted: /* Ask the in-process agent to run a command. Since we don't want to have to handle the IPA hitting breakpoints while running the command, we pause all threads, remove all breakpoints, and then set the helper thread re-running. We communicate with the helper thread by means of direct memory xfering, and a socket for synchronization. */ static int run_inferior_command (char *cmd, int len) { int err = -1; int pid = ptid_get_pid (current_ptid); trace_debug ("run_inferior_command: running: %s", cmd); pause_all (0); uninsert_all_breakpoints (); err = agent_run_command (pid, (const char *) cmd, len); If we want to reuse target_stop_ptid for other cases in the future, we'll likely make it return the stop waitstatus then. For now, I think just documenting target_continue_ptid as resuming with no signal is good enough. Thanks, Pedro Alves