Re: [RFC,v2] Yank out target_ops->to_sections

[Pedro Alves <pedro at codesourcery dot com>]

On Wednesday 27 May 2009 19:24:06, Ulrich Weigand wrote:
> Your changes to overlay support look good to me, and I've verified that the
> patch introduces no regressions on spu-elf, including the overlay test cases.

Great.  Thank you very much for testing and for the speedy review.

> In general, I like the idea of your patch, in particular the elimination of
> sections from target_ops, and removal of xfer_memory.
> 
> I'm not quite sure what to think of the new current_target_sections variable,
> in particular its interaction with target_get_section_table.  Why do you need
> a generic fallback to current_target_sections, instead of e.g. having exec_ops
> implement a to_get_section_table method? 

The issue I had in mind is that currently, it is possible to debug
with process_stratum target pushed, without having the exec_ops
target pushed.  In that case, nothing would reply to
a to_get_section_table request, and so "trust-readonly-sections"
would fail in those cases.

I've been thinking for a while that the exec_ops target
should be pushed whenever we have any kind of file loaded (main
executable or dynamic shared module) that we can read memory
from.  It implements the file_stratum after all, and target.h says:

file_stratum,               /* Executable files, etc */
                                                 ^^^

Maybe rename it file_ops in the process, maybe.

A couple of patches ahead, I'll need to make to_have_memory,
to_have_registers, etc. methods, so that target_has_memory etc.,
returns a different result depending on the current inferior (or
current executable not inferior yet).  I think that with that
in place, it wouldn't be hard to make file_stratum always be
pushed and get rid of dummy_ops, but I haven't tried it.

An example where exec_ops isn't used at all currently is DICOS.
In that case, there's no concept of a "main" executable.  All
code comes from dynamically loaded modules the target reports.  As
a consequence, there's no exec_ops target pushed, as the user
isn't expected to use the file command (as it doesn't make sense
to use it here).
There are a few cases where GDB reads memory from the target sections
when you don't have an inferior yet.  E.g., when setting breakpoints
on functions (e.g., "break main") prologue skipping needs to read
memory.  Since on DICOS I don't have an exec_ops target pushed,
I needed to hack remote.c's xfer_partial routine to defer to
(the now being eliminated) xfer_memory itself.  I wouldn't need
that hack if there was a file_stratum layer pushed whenever GDB has
(any kind of) files to read from.

> How to you see this work in a 
> multi-inferior / multi-address-space setup?

This is all up to discussion of course, but, currently,
I've got a new structure that holds among other things,
the target sections.  Then, the current_target_sections_1 and
current_target_sections global this patch is introducing
go away, and instead we access the corresponing fields of
that new structure.

Here's a stripped down version of what I've got here currently:

/* A symbol space represents a symbolic view of an address space.  It
   holds all the data associated with a non-running-yet program (main
   executable, main symbols), and when an inferior is running and is
   bound to it, includes the list of its mapped in shared libraries.

   In the traditional debugging scenario, there's a 1-1 correspondence
   among symbol spaces, inferiors and address spaces, like so:

     sspace1 (prog1) <--> inf1(pid1) <--> aspace1

   In the case of debugging more than one traditional unix process or
   program, we still have:

     |-----------------+------------+---------|
     | sspace1 (prog1) | inf1(pid1) | aspace1 |
     |----------------------------------------|
     | sspace2 (prog1) | no inf yet | aspace2 |
     |-----------------+------------+---------|
     | sspace3 (prog2) | inf2(pid2) | aspace3 |
     |-----------------+------------+---------|

   In the former example, if inf1 forks (and GDB wants to stay
   attached to both processes), the new child will have its own symbol
   space, and address spaces.  Like so:

     |-----------------+------------+---------|
     | sspace1 (prog1) | inf1(pid1) | aspace1 |
     |-----------------+------------+---------|
     | sspace2 (prog1) | inf2(pid2) | aspace2 |
     |-----------------+------------+---------|

   However, had inf1 from the latter case vforked instead, it would
   share the symbol and address spaces with its parent, until it execs
   or exits, like so:

     |-----------------+------------+---------|
     | sspace1 (prog1) | inf1(pid1) | aspace1 |
     |                 | inf2(pid2) |         |
     |-----------------+------------+---------|

   When the vfork child execs, it is finally given new symbol and
   address spaces.

     |-----------------+------------+---------|
     | sspace1 (prog1) | inf1(pid1) | aspace1 |
     |-----------------+------------+---------|
     | sspace2 (prog1) | inf2(pid2) | aspace2 |
     |-----------------+------------+---------|

   There are targets where the OS (if any) doesn't provide memory
   management and VM protection, where all inferiors share the same
   address space --- e.g. uClinux.  This is modelled by having all
   inferiors share the same address space, but, giving each its own
   symbol space, like so:

     |-----------------+------------+---------|
     | sspace1 (prog1) | inf1(pid1) |         |
     |-----------------+------------+         |
     | sspace2 (prog1) | inf2(pid2) | aspace1 |
     |-----------------+------------+         |
     | sspace3 (prog2) | inf3(pid3) |         |
     |-----------------+------------+---------|

   The address sharing is important for run control (did we just hit a
   known breakpoint that we need to step over?) and for the
   breakpoints module (is this breakpoint a duplicate of this other
   one, or do I need to insert a trap?).

   Then, there are targets where all symbols look the same for all
   inferiors, although each has its own address space, as e.g.,
   Ericsson DICOS.  There, the correct model would be:

     |---------+------------+---------|
     |         | inf1(pid1) | aspace1 |
     |         +------------+---------|
     | sspace  | inf2(pid2) | aspace2 |
     |         +------------+---------|
     |         | inf3(pid3) | aspace3 |
     |---------+------------+---------|

   Although currently we don't model this, because the DICOS debug API
   takes care of making GDB believe that breakpoints are "global".
   That is, although each process does have its own private copy of
   data symbols (just like a bunch of forks), to the breakpoints
   module, all processes share a single address space, so all
   breakpoints set at the same address are duplicates of each other,
   even breakpoints set in the data space (e.g., call dummy
   breakpoints placed on stack).  DICOS is currently modelled by
   having all inferiors share a symbol space and an address space.  */

/* The symbol space structure.  */

struct symbol_space
  {
    /* Pointer to next in linked list.  */
    struct symbol_space *next;

    /* Unique ID number.  */
    int num;

    /* The main executable loaded into this symbol space.  This is
       managed by the exec target.  */
    struct exec *exec;

    /* The address space attached to this symbol space.  More than one
       symbol space may be bound to the same address space.  */
    struct address_space *aspace;

    /* The object file that the main symbol table was loaded from
       (e.g. the argument to the "symbol-file" or "file" command).  */
    struct objfile *symfile_objfile_1;

    /* All known objfiles are kept in a linked list.  This points to
       the root of this list. */
    struct objfile *objfiles;

    /* The set of target sections matching the sections mapped into
       this symbol space.  Managed by both exec_ops and solib.c.  */
    struct target_section_table target_sections;

    /* List of shared objects mapped into this space.  Managed by
       solib.c.  */
    struct so_list *so_list;

    /* True if this was an auto-created sspace, e.g. created from
       following a fork/exec; false, if this sspace was manually added
       by the user, and should not be pruned automatically.  */
    int removable;
  };

/* The object file that the main symbol table was loaded from (e.g. the
   argument to the "symbol-file" or "file" command).  */

#define symfile_objfile current_symbol_space->symfile_objfile_1

/* All known objfiles are kept in a linked list.  This points to the
   root of this list. */
#define object_files current_symbol_space->objfiles

/* The set of target sections matching the sections mapped into the
   current symbol address space.  */
#define current_target_sections (&current_symbol_space->target_sections)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

There still needs to be a global somewhere, there's no way around it,
but, it will be the "current_symbol_space", not the target sections.
This global is then switched whenever we need to switch context
--- switch thread, insert breakpoints, or by user command, to e.g.,
prepare to start a new inferior --- in a sense, this structure holds
the globals that currently form a "prototype" for an inferior.

> Also, I'm not completely sure I understand the implications of the solib_add
> change.  The old method allowed callers to explicitly ask that the library
> sections be *not* added to the section table, and several callers did that.
> With your patch, sections are always added.  Was it always an error to not
> add sections?  I'm not really sure why this feature was introduced ...

I think that historically, it was the other way around.  They were never
added in the beggining, and when shared libraries support for core
files was added to GDB, the corresponing targets that supported it
were adjusted to pass a target_ops pointer.

We've currently got more use for the target sections (trust-readonly-sections,
for once), so I think we should always add the sections.  I've done a bit
of history digging.  Looking at gdb 3.98's sources, and I see in solib.c:

/*
** Called by core_xfer_memory if the transfer form the core file failed.
** We try to satisfy the request from the text sections of the shared libs.
*/
int
solib_xfer_memory (memaddr, myaddr, len, write)
     CORE_ADDR memaddr;
     char *myaddr;
     int len;
     int write;
{
  int res;
  register struct so_list *so = 0;

  while (so = find_solib(so))
    {
      res = xfer_memory (memaddr, myaddr, len, write,
                         so->so_bfd, so->so_sections, so->so_sections_end);
      if (res)
        return res;
    }
  return 0;
}


and in corelow.c:
static int
core_xfer_memory (memaddr, myaddr, len, write)
     CORE_ADDR memaddr;
     char *myaddr;
     int len;
     int write;
{
  int res;
  res = xfer_memory (memaddr, myaddr, len, write,
                      core_bfd, core_sections, core_sections_end);
#ifdef SOLIB_XFER_MEMORY
  if (res == 0)
    res = SOLIB_XFER_MEMORY (memaddr, myaddr, len, write);
#endif
  return res;
}

The changelog mentions:

        * Shared library/corefile changes from Peter Schauer:
        core.c (core_close): Call CLEAR_SOLIB.
        (core_open): Remove comment about "should deal with shared lib".
        (core_xfer_memory): If we can't xfer the usual way, try the
        shared libraries.
        solib.c (so_list): New fields so_bfd and so_sections{,_end}.
        (find_solib): Use solib_map_sections to get ld_text.
        (solib_map_sections, solib_xfer_memory): New functions.
        (clear_solib): Free so_sections and close so_bfd.
        tm-sunos.h: Add solib_xfer_memory, solib_add.

Then on ChangeLog-9091, I see:

Tue Aug 13 16:17:56 1991  John Gilmore  (gnu at cygint.cygnus.com)

...
        * gdbcore.h:  Move struct section_table.
        * target.h:  New home of struct section_table.

        * solib.c (solib_add):  New argument is the target_ops whose
        section list is to be added to, if any.  Reallocate the
        sections in that target to add any that come from shared libs.
        (throughout) so_sections renamed to sections.
        (solib_xfer_memory):  Deleted.
        * tm-sunos.h (SOLIB_ADD):  Add target argument.
        (SOLIB_XFER_MEMORY):  Delete.

... and from there on I would guess that a few target gained generic
shared library support after solaris, and just missed adding
the section tables to to_sections, probably because they didn't
have either proper core file support --- remember that when debugging a
process_stratum layer, usually to_has_all_memory is set,
and so unless "set trust-readonly-sections" is on, or overlay
support is needed, target sections aren't accessed --- or
the system didn't support partial cores (cores always included all memory).

Interestingly, by gdb 4.6, we have:

 >ls *solib*
 solib.c  solib.h  xcoffsolib.c  xcoffsolib.h

... and we're still stuck with xcoffsolib.c.  :-)

I took a look at all solib_add calls that don't pass a target
ops pointer:

 solib.c:  solib_add (args, from_tty, (struct target_ops *) 0, 1);
 solib.c:  solib_add (NULL, from_tty, NULL, auto_solib_add);

These two are the broken sharedlibrary_command and reload_shared_libraries
calls.

 solib-frv.c:    solib_add (0, 0, 0, 1);

This module is only used when remote debugging, there's no core
file support.  Doesn't look like adding target sections would do
any harm, and it would make trust-readonly-sections work better.

 solib-irix.c:  solib_add ((char *) 0, 0, (struct target_ops *) 0, auto_solib_add);

I can't see a native .mt file pulling this in.  This shared library
support can't be being used with core inferiors, since
irix_solib_create_inferior_hook always wants to resume the target.  Since
target sections were primarily a feature for core files, it doesn't
surprise me to see a NULL target being passed here (remember to_has_all_memory),
but I can't see a reason why not to --- it would make "set trust-readonly-sections"
work for shared library code on this target.

 solib-osf.c:  solib_add ((char *) 0, 0, (struct target_ops *) 0, auto_solib_add);

Same here, although this one handles core files.  It is mistifying then,
since when debugging core files, if the user tries to read memory from shared
libraries' .text sections must be failing on this target, unless the core files
on this system include all memory, of course.

 solib-sunos.c:  solib_add ((char *) 0, 0, (struct target_ops *) 0, auto_solib_add);

Same as solib-irix.c.  This one must be failing badly on core inferiors.  A
configuration where that would be happening is
config/arm/nbsdaout.mh (NetBSD/arm).

 windows-nat.c:  solib_add (NULL, 0, NULL, auto_solib_add);

Up until not so long ago, there was no core file support on Windows/Cygwin.
Cygwin can now generate elf core dumps, and minidump support is being worked on
(Windows version of unix cores).  Windows configurations use solib-target.c.
Again, I can't see why it wouldn't work to pass target sections here, since,
the solib.c model always relocates target sections before sos and their
sections are added to the generic lists.

> (Maybe as a follow-on, ...)  Now that we have a struct target_section_table,
> some of the interfaces that use two target_section pointers should be changed
> to take a single target_section_table pointer, perhaps?

Sure, can't see why not.

-- 
Pedro Alves