From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31051 invoked by alias); 29 May 2009 14:13:32 -0000 Received: (qmail 31040 invoked by uid 22791); 29 May 2009 14:13:30 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 29 May 2009 14:13:24 +0000 Received: (qmail 31400 invoked from network); 29 May 2009 14:13:21 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 29 May 2009 14:13:21 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFC,v2] Yank out target_ops->to_sections Date: Fri, 29 May 2009 14:13:00 -0000 User-Agent: KMail/1.9.10 Cc: "Ulrich Weigand" References: <200905271824.n4RIO6m6010275@d12av02.megacenter.de.ibm.com> In-Reply-To: <200905271824.n4RIO6m6010275@d12av02.megacenter.de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200905291513.42076.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-05/txt/msg00623.txt.bz2 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 (¤t_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