Re: [RFC] Remove addr, endaddr, offset from obj_section

[Pedro Alves <pedro at codesourcery dot com>]

On Thursday 05 June 2008 20:16:02, Daniel Jacobowitz wrote:
> On Fri, May 16, 2008 at 05:21:15PM +0100, Pedro Alves wrote:
> > I've been meaning to test this on Cygwin/MinGW/Dwarf before
> > posting, but since the subject came up, here it goes anyway.
>
> I looked through the patch, and I can't see anything wrong with it.
> It's OK to commit, though you might want to do that Cygwin test
> first...

I've finally passed this through the testsuite on mingw32 + dwarf, and
found no regressions.

I checked it in after also retesting on x86_64-unknown-linux-gnu.

-- 
Pedro Alves

On Friday 16 May 2008 17:21:15, Pedro Alves wrote:
> Hi,
>
> I've been meaning to test this on Cygwin/MinGW/Dwarf before
> posting, but since the subject came up, here it goes anyway.
>
> GDB has currently several places where section addresses/offsets
> are stored: bfd, so_list, exec_ops, and objfile->section_offsets[],
> and obj_section.
>
> We should be consolidating most of these.  As a first step, the place
> that looks most redundant is the one in obj_section.
>
> This patch removes the addresses in obj_section, in objfiles.h:
> > /* Sections in an objfile.
> >
> >    It is strange that we have both this notion of "sections"
> >    and the one used by section_offsets.  Section as used
> >    here, (currently at least) means a BFD section, and the sections
> >    are set up from the BFD sections in allocate_objfile.
> >
> >    The sections in section_offsets have their meaning determined by
> >    the symbol format, and they are set up by the sym_offsets function
> >    for that symbol file format.
> >
> >    I'm not sure this could or should be changed, however.  */
> >
> > struct obj_section
> >   {
> >     CORE_ADDR addr;		/* lowest address in section */
> >     CORE_ADDR endaddr;		/* 1+highest address in section */
> >
> >     /* This field is being used for nefarious purposes by
> > syms_from_objfile. It is said to be redundant with section_offsets; it's
> > not really
>
> being
>
> >        used that way, however, it's some sort of hack I don't understand
> >        and am not going to try to eliminate (yet, anyway).  FIXME.
> >
> >        It was documented as "offset between (end)addr and actual memory
> >        addresses", but that's not true; addr & endaddr are actual memory
> >        addresses.  */
> >     CORE_ADDR offset;
> >
> >     struct bfd_section *the_bfd_section;	/* BFD section pointer */
> >
> >     /* Objfile this section is part of.  */
> >     struct objfile *objfile;
> >
> >     /* True if this "overlay section" is mapped into an "overlay region".
> > */ int ovly_mapped;
> >   };
>
> - When using a relocatable object as an exec_file, we adjust all
>   allocatable sections to not have overlapping vma's by adjusting the
> bfd_vma and the objfile->section_offsets[] and the exec_ops.to_sections. 
> The obj_section->(addr,endaddr,offset) are not.  Although this ends up
> being harmless, it is still bad for consistency.
>
> - There the hack in symfile.c guarded by the
>   DEPRECATED_IBM6000_TARGET macro, which sets the
>   objfile->section_offsets[] back to obj_section->(addr,endaddr,offset),
>   but even that looks wrong:
>
>   ADDR is the memory address (after relocation), OFFSET is the
>   relocation offset that was applied (comments mine),
>
>  s->addr -= s->offset; /* revert offset, so we get the VMA on file.  */
>  s->addr += s_addr;   /* (addr has been converted to offsets,
>                          so s_addr is now the new offset.)  */
>  s->endaddr -= s->offset;
>  s->endaddr += s_addr;
>
>  s->offset += s_addr;  /* This looks wrong.  It should be '=', not '+=' ?? 
> */
>
>   See patch for context.
>
> - The DEPRECATED_IBM6000_TARGET guard is there, to the best of my
>   knowledge, because xcoffread.c ignores the offsets that are passed to
>   xcoff_symfile_offsets (sym->fns->sym_offsets), and sets the
>   objfile->sections_offsets[] all to 0.  By merging the obj_section offsets
>   with objfile->section_offsets[], we just get rid of that block, and
> things should work.  I have no means to test it, though, so this worries be
> me.
>
>   xcoff_symfile_offsets:
>
>   for (i = 0; i < objfile->num_sections; ++i)
>     {
>       /* syms_from_objfile kindly subtracts from addr the
> 	 bfd_section_vma of the .text section.  This strikes me as
> 	 wrong--whether the offset to be applied to symbol reading is
> 	 relative to the start address of the section depends on the
> 	 symbol format.  In any event, this whole "addr" concept is
> 	 pretty broken (it doesn't handle any section but .text
> 	 sensibly), so just ignore the addr parameter and use 0.
> 	 rs6000-nat.c will set the correct section offsets via
> 	 objfile_relocate.  */
> 	(objfile->section_offsets)->offsets[i] = 0;
>     }
>
>   I seems the comment is a bit offbase.  The reader still applies
>   ANOFFSET to the symbols, so psymtab->symtab expansion time sees
>   the correct section offsets.  It feels like the xcoff reader
>   should cope with the section offsets != 0 internally, and not
>   do this ignoring, but since I don't have access to any AIX system,
>   I didn't investigate that further.  But I do think things should
>   still work as they used to.
>
> I just gave it another testsuite run on on x86-64-unknown-linux-gnu.
> No changes recorded.

2008-08-20  Pedro Alves  <pedro@codesourcery.com>

	* objfiles.h (struct obj_section): Remove addr and endaddr fields.
	(obj_section_offset, obj_section_addr, obj_section_endaddr): New
	macros.
	* objfiles.c (add_to_objfile_sections): Don't set addr, endaddr
	and offset.  Use size_t instead of unsigned long.
	(build_objfile_section_table): Use size_t instead of unsigned
	long.
	(objfile_relocate): Don't relocate s->addr and s->endaddr, they're
	gone.
	(find_pc_sect_section): Use obj_section_addr and
	obj_section_endaddr.
	* symfile.c (symfile.c): Remove code that maps sections
	offsets in "addr" to the object's sections.
	* blockframe.c (find_pc_partial_function): Use obj_section_endaddr.
	* gcore.c (gcore_create_callback): Use obj_section_addr and
	obj_section_endaddr.
	* maint.c (print_objfile_section_info): Likewise.
	* printcmd.c (sym_info): Use obj_section_addr and
	obj_section_endaddr.
	* symtab.c (fixup_section): Likewise.

---
 gdb/blockframe.c |    4 ++--
 gdb/gcore.c      |    6 +++---
 gdb/maint.c      |    6 ++++--
 gdb/objfiles.c   |   35 +++++++++--------------------------
 gdb/objfiles.h   |   42 +++++++++++++++++-------------------------
 gdb/printcmd.c   |    5 +++--
 gdb/symfile.c    |   50 --------------------------------------------------
 gdb/symtab.c     |    3 ++-
 8 files changed, 40 insertions(+), 111 deletions(-)

Index: src/gdb/objfiles.h
===================================================================
--- src.orig/gdb/objfiles.h	2008-08-20 11:52:06.000000000 +0100
+++ src/gdb/objfiles.h	2008-08-20 11:52:37.000000000 +0100
@@ -110,34 +110,11 @@ struct entry_info
 
   };
 
-/* Sections in an objfile.
-
-   It is strange that we have both this notion of "sections"
-   and the one used by section_offsets.  Section as used
-   here, (currently at least) means a BFD section, and the sections
-   are set up from the BFD sections in allocate_objfile.
-
-   The sections in section_offsets have their meaning determined by
-   the symbol format, and they are set up by the sym_offsets function
-   for that symbol file format.
-
-   I'm not sure this could or should be changed, however.  */
+/* Sections in an objfile.  The section offsets are stored in the
+   OBJFILE.  */
 
 struct obj_section
   {
-    CORE_ADDR addr;		/* lowest address in section */
-    CORE_ADDR endaddr;		/* 1+highest address in section */
-
-    /* This field is being used for nefarious purposes by syms_from_objfile.
-       It is said to be redundant with section_offsets; it's not really being
-       used that way, however, it's some sort of hack I don't understand
-       and am not going to try to eliminate (yet, anyway).  FIXME.
-
-       It was documented as "offset between (end)addr and actual memory
-       addresses", but that's not true; addr & endaddr are actual memory
-       addresses.  */
-    CORE_ADDR offset;
-
     struct bfd_section *the_bfd_section;	/* BFD section pointer */
 
     /* Objfile this section is part of.  */
@@ -147,6 +124,21 @@ struct obj_section
     int ovly_mapped;
   };
 
+/* Relocation offset applied to S.  */
+#define obj_section_offset(s)						\
+  (((s)->objfile->section_offsets)->offsets[(s)->the_bfd_section->index])
+
+/* The memory address of section S (vma + offset).  */
+#define obj_section_addr(s)				      		\
+  (bfd_get_section_vma ((s)->objfile->abfd, s->the_bfd_section)		\
+   + obj_section_offset (s))
+
+/* The one-passed-the-end memory address of section S
+   (vma + size + offset).  */
+#define obj_section_endaddr(s)						\
+  (bfd_get_section_vma ((s)->objfile->abfd, s->the_bfd_section)		\
+   + bfd_get_section_size ((s)->the_bfd_section)			\
+   + obj_section_offset (s))
 
 /* The "objstats" structure provides a place for gdb to record some
    interesting information about its internal state at runtime, on a
Index: src/gdb/objfiles.c
===================================================================
--- src.orig/gdb/objfiles.c	2008-08-20 11:52:06.000000000 +0100
+++ src/gdb/objfiles.c	2008-08-20 11:52:37.000000000 +0100
@@ -88,14 +88,12 @@ add_to_objfile_sections (struct bfd *abf
 
   if (0 == bfd_section_size (abfd, asect))
     return;
-  section.offset = 0;
   section.objfile = objfile;
   section.the_bfd_section = asect;
   section.ovly_mapped = 0;
-  section.addr = bfd_section_vma (abfd, asect);
-  section.endaddr = section.addr + bfd_section_size (abfd, asect);
   obstack_grow (&objfile->objfile_obstack, (char *) &section, sizeof (section));
-  objfile->sections_end = (struct obj_section *) (((unsigned long) objfile->sections_end) + 1);
+  objfile->sections_end
+    = (struct obj_section *) (((size_t) objfile->sections_end) + 1);
 }
 
 /* Builds a section table for OBJFILE.
@@ -124,10 +122,10 @@ build_objfile_section_table (struct objf
      waste some memory.  */
 
   objfile->sections_end = 0;
-  bfd_map_over_sections (objfile->obfd, add_to_objfile_sections, (char *) objfile);
-  objfile->sections = (struct obj_section *)
-    obstack_finish (&objfile->objfile_obstack);
-  objfile->sections_end = objfile->sections + (unsigned long) objfile->sections_end;
+  bfd_map_over_sections (objfile->obfd,
+			 add_to_objfile_sections, (void *) objfile);
+  objfile->sections = obstack_finish (&objfile->objfile_obstack);
+  objfile->sections_end = objfile->sections + (size_t) objfile->sections_end;
   return (0);
 }
 
@@ -664,28 +662,13 @@ objfile_relocate (struct objfile *objfil
         objfile->ei.entry_point += ANOFFSET (delta, SECT_OFF_TEXT (objfile));
     }
 
-  {
-    struct obj_section *s;
-    bfd *abfd;
-
-    abfd = objfile->obfd;
-
-    ALL_OBJFILE_OSECTIONS (objfile, s)
-      {
-      	int idx = s->the_bfd_section->index;
-	
-	s->addr += ANOFFSET (delta, idx);
-	s->endaddr += ANOFFSET (delta, idx);
-      }
-  }
-
   /* Update the table in exec_ops, used to read memory.  */
   ALL_OBJFILE_OSECTIONS (objfile, s)
     {
       int idx = s->the_bfd_section->index;
 
       exec_set_section_address (bfd_get_filename (objfile->obfd), idx,
-				s->addr);
+				obj_section_addr (s));
     }
 
   /* Relocate breakpoints as necessary, after things are relocated. */
@@ -784,8 +767,8 @@ find_pc_sect_section (CORE_ADDR pc, stru
   struct objfile *objfile;
 
   ALL_OBJSECTIONS (objfile, s)
-    if ((section == 0 || section == s->the_bfd_section) &&
-	s->addr <= pc && pc < s->endaddr)
+    if ((section == 0 || section == s->the_bfd_section)
+	&& obj_section_addr (s) <= pc && pc < obj_section_endaddr (s))
       return (s);
 
   return (NULL);
Index: src/gdb/symfile.c
===================================================================
--- src.orig/gdb/symfile.c	2008-08-20 11:52:06.000000000 +0100
+++ src/gdb/symfile.c	2008-08-20 11:52:37.000000000 +0100
@@ -895,56 +895,6 @@ syms_from_objfile (struct objfile *objfi
       init_objfile_sect_indices (objfile);
     }
 
-#ifndef DEPRECATED_IBM6000_TARGET
-  /* This is a SVR4/SunOS specific hack, I think.  In any event, it
-     screws RS/6000.  sym_offsets should be doing this sort of thing,
-     because it knows the mapping between bfd sections and
-     section_offsets.  */
-  /* This is a hack.  As far as I can tell, section offsets are not
-     target dependent.  They are all set to addr with a couple of
-     exceptions.  The exceptions are sysvr4 shared libraries, whose
-     offsets are kept in solib structures anyway and rs6000 xcoff
-     which handles shared libraries in a completely unique way.
-
-     Section offsets are built similarly, except that they are built
-     by adding addr in all cases because there is no clear mapping
-     from section_offsets into actual sections.  Note that solib.c
-     has a different algorithm for finding section offsets.
-
-     These should probably all be collapsed into some target
-     independent form of shared library support.  FIXME.  */
-
-  if (addrs)
-    {
-      struct obj_section *s;
-
- 	/* Map section offsets in "addr" back to the object's
- 	   sections by comparing the section names with bfd's
- 	   section names.  Then adjust the section address by
- 	   the offset. */ /* for gdb/13815 */
-
-      ALL_OBJFILE_OSECTIONS (objfile, s)
-	{
-	  CORE_ADDR s_addr = 0;
-	  int i;
-
- 	    for (i = 0;
-	         !s_addr && i < addrs->num_sections && addrs->other[i].name;
-		 i++)
- 	      if (strcmp (bfd_section_name (s->objfile->obfd,
-					    s->the_bfd_section),
-			  addrs->other[i].name) == 0)
- 	        s_addr = addrs->other[i].addr; /* end added for gdb/13815 */
-
-	  s->addr -= s->offset;
-	  s->addr += s_addr;
-	  s->endaddr -= s->offset;
-	  s->endaddr += s_addr;
-	  s->offset += s_addr;
-	}
-    }
-#endif /* not DEPRECATED_IBM6000_TARGET */
-
   (*objfile->sf->sym_read) (objfile, mainline);
 
   /* Don't allow char * to have a typename (else would get caddr_t).
Index: src/gdb/blockframe.c
===================================================================
--- src.orig/gdb/blockframe.c	2008-08-20 11:52:06.000000000 +0100
+++ src/gdb/blockframe.c	2008-08-20 11:52:37.000000000 +0100
@@ -299,12 +299,12 @@ find_pc_partial_function (CORE_ADDR pc, 
 	}
 
       if (DEPRECATED_SYMBOL_NAME (msymbol + i) != NULL
-	  && SYMBOL_VALUE_ADDRESS (msymbol + i) < osect->endaddr)
+	  && SYMBOL_VALUE_ADDRESS (msymbol + i) < obj_section_endaddr (osect))
 	cache_pc_function_high = SYMBOL_VALUE_ADDRESS (msymbol + i);
       else
 	/* We got the start address from the last msymbol in the objfile.
 	   So the end address is the end of the section.  */
-	cache_pc_function_high = osect->endaddr;
+	cache_pc_function_high = obj_section_endaddr (osect);
     }
 
  return_cached_value:
Index: src/gdb/gcore.c
===================================================================
--- src.orig/gdb/gcore.c	2008-08-20 11:52:06.000000000 +0100
+++ src/gdb/gcore.c	2008-08-20 11:52:37.000000000 +0100
@@ -344,8 +344,8 @@ gcore_create_callback (CORE_ADDR vaddr, 
 	  asection *asec = objsec->the_bfd_section;
 	  bfd_vma align = (bfd_vma) 1 << bfd_get_section_alignment (abfd,
 								    asec);
-	  bfd_vma start = objsec->addr & -align;
-	  bfd_vma end = (objsec->endaddr + align - 1) & -align;
+	  bfd_vma start = obj_section_addr (objsec) & -align;
+	  bfd_vma end = (obj_section_endaddr (objsec) + align - 1) & -align;
 	  /* Match if either the entire memory region lies inside the
 	     section (i.e. a mapping covering some pages of a large
 	     segment) or the entire section lies inside the memory region
@@ -415,7 +415,7 @@ objfile_find_memory_regions (int (*func)
 	  int size = bfd_section_size (ibfd, isec);
 	  int ret;
 
-	  ret = (*func) (objsec->addr, bfd_section_size (ibfd, isec),
+	  ret = (*func) (obj_section_addr (objsec), bfd_section_size (ibfd, isec),
 			 1, /* All sections will be readable.  */
 			 (flags & SEC_READONLY) == 0, /* Writable.  */
 			 (flags & SEC_CODE) != 0, /* Executable.  */
Index: src/gdb/maint.c
===================================================================
--- src.orig/gdb/maint.c	2008-08-20 11:52:06.000000000 +0100
+++ src/gdb/maint.c	2008-08-20 11:52:37.000000000 +0100
@@ -348,8 +348,10 @@ print_objfile_section_info (bfd *abfd, 
       || match_substring (string, name)
       || match_bfd_flags (string, flags))
     {
-      maint_print_section_info (name, flags, asect->addr, asect->endaddr, 
-			  asect->the_bfd_section->filepos);
+      maint_print_section_info (name, flags,
+				obj_section_addr (asect),
+				obj_section_endaddr (asect),
+				asect->the_bfd_section->filepos);
     }
 }
 
Index: src/gdb/printcmd.c
===================================================================
--- src.orig/gdb/printcmd.c	2008-08-20 11:52:06.000000000 +0100
+++ src/gdb/printcmd.c	2008-08-20 11:52:37.000000000 +0100
@@ -995,8 +995,9 @@ sym_info (char *arg, int from_tty)
     sect = osect->the_bfd_section;
     sect_addr = overlay_mapped_address (addr, sect);
 
-    if (osect->addr <= sect_addr && sect_addr < osect->endaddr &&
-	(msymbol = lookup_minimal_symbol_by_pc_section (sect_addr, sect)))
+    if (obj_section_addr (osect) <= sect_addr
+	&& sect_addr < obj_section_endaddr (osect)
+	&& (msymbol = lookup_minimal_symbol_by_pc_section (sect_addr, sect)))
       {
 	matches = 1;
 	offset = sect_addr - SYMBOL_VALUE_ADDRESS (msymbol);
Index: src/gdb/symtab.c
===================================================================
--- src.orig/gdb/symtab.c	2008-08-20 11:52:06.000000000 +0100
+++ src/gdb/symtab.c	2008-08-20 11:52:37.000000000 +0100
@@ -1081,7 +1081,8 @@ fixup_section (struct general_symbol_inf
 	  int idx = s->the_bfd_section->index;
 	  CORE_ADDR offset = ANOFFSET (objfile->section_offsets, idx);
 
-	  if (s->addr - offset <= addr && addr < s->endaddr - offset)
+	  if (obj_section_addr (s) - offset <= addr
+	      && addr < obj_section_endaddr (s) - offset)
 	    {
 	      ginfo->bfd_section = s->the_bfd_section;
 	      ginfo->section = idx;