[binutils-gdb] gdb/infrun: assert !step_over_info_valid_p in restart_threads

[binutils-gdb] gdb/infrun: assert !step_over_info_valid_p in restart_threads

[Lancelot SIX]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=2b718529b99d2ca53552558ba9b3ff3f22663795

commit 2b718529b99d2ca53552558ba9b3ff3f22663795
Author: Lancelot SIX <lancelot.six@amd.com>
Date:   Tue Apr 19 23:13:29 2022 +0100

    gdb/infrun: assert !step_over_info_valid_p in restart_threads
    
    While working in gdb/infrun.c:restart_threads, I was wondering what are
    the preconditions to call the function.  It seems to me that
    !step_over_info_valid_p should be a precondition (i.e. if we are doing
    an inline step over breakpoint, we do not want to resume non stepping
    threads as one of them might miss the breakpoint which is temporally
    disabled).
    
    To convince myself that this is true, I have added an assertion to
    enforce this, and got one regression in the testsuite:
    
        FAIL: gdb.base/step-over-syscall.exp: vfork: displaced=off: single step over vfork (GDB internal error)
    
    This call to restart_threads originates from handle_vfork_done which
    does not check if a step over is active when restarting other threads:
    
        if (target_is_non_stop_p ())
          {
            scoped_restore_current_thread restore_thread;
    
            insert_breakpoints ();
            restart_threads (event_thread, event_thread->inf);
            start_step_over ();
          }
    
    In this patch, I propose to:
    - Call start_step_over before restart_threads.  If a step over is already
      in progress (as it is the case in the failing testcase),
      start_step_over return immediately, and there is no point in restarting
      all threads just to stop them right away for a step over breakpoint.
    - Only call restart_threads if no step over is in progress at this
      point.
    
    In this patch, I also propose to keep the assertion in restart_threads
    to help enforce this precondition, and state it explicitly.
    
    I have also checked all other places which call restart_threads, and
    they all seem to check that there is no step over currently active
    before doing the call.
    
    As for infrun-related things, I am never completely sure I did not miss
    something.  So as usual, all feedback and thoughts are very welcome.
    
    Tested on x86_64-linux-gnu.
    
    Change-Id: If5f5f98ec4cf9aaeaabb5e3aa88ae6ffd70d4f37

Diff:
---
 gdb/infrun.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4e7ca803c79..e0a5bde037b 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -101,6 +101,8 @@ static void restart_threads (struct thread_info *event_thread,
 
 static bool start_step_over (void);
 
+static bool step_over_info_valid_p (void);
+
 /* Asynchronous signal handler registered as event loop source for
    when we have pending events ready to be passed to the core.  */
 static struct async_event_handler *infrun_async_inferior_event_token;
@@ -1088,8 +1090,10 @@ handle_vfork_done (thread_info *event_thread)
       scoped_restore_current_thread restore_thread;
 
       insert_breakpoints ();
-      restart_threads (event_thread, event_thread->inf);
       start_step_over ();
+
+      if (!step_over_info_valid_p ())
+	restart_threads (event_thread, event_thread->inf);
     }
 }
 
@@ -5877,6 +5881,8 @@ restart_threads (struct thread_info *event_thread, inferior *inf)
 				 event_thread->ptid.to_string ().c_str (),
 				 inf != nullptr ? inf->num : -1);
 
+  gdb_assert (!step_over_info_valid_p ());
+
   /* In case the instruction just stepped spawned a new thread.  */
   update_thread_list ();