(maybe) Async mode failures on PPC

(maybe) Async mode failures on PPC

[Luis Machado]

Hey guys,

I was giving a shot at the testsuite for PPC and some failures showed
up, related, apparently, to the async mode.

The pattern is like this, first there's a message informing you that the
"Target is running" and then no other commands will be accepted due to
"Cannot execute this command while the selected thread is running.".

Is this something we're still addressing or a new problem? Should we
modify the testcase so it becomes async-aware?

For example, this piece of the watchpoint.exp testcase

(gdb) PASS: gdb.base/watchpoint.exp: until to ival3 assignment
until^M
Target is executing.^M
(gdb) FAIL: gdb.base/watchpoint.exp: until out of loop
step^M
Cannot execute this command while the selected thread is running.^M

Regards,
Luis

Re: (maybe) Async mode failures on PPC

[Luis Machado]

Steps to reproduce this failure:

1 - Load the watchpoint.exp test binary into gdb.
2 - start it and next over a few lines.

Results:

(gdb) start
Temporary breakpoint 1 at 0x10000894:
file /home/luis/src/gdb/gdb-head/HEAD/gdb/testsuite/gdb.base/watchpoint.c, line 129.
Starting
program: /home/luis/builds/gdb-head/DFP/gdb/testsuite/gdb.base/watchpoint

Temporary breakpoint 1, main ()
at /home/luis/src/gdb/gdb-head/HEAD/gdb/testsuite/gdb.base/watchpoint.c:129
129       struct1.val = 1;
(gdb) watch ptr1
Hardware watchpoint 2: ptr1
(gdb) n
During symbol reading, incomplete CFI data; unspecified registers (e.g.,
r2) at 0x10000894.
130       struct2.val = 2;
(gdb)
131       ptr1 = &struct1;
(gdb)
Target is executing.
(gdb)
Cannot execute this command while the selected thread is running.
(gdb)
Cannot execute this command while the selected thread is running.
(gdb)
Cannot execute this command while the selected thread is running.
(gdb)


On Mon, 2008-07-14 at 12:44 -0300, Luis Machado wrote:
> Hey guys,
> 
> I was giving a shot at the testsuite for PPC and some failures showed
> up, related, apparently, to the async mode.
> 
> The pattern is like this, first there's a message informing you that the
> "Target is running" and then no other commands will be accepted due to
> "Cannot execute this command while the selected thread is running.".
> 
> Is this something we're still addressing or a new problem? Should we
> modify the testcase so it becomes async-aware?
> 
> For example, this piece of the watchpoint.exp testcase
> 
> (gdb) PASS: gdb.base/watchpoint.exp: until to ival3 assignment
> until^M
> Target is executing.^M
> (gdb) FAIL: gdb.base/watchpoint.exp: until out of loop
> step^M
> Cannot execute this command while the selected thread is running.^M
> 
> Regards,
> Luis

Re: (maybe) Async mode failures on PPC

[Pedro Alves]

Hi Luis,

On Monday 14 July 2008 17:31:42, Luis Machado wrote:
> Steps to reproduce this failure:
>
> 1 - Load the watchpoint.exp test binary into gdb.
> 2 - start it and next over a few lines.

I can't reproduce it on x86 (sync or async), neither with your
recipe or by running the watchpoint.exp test.

:-(

The "Target is executing" notice comes from frame.c:get_current_frame.

I couldn't figure out for sure from your description of the
problem if this only happens in async, or if it happen in sync as well,
but I understood async only.

Why is GDB trying to get the current frame of an executing
thread, or why does GDB think that the current thread is
executing at this point, if it isn't?

Can you do a bit of debugging and figure out what is the
code path that ended up triggering the first error?
get_current_frame is called from too many places to be able
to do an educated guess.

-- 
Pedro Alves

Re: (maybe) Async mode failures on PPC

[Luis Machado]

Hey Pedro,

On Mon, 2008-07-14 at 17:57 +0100, Pedro Alves wrote:
> Hi Luis,
> 
> On Monday 14 July 2008 17:31:42, Luis Machado wrote:
> > Steps to reproduce this failure:
> >
> > 1 - Load the watchpoint.exp test binary into gdb.
> > 2 - start it and next over a few lines.
> 
> I can't reproduce it on x86 (sync or async), neither with your
> recipe or by running the watchpoint.exp test.

It seems to be a ppc-specific thing. I forgot to mention that you need
to place a watchpoint on variable "ptr1" to trigger this. When no
watchpoints are inserted, it runs as expected. And this is sync mode, i
did not turn async on, and i presume it's off by default, like it was
until recently.

> :-(
> 
> The "Target is executing" notice comes from frame.c:get_current_frame.
> 
> I couldn't figure out for sure from your description of the
> problem if this only happens in async, or if it happen in sync as well,
> but I understood async only.
> 
> Why is GDB trying to get the current frame of an executing
> thread, or why does GDB think that the current thread is
> executing at this point, if it isn't?
> 
> Can you do a bit of debugging and figure out what is the
> code path that ended up triggering the first error?
> get_current_frame is called from too many places to be able
> to do an educated guess.

Yes, sorry. I was just wondering if anyone else was having the same
problem and did not provide much info about it. I'll dig into it and
will provide the code path for the error as it seems to be a regression.
But i feel it's related somehow to watchpoints.

Regards,
Luis

Re: (maybe) Async mode failures on PPC

[Andreas Schwab]

Pedro Alves <pedro@codesourcery.com> writes:

> Can you do a bit of debugging and figure out what is the
> code path that ended up triggering the first error?

handle_inferior_event is calling insert_breakpoints before
set_executing.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: (maybe) Async mode failures on PPC

[Luis Machado]

On Mon, 2008-07-14 at 14:06 -0300, Luis Machado wrote:
> Hey Pedro,
> 
> On Mon, 2008-07-14 at 17:57 +0100, Pedro Alves wrote:
> > Hi Luis,
> > 
> > On Monday 14 July 2008 17:31:42, Luis Machado wrote:
> > > Steps to reproduce this failure:
> > >
> > > 1 - Load the watchpoint.exp test binary into gdb.
> > > 2 - start it and next over a few lines.
> > 
> > I can't reproduce it on x86 (sync or async), neither with your
> > recipe or by running the watchpoint.exp test.
> 
> It seems to be a ppc-specific thing. I forgot to mention that you need
> to place a watchpoint on variable "ptr1" to trigger this. When no
> watchpoints are inserted, it runs as expected. And this is sync mode,
> i
> did not turn async on, and i presume it's off by default, like it was
> until recently.
> 
> > :-(
> > 
> > The "Target is executing" notice comes from
> frame.c:get_current_frame.
> > 
> > I couldn't figure out for sure from your description of the
> > problem if this only happens in async, or if it happen in sync as
> well,
> > but I understood async only.
> > 
> > Why is GDB trying to get the current frame of an executing
> > thread, or why does GDB think that the current thread is
> > executing at this point, if it isn't?
> > 
> > Can you do a bit of debugging and figure out what is the
> > code path that ended up triggering the first error?
> > get_current_frame is called from too many places to be able
> > to do an educated guess.
> 
> Yes, sorry. I was just wondering if anyone else was having the same
> problem and did not provide much info about it. I'll dig into it and
> will provide the code path for the error as it seems to be a
> regression.
> But i feel it's related somehow to watchpoints.
> 
> Regards,
> Luis

Here is a patch i discussed with Pedro to fix this regression. Tested
and nothing showed up in the testsuite.

Is this OK?


2008-07-15  Luis Machado  <luisgpm@br.ibm.com>

	* infrun.c (handle_inferior_event): reinit_frame_cache before
	handling events on threads. 

Index: gdb/infrun.c
===================================================================
--- gdb.orig/infrun.c	2008-07-15 09:36:19.000000000 -0500
+++ gdb/infrun.c	2008-07-15 09:37:20.000000000 -0500
@@ -1828,6 +1828,29 @@
 
   adjust_pc_after_break (ecs);
 
+  reinit_frame_cache ();
+
+  /* If it's a new process, add it to the thread database */
+
+  ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
+			   && !ptid_equal (ecs->ptid, minus_one_ptid)
+			   && !in_thread_list (ecs->ptid));
+
+  if (ecs->ws.kind != TARGET_WAITKIND_EXITED
+      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event)
+    add_thread (ecs->ptid);
+
+  if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
+    {
+      /* Mark the non-executing threads accordingly.  */
+      if (!non_stop
+ 	  || ecs->ws.kind == TARGET_WAITKIND_EXITED
+ 	  || ecs->ws.kind == TARGET_WAITKIND_SIGNALLED)
+ 	set_executing (pid_to_ptid (-1), 0);
+      else
+ 	set_executing (ecs->ptid, 0);
+    }
+
   switch (infwait_state)
     {
     case infwait_thread_hop_state:
@@ -1867,29 +1890,6 @@
     }
   infwait_state = infwait_normal_state;
 
-  reinit_frame_cache ();
-
-  /* If it's a new process, add it to the thread database */
-
-  ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
-			   && !ptid_equal (ecs->ptid, minus_one_ptid)
-			   && !in_thread_list (ecs->ptid));
-
-  if (ecs->ws.kind != TARGET_WAITKIND_EXITED
-      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event)
-    add_thread (ecs->ptid);
-
-  if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
-    {
-      /* Mark the non-executing threads accordingly.  */
-      if (!non_stop
- 	  || ecs->ws.kind == TARGET_WAITKIND_EXITED
- 	  || ecs->ws.kind == TARGET_WAITKIND_SIGNALLED)
- 	set_executing (pid_to_ptid (-1), 0);
-      else
- 	set_executing (ecs->ptid, 0);
-    }
-
   switch (ecs->ws.kind)
     {
     case TARGET_WAITKIND_LOADED:

Re: (maybe) Async mode failures on PPC

[Pedro Alves]

Hi Luis,

Thanks for taking care of this.

> 2008-07-15  Luis Machado  <luisgpm@br.ibm.com>
>
> 	* infrun.c (handle_inferior_event): reinit_frame_cache before
> 	handling events on threads.

As I noted on IRC, this is a bit incomplete, but I know you're already
handling it.  :-)

On Tuesday 15 July 2008 15:46:40, Luis Machado wrote:
> On Mon, 2008-07-14 at 14:06 -0300, Luis Machado wrote:
> Here is a patch i discussed with Pedro to fix this regression. Tested
> and nothing showed up in the testsuite.

For everyone else not following the discussion on IRC:

The main issue was the insert_breakpoints call being done
before the stopped thread being tagged as stopped, as Andreas
also noticed.  Inside insert_breakpoints, when updating
watchpoints, there are calls into frame routines, which
bail out if the thread is considered to be running.  It isn't
at this point, but we were only tagging it as stopped a bit
afterwards.

handle_inferior_event()
{
...
  switch (infwait_state)
    {
...
    case infwait_nonstep_watch_state:
      if (debug_infrun)
        fprintf_unfiltered (gdb_stdlog,
			    "infrun: infwait_nonstep_watch_state\n");
      insert_breakpoints ();
...
      break;
...

    }
...

  reinit_frame_cache ();

  /* If it's a new process, add it to the thread database */

  ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
			   && !ptid_equal (ecs->ptid, minus_one_ptid)
			   && !in_thread_list (ecs->ptid));

  if (ecs->ws.kind != TARGET_WAITKIND_EXITED
      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event)
    add_thread (ecs->ptid);

  if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
    {
      /* Mark the non-executing threads accordingly.  */
      if (!non_stop
 	  || ecs->ws.kind == TARGET_WAITKIND_EXITED
 	  || ecs->ws.kind == TARGET_WAITKIND_SIGNALLED)
 	set_executing (pid_to_ptid (-1), 0);
      else
 	set_executing (ecs->ptid, 0);
    }
...




> Index: gdb/infrun.c
> ===================================================================
> --- gdb.orig/infrun.c	2008-07-15 09:36:19.000000000 -0500
> +++ gdb/infrun.c	2008-07-15 09:37:20.000000000 -0500
> @@ -1828,6 +1828,29 @@
>
>    adjust_pc_after_break (ecs);
>
> +  reinit_frame_cache ();

As seen above, this was being done only after the insert_breakpoints call.
Inserting breakpoints ends up updating the watchpoints, which
triggers calls into frame handling (evaluating if the frame is
in scope, temporalily changing the selected frame, etc.).

There's a registers_changed call just before resuming and switching
to infwait_nonstep_watch_state (which calls reinit_frame_cache),
and we tell the target to step only one thread, so there should be no 
practical problem in leaving the call where it was, but, might
as well move up this call too while we're at it, in case something
else changes in the future, and uncovers this issue.

> +
> +  /* If it's a new process, add it to the thread database */
> +
> +  ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
> +			   && !ptid_equal (ecs->ptid, minus_one_ptid)
> +			   && !in_thread_list (ecs->ptid));
> +
> +  if (ecs->ws.kind != TARGET_WAITKIND_EXITED
> +      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED &&
> ecs->new_thread_event) +    add_thread (ecs->ptid);

This moves up along, so any new thread that ends up being added by
this is also tagged as stopped.  It's the default thread state, when
one adds a new thread, but we shouldn't rely on that.

> +
> +  if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
> +    {
> +      /* Mark the non-executing threads accordingly.  */
> +      if (!non_stop
> + 	  || ecs->ws.kind == TARGET_WAITKIND_EXITED
> + 	  || ecs->ws.kind == TARGET_WAITKIND_SIGNALLED)
> + 	set_executing (pid_to_ptid (-1), 0);
> +      else
> + 	set_executing (ecs->ptid, 0);
> +    }
> +

This moves up, because doing this later is what was creating
the problem in the first place, due to insert_breakpoints
ending up calling get_current_frame.

-- 
Pedro Alves

Re: (maybe) Async mode failures on PPC

[Daniel Jacobowitz]

On Tue, Jul 15, 2008 at 05:34:30PM +0100, Pedro Alves wrote:
> For everyone else not following the discussion on IRC:

Just checking: is the summary of your message "the patch looks OK to
me except for the changelog entry"?

-- 
Daniel Jacobowitz
CodeSourcery

Re: (maybe) Async mode failures on PPC

[Pedro Alves]

A Tuesday 15 July 2008 17:39:32, Daniel Jacobowitz wrote:
> On Tue, Jul 15, 2008 at 05:34:30PM +0100, Pedro Alves wrote:
> > For everyone else not following the discussion on IRC:
>
> Just checking: is the summary of your message "the patch looks OK to
> me except for the changelog entry"?

Yes.

-- 
Pedro Alves

Re: (maybe) Async mode failures on PPC

[Luis Machado]

On Tue, 2008-07-15 at 11:46 -0300, Luis Machado wrote:
> On Mon, 2008-07-14 at 14:06 -0300, Luis Machado wrote:
> > Hey Pedro,
> > 
> > On Mon, 2008-07-14 at 17:57 +0100, Pedro Alves wrote:
> > > Hi Luis,
> > > 
> > > On Monday 14 July 2008 17:31:42, Luis Machado wrote:
> > > > Steps to reproduce this failure:
> > > >
> > > > 1 - Load the watchpoint.exp test binary into gdb.
> > > > 2 - start it and next over a few lines.
> > > 
> > > I can't reproduce it on x86 (sync or async), neither with your
> > > recipe or by running the watchpoint.exp test.
> > 
> > It seems to be a ppc-specific thing. I forgot to mention that you need
> > to place a watchpoint on variable "ptr1" to trigger this. When no
> > watchpoints are inserted, it runs as expected. And this is sync mode,
> > i
> > did not turn async on, and i presume it's off by default, like it was
> > until recently.
> > 
> > > :-(
> > > 
> > > The "Target is executing" notice comes from
> > frame.c:get_current_frame.
> > > 
> > > I couldn't figure out for sure from your description of the
> > > problem if this only happens in async, or if it happen in sync as
> > well,
> > > but I understood async only.
> > > 
> > > Why is GDB trying to get the current frame of an executing
> > > thread, or why does GDB think that the current thread is
> > > executing at this point, if it isn't?
> > > 
> > > Can you do a bit of debugging and figure out what is the
> > > code path that ended up triggering the first error?
> > > get_current_frame is called from too many places to be able
> > > to do an educated guess.
> > 
> > Yes, sorry. I was just wondering if anyone else was having the same
> > problem and did not provide much info about it. I'll dig into it and
> > will provide the code path for the error as it seems to be a
> > regression.
> > But i feel it's related somehow to watchpoints.
> > 
> > Regards,
> > Luis
> 
> Here is a patch i discussed with Pedro to fix this regression. Tested
> and nothing showed up in the testsuite.
> 
> Is this OK?

Updated the changelog entry according to Pedro's suggestion.

2008-07-15  Luis Machado  <luisgpm@br.ibm.com>

	* infrun.c (handle_inferior_event): Tag threads as stopped
	before inserting breakpoints. 

Index: gdb/infrun.c
===================================================================
--- gdb.orig/infrun.c	2008-07-15 09:36:19.000000000 -0500
+++ gdb/infrun.c	2008-07-15 09:37:20.000000000 -0500
@@ -1828,6 +1828,29 @@
 
   adjust_pc_after_break (ecs);
 
+  reinit_frame_cache ();
+
+  /* If it's a new process, add it to the thread database */
+
+  ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
+			   && !ptid_equal (ecs->ptid, minus_one_ptid)
+			   && !in_thread_list (ecs->ptid));
+
+  if (ecs->ws.kind != TARGET_WAITKIND_EXITED
+      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event)
+    add_thread (ecs->ptid);
+
+  if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
+    {
+      /* Mark the non-executing threads accordingly.  */
+      if (!non_stop
+ 	  || ecs->ws.kind == TARGET_WAITKIND_EXITED
+ 	  || ecs->ws.kind == TARGET_WAITKIND_SIGNALLED)
+ 	set_executing (pid_to_ptid (-1), 0);
+      else
+ 	set_executing (ecs->ptid, 0);
+    }
+
   switch (infwait_state)
     {
     case infwait_thread_hop_state:
@@ -1867,29 +1890,6 @@
     }
   infwait_state = infwait_normal_state;
 
-  reinit_frame_cache ();
-
-  /* If it's a new process, add it to the thread database */
-
-  ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
-			   && !ptid_equal (ecs->ptid, minus_one_ptid)
-			   && !in_thread_list (ecs->ptid));
-
-  if (ecs->ws.kind != TARGET_WAITKIND_EXITED
-      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event)
-    add_thread (ecs->ptid);
-
-  if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
-    {
-      /* Mark the non-executing threads accordingly.  */
-      if (!non_stop
- 	  || ecs->ws.kind == TARGET_WAITKIND_EXITED
- 	  || ecs->ws.kind == TARGET_WAITKIND_SIGNALLED)
- 	set_executing (pid_to_ptid (-1), 0);
-      else
- 	set_executing (ecs->ptid, 0);
-    }
-
   switch (ecs->ws.kind)
     {
     case TARGET_WAITKIND_LOADED:

Re: (maybe) Async mode failures on PPC

[Daniel Jacobowitz]

On Tue, Jul 15, 2008 at 01:44:49PM -0300, Luis Machado wrote:
> 2008-07-15  Luis Machado  <luisgpm@br.ibm.com>
> 
> 	* infrun.c (handle_inferior_event): Tag threads as stopped
> 	before inserting breakpoints. 

OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: (maybe) Async mode failures on PPC

[Luis Machado]

Thanks folks. Checked it in.

On Tue, 2008-07-15 at 12:49 -0400, Daniel Jacobowitz wrote:
> On Tue, Jul 15, 2008 at 01:44:49PM -0300, Luis Machado wrote:
> > 2008-07-15  Luis Machado  <luisgpm@br.ibm.com>
> > 
> > 	* infrun.c (handle_inferior_event): Tag threads as stopped
> > 	before inserting breakpoints. 
> 
> OK.
>