Re: [PATCH 2/7] [python] API for macros: memory management quirks.

[Phil Muldoon <pmuldoon at redhat dot com>]

matt rice <ratmice@gmail.com> writes:

> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index f66c1d9..c56f431 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -14351,8 +14351,7 @@ macro_start_file (int file, int line,
>    /* We don't create a macro table for this compilation unit
>       at all until we actually get a filename.  */
>    if (! pending_macros)
> -    pending_macros = new_macro_table (&objfile->objfile_obstack,
> -                                      objfile->macro_cache);
> +    pending_macros = new_macro_table (objfile, macro_table_type_from_objfile);

The comments in macrotab.h need to be updated with the new parameter
info.  This and others.

This is code I am not very sure of, having never dabbled in this area.
So this review would benefit from a maintainer that is more familiar
with this code.  But what benefit do we accrue now that we purely store
macros in the objfile's obstack over (the now defunct) bcache?  Was
there an underlying reason for dual this you discovered?  It seems odd that
in the pre-patched version that struct macro_table had two strategies
for this. Contextually, why did you elect to use the obstack in the
objfile, instead of the previous ancillary related structure?

> diff --git a/gdb/macrotab.c b/gdb/macrotab.c
> index efcf835..b2825d2 100644
> --- a/gdb/macrotab.c
> +++ b/gdb/macrotab.c
> @@ -35,13 +35,10 @@
>  
>  struct macro_table
>  {
> -  /* The obstack this table's data should be allocated in, or zero if
> -     we should use xmalloc.  */
> -  struct obstack *obstack;
> -
> -  /* The bcache we should use to hold macro names, argument names, and
> -     definitions, or zero if we should use xmalloc.  */
> -  struct bcache *bcache;
> +  /* The objfile who's obstack this table's data should be allocated in,
> +     and bcache we should use to hold macro names, argument
> +     names, and definitions, or zero if we should use xmalloc. */
> +  struct objfile *objfile;


Comment seems wrong, as bcache data structure no longer exists.
Additionally, (not in the code, but in your email), a narrative why we
are removing the bcache is needed.  
  


>  static void
>  macro_bcache_free (struct macro_table *t, void *obj)
>  {
> -  if (t->bcache)
> +  if (t->objfile)
>      /* There are cases where we need to remove entries from a macro
>         table, even when reading debugging information.  This should be
>         rare, and there's no easy way to free data from a bcache, so we
> @@ -147,6 +146,12 @@ macro_bcache_free (struct macro_table *t, void *obj)
>      xfree (obj);
>  }

Updated comments needed.  Referral to bcache.
  
> +/* Create a new, empty macro table.  Allocate it in OBJFILE's obstack,
> +   or use xmalloc if OBJFILE is zero.  Use OBJFILE's bcache to store
> +   all macro names, arguments, definitions, and anything else that
> +   might be the same amongst compilation units in an executable file;
> +   if OBJFILE is zero, don't cache these things.
> +
> +   Note that, if OBJFILE is non-zero, then removing information from the
> +   table may leak memory.  Neither obstacks nor bcaches really allow
> +   you to remove information, so although we can update the data
> +   structure to record the change, we can't free the old data.
> +   At the moment, since we only provide obstacks and bcaches for macro
> +   tables for symtabs, this isn't a problem; only odd debugging
> +   information makes a definition and then deletes it at the same
> +   source location (although 'gcc -DFOO -UFOO -DFOO=2' does do that
> +   in GCC 4.1.2.).  */
> +struct macro_table *new_macro_table (struct objfile *objfile,
> +				     enum macro_table_type table_type);

Comments need to be updated.
  
Cheers,

Phil