Re: [PATCH 3/7] de-couple %Stop from notification: gdb

[Yao Qi <yao at codesourcery dot com>]

On 12/01/2012 01:36 AM, Pedro Alves wrote:
> > +/* Asynchronous signal handle registered as event loop source for when
> > >+   the remote sent us a notification.  The registered callback
> > >+   will do a ACK sequence to pull the rest of the events out of
> > >+   the remote side into our event queue.  */
> > >+
> > >+static struct async_event_handler *remote_async_get_pending_events_token;
> Hmm?  So there's only one token for all notification types?

Yes.

> What if two notifications of different types are pending?

We have a queue called 'notif_queue' for 'struct notif'.  When multiple 
types of notification are pending, pointers to 'struct notif' objects 
are put in 'notif_queue' and mark 
'remote_async_get_pending_events_token'.  In 
remote_async_get_pending_events_handler, GDB will iterate over 
'notif_queue', and process them one by one.

> > >+
> > >+/* A stop notification.  */
> > >+
> > >+struct notif_stop
> > >+{
> > >+  struct notif base;
> > >+  /* The list of already fetched and acknowledged events.  */
> > >+  QUEUE (notif_reply_p) *ack_queue;
> > >+};
> I'm confused on the abstraction layers here.  So other notifications
> won't need a queue?  How do they work then?  And then, if only stops
> need it, why do we need to stuff it in a new type, and come up with
> "struct notif_stop" at all?  This seems to be related to the comment
> in notif.c that talks about stop replies being different.  Then, it seems
> we're just adding layers for no no real generalization.  I'd rather
> just leave the queue global as it was, and make notif_packet_stop a plain
> struct notif.  Make struct notif|notif.c as simple as possible, a layer
> to build uppon, with no knowlege of the sub types of notifications.

The notification %Stop needs a queue because GDB fetches mulitple 
'notif_reply', but can't consume them immediately (these 'notif_reply' 
will be consumed in remote_wait_{as,ns} one per time).  So a queue is 
needed.  However, each 'notif_reply' of other notifications can be 
consumed immediately GDB gets them.  For example, when GDB is fetching 
queued trace events from GDBserver (via 'vTraced'), each event can be 
processed at once, for example emitting a MI '=trace-stopped' 
notification', and continue to fetch the next event.

My original intention is to define a struct 'notif_postmortem' like this,

  struct notif_postmortem
  {
    struct notif base;
    /* The list of already fetched and acknowledged events.  */
    QUEUE (notif_reply_p) *ack_queue;
  };

to handle notification Stop and other similar notifications 
to-be-appeared in the future.  Considering notification Stop is the only 
one, so I renamed this struct to notif_stop later.

I agree with you that we can leave the queue global and use 'struct 
notif' for Stop notification.  Once we have the new notification similar 
to Stop in the future, we can revisit the generalization here.

> > >-    {
> > >-      VEC_free (cached_reg_t, r->regcache);
> > >-      xfree (r);
> > >-    }
> > >+    VEC_free (cached_reg_t, r->regcache);
> > >  }
> > >
> > >-/* Discard all pending stop replies of inferior PID.  If PID is -1,
> > >-   discard everything.  */
> > >+static struct notif_reply *
> > >+remote_notif_alloc_reply_stop (void)
> > >+{
> > >+  struct notif_reply *r = (struct notif_reply *) XMALLOC (struct stop_reply);
> No need for the cast.  See XMALLOC's definition.

We are casting 'struct stop_reply *' to 'struct notif_reply *', which is 
needed.

The rest of comments will be addressed in V4.

--
Yao (éå)