[PATCH 0/2] Add thread info in 'struct step_over_info'

[PATCH 0/2] Add thread info in 'struct step_over_info'

[Yao Qi]

During the review https://sourceware.org/ml/gdb-patches/2016-04/msg00211.html,
we need to record the thread infor in step_over_info so that we know
the thread we are stepping over.  After I add thread info into
'struct step_over_info', I find other two fields in 'struct step_over_info'
become redundant, because 'aspace' and 'address' can be got from
'thread'.  On the hand, the step over info can be regarded as the
the thread we stepping over.

This patch series add a new field 'thread' and remove fields 'aspace' and
'address'.

Regression tested on x86_64-linux and aarch64-linux.

*** BLURB HERE ***

Yao Qi (2):
  Refactor function set_step_over_info
  Replace address and aspace with thread in struct step_over_info

 gdb/infrun.c | 50 ++++++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

-- 
1.9.1

[PATCH 2/2] Replace address and aspace with thread in struct step_over_info

[Yao Qi]

This patch replaces the fields aspace and address in
'struct step_over_info' with 'thread', because aspace and thread can
be got from thread.

gdb:

2016-04-15  Yao Qi  <yao.qi@linaro.org>

	* infrun.c (struct step_over_info) <aspace>: Remove
	<address>: Remove.
	<thread>: New field.
	(set_step_over_info): Update.
	(clear_step_over_info): Update.
	(stepping_past_nonsteppable_watchpoint): Update.
---
 gdb/infrun.c | 47 +++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 9017b0a..72f7fe4 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1282,16 +1282,12 @@ enum step_over_what_flag
   };
 DEF_ENUM_FLAGS_TYPE (enum step_over_what_flag, step_over_what);
 
-/* Info about an instruction that is being stepped over.  */
+/* Info about a thread that is being stepped over.  */
 
 struct step_over_info
 {
-  /* If we're stepping past a breakpoint, this is the address space
-     and address of the instruction the breakpoint is set at.  We'll
-     skip inserting all breakpoints here.  Valid iff ASPACE is
-     non-NULL.  */
-  struct address_space *aspace;
-  CORE_ADDR address;
+  /* We're stepping over the thread to pass a breakpoint.  */
+  struct thread_info *thread;
 
   /* The instruction being stepped over triggers a nonsteppable
      watchpoint.  If true, we'll skip inserting watchpoints.  */
@@ -1331,18 +1327,7 @@ static void
 set_step_over_info (struct thread_info *thread,
 		    int nonsteppable_watchpoint_p)
 {
-  if (thread != NULL)
-    {
-      struct regcache *regcache = get_thread_regcache (thread->ptid);
-
-      step_over_info.aspace = get_regcache_aspace (regcache);
-      step_over_info.address = regcache_read_pc (regcache);
-    }
-  else
-    {
-      step_over_info.aspace = NULL;
-      step_over_info.address = 0;
-    }
+  step_over_info.thread = thread;
   step_over_info.nonsteppable_watchpoint_p = nonsteppable_watchpoint_p;
 }
 
@@ -1355,8 +1340,7 @@ clear_step_over_info (void)
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog,
 			"infrun: clear_step_over_info\n");
-  step_over_info.aspace = NULL;
-  step_over_info.address = 0;
+  step_over_info.thread = NULL;
   step_over_info.nonsteppable_watchpoint_p = 0;
 }
 
@@ -1366,10 +1350,21 @@ int
 stepping_past_instruction_at (struct address_space *aspace,
 			      CORE_ADDR address)
 {
-  return (step_over_info.aspace != NULL
-	  && breakpoint_address_match (aspace, address,
-				       step_over_info.aspace,
-				       step_over_info.address));
+  if (step_over_info.thread != NULL)
+    {
+      struct regcache *regcache;
+
+      regcache = get_thread_regcache (step_over_info.thread->ptid);
+
+      /* The step-over isn't finished or is still valid, so the PC got
+	 from regcache is the value when thread stops, rather than the
+	 value after step-over.  */
+      return breakpoint_address_match (aspace, address,
+				       get_regcache_aspace (regcache) ,
+				       regcache_read_pc (regcache));
+    }
+  else
+    return 0;
 }
 
 /* See infrun.h.  */
@@ -1385,7 +1380,7 @@ stepping_past_nonsteppable_watchpoint (void)
 static int
 step_over_info_valid_p (void)
 {
-  return (step_over_info.aspace != NULL
+  return (step_over_info.thread != NULL
 	  || stepping_past_nonsteppable_watchpoint ());
 }
 
-- 
1.9.1

Re: [PATCH 2/2] Replace address and aspace with thread in struct step_over_info

[Pedro Alves]

On 04/15/2016 02:29 PM, Yao Qi wrote:
> This patch replaces the fields aspace and address in
> 'struct step_over_info' with 'thread', because aspace and thread can
> be got from thread.
> 

>  
> @@ -1366,10 +1350,21 @@ int
>  stepping_past_instruction_at (struct address_space *aspace,
>  			      CORE_ADDR address)
>  {
> -  return (step_over_info.aspace != NULL
> -	  && breakpoint_address_match (aspace, address,
> -				       step_over_info.aspace,
> -				       step_over_info.address));
> +  if (step_over_info.thread != NULL)
> +    {
> +      struct regcache *regcache;
> +
> +      regcache = get_thread_regcache (step_over_info.thread->ptid);
> +
> +      /* The step-over isn't finished or is still valid, so the PC got
> +	 from regcache is the value when thread stops, rather than the
> +	 value after step-over.  */

I think this is problematic.

While a thread is being stepped past a breakpoint, it's possible that the
user sets some other breakpoint, and then we end up in stepping_past_instruction_at
deciding whether we can insert that new breakpoint, while the step-over thread
is running.

As soon as the step-over thread is resumed for the actual step-over, it's
regcache is flushed (target_resume -> registers_changed_ptid).  From that point
and until the thread stops again, trying to fetch its regcache will error out,
because you can't read registers from a thread that is running.

Example (haven't tried it):

- A program with two threads, thread 1 and thread 2.

- non-stop mode on.

- Thread 1 continuously stepping over this:

  while (1) i++;     << breakpoint here:

  E.g., with:

  (gdb) thread 1
  (gdb) b $breakpoint_here_line
  (gdb) n&

- Switch to thread 2, which is stopped elsewhere (so inserting
  a breakpoint works when native debugging), and set some breakpoint:

  (gdb) thread 2
  (gdb) b foo

Thanks,
Pedro Alves

Re: [PATCH 2/2] Replace address and aspace with thread in struct step_over_info

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> I think this is problematic.
>
> While a thread is being stepped past a breakpoint, it's possible that the
> user sets some other breakpoint, and then we end up in
> stepping_past_instruction_at
> deciding whether we can insert that new breakpoint, while the step-over thread
> is running.
>
> As soon as the step-over thread is resumed for the actual step-over, it's
> regcache is flushed (target_resume -> registers_changed_ptid).  From that point
> and until the thread stops again, trying to fetch its regcache will error out,
> because you can't read registers from a thread that is running.

OK, that is a good case.  I didn't think of it.  I withdraw the patch.

-- 
Yao (齐尧)

Re: [PATCH 2/2] Replace address and aspace with thread in struct step_over_info

[Doug Evans]

Yao Qi <qiyaoltc@gmail.com> writes:
> Pedro Alves <palves@redhat.com> writes:
>
>> I think this is problematic.
>>
>> While a thread is being stepped past a breakpoint, it's possible that the
>> user sets some other breakpoint, and then we end up in
>> stepping_past_instruction_at
>> deciding whether we can insert that new breakpoint, while the step-over thread
>> is running.
>>
>> As soon as the step-over thread is resumed for the actual step-over, it's
>> regcache is flushed (target_resume -> registers_changed_ptid).  From that point
>> and until the thread stops again, trying to fetch its regcache will error out,
>> because you can't read registers from a thread that is running.
>
> OK, that is a good case.  I didn't think of it.  I withdraw the patch.

A good place for a comment explaining Why Things Are The Way They Are.

Not sure how you want to word this.

2016-04-20  Doug Evans  <xdje42@gmail.com>

	* infrun.c (set_step_over_info): Add comment.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 696105d..c7ea5e2 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1325,7 +1325,9 @@ struct step_over_info
 static struct step_over_info step_over_info;
 
 /* Record the address of the breakpoint/instruction we're currently
-   stepping over.  */
+   stepping over.
+   N.B. We record the aspace and address now, instead of say just the thread,
+   because when we need the info later the thread may be running.  */
 
 static void
 set_step_over_info (struct address_space *aspace, CORE_ADDR address,

Re: [PATCH 2/2] Replace address and aspace with thread in struct step_over_info

[Pedro Alves]

On 04/20/2016 07:01 PM, Doug Evans wrote:

> A good place for a comment explaining Why Things Are The Way They Are.
> 
> Not sure how you want to word this.

That looks just fine to me.

Thanks,
Pedro Alves

Re: [PATCH 2/2] Replace address and aspace with thread in struct step_over_info

[Doug Evans]

Pedro Alves <palves@redhat.com> writes:
> On 04/20/2016 07:01 PM, Doug Evans wrote:
>
>> A good place for a comment explaining Why Things Are The Way They Are.
>> 
>> Not sure how you want to word this.
>
> That looks just fine to me.

Thanks.
Committed.

[PATCH 1/2] Refactor function set_step_over_info

[Yao Qi]

This patch replace two parameters of set_step_over_info (aspace and
address) with one (thread).

gdb:

2016-04-15  Yao Qi  <yao.qi@linaro.org>

	* infrun.c (set_step_over_info): Remove parameter aspace and
	address.  Add thread.  Set field aspace and address.
	Callers updated.
	(keep_going_pass_signal): Remove local variable regcache.
---
 gdb/infrun.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 696105d..9017b0a 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1325,14 +1325,24 @@ struct step_over_info
 static struct step_over_info step_over_info;
 
 /* Record the address of the breakpoint/instruction we're currently
-   stepping over.  */
+   stepping over THREAD.  */
 
 static void
-set_step_over_info (struct address_space *aspace, CORE_ADDR address,
+set_step_over_info (struct thread_info *thread,
 		    int nonsteppable_watchpoint_p)
 {
-  step_over_info.aspace = aspace;
-  step_over_info.address = address;
+  if (thread != NULL)
+    {
+      struct regcache *regcache = get_thread_regcache (thread->ptid);
+
+      step_over_info.aspace = get_regcache_aspace (regcache);
+      step_over_info.address = regcache_read_pc (regcache);
+    }
+  else
+    {
+      step_over_info.aspace = NULL;
+      step_over_info.address = 0;
+    }
   step_over_info.nonsteppable_watchpoint_p = nonsteppable_watchpoint_p;
 }
 
@@ -2578,8 +2588,7 @@ resume (enum gdb_signal sig)
 	  if (target_is_non_stop_p ())
 	    stop_all_threads ();
 
-	  set_step_over_info (get_regcache_aspace (regcache),
-			      regcache_read_pc (regcache), 0);
+	  set_step_over_info (tp, 0);
 
 	  step = maybe_software_singlestep (gdbarch, pc);
 
@@ -7713,7 +7722,6 @@ keep_going_pass_signal (struct execution_control_state *ecs)
     }
   else
     {
-      struct regcache *regcache = get_current_regcache ();
       int remove_bp;
       int remove_wps;
       step_over_what step_what;
@@ -7749,11 +7757,10 @@ keep_going_pass_signal (struct execution_control_state *ecs)
       if (remove_bp
 	  && (remove_wps || !use_displaced_stepping (ecs->event_thread)))
 	{
-	  set_step_over_info (get_regcache_aspace (regcache),
-			      regcache_read_pc (regcache), remove_wps);
+	  set_step_over_info (ecs->event_thread, remove_wps);
 	}
       else if (remove_wps)
-	set_step_over_info (NULL, 0, remove_wps);
+	set_step_over_info (NULL, remove_wps);
 
       /* If we now need to do an in-line step-over, we need to stop
 	 all other threads.  Note this must be done before
-- 
1.9.1