Re: [PATCH RFA] process/thread/lwp identifier mega-patch

Re: [PATCH RFA] process/thread/lwp identifier mega-patch

[Andrew Cagney]

Kevin Buettner wrote:
> [...]

Kevin,

One thought on this (prompted by a side conversation).  From memory you
had concerns with your memory [groan] (i.e. memory leaks).

Have you considered ignoring the problem?  Well actually just
accumulating a list of all the created threads and then, when GDB
re-starts a target, deleting the lot?  Yes, this will clearly not scale
well in an application that creates then deletes millions of threads. 
Hopefully though, the benefits (such as improved performance) of having
per thread objects will far out way this.

I suspect that the way GDB currently tries to delete threads is
technically flawed - it only worked because there was really only one
thread.  A better more robust model needs to be developed but I don't
think solving that problem should be part of this patch.  The person
with the million threads can solve that one :-)

	Andrew

Re: [PATCH RFA] process/thread/lwp identifier mega-patch

[Kevin Buettner]

On Feb 16,  9:24am, Andrew Cagney wrote:

> One thought on this (prompted by a side conversation).  From memory you
> had concerns with your memory [groan] (i.e. memory leaks).
> 
> Have you considered ignoring the problem?  Well actually just
> accumulating a list of all the created threads and then, when GDB
> re-starts a target, deleting the lot?  Yes, this will clearly not scale
> well in an application that creates then deletes millions of threads. 
> Hopefully though, the benefits (such as improved performance) of having
> per thread objects will far out way this.
> 
> I suspect that the way GDB currently tries to delete threads is
> technically flawed - it only worked because there was really only one
> thread.  A better more robust model needs to be developed but I don't
> think solving that problem should be part of this patch.  The person
> with the million threads can solve that one :-)

I think that your suggestion could be made to work, though it won't
be as simple as merely wiping out the thread list when GDB restarts
a target.  The reason?  Dangling pointers in static globals.

E.g, in infrun.c, we have the following declaration:

	static int static int previous_inferior_pid;

My patches change this declaration to:

	static struct ptid *previous_inferior_ptid;

We would need to make sure this (and other static globals) are
reinitialized when the thread list is wiped out.

Another alternative is to make the execution context identifiers (or
ECIs for short) ``struct ptid'' instead of ``struct ptid *''.  I.e,
make the ECI a struct instead of a pointer to a struct.  The problem
with doing this is that the ECI's type can no longer be opaque.

One can argue that if GDB accesses a defunct ECI (regardless of
implementation) at all, it is behaving incorrectly, because this
behavior is wrong regardless of whether the ECI is a struct or a
dangling pointer.  It's just that it could be catastrophic if it's the
latter...

So maybe it'd be best if we make sure that each and every ECI
occurence in the code is initialized properly when the thread list is
cleaned up.  (In other words, I'm coming around to liking your
suggestion again...)

Kevin

Re: [PATCH RFA] process/thread/lwp identifier mega-patch

[Andrew Cagney]

Kevin Buettner wrote:
> 

> > Have you considered ignoring the problem?  Well actually just
> > accumulating a list of all the created threads and then, when GDB
> > re-starts a target, deleting the lot?  Yes, this will clearly not scale
> > well in an application that creates then deletes millions of threads.
> > Hopefully though, the benefits (such as improved performance) of having
> > per thread objects will far out way this.

> I think that your suggestion could be made to work, though it won't
> be as simple as merely wiping out the thread list when GDB restarts
> a target.  The reason?  Dangling pointers in static globals.
> 
> E.g, in infrun.c, we have the following declaration:
> 
>         static int static int previous_inferior_pid;

ARRG!

> My patches change this declaration to:
> 
>         static struct ptid *previous_inferior_ptid;
> 
> We would need to make sure this (and other static globals) are
> reinitialized when the thread list is wiped out.

Really nasty would be to enter each of those globals into a database and
trash them at the same time as the thread pool is trashed.

It might even be a tolerable workaround since those globals will
eventually need to be deleted.

> Another alternative is to make the execution context identifiers (or
> ECIs for short) ``struct ptid'' instead of ``struct ptid *''.  I.e,
> make the ECI a struct instead of a pointer to a struct.  The problem
> with doing this is that the ECI's type can no longer be opaque.

Again as an imtermediate step yes.

> One can argue that if GDB accesses a defunct ECI (regardless of
> implementation) at all, it is behaving incorrectly, because this
> behavior is wrong regardless of whether the ECI is a struct or a
> dangling pointer.  It's just that it could be catastrophic if it's the
> latter...
> 
> So maybe it'd be best if we make sure that each and every ECI
> occurence in the code is initialized properly when the thread list is
> cleaned up.  (In other words, I'm coming around to liking your
> suggestion again...)

You should probably look carefully at
http://sources.redhat.com/ml/gdb/2001-02/msg00210.html .  In that
diagram, ``context'' roughly correspond to ``struct ptid *''.

	Andrew

Re: [PATCH RFA] process/thread/lwp identifier mega-patch

[Kevin Buettner]

On Feb 16,  5:14pm, Andrew Cagney wrote:

> > E.g, in infrun.c, we have the following declaration:
> > 
> >         static int static int previous_inferior_pid;
> 
> ARRG!
> 
> > My patches change this declaration to:
> > 
> >         static struct ptid *previous_inferior_ptid;
> > 
> > We would need to make sure this (and other static globals) are
> > reinitialized when the thread list is wiped out.
> 
> Really nasty would be to enter each of those globals into a database and
> trash them at the same time as the thread pool is trashed.

That's what I had in mind.  I'd make the _initialize_* functions
responsible for registering the various static globals in a simple
database (probably just a linked list).

> It might even be a tolerable workaround since those globals will
> eventually need to be deleted.

Right.

I seem to recall that there were some other data structures which
might need to be reinitialized as well.  (The thread list comes
to mind; OTOH, since we're wiping the threads anyway, this might
not be a problem.  But I think there might've been others as well.
I'll need to revisit the code to be sure.)

> > Another alternative is to make the execution context identifiers (or
> > ECIs for short) ``struct ptid'' instead of ``struct ptid *''.  I.e,
> > make the ECI a struct instead of a pointer to a struct.  The problem
> > with doing this is that the ECI's type can no longer be opaque.
> 
> Again as an imtermediate step yes.

Hmmm... in some respects, I really prefer this route.  Now if I
could just get you to agree to using a typedef, I could do the
following:

    struct ptid			/* Alas, not opaque... */
      {
        ...
      };
    typedef struct ptid ptid;

The code would then be changed to use ``ptid'' everywhere that
``struct ptid *'' currently appears (in my patch).

Later on, when we're ready to move to using a pointer to a struct,
we'll be able to use something along the following lines:

    struct ptid;		/* Now struct ptid is opaque */
    typedef struct ptid *ptid;

The nice thing about this is that very little other code would need
to change.  (Just the accessors and constructors.)

But I seem to recall that you had a problem with typedef...

> > One can argue that if GDB accesses a defunct ECI (regardless of
> > implementation) at all, it is behaving incorrectly, because this
> > behavior is wrong regardless of whether the ECI is a struct or a
> > dangling pointer.  It's just that it could be catastrophic if it's the
> > latter...
> > 
> > So maybe it'd be best if we make sure that each and every ECI
> > occurence in the code is initialized properly when the thread list is
> > cleaned up.  (In other words, I'm coming around to liking your
> > suggestion again...)
> 
> You should probably look carefully at
> http://sources.redhat.com/ml/gdb/2001-02/msg00210.html .  In that
> diagram, ``context'' roughly correspond to ``struct ptid *''.

Sort of.  I have the feeling that I'm just quibbling about
terminology, but at the moment I would call ``struct ptid *'' a
context identifier since it contains nothing more than the identifiers
which may be used to refer to a context.  It is certainly the case
that we could add members to struct ptid (or maybe just use struct
thread_info) to (more) fully represent a context.

Kevin

Re: [PATCH RFA] process/thread/lwp identifier mega-patch

[Andrew Cagney]

> Later on, when we're ready to move to using a pointer to a struct,
> we'll be able to use something along the following lines:
> 
>     struct ptid;                /* Now struct ptid is opaque */
>     typedef struct ptid *ptid;
> 
> The nice thing about this is that very little other code would need
> to change.  (Just the accessors and constructors.)
> 
> But I seem to recall that you had a problem with typedef...

Try declaring something like:

    xyz.h:
	struct xyz;
	exter void xyz_foo (struct xyz *self, ...);

    abc.h:
	struct xyz;
	struct abc;
	extern abc_on_xyz (struct abc *self, struct xyz *on);

using typedefs.  It ends up creating include spaghetti :-(

	enjoy,
		Andrew

Re: [PATCH RFA] process/thread/lwp identifier mega-patch

[Michael Elizabeth Chastain]

Hi Andrew,

How about:

  typedef struct xyz xyz_t;
  typedef struct abc abc_t;

  extern void abc_on_xyz (abc_t *self, xyz_t *on);

?

It compiles for me with "gcc -ansi -pedantic -Wall" and also with
/usr/ucb/cc on a Solaris 2.6 system.  That's all the compilers I have
access to.

Michael