* [PATCH 0/3] Add assertion when marking the remote async flag @ 2023-10-04 2:03 Simon Marchi 2023-10-04 2:03 ` [PATCH 1/3] gdb: make remote_state's async token private Simon Marchi ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Simon Marchi @ 2023-10-04 2:03 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] gdb: make remote_state's async token private 2023-10-04 2:03 [PATCH 0/3] Add assertion when marking the remote async flag Simon Marchi @ 2023-10-04 2:03 ` Simon Marchi 2023-10-09 9:11 ` Andrew Burgess 2023-10-04 2:04 ` [PATCH 2/3] gdb: add remote_state::{is_async_p,can_async_p} Simon Marchi ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Simon Marchi @ 2023-10-04 2:03 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] gdb: make remote_state's async token private 2023-10-04 2:03 ` [PATCH 1/3] gdb: make remote_state's async token private Simon Marchi @ 2023-10-09 9:11 ` Andrew Burgess 2023-10-10 14:56 ` Simon Marchi 0 siblings, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2023-10-09 9:11 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: Simon Marchi 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] gdb: make remote_state's async token private 2023-10-09 9:11 ` Andrew Burgess @ 2023-10-10 14:56 ` Simon Marchi 0 siblings, 0 replies; 13+ messages in thread From: Simon Marchi @ 2023-10-10 14:56 UTC (permalink / raw) To: Andrew Burgess, Simon Marchi, gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] gdb: add remote_state::{is_async_p,can_async_p} 2023-10-04 2:03 [PATCH 0/3] Add assertion when marking the remote async flag Simon Marchi 2023-10-04 2:03 ` [PATCH 1/3] gdb: make remote_state's async token private Simon Marchi @ 2023-10-04 2:04 ` Simon Marchi 2023-10-09 9:20 ` Andrew Burgess 2023-10-04 2:04 ` [PATCH 3/3] gdb: add assertion when marking the remote async flag Simon Marchi 2023-10-06 21:28 ` [PATCH 0/3] Add " Terekhov, Mikhail 3 siblings, 1 reply; 13+ messages in thread From: Simon Marchi @ 2023-10-04 2:04 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] gdb: add remote_state::{is_async_p,can_async_p} 2023-10-04 2:04 ` [PATCH 2/3] gdb: add remote_state::{is_async_p,can_async_p} Simon Marchi @ 2023-10-09 9:20 ` Andrew Burgess 2023-10-10 15:01 ` Simon Marchi 0 siblings, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2023-10-09 9:20 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: Simon Marchi 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] gdb: add remote_state::{is_async_p,can_async_p} 2023-10-09 9:20 ` Andrew Burgess @ 2023-10-10 15:01 ` Simon Marchi 0 siblings, 0 replies; 13+ messages in thread From: Simon Marchi @ 2023-10-10 15:01 UTC (permalink / raw) To: Andrew Burgess, Simon Marchi, gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] gdb: add assertion when marking the remote async flag 2023-10-04 2:03 [PATCH 0/3] Add assertion when marking the remote async flag Simon Marchi 2023-10-04 2:03 ` [PATCH 1/3] gdb: make remote_state's async token private Simon Marchi 2023-10-04 2:04 ` [PATCH 2/3] gdb: add remote_state::{is_async_p,can_async_p} Simon Marchi @ 2023-10-04 2:04 ` Simon Marchi 2023-10-06 21:09 ` Terekhov, Mikhail 2023-10-09 9:25 ` Andrew Burgess 2023-10-06 21:28 ` [PATCH 0/3] Add " Terekhov, Mikhail 3 siblings, 2 replies; 13+ messages in thread From: Simon Marchi @ 2023-10-04 2:04 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 3/3] gdb: add assertion when marking the remote async flag 2023-10-04 2:04 ` [PATCH 3/3] gdb: add assertion when marking the remote async flag Simon Marchi @ 2023-10-06 21:09 ` Terekhov, Mikhail 2023-10-07 1:35 ` Simon Marchi 2023-10-09 9:25 ` Andrew Burgess 1 sibling, 1 reply; 13+ messages in thread From: Terekhov, Mikhail @ 2023-10-06 21:09 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: Simon Marchi > -----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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] gdb: add assertion when marking the remote async flag 2023-10-06 21:09 ` Terekhov, Mikhail @ 2023-10-07 1:35 ` Simon Marchi 0 siblings, 0 replies; 13+ messages in thread From: Simon Marchi @ 2023-10-07 1:35 UTC (permalink / raw) To: Terekhov, Mikhail, gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] gdb: add assertion when marking the remote async flag 2023-10-04 2:04 ` [PATCH 3/3] gdb: add assertion when marking the remote async flag Simon Marchi 2023-10-06 21:09 ` Terekhov, Mikhail @ 2023-10-09 9:25 ` Andrew Burgess 2023-10-10 15:01 ` Simon Marchi 1 sibling, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2023-10-09 9:25 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: Simon Marchi 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] gdb: add assertion when marking the remote async flag 2023-10-09 9:25 ` Andrew Burgess @ 2023-10-10 15:01 ` Simon Marchi 0 siblings, 0 replies; 13+ messages in thread From: Simon Marchi @ 2023-10-10 15:01 UTC (permalink / raw) To: Andrew Burgess, Simon Marchi, gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 0/3] Add assertion when marking the remote async flag 2023-10-04 2:03 [PATCH 0/3] Add assertion when marking the remote async flag Simon Marchi ` (2 preceding siblings ...) 2023-10-04 2:04 ` [PATCH 3/3] gdb: add assertion when marking the remote async flag Simon Marchi @ 2023-10-06 21:28 ` Terekhov, Mikhail 3 siblings, 0 replies; 13+ messages in thread From: Terekhov, Mikhail @ 2023-10-06 21:28 UTC (permalink / raw) To: Simon Marchi, gdb-patches > -----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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-10-10 15:01 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-10-04 2:03 [PATCH 0/3] Add assertion when marking the remote async flag Simon Marchi 2023-10-04 2:03 ` [PATCH 1/3] gdb: make remote_state's async token private Simon Marchi 2023-10-09 9:11 ` Andrew Burgess 2023-10-10 14:56 ` Simon Marchi 2023-10-04 2:04 ` [PATCH 2/3] gdb: add remote_state::{is_async_p,can_async_p} Simon Marchi 2023-10-09 9:20 ` Andrew Burgess 2023-10-10 15:01 ` Simon Marchi 2023-10-04 2:04 ` [PATCH 3/3] gdb: add assertion when marking the remote async flag Simon Marchi 2023-10-06 21:09 ` Terekhov, Mikhail 2023-10-07 1:35 ` Simon Marchi 2023-10-09 9:25 ` Andrew Burgess 2023-10-10 15:01 ` Simon Marchi 2023-10-06 21:28 ` [PATCH 0/3] Add " Terekhov, Mikhail
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).