Re: [PATCH RFA] solib.c relocation improvements

[Jim Blandy <jimb at zwingli dot cygnus dot com>]

This change is approved.

Kevin Buettner <kevinb@cygnus.com> writes:

> 
> The patch below is based on feedback from Peter Schauer regarding my
> solib.c reorg patch of last weekend.  Specifically, Peter described
> some additional cleanup that would be necessary for making solib.c
> useful for platforms aside from the ones already using it.  I used the
> following remarks from Peter as an outline for making the changes
> contained in the patch:
> 
> > - This code from solib.c:solib_map_sections
> > 
> >   for (p = so->sections; p < so->sections_end; p++)
> >     {
> >       /* Relocate the section binding addresses as recorded in the shared
> >          object's file by the base address to which the object was actually
> >          mapped. */
> >       p->addr += TARGET_SO_LM_ADDR (so);
> >       p->endaddr += TARGET_SO_LM_ADDR (so);
> >       so->lmend = max (p->endaddr, so->lmend);
> >       if (STREQ (p->the_bfd_section->name, ".text"))
> >         {
> >           so->textsection = p;
> >         }
> >     }
> > 
> >   might be too specialized for future targets, as it assumes that all sections
> >   are loaded with the same offset (perhaps I will find some time someday
> >   to convert the horrible AIX shlib code to your scheme, and then we will
> >   definitely need it).
> >   It might be better to perform the relocation via a target_so_ops entry.
> > 
> > - I am not sure if we shouldn't get rid of the TARGET_SO_LM_ADDR and
> >   so->lmend crap altogether.
> > 
> >   solib_address could examine so->sections.
> > 
> >   info_sharedlibrary_command could put out the addresses of the textsection
> >   (or of all sections, perhaps via a new `maint info sharedlibrary' command,
> >   which I could have used quite often in the past).
> > 
> >   The lowest section handling in symbol_add_stub is questionable anyway
> >   and needs some rethinking. I put in some debug output and ran the testsuite
> >   on Solaris2 and Linux x86 targets, and it seems that all the business with
> >   hacking sap->other[lowest_index].addr is not needed anyway, as
> >   sap->other[lowest_index].addr == lowest_addr in all test cases.
> >   I no longer have a SunOS4 platform handy, so perhaps it is needed there.
> 
> I did my own examination of the lowest section handling code in
> symbol_add_stub() and concluded that it was completely unnecessary. 
> Here's what it was doing:
> 
>  - It finds the "lowest" section.  This is always taken to be the
>    .text section if it exists.  Otherwise it is the section with the
>    lowest (unrelocated) address from BFD.
> 
>    In the case of the .text section, the lowest address was taken
>    to be so->textsection->addr.  Note that this is the relocated
>    address of the .text section.
> 
>    In the case of one of the other sections, the lowest address is
>    computed with the following expression:
> 
>      bfd_section_vma (so->abfd, lowest_sect) + TARGET_SO_LM_ADDR (so);
> 
>    I.e, it is the unrelocated address from the BFD section plus the
>    address needed for relocation.  Note that this value could have
>    been obtained from the shared object's section table since
>    solib_map_sections() already did the same work.  (You have to
>    look at build_section_table() and add_to_section_table() to
>    see the bit regarding the BFD section vma though.)
> 
>    In either case, when we compute lowest_addr, it will have the same
>    address as what's already in the section table.  (This was also
>    verified by Peter's empirical study.  See above.)
> 
>  - A section_add_info struct is created from the shared object's
>    section offsets.  This is nothing more than putting the section
>    offsets into a slightly different form so that we can pass them for
>    purposes of doing symbol relocations to symbol_file_add().
> 
>  - Finally, the following line...
> 
> 	sap->other[lowest_index].addr = lowest_addr;
> 
>    is attempting to assign the "lowest" address that we computed
>    earlier to the corresponding address in the section_addr_info
>    struct.
> 
>    But this code is WRONG.  lowest_index is a BFD section index
>    and if you look at the way that the section_addr_info struct
>    is created, you'll see that it is possible for it to be created
>    in such a way that the indices won't match those of the BFD
>    section indices.  (In particular, only those sections whose
>    SEC_ALLOC or SEC_LOAD flags are included in these sections.)
> 
>    In order for this code to be correct, we'd need to have
>    a loop which looks for the bfd section index.
> 
>    If it was correct, however, it would turn out that it isn't
>    doing anything useful since the addr field is already set
>    correctly.
> 
> I've tested these changes on i686-pc-linux-gnu,
> sparc-sun-solaris2.5.1, and sparc-sun-sunos4.1.4.  No regressions. 
> Also, I've implemented a backend for AIX5/IA-64 which uses the
> TARGET_SO_RELOCATE_SECTION_ADDRESSES machinery for sections that are
> relocated by different amounts and it works quite well.  I will add
> this file to the public repository this file as soon as IBM gives
> their approval.
> 
> The one change that I haven't made from Peter's remarks above is
> to add a "maint info sharedlibrary" command.  I agree that this
> would be useful, but Peter has sent me some other comments that
> have some bearing on this issue that should be addressed first.
> 
> Okay to commit?
> 
> 	Changes based on analysis from Peter Schauer:
> 	* solist.h (struct so_list): Remove field lmend.
> 	(struct target_so_ops): Remove field lm_addr.  Add field
> 	relocate_section_addresses.  Add comments for all fields
> 	in this structure
> 	(TARGET_SO_LM_ADDR): Remove.
> 	(TARGET_SO_RELOCATE_SECTION_ADDRESSES): New macro.
> 	* solib-svr4.c (svr4_relocate_section_addresses): New function.
> 	(_initialize_svr4_solib): Remove lm_addr initialization.  Add
> 	initialization for relocate_section_addresses.
> 	* solib.c (solib_map_sections): Invoke 
> 	TARGET_SO_RELOCATE_SECTION_ADDRESSES instead of using now
> 	defunct TARGET_SO_LM_ADDR to relocate the section addresses.
> 	Also, eliminate assignment to the lmend field since this
> 	field no longer exists.
> 	(symbol_add_stub): Remove machinery for determining the lowest
> 	section.
> 	(info_sharedlibrary_command): Print the text section starting
> 	and ending addresses.
> 	(solib_address): Don't use TARGET_SO_LM_ADDR, nor so->lmend to
> 	determine if an address is in a shared object.  Instead, scan
> 	the section table and test against the starting and ending
> 	addresses for each section.
> 
> Index: solib-svr4.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-svr4.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 solib-svr4.c
> --- solib-svr4.c	2000/10/24 20:05:35	1.1
> +++ solib-svr4.c	2000/10/28 22:51:30
> @@ -1567,12 +1567,20 @@ svr4_free_so (struct so_list *so)
>    free (so->lm_info);
>  }
>  
> +static void
> +svr4_relocate_section_addresses (struct so_list *so,
> +                                 struct section_table *sec)
> +{
> +  sec->addr += LM_ADDR (so);
> +  sec->endaddr += LM_ADDR (so);
> +}
> +
>  static struct target_so_ops svr4_so_ops;
>  
>  void
>  _initialize_svr4_solib (void)
>  {
> -  svr4_so_ops.lm_addr = LM_ADDR;
> +  svr4_so_ops.relocate_section_addresses = svr4_relocate_section_addresses;
>    svr4_so_ops.free_so = svr4_free_so;
>    svr4_so_ops.clear_solib = svr4_clear_solib;
>    svr4_so_ops.solib_create_inferior_hook = svr4_solib_create_inferior_hook;
> Index: solib.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 solib.c
> --- solib.c	2000/10/24 20:05:35	1.25
> +++ solib.c	2000/10/28 22:51:32
> @@ -180,9 +180,7 @@ solib_map_sections (PTR arg)
>        /* Relocate the section binding addresses as recorded in the shared
>           object's file by the base address to which the object was actually
>           mapped. */
> -      p->addr += TARGET_SO_LM_ADDR (so);
> -      p->endaddr += TARGET_SO_LM_ADDR (so);
> -      so->lmend = max (p->endaddr, so->lmend);
> +      TARGET_SO_RELOCATE_SECTION_ADDRESSES (so, p);
>        if (STREQ (p->the_bfd_section->name, ".text"))
>  	{
>  	  so->textsection = p;
> @@ -248,9 +246,6 @@ symbol_add_stub (PTR arg)
>  {
>    register struct so_list *so = (struct so_list *) arg;  /* catch_errs bogon */
>    struct section_addr_info *sap;
> -  CORE_ADDR lowest_addr = 0;
> -  int lowest_index;
> -  asection *lowest_sect = NULL;
>  
>    /* Have we already loaded this shared object?  */
>    ALL_OBJFILES (so->objfile)
> @@ -259,35 +254,9 @@ symbol_add_stub (PTR arg)
>  	return 1;
>      }
>  
> -  /* Find the shared object's text segment.  */
> -  if (so->textsection)
> -    {
> -      lowest_addr = so->textsection->addr;
> -      lowest_sect = bfd_get_section_by_name (so->abfd, ".text");
> -      lowest_index = lowest_sect->index;
> -    }
> -  else if (so->abfd != NULL)
> -    {
> -      /* If we didn't find a mapped non zero sized .text section, set
> -         up lowest_addr so that the relocation in symbol_file_add does
> -         no harm.  */
> -      lowest_sect = bfd_get_section_by_name (so->abfd, ".text");
> -      if (lowest_sect == NULL)
> -	bfd_map_over_sections (so->abfd, find_lowest_section,
> -			       (PTR) &lowest_sect);
> -      if (lowest_sect)
> -	{
> -	  lowest_addr = bfd_section_vma (so->abfd, lowest_sect)
> -	    + TARGET_SO_LM_ADDR (so);
> -	  lowest_index = lowest_sect->index;
> -	}
> -    }
> -
>    sap = build_section_addr_info_from_section_table (so->sections,
>                                                      so->sections_end);
>  
> -  sap->other[lowest_index].addr = lowest_addr;
> -
>    so->objfile = symbol_file_add (so->so_name, so->from_tty,
>  				 sap, 0, OBJF_SHARED);
>    free_section_addr_info (sap);
> @@ -610,12 +579,17 @@ info_sharedlibrary_command (char *ignore
>  	    }
>  
>  	  printf_unfiltered ("%-*s", addr_width,
> -                             local_hex_string_custom (
> -			       (unsigned long) TARGET_SO_LM_ADDR (so),
> -	                       addr_fmt));
> +			     so->textsection != NULL 
> +			       ? local_hex_string_custom (
> +			           (unsigned long) so->textsection->addr,
> +	                           addr_fmt)
> +			       : "");
>  	  printf_unfiltered ("%-*s", addr_width,
> -			 local_hex_string_custom ((unsigned long) so->lmend,
> -						  addr_fmt));
> +			     so->textsection != NULL 
> +			       ? local_hex_string_custom (
> +			           (unsigned long) so->textsection->endaddr,
> +	                           addr_fmt)
> +			       : "");
>  	  printf_unfiltered ("%-12s", so->symbols_loaded ? "Yes" : "No");
>  	  printf_unfiltered ("%s\n", so->so_name);
>  	}
> @@ -640,10 +614,7 @@ info_sharedlibrary_command (char *ignore
>  
>     Provides a hook for other gdb routines to discover whether or
>     not a particular address is within the mapped address space of
> -   a shared library.  Any address between the base mapping address
> -   and the first address beyond the end of the last mapping, is
> -   considered to be within the shared library address space, for
> -   our purposes.
> +   a shared library.
>  
>     For example, this routine is called at one point to disable
>     breakpoints which are in shared libraries that are not currently
> @@ -657,8 +628,13 @@ solib_address (CORE_ADDR address)
>  
>    for (so = so_list_head; so; so = so->next)
>      {
> -      if (TARGET_SO_LM_ADDR (so) <= address && address < so->lmend)
> -	return (so->so_name);
> +      struct section_table *p;
> +
> +      for (p = so->sections; p < so->sections_end; p++)
> +	{
> +	  if (p->addr <= address && address < p->endaddr)
> +	    return (so->so_name);
> +	}
>      }
>  
>    return (0);
> Index: solist.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/solist.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 solist.h
> --- solist.h	2000/10/24 20:05:35	1.1
> +++ solist.h	2000/10/28 22:51:32
> @@ -54,7 +54,6 @@ struct so_list
>         are initialized when we actually add it to our symbol tables.  */
>  
>      bfd *abfd;
> -    CORE_ADDR lmend;		/* upper addr bound of mapped object */
>      char symbols_loaded;	/* flag: symbols read in yet? */
>      char from_tty;		/* flag: print msgs? */
>      struct objfile *objfile;	/* objfile for loaded lib */
> @@ -65,12 +64,30 @@ struct so_list
>  
>  struct target_so_ops
>    {
> -    CORE_ADDR (*lm_addr) (struct so_list *so);
> +    /* Adjust the section binding addresses by the base address at
> +       which the object was actually mapped.  */
> +    void (*relocate_section_addresses) (struct so_list *so,
> +                                        struct section_table *);
> +
> +    /* Free the the link map info and any other private data
> +       structures associated with a so_list entry.  */
>      void (*free_so) (struct so_list *so);
> +
> +    /* Reset or free private data structures not associated with
> +       so_list entries.  */
>      void (*clear_solib) (void);
> +
> +    /* Target dependent code to run after child process fork.  */
>      void (*solib_create_inferior_hook) (void);
> +
> +    /* Do additional symbol handling, lookup, etc. after symbols
> +       for a shared object have been loaded.  */
>      void (*special_symbol_handling) (void);
> +
> +    /* Construct a list of the currently loaded shared objects.  */
>      struct so_list *(*current_sos) (void);
> +
> +    /* Find, open, and read the symbols for the main executable.  */
>      int (*open_symbol_file_object) (void *from_ttyp);
>    };
>  
> @@ -79,7 +96,8 @@ void free_so (struct so_list *so);
>  /* FIXME: gdbarch needs to control this variable */
>  extern struct target_so_ops *current_target_so_ops;
>  
> -#define TARGET_SO_LM_ADDR (current_target_so_ops->lm_addr)
> +#define TARGET_SO_RELOCATE_SECTION_ADDRESSES \
> +  (current_target_so_ops->relocate_section_addresses)
>  #define TARGET_SO_FREE_SO (current_target_so_ops->free_so)
>  #define TARGET_SO_CLEAR_SOLIB (current_target_so_ops->clear_solib)
>  #define TARGET_SO_SOLIB_CREATE_INFERIOR_HOOK \
> 
>