From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6397 invoked by alias); 2 Mar 2010 17:16:01 -0000 Received: (qmail 6334 invoked by uid 22791); 2 Mar 2010 17:15:56 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 02 Mar 2010 17:15:52 +0000 Received: (qmail 7425 invoked from network); 2 Mar 2010 17:15:50 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 2 Mar 2010 17:15:50 -0000 From: Pedro Alves To: Daniel Jacobowitz Subject: Re: [for discussion] Update inferior address spaces Date: Tue, 02 Mar 2010 17:16:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-19-generic; KDE/4.3.2; x86_64; ; ) Cc: gdb-patches@sourceware.org, Doug Evans References: <20100301213735.GA17815@caradoc.them.org> In-Reply-To: <20100301213735.GA17815@caradoc.them.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201003021715.47165.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: 2010-03/txt/msg00056.txt.bz2 On Monday 01 March 2010 21:37:38, Daniel Jacobowitz wrote: > I ran into the same problem that Doug reported recently, about > update_address_spaces. Pedro was kind enough to point me at the > problematic code. This patch updates all inferiors, which does stop > the wrong behavior... Right, when that function was originally written, inferiors were only added to the inferior list when a process_stratum target was already pushed. There's a comment in the function description that reads: " It is assumed that there are no bound inferiors yet, otherwise, they'd be left with stale references to released aspaces." (the actual comment has a typo: s/referenced/references, whoops.) After several iterations in the multi-exec support review process, we ended up with having inferiors in the inferior list even before they start running, so, that assumption no longer holds. Bah, comments don't assert. :-) There should have been an `gdb_assert (inferior_list == NULL)' there. > but I can see why Pedro described this to me as > a quick fix. It raises a question. > > If I'm reading this right, there's no actual case of inf->aspace != > inf->pspace->aspace in the GDB source code. The DICOS target manages > this by having all breakpoints transparently global. So the > inf->aspace pointer is redundant. > > If I'm wrong, or if there's a patch I don't have which changes this > for DICOS, could you explain the relation of these three things to me? I'll try, but I don't think I can in few words. The currently implementation takes shortcuts in the model described in progspace.h. This is one of the shortcuts. inf->aspace's main existance is for target.c:target_thread_address_space consumption. In a true multi-address-space per-inferior world, target_thread_address_space, similarly to target_thread_architecture, would return the address space where the target is currently stopped at. Borrowing from target_thread_architecture's description: On Cell, if a target is in spu_run, target_thread_address_space would return the SPU address space, otherwise the PPC's address space). Now, most targets will only have to care for a single address space mapped into a process, and this relationship needs to be recorded somewhere, hence the `inf->aspace' pointer in common code. This could instead be a list if address spaces, but I thought of leaving that complication out until someone actually tries to make multiple `struct address_space's per inferior work. > There's a nice comment in progspace.h, but it doesn't answer this > question: if an inf->aspace != inf->pspace->aspace, what does that > mean for anything that looks at a program space's aspace pointer? Once inferiors can have more than one aspace, so must they be able to have more than one pspace. In that perspective, when we get there, both the `inf->aspace', and `inf->pspace' direct links are meaningless, as which pspace and aspace is right will depend on context. We can assume a pspace has always a single address space behind it, so, we can move from a pspace and its backend address space: pspace->aspace ==> the target/physical address space behind this logical view of the address space. This is the address space where e.g., a breakpoint at `main' ends up physically patched. I often find that a picture helps me see these things: from progspace.h, the uClinux-like model one is the one that I think shows this off better: |-----------------+------------+---------| | pspace1 (prog1) | inf1(pid1) | | |-----------------+------------+ | | pspace2 (prog1) | inf2(pid2) | aspace1 | |-----------------+------------+ | | pspace3 (prog2) | inf3(pid3) | | |-----------------+------------+---------| Moving the other way around, that is, when e.g., a breakpoint is hit, know the corresponding pspace, is harder. The target knows the inferior/process that got the trap, the corresponding PC, and the address space the PC belongs to (think Cell/SPU here). From these, GDB should be able to infer the program space, to get at the corresponding program and symbols, etc.: { INF, ASPACE, PC } ==> { PSPACE } This function doesn't currently exist, again, due to the shortcut taken of assuming no targets have been made to be true-multi-address space aware yet. So, currently, we just use the `inf->pspace' direct link shortcut instead. If/when Cell grows support for multiple address spaces, without CORE_ADDR hacks, the way I see it, it could fit in like this: |---------------------+----------------+---------------| | pspace1 (prog_ppc) |<- - - - - ->| aspace1 (ppu) | |---------------------+ +---------------| | pspace2 (prog_spu1) |<- inf1(pid1) ->| aspace2 (spu) | |---------------------+ +---------------| | pspace2 (prog_spu2) |<- - - - - ->| aspace3 (spu) | |---------------------+----------------+---------------| | pspace3 (prog_ppc) | inf2(pid2) | aspace4 (ppu) | |---------------------+----------------+---------------| Since target_thread_address_space is consulting the target for the current address space, and GDB should be using that to get at the program space, it's pedanticaly a model violation to use inf->pspace->aspace within target_thread_address_space itself. From another perspecive, for true multi-address-space per-inferior support, the target memory read/write routines need to be somehow passed an address space to access, and on DICOS, that can't be inf->pspace->aspace, so again it would be a model violation to use it. To answer your question, in light of the above, the case where `inf->aspace' can be different from `inf->pspace->aspace' is the DICOS model, where we have a single program space for all inferiors (see remote_add_inferior). If nothing else is done, the assert you're adding would trip there, but yeah, this function is already broken for DICOS. :-/ It's true that the breakpoints module ignores address spaces on DICOS due to transparent global breakpoints, so effectively, yes, codewise, `inf->aspace' probably could be yanked out. > Also, I believe there's a double free in the existing code, fixed in > this patch. For the shared address space case. Hmm, yes, in the not-shared -> shared direction. With the patch, it's now leaking in the shared -> not-shared direction, I think? This function is missing one bit of info: if the previous target had a shared address space. This would be simpler if address spaces were refcounted. Hmm, maybe we could find a way to get rid of the need for this function instead? Something to ponder about. Anyway, a few leaks per session is much better than double free, so that's still an improvement. > This patch works around the bug, but I don't think it's right as-is. I think it's still good. I'd just drop the assertion, and maybe special case gdbarch_has_global_solist like remote_add_inferior. But I can take care of that when I get a chance of testing with DICOS, if you prefer. -- Pedro Alves