Re: [PATCH 3/3] Introduce a separate debug objfile iterator

[Simon Marchi <simark@simark.ca>]

On 2019-04-09 2:09 p.m., Tom Tromey wrote:
> This introduces a new iterator and range adapter for iteration over
> the separate debug files of a given objfile.  As in the current
> approach, the requested objfile is returned first, followed by the
> separate debug objfiles.
> 
> gdb/ChangeLog
> 2019-04-09  Tom Tromey  <tom@tromey.com>
> 
> 	* symtab.c (lookup_global_symbol_from_objfile)
> 	(lookup_symbol_in_objfile_from_linkage_name): Use the iterator.
> 	* objfiles.h (class separate_debug_iterator): New.
> 	(class separate_debug_range): New.
> 	(struct objfile) <separate_debug_objfiles>: New method.
> 	(objfile_separate_debug_iterate): Don't declare.
> 	* objfiles.c (separate_debug_iterator::operator++): Rename from
> 	objfile_separate_debug_iterate.
> 	(objfile_relocate, objfile_rebase, objfile_has_symbols): Use the
> 	iterator.
> 	* minsyms.c (lookup_minimal_symbol_by_pc_section): Use the
> 	iterator.
> ---
>  gdb/ChangeLog  | 15 +++++++++++
>  gdb/minsyms.c  |  5 +---
>  gdb/objfiles.c | 67 ++++++++++++++++++++++++-------------------------
>  gdb/objfiles.h | 68 +++++++++++++++++++++++++++++++++++++++++++++++---
>  gdb/symtab.c   | 12 +++------
>  5 files changed, 117 insertions(+), 50 deletions(-)
> 
> diff --git a/gdb/minsyms.c b/gdb/minsyms.c
> index 34198d122dc..8037329a862 100644
> --- a/gdb/minsyms.c
> +++ b/gdb/minsyms.c
> @@ -696,7 +696,6 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
>    int lo;
>    int hi;
>    int newobj;
> -  struct objfile *objfile;
>    struct minimal_symbol *msymbol;
>    struct minimal_symbol *best_symbol = NULL;
>    struct objfile *best_objfile = NULL;
> @@ -722,9 +721,7 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
>  
>    gdb_assert (section != NULL);
>  
> -  for (objfile = section->objfile;
> -       objfile != NULL;
> -       objfile = objfile_separate_debug_iterate (section->objfile, objfile))
> +  for (struct objfile *objfile : section->objfile->separate_debug_objfiles ())
>      {
>        CORE_ADDR pc = pc_in;
>  
> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> index ada5edc42fa..71518ea5b24 100644
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
> @@ -446,44 +446,50 @@ entry_point_address (void)
>    return retval;
>  }
>  
> -/* Iterator on PARENT and every separate debug objfile of PARENT.
> -   The usage pattern is:
> -     for (objfile = parent;
> -          objfile;
> -          objfile = objfile_separate_debug_iterate (parent, objfile))
> -       ...
> -*/
> -
> -struct objfile *
> -objfile_separate_debug_iterate (const struct objfile *parent,
> -                                const struct objfile *objfile)
> +separate_debug_iterator &
> +separate_debug_iterator::operator++ ()
>  {
> +  gdb_assert (m_objfile != nullptr);
> +
>    struct objfile *res;
>  
>    /* If any, return the first child.  */
> -  res = objfile->separate_debug_objfile;
> +  res = m_objfile->separate_debug_objfile;
>    if (res)
> -    return res;
> +    {
> +      m_objfile = res;
> +      return *this;
> +    }
>  
>    /* Common case where there is no separate debug objfile.  */
> -  if (objfile == parent)
> -    return NULL;
> +  if (m_objfile == m_parent)
> +    {
> +      m_objfile = nullptr;
> +      return *this;
> +    }
>  
>    /* Return the brother if any.  Note that we don't iterate on brothers of
>       the parents.  */
> -  res = objfile->separate_debug_objfile_link;
> +  res = m_objfile->separate_debug_objfile_link;
>    if (res)
> -    return res;
> +    {
> +      m_objfile = res;
> +      return *this;
> +    }
>  
> -  for (res = objfile->separate_debug_objfile_backlink;
> -       res != parent;
> +  for (res = m_objfile->separate_debug_objfile_backlink;
> +       res != m_parent;
>         res = res->separate_debug_objfile_backlink)
>      {
>        gdb_assert (res != NULL);
>        if (res->separate_debug_objfile_link)
> -        return res->separate_debug_objfile_link;
> +	{
> +	  m_objfile = res->separate_debug_objfile_link;
> +	  return *this;
> +	}
>      }
> -  return NULL;
> +  m_objfile = nullptr;
> +  return *this;
>  }
>  
>  /* Put one object file before a specified on in the global list.
> @@ -860,15 +866,15 @@ void
>  objfile_relocate (struct objfile *objfile,
>  		  const struct section_offsets *new_offsets)
>  {
> -  struct objfile *debug_objfile;
>    int changed = 0;
>  
>    changed |= objfile_relocate1 (objfile, new_offsets);
>  
> -  for (debug_objfile = objfile->separate_debug_objfile;
> -       debug_objfile;
> -       debug_objfile = objfile_separate_debug_iterate (objfile, debug_objfile))
> +  for (struct objfile *debug_objfile : objfile->separate_debug_objfiles ())
>      {
> +      if (debug_objfile == objfile)
> +	continue;
> +
>        section_addr_info objfile_addrs
>  	= build_section_addr_info_from_objfile (objfile);
>  
> @@ -917,14 +923,9 @@ objfile_rebase1 (struct objfile *objfile, CORE_ADDR slide)
>  void
>  objfile_rebase (struct objfile *objfile, CORE_ADDR slide)
>  {
> -  struct objfile *debug_objfile;
>    int changed = 0;
>  
> -  changed |= objfile_rebase1 (objfile, slide);
> -
> -  for (debug_objfile = objfile->separate_debug_objfile;
> -       debug_objfile;
> -       debug_objfile = objfile_separate_debug_iterate (objfile, debug_objfile))
> +  for (struct objfile *debug_objfile : objfile->separate_debug_objfiles ())
>      changed |= objfile_rebase1 (debug_objfile, slide);
>  
>    /* Relocate breakpoints as necessary, after things are relocated.  */
> @@ -965,9 +966,7 @@ objfile_has_full_symbols (struct objfile *objfile)
>  int
>  objfile_has_symbols (struct objfile *objfile)
>  {
> -  struct objfile *o;
> -
> -  for (o = objfile; o; o = objfile_separate_debug_iterate (objfile, o))
> +  for (struct objfile *o : objfile->separate_debug_objfiles ())
>      if (objfile_has_partial_symbols (o) || objfile_has_full_symbols (o))
>        return 1;
>    return 0;
> diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> index 368d9f3abe2..168f7fc275b 100644
> --- a/gdb/objfiles.h
> +++ b/gdb/objfiles.h
> @@ -318,6 +318,63 @@ struct objfile_per_bfd_storage
>    std::bitset<nr_languages> demangled_hash_languages;
>  };
>  
> +/* An iterator that first returns a parent objfile, and then each
> +   separate debug objfile.  */
> +
> +class separate_debug_iterator
> +{
> +public:
> +
> +  explicit separate_debug_iterator (struct objfile *objfile)
> +    : m_objfile (objfile),
> +      m_parent (objfile)
> +  {
> +  }
> +
> +  bool operator!= (const separate_debug_iterator &other)
> +  {
> +    return m_objfile != other.m_objfile;
> +  }
> +
> +  separate_debug_iterator &operator++ ();
> +
> +  struct objfile *operator* ()
> +  {
> +    return m_objfile;
> +  }
> +
> +private:
> +
> +  struct objfile *m_objfile;
> +  struct objfile *m_parent;
> +};
> +
> +/* A range adapter wrapping separate_debug_iterator.  */
> +
> +class separate_debug_range
> +{
> +public:
> +
> +  explicit separate_debug_range (struct objfile *objfile)
> +    : m_objfile (objfile)
> +  {
> +  }
> +
> +  separate_debug_iterator begin ()
> +  {
> +    return separate_debug_iterator (m_objfile);
> +  }
> +
> +  separate_debug_iterator end ()
> +  {
> +    return separate_debug_iterator (nullptr);
> +  }
> +
> +private:
> +
> +  struct objfile *m_objfile;
> +};
> +
>  /* Master structure for keeping track of each file from which
>     gdb reads symbols.  There are several ways these get allocated: 1.
>     The main symbol file, symfile_objfile, set by the symbol-file command,
> @@ -396,6 +453,14 @@ struct objfile
>      return msymbols_range (this);
>    }
>  
> +  /* Return a range adapter for iterating over all the separate debug
> +     objfiles of this objfile.  */
> +
> +  separate_debug_range separate_debug_objfiles ()
> +  {
> +    return separate_debug_range (this);
> +  }
> +
>  
>    /* All struct objfile's are chained together by their next pointers.
>       The program space field "objfiles"  (frequently referenced via
> @@ -563,9 +628,6 @@ extern CORE_ADDR entry_point_address (void);
>  
>  extern void build_objfile_section_table (struct objfile *);
>  
> -extern struct objfile *objfile_separate_debug_iterate (const struct objfile *,
> -                                                       const struct objfile *);
> -
>  extern void put_objfile_before (struct objfile *, struct objfile *);
>  
>  extern void add_separate_debug_objfile (struct objfile *, struct objfile *);
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index d25f560f084..16e641a830b 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -2246,11 +2246,7 @@ lookup_global_symbol_from_objfile (struct objfile *main_objfile,
>  				   const char *name,
>  				   const domain_enum domain)
>  {
> -  struct objfile *objfile;
> -
> -  for (objfile = main_objfile;
> -       objfile;
> -       objfile = objfile_separate_debug_iterate (main_objfile, objfile))
> +  for (struct objfile *objfile : main_objfile->separate_debug_objfiles ())
>      {
>        struct block_symbol result
>          = lookup_symbol_in_objfile (objfile, GLOBAL_BLOCK, name, domain);
> @@ -2327,7 +2323,7 @@ lookup_symbol_in_objfile_from_linkage_name (struct objfile *objfile,
>  					    domain_enum domain)
>  {
>    enum language lang = current_language->la_language;
> -  struct objfile *main_objfile, *cur_objfile;
> +  struct objfile *main_objfile;
>  
>    demangle_result_storage storage;
>    const char *modified_name = demangle_for_lookup (linkage_name, lang, storage);
> @@ -2337,9 +2333,7 @@ lookup_symbol_in_objfile_from_linkage_name (struct objfile *objfile,
>    else
>      main_objfile = objfile;
>  
> -  for (cur_objfile = main_objfile;
> -       cur_objfile;
> -       cur_objfile = objfile_separate_debug_iterate (main_objfile, cur_objfile))
> +  for (struct objfile *cur_objfile : main_objfile->separate_debug_objfiles ())
>      {
>        struct block_symbol result;
>  
> 

Thanks, this LGTM.

While touching this code, could you fixup the various?

  - if (res)
  + if (res != NULL)

Simon