[PATCH v1] [gdb/python]: Add StepEndedEvent (simplifies DAP)

[PATCH v1] [gdb/python]: Add StepEndedEvent (simplifies DAP)

[Simon Farre]

Adds the StepEndedEvent which signals that one of the thread finite state machines
finished. Matches the behavior of what is generated by MI; the "end stepping range".

This should simplify some of the DAP code, where "expected stop reason" is being tracked.
This logic should be handled by the Python interpreter to begin with, instead.
---
 gdb/NEWS                      |  3 +++
 gdb/doc/python.texi           |  3 +++
 gdb/python/py-event-types.def |  5 +++++
 gdb/python/py-stopevent.c     | 17 +++++++++++++++++
 4 files changed, 28 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index 81264c0cfb3..b2abaa6a2ce 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -11,6 +11,9 @@
   ** New function gdb.notify_mi(NAME, DATA), that emits custom
      GDB/MI async notification.
 
+  ** Added StepEndedEvent which is emitted during stops where it could be determined
+     that what triggered the stop was that the stepping state machine finished.
+
 *** Changes in GDB 14
 
 * GDB now supports the AArch64 Scalable Matrix Extension 2 (SME2), which
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 546b4d4b962..c6968791eb1 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3733,6 +3733,9 @@ registry extend @code{gdb.StopEvent}.  As a child of
 thread when @value{GDBN} is running in non-stop mode.  Refer to
 @code{gdb.ThreadEvent} above for more details.
 
+Emits @code{gdb.StepEndedEvent} that signals that this stop event was generated
+because one of the step-like commands finished.
+
 Emits @code{gdb.SignalEvent}, which extends @code{gdb.StopEvent}.
 
 This event indicates that the inferior or one of its threads has
diff --git a/gdb/python/py-event-types.def b/gdb/python/py-event-types.def
index c6225115027..36146b181c5 100644
--- a/gdb/python/py-event-types.def
+++ b/gdb/python/py-event-types.def
@@ -106,6 +106,11 @@ GDB_PY_DEFINE_EVENT_TYPE (signal,
 			  "GDB signal event object",
 			  stop_event_object_type);
 
+GDB_PY_DEFINE_EVENT_TYPE (step_ended,
+			  "StepEndedEvent",
+			  "GDB step ended event object",
+			  stop_event_object_type);
+
 GDB_PY_DEFINE_EVENT_TYPE (stop,
 			  "StopEvent",
 			  "GDB stop event object",
diff --git a/gdb/python/py-stopevent.c b/gdb/python/py-stopevent.c
index 0aa9d5381f8..13e1b12d33a 100644
--- a/gdb/python/py-stopevent.c
+++ b/gdb/python/py-stopevent.c
@@ -18,6 +18,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "mi/mi-common.h"
 #include "py-stopevent.h"
 
 gdbpy_ref<>
@@ -27,6 +28,21 @@ create_stop_event_object (PyTypeObject *py_type)
   return create_thread_event_object (py_type, thread.get ());
 }
 
+/** If thread finite state machine reports that it has finished, inform
+    the Python-interpreter of this. */
+
+static gdbpy_ref<> maybe_create_step_ended () 
+{
+  const auto tp = inferior_thread();
+  const auto fsm = tp != nullptr ? tp->thread_fsm() : nullptr;
+  if (fsm != nullptr && fsm->finished_p () && 
+	fsm->async_reply_reason () == EXEC_ASYNC_END_STEPPING_RANGE)
+    {
+      return create_stop_event_object (&step_ended_event_object_type);
+    }
+   return nullptr;
+}
+
 /* Callback observers when a stop event occurs.  This function will create a
    new Python stop event object.  If only a specific thread is stopped the
    thread object of the event will be set to that thread.  Otherwise, if all
@@ -86,6 +102,7 @@ emit_stop_event (struct bpstat *bs, enum gdb_signal stop_signal)
 	return -1;
     }
 
+  stop_event_obj = maybe_create_step_ended();
   /* If all fails emit an unknown stop event.  All event types should
      be known and this should eventually be unused.  */
   if (stop_event_obj == NULL)
-- 
2.41.0

Re: [PATCH v1] [gdb/python]: Add StepEndedEvent (simplifies DAP)

[Eli Zaretskii]

> From: Simon Farre <simon.farre.cx@gmail.com>
> Cc: tromey@adacore.com,
> 	Simon Farre <simon.farre.cx@gmail.com>
> Date: Mon, 16 Oct 2023 13:50:26 +0200
> 
> Adds the StepEndedEvent which signals that one of the thread finite state machines
> finished. Matches the behavior of what is generated by MI; the "end stepping range".

Thanks.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 81264c0cfb3..b2abaa6a2ce 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -11,6 +11,9 @@
>    ** New function gdb.notify_mi(NAME, DATA), that emits custom
>       GDB/MI async notification.
>  
> +  ** Added StepEndedEvent which is emitted during stops where it could be determined
> +     that what triggered the stop was that the stepping state machine finished.
> +
>  *** Changes in GDB 14

This is OK, but please reformat the text so that each line is no
longer than 79 columns.

> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -3733,6 +3733,9 @@ registry extend @code{gdb.StopEvent}.  As a child of
>  thread when @value{GDBN} is running in non-stop mode.  Refer to
>  @code{gdb.ThreadEvent} above for more details.
>  
> +Emits @code{gdb.StepEndedEvent} that signals that this stop event was generated
> +because one of the step-like commands finished.
> +
>  Emits @code{gdb.SignalEvent}, which extends @code{gdb.StopEvent}.

Likewise here.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH v1] [gdb/python]: Add StepEndedEvent (simplifies DAP)

[Andrew Burgess]

Simon Farre <simon.farre.cx@gmail.com> writes:

> Adds the StepEndedEvent which signals that one of the thread finite state machines
> finished. Matches the behavior of what is generated by MI; the "end stepping range".
>
> This should simplify some of the DAP code, where "expected stop reason" is being tracked.
> This logic should be handled by the Python interpreter to begin with, instead.
> ---
>  gdb/NEWS                      |  3 +++
>  gdb/doc/python.texi           |  3 +++
>  gdb/python/py-event-types.def |  5 +++++
>  gdb/python/py-stopevent.c     | 17 +++++++++++++++++

This will definitely need some tests writing for it.  It might be
possible to just add some additional cases to gdb.python/py-events.exp
to cover this new functionality.

>  4 files changed, 28 insertions(+)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 81264c0cfb3..b2abaa6a2ce 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -11,6 +11,9 @@
>    ** New function gdb.notify_mi(NAME, DATA), that emits custom
>       GDB/MI async notification.
>  
> +  ** Added StepEndedEvent which is emitted during stops where it could be determined
> +     that what triggered the stop was that the stepping state machine finished.
> +
>  *** Changes in GDB 14
>  
>  * GDB now supports the AArch64 Scalable Matrix Extension 2 (SME2), which
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 546b4d4b962..c6968791eb1 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -3733,6 +3733,9 @@ registry extend @code{gdb.StopEvent}.  As a child of
>  thread when @value{GDBN} is running in non-stop mode.  Refer to
>  @code{gdb.ThreadEvent} above for more details.
>  
> +Emits @code{gdb.StepEndedEvent} that signals that this stop event was generated
> +because one of the step-like commands finished.
> +
>  Emits @code{gdb.SignalEvent}, which extends @code{gdb.StopEvent}.
>  
>  This event indicates that the inferior or one of its threads has
> diff --git a/gdb/python/py-event-types.def b/gdb/python/py-event-types.def
> index c6225115027..36146b181c5 100644
> --- a/gdb/python/py-event-types.def
> +++ b/gdb/python/py-event-types.def
> @@ -106,6 +106,11 @@ GDB_PY_DEFINE_EVENT_TYPE (signal,
>  			  "GDB signal event object",
>  			  stop_event_object_type);
>  
> +GDB_PY_DEFINE_EVENT_TYPE (step_ended,
> +			  "StepEndedEvent",
> +			  "GDB step ended event object",
> +			  stop_event_object_type);
> +
>  GDB_PY_DEFINE_EVENT_TYPE (stop,
>  			  "StopEvent",
>  			  "GDB stop event object",
> diff --git a/gdb/python/py-stopevent.c b/gdb/python/py-stopevent.c
> index 0aa9d5381f8..13e1b12d33a 100644
> --- a/gdb/python/py-stopevent.c
> +++ b/gdb/python/py-stopevent.c
> @@ -18,6 +18,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #include "defs.h"
> +#include "mi/mi-common.h"
>  #include "py-stopevent.h"
>  
>  gdbpy_ref<>
> @@ -27,6 +28,21 @@ create_stop_event_object (PyTypeObject *py_type)
>    return create_thread_event_object (py_type, thread.get ());
>  }
>  
> +/** If thread finite state machine reports that it has finished, inform
> +    the Python-interpreter of this. */

Start comments with '/*' and end with two spaces after the '.' before '*/'.

> +
> +static gdbpy_ref<> maybe_create_step_ended () 

Newline before the function name.  Plus should have a comment above the
function explaining what it does.

> +{
> +  const auto tp = inferior_thread();
> +  const auto fsm = tp != nullptr ? tp->thread_fsm() : nullptr;

2x missing space before ().

> +  if (fsm != nullptr && fsm->finished_p () && 
> +	fsm->async_reply_reason () == EXEC_ASYNC_END_STEPPING_RANGE)

The '&&' should be at the start of the new line.

> +    {
> +      return create_stop_event_object (&step_ended_event_object_type);
> +    }

Single statement blocks don't require { ... } around them.

Thanks,
Andrew

> +   return nullptr;
> +}
> +
>  /* Callback observers when a stop event occurs.  This function will create a
>     new Python stop event object.  If only a specific thread is stopped the
>     thread object of the event will be set to that thread.  Otherwise, if all
> @@ -86,6 +102,7 @@ emit_stop_event (struct bpstat *bs, enum gdb_signal stop_signal)
>  	return -1;
>      }
>  
> +  stop_event_obj = maybe_create_step_ended();
>    /* If all fails emit an unknown stop event.  All event types should
>       be known and this should eventually be unused.  */
>    if (stop_event_obj == NULL)
> -- 
> 2.41.0

Re: [PATCH v1] [gdb/python]: Add StepEndedEvent (simplifies DAP)

[Simon Farre]

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

> This will definitely need some tests writing for it.  It might be
> possible to just add some additional cases to gdb.python/py-events.exp
> to cover this new functionality.
I'll get to that. I'll probably add a completely new test, because I 
find the Tcl test suites more or less unintelligible, so altering them 
becomes almost unreasonably difficult to modify and still be sure that 
the test still tests what it was meant to.If that's ok, that is?

I'll also address the formatting errors (you can ignore v2 - it just 
contained a bug fix but without the coming changes).

Thanks!

Re: [PATCH v1] [gdb/python]: Add StepEndedEvent (simplifies DAP)

[Tom Tromey]

>>>>> "Simon" == Simon Farre <simon.farre.cx@gmail.com> writes:

Simon> Adds the StepEndedEvent which signals that one of the thread
Simon> finite state machines finished. Matches the behavior of what is
Simon> generated by MI; the "end stepping range".

Simon> This should simplify some of the DAP code, where "expected stop
Simon> reason" is being tracked.  This logic should be handled by the
Simon> Python interpreter to begin with, instead.

If possible it would be better to go all the way and add the stop reason
to the stop event all the time.  This has been wanted for a long time,
see

https://sourceware.org/bugzilla/show_bug.cgi?id=13587

Simon> +#include "mi/mi-common.h"

I think this isn't needed in the current patch, but I guess would be
needed if this called async_reason_lookup.

Simon> +static gdbpy_ref<> maybe_create_step_ended () 
Simon> +{
Simon> +  const auto tp = inferior_thread();
Simon> +  const auto fsm = tp != nullptr ? tp->thread_fsm() : nullptr;
Simon> +  if (fsm != nullptr && fsm->finished_p () && 
Simon> +	fsm->async_reply_reason () == EXEC_ASYNC_END_STEPPING_RANGE)

The stop reason idea is basically -- why single out one kind of stop
reason when we could do something more generic.

We wouldn't need a subclass, I guess, we could just stick the extra info
into the current event.

Not sure though.  This area is kind of messy due to some unfortunate
choices early on.

Tom

Re: [PATCH v1] [gdb/python]: Add StepEndedEvent (simplifies DAP)

[Tom Tromey]

>>>>> "Simon" == Simon Farre <simon.farre.cx@gmail.com> writes:

Simon> Adds the StepEndedEvent which signals that one of the thread finite state machines
Simon> finished. Matches the behavior of what is generated by MI; the "end stepping range".

Simon> This should simplify some of the DAP code, where "expected stop reason" is being tracked.
Simon> This logic should be handled by the Python interpreter to begin with, instead.

I ended up writing what we should have done ages ago, adding stop reason
information to stop events.  I'll send the patch soon.  It includes a
change to simplify the DAP code.  Let me know what you think.

Tom