[for discussion] Update inferior address spaces

[for discussion] Update inferior address spaces

[Daniel Jacobowitz]

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... 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?
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?

Also, I believe there's a double free in the existing code, fixed in
this patch.  For the shared address space case.

This patch works around the bug, but I don't think it's right as-is.

-- 
Daniel Jacobowitz
CodeSourcery

2010-03-01  Daniel Jacobowitz  <dan@codesourcery.com>

	* progspace.c (update_address_spaces): Update inferior address spaces
	also.

Index: progspace.c
===================================================================
--- progspace.c	(revision 277420)
+++ progspace.c	(working copy)
@@ -430,24 +430,30 @@ void
 update_address_spaces (void)
 {
   int shared_aspace = gdbarch_has_shared_address_space (target_gdbarch);
-  struct address_space *aspace = NULL;
   struct program_space *pspace;
+  struct inferior *inf;
+
+  for (inf = inferior_list; inf; inf = inf->next)
+    gdb_assert (inf->aspace == inf->pspace->aspace);
 
   init_address_spaces ();
 
-  ALL_PSPACES (pspace)
+  if (shared_aspace)
     {
-      free_address_space (pspace->aspace);
-
-      if (shared_aspace)
-	{
-	  if (aspace == NULL)
-	    aspace = new_address_space ();
-	  pspace->aspace = aspace;
-	}
-      else
-	pspace->aspace = new_address_space ();
+      struct address_space *aspace = new_address_space ();
+      free_address_space (current_program_space->aspace);
+      ALL_PSPACES (pspace)
+	pspace->aspace = aspace;
     }
+  else
+    ALL_PSPACES (pspace)
+      {
+	free_address_space (pspace->aspace);
+	pspace->aspace = new_address_space ();
+      }
+
+  for (inf = inferior_list; inf; inf = inf->next)
+    inf->aspace = inf->pspace->aspace;
 }
 
 /* Save the current program space so that it may be restored by a later

Re: [for discussion] Update inferior address spaces

[Pedro Alves]

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

Re: [for discussion] Update inferior address spaces

[Daniel Jacobowitz]

On Tue, Mar 02, 2010 at 05:15:47PM +0000, Pedro Alves wrote:
> > 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.

Oh, I wasn't even thinking about changes of architecture.  I was
suggesting something much simpler: there's a double free when both
before and after versions used shared address space, because we always
free pspace->aspace, but we don't assign a shared object there.

If the gdbarch changes those properties, this is going to blow up.

> > 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.

I think I've been confusing the two gdbarch hooks (global solist,
shared address space).  They looked too much alike yesterday...

Is this about right?

-- 
Daniel Jacobowitz
CodeSourcery

2010-03-02  Daniel Jacobowitz  <dan@codesourcery.com>

	* progspace.c (update_address_spaces): Update inferior address spaces
	also.

Index: progspace.c
===================================================================
RCS file: /cvs/src/src/gdb/progspace.c,v
retrieving revision 1.4
diff -u -p -r1.4 progspace.c
--- progspace.c	19 Jan 2010 09:39:12 -0000	1.4
+++ progspace.c	2 Mar 2010 17:40:56 -0000
@@ -430,24 +430,30 @@ void
 update_address_spaces (void)
 {
   int shared_aspace = gdbarch_has_shared_address_space (target_gdbarch);
-  struct address_space *aspace = NULL;
   struct program_space *pspace;
+  struct inferior *inf;
 
   init_address_spaces ();
 
-  ALL_PSPACES (pspace)
+  if (shared_aspace)
     {
-      free_address_space (pspace->aspace);
-
-      if (shared_aspace)
-	{
-	  if (aspace == NULL)
-	    aspace = new_address_space ();
-	  pspace->aspace = aspace;
-	}
-      else
-	pspace->aspace = new_address_space ();
+      struct address_space *aspace = new_address_space ();
+      free_address_space (current_program_space->aspace);
+      ALL_PSPACES (pspace)
+	pspace->aspace = aspace;
     }
+  else
+    ALL_PSPACES (pspace)
+      {
+	free_address_space (pspace->aspace);
+	pspace->aspace = new_address_space ();
+      }
+
+  for (inf = inferior_list; inf; inf = inf->next)
+    if (gdbarch_has_global_solist (target_gdbarch))
+      inf->aspace = maybe_new_address_space ();
+    else
+      inf->aspace = inf->pspace->aspace;
 }
 
 /* Save the current program space so that it may be restored by a later

Re: [for discussion] Update inferior address spaces

[Pedro Alves]

On Tuesday 02 March 2010 17:41:44, Daniel Jacobowitz wrote:
> Is this about right?

Yes, thanks!

-- 
Pedro Alves

Re: [for discussion] Update inferior address spaces

[Daniel Jacobowitz]

On Tue, Mar 02, 2010 at 05:51:55PM +0000, Pedro Alves wrote:
> On Tuesday 02 March 2010 17:41:44, Daniel Jacobowitz wrote:
> > Is this about right?
> 
> Yes, thanks!

Checked in, HEAD and branch.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [for discussion] Update inferior address spaces

[Doug Evans]

On Tue, Mar 2, 2010 at 9:51 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Tuesday 02 March 2010 17:41:44, Daniel Jacobowitz wrote:
>> Is this about right?
>
> Yes, thanks!

Ditto, thanks.