* [PATCH] Conditionally restore displaced stepping state after fork.
@ 2021-05-31 16:15 John Baldwin
2021-05-31 16:32 ` John Baldwin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: John Baldwin @ 2021-05-31 16:15 UTC (permalink / raw)
To: gdb-patches
There is no default method for
gdbarch_displaced_step_restore_all_in_ptid, so calling it
unconditionally for fork events triggered an assertion failure on
platforms that do not support displaced stepping. To fix, only invoke
the method if the gdbarch supports displaced stepping.
gdb/ChangeLog:
* infrun.c (handle_inferior_event): Only call
gdbarch_displaced_step_restore_all_in_ptid if
gdbarch_supports_displaced_stepping is true.
---
gdb/ChangeLog | 6 ++++++
gdb/infrun.c | 3 ++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 03910c0634..b0f448a35e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2021-05-30 John Baldwin <jhb@FreeBSD.org>
+
+ * infrun.c (handle_inferior_event): Only call
+ gdbarch_displaced_step_restore_all_in_ptid if
+ gdbarch_supports_displaced_stepping is true.
+
2021-05-27 Simon Marchi <simon.marchi@polymtl.ca>
* Fix tab after space indentation issues throughout.
diff --git a/gdb/infrun.c b/gdb/infrun.c
index e9624d2a9b..6fd077796f 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5496,7 +5496,8 @@ handle_inferior_event (struct execution_control_state *ecs)
/* If this is a fork (child gets its own address space copy) and some
displaced step buffers were in use at the time of the fork, restore
the displaced step buffer bytes in the child process. */
- if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
+ if (ecs->ws.kind == TARGET_WAITKIND_FORKED
+ && gdbarch_supports_displaced_stepping (gdbarch))
gdbarch_displaced_step_restore_all_in_ptid
(gdbarch, parent_inf, ecs->ws.value.related_pid);
--
2.31.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Conditionally restore displaced stepping state after fork.
2021-05-31 16:15 [PATCH] Conditionally restore displaced stepping state after fork John Baldwin
@ 2021-05-31 16:32 ` John Baldwin
2021-06-01 0:59 ` Simon Marchi
2021-06-01 21:11 ` [PATCH v2] " John Baldwin
2 siblings, 0 replies; 6+ messages in thread
From: John Baldwin @ 2021-05-31 16:32 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
On 5/31/21 9:15 AM, John Baldwin wrote:
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index e9624d2a9b..6fd077796f 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -5496,7 +5496,8 @@ handle_inferior_event (struct execution_control_state *ecs)
> /* If this is a fork (child gets its own address space copy) and some
> displaced step buffers were in use at the time of the fork, restore
> the displaced step buffer bytes in the child process. */
> - if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
> + if (ecs->ws.kind == TARGET_WAITKIND_FORKED
> + && gdbarch_supports_displaced_stepping (gdbarch))
> gdbarch_displaced_step_restore_all_in_ptid
> (gdbarch, parent_inf, ecs->ws.value.related_pid);
This appears to be a regression from 187b041e2514827b9d86190ed2471c4c7a352874
where previously stepping was only cleared from a single thread if it had
a stepping buffer (which implicitly was only true if the gdbarch supported
displaced stepping).
--
John Baldwin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Conditionally restore displaced stepping state after fork.
2021-05-31 16:15 [PATCH] Conditionally restore displaced stepping state after fork John Baldwin
2021-05-31 16:32 ` John Baldwin
@ 2021-06-01 0:59 ` Simon Marchi
2021-06-01 17:25 ` John Baldwin
2021-06-01 21:11 ` [PATCH v2] " John Baldwin
2 siblings, 1 reply; 6+ messages in thread
From: Simon Marchi @ 2021-06-01 0:59 UTC (permalink / raw)
To: John Baldwin, gdb-patches
On 2021-05-31 12:15 p.m., John Baldwin wrote:
> There is no default method for
> gdbarch_displaced_step_restore_all_in_ptid, so calling it
> unconditionally for fork events triggered an assertion failure on
> platforms that do not support displaced stepping. To fix, only invoke
> the method if the gdbarch supports displaced stepping.
>
> gdb/ChangeLog:
>
> * infrun.c (handle_inferior_event): Only call
> gdbarch_displaced_step_restore_all_in_ptid if
> gdbarch_supports_displaced_stepping is true.
> ---
> gdb/ChangeLog | 6 ++++++
> gdb/infrun.c | 3 ++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 03910c0634..b0f448a35e 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2021-05-30 John Baldwin <jhb@FreeBSD.org>
> +
> + * infrun.c (handle_inferior_event): Only call
> + gdbarch_displaced_step_restore_all_in_ptid if
> + gdbarch_supports_displaced_stepping is true.
> +
> 2021-05-27 Simon Marchi <simon.marchi@polymtl.ca>
>
> * Fix tab after space indentation issues throughout.
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index e9624d2a9b..6fd077796f 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -5496,7 +5496,8 @@ handle_inferior_event (struct execution_control_state *ecs)
> /* If this is a fork (child gets its own address space copy) and some
> displaced step buffers were in use at the time of the fork, restore
> the displaced step buffer bytes in the child process. */
> - if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
> + if (ecs->ws.kind == TARGET_WAITKIND_FORKED
> + && gdbarch_supports_displaced_stepping (gdbarch))
> gdbarch_displaced_step_restore_all_in_ptid
> (gdbarch, parent_inf, ecs->ws.value.related_pid);
gdbarch_supports_displaced_stepping checks whether the gdbarch implement
gdbarch_displaced_step_prepare. But a gdbarch could technically
implement gdbarch_displaced_step_prepare and not
gdbarch_displaced_step_restore_all_in_ptid. Would it be useful though?
If not, we force that arches implementing gdbarch_displaced_step_prepare
also implement gdbarch_displaced_step_restore_all_in_ptid by using a
post-check on gdbarch creation (like we check that if an arch implements
gdbarch_displaced_step_prepare, it also implements
gdbarch_displaced_step_finish). In that case, checking
gdbarch_supports_displaced_stepping here will be sufficient.
If there's a case for having gdbarch_displaced_step_prepare but not
gdbarch_displaced_step_restore_all_in_ptid, then we would probably need
to use gdbarch_displaced_step_restore_all_in_ptid_p. But I don't see a
use case for that, so I'd go with the other solution.
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Conditionally restore displaced stepping state after fork.
2021-06-01 0:59 ` Simon Marchi
@ 2021-06-01 17:25 ` John Baldwin
0 siblings, 0 replies; 6+ messages in thread
From: John Baldwin @ 2021-06-01 17:25 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 5/31/21 5:59 PM, Simon Marchi wrote:
> On 2021-05-31 12:15 p.m., John Baldwin wrote:
>> There is no default method for
>> gdbarch_displaced_step_restore_all_in_ptid, so calling it
>> unconditionally for fork events triggered an assertion failure on
>> platforms that do not support displaced stepping. To fix, only invoke
>> the method if the gdbarch supports displaced stepping.
>>
>> gdb/ChangeLog:
>>
>> * infrun.c (handle_inferior_event): Only call
>> gdbarch_displaced_step_restore_all_in_ptid if
>> gdbarch_supports_displaced_stepping is true.
>> ---
>> gdb/ChangeLog | 6 ++++++
>> gdb/infrun.c | 3 ++-
>> 2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 03910c0634..b0f448a35e 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,9 @@
>> +2021-05-30 John Baldwin <jhb@FreeBSD.org>
>> +
>> + * infrun.c (handle_inferior_event): Only call
>> + gdbarch_displaced_step_restore_all_in_ptid if
>> + gdbarch_supports_displaced_stepping is true.
>> +
>> 2021-05-27 Simon Marchi <simon.marchi@polymtl.ca>
>>
>> * Fix tab after space indentation issues throughout.
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index e9624d2a9b..6fd077796f 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -5496,7 +5496,8 @@ handle_inferior_event (struct execution_control_state *ecs)
>> /* If this is a fork (child gets its own address space copy) and some
>> displaced step buffers were in use at the time of the fork, restore
>> the displaced step buffer bytes in the child process. */
>> - if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
>> + if (ecs->ws.kind == TARGET_WAITKIND_FORKED
>> + && gdbarch_supports_displaced_stepping (gdbarch))
>> gdbarch_displaced_step_restore_all_in_ptid
>> (gdbarch, parent_inf, ecs->ws.value.related_pid);
>
> gdbarch_supports_displaced_stepping checks whether the gdbarch implement
> gdbarch_displaced_step_prepare. But a gdbarch could technically
> implement gdbarch_displaced_step_prepare and not
> gdbarch_displaced_step_restore_all_in_ptid. Would it be useful though?
> If not, we force that arches implementing gdbarch_displaced_step_prepare
> also implement gdbarch_displaced_step_restore_all_in_ptid by using a
> post-check on gdbarch creation (like we check that if an arch implements
> gdbarch_displaced_step_prepare, it also implements
> gdbarch_displaced_step_finish). In that case, checking
> gdbarch_supports_displaced_stepping here will be sufficient.
>
> If there's a case for having gdbarch_displaced_step_prepare but not
> gdbarch_displaced_step_restore_all_in_ptid, then we would probably need
> to use gdbarch_displaced_step_restore_all_in_ptid_p. But I don't see a
> use case for that, so I'd go with the other solution.
I should have been clearer that I wasn't fully sure this was the right fix.
I had read the comment for gdbarch_supports_displaced_stepping as meaning
it only checked the one method as a check for the group.
/* Return true if the gdbarch implements the required methods to use
displaced stepping. */
static bool
gdbarch_supports_displaced_stepping (gdbarch *arch)
{
/* Only check for the presence of step_copy_insn. Other required methods
are checked by the gdbarch validation. */
return gdbarch_displaced_step_copy_insn_p (arch);
}
If I'm understanding you correctly, we don't currently enforce a check
for this particular method in gdbarch validation so I need to add that?
--
John Baldwin
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] Conditionally restore displaced stepping state after fork.
2021-05-31 16:15 [PATCH] Conditionally restore displaced stepping state after fork John Baldwin
2021-05-31 16:32 ` John Baldwin
2021-06-01 0:59 ` Simon Marchi
@ 2021-06-01 21:11 ` John Baldwin
2021-06-01 21:18 ` Simon Marchi
2 siblings, 1 reply; 6+ messages in thread
From: John Baldwin @ 2021-06-01 21:11 UTC (permalink / raw)
To: gdb-patches
There is no default method for
gdbarch_displaced_step_restore_all_in_ptid, so calling it
unconditionally for fork events triggered an assertion failure on
platforms that do not support displaced stepping. To fix, only invoke
the method if the gdbarch supports displaced stepping.
Note that not all gdbarches support both displaced stepping and fork
events, so gdbarch validation does not require
gdbarch_displaced_step_restore_all_in_ptid for any gdbarch supporting
displaced stepping. However, the internal assertion in
gdbarch_displaced_step_restore_all_in_ptid should catch any gdbarches
which do support both but fail to provide this method.
gdb/ChangeLog:
* infrun.c (handle_inferior_event): Only call
gdbarch_displaced_step_restore_all_in_ptid if
gdbarch_supports_displaced_stepping is true.
---
gdb/ChangeLog | 6 ++++++
gdb/infrun.c | 16 ++++++++++++----
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 03910c0634..b0f448a35e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2021-05-30 John Baldwin <jhb@FreeBSD.org>
+
+ * infrun.c (handle_inferior_event): Only call
+ gdbarch_displaced_step_restore_all_in_ptid if
+ gdbarch_supports_displaced_stepping is true.
+
2021-05-27 Simon Marchi <simon.marchi@polymtl.ca>
* Fix tab after space indentation issues throughout.
diff --git a/gdb/infrun.c b/gdb/infrun.c
index e9624d2a9b..488bcc1e10 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5493,10 +5493,18 @@ handle_inferior_event (struct execution_control_state *ecs)
struct gdbarch *gdbarch = regcache->arch ();
inferior *parent_inf = find_inferior_ptid (ecs->target, ecs->ptid);
- /* If this is a fork (child gets its own address space copy) and some
- displaced step buffers were in use at the time of the fork, restore
- the displaced step buffer bytes in the child process. */
- if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
+ /* If this is a fork (child gets its own address space copy)
+ and some displaced step buffers were in use at the time of
+ the fork, restore the displaced step buffer bytes in the
+ child process.
+
+ Architectures which support displaced stepping and fork
+ events must supply an implementation of
+ gdbarch_displaced_step_restore_all_in_ptid. This is not
+ enforced during gdbarch validation to support architectures
+ which support displaced stepping but not forks. */
+ if (ecs->ws.kind == TARGET_WAITKIND_FORKED
+ && gdbarch_supports_displaced_stepping (gdbarch))
gdbarch_displaced_step_restore_all_in_ptid
(gdbarch, parent_inf, ecs->ws.value.related_pid);
--
2.31.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Conditionally restore displaced stepping state after fork.
2021-06-01 21:11 ` [PATCH v2] " John Baldwin
@ 2021-06-01 21:18 ` Simon Marchi
0 siblings, 0 replies; 6+ messages in thread
From: Simon Marchi @ 2021-06-01 21:18 UTC (permalink / raw)
To: John Baldwin, gdb-patches
On 2021-06-01 5:11 p.m., John Baldwin wrote:
> There is no default method for
> gdbarch_displaced_step_restore_all_in_ptid, so calling it
> unconditionally for fork events triggered an assertion failure on
> platforms that do not support displaced stepping. To fix, only invoke
> the method if the gdbarch supports displaced stepping.
>
> Note that not all gdbarches support both displaced stepping and fork
> events, so gdbarch validation does not require
> gdbarch_displaced_step_restore_all_in_ptid for any gdbarch supporting
> displaced stepping. However, the internal assertion in
> gdbarch_displaced_step_restore_all_in_ptid should catch any gdbarches
> which do support both but fail to provide this method.
>
> gdb/ChangeLog:
>
> * infrun.c (handle_inferior_event): Only call
> gdbarch_displaced_step_restore_all_in_ptid if
> gdbarch_supports_displaced_stepping is true.
> ---
> gdb/ChangeLog | 6 ++++++
> gdb/infrun.c | 16 ++++++++++++----
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 03910c0634..b0f448a35e 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2021-05-30 John Baldwin <jhb@FreeBSD.org>
> +
> + * infrun.c (handle_inferior_event): Only call
> + gdbarch_displaced_step_restore_all_in_ptid if
> + gdbarch_supports_displaced_stepping is true.
> +
> 2021-05-27 Simon Marchi <simon.marchi@polymtl.ca>
>
> * Fix tab after space indentation issues throughout.
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index e9624d2a9b..488bcc1e10 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -5493,10 +5493,18 @@ handle_inferior_event (struct execution_control_state *ecs)
> struct gdbarch *gdbarch = regcache->arch ();
> inferior *parent_inf = find_inferior_ptid (ecs->target, ecs->ptid);
>
> - /* If this is a fork (child gets its own address space copy) and some
> - displaced step buffers were in use at the time of the fork, restore
> - the displaced step buffer bytes in the child process. */
> - if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
> + /* If this is a fork (child gets its own address space copy)
> + and some displaced step buffers were in use at the time of
> + the fork, restore the displaced step buffer bytes in the
> + child process.
> +
> + Architectures which support displaced stepping and fork
> + events must supply an implementation of
> + gdbarch_displaced_step_restore_all_in_ptid. This is not
> + enforced during gdbarch validation to support architectures
> + which support displaced stepping but not forks. */
> + if (ecs->ws.kind == TARGET_WAITKIND_FORKED
> + && gdbarch_supports_displaced_stepping (gdbarch))
> gdbarch_displaced_step_restore_all_in_ptid
> (gdbarch, parent_inf, ecs->ws.value.related_pid);
>
>
LGTM, thanks!
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-01 21:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-31 16:15 [PATCH] Conditionally restore displaced stepping state after fork John Baldwin
2021-05-31 16:32 ` John Baldwin
2021-06-01 0:59 ` Simon Marchi
2021-06-01 17:25 ` John Baldwin
2021-06-01 21:11 ` [PATCH v2] " John Baldwin
2021-06-01 21:18 ` Simon Marchi
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).