target_find_description question

target_find_description question

[Ulrich Weigand]

Hi Dan,

in testing the Cell debugger, I came across the question how to handle
target_find_description for native (inf-ptrace) targets.  As I understand
it, the idea is to determine the target description *before* any access
to target registers happens.  However, for inf-ptrace targets this is
not the case: inf_ptrace_create_inferior calls fork_inferior, which
call the inf_ptrace_him callback, which calls startup_inferior.

That latter function now calls wait_for_inferior.  This does not only
do a target_wait, however, but does the full handle_inferior_event
processing -- which at the very least reads the PC register (for
decr_pc_after_break processing and to set stop_pc).  This happens
*before* target_find_description is called (from post_create_inferior).

Now, this ususally doesn't matter much, because the PC tends not to
be changed by target descriptions -- and even if it were, this would
just mean we got (and igored) and invalid value for stop_pc on startup.

However, with the multi-arch debugger, this can actually mean that
the ppc_linux_fetch_inferior_registers routine gets called with a
regcache architecture that is not even PowerPC!  This is a problem
in that subroutines of ppc_linux_fetch_inferior_registers will do
a gdbarch_tdep () on that architecture and blindly access it as if
it were the PowerPC version, causing random memory accesses.

In fact, I guess you could even construct cases where that happens with
today's mainline GDB when built with secondary target support: suppose
you debug some non-PowerPC remote target, detach from it (leaving
current_gdbarch pointing to that other architecture), and then attach
to a native PowerPC process in the same GDB session.

Right now I've solved the invalid memory access by a simple check
in ppc_linux_fetch_inferior_registers whether the regcache is actually
for PowerPC, and just silently returning if it isn't.

But it seems a proper fix should be rather to call target_find_description
earlier, indeed before any register access happens.  Unfortunately this 
is a bit awkward as it would mean that either startup_inferior can no
longer just call wait_for_inferior, or else that there would need to 
be special handling for this case in wait_for_inferior ...

Do you have any suggestions how best to handle this?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: target_find_description question

[Daniel Jacobowitz]

On Wed, Sep 03, 2008 at 01:47:56AM +0200, Ulrich Weigand wrote:
> Hi Dan,
> 
> in testing the Cell debugger, I came across the question how to handle
> target_find_description for native (inf-ptrace) targets.  As I understand
> it, the idea is to determine the target description *before* any access
> to target registers happens.

That's right.  The remote target used to have a similar problem to the
one you describe, and before final merge I redid bits of it in order
to move target description reads earlier.

> But it seems a proper fix should be rather to call target_find_description
> earlier, indeed before any register access happens.  Unfortunately this 
> is a bit awkward as it would mean that either startup_inferior can no
> longer just call wait_for_inferior, or else that there would need to 
> be special handling for this case in wait_for_inferior ...
> 
> Do you have any suggestions how best to handle this?

target_find_description currently has a global flag,
target_desc_fetched, which means that reading the description multiple
times is harmless.  The flag will need to become per-target in a
multi-target GDB, of course.  So it's OK to call
target_find_description by hand at an earlier point, before any
register access.

Unfortunately we're supposed to do that when the target is stopped.
Otherwise, any check which attempts to read the registers or aux
vector will not work.

I suppose the easiest thing to do would be to call
target_find_description right before handle_inferior_event, and rely
on target_desc_fetched to prevent duplicate work.

-- 
Daniel Jacobowitz
CodeSourcery

Re: target_find_description question

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> I suppose the easiest thing to do would be to call
> target_find_description right before handle_inferior_event, and rely
> on target_desc_fetched to prevent duplicate work.

Unfortunately, it turns out this doesn't work.  Or rather, it works
too well: target_find_description is called after the first stop,
and determines target properties -- while the inferior process is
still executing the shell we're using to start up the real inferior!

So it'll detect e.g. a 32-bit inferior because the shell is 32-bit,
even though the real inferior is a 64-bit application ...

I guess we do need to defer target_find_description until after the
real inferior is started.  However, we then have the problem of how
to handle those register/memory accessed in the mean time.

Maybe we can change handle_inferior_event to not do any PC processing
if stop_soon is set?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: target_find_description question

[Daniel Jacobowitz]

On Thu, Sep 04, 2008 at 09:16:33PM +0200, Ulrich Weigand wrote:
> Daniel Jacobowitz wrote:
> 
> > I suppose the easiest thing to do would be to call
> > target_find_description right before handle_inferior_event, and rely
> > on target_desc_fetched to prevent duplicate work.
> 
> Unfortunately, it turns out this doesn't work.  Or rather, it works
> too well: target_find_description is called after the first stop,
> and determines target properties -- while the inferior process is
> still executing the shell we're using to start up the real inferior!

Whoops.  I didn't think of that.

> So it'll detect e.g. a 32-bit inferior because the shell is 32-bit,
> even though the real inferior is a 64-bit application ...
> 
> I guess we do need to defer target_find_description until after the
> real inferior is started.  However, we then have the problem of how
> to handle those register/memory accessed in the mean time.

Shouldn't accessing the shell's registers always be a bug, admitted
one that we already have?  We don't have symbols for it, we don't
support breakpoints in it, et cetera.  We used to have some global
information about the number of expected traps, but now it's local.

> Maybe we can change handle_inferior_event to not do any PC processing
> if stop_soon is set?

I don't think that will work, e.g. solib-irix.c and various other
solib modules run expecting to hit a breakpoint.

-- 
Daniel Jacobowitz
CodeSourcery

Re: target_find_description question

[Pedro Alves]

On Thursday 04 September 2008 22:51:32, Daniel Jacobowitz wrote:

> I don't think that will work, e.g. solib-irix.c and various other
> solib modules run expecting to hit a breakpoint.

Isn't that a breakpoint in the real inferior, not the shell, and
expected *after* stop_soon is reset (after create_inferior), in
post_create_inferior/solib_create_inferior_hook?

-- 
Pedro Alves

Re: target_find_description question

[Pedro Alves]

Sorry, hit reply too soon.

On Thursday 04 September 2008 23:12:10, Pedro Alves wrote:
> On Thursday 04 September 2008 22:51:32, Daniel Jacobowitz wrote:
> > I don't think that will work, e.g. solib-irix.c and various other
> > solib modules run expecting to hit a breakpoint.
>
> Isn't that a breakpoint in the real inferior, not the shell, and
> expected *after* stop_soon is reset (after create_inferior), in
> post_create_inferior/solib_create_inferior_hook?

I meant to suggest un-overload STOP_QUIETLY, and split it in two,
at the "and" below:

 "STOP_QUIETLY is used when running in the shell before the child
 program has been exec'd and when running through shared
 library loading."

-- 
Pedro Alves

[patch, rfc] Re: target_find_description question

[Ulrich Weigand]

Pedro Alves wrote.
> On Thursday 04 September 2008 23:12:10, Pedro Alves wrote:
> > On Thursday 04 September 2008 22:51:32, Daniel Jacobowitz wrote:
> > > I don't think that will work, e.g. solib-irix.c and various other
> > > solib modules run expecting to hit a breakpoint.
> >
> > Isn't that a breakpoint in the real inferior, not the shell, and
> > expected *after* stop_soon is reset (after create_inferior), in
> > post_create_inferior/solib_create_inferior_hook?
> 
> I meant to suggest un-overload STOP_QUIETLY, and split it in two,
> at the "and" below:
> 
>  "STOP_QUIETLY is used when running in the shell before the child
>  program has been exec'd and when running through shared
>  library loading."


At this point it seems a mistake to try to re-use wait_for_inferior for the
startup sequence in the first place.  Most of what this cares about cannot
ever happen during startup, and some of the remaining actions are actively
harmful (accessing registers / memory).

I've tried to strip down the wait_for_inferior and resume calls done by
startup_inferior to the minimum that is actually needed for startup, and
the result looks pretty straigtforward, and boils down to mostly using
target_wait and target_resume directly.

In addition, this has some further benefits:
- We can now properly handle cases where the inferior dies while in the
  shell (the FIXME in the old startup_inferior code)
- The treat_exec_as_sigtrap argument to handle_inferior_event is no longer
  used and could be removed (not part of this patch)

As startup_inferior no longer calls init_wait_for_inferior, I've moved this
to run_command_1 (which used to call init_wait_for_inferior indirectly via
kill_if_already_running, but not if the program was already killed).  This
means that kill_if_already_running no longer needs to call it (nor does it
need to call no_shared_libraries, which is now always called from
target_pre_inferior.)

Tested on amd64-linux with no regressions.

What do you think?

Bye,
Ulrich


ChangeLog:

	* fork-child.c (startup_inferior): Use target_wait and target_resume
	directly instead of calling wait_for_inferior / resume.

	* infcmd.c (kill_if_already_running): Do not call no_shared_libraries
	or init_wait_for_inferior.
	(run_command_1): Call init_wait_for_inferior.


diff -urNp gdb-orig/gdb/fork-child.c gdb-head/gdb/fork-child.c
--- gdb-orig/gdb/fork-child.c	2008-05-25 18:19:22.000000000 +0200
+++ gdb-head/gdb/fork-child.c	2008-09-05 01:35:03.000000000 +0200
@@ -422,21 +422,67 @@ startup_inferior (int ntraps)
   if (exec_wrapper)
     pending_execs++;
 
-  clear_proceed_status ();
-
-  init_wait_for_inferior ();
-
   while (1)
     {
-      /* Make wait_for_inferior be quiet. */
-      stop_soon = STOP_QUIETLY;
-      wait_for_inferior (1);
-      if (stop_signal != TARGET_SIGNAL_TRAP)
+      int resume_signal = TARGET_SIGNAL_0;
+      ptid_t resume_ptid;
+
+      struct target_waitstatus ws;
+      memset (&ws, 0, sizeof (ws));
+      resume_ptid = target_wait (pid_to_ptid (-1), &ws);
+
+      /* Mark all threads non-executing.  */
+      set_executing (pid_to_ptid (-1), 0);
+
+      /* In all-stop mode, resume all threads.  */
+      if (!non_stop)
+	resume_ptid = pid_to_ptid (-1);
+
+      switch (ws.kind)
+	{
+	  case TARGET_WAITKIND_IGNORE:
+	  case TARGET_WAITKIND_SPURIOUS:
+	  case TARGET_WAITKIND_LOADED:
+	  case TARGET_WAITKIND_FORKED:
+	  case TARGET_WAITKIND_VFORKED:
+	  case TARGET_WAITKIND_SYSCALL_ENTRY:
+	  case TARGET_WAITKIND_SYSCALL_RETURN:
+	    /* Ignore gracefully during startup of the inferior.  */
+	    break;
+
+	  case TARGET_WAITKIND_SIGNALLED:
+	    target_terminal_ours ();
+	    target_mourn_inferior ();
+	    error (_("During startup program terminated with signal %s, %s."),
+		   target_signal_to_name (ws.value.sig),
+		   target_signal_to_string (ws.value.sig));
+	    return;
+
+	  case TARGET_WAITKIND_EXITED:
+	    target_terminal_ours ();
+	    target_mourn_inferior ();
+	    if (ws.value.integer)
+	      error (_("During startup program exited with code %d."),
+		     ws.value.integer);
+	    else
+	      error (_("During startup program exited normally."));
+	    return;
+
+	  case TARGET_WAITKIND_EXECD:
+	    /* Handle EXEC signals as if they were SIGTRAP signals.  */
+	    xfree (ws.value.execd_pathname);
+	    resume_signal = TARGET_SIGNAL_TRAP;
+	    break;
+
+	  case TARGET_WAITKIND_STOPPED:
+	    resume_signal = ws.value.sig;
+	    break;
+	}
+
+      if (resume_signal != TARGET_SIGNAL_TRAP)
 	{
-	  /* Let shell child handle its own signals in its own way.
-	     FIXME: what if child has exited?  Must exit loop
-	     somehow.  */
-	  resume (0, stop_signal);
+	  /* Let shell child handle its own signals in its own way.  */
+	  target_resume (resume_ptid, 0, resume_signal);
 	}
       else
 	{
@@ -461,10 +507,10 @@ startup_inferior (int ntraps)
 	  if (--pending_execs == 0)
 	    break;
 
-	  resume (0, TARGET_SIGNAL_0);	/* Just make it go on.  */
+	  /* Just make it go on.  */
+	  target_resume (resume_ptid, 0, TARGET_SIGNAL_0);
 	}
     }
-  stop_soon = NO_STOP_QUIETLY;
 }
 
 /* Implement the "unset exec-wrapper" command.  */
diff -urNp gdb-orig/gdb/infcmd.c gdb-head/gdb/infcmd.c
--- gdb-orig/gdb/infcmd.c	2008-08-22 03:25:28.000000000 +0200
+++ gdb-head/gdb/infcmd.c	2008-09-05 01:02:02.000000000 +0200
@@ -464,8 +464,6 @@ kill_if_already_running (int from_tty)
 Start it from the beginning? "))
 	error (_("Program not restarted."));
       target_kill ();
-      no_shared_libraries (NULL, from_tty);
-      init_wait_for_inferior ();
     }
 }
 
@@ -481,6 +479,8 @@ run_command_1 (char *args, int from_tty,
   dont_repeat ();
 
   kill_if_already_running (from_tty);
+
+  init_wait_for_inferior ();
   clear_breakpoint_hit_counts ();
 
   /* Clean up any leftovers from other runs.  Some other things from


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch, rfc] Re: target_find_description question

[Daniel Jacobowitz]

On Fri, Sep 05, 2008 at 01:59:29AM +0200, Ulrich Weigand wrote:
> ChangeLog:
> 
> 	* fork-child.c (startup_inferior): Use target_wait and target_resume
> 	directly instead of calling wait_for_inferior / resume.
> 
> 	* infcmd.c (kill_if_already_running): Do not call no_shared_libraries
> 	or init_wait_for_inferior.
> 	(run_command_1): Call init_wait_for_inferior.

I like this a lot, though I'd like Pedro's opinion too.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch, rfc] Re: target_find_description question

[Pedro Alves]

On Friday 05 September 2008 00:59:29, Ulrich Weigand wrote:

> In addition, this has some further benefits:
> - We can now properly handle cases where the inferior dies while in the
>   shell (the FIXME in the old startup_inferior code)
> - The treat_exec_as_sigtrap argument to handle_inferior_event is no longer
>   used and could be removed (not part of this patch)

Of course, a specific stop_soon flag for startup/going through the shell
would also make it possible to do this in handle_inferior_event, but indeed,
it would be more complicated.  

> Tested on amd64-linux with no regressions.
>
> What do you think?

I like it.

Notice that there are fork-child targets that handle this even
at a lower level, within target_wait (see gnu-nat.c).

-- 
Pedro Alves

Re: [patch, rfc] Re: target_find_description question

[Joel Brobecker]

> I've tried to strip down the wait_for_inferior and resume calls done by
> startup_inferior to the minimum that is actually needed for startup, and
> the result looks pretty straigtforward, and boils down to mostly using
> target_wait and target_resume directly.

Looks like a nice solution to the problem.

> - The treat_exec_as_sigtrap argument to handle_inferior_event is no longer
>   used and could be removed (not part of this patch)

I would love to see that parameter go. I wasn't entirely happy when
I introduced it.

-- 
Joel

Re: [patch, rfc] Re: target_find_description question

[Ulrich Weigand]

> 	* fork-child.c (startup_inferior): Use target_wait and target_resume
> 	directly instead of calling wait_for_inferior / resume.
> 
> 	* infcmd.c (kill_if_already_running): Do not call no_shared_libraries
> 	or init_wait_for_inferior.
> 	(run_command_1): Call init_wait_for_inferior.

I've checked this in now.  Updated patch is appended below.

Bye,
Ulrich


Index: gdb/fork-child.c
===================================================================
RCS file: /cvs/src/src/gdb/fork-child.c,v
retrieving revision 1.43
diff -c -p -r1.43 fork-child.c
*** gdb/fork-child.c	8 Sep 2008 21:51:18 -0000	1.43
--- gdb/fork-child.c	11 Sep 2008 14:49:49 -0000
*************** startup_inferior (int ntraps)
*** 428,451 ****
    if (exec_wrapper)
      pending_execs++;
  
-   clear_proceed_status ();
- 
-   init_wait_for_inferior ();
- 
    while (1)
      {
!       struct thread_info *tp;
  
!       /* Make wait_for_inferior be quiet. */
!       stop_soon = STOP_QUIETLY;
!       wait_for_inferior (1);
!       tp = inferior_thread ();
!       if (tp->stop_signal != TARGET_SIGNAL_TRAP)
  	{
! 	  /* Let shell child handle its own signals in its own way.
! 	     FIXME: what if child has exited?  Must exit loop
! 	     somehow.  */
! 	  resume (0, tp->stop_signal);
  	}
        else
  	{
--- 428,494 ----
    if (exec_wrapper)
      pending_execs++;
  
    while (1)
      {
!       int resume_signal = TARGET_SIGNAL_0;
!       ptid_t resume_ptid;
! 
!       struct target_waitstatus ws;
!       memset (&ws, 0, sizeof (ws));
!       resume_ptid = target_wait (pid_to_ptid (-1), &ws);
! 
!       /* Mark all threads non-executing.  */
!       set_executing (pid_to_ptid (-1), 0);
! 
!       /* In all-stop mode, resume all threads.  */
!       if (!non_stop)
! 	resume_ptid = pid_to_ptid (-1);
! 
!       switch (ws.kind)
! 	{
! 	  case TARGET_WAITKIND_IGNORE:
! 	  case TARGET_WAITKIND_SPURIOUS:
! 	  case TARGET_WAITKIND_LOADED:
! 	  case TARGET_WAITKIND_FORKED:
! 	  case TARGET_WAITKIND_VFORKED:
! 	  case TARGET_WAITKIND_SYSCALL_ENTRY:
! 	  case TARGET_WAITKIND_SYSCALL_RETURN:
! 	    /* Ignore gracefully during startup of the inferior.  */
! 	    break;
! 
! 	  case TARGET_WAITKIND_SIGNALLED:
! 	    target_terminal_ours ();
! 	    target_mourn_inferior ();
! 	    error (_("During startup program terminated with signal %s, %s."),
! 		   target_signal_to_name (ws.value.sig),
! 		   target_signal_to_string (ws.value.sig));
! 	    return;
! 
! 	  case TARGET_WAITKIND_EXITED:
! 	    target_terminal_ours ();
! 	    target_mourn_inferior ();
! 	    if (ws.value.integer)
! 	      error (_("During startup program exited with code %d."),
! 		     ws.value.integer);
! 	    else
! 	      error (_("During startup program exited normally."));
! 	    return;
! 
! 	  case TARGET_WAITKIND_EXECD:
! 	    /* Handle EXEC signals as if they were SIGTRAP signals.  */
! 	    xfree (ws.value.execd_pathname);
! 	    resume_signal = TARGET_SIGNAL_TRAP;
! 	    break;
! 
! 	  case TARGET_WAITKIND_STOPPED:
! 	    resume_signal = ws.value.sig;
! 	    break;
! 	}
  
!       if (resume_signal != TARGET_SIGNAL_TRAP)
  	{
! 	  /* Let shell child handle its own signals in its own way.  */
! 	  target_resume (resume_ptid, 0, resume_signal);
  	}
        else
  	{
*************** startup_inferior (int ntraps)
*** 470,479 ****
  	  if (--pending_execs == 0)
  	    break;
  
! 	  resume (0, TARGET_SIGNAL_0);	/* Just make it go on.  */
  	}
      }
-   stop_soon = NO_STOP_QUIETLY;
  }
  
  /* Implement the "unset exec-wrapper" command.  */
--- 513,522 ----
  	  if (--pending_execs == 0)
  	    break;
  
! 	  /* Just make it go on.  */
! 	  target_resume (resume_ptid, 0, TARGET_SIGNAL_0);
  	}
      }
  }
  
  /* Implement the "unset exec-wrapper" command.  */
Index: gdb/infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.210
diff -c -p -r1.210 infcmd.c
*** gdb/infcmd.c	8 Sep 2008 22:10:20 -0000	1.210
--- gdb/infcmd.c	11 Sep 2008 14:49:49 -0000
*************** kill_if_already_running (int from_tty)
*** 431,438 ****
  Start it from the beginning? "))
  	error (_("Program not restarted."));
        target_kill ();
-       no_shared_libraries (NULL, from_tty);
-       init_wait_for_inferior ();
      }
  }
  
--- 431,436 ----
*************** run_command_1 (char *args, int from_tty,
*** 448,453 ****
--- 446,453 ----
    dont_repeat ();
  
    kill_if_already_running (from_tty);
+ 
+   init_wait_for_inferior ();
    clear_breakpoint_hit_counts ();
  
    /* Clean up any leftovers from other runs.  Some other things from

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com