[v3,4/6] Share parts of gdb/gdbthread.h with gdbserver

Message ID 20170208032257.15443-5-sergiodj@redhat.com
State New, archived
Headers

Commit Message

Sergio Durigan Junior Feb. 8, 2017, 3:22 a.m. UTC
  Again, it was necessary to share a few functions declared on
gdb/gdbthread.h with gdbserver, because they are needed by
fork_inferior.  I decided to implement them on
gdb/gdbserver/inferiors.c because that's where the thread functions
are also implemented on gdbserver.  Some of these functions do not
need to be implemented on gdbserver, or don't make sense there, so
they are left blank and commented properly.

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

	* Makefile.in (HFILES_NO_SRCDIR): Add "common/common-gdbthread.h".
	* common/common-gdbthread.h: New file, with parts from
	"gdb/gdbthread.h".
	* fork-child.c (fork_inferior): Update call of "set_executing".
	* gdbthread.h: Include "common-gdbthread.h".
	(init_thread_list): Moved to "common/common-gdbthread.h".
	(add_thread_silent): Likewise.
	(switch_to_thread): Likewise.
	(set_executing): Likewise.
	* thread.c (set_executing): Update function comment.

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

	* inferiors.c (init_thread_list): New function.
	(switch_to_thread): Likewise.
	(set_executing): Likewise.
	(add_thread_silent): Likewise.
	* server.c (get_last_target_waitstatus): Likewise.
	* server.h (get_last_target_waitstatus): Likewise.
---
 gdb/Makefile.in               |  1 +
 gdb/common/common-gdbthread.h | 45 +++++++++++++++++++++++++++++++++++++++++++
 gdb/gdbserver/inferiors.c     | 41 +++++++++++++++++++++++++++++++++++++++
 gdb/gdbserver/server.c        |  8 ++++++++
 gdb/gdbserver/server.h        |  3 +++
 gdb/gdbthread.h               | 20 +------------------
 gdb/thread.c                  |  2 ++
 7 files changed, 101 insertions(+), 19 deletions(-)
 create mode 100644 gdb/common/common-gdbthread.h
  

Comments

Pedro Alves Feb. 15, 2017, 4:15 p.m. UTC | #1
On 02/08/2017 03:22 AM, Sergio Durigan Junior wrote:

> diff --git a/gdb/common/common-gdbthread.h b/gdb/common/common-gdbthread.h
> new file mode 100644
> index 0000000..eb66de9
> --- /dev/null
> +++ b/gdb/common/common-gdbthread.h
> @@ -0,0 +1,45 @@
> +/* Common multi-process/thread control defs for GDB and gdbserver.
> +   Copyright (C) 1987-2017 Free Software Foundation, Inc.
> +   Contributed by Lynx Real-Time Systems, Inc.  Los Gatos, CA.
> +   
> +

Spurious blank line.  That "contributed by" line is meaningless
for this new file, please remove it.

> +   This file is part of GDB.

> +#ifndef COMMON_THREAD_H
> +#define COMMON_THREAD_H

This macro name does not patch the file name.

> +/* See common/common-gdbthread.h.  */
> +
> +void
> +set_executing (ptid_t ptid ATTRIBUTE_UNUSED, int executing ATTRIBUTE_UNUSED)
> +{
> +  gdb_assert (current_thread != NULL);
> +  current_thread->last_resume_kind = resume_stop;
> +  current_thread->last_status = get_last_target_waitstatus ();
> +}
> +

This is a bit too hacky to live IMO.  :-/  The implementation
of the function is doing nothing related to its interface.

Do we really need the set_executing call in the new shared file?  AFAICS, 
set_executing is only called at the very end of startup_inferior today.
Couldn't we leave that call out of the common code and add it on the
gdb side, after the common startup_inferior returns?

>  
> -/* Marks thread PTID as executing, or not.  If PTID is minus_one_ptid,
> -   marks all threads.
> -
> -   Note that this is different from the running state.  See the
> -   description of state and executing fields of struct
> -   thread_info.  */
> -extern void set_executing (ptid_t ptid, int executing);
> -
>  /* Reports if thread PTID is executing.  */
>  extern int is_executing (ptid_t ptid);

Having a setter in one place, and the getter somewhere
else is sure to generate confusion.  In cases like these,
please leave a breadcrumb comment.  (there may be more cases.)

Thanks,
Pedro Alves
  
Sergio Durigan Junior Feb. 21, 2017, 9:27 p.m. UTC | #2
Thanks for the review.

On Wednesday, February 15 2017, Pedro Alves wrote:

> On 02/08/2017 03:22 AM, Sergio Durigan Junior wrote:
>
>> diff --git a/gdb/common/common-gdbthread.h b/gdb/common/common-gdbthread.h
>> new file mode 100644
>> index 0000000..eb66de9
>> --- /dev/null
>> +++ b/gdb/common/common-gdbthread.h
>> @@ -0,0 +1,45 @@
>> +/* Common multi-process/thread control defs for GDB and gdbserver.
>> +   Copyright (C) 1987-2017 Free Software Foundation, Inc.
>> +   Contributed by Lynx Real-Time Systems, Inc.  Los Gatos, CA.
>> +   
>> +
>
> Spurious blank line.  That "contributed by" line is meaningless
> for this new file, please remove it.

Fixed.

>> +   This file is part of GDB.
>
>> +#ifndef COMMON_THREAD_H
>> +#define COMMON_THREAD_H
>
> This macro name does not patch the file name.

Fixed.

>> +/* See common/common-gdbthread.h.  */
>> +
>> +void
>> +set_executing (ptid_t ptid ATTRIBUTE_UNUSED, int executing ATTRIBUTE_UNUSED)
>> +{
>> +  gdb_assert (current_thread != NULL);
>> +  current_thread->last_resume_kind = resume_stop;
>> +  current_thread->last_status = get_last_target_waitstatus ();
>> +}
>> +
>
> This is a bit too hacky to live IMO.  :-/  The implementation
> of the function is doing nothing related to its interface.
>
> Do we really need the set_executing call in the new shared file?  AFAICS, 
> set_executing is only called at the very end of startup_inferior today.
> Couldn't we leave that call out of the common code and add it on the
> gdb side, after the common startup_inferior returns?

Yes, that works and is better than the current solution, I agree.  I'll
implement it.

>>  
>> -/* Marks thread PTID as executing, or not.  If PTID is minus_one_ptid,
>> -   marks all threads.
>> -
>> -   Note that this is different from the running state.  See the
>> -   description of state and executing fields of struct
>> -   thread_info.  */
>> -extern void set_executing (ptid_t ptid, int executing);
>> -
>>  /* Reports if thread PTID is executing.  */
>>  extern int is_executing (ptid_t ptid);
>
> Having a setter in one place, and the getter somewhere
> else is sure to generate confusion.  In cases like these,
> please leave a breadcrumb comment.  (there may be more cases.)

With set_executing being called after startup_inferior on GDB, this
change will not be there anymore (i.e., set_executing will not be
touched).  But thanks for the heads up, I'll check other places where I
moved functions from GDB to gdbserver and make sure everything is
commented.

Thanks,
  

Patch

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 84762c4..cc0fdec 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1470,6 +1470,7 @@  HFILES_NO_SRCDIR = \
 	common/common-debug.h \
 	common/common-defs.h \
 	common/common-exceptions.h \
+	common/common-gdbthread.h \
 	common/common-regcache.h \
 	common/common-types.h \
 	common/common-utils.h \
diff --git a/gdb/common/common-gdbthread.h b/gdb/common/common-gdbthread.h
new file mode 100644
index 0000000..eb66de9
--- /dev/null
+++ b/gdb/common/common-gdbthread.h
@@ -0,0 +1,45 @@ 
+/* Common multi-process/thread control defs for GDB and gdbserver.
+   Copyright (C) 1987-2017 Free Software Foundation, Inc.
+   Contributed by Lynx Real-Time Systems, Inc.  Los Gatos, CA.
+   
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_THREAD_H
+#define COMMON_THREAD_H
+
+struct target_waitstatus;
+
+/* Create an empty thread list, or empty the existing one.  */
+extern void init_thread_list (void);
+
+/* Switch from one thread to another.  */
+extern void switch_to_thread (ptid_t ptid);
+
+/* Marks thread PTID as executing, or not.  If PTID is minus_one_ptid,
+   marks all threads.
+
+   Note that this is different from the running state.  See the
+   description of state and executing fields of struct
+   thread_info.  */
+extern void set_executing (ptid_t ptid, int executing);
+
+/* Add a thread to the thread list and return the pointer to the new
+   thread.  Caller may use this pointer to initialize the private
+   thread data.  */
+extern struct thread_info *add_thread_silent (ptid_t ptid);
+
+#endif /* ! COMMON_THREAD_H */
diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
index b65a726..441ec2c 100644
--- a/gdb/gdbserver/inferiors.c
+++ b/gdb/gdbserver/inferiors.c
@@ -468,3 +468,44 @@  make_cleanup_restore_current_thread (void)
 {
   return make_cleanup (do_restore_current_thread_cleanup, current_thread);
 }
+
+/* See common/common-gdbthread.h.  */
+
+void
+init_thread_list (void)
+{
+  /* To be implemented.  */
+}
+
+/* See common/common-gdbthread.h.  */
+
+void
+switch_to_thread (ptid_t ptid)
+{
+  if (!ptid_equal (ptid, minus_one_ptid))
+    current_thread = find_thread_ptid (ptid);
+}
+
+/* See common/common-gdbthread.h.  */
+
+void
+set_executing (ptid_t ptid ATTRIBUTE_UNUSED, int executing ATTRIBUTE_UNUSED)
+{
+  gdb_assert (current_thread != NULL);
+  current_thread->last_resume_kind = resume_stop;
+  current_thread->last_status = get_last_target_waitstatus ();
+}
+
+/* See common/common-gdbthread.h.  */
+
+struct thread_info *
+add_thread_silent (ptid_t ptid)
+{
+  pid_t pid = ptid_get_pid (ptid);
+
+  /* Check if there is a process already.  */
+  if (find_process_pid (pid) == NULL)
+    add_process (pid, 0);
+
+  return add_thread (ptid_build (pid, pid, 0), NULL);
+}
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 4bc7f71..43d9406 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -236,6 +236,14 @@  target_running (void)
   return get_first_thread () != NULL;
 }
 
+/* See server.h.  */
+
+struct target_waitstatus
+get_last_target_waitstatus (void)
+{
+  return last_status;
+}
+
 static int
 start_inferior (char **argv)
 {
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index d5fee38..71848f1 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -148,4 +148,7 @@  extern int in_queued_stop_replies (ptid_t ptid);
 /* Definition for any syscall, used for unfiltered syscall reporting.  */
 #define ANY_SYSCALL (-2)
 
+/* Return the global variable LAST_STATUS from server.c.  */
+extern struct target_waitstatus get_last_target_waitstatus (void);
+
 #endif /* SERVER_H */
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 06ed78f..0d84f77 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -31,6 +31,7 @@  struct symtab;
 #include "common/vec.h"
 #include "target/waitstatus.h"
 #include "cli/cli-utils.h"
+#include "common-gdbthread.h"
 
 /* Frontend view of the thread state.  Possible extensions: stepping,
    finishing, until(ling),...  */
@@ -344,19 +345,12 @@  struct thread_info
   struct thread_info *step_over_next;
 };
 
-/* Create an empty thread list, or empty the existing one.  */
-extern void init_thread_list (void);
-
 /* Add a thread to the thread list, print a message
    that a new thread is found, and return the pointer to
    the new thread.  Caller my use this pointer to 
    initialize the private thread data.  */
 extern struct thread_info *add_thread (ptid_t ptid);
 
-/* Same as add_thread, but does not print a message
-   about new thread.  */
-extern struct thread_info *add_thread_silent (ptid_t ptid);
-
 /* Same as add_thread, and sets the private info.  */
 extern struct thread_info *add_thread_with_info (ptid_t ptid,
 						 struct private_thread_info *);
@@ -469,10 +463,6 @@  extern struct thread_info *iterate_over_threads (thread_callback_func, void *);
 
 extern int thread_count (void);
 
-/* Switch from one thread to another.  Also sets the STOP_PC
-   global.  */
-extern void switch_to_thread (ptid_t ptid);
-
 /* Switch from one thread to another.  Does not read registers and
    sets STOP_PC to -1.  */
 extern void switch_to_thread_no_regs (struct thread_info *thread);
@@ -518,14 +508,6 @@  extern int is_exited (ptid_t ptid);
 /* In the frontend's perpective, is this thread stopped?  */
 extern int is_stopped (ptid_t ptid);
 
-/* Marks thread PTID as executing, or not.  If PTID is minus_one_ptid,
-   marks all threads.
-
-   Note that this is different from the running state.  See the
-   description of state and executing fields of struct
-   thread_info.  */
-extern void set_executing (ptid_t ptid, int executing);
-
 /* Reports if thread PTID is executing.  */
 extern int is_executing (ptid_t ptid);
 
diff --git a/gdb/thread.c b/gdb/thread.c
index 99fe424..b4ba4c9 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1011,6 +1011,8 @@  is_executing (ptid_t ptid)
   return tp->executing;
 }
 
+/* See common/common-gdbthread.h.  */
+
 void
 set_executing (ptid_t ptid, int executing)
 {