Re: [PATCH v2 4/6] Share parts of gdb/gdbthread.h with gdbserver

[Luis Machado <lgustavo@codesourcery.com>]

On 02/07/2017 04:41 PM, Sergio Durigan Junior wrote:
> On Wednesday, February 01 2017, Luis Machado wrote:
>
>> On 01/18/2017 09:36 AM, Sergio Durigan Junior wrote:
>>> 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:
>>> 2017-01-17  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:
>>> 2017-01-17  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
>>>
>>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>>> index c0325d5..3e49e6e 100644
>>> --- a/gdb/Makefile.in
>>> +++ b/gdb/Makefile.in
>>> @@ -1467,6 +1467,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 ();
>>> +}
>>> +
>>
>>
>> I'm still not sure about this particular function. But i'd like to
>> hear what other think.
>
> What are your concerns?

Having those parameters that are not actually used, as opposed to having 
a function that is the same for both GDB and GDBserver.