This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] protocol doc vs. gdbserver on H and pPID.-1 etc. [Re: [patch 2/2] Fix watchpoints for multi-inferior #2]
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 <jan.kratochvil@redhat.com>
>
> * gdb.texinfo (Packets): Document values for the `H' packet.
> Deprecate `Hc'.
>
> gdb/gdbserver/
> 2012-01-26 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * 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 <sys/wait.h>
> #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