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

Luis Machado
2017-02-08T09:07:14Z

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.