* [PATCH][gdb] Fix assert in remote_async_get_pending_events_handler
@ 2021-04-22 8:51 Tom de Vries
2021-04-22 10:19 ` Andrew Burgess
0 siblings, 1 reply; 5+ messages in thread
From: Tom de Vries @ 2021-04-22 8:51 UTC (permalink / raw)
To: gdb-patches
Hi,
Occassionally I run into the following assert:
...
(gdb) PASS: gdb.multi/multi-target-continue.exp: inferior 5
Remote debugging from host ::1, port 49990^M
Process multi-target-continue created; pid = 31241^M
src/gdb/remote-notif.c:113: internal-error: \
void remote_async_get_pending_events_handler(gdb_client_data): \
Assertion `target_is_non_stop_p ()' failed.^M
...
The assert checks target_is_non_stop_p, which is related to the current
target.
Fix this by changing the assert such that it checks non-stopness related to
the event it's handling.
Tested on x86_64-linux.
Any comments?
Thanks,
- Tom
[gdb] Fix assert in remote_async_get_pending_events_handler
2021-04-16 Simon Marchi <simon.marchi@polymtl.ca>
Tom de Vries <tdevries@suse.de>
PR remote/27710
* remote.c (remote_target_is_non_stop_p): New function.
* remote.h (remote_target_is_non_stop_p): Declare.
* remote-notif.c (remote_async_get_pending_events_handler): Fix assert
to check non-stopness using notif_state->remote rather current target.
---
gdb/remote-notif.c | 2 +-
gdb/remote.c | 11 +++++++++++
gdb/remote.h | 1 +
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index 5a3e1395b76..4245015ae37 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -110,7 +110,7 @@ remote_async_get_pending_events_handler (gdb_client_data data)
{
remote_notif_state *notif_state = (remote_notif_state *) data;
clear_async_event_handler (notif_state->get_pending_events_token);
- gdb_assert (target_is_non_stop_p ());
+ gdb_assert (remote_target_is_non_stop_p (notif_state->remote));
remote_notif_process (notif_state, NULL);
}
diff --git a/gdb/remote.c b/gdb/remote.c
index 7429e1a86b3..2e365df9bba 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14730,6 +14730,17 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
return packet_check_result (rs->buf.data ()) == PACKET_OK;
}
+/* Return true if remote target T is non-stop. */
+
+bool
+remote_target_is_non_stop_p (remote_target *t)
+{
+ scoped_restore_current_thread restore_thread;
+ switch_to_target_no_thread (t);
+
+ return target_is_non_stop_p ();
+}
+
#if GDB_SELF_TEST
namespace selftests {
diff --git a/gdb/remote.h b/gdb/remote.h
index 18352ddb866..46bfa01fc79 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -77,4 +77,5 @@ extern int remote_register_number_and_offset (struct gdbarch *gdbarch,
extern void remote_notif_get_pending_events (remote_target *remote,
struct notif_client *np);
+extern bool remote_target_is_non_stop_p (remote_target *t);
#endif
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][gdb] Fix assert in remote_async_get_pending_events_handler
2021-04-22 8:51 [PATCH][gdb] Fix assert in remote_async_get_pending_events_handler Tom de Vries
@ 2021-04-22 10:19 ` Andrew Burgess
2021-04-22 11:03 ` Tom de Vries
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Burgess @ 2021-04-22 10:19 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches
* Tom de Vries <tdevries@suse.de> [2021-04-22 10:51:29 +0200]:
> Hi,
>
> Occassionally I run into the following assert:
> ...
> (gdb) PASS: gdb.multi/multi-target-continue.exp: inferior 5
> Remote debugging from host ::1, port 49990^M
> Process multi-target-continue created; pid = 31241^M
> src/gdb/remote-notif.c:113: internal-error: \
> void remote_async_get_pending_events_handler(gdb_client_data): \
> Assertion `target_is_non_stop_p ()' failed.^M
> ...
>
> The assert checks target_is_non_stop_p, which is related to the current
> target.
>
> Fix this by changing the assert such that it checks non-stopness related to
> the event it's handling.
>
> Tested on x86_64-linux.
>
> Any comments?
This seems fine to me. I wonder though if you considered converting
target_is_non_stop_p into a member function on target_ops? If we did
then we would avoid having to switch targets just to ask this
question. All of the helper functions that target_is_non_stop_p calls
are already available as member functions so there would be no
additional changes needed I think.
Just a thought.
Thanks,
Andrew
>
> Thanks,
> - Tom
>
> [gdb] Fix assert in remote_async_get_pending_events_handler
>
> 2021-04-16 Simon Marchi <simon.marchi@polymtl.ca>
> Tom de Vries <tdevries@suse.de>
>
> PR remote/27710
> * remote.c (remote_target_is_non_stop_p): New function.
> * remote.h (remote_target_is_non_stop_p): Declare.
> * remote-notif.c (remote_async_get_pending_events_handler): Fix assert
> to check non-stopness using notif_state->remote rather current target.
>
> ---
> gdb/remote-notif.c | 2 +-
> gdb/remote.c | 11 +++++++++++
> gdb/remote.h | 1 +
> 3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
> index 5a3e1395b76..4245015ae37 100644
> --- a/gdb/remote-notif.c
> +++ b/gdb/remote-notif.c
> @@ -110,7 +110,7 @@ remote_async_get_pending_events_handler (gdb_client_data data)
> {
> remote_notif_state *notif_state = (remote_notif_state *) data;
> clear_async_event_handler (notif_state->get_pending_events_token);
> - gdb_assert (target_is_non_stop_p ());
> + gdb_assert (remote_target_is_non_stop_p (notif_state->remote));
> remote_notif_process (notif_state, NULL);
> }
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 7429e1a86b3..2e365df9bba 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -14730,6 +14730,17 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
> return packet_check_result (rs->buf.data ()) == PACKET_OK;
> }
>
> +/* Return true if remote target T is non-stop. */
> +
> +bool
> +remote_target_is_non_stop_p (remote_target *t)
> +{
> + scoped_restore_current_thread restore_thread;
> + switch_to_target_no_thread (t);
> +
> + return target_is_non_stop_p ();
> +}
> +
> #if GDB_SELF_TEST
>
> namespace selftests {
> diff --git a/gdb/remote.h b/gdb/remote.h
> index 18352ddb866..46bfa01fc79 100644
> --- a/gdb/remote.h
> +++ b/gdb/remote.h
> @@ -77,4 +77,5 @@ extern int remote_register_number_and_offset (struct gdbarch *gdbarch,
>
> extern void remote_notif_get_pending_events (remote_target *remote,
> struct notif_client *np);
> +extern bool remote_target_is_non_stop_p (remote_target *t);
> #endif
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][gdb] Fix assert in remote_async_get_pending_events_handler
2021-04-22 10:19 ` Andrew Burgess
@ 2021-04-22 11:03 ` Tom de Vries
2021-04-22 13:33 ` Simon Marchi
0 siblings, 1 reply; 5+ messages in thread
From: Tom de Vries @ 2021-04-22 11:03 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, Simon Marchi
On 4/22/21 12:19 PM, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2021-04-22 10:51:29 +0200]:
>
>> Hi,
>>
>> Occassionally I run into the following assert:
>> ...
>> (gdb) PASS: gdb.multi/multi-target-continue.exp: inferior 5
>> Remote debugging from host ::1, port 49990^M
>> Process multi-target-continue created; pid = 31241^M
>> src/gdb/remote-notif.c:113: internal-error: \
>> void remote_async_get_pending_events_handler(gdb_client_data): \
>> Assertion `target_is_non_stop_p ()' failed.^M
>> ...
>>
>> The assert checks target_is_non_stop_p, which is related to the current
>> target.
>>
>> Fix this by changing the assert such that it checks non-stopness related to
>> the event it's handling.
>>
>> Tested on x86_64-linux.
>>
>> Any comments?
>
> This seems fine to me. I wonder though if you considered converting
> target_is_non_stop_p into a member function on target_ops?
> If we did
> then we would avoid having to switch targets just to ask this
> question. All of the helper functions that target_is_non_stop_p calls
> are already available as member functions so there would be no
> additional changes needed I think.
>
Um, I'm the one who ran into the problem, Simon is the one who came up
with the fix, so I guess this is a question for him. I'm afraid I'm not
familiar with this code at all.
Thanks,
- Tom
> Just a thought.
>
> Thanks,
> Andrew
>
>>
>> Thanks,
>> - Tom
>>
>> [gdb] Fix assert in remote_async_get_pending_events_handler
>>
>> 2021-04-16 Simon Marchi <simon.marchi@polymtl.ca>
>> Tom de Vries <tdevries@suse.de>
>>
>> PR remote/27710
>> * remote.c (remote_target_is_non_stop_p): New function.
>> * remote.h (remote_target_is_non_stop_p): Declare.
>> * remote-notif.c (remote_async_get_pending_events_handler): Fix assert
>> to check non-stopness using notif_state->remote rather current target.
>>
>> ---
>> gdb/remote-notif.c | 2 +-
>> gdb/remote.c | 11 +++++++++++
>> gdb/remote.h | 1 +
>> 3 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
>> index 5a3e1395b76..4245015ae37 100644
>> --- a/gdb/remote-notif.c
>> +++ b/gdb/remote-notif.c
>> @@ -110,7 +110,7 @@ remote_async_get_pending_events_handler (gdb_client_data data)
>> {
>> remote_notif_state *notif_state = (remote_notif_state *) data;
>> clear_async_event_handler (notif_state->get_pending_events_token);
>> - gdb_assert (target_is_non_stop_p ());
>> + gdb_assert (remote_target_is_non_stop_p (notif_state->remote));
>> remote_notif_process (notif_state, NULL);
>> }
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 7429e1a86b3..2e365df9bba 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -14730,6 +14730,17 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>> return packet_check_result (rs->buf.data ()) == PACKET_OK;
>> }
>>
>> +/* Return true if remote target T is non-stop. */
>> +
>> +bool
>> +remote_target_is_non_stop_p (remote_target *t)
>> +{
>> + scoped_restore_current_thread restore_thread;
>> + switch_to_target_no_thread (t);
>> +
>> + return target_is_non_stop_p ();
>> +}
>> +
>> #if GDB_SELF_TEST
>>
>> namespace selftests {
>> diff --git a/gdb/remote.h b/gdb/remote.h
>> index 18352ddb866..46bfa01fc79 100644
>> --- a/gdb/remote.h
>> +++ b/gdb/remote.h
>> @@ -77,4 +77,5 @@ extern int remote_register_number_and_offset (struct gdbarch *gdbarch,
>>
>> extern void remote_notif_get_pending_events (remote_target *remote,
>> struct notif_client *np);
>> +extern bool remote_target_is_non_stop_p (remote_target *t);
>> #endif
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][gdb] Fix assert in remote_async_get_pending_events_handler
2021-04-22 11:03 ` Tom de Vries
@ 2021-04-22 13:33 ` Simon Marchi
2021-04-22 15:17 ` Andrew Burgess
0 siblings, 1 reply; 5+ messages in thread
From: Simon Marchi @ 2021-04-22 13:33 UTC (permalink / raw)
To: Tom de Vries, Andrew Burgess; +Cc: gdb-patches
On 2021-04-22 7:03 a.m., Tom de Vries wrote:
> On 4/22/21 12:19 PM, Andrew Burgess wrote:
>> * Tom de Vries <tdevries@suse.de> [2021-04-22 10:51:29 +0200]:
>>
>>> Hi,
>>>
>>> Occassionally I run into the following assert:
>>> ...
>>> (gdb) PASS: gdb.multi/multi-target-continue.exp: inferior 5
>>> Remote debugging from host ::1, port 49990^M
>>> Process multi-target-continue created; pid = 31241^M
>>> src/gdb/remote-notif.c:113: internal-error: \
>>> void remote_async_get_pending_events_handler(gdb_client_data): \
>>> Assertion `target_is_non_stop_p ()' failed.^M
>>> ...
>>>
>>> The assert checks target_is_non_stop_p, which is related to the current
>>> target.
>>>
>>> Fix this by changing the assert such that it checks non-stopness related to
>>> the event it's handling.
>>>
>>> Tested on x86_64-linux.
>>>
>>> Any comments?
>>
>> This seems fine to me. I wonder though if you considered converting
>> target_is_non_stop_p into a member function on target_ops?
>> If we did
>> then we would avoid having to switch targets just to ask this
>> question. All of the helper functions that target_is_non_stop_p calls
>> are already available as member functions so there would be no
>> additional changes needed I think.
>>
>
> Um, I'm the one who ran into the problem, Simon is the one who came up
> with the fix, so I guess this is a question for him. I'm afraid I'm not
> familiar with this code at all.
This was proposed in the bug as well:
https://sourceware.org/bugzilla/show_bug.cgi?id=27710#c16
I am not against it, but I think Tom's patch is OK, given it follows
current practices.
Simon
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][gdb] Fix assert in remote_async_get_pending_events_handler
2021-04-22 13:33 ` Simon Marchi
@ 2021-04-22 15:17 ` Andrew Burgess
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Burgess @ 2021-04-22 15:17 UTC (permalink / raw)
To: Simon Marchi; +Cc: Tom de Vries, gdb-patches
* Simon Marchi <simon.marchi@polymtl.ca> [2021-04-22 09:33:28 -0400]:
> On 2021-04-22 7:03 a.m., Tom de Vries wrote:
> > On 4/22/21 12:19 PM, Andrew Burgess wrote:
> >> * Tom de Vries <tdevries@suse.de> [2021-04-22 10:51:29 +0200]:
> >>
> >>> Hi,
> >>>
> >>> Occassionally I run into the following assert:
> >>> ...
> >>> (gdb) PASS: gdb.multi/multi-target-continue.exp: inferior 5
> >>> Remote debugging from host ::1, port 49990^M
> >>> Process multi-target-continue created; pid = 31241^M
> >>> src/gdb/remote-notif.c:113: internal-error: \
> >>> void remote_async_get_pending_events_handler(gdb_client_data): \
> >>> Assertion `target_is_non_stop_p ()' failed.^M
> >>> ...
> >>>
> >>> The assert checks target_is_non_stop_p, which is related to the current
> >>> target.
> >>>
> >>> Fix this by changing the assert such that it checks non-stopness related to
> >>> the event it's handling.
> >>>
> >>> Tested on x86_64-linux.
> >>>
> >>> Any comments?
> >>
> >> This seems fine to me. I wonder though if you considered converting
> >> target_is_non_stop_p into a member function on target_ops?
> >> If we did
> >> then we would avoid having to switch targets just to ask this
> >> question. All of the helper functions that target_is_non_stop_p calls
> >> are already available as member functions so there would be no
> >> additional changes needed I think.
> >>
> >
> > Um, I'm the one who ran into the problem, Simon is the one who came up
> > with the fix, so I guess this is a question for him. I'm afraid I'm not
> > familiar with this code at all.
>
> This was proposed in the bug as well:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=27710#c16
>
> I am not against it, but I think Tom's patch is OK, given it follows
> current practices.
I agree, I'm certainly not blocking the patch.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-04-22 15:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-22 8:51 [PATCH][gdb] Fix assert in remote_async_get_pending_events_handler Tom de Vries
2021-04-22 10:19 ` Andrew Burgess
2021-04-22 11:03 ` Tom de Vries
2021-04-22 13:33 ` Simon Marchi
2021-04-22 15:17 ` Andrew Burgess
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).