This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/7] de-couple %Stop from notification: gdbserver


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]