This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/7] de-couple %Stop from notification: gdbserver
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 07 Dec 2012 16:14:43 +0000
- Subject: Re: [PATCH 2/7] de-couple %Stop from notification: gdbserver
- References: <1350991620-12950-1-git-send-email-yao@codesourcery.com> <1350991620-12950-3-git-send-email-yao@codesourcery.com> <50B7AC0B.2090502@redhat.com> <50C0383B.9040705@codesourcery.com>
On 12/06/2012 06:16 AM, Yao Qi wrote:
> On 11/30/2012 02:40 AM, Pedro Alves wrote:
>>> + The timing of creating REPLY for FOO varies on different types of
>>> >+ notifications, nowadays, there are two kinds:
>>> >+ 1. The information of notification FOO is ready when FOO happens.
>>> >+ Most of the notifications are of this kind.
>>> >+ 2. The information of notification FOO is not ready or complete
>>> >+ when FOO happens. Only %Stop notification is of this kind, because
>>> >+ GDBserver has to call 'mywait' to get the waitstatus which is needed.
>> I don't understand the implications of this distinction. This patch doesn't
>> appear to make it anywhere?
>>
>
> No, we don't differentiate two types of reply in code, but I feel it would be useful to understand the concept of 'reply' (I call it 'event' in V4).
> If you think they are misleading, I don't mind removing them.
Yes, please.
>
>>> >diff --git a/gdb/gdbserver/notif.h b/gdb/gdbserver/notif.h
>>> >new file mode 100644
>>> >index 0000000..bf45910
>>> >--- /dev/null
>>> >+++ b/gdb/gdbserver/notif.h
>>> >@@ -0,0 +1,83 @@
>>> >+/* Notification to GDB.
>>> >+ Copyright (C) 1989, 1993-1995, 1997-2000, 2002-2012 Free Software
>>> >+ Foundation, Inc.
>>> >+
>>> >+ This file is part of GDB.
>>> >+
>>> >+ This program is free software; you can redistribute it and/or modify
>>> >+ it under the terms of the GNU General Public License as published by
>>> >+ the Free Software Foundation; either version 3 of the License, or
>>> >+ (at your option) any later version.
>>> >+
>>> >+ This program is distributed in the hope that it will be useful,
>>> >+ but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> >+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> >+ GNU General Public License for more details.
>>> >+
>>> >+ You should have received a copy of the GNU General Public License
>>> >+ along with this program. If not, see<http://www.gnu.org/licenses/>. */
>>> >+
>>> >+#include "ptid.h"
>>> >+#include "server.h"
>>> >+#include "target.h"
>>> >+#include "queue.h"
>>> >+
>>> >+/* Structure holding information relative to a single reply. We
>>> >+ keep a queue of these to push to GDB. */
>>> >+
>>> >+typedef struct notif_reply
>>> >+{
>>> >+ /* Thread or process that got the event. */
>>> >+ ptid_t ptid;
>> Seems very odd to me that this is here, and instead of on
>> the subclass. The fact that stop replies are associated
>> with a ptid seems to me to be a detail of stop replies.
>>
>> I think it's better to not store knowlege of specific notification
>> types in notif.c, but instead make the caller/client hold the struct
>> notif instances. Then if discarding stop replies for a given ptid
>> makes sense, that is handled by the client. It may not make sense
>> for some other notification.
>>
>
> The 'ptid' is not the details specific to stop notification. PTID is also useful to other types of notification.
I don't think it's useful to all. Those that need a ptid should add it to their
own payload type (the type that extends notif_reply). A "stop" always has a ptid and a
wait status. A breakpoint-created-on-the-target-side notification can have no ptid, or more
than one, really, or a address-space id, or something else even. A trace-session-stopped carries no ptid
currently, but it could have one, or more than one, if multi-process tracing would
apply to several inferiors. A "connection to downstream debug agent API lost"
notification would likely have other kinds of info associated instead of a PTID.
So it feels like an abstraction violation to me.
> When an inferior exits, only the 'notif_reply' associated with this inferior should
> be cleared, instead of all 'notif_reply'.
IMO that's orthogonal, and should be handled on a per-reply-type basis.
I think/feel we'll have types that happen to have a ptid in their payload, but
you don't _want_ the notifications to get removed when the inferior exits.
Anyways, not really a big deal.
--
Pedro Alves