Re: [non-stop] 10/10 split user/internal threads

[Daniel Jacobowitz <drow at false dot org>]

On Sun, Jun 15, 2008 at 10:09:44PM +0100, Pedro Alves wrote:
> I've added three points where GDB makes the internal (inferior_ptid)
> thread switch to the user selected thread (user_selected_ptid):
> 
> ?1 - execute_command, before executing a command
> ?2 - execute_command, after executing a command,
> ?3 - fetch_inferior_event, after handling the event.
> 
> With these in place, the user never notices when GDB switches
> between threads internally while handing events.

The _internal name is a warning sign to me: it means we have two
functions that conceptually do the same thing, but one of them is only
"safe for internal use".  We call both of them from internal to GDB,
so let's try to find a better name.  How about
execute_command_in_current_context?

Is -interpreter-exec ever going to get a --thread argument?  We have
to be careful because it currently restores the globally selected
thread via cli_interpreter_exec, and even if it didn't it may call
a user defined command.

There are 22 calls to execute_command in GDB (including two from
Insight).  This patch changes seven of them to
execute_command_internal: thread.c, mi-main.c, mi-cmd-env.c.
I checked all the others, just in case - they mostly look correct.

The only cases I'm worried about are breakpoint commands and
conditions.  For instance bpstat_do_actions to execute_control_command
to execute_command will mess with inferior_ptid.  Should it?
What about bpstat_check_breakpoint_conditions to breakpoint_cond_eval
to evaluate_expression?

Also, hook- and hookpost- commands will still be run after
execute_command_internal - but every call to execute_command_internal
manually sets the appropriate thread, so this looks fine.

> +enum thread_state
> +{
> +  THREAD_STOPPED,
> +  THREAD_RUNNING,
> +  THREAD_EXITED,
> +};
> +
> +static enum thread_state main_thread_state = 0;

THREAD_STOPPED instead of 0?

> -  /* Backup current thread and selected frame.  */
> -  if (!is_running (inferior_ptid))
> -    saved_frame_id = get_frame_id (get_selected_frame (NULL));
> -  else
> -    saved_frame_id = null_frame_id;
> -
> -  old_chain = make_cleanup_restore_current_thread (inferior_ptid, saved_frame_id);

Is this sort of simplification really a good idea?  Functions which
leave inferior_ptid with an essentially unspecified value, while it's
still global state used by most of GDB.

> +/* Only safe to use this cleanup on a stopped thread, and
> +   inferior_ptid isn't ran before running the cleanup.  */
> +
>  struct current_thread_cleanup

Not sure what you mean by this comment.

> +  /* In non-stop mode, we don't want GDB to switch threads on the
> +     users back, to avoid races where the user is typing a command to

user's

-- 
Daniel Jacobowitz
CodeSourcery