[PATCH 0/3] Add assertion when marking the remote async flag

[PATCH 0/3] Add assertion when marking the remote async flag

[Simon Marchi]

I had this idea while reviewing this patch [1].  When marking the remote
async flag, assert that the target is actually in async mode, instead of
relying on an assert that comes at a later time, in the wait method.
The first two patches are small preparatory refactorings, and the third
one adds the assertion.

[1] https://inbox.sourceware.org/gdb-patches/3d728a6e-1cb0-49c2-a4c8-0a974be39fee@simark.ca/T/#ma1903117423ae09c3574fd45ade2dd4af5280633

Simon Marchi (3):
  gdb: make remote_state's async token private
  gdb: add remote_state::{is_async_p,can_async_p}
  gdb: add assertion when marking the remote async flag

 gdb/remote.c | 93 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 62 insertions(+), 31 deletions(-)


base-commit: 1181bcd0d2572aee2c0947040e56bc1f9af634e3
-- 
2.42.0

[PATCH 1/3] gdb: make remote_state's async token private

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Make remote_async_inferior_event_token private (rename to
m_async_event_handler_token) and add methods for the various operations
we do on it.  This will help by:

 - allowing to break on those methods when debugging
 - allowing to add assertions in the methods

Change-Id: Ia3b8a2bc48ad4849dbbe83442c3f83920f03334d
---
 gdb/remote.c | 70 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 9bb4f1de5982..2deba06d7d47 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -413,6 +413,31 @@ class remote_state
   /* Get the remote arch state for GDBARCH.  */
   struct remote_arch_state *get_remote_arch_state (struct gdbarch *gdbarch);
 
+  void create_async_event_handler ()
+  {
+    m_async_event_handler_token
+      = ::create_async_event_handler ([] (gdb_client_data data)
+      				      {
+					inferior_event_handler (INF_REG_EVENT);
+				      },
+				      nullptr, "remote");
+  }
+
+  void mark_async_event_handler ()
+  { ::mark_async_event_handler (m_async_event_handler_token); }
+
+  void clear_async_event_handler ()
+  { ::clear_async_event_handler (m_async_event_handler_token); }
+
+  bool async_event_handler_marked () const
+  { return ::async_event_handler_marked (m_async_event_handler_token); }
+
+  void delete_async_event_handler ()
+  {
+    if (m_async_event_handler_token != nullptr)
+      ::delete_async_event_handler (&m_async_event_handler_token);
+  }
+
 public: /* data */
 
   /* A buffer to use for incoming packets, and its current size.  The
@@ -540,10 +565,6 @@ class remote_state
      immediately, so queue is not needed for them.  */
   std::vector<stop_reply_up> stop_reply_queue;
 
-  /* Asynchronous signal handle registered as event loop source for
-     when we have pending events ready to be passed to the core.  */
-  struct async_event_handler *remote_async_inferior_event_token = nullptr;
-
   /* FIXME: cagney/1999-09-23: Even though getpkt was called with
      ``forever'' still use the normal timeout mechanism.  This is
      currently used by the ASYNC code to guarentee that target reads
@@ -554,6 +575,10 @@ class remote_state
   bool wait_forever_enabled_p = true;
 
 private:
+  /* Asynchronous signal handle registered as event loop source for
+     when we have pending events ready to be passed to the core.  */
+  async_event_handler *m_async_event_handler_token = nullptr;
+  
   /* Mapping of remote protocol data for each gdbarch.  Usually there
      is only one entry here, though we may see more with stubs that
      support multi-process.  */
@@ -1384,8 +1409,6 @@ static void show_remote_protocol_packet_cmd (struct ui_file *file,
 
 static ptid_t read_ptid (const char *buf, const char **obuf);
 
-static void remote_async_inferior_event_handler (gdb_client_data);
-
 static bool remote_read_description_p (struct target_ops *target);
 
 static void remote_console_output (const char *msg);
@@ -4389,8 +4412,7 @@ remote_target::~remote_target ()
      everything of this target.  */
   discard_pending_stop_replies_in_queue ();
 
-  if (rs->remote_async_inferior_event_token)
-    delete_async_event_handler (&rs->remote_async_inferior_event_token);
+  rs->delete_async_event_handler ();
 
   delete rs->notif_state;
 }
@@ -5989,9 +6011,8 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
   current_inferior ()->push_target (std::move (target_holder));
 
   /* Register extra event sources in the event loop.  */
-  rs->remote_async_inferior_event_token
-    = create_async_event_handler (remote_async_inferior_event_handler, nullptr,
-				  "remote");
+  rs->create_async_event_handler ();
+
   rs->notif_state = remote_notif_state_allocate (remote);
 
   /* Reset the target state; these things will be queried either by
@@ -7086,7 +7107,7 @@ remote_target::has_pending_events ()
     {
       remote_state *rs = get_remote_state ();
 
-      if (async_event_handler_marked (rs->remote_async_inferior_event_token))
+      if (rs->async_event_handler_marked ())
 	return true;
 
       /* Note that BUFCNT can be negative, indicating sticky
@@ -7420,7 +7441,7 @@ remote_notif_stop_can_get_pending_events (remote_target *remote,
      may exit and we have no chance to process them back in
      remote_wait_ns.  */
   remote_state *rs = remote->get_remote_state ();
-  mark_async_event_handler (rs->remote_async_inferior_event_token);
+  rs->mark_async_event_handler ();
   return 0;
 }
 
@@ -7628,7 +7649,7 @@ remote_target::queued_stop_reply (ptid_t ptid)
   if (!rs->stop_reply_queue.empty () && target_can_async_p ())
     {
       /* There's still at least an event left.  */
-      mark_async_event_handler (rs->remote_async_inferior_event_token);
+      rs->mark_async_event_handler ();
     }
 
   return r;
@@ -7655,7 +7676,7 @@ remote_target::push_stop_reply (struct stop_reply *new_event)
      enabled, and there are events in this queue, we will mark the event
      token at that point, see remote_target::async.  */
   if (target_is_async_p ())
-    mark_async_event_handler (rs->remote_async_inferior_event_token);
+    rs->mark_async_event_handler ();
 }
 
 /* Returns true if we have a stop reply for PTID.  */
@@ -8515,10 +8536,9 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
      we'll mark it again at the end if needed.  If the target is not in
      async mode then the async token should not be marked.  */
   if (target_is_async_p ())
-    clear_async_event_handler (rs->remote_async_inferior_event_token);
+    rs->clear_async_event_handler ();
   else
-    gdb_assert (!async_event_handler_marked
-		(rs->remote_async_inferior_event_token));
+    gdb_assert (!rs->async_event_handler_marked ());
 
   ptid_t event_ptid;
 
@@ -8533,7 +8553,7 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
 	 notifications, then tell the event loop to call us again.  */
       if (!rs->stop_reply_queue.empty ()
 	  || rs->notif_state->pending_event[notif_client_stop.id] != nullptr)
-	mark_async_event_handler (rs->remote_async_inferior_event_token);
+	rs->mark_async_event_handler ();
     }
 
   return event_ptid;
@@ -14859,12 +14879,6 @@ remote_async_serial_handler (struct serial *scb, void *context)
   inferior_event_handler (INF_REG_EVENT);
 }
 
-static void
-remote_async_inferior_event_handler (gdb_client_data data)
-{
-  inferior_event_handler (INF_REG_EVENT);
-}
-
 int
 remote_target::async_wait_fd ()
 {
@@ -14884,7 +14898,8 @@ remote_target::async (bool enable)
       /* If there are pending events in the stop reply queue tell the
 	 event loop to process them.  */
       if (!rs->stop_reply_queue.empty ())
-	mark_async_event_handler (rs->remote_async_inferior_event_token);
+	rs->mark_async_event_handler ();
+
       /* For simplicity, below we clear the pending events token
 	 without remembering whether it is marked, so here we always
 	 mark it.  If there's actually no pending notification to
@@ -14899,7 +14914,8 @@ remote_target::async (bool enable)
       /* If the core is disabling async, it doesn't want to be
 	 disturbed with target events.  Clear all async event sources
 	 too.  */
-      clear_async_event_handler (rs->remote_async_inferior_event_token);
+      rs->clear_async_event_handler ();
+
       if (target_is_non_stop_p ())
 	clear_async_event_handler (rs->notif_state->get_pending_events_token);
     }
-- 
2.42.0

[PATCH 2/3] gdb: add remote_state::{is_async_p,can_async_p}

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

A subsequent patch will want to know if the remote is async within a
remote_state method.  Add a helper method for that, and for "can async"
as well, for symmetry.

Change-Id: Id0f648ee4896736479fa942f5453eeeb0e5d4352
---
 gdb/remote.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 2deba06d7d47..38d0027dbf9e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -438,6 +438,20 @@ class remote_state
       ::delete_async_event_handler (&m_async_event_handler_token);
   }
 
+  bool is_async_p () const
+  {
+    /* We're async whenever the serial device is.  */
+    return (this->remote_desc
+	    != nullptr && serial_is_async_p (this->remote_desc));
+  }
+
+  bool can_async_p () const
+  {
+    /* We can async whenever the serial device can.  */
+    return (this->remote_desc
+	    != nullptr && serial_can_async_p (this->remote_desc));
+  }
+  
 public: /* data */
 
   /* A buffer to use for incoming packets, and its current size.  The
@@ -14853,16 +14867,14 @@ remote_target::can_async_p ()
   gdb_assert (target_async_permitted);
 
   /* We're async whenever the serial device can.  */
-  struct remote_state *rs = get_remote_state ();
-  return serial_can_async_p (rs->remote_desc);
+  return get_remote_state ()->can_async_p ();
 }
 
 bool
 remote_target::is_async_p ()
 {
   /* We're async whenever the serial device is.  */
-  struct remote_state *rs = get_remote_state ();
-  return serial_is_async_p (rs->remote_desc);
+  return get_remote_state ()->is_async_p ();
 }
 
 /* Pass the SERIAL event on and up to the client.  One day this code
-- 
2.42.0

[PATCH 3/3] gdb: add assertion when marking the remote async flag

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

As reported in bug 30630 [1], we hit a case where the remote target's
async flag is marked while the target is not configured (yet) to work
async.  This should not happen.  It is caught thanks to this assert in
remote_target::wait:

    /* Start by clearing the flag that asks for our wait method to be called,
       we'll mark it again at the end if needed.  If the target is not in
       async mode then the async token should not be marked.  */
    if (target_is_async_p ())
      rs->clear_async_event_handler ();
    else
      gdb_assert (!rs->async_event_handler_marked ());

This is helpful, but I think that we could have caught the problem earlier than
that, at the moment we marked the handler.  Catching problems earlier
makes them easier to debug.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=30630

Change-Id: I7e229c74b04da82bef6a817d5a676be5cf52e833
---
 gdb/remote.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 38d0027dbf9e..7830b5cec33f 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -424,7 +424,10 @@ class remote_state
   }
 
   void mark_async_event_handler ()
-  { ::mark_async_event_handler (m_async_event_handler_token); }
+  {
+    gdb_assert (this->is_async_p ());
+    ::mark_async_event_handler (m_async_event_handler_token);
+  }
 
   void clear_async_event_handler ()
   { ::clear_async_event_handler (m_async_event_handler_token); }
-- 
2.42.0

RE: [PATCH 3/3] gdb: add assertion when marking the remote async flag

[Terekhov, Mikhail]

> -----Original Message-----
> From: Gdb-patches <gdb-patches-
> bounces+mikhail.terekhov=dell.com@sourceware.org> On Behalf Of Simon
> Marchi via Gdb-patches
> Sent: Tuesday, October 3, 2023 10:04 PM
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@efficios.com>
> Subject: [PATCH 3/3] gdb: add assertion when marking the remote async flag
>
>
> [EXTERNAL EMAIL]
>
> From: Simon Marchi <simon.marchi@efficios.com>
>
> As reported in bug 30630 [1], we hit a case where the remote target's async flag
> is marked while the target is not configured (yet) to work async.  This should not
> happen.  It is caught thanks to this assert in
> remote_target::wait:
>
>     /* Start by clearing the flag that asks for our wait method to be called,
>        we'll mark it again at the end if needed.  If the target is not in
>        async mode then the async token should not be marked.  */
>     if (target_is_async_p ())
>       rs->clear_async_event_handler ();
>     else
>       gdb_assert (!rs->async_event_handler_marked ());
>
> This is helpful, but I think that we could have caught the problem earlier than
> that, at the moment we marked the handler.  Catching problems earlier makes
> them easier to debug.
>
> [1]
> https://urldefense.com/v3/__https://sourceware.org/bugzilla/show_bug.cgi?id
> =30630__;!!LpKI!j0fTzubsC8RVdwinGn-r82-
> mX7hLUvNffNCQWwFTCYyR9EWj0CtlY9ERyT_AXgqsNQ0ZjS4Ls53fdBnUrLhWIw
> hIBW9P$ [sourceware[.]org]
>
> Change-Id: I7e229c74b04da82bef6a817d5a676be5cf52e833
> ---
>  gdb/remote.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c index 38d0027dbf9e..7830b5cec33f
> 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -424,7 +424,10 @@ class remote_state
>    }
>
>    void mark_async_event_handler ()
> -  { ::mark_async_event_handler (m_async_event_handler_token); }
> +  {
> +    gdb_assert (this->is_async_p ());
> +    ::mark_async_event_handler (m_async_event_handler_token);  }

This change made the need for fix suggested in [1] more obvious.
The assert in mark_async_event_handler () is stronger than check in
remote_target::queued_stop_reply().
I.e. mark_async_event_handler () should not be called in
remote_target::queued_stop_reply() unless async_handler != NULL i.e.
target_is_async_p() !=0.

I'd suggest to merge in fix from [1] into this series.

>
>    void clear_async_event_handler ()
>    { ::clear_async_event_handler (m_async_event_handler_token); }
> --
> 2.42.0


Internal Use - Confidential

RE: [PATCH 0/3] Add assertion when marking the remote async flag

[Terekhov, Mikhail]

> -----Original Message-----
> From: Gdb-patches <gdb-patches-
> bounces+mikhail.terekhov=dell.com@sourceware.org> On Behalf Of Simon
> Marchi via Gdb-patches
> Sent: Tuesday, October 3, 2023 10:04 PM
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@polymtl.ca>
> Subject: [PATCH 0/3] Add assertion when marking the remote async flag
>
> I had this idea while reviewing this patch [1].  When marking the remote
> async flag, assert that the target is actually in async mode, instead of relying
> on an assert that comes at a later time, in the wait method.

After applying this series GDB fails in my setup in assert in mark_async_event_handler
right after call to it from queued_stop_reply. See my comment to PATCH 3/3.

> The first two patches are small preparatory refactorings, and the third one
> adds the assertion.
>
> [1] https://urldefense.com/v3/__https://inbox.sourceware.org/gdb-
> patches/3d728a6e-1cb0-49c2-a4c8-
> 0a974be39fee@simark.ca/T/*ma1903117423ae09c3574fd45ade2dd4af528063
> 3__;Iw!!LpKI!kBSMETr5QzzmvTDT4A0kxAcpgnOpWd2ZyualTzxDwp42So5Om
> byWvIb9bglZjsO4OCDKV_fIXMUj-
> Yh5Q6PUPj26WV9J$ [inbox[.]sourceware[.]org]
>
> Simon Marchi (3):
>   gdb: make remote_state's async token private
>   gdb: add remote_state::{is_async_p,can_async_p}
>   gdb: add assertion when marking the remote async flag
>
>  gdb/remote.c | 93 ++++++++++++++++++++++++++++++++++----------------
> --
>  1 file changed, 62 insertions(+), 31 deletions(-)
>
>
> base-commit: 1181bcd0d2572aee2c0947040e56bc1f9af634e3
> --
> 2.42.0


Internal Use - Confidential

Re: [PATCH 3/3] gdb: add assertion when marking the remote async flag

[Simon Marchi]

On 10/6/23 17:09, Terekhov, Mikhail wrote:
>> -----Original Message-----
>> From: Gdb-patches <gdb-patches-
>> bounces+mikhail.terekhov=dell.com@sourceware.org> On Behalf Of Simon
>> Marchi via Gdb-patches
>> Sent: Tuesday, October 3, 2023 10:04 PM
>> To: gdb-patches@sourceware.org
>> Cc: Simon Marchi <simon.marchi@efficios.com>
>> Subject: [PATCH 3/3] gdb: add assertion when marking the remote async flag
>>
>>
>> [EXTERNAL EMAIL]
>>
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> As reported in bug 30630 [1], we hit a case where the remote target's async flag
>> is marked while the target is not configured (yet) to work async.  This should not
>> happen.  It is caught thanks to this assert in
>> remote_target::wait:
>>
>>     /* Start by clearing the flag that asks for our wait method to be called,
>>        we'll mark it again at the end if needed.  If the target is not in
>>        async mode then the async token should not be marked.  */
>>     if (target_is_async_p ())
>>       rs->clear_async_event_handler ();
>>     else
>>       gdb_assert (!rs->async_event_handler_marked ());
>>
>> This is helpful, but I think that we could have caught the problem earlier than
>> that, at the moment we marked the handler.  Catching problems earlier makes
>> them easier to debug.
>>
>> [1]
>> https://urldefense.com/v3/__https://sourceware.org/bugzilla/show_bug.cgi?id
>> =30630__;!!LpKI!j0fTzubsC8RVdwinGn-r82-
>> mX7hLUvNffNCQWwFTCYyR9EWj0CtlY9ERyT_AXgqsNQ0ZjS4Ls53fdBnUrLhWIw
>> hIBW9P$ [sourceware[.]org]
>>
>> Change-Id: I7e229c74b04da82bef6a817d5a676be5cf52e833
>> ---
>>  gdb/remote.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c index 38d0027dbf9e..7830b5cec33f
>> 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -424,7 +424,10 @@ class remote_state
>>    }
>>
>>    void mark_async_event_handler ()
>> -  { ::mark_async_event_handler (m_async_event_handler_token); }
>> +  {
>> +    gdb_assert (this->is_async_p ());
>> +    ::mark_async_event_handler (m_async_event_handler_token);  }
> 
> This change made the need for fix suggested in [1] more obvious.
> The assert in mark_async_event_handler () is stronger than check in
> remote_target::queued_stop_reply().
> I.e. mark_async_event_handler () should not be called in
> remote_target::queued_stop_reply() unless async_handler != NULL i.e.
> target_is_async_p() !=0.
> 
> I'd suggest to merge in fix from [1] into this series.

Yes, your fix needs to be merged independently of my series.  My series
is only to add an assertion closer to the root of the problem (which
could help catch other problems in the future).

Simon

Re: [PATCH 1/3] gdb: make remote_state's async token private

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> Make remote_async_inferior_event_token private (rename to
> m_async_event_handler_token) and add methods for the various operations
> we do on it.  This will help by:
>
>  - allowing to break on those methods when debugging
>  - allowing to add assertions in the methods

Looks good.  Couple of minor nits.

>
> Change-Id: Ia3b8a2bc48ad4849dbbe83442c3f83920f03334d
> ---
>  gdb/remote.c | 70 ++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 9bb4f1de5982..2deba06d7d47 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -413,6 +413,31 @@ class remote_state
>    /* Get the remote arch state for GDBARCH.  */
>    struct remote_arch_state *get_remote_arch_state (struct gdbarch *gdbarch);
>  
> +  void create_async_event_handler ()
> +  {

Maybe add:

  gdb_assert (m_async_event_handler_token == nullptr);

If this was ever not true then we'd leak a token, right?

> +    m_async_event_handler_token
> +      = ::create_async_event_handler ([] (gdb_client_data data)
> +      				      {

Spaces before tabs here.

> +					inferior_event_handler (INF_REG_EVENT);
> +				      },
> +				      nullptr, "remote");
> +  }
> +
> +  void mark_async_event_handler ()
> +  { ::mark_async_event_handler (m_async_event_handler_token); }
> +
> +  void clear_async_event_handler ()
> +  { ::clear_async_event_handler (m_async_event_handler_token); }
> +
> +  bool async_event_handler_marked () const
> +  { return ::async_event_handler_marked (m_async_event_handler_token); }
> +
> +  void delete_async_event_handler ()
> +  {
> +    if (m_async_event_handler_token != nullptr)
> +      ::delete_async_event_handler (&m_async_event_handler_token);
> +  }
> +
>  public: /* data */
>  
>    /* A buffer to use for incoming packets, and its current size.  The
> @@ -540,10 +565,6 @@ class remote_state
>       immediately, so queue is not needed for them.  */
>    std::vector<stop_reply_up> stop_reply_queue;
>  
> -  /* Asynchronous signal handle registered as event loop source for
> -     when we have pending events ready to be passed to the core.  */
> -  struct async_event_handler *remote_async_inferior_event_token = nullptr;
> -
>    /* FIXME: cagney/1999-09-23: Even though getpkt was called with
>       ``forever'' still use the normal timeout mechanism.  This is
>       currently used by the ASYNC code to guarentee that target reads
> @@ -554,6 +575,10 @@ class remote_state
>    bool wait_forever_enabled_p = true;
>  
>  private:
> +  /* Asynchronous signal handle registered as event loop source for
> +     when we have pending events ready to be passed to the core.  */
> +  async_event_handler *m_async_event_handler_token = nullptr;
> +  

Trailing white-space here.

Thanks,
Andrew

>    /* Mapping of remote protocol data for each gdbarch.  Usually there
>       is only one entry here, though we may see more with stubs that
>       support multi-process.  */
> @@ -1384,8 +1409,6 @@ static void show_remote_protocol_packet_cmd (struct ui_file *file,
>  
>  static ptid_t read_ptid (const char *buf, const char **obuf);
>  
> -static void remote_async_inferior_event_handler (gdb_client_data);
> -
>  static bool remote_read_description_p (struct target_ops *target);
>  
>  static void remote_console_output (const char *msg);
> @@ -4389,8 +4412,7 @@ remote_target::~remote_target ()
>       everything of this target.  */
>    discard_pending_stop_replies_in_queue ();
>  
> -  if (rs->remote_async_inferior_event_token)
> -    delete_async_event_handler (&rs->remote_async_inferior_event_token);
> +  rs->delete_async_event_handler ();
>  
>    delete rs->notif_state;
>  }
> @@ -5989,9 +6011,8 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
>    current_inferior ()->push_target (std::move (target_holder));
>  
>    /* Register extra event sources in the event loop.  */
> -  rs->remote_async_inferior_event_token
> -    = create_async_event_handler (remote_async_inferior_event_handler, nullptr,
> -				  "remote");
> +  rs->create_async_event_handler ();
> +
>    rs->notif_state = remote_notif_state_allocate (remote);
>  
>    /* Reset the target state; these things will be queried either by
> @@ -7086,7 +7107,7 @@ remote_target::has_pending_events ()
>      {
>        remote_state *rs = get_remote_state ();
>  
> -      if (async_event_handler_marked (rs->remote_async_inferior_event_token))
> +      if (rs->async_event_handler_marked ())
>  	return true;
>  
>        /* Note that BUFCNT can be negative, indicating sticky
> @@ -7420,7 +7441,7 @@ remote_notif_stop_can_get_pending_events (remote_target *remote,
>       may exit and we have no chance to process them back in
>       remote_wait_ns.  */
>    remote_state *rs = remote->get_remote_state ();
> -  mark_async_event_handler (rs->remote_async_inferior_event_token);
> +  rs->mark_async_event_handler ();
>    return 0;
>  }
>  
> @@ -7628,7 +7649,7 @@ remote_target::queued_stop_reply (ptid_t ptid)
>    if (!rs->stop_reply_queue.empty () && target_can_async_p ())
>      {
>        /* There's still at least an event left.  */
> -      mark_async_event_handler (rs->remote_async_inferior_event_token);
> +      rs->mark_async_event_handler ();
>      }
>  
>    return r;
> @@ -7655,7 +7676,7 @@ remote_target::push_stop_reply (struct stop_reply *new_event)
>       enabled, and there are events in this queue, we will mark the event
>       token at that point, see remote_target::async.  */
>    if (target_is_async_p ())
> -    mark_async_event_handler (rs->remote_async_inferior_event_token);
> +    rs->mark_async_event_handler ();
>  }
>  
>  /* Returns true if we have a stop reply for PTID.  */
> @@ -8515,10 +8536,9 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
>       we'll mark it again at the end if needed.  If the target is not in
>       async mode then the async token should not be marked.  */
>    if (target_is_async_p ())
> -    clear_async_event_handler (rs->remote_async_inferior_event_token);
> +    rs->clear_async_event_handler ();
>    else
> -    gdb_assert (!async_event_handler_marked
> -		(rs->remote_async_inferior_event_token));
> +    gdb_assert (!rs->async_event_handler_marked ());
>  
>    ptid_t event_ptid;
>  
> @@ -8533,7 +8553,7 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
>  	 notifications, then tell the event loop to call us again.  */
>        if (!rs->stop_reply_queue.empty ()
>  	  || rs->notif_state->pending_event[notif_client_stop.id] != nullptr)
> -	mark_async_event_handler (rs->remote_async_inferior_event_token);
> +	rs->mark_async_event_handler ();
>      }
>  
>    return event_ptid;
> @@ -14859,12 +14879,6 @@ remote_async_serial_handler (struct serial *scb, void *context)
>    inferior_event_handler (INF_REG_EVENT);
>  }
>  
> -static void
> -remote_async_inferior_event_handler (gdb_client_data data)
> -{
> -  inferior_event_handler (INF_REG_EVENT);
> -}
> -
>  int
>  remote_target::async_wait_fd ()
>  {
> @@ -14884,7 +14898,8 @@ remote_target::async (bool enable)
>        /* If there are pending events in the stop reply queue tell the
>  	 event loop to process them.  */
>        if (!rs->stop_reply_queue.empty ())
> -	mark_async_event_handler (rs->remote_async_inferior_event_token);
> +	rs->mark_async_event_handler ();
> +
>        /* For simplicity, below we clear the pending events token
>  	 without remembering whether it is marked, so here we always
>  	 mark it.  If there's actually no pending notification to
> @@ -14899,7 +14914,8 @@ remote_target::async (bool enable)
>        /* If the core is disabling async, it doesn't want to be
>  	 disturbed with target events.  Clear all async event sources
>  	 too.  */
> -      clear_async_event_handler (rs->remote_async_inferior_event_token);
> +      rs->clear_async_event_handler ();
> +
>        if (target_is_non_stop_p ())
>  	clear_async_event_handler (rs->notif_state->get_pending_events_token);
>      }
> -- 
> 2.42.0

Re: [PATCH 2/3] gdb: add remote_state::{is_async_p,can_async_p}

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> A subsequent patch will want to know if the remote is async within a
> remote_state method.  Add a helper method for that, and for "can async"
> as well, for symmetry.
>
> Change-Id: Id0f648ee4896736479fa942f5453eeeb0e5d4352
> ---
>  gdb/remote.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 2deba06d7d47..38d0027dbf9e 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -438,6 +438,20 @@ class remote_state
>        ::delete_async_event_handler (&m_async_event_handler_token);
>    }
>  
> +  bool is_async_p () const
> +  {
> +    /* We're async whenever the serial device is.  */
> +    return (this->remote_desc
> +	    != nullptr && serial_is_async_p (this->remote_desc));

This seems like an off choice for line wrapping.  I'd expect the newline
to start with the '&&'.

But... do we ever expect to ask is_async_p() before the remote_desc has
been initialised?  Does the answer even make sense in such a context?

I wonder if we'd be better doing:

  bool is_async_p () const
  {
    gdb_assert (this->remote_desc != nullptr);
    return serial_is_async_p (this->remote_desc);
  }

I'm thinking, that if we did ask this question before remote_desc was
initialised, then when remote_desc was initialised the answer would
change, and so we're probably asking to question too early...

> +  }
> +
> +  bool can_async_p () const
> +  {
> +    /* We can async whenever the serial device can.  */
> +    return (this->remote_desc
> +	    != nullptr && serial_can_async_p (this->remote_desc));

Same here.

Thanks,
Andrew

> +  }
> +  
>  public: /* data */
>  
>    /* A buffer to use for incoming packets, and its current size.  The
> @@ -14853,16 +14867,14 @@ remote_target::can_async_p ()
>    gdb_assert (target_async_permitted);
>  
>    /* We're async whenever the serial device can.  */
> -  struct remote_state *rs = get_remote_state ();
> -  return serial_can_async_p (rs->remote_desc);
> +  return get_remote_state ()->can_async_p ();
>  }
>  
>  bool
>  remote_target::is_async_p ()
>  {
>    /* We're async whenever the serial device is.  */
> -  struct remote_state *rs = get_remote_state ();
> -  return serial_is_async_p (rs->remote_desc);
> +  return get_remote_state ()->is_async_p ();
>  }
>  
>  /* Pass the SERIAL event on and up to the client.  One day this code
> -- 
> 2.42.0

Re: [PATCH 3/3] gdb: add assertion when marking the remote async flag

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> As reported in bug 30630 [1], we hit a case where the remote target's
> async flag is marked while the target is not configured (yet) to work
> async.  This should not happen.  It is caught thanks to this assert in
> remote_target::wait:
>
>     /* Start by clearing the flag that asks for our wait method to be called,
>        we'll mark it again at the end if needed.  If the target is not in
>        async mode then the async token should not be marked.  */
>     if (target_is_async_p ())
>       rs->clear_async_event_handler ();
>     else
>       gdb_assert (!rs->async_event_handler_marked ());
>
> This is helpful, but I think that we could have caught the problem earlier than
> that, at the moment we marked the handler.  Catching problems earlier
> makes them easier to debug.

Agreed.  Looked through this series, all looks good.  I had a few nits
that I reported, but otherwise:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

>
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=30630
>
> Change-Id: I7e229c74b04da82bef6a817d5a676be5cf52e833
> ---
>  gdb/remote.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 38d0027dbf9e..7830b5cec33f 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -424,7 +424,10 @@ class remote_state
>    }
>  
>    void mark_async_event_handler ()
> -  { ::mark_async_event_handler (m_async_event_handler_token); }
> +  {
> +    gdb_assert (this->is_async_p ());
> +    ::mark_async_event_handler (m_async_event_handler_token);
> +  }
>  
>    void clear_async_event_handler ()
>    { ::clear_async_event_handler (m_async_event_handler_token); }
> -- 
> 2.42.0

Re: [PATCH 1/3] gdb: make remote_state's async token private

[Simon Marchi]

On 10/9/23 05:11, Andrew Burgess wrote:
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> Make remote_async_inferior_event_token private (rename to
>> m_async_event_handler_token) and add methods for the various operations
>> we do on it.  This will help by:
>>
>>  - allowing to break on those methods when debugging
>>  - allowing to add assertions in the methods
> 
> Looks good.  Couple of minor nits.
> 
>>
>> Change-Id: Ia3b8a2bc48ad4849dbbe83442c3f83920f03334d
>> ---
>>  gdb/remote.c | 70 ++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 43 insertions(+), 27 deletions(-)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 9bb4f1de5982..2deba06d7d47 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -413,6 +413,31 @@ class remote_state
>>    /* Get the remote arch state for GDBARCH.  */
>>    struct remote_arch_state *get_remote_arch_state (struct gdbarch *gdbarch);
>>  
>> +  void create_async_event_handler ()
>> +  {
> 
> Maybe add:
> 
>   gdb_assert (m_async_event_handler_token == nullptr);
> 
> If this was ever not true then we'd leak a token, right?

Makes sense, added.

>> +    m_async_event_handler_token
>> +      = ::create_async_event_handler ([] (gdb_client_data data)
>> +      				      {
> 
> Spaces before tabs here.

Fixed.

>>  private:
>> +  /* Asynchronous signal handle registered as event loop source for
>> +     when we have pending events ready to be passed to the core.  */
>> +  async_event_handler *m_async_event_handler_token = nullptr;
>> +  
> 
> Trailing white-space here.

Fixed, thanks.

Simon

Re: [PATCH 2/3] gdb: add remote_state::{is_async_p,can_async_p}

[Simon Marchi]

On 10/9/23 05:20, Andrew Burgess wrote:
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> A subsequent patch will want to know if the remote is async within a
>> remote_state method.  Add a helper method for that, and for "can async"
>> as well, for symmetry.
>>
>> Change-Id: Id0f648ee4896736479fa942f5453eeeb0e5d4352
>> ---
>>  gdb/remote.c | 20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 2deba06d7d47..38d0027dbf9e 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -438,6 +438,20 @@ class remote_state
>>        ::delete_async_event_handler (&m_async_event_handler_token);
>>    }
>>  
>> +  bool is_async_p () const
>> +  {
>> +    /* We're async whenever the serial device is.  */
>> +    return (this->remote_desc
>> +	    != nullptr && serial_is_async_p (this->remote_desc));
> 
> This seems like an off choice for line wrapping.  I'd expect the newline
> to start with the '&&'.

Oops, fixed.

> But... do we ever expect to ask is_async_p() before the remote_desc has
> been initialised?  Does the answer even make sense in such a context?
> 
> I wonder if we'd be better doing:
> 
>   bool is_async_p () const
>   {
>     gdb_assert (this->remote_desc != nullptr);
>     return serial_is_async_p (this->remote_desc);
>   }

That's probably right, I'll try to make that change and re-run the
tests.

> I'm thinking, that if we did ask this question before remote_desc was
> initialised, then when remote_desc was initialised the answer would
> change, and so we're probably asking to question too early...

Yeah... I think for "is_async_p" it would still be a correct answer, the
target isn't in async mode at that moment (before remote_desc is set).
But "can_async_p" could change, once we initialize remote_desc.  Anyhow,
I've put asserts as you suggested.

> 
>> +  }
>> +
>> +  bool can_async_p () const
>> +  {
>> +    /* We can async whenever the serial device can.  */
>> +    return (this->remote_desc
>> +	    != nullptr && serial_can_async_p (this->remote_desc));
> 
> Same here.

Fixed.

Simon

Re: [PATCH 3/3] gdb: add assertion when marking the remote async flag

[Simon Marchi]

On 10/9/23 05:25, Andrew Burgess wrote:
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> As reported in bug 30630 [1], we hit a case where the remote target's
>> async flag is marked while the target is not configured (yet) to work
>> async.  This should not happen.  It is caught thanks to this assert in
>> remote_target::wait:
>>
>>     /* Start by clearing the flag that asks for our wait method to be called,
>>        we'll mark it again at the end if needed.  If the target is not in
>>        async mode then the async token should not be marked.  */
>>     if (target_is_async_p ())
>>       rs->clear_async_event_handler ();
>>     else
>>       gdb_assert (!rs->async_event_handler_marked ());
>>
>> This is helpful, but I think that we could have caught the problem earlier than
>> that, at the moment we marked the handler.  Catching problems earlier
>> makes them easier to debug.
> 
> Agreed.  Looked through this series, all looks good.  I had a few nits
> that I reported, but otherwise:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks, I'll push if my CI run is clean with the changes applied.

Simon