[RFC] When debugging infrun, print stop_pc in all possible cases.

[RFC] When debugging infrun, print stop_pc in all possible cases.

[Doug Evans]

Hi.

I've been debugging a few issues and it's been really helpful to know
stop_pc in cases that aren't printed today.
This patch moves the debug printing of stop_pc up.

I'm not submitting it RFA because I'm not entirely happy with it.
Printing stop_pc has been helpful, I'd like to improve what's there today,
but it's not clear to me that this patch reasonably handles the cases
when one can't read pc before the big switch() on ecs->ws.kind.

Comments?

2009-02-26  Doug Evans  <dje@google.com>

	* infrun.c (handle_inferior_event): Move the debug printing of stop_pc,
	and possible watchpoint address, to an earlier point so we print it
	in all cases (where it's possible).

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.360
diff -u -p -r1.360 infrun.c
--- infrun.c	25 Feb 2009 02:14:22 -0000	1.360
+++ infrun.c	26 Feb 2009 17:52:28 -0000
@@ -2178,6 +2178,46 @@ handle_inferior_event (struct execution_
   /* Dependent on the current PC value modified by adjust_pc_after_break.  */
   reinit_frame_cache ();
 
+  if (debug_infrun)
+    {
+      /* It's helpful to know the stop pc in all cases when we stop
+	 (even if we're eventually going to resume).
+	 To keep things simple we print it here before the big switch on
+	 ecs->ws.kind.  If we leave printing of stop_pc until later,
+	 we will miss the cases where we "return;" early.
+	 There are a few cases where we can't read pc though.  */
+
+      switch (ecs->ws.kind)
+	{
+	case TARGET_WAITKIND_EXITED:
+	case TARGET_WAITKIND_SIGNALLED:
+	  break;
+
+	default:
+	  {
+	    CORE_ADDR debug_stop_pc =
+	      regcache_read_pc (get_thread_regcache (ecs->ptid));
+
+	    fprintf_unfiltered (gdb_stdlog, "infrun: stop_pc = 0x%s\n",
+				paddr_nz (debug_stop_pc));
+
+	    if (STOPPED_BY_WATCHPOINT (&ecs->ws))
+	      {
+		CORE_ADDR addr;
+
+		if (target_stopped_data_address (&current_target, &addr))
+		  fprintf_unfiltered (gdb_stdlog,
+				      "infrun: stopped by watchpoint, data address = 0x%s\n",
+				      paddr_nz (addr));
+		else
+		  fprintf_unfiltered (gdb_stdlog,
+				      "infrun: stopped by watchpoint, no data address available\n");
+	      }
+	    break;
+	  }
+	}
+    }
+
   if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
     {
       breakpoint_retire_moribund ();
@@ -2507,25 +2547,6 @@ targets should add new threads to the th
 
   stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));
 
-  if (debug_infrun)
-    {
-      fprintf_unfiltered (gdb_stdlog, "infrun: stop_pc = 0x%s\n",
-                          paddr_nz (stop_pc));
-      if (STOPPED_BY_WATCHPOINT (&ecs->ws))
-	{
-          CORE_ADDR addr;
-	  fprintf_unfiltered (gdb_stdlog, "infrun: stopped by watchpoint\n");
-
-          if (target_stopped_data_address (&current_target, &addr))
-            fprintf_unfiltered (gdb_stdlog,
-                                "infrun: stopped data address = 0x%s\n",
-                                paddr_nz (addr));
-          else
-            fprintf_unfiltered (gdb_stdlog,
-                                "infrun: (no data address available)\n");
-	}
-    }
-
   if (stepping_past_singlestep_breakpoint)
     {
       gdb_assert (singlestep_breakpoints_inserted_p);

Re: [RFC] When debugging infrun, print stop_pc in all possible cases.

[Doug Evans]

On Thu, Feb 26, 2009 at 11:01 AM, Doug Evans <dje@google.com> wrote:
> Hi.
>
> I've been debugging a few issues and it's been really helpful to know
> stop_pc in cases that aren't printed today.
> This patch moves the debug printing of stop_pc up.
>
> I'm not submitting it RFA because I'm not entirely happy with it.
> Printing stop_pc has been helpful, I'd like to improve what's there today,
> but it's not clear to me that this patch reasonably handles the cases
> when one can't read pc before the big switch() on ecs->ws.kind.
>
> Comments?
>
> 2009-02-26  Doug Evans  <dje@google.com>
>
>        * infrun.c (handle_inferior_event): Move the debug printing of stop_pc,
>        and possible watchpoint address, to an earlier point so we print it
>        in all cases (where it's possible).
>
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.360
> diff -u -p -r1.360 infrun.c
> --- infrun.c    25 Feb 2009 02:14:22 -0000      1.360
> +++ infrun.c    26 Feb 2009 17:52:28 -0000
> @@ -2178,6 +2178,46 @@ handle_inferior_event (struct execution_
>   /* Dependent on the current PC value modified by adjust_pc_after_break.  */
>   reinit_frame_cache ();
>
> +  if (debug_infrun)
> +    {
> +      /* It's helpful to know the stop pc in all cases when we stop
> +        (even if we're eventually going to resume).
> +        To keep things simple we print it here before the big switch on
> +        ecs->ws.kind.  If we leave printing of stop_pc until later,
> +        we will miss the cases where we "return;" early.
> +        There are a few cases where we can't read pc though.  */
> +
> +      switch (ecs->ws.kind)
> +       {
> +       case TARGET_WAITKIND_EXITED:
> +       case TARGET_WAITKIND_SIGNALLED:
> +         break;
> +
> +       default:
> +         {
> +           CORE_ADDR debug_stop_pc =
> +             regcache_read_pc (get_thread_regcache (ecs->ptid));
> +
> +           fprintf_unfiltered (gdb_stdlog, "infrun: stop_pc = 0x%s\n",
> +                               paddr_nz (debug_stop_pc));
> +
> +           if (STOPPED_BY_WATCHPOINT (&ecs->ws))
> +             {
> +               CORE_ADDR addr;
> +
> +               if (target_stopped_data_address (&current_target, &addr))
> +                 fprintf_unfiltered (gdb_stdlog,
> +                                     "infrun: stopped by watchpoint, data address = 0x%s\n",
> +                                     paddr_nz (addr));
> +               else
> +                 fprintf_unfiltered (gdb_stdlog,
> +                                     "infrun: stopped by watchpoint, no data address available\n");
> +             }
> +           break;
> +         }
> +       }
> +    }
> +
>   if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
>     {
>       breakpoint_retire_moribund ();
> @@ -2507,25 +2547,6 @@ targets should add new threads to the th
>
>   stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));
>
> -  if (debug_infrun)
> -    {
> -      fprintf_unfiltered (gdb_stdlog, "infrun: stop_pc = 0x%s\n",
> -                          paddr_nz (stop_pc));
> -      if (STOPPED_BY_WATCHPOINT (&ecs->ws))
> -       {
> -          CORE_ADDR addr;
> -         fprintf_unfiltered (gdb_stdlog, "infrun: stopped by watchpoint\n");
> -
> -          if (target_stopped_data_address (&current_target, &addr))
> -            fprintf_unfiltered (gdb_stdlog,
> -                                "infrun: stopped data address = 0x%s\n",
> -                                paddr_nz (addr));
> -          else
> -            fprintf_unfiltered (gdb_stdlog,
> -                                "infrun: (no data address available)\n");
> -       }
> -    }
> -
>   if (stepping_past_singlestep_breakpoint)
>     {
>       gdb_assert (singlestep_breakpoints_inserted_p);
>


Ping.

Any comments on this approach?
Would you do it differently?
[E.g. One could add code to print the pc to all the appropriate cases
in the switch(), but that seems clumsy.]

Re: [RFC] When debugging infrun, print stop_pc in all possible cases.

[Pedro Alves]

On Friday 13 March 2009 17:06:13, Doug Evans wrote:
> On Thu, Feb 26, 2009 at 11:01 AM, Doug Evans <dje@google.com> wrote:
> > Hi.
> >
> > I've been debugging a few issues and it's been really helpful to know
> > stop_pc in cases that aren't printed today.
> > This patch moves the debug printing of stop_pc up.
> >
> > I'm not submitting it RFA because I'm not entirely happy with it.
> > Printing stop_pc has been helpful, I'd like to improve what's there today,
> > but it's not clear to me that this patch reasonably handles the cases
> > when one can't read pc before the big switch() on ecs->ws.kind.

Hmm,:

 - not ignoring TARGET_WAITKIND_IGNORE.
 - accessing the target before the set_executing call.  It may end
   up being a problem in the future.
 - With displaced stepping, we lost the printing of the stop_pc
   *after* the fixup.
 - I'm not sure we want to move the watchpoint debug bits
   as well.

> Any comments on this approach?
> Would you do it differently?
> [E.g. One could add code to print the pc to all the appropriate cases
> in the switch(), but that seems clumsy.]

Dunno, perhaps, come up with a "set_stop_pc (CORE_ADDR)" function, do
the debug printing there, and replace all the appropriate accesses
to that global by a call to the new function?

This is probably one of the cases where I'd hack the debug
output, fix the issue, and then remove the hack and forget
about it.  :-)

> >
> > Comments?
> >
> > 2009-02-26  Doug Evans  <dje@google.com>
> >
> >        * infrun.c (handle_inferior_event): Move the debug printing of stop_pc,
> >        and possible watchpoint address, to an earlier point so we print it
> >        in all cases (where it's possible).
> >
> > Index: infrun.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/infrun.c,v
> > retrieving revision 1.360
> > diff -u -p -r1.360 infrun.c
> > --- infrun.c    25 Feb 2009 02:14:22 -0000      1.360
> > +++ infrun.c    26 Feb 2009 17:52:28 -0000
> > @@ -2178,6 +2178,46 @@ handle_inferior_event (struct execution_
> >   /* Dependent on the current PC value modified by adjust_pc_after_break.  */
> >   reinit_frame_cache ();
> >
> > +  if (debug_infrun)
> > +    {
> > +      /* It's helpful to know the stop pc in all cases when we stop
> > +        (even if we're eventually going to resume).
> > +        To keep things simple we print it here before the big switch on
> > +        ecs->ws.kind.  If we leave printing of stop_pc until later,
> > +        we will miss the cases where we "return;" early.
> > +        There are a few cases where we can't read pc though.  */
> > +
> > +      switch (ecs->ws.kind)
> > +       {
> > +       case TARGET_WAITKIND_EXITED:
> > +       case TARGET_WAITKIND_SIGNALLED:
> > +         break;
> > +
> > +       default:
> > +         {
> > +           CORE_ADDR debug_stop_pc =
> > +             regcache_read_pc (get_thread_regcache (ecs->ptid));
> > +
> > +           fprintf_unfiltered (gdb_stdlog, "infrun: stop_pc = 0x%s\n",
> > +                               paddr_nz (debug_stop_pc));
> > +
> > +           if (STOPPED_BY_WATCHPOINT (&ecs->ws))
> > +             {
> > +               CORE_ADDR addr;
> > +
> > +               if (target_stopped_data_address (&current_target, &addr))
> > +                 fprintf_unfiltered (gdb_stdlog,
> > +                                     "infrun: stopped by watchpoint, data address = 0x%s\n",
> > +                                     paddr_nz (addr));
> > +               else
> > +                 fprintf_unfiltered (gdb_stdlog,
> > +                                     "infrun: stopped by watchpoint, no data address available\n");
> > +             }
> > +           break;
> > +         }
> > +       }
> > +    }
> > +
> >   if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
> >     {
> >       breakpoint_retire_moribund ();
> > @@ -2507,25 +2547,6 @@ targets should add new threads to the th
> >
> >   stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));
> >
> > -  if (debug_infrun)
> > -    {
> > -      fprintf_unfiltered (gdb_stdlog, "infrun: stop_pc = 0x%s\n",
> > -                          paddr_nz (stop_pc));
> > -      if (STOPPED_BY_WATCHPOINT (&ecs->ws))
> > -       {
> > -          CORE_ADDR addr;
> > -         fprintf_unfiltered (gdb_stdlog, "infrun: stopped by watchpoint\n");
> > -
> > -          if (target_stopped_data_address (&current_target, &addr))
> > -            fprintf_unfiltered (gdb_stdlog,
> > -                                "infrun: stopped data address = 0x%s\n",
> > -                                paddr_nz (addr));
> > -          else
> > -            fprintf_unfiltered (gdb_stdlog,
> > -                                "infrun: (no data address available)\n");
> > -       }
> > -    }
> > -
> >   if (stepping_past_singlestep_breakpoint)
> >     {
> >       gdb_assert (singlestep_breakpoints_inserted_p);
> >
> 

-- 
Pedro Alves