From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3727 invoked by alias); 20 Aug 2014 14:49:43 -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 3669 invoked by uid 89); 20 Aug 2014 14:49:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 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; Wed, 20 Aug 2014 14:49:35 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s7KEn37a021424 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 20 Aug 2014 10:49:03 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s7KEmx21003538; Wed, 20 Aug 2014 10:49:00 -0400 Message-ID: <53F4B55B.3020207@redhat.com> Date: Wed, 20 Aug 2014 14:49:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Gary Benson , Doug Evans CC: gdb-patches@sourceware.org, Tom Tromey Subject: Re: [PATCH 05/11 v5] Add target/target.h References: <1406888377-25795-1-git-send-email-gbenson@redhat.com> <1406888377-25795-6-git-send-email-gbenson@redhat.com> <21474.27284.80140.944680@ruffy.mtv.corp.google.com> <20140807134840.GC19737@blade.nx> In-Reply-To: <20140807134840.GC19737@blade.nx> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-08/txt/msg00404.txt.bz2 On 08/07/2014 02:48 PM, Gary Benson wrote: > Doug Evans wrote: >> Gary Benson writes: >>> @@ -284,37 +253,18 @@ agent_run_command (int pid, const char *cmd, >>> int was_non_stop = non_stop; >>> /* Stop thread PTID. */ >>> DEBUG_AGENT ("agent: stop helper thread\n"); >>> -#ifdef GDBSERVER >>> - { >>> - struct thread_resume resume_info; >>> - >>> - resume_info.thread = ptid; >>> - resume_info.kind = resume_stop; >>> - resume_info.sig = GDB_SIGNAL_0; >>> - (*the_target->resume) (&resume_info, 1); >>> - } >>> - >>> - non_stop = 1; >>> - mywait (ptid, &status, 0, 0); >>> -#else >>> non_stop = 1; >>> target_stop (ptid); >>> >>> memset (&status, 0, sizeof (status)); >>> target_wait (ptid, &status, 0); >>> -#endif >>> non_stop = was_non_stop; >> >> The old gdbserver code set non_stop = 1 *after* asking the target to >> stop, whereas now it'll be done before (right?). Just checking that >> that's ok. >> E.g., I see a test for non_stop in linux_resume (which feels weird to be >> using in this context given that we're talking about target_stop :-)). > > Good catch! I did not notice that change. I also don't know if it's > ok. > > In the gdbserver case forcing non_stop to 1 causes need_step_over > in linux_resume to become maybe set. > If non_stop had been 0 > need_step_over would definitely be NULL. That isn't really true, see: any_pending = 0; if (!non_stop) find_inferior (&all_threads, resume_status_pending_p, &any_pending); #1 ... if (!any_pending && supports_breakpoints ()) need_step_over = (struct thread_info *) find_inferior (&all_threads, need_step_over_p, NULL); If non_stop is 0, then we execute #1 above, true. But, that may well return with ANY_PENDING still clear/0, and so 'need_step_over' may end up set anyway. So looks fine to me. > So forcing non_stop to 1 > beforehand like this patch does means a step over might take place > that would otherwise not have. See above. > > In the GDB case forcing non_stop to 1 before target_stop forces GDB > to send a SIGSTOP to each LWP. Note we're just really just stopping one LWP here, the agent helper thread, specified in PTID, not all threads. > If non_stop had been 0 linux_nat_stop > would have fallen back to inf_ptrace_stop which sends one SIGINT to > the process group. > Yeah, we definitely want SIGSTOP, not SIGINT here. Really, GDB_SIGNAL_0: SIGSTOP is how we implement "quiesce with no signal" on Linux -- the SIGSTOP is not visible to the target_wait caller. Unfortunately we have a mixup of "interrupt/ctrl-c" vs "quiesce" in the interface. > I don't know enough about this to know which is the best solution. > Pedro would know, but he's away for the next two weeks. If what is > happening currently is correct in both cases then maybe a new target > method "target_stop_all" is required to encompass the whole block of > code. > > In the interests of keeping things moving would you be ok for me to > commit the following until Pedro is back and able to advise? > > /* FIXME: This conditional code needs removing, either by > setting non_stop in the same place for both cases or by > implementing a new client method for this whole block > (less the printing code) from "int was_non_stop = non_stop;" > to "non_stop = was_non_stop;". */ > #ifdef GDBSERVER > target_stop (ptid); > non_stop = 1; > #else > non_stop = 1; > target_stop (ptid); > #endif Thanks, Pedro Alves