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

[Luis Machado <lgustavo@codesourcery.com>]

On 12/22/2016 09:39 PM, 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.  As a side note, due to the way
> gdbserver marks a thread as executing, a new argument needed to be
> added on set_executing.  This argument is a 'struct target_waitstatus
> *', and is not necessary on GDB, so it was marked with
> ATTRIBUTE_UNUSED accordingly.
>
> gdb/ChangeLog:
> 2016-12-22  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.  Added an extra argument, "struct
> 	target_waitstatus *".
> 	* infrun.c (handle_inferior_event_1): Update call of
> 	"set_executing".
> 	* linux-nat.c (attach_proc_task_lwp_callback): Likewise.
> 	(linux_handle_extended_wait): Likewise.
> 	* record-btrace.c (get_thread_current_frame): Likewise.
> 	* record-full.c (record_full_wait_1): Likewise.
> 	* remote.c (remote_add_thread): Likewise.
> 	(process_initial_stop_replies): Likewise.
> 	* solib-spu.c (spu_skip_standalone_loader): Likewise.
> 	* target.c (target_resume): Likewise.
> 	* thread.c (set_executing): Update function declaration to add
> 	third argument.  Mark it as unused.
>
> gdb/gdbserver/ChangeLog:
> 2016-12-22  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* inferiors.c (init_thread_list): New function.
> 	(switch_to_thread): Likewise.
> 	(set_executing): Likewise.
> 	(add_thread_silent): Likewise.
> ---
>  gdb/Makefile.in               |  1 +
>  gdb/common/common-gdbthread.h | 50 +++++++++++++++++++++++++++++++++++++++++++
>  gdb/fork-child.c              |  2 +-
>  gdb/gdbserver/inferiors.c     | 42 ++++++++++++++++++++++++++++++++++++
>  gdb/gdbthread.h               | 20 +----------------
>  gdb/infrun.c                  |  2 +-
>  gdb/linux-nat.c               |  4 ++--
>  gdb/record-btrace.c           |  6 +++---
>  gdb/record-full.c             |  4 ++--
>  gdb/remote.c                  |  4 ++--
>  gdb/solib-spu.c               |  2 +-
>  gdb/target.c                  |  2 +-
>  gdb/thread.c                  |  5 ++++-
>  13 files changed, 111 insertions(+), 33 deletions(-)
>  create mode 100644 gdb/common/common-gdbthread.h
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 8372b4a..ca13a80 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..3560892
> --- /dev/null
> +++ b/gdb/common/common-gdbthread.h
> @@ -0,0 +1,50 @@
> +/* Multi-process/thread control defs for GDB, the GNU debugger.
> +   Copyright (C) 1987-2016 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.  */
> +

Spurious newline, multiple cases of this throughout.

> +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,
> +			   struct target_waitstatus *ws);
> +
> +/* 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/fork-child.c b/gdb/fork-child.c
> index 15f8249..38fca60 100644
> --- a/gdb/fork-child.c
> +++ b/gdb/fork-child.c
> @@ -545,7 +545,7 @@ startup_inferior (int ntraps)
>      }
>
>    /* Mark all threads non-executing.  */
> -  set_executing (resume_ptid, 0);
> +  set_executing (resume_ptid, 0, NULL);
>  }
>
>  /* Implement the "unset exec-wrapper" command.  */
> diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
> index 574a7ba..6b981d0 100644
> --- a/gdb/gdbserver/inferiors.c
> +++ b/gdb/gdbserver/inferiors.c
> @@ -468,3 +468,45 @@ 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,
> +	       struct target_waitstatus *ws)
> +{
> +  gdb_assert (current_thread != NULL);
> +  current_thread->last_resume_kind = resume_stop;
> +  current_thread->last_status = *ws;

Is there a chance ws will be NULL and cause a crash here? Should we assert?

> +}
> +
> +/* 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/gdbthread.h b/gdb/gdbthread.h
> index 8f37fbb..d6fdc42 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/infrun.c b/gdb/infrun.c
> index bf0632e..f5cc965 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -4947,7 +4947,7 @@ handle_inferior_event_1 (struct execution_control_state *ecs)
>      else
>        mark_ptid = ecs->ptid;
>
> -    set_executing (mark_ptid, 0);
> +    set_executing (mark_ptid, 0, NULL);
>
>      /* Likewise the resumed flag.  */
>      set_resumed (mark_ptid, 0);
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index cbf94ed..9db620c 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1190,7 +1190,7 @@ attach_proc_task_lwp_callback (ptid_t ptid)
>  	     raw clone).  */
>  	  add_thread (lp->ptid);
>  	  set_running (lp->ptid, 1);
> -	  set_executing (lp->ptid, 1);
> +	  set_executing (lp->ptid, 1, NULL);
>  	}
>
>        return 1;
> @@ -2083,7 +2083,7 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
>  	     and the user/frontend, this new thread is running until
>  	     it next reports a stop.  */
>  	  set_running (new_lp->ptid, 1);
> -	  set_executing (new_lp->ptid, 1);
> +	  set_executing (new_lp->ptid, 1, NULL);
>
>  	  if (WSTOPSIG (status) != SIGSTOP)
>  	    {
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 7c0e39f..80cf274 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -1928,7 +1928,7 @@ get_thread_current_frame (struct thread_info *tp)
>       move the thread.  Since we need to recompute the stack, we temporarily
>       set EXECUTING to flase.  */
>    executing = is_executing (inferior_ptid);
> -  set_executing (inferior_ptid, 0);
> +  set_executing (inferior_ptid, 0, NULL);
>
>    frame = NULL;
>    TRY
> @@ -1938,7 +1938,7 @@ get_thread_current_frame (struct thread_info *tp)
>    CATCH (except, RETURN_MASK_ALL)
>      {
>        /* Restore the previous execution state.  */
> -      set_executing (inferior_ptid, executing);
> +      set_executing (inferior_ptid, executing, NULL);
>
>        /* Restore the previous inferior_ptid.  */
>        inferior_ptid = old_inferior_ptid;
> @@ -1948,7 +1948,7 @@ get_thread_current_frame (struct thread_info *tp)
>    END_CATCH
>
>    /* Restore the previous execution state.  */
> -  set_executing (inferior_ptid, executing);
> +  set_executing (inferior_ptid, executing, NULL);
>
>    /* Restore the previous inferior_ptid.  */
>    inferior_ptid = old_inferior_ptid;
> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index 5608e70..3ad9a08 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -1156,12 +1156,12 @@ record_full_wait_1 (struct target_ops *ops,
>  			{
>  			  /* Try to insert the software single step breakpoint.
>  			     If insert success, set step to 0.  */
> -			  set_executing (inferior_ptid, 0);
> +			  set_executing (inferior_ptid, 0, NULL);
>  			  reinit_frame_cache ();
>
>  			  step = !insert_single_step_breakpoints (gdbarch);
>
> -			  set_executing (inferior_ptid, 1);
> +			  set_executing (inferior_ptid, 1, NULL);
>  			}
>
>  		      if (record_debug)
> diff --git a/gdb/remote.c b/gdb/remote.c
> index ef6c54e..0b5f837 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -1847,7 +1847,7 @@ remote_add_thread (ptid_t ptid, int running, int executing)
>      thread = add_thread (ptid);
>
>    get_private_info_thread (thread)->vcont_resumed = executing;
> -  set_executing (ptid, executing);
> +  set_executing (ptid, executing, NULL);
>    set_running (ptid, running);
>  }
>
> @@ -3949,7 +3949,7 @@ process_initial_stop_replies (int from_tty)
>  	  || ws.value.sig != GDB_SIGNAL_0)
>  	thread->suspend.waitstatus_pending_p = 1;
>
> -      set_executing (event_ptid, 0);
> +      set_executing (event_ptid, 0, NULL);
>        set_running (event_ptid, 0);
>        thread->priv->vcont_resumed = 0;
>      }
> diff --git a/gdb/solib-spu.c b/gdb/solib-spu.c
> index fa2977e..f55e0a0 100644
> --- a/gdb/solib-spu.c
> +++ b/gdb/solib-spu.c
> @@ -88,7 +88,7 @@ spu_skip_standalone_loader (void)
>
>        target_resume (inferior_ptid, 1, GDB_SIGNAL_0);
>        target_wait (minus_one_ptid, &ws, 0);
> -      set_executing (minus_one_ptid, 0);
> +      set_executing (minus_one_ptid, 0, NULL);
>
>        inferior_thread ()->control.in_infcall = 0;
>      }
> diff --git a/gdb/target.c b/gdb/target.c
> index 246d292..e7c68bf 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2325,7 +2325,7 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal)
>    registers_changed_ptid (ptid);
>    /* We only set the internal executing state here.  The user/frontend
>       running state is set at a higher level.  */
> -  set_executing (ptid, 1);
> +  set_executing (ptid, 1, NULL);
>    clear_inline_frame_state (ptid);
>  }
>
> diff --git a/gdb/thread.c b/gdb/thread.c
> index e5d6c5f..1114aac 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1011,8 +1011,11 @@ is_executing (ptid_t ptid)
>    return tp->executing;
>  }
>
> +/* See common/common-gdbthread.h.  */
> +
>  void
> -set_executing (ptid_t ptid, int executing)
> +set_executing (ptid_t ptid, int executing,
> +	       struct target_waitstatus *ws ATTRIBUTE_UNUSED)

I'm thinking about this change. Isn't there a better way to do this 
without adding gdb/gdbserver-specific parameters and then passing dummy 
values for the unused parameters depending on who's calling?

Maybe making gdbserver accept the same parameters as gdb and then do 
some extra processing in gdbserver to get at the waitstatus data if it 
still needs that? Or maybe unifying the function and making gdb and 
gdbserver operate the same way in this regard.

>  {
>    struct thread_info *tp;
>    int all = ptid_equal (ptid, minus_one_ptid);
>