* [PATCH 1/2] Eliminate target_have_continuable_watchpoint
2018-08-31 15:12 [PATCH 0/2] continuable/steppable/non-steppable watchpoints Pedro Alves
2018-08-31 15:12 ` [PATCH 2/2] Add comment describing " Pedro Alves
@ 2018-08-31 15:12 ` Pedro Alves
2018-08-31 15:33 ` Tom Tromey
1 sibling, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2018-08-31 15:12 UTC (permalink / raw)
To: gdb-patches
target_have_continuable_watchpoint isn't used anywhere so remove it.
The property isn't necessary because checking for "continuable" is the
same as checking for "!steppable && !non-steppable".
gdb/ChangeLog:
2018-08-31 Pedro Alves <palves@redhat.com>
* nto-procfs.c (nto_procfs_target::have_continuable_watchpoint):
Delete.
* s390-linux-nat.c
(s390_linux_nat_target::have_continuable_watchpoint): Delete.
* target.h (target_ops::have_continuable_watchpoint): Delete.
(target_have_continuable_watchpoint): Delete.
* x86-nat.h (x86_nat_target::have_continuable_watchpoint): Delete.
* target-delegates.c: Regenerate.
---
gdb/nto-procfs.c | 3 ---
gdb/s390-linux-nat.c | 1 -
gdb/target-delegates.c | 27 ---------------------------
gdb/target.h | 7 -------
gdb/x86-nat.h | 2 --
5 files changed, 40 deletions(-)
diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
index 117edb161c3..54357af85b2 100644
--- a/gdb/nto-procfs.c
+++ b/gdb/nto-procfs.c
@@ -119,9 +119,6 @@ struct nto_procfs_target : public inf_child_target
void interrupt () override;
- bool have_continuable_watchpoint ()
- { return true; }
-
const char *extra_thread_info (struct thread_info *) override;
char *pid_to_exec_file (int pid) override;
diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index 2c605628105..7a04e854eb7 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -122,7 +122,6 @@ public:
int remove_hw_breakpoint (struct gdbarch *, struct bp_target_info *)
override;
int region_ok_for_hw_watchpoint (CORE_ADDR, int) override;
- bool have_continuable_watchpoint () override { return true; }
bool stopped_by_watchpoint () override;
int insert_watchpoint (CORE_ADDR, int, enum target_hw_bp_type,
struct expression *) override;
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 319a9bb46b4..03136dfacf2 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -36,7 +36,6 @@ struct dummy_target : public target_ops
int remove_mask_watchpoint (CORE_ADDR arg0, CORE_ADDR arg1, enum target_hw_bp_type arg2) override;
bool stopped_by_watchpoint () override;
int have_steppable_watchpoint () override;
- bool have_continuable_watchpoint () override;
bool stopped_data_address (CORE_ADDR *arg0) override;
bool watchpoint_addr_within_range (CORE_ADDR arg0, CORE_ADDR arg1, int arg2) override;
int region_ok_for_hw_watchpoint (CORE_ADDR arg0, int arg1) override;
@@ -204,7 +203,6 @@ struct debug_target : public target_ops
int remove_mask_watchpoint (CORE_ADDR arg0, CORE_ADDR arg1, enum target_hw_bp_type arg2) override;
bool stopped_by_watchpoint () override;
int have_steppable_watchpoint () override;
- bool have_continuable_watchpoint () override;
bool stopped_data_address (CORE_ADDR *arg0) override;
bool watchpoint_addr_within_range (CORE_ADDR arg0, CORE_ADDR arg1, int arg2) override;
int region_ok_for_hw_watchpoint (CORE_ADDR arg0, int arg1) override;
@@ -1016,31 +1014,6 @@ debug_target::have_steppable_watchpoint ()
return result;
}
-bool
-target_ops::have_continuable_watchpoint ()
-{
- return this->beneath ()->have_continuable_watchpoint ();
-}
-
-bool
-dummy_target::have_continuable_watchpoint ()
-{
- return false;
-}
-
-bool
-debug_target::have_continuable_watchpoint ()
-{
- bool result;
- fprintf_unfiltered (gdb_stdlog, "-> %s->have_continuable_watchpoint (...)\n", this->beneath ()->shortname ());
- result = this->beneath ()->have_continuable_watchpoint ();
- fprintf_unfiltered (gdb_stdlog, "<- %s->have_continuable_watchpoint (", this->beneath ()->shortname ());
- fputs_unfiltered (") = ", gdb_stdlog);
- target_debug_print_bool (result);
- fputs_unfiltered ("\n", gdb_stdlog);
- return result;
-}
-
bool
target_ops::stopped_data_address (CORE_ADDR *arg0)
{
diff --git a/gdb/target.h b/gdb/target.h
index 39aa8c3c730..b29d34b8a65 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -552,8 +552,6 @@ struct target_ops
TARGET_DEFAULT_RETURN (false);
virtual int have_steppable_watchpoint ()
TARGET_DEFAULT_RETURN (false);
- virtual bool have_continuable_watchpoint ()
- TARGET_DEFAULT_RETURN (false);
virtual bool stopped_data_address (CORE_ADDR *)
TARGET_DEFAULT_RETURN (false);
virtual bool watchpoint_addr_within_range (CORE_ADDR, CORE_ADDR, int)
@@ -1933,11 +1931,6 @@ extern struct thread_info *target_thread_handle_to_thread_info
#define target_have_steppable_watchpoint \
(current_top_target ()->have_steppable_watchpoint ())
-/* Non-zero if we have continuable watchpoints */
-
-#define target_have_continuable_watchpoint \
- (current_top_target ()->have_continuable_watchpoint ())
-
/* Provide defaults for hardware watchpoint functions. */
/* If the *_hw_beakpoint functions have not been defined
diff --git a/gdb/x86-nat.h b/gdb/x86-nat.h
index cc27fa43b4d..f6d8a26baf9 100644
--- a/gdb/x86-nat.h
+++ b/gdb/x86-nat.h
@@ -76,8 +76,6 @@ struct x86_nat_target : public BaseTarget
the one that caused the trap. Therefore we don't need to step
over it. But we do need to reset the status register to avoid
another trap. */
- bool have_continuable_watchpoint () override
- { return true; }
int can_use_hw_breakpoint (enum bptype type, int cnt, int othertype) override
{ return x86_can_use_hw_breakpoint (type, cnt, othertype); }
--
2.14.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 0/2] continuable/steppable/non-steppable watchpoints
@ 2018-08-31 15:12 Pedro Alves
2018-08-31 15:12 ` [PATCH 2/2] Add comment describing " Pedro Alves
2018-08-31 15:12 ` [PATCH 1/2] Eliminate target_have_continuable_watchpoint Pedro Alves
0 siblings, 2 replies; 9+ messages in thread
From: Pedro Alves @ 2018-08-31 15:12 UTC (permalink / raw)
To: gdb-patches
This is a follow up to
<https://sourceware.org/ml/gdb/2018-08/msg00054.html>.
Pedro Alves (2):
Eliminate target_have_continuable_watchpoint
Add comment describing continuable/steppable/non-steppable watchpoints
gdb/gdbarch.h | 2 ++
gdb/gdbarch.sh | 1 +
gdb/nto-procfs.c | 3 ---
gdb/s390-linux-nat.c | 1 -
gdb/target-delegates.c | 27 ---------------------------
gdb/target.h | 41 ++++++++++++++++++++++++++++++++++-------
gdb/x86-nat.h | 2 --
7 files changed, 37 insertions(+), 40 deletions(-)
--
2.14.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] Add comment describing continuable/steppable/non-steppable watchpoints
2018-08-31 15:12 [PATCH 0/2] continuable/steppable/non-steppable watchpoints Pedro Alves
@ 2018-08-31 15:12 ` Pedro Alves
2018-08-31 15:41 ` Tom Tromey
2018-08-31 15:12 ` [PATCH 1/2] Eliminate target_have_continuable_watchpoint Pedro Alves
1 sibling, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2018-08-31 15:12 UTC (permalink / raw)
To: gdb-patches
These weren't described anywhere in the sources.
gdb/ChangeLog:
2018-08-31 Pedro Alves <palves@redhat.com>
* gdbarch.sh (have_nonsteppable_watchpoint): Add comment.
* target.h (Hardware watchpoint interfaces): Describe
continuable/steppable/non-steppable watchpoints.
---
gdb/gdbarch.h | 2 ++
gdb/gdbarch.sh | 1 +
gdb/target.h | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 37 insertions(+)
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index b563f8d165a..83408812b2a 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -818,6 +818,8 @@ extern void set_gdbarch_adjust_dwarf2_line (struct gdbarch *gdbarch, gdbarch_adj
extern int gdbarch_cannot_step_breakpoint (struct gdbarch *gdbarch);
extern void set_gdbarch_cannot_step_breakpoint (struct gdbarch *gdbarch, int cannot_step_breakpoint);
+/* See comment in target.h about continue, steppable and non-steppable watchpoints. */
+
extern int gdbarch_have_nonsteppable_watchpoint (struct gdbarch *gdbarch);
extern void set_gdbarch_have_nonsteppable_watchpoint (struct gdbarch *gdbarch, int have_nonsteppable_watchpoint);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 24a553b73c3..941c11b430e 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -707,6 +707,7 @@ f;CORE_ADDR;adjust_dwarf2_addr;CORE_ADDR pc;pc;;default_adjust_dwarf2_addr;;0
# stop PC.
f;CORE_ADDR;adjust_dwarf2_line;CORE_ADDR addr, int rel;addr, rel;;default_adjust_dwarf2_line;;0
v;int;cannot_step_breakpoint;;;0;0;;0
+# See comment in target.h about continue, steppable and non-steppable watchpoints.
v;int;have_nonsteppable_watchpoint;;;0;0;;0
F;int;address_class_type_flags;int byte_size, int dwarf2_addr_class;byte_size, dwarf2_addr_class
M;const char *;address_class_type_flags_to_name;int type_flags;type_flags
diff --git a/gdb/target.h b/gdb/target.h
index b29d34b8a65..229b5d0551a 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1905,6 +1905,40 @@ extern struct thread_info *target_thread_handle_to_thread_info
/* Hardware watchpoint interfaces. */
+/* GDB's current model is that there are three "kinds" of watchpoints,
+ with respect to when they trigger and how you can move past them.
+
+ Those are: continuable, steppable, and non-steppable.
+
+ Continuable watchpoints are like x86's -- those trigger after the
+ memory access's side effects are fully committed to memory. I.e.,
+ they trap with the PC pointing at the next instruction already.
+ Continuing past such a watchpoint is doable by just normally
+ continuing, hence the name.
+
+ Both steppable and non-steppable watchpoints trap before the memory
+ access. I.e, the PC points at the instruction that is accessing
+ the memory. So GDB needs to single-step once past the current
+ instruction in order to make the access effective and check whether
+ the instruction's side effects change the watched expression.
+
+ Now, in order to step past that instruction, depending on
+ architecture and target, you can have two situations:
+
+ - steppable watchpoints: you can single-step with the watchpoint
+ still armed, and the watchpoint won't trigger again.
+
+ - non-steppable watchpoints: if you try to single-step with the
+ watchpoint still armed, you'd trap the watchpoint again and the
+ thread wouldn't make any progress. So GDB needs to temporarily
+ remove the watchpoint in order to step past it.
+
+ If your target/architecture does not signal that it has either
+ steppable or non-steppable watchpoints via either
+ target_have_steppable_watchpoint or
+ gdbarch_have_nonsteppable_watchpoint, GDB assumes continuable
+ watchpoints. */
+
/* Returns non-zero if we were stopped by a hardware watchpoint (memory read or
write). Only the INFERIOR_PTID task is being queried. */
--
2.14.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Eliminate target_have_continuable_watchpoint
2018-08-31 15:12 ` [PATCH 1/2] Eliminate target_have_continuable_watchpoint Pedro Alves
@ 2018-08-31 15:33 ` Tom Tromey
2018-08-31 15:40 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2018-08-31 15:33 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> target_have_continuable_watchpoint isn't used anywhere so remove it.
Pedro> The property isn't necessary because checking for "continuable" is the
Pedro> same as checking for "!steppable && !non-steppable".
Pedro> /* If the *_hw_beakpoint functions have not been defined
Pedro> diff --git a/gdb/x86-nat.h b/gdb/x86-nat.h
Pedro> index cc27fa43b4d..f6d8a26baf9 100644
Pedro> --- a/gdb/x86-nat.h
Pedro> +++ b/gdb/x86-nat.h
Pedro> @@ -76,8 +76,6 @@ struct x86_nat_target : public BaseTarget
Pedro> the one that caused the trap. Therefore we don't need to step
Pedro> over it. But we do need to reset the status register to avoid
Pedro> another trap. */
Pedro> - bool have_continuable_watchpoint () override
Pedro> - { return true; }
I think the comment just before this function should also be removed.
Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Eliminate target_have_continuable_watchpoint
2018-08-31 15:33 ` Tom Tromey
@ 2018-08-31 15:40 ` Pedro Alves
2018-08-31 15:43 ` Tom Tromey
0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2018-08-31 15:40 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 08/31/2018 04:32 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> target_have_continuable_watchpoint isn't used anywhere so remove it.
> Pedro> The property isn't necessary because checking for "continuable" is the
> Pedro> same as checking for "!steppable && !non-steppable".
>
> Pedro> /* If the *_hw_beakpoint functions have not been defined
> Pedro> diff --git a/gdb/x86-nat.h b/gdb/x86-nat.h
> Pedro> index cc27fa43b4d..f6d8a26baf9 100644
> Pedro> --- a/gdb/x86-nat.h
> Pedro> +++ b/gdb/x86-nat.h
> Pedro> @@ -76,8 +76,6 @@ struct x86_nat_target : public BaseTarget
> Pedro> the one that caused the trap. Therefore we don't need to step
> Pedro> over it. But we do need to reset the status register to avoid
> Pedro> another trap. */
> Pedro> - bool have_continuable_watchpoint () override
> Pedro> - { return true; }
>
> I think the comment just before this function should also be removed.
I thought it was still useful as is. It's describing x86 watchpoints
generically enough before a set of functions implementing watchpoints
support that I thought that it's still positioned in a good place as
is. WDYT?
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Add comment describing continuable/steppable/non-steppable watchpoints
2018-08-31 15:12 ` [PATCH 2/2] Add comment describing " Pedro Alves
@ 2018-08-31 15:41 ` Tom Tromey
2018-08-31 17:56 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2018-08-31 15:41 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> These weren't described anywhere in the sources.
This is excellent, thank you very much for doing this.
Pedro> +# See comment in target.h about continue, steppable and non-steppable watchpoints.
Probably "continuable" here as well.
Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Eliminate target_have_continuable_watchpoint
2018-08-31 15:40 ` Pedro Alves
@ 2018-08-31 15:43 ` Tom Tromey
2018-08-31 17:52 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2018-08-31 15:43 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>> I think the comment just before this function should also be removed.
Pedro> I thought it was still useful as is. It's describing x86 watchpoints
Pedro> generically enough before a set of functions implementing watchpoints
Pedro> support that I thought that it's still positioned in a good place as
Pedro> is. WDYT?
I looked at that but I didn't understand how the comment related to the
nearby code; like I didn't see anything else in that file about the
status register. However I think it's a minor point and if it makes
sense to you it is fine.
Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Eliminate target_have_continuable_watchpoint
2018-08-31 15:43 ` Tom Tromey
@ 2018-08-31 17:52 ` Pedro Alves
0 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2018-08-31 17:52 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 08/31/2018 04:43 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
>>> I think the comment just before this function should also be removed.
>
> Pedro> I thought it was still useful as is. It's describing x86 watchpoints
> Pedro> generically enough before a set of functions implementing watchpoints
> Pedro> support that I thought that it's still positioned in a good place as
> Pedro> is. WDYT?
>
> I looked at that but I didn't understand how the comment related to the
> nearby code; like I didn't see anything else in that file about the
> status register. However I think it's a minor point and if it makes
> sense to you it is fine.
Ah, I was looking more at the bit explaining that the watchpoints
are continuable, so don't need to override target_has_steppable_watchpoint.
I won't miss it myself, so if it isn't helping others, might as well
remove it.
I've done that before pushing the patch in.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Add comment describing continuable/steppable/non-steppable watchpoints
2018-08-31 15:41 ` Tom Tromey
@ 2018-08-31 17:56 ` Pedro Alves
0 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2018-08-31 17:56 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 08/31/2018 04:40 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> These weren't described anywhere in the sources.
>
> This is excellent, thank you very much for doing this.
>
> Pedro> +# See comment in target.h about continue, steppable and non-steppable watchpoints.
>
> Probably "continuable" here as well.
Whoops, yes. Pushed with that fixed.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-08-31 17:56 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-31 15:12 [PATCH 0/2] continuable/steppable/non-steppable watchpoints Pedro Alves
2018-08-31 15:12 ` [PATCH 2/2] Add comment describing " Pedro Alves
2018-08-31 15:41 ` Tom Tromey
2018-08-31 17:56 ` Pedro Alves
2018-08-31 15:12 ` [PATCH 1/2] Eliminate target_have_continuable_watchpoint Pedro Alves
2018-08-31 15:33 ` Tom Tromey
2018-08-31 15:40 ` Pedro Alves
2018-08-31 15:43 ` Tom Tromey
2018-08-31 17:52 ` Pedro Alves
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).