Re-initializing a list after the control returns to gdb...

Re-initializing a list after the control returns to gdb...

[Joel Brobecker]

Hello,

I am trying to do a small cleanup in our ada-tasks module, and I was
wondering if somebody could help me making sure I am using the right
approach.

Basically: the ada_language provides a set of commands, namely "info
tasks" and "task". Here is the syntax:

  - info tasks  -> prints the list of tasks, equivalent of "info threads"
  - info tasks <task_id> -> the detailed information on a given task
  - task -> print the current task id, equivalent of "thread"
  - task <task_id> -> switch to the given task

The small annoyance I am trying to fix is that all these commands use
a list internally maintained in ada-tasks.c, which gets built only
when the "info tasks" command is called. So far, we have documented that
the above command may only be used after "info tasks" has been issued.

What I am trying to do is to lift this limitation, by updating the
list of tasks every time the user has resumed the execution of the
inferior, just before the user gets back the prompt.

I did it by inserting a call to ada_reset_tasks_list() inside
normal_stop(), like so:

*************** print_stop_reason (enum inferior_stop_re
*** 3360,3365 ****
--- 3362,3369 ----
  void
  normal_stop (void)
  {
+   ada_reset_tasks_list (NULL);
+ 
    /* As with the notification of thread events, we want to delay
       notifying the user that we've switched thread context until
       the inferior actually stops.

My testing shows that it seems to be working pretty well. But do you
think that this is a good idea? And does this change have any chance of
being accepted for inclusion (eventually, we are still ironing out some
wrinkles here and there to make our changes more acceptable, but hope to
be able to contribute our ada-language support changes soon)?

-- 
Joel

Re: Re-initializing a list after the control returns to gdb...

[Andrew Cagney]

> Hello,
> 
> I am trying to do a small cleanup in our ada-tasks module, and I was
> wondering if somebody could help me making sure I am using the right
> approach.

Joel,

A more higher level question.  What exactly is the story behind 
ada-task*.  I glanced at the code and it looked very like a clone of the 
existing thread code.

Andrew


> Basically: the ada_language provides a set of commands, namely "info
> tasks" and "task". Here is the syntax:
> 
>   - info tasks  -> prints the list of tasks, equivalent of "info threads"
>   - info tasks <task_id> -> the detailed information on a given task
>   - task -> print the current task id, equivalent of "thread"
>   - task <task_id> -> switch to the given task
> 
> The small annoyance I am trying to fix is that all these commands use
> a list internally maintained in ada-tasks.c, which gets built only
> when the "info tasks" command is called. So far, we have documented that
> the above command may only be used after "info tasks" has been issued.
> 
> What I am trying to do is to lift this limitation, by updating the
> list of tasks every time the user has resumed the execution of the
> inferior, just before the user gets back the prompt.
> 
> I did it by inserting a call to ada_reset_tasks_list() inside
> normal_stop(), like so:
> 
> *************** print_stop_reason (enum inferior_stop_re
> *** 3360,3365 ****
> --- 3362,3369 ----
>   void
>   normal_stop (void)
>   {
> +   ada_reset_tasks_list (NULL);
> + 
>     /* As with the notification of thread events, we want to delay
>        notifying the user that we've switched thread context until
>        the inferior actually stops.
> 
> My testing shows that it seems to be working pretty well. But do you
> think that this is a good idea? And does this change have any chance of
> being accepted for inclusion (eventually, we are still ironing out some
> wrinkles here and there to make our changes more acceptable, but hope to
> be able to contribute our ada-language support changes soon)?
> 
> -- Joel 

Re: Re-initializing a list after the control returns to gdb...

[Joel Brobecker]

> A more higher level question.  What exactly is the story behind 
> ada-task*.  I glanced at the code and it looked very like a clone of the 
> existing thread code.

Not quite: What happens is that the GNAT runtime maintains an array
of Ada_Task_Control_Block. The ada-tasks module reads this array
to build the list of tasks, and display the status information for
each task. We then try to use the thread module to do the task
switching.

(We realize the code is a bit horrible, that's why we are trying to
clean it up a bit. The question I asked was in fact for one of these
cleanups. I can send my latest WIP version if yo are interested in
the current state of this file).

Does this answer your question?
-- 
Joel

Re: Re-initializing a list after the control returns to gdb...

[Andrew Cagney]

>> A more higher level question.  What exactly is the story behind 
>> ada-task*.  I glanced at the code and it looked very like a clone of the 
>> existing thread code.
> 
> 
> Not quite: What happens is that the GNAT runtime maintains an array
> of Ada_Task_Control_Block. The ada-tasks module reads this array
> to build the list of tasks, and display the status information for
> each task. We then try to use the thread module to do the task
> switching.

So let me get my head round this, the target stack should look like:

ada-tasks
threads
lwp
proc/ptrace

or possibly the ada-tasks sits more beside the thread code?

Andrew


> (We realize the code is a bit horrible, that's why we are trying to
> clean it up a bit. The question I asked was in fact for one of these
> cleanups. I can send my latest WIP version if yo are interested in
> the current state of this file).

No, I just know it needs that cleanup.

Andrew

Re: Re-initializing a list after the control returns to gdb...

[Joel Brobecker]

> ada-tasks
> threads
> lwp
> proc/ptrace
> 
> or possibly the ada-tasks sits more beside the thread code?

It sits more besides the thread module. Something like this:

    threads  <--|uses|-- ada-tasks
    lwp
    ...

-- 
Joel

Re: Re-initializing a list after the control returns to gdb...

[Andrew Cagney]

> I did it by inserting a call to ada_reset_tasks_list() inside
> normal_stop(), like so:
> 
> *************** print_stop_reason (enum inferior_stop_re
> *** 3360,3365 ****
> --- 3362,3369 ----
>   void
>   normal_stop (void)
>   {
> +   ada_reset_tasks_list (NULL);
> + 
>     /* As with the notification of thread events, we want to delay
>        notifying the user that we've switched thread context until
>        the inferior actually stops.
> 
> My testing shows that it seems to be working pretty well. But do you
> think that this is a good idea? And does this change have any chance of
> being accepted for inclusion (eventually, we are still ironing out some
> wrinkles here and there to make our changes more acceptable, but hope to
> be able to contribute our ada-language support changes soon)?

You've the `observer' problem.

GDB's observer code is very primative.  A combination of gdb-events 
(needs direction) and chained hooks (see target_new_objfile_hook).

I suspect what you really want is a `target changed' event.

Andrew

Re: Re-initializing a list after the control returns to gdb...

[Joel Brobecker]

> You've the `observer' problem.

Just like when you are sick, you already feel better when you know which
disease you have contracted :).

> GDB's observer code is very primative.  A combination of gdb-events 
> (needs direction) and chained hooks (see target_new_objfile_hook).
> 
> I suspect what you really want is a `target changed' event.

I need to update the task list every time the inferior stops and
the control returns to the user. I think the `target change' event
is not exactly what I would need?

Do we have a list of currently supported events? Is there a framework
for implementing these events?

-- 
Joel

Re: Re-initializing a list after the control returns to gdb...

[Andrew Cagney]

>> You've the `observer' problem.
> 
> 
> Just like when you are sick, you already feel better when you know which
> disease you have contracted :).
> 
> 
>> GDB's observer code is very primative.  A combination of gdb-events 
>> (needs direction) and chained hooks (see target_new_objfile_hook).
>> 
>> I suspect what you really want is a `target changed' event.
> 
> 
> I need to update the task list every time the inferior stops and
> the control returns to the user. I think the `target change' event
> is not exactly what I would need?

The code relies on global state so I think `target changed' is better - 
that way you know that your state is up-to-date.  From memory, right now 
we've actually memory_changed and registers_changed (I think they should 
be merged).  There is also a target run hook that insight uses.

> Do we have a list of currently supported events? Is there a framework
> for implementing these events?

That's the problem (you said sick), we've three:
- gdb-events
- chained hooks
- simple hooks
gdb-events started on the problem but lost direction.

Andrew

Re: Re-initializing a list after the control returns to gdb...

[Joel Brobecker]

> The code relies on global state so I think `target changed' is better - 
> that way you know that your state is up-to-date.  From memory, right now 
> we've actually memory_changed and registers_changed (I think they should 
> be merged).  There is also a target run hook that insight uses.

Unfortunately `target changed' does not trigger at least every time
I need it to. I looked at the code, and it should basically trigger
everytime we change some data in memory. 

A quick attempt with "set debug event 1" shows that the only events
I see in a simple "break; run" sequence are breakpoint events...

> That's the problem (you said sick), we've three:
> - gdb-events
> - chained hooks
> - simple hooks
> gdb-events started on the problem but lost direction.

I like the events mechanism, it's a paradigm that's used very widely.
But the gdb-event mechanism does not seem to allow several clients
subscribing for these events at the same time. This mechanism is
then very close to the simple hooks.

I am therefore thinking of implementing a new chained hook in place
of the direct call to ada_read_tasks_list. Would that be an acceptable
solution?

-- 
Joel

Re: Re-initializing a list after the control returns to gdb...

[Andrew Cagney]

>> The code relies on global state so I think `target changed' is better - 
>> that way you know that your state is up-to-date.  From memory, right now 
>> we've actually memory_changed and registers_changed (I think they should 
>> be merged).  There is also a target run hook that insight uses.
> 
> 
> Unfortunately `target changed' does not trigger at least every time
> I need it to. I looked at the code, and it should basically trigger
> everytime we change some data in memory. 

Yes.

> A quick attempt with "set debug event 1" shows that the only events
> I see in a simple "break; run" sequence are breakpoint events...
> 
> 
>> That's the problem (you said sick), we've three:
>> - gdb-events
>> - chained hooks
>> - simple hooks
>> gdb-events started on the problem but lost direction.
> 
> 
> I like the events mechanism, it's a paradigm that's used very widely.
> But the gdb-event mechanism does not seem to allow several clients
> subscribing for these events at the same time. This mechanism is
> then very close to the simple hooks.

That's why I used the word `observer'.  When gdb-events was written (ok 
when I hacked it up) I wasn't sure if it should be all (observer) or one 
(as it is now).

In hindsite, it needs to be converted to an observer model (or a new 
observer model introduced and the current gdb-hooks changed to one of 
the many observers).

> I am therefore thinking of implementing a new chained hook in place
> of the direct call to ada_read_tasks_list. Would that be an acceptable
> solution?

Have a look at insight's gdb/gdbtk/generic/gdbtk-hooks.c for what it uses.

Andrew

Re: Re-initializing a list after the control returns to gdb...

[Joel Brobecker]

[Sorry for answering late, got preempted...]

> In hindsite, it needs to be converted to an observer model (or a new 
> observer model introduced and the current gdb-hooks changed to one of 
> the many observers).

One model that works extremely well is the signals system implemented in
glib. I find it very nicely done. It would be nice if we could reuse
this, but this is a very very complex machinery doing probably way too
much for what we need in GDB.

I would instead suggest something much more simple, like this:

   enum notice_kinds
   {
     breakpoint_created_notice,
     breakpoint_deleted_notice,
     ...
     invalid_last_notice,   /* Should always be last.  */
   };

   typedef void (notice_ftype) (void *args, void *data);
   typedef void (free_notice_data) (void *data);
   
   /* Registers NOTICE_CALLBACK for notification when NOTICE_KIND
      events occur.  Returns an id of the registration, which can be
      used later to unregister.  DATA needs to be allocated on the heap,
      as it will be passed to NOTICE_CALLBACK when called.
      If FREE_NOTICE_DATA is not null, then this function will be called
      to free DATA when unsubscribing.  */
   int notices_subscribe (enum notice_kinds notice_kind,
                          notice_ftype *notice_callback,
                          void *data,
                          void *free_notice_data);

   /* Equivalent of:
      notices_subscribe (notice_kind, notice_callback, NULL, NULL).
      FIXME: This is just a convenience function. Do we want to keep it?  */
   int notices_subscribe_no_data (enum notice_kinds notice_kind,
                                  notice_ftype *notice_callback);

   /* Unregister an observer.  */
   void notices_unsubscribe (enum notice_kinds notice_kind, int notice_id);

   /* Send a notice to all the observers who subscribed for the given
      NOTICE_KIND. ARGS is given to the notice callback who should
      not how to decipher it.  */
   void notices_send (enum notice_kinds notice_kind, void *args);

The notices would be more or less implemented as an array of linked
lists of observers.

The DATA parameter is not strictly need in any of the forms of hooks
or events that I have seen so far in GDB's code. This might prove useful
in the future, but we can certainly do without.

This should be fairly easy to implement, and should address all our
current needs. Shall I go ahead and do that?

-- 
Joel (who must have placed his finger at the wrong spot, all I wanted was
to add a hook, but suddently I found myself swallowed in something a
bit bigger :-). 

Re: Re-initializing a list after the control returns to gdb...

[Andrew Cagney]

> [Sorry for answering late, got preempted...]
> 
> 
>> In hindsite, it needs to be converted to an observer model (or a new 
>> observer model introduced and the current gdb-hooks changed to one of 
>> the many observers).
> 
> 
> One model that works extremely well is the signals system implemented in
> glib. I find it very nicely done. It would be nice if we could reuse
> this, but this is a very very complex machinery doing probably way too
> much for what we need in GDB.

Yes.  GDB just needs a single consistent event mechanism.

> I would instead suggest something much more simple, like this:
> 
>    enum notice_kinds
>    {
>      breakpoint_created_notice,
>      breakpoint_deleted_notice,
>      ...
>      invalid_last_notice,   /* Should always be last.  */
>    };

gdb-events.sh used individual wrapper functions rather than trusting an 
enum, a void *, and dodgy casts.  This was to ensure that the external 
iterfaces were all strongly typed (calls checked with -Werror) and all 
nasty casts were burried in the .c file.

It did that right.

It, however, made several mistakes:

- it tried to group all the events together (you've proposed individual 
registration which is much more incremental)

- it didn't implement broadcast (as proposed here)

- `event' in GDB has multiple meanings so the name is confusing.

So, can you get a prototype observer working for just your ada event 
(but keep in mind that other events will be boiler-plated)?

As for the names, just use the terminology found in the patterns book - 
observer.[hc].  observer_update_xxx (or notify_xxx), 
observer_attach_xxx, observer_detach_xxx I think.  If GDB needs more 
than one observer, there's a problem.

(oh, unless you've an immediate need for that data/context parameter, 
i'd leave it out - such a mechanis can be left to the person that 
actually needs)

(shh, doco - but I have a cunning plan - see my recent post about 
revamping gdbarch.sh into doc/gdbarch.texi).

Andrew

Re: Re-initializing a list after the control returns to gdb...

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 2833 bytes --]

> So, can you get a prototype observer working for just your ada event 
> (but keep in mind that other events will be boiler-plated)?
> 
> As for the names, just use the terminology found in the patterns book - 
> observer.[hc].  observer_update_xxx (or notify_xxx), 
> observer_attach_xxx, observer_detach_xxx I think.  If GDB needs more 
> than one observer, there's a problem.

Attached is a prototype that appears to be working. I assumed that you
had in mind an approach were the exported interface is nice and clean
and the implementation uses an underlying generic mechanism with
nasty casts and marshalling/unmarshalling.

While looking more closely at gdb-events, I understood better your
comment about doco and gdbarch.sh. I think that with the current
prototype, it should be relatively easy to generate observer.[hc].

There was another approach that came to my mind when I realized that
observer.[hc] could be generated by a script: instead of using a
internal generic observer, we could simply duplicate the code for
each external observer... Although it does avoid the type casting et al,
I discarded this approach as a bit too brutal and not very elegant. 

I tested this code by using a little test program. I will try not to
forget to attach it. Would there be any way to build this program or
some equivalent somewhere in our testsuite? My (very limited) first test
was also inside GDB itself, where I added a ada-tasks as an observer for
the normal_stop notification, and added a call to
observer_notify_normal_stop in normal_stop.

About the struct observer: the data field is currently used to hold the
pointer to the real observer notification function. So there is no
deallocation issue for now. I tried to make it clear that no
deallocation mechanism has been put in place for this field by adding a
comment next to the field declaration. It can later be extended to also
hold some context data, if needed. However, if/when this happens, we'll
have to extend the attach routine to accept 2 new parameters: a pointer
to the context data, a pointer to a function to deallocate this data.

I also have to admit upfront that choosing names has always been one
of my big weaknesses. I warmly welcome suggestions.

One last thing, I omitted the copyright headers for now. I'll make sure
to add them when I submit the RFA.

> (oh, unless you've an immediate need for that data/context parameter, 
> i'd leave it out - such a mechanis can be left to the person that 
> actually needs)

Cools, that saves a bit of complexity for later, although there isn't
much more needed.

> (shh, doco - but I have a cunning plan - see my recent post about 
> revamping gdbarch.sh into doc/gdbarch.texi).

Right, I will add the proper documentation too. Ideally, having the doco
in an observer.sh script would be great.

-- 
Joel

[-- Attachment #2: observer.h --]
[-- Type: text/plain, Size: 372 bytes --]

#ifndef OBSERVER_H
#define OBSERVER_H

struct observer;

/* normal_stop notifications.  */
typedef void (observer_normal_stop_ftype) (void);

extern struct observer *
  observer_attach_normal_stop (observer_normal_stop_ftype *f);
extern void observer_detach_normal_stop (struct observer *observer);
extern void observer_notify_normal_stop (void);

#endif /* OBSERVER_H */

[-- Attachment #3: observer.c --]
[-- Type: text/plain, Size: 3066 bytes --]

#include "defs.h"
#include "observer.h"

typedef void (generic_observer_notification_ftype) (const void *data,
                                                    const void *args);

struct observer
{
  generic_observer_notification_ftype *notify;
  void *data; /* No memory management needed for this field for now.  */
};

struct observer_list
{
  struct observer_list *next;
  struct observer *observer;
};

static struct observer_list *
xalloc_observer_list_node (void)
{
  struct observer_list *node = XMALLOC (struct observer_list);
  node->observer = XMALLOC (struct observer);
  return node;
}

static void
xfree_observer_list_node (struct observer_list *node)
{
  xfree (node->observer);
  xfree (node);
}

static struct observer *
generic_observer_attach (struct observer_list **list,
                         generic_observer_notification_ftype *notify,
                         void *data)
{
  struct observer_list *observer_list = xalloc_observer_list_node ();

  observer_list->next = *list;
  observer_list->observer->notify = notify;
  observer_list->observer->data = data;
  *list = observer_list;

  return observer_list->observer;
}

static void
generic_observer_detach (struct observer_list **list,
                         const struct observer *observer)
{
  struct observer_list *previous_node = NULL;
  struct observer_list *current_node = *list;

  while (current_node != NULL)
    {
      if (current_node->observer == observer)
        {
          if (previous_node != NULL)
            previous_node->next = current_node->next;
          else
            *list = current_node->next;
          xfree_observer_list_node (current_node);
          return;
        }
      previous_node = current_node;
      current_node = current_node->next;
    }

  /* We should never reach this point.  However, this should not be
     a very serious error, so simply report a warning to the user.  */
  warning ("Failed to detach observer");
}

static void
generic_observer_notify (struct observer_list *list, const void *args)
{
  struct observer_list *current_node = list;

  while (current_node != NULL)
    {
      (*current_node->observer->notify) (current_node->observer->data, args);
      current_node = current_node->next;
    }
}

/* normal_stop notifications.  */

static struct observer_list *normal_stop_observers = NULL;

void
observer_normal_stop_notification_stub (const void *data,
                                        const void *unused_args)
{
  observer_normal_stop_ftype *notify = (observer_normal_stop_ftype *) data;
  (*notify) ();
}

struct observer *
observer_attach_normal_stop (observer_normal_stop_ftype *f)
{
  return generic_observer_attach (&normal_stop_observers, 
                                  &observer_normal_stop_notification_stub,
                                  (void *) f);
}

void
observer_detach_normal_stop (struct observer *observer)
{
  generic_observer_detach (&normal_stop_observers, observer);
}

void
observer_notify_normal_stop (void)
{
  generic_observer_notify (normal_stop_observers, NULL);
}

[-- Attachment #4: test_observer.c --]
[-- Type: text/plain, Size: 3650 bytes --]

#include <stdio.h>

#include "observer.h"

#include <stdlib.h>
void *xmalloc (size_t size) { return malloc (size); }
void xfree (void *ptr) { free (ptr); }
void warning (const char *str, ...) { printf ("warning: %s.\n", str); }

int first_observer = 0;
int second_observer = 0;
int third_observer = 0;

void
reset_counters (void)
{
  first_observer = 0;
  second_observer = 0;
  third_observer = 0;
}

void
check_counters (int first, int second, int third)
{
  if (first_observer != first)
    printf ("ERROR: first observer incorrect count: %d (expected %d).\n",
            first_observer, first);
  if (second_observer != second)
    printf ("ERROR: second observer incorrect count: %d (expected %d).\n",
            second_observer, second);
  if (third_observer != third)
    printf ("ERROR: third observer incorrect count: %d (expected %d).\n",
            third_observer, third);
}

void
test_notifications (int first, int second, int third, char *msg)
{
  printf ("--  Sending notification (%s)...\n", msg);
  reset_counters ();
  observer_notify_normal_stop ();
  check_counters (first, second, third);
}

void
first_observer_notification (void)
{
  first_observer++;
}

void
second_observer_notification (void)
{
  second_observer++;
}

void
third_observer_notification (void)
{
  third_observer++;
}

void
general_observer_test (void)
{
  struct observer *first_obs = NULL;
  struct observer *second_obs = NULL;
  struct observer *third_obs = NULL;

  /* First, try sending a notification without any observers attached.  */
  test_notifications (0, 0, 0, "no observer");

  /* Now, attach one observer, and send a notification.  */
  second_obs = observer_attach_normal_stop (&second_observer_notification);
  test_notifications (0, 1, 0, "one observer");
  
  /* Remove the observer, and send a notification.  */
  observer_detach_normal_stop (second_obs);
  test_notifications (0, 0, 0, "no observer");

  /* With a new observer.  */
  first_obs = observer_attach_normal_stop (&first_observer_notification);
  test_notifications (1, 0, 0, "a new observer");

  /* With 2 observers.  */
  second_obs = observer_attach_normal_stop (&second_observer_notification);
  test_notifications (1, 1, 0, "2 observers");

  /* With 3 observers.  */
  third_obs = observer_attach_normal_stop (&third_observer_notification);
  test_notifications (1, 1, 1, "3 observers");

  /* Remove the middle observer.  */
  observer_detach_normal_stop (second_obs);
  test_notifications (1, 0, 1, "middle observer removed");

  /* Remove first observer.  */
  observer_detach_normal_stop (first_obs);
  test_notifications (0, 0, 1, "first observer removed");

  /* Remove last observer.  */
  observer_detach_normal_stop (third_obs);
  test_notifications (0, 0, 0, "last observer removed");

  /* Go back to 3 observers, and remove them in a different order...  */
  first_obs = observer_attach_normal_stop (&first_observer_notification);
  second_obs = observer_attach_normal_stop (&second_observer_notification);
  third_obs = observer_attach_normal_stop (&third_observer_notification);
  test_notifications (1, 1, 1, "3 observers");

  /* Remove the third observer... */
  observer_detach_normal_stop (third_obs);
  test_notifications (1, 1, 0, "third observer removed");

  /* Remove the second observer... */
  observer_detach_normal_stop (second_obs);
  test_notifications (1, 0, 0, "second observer removed");

  /* Remove the first observer...  No more observers.   */
  observer_detach_normal_stop (first_obs);
  test_notifications (0, 0, 0, "first observer removed - no more observers");

}

int
main (void)
{
  general_observer_test ();
  return 0;
}

Re: Re-initializing a list after the control returns to gdb...

[Andrew Cagney]

> Attached is a prototype that appears to be working. I assumed that you
> had in mind an approach were the exported interface is nice and clean
> and the implementation uses an underlying generic mechanism with
> nasty casts and marshalling/unmarshalling.
> 
> While looking more closely at gdb-events, I understood better your
> comment about doco and gdbarch.sh. I think that with the current
> prototype, it should be relatively easy to generate observer.[hc].

Yes.

gdbarch.[ch] and gdb-events.[ch] both started out as normal files.  It 
is only when they grew beyond a certain point that they turned into 
scripts.  In terms of getting it right, mumble something about `third 
time lucky' :-)

> There was another approach that came to my mind when I realized that
> observer.[hc] could be generated by a script: instead of using a
> internal generic observer, we could simply duplicate the code for
> each external observer... Although it does avoid the type casting et al,
> I discarded this approach as a bit too brutal and not very elegant. 

It's an internal implementation detail.  Provided the code is 
`interface' and `coding standard' compliant (doing no externally visible 
evil, indented using gdb_indent.sh, gets past gdb_ari.sh -Wari) can any 
one really complain about how version 1.0 is implemented using wire and 
bits of string :-)

> I tested this code by using a little test program. I will try not to
> forget to attach it. Would there be any way to build this program or
> some equivalent somewhere in our testsuite? My (very limited) first test
> was also inside GDB itself, where I added a ada-tasks as an observer for
> the normal_stop notification, and added a call to
> observer_notify_normal_stop in normal_stop.

The testsuite/gdb.gdb directory was created for just this purpose!

> About the struct observer: the data field is currently used to hold the
> pointer to the real observer notification function. So there is no
> deallocation issue for now. I tried to make it clear that no
> deallocation mechanism has been put in place for this field by adding a
> comment next to the field declaration. It can later be extended to also
> hold some context data, if needed. However, if/when this happens, we'll
> have to extend the attach routine to accept 2 new parameters: a pointer
> to the context data, a pointer to a function to deallocate this data.
> 
> I also have to admit upfront that choosing names has always been one
> of my big weaknesses. I warmly welcome suggestions.

Ditto.  That is why I suggested stealing terms from the patterns book - 
you get to blame the decision on someone else.

> One last thing, I omitted the copyright headers for now. I'll make sure
> to add them when I submit the RFA.
> 
> 
>> (oh, unless you've an immediate need for that data/context parameter, 
>> i'd leave it out - such a mechanis can be left to the person that 
>> actually needs)
> 
> 
> Cools, that saves a bit of complexity for later, although there isn't
> much more needed.
> 
> 
>> (shh, doco - but I have a cunning plan - see my recent post about 
>> revamping gdbarch.sh into doc/gdbarch.texi).
> 
> 
> Right, I will add the proper documentation too. Ideally, having the doco
> in an observer.sh script would be great.

I was thinking of something like:

Chapter Observing changes in @value{GDBN} internals
Light highlevel commentary largely refering the reader to the patterns book

Appendix `observer' (observer.texi)
@defun void normal_stop (void)
Normal stop notification.
@end defun

observer.sh could then munge that into your file.

Can you please add the missing (C) notice and then commit this part 
(adding it to the build & makefile).

Then slap a summary of our `grand plans' somewhere in the header.

Andrew


> #ifndef OBSERVER_H
> #define OBSERVER_H
> 
> struct observer;
> 
> /* normal_stop notifications.  */
> typedef void (observer_normal_stop_ftype) (void);
> 
> extern struct observer *
>   observer_attach_normal_stop (observer_normal_stop_ftype *f);
> extern void observer_detach_normal_stop (struct observer *observer);
> extern void observer_notify_normal_stop (void);
> 
> #endif /* OBSERVER_H */
> 
> 
> 
> #include "defs.h"
> #include "observer.h"
> 
> typedef void (generic_observer_notification_ftype) (const void *data,
>                                                     const void *args);
> 
> struct observer
> {
>   generic_observer_notification_ftype *notify;
>   void *data; /* No memory management needed for this field for now.  */
> };
> 
> struct observer_list
> {
>   struct observer_list *next;
>   struct observer *observer;
> };
> 
> static struct observer_list *
> xalloc_observer_list_node (void)
> {
>   struct observer_list *node = XMALLOC (struct observer_list);
>   node->observer = XMALLOC (struct observer);
>   return node;
> }
> 
> static void
> xfree_observer_list_node (struct observer_list *node)
> {
>   xfree (node->observer);
>   xfree (node);
> }
> 
> static struct observer *
> generic_observer_attach (struct observer_list **list,
>                          generic_observer_notification_ftype *notify,
>                          void *data)
> {
>   struct observer_list *observer_list = xalloc_observer_list_node ();
> 
>   observer_list->next = *list;
>   observer_list->observer->notify = notify;
>   observer_list->observer->data = data;
>   *list = observer_list;
> 
>   return observer_list->observer;
> }
> 
> static void
> generic_observer_detach (struct observer_list **list,
>                          const struct observer *observer)
> {
>   struct observer_list *previous_node = NULL;
>   struct observer_list *current_node = *list;
> 
>   while (current_node != NULL)
>     {
>       if (current_node->observer == observer)
>         {
>           if (previous_node != NULL)
>             previous_node->next = current_node->next;
>           else
>             *list = current_node->next;
>           xfree_observer_list_node (current_node);
>           return;
>         }
>       previous_node = current_node;
>       current_node = current_node->next;
>     }
> 
>   /* We should never reach this point.  However, this should not be
>      a very serious error, so simply report a warning to the user.  */
>   warning ("Failed to detach observer");
> }
> 
> static void
> generic_observer_notify (struct observer_list *list, const void *args)
> {
>   struct observer_list *current_node = list;
> 
>   while (current_node != NULL)
>     {
>       (*current_node->observer->notify) (current_node->observer->data, args);
>       current_node = current_node->next;
>     }
> }
> 
> /* normal_stop notifications.  */
> 
> static struct observer_list *normal_stop_observers = NULL;
> 
> void
> observer_normal_stop_notification_stub (const void *data,
>                                         const void *unused_args)
> {
>   observer_normal_stop_ftype *notify = (observer_normal_stop_ftype *) data;
>   (*notify) ();
> }
> 
> struct observer *
> observer_attach_normal_stop (observer_normal_stop_ftype *f)
> {
>   return generic_observer_attach (&normal_stop_observers, 
>                                   &observer_normal_stop_notification_stub,
>                                   (void *) f);
> }
> 
> void
> observer_detach_normal_stop (struct observer *observer)
> {
>   generic_observer_detach (&normal_stop_observers, observer);
> }
> 
> void
> observer_notify_normal_stop (void)
> {
>   generic_observer_notify (normal_stop_observers, NULL);
> }
> 
> 
> 
> #include <stdio.h>
> 
> #include "observer.h"
> 
> #include <stdlib.h>
> void *xmalloc (size_t size) { return malloc (size); }
> void xfree (void *ptr) { free (ptr); }
> void warning (const char *str, ...) { printf ("warning: %s.\n", str); }
> 
> int first_observer = 0;
> int second_observer = 0;
> int third_observer = 0;
> 
> void
> reset_counters (void)
> {
>   first_observer = 0;
>   second_observer = 0;
>   third_observer = 0;
> }
> 
> void
> check_counters (int first, int second, int third)
> {
>   if (first_observer != first)
>     printf ("ERROR: first observer incorrect count: %d (expected %d).\n",
>             first_observer, first);
>   if (second_observer != second)
>     printf ("ERROR: second observer incorrect count: %d (expected %d).\n",
>             second_observer, second);
>   if (third_observer != third)
>     printf ("ERROR: third observer incorrect count: %d (expected %d).\n",
>             third_observer, third);
> }
> 
> void
> test_notifications (int first, int second, int third, char *msg)
> {
>   printf ("--  Sending notification (%s)...\n", msg);
>   reset_counters ();
>   observer_notify_normal_stop ();
>   check_counters (first, second, third);
> }
> 
> void
> first_observer_notification (void)
> {
>   first_observer++;
> }
> 
> void
> second_observer_notification (void)
> {
>   second_observer++;
> }
> 
> void
> third_observer_notification (void)
> {
>   third_observer++;
> }
> 
> void
> general_observer_test (void)
> {
>   struct observer *first_obs = NULL;
>   struct observer *second_obs = NULL;
>   struct observer *third_obs = NULL;
> 
>   /* First, try sending a notification without any observers attached.  */
>   test_notifications (0, 0, 0, "no observer");
> 
>   /* Now, attach one observer, and send a notification.  */
>   second_obs = observer_attach_normal_stop (&second_observer_notification);
>   test_notifications (0, 1, 0, "one observer");
>   
>   /* Remove the observer, and send a notification.  */
>   observer_detach_normal_stop (second_obs);
>   test_notifications (0, 0, 0, "no observer");
> 
>   /* With a new observer.  */
>   first_obs = observer_attach_normal_stop (&first_observer_notification);
>   test_notifications (1, 0, 0, "a new observer");
> 
>   /* With 2 observers.  */
>   second_obs = observer_attach_normal_stop (&second_observer_notification);
>   test_notifications (1, 1, 0, "2 observers");
> 
>   /* With 3 observers.  */
>   third_obs = observer_attach_normal_stop (&third_observer_notification);
>   test_notifications (1, 1, 1, "3 observers");
> 
>   /* Remove the middle observer.  */
>   observer_detach_normal_stop (second_obs);
>   test_notifications (1, 0, 1, "middle observer removed");
> 
>   /* Remove first observer.  */
>   observer_detach_normal_stop (first_obs);
>   test_notifications (0, 0, 1, "first observer removed");
> 
>   /* Remove last observer.  */
>   observer_detach_normal_stop (third_obs);
>   test_notifications (0, 0, 0, "last observer removed");
> 
>   /* Go back to 3 observers, and remove them in a different order...  */
>   first_obs = observer_attach_normal_stop (&first_observer_notification);
>   second_obs = observer_attach_normal_stop (&second_observer_notification);
>   third_obs = observer_attach_normal_stop (&third_observer_notification);
>   test_notifications (1, 1, 1, "3 observers");
> 
>   /* Remove the third observer... */
>   observer_detach_normal_stop (third_obs);
>   test_notifications (1, 1, 0, "third observer removed");
> 
>   /* Remove the second observer... */
>   observer_detach_normal_stop (second_obs);
>   test_notifications (1, 0, 0, "second observer removed");
> 
>   /* Remove the first observer...  No more observers.   */
>   observer_detach_normal_stop (first_obs);
>   test_notifications (0, 0, 0, "first observer removed - no more observers");
> 
> }
> 
> int
> main (void)
> {
>   general_observer_test ();
>   return 0;
> }

Re: Re-initializing a list after the control returns to gdb...

[Joel Brobecker]

> Can you please add the missing (C) notice and then commit this part 
> (adding it to the build & makefile).

Ok. I checked these files in, after having made some minor
modifications, added the headers, some doco, etc. See
http://sources.redhat.com/ml/gdb-patches/2003-02/msg00803.html
for slightly more details (and the patch).


> The testsuite/gdb.gdb directory was created for just this purpose!

Ok, will take care of that part in a separate RFA.

> >Right, I will add the proper documentation too. Ideally, having the doco
> >in an observer.sh script would be great.
> 
> I was thinking of something like:
> 
> Chapter Observing changes in @value{GDBN} internals
> Light highlevel commentary largely refering the reader to the patterns book
> 
> Appendix `observer' (observer.texi)
> @defun void normal_stop (void)
> Normal stop notification.
> @end defun

I will add some light documentation in gdbint.texi (yet another RFA).

-- 
Joel