[PATCH 1/3] gdb/objfiles: make objfile::sections yield references

[PATCH 1/3] gdb/objfiles: make objfile::sections yield references

[Simon Marchi]

A patch later in this series would change objfile::section_iterator to
yield `obj_section &` instead of `obj_section *`.  Do it as a
preparatory patch to avoid polluting that subsequent patch.  I think it
would make sense on its own anyway.

Change-Id: I7bbee50ed52599e64c4f3b06bdbbde597feba9aa
---
 gdb/arm-tdep.c        | 10 +++---
 gdb/exec.c            |  8 ++---
 gdb/gcore.c           | 14 ++++----
 gdb/hppa-bsd-tdep.c   | 10 +++---
 gdb/hppa-linux-tdep.c |  8 ++---
 gdb/hppa-tdep.c       |  8 ++---
 gdb/ia64-tdep.c       | 16 ++++-----
 gdb/machoread.c       | 12 +++----
 gdb/maint.c           |  4 +--
 gdb/minsyms.c         |  6 ++--
 gdb/objfiles.c        | 28 ++++++++--------
 gdb/objfiles.h        | 12 +++----
 gdb/printcmd.c        | 28 ++++++++--------
 gdb/solib-aix.c       |  6 ++--
 gdb/solib-dsbt.c      |  6 ++--
 gdb/solib-frv.c       |  8 ++---
 gdb/solib-svr4.c      |  4 +--
 gdb/symfile.c         | 78 +++++++++++++++++++++----------------------
 gdb/symtab.c          |  8 ++---
 gdb/xstormy16-tdep.c  |  8 ++---
 gdb/z80-tdep.c        | 16 ++++-----
 21 files changed, 149 insertions(+), 149 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index a7648250af6d..a4bc0bd3e3de 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -2504,15 +2504,15 @@ static const registry<bfd>::key<arm_exidx_data> arm_exidx_data_key;
 static struct obj_section *
 arm_obj_section_from_vma (struct objfile *objfile, bfd_vma vma)
 {
-  for (obj_section *osect : objfile->sections ())
-    if (bfd_section_flags (osect->the_bfd_section) & SEC_ALLOC)
+  for (obj_section &osect : objfile->sections ())
+    if (bfd_section_flags (osect.the_bfd_section) & SEC_ALLOC)
       {
 	bfd_vma start, size;
-	start = bfd_section_vma (osect->the_bfd_section);
-	size = bfd_section_size (osect->the_bfd_section);
+	start = bfd_section_vma (osect.the_bfd_section);
+	size = bfd_section_size (osect.the_bfd_section);
 
 	if (start <= vma && vma < start + size)
-	  return osect;
+	  return &osect;
       }
 
   return NULL;
diff --git a/gdb/exec.c b/gdb/exec.c
index c2a1f8a7adaa..0db5d9030bd1 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -653,13 +653,13 @@ program_space::add_target_sections (struct objfile *objfile)
   gdb_assert (objfile != nullptr);
 
   /* Compute the number of sections to add.  */
-  for (obj_section *osect : objfile->sections ())
+  for (obj_section &osect : objfile->sections ())
     {
-      if (bfd_section_size (osect->the_bfd_section) == 0)
+      if (bfd_section_size (osect.the_bfd_section) == 0)
 	continue;
 
-      m_target_sections.emplace_back (osect->addr (), osect->endaddr (),
-				      osect->the_bfd_section, objfile);
+      m_target_sections.emplace_back (osect.addr (), osect.endaddr (),
+				      osect.the_bfd_section, objfile);
     }
 }
 
diff --git a/gdb/gcore.c b/gdb/gcore.c
index d53265240ef2..ce5f570aa464 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -425,13 +425,13 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size, int read,
 	 If so, we can avoid copying its contents by clearing SEC_LOAD.  */
 
       for (objfile *objfile : current_program_space->objfiles ())
-	for (obj_section *objsec : objfile->sections ())
+	for (obj_section &objsec : objfile->sections ())
 	  {
 	    bfd *abfd = objfile->obfd.get ();
-	    asection *asec = objsec->the_bfd_section;
+	    asection *asec = objsec.the_bfd_section;
 	    bfd_vma align = (bfd_vma) 1 << bfd_section_alignment (asec);
-	    bfd_vma start = objsec->addr () & -align;
-	    bfd_vma end = (objsec->endaddr () + align - 1) & -align;
+	    bfd_vma start = objsec.addr () & -align;
+	    bfd_vma end = (objsec.endaddr () + align - 1) & -align;
 
 	    /* Match if either the entire memory region lies inside the
 	       section (i.e. a mapping covering some pages of a large
@@ -533,9 +533,9 @@ objfile_find_memory_regions (struct target_ops *self,
 
   /* Call callback function for each objfile section.  */
   for (objfile *objfile : current_program_space->objfiles ())
-    for (obj_section *objsec : objfile->sections ())
+    for (obj_section &objsec : objfile->sections ())
       {
-	asection *isec = objsec->the_bfd_section;
+	asection *isec = objsec.the_bfd_section;
 	flagword flags = bfd_section_flags (isec);
 
 	/* Separate debug info files are irrelevant for gcore.  */
@@ -547,7 +547,7 @@ objfile_find_memory_regions (struct target_ops *self,
 	    int size = bfd_section_size (isec);
 	    int ret;
 
-	    ret = (*func) (objsec->addr (), size,
+	    ret = (*func) (objsec.addr (), size,
 			   1, /* All sections will be readable.  */
 			   (flags & SEC_READONLY) == 0, /* Writable.  */
 			   (flags & SEC_CODE) != 0, /* Executable.  */
diff --git a/gdb/hppa-bsd-tdep.c b/gdb/hppa-bsd-tdep.c
index 715e79457a86..b9cd61e0c979 100644
--- a/gdb/hppa-bsd-tdep.c
+++ b/gdb/hppa-bsd-tdep.c
@@ -54,12 +54,12 @@ hppabsd_find_global_pointer (struct gdbarch *gdbarch, struct value *function)
   faddr_sec = find_pc_section (faddr);
   if (faddr_sec != NULL)
     {
-      for (struct obj_section *sec : faddr_sec->objfile->sections ())
+      for (struct obj_section &sec : faddr_sec->objfile->sections ())
 	{
-	  if (strcmp (sec->the_bfd_section->name, ".dynamic") == 0)
+	  if (strcmp (sec.the_bfd_section->name, ".dynamic") == 0)
 	    {
-	      CORE_ADDR addr = sec->addr ();
-	      CORE_ADDR endaddr = sec->endaddr ();
+	      CORE_ADDR addr = sec.addr ();
+	      CORE_ADDR endaddr = sec.endaddr ();
 
 	      while (addr < endaddr)
 		{
@@ -81,7 +81,7 @@ hppabsd_find_global_pointer (struct gdbarch *gdbarch, struct value *function)
 			 DT_PLTGOT, so we have to do it ourselves.  */
 		      pltgot = extract_unsigned_integer (buf, sizeof buf,
 							 byte_order);
-		      pltgot += sec->objfile->text_section_offset ();
+		      pltgot += sec.objfile->text_section_offset ();
 
 		      return pltgot;
 		    }
diff --git a/gdb/hppa-linux-tdep.c b/gdb/hppa-linux-tdep.c
index 2eb8d4656504..d0bb56214efc 100644
--- a/gdb/hppa-linux-tdep.c
+++ b/gdb/hppa-linux-tdep.c
@@ -362,14 +362,14 @@ hppa_linux_find_global_pointer (struct gdbarch *gdbarch,
   faddr_sect = find_pc_section (faddr);
   if (faddr_sect != NULL)
     {
-      for (obj_section *osect : faddr_sect->objfile->sections ())
+      for (obj_section &osect : faddr_sect->objfile->sections ())
 	{
-	  if (strcmp (osect->the_bfd_section->name, ".dynamic") == 0)
+	  if (strcmp (osect.the_bfd_section->name, ".dynamic") == 0)
 	    {
 	      CORE_ADDR addr, endaddr;
 
-	      addr = osect->addr ();
-	      endaddr = osect->endaddr ();
+	      addr = osect.addr ();
+	      endaddr = osect.endaddr ();
 
 	      while (addr < endaddr)
 		{
diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c
index 61e6cb2eb02b..6f0cebe6d95a 100644
--- a/gdb/hppa-tdep.c
+++ b/gdb/hppa-tdep.c
@@ -920,12 +920,12 @@ hppa64_convert_code_addr_to_fptr (struct gdbarch *gdbarch, CORE_ADDR code)
   if (!(sec->the_bfd_section->flags & SEC_CODE))
     return code;
 
-  for (obj_section *opd : sec->objfile->sections ())
+  for (obj_section &opd : sec->objfile->sections ())
     {
-      if (strcmp (opd->the_bfd_section->name, ".opd") == 0)
+      if (strcmp (opd.the_bfd_section->name, ".opd") == 0)
 	{
-	  for (CORE_ADDR addr = opd->addr ();
-	       addr < opd->endaddr ();
+	  for (CORE_ADDR addr = opd.addr ();
+	       addr < opd.endaddr ();
 	       addr += 2 * 8)
 	    {
 	      ULONGEST opdaddr;
diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
index 7c6526a2b4d5..6cf29867565a 100644
--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -3432,12 +3432,12 @@ ia64_find_global_pointer_from_dynamic_section (struct gdbarch *gdbarch,
   faddr_sect = find_pc_section (faddr);
   if (faddr_sect != NULL)
     {
-      for (obj_section *osect : faddr_sect->objfile->sections ())
+      for (obj_section &osect : faddr_sect->objfile->sections ())
 	{
-	  if (strcmp (osect->the_bfd_section->name, ".dynamic") == 0)
+	  if (strcmp (osect.the_bfd_section->name, ".dynamic") == 0)
 	    {
-	      CORE_ADDR addr = osect->addr ();
-	      CORE_ADDR endaddr = osect->endaddr ();
+	      CORE_ADDR addr = osect.addr ();
+	      CORE_ADDR endaddr = osect.endaddr ();
 
 	      while (addr < endaddr)
 		{
@@ -3513,12 +3513,12 @@ find_extant_func_descr (struct gdbarch *gdbarch, CORE_ADDR faddr)
 
   if (faddr_sect != NULL)
     {
-      for (obj_section *osect : faddr_sect->objfile->sections ())
+      for (obj_section &osect : faddr_sect->objfile->sections ())
 	{
-	  if (strcmp (osect->the_bfd_section->name, ".opd") == 0)
+	  if (strcmp (osect.the_bfd_section->name, ".opd") == 0)
 	    {
-	      CORE_ADDR addr = osect->addr ();
-	      CORE_ADDR endaddr = osect->endaddr ();
+	      CORE_ADDR addr = osect.addr ();
+	      CORE_ADDR endaddr = osect.endaddr ();
 
 	      while (addr < endaddr)
 		{
diff --git a/gdb/machoread.c b/gdb/machoread.c
index ffee18171563..30d376c76754 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -901,13 +901,13 @@ macho_symfile_offsets (struct objfile *objfile,
 
   for (i = 0; i < addrs.size (); i++)
     {
-      for (obj_section *osect : objfile->sections ())
+      for (obj_section &osect : objfile->sections ())
 	{
-	  const char *bfd_sect_name = osect->the_bfd_section->name;
+	  const char *bfd_sect_name = osect.the_bfd_section->name;
 
 	  if (bfd_sect_name == addrs[i].name)
 	    {
-	      osect->set_offset (addrs[i].addr);
+	      osect.set_offset (addrs[i].addr);
 	      break;
 	    }
 	}
@@ -915,10 +915,10 @@ macho_symfile_offsets (struct objfile *objfile,
 
   objfile->sect_index_text = 0;
 
-  for (obj_section *osect : objfile->sections ())
+  for (obj_section &osect : objfile->sections ())
     {
-      const char *bfd_sect_name = osect->the_bfd_section->name;
-      int sect_index = osect - objfile->sections_start;
+      const char *bfd_sect_name = osect.the_bfd_section->name;
+      int sect_index = &osect - objfile->sections_start;
 
       if (startswith (bfd_sect_name, "LC_SEGMENT."))
 	bfd_sect_name += 11;
diff --git a/gdb/maint.c b/gdb/maint.c
index 78dea22541f1..d76d957b6e66 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -582,9 +582,9 @@ maintenance_translate_address (const char *arg, int from_tty)
       p = skip_spaces (p + 1);
 
       for (objfile *objfile : current_program_space->objfiles ())
-	for (obj_section *iter : objfile->sections ())
+	for (obj_section &iter : objfile->sections ())
 	  {
-	    if (strncmp (iter->the_bfd_section->name, arg, arg_len) == 0)
+	    if (strncmp (iter.the_bfd_section->name, arg, arg_len) == 0)
 	      goto found;
 	  }
 
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 4a6459a6f2d2..16befa778207 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -712,11 +712,11 @@ static int
 frob_address (struct objfile *objfile, CORE_ADDR pc,
 	      unrelocated_addr *unrel_addr)
 {
-  for (obj_section *iter : objfile->sections ())
+  for (obj_section &iter : objfile->sections ())
     {
-      if (iter->contains (pc))
+      if (iter.contains (pc))
 	{
-	  *unrel_addr = unrelocated_addr (pc - iter->offset ());
+	  *unrel_addr = unrelocated_addr (pc - iter.offset ());
 	  return 1;
 	}
     }
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 9ea07cbaa66e..d0eb678f4512 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -587,12 +587,12 @@ objfile_relocate1 (struct objfile *objfile,
   get_objfile_pspace_data (objfile->pspace ())->section_map_dirty = 1;
 
   /* Update the table in exec_ops, used to read memory.  */
-  for (obj_section *s : objfile->sections ())
+  for (obj_section &s : objfile->sections ())
     {
-      int idx = s - objfile->sections_start;
+      int idx = &s - objfile->sections_start;
 
       exec_set_section_address (bfd_get_filename (objfile->obfd.get ()), idx,
-				s->addr ());
+				s.addr ());
     }
 
   /* Data changed.  */
@@ -790,10 +790,10 @@ sort_cmp (const struct obj_section *sect1, const obj_section *sect2)
 	     second case shouldn't occur during normal use, but std::sort
 	     does check that '!(a < a)' when compiled in debug mode.  */
 
-	  for (const obj_section *osect : objfile1->sections ())
-	    if (osect == sect2)
+	  for (const obj_section &osect : objfile1->sections ())
+	    if (&osect == sect2)
 	      return false;
-	    else if (osect == sect1)
+	    else if (&osect == sect1)
 	      return true;
 
 	  /* We should have found one of the sections before getting here.  */
@@ -994,8 +994,8 @@ update_section_map (struct program_space *pspace,
 
   alloc_size = 0;
   for (objfile *objfile : pspace->objfiles ())
-    for (obj_section *s : objfile->sections ())
-      if (insert_section_p (objfile->obfd.get (), s->the_bfd_section))
+    for (obj_section &s : objfile->sections ())
+      if (insert_section_p (objfile->obfd.get (), s.the_bfd_section))
 	alloc_size += 1;
 
   /* This happens on detach/attach (e.g. in gdb.base/attach.exp).  */
@@ -1010,9 +1010,9 @@ update_section_map (struct program_space *pspace,
 
   i = 0;
   for (objfile *objfile : pspace->objfiles ())
-    for (obj_section *s : objfile->sections ())
-      if (insert_section_p (objfile->obfd.get (), s->the_bfd_section))
-	map[i++] = s;
+    for (obj_section &s : objfile->sections ())
+      if (insert_section_p (objfile->obfd.get (), s.the_bfd_section))
+	map[i++] = &s;
 
   std::sort (map, map + alloc_size, sort_cmp);
   map_size = filter_debuginfo_sections(map, alloc_size);
@@ -1127,12 +1127,12 @@ is_addr_in_objfile (CORE_ADDR addr, const struct objfile *objfile)
   if (objfile == NULL)
     return false;
 
-  for (obj_section *osect : objfile->sections ())
+  for (obj_section &osect : objfile->sections ())
     {
-      if (section_is_overlay (osect) && !section_is_mapped (osect))
+      if (section_is_overlay (&osect) && !section_is_mapped (&osect))
 	continue;
 
-      if (osect->contains (addr))
+      if (osect.contains (addr))
 	return true;
     }
   return false;
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index da9d2f3f9389..fe7a2ac91fda 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -665,11 +665,11 @@ struct objfile : intrusive_list_node<objfile>
     section_iterator &operator= (const section_iterator &) = default;
     section_iterator &operator= (section_iterator &&) = default;
 
-    typedef section_iterator self_type;
-    typedef obj_section *value_type;
+    using self_type = section_iterator;
+    using reference = obj_section &;
 
-    value_type operator* ()
-    { return m_iter; }
+    reference operator* ()
+    { return *m_iter; }
 
     section_iterator &operator++ ()
     {
@@ -701,8 +701,8 @@ struct objfile : intrusive_list_node<objfile>
 	++m_iter;
     }
 
-    value_type m_iter;
-    value_type m_end;
+    obj_section *m_iter;
+    obj_section *m_end;
   };
 
   iterator_range<section_iterator> sections ()
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 1c50b6cbae8d..210cf79a537b 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1494,27 +1494,27 @@ info_symbol_command (const char *arg, int from_tty)
 
   addr = parse_and_eval_address (arg);
   for (objfile *objfile : current_program_space->objfiles ())
-    for (obj_section *osect : objfile->sections ())
+    for (obj_section &osect : objfile->sections ())
       {
 	/* Only process each object file once, even if there's a separate
 	   debug file.  */
 	if (objfile->separate_debug_objfile_backlink)
 	  continue;
 
-	sect_addr = overlay_mapped_address (addr, osect);
+	sect_addr = overlay_mapped_address (addr, &osect);
 
-	if (osect->contains (sect_addr)
+	if (osect.contains (sect_addr)
 	    && (msymbol
 		= lookup_minimal_symbol_by_pc_section (sect_addr,
-						       osect).minsym))
+						       &osect).minsym))
 	  {
 	    const char *obj_name, *mapped, *sec_name, *msym_name;
 	    const char *loc_string;
 
 	    matches = 1;
 	    offset = sect_addr - msymbol->value_address (objfile);
-	    mapped = section_is_mapped (osect) ? _("mapped") : _("unmapped");
-	    sec_name = osect->the_bfd_section->name;
+	    mapped = section_is_mapped (&osect) ? _("mapped") : _("unmapped");
+	    sec_name = osect.the_bfd_section->name;
 	    msym_name = msymbol->print_name ();
 
 	    /* Don't print the offset if it is zero.
@@ -1528,12 +1528,12 @@ info_symbol_command (const char *arg, int from_tty)
 	    else
 	      loc_string = msym_name;
 
-	    gdb_assert (osect->objfile && objfile_name (osect->objfile));
-	    obj_name = objfile_name (osect->objfile);
+	    gdb_assert (osect.objfile && objfile_name (osect.objfile));
+	    obj_name = objfile_name (osect.objfile);
 
 	    if (current_program_space->multi_objfile_p ())
-	      if (pc_in_unmapped_range (addr, osect))
-		if (section_is_overlay (osect))
+	      if (pc_in_unmapped_range (addr, &osect))
+		if (section_is_overlay (&osect))
 		  gdb_printf (_("%s in load address range of "
 				"%s overlay section %s of %s\n"),
 			      loc_string, mapped, sec_name, obj_name);
@@ -1542,15 +1542,15 @@ info_symbol_command (const char *arg, int from_tty)
 				"section %s of %s\n"),
 			      loc_string, sec_name, obj_name);
 	      else
-		if (section_is_overlay (osect))
+		if (section_is_overlay (&osect))
 		  gdb_printf (_("%s in %s overlay section %s of %s\n"),
 			      loc_string, mapped, sec_name, obj_name);
 		else
 		  gdb_printf (_("%s in section %s of %s\n"),
 			      loc_string, sec_name, obj_name);
 	    else
-	      if (pc_in_unmapped_range (addr, osect))
-		if (section_is_overlay (osect))
+	      if (pc_in_unmapped_range (addr, &osect))
+		if (section_is_overlay (&osect))
 		  gdb_printf (_("%s in load address range of %s overlay "
 				"section %s\n"),
 			      loc_string, mapped, sec_name);
@@ -1559,7 +1559,7 @@ info_symbol_command (const char *arg, int from_tty)
 		    (_("%s in load address range of section %s\n"),
 		     loc_string, sec_name);
 	      else
-		if (section_is_overlay (osect))
+		if (section_is_overlay (&osect))
 		  gdb_printf (_("%s in %s overlay section %s\n"),
 			      loc_string, mapped, sec_name);
 		else
diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
index fb81a56ca190..6859092a7836 100644
--- a/gdb/solib-aix.c
+++ b/gdb/solib-aix.c
@@ -622,9 +622,9 @@ aix_solib_ops::bfd_open (const char *pathname) const
 static struct obj_section *
 data_obj_section_from_objfile (struct objfile *objfile)
 {
-  for (obj_section *osect : objfile->sections ())
-    if (strcmp (bfd_section_name (osect->the_bfd_section), ".data") == 0)
-      return osect;
+  for (obj_section &osect : objfile->sections ())
+    if (strcmp (bfd_section_name (osect.the_bfd_section), ".data") == 0)
+      return &osect;
 
   return NULL;
 }
diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index 4f2c918c11b9..6b5b646480e6 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -816,16 +816,16 @@ dsbt_relocate_main_executable (void)
   section_offsets new_offsets (objf->section_offsets.size ());
   changed = 0;
 
-  for (obj_section *osect : objf->sections ())
+  for (obj_section &osect : objf->sections ())
     {
       CORE_ADDR orig_addr, addr, offset;
       int osect_idx;
       int seg;
 
-      osect_idx = osect - objf->sections_start;
+      osect_idx = &osect - objf->sections_start;
 
       /* Current address of section.  */
-      addr = osect->addr ();
+      addr = osect.addr ();
       /* Offset from where this section started.  */
       offset = objf->section_offsets[osect_idx];
       /* Original address prior to any past relocations.  */
diff --git a/gdb/solib-frv.c b/gdb/solib-frv.c
index 9a9b65d29646..e6499f469378 100644
--- a/gdb/solib-frv.c
+++ b/gdb/solib-frv.c
@@ -750,16 +750,16 @@ frv_relocate_main_executable (void)
   section_offsets new_offsets (objf->section_offsets.size ());
   changed = 0;
 
-  for (obj_section *osect : objf->sections ())
+  for (obj_section &osect : objf->sections ())
     {
       CORE_ADDR orig_addr, addr, offset;
       int osect_idx;
       int seg;
-      
-      osect_idx = osect - objf->sections_start;
+
+      osect_idx = &osect - objf->sections_start;
 
       /* Current address of section.  */
-      addr = osect->addr ();
+      addr = osect.addr ();
       /* Offset from where this section started.  */
       offset = objf->section_offsets[osect_idx];
       /* Original address prior to any past relocations.  */
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 66409e7e239a..635e52a19877 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1622,8 +1622,8 @@ is_thread_local_section (struct bfd_section *bfd_sect)
 static bool
 has_thread_local_section (const objfile *objf)
 {
-  for (obj_section *objsec : objf->sections ())
-    if (is_thread_local_section (objsec->the_bfd_section))
+  for (obj_section &objsec : objf->sections ())
+    if (is_thread_local_section (objsec.the_bfd_section))
       return true;
   return false;
 }
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 0e47f5099943..8e4888fe0087 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -835,9 +835,9 @@ init_entry_point_info (struct objfile *objfile)
 	= gdbarch_addr_bits_remove (objfile->arch (), entry_point);
 
       found = 0;
-      for (obj_section *osect : objfile->sections ())
+      for (obj_section &osect : objfile->sections ())
 	{
-	  struct bfd_section *sect = osect->the_bfd_section;
+	  struct bfd_section *sect = osect.the_bfd_section;
 
 	  if (entry_point >= bfd_section_vma (sect)
 	      && entry_point < (bfd_section_vma (sect)
@@ -3007,9 +3007,9 @@ static void
 overlay_invalidate_all (program_space *pspace)
 {
   for (objfile *objfile : pspace->objfiles ())
-    for (obj_section *sect : objfile->sections ())
-      if (section_is_overlay (sect))
-	sect->ovly_mapped = -1;
+    for (obj_section &sect : objfile->sections ())
+      if (section_is_overlay (&sect))
+	sect.ovly_mapped = -1;
 }
 
 /* Function: section_is_mapped (SECTION)
@@ -3183,18 +3183,18 @@ find_pc_overlay (CORE_ADDR pc)
   if (overlay_debugging)
     {
       for (objfile *objfile : current_program_space->objfiles ())
-	for (obj_section *osect : objfile->sections ())
-	  if (section_is_overlay (osect))
+	for (obj_section &osect : objfile->sections ())
+	  if (section_is_overlay (&osect))
 	    {
-	      if (pc_in_mapped_range (pc, osect))
+	      if (pc_in_mapped_range (pc, &osect))
 		{
-		  if (section_is_mapped (osect))
-		    return osect;
+		  if (section_is_mapped (&osect))
+		    return &osect;
 		  else
-		    best_match = osect;
+		    best_match = &osect;
 		}
-	      else if (pc_in_unmapped_range (pc, osect))
-		best_match = osect;
+	      else if (pc_in_unmapped_range (pc, &osect))
+		best_match = &osect;
 	    }
     }
   return best_match;
@@ -3210,9 +3210,9 @@ find_pc_mapped_section (CORE_ADDR pc)
   if (overlay_debugging)
     {
       for (objfile *objfile : current_program_space->objfiles ())
-	for (obj_section *osect : objfile->sections ())
-	  if (pc_in_mapped_range (pc, osect) && section_is_mapped (osect))
-	    return osect;
+	for (obj_section &osect : objfile->sections ())
+	  if (pc_in_mapped_range (pc, &osect) && section_is_mapped (&osect))
+	    return &osect;
     }
 
   return NULL;
@@ -3229,18 +3229,18 @@ list_overlays_command (const char *args, int from_tty)
   if (overlay_debugging)
     {
       for (objfile *objfile : current_program_space->objfiles ())
-	for (obj_section *osect : objfile->sections ())
-	  if (section_is_mapped (osect))
+	for (obj_section &osect : objfile->sections ())
+	  if (section_is_mapped (&osect))
 	    {
 	      struct gdbarch *gdbarch = objfile->arch ();
 	      const char *name;
 	      bfd_vma lma, vma;
 	      int size;
 
-	      vma = bfd_section_vma (osect->the_bfd_section);
-	      lma = bfd_section_lma (osect->the_bfd_section);
-	      size = bfd_section_size (osect->the_bfd_section);
-	      name = bfd_section_name (osect->the_bfd_section);
+	      vma = bfd_section_vma (osect.the_bfd_section);
+	      lma = bfd_section_lma (osect.the_bfd_section);
+	      size = bfd_section_size (osect.the_bfd_section);
+	      name = bfd_section_name (osect.the_bfd_section);
 
 	      gdb_printf ("Section %s, loaded at ", name);
 	      gdb_puts (paddress (gdbarch, lma));
@@ -3275,27 +3275,27 @@ map_overlay_command (const char *args, int from_tty)
 
   /* First, find a section matching the user supplied argument.  */
   for (objfile *obj_file : current_program_space->objfiles ())
-    for (obj_section *sec : obj_file->sections ())
-      if (!strcmp (bfd_section_name (sec->the_bfd_section), args))
+    for (obj_section &sec : obj_file->sections ())
+      if (!strcmp (bfd_section_name (sec.the_bfd_section), args))
 	{
 	  /* Now, check to see if the section is an overlay.  */
-	  if (!section_is_overlay (sec))
+	  if (!section_is_overlay (&sec))
 	    continue;		/* not an overlay section */
 
 	  /* Mark the overlay as "mapped".  */
-	  sec->ovly_mapped = 1;
+	  sec.ovly_mapped = 1;
 
 	  /* Next, make a pass and unmap any sections that are
 	     overlapped by this new section: */
 	  for (objfile *objfile2 : current_program_space->objfiles ())
-	    for (obj_section *sec2 : objfile2->sections ())
-	      if (sec2->ovly_mapped && sec != sec2 && sections_overlap (sec,
-									sec2))
+	    for (obj_section &sec2 : objfile2->sections ())
+	      if (sec2.ovly_mapped && &sec != &sec2 && sections_overlap (&sec,
+									 &sec2))
 		{
 		  if (info_verbose)
 		    gdb_printf (_("Note: section %s unmapped by overlap\n"),
-				bfd_section_name (sec2->the_bfd_section));
-		  sec2->ovly_mapped = 0; /* sec2 overlaps sec: unmap sec2.  */
+				bfd_section_name (sec2.the_bfd_section));
+		  sec2.ovly_mapped = 0; /* sec2 overlaps sec: unmap sec2.  */
 		}
 	  return;
 	}
@@ -3319,12 +3319,12 @@ unmap_overlay_command (const char *args, int from_tty)
 
   /* First, find a section matching the user supplied argument.  */
   for (objfile *objfile : current_program_space->objfiles ())
-    for (obj_section *sec : objfile->sections ())
-      if (!strcmp (bfd_section_name (sec->the_bfd_section), args))
+    for (obj_section &sec : objfile->sections ())
+      if (!strcmp (bfd_section_name (sec.the_bfd_section), args))
 	{
-	  if (!sec->ovly_mapped)
+	  if (!sec.ovly_mapped)
 	    error (_("Section %s is not mapped"), args);
-	  sec->ovly_mapped = 0;
+	  sec.ovly_mapped = 0;
 	  return;
 	}
   error (_("No overlay section called %s"), args);
@@ -3578,17 +3578,17 @@ simple_overlay_update (struct obj_section *osect)
 
   /* Now may as well update all sections, even if only one was requested.  */
   for (objfile *objfile : current_program_space->objfiles ())
-    for (obj_section *sect : objfile->sections ())
-      if (section_is_overlay (sect))
+    for (obj_section &sect : objfile->sections ())
+      if (section_is_overlay (&sect))
 	{
 	  int i;
-	  asection *bsect = sect->the_bfd_section;
+	  asection *bsect = sect.the_bfd_section;
 
 	  for (i = 0; i < cache_novlys; i++)
 	    if (cache_ovly_table[i][VMA] == bfd_section_vma (bsect)
 		&& cache_ovly_table[i][LMA] == bfd_section_lma (bsect))
 	      { /* obj_section matches i'th entry in ovly_table.  */
-		sect->ovly_mapped = cache_ovly_table[i][MAPPED];
+		sect.ovly_mapped = cache_ovly_table[i][MAPPED];
 		break;		/* finished with inner for loop: break out.  */
 	      }
 	}
diff --git a/gdb/symtab.c b/gdb/symtab.c
index b6bd0a481713..245eab20cfee 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1846,18 +1846,18 @@ fixup_symbol_section (struct symbol *sym, struct objfile *objfile)
 	 this reason, we still attempt a lookup by name prior to doing
 	 a search of the section table.  */
 
-      for (obj_section *s : objfile->sections ())
+      for (obj_section &s : objfile->sections ())
 	{
-	  if ((bfd_section_flags (s->the_bfd_section) & SEC_ALLOC) == 0)
+	  if ((bfd_section_flags (s.the_bfd_section) & SEC_ALLOC) == 0)
 	    continue;
 
-	  int idx = s - objfile->sections_start;
+	  int idx = &s - objfile->sections_start;
 	  CORE_ADDR offset = objfile->section_offsets[idx];
 
 	  if (fallback == -1)
 	    fallback = idx;
 
-	  if (s->addr () - offset <= addr && addr < s->endaddr () - offset)
+	  if (s.addr () - offset <= addr && addr < s.endaddr () - offset)
 	    {
 	      sym->set_section_index (idx);
 	      return;
diff --git a/gdb/xstormy16-tdep.c b/gdb/xstormy16-tdep.c
index b4042f1498c5..b63dfa04be00 100644
--- a/gdb/xstormy16-tdep.c
+++ b/gdb/xstormy16-tdep.c
@@ -546,14 +546,14 @@ xstormy16_find_jmp_table_entry (struct gdbarch *gdbarch, CORE_ADDR faddr)
       if (!strcmp (faddr_sect->the_bfd_section->name, ".plt"))
 	return faddr;
 
-      for (obj_section *osect : faddr_sect->objfile->sections ())
+      for (obj_section &osect : faddr_sect->objfile->sections ())
 	{
-	  if (!strcmp (osect->the_bfd_section->name, ".plt"))
+	  if (!strcmp (osect.the_bfd_section->name, ".plt"))
 	    {
 	      CORE_ADDR addr, endaddr;
 
-	      addr = osect->addr ();
-	      endaddr = osect->endaddr ();
+	      addr = osect.addr ();
+	      endaddr = osect.endaddr ();
 
 	      for (; addr < endaddr; addr += 2 * xstormy16_inst_size)
 		{
diff --git a/gdb/z80-tdep.c b/gdb/z80-tdep.c
index 8f3924d5cad3..c8f0038e31d3 100644
--- a/gdb/z80-tdep.c
+++ b/gdb/z80-tdep.c
@@ -962,11 +962,11 @@ z80_overlay_update_1 (struct obj_section *osect)
 
   /* we have interest for sections with same VMA */
   for (objfile *objfile : current_program_space->objfiles ())
-    for (obj_section *sect : objfile->sections ())
-      if (section_is_overlay (sect))
+    for (obj_section &sect : objfile->sections ())
+      if (section_is_overlay (&sect))
 	{
-	  sect->ovly_mapped = (lma == bfd_section_lma (sect->the_bfd_section));
-	  i |= sect->ovly_mapped; /* true, if at least one section is mapped */
+	  sect.ovly_mapped = (lma == bfd_section_lma (sect.the_bfd_section));
+	  i |= sect.ovly_mapped; /* true, if at least one section is mapped */
 	}
   return i;
 }
@@ -985,18 +985,18 @@ z80_overlay_update (struct obj_section *osect)
 
   /* Update all sections, even if only one was requested.  */
   for (objfile *objfile : current_program_space->objfiles ())
-    for (obj_section *sect : objfile->sections ())
+    for (obj_section &sect : objfile->sections ())
       {
-	if (!section_is_overlay (sect))
+	if (!section_is_overlay (&sect))
 	  continue;
 
-	asection *bsect = sect->the_bfd_section;
+	asection *bsect = sect.the_bfd_section;
 	bfd_vma lma = bfd_section_lma (bsect);
 	bfd_vma vma = bfd_section_vma (bsect);
 
 	for (int i = 0; i < cache_novly_regions; ++i)
 	  if (cache_ovly_region_table[i][Z80_VMA] == vma)
-	    sect->ovly_mapped =
+	    sect.ovly_mapped =
 	      (cache_ovly_region_table[i][Z80_MAPPED_TO_LMA] == lma);
       }
 }

base-commit: 69b9f23264b1c9156f9499125df96c69e9927f14
-- 
2.51.0

[PATCH 2/3] gdbsupport: make filtered_iterator work with pointers

[Simon Marchi]

It's currently not possible to use filtered_iterator with a pointer as
the base iterator type.  This patch makes it possible.  The indended
usage is:

  Foo array[12];
  Foo *begin = array;
  Foo *end = array + ARRAY_SIZE (array);
  filtered_iterator<Foo *, FooFilter> (begin, end);

Here are the things that needed changing:

 - Give filtered_iterator a constructor where the caller provides
   already constructed begin and end iterators.  filtered_iterator
   currently assumes that default-constructing a BaseIterator will
   produce a valid "end" iterator.  This is not the case if BaseIterator
   is a pointer.  The caller needs to pass in the end of the array /
   region to iterate on as the end.

 - Typedefs of member types like wouldn't work:

     typedef typename BaseIterator::value_type value_type;

   The compiler would complain that it's not possible to apply `::` to
   type `BaseIterator` (aka `Foo *`).  Use std::iterator_traits to fix
   it [1].

 - Similarly, the compiler would complain about the use of
   `BaseIterator::operator*` in the return type of
   `filtered_iterator::operator*`.  Fix this by using `decltype(auto)`
   as the return type.  This lets the compiler deduce the return type
   from the return statement.  Unlike `auto`, `decltype(auto)` perfectly
   preserves the "cvref-ness" of the deduced return type.  If the return
   expression yields a `Foo &`, then the function will return a `Foo &`
   (which is what we want), whereas it would return a `Foo` if we used
   just `auto`.

Improve the filtered_iterator unit tests to run the same tests but with
pointers as iterators.  Because the filtered_iterator objects are
initialized differently in the two scenarios, I chose to copy the
existing code and adapt it.  It would probably be possible to add a
layer of abstraction to avoid code duplication, but it would end up more
complicated and messy.  If we ever add a third scenario, we can revisit
that.

[1] https://en.cppreference.com/w/cpp/iterator/iterator_traits.html

Change-Id: Id962ffbcd960a705a82bc5eb4808b4fe118a2761
---
 gdb/unittests/filtered_iterator-selftests.c | 52 +++++++++++++++++++++
 gdbsupport/filtered-iterator.h              | 26 ++++++-----
 2 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/gdb/unittests/filtered_iterator-selftests.c b/gdb/unittests/filtered_iterator-selftests.c
index 49c95cb2bf94..c04cae4963e0 100644
--- a/gdb/unittests/filtered_iterator-selftests.c
+++ b/gdb/unittests/filtered_iterator-selftests.c
@@ -125,6 +125,26 @@ test_filtered_iterator ()
   SELF_CHECK (even_ints == expected_even_ints);
 }
 
+/* Same as the above, but using pointers as the iterator base type.  */
+
+static void
+test_filtered_iterator_ptr ()
+{
+  int array[] = { 4, 4, 5, 6, 7, 8, 9 };
+  std::vector<int> even_ints;
+  const std::vector<int> expected_even_ints { 4, 4, 6, 8 };
+
+  filtered_iterator<int *, even_numbers_only> iter
+    (array, array + ARRAY_SIZE (array));
+  filtered_iterator<int *, even_numbers_only> end
+    (array + ARRAY_SIZE (array), array + ARRAY_SIZE (array));
+
+  for (; iter != end; ++iter)
+    even_ints.push_back (*iter);
+
+  SELF_CHECK (even_ints == expected_even_ints);
+}
+
 /* Test operator== and operator!=. */
 
 static void
@@ -152,6 +172,34 @@ test_filtered_iterator_eq ()
   SELF_CHECK (!(iter1 != iter2));
 }
 
+
+/* Same as the above, but using pointers as the iterator base type.  */
+
+static void
+test_filtered_iterator_eq_ptr ()
+{
+  int array[] = { 4, 4, 5, 6, 7, 8, 9 };
+
+  filtered_iterator<int *, even_numbers_only> iter1
+    (array, array + ARRAY_SIZE(array));
+  filtered_iterator<int *, even_numbers_only> iter2
+    (array, array + ARRAY_SIZE(array));
+
+  /* They start equal.  */
+  SELF_CHECK (iter1 == iter2);
+  SELF_CHECK (!(iter1 != iter2));
+
+  /* Advance 1, now they aren't equal (despite pointing to equal values).  */
+  ++iter1;
+  SELF_CHECK (!(iter1 == iter2));
+  SELF_CHECK (iter1 != iter2);
+
+  /* Advance 2, now they are equal again.  */
+  ++iter2;
+  SELF_CHECK (iter1 == iter2);
+  SELF_CHECK (!(iter1 != iter2));
+}
+
 } /* namespace selftests */
 
 INIT_GDB_FILE (filtered_iterator_selftests)
@@ -160,4 +208,8 @@ INIT_GDB_FILE (filtered_iterator_selftests)
 			    selftests::test_filtered_iterator);
   selftests::register_test ("filtered_iterator_eq",
 			    selftests::test_filtered_iterator_eq);
+  selftests::register_test ("filtered_iterator_ptr",
+			    selftests::test_filtered_iterator_ptr);
+  selftests::register_test ("filtered_iterator_eq_ptr",
+			    selftests::test_filtered_iterator_eq_ptr);
 }
diff --git a/gdbsupport/filtered-iterator.h b/gdbsupport/filtered-iterator.h
index e824d6115a8a..38b4bb1236be 100644
--- a/gdbsupport/filtered-iterator.h
+++ b/gdbsupport/filtered-iterator.h
@@ -19,8 +19,6 @@
 #ifndef GDBSUPPORT_FILTERED_ITERATOR_H
 #define GDBSUPPORT_FILTERED_ITERATOR_H
 
-#include <type_traits>
-
 /* A filtered iterator.  This wraps BaseIterator and automatically
    skips elements that FilterFunc filters out.  Requires that
    default-constructing a BaseIterator creates a valid one-past-end
@@ -30,12 +28,16 @@ template<typename BaseIterator, typename FilterFunc>
 class filtered_iterator
 {
 public:
-  typedef filtered_iterator self_type;
-  typedef typename BaseIterator::value_type value_type;
-  typedef typename BaseIterator::reference reference;
-  typedef typename BaseIterator::pointer pointer;
-  typedef typename BaseIterator::iterator_category iterator_category;
-  typedef typename BaseIterator::difference_type difference_type;
+  using self_type = filtered_iterator;
+  using value_type = typename std::iterator_traits<BaseIterator>::value_type;
+  using reference = typename std::iterator_traits<BaseIterator>::reference;
+  using pointer = typename std::iterator_traits<BaseIterator>::pointer;
+  using iterator_category =
+      typename std::iterator_traits<BaseIterator>::iterator_category;
+  ;
+  using difference_type =
+      typename std::iterator_traits<BaseIterator>::difference_type;
+  ;
 
   /* Construct by forwarding all arguments to the underlying
      iterator.  */
@@ -44,6 +46,10 @@ class filtered_iterator
     : m_it (std::forward<Args> (args)...)
   { skip_filtered (); }
 
+  filtered_iterator (BaseIterator begin, BaseIterator end)
+    : m_it (begin), m_end (end)
+  { skip_filtered (); }
+
   /* Create a one-past-end iterator.  */
   filtered_iterator () = default;
 
@@ -56,9 +62,7 @@ class filtered_iterator
     : filtered_iterator (static_cast<const filtered_iterator &> (other))
   {}
 
-  typename std::invoke_result<decltype(&BaseIterator::operator*),
-			      BaseIterator>::type
-    operator* () const
+  decltype(auto) operator* () const
   { return *m_it; }
 
   self_type &operator++ ()
-- 
2.51.0

[PATCH 3/3] gdb/objfiles: use filtered_iterator as objfile::section_iterator

[Simon Marchi]

objfile::section_iterator looks like a good candidate to be implemented
with filtered_iterator.  Following the enhancements to filtered_iterator
in the previous patch, it's pretty straighforward.

I removed the non-const version of objfile::sections, because it didn't
seem useful to have the two methods returning the exact same type and
value.  Having just the const version achieves the same thing.

Change-Id: I2f29c2fb3f95605cb816cc1ff8935c10e0496052
---
 gdb/objfiles.h | 59 +++++++-------------------------------------------
 1 file changed, 8 insertions(+), 51 deletions(-)

diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index fe7a2ac91fda..2711d4dcb06f 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -657,61 +657,18 @@ struct objfile : intrusive_list_node<objfile>
     this->section_offsets[idx] = offset;
   }
 
-  class section_iterator
+  /* Filter function for section_iterator.  */
+  struct filter_out_null_bfd_section
   {
-  public:
-    section_iterator (const section_iterator &) = default;
-    section_iterator (section_iterator &&) = default;
-    section_iterator &operator= (const section_iterator &) = default;
-    section_iterator &operator= (section_iterator &&) = default;
-
-    using self_type = section_iterator;
-    using reference = obj_section &;
-
-    reference operator* ()
-    { return *m_iter; }
-
-    section_iterator &operator++ ()
-    {
-      ++m_iter;
-      skip_null ();
-      return *this;
-    }
-
-    bool operator== (const section_iterator &other) const
-    { return m_iter == other.m_iter && m_end == other.m_end; }
-
-    bool operator!= (const section_iterator &other) const
-    { return !(*this == other); }
-
-  private:
-
-    friend class objfile;
-
-    section_iterator (obj_section *iter, obj_section *end)
-      : m_iter (iter),
-	m_end (end)
-    {
-      skip_null ();
-    }
-
-    void skip_null ()
-    {
-      while (m_iter < m_end && m_iter->the_bfd_section == nullptr)
-	++m_iter;
-    }
-
-    obj_section *m_iter;
-    obj_section *m_end;
+    bool operator() (const obj_section &sec) const noexcept
+    { return sec.the_bfd_section != nullptr; }
   };
 
-  iterator_range<section_iterator> sections ()
-  {
-    return (iterator_range<section_iterator>
-	    (section_iterator (sections_start, sections_end),
-	     section_iterator (sections_end, sections_end)));
-  }
+  using section_iterator = filtered_iterator<obj_section *, filter_out_null_bfd_section>;
 
+  /* Return an iterable that yields the "non-null" sections of this objfile.
+     That is, the sections for which obj_section::the_bfd_section is
+     non-nullptr.  */
   iterator_range<section_iterator> sections () const
   {
     return (iterator_range<section_iterator>
-- 
2.51.0

Re: [PATCH 1/3] gdb/objfiles: make objfile::sections yield references

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> A patch later in this series would change objfile::section_iterator to
Simon> yield `obj_section &` instead of `obj_section *`.  Do it as a
Simon> preparatory patch to avoid polluting that subsequent patch.  I think it
Simon> would make sense on its own anyway.

Seems ok.

Simon> -    typedef section_iterator self_type;
Simon> -    typedef obj_section *value_type;
Simon> +    using self_type = section_iterator;
Simon> +    using reference = obj_section &;
 
I never remember when value_type or reference need to be declared.
Is it really correct to remove value_type?

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 2/3] gdbsupport: make filtered_iterator work with pointers

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> It's currently not possible to use filtered_iterator with a pointer as
Simon> the base iterator type.  This patch makes it possible.

I have a question about the forwarding constructor.

Simon> +  using iterator_category =
Simon> +      typename std::iterator_traits<BaseIterator>::iterator_category;
Simon> +  ;
Simon> +  using difference_type =
Simon> +      typename std::iterator_traits<BaseIterator>::difference_type;
Simon> +  ;
 
Couple of random ";" there.

Simon> @@ -44,6 +46,10 @@ class filtered_iterator
Simon>      : m_it (std::forward<Args> (args)...)
Simon>    { skip_filtered (); }
 
It seems to me that the presence of this constructor means that some
existing code could change meaning.  It's not really likely but I wonder
how we would know.

I also wonder what this constructor is even needed for since it seems
like a filtered iterator should just wrap an ordinary one anyway.

Tom

Re: [PATCH 3/3] gdb/objfiles: use filtered_iterator as objfile::section_iterator

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> objfile::section_iterator looks like a good candidate to be implemented
Simon> with filtered_iterator.  Following the enhancements to filtered_iterator
Simon> in the previous patch, it's pretty straighforward.

Simon> I removed the non-const version of objfile::sections, because it didn't
Simon> seem useful to have the two methods returning the exact same type and
Simon> value.  Having just the const version achieves the same thing.

I would have thought the const one would return a const iterator and not
allow modifications to the underlying objects.

Anyway if it didn't then that's just some pre-existing bug, probably not
too important.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 1/3] gdb/objfiles: make objfile::sections yield references

[Simon Marchi]

On 2025-08-29 09:34, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> A patch later in this series would change objfile::section_iterator to
> Simon> yield `obj_section &` instead of `obj_section *`.  Do it as a
> Simon> preparatory patch to avoid polluting that subsequent patch.  I think it
> Simon> would make sense on its own anyway.
> 
> Seems ok.
> 
> Simon> -    typedef section_iterator self_type;
> Simon> -    typedef obj_section *value_type;
> Simon> +    using self_type = section_iterator;
> Simon> +    using reference = obj_section &;
>  
> I never remember when value_type or reference need to be declared.
> Is it really correct to remove value_type?

I don't know for sure, but cppreference's iterator_traits page [1] says
this for C++17:

    Nested type        Definition
    -----------        ----------
    difference_type    Iter::difference_type
    value_type         Iter::value_type
    pointer            Iter::pointer
    reference          Iter::reference
    iterator_category  Iter::iterator_category

    If Iter does not have any of the five nested types above, then this
    template has no members by any of those names (std::iterator_traits
    is SFINAE-friendly).  (valid for C++17, until C++20)

So I guess if you tried to use iterator_traits with a type missing any
of those typedefs, it wouldn't work.  But the original section_iterator
already missing reference and som others, so it's already in that state.
If that ever becomes a problem, I think we'll get a build failure, so
we'll know.  I think I had some cases of that in an earlier iteration of
this series, where I had to add the missing member types for the
compiler to be happy.

And anyway, this particular code disappears in the last patch of the
series, so I'm not too worried about that particular instance.

[1] https://en.cppreference.com/w/cpp/iterator/iterator_traits.html

> 
> Approved-By: Tom Tromey <tom@tromey.com>

Thanks, I will push this one right away.

Simon

Re: [PATCH 2/3] gdbsupport: make filtered_iterator work with pointers

[Simon Marchi]

On 2025-08-29 09:40, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> It's currently not possible to use filtered_iterator with a pointer as
> Simon> the base iterator type.  This patch makes it possible.
> 
> I have a question about the forwarding constructor.
> 
> Simon> +  using iterator_category =
> Simon> +      typename std::iterator_traits<BaseIterator>::iterator_category;
> Simon> +  ;
> Simon> +  using difference_type =
> Simon> +      typename std::iterator_traits<BaseIterator>::difference_type;
> Simon> +  ;
>  
> Couple of random ";" there.

Oops, thanks, assignment operators were misplaced too.

> Simon> @@ -44,6 +46,10 @@ class filtered_iterator
> Simon>      : m_it (std::forward<Args> (args)...)
> Simon>    { skip_filtered (); }
>  
> It seems to me that the presence of this constructor means that some
> existing code could change meaning.  It's not really likely but I wonder
> how we would know.

Yeah, you're right.  For the meaning of a call site to change, that call
site would need:

 - a BaseIterator that can be constructed from two arguments A and B
   (the existing constructor of filtered_iterator would then be called)
 - that same BaseIterator can be constructed from the single argument A, and
   also from the single argument B (the new constructor of
   filtered_iterator would then be called)

I don't know how to know for sure whether this happens.

> I also wonder what this constructor is even needed for since it seems
> like a filtered iterator should just wrap an ordinary one anyway.

I think it's just a shortcut to have less boilerplate when you need to
instantiate a filtered_iterator.  I think I would prefer having just the
begin/end version, so that there is less magic involved, making it
easier to debug compilation errors, at the expense of having to be more
explicit at the call sites.  I think I tried it in a previous iteration
of this series and gave up, but if you think it's a good idea, I can try
it again.

Simon

Re: [PATCH 3/3] gdb/objfiles: use filtered_iterator as objfile::section_iterator

[Simon Marchi]

On 2025-08-29 09:42, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> objfile::section_iterator looks like a good candidate to be implemented
> Simon> with filtered_iterator.  Following the enhancements to filtered_iterator
> Simon> in the previous patch, it's pretty straighforward.
> 
> Simon> I removed the non-const version of objfile::sections, because it didn't
> Simon> seem useful to have the two methods returning the exact same type and
> Simon> value.  Having just the const version achieves the same thing.
> 
> I would have thought the const one would return a const iterator and not
> allow modifications to the underlying objects.

I don't think it did that, because section_iterator specifically used
a non-const `obj_section *` as the value type.

I can see it easily with clangd in VSCode.  With this function:

void test ()
{
  decltype(auto) nc = *static_cast<objfile *> (nullptr)->sections ().begin ();
  decltype(auto) c = *static_cast<const objfile *> (nullptr)->sections ().begin ();
}

... when I hover `decltype(auto)` in both cases, it says the deduced
type is `obj_section &` (non-const).  It was `obj_section *` in both
cases before my patch that make the iterator yield references.

> Anyway if it didn't then that's just some pre-existing bug, probably not
> too important.

I'll see if I can make it right, while at it, but otherwise I don't
think it's a big deal.  Not that it's good, but it's not like we are
anywhere near to have const-correctness in GDB (yet).

Simon

Re: [PATCH 2/3] gdbsupport: make filtered_iterator work with pointers

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

>> I also wonder what this constructor is even needed for since it seems
>> like a filtered iterator should just wrap an ordinary one anyway.

Simon> I think it's just a shortcut to have less boilerplate when you need to
Simon> instantiate a filtered_iterator.  I think I would prefer having just the
Simon> begin/end version, so that there is less magic involved, making it
Simon> easier to debug compilation errors, at the expense of having to be more
Simon> explicit at the call sites.  I think I tried it in a previous iteration
Simon> of this series and gave up, but if you think it's a good idea, I can try
Simon> it again.

I think you should go ahead, we can try to remove the template
constructor after.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 2/3] gdbsupport: make filtered_iterator work with pointers

[Simon Marchi]

On 2025-08-29 09:40, Tom Tromey wrote:
> I also wonder what this constructor is even needed for since it seems
> like a filtered iterator should just wrap an ordinary one anyway.

To answer to that specifically: filtered_iterator needs to know the end
in addition to the begin, so that it knows when to stop advancing, in
operator++.

The goal of these variadic constructors is so that you don't need to be
explicit about all the iterator layers.  To build a
filtered_iterator<my_iterator>, instead of writing:

    filtered_iterator<my_iterator> (my_iterator (arg1, arg2))

You can write:

    filtered_iterator<my_iterator> (arg1, arg2);

In my experience, it has been very confusing to work with these
iterators though.  Compilation failures are hard to debug, like finding
which of the N layers of iterator is not happy.  The complicated
template error messages certainly don't help.  So I would propose to
ditch all those, and just have explicit and straightforward constructors
like the one I add in this patch.  The code that uses these iterators
will be more explicit and larger, but typically we only write it once in
a factory function (e.g. all_threads).

Simon

Re: [PATCH 2/3] gdbsupport: make filtered_iterator work with pointers

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> So I would propose to
Simon> ditch all those, and just have explicit and straightforward constructors
Simon> like the one I add in this patch.  The code that uses these iterators
Simon> will be more explicit and larger, but typically we only write it once in
Simon> a factory function (e.g. all_threads).

Yeah, this sounds good.

Tom

Re: [PATCH 2/3] gdbsupport: make filtered_iterator work with pointers

[Simon Marchi]

On 2025-08-29 13:39, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
>>> I also wonder what this constructor is even needed for since it seems
>>> like a filtered iterator should just wrap an ordinary one anyway.
> 
> Simon> I think it's just a shortcut to have less boilerplate when you need to
> Simon> instantiate a filtered_iterator.  I think I would prefer having just the
> Simon> begin/end version, so that there is less magic involved, making it
> Simon> easier to debug compilation errors, at the expense of having to be more
> Simon> explicit at the call sites.  I think I tried it in a previous iteration
> Simon> of this series and gave up, but if you think it's a good idea, I can try
> Simon> it again.
> 
> I think you should go ahead, we can try to remove the template
> constructor after.
> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom

All right, I'll push this patch and do some further cleanups.

Thanks,

Simon

Re: [PATCH 3/3] gdb/objfiles: use filtered_iterator as objfile::section_iterator

[Simon Marchi]

On 2025-08-29 12:06, Simon Marchi wrote:
>> Anyway if it didn't then that's just some pre-existing bug, probably not
>> too important.
> 
> I'll see if I can make it right, while at it, but otherwise I don't
> think it's a big deal.  Not that it's good, but it's not like we are
> anywhere near to have const-correctness in GDB (yet).

I tried to have:

  using section_iterator = filtered_iterator<obj_section *, filter_out_null_bfd_section>;
  using const_section_iterator
    = filtered_iterator<const obj_section *, filter_out_null_bfd_section>;

  /* Return an iterable that yields the "non-null" sections of this objfile.
     That is, the sections for which obj_section::the_bfd_section is
     non-nullptr.  */
  iterator_range<section_iterator> sections ()
  {
    return (iterator_range<section_iterator>
	    (section_iterator (sections_start, sections_end),
	     section_iterator (sections_end, sections_end)));
  }

  iterator_range<const_section_iterator> sections () const
  {
    return (iterator_range<const_section_iterator>
	    (const_section_iterator (sections_start, sections_end),
	     const_section_iterator (sections_end, sections_end)));
  }


But it would require some changes that extend past the scope of this
patch.  Given that this patch doesn't make things worse than they
already are, I'll push my original patch.

Thanks,

Simon

Re: [PATCH 2/3] gdbsupport: make filtered_iterator work with pointers

[Tom Tromey]

>> It seems to me that the presence of this constructor means that some
>> existing code could change meaning.  It's not really likely but I wonder
>> how we would know.

Simon> Yeah, you're right.  For the meaning of a call site to change, that call
Simon> site would need:
...
Simon> I don't know how to know for sure whether this happens.

I thought of one way, which is to remove the template constructor,
recompile, and count the errors.  Then, add the two-argument constructor
and do it again.  If the second compilation has fewer errors, then
there's some spot that calls the new constructor.

I don't think you should actually do this though, I'm not planning to
either.

Tom