[Patch 1/2] infrun.c support for MIPS hardware watchpoints.

[Patch 1/2] infrun.c support for MIPS hardware watchpoints.

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

For targets that are gdbarch_software_single_step_p (like MIPS), we need to use software single stepping when stepping over a watchpoint.  The existing code that sets up software single step was in resume().

This patch factors the software single step code into a new function, set_for_singlestep(), and then calls it from handle_inferior_event() when we need to step over a watchpoint.  This is a prerequisite for the following patch that adds MIPS hardware watchpoint support.

Tested on x86_64-unknown-linux-gnu (Fedora 10) with no regressions found.

OK to commit?


[-- Attachment #2: infrun.patch --]
[-- Type: text/plain, Size: 2892 bytes --]

2009-04-05  David Daney  <ddaney@caviumnetworks.com>

	* infrun.c (set_for_singlestep): New function.
	(resume): Call set_for_singlestep.
	(handle_inferior_event): Same.


Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.367
diff -u -p -r1.367 infrun.c
--- infrun.c	25 Mar 2009 21:53:10 -0000	1.367
+++ infrun.c	6 Apr 2009 05:45:38 -0000
@@ -950,6 +950,28 @@ set_schedlock_func (char *args, int from
     }
 }
 
+/* Try to setup for software single stepping over the specified location.
+   Return 1 if target_resume() should use hardware single step.
+
+   GDBARCH the current gdbarch.
+   PC the location to step over.  */
+static int
+set_for_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  int step = 1;
+
+  if (gdbarch_software_single_step_p (gdbarch)
+      && gdbarch_software_single_step (gdbarch, get_current_frame ()))
+    {
+      step = 0;
+      /* Do not pull these breakpoints until after a `wait' in
+	 `wait_for_inferior' */
+      singlestep_breakpoints_inserted_p = 1;
+      singlestep_ptid = inferior_ptid;
+      singlestep_pc = pc;
+    }
+  return step;
+}
 
 /* Resume the inferior, but allow a QUIT.  This is useful if the user
    wants to interrupt some lengthy single-stepping operation
@@ -1031,20 +1053,9 @@ a command like `return' or `jump' to con
 	}
     }
 
-  if (step && gdbarch_software_single_step_p (gdbarch))
-    {
-      /* Do it the hard way, w/temp breakpoints */
-      if (gdbarch_software_single_step (gdbarch, get_current_frame ()))
-        {
-          /* ...and don't ask hardware to do it.  */
-          step = 0;
-          /* and do not pull these breakpoints until after a `wait' in
-          `wait_for_inferior' */
-          singlestep_breakpoints_inserted_p = 1;
-          singlestep_ptid = inferior_ptid;
-          singlestep_pc = pc;
-        }
-    }
+  /* Do we need to do it the hard way, w/temp breakpoints?  */
+  if (step)
+      step = set_for_singlestep (gdbarch, pc);
 
   /* If there were any forks/vforks/execs that were caught and are
      now to be followed, then do so.  */
@@ -2826,11 +2837,14 @@ targets should add new threads to the th
 	 the inferior over it.  If we have non-steppable watchpoints,
 	 we must disable the current watchpoint; it's simplest to
 	 disable all watchpoints and breakpoints.  */
-	 
+      int step_over_watchpoint = 1;
+
       if (!HAVE_STEPPABLE_WATCHPOINT)
 	remove_breakpoints ();
       registers_changed ();
-      target_resume (ecs->ptid, 1, TARGET_SIGNAL_0);	/* Single step */
+	/* Single step */
+      step_over_watchpoint = set_for_singlestep (current_gdbarch, read_pc ());
+      target_resume (ecs->ptid, step_over_watchpoint, TARGET_SIGNAL_0);
       waiton_ptid = ecs->ptid;
       if (HAVE_STEPPABLE_WATCHPOINT)
 	infwait_state = infwait_step_watch_state;

Re: [Patch 1/2] infrun.c support for MIPS hardware watchpoints.

[Pedro Alves]

On Monday 06 April 2009 07:49:11, David Daney wrote:

> +/* Try to setup for software single stepping over the specified location.
> +   Return 1 if target_resume() should use hardware single step.
> +
> +   GDBARCH the current gdbarch.
> +   PC the location to step over.  */
> +static int

Add an empty line between comment and function, please.

> +set_for_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{
> +  int step = 1;
> +
> +  if (gdbarch_software_single_step_p (gdbarch)
> +      && gdbarch_software_single_step (gdbarch, get_current_frame ()))
> +    {
> +      step = 0;
> +      /* Do not pull these breakpoints until after a `wait' in
> +        `wait_for_inferior' */
> +      singlestep_breakpoints_inserted_p = 1;
> +      singlestep_ptid = inferior_ptid;
> +      singlestep_pc = pc;
> +    }
> +  return step;
> +}

Because I'm dumb, while reading this, 'set_for_singlestep' and
'int step' didn't cross my bridge.  How about renaming the local
variable 'hw_step'?  I don't like the "set_for_singlestep" name much.
It isn't much self-describing, which is something very important in
infrun.c, since it contains horrible, huge, full-of-states, hard to
read code --- it is write-once, read-and-debug-millions-of-times
code.  But, I tried to come up with a descriptive name to suggest
for it, and I didn't come up with something that made me happy,
so, ... that's OK.

>  
>  /* Resume the inferior, but allow a QUIT.  This is useful if the user
>     wants to interrupt some lengthy single-stepping operation
> @@ -1031,20 +1053,9 @@ a command like `return' or `jump' to con
>         }
>      }
>  
> -  if (step && gdbarch_software_single_step_p (gdbarch))
> -    {
> -      /* Do it the hard way, w/temp breakpoints */
> -      if (gdbarch_software_single_step (gdbarch, get_current_frame ()))
> -        {
> -          /* ...and don't ask hardware to do it.  */
> -          step = 0;
> -          /* and do not pull these breakpoints until after a `wait' in
> -          `wait_for_inferior' */
> -          singlestep_breakpoints_inserted_p = 1;
> -          singlestep_ptid = inferior_ptid;
> -          singlestep_pc = pc;
> -        }
> -    }
> +  /* Do we need to do it the hard way, w/temp breakpoints?  */
> +  if (step)
> +      step = set_for_singlestep (gdbarch, pc);

    ^ something looks strange with the indentation here.
>  
>    /* If there were any forks/vforks/execs that were caught and are
>       now to be followed, then do so.  */
> @@ -2826,11 +2837,14 @@ targets should add new threads to the th
>          the inferior over it.  If we have non-steppable watchpoints,
>          we must disable the current watchpoint; it's simplest to
>          disable all watchpoints and breakpoints.  */
> -        
> +      int step_over_watchpoint = 1;

Similarly, rename this to hw_step.  Even if this is false, we're
still doing a a high-level step-over-watchpoint.

> +
>        if (!HAVE_STEPPABLE_WATCHPOINT)
>         remove_breakpoints ();
>        registers_changed ();
> -      target_resume (ecs->ptid, 1, TARGET_SIGNAL_0);   /* Single step */
> +       /* Single step */
> +      step_over_watchpoint = set_for_singlestep (current_gdbarch, read_pc ());
> +      target_resume (ecs->ptid, step_over_watchpoint, TARGET_SIGNAL_0);
>        waiton_ptid = ecs->ptid;
>        if (HAVE_STEPPABLE_WATCHPOINT)
>         infwait_state = infwait_step_watch_state;

OK with the above changes.

-- 
Pedro Alves

Re: [Patch 1/2] infrun.c support for MIPS hardware watchpoints.

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 2386 bytes --]

Pedro Alves wrote:
> On Monday 06 April 2009 07:49:11, David Daney wrote:
> 
>> +/* Try to setup for software single stepping over the specified location.
>> +   Return 1 if target_resume() should use hardware single step.
>> +
>> +   GDBARCH the current gdbarch.
>> +   PC the location to step over.  */
>> +static int
> 
> Add an empty line between comment and function, please.
> 

OK.

>> +set_for_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc)
>> +{
>> +  int step = 1;
>> +
>> +  if (gdbarch_software_single_step_p (gdbarch)
>> +      && gdbarch_software_single_step (gdbarch, get_current_frame ()))
>> +    {
>> +      step = 0;
>> +      /* Do not pull these breakpoints until after a `wait' in
>> +        `wait_for_inferior' */
>> +      singlestep_breakpoints_inserted_p = 1;
>> +      singlestep_ptid = inferior_ptid;
>> +      singlestep_pc = pc;
>> +    }
>> +  return step;
>> +}
> 
> Because I'm dumb, while reading this, 'set_for_singlestep' and
> 'int step' didn't cross my bridge.  How about renaming the local
> variable 'hw_step'? 

OK

> I don't like the "set_for_singlestep" name much.
> It isn't much self-describing, which is something very important in
> infrun.c, since it contains horrible, huge, full-of-states, hard to
> read code --- it is write-once, read-and-debug-millions-of-times
> code.  But, I tried to come up with a descriptive name to suggest
> for it, and I didn't come up with something that made me happy,
> so, ... that's OK.
> 

We went with: maybe_software_singlestep ().


>> +  /* Do we need to do it the hard way, w/temp breakpoints?  */
>> +  if (step)
>> +      step = set_for_singlestep (gdbarch, pc);
> 
>     ^ something looks strange with the indentation here.

I was incorrect, now fixed.

>>  
>>    /* If there were any forks/vforks/execs that were caught and are
>>       now to be followed, then do so.  */
>> @@ -2826,11 +2837,14 @@ targets should add new threads to the th
>>          the inferior over it.  If we have non-steppable watchpoints,
>>          we must disable the current watchpoint; it's simplest to
>>          disable all watchpoints and breakpoints.  */
>> -        
>> +      int step_over_watchpoint = 1;
> 
> Similarly, rename this to hw_step.  Even if this is false, we're
> still doing a a high-level step-over-watchpoint.
> 

OK.


> 
> OK with the above changes.
> 

This is what I committed.


[-- Attachment #2: infrun.patch --]
[-- Type: text/plain, Size: 2898 bytes --]

2009-04-13  David Daney  <ddaney@caviumnetworks.com>

	* infrun.c (maybe_software_singlestep): New function.
	(resume): Call maybe_software_singlestep.
	(handle_inferior_event): Same.


Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.367
diff -u -p -r1.367 infrun.c
--- infrun.c	25 Mar 2009 21:53:10 -0000	1.367
+++ infrun.c	14 Apr 2009 00:56:16 -0000
@@ -950,6 +950,29 @@ set_schedlock_func (char *args, int from
     }
 }
 
+/* Try to setup for software single stepping over the specified location.
+   Return 1 if target_resume() should use hardware single step.
+
+   GDBARCH the current gdbarch.
+   PC the location to step over.  */
+
+static int
+maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  int hw_step = 1;
+
+  if (gdbarch_software_single_step_p (gdbarch)
+      && gdbarch_software_single_step (gdbarch, get_current_frame ()))
+    {
+      hw_step = 0;
+      /* Do not pull these breakpoints until after a `wait' in
+	 `wait_for_inferior' */
+      singlestep_breakpoints_inserted_p = 1;
+      singlestep_ptid = inferior_ptid;
+      singlestep_pc = pc;
+    }
+  return hw_step;
+}
 
 /* Resume the inferior, but allow a QUIT.  This is useful if the user
    wants to interrupt some lengthy single-stepping operation
@@ -1031,20 +1054,9 @@ a command like `return' or `jump' to con
 	}
     }
 
-  if (step && gdbarch_software_single_step_p (gdbarch))
-    {
-      /* Do it the hard way, w/temp breakpoints */
-      if (gdbarch_software_single_step (gdbarch, get_current_frame ()))
-        {
-          /* ...and don't ask hardware to do it.  */
-          step = 0;
-          /* and do not pull these breakpoints until after a `wait' in
-          `wait_for_inferior' */
-          singlestep_breakpoints_inserted_p = 1;
-          singlestep_ptid = inferior_ptid;
-          singlestep_pc = pc;
-        }
-    }
+  /* Do we need to do it the hard way, w/temp breakpoints?  */
+  if (step)
+    step = maybe_software_singlestep (gdbarch, pc);
 
   /* If there were any forks/vforks/execs that were caught and are
      now to be followed, then do so.  */
@@ -2826,11 +2838,14 @@ targets should add new threads to the th
 	 the inferior over it.  If we have non-steppable watchpoints,
 	 we must disable the current watchpoint; it's simplest to
 	 disable all watchpoints and breakpoints.  */
-	 
+      int hw_step = 1;
+
       if (!HAVE_STEPPABLE_WATCHPOINT)
 	remove_breakpoints ();
       registers_changed ();
-      target_resume (ecs->ptid, 1, TARGET_SIGNAL_0);	/* Single step */
+	/* Single step */
+      hw_step = maybe_software_singlestep (current_gdbarch, read_pc ());
+      target_resume (ecs->ptid, hw_step, TARGET_SIGNAL_0);
       waiton_ptid = ecs->ptid;
       if (HAVE_STEPPABLE_WATCHPOINT)
 	infwait_state = infwait_step_watch_state;