[RFC] Replace deprecated_target_new_objfile_hook by observer?

[RFC] Replace deprecated_target_new_objfile_hook by observer?

[Joel Brobecker]

Hello,

While working on the problem described in

        http://www.sourceware.org/ml/gdb/2006-09/msg00065.html

one of the things I noticed was that we're using a hook where
each client that wants to be notified inserts himself. The way
it is done is through cooperation, like so:

  1. Declare a static variable:
  
        static void (*target_new_objfile_chain)(struct objfile *);

  2. During the module initialization, store the current value
     of that hook into our static variable, and replace its
     value with our own callback.

        /* Notice when object files get loaded and unloaded.  */
          target_new_objfile_chain = deprecated_target_new_objfile_hook;
          deprecated_target_new_objfile_hook = new_objfile;

  3. At the end of our callback code, check our static variable
     and call the previous client:

        /* deprecated_target_new_objfile_hook callback.
           [snip]  */
        
        static void
        new_objfile (struct objfile *objfile)
        {
          [snip]

          if (target_new_objfile_chain)
            target_new_objfile_chain (objfile);
        }

I propose to replace this with an observer. Would that be OK?

Assuming that it is, there are several platforms that use that
mechanism. It's going to be hard to test all of them. But the changes
themselves should be pretty mechanical. So what I can propose is
to make all the necessary changes to replace that hook model with
an approach using observers and test on x86-linux. And then rely
on the mechanical aspect of the change together with the review of
another pair of eyes. Would that be OK?

The alternative is to have the two approaches coexist for a while.
I just don't think this is necessary, because I don't think the change
is that risky.

-- 
Joel

Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Daniel Jacobowitz]

On Tue, Oct 17, 2006 at 07:32:17PM -0400, Joel Brobecker wrote:
> I propose to replace this with an observer. Would that be OK?

There Be Dragons!  Note that there is a case where the remote objfile
hook does _not_ call the next thing on the chain.  This is somewhat
deliberate, in that it's what prevents thread-db from being enabled
when talking to gdbserver, and somewhat accidental, in that I'm sure it
wasn't meant to work this way.  But we'll need to do some rearranging
in order to keep the current state of affairs working.

> Assuming that it is, there are several platforms that use that
> mechanism. It's going to be hard to test all of them. But the changes
> themselves should be pretty mechanical. So what I can propose is
> to make all the necessary changes to replace that hook model with
> an approach using observers and test on x86-linux. And then rely
> on the mechanical aspect of the change together with the review of
> another pair of eyes. Would that be OK?

Isn't there already an adequate observer?  Well, not a single one I
guess, you'd have to survey the uses and see what they really wanted.
But solib_loaded goes a long way.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Joel Brobecker]

> There Be Dragons!  Note that there is a case where the remote objfile
> hook does _not_ call the next thing on the chain.

Is this then one instance you were mentioning (remote.c)?

  /* Call predecessor on chain, if any.  */
  if (remote_new_objfile_chain != 0 &&
      remote_desc == 0)
    remote_new_objfile_chain (objfile);

I'm trying to understand how this is supposed to work, since
my understanding is that the order in which _init routines are
called is not guarantied. So the above breaks the chain, but we
just don't know where...

> > Assuming that it is, there are several platforms that use that
> > mechanism. It's going to be hard to test all of them. But the changes
> > themselves should be pretty mechanical. So what I can propose is
> > to make all the necessary changes to replace that hook model with
> > an approach using observers and test on x86-linux. And then rely
> > on the mechanical aspect of the change together with the review of
> > another pair of eyes. Would that be OK?
> 
> Isn't there already an adequate observer?  Well, not a single one I
> guess, you'd have to survey the uses and see what they really wanted.
> But solib_loaded goes a long way.

Even better, but that would make the change a little less mechanical
because the current solib_loaded observer has a different profile...
Perhaps I can reconcile the two - I'll have a look at that tomorrow.

-- 
Joel

Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Daniel Jacobowitz]

On Tue, Oct 17, 2006 at 08:20:21PM -0400, Joel Brobecker wrote:
> Is this then one instance you were mentioning (remote.c)?
> 
>   /* Call predecessor on chain, if any.  */
>   if (remote_new_objfile_chain != 0 &&
>       remote_desc == 0)
>     remote_new_objfile_chain (objfile);
> 
> I'm trying to understand how this is supposed to work, since
> my understanding is that the order in which _init routines are
> called is not guarantied. So the above breaks the chain, but we
> just don't know where...

Luck works it out - they're sorted by function name, IIRC, and it's
still called _initialize_thread_db ().

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> There Be Dragons!  Note that there is a case where the remote objfile
> hook does _not_ call the next thing on the chain.  This is somewhat
> deliberate, in that it's what prevents thread-db from being enabled
> when talking to gdbserver, and somewhat accidental, in that I'm sure it
> wasn't meant to work this way.  But we'll need to do some rearranging
> in order to keep the current state of affairs working.

I've run into this in my work on GDB support for the Cell BE (which is
getting a lot closer to a state where we can try to submit it, b.t.w.).

We're working on providing code overlay support for the Cell SPEs, and
to support this in the debugger, I needed to hook into the objfile chain;
I ran into exactly the problem you describe above.

To fix this, I'm currently using this patch, which attempts to express
more directly that linux-thread-db should not be used if the target is
remote:

diff -urN gdb-orig/gdb/linux-thread-db.c gdb-6.5/gdb/linux-thread-db.c
--- gdb-orig/gdb/linux-thread-db.c	2006-05-06 00:42:43.000000000 +0200
+++ gdb-6.5/gdb/linux-thread-db.c	2006-09-25 02:12:11.382901136 +0200
@@ -669,6 +669,10 @@
   if (!target_has_execution)
     return;
 
+  /* Don't attempt to use thread_db for remote targets.  */
+  if (!target_can_run (&current_target))
+    return;
+
   /* Initialize the structure that identifies the child process.  */
   proc_handle.pid = GET_PID (inferior_ptid);
 
diff -urN gdb-orig/gdb/remote.c gdb-6.5/gdb/remote.c
--- gdb-orig/gdb/remote.c	2006-05-05 22:08:45.000000000 +0200
+++ gdb-6.5/gdb/remote.c	2006-09-25 02:12:11.449890952 +0200
@@ -5466,8 +5466,7 @@
       remote_check_symbols (objfile);
     }
   /* Call predecessor on chain, if any.  */
-  if (remote_new_objfile_chain != 0 &&
-      remote_desc == 0)
+  if (remote_new_objfile_chain)
     remote_new_objfile_chain (objfile);
 }
 
Does this look reasonable?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com

Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Daniel Jacobowitz]

On Wed, Oct 18, 2006 at 06:29:02PM +0200, Ulrich Weigand wrote:
> +  /* Don't attempt to use thread_db for remote targets.  */
> +  if (!target_can_run (&current_target))
> +    return;
> +

That's clever!  I hadn't thought of using to_can_run that way.  I think
that should be fine.  If we want to restructure to_can_run later, then
it's easy enough to change.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Daniel Jacobowitz]

On Wed, Oct 18, 2006 at 10:06:43AM -0700, Jim Ingham wrote:
> Isn't the initialization order given by the order of .o files listed  
> in COMMON_OBS in Makefile.in?  So if you need to enforce a particular  
> order of initialization, you can adjust the ordering in this list.

Looks like you're right.  But, still, let's just not go there.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Joel Brobecker]

Ulrich,

> To fix this, I'm currently using this patch, which attempts to express
> more directly that linux-thread-db should not be used if the target is
> remote:

Would you mind submitting your change for inclusion? I could then
leverage off of this change for the transition to observers.

Thanks!

> 
> diff -urN gdb-orig/gdb/linux-thread-db.c gdb-6.5/gdb/linux-thread-db.c
> --- gdb-orig/gdb/linux-thread-db.c	2006-05-06 00:42:43.000000000 +0200
> +++ gdb-6.5/gdb/linux-thread-db.c	2006-09-25 02:12:11.382901136 +0200
> @@ -669,6 +669,10 @@
>    if (!target_has_execution)
>      return;
>  
> +  /* Don't attempt to use thread_db for remote targets.  */
> +  if (!target_can_run (&current_target))
> +    return;
> +
>    /* Initialize the structure that identifies the child process.  */
>    proc_handle.pid = GET_PID (inferior_ptid);
>  
> diff -urN gdb-orig/gdb/remote.c gdb-6.5/gdb/remote.c
> --- gdb-orig/gdb/remote.c	2006-05-05 22:08:45.000000000 +0200
> +++ gdb-6.5/gdb/remote.c	2006-09-25 02:12:11.449890952 +0200
> @@ -5466,8 +5466,7 @@
>        remote_check_symbols (objfile);
>      }
>    /* Call predecessor on chain, if any.  */
> -  if (remote_new_objfile_chain != 0 &&
> -      remote_desc == 0)
> +  if (remote_new_objfile_chain)
>      remote_new_objfile_chain (objfile);
>  }
>  
> Does this look reasonable?
> 
> Bye,
> Ulrich
> 
> -- 
>   Dr. Ulrich Weigand
>   Linux on zSeries Development
>   Ulrich.Weigand@de.ibm.com

-- 
Joel

[PATCH] Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Ulrich Weigand]

Joel Brobecker wrote:

> Would you mind submitting your change for inclusion? I could then
> leverage off of this change for the transition to observers.

Sure, here goes the patch again.

Tested on s390-ibm-linux and s390x-ibm-linux.
OK for mainline?

Bye,
Ulrich


ChangeLog:

        * linux-thread-db.c (check_for_thread_db): Don't attempt to use
        thread_db for remote targets.
        * remote.c (remote_new_objfile): Always call predecessor on
        new_objfile event chain.

Index: gdb/linux-thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread-db.c,v
retrieving revision 1.20
diff -u -p -r1.20 linux-thread-db.c
--- gdb/linux-thread-db.c	15 Oct 2006 19:38:45 -0000	1.20
+++ gdb/linux-thread-db.c	19 Oct 2006 20:08:44 -0000
@@ -599,6 +599,10 @@ check_for_thread_db (void)
   if (!target_has_execution)
     return;
 
+  /* Don't attempt to use thread_db for remote targets.  */
+  if (!target_can_run (&current_target))
+    return;
+
   /* Initialize the structure that identifies the child process.  */
   proc_handle.pid = GET_PID (inferior_ptid);
 
Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.234
diff -u -p -r1.234 remote.c
--- gdb/remote.c	18 Oct 2006 16:56:13 -0000	1.234
+++ gdb/remote.c	19 Oct 2006 20:08:44 -0000
@@ -6126,8 +6126,7 @@ remote_new_objfile (struct objfile *objf
       remote_check_symbols (objfile);
     }
   /* Call predecessor on chain, if any.  */
-  if (remote_new_objfile_chain != 0 &&
-      remote_desc == 0)
+  if (remote_new_objfile_chain)
     remote_new_objfile_chain (objfile);
 }
 


-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Daniel Jacobowitz]

On Fri, Oct 20, 2006 at 02:41:38AM +0200, Ulrich Weigand wrote:
> Joel Brobecker wrote:
> 
> > Would you mind submitting your change for inclusion? I could then
> > leverage off of this change for the transition to observers.
> 
> Sure, here goes the patch again.
> 
> Tested on s390-ibm-linux and s390x-ibm-linux.
> OK for mainline?

Yes.  Thanks!

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Fri, Oct 20, 2006 at 02:41:38AM +0200, Ulrich Weigand wrote:
> > OK for mainline?
> 
> Yes.  Thanks!

Committed.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com

Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Ulrich Weigand]

Joel Brobecker wrote:

> > To fix this, I'm currently using this patch, which attempts to express
> > more directly that linux-thread-db should not be used if the target is
> > remote:
> 
> Would you mind submitting your change for inclusion? I could then
> leverage off of this change for the transition to observers.

Did you get around to implementing the observer transition?  If not,
I'll have a go at it, as I'm working on another patch that need to 
install a new objfile hook, and I'd prefer not to add another instance
of the deprecated feature ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] Replace deprecated_target_new_objfile_hook by observer?

[Joel Brobecker]

Hello Ulrich,

> Did you get around to implementing the observer transition?  If not,
> I'll have a go at it, as I'm working on another patch that need to 
> install a new objfile hook, and I'd prefer not to add another instance
> of the deprecated feature ...

No, I haven't had time to work on this, and I'm afraid I won't have
much time in the near future, so I'll be very glad if you take this
over.

-- 
Joel