[PATCH] Use "switch_to_thread" more thoroughly on gdbserver

[PATCH] Use "switch_to_thread" more thoroughly on gdbserver

[Sergio Durigan Junior]

This is a technical debt that I left when I ported "switch_to_thread"
to gdbserver.  It's a simple patch that converts occurences of:

  current_thread = find_thread_ptid (ptid);

to:

  switch_to_thread (ptid);

It helps to leave things more uniform across the code.

Regtested on BuildBot.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdbthread.h: Include "common-gdbthread.h".
	* remote-utils.c: Include "gdbthread.h".
	(prepare_resume_reply): Use "switch_to_thread".
	* target.c (set_desired_thread): Likewise.
	(done_accessing_memory): Likewise.
---
 gdb/gdbserver/gdbthread.h    | 1 +
 gdb/gdbserver/remote-utils.c | 3 ++-
 gdb/gdbserver/target.c       | 9 +++------
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/gdb/gdbserver/gdbthread.h b/gdb/gdbserver/gdbthread.h
index 04cc47ba71..a864f95884 100644
--- a/gdb/gdbserver/gdbthread.h
+++ b/gdb/gdbserver/gdbthread.h
@@ -19,6 +19,7 @@
 #ifndef GDB_THREAD_H
 #define GDB_THREAD_H
 
+#include "common-gdbthread.h"
 #include "inferiors.h"
 
 struct btrace_target_info;
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 25b7e41a91..761604683c 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -23,6 +23,7 @@
 #include "tdesc.h"
 #include "dll.h"
 #include "rsp-low.h"
+#include "gdbthread.h"
 #include <ctype.h>
 #if HAVE_SYS_IOCTL_H
 #include <sys/ioctl.h>
@@ -1188,7 +1189,7 @@ prepare_resume_reply (char *buf, ptid_t ptid,
 
 	saved_thread = current_thread;
 
-	current_thread = find_thread_ptid (ptid);
+	switch_to_thread (ptid);
 
 	regp = current_target_desc ()->expedite_regs;
 
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 752646310a..2226dc802d 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -26,14 +26,11 @@ struct target_ops *the_target;
 int
 set_desired_thread (int use_general)
 {
-  struct thread_info *found;
-
   if (use_general == 1)
-    found = find_thread_ptid (general_thread);
+    switch_to_thread (general_thread);
   else
-    found = find_thread_ptid (cont_thread);
+    switch_to_thread (cont_thread);
 
-  current_thread = found;
   return (current_thread != NULL);
 }
 
@@ -143,7 +140,7 @@ done_accessing_memory (void)
 
   /* Restore the previous selected thread.  */
   general_thread = prev_general_thread;
-  current_thread = find_thread_ptid (general_thread);
+  switch_to_thread (general_thread);
 }
 
 int
-- 
2.13.3

Re: [PATCH] Use "switch_to_thread" more thoroughly on gdbserver

[Pedro Alves]

On 09/15/2017 05:33 AM, Sergio Durigan Junior wrote:

> diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
> index 752646310a..2226dc802d 100644
> --- a/gdb/gdbserver/target.c
> +++ b/gdb/gdbserver/target.c
> @@ -26,14 +26,11 @@ struct target_ops *the_target;
>  int
>  set_desired_thread (int use_general)
>  {
> -  struct thread_info *found;
> -
>    if (use_general == 1)
> -    found = find_thread_ptid (general_thread);
> +    switch_to_thread (general_thread);
>    else
> -    found = find_thread_ptid (cont_thread);
> +    switch_to_thread (cont_thread);
>  
> -  current_thread = found;
>    return (current_thread != NULL);

Did you double-check whether this is equivalent?  Off hand, I'd
think it is, but the minus_one_ptid check in switch_to_thread
gave me pause. there's a 'set_continue_thread (minus_one_ptid)' in
gdb in early connection setup.  Looks like we never call 
'set_desired_thread(0)' nowadays though.  [Hc predates vCont].

Do you remember why that check is there in switch_to_thread in the
first place, as opposed to, say, an assertion?

Thanks,
Pedro Alves

Re: [PATCH] Use "switch_to_thread" more thoroughly on gdbserver

[Sergio Durigan Junior]

On Friday, September 15 2017, Pedro Alves wrote:

> On 09/15/2017 05:33 AM, Sergio Durigan Junior wrote:
>
>> diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
>> index 752646310a..2226dc802d 100644
>> --- a/gdb/gdbserver/target.c
>> +++ b/gdb/gdbserver/target.c
>> @@ -26,14 +26,11 @@ struct target_ops *the_target;
>>  int
>>  set_desired_thread (int use_general)
>>  {
>> -  struct thread_info *found;
>> -
>>    if (use_general == 1)
>> -    found = find_thread_ptid (general_thread);
>> +    switch_to_thread (general_thread);
>>    else
>> -    found = find_thread_ptid (cont_thread);
>> +    switch_to_thread (cont_thread);
>>  
>> -  current_thread = found;
>>    return (current_thread != NULL);
>
> Did you double-check whether this is equivalent?  Off hand, I'd
> think it is, but the minus_one_ptid check in switch_to_thread
> gave me pause. there's a 'set_continue_thread (minus_one_ptid)' in
> gdb in early connection setup.  Looks like we never call 
> 'set_desired_thread(0)' nowadays though.  [Hc predates vCont].

I think I see what you mean, and I confess I didn't know that you could
set the current thread as minus_one_ptid.

No, I haven't checked if cont_thread can be minus_one_ptid in any moment
in time.  As I said I did some tests here before sending the patch and
nothing came up wrong.

> Do you remember why that check is there in switch_to_thread in the
> first place, as opposed to, say, an assertion?

I tried to do some archaeology here and find the patch that implemented
this feature, and it seems to me that the patch always used 'if' instead
of an 'assert'.  Maybe that's because GDB's version of switch_to_thread
also doesn't assert that ptid != minus_one_ptid...

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Use "switch_to_thread" more thoroughly on gdbserver

[Pedro Alves]

On 09/15/2017 07:25 PM, Sergio Durigan Junior wrote:
> On Friday, September 15 2017, Pedro Alves wrote:
> 
>> Do you remember why that check is there in switch_to_thread in the
>> first place, as opposed to, say, an assertion?
> 
> I tried to do some archaeology here and find the patch that implemented
> this feature, and it seems to me that the patch always used 'if' instead
> of an 'assert'.  

Yeah, gdbserver's switch_to_thread was added with your fork
inferior sharing series.

> Maybe that's because GDB's version of switch_to_thread
> also doesn't assert that ptid != minus_one_ptid...

Except it does:

static void
switch_to_thread (thread_info *thr)
{
  gdb_assert (thr != NULL);
  ^^^^^^^^^^^^^^^^^^^^^^^^
...

Called from:

void
switch_to_thread (ptid_t ptid)
{
  if (ptid == null_ptid)
    switch_to_no_thread ();
  else
    switch_to_thread (find_thread_ptid (ptid));
}

... and find_thread_ptid returns NULL because there's
never a thread with ptid == minus_one_ptid.

Thanks,
Pedro Alves

Re: [PATCH] Use "switch_to_thread" more thoroughly on gdbserver

[Sergio Durigan Junior]

On Thursday, September 21 2017, Pedro Alves wrote:

> On 09/15/2017 07:25 PM, Sergio Durigan Junior wrote:
>> On Friday, September 15 2017, Pedro Alves wrote:
>> 
>>> Do you remember why that check is there in switch_to_thread in the
>>> first place, as opposed to, say, an assertion?
>> 
>> I tried to do some archaeology here and find the patch that implemented
>> this feature, and it seems to me that the patch always used 'if' instead
>> of an 'assert'.  
>
> Yeah, gdbserver's switch_to_thread was added with your fork
> inferior sharing series.

Exactly.

>> Maybe that's because GDB's version of switch_to_thread
>> also doesn't assert that ptid != minus_one_ptid...
>
> Except it does:
>
> static void
> switch_to_thread (thread_info *thr)
> {
>   gdb_assert (thr != NULL);
>   ^^^^^^^^^^^^^^^^^^^^^^^^
> ...
>
> Called from:
>
> void
> switch_to_thread (ptid_t ptid)
> {
>   if (ptid == null_ptid)
>     switch_to_no_thread ();
>   else
>     switch_to_thread (find_thread_ptid (ptid));
> }
>
> ... and find_thread_ptid returns NULL because there's
> never a thread with ptid == minus_one_ptid.

Oh.  There's an overload, then.  And there's also a check to see if ptid
== null_ptid.  Maybe it's worth extending gdbserver's switch_to_thread
to mimic that behaviour.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[PATCH v2] Use "switch_to_thread" more thoroughly on gdbserver

[Sergio Durigan Junior]

This is a technical debt that I left when I ported "switch_to_thread"
to gdbserver.  It's a simple patch that converts occurences of:

  current_thread = find_thread_ptid (ptid);

to:

  switch_to_thread (ptid);

This patch also converts a simple "if" to a "gdb_assert" on
gdbserver's version of "switch_to_thread":

  gdb_assert (!ptid_equal (ptid, minus_one_ptid));

This change makes the code more similar to what GDB already does.

Regtested on BuildBot.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdbthread.h: Include "common-gdbthread.h".
	* inferiors.c (switch_to_thread): Use "gdb_assert" instead of
	"if" when validating the ptid.
	* remote-utils.c: Include "gdbthread.h".
	(prepare_resume_reply): Use "switch_to_thread".
	* target.c (done_accessing_memory): Likewise.
---
 gdb/gdbserver/gdbthread.h    | 1 +
 gdb/gdbserver/inferiors.c    | 4 ++--
 gdb/gdbserver/remote-utils.c | 3 ++-
 gdb/gdbserver/target.c       | 2 +-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/gdb/gdbserver/gdbthread.h b/gdb/gdbserver/gdbthread.h
index 04cc47ba71..a864f95884 100644
--- a/gdb/gdbserver/gdbthread.h
+++ b/gdb/gdbserver/gdbthread.h
@@ -19,6 +19,7 @@
 #ifndef GDB_THREAD_H
 #define GDB_THREAD_H
 
+#include "common-gdbthread.h"
 #include "inferiors.h"
 
 struct btrace_target_info;
diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
index 72f0412757..7705c2af47 100644
--- a/gdb/gdbserver/inferiors.c
+++ b/gdb/gdbserver/inferiors.c
@@ -442,6 +442,6 @@ make_cleanup_restore_current_thread (void)
 void
 switch_to_thread (ptid_t ptid)
 {
-  if (!ptid_equal (ptid, minus_one_ptid))
-    current_thread = find_thread_ptid (ptid);
+  gdb_assert (!ptid_equal (ptid, minus_one_ptid));
+  current_thread = find_thread_ptid (ptid);
 }
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 25b7e41a91..761604683c 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -23,6 +23,7 @@
 #include "tdesc.h"
 #include "dll.h"
 #include "rsp-low.h"
+#include "gdbthread.h"
 #include <ctype.h>
 #if HAVE_SYS_IOCTL_H
 #include <sys/ioctl.h>
@@ -1188,7 +1189,7 @@ prepare_resume_reply (char *buf, ptid_t ptid,
 
 	saved_thread = current_thread;
 
-	current_thread = find_thread_ptid (ptid);
+	switch_to_thread (ptid);
 
 	regp = current_target_desc ()->expedite_regs;
 
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 7f5bed7ec7..94224a8a7c 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -138,7 +138,7 @@ done_accessing_memory (void)
 
   /* Restore the previous selected thread.  */
   general_thread = prev_general_thread;
-  current_thread = find_thread_ptid (general_thread);
+  switch_to_thread (general_thread);
 }
 
 int
-- 
2.13.3

Re: [PATCH v2] Use "switch_to_thread" more thoroughly on gdbserver

[Pedro Alves]

On 09/26/2017 05:22 AM, Sergio Durigan Junior wrote:

> gdb/gdbserver/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* gdbthread.h: Include "common-gdbthread.h".
> 	* inferiors.c (switch_to_thread): Use "gdb_assert" instead of
> 	"if" when validating the ptid.
> 	* remote-utils.c: Include "gdbthread.h".
> 	(prepare_resume_reply): Use "switch_to_thread".
> 	* target.c (done_accessing_memory): Likewise.

OK

>  void
>  switch_to_thread (ptid_t ptid)
>  {
> -  if (!ptid_equal (ptid, minus_one_ptid))
> -    current_thread = find_thread_ptid (ptid);
> +  gdb_assert (!ptid_equal (ptid, minus_one_ptid));

Note you can write:

  gdb_assert (ptid != minus_one_ptid);

nowadays.

Thanks,
Pedro Alves

Re: [PATCH v2] Use "switch_to_thread" more thoroughly on gdbserver

[Sergio Durigan Junior]

On Tuesday, September 26 2017, Pedro Alves wrote:

> On 09/26/2017 05:22 AM, Sergio Durigan Junior wrote:
>
>> gdb/gdbserver/ChangeLog:
>> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	* gdbthread.h: Include "common-gdbthread.h".
>> 	* inferiors.c (switch_to_thread): Use "gdb_assert" instead of
>> 	"if" when validating the ptid.
>> 	* remote-utils.c: Include "gdbthread.h".
>> 	(prepare_resume_reply): Use "switch_to_thread".
>> 	* target.c (done_accessing_memory): Likewise.
>
> OK
>
>>  void
>>  switch_to_thread (ptid_t ptid)
>>  {
>> -  if (!ptid_equal (ptid, minus_one_ptid))
>> -    current_thread = find_thread_ptid (ptid);
>> +  gdb_assert (!ptid_equal (ptid, minus_one_ptid));
>
> Note you can write:
>
>   gdb_assert (ptid != minus_one_ptid);
>
> nowadays.

That's right; pushed with this small nit fixed.

75352e283fb2b265d14c750859156943f6eb2693

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/