From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12690 invoked by alias); 27 Jan 2012 11:49:18 -0000 Received: (qmail 12681 invoked by uid 22791); 27 Jan 2012 11:49:17 -0000 X-SWARE-Spam-Status: No, hits=-6.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 27 Jan 2012 11:48:59 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q0RBmx33029878 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 27 Jan 2012 06:48:59 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q0RBmvGN001219; Fri, 27 Jan 2012 06:48:58 -0500 Message-ID: <4F228F29.3000904@redhat.com> Date: Fri, 27 Jan 2012 11:53:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20111222 Thunderbird/9.0 MIME-Version: 1.0 To: Jan Kratochvil CC: gdb-patches@sourceware.org Subject: Re: [patch] protocol doc vs. gdbserver on H and pPID.-1 etc. [Re: [patch 2/2] Fix watchpoints for multi-inferior #2] References: <20120102164652.GB10231@host2.jankratochvil.net> <4F02020F.5090906@gmail.com> <20120120213110.GB424@host2.jankratochvil.net> <4F1EAFE6.30202@redhat.com> <20120125152240.GA26914@host2.jankratochvil.net> <4F203B6A.7090605@redhat.com> <4F204408.4090607@redhat.com> <4F205F3C.7090406@redhat.com> <20120126215039.GA6943@host2.jankratochvil.net> In-Reply-To: <20120126215039.GA6943@host2.jankratochvil.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2012-01/txt/msg00942.txt.bz2 Hi Jan, On 01/26/2012 09:50 PM, Jan Kratochvil wrote: > On Wed, 25 Jan 2012 20:59:56 +0100, Pedro Alves wrote: >> Makes no sense setting the cont_thread to a process wildcard, there's no >> such thing in the protocol. So this brings in a variant of your patch, >> that fixes it early on, instead of late in the target, along with clearing >> cont_thread on vRun. > > This behavior matches what what does gdbserver for the `H' packet - it also > does not recognize pPID.-1. > > But gdb.texinfo implicitly says pPID.-1 should be recognized as pPID but that > does not correspond to what gdbserver does. A stub that supports multi-process is required to support vCont -- vCont is not optional in that case. When you use vCont, Hc is not used. They're mutually exclusive. This what I was thinking about when I said Hc pPID.-1 does not really make sense in the protocol. It could have some meaning, but we chose not to support it, since s/c/Hc is broken anyway for multi-threaded, and vCont fixes the breakage. Since multiprocess extensions were new, we just made vCont required. As such, whatever gdbserver may be doing with Hc pPID.-1 doesn't really matter much as it's undefined territory -- GDB will never send it. The stub must support @samp{vCont} if it reports support for multiprocess extensions (@pxref{multiprocess extensions}). Note that in this case @samp{vCont} actions can be specified to apply to all threads in a process by using the @samp{p@var{pid}.-1} form of the @var{thread-id}. Unfortunately, the text has been misplaced until very recently: http://sourceware.org/ml/gdb-patches/2012-01/msg00908.html > > So I made somehow doc <-> gdbserver in sync and made it all more clear. > Do you agree with it this way? It is more important what does GDB do and the docs say than what gdbserver does, especially with regards to Hc, since gdbserver hasn't been relying on it for a long while, and the multi-process changes may have changed the legacy Hc support by mistake (I added support for starting gdbserver with --disable-packet=vCont so we can occasionally test it in legacy modes easily, though). > gdb/doc/ > 2012-01-26 Jan Kratochvil > > * gdb.texinfo (Packets): Document values for the `H' packet. > Deprecate `Hc'. > > gdb/gdbserver/ > 2012-01-26 Jan Kratochvil > > * server.c (cont_thread): New comment for it. > (process_serial_event): Handle pPID.-1 values for 'H'. > > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -33798,7 +33798,13 @@ it should be @samp{c} for step and continue operations (note that this > is deprecated, supporting the @samp{vCont} command is a better > option), @samp{g} for other operations. The thread designator > @var{thread-id} has the format and interpretation described in > -@ref{thread-id syntax}. > +@ref{thread-id syntax} - value @samp{0} will act like @samp{-1} - to > +choose all threads of all inferiors. This isn't right, and it isn't what gdbserver is doing. if (ptid_equal (gdb_id, null_ptid) || ptid_equal (gdb_id, minus_one_ptid)) thread_id = null_ptid; ... if (own_buf[1] == 'g') { if (ptid_equal (thread_id, null_ptid)) { /* GDB is telling us to choose any thread. Check if the currently selected thread is still valid. If it is not, select the first available. */ struct thread_info *thread = (struct thread_info *) find_inferior_id (&all_threads, general_thread); if (thread == NULL) thread_id = all_threads.head->id; } general_thread = thread_id; set_desired_inferior (1); Hg0 means pick any thread, not all threads. But that is documented in the first paragraph of the @ref{thread-id syntax}. I don't think it's necessary or good to re-state it here. > @samp{pPID} (or equivalent > +@samp{pPID.-1}) means to choose any single thread of that @samp{PID}. > +It does not mean all threads of that @samp{PID} process - use > +@samp{vCont} packet instead in such case. You're only talking about Hc then, right? It isn't clear due to the way the text flows. It looked as though the text was talking about all kinds of H packets right until the vCont suggestion in the end. > + > +Use of @samp{Hc} is deprecated in favor of @xref{vCont packet}. This is a good idea. > > Reply: > @table @samp > --- a/gdb/gdbserver/server.c > +++ b/gdb/gdbserver/server.c > @@ -29,7 +29,15 @@ > #include > #endif > > +/* Deprecated. See gdb.texinfo description of the `Hc' packet - use > + 'vCont' packet instead. It can be null_ptid for no inferior, > + minus_one_ptid for resuming all inferior or a specific thread ptid_t. > + Particularly process wildcard (pid > 0 && lwp == -1) for resuming > + whole one inferior is not supported. This variable is provided only > + for backward compatibility with both clients and backend, backend > + 'resume' method supports process wildcards instead. */ > ptid_t cont_thread; > + > ptid_t general_thread; > > int server_waiting; > @@ -2980,8 +2988,8 @@ process_serial_event (void) > || ptid_equal (gdb_id, minus_one_ptid)) > thread_id = null_ptid; > else if (pid != 0 > - && ptid_equal (pid_to_ptid (pid), > - gdb_id)) > + && (ptid_equal (pid_to_ptid (pid), gdb_id) > + || ptid_equal (ptid_build (pid, -1, 0), gdb_id))) The only H packet that makes sense in multiprocess presently is Hg. Which means select thread foo as general thread. But `Hg pPID.-1' would mean all threads of PID, which looks to be a nonsense packet for GDB to send. If this is for `Hc pPID.-1', as said above, that packet doesn't really "exist". So although we could find some sense to that packet, this really trades one undefined behavior for another. So I don't mind if you want to put it in, but IMO, if it doesn't serve a useful purpose (which I may well be missing), best leave things as they are. > { > struct thread_info *thread = > (struct thread_info *) find_inferior (&all_threads, -- Pedro Alves