[PATCH] gdb/infrum: assert !step_over_info_valid_p in restart_threads

[PATCH] gdb/infrum: assert !step_over_info_valid_p in restart_threads

[Lancelot SIX]

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
---
 gdb/infrun.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index c311240b78c..6564a177e0e 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);
     }
 }
 
@@ -5866,6 +5870,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 static void
 restart_threads (struct thread_info *event_thread, inferior *inf)
 {
+  gdb_assert (!step_over_info_valid_p ());
   INFRUN_SCOPED_DEBUG_START_END ("event_thread=%s, inf=%d",
 				 event_thread->ptid.to_string ().c_str (),
 				 inf != nullptr ? inf->num : -1);

base-commit: 6ea673e2d643b7b2283aa73d35b02dfc9aa7115f
-- 
2.25.1

Re: [PATCH] gdb/infrum: assert !step_over_info_valid_p in restart_threads

[Pedro Alves]

On 2022-04-19 23:47, Lancelot SIX via Gdb-patches wrote:
> 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
> ---
>  gdb/infrun.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c311240b78c..6564a177e0e 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);

This looks fine.

>      }
>  }
>  
> @@ -5866,6 +5870,7 @@ handle_inferior_event (struct execution_control_state *ecs)
>  static void
>  restart_threads (struct thread_info *event_thread, inferior *inf)
>  {
> +  gdb_assert (!step_over_info_valid_p ());
>    INFRUN_SCOPED_DEBUG_START_END ("event_thread=%s, inf=%d",
>  				 event_thread->ptid.to_string ().c_str (),
>  				 inf != nullptr ? inf->num : -1);
> 

Nit: this looks a bit too crammed to my eyes -- I mean, I would expect either the assertion
to be below the INFRUN_SCOPED_DEBUG_START_END (my preference, as it'll probably
print useful context to debug an assertion failure) with an empty line in between,
or at least put an empty line between the assert and the INFRUN_SCOPED_DEBUG_START_END.

Re: [PATCH] gdb/infrum: assert !step_over_info_valid_p in restart_threads

[Pedro Alves]

Also, infrum -> infrun.

Re: [PATCH] gdb/infrum: assert !step_over_info_valid_p in restart_threads

[Lancelot SIX]

>> @@ -5866,6 +5870,7 @@ handle_inferior_event (struct execution_control_state *ecs)
>>   static void
>>   restart_threads (struct thread_info *event_thread, inferior *inf)
>>   {
>> +  gdb_assert (!step_over_info_valid_p ());
>>     INFRUN_SCOPED_DEBUG_START_END ("event_thread=%s, inf=%d",
>>                                 event_thread->ptid.to_string ().c_str (),
>>                                 inf != nullptr ? inf->num : -1);
>>
> 
> Nit: this looks a bit too crammed to my eyes -- I mean, I would expect either the assertion
> to be below the INFRUN_SCOPED_DEBUG_START_END (my preference, as it'll probably
> print useful context to debug an assertion failure) with an empty line in between,
> or at least put an empty line between the assert and the INFRUN_SCOPED_DEBUG_START_END.

Thanks,

I changed this locally (i.e. the gdb_assert goes after the 
INFRUN_SCOPED_DEBUG_START_END call, with one empty line in between), as 
well as fixed the commit title.

I'll send the V2 shortly.

Best,
Lancelot.

[PATCH v2] gdb/infrun: assert !step_over_info_valid_p in restart_threads

[Lancelot SIX]

Hi,

This is a V2 for https://sourceware.org/pipermail/gdb-patches/2022-April/188062.html

Since V1:

- Fixed typo in the subject
- Placed the gdb_assert in restart_threads after INFRUN_SCOPED_DEBUG_START_END

Best,
Lancelot

---

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
---
 gdb/infrun.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index c311240b78c..fa2adb6d2a0 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);
     }
 }
 
@@ -5870,6 +5874,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 ();
 

base-commit: 9d748d023d2df565e04974b5bb45618c24b30c15
-- 
2.25.1

Re: [PATCH v2] gdb/infrun: assert !step_over_info_valid_p in restart_threads

[Pedro Alves]

This is OK.

Re: [PATCH v2] gdb/infrun: assert !step_over_info_valid_p in restart_threads

[Lancelot SIX]

Thanks.

I just pushed this patch.

Best,
Lancelot.

On 22/04/2022 17:54, Pedro Alves wrote:
> [CAUTION: External Email]
> 
> This is OK.