RE: [PATCH 01/23] Preserve selected thread in all-stop w/ background execution

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3872 bytes --]

* On September 7, 2019 1:28 AM, Pedro Alves wrote:
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index a9588f896a..9c888aa72f 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3048,6 +3048,11 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
> 
>    finish_state.release ();
> 
> +  /* If we've switched threads above, switch back to the previously
> +     current thread.  We don't want the user to see a different
> +     selected thread.  */
> +  switch_to_thread (cur_thr);
> +
>    /* Tell the event loop to wait for it to stop.  If the target
>       supports asynchronous execution, it'll do this from within
>       target_resume.  */
> @@ -3702,14 +3707,11 @@ fetch_inferior_event (void *client_data)
>  	set_current_traceframe (-1);
>        }
> 
> -    gdb::optional<scoped_restore_current_thread> maybe_restore_thread;
> -
> -    if (non_stop)
> -      /* In non-stop mode, the user/frontend should not notice a thread
> -	 switch due to internal events.  Make sure we reverse to the
> -	 user selected thread and frame after handling the event and
> -	 running any breakpoint commands.  */
> -      maybe_restore_thread.emplace ();
> +    /* The user/frontend should not notice a thread switch due to
> +       internal events.  Make sure we revert to the user selected
> +       thread and frame after handling the event and running any
> +       breakpoint commands.  */
> +    scoped_restore_current_thread restore_thread;
> 

Because this increases the refcount of the current thread, in case the
fetched inferior event denotes a thread exit, the thread will not be deleted
right away.  A non-deleted but exited thread stays in the inferior's thread
list.  This, in turn, causes the "init_thread_list" call in inferior.c to
be skipped.  As a side effect, a regression is observed in

  gdb.arch/i386-mpx-simple_segv.exp

IMHO, the 'any_thread_p' predicate should be updated.  This predicate is used
in two places (one in 'inferior.c' and the other in 'mi/mi-main.c').  Both
uses, I believe, are in fact interested in whether there are any non-exited
threads.  I'd suggest updating 'any_thread_p' to 'any_non_exited_thread_p'.

I'm attaching a patch that can be used to test.

>      overlay_cache_invalid = 1;
>      /* Flush target cache before starting to handle each event.  Target
> @@ -3786,6 +3788,19 @@ fetch_inferior_event (void *client_data)
>  		inferior_event_handler (INF_EXEC_COMPLETE, NULL);
>  		cmd_done = 1;
>  	      }
> +
> +	    /* If we got a TARGET_WAITKIND_NO_RESUMED event, then the
> +	       previously selected thread is gone.  We have two
> +	       choices - switch to no thread selected, or restore the
> +	       previously selected thread (now exited).  We chose the
> +	       later, just because that's what GDB used to do.  After
> +	       this, "info threads" says "The current thread <Thread
> +	       ID 2> has terminated." instead of "No thread
> +	       selected.".  */
> +	    if (!non_stop
> +		&& cmd_done
> +		&& ecs->ws.kind != TARGET_WAITKIND_NO_RESUMED)
> +	      restore_thread.dont_restore ();
>  	  }
>        }
> 

The comment and the code seem to contradict each other.  The comment says
"if we got a TARGET_WAITKIND_NO_RESUMED" whereas the condition is
  
  ecs->ws.kind != TARGET_WAITKIND_NO_RESUMED

Should TARGET_WAITKIND_THREAD_EXITED, TARGET_WAITKIND_EXITED, and
TARGET_WAITKIND_SIGNALLED be also included in the condition?  They also mean
that the thread is gone, right?

Regards,
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[-- Attachment #2: any_thread_p.patch --]
[-- Type: application/octet-stream, Size: 1974 bytes --]

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index d00cc3fb8f..ce04fa6e42 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -579,8 +579,8 @@ all_threads_safe ()
 
 extern int thread_count (process_stratum_target *proc_target);
 
-/* Return true if we have any thread in any inferior.  */
-extern bool any_thread_p ();
+/* Return true if we have any non-exited thread in any inferior.  */
+extern bool any_non_exited_thread_p ();
 
 /* Switch context to thread THR.  Also sets the STOP_PC global.  */
 extern void switch_to_thread (struct thread_info *thr);
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 96d38f865d..9e254a96af 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -249,7 +249,7 @@ inferior_appeared (struct inferior *inf, int pid)
 {
   /* If this is the first inferior with threads, reset the global
      thread id.  */
-  if (!any_thread_p ())
+  if (!any_non_exited_thread_p ())
     init_thread_list ();
 
   inf->pid = pid;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 5dc436bfd8..ee690e6daf 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1991,7 +1991,7 @@ mi_execute_command (const char *cmd, int from_tty)
 	  top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ()
 	  /* Don't try report anything if there are no threads --
 	     the program is dead.  */
-	  && any_thread_p ()
+	  && any_non_exited_thread_p ()
 	  /* If the command already reports the thread change, no need to do it
 	     again.  */
 	  && !command_notifies_uscc_observer (command.get ()))
diff --git a/gdb/thread.c b/gdb/thread.c
index 9dafca57bf..37e007d9c5 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -581,9 +581,9 @@ iterate_over_threads (int (*callback) (struct thread_info *, void *),
 /* See gdbthread.h.  */
 
 bool
-any_thread_p ()
+any_non_exited_thread_p ()
 {
-  for (thread_info *tp ATTRIBUTE_UNUSED : all_threads ())
+  for (thread_info *tp ATTRIBUTE_UNUSED : all_non_exited_threads ())
     return true;
   return false;
 }