[RFA] Fix for testsuite errors with gdbserver (remote)

[RFA] Fix for testsuite errors with gdbserver (remote)

[Pierre Muller]

> Nonetheless, if you pass target_gdbarch instead to ensure_python_env in
> python_inferior_exit (in python/py_inferior.c), things will behave much
> better. Once again, I don't know if this is strictly correct, but it
> should at least get you going with gdbserver again.
> 
> I hope this leads someone to a better solution.

  I investigated a little further and it seems to me
that the problem is that instead of a null_ptid,
the TARGET_WAITKIND_EXITED event has a (pid,0,0) form,
which is also not a valid thread, but doesn't get
filtered out yet by has_stack_frames  before
calling is_exited.


  I first wanted to add a test "lwp == 0 && tid == 0"
inside has_stack_frames, but as I was not sure that
other target could not have valid thread with a zero identifier,
I thought it would be safer to look for a fix inside
remote specific code.

  Inverting two lines in remote_close fixes hopefully this issue.
At least using --target_board=native_gdbsderver.exp
works again!

Is this patch OK?

 2011-02-18  Pierre Muller  <muller@ics.u-strasbg.fr>

	* remote.c (remote_close): Reset INFERIOR_PTID to NULL_PTID
	before calling discard_all_inferiors.

Index: src/gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.434
diff -u -p -r1.434 remote.c
--- src/gdb/remote.c	14 Feb 2011 11:22:29 -0000	1.434
+++ src/gdb/remote.c	18 Feb 2011 16:41:42 -0000
@@ -2908,9 +2908,11 @@ remote_close (int quitting)
   remote_desc = NULL;
 
   /* We don't have a connection to the remote stub anymore.  Get rid
-     of all the inferiors and their threads we were controlling.  */
-  discard_all_inferiors ();
+     of all the inferiors and their threads we were controlling.
+     Reset inferior_ptid to null_ptid first, as otherwise has_stack_frames
+     will be unable to find the thread corresponding to (pid, 0, 0).  */
   inferior_ptid = null_ptid;
+  discard_all_inferiors ();
 
   /* We're no longer interested in any of these events.  */
   discard_pending_stop_replies (-1);

Re: [RFA] Fix for testsuite errors with gdbserver (remote)

[Keith Seitz]

On 02/18/2011 08:48 AM, Pierre Muller wrote:
>    Inverting two lines in remote_close fixes hopefully this issue.
> At least using --target_board=native_gdbsderver.exp
> works again!

Duh. I didn't even notice the subsequent call to reset inferior_ptid!

I definitely like your patch better than the workaround I posted. 
Although I do wonder if there are any reasons why we might want to reset 
inferior_ptid after discard_all_inferiors (notifications?). Nonetheless, 
I'm the pragmatic sort: if the test suite doesn't reveal such problems, 
then none exist -- or we'll deal with them when we have hard test cases. :-)

Alas, I am not an ordained maintainer, so someone will still need to 
officially approve this.

Thank you for following-up on this.

Keith

Re: [RFA] Fix for testsuite errors with gdbserver (remote)

[Pedro Alves]

On Friday 18 February 2011 18:49:22, Keith Seitz wrote:
> On 02/18/2011 08:48 AM, Pierre Muller wrote:
> >    Inverting two lines in remote_close fixes hopefully this issue.
> > At least using --target_board=native_gdbsderver.exp
> > works again!
> 
> Duh. I didn't even notice the subsequent call to reset inferior_ptid!
> 
> I definitely like your patch better than the workaround I posted. 

I'm not convinced.  It looks like a workaround for something
the python exited observer isn't doing right.

-- 
Pedro Alves

Re: [RFA] Fix for testsuite errors with gdbserver (remote)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> I'm not convinced.  It looks like a workaround for something
Pedro> the python exited observer isn't doing right.

That could be, but what is that?

All that python_inferior_exit is doing to provoke the crash is calling
get_current_arch.  Under what circumstances is this not safe?  I would
have thought -- perhaps naively -- that it was always safe.

Tom

Re: [RFA] Fix for testsuite errors with gdbserver (remote)

[Tom Tromey]

Tom> All that python_inferior_exit is doing to provoke the crash is calling
Tom> get_current_arch.  Under what circumstances is this not safe?  I would
Tom> have thought -- perhaps naively -- that it was always safe.

This is maybe a little too brief.
get_current_arch calls has_stack_frames.

My view is that has_stack_frames relies on some invariants that should
hold for the various globals it accesses.  Maybe it is using them
incorrectly.  Or maybe the invariants, whatever they are, are violated
elsewhere.

I think Pierre's patch operates on the latter theory.  I don't really
know enough to know whether it is correct.

Tom

Re: [RFA] Fix for testsuite errors with gdbserver (remote)

[Pedro Alves]

On Friday 18 February 2011 20:30:15, Tom Tromey wrote:
> Tom> All that python_inferior_exit is doing to provoke the crash is calling
> Tom> get_current_arch.  Under what circumstances is this not safe?  I would
> Tom> have thought -- perhaps naively -- that it was always safe.

You should only call get_current_arch when preparing to run a command, as
default context gdbarch for the user commands (so "global" is really the
interpreter's default gdbarch).
That was part of the whole effort to eliminate the current_gdbarch global.
All other places should use the gdbarch associated with whatever
operation/object the code is doing/manipulating.  E.g., do something
in the current context while the inferior is running, use the current
frame's gdbarch.  Do something related to an obfile, use the obfile
gdbarch, etc.

At the top level, preparing to execute a command, the whole global
state (inferior_ptid, current_inferior, etc.) is in a state that
the core wants to see it in.  Several places in GDB need to
switch inferior_ptid to something else temporarily, at which point
you lose the mapping the core side relies on at most places, between
inferior_ptid, and a thread.  The tear down code paths 
do _not_ and cannot assume there's such a connection.
inferior_ptid will point to a whole process, not a thread.  There
are other places, but mostly at boundaries between the core, and
a target_ops.  Python is probably not being involved at such
times.  I hope, at least.

Now, the python hooks added a new subpath in the teardown path
that decided to query global state at a time the state is
transient, expecting to see it like the top level would.

Looking at the code,

 static void
 python_inferior_exit (struct inferior *inf)
 {
   struct cleanup *cleanup;
   LONGEST exit_code = -1;
   ptid_t ptidp;
   struct target_waitstatus status;

   cleanup = ensure_python_env (get_current_arch (), current_language);
 
   get_last_target_status (&ptidp, &status);

   exit_code = status.value.integer;

   if (exit_code >= 0
       && emit_exited_event (exit_code) < 0)
     gdbpy_print_stack ();

   do_cleanups (cleanup);
 }

I noted in <http://sourceware.org/ml/gdb/2011-02/msg00108.html>
that the get_current_arch() doesn't have any connection with
the INF argument.  It can be seen in the case at hand: we're
iterating over all inferiors and "exiting" them.  The current
inferior stays the same throughout the whole iteration.

I don't really understand the existence of python_gdbarch,
and why do we need to keep ensuring/preserving it throughout,
instead of using the most appropriate gdbarch whenever one
is necessary.

As I asked on the gdb@ list, what is the gdbarch that the
exited_event callback wants to see as current python_gdbarch?
One related to INF?  That's not where the whole current
context is pointing.  Or to whatever current thread or inferior
was selected at the top level before gdb started handling
the inferior's exit (think non-stop)?  Is it
just making sure the python_gdbarch remains unaltered
by the callback?  Then why not python_gdbarch instead of
get_current_arch()?  That is the "something" that I
was thinking this code may not be doing right.

That all said, it looks like the python api doesn't define
exactly what can be done on an ExitedEvent, and even if
we address the gdbarch issue, if it's intended that the
python user code can do whatever, we're likely to see python/gdb
calls there that try to do something that calls has_stack_frames,
and that will internal error too.  On that ground, I do think
we should put in Pierre's patch, as I'm not seeing what
possible harm can it do, and that gets things working again.
But I'll note that if the handler is allowed to assume the
whole api is available at this point, and that the current
global state can be used as if in the top-level, we'll
probably see other similar cases triggering.

-- 
Pedro Alves

RE: [RFA] Fix for testsuite errors with gdbserver (remote)

[Pierre Muller]

> That all said, it looks like the python api doesn't define
> exactly what can be done on an ExitedEvent, and even if
> we address the gdbarch issue, if it's intended that the
> python user code can do whatever, we're likely to see python/gdb
> calls there that try to do something that calls has_stack_frames,
> and that will internal error too.  On that ground, I do think
> we should put in Pierre's patch, as I'm not seeing what
> possible harm can it do, and that gets things working again.
> But I'll note that if the handler is allowed to assume the
> whole api is available at this point, and that the current
> global state can be used as if in the top-level, we'll
> probably see other similar cases triggering.


  Should I take this as an approval of my patch?
Or do you want me to wait some more in case someone
else has a better idea of how to fix this issue?

Pierre

Re: [RFA] Fix for testsuite errors with gdbserver (remote)

[Pedro Alves]

On Monday 21 February 2011 15:24:33, Pierre Muller wrote:
> > That all said, it looks like the python api doesn't define
> > exactly what can be done on an ExitedEvent, and even if
> > we address the gdbarch issue, if it's intended that the
> > python user code can do whatever, we're likely to see python/gdb
> > calls there that try to do something that calls has_stack_frames,
> > and that will internal error too.  On that ground, I do think
> > we should put in Pierre's patch, as I'm not seeing what
> > possible harm can it do, and that gets things working again.
> > But I'll note that if the handler is allowed to assume the
> > whole api is available at this point, and that the current
> > global state can be used as if in the top-level, we'll
> > probably see other similar cases triggering.
> 
> 
>   Should I take this as an approval of my patch?
> Or do you want me to wait some more in case someone
> else has a better idea of how to fix this issue?

Go ahead and put it in, thanks.

-- 
Pedro Alves