TYPE_NAME memory management

TYPE_NAME memory management

[Doug Evans]

On Fri, Sep 4, 2009 at 11:03 AM, Doug Evans<dje@google.com> wrote:
> On Fri, Sep 4, 2009 at 10:59 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:
>> On Fri, Sep 4, 2009 at 8:25 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:
>>
>>> I am still working on a stand-alone repro case.
>>
>> Here it is:
>>
>>[...]
>> Segmentation fault (core dumped)
>>
>> I will not be able to work on a fix before next Tuesday, so if anybody
>> fixes this before then, please let me know.

Memory management for TYPE_NAME and TYPE_TAG_NAME is a bit random.

E.g.
bash$ grep "TYPE_NAME *(.*) *= " *.c
bash$ grep "TYPE_TAG_NAME *(.*) *= " *.c

Sometimes it's a string constant, sometimes it's in malloc space,
sometimes it's on objfile's obstack, and now sometimes it can live in
mmap'd space.

Obviously one would rather not place ordering constraints on objfile
data cleanups.  All the above uses are "ok" (modulo any memory leaks
from malloc'd strings) except for the new mmap'd values, so it seems
like the thing to do for now is copy such strings onto the objfile's
obstack.
I'm not sure what the speed loss will be, but I think it's the thing
to do pending data that says something more clever is needed.

Re: TYPE_NAME memory management

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> Memory management for TYPE_NAME and TYPE_TAG_NAME is a bit random.

Doug> Sometimes it's a string constant, sometimes it's in malloc space,
Doug> sometimes it's on objfile's obstack, and now sometimes it can live in
Doug> mmap'd space.

Doug> Obviously one would rather not place ordering constraints on objfile
Doug> data cleanups.  All the above uses are "ok" (modulo any memory leaks
Doug> from malloc'd strings) except for the new mmap'd values, so it seems
Doug> like the thing to do for now is copy such strings onto the objfile's
Doug> obstack.
Doug> I'm not sure what the speed loss will be, but I think it's the thing
Doug> to do pending data that says something more clever is needed.

My understanding is that in the past the rule was that if a type had an
objfile, then the type name could come directly from the debuginfo
(allocated on the objfile's obstack), because GDB made a guarantee about
the relative lifetimes of these objects.  In particular, types were
copied by preserve_one_value at a point where the string data was still
live.

Why can't we maintain that guarantee for mmap'd debuginfo as well?

I realize that having a lot of lifetime dependencies can be tricky.
But, this one is fairly well established already.

For objfile-less types, I suspect we ought to always malloc any
associated strings.  That will let us avoid memory leaks once the type
GC work is completed.  (Currently I don't think we ever free such
types.)

Tom

Re: TYPE_NAME memory management

[Doug Evans]

On Fri, Sep 4, 2009 at 3:41 PM, Tom Tromey<tromey@redhat.com> wrote:
>
> My understanding is that in the past the rule was that if a type had an
> objfile, then the type name could come directly from the debuginfo
> (allocated on the objfile's obstack), because GDB made a guarantee about
> the relative lifetimes of these objects.  In particular, types were
> copied by preserve_one_value at a point where the string data was still
> live.
>
> Why can't we maintain that guarantee for mmap'd debuginfo as well?

Oh, we can.  It's fairly straightforward.  I just wanted to make sure
I was on solid ground.

> I realize that having a lot of lifetime dependencies can be tricky.
> But, this one is fairly well established already.

Thanks.

> For objfile-less types, I suspect we ought to always malloc any
> associated strings.  That will let us avoid memory leaks once the type
> GC work is completed.  (Currently I don't think we ever free such
> types.)

Re: TYPE_NAME memory management

[Joel Brobecker]

> Oh, we can.  It's fairly straightforward.  I just wanted to make sure
> I was on solid ground.

For some reason, I thought that the mmap'ed memory could(/should?) not
be kept in memory... (I thought it was a file-backed mmap)

-- 
Joel

Re: TYPE_NAME memory management

[Doug Evans]

On Fri, Sep 4, 2009 at 3:41 PM, Tom Tromey<tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> Memory management for TYPE_NAME and TYPE_TAG_NAME is a bit random.
>
> Doug> Sometimes it's a string constant, sometimes it's in malloc space,
> Doug> sometimes it's on objfile's obstack, and now sometimes it can live in
> Doug> mmap'd space.
>
> Doug> Obviously one would rather not place ordering constraints on objfile
> Doug> data cleanups.  All the above uses are "ok" (modulo any memory leaks
> Doug> from malloc'd strings) except for the new mmap'd values, so it seems
> Doug> like the thing to do for now is copy such strings onto the objfile's
> Doug> obstack.
> Doug> I'm not sure what the speed loss will be, but I think it's the thing
> Doug> to do pending data that says something more clever is needed.
>
> My understanding is that in the past the rule was that if a type had an
> objfile, then the type name could come directly from the debuginfo
> (allocated on the objfile's obstack), because GDB made a guarantee about
> the relative lifetimes of these objects.  In particular, types were
> copied by preserve_one_value at a point where the string data was still
> live.
>
> Why can't we maintain that guarantee for mmap'd debuginfo as well?
>
> I realize that having a lot of lifetime dependencies can be tricky.
> But, this one is fairly well established already.
>
> For objfile-less types, I suspect we ought to always malloc any
> associated strings.  That will let us avoid memory leaks once the type
> GC work is completed.  (Currently I don't think we ever free such
> types.)

In an effort to move this along, how about if we partition the
objfile_data cleanups into two steps.  Given that we don't want to
require an ordering on the objfile_data cleanups, and (to phrase it
one way) we need to allow different modules' cleanups to refer to
other module's "objfile_data", and given my lack of skill in picking
good names, suppose we call the first step "save" and we call the
second step "free".

i.e. in objfiles.c change

struct objfile_data
{
  unsigned index;
  void (*cleanup) (struct objfile *, void *);
};

to

struct objfile_data
{
  unsigned index;
  /* The objfile is about to be freed.  Save anything needed from it.  */
  void (*save) (struct objfile *, void *);
  /* Free all objfile-related resources held.  */
  void (*free) (struct objfile *, void *);
};

It would require adding a parameter to
register_objfile_data_with_cleanup and updating all the callers.

?

Re: TYPE_NAME memory management

[Paul Pluzhnikov]

On Wed, Sep 9, 2009 at 9:40 AM, Doug Evans<dje@google.com> wrote:

> struct objfile_data
> {
>   unsigned index;
>   /* The objfile is about to be freed.  Save anything needed from it.  */
>   void (*save) (struct objfile *, void *);
>   /* Free all objfile-related resources held.  */
>   void (*free) (struct objfile *, void *);
> };

How about s/save/cleanup/ and s/free/destroy/:

> It would require adding a parameter to
> register_objfile_data_with_cleanup and updating all the callers.

Perhaps a new 'register_objfile_data_with_cleanup_and_destroy' function
which register_objfile_data_with_cleanup can call with NULL as the destroy
parameter?

But then there are only 4 places which call it, so not a big deal to
update them all.

-- 
Paul Pluzhnikov

Re: TYPE_NAME memory management

[Doug Evans]

On Wed, Sep 9, 2009 at 10:29 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:
> On Wed, Sep 9, 2009 at 9:40 AM, Doug Evans<dje@google.com> wrote:
>
>> struct objfile_data
>> {
>>   unsigned index;
>>   /* The objfile is about to be freed.  Save anything needed from it.  */
>>   void (*save) (struct objfile *, void *);
>>   /* Free all objfile-related resources held.  */
>>   void (*free) (struct objfile *, void *);
>> };
>
> How about s/save/cleanup/ and s/free/destroy/:

[fwiw]
I chose to stay away from "cleanup" because cleanups in gdb have the
connotation of freeing resources, which we want the cleanup in
"cleanup/destroy" to explicitly not do.

>> It would require adding a parameter to
>> register_objfile_data_with_cleanup and updating all the callers.
>
> Perhaps a new 'register_objfile_data_with_cleanup_and_destroy' function
> which register_objfile_data_with_cleanup can call with NULL as the destroy
> parameter?

Yeah, though given that I was explicitly staying away from "cleanup"
in the name of the functions to call I chose to avoid this.
[I don't mind "cleanup" appearing in
register_objfile_data_with_cleanup because here it encompasses both
the "save" and "free" routines.]

> But then there are only 4 places which call it, so not a big deal to
> update them all.

Yeah, that was my thinking.

Re: TYPE_NAME memory management

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> In an effort to move this along, how about if we partition the
Doug> objfile_data cleanups into two steps.  Given that we don't want to
Doug> require an ordering on the objfile_data cleanups, and (to phrase it
Doug> one way) we need to allow different modules' cleanups to refer to
Doug> other module's "objfile_data", and given my lack of skill in picking
Doug> good names, suppose we call the first step "save" and we call the
Doug> second step "free".

This sounds like a good idea to me.

Tom

Re: TYPE_NAME memory management

[Joel Brobecker]

> Sometimes it's a string constant, sometimes it's in malloc space,
> sometimes it's on objfile's obstack, and now sometimes it can live in
> mmap'd space.

My understanding was that, if the type belongs to an objfile, then
the type name should be allocated on the objfile obstack. I know we
have types that do not belong to objfiles (types that are duplicated
from objfile types in order to preserve a value from the value history,
or gdbarch types; any others?), and these types are allocated on the
heap, including the type name, but I don't think we ever deallocate
these types.

Perhaps we might want to review all TYPE_NAME and TYPE_TAG_NAME
assignments. If there is a memory leak due to an incorrect malloc,
we might want to fix that too.

> [...] except for the new mmap'd values, so it seems like the thing to
> do for now is copy such strings onto the objfile's obstack.

This sounds right.

> I'm not sure what the speed loss will be, but I think it's the thing
> to do pending data that says something more clever is needed.

Agreed - correctness before speed :)

-- 
Joel