* [patch 1/8] Download tracepoint on location level
2011-11-08 6:00 [patch 0/8] Download tracepoint locations when tracing is running Yao Qi
@ 2011-11-08 6:08 ` Yao Qi
2011-11-09 3:34 ` Yao Qi
2011-11-08 6:12 ` [patch 2/8] New remote feature InstallInTrace Yao Qi
` (6 subsequent siblings)
7 siblings, 1 reply; 31+ messages in thread
From: Yao Qi @ 2011-11-08 6:08 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 568 bytes --]
Existing gdb target hook to_download_tracepoint is about downloading
tracepoint on tracepoint level, rather than location level. However,
locations of one tracepoint may change during inferior run, so we need a
fine-granularity target hook on tracepoint locations, instead of tracepoint.
This patch is to refactor to_download_tracepoint to
to_download_tracepoint_loc. Beside this refactor, a return value is
added in new hook to reflect whether tracepoint location is
downloaded/installed successfully. Functionality of gdb is not changed.
--
Yao (é½å°§)
[-- Attachment #2: 0001-refactor-download_tracepoint-to-download_tracepoint_.patch --]
[-- Type: text/x-patch, Size: 15717 bytes --]
* target.h (struct target): Rename field to_download_tracepoint
to to_download_tracepoint_loc. Change type of parameter from
tracepoint bp_location.
* target.c (update_current_target): Update.
* tracepoint.c (start_tracing): Update.
* remote.c: Move remote_download_tracepoint. to
remote_download_tracepoint_loc. Remove loop for each location
of a tracepoint.
---
gdb/remote.c | 284 +++++++++++++++++++++++++++---------------------------
gdb/target.c | 6 +-
gdb/target.h | 12 ++-
gdb/tracepoint.c | 6 +-
4 files changed, 157 insertions(+), 151 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index 5182ef1..1c43f39 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -9834,10 +9834,10 @@ remote_download_command_source (int num, ULONGEST addr,
}
}
-static void
-remote_download_tracepoint (struct breakpoint *b)
+static int
+remote_download_tracepoint_loc (struct bp_location *loc)
{
- struct bp_location *loc;
+
CORE_ADDR tpaddr;
char addrbuf[40];
char buf[2048];
@@ -9848,169 +9848,167 @@ remote_download_tracepoint (struct breakpoint *b)
struct agent_expr *aexpr;
struct cleanup *aexpr_chain = NULL;
char *pkt;
+ struct breakpoint *b = loc->owner;
struct tracepoint *t = (struct tracepoint *) b;
- /* Iterate over all the tracepoint locations. It's up to the target to
- notice multiple tracepoint packets with the same number but different
- addresses, and treat them as multiple locations. */
- for (loc = b->loc; loc; loc = loc->next)
- {
- encode_actions (b, loc, &tdp_actions, &stepping_actions);
- old_chain = make_cleanup (free_actions_list_cleanup_wrapper,
- tdp_actions);
- (void) make_cleanup (free_actions_list_cleanup_wrapper,
- stepping_actions);
-
- tpaddr = loc->address;
- sprintf_vma (addrbuf, tpaddr);
- sprintf (buf, "QTDP:%x:%s:%c:%lx:%x", b->number,
- addrbuf, /* address */
- (b->enable_state == bp_enabled ? 'E' : 'D'),
- t->step_count, t->pass_count);
- /* Fast tracepoints are mostly handled by the target, but we can
- tell the target how big of an instruction block should be moved
- around. */
- if (b->type == bp_fast_tracepoint)
+ encode_actions (loc->owner, loc, &tdp_actions, &stepping_actions);
+ old_chain = make_cleanup (free_actions_list_cleanup_wrapper,
+ tdp_actions);
+ (void) make_cleanup (free_actions_list_cleanup_wrapper,
+ stepping_actions);
+
+ tpaddr = loc->address;
+ sprintf_vma (addrbuf, tpaddr);
+ sprintf (buf, "QTDP:%x:%s:%c:%lx:%x", b->number,
+ addrbuf, /* address */
+ (b->enable_state == bp_enabled ? 'E' : 'D'),
+ t->step_count, t->pass_count);
+ /* Fast tracepoints are mostly handled by the target, but we can
+ tell the target how big of an instruction block should be moved
+ around. */
+ if (b->type == bp_fast_tracepoint)
+ {
+ /* Only test for support at download time; we may not know
+ target capabilities at definition time. */
+ if (remote_supports_fast_tracepoints ())
{
- /* Only test for support at download time; we may not know
- target capabilities at definition time. */
- if (remote_supports_fast_tracepoints ())
- {
- int isize;
+ int isize;
- if (gdbarch_fast_tracepoint_valid_at (target_gdbarch,
- tpaddr, &isize, NULL))
- sprintf (buf + strlen (buf), ":F%x", isize);
- else
- /* If it passed validation at definition but fails now,
- something is very wrong. */
- internal_error (__FILE__, __LINE__,
- _("Fast tracepoint not "
- "valid during download"));
- }
+ if (gdbarch_fast_tracepoint_valid_at (target_gdbarch,
+ tpaddr, &isize, NULL))
+ sprintf (buf + strlen (buf), ":F%x", isize);
else
- /* Fast tracepoints are functionally identical to regular
- tracepoints, so don't take lack of support as a reason to
- give up on the trace run. */
- warning (_("Target does not support fast tracepoints, "
- "downloading %d as regular tracepoint"), b->number);
+ /* If it passed validation at definition but fails now,
+ something is very wrong. */
+ internal_error (__FILE__, __LINE__,
+ _("Fast tracepoint not "
+ "valid during download"));
}
- else if (b->type == bp_static_tracepoint)
+ else
+ /* Fast tracepoints are functionally identical to regular
+ tracepoints, so don't take lack of support as a reason to
+ give up on the trace run. */
+ warning (_("Target does not support fast tracepoints, "
+ "downloading %d as regular tracepoint"), b->number);
+ }
+ else if (b->type == bp_static_tracepoint)
+ {
+ /* Only test for support at download time; we may not know
+ target capabilities at definition time. */
+ if (remote_supports_static_tracepoints ())
{
- /* Only test for support at download time; we may not know
- target capabilities at definition time. */
- if (remote_supports_static_tracepoints ())
- {
- struct static_tracepoint_marker marker;
+ struct static_tracepoint_marker marker;
- if (target_static_tracepoint_marker_at (tpaddr, &marker))
- strcat (buf, ":S");
- else
- error (_("Static tracepoint not valid during download"));
- }
+ if (target_static_tracepoint_marker_at (tpaddr, &marker))
+ strcat (buf, ":S");
else
- /* Fast tracepoints are functionally identical to regular
- tracepoints, so don't take lack of support as a reason
- to give up on the trace run. */
- error (_("Target does not support static tracepoints"));
+ error (_("Static tracepoint not valid during download"));
}
- /* If the tracepoint has a conditional, make it into an agent
- expression and append to the definition. */
- if (loc->cond)
+ else
+ /* Fast tracepoints are functionally identical to regular
+ tracepoints, so don't take lack of support as a reason
+ to give up on the trace run. */
+ error (_("Target does not support static tracepoints"));
+ }
+ /* If the tracepoint has a conditional, make it into an agent
+ expression and append to the definition. */
+ if (loc->cond)
+ {
+ /* Only test support at download time, we may not know target
+ capabilities at definition time. */
+ if (remote_supports_cond_tracepoints ())
{
- /* Only test support at download time, we may not know target
- capabilities at definition time. */
- if (remote_supports_cond_tracepoints ())
- {
- aexpr = gen_eval_for_expr (tpaddr, loc->cond);
- aexpr_chain = make_cleanup_free_agent_expr (aexpr);
- sprintf (buf + strlen (buf), ":X%x,", aexpr->len);
- pkt = buf + strlen (buf);
- for (ndx = 0; ndx < aexpr->len; ++ndx)
- pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
- *pkt = '\0';
- do_cleanups (aexpr_chain);
- }
- else
- warning (_("Target does not support conditional tracepoints, "
- "ignoring tp %d cond"), b->number);
+ aexpr = gen_eval_for_expr (tpaddr, loc->cond);
+ aexpr_chain = make_cleanup_free_agent_expr (aexpr);
+ sprintf (buf + strlen (buf), ":X%x,", aexpr->len);
+ pkt = buf + strlen (buf);
+ for (ndx = 0; ndx < aexpr->len; ++ndx)
+ pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
+ *pkt = '\0';
+ do_cleanups (aexpr_chain);
}
+ else
+ warning (_("Target does not support conditional tracepoints, "
+ "ignoring tp %d cond"), b->number);
+ }
if (b->commands || *default_collect)
- strcat (buf, "-");
- putpkt (buf);
- remote_get_noisy_reply (&target_buf, &target_buf_size);
- if (strcmp (target_buf, "OK"))
- error (_("Target does not support tracepoints."));
+ strcat (buf, "-");
+ putpkt (buf);
+ remote_get_noisy_reply (&target_buf, &target_buf_size);
+ if (strcmp (target_buf, "OK"))
+ error (_("Target does not support tracepoints."));
- /* do_single_steps (t); */
- if (tdp_actions)
+ /* do_single_steps (t); */
+ if (tdp_actions)
+ {
+ for (ndx = 0; tdp_actions[ndx]; ndx++)
{
- for (ndx = 0; tdp_actions[ndx]; ndx++)
- {
- QUIT; /* Allow user to bail out with ^C. */
- sprintf (buf, "QTDP:-%x:%s:%s%c",
- b->number, addrbuf, /* address */
- tdp_actions[ndx],
- ((tdp_actions[ndx + 1] || stepping_actions)
- ? '-' : 0));
- putpkt (buf);
- remote_get_noisy_reply (&target_buf,
- &target_buf_size);
- if (strcmp (target_buf, "OK"))
- error (_("Error on target while setting tracepoints."));
- }
+ QUIT; /* Allow user to bail out with ^C. */
+ sprintf (buf, "QTDP:-%x:%s:%s%c",
+ b->number, addrbuf, /* address */
+ tdp_actions[ndx],
+ ((tdp_actions[ndx + 1] || stepping_actions)
+ ? '-' : 0));
+ putpkt (buf);
+ remote_get_noisy_reply (&target_buf,
+ &target_buf_size);
+ if (strcmp (target_buf, "OK"))
+ error (_("Error on target while setting tracepoints."));
}
- if (stepping_actions)
+ }
+ if (stepping_actions)
+ {
+ for (ndx = 0; stepping_actions[ndx]; ndx++)
{
- for (ndx = 0; stepping_actions[ndx]; ndx++)
- {
- QUIT; /* Allow user to bail out with ^C. */
- sprintf (buf, "QTDP:-%x:%s:%s%s%s",
- b->number, addrbuf, /* address */
- ((ndx == 0) ? "S" : ""),
- stepping_actions[ndx],
- (stepping_actions[ndx + 1] ? "-" : ""));
- putpkt (buf);
- remote_get_noisy_reply (&target_buf,
- &target_buf_size);
- if (strcmp (target_buf, "OK"))
- error (_("Error on target while setting tracepoints."));
- }
+ QUIT; /* Allow user to bail out with ^C. */
+ sprintf (buf, "QTDP:-%x:%s:%s%s%s",
+ b->number, addrbuf, /* address */
+ ((ndx == 0) ? "S" : ""),
+ stepping_actions[ndx],
+ (stepping_actions[ndx + 1] ? "-" : ""));
+ putpkt (buf);
+ remote_get_noisy_reply (&target_buf,
+ &target_buf_size);
+ if (strcmp (target_buf, "OK"))
+ error (_("Error on target while setting tracepoints."));
}
+ }
- if (remote_protocol_packets[PACKET_TracepointSource].support
- == PACKET_ENABLE)
+ if (remote_protocol_packets[PACKET_TracepointSource].support
+ == PACKET_ENABLE)
+ {
+ if (b->addr_string)
{
- if (b->addr_string)
- {
- strcpy (buf, "QTDPsrc:");
- encode_source_string (b->number, loc->address,
- "at", b->addr_string, buf + strlen (buf),
- 2048 - strlen (buf));
+ strcpy (buf, "QTDPsrc:");
+ encode_source_string (b->number, loc->address,
+ "at", b->addr_string, buf + strlen (buf),
+ 2048 - strlen (buf));
- putpkt (buf);
- remote_get_noisy_reply (&target_buf, &target_buf_size);
- if (strcmp (target_buf, "OK"))
- warning (_("Target does not support source download."));
- }
- if (b->cond_string)
- {
- strcpy (buf, "QTDPsrc:");
- encode_source_string (b->number, loc->address,
- "cond", b->cond_string, buf + strlen (buf),
- 2048 - strlen (buf));
- putpkt (buf);
- remote_get_noisy_reply (&target_buf, &target_buf_size);
- if (strcmp (target_buf, "OK"))
- warning (_("Target does not support source download."));
- }
- remote_download_command_source (b->number, loc->address,
- breakpoint_commands (b));
+ putpkt (buf);
+ remote_get_noisy_reply (&target_buf, &target_buf_size);
+ if (strcmp (target_buf, "OK"))
+ warning (_("Target does not support source download."));
}
-
- do_cleanups (old_chain);
+ if (b->cond_string)
+ {
+ strcpy (buf, "QTDPsrc:");
+ encode_source_string (b->number, loc->address,
+ "cond", b->cond_string, buf + strlen (buf),
+ 2048 - strlen (buf));
+ putpkt (buf);
+ remote_get_noisy_reply (&target_buf, &target_buf_size);
+ if (strcmp (target_buf, "OK"))
+ warning (_("Target does not support source download."));
+ }
+ remote_download_command_source (b->number, loc->address,
+ breakpoint_commands (b));
}
+
+ do_cleanups (old_chain);
+
+ return 0;
+
}
static void
@@ -10484,7 +10482,7 @@ Specify the serial device it is connected to\n\
remote_ops.to_supports_enable_disable_tracepoint = remote_supports_enable_disable_tracepoint;
remote_ops.to_supports_string_tracing = remote_supports_string_tracing;
remote_ops.to_trace_init = remote_trace_init;
- remote_ops.to_download_tracepoint = remote_download_tracepoint;
+ remote_ops.to_download_tracepoint_loc = remote_download_tracepoint_loc;
remote_ops.to_download_trace_state_variable
= remote_download_trace_state_variable;
remote_ops.to_enable_tracepoint = remote_enable_tracepoint;
diff --git a/gdb/target.c b/gdb/target.c
index 41ff6cf..48cd888 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -674,7 +674,7 @@ update_current_target (void)
INHERIT (to_supports_enable_disable_tracepoint, t);
INHERIT (to_supports_string_tracing, t);
INHERIT (to_trace_init, t);
- INHERIT (to_download_tracepoint, t);
+ INHERIT (to_download_tracepoint_loc, t);
INHERIT (to_download_trace_state_variable, t);
INHERIT (to_enable_tracepoint, t);
INHERIT (to_disable_tracepoint, t);
@@ -847,8 +847,8 @@ update_current_target (void)
de_fault (to_trace_init,
(void (*) (void))
tcomplain);
- de_fault (to_download_tracepoint,
- (void (*) (struct breakpoint *))
+ de_fault (to_download_tracepoint_loc,
+ (int (*) (struct bp_location *))
tcomplain);
de_fault (to_download_trace_state_variable,
(void (*) (struct trace_state_variable *))
diff --git a/gdb/target.h b/gdb/target.h
index 352f2df..74e1d8a 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -686,8 +686,12 @@ struct target_ops
/* Prepare the target for a tracing run. */
void (*to_trace_init) (void);
- /* Send full details of a tracepoint to the target. */
- void (*to_download_tracepoint) (struct breakpoint *t);
+ /* Send full details of a tracepoint location to the target. Target may
+ install or not install tracepoint locations to inferior, which is
+ determined by target's factors, such tracing state. If the location
+ of tracepoint is downloaded and installed successfully, return 0,
+ otherwise return 1. */
+ int (*to_download_tracepoint_loc) (struct bp_location *location);
/* Send full details of a trace state variable to the target. */
void (*to_download_trace_state_variable) (struct trace_state_variable *tsv);
@@ -1474,8 +1478,8 @@ extern int target_search_memory (CORE_ADDR start_addr,
#define target_trace_init() \
(*current_target.to_trace_init) ()
-#define target_download_tracepoint(t) \
- (*current_target.to_download_tracepoint) (t)
+#define target_download_tracepoint_loc(t) \
+ (*current_target.to_download_tracepoint_loc) (t)
#define target_download_trace_state_variable(tsv) \
(*current_target.to_download_trace_state_variable) (tsv)
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 4ca4ec2..493d324 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1701,6 +1701,7 @@ start_tracing (void)
for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
{
struct tracepoint *t = (struct tracepoint *) b;
+ struct bp_location *loc;
if ((b->type == bp_fast_tracepoint
? !may_insert_fast_tracepoints
@@ -1708,7 +1709,10 @@ start_tracing (void)
continue;
t->number_on_target = 0;
- target_download_tracepoint (b);
+
+ for (loc = b->loc; loc; loc = loc->next)
+ target_download_tracepoint_loc (loc);
+
t->number_on_target = b->number;
}
VEC_free (breakpoint_p, tp_vec);
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 1/8] Download tracepoint on location level
2011-11-08 6:08 ` [patch 1/8] Download tracepoint on location level Yao Qi
@ 2011-11-09 3:34 ` Yao Qi
2011-11-10 14:54 ` Pedro Alves
0 siblings, 1 reply; 31+ messages in thread
From: Yao Qi @ 2011-11-09 3:34 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 483 bytes --]
On 11/08/2011 02:08 PM, Yao Qi wrote:
> This patch is to refactor to_download_tracepoint to
> to_download_tracepoint_loc. Beside this refactor, a return value is
> added in new hook to reflect whether tracepoint location is
> downloaded/installed successfully. Functionality of gdb is not changed.
I re-read this patch again, and find the return value is unnecessary,
because remote_download_tracepoint_loc executes successfully if no error
is thrown out.
--
Yao (é½å°§)
[-- Attachment #2: 0001-refactor-download_tracepoint-to-download_tracepoint_.patch --]
[-- Type: text/x-patch, Size: 15560 bytes --]
* target.h (struct target): Rename field to_download_tracepoint
to to_download_tracepoint_loc. Change type of parameter from
tracepoint bp_location.
* target.c (update_current_target): Update.
* tracepoint.c (start_tracing): Update.
* remote.c: Move remote_download_tracepoint. to
remote_download_tracepoint_loc. Remove loop for each location
of a tracepoint.
---
gdb/remote.c | 279 ++++++++++++++++++++++++++---------------------------
gdb/target.c | 6 +-
gdb/target.h | 10 +-
gdb/tracepoint.c | 6 +-
4 files changed, 151 insertions(+), 150 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index 5182ef1..5f7a466 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -9835,9 +9835,9 @@ remote_download_command_source (int num, ULONGEST addr,
}
static void
-remote_download_tracepoint (struct breakpoint *b)
+remote_download_tracepoint_loc (struct bp_location *loc)
{
- struct bp_location *loc;
+
CORE_ADDR tpaddr;
char addrbuf[40];
char buf[2048];
@@ -9848,169 +9848,164 @@ remote_download_tracepoint (struct breakpoint *b)
struct agent_expr *aexpr;
struct cleanup *aexpr_chain = NULL;
char *pkt;
+ struct breakpoint *b = loc->owner;
struct tracepoint *t = (struct tracepoint *) b;
- /* Iterate over all the tracepoint locations. It's up to the target to
- notice multiple tracepoint packets with the same number but different
- addresses, and treat them as multiple locations. */
- for (loc = b->loc; loc; loc = loc->next)
- {
- encode_actions (b, loc, &tdp_actions, &stepping_actions);
- old_chain = make_cleanup (free_actions_list_cleanup_wrapper,
- tdp_actions);
- (void) make_cleanup (free_actions_list_cleanup_wrapper,
- stepping_actions);
-
- tpaddr = loc->address;
- sprintf_vma (addrbuf, tpaddr);
- sprintf (buf, "QTDP:%x:%s:%c:%lx:%x", b->number,
- addrbuf, /* address */
- (b->enable_state == bp_enabled ? 'E' : 'D'),
- t->step_count, t->pass_count);
- /* Fast tracepoints are mostly handled by the target, but we can
- tell the target how big of an instruction block should be moved
- around. */
- if (b->type == bp_fast_tracepoint)
+ encode_actions (loc->owner, loc, &tdp_actions, &stepping_actions);
+ old_chain = make_cleanup (free_actions_list_cleanup_wrapper,
+ tdp_actions);
+ (void) make_cleanup (free_actions_list_cleanup_wrapper,
+ stepping_actions);
+
+ tpaddr = loc->address;
+ sprintf_vma (addrbuf, tpaddr);
+ sprintf (buf, "QTDP:%x:%s:%c:%lx:%x", b->number,
+ addrbuf, /* address */
+ (b->enable_state == bp_enabled ? 'E' : 'D'),
+ t->step_count, t->pass_count);
+ /* Fast tracepoints are mostly handled by the target, but we can
+ tell the target how big of an instruction block should be moved
+ around. */
+ if (b->type == bp_fast_tracepoint)
+ {
+ /* Only test for support at download time; we may not know
+ target capabilities at definition time. */
+ if (remote_supports_fast_tracepoints ())
{
- /* Only test for support at download time; we may not know
- target capabilities at definition time. */
- if (remote_supports_fast_tracepoints ())
- {
- int isize;
+ int isize;
- if (gdbarch_fast_tracepoint_valid_at (target_gdbarch,
- tpaddr, &isize, NULL))
- sprintf (buf + strlen (buf), ":F%x", isize);
- else
- /* If it passed validation at definition but fails now,
- something is very wrong. */
- internal_error (__FILE__, __LINE__,
- _("Fast tracepoint not "
- "valid during download"));
- }
+ if (gdbarch_fast_tracepoint_valid_at (target_gdbarch,
+ tpaddr, &isize, NULL))
+ sprintf (buf + strlen (buf), ":F%x", isize);
else
- /* Fast tracepoints are functionally identical to regular
- tracepoints, so don't take lack of support as a reason to
- give up on the trace run. */
- warning (_("Target does not support fast tracepoints, "
- "downloading %d as regular tracepoint"), b->number);
+ /* If it passed validation at definition but fails now,
+ something is very wrong. */
+ internal_error (__FILE__, __LINE__,
+ _("Fast tracepoint not "
+ "valid during download"));
}
- else if (b->type == bp_static_tracepoint)
+ else
+ /* Fast tracepoints are functionally identical to regular
+ tracepoints, so don't take lack of support as a reason to
+ give up on the trace run. */
+ warning (_("Target does not support fast tracepoints, "
+ "downloading %d as regular tracepoint"), b->number);
+ }
+ else if (b->type == bp_static_tracepoint)
+ {
+ /* Only test for support at download time; we may not know
+ target capabilities at definition time. */
+ if (remote_supports_static_tracepoints ())
{
- /* Only test for support at download time; we may not know
- target capabilities at definition time. */
- if (remote_supports_static_tracepoints ())
- {
- struct static_tracepoint_marker marker;
+ struct static_tracepoint_marker marker;
- if (target_static_tracepoint_marker_at (tpaddr, &marker))
- strcat (buf, ":S");
- else
- error (_("Static tracepoint not valid during download"));
- }
+ if (target_static_tracepoint_marker_at (tpaddr, &marker))
+ strcat (buf, ":S");
else
- /* Fast tracepoints are functionally identical to regular
- tracepoints, so don't take lack of support as a reason
- to give up on the trace run. */
- error (_("Target does not support static tracepoints"));
+ error (_("Static tracepoint not valid during download"));
}
- /* If the tracepoint has a conditional, make it into an agent
- expression and append to the definition. */
- if (loc->cond)
+ else
+ /* Fast tracepoints are functionally identical to regular
+ tracepoints, so don't take lack of support as a reason
+ to give up on the trace run. */
+ error (_("Target does not support static tracepoints"));
+ }
+ /* If the tracepoint has a conditional, make it into an agent
+ expression and append to the definition. */
+ if (loc->cond)
+ {
+ /* Only test support at download time, we may not know target
+ capabilities at definition time. */
+ if (remote_supports_cond_tracepoints ())
{
- /* Only test support at download time, we may not know target
- capabilities at definition time. */
- if (remote_supports_cond_tracepoints ())
- {
- aexpr = gen_eval_for_expr (tpaddr, loc->cond);
- aexpr_chain = make_cleanup_free_agent_expr (aexpr);
- sprintf (buf + strlen (buf), ":X%x,", aexpr->len);
- pkt = buf + strlen (buf);
- for (ndx = 0; ndx < aexpr->len; ++ndx)
- pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
- *pkt = '\0';
- do_cleanups (aexpr_chain);
- }
- else
- warning (_("Target does not support conditional tracepoints, "
- "ignoring tp %d cond"), b->number);
+ aexpr = gen_eval_for_expr (tpaddr, loc->cond);
+ aexpr_chain = make_cleanup_free_agent_expr (aexpr);
+ sprintf (buf + strlen (buf), ":X%x,", aexpr->len);
+ pkt = buf + strlen (buf);
+ for (ndx = 0; ndx < aexpr->len; ++ndx)
+ pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
+ *pkt = '\0';
+ do_cleanups (aexpr_chain);
}
+ else
+ warning (_("Target does not support conditional tracepoints, "
+ "ignoring tp %d cond"), b->number);
+ }
if (b->commands || *default_collect)
- strcat (buf, "-");
- putpkt (buf);
- remote_get_noisy_reply (&target_buf, &target_buf_size);
- if (strcmp (target_buf, "OK"))
- error (_("Target does not support tracepoints."));
+ strcat (buf, "-");
+ putpkt (buf);
+ remote_get_noisy_reply (&target_buf, &target_buf_size);
+ if (strcmp (target_buf, "OK"))
+ error (_("Target does not support tracepoints."));
- /* do_single_steps (t); */
- if (tdp_actions)
+ /* do_single_steps (t); */
+ if (tdp_actions)
+ {
+ for (ndx = 0; tdp_actions[ndx]; ndx++)
{
- for (ndx = 0; tdp_actions[ndx]; ndx++)
- {
- QUIT; /* Allow user to bail out with ^C. */
- sprintf (buf, "QTDP:-%x:%s:%s%c",
- b->number, addrbuf, /* address */
- tdp_actions[ndx],
- ((tdp_actions[ndx + 1] || stepping_actions)
- ? '-' : 0));
- putpkt (buf);
- remote_get_noisy_reply (&target_buf,
- &target_buf_size);
- if (strcmp (target_buf, "OK"))
- error (_("Error on target while setting tracepoints."));
- }
+ QUIT; /* Allow user to bail out with ^C. */
+ sprintf (buf, "QTDP:-%x:%s:%s%c",
+ b->number, addrbuf, /* address */
+ tdp_actions[ndx],
+ ((tdp_actions[ndx + 1] || stepping_actions)
+ ? '-' : 0));
+ putpkt (buf);
+ remote_get_noisy_reply (&target_buf,
+ &target_buf_size);
+ if (strcmp (target_buf, "OK"))
+ error (_("Error on target while setting tracepoints."));
}
- if (stepping_actions)
+ }
+ if (stepping_actions)
+ {
+ for (ndx = 0; stepping_actions[ndx]; ndx++)
{
- for (ndx = 0; stepping_actions[ndx]; ndx++)
- {
- QUIT; /* Allow user to bail out with ^C. */
- sprintf (buf, "QTDP:-%x:%s:%s%s%s",
- b->number, addrbuf, /* address */
- ((ndx == 0) ? "S" : ""),
- stepping_actions[ndx],
- (stepping_actions[ndx + 1] ? "-" : ""));
- putpkt (buf);
- remote_get_noisy_reply (&target_buf,
- &target_buf_size);
- if (strcmp (target_buf, "OK"))
- error (_("Error on target while setting tracepoints."));
- }
+ QUIT; /* Allow user to bail out with ^C. */
+ sprintf (buf, "QTDP:-%x:%s:%s%s%s",
+ b->number, addrbuf, /* address */
+ ((ndx == 0) ? "S" : ""),
+ stepping_actions[ndx],
+ (stepping_actions[ndx + 1] ? "-" : ""));
+ putpkt (buf);
+ remote_get_noisy_reply (&target_buf,
+ &target_buf_size);
+ if (strcmp (target_buf, "OK"))
+ error (_("Error on target while setting tracepoints."));
}
+ }
- if (remote_protocol_packets[PACKET_TracepointSource].support
- == PACKET_ENABLE)
+ if (remote_protocol_packets[PACKET_TracepointSource].support
+ == PACKET_ENABLE)
+ {
+ if (b->addr_string)
{
- if (b->addr_string)
- {
- strcpy (buf, "QTDPsrc:");
- encode_source_string (b->number, loc->address,
- "at", b->addr_string, buf + strlen (buf),
- 2048 - strlen (buf));
+ strcpy (buf, "QTDPsrc:");
+ encode_source_string (b->number, loc->address,
+ "at", b->addr_string, buf + strlen (buf),
+ 2048 - strlen (buf));
- putpkt (buf);
- remote_get_noisy_reply (&target_buf, &target_buf_size);
- if (strcmp (target_buf, "OK"))
- warning (_("Target does not support source download."));
- }
- if (b->cond_string)
- {
- strcpy (buf, "QTDPsrc:");
- encode_source_string (b->number, loc->address,
- "cond", b->cond_string, buf + strlen (buf),
- 2048 - strlen (buf));
- putpkt (buf);
- remote_get_noisy_reply (&target_buf, &target_buf_size);
- if (strcmp (target_buf, "OK"))
- warning (_("Target does not support source download."));
- }
- remote_download_command_source (b->number, loc->address,
- breakpoint_commands (b));
+ putpkt (buf);
+ remote_get_noisy_reply (&target_buf, &target_buf_size);
+ if (strcmp (target_buf, "OK"))
+ warning (_("Target does not support source download."));
}
-
- do_cleanups (old_chain);
+ if (b->cond_string)
+ {
+ strcpy (buf, "QTDPsrc:");
+ encode_source_string (b->number, loc->address,
+ "cond", b->cond_string, buf + strlen (buf),
+ 2048 - strlen (buf));
+ putpkt (buf);
+ remote_get_noisy_reply (&target_buf, &target_buf_size);
+ if (strcmp (target_buf, "OK"))
+ warning (_("Target does not support source download."));
+ }
+ remote_download_command_source (b->number, loc->address,
+ breakpoint_commands (b));
}
+
+ do_cleanups (old_chain);
}
static void
@@ -10484,7 +10479,7 @@ Specify the serial device it is connected to\n\
remote_ops.to_supports_enable_disable_tracepoint = remote_supports_enable_disable_tracepoint;
remote_ops.to_supports_string_tracing = remote_supports_string_tracing;
remote_ops.to_trace_init = remote_trace_init;
- remote_ops.to_download_tracepoint = remote_download_tracepoint;
+ remote_ops.to_download_tracepoint_loc = remote_download_tracepoint_loc;
remote_ops.to_download_trace_state_variable
= remote_download_trace_state_variable;
remote_ops.to_enable_tracepoint = remote_enable_tracepoint;
diff --git a/gdb/target.c b/gdb/target.c
index 41ff6cf..c706f1d 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -674,7 +674,7 @@ update_current_target (void)
INHERIT (to_supports_enable_disable_tracepoint, t);
INHERIT (to_supports_string_tracing, t);
INHERIT (to_trace_init, t);
- INHERIT (to_download_tracepoint, t);
+ INHERIT (to_download_tracepoint_loc, t);
INHERIT (to_download_trace_state_variable, t);
INHERIT (to_enable_tracepoint, t);
INHERIT (to_disable_tracepoint, t);
@@ -847,8 +847,8 @@ update_current_target (void)
de_fault (to_trace_init,
(void (*) (void))
tcomplain);
- de_fault (to_download_tracepoint,
- (void (*) (struct breakpoint *))
+ de_fault (to_download_tracepoint_loc,
+ (void (*) (struct bp_location *))
tcomplain);
de_fault (to_download_trace_state_variable,
(void (*) (struct trace_state_variable *))
diff --git a/gdb/target.h b/gdb/target.h
index 352f2df..074515a 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -686,8 +686,10 @@ struct target_ops
/* Prepare the target for a tracing run. */
void (*to_trace_init) (void);
- /* Send full details of a tracepoint to the target. */
- void (*to_download_tracepoint) (struct breakpoint *t);
+ /* Send full details of a tracepoint location to the target. Target may
+ install or not install tracepoint locations to inferior, which is
+ determined by target's factors, such tracing state. */
+ void (*to_download_tracepoint_loc) (struct bp_location *location);
/* Send full details of a trace state variable to the target. */
void (*to_download_trace_state_variable) (struct trace_state_variable *tsv);
@@ -1474,8 +1476,8 @@ extern int target_search_memory (CORE_ADDR start_addr,
#define target_trace_init() \
(*current_target.to_trace_init) ()
-#define target_download_tracepoint(t) \
- (*current_target.to_download_tracepoint) (t)
+#define target_download_tracepoint_loc(t) \
+ (*current_target.to_download_tracepoint_loc) (t)
#define target_download_trace_state_variable(tsv) \
(*current_target.to_download_trace_state_variable) (tsv)
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 4ca4ec2..493d324 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1701,6 +1701,7 @@ start_tracing (void)
for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
{
struct tracepoint *t = (struct tracepoint *) b;
+ struct bp_location *loc;
if ((b->type == bp_fast_tracepoint
? !may_insert_fast_tracepoints
@@ -1708,7 +1709,10 @@ start_tracing (void)
continue;
t->number_on_target = 0;
- target_download_tracepoint (b);
+
+ for (loc = b->loc; loc; loc = loc->next)
+ target_download_tracepoint_loc (loc);
+
t->number_on_target = b->number;
}
VEC_free (breakpoint_p, tp_vec);
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 1/8] Download tracepoint on location level
2011-11-09 3:34 ` Yao Qi
@ 2011-11-10 14:54 ` Pedro Alves
0 siblings, 0 replies; 31+ messages in thread
From: Pedro Alves @ 2011-11-10 14:54 UTC (permalink / raw)
To: gdb-patches; +Cc: Yao Qi
On Wednesday 09 November 2011 03:34:01, Yao Qi wrote:
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -674,7 +674,7 @@ update_current_target (void)
> INHERIT (to_supports_enable_disable_tracepoint, t);
> INHERIT (to_supports_string_tracing, t);
> INHERIT (to_trace_init, t);
> - INHERIT (to_download_tracepoint, t);
> + INHERIT (to_download_tracepoint_loc, t);
Please drop the _loc from the function name. We don't have it
in the other methods like to_enable_tracepoint/to_disable_tracepoint
which take a location too, and others like to_insert_breakpoint
aren't called to_insert_breakpoint_gdbarch_target_info either.
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -9835,9 +9835,9 @@ remote_download_command_source (int num, ULONGEST addr,
> }
>
> static void
> -remote_download_tracepoint (struct breakpoint *b)
> +remote_download_tracepoint_loc (struct bp_location *loc)
> {
Ditto.
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -686,8 +686,10 @@ struct target_ops
> /* Prepare the target for a tracing run. */
> void (*to_trace_init) (void);
>
> - /* Send full details of a tracepoint to the target. */
> - void (*to_download_tracepoint) (struct breakpoint *t);
> + /* Send full details of a tracepoint location to the target. Target may
> + install or not install tracepoint locations to inferior, which is
> + determined by target's factors, such tracing state. */
This comment doesn't belong in this patch. And I think it only
made sense on a previous version of the patch series.
> + void (*to_download_tracepoint_loc) (struct bp_location *location);
>
> /* Send full details of a trace state variable to the target. */
> void (*to_download_trace_state_variable) (struct trace_state_variable *tsv);
> @@ -1474,8 +1476,8 @@ extern int target_search_memory (CORE_ADDR start_addr,
> #define target_trace_init() \
> (*current_target.to_trace_init) ()
>
> -#define target_download_tracepoint(t) \
> - (*current_target.to_download_tracepoint) (t)
> +#define target_download_tracepoint_loc(t) \
> + (*current_target.to_download_tracepoint_loc) (t)
Drop _loc.
Okay with those changes.
--
Pedro Alves
^ permalink raw reply [flat|nested] 31+ messages in thread
* [patch 2/8] New remote feature InstallInTrace
2011-11-08 6:00 [patch 0/8] Download tracepoint locations when tracing is running Yao Qi
2011-11-08 6:08 ` [patch 1/8] Download tracepoint on location level Yao Qi
@ 2011-11-08 6:12 ` Yao Qi
2011-11-10 15:03 ` Pedro Alves
2011-11-08 6:21 ` [patch 3/8] New target hook `to_can_download_tracepoint_loc' Yao Qi
` (5 subsequent siblings)
7 siblings, 1 reply; 31+ messages in thread
From: Yao Qi @ 2011-11-08 6:12 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 228 bytes --]
This patch is about adding a new remote feature `InstallInTrace' which
indicates remote stubs supports downloading/installing tracepoint
locations while tracing is running.
It will be used in patch 4/8.
--
Yao (é½å°§)
[-- Attachment #2: 0002-install-in-trace-feature.patch --]
[-- Type: text/x-patch, Size: 2959 bytes --]
* remote.c (struct remote_state): <install_in_trace> new field.
(PACKET_InstallInTrace): New enum value.
(remote_install_in_trace_feature): Support InstallInTrace.
(remote_supports_install_in_trace): Likewise.
(remote_protocol_features): Likewise.
(_initialize_remote): Likewise.
---
gdb/remote.c | 28 ++++++++++++++++++++++++++++
1 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index 1c43f39..c34d47b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -323,6 +323,10 @@ struct remote_state
/* True if the stub reports support for static tracepoints. */
int static_tracepoints;
+ /* True if the stub reports support for installing tracepoint while
+ tracing. */
+ int install_in_trace;
+
/* True if the stub can continue running a trace while GDB is
disconnected. */
int disconnected_tracing;
@@ -1261,6 +1265,7 @@ enum {
PACKET_ConditionalTracepoints,
PACKET_FastTracepoints,
PACKET_StaticTracepoints,
+ PACKET_InstallInTrace,
PACKET_bc,
PACKET_bs,
PACKET_TracepointSource,
@@ -3696,6 +3701,16 @@ remote_static_tracepoint_feature (const struct protocol_feature *feature,
}
static void
+remote_install_in_trace_feature (const struct protocol_feature *feature,
+ enum packet_support support,
+ const char *value)
+{
+ struct remote_state *rs = get_remote_state ();
+
+ rs->install_in_trace = (support == PACKET_ENABLE);
+}
+
+static void
remote_disconnected_tracing_feature (const struct protocol_feature *feature,
enum packet_support support,
const char *value)
@@ -3761,6 +3776,8 @@ static struct protocol_feature remote_protocol_features[] = {
PACKET_FastTracepoints },
{ "StaticTracepoints", PACKET_DISABLE, remote_static_tracepoint_feature,
PACKET_StaticTracepoints },
+ {"InstallInTrace", PACKET_DISABLE, remote_install_in_trace_feature,
+ PACKET_InstallInTrace},
{ "DisconnectedTracing", PACKET_DISABLE, remote_disconnected_tracing_feature,
-1 },
{ "ReverseContinue", PACKET_DISABLE, remote_supported_packet,
@@ -9748,6 +9765,14 @@ remote_supports_static_tracepoints (void)
}
static int
+remote_supports_install_in_trace (void)
+{
+ struct remote_state *rs = get_remote_state ();
+
+ return rs->install_in_trace;
+}
+
+static int
remote_supports_enable_disable_tracepoint (void)
{
struct remote_state *rs = get_remote_state ();
@@ -11000,6 +11025,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
add_packet_config_cmd (&remote_protocol_packets[PACKET_StaticTracepoints],
"StaticTracepoints", "static-tracepoints", 0);
+ add_packet_config_cmd (&remote_protocol_packets[PACKET_InstallInTrace],
+ "InstallInTrace", "install-in-trace", 0);
+
add_packet_config_cmd (&remote_protocol_packets[PACKET_qXfer_statictrace_read],
"qXfer:statictrace:read", "read-sdata-object", 0);
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* [patch 3/8] New target hook `to_can_download_tracepoint_loc'
2011-11-08 6:00 [patch 0/8] Download tracepoint locations when tracing is running Yao Qi
2011-11-08 6:08 ` [patch 1/8] Download tracepoint on location level Yao Qi
2011-11-08 6:12 ` [patch 2/8] New remote feature InstallInTrace Yao Qi
@ 2011-11-08 6:21 ` Yao Qi
2011-11-10 15:11 ` Pedro Alves
2011-11-08 6:30 ` [patch 4/8] Download tracepoint locations and track its status Yao Qi
` (4 subsequent siblings)
7 siblings, 1 reply; 31+ messages in thread
From: Yao Qi @ 2011-11-08 6:21 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 371 bytes --]
This patch is to add a new target hook 'to_can_download_tracepoint_loc'
to determine whether it is OK for remote target to receive tracepoint
locations.
Note that, `download_tracepoint_loc' itself can check this in lower
level, but it is not efficient, and it leaves managing location's status
to lower level.
This patch is needed by patch 4/8.
--
Yao (é½å°§)
[-- Attachment #2: 0003-new-target-hook-can_download_tracepoint_loc.patch --]
[-- Type: text/x-patch, Size: 3450 bytes --]
* target.h (struct target): New field
`to_can_download_tracepoint_loc'.
(target_can_download_tracepoint_loc): New macro.
* target.c (update_current_target): Update.
* remote.c (remote_can_download_tracepoint_loc): New.
---
gdb/remote.c | 19 +++++++++++++++++++
gdb/target.c | 4 ++++
gdb/target.h | 7 +++++++
3 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index c34d47b..5ba497a 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10036,6 +10036,24 @@ remote_download_tracepoint_loc (struct bp_location *loc)
}
+static int
+remote_can_download_tracepoint_loc ()
+{
+ struct trace_status *ts = current_trace_status ();
+ int status = remote_get_trace_status (ts);
+
+ if (status == -1 || !ts->running_known || !ts->running)
+ return 0;
+
+ /* If we are in a tracing experiment, but remote stub doesn't support
+ installing tracepoint in trace, we have to return. */
+ if (!remote_supports_install_in_trace ())
+ return 0;
+
+ return 1;
+}
+
+
static void
remote_download_trace_state_variable (struct trace_state_variable *tsv)
{
@@ -10508,6 +10526,7 @@ Specify the serial device it is connected to\n\
remote_ops.to_supports_string_tracing = remote_supports_string_tracing;
remote_ops.to_trace_init = remote_trace_init;
remote_ops.to_download_tracepoint_loc = remote_download_tracepoint_loc;
+ remote_ops.to_can_download_tracepoint_loc = remote_can_download_tracepoint_loc;
remote_ops.to_download_trace_state_variable
= remote_download_trace_state_variable;
remote_ops.to_enable_tracepoint = remote_enable_tracepoint;
diff --git a/gdb/target.c b/gdb/target.c
index 48cd888..d236530 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -675,6 +675,7 @@ update_current_target (void)
INHERIT (to_supports_string_tracing, t);
INHERIT (to_trace_init, t);
INHERIT (to_download_tracepoint_loc, t);
+ INHERIT (to_can_download_tracepoint_loc, t);
INHERIT (to_download_trace_state_variable, t);
INHERIT (to_enable_tracepoint, t);
INHERIT (to_disable_tracepoint, t);
@@ -850,6 +851,9 @@ update_current_target (void)
de_fault (to_download_tracepoint_loc,
(int (*) (struct bp_location *))
tcomplain);
+ de_fault (to_can_download_tracepoint_loc,
+ (int (*) (void))
+ return_zero);
de_fault (to_download_trace_state_variable,
(void (*) (struct trace_state_variable *))
tcomplain);
diff --git a/gdb/target.h b/gdb/target.h
index 74e1d8a..69a2a32 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -693,6 +693,10 @@ struct target_ops
otherwise return 1. */
int (*to_download_tracepoint_loc) (struct bp_location *location);
+ /* Can the target be able to download tracepoint locations in current
+ status? */
+ int (*to_can_download_tracepoint_loc) (void);
+
/* Send full details of a trace state variable to the target. */
void (*to_download_trace_state_variable) (struct trace_state_variable *tsv);
@@ -1481,6 +1485,9 @@ extern int target_search_memory (CORE_ADDR start_addr,
#define target_download_tracepoint_loc(t) \
(*current_target.to_download_tracepoint_loc) (t)
+#define target_can_download_tracepoint_loc() \
+ (*current_target.to_can_download_tracepoint_loc) ()
+
#define target_download_trace_state_variable(tsv) \
(*current_target.to_download_trace_state_variable) (tsv)
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 3/8] New target hook `to_can_download_tracepoint_loc'
2011-11-08 6:21 ` [patch 3/8] New target hook `to_can_download_tracepoint_loc' Yao Qi
@ 2011-11-10 15:11 ` Pedro Alves
0 siblings, 0 replies; 31+ messages in thread
From: Pedro Alves @ 2011-11-10 15:11 UTC (permalink / raw)
To: gdb-patches; +Cc: Yao Qi
On Tuesday 08 November 2011 06:21:11, Yao Qi wrote:
> This patch is to add a new target hook 'to_can_download_tracepoint_loc'
> to determine whether it is OK for remote target to receive tracepoint
> locations.
Please drop the _loc from this one too.
> 0003-new-target-hook-can_download_tracepoint_loc.patch
> * target.h (struct target): New field
> `to_can_download_tracepoint_loc'.
Note tabs vs spaces. The log entries should be indented with
tabs.
> +static int
> +remote_can_download_tracepoint_loc ()
(void)
> + /* Can the target be able to download tracepoint locations in current
> + status? */
Is the target able to ... in the current state
> + int (*to_can_download_tracepoint_loc) (void);
Okay with those changes.
--
Pedro Alves
^ permalink raw reply [flat|nested] 31+ messages in thread
* [patch 4/8] Download tracepoint locations and track its status
2011-11-08 6:00 [patch 0/8] Download tracepoint locations when tracing is running Yao Qi
` (2 preceding siblings ...)
2011-11-08 6:21 ` [patch 3/8] New target hook `to_can_download_tracepoint_loc' Yao Qi
@ 2011-11-08 6:30 ` Yao Qi
2011-11-09 3:47 ` Yao Qi
2011-11-08 6:33 ` [patch 5/8] refactor gdbserver on installing fast tracepoint Yao Qi
` (3 subsequent siblings)
7 siblings, 1 reply; 31+ messages in thread
From: Yao Qi @ 2011-11-08 6:30 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 676 bytes --]
This is the major part in gdb side of this patch set. This patch is to
make use of `inserted' field in bp_location to track whether this
tracepoint location has been downloaded. This bit is quite similar to
what we are doing to breakpoint.
The difference on breakpoint locations and tracepoint locations is about
`duplicate' field in bp_location, because on the same address, there can
be multiple instances of bp_location which have different trace actions,
so we can't treat one duplicates the other. In current implementation,
we treat tracepoint location never duplicates with other tracepoint
locations and locations of other breakpoint type.
--
Yao (é½å°§)
[-- Attachment #2: 0004-tracepoint-change-loc.patch --]
[-- Type: text/x-patch, Size: 9879 bytes --]
* breakpoint.h (struct bp_location): Add comment on field
`duplicate'.
* breakpoint.c (should_be_inserted): Don't differentiate breakpoint
and tracepoint.
(remove_breakpoints): Don't remove tracepoints.
(disable_breakpoints_in_unloaded_shlib): Handle tracepoint.
(download_tracepoint_locations): New.
(update_global_location_list): Call it. Consider bp's type when
swap bp_location.
* tracepoint.c (find_matching_tracepoint): Delete.
(find_matching_tracepoint_location): Renamed from
find_matching_tracepoint. Return bp_location rather than
tracepoint.
(merge_uploaded_tracepoints): Set `inserted' field to 1 if
tracepoint is found.
---
gdb/breakpoint.c | 90 +++++++++++++++++++++++++++++++++++++++++++----------
gdb/breakpoint.h | 8 ++++-
gdb/tracepoint.c | 43 ++++++++++++++++++-------
3 files changed, 111 insertions(+), 30 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8c98bef..4672cea 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1555,7 +1555,10 @@ in which its expression is valid.\n"),
/* Returns 1 iff breakpoint location should be
- inserted in the inferior. */
+ inserted in the inferior. We don't differentiate the type of BL's owner
+ (breakpoint vs. tracepoint), although insert_location in tracepoint's
+ breakpoint_ops is not defined, because in insert_bp_location,
+ tracepoint's insert_location will not be called. */
static int
should_be_inserted (struct bp_location *bl)
{
@@ -1579,11 +1582,6 @@ should_be_inserted (struct bp_location *bl)
if (bl->pspace->breakpoints_not_allowed)
return 0;
- /* Tracepoints are inserted by the target at a time of its choosing,
- not by us. */
- if (is_tracepoint (bl->owner))
- return 0;
-
return 1;
}
@@ -2049,7 +2047,7 @@ remove_breakpoints (void)
ALL_BP_LOCATIONS (bl, blp_tmp)
{
- if (bl->inserted)
+ if (bl->inserted && !is_tracepoint (bl->owner))
val |= remove_breakpoint (bl, mark_uninserted);
}
return val;
@@ -6071,8 +6069,8 @@ disable_breakpoints_in_shlibs (void)
}
}
-/* Disable any breakpoints that are in an unloaded shared library.
- Only apply to enabled breakpoints, disabled ones can just stay
+/* Disable any breakpoints and tracepoints that are in an unloaded shared
+ library. Only apply to enabled breakpoints, disabled ones can just stay
disabled. */
static void
@@ -6094,13 +6092,14 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
/* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL. */
struct breakpoint *b = loc->owner;
- if ((loc->loc_type == bp_loc_hardware_breakpoint
- || loc->loc_type == bp_loc_software_breakpoint)
- && solib->pspace == loc->pspace
+ if (solib->pspace == loc->pspace
&& !loc->shlib_disabled
- && (b->type == bp_breakpoint
- || b->type == bp_jit_event
- || b->type == bp_hardware_breakpoint)
+ && (((b->type == bp_breakpoint
+ || b->type == bp_jit_event
+ || b->type == bp_hardware_breakpoint)
+ && (loc->loc_type == bp_loc_hardware_breakpoint
+ || loc->loc_type == bp_loc_software_breakpoint))
+ || is_tracepoint (b))
&& solib_contains_address_p (solib, loc->address))
{
loc->shlib_disabled = 1;
@@ -10325,6 +10324,50 @@ bp_location_target_extensions_update (void)
}
}
+/* Download tracepoint locations if they haven't been. */
+
+static void
+download_tracepoint_locations (void)
+{
+ struct bp_location *bl, **blp_tmp;
+ struct cleanup *old_chain;
+
+ if (!target_can_download_tracepoint_loc ())
+ return;
+
+ old_chain = save_current_space_and_thread ();
+
+ ALL_BP_LOCATIONS (bl, blp_tmp)
+ {
+ if (!is_tracepoint (bl->owner))
+ continue;
+
+ if ((bl->owner->type == bp_fast_tracepoint
+ ? !may_insert_fast_tracepoints
+ : !may_insert_tracepoints))
+ continue;
+
+ /* In tracepoint, locations are _never_ duplicated, so
+ should_be_inserted is equivalent to
+ unduplicated_should_be_inserted. */
+ if (!should_be_inserted (bl) || bl->inserted)
+ continue;
+
+ switch_to_program_space_and_thread (bl->pspace);
+
+ if (target_download_tracepoint_loc (bl) == 0)
+ {
+ struct tracepoint *t;
+
+ bl->inserted = 1;
+ t = (struct tracepoint *) bl->owner;
+ t->number_on_target = bl->owner->number;
+ }
+ }
+
+ do_cleanups (old_chain);
+}
+
/* Swap the insertion/duplication state between two locations. */
static void
@@ -10334,6 +10377,12 @@ swap_insertion (struct bp_location *left, struct bp_location *right)
const int left_duplicate = left->duplicate;
const struct bp_target_info left_target_info = left->target_info;
+ /* Locations of tracepoints never be duplicated. */
+ if (is_tracepoint (left->owner))
+ gdb_assert (left->duplicate == 0);
+ if (is_tracepoint (right->owner))
+ gdb_assert (right->duplicate == 0);
+
left->inserted = right->inserted;
left->duplicate = right->duplicate;
left->target_info = right->target_info;
@@ -10424,7 +10473,7 @@ update_global_location_list (int should_insert)
int keep_in_target = 0;
int removed = 0;
- /* Skip LOCP entries which will definitely never be needed.
+ /* skip LOCP entries which will definitely never be needed.
Stop either at or being the one matching OLD_LOC. */
while (locp < bp_location + bp_location_count
&& (*locp)->address < old_loc->address)
@@ -10491,7 +10540,9 @@ update_global_location_list (int should_insert)
if it should be inserted in case it will be
unduplicated. */
if (loc2 != old_loc
- && unduplicated_should_be_inserted (loc2))
+ && unduplicated_should_be_inserted (loc2)
+ && (is_tracepoint (old_loc->owner)
+ == is_tracepoint (loc2->owner)))
{
swap_insertion (old_loc, loc2);
keep_in_target = 1;
@@ -10613,6 +10664,9 @@ update_global_location_list (int should_insert)
|| !loc->enabled
|| loc->shlib_disabled
|| !breakpoint_address_is_meaningful (b)
+ /* Don't detect duplicate for tracepoint locations because they are
+ never duplicated. See the comments in field `duplicate' of
+ `struct bp_location'. */
|| is_tracepoint (b))
continue;
@@ -10660,6 +10714,8 @@ update_global_location_list (int should_insert)
|| (gdbarch_has_global_breakpoints (target_gdbarch))))
insert_breakpoint_locations ();
+ download_tracepoint_locations ();
+
do_cleanups (cleanups);
}
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index fe381df..a1aa9ea 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -335,7 +335,13 @@ struct bp_location
char inserted;
/* Nonzero if this is not the first breakpoint in the list
- for the given address. */
+ for the given address. In current implementation, location
+ of tracepoint _never_ be duplicated with other locations of
+ tracepoints and other kinds of breakpoints, because two locations
+ at the same address may have different actions, so both of
+ these locations should be downloaded. It is possible that
+ two locations have the same address and actions, and they can
+ be treated as `duplicated', but we didn't do it in code. */
char duplicate;
/* If we someday support real thread-specific breakpoints, then
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 493d324..504178c 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1711,7 +1711,19 @@ start_tracing (void)
t->number_on_target = 0;
for (loc = b->loc; loc; loc = loc->next)
- target_download_tracepoint_loc (loc);
+ {
+ int ret;
+
+ /* Since tracepoint locations are never duplicated, `inserted'
+ flag should be zero. */
+ gdb_assert (loc->inserted == 0);
+
+ ret = target_download_tracepoint_loc (loc);
+ /* It should be always sucessfull to download tracepoint
+ locations. */
+ gdb_assert (ret == 0);
+ loc->inserted = 1;
+ }
t->number_on_target = b->number;
}
@@ -3203,10 +3215,10 @@ cond_string_is_same (char *str1, char *str2)
/* Look for an existing tracepoint that seems similar enough to the
uploaded one. Enablement isn't compared, because the user can
toggle that freely, and may have done so in anticipation of the
- next trace run. */
+ next trace run. Return the location of matched tracepoint. */
-struct tracepoint *
-find_matching_tracepoint (struct uploaded_tp *utp)
+struct bp_location *
+find_matching_tracepoint_location (struct uploaded_tp *utp)
{
VEC(breakpoint_p) *tp_vec = all_tracepoints ();
int ix;
@@ -3228,7 +3240,7 @@ find_matching_tracepoint (struct uploaded_tp *utp)
for (loc = b->loc; loc; loc = loc->next)
{
if (loc->address == utp->addr)
- return t;
+ return loc;
}
}
}
@@ -3243,17 +3255,24 @@ void
merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
{
struct uploaded_tp *utp;
- struct tracepoint *t;
/* Look for GDB tracepoints that match up with our uploaded versions. */
for (utp = *uploaded_tps; utp; utp = utp->next)
{
- t = find_matching_tracepoint (utp);
- if (t)
- printf_filtered (_("Assuming tracepoint %d is same "
- "as target's tracepoint %d at %s.\n"),
- t->base.number, utp->number,
- paddress (get_current_arch (), utp->addr));
+ struct bp_location *loc;
+ struct tracepoint *t;
+
+ loc = find_matching_tracepoint_location (utp);
+ if (loc)
+ {
+ /* Mark this location as already inserted. */
+ loc->inserted = 1;
+ t = (struct tracepoint *) loc->owner;
+ printf_filtered (_("Assuming tracepoint %d is same "
+ "as target's tracepoint %d at %s.\n"),
+ loc->owner->number, utp->number,
+ paddress (loc->gdbarch, utp->addr));
+ }
else
{
t = create_tracepoint_from_upload (utp);
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 4/8] Download tracepoint locations and track its status
2011-11-08 6:30 ` [patch 4/8] Download tracepoint locations and track its status Yao Qi
@ 2011-11-09 3:47 ` Yao Qi
2011-11-10 15:51 ` Pedro Alves
0 siblings, 1 reply; 31+ messages in thread
From: Yao Qi @ 2011-11-09 3:47 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 132 bytes --]
This is a updated version to reflect the change in return value of
to_download_tracepoint_loc in patch 1/8.
--
Yao (é½å°§)
[-- Attachment #2: 0004-tracepoint-change-loc.patch --]
[-- Type: text/x-patch, Size: 9744 bytes --]
* breakpoint.h (struct bp_location): Add comment on field
`duplicate'.
* breakpoint.c (should_be_inserted): Don't differentiate breakpoint
and tracepoint.
(remove_breakpoints): Don't remove tracepoints.
(disable_breakpoints_in_unloaded_shlib): Handle tracepoint.
(download_tracepoint_locations): New.
(update_global_location_list): Call it. Consider bp's type when
swap bp_location.
* tracepoint.c (find_matching_tracepoint): Delete.
(find_matching_tracepoint_location): Renamed from
find_matching_tracepoint. Return bp_location rather than
tracepoint.
(merge_uploaded_tracepoints): Set `inserted' field to 1 if
tracepoint is found.
---
gdb/breakpoint.c | 89 +++++++++++++++++++++++++++++++++++++++++++----------
gdb/breakpoint.h | 8 ++++-
gdb/tracepoint.c | 39 ++++++++++++++++-------
3 files changed, 106 insertions(+), 30 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8c98bef..7444b38 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1555,7 +1555,10 @@ in which its expression is valid.\n"),
/* Returns 1 iff breakpoint location should be
- inserted in the inferior. */
+ inserted in the inferior. We don't differentiate the type of BL's owner
+ (breakpoint vs. tracepoint), although insert_location in tracepoint's
+ breakpoint_ops is not defined, because in insert_bp_location,
+ tracepoint's insert_location will not be called. */
static int
should_be_inserted (struct bp_location *bl)
{
@@ -1579,11 +1582,6 @@ should_be_inserted (struct bp_location *bl)
if (bl->pspace->breakpoints_not_allowed)
return 0;
- /* Tracepoints are inserted by the target at a time of its choosing,
- not by us. */
- if (is_tracepoint (bl->owner))
- return 0;
-
return 1;
}
@@ -2049,7 +2047,7 @@ remove_breakpoints (void)
ALL_BP_LOCATIONS (bl, blp_tmp)
{
- if (bl->inserted)
+ if (bl->inserted && !is_tracepoint (bl->owner))
val |= remove_breakpoint (bl, mark_uninserted);
}
return val;
@@ -6071,8 +6069,8 @@ disable_breakpoints_in_shlibs (void)
}
}
-/* Disable any breakpoints that are in an unloaded shared library.
- Only apply to enabled breakpoints, disabled ones can just stay
+/* Disable any breakpoints and tracepoints that are in an unloaded shared
+ library. Only apply to enabled breakpoints, disabled ones can just stay
disabled. */
static void
@@ -6094,13 +6092,14 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
/* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL. */
struct breakpoint *b = loc->owner;
- if ((loc->loc_type == bp_loc_hardware_breakpoint
- || loc->loc_type == bp_loc_software_breakpoint)
- && solib->pspace == loc->pspace
+ if (solib->pspace == loc->pspace
&& !loc->shlib_disabled
- && (b->type == bp_breakpoint
- || b->type == bp_jit_event
- || b->type == bp_hardware_breakpoint)
+ && (((b->type == bp_breakpoint
+ || b->type == bp_jit_event
+ || b->type == bp_hardware_breakpoint)
+ && (loc->loc_type == bp_loc_hardware_breakpoint
+ || loc->loc_type == bp_loc_software_breakpoint))
+ || is_tracepoint (b))
&& solib_contains_address_p (solib, loc->address))
{
loc->shlib_disabled = 1;
@@ -10325,6 +10324,49 @@ bp_location_target_extensions_update (void)
}
}
+/* Download tracepoint locations if they haven't been. */
+
+static void
+download_tracepoint_locations (void)
+{
+ struct bp_location *bl, **blp_tmp;
+ struct cleanup *old_chain;
+
+ if (!target_can_download_tracepoint_loc ())
+ return;
+
+ old_chain = save_current_space_and_thread ();
+
+ ALL_BP_LOCATIONS (bl, blp_tmp)
+ {
+ struct tracepoint *t;
+
+ if (!is_tracepoint (bl->owner))
+ continue;
+
+ if ((bl->owner->type == bp_fast_tracepoint
+ ? !may_insert_fast_tracepoints
+ : !may_insert_tracepoints))
+ continue;
+
+ /* In tracepoint, locations are _never_ duplicated, so
+ should_be_inserted is equivalent to
+ unduplicated_should_be_inserted. */
+ if (!should_be_inserted (bl) || bl->inserted)
+ continue;
+
+ switch_to_program_space_and_thread (bl->pspace);
+
+ target_download_tracepoint_loc (bl);
+
+ bl->inserted = 1;
+ t = (struct tracepoint *) bl->owner;
+ t->number_on_target = bl->owner->number;
+ }
+
+ do_cleanups (old_chain);
+}
+
/* Swap the insertion/duplication state between two locations. */
static void
@@ -10334,6 +10376,12 @@ swap_insertion (struct bp_location *left, struct bp_location *right)
const int left_duplicate = left->duplicate;
const struct bp_target_info left_target_info = left->target_info;
+ /* Locations of tracepoints never be duplicated. */
+ if (is_tracepoint (left->owner))
+ gdb_assert (left->duplicate == 0);
+ if (is_tracepoint (right->owner))
+ gdb_assert (right->duplicate == 0);
+
left->inserted = right->inserted;
left->duplicate = right->duplicate;
left->target_info = right->target_info;
@@ -10424,7 +10472,7 @@ update_global_location_list (int should_insert)
int keep_in_target = 0;
int removed = 0;
- /* Skip LOCP entries which will definitely never be needed.
+ /* skip LOCP entries which will definitely never be needed.
Stop either at or being the one matching OLD_LOC. */
while (locp < bp_location + bp_location_count
&& (*locp)->address < old_loc->address)
@@ -10491,7 +10539,9 @@ update_global_location_list (int should_insert)
if it should be inserted in case it will be
unduplicated. */
if (loc2 != old_loc
- && unduplicated_should_be_inserted (loc2))
+ && unduplicated_should_be_inserted (loc2)
+ && (is_tracepoint (old_loc->owner)
+ == is_tracepoint (loc2->owner)))
{
swap_insertion (old_loc, loc2);
keep_in_target = 1;
@@ -10613,6 +10663,9 @@ update_global_location_list (int should_insert)
|| !loc->enabled
|| loc->shlib_disabled
|| !breakpoint_address_is_meaningful (b)
+ /* Don't detect duplicate for tracepoint locations because they are
+ never duplicated. See the comments in field `duplicate' of
+ `struct bp_location'. */
|| is_tracepoint (b))
continue;
@@ -10660,6 +10713,8 @@ update_global_location_list (int should_insert)
|| (gdbarch_has_global_breakpoints (target_gdbarch))))
insert_breakpoint_locations ();
+ download_tracepoint_locations ();
+
do_cleanups (cleanups);
}
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index fe381df..a1aa9ea 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -335,7 +335,13 @@ struct bp_location
char inserted;
/* Nonzero if this is not the first breakpoint in the list
- for the given address. */
+ for the given address. In current implementation, location
+ of tracepoint _never_ be duplicated with other locations of
+ tracepoints and other kinds of breakpoints, because two locations
+ at the same address may have different actions, so both of
+ these locations should be downloaded. It is possible that
+ two locations have the same address and actions, and they can
+ be treated as `duplicated', but we didn't do it in code. */
char duplicate;
/* If we someday support real thread-specific breakpoints, then
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 493d324..98f1ade 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1711,7 +1711,15 @@ start_tracing (void)
t->number_on_target = 0;
for (loc = b->loc; loc; loc = loc->next)
- target_download_tracepoint_loc (loc);
+ {
+ /* Since tracepoint locations are never duplicated, `inserted'
+ flag should be zero. */
+ gdb_assert (loc->inserted == 0);
+
+ target_download_tracepoint_loc (loc);
+
+ loc->inserted = 1;
+ }
t->number_on_target = b->number;
}
@@ -3203,10 +3211,10 @@ cond_string_is_same (char *str1, char *str2)
/* Look for an existing tracepoint that seems similar enough to the
uploaded one. Enablement isn't compared, because the user can
toggle that freely, and may have done so in anticipation of the
- next trace run. */
+ next trace run. Return the location of matched tracepoint. */
-struct tracepoint *
-find_matching_tracepoint (struct uploaded_tp *utp)
+struct bp_location *
+find_matching_tracepoint_location (struct uploaded_tp *utp)
{
VEC(breakpoint_p) *tp_vec = all_tracepoints ();
int ix;
@@ -3228,7 +3236,7 @@ find_matching_tracepoint (struct uploaded_tp *utp)
for (loc = b->loc; loc; loc = loc->next)
{
if (loc->address == utp->addr)
- return t;
+ return loc;
}
}
}
@@ -3243,17 +3251,24 @@ void
merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
{
struct uploaded_tp *utp;
- struct tracepoint *t;
/* Look for GDB tracepoints that match up with our uploaded versions. */
for (utp = *uploaded_tps; utp; utp = utp->next)
{
- t = find_matching_tracepoint (utp);
- if (t)
- printf_filtered (_("Assuming tracepoint %d is same "
- "as target's tracepoint %d at %s.\n"),
- t->base.number, utp->number,
- paddress (get_current_arch (), utp->addr));
+ struct bp_location *loc;
+ struct tracepoint *t;
+
+ loc = find_matching_tracepoint_location (utp);
+ if (loc)
+ {
+ /* Mark this location as already inserted. */
+ loc->inserted = 1;
+ t = (struct tracepoint *) loc->owner;
+ printf_filtered (_("Assuming tracepoint %d is same "
+ "as target's tracepoint %d at %s.\n"),
+ loc->owner->number, utp->number,
+ paddress (loc->gdbarch, utp->addr));
+ }
else
{
t = create_tracepoint_from_upload (utp);
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 4/8] Download tracepoint locations and track its status
2011-11-09 3:47 ` Yao Qi
@ 2011-11-10 15:51 ` Pedro Alves
2011-11-12 2:11 ` Yao Qi
0 siblings, 1 reply; 31+ messages in thread
From: Pedro Alves @ 2011-11-10 15:51 UTC (permalink / raw)
To: gdb-patches; +Cc: Yao Qi
On Tuesday 08 November 2011 06:30:17, Yao Qi wrote:
> @@ -10660,6 +10713,8 @@ update_global_location_list (int should_insert)
> || (gdbarch_has_global_breakpoints (target_gdbarch))))
> insert_breakpoint_locations ();
>
> + download_tracepoint_locations ();
Should be guarded by `should_insert'.
> - /* Skip LOCP entries which will definitely never be needed.
> + /* skip LOCP entries which will definitely never be needed.
Drop this spurious change please.
> @@ -10491,7 +10539,9 @@ update_global_location_list (int should_insert)
> if it should be inserted in case it will be
> unduplicated. */
> if (loc2 != old_loc
> - && unduplicated_should_be_inserted (loc2))
> + && unduplicated_should_be_inserted (loc2)
> + && (is_tracepoint (old_loc->owner)
> + == is_tracepoint (loc2->owner)))
What if both are tracepoints, but of different kind? I think this
might be better handled within breakpoint_locations_match (called just above).
> /* Nonzero if this is not the first breakpoint in the list
> - for the given address. */
> + for the given address. In current implementation, location
> + of tracepoint never be duplicated with other locations of
> + tracepoints and other kinds of breakpoints, because two locations
> + at the same address may have different actions, so both of
> + these locations should be downloaded.
and so that "tfind N" always works.
Please drop the "In current implementation" bit, and the "it is
possible" comment.
> + /* Locations of tracepoints never be duplicated. */
A verb is missing. "can never be", perhaps?
> + if (is_tracepoint (left->owner))
> + gdb_assert (left->duplicate == 0);
`duplicate' is a boolean. Make that `gdb_assert (!left->duplicate)'.
> + if (is_tracepoint (right->owner))
> + gdb_assert (right->duplicate == 0);
> + gdb_assert (loc->inserted == 0);
Boolean ==> `gdb_assert (!loc->inserted)'
--
Pedro Alves
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [patch 4/8] Download tracepoint locations and track its status
2011-11-10 15:51 ` Pedro Alves
@ 2011-11-12 2:11 ` Yao Qi
2011-11-14 12:24 ` Pedro Alves
0 siblings, 1 reply; 31+ messages in thread
From: Yao Qi @ 2011-11-12 2:11 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]
On 11/10/2011 11:51 PM, Pedro Alves wrote:
> On Tuesday 08 November 2011 06:30:17, Yao Qi wrote:
>
>> @@ -10660,6 +10713,8 @@ update_global_location_list (int should_insert)
>> || (gdbarch_has_global_breakpoints (target_gdbarch))))
>> insert_breakpoint_locations ();
>>
>> + download_tracepoint_locations ();
>
> Should be guarded by `should_insert'.
>
Right. Fixed.
>> - /* Skip LOCP entries which will definitely never be needed.
>> + /* skip LOCP entries which will definitely never be needed.
>
> Drop this spurious change please.
Removed.
>
>> @@ -10491,7 +10539,9 @@ update_global_location_list (int should_insert)
>> if it should be inserted in case it will be
>> unduplicated. */
>> if (loc2 != old_loc
>> - && unduplicated_should_be_inserted (loc2))
>> + && unduplicated_should_be_inserted (loc2)
>> + && (is_tracepoint (old_loc->owner)
>> + == is_tracepoint (loc2->owner)))
>
> What if both are tracepoints, but of different kind? I think this
> might be better handled within breakpoint_locations_match (called just above).
>
I move this part into a new function tracepoint_locations_match, and
call it in breakpoint_locations_match.
>
>> /* Nonzero if this is not the first breakpoint in the list
>> - for the given address. */
>> + for the given address. In current implementation, location
>> + of tracepoint never be duplicated with other locations of
>> + tracepoints and other kinds of breakpoints, because two locations
>> + at the same address may have different actions, so both of
>> + these locations should be downloaded.
>
> and so that "tfind N" always works.
>
> Please drop the "In current implementation" bit, and the "it is
> possible" comment.
OK, comment is changed as you suggested.
>
>> + /* Locations of tracepoints never be duplicated. */
>
> A verb is missing. "can never be", perhaps?
>
Oops. Fixed.
>> + if (is_tracepoint (left->owner))
>> + gdb_assert (left->duplicate == 0);
>
> `duplicate' is a boolean. Make that `gdb_assert (!left->duplicate)'.
>
>> + if (is_tracepoint (right->owner))
>> + gdb_assert (right->duplicate == 0);
>
>> + gdb_assert (loc->inserted == 0);
>
> Boolean ==> `gdb_assert (!loc->inserted)'
>
Fixed.
--
Yao (é½å°§)
[-- Attachment #2: 0004-tracepoint-change-loc.patch --]
[-- Type: text/x-patch, Size: 10124 bytes --]
* breakpoint.h (struct bp_location): Add comment on field
`duplicate'.
* breakpoint.c (should_be_inserted): Don't differentiate breakpoint
and tracepoint.
(remove_breakpoints): Don't remove tracepoints.
(tracepoint_locations_match ): New.
(breakpoint_locations_match): Call it.
(disable_breakpoints_in_unloaded_shlib): Handle tracepoint.
(download_tracepoint_locations): New.
(update_global_location_list): Call it.
* tracepoint.c (find_matching_tracepoint): Delete.
(find_matching_tracepoint_location): Renamed from
find_matching_tracepoint. Return bp_location rather than
tracepoint.
(merge_uploaded_tracepoints): Set `inserted' field to 1 if
tracepoint is found.
---
gdb/breakpoint.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++--------
gdb/breakpoint.h | 6 +++-
gdb/tracepoint.c | 39 ++++++++++++++------
3 files changed, 120 insertions(+), 28 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8c98bef..55d707d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1555,7 +1555,10 @@ in which its expression is valid.\n"),
/* Returns 1 iff breakpoint location should be
- inserted in the inferior. */
+ inserted in the inferior. We don't differentiate the type of BL's owner
+ (breakpoint vs. tracepoint), although insert_location in tracepoint's
+ breakpoint_ops is not defined, because in insert_bp_location,
+ tracepoint's insert_location will not be called. */
static int
should_be_inserted (struct bp_location *bl)
{
@@ -1579,11 +1582,6 @@ should_be_inserted (struct bp_location *bl)
if (bl->pspace->breakpoints_not_allowed)
return 0;
- /* Tracepoints are inserted by the target at a time of its choosing,
- not by us. */
- if (is_tracepoint (bl->owner))
- return 0;
-
return 1;
}
@@ -2049,7 +2047,7 @@ remove_breakpoints (void)
ALL_BP_LOCATIONS (bl, blp_tmp)
{
- if (bl->inserted)
+ if (bl->inserted && !is_tracepoint (bl->owner))
val |= remove_breakpoint (bl, mark_uninserted);
}
return val;
@@ -5443,6 +5441,23 @@ breakpoint_location_address_match (struct bp_location *bl,
aspace, addr)));
}
+/* If LOC1 and LOC2's owners are not tracepoints, returns false directly.
+ Then, if LOC1 and LOC2 represent the same tracepoint location, returns
+ true, otherwise returns false. */
+
+static int
+tracepoint_locations_match (struct bp_location *loc1,
+ struct bp_location *loc2)
+{
+ if (is_tracepoint (loc1->owner) && is_tracepoint (loc2->owner))
+ /* Since tracepoint locations are never duplicated with others', tracepoint
+ locations at the same address of different tracepoints are regarded as
+ different locations. */
+ return (loc1->address == loc2->address && loc1->owner == loc2->owner);
+ else
+ return 0;
+}
+
/* Assuming LOC1 and LOC2's types' have meaningful target addresses
(breakpoint_address_is_meaningful), returns true if LOC1 and LOC2
represent the same location. */
@@ -5464,6 +5479,8 @@ breakpoint_locations_match (struct bp_location *loc1,
return 0;
else if (hw_point1)
return watchpoint_locations_match (loc1, loc2);
+ else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner))
+ return tracepoint_locations_match (loc1, loc2);
else
/* We compare bp_location.length in order to cover ranged breakpoints. */
return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
@@ -6071,8 +6088,8 @@ disable_breakpoints_in_shlibs (void)
}
}
-/* Disable any breakpoints that are in an unloaded shared library.
- Only apply to enabled breakpoints, disabled ones can just stay
+/* Disable any breakpoints and tracepoints that are in an unloaded shared
+ library. Only apply to enabled breakpoints, disabled ones can just stay
disabled. */
static void
@@ -6094,13 +6111,14 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
/* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL. */
struct breakpoint *b = loc->owner;
- if ((loc->loc_type == bp_loc_hardware_breakpoint
- || loc->loc_type == bp_loc_software_breakpoint)
- && solib->pspace == loc->pspace
+ if (solib->pspace == loc->pspace
&& !loc->shlib_disabled
- && (b->type == bp_breakpoint
- || b->type == bp_jit_event
- || b->type == bp_hardware_breakpoint)
+ && (((b->type == bp_breakpoint
+ || b->type == bp_jit_event
+ || b->type == bp_hardware_breakpoint)
+ && (loc->loc_type == bp_loc_hardware_breakpoint
+ || loc->loc_type == bp_loc_software_breakpoint))
+ || is_tracepoint (b))
&& solib_contains_address_p (solib, loc->address))
{
loc->shlib_disabled = 1;
@@ -10325,6 +10343,49 @@ bp_location_target_extensions_update (void)
}
}
+/* Download tracepoint locations if they haven't been. */
+
+static void
+download_tracepoint_locations (void)
+{
+ struct bp_location *bl, **blp_tmp;
+ struct cleanup *old_chain;
+
+ if (!target_can_download_tracepoint ())
+ return;
+
+ old_chain = save_current_space_and_thread ();
+
+ ALL_BP_LOCATIONS (bl, blp_tmp)
+ {
+ struct tracepoint *t;
+
+ if (!is_tracepoint (bl->owner))
+ continue;
+
+ if ((bl->owner->type == bp_fast_tracepoint
+ ? !may_insert_fast_tracepoints
+ : !may_insert_tracepoints))
+ continue;
+
+ /* In tracepoint, locations are _never_ duplicated, so
+ should_be_inserted is equivalent to
+ unduplicated_should_be_inserted. */
+ if (!should_be_inserted (bl) || bl->inserted)
+ continue;
+
+ switch_to_program_space_and_thread (bl->pspace);
+
+ target_download_tracepoint (bl);
+
+ bl->inserted = 1;
+ t = (struct tracepoint *) bl->owner;
+ t->number_on_target = bl->owner->number;
+ }
+
+ do_cleanups (old_chain);
+}
+
/* Swap the insertion/duplication state between two locations. */
static void
@@ -10334,6 +10395,12 @@ swap_insertion (struct bp_location *left, struct bp_location *right)
const int left_duplicate = left->duplicate;
const struct bp_target_info left_target_info = left->target_info;
+ /* Locations of tracepoints can never be duplicated. */
+ if (is_tracepoint (left->owner))
+ gdb_assert (!left->duplicate);
+ if (is_tracepoint (right->owner))
+ gdb_assert (!right->duplicate);
+
left->inserted = right->inserted;
left->duplicate = right->duplicate;
left->target_info = right->target_info;
@@ -10613,6 +10680,9 @@ update_global_location_list (int should_insert)
|| !loc->enabled
|| loc->shlib_disabled
|| !breakpoint_address_is_meaningful (b)
+ /* Don't detect duplicate for tracepoint locations because they are
+ never duplicated. See the comments in field `duplicate' of
+ `struct bp_location'. */
|| is_tracepoint (b))
continue;
@@ -10660,6 +10730,9 @@ update_global_location_list (int should_insert)
|| (gdbarch_has_global_breakpoints (target_gdbarch))))
insert_breakpoint_locations ();
+ if (should_insert)
+ download_tracepoint_locations ();
+
do_cleanups (cleanups);
}
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index fe381df..506ae80 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -335,7 +335,11 @@ struct bp_location
char inserted;
/* Nonzero if this is not the first breakpoint in the list
- for the given address. */
+ for the given address. location of tracepoint can _never_
+ be duplicated with other locations of tracepoints and other
+ kinds of breakpoints, because two locations at the same
+ address may have different actions, so both of these locations
+ should be downloaded and so that `tfind N' always works. */
char duplicate;
/* If we someday support real thread-specific breakpoints, then
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index a7035d1..82ca0b8 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1711,7 +1711,15 @@ start_tracing (void)
t->number_on_target = 0;
for (loc = b->loc; loc; loc = loc->next)
- target_download_tracepoint (loc);
+ {
+ /* Since tracepoint locations are never duplicated, `inserted'
+ flag should be zero. */
+ gdb_assert (!loc->inserted);
+
+ target_download_tracepoint (loc);
+
+ loc->inserted = 1;
+ }
t->number_on_target = b->number;
}
@@ -3203,10 +3211,10 @@ cond_string_is_same (char *str1, char *str2)
/* Look for an existing tracepoint that seems similar enough to the
uploaded one. Enablement isn't compared, because the user can
toggle that freely, and may have done so in anticipation of the
- next trace run. */
+ next trace run. Return the location of matched tracepoint. */
-struct tracepoint *
-find_matching_tracepoint (struct uploaded_tp *utp)
+struct bp_location *
+find_matching_tracepoint_location (struct uploaded_tp *utp)
{
VEC(breakpoint_p) *tp_vec = all_tracepoints ();
int ix;
@@ -3228,7 +3236,7 @@ find_matching_tracepoint (struct uploaded_tp *utp)
for (loc = b->loc; loc; loc = loc->next)
{
if (loc->address == utp->addr)
- return t;
+ return loc;
}
}
}
@@ -3243,17 +3251,24 @@ void
merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
{
struct uploaded_tp *utp;
- struct tracepoint *t;
/* Look for GDB tracepoints that match up with our uploaded versions. */
for (utp = *uploaded_tps; utp; utp = utp->next)
{
- t = find_matching_tracepoint (utp);
- if (t)
- printf_filtered (_("Assuming tracepoint %d is same "
- "as target's tracepoint %d at %s.\n"),
- t->base.number, utp->number,
- paddress (get_current_arch (), utp->addr));
+ struct bp_location *loc;
+ struct tracepoint *t;
+
+ loc = find_matching_tracepoint_location (utp);
+ if (loc)
+ {
+ /* Mark this location as already inserted. */
+ loc->inserted = 1;
+ t = (struct tracepoint *) loc->owner;
+ printf_filtered (_("Assuming tracepoint %d is same "
+ "as target's tracepoint %d at %s.\n"),
+ loc->owner->number, utp->number,
+ paddress (loc->gdbarch, utp->addr));
+ }
else
{
t = create_tracepoint_from_upload (utp);
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 4/8] Download tracepoint locations and track its status
2011-11-12 2:11 ` Yao Qi
@ 2011-11-14 12:24 ` Pedro Alves
0 siblings, 0 replies; 31+ messages in thread
From: Pedro Alves @ 2011-11-14 12:24 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On Saturday 12 November 2011 02:11:06, Yao Qi wrote:
> 0004-tracepoint-change-loc.patch
> * breakpoint.h (struct bp_location): Add comment on field
> `duplicate'.
> * breakpoint.c (should_be_inserted): Don't differentiate breakpoint
> and tracepoint.
> (remove_breakpoints): Don't remove tracepoints.
> (tracepoint_locations_match ): New.
> (breakpoint_locations_match): Call it.
> (disable_breakpoints_in_unloaded_shlib): Handle tracepoint.
> (download_tracepoint_locations): New.
> (update_global_location_list): Call it.
> * tracepoint.c (find_matching_tracepoint): Delete.
> (find_matching_tracepoint_location): Renamed from
> find_matching_tracepoint. Return bp_location rather than
> tracepoint.
> (merge_uploaded_tracepoints): Set `inserted' field to 1 if
> tracepoint is found.
This version is okay, thanks.
--
Pedro Alves
^ permalink raw reply [flat|nested] 31+ messages in thread
* [patch 5/8] refactor gdbserver on installing fast tracepoint
2011-11-08 6:00 [patch 0/8] Download tracepoint locations when tracing is running Yao Qi
` (3 preceding siblings ...)
2011-11-08 6:30 ` [patch 4/8] Download tracepoint locations and track its status Yao Qi
@ 2011-11-08 6:33 ` Yao Qi
2011-11-10 15:57 ` Pedro Alves
2011-11-08 6:40 ` [patch 6/8] gdbserver - Install tracepoint when tracing is running Yao Qi
` (2 subsequent siblings)
7 siblings, 1 reply; 31+ messages in thread
From: Yao Qi @ 2011-11-08 6:33 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 169 bytes --]
This is a refactor patch, which convert some code into routines so that
the next patch (6/8) can use.
No functionality is changed in gdbserver.
--
Yao (é½å°§)
[-- Attachment #2: 0005-gdbserver-fast-tp-refactor.patch --]
[-- Type: text/x-patch, Size: 13942 bytes --]
gdb/gdbserver/
* mem-break.c (inc_ref_fast_tracepoint_jump): New.
* mem-break.h: Declare.
* tracepoint.c (cmd_qtstart): Move some code to ...
(clone_fast_tracepoint, install_fast_tracepoint): ... here.
New.
(download_tracepoints): Move some code to ...
(download_tracepoint_1): ... here. New.
---
gdb/gdbserver/mem-break.c | 6 +
gdb/gdbserver/mem-break.h | 3 +
gdb/gdbserver/tracepoint.c | 354 +++++++++++++++++++++++--------------------
3 files changed, 198 insertions(+), 165 deletions(-)
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 348f59d..a6c8ce6 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -302,6 +302,12 @@ delete_fast_tracepoint_jump (struct fast_tracepoint_jump *todel)
return ENOENT;
}
+void
+inc_ref_fast_tracepoint_jump (struct fast_tracepoint_jump *jp)
+{
+ jp->refcount++;
+}
+
struct fast_tracepoint_jump *
set_fast_tracepoint_jump (CORE_ADDR where,
unsigned char *insn, ULONGEST length)
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index c5cb20c..2f8bee9 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -137,6 +137,9 @@ struct fast_tracepoint_jump *set_fast_tracepoint_jump (CORE_ADDR where,
unsigned char *insn,
ULONGEST length);
+/* Increment reference counter of JP. */
+void inc_ref_fast_tracepoint_jump (struct fast_tracepoint_jump *jp);
+
/* Delete fast tracepoint jump TODEL from our tables, and uninsert if
from memory. */
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 95119d3..3a6a0f3 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -1245,6 +1245,7 @@ static void do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
#ifndef IN_PROCESS_AGENT
static struct tracepoint *fast_tracepoint_from_ipa_tpoint_address (CORE_ADDR);
+static int install_fast_tracepoint (struct tracepoint *);
#endif
#if defined(__GNUC__)
@@ -2737,19 +2738,71 @@ probe_marker_at (CORE_ADDR address, char *errout)
return err;
}
-#define MAX_JUMP_SIZE 20
-
static void
-cmd_qtstart (char *packet)
+clone_fast_tracepoint (struct tracepoint *to, const struct tracepoint *from)
{
- struct tracepoint *tpoint, *prev_ftpoint, *prev_stpoint;
- CORE_ADDR jump_entry;
+ to->jump_pad = from->jump_pad;
+ to->jump_pad_end = from->jump_pad_end;
+ to->adjusted_insn_addr = from->adjusted_insn_addr;
+ to->adjusted_insn_addr_end = from->adjusted_insn_addr_end;
+ to->handle = from->handle;
+ gdb_assert (from->handle);
+ inc_ref_fast_tracepoint_jump ((struct fast_tracepoint_jump *) from->handle);
+}
+
+#define MAX_JUMP_SIZE 20
+
+/* Install fast tracepoint. Return 0 if successful, otherwise return
+ non-zero. */
+
+static int
+install_fast_tracepoint (struct tracepoint *tpoint)
+{
+ CORE_ADDR jentry, jump_entry;
+ int err = 0;
/* The jump to the jump pad of the last fast tracepoint
installed. */
unsigned char fjump[MAX_JUMP_SIZE];
ULONGEST fjump_size;
+ jentry = jump_entry = get_jump_space_head ();
+
+ /* Install the jump pad. */
+ err = install_fast_tracepoint_jump_pad (tpoint->obj_addr_on_target,
+ tpoint->address,
+ ipa_sym_addrs.addr_gdb_collect,
+ ipa_sym_addrs.addr_collecting,
+ tpoint->orig_size,
+ &jentry, fjump, &fjump_size,
+ &tpoint->adjusted_insn_addr,
+ &tpoint->adjusted_insn_addr_end);
+
+ if (err)
+ return 1;
+
+ /* Wire it in. */
+ tpoint->handle = set_fast_tracepoint_jump (tpoint->address, fjump,
+ fjump_size);
+
+ if (tpoint->handle != NULL)
+ {
+ tpoint->jump_pad = jump_entry;
+ tpoint->jump_pad_end = jentry;
+
+ /* Pad to 8-byte alignment. */
+ jentry = ((jentry + 7) & ~0x7);
+ claim_jump_space (jentry - jump_entry);
+ }
+
+ return 0;
+}
+
+static void
+cmd_qtstart (char *packet)
+{
+ struct tracepoint *tpoint, *prev_ftpoint, *prev_stpoint;
+
trace_debug ("Starting the trace");
/* Sort tracepoints by ascending address. This makes installing
@@ -2808,55 +2861,12 @@ cmd_qtstart (char *packet)
}
if (prev_ftpoint != NULL && prev_ftpoint->address == tpoint->address)
- {
- tpoint->handle = set_fast_tracepoint_jump (tpoint->address,
- fjump,
- fjump_size);
- tpoint->jump_pad = prev_ftpoint->jump_pad;
- tpoint->jump_pad_end = prev_ftpoint->jump_pad_end;
- tpoint->adjusted_insn_addr = prev_ftpoint->adjusted_insn_addr;
- tpoint->adjusted_insn_addr_end
- = prev_ftpoint->adjusted_insn_addr_end;
- }
+ clone_fast_tracepoint (tpoint, prev_ftpoint);
else
{
- CORE_ADDR jentry;
- int err = 0;
-
- prev_ftpoint = NULL;
-
- jentry = jump_entry = get_jump_space_head ();
-
- /* Install the jump pad. */
- err = install_fast_tracepoint_jump_pad
- (tpoint->obj_addr_on_target,
- tpoint->address,
- ipa_sym_addrs.addr_gdb_collect,
- ipa_sym_addrs.addr_collecting,
- tpoint->orig_size,
- &jentry,
- fjump, &fjump_size,
- &tpoint->adjusted_insn_addr,
- &tpoint->adjusted_insn_addr_end);
-
- /* Wire it in. */
- if (!err)
- tpoint->handle = set_fast_tracepoint_jump (tpoint->address,
- fjump, fjump_size);
-
- if (tpoint->handle != NULL)
- {
- tpoint->jump_pad = jump_entry;
- tpoint->jump_pad_end = jentry;
+ if (install_fast_tracepoint (tpoint) == 0)
+ prev_ftpoint = tpoint;
- /* Pad to 8-byte alignment. */
- jentry = ((jentry + 7) & ~0x7);
- claim_jump_space (jentry - jump_entry);
-
- /* So that we can handle multiple fast tracepoints
- at the same address easily. */
- prev_ftpoint = tpoint;
- }
}
}
else if (tpoint->type == static_tracepoint)
@@ -6325,50 +6335,152 @@ download_agent_expr (struct agent_expr *expr)
/* Align V up to N bits. */
#define UALIGN(V, N) (((V) + ((N) - 1)) & ~((N) - 1))
+/* Sync tracepoint with IPA, but leave maintenance of linked list to caller. */
+
static void
-download_tracepoints (void)
+download_tracepoint_1 (struct tracepoint *tpoint)
{
- CORE_ADDR tpptr = 0, prev_tpptr = 0;
- struct tracepoint *tpoint;
+ struct tracepoint target_tracepoint;
+ CORE_ADDR tpptr = 0;
- /* Start out empty. */
- write_inferior_data_ptr (ipa_sym_addrs.addr_tracepoints, 0);
+ gdb_assert (tpoint->type == fast_tracepoint
+ || tpoint->type == static_tracepoint);
- for (tpoint = tracepoints; tpoint; tpoint = tpoint->next)
+ if (tpoint->cond != NULL && target_emit_ops () != NULL)
{
- struct tracepoint target_tracepoint;
+ CORE_ADDR jentry, jump_entry;
- if (tpoint->type != fast_tracepoint
- && tpoint->type != static_tracepoint)
- continue;
+ jentry = jump_entry = get_jump_space_head ();
- /* Maybe download a compiled condition. */
- if (tpoint->cond != NULL && target_emit_ops () != NULL)
+ if (tpoint->cond != NULL)
{
- CORE_ADDR jentry, jump_entry;
+ /* Pad to 8-byte alignment. (needed?) */
+ /* Actually this should be left for the target to
+ decide. */
+ jentry = UALIGN (jentry, 8);
+
+ compile_tracepoint_condition (tpoint, &jentry);
+ }
+
+ /* Pad to 8-byte alignment. */
+ jentry = UALIGN (jentry, 8);
+ claim_jump_space (jentry - jump_entry);
+ }
- jentry = jump_entry = get_jump_space_head ();
+ target_tracepoint = *tpoint;
- if (tpoint->cond != NULL)
+ tpptr = target_malloc (sizeof (*tpoint));
+ tpoint->obj_addr_on_target = tpptr;
+
+ /* Write the whole object. We'll fix up its pointers in a bit.
+ Assume no next for now. This is fixed up above on the next
+ iteration, if there's any. */
+ target_tracepoint.next = NULL;
+ /* Need to clear this here too, since we're downloading the
+ tracepoints before clearing our own copy. */
+ target_tracepoint.hit_count = 0;
+
+ write_inferior_memory (tpptr, (unsigned char *) &target_tracepoint,
+ sizeof (target_tracepoint));
+
+ if (tpoint->cond)
+ write_inferior_data_ptr (tpptr + offsetof (struct tracepoint,
+ cond),
+ download_agent_expr (tpoint->cond));
+
+ if (tpoint->numactions)
+ {
+ int i;
+ CORE_ADDR actions_array;
+
+ /* The pointers array. */
+ actions_array
+ = target_malloc (sizeof (*tpoint->actions) * tpoint->numactions);
+ write_inferior_data_ptr (tpptr + offsetof (struct tracepoint,
+ actions),
+ actions_array);
+
+ /* Now for each pointer, download the action. */
+ for (i = 0; i < tpoint->numactions; i++)
+ {
+ CORE_ADDR ipa_action = 0;
+ struct tracepoint_action *action = tpoint->actions[i];
+
+ switch (action->type)
{
- /* Pad to 8-byte alignment. (needed?) */
- /* Actually this should be left for the target to
- decide. */
- jentry = UALIGN (jentry, 8);
+ case 'M':
+ ipa_action
+ = target_malloc (sizeof (struct collect_memory_action));
+ write_inferior_memory (ipa_action,
+ (unsigned char *) action,
+ sizeof (struct collect_memory_action));
+ break;
+ case 'R':
+ ipa_action
+ = target_malloc (sizeof (struct collect_registers_action));
+ write_inferior_memory (ipa_action,
+ (unsigned char *) action,
+ sizeof (struct collect_registers_action));
+ break;
+ case 'X':
+ {
+ CORE_ADDR expr;
+ struct eval_expr_action *eaction
+ = (struct eval_expr_action *) action;
- compile_tracepoint_condition (tpoint, &jentry);
+ ipa_action = target_malloc (sizeof (*eaction));
+ write_inferior_memory (ipa_action,
+ (unsigned char *) eaction,
+ sizeof (*eaction));
+
+ expr = download_agent_expr (eaction->expr);
+ write_inferior_data_ptr
+ (ipa_action + offsetof (struct eval_expr_action, expr),
+ expr);
+ break;
+ }
+ case 'L':
+ ipa_action = target_malloc
+ (sizeof (struct collect_static_trace_data_action));
+ write_inferior_memory
+ (ipa_action,
+ (unsigned char *) action,
+ sizeof (struct collect_static_trace_data_action));
+ break;
+ default:
+ trace_debug ("unknown trace action '%c', ignoring",
+ action->type);
+ break;
}
- /* Pad to 8-byte alignment. */
- jentry = UALIGN (jentry, 8);
- claim_jump_space (jentry - jump_entry);
+ if (ipa_action != 0)
+ write_inferior_data_ptr
+ (actions_array + i * sizeof (sizeof (*tpoint->actions)),
+ ipa_action);
}
+ }
+}
- target_tracepoint = *tpoint;
+static void
+download_tracepoints (void)
+{
+ CORE_ADDR tpptr = 0, prev_tpptr = 0;
+ struct tracepoint *tpoint;
+
+ /* Start out empty. */
+ write_inferior_data_ptr (ipa_sym_addrs.addr_tracepoints, 0);
+
+ for (tpoint = tracepoints; tpoint; tpoint = tpoint->next)
+ {
+ if (tpoint->type != fast_tracepoint
+ && tpoint->type != static_tracepoint)
+ continue;
prev_tpptr = tpptr;
- tpptr = target_malloc (sizeof (*tpoint));
- tpoint->obj_addr_on_target = tpptr;
+
+ download_tracepoint_1 (tpoint);
+
+ tpptr = tpoint->obj_addr_on_target;
if (tpoint == tracepoints)
{
@@ -6382,94 +6494,6 @@ download_tracepoints (void)
next),
tpptr);
}
-
- /* Write the whole object. We'll fix up its pointers in a bit.
- Assume no next for now. This is fixed up above on the next
- iteration, if there's any. */
- target_tracepoint.next = NULL;
- /* Need to clear this here too, since we're downloading the
- tracepoints before clearing our own copy. */
- target_tracepoint.hit_count = 0;
-
- write_inferior_memory (tpptr, (unsigned char *) &target_tracepoint,
- sizeof (target_tracepoint));
-
- if (tpoint->cond)
- write_inferior_data_ptr (tpptr + offsetof (struct tracepoint,
- cond),
- download_agent_expr (tpoint->cond));
-
- if (tpoint->numactions)
- {
- int i;
- CORE_ADDR actions_array;
-
- /* The pointers array. */
- actions_array
- = target_malloc (sizeof (*tpoint->actions) * tpoint->numactions);
- write_inferior_data_ptr (tpptr + offsetof (struct tracepoint,
- actions),
- actions_array);
-
- /* Now for each pointer, download the action. */
- for (i = 0; i < tpoint->numactions; i++)
- {
- CORE_ADDR ipa_action = 0;
- struct tracepoint_action *action = tpoint->actions[i];
-
- switch (action->type)
- {
- case 'M':
- ipa_action
- = target_malloc (sizeof (struct collect_memory_action));
- write_inferior_memory (ipa_action,
- (unsigned char *) action,
- sizeof (struct collect_memory_action));
- break;
- case 'R':
- ipa_action
- = target_malloc (sizeof (struct collect_registers_action));
- write_inferior_memory (ipa_action,
- (unsigned char *) action,
- sizeof (struct collect_registers_action));
- break;
- case 'X':
- {
- CORE_ADDR expr;
- struct eval_expr_action *eaction
- = (struct eval_expr_action *) action;
-
- ipa_action = target_malloc (sizeof (*eaction));
- write_inferior_memory (ipa_action,
- (unsigned char *) eaction,
- sizeof (*eaction));
-
- expr = download_agent_expr (eaction->expr);
- write_inferior_data_ptr
- (ipa_action + offsetof (struct eval_expr_action, expr),
- expr);
- break;
- }
- case 'L':
- ipa_action = target_malloc
- (sizeof (struct collect_static_trace_data_action));
- write_inferior_memory
- (ipa_action,
- (unsigned char *) action,
- sizeof (struct collect_static_trace_data_action));
- break;
- default:
- trace_debug ("unknown trace action '%c', ignoring",
- action->type);
- break;
- }
-
- if (ipa_action != 0)
- write_inferior_data_ptr
- (actions_array + i * sizeof (sizeof (*tpoint->actions)),
- ipa_action);
- }
- }
}
}
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* [patch 6/8] gdbserver - Install tracepoint when tracing is running
2011-11-08 6:00 [patch 0/8] Download tracepoint locations when tracing is running Yao Qi
` (4 preceding siblings ...)
2011-11-08 6:33 ` [patch 5/8] refactor gdbserver on installing fast tracepoint Yao Qi
@ 2011-11-08 6:40 ` Yao Qi
2011-11-08 8:20 ` Eli Zaretskii
2011-11-10 16:53 ` Pedro Alves
2011-11-08 6:43 ` [patch 7/8] Documentation changes Yao Qi
2011-11-08 6:56 ` [patch 8/8] Test cases Yao Qi
7 siblings, 2 replies; 31+ messages in thread
From: Yao Qi @ 2011-11-08 6:40 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 362 bytes --]
This patch is the major part of gdbserver side in this patch set.
gdbserver will receive tracepoint locations details, and install them to
inferior. The order of tracepoint list in IPA is kept when adding new
tracepoints, to make sure new added fast tracepoint works correctly with
existing fast tracepoints which are already installed.
--
Yao (é½å°§)
[-- Attachment #2: 0006-gdbserver-install-tp-if-in-tracing.patch --]
[-- Type: text/x-patch, Size: 9397 bytes --]
* tracepoint.c (install_tracepoint, download_tracepoint): New.
(cmd_qtdp): Call them.
(add_tracepoint): Add one parameter to sort list conditionally.
* server.c (handle_query): Handle InstallInTrace for qSupported.
---
gdb/gdbserver/server.c | 1 +
gdb/gdbserver/tracepoint.c | 213 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 207 insertions(+), 7 deletions(-)
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 4612457..0f963e8 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1584,6 +1584,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
if (gdb_supports_qRelocInsn && target_supports_fast_tracepoints ())
strcat (own_buf, ";FastTracepoints+");
strcat (own_buf, ";StaticTracepoints+");
+ strcat (own_buf, ";InstallInTrace+");
strcat (own_buf, ";qXfer:statictrace:read+");
strcat (own_buf, ";qXfer:traceframe-info:read+");
strcat (own_buf, ";EnableDisableTracepoints+");
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 3a6a0f3..6a5e111 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -1245,6 +1245,9 @@ static void do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
#ifndef IN_PROCESS_AGENT
static struct tracepoint *fast_tracepoint_from_ipa_tpoint_address (CORE_ADDR);
+
+static void install_tracepoint (struct tracepoint *, char *own_buf);
+static void download_tracepoint (struct tracepoint *);
static int install_fast_tracepoint (struct tracepoint *);
#endif
@@ -1641,10 +1644,11 @@ free_space (void)
static int seen_step_action_flag;
-/* Create a tracepoint (location) with given number and address. */
+/* Create a tracepoint (location) with given number and address. If SORT is
+ non-zero, the list of tracepoint will be sorted. */
static struct tracepoint *
-add_tracepoint (int num, CORE_ADDR addr)
+add_tracepoint (int num, CORE_ADDR addr, int sort)
{
struct tracepoint *tpoint;
@@ -1666,10 +1670,48 @@ add_tracepoint (int num, CORE_ADDR addr)
tpoint->handle = NULL;
tpoint->next = NULL;
- if (!last_tracepoint)
- tracepoints = tpoint;
+ if (sort)
+ {
+ struct tracepoint *tp, *tp_prev;
+
+ /* Find a place to insert this tracepoint into list in order to keep
+ the tracepoint list still in an ascending order. There may be
+ multiple tracepoints at the same address as TPOINT's, and this
+ guarantee that TP_PREV is the last tracepoint entry of them, so that
+ TPOINT is inserted at the last of them. For example, fast tracepoint
+ A, B, C are set at the same address, and D is to be insert at the same
+ place as well,
+
+ -->| A |--> | B |-->| C |->...
+
+ One jump pad was created for tracepoint A, B, and C, and the target
+ address of A is referenced/used in jump pad. So jump pad will let
+ inferior jump to A. If D is inserted in front of A, like this,
+
+ -->| D |-->| A |--> | B |-->| C |->...
+
+ without updating jump pad, D is not reachable during collect, which
+ is wrong. As we can see, the order of B, C and D doesn't matter, but
+ A should always be the `first' one. */
+ for (tp_prev = tp = tracepoints; tp && tp->address <= tpoint->address;
+ tp = tp->next)
+ tp_prev = tp;
+
+ if (tp_prev)
+ {
+ tpoint->next = tp_prev->next;
+ tp_prev->next = tpoint;
+ }
+ else
+ tracepoints = tpoint;
+ }
else
- last_tracepoint->next = tpoint;
+ {
+ if (!last_tracepoint)
+ tracepoints = tpoint;
+ else
+ last_tracepoint->next = tpoint;
+ }
last_tracepoint = tpoint;
seen_step_action_flag = 0;
@@ -2269,6 +2311,8 @@ static void
cmd_qtdp (char *own_buf)
{
int tppacket;
+ /* Whether there is a trailing hyphen at the end of the QTDP packet. */
+ int trail_hyphen = 0;
ULONGEST num;
ULONGEST addr;
ULONGEST count;
@@ -2306,7 +2350,11 @@ cmd_qtdp (char *own_buf)
return;
}
- tpoint = add_tracepoint (num, addr);
+ /* When it is not tracing, we don't have to sort tracepoints, because
+ they will be sorted in cmd_qtstart later. When it is tracing, the
+ list has been sorted, and we should still keep ascending order of
+ list after adding new entry. */
+ tpoint = add_tracepoint (num, addr, tracing);
tpoint->enabled = (*packet == 'E');
++packet; /* skip 'E' */
@@ -2346,7 +2394,10 @@ cmd_qtdp (char *own_buf)
trace_debug ("Unknown optional tracepoint field");
}
if (*packet == '-')
- trace_debug ("Also has actions\n");
+ {
+ trail_hyphen = 1;
+ trace_debug ("Also has actions\n");
+ }
trace_debug ("Defined %stracepoint %d at 0x%s, "
"enabled %d step %ld pass %ld",
@@ -2365,6 +2416,29 @@ cmd_qtdp (char *own_buf)
return;
}
+ /* Install tracepoint during tracing only once of each tracepoint location.
+ For each tracepoint loc, GDB may send multiple QTDP packets, and we can
+ determine the last QTDP packet for one tracepoint location by checking
+ trailing hyphen in QTDP packet. */
+ if (tracing && !trail_hyphen)
+ {
+ /* Pause all threads temporarily while we patch tracepoints. */
+ pause_all (0);
+
+ /* download_tracepoint will update global `tracepoints'
+ list, so it is unsafe to leave threads in jump pad. */
+ stabilize_threads ();
+
+ /* Freeze threads. */
+ pause_all (1);
+
+ download_tracepoint (tpoint);
+ install_tracepoint (tpoint, own_buf);
+
+ unpause_all (1);
+ return;
+ }
+
write_ok (own_buf);
}
@@ -2798,6 +2872,84 @@ install_fast_tracepoint (struct tracepoint *tpoint)
return 0;
}
+
+/* Install tracepoint TPOINT, and write reply message in OWN_BUF. */
+
+static void
+install_tracepoint (struct tracepoint *tpoint, char *own_buf)
+{
+ tpoint->handle = NULL;
+
+ if (tpoint->type == trap_tracepoint)
+ {
+ /* Tracepoints are installed as memory breakpoints. Just go
+ ahead and install the trap. The breakpoints module
+ handles duplicated breakpoints, and the memory read
+ routine handles un-patching traps from memory reads. */
+ tpoint->handle = set_breakpoint_at (tpoint->address,
+ tracepoint_handler);
+ }
+ else if (tpoint->type == fast_tracepoint || tpoint->type == static_tracepoint)
+ {
+ struct tracepoint *tp;
+
+ if (!in_process_agent_loaded ())
+ {
+ trace_debug ("Requested a %s tracepoint, but fast "
+ "tracepoints aren't supported.",
+ tpoint->type == static_tracepoint ? "static" : "fast");
+ write_e_ipa_not_loaded (own_buf);
+ unpause_all (1);
+ return;
+ }
+ if (tpoint->type == static_tracepoint && !in_process_agent_loaded_ust ())
+ {
+ trace_debug ("Requested a static tracepoint, but static "
+ "tracepoints are not supported.");
+ write_e_ust_not_loaded (own_buf);
+ unpause_all (1);
+ return;
+ }
+
+ /* Find another fast or static tracepoint at the same address. */
+ for (tp = tracepoints; tp; tp = tp->next)
+ {
+ if (tp->address == tpoint->address && tp->type == tpoint->type
+ && tp->number != tpoint->number)
+ break;
+ }
+
+ if (tpoint->type == fast_tracepoint)
+ {
+ if (tp) /* TPOINT is installed at the same address as TP. */
+ clone_fast_tracepoint (tpoint, tp);
+ else
+ install_fast_tracepoint (tpoint);
+ }
+ else
+ {
+ if (tp)
+ tpoint->handle = (void *) -1;
+ else
+ {
+ if (probe_marker_at (tpoint->address, own_buf) == 0)
+ tpoint->handle = (void *) -1;
+ }
+ }
+
+ }
+ else
+ internal_error (__FILE__, __LINE__, "Unknown tracepoint type");
+
+ if (tpoint->handle == NULL)
+ {
+ if (tpoint->type == fast_tracepoint)
+ write_enn (own_buf);
+ }
+ else
+ write_ok (own_buf);
+}
+
static void
cmd_qtstart (char *packet)
{
@@ -6462,6 +6614,53 @@ download_tracepoint_1 (struct tracepoint *tpoint)
}
static void
+download_tracepoint (struct tracepoint *tpoint)
+{
+ struct tracepoint *tp, *tp_prev;
+
+ if (tpoint->type != fast_tracepoint
+ && tpoint->type != static_tracepoint)
+ return;
+
+ download_tracepoint_1 (tpoint);
+
+ /* Find the previous entry of TPOINT, which is fast tracepoint or
+ or static tracepoint. */
+ tp_prev = NULL;
+ for (tp = tracepoints; tp != tpoint; tp = tp->next)
+ {
+ if (tp->type == fast_tracepoint || tp->type == static_tracepoint)
+ tp_prev = tp;
+ }
+
+ if (tp_prev)
+ {
+ CORE_ADDR tp_prev_target_next_addr;
+
+ /* Insert TPOINT after TP_PREV in IPA. */
+ if (read_inferior_data_pointer (tp_prev->obj_addr_on_target
+ + offsetof (struct tracepoint, next),
+ &tp_prev_target_next_addr))
+ fatal ("error reading `tp_prev->next'");
+
+ /* tpoint->next = tp_prev->next */
+ write_inferior_data_ptr (tpoint->obj_addr_on_target
+ + offsetof (struct tracepoint, next),
+ tp_prev_target_next_addr);
+ /* tp_prev->next = tpoint */
+ write_inferior_data_ptr (tp_prev->obj_addr_on_target
+ + offsetof (struct tracepoint, next),
+ tpoint->obj_addr_on_target);
+ }
+ else
+ /* First object in list, set the head pointer in the
+ inferior. */
+ write_inferior_data_ptr (ipa_sym_addrs.addr_tracepoints,
+ tpoint->obj_addr_on_target);
+
+}
+
+static void
download_tracepoints (void)
{
CORE_ADDR tpptr = 0, prev_tpptr = 0;
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 6/8] gdbserver - Install tracepoint when tracing is running
2011-11-08 6:40 ` [patch 6/8] gdbserver - Install tracepoint when tracing is running Yao Qi
@ 2011-11-08 8:20 ` Eli Zaretskii
2011-11-08 8:41 ` Yao Qi
2011-11-10 16:53 ` Pedro Alves
1 sibling, 1 reply; 31+ messages in thread
From: Eli Zaretskii @ 2011-11-08 8:20 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
> Date: Tue, 08 Nov 2011 14:40:04 +0800
> From: Yao Qi <yao@codesourcery.com>
>
> -/* Create a tracepoint (location) with given number and address. */
> +/* Create a tracepoint (location) with given number and address. If SORT is
> + non-zero, the list of tracepoint will be sorted. */
^^^^^^^^^^
"tracepoints"
> + /* Find a place to insert this tracepoint into list in order to keep
> + the tracepoint list still in an ascending order. There may be
^^^^^^^^^^^^^^^^^^
"the ascending order"
> + multiple tracepoints at the same address as TPOINT's, and this
> + guarantee that TP_PREV is the last tracepoint entry of them, so that
^^^^^^^^^
"guarantees"
> + TPOINT is inserted at the last of them.
"inserted at the last of them" is not clear. I would suggest using
"before" or "after". Or maybe "at the highest address" if you mean
"at address" (I don't really understand the meaning of what the code
does.)
> For example, fast tracepoint
"tracepoints"
> + /* Install tracepoint during tracing only once of each tracepoint location.
^^^^^^^
"for each", I think.
> + /* Find the previous entry of TPOINT, which is fast tracepoint or
> + or static tracepoint. */
2 "or" in a row.
Thanks.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 6/8] gdbserver - Install tracepoint when tracing is running
2011-11-08 8:20 ` Eli Zaretskii
@ 2011-11-08 8:41 ` Yao Qi
2011-11-08 11:38 ` Eli Zaretskii
0 siblings, 1 reply; 31+ messages in thread
From: Yao Qi @ 2011-11-08 8:41 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
On 11/08/2011 04:20 PM, Eli Zaretskii wrote:
>> + TPOINT is inserted at the last of them.
>
> "inserted at the last of them" is not clear. I would suggest using
> "before" or "after". Or maybe "at the highest address" if you mean
> "at address" (I don't really understand the meaning of what the code
> does.)
>
What I want to describe is there have been three, for example,
tracepoints setting at the same address, and the forth is to be set at
the same place as well, and insert the forth into linked list. What I
meant in comment is "insert the forth tracepoint after these three
existing tracepoints", and of course, there may be other tracepoints, at
higher address, after them in the list.
How about "TPOINT is inserted after all the tracepoints which are set at
the same address"?
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [patch 6/8] gdbserver - Install tracepoint when tracing is running
2011-11-08 6:40 ` [patch 6/8] gdbserver - Install tracepoint when tracing is running Yao Qi
2011-11-08 8:20 ` Eli Zaretskii
@ 2011-11-10 16:53 ` Pedro Alves
2011-11-12 2:40 ` Yao Qi
1 sibling, 1 reply; 31+ messages in thread
From: Pedro Alves @ 2011-11-10 16:53 UTC (permalink / raw)
To: gdb-patches; +Cc: Yao Qi
On Tuesday 08 November 2011 06:40:04, Yao Qi wrote:
> @@ -1666,10 +1670,48 @@ add_tracepoint (int num, CORE_ADDR addr)
> tpoint->handle = NULL;
> tpoint->next = NULL;
>
> - if (!last_tracepoint)
> - tracepoints = tpoint;
> + if (sort)
> + {
> + struct tracepoint *tp, *tp_prev;
> +
> + /* Find a place to insert this tracepoint into list in order to keep
> + the tracepoint list still in an ascending order. There may be
> + multiple tracepoints at the same address as TPOINT's, and this
> + guarantee that TP_PREV is the last tracepoint entry of them, so that
> + TPOINT is inserted at the last of them. For example, fast tracepoint
> + A, B, C are set at the same address, and D is to be insert at the same
> + place as well,
> +
> + -->| A |--> | B |-->| C |->...
> +
> + One jump pad was created for tracepoint A, B, and C, and the target
> + address of A is referenced/used in jump pad. So jump pad will let
> + inferior jump to A. If D is inserted in front of A, like this,
> +
> + -->| D |-->| A |--> | B |-->| C |->...
> +
> + without updating jump pad, D is not reachable during collect, which
> + is wrong. As we can see, the order of B, C and D doesn't matter, but
> + A should always be the `first' one. */
> + for (tp_prev = tp = tracepoints; tp && tp->address <= tpoint->address;
> + tp = tp->next)
> + tp_prev = tp;
> +
> + if (tp_prev)
> + {
> + tpoint->next = tp_prev->next;
> + tp_prev->next = tpoint;
> + }
> + else
> + tracepoints = tpoint;
This looks incorrect. If there was only one tracepoint on the list,
and its address was higher than TPOINT's, then you'd end up with
tp_prev = tracepoints; // the existing tracepoint
and then this
+ if (tp_prev)
+ {
+ tpoint->next = tp_prev->next;
+ tp_prev->next = tpoint;
+ }
inserts the new tracepoint after that existing
tracepoint, but it should be before.
IOW, tp_prev should start out NULL:
for (tp_prev = NULL, tp = tracepoints;
tp != NULL && tp->address <= tpoint->address;
tp_prev = tp, tp = tp->next)
;
if (tp_prev)
{
tpoint->next = tp_prev->next;
tp_prev->next = tpoint;
}
else
{
tpoint->next = tracepoints;
tracepoints = tpoint;
}
which can be written in a more compact form as:
struct tracepoint **tp_next;
for (tp_next = &tracepoints;
(*tp_next) != NULL && (*tp_next)->address <= tpoint->address;
tp_next = &(*tp_next)->next)
;
tpoint->next = *tp_next;
*tp_next = tpoint;
> + }
> else
> - last_tracepoint->next = tpoint;
> + {
> + if (!last_tracepoint)
> + tracepoints = tpoint;
> + else
> + last_tracepoint->next = tpoint;
> + }
> last_tracepoint = tpoint;
>
If we have a path that needs a sorted insert, how about we
always do the inserted sort, and get rid of the non-inserted
sort, and the sort at tstart time? That'd mean less code to
maintain.
> seen_step_action_flag = 0;
> @@ -2269,6 +2311,8 @@ static void
> cmd_qtdp (char *own_buf)
> {
> int tppacket;
> + /* Whether there is a trailing hyphen at the end of the QTDP packet. */
> + int trail_hyphen = 0;
> ULONGEST num;
> ULONGEST addr;
> ULONGEST count;
> @@ -2306,7 +2350,11 @@ cmd_qtdp (char *own_buf)
> return;
> }
>
> - tpoint = add_tracepoint (num, addr);
> + /* When it is not tracing, we don't have to sort tracepoints, because
> + they will be sorted in cmd_qtstart later. When it is tracing, the
> + list has been sorted, and we should still keep ascending order of
> + list after adding new entry. */
> + tpoint = add_tracepoint (num, addr, tracing);
>
> tpoint->enabled = (*packet == 'E');
> ++packet; /* skip 'E' */
> @@ -2346,7 +2394,10 @@ cmd_qtdp (char *own_buf)
> trace_debug ("Unknown optional tracepoint field");
> }
> if (*packet == '-')
> - trace_debug ("Also has actions\n");
> + {
> + trail_hyphen = 1;
> + trace_debug ("Also has actions\n");
> + }
>
> trace_debug ("Defined %stracepoint %d at 0x%s, "
> "enabled %d step %ld pass %ld",
> @@ -2365,6 +2416,29 @@ cmd_qtdp (char *own_buf)
> return;
> }
>
> + /* Install tracepoint during tracing only once of each tracepoint location.
> + For each tracepoint loc, GDB may send multiple QTDP packets, and we can
> + determine the last QTDP packet for one tracepoint location by checking
> + trailing hyphen in QTDP packet. */
> + if (tracing && !trail_hyphen)
> + {
> + /* Pause all threads temporarily while we patch tracepoints. */
> + pause_all (0);
> +
> + /* download_tracepoint will update global `tracepoints'
> + list, so it is unsafe to leave threads in jump pad. */
> + stabilize_threads ();
> +
> + /* Freeze threads. */
> + pause_all (1);
> +
> + download_tracepoint (tpoint);
> + install_tracepoint (tpoint, own_buf);
> +
> + unpause_all (1);
> + return;
> + }
> +
> write_ok (own_buf);
> }
>
> @@ -2798,6 +2872,84 @@ install_fast_tracepoint (struct tracepoint *tpoint)
> return 0;
> }
>
> +
> +/* Install tracepoint TPOINT, and write reply message in OWN_BUF. */
> +
> +static void
> +install_tracepoint (struct tracepoint *tpoint, char *own_buf)
> +{
> + tpoint->handle = NULL;
> +
> + if (tpoint->type == trap_tracepoint)
> + {
> + /* Tracepoints are installed as memory breakpoints. Just go
> + ahead and install the trap. The breakpoints module
> + handles duplicated breakpoints, and the memory read
> + routine handles un-patching traps from memory reads. */
> + tpoint->handle = set_breakpoint_at (tpoint->address,
> + tracepoint_handler);
> + }
> + else if (tpoint->type == fast_tracepoint || tpoint->type == static_tracepoint)
> + {
> + struct tracepoint *tp;
> +
> + if (!in_process_agent_loaded ())
> + {
> + trace_debug ("Requested a %s tracepoint, but fast "
> + "tracepoints aren't supported.",
> + tpoint->type == static_tracepoint ? "static" : "fast");
> + write_e_ipa_not_loaded (own_buf);
> + unpause_all (1);
This unpause_all is a leftover from a previous version. You now unpause
at the caller, so remove this.
> + return;
> + }
> + if (tpoint->type == static_tracepoint && !in_process_agent_loaded_ust ())
> + {
> + trace_debug ("Requested a static tracepoint, but static "
> + "tracepoints are not supported.");
> + write_e_ust_not_loaded (own_buf);
> + unpause_all (1);
Ditto.
> + return;
> + }
> +
> + /* Find another fast or static tracepoint at the same address. */
> + for (tp = tracepoints; tp; tp = tp->next)
> + {
> + if (tp->address == tpoint->address && tp->type == tpoint->type
> + && tp->number != tpoint->number)
> + break;
> + }
> +
> + if (tpoint->type == fast_tracepoint)
> + {
> + if (tp) /* TPOINT is installed at the same address as TP. */
> + clone_fast_tracepoint (tpoint, tp);
> + else
> + install_fast_tracepoint (tpoint);
> + }
> + else
> + {
> + if (tp)
> + tpoint->handle = (void *) -1;
> + else
> + {
> + if (probe_marker_at (tpoint->address, own_buf) == 0)
> + tpoint->handle = (void *) -1;
> + }
> + }
> +
> + }
> + else
> + internal_error (__FILE__, __LINE__, "Unknown tracepoint type");
> +
> + if (tpoint->handle == NULL)
> + {
> + if (tpoint->type == fast_tracepoint)
> + write_enn (own_buf);
See cmd_qtstart:
*packet = '\0';
...
if (*packet == '\0')
write_enn (packet);
We should use the same pattern:
*own_buf = '\0';
...
if (*own_buf == '\0')
write_enn (own_buf);
Because probe_marker_at only fills the error string
on a class of errors.
--
Pedro Alves
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 6/8] gdbserver - Install tracepoint when tracing is running
2011-11-10 16:53 ` Pedro Alves
@ 2011-11-12 2:40 ` Yao Qi
2011-11-14 12:32 ` Pedro Alves
0 siblings, 1 reply; 31+ messages in thread
From: Yao Qi @ 2011-11-12 2:40 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 5891 bytes --]
On 11/11/2011 12:53 AM, Pedro Alves wrote:
>> > @@ -1666,10 +1670,48 @@ add_tracepoint (int num, CORE_ADDR addr)
>> > tpoint->handle = NULL;
>> > tpoint->next = NULL;
>> >
>> > - if (!last_tracepoint)
>> > - tracepoints = tpoint;
>> > + if (sort)
>> > + {
>> > + struct tracepoint *tp, *tp_prev;
>> > +
>> > + /* Find a place to insert this tracepoint into list in order to keep
>> > + the tracepoint list still in an ascending order. There may be
>> > + multiple tracepoints at the same address as TPOINT's, and this
>> > + guarantee that TP_PREV is the last tracepoint entry of them, so that
>> > + TPOINT is inserted at the last of them. For example, fast tracepoint
>> > + A, B, C are set at the same address, and D is to be insert at the same
>> > + place as well,
>> > +
>> > + -->| A |--> | B |-->| C |->...
>> > +
>> > + One jump pad was created for tracepoint A, B, and C, and the target
>> > + address of A is referenced/used in jump pad. So jump pad will let
>> > + inferior jump to A. If D is inserted in front of A, like this,
>> > +
>> > + -->| D |-->| A |--> | B |-->| C |->...
>> > +
>> > + without updating jump pad, D is not reachable during collect, which
>> > + is wrong. As we can see, the order of B, C and D doesn't matter, but
>> > + A should always be the `first' one. */
>> > + for (tp_prev = tp = tracepoints; tp && tp->address <= tpoint->address;
>> > + tp = tp->next)
>> > + tp_prev = tp;
>> > +
>> > + if (tp_prev)
>> > + {
>> > + tpoint->next = tp_prev->next;
>> > + tp_prev->next = tpoint;
>> > + }
>> > + else
>> > + tracepoints = tpoint;
> This looks incorrect. If there was only one tracepoint on the list,
> and its address was higher than TPOINT's, then you'd end up with
>
> tp_prev = tracepoints; // the existing tracepoint
>
> and then this
>
> + if (tp_prev)
> + {
> + tpoint->next = tp_prev->next;
> + tp_prev->next = tpoint;
> + }
>
> inserts the new tracepoint after that existing
> tracepoint, but it should be before.
Hmmm, you are right.
>
> IOW, tp_prev should start out NULL:
>
> for (tp_prev = NULL, tp = tracepoints;
> tp != NULL && tp->address <= tpoint->address;
> tp_prev = tp, tp = tp->next)
> ;
>
> if (tp_prev)
> {
> tpoint->next = tp_prev->next;
> tp_prev->next = tpoint;
> }
> else
> {
> tpoint->next = tracepoints;
> tracepoints = tpoint;
> }
>
> which can be written in a more compact form as:
>
> struct tracepoint **tp_next;
>
> for (tp_next = &tracepoints;
> (*tp_next) != NULL && (*tp_next)->address <= tpoint->address;
> tp_next = &(*tp_next)->next)
> ;
> tpoint->next = *tp_next;
> *tp_next = tpoint;
>
This compact form is great! Thanks.
>
>> > + }
>> > else
>> > - last_tracepoint->next = tpoint;
>> > + {
>> > + if (!last_tracepoint)
>> > + tracepoints = tpoint;
>> > + else
>> > + last_tracepoint->next = tpoint;
>> > + }
>> > last_tracepoint = tpoint;
>> >
> If we have a path that needs a sorted insert, how about we
> always do the inserted sort, and get rid of the non-inserted
> sort, and the sort at tstart time? That'd mean less code to
> maintain.
>
That is fine. sort_tracepoints is removed.
>
>> > seen_step_action_flag = 0;
>> > @@ -2269,6 +2311,8 @@ static void
>> > +
>> > +/* Install tracepoint TPOINT, and write reply message in OWN_BUF. */
>> > +
>> > +static void
>> > +install_tracepoint (struct tracepoint *tpoint, char *own_buf)
>> > +{
>> > + tpoint->handle = NULL;
>> > +
>> > + if (tpoint->type == trap_tracepoint)
>> > + {
>> > + /* Tracepoints are installed as memory breakpoints. Just go
>> > + ahead and install the trap. The breakpoints module
>> > + handles duplicated breakpoints, and the memory read
>> > + routine handles un-patching traps from memory reads. */
>> > + tpoint->handle = set_breakpoint_at (tpoint->address,
>> > + tracepoint_handler);
>> > + }
>> > + else if (tpoint->type == fast_tracepoint || tpoint->type == static_tracepoint)
>> > + {
>> > + struct tracepoint *tp;
>> > +
>> > + if (!in_process_agent_loaded ())
>> > + {
>> > + trace_debug ("Requested a %s tracepoint, but fast "
>> > + "tracepoints aren't supported.",
>> > + tpoint->type == static_tracepoint ? "static" : "fast");
>> > + write_e_ipa_not_loaded (own_buf);
>> > + unpause_all (1);
> This unpause_all is a leftover from a previous version. You now unpause
> at the caller, so remove this.
>
Removed.
>> > + return;
>> > + }
>> > + if (tpoint->type == static_tracepoint && !in_process_agent_loaded_ust ())
>> > + {
>> > + trace_debug ("Requested a static tracepoint, but static "
>> > + "tracepoints are not supported.");
>> > + write_e_ust_not_loaded (own_buf);
>> > + unpause_all (1);
> Ditto.
>
Removed.
>> > + else
>> > + internal_error (__FILE__, __LINE__, "Unknown tracepoint type");
>> > +
>> > + if (tpoint->handle == NULL)
>> > + {
>> > + if (tpoint->type == fast_tracepoint)
>> > + write_enn (own_buf);
> See cmd_qtstart:
>
> *packet = '\0';
> ...
> if (*packet == '\0')
> write_enn (packet);
>
> We should use the same pattern:
>
> *own_buf = '\0';
> ...
> if (*own_buf == '\0')
> write_enn (own_buf);
>
> Because probe_marker_at only fills the error string
> on a class of errors.
OK, I understand now.
--
Yao (é½å°§)
[-- Attachment #2: 0006-gdbserver-install-tp-if-in-tracing.patch --]
[-- Type: text/x-patch, Size: 10494 bytes --]
* server.c (handle_query): Handle InstallInTrace for qSupported.
* tracepoint.c (add_tracepoint): Sort list.
(install_tracepoint, download_tracepoint): New.
(cmd_qtdp): Call them to install and download tracepoints.
(sort_tracepoints): Removed.
(cmd_qtstart): Update.
---
gdb/gdbserver/server.c | 1 +
gdb/gdbserver/tracepoint.c | 248 ++++++++++++++++++++++++++++++++-----------
2 files changed, 185 insertions(+), 64 deletions(-)
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 4612457..0f963e8 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1584,6 +1584,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
if (gdb_supports_qRelocInsn && target_supports_fast_tracepoints ())
strcat (own_buf, ";FastTracepoints+");
strcat (own_buf, ";StaticTracepoints+");
+ strcat (own_buf, ";InstallInTrace+");
strcat (own_buf, ";qXfer:statictrace:read+");
strcat (own_buf, ";qXfer:traceframe-info:read+");
strcat (own_buf, ";EnableDisableTracepoints+");
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 3a6a0f3..f000f63 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -1245,6 +1245,9 @@ static void do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
#ifndef IN_PROCESS_AGENT
static struct tracepoint *fast_tracepoint_from_ipa_tpoint_address (CORE_ADDR);
+
+static void install_tracepoint (struct tracepoint *, char *own_buf);
+static void download_tracepoint (struct tracepoint *);
static int install_fast_tracepoint (struct tracepoint *);
#endif
@@ -1641,12 +1644,13 @@ free_space (void)
static int seen_step_action_flag;
-/* Create a tracepoint (location) with given number and address. */
+/* Create a tracepoint (location) with given number and address. Add this
+ new tracepoint to list and sort this list. */
static struct tracepoint *
add_tracepoint (int num, CORE_ADDR addr)
{
- struct tracepoint *tpoint;
+ struct tracepoint *tpoint, **tp_next;
tpoint = xmalloc (sizeof (struct tracepoint));
tpoint->number = num;
@@ -1666,10 +1670,31 @@ add_tracepoint (int num, CORE_ADDR addr)
tpoint->handle = NULL;
tpoint->next = NULL;
- if (!last_tracepoint)
- tracepoints = tpoint;
- else
- last_tracepoint->next = tpoint;
+ /* Find a place to insert this tracepoint into list in order to keep
+ the tracepoint list still in the ascending order. There may be
+ multiple tracepoints at the same address as TPOINT's, and this
+ guarantees TPOINT is inserted after all the tracepoints which are
+ set at the same address. For example, fast tracepoints A, B, C are
+ set at the same address, and D is to be insert at the same place as
+ well,
+
+ -->| A |--> | B |-->| C |->...
+
+ One jump pad was created for tracepoint A, B, and C, and the target
+ address of A is referenced/used in jump pad. So jump pad will let
+ inferior jump to A. If D is inserted in front of A, like this,
+
+ -->| D |-->| A |--> | B |-->| C |->...
+
+ without updating jump pad, D is not reachable during collect, which
+ is wrong. As we can see, the order of B, C and D doesn't matter, but
+ A should always be the `first' one. */
+ for (tp_next = &tracepoints;
+ (*tp_next) != NULL && (*tp_next)->address <= tpoint->address;
+ tp_next = &(*tp_next)->next)
+ ;
+ tpoint->next = *tp_next;
+ *tp_next = tpoint;
last_tracepoint = tpoint;
seen_step_action_flag = 0;
@@ -2269,6 +2294,8 @@ static void
cmd_qtdp (char *own_buf)
{
int tppacket;
+ /* Whether there is a trailing hyphen at the end of the QTDP packet. */
+ int trail_hyphen = 0;
ULONGEST num;
ULONGEST addr;
ULONGEST count;
@@ -2346,7 +2373,10 @@ cmd_qtdp (char *own_buf)
trace_debug ("Unknown optional tracepoint field");
}
if (*packet == '-')
- trace_debug ("Also has actions\n");
+ {
+ trail_hyphen = 1;
+ trace_debug ("Also has actions\n");
+ }
trace_debug ("Defined %stracepoint %d at 0x%s, "
"enabled %d step %ld pass %ld",
@@ -2365,6 +2395,29 @@ cmd_qtdp (char *own_buf)
return;
}
+ /* Install tracepoint during tracing only once for each tracepoint location.
+ For each tracepoint loc, GDB may send multiple QTDP packets, and we can
+ determine the last QTDP packet for one tracepoint location by checking
+ trailing hyphen in QTDP packet. */
+ if (tracing && !trail_hyphen)
+ {
+ /* Pause all threads temporarily while we patch tracepoints. */
+ pause_all (0);
+
+ /* download_tracepoint will update global `tracepoints'
+ list, so it is unsafe to leave threads in jump pad. */
+ stabilize_threads ();
+
+ /* Freeze threads. */
+ pause_all (1);
+
+ download_tracepoint (tpoint);
+ install_tracepoint (tpoint, own_buf);
+
+ unpause_all (1);
+ return;
+ }
+
write_ok (own_buf);
}
@@ -2658,59 +2711,6 @@ claim_jump_space (ULONGEST used)
gdb_jump_pad_head += used;
}
-/* Sort tracepoints by PC, using a bubble sort. */
-
-static void
-sort_tracepoints (void)
-{
- struct tracepoint *lst, *tmp, *prev = NULL;
- int i, j, n = 0;
-
- if (tracepoints == NULL)
- return;
-
- /* Count nodes. */
- for (tmp = tracepoints; tmp->next; tmp = tmp->next)
- n++;
-
- for (i = 0; i < n - 1; i++)
- for (j = 0, lst = tracepoints;
- lst && lst->next && (j <= n - 1 - i);
- j++)
- {
- /* If we're at beginning, the start node is the prev
- node. */
- if (j == 0)
- prev = lst;
-
- /* Compare neighbors. */
- if (lst->next->address < lst->address)
- {
- struct tracepoint *p;
-
- /* Swap'em. */
- tmp = (lst->next ? lst->next->next : NULL);
-
- if (j == 0 && prev == tracepoints)
- tracepoints = lst->next;
-
- p = lst->next;
- prev->next = lst->next;
- lst->next->next = lst;
- lst->next = tmp;
- prev = p;
- }
- else
- {
- lst = lst->next;
- /* Keep track of the previous node. We need it if we need
- to swap nodes. */
- if (j != 0)
- prev = prev->next;
- }
- }
-}
-
/* Ask the IPA to probe the marker at ADDRESS. Returns -1 if running
the command fails, or 0 otherwise. If the command ran
successfully, but probing the marker failed, ERROUT will be filled
@@ -2798,6 +2798,83 @@ install_fast_tracepoint (struct tracepoint *tpoint)
return 0;
}
+
+/* Install tracepoint TPOINT, and write reply message in OWN_BUF. */
+
+static void
+install_tracepoint (struct tracepoint *tpoint, char *own_buf)
+{
+ tpoint->handle = NULL;
+ *own_buf = '\0';
+
+ if (tpoint->type == trap_tracepoint)
+ {
+ /* Tracepoints are installed as memory breakpoints. Just go
+ ahead and install the trap. The breakpoints module
+ handles duplicated breakpoints, and the memory read
+ routine handles un-patching traps from memory reads. */
+ tpoint->handle = set_breakpoint_at (tpoint->address,
+ tracepoint_handler);
+ }
+ else if (tpoint->type == fast_tracepoint || tpoint->type == static_tracepoint)
+ {
+ struct tracepoint *tp;
+
+ if (!in_process_agent_loaded ())
+ {
+ trace_debug ("Requested a %s tracepoint, but fast "
+ "tracepoints aren't supported.",
+ tpoint->type == static_tracepoint ? "static" : "fast");
+ write_e_ipa_not_loaded (own_buf);
+ return;
+ }
+ if (tpoint->type == static_tracepoint && !in_process_agent_loaded_ust ())
+ {
+ trace_debug ("Requested a static tracepoint, but static "
+ "tracepoints are not supported.");
+ write_e_ust_not_loaded (own_buf);
+ return;
+ }
+
+ /* Find another fast or static tracepoint at the same address. */
+ for (tp = tracepoints; tp; tp = tp->next)
+ {
+ if (tp->address == tpoint->address && tp->type == tpoint->type
+ && tp->number != tpoint->number)
+ break;
+ }
+
+ if (tpoint->type == fast_tracepoint)
+ {
+ if (tp) /* TPOINT is installed at the same address as TP. */
+ clone_fast_tracepoint (tpoint, tp);
+ else
+ install_fast_tracepoint (tpoint);
+ }
+ else
+ {
+ if (tp)
+ tpoint->handle = (void *) -1;
+ else
+ {
+ if (probe_marker_at (tpoint->address, own_buf) == 0)
+ tpoint->handle = (void *) -1;
+ }
+ }
+
+ }
+ else
+ internal_error (__FILE__, __LINE__, "Unknown tracepoint type");
+
+ if (tpoint->handle == NULL)
+ {
+ if (*own_buf == '\0')
+ write_enn (own_buf);
+ }
+ else
+ write_ok (own_buf);
+}
+
static void
cmd_qtstart (char *packet)
{
@@ -2805,10 +2882,6 @@ cmd_qtstart (char *packet)
trace_debug ("Starting the trace");
- /* Sort tracepoints by ascending address. This makes installing
- fast tracepoints at the same address easier to handle. */
- sort_tracepoints ();
-
/* Pause all threads temporarily while we patch tracepoints. */
pause_all (0);
@@ -6462,6 +6535,53 @@ download_tracepoint_1 (struct tracepoint *tpoint)
}
static void
+download_tracepoint (struct tracepoint *tpoint)
+{
+ struct tracepoint *tp, *tp_prev;
+
+ if (tpoint->type != fast_tracepoint
+ && tpoint->type != static_tracepoint)
+ return;
+
+ download_tracepoint_1 (tpoint);
+
+ /* Find the previous entry of TPOINT, which is fast tracepoint or
+ static tracepoint. */
+ tp_prev = NULL;
+ for (tp = tracepoints; tp != tpoint; tp = tp->next)
+ {
+ if (tp->type == fast_tracepoint || tp->type == static_tracepoint)
+ tp_prev = tp;
+ }
+
+ if (tp_prev)
+ {
+ CORE_ADDR tp_prev_target_next_addr;
+
+ /* Insert TPOINT after TP_PREV in IPA. */
+ if (read_inferior_data_pointer (tp_prev->obj_addr_on_target
+ + offsetof (struct tracepoint, next),
+ &tp_prev_target_next_addr))
+ fatal ("error reading `tp_prev->next'");
+
+ /* tpoint->next = tp_prev->next */
+ write_inferior_data_ptr (tpoint->obj_addr_on_target
+ + offsetof (struct tracepoint, next),
+ tp_prev_target_next_addr);
+ /* tp_prev->next = tpoint */
+ write_inferior_data_ptr (tp_prev->obj_addr_on_target
+ + offsetof (struct tracepoint, next),
+ tpoint->obj_addr_on_target);
+ }
+ else
+ /* First object in list, set the head pointer in the
+ inferior. */
+ write_inferior_data_ptr (ipa_sym_addrs.addr_tracepoints,
+ tpoint->obj_addr_on_target);
+
+}
+
+static void
download_tracepoints (void)
{
CORE_ADDR tpptr = 0, prev_tpptr = 0;
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 6/8] gdbserver - Install tracepoint when tracing is running
2011-11-12 2:40 ` Yao Qi
@ 2011-11-14 12:32 ` Pedro Alves
0 siblings, 0 replies; 31+ messages in thread
From: Pedro Alves @ 2011-11-14 12:32 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On Saturday 12 November 2011 02:39:43, Yao Qi wrote:
> * server.c (handle_query): Handle InstallInTrace for qSupported.
> * tracepoint.c (add_tracepoint): Sort list.
> (install_tracepoint, download_tracepoint): New.
> (cmd_qtdp): Call them to install and download tracepoints.
> (sort_tracepoints): Removed.
> (cmd_qtstart): Update.
Okay.
--
Pedro Alves
^ permalink raw reply [flat|nested] 31+ messages in thread
* [patch 7/8] Documentation changes
2011-11-08 6:00 [patch 0/8] Download tracepoint locations when tracing is running Yao Qi
` (5 preceding siblings ...)
2011-11-08 6:40 ` [patch 6/8] gdbserver - Install tracepoint when tracing is running Yao Qi
@ 2011-11-08 6:43 ` Yao Qi
2011-11-08 8:37 ` Eli Zaretskii
2011-11-10 17:02 ` Pedro Alves
2011-11-08 6:56 ` [patch 8/8] Test cases Yao Qi
7 siblings, 2 replies; 31+ messages in thread
From: Yao Qi @ 2011-11-08 6:43 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 141 bytes --]
This patch is about documentation changes to reflect 1) the changes on
tracepoint behavior, 2) a new remote feature.
--
Yao (é½å°§)
[-- Attachment #2: 0007-doc-InstallInTrace.patch --]
[-- Type: text/x-patch, Size: 1700 bytes --]
gdb/doc/
* gdb.texinfo (Create and Delete Tracepoints): Describe changed
behavior of tracepoint.
(General Query Packets): New feature InstallInTrace.
---
gdb/doc/gdb.texinfo | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 520360f..2ed00bf 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -10330,7 +10330,11 @@ an address in the target program. @xref{Specify Location}. The
@code{trace} command defines a tracepoint, which is a point in the
target program where the debugger will briefly stop, collect some
data, and then allow the program to continue. Setting a tracepoint or
-changing its actions doesn't take effect until the next @code{tstart}
+changing its actions takes effect immediately, which depends on the
+remote stub's support of @samp{InstallInTrace} feature (see
+@ref{install tracepoint in tracing})for remote targets.
+If remote stub doesn't support @samp{InstallInTrace} feature, all these
+changes don't take effect until the next @code{tstart}
command, and once a trace experiment is running, further changes will
not have any effect until the next trace experiment starts.
@@ -34845,6 +34849,10 @@ The remote stub understands the @samp{QDisableRandomization} packet.
@cindex static tracepoints, in remote protocol
The remote stub supports static tracepoints.
+@item InstallInTrace
+@anchor{install tracepoint in tracing}
+The remote stub supports installing tracepoint in tracing.
+
@item EnableDisableTracepoints
The remote stub supports the @samp{QTEnable} (@pxref{QTEnable}) and
@samp{QTDisable} (@pxref{QTDisable}) packets that allow tracepoints
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 7/8] Documentation changes
2011-11-08 6:43 ` [patch 7/8] Documentation changes Yao Qi
@ 2011-11-08 8:37 ` Eli Zaretskii
2011-11-10 17:02 ` Pedro Alves
1 sibling, 0 replies; 31+ messages in thread
From: Eli Zaretskii @ 2011-11-08 8:37 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
> Date: Tue, 08 Nov 2011 14:43:18 +0800
> From: Yao Qi <yao@codesourcery.com>
>
> This patch is about documentation changes to reflect 1) the changes on
> tracepoint behavior, 2) a new remote feature.
Thanks. A few comments about this part:
> data, and then allow the program to continue. Setting a tracepoint or
> -changing its actions doesn't take effect until the next @code{tstart}
> +changing its actions takes effect immediately, which depends on the
> +remote stub's support of @samp{InstallInTrace} feature
I think you meant something like this:
Setting a tracepoint or changing its actions takes effect
immediately if the remote stub supports the @samp{InstallInTrace}
feature.
> (see
> +@ref{install tracepoint in tracing})for remote targets.
Please use @pxref here, which does TRT for a cross-reference in
parentheses; @ref doesn't (it needs a comma or a period after the
closing brace; makeinfo will bitch at you if you don't put one or the
other).
> +If remote stub doesn't support @samp{InstallInTrace} feature, all these
^^
"the" is missing here.
Okay with these changes.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 7/8] Documentation changes
2011-11-08 6:43 ` [patch 7/8] Documentation changes Yao Qi
2011-11-08 8:37 ` Eli Zaretskii
@ 2011-11-10 17:02 ` Pedro Alves
2011-11-12 3:09 ` Yao Qi
1 sibling, 1 reply; 31+ messages in thread
From: Pedro Alves @ 2011-11-10 17:02 UTC (permalink / raw)
To: gdb-patches; +Cc: Yao Qi, Eli Zaretskii
The new "set remote install-in-trace-packet" command should get
an entry in the table below:
For each packet @var{name}, the command to enable or disable the
packet is @code{set remote @var{name}-packet}. The available settings
are:"
In the "Remote Configuration" node.
--
Pedro Alves
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 7/8] Documentation changes
2011-11-10 17:02 ` Pedro Alves
@ 2011-11-12 3:09 ` Yao Qi
0 siblings, 0 replies; 31+ messages in thread
From: Yao Qi @ 2011-11-12 3:09 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches, Eli Zaretskii
[-- Attachment #1: Type: text/plain, Size: 411 bytes --]
On 11/11/2011 01:02 AM, Pedro Alves wrote:
> The new "set remote install-in-trace-packet" command should get
> an entry in the table below:
>
> For each packet @var{name}, the command to enable or disable the
> packet is @code{set remote @var{name}-packet}. The available settings
> are:"
>
> In the "Remote Configuration" node.
>
Added one row in table for install-in-trace.
--
Yao (é½å°§)
[-- Attachment #2: 0007-doc-InstallInTrace.patch --]
[-- Type: text/x-patch, Size: 2039 bytes --]
* gdb.texinfo (Create and Delete Tracepoints): Describe changed
behavior of tracepoint.
(General Query Packets): New feature InstallInTrace.
(Remote Configuration): Document "set remote
install-in-trace-packet".
---
gdb/doc/gdb.texinfo | 14 +++++++++++++-
1 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 520360f..e45f75f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -10330,7 +10330,11 @@ an address in the target program. @xref{Specify Location}. The
@code{trace} command defines a tracepoint, which is a point in the
target program where the debugger will briefly stop, collect some
data, and then allow the program to continue. Setting a tracepoint or
-changing its actions doesn't take effect until the next @code{tstart}
+changing its actions takes effect immediately if the remote stub
+supports the @samp{InstallInTrace} feature (@pxref{install tracepoint
+in tracing}).
+If remote stub doesn't support the @samp{InstallInTrace} feature, all
+these changes don't take effect until the next @code{tstart}
command, and once a trace experiment is running, further changes will
not have any effect until the next trace experiment starts.
@@ -17316,6 +17320,10 @@ are:
@tab @code{qXfer:traceframe-info:read}
@tab Traceframe info
+@item @code{install-in-trace}
+@tab @code{InstallInTrace}
+@tab Install tracepoint in tracing
+
@item @code{disable-randomization}
@tab @code{QDisableRandomization}
@tab @code{set disable-randomization}
@@ -34845,6 +34853,10 @@ The remote stub understands the @samp{QDisableRandomization} packet.
@cindex static tracepoints, in remote protocol
The remote stub supports static tracepoints.
+@item InstallInTrace
+@anchor{install tracepoint in tracing}
+The remote stub supports installing tracepoint in tracing.
+
@item EnableDisableTracepoints
The remote stub supports the @samp{QTEnable} (@pxref{QTEnable}) and
@samp{QTDisable} (@pxref{QTDisable}) packets that allow tracepoints
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread
* [patch 8/8] Test cases
2011-11-08 6:00 [patch 0/8] Download tracepoint locations when tracing is running Yao Qi
` (6 preceding siblings ...)
2011-11-08 6:43 ` [patch 7/8] Documentation changes Yao Qi
@ 2011-11-08 6:56 ` Yao Qi
2011-11-10 17:16 ` Pedro Alves
2011-11-10 18:28 ` Pedro Alves
7 siblings, 2 replies; 31+ messages in thread
From: Yao Qi @ 2011-11-08 6:56 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 587 bytes --]
These new test cases are to verify that tracepoint changed after
`tstart' still work properly. There are two FAILs in change-loc.exp on
x86_64-linux, but they are not related to this patch set. They are
caused by an existing problem that jmp insn is incorrectly generated in
jump pad if offset exceeds the limit of integer (32-bit). It could
happen on x86_64 system.
I opened PR 13392, and KFAIL'ed them.
KFAIL: gdb.trace/change-loc.exp: 1 trace: tfind frame 0 (PRMS: gdb/13392)
KFAIL: gdb.trace/change-loc.exp: 1 ftrace: tfind frame 0 (PRMS: gdb/13392)
--
Yao (é½å°§)
[-- Attachment #2: 0008-testsuite-tracepoint-change-loc.patch --]
[-- Type: text/x-patch, Size: 19357 bytes --]
2011-11-08 Yao Qi <yao@codesourcery.com>
* gdb.trace/change-loc-1.c: New.
* gdb.trace/change-loc-2.c: New.
* gdb.trace/change-loc.c: New.
* gdb.trace/change-loc.exp: New.
* gdb.trace/change-loc.h: New.
* gdb.trace/trace-break.c (marker): Define new symbol.
* gdb.trace/trace-break.exp (break_trace_same_addr_5):
New.
(break_trace_same_addr_6): New.
---
gdb/testsuite/gdb.trace/change-loc-1.c | 29 +++++
gdb/testsuite/gdb.trace/change-loc-2.c | 24 ++++
gdb/testsuite/gdb.trace/change-loc.c | 53 +++++++++
gdb/testsuite/gdb.trace/change-loc.exp | 157 +++++++++++++++++++++++++
gdb/testsuite/gdb.trace/change-loc.h | 42 +++++++
gdb/testsuite/gdb.trace/trace-break.c | 7 +
gdb/testsuite/gdb.trace/trace-break.exp | 193 +++++++++++++++++++++++++++++++
7 files changed, 505 insertions(+), 0 deletions(-)
create mode 100644 gdb/testsuite/gdb.trace/change-loc-1.c
create mode 100644 gdb/testsuite/gdb.trace/change-loc-2.c
create mode 100644 gdb/testsuite/gdb.trace/change-loc.c
create mode 100644 gdb/testsuite/gdb.trace/change-loc.exp
create mode 100644 gdb/testsuite/gdb.trace/change-loc.h
diff --git a/gdb/testsuite/gdb.trace/change-loc-1.c b/gdb/testsuite/gdb.trace/change-loc-1.c
new file mode 100644
index 0000000..92d453c
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/change-loc-1.c
@@ -0,0 +1,29 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2011 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include "change-loc.h"
+
+void func1 (int x)
+{
+ int y = x + 4;
+ func4 ();
+}
+
+void func (int x)
+{
+ func1 (x);
+}
diff --git a/gdb/testsuite/gdb.trace/change-loc-2.c b/gdb/testsuite/gdb.trace/change-loc-2.c
new file mode 100644
index 0000000..d479917
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/change-loc-2.c
@@ -0,0 +1,24 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2011 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include "change-loc.h"
+
+void
+func2 (int x)
+{
+ func4 ();
+}
diff --git a/gdb/testsuite/gdb.trace/change-loc.c b/gdb/testsuite/gdb.trace/change-loc.c
new file mode 100644
index 0000000..d1e0a7f
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/change-loc.c
@@ -0,0 +1,53 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2011 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <dlfcn.h>
+#include "change-loc.h"
+
+extern void func (int x);
+
+static void
+marker () {}
+
+int main()
+{
+ const char *libname = "change-loc-2.sl";
+ void *h;
+ int (*p_func) (int);
+
+ func (3);
+
+ func4 ();
+
+ marker ();
+
+ h = dlopen (libname, RTLD_LAZY);
+ if (h == NULL) return 1;
+
+ p_func = dlsym (h, "func2");
+ if (p_func == NULL) return 2;
+
+ (*p_func) (4);
+
+ marker ();
+
+ dlclose (h);
+
+ marker ();
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/change-loc.exp b/gdb/testsuite/gdb.trace/change-loc.exp
new file mode 100644
index 0000000..6425a26
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/change-loc.exp
@@ -0,0 +1,157 @@
+# Copyright 2011 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "trace-support.exp";
+
+if $tracelevel then {
+ strace $tracelevel
+}
+
+#
+# test running programs
+#
+
+if {[skip_shlib_tests]} {
+ return 0
+}
+
+set testfile "change-loc"
+set libfile1 "change-loc-1"
+set libfile2 "change-loc-2"
+set srcfile $testfile.c
+set executable $testfile
+set libsrc1 $srcdir/$subdir/$libfile1.c
+set libsrc2 $srcdir/$subdir/$libfile2.c
+set binfile $objdir/$subdir/$testfile
+set lib_sl1 $objdir/$subdir/$libfile1.sl
+set lib_sl2 $objdir/$subdir/$libfile2.sl
+
+set lib_opts debug
+
+if [get_compiler_info ${binfile}] {
+ return -1
+}
+
+# Some targets have leading underscores on assembly symbols.
+set additional_flags [list debug shlib=$lib_sl1 shlib_load [gdb_target_symbol_prefix_flags]]
+
+if { [gdb_compile_shlib $libsrc1 $lib_sl1 $lib_opts] != ""
+ || [gdb_compile_shlib $libsrc2 $lib_sl2 $lib_opts] != ""
+ || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags] != ""} {
+ untested "Could not compile either $libsrc1 or $srcdir/$subdir/$srcfile."
+ return -1
+}
+
+clean_restart $executable
+
+gdb_load_shlibs $lib_sl1
+gdb_load_shlibs $lib_sl2
+
+if ![runto_main] {
+ fail "Can't run to main to check for trace support"
+ return -1
+}
+
+if { ![gdb_target_supports_trace] } then {
+ unsupported "Current target does not support trace"
+ return -1;
+}
+
+if [is_amd64_regs_target] {
+ set pcreg "rip"
+} elseif [is_x86_like_target] {
+ set pcreg "eip"
+} else {
+ set pcreg "pc"
+}
+
+
+# Set tracepoint during tracing experiment.
+
+proc tracepoint_change_loc_1 { trace_type } {
+ global testfile
+ global srcfile
+ global pcreg
+ global gdb_prompt
+ global pf_prefix
+
+ set old_pf_prefix $pf_prefix
+ set pf_prefix "$pf_prefix 1 $trace_type:"
+
+ clean_restart ${testfile}
+ if ![runto_main] {
+ fail "Can't run to main"
+ set pf_prefix $old_pf_prefix
+ return -1
+ }
+ gdb_test_no_output "delete break 1"
+
+ # Set a tracepoint we'll never meet. Just to avoid the complain after
+ # type `tstart' later.
+ gdb_test "next" ".*"
+ gdb_test "trace main" "Tracepoint \[0-9\] at.* file .*$srcfile, line.*" \
+ "set tracepoint on main"
+
+ gdb_test "break marker" "Breakpoint.*at.* file .*$srcfile, line.*" \
+ "breakpoint on marker"
+
+ gdb_test_no_output "tstart"
+
+ gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \
+ "continue to marker 1"
+ # Set a tracepoint during tracing.
+ gdb_test "${trace_type} set_tracepoint" ".*" "set tracepoint on set_tracepoint"
+
+ gdb_trace_setactions "set action for tracepoint" "" \
+ "collect \$$pcreg" "^$"
+
+ # tracepoint has two locations after shlib change-loc-1 is loaded.
+ gdb_test "info trace" \
+ "Num Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+\(|fast \)tracepoint\[ \]+keep y.*\<MULTIPLE\>.*4\.1.* in func4.*4\.2.* in func4.*" \
+ "tracepoint with two locations"
+
+ setup_kfail "gdb/13392" x86_64-*-*
+ gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \
+ "continue to marker 2"
+
+ # tracepoint has three locations after shlib change-loc-2 is loaded.
+ gdb_test "info trace" \
+ "Num Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+\(|fast \)tracepoint\[ \]+keep y.*\<MULTIPLE\>.*4\.1.* in func4.*4\.2.* in func4.*4\.3.* in func4 .*" \
+ "tracepoint with three locations"
+
+ gdb_test_no_output "tstop"
+
+ setup_kfail "gdb/13392" x86_64-*-*
+ gdb_test "tfind" "Found trace frame 0, tracepoint 4.*" "tfind frame 0"
+ gdb_test "tfind" "Target failed to find requested trace frame\\..*"
+
+ set pf_prefix $old_pf_prefix
+}
+
+
+tracepoint_change_loc_1 "trace"
+
+# Re-compile test case with IPA.
+set libipa $objdir/../gdbserver/libinproctrace.so
+gdb_load_shlibs $libipa
+
+if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable \
+ [list debug nowarnings shlib=$libipa shlib=$lib_sl1 shlib_load] ] != "" } {
+ untested change-loc.exp
+ return -1
+}
+
+tracepoint_change_loc_1 "ftrace"
diff --git a/gdb/testsuite/gdb.trace/change-loc.h b/gdb/testsuite/gdb.trace/change-loc.h
new file mode 100644
index 0000000..1b0e303
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/change-loc.h
@@ -0,0 +1,42 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2011 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifdef SYMBOL_PREFIX
+#define SYMBOL(str) SYMBOL_PREFIX #str
+#else
+#define SYMBOL(str) #str
+#endif
+
+/* Called from asm. */
+static void __attribute__((used))
+func5 (void)
+{}
+
+static void
+func4 (void)
+{
+ /* `set_tracepoint' is the label where we'll set multiple tracepoints and
+ breakpoints at. The insn at the label must the large enough to
+ fit a fast tracepoint jump. */
+ asm (" .global " SYMBOL(set_tracepoint) "\n"
+ SYMBOL(set_tracepoint) ":\n"
+#if (defined __x86_64__ || defined __i386__)
+ " call " SYMBOL(func5) "\n"
+#endif
+ );
+
+}
diff --git a/gdb/testsuite/gdb.trace/trace-break.c b/gdb/testsuite/gdb.trace/trace-break.c
index fd06142..a327202 100644
--- a/gdb/testsuite/gdb.trace/trace-break.c
+++ b/gdb/testsuite/gdb.trace/trace-break.c
@@ -43,6 +43,13 @@ marker (void)
" call " SYMBOL(func) "\n"
#endif
);
+
+ asm (" .global " SYMBOL(after_set_point) "\n"
+ SYMBOL(after_set_point) ":\n"
+#if (defined __x86_64__ || defined __i386__)
+ " call " SYMBOL(func) "\n"
+#endif
+ );
}
static void
diff --git a/gdb/testsuite/gdb.trace/trace-break.exp b/gdb/testsuite/gdb.trace/trace-break.exp
index c2d7b2c..b80f7cd 100644
--- a/gdb/testsuite/gdb.trace/trace-break.exp
+++ b/gdb/testsuite/gdb.trace/trace-break.exp
@@ -39,6 +39,20 @@ if ![gdb_target_supports_trace] {
return -1;
}
+set fpreg "fp"
+set spreg "sp"
+set pcreg "pc"
+
+if [is_amd64_regs_target] {
+ set fpreg "rbp"
+ set spreg "rsp"
+ set pcreg "rip"
+} elseif [is_x86_like_target] {
+ set fpreg "ebp"
+ set spreg "esp"
+ set pcreg "eip"
+}
+
# Set breakpoint and tracepoint at the same address.
proc break_trace_same_addr_1 { trace_type option } {
@@ -200,6 +214,150 @@ proc break_trace_same_addr_4 { trace_type option } {
set pf_prefix $old_pf_prefix
}
+# Set two tracepoints TRACE1 and TRACE2 at two locations, and start tracing.
+# Then, set tracepoint TRACE3 at either of these two locations.
+# TRACE3_AT_FIRST_LOC is a boolean variable to decide insert TRACE3 at which
+# of two locations. Verify these tracepoints work as expected.
+
+proc break_trace_same_addr_5 { trace1 trace2 trace3 trace3_at_first_loc } {
+ global executable
+ global pf_prefix
+ global hex
+ global fpreg
+ global spreg
+ global pcreg
+
+ set old_pf_prefix $pf_prefix
+ set pf_prefix "$pf_prefix 5 $trace1 $trace2 ${trace3}@${trace3_at_first_loc}:"
+
+ # Start with a fresh gdb.
+ clean_restart ${executable}
+ if ![runto_main] {
+ fail "Can't run to main"
+ set pf_prefix $old_pf_prefix
+ return -1
+ }
+
+ gdb_test "break marker" "Breakpoint \[0-9\] at $hex: file.*"
+ gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+ gdb_test "${trace1} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+ gdb_trace_setactions "set action for tracepoint 1" "" \
+ "collect \$$pcreg" "^$"
+ gdb_test "${trace2} after_set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+ gdb_trace_setactions "set action for tracepoint 2" "" \
+ "collect \$$spreg" "^$"
+
+ gdb_test_no_output "tstart"
+
+ gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
+
+ if [string equal $trace3_at_first_loc "1"] {
+ gdb_test "${trace3} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+ } else {
+ gdb_test "${trace3} after_set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+ }
+ gdb_trace_setactions "set action for tracepoint 3" "" \
+ "collect \$$fpreg" "^$"
+
+ gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+ gdb_test_no_output "tstop"
+
+ gdb_test "tfind tracepoint 4" "Found trace frame \[0-9\], tracepoint .*" \
+ "tfind test frame of tracepoint 4"
+ gdb_test "tdump" \
+ "Data collected at tracepoint .*, trace frame \[0-9\]:.*\\$${pcreg} = .*" \
+ "tdump 1"
+ gdb_test "tfind 0" "Found trace frame 0, tracepoint .*" \
+ "reset to frame 0 (1)"
+ gdb_test "tfind tracepoint 5" "Found trace frame \[0-9\], tracepoint .*" \
+ "tfind test frame of tracepoint 5"
+ gdb_test "tdump" \
+ "Data collected at tracepoint .*, trace frame \[0-9\]:.*\\$${spreg} = .*" \
+ "tdump 2"
+ gdb_test "tfind 0" "Found trace frame 0, tracepoint .*" \
+ "reset to frame 0 (2)"
+ gdb_test "tfind tracepoint 6" "Found trace frame \[0-9\], tracepoint .*" \
+ "tfind test frame of tracepoint 6"
+ gdb_test "tdump" \
+ "Data collected at tracepoint .*, trace frame \[0-9\]:.*\\$${fpreg} = .*" \
+ "tdump 3"
+
+ set pf_prefix $old_pf_prefix
+}
+
+# Set two tracepoints at the same address, and enable/disable them. Verify
+# tracepoints work as expect.
+
+proc break_trace_same_addr_6 { trace1 enable1 trace2 enable2 } {
+ global executable
+ global pf_prefix
+ global hex
+ global gdb_prompt
+ global spreg
+ global pcreg
+
+ set old_pf_prefix $pf_prefix
+ set pf_prefix "$pf_prefix 6 $trace1 $enable1 $trace2 $enable2:"
+
+ # Start with a fresh gdb.
+ clean_restart ${executable}
+ if ![runto_main] {
+ fail "Can't run to main"
+ set pf_prefix $old_pf_prefix
+ return -1
+ }
+
+ gdb_test "break marker" "Breakpoint \[0-9\] at $hex: file.*"
+ gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+ gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
+
+ gdb_test "${trace1} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+ gdb_trace_setactions "set action for tracepoint 1" "" \
+ "collect \$$pcreg" "^$"
+ gdb_test "${trace2} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+ gdb_trace_setactions "set action for tracepoint 2" "" \
+ "collect \$$spreg" "^$"
+
+ gdb_test_no_output "$enable1 4"
+ gdb_test_no_output "$enable2 5"
+
+ gdb_test_no_output "tstart"
+ gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+ gdb_test_no_output "tstop"
+
+
+ if [string equal $enable1 "enable"] {
+ gdb_test "tfind tracepoint 4" "Found trace frame \[0-9\], tracepoint .*" \
+ "tfind test frame of tracepoint 4"
+ gdb_test "tdump" \
+ "Data collected at tracepoint .*, trace frame \[0-9\]:.*\\$${pcreg} = .*" \
+ "tdump 1"
+ gdb_test "tfind 0" "Found trace frame 0, tracepoint .*" \
+ "reset to frame 0 (1)"
+ } else {
+ gdb_test "tfind tracepoint 4" "Target failed to find requested trace frame.*" \
+ "tfind test frame of tracepoint 4"
+ }
+
+ if [string equal $enable2 "enable"] {
+ gdb_test "tfind tracepoint 5" "Found trace frame \[0-9\], tracepoint .*" \
+ "tfind test frame of tracepoint 5"
+ gdb_test "tdump" \
+ "Data collected at tracepoint .*, trace frame \[0-9\]:.*\\$${spreg} = .*" \
+ "tdump 2"
+ gdb_test "tfind 0" "Found trace frame 0, tracepoint .*" \
+ "reset to frame 0 (2)"
+ } else {
+ gdb_test "tfind tracepoint 5" "Target failed to find requested trace frame.*" \
+ "tfind test frame of tracepoint 5"
+ }
+
+ set pf_prefix $old_pf_prefix
+}
+
+
foreach break_always_inserted { "on" "off" } {
break_trace_same_addr_1 "trace" ${break_always_inserted}
break_trace_same_addr_2 "trace" "trace" ${break_always_inserted}
@@ -207,6 +365,13 @@ foreach break_always_inserted { "on" "off" } {
break_trace_same_addr_4 "trace" ${break_always_inserted}
}
+foreach at_first_loc { "1" "0" } {
+ break_trace_same_addr_5 "trace" "trace" "trace" ${at_first_loc}
+}
+
+break_trace_same_addr_6 "trace" "enable" "trace" "disable"
+break_trace_same_addr_6 "trace" "disable" "trace" "enable"
+
set libipa $objdir/../gdbserver/libinproctrace.so
gdb_load_shlibs $libipa
@@ -238,4 +403,32 @@ if { [gdb_test "info sharedlibrary" ".*libinproctrace\.so.*" "IPA loaded"] != 0
break_trace_same_addr_3 "ftrace" ${break_always_inserted}
break_trace_same_addr_4 "ftrace" ${break_always_inserted}
}
+
+ foreach trace1 { "trace" "ftrace" } {
+ foreach trace2 { "trace" "ftrace" } {
+ foreach trace3 { "trace" "ftrace" } {
+
+ if { [string equal $trace1 "trace"]
+ && [string equal $trace2 "trace"]
+ && [string equal $trace3 "trace"] } {
+ continue
+ }
+
+ foreach at_first_loc { "1" "0" } {
+ break_trace_same_addr_5 $trace1 $trace2 $trace3 $at_first_loc
+ }
+ }
+ }
+ }
+
+ foreach trace1 { "trace" "ftrace" } {
+ foreach trace2 { "trace" "ftrace" } {
+ if { [string equal $trace1 "trace"]
+ && [string equal $trace2 "trace"] } {
+ continue
+ }
+ break_trace_same_addr_6 $trace1 "enable" $trace2 "disable"
+ break_trace_same_addr_6 $trace1 "disable" $trace2 "enable"
+ }
+ }
}
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 8/8] Test cases
2011-11-08 6:56 ` [patch 8/8] Test cases Yao Qi
@ 2011-11-10 17:16 ` Pedro Alves
2011-11-10 18:28 ` Pedro Alves
1 sibling, 0 replies; 31+ messages in thread
From: Pedro Alves @ 2011-11-10 17:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Yao Qi
On Tuesday 08 November 2011 06:55:26, Yao Qi wrote:
> +
> +#
> +# test running programs
> +#
> +
This is IMO one of the funniest example of copy&paste-programing
we have on the testsuite. :-)
$ grep -rn "test running programs" * | wc -l
77
I'd guess that string appeared on one of the first tests ever
written for GDB (that actually tested basic functionality as
running programs), and then spread around as people copied tests
as a starting base rather than start from scratch. I do that to!
Proper review coming...
--
Pedro Alves
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [patch 8/8] Test cases
2011-11-08 6:56 ` [patch 8/8] Test cases Yao Qi
2011-11-10 17:16 ` Pedro Alves
@ 2011-11-10 18:28 ` Pedro Alves
2011-11-12 3:35 ` Yao Qi
1 sibling, 1 reply; 31+ messages in thread
From: Pedro Alves @ 2011-11-10 18:28 UTC (permalink / raw)
To: gdb-patches; +Cc: Yao Qi
On Tuesday 08 November 2011 06:55:26, Yao Qi wrote:
>
> These new test cases are to verify that tracepoint changed after
> `tstart' still work properly. There are two FAILs in change-loc.exp on
> x86_64-linux, but they are not related to this patch set. They are
> caused by an existing problem that jmp insn is incorrectly generated in
> jump pad if offset exceeds the limit of integer (32-bit). It could
> happen on x86_64 system.
We were meant to error out in these cases. Guess something's missing.
> +set testfile "change-loc"
> +set libfile1 "change-loc-1"
> +set libfile2 "change-loc-2"
> +set srcfile $testfile.c
> +set executable $testfile
> +set libsrc1 $srcdir/$subdir/$libfile1.c
> +set libsrc2 $srcdir/$subdir/$libfile2.c
Spurious spaces.
> +set binfile $objdir/$subdir/$testfile
> +set lib_sl1 $objdir/$subdir/$libfile1.sl
> +set lib_sl2 $objdir/$subdir/$libfile2.sl
Ditto.
> +
> +set lib_opts debug
Ditto.
Please make sure the new test messages are unique:
$ cat testsuite/gdb.sum | grep "PASS" | sort | uniq -c | sort -n | tail -n 15
1 PASS: gdb.trace/trace-break.exp: 6 trace enable trace disable: tstart
1 PASS: gdb.trace/trace-break.exp: 6 trace enable trace disable: tstop
1 PASS: gdb.trace/trace-break.exp: IPA loaded
2 PASS: gdb.trace/trace-break.exp: 5 ftrace ftrace ftrace@0: ftrace after_set_point
2 PASS: gdb.trace/trace-break.exp: 5 ftrace ftrace ftrace@1: ftrace set_point
2 PASS: gdb.trace/trace-break.exp: 5 ftrace trace ftrace@1: ftrace set_point
2 PASS: gdb.trace/trace-break.exp: 5 ftrace trace trace@0: trace after_set_point
2 PASS: gdb.trace/trace-break.exp: 5 trace ftrace ftrace@0: ftrace after_set_point
2 PASS: gdb.trace/trace-break.exp: 5 trace ftrace trace@1: trace set_point
2 PASS: gdb.trace/trace-break.exp: 5 trace trace trace@0: trace after_set_point
2 PASS: gdb.trace/trace-break.exp: 5 trace trace trace@1: trace set_point
2 PASS: gdb.trace/trace-break.exp: 6 ftrace disable ftrace enable: ftrace set_point
2 PASS: gdb.trace/trace-break.exp: 6 ftrace enable ftrace disable: ftrace set_point
2 PASS: gdb.trace/trace-break.exp: 6 trace disable trace enable: trace set_point
2 PASS: gdb.trace/trace-break.exp: 6 trace enable trace disable: trace set_point
Otherwise okay.
--
Pedro Alves
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 8/8] Test cases
2011-11-10 18:28 ` Pedro Alves
@ 2011-11-12 3:35 ` Yao Qi
2011-11-14 13:00 ` Pedro Alves
0 siblings, 1 reply; 31+ messages in thread
From: Yao Qi @ 2011-11-12 3:35 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 924 bytes --]
On 11/11/2011 02:28 AM, Pedro Alves wrote:
>> > +set executable $testfile
>> > +set libsrc1 $srcdir/$subdir/$libfile1.c
>> > +set libsrc2 $srcdir/$subdir/$libfile2.c
> Spurious spaces.
>
>> > +set binfile $objdir/$subdir/$testfile
>> > +set lib_sl1 $objdir/$subdir/$libfile1.sl
>> > +set lib_sl2 $objdir/$subdir/$libfile2.sl
> Ditto.
>
>> > +
>> > +set lib_opts debug
> Ditto.
>
These spurious spaces are removed.
> Please make sure the new test messages are unique:
>
> $ cat testsuite/gdb.sum | grep "PASS" | sort | uniq -c | sort -n | tail -n 15
> 1 PASS: gdb.trace/trace-break.exp: 6 trace enable trace disable: tstart
I am good at making such duplicated test messages. :) These duplicated
messages are fixed in my test case. I put this tip to wiki, and hope it
is useful to other people.
http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique
--
Yao (é½å°§)
[-- Attachment #2: 0008-testsuite-tracepoint-change-loc.patch --]
[-- Type: text/x-patch, Size: 19422 bytes --]
* gdb.trace/change-loc-1.c: New.
* gdb.trace/change-loc-2.c: New.
* gdb.trace/change-loc.c: New.
* gdb.trace/change-loc.exp: New.
* gdb.trace/change-loc.h: New.
* gdb.trace/trace-break.c (marker): Define new symbol.
* gdb.trace/trace-break.exp (break_trace_same_addr_5):
New.
(break_trace_same_addr_6): New.
---
gdb/testsuite/gdb.trace/change-loc-1.c | 29 +++++
gdb/testsuite/gdb.trace/change-loc-2.c | 24 ++++
gdb/testsuite/gdb.trace/change-loc.c | 53 ++++++++
gdb/testsuite/gdb.trace/change-loc.exp | 153 +++++++++++++++++++++++
gdb/testsuite/gdb.trace/change-loc.h | 42 +++++++
gdb/testsuite/gdb.trace/trace-break.c | 7 +
gdb/testsuite/gdb.trace/trace-break.exp | 202 +++++++++++++++++++++++++++++++
7 files changed, 510 insertions(+), 0 deletions(-)
create mode 100644 gdb/testsuite/gdb.trace/change-loc-1.c
create mode 100644 gdb/testsuite/gdb.trace/change-loc-2.c
create mode 100644 gdb/testsuite/gdb.trace/change-loc.c
create mode 100644 gdb/testsuite/gdb.trace/change-loc.exp
create mode 100644 gdb/testsuite/gdb.trace/change-loc.h
diff --git a/gdb/testsuite/gdb.trace/change-loc-1.c b/gdb/testsuite/gdb.trace/change-loc-1.c
new file mode 100644
index 0000000..92d453c
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/change-loc-1.c
@@ -0,0 +1,29 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2011 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include "change-loc.h"
+
+void func1 (int x)
+{
+ int y = x + 4;
+ func4 ();
+}
+
+void func (int x)
+{
+ func1 (x);
+}
diff --git a/gdb/testsuite/gdb.trace/change-loc-2.c b/gdb/testsuite/gdb.trace/change-loc-2.c
new file mode 100644
index 0000000..d479917
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/change-loc-2.c
@@ -0,0 +1,24 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2011 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include "change-loc.h"
+
+void
+func2 (int x)
+{
+ func4 ();
+}
diff --git a/gdb/testsuite/gdb.trace/change-loc.c b/gdb/testsuite/gdb.trace/change-loc.c
new file mode 100644
index 0000000..d1e0a7f
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/change-loc.c
@@ -0,0 +1,53 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2011 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <dlfcn.h>
+#include "change-loc.h"
+
+extern void func (int x);
+
+static void
+marker () {}
+
+int main()
+{
+ const char *libname = "change-loc-2.sl";
+ void *h;
+ int (*p_func) (int);
+
+ func (3);
+
+ func4 ();
+
+ marker ();
+
+ h = dlopen (libname, RTLD_LAZY);
+ if (h == NULL) return 1;
+
+ p_func = dlsym (h, "func2");
+ if (p_func == NULL) return 2;
+
+ (*p_func) (4);
+
+ marker ();
+
+ dlclose (h);
+
+ marker ();
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/change-loc.exp b/gdb/testsuite/gdb.trace/change-loc.exp
new file mode 100644
index 0000000..e125024
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/change-loc.exp
@@ -0,0 +1,153 @@
+# Copyright 2011 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "trace-support.exp";
+
+if $tracelevel then {
+ strace $tracelevel
+}
+
+if {[skip_shlib_tests]} {
+ return 0
+}
+
+set testfile "change-loc"
+set libfile1 "change-loc-1"
+set libfile2 "change-loc-2"
+set srcfile $testfile.c
+set executable $testfile
+set libsrc1 $srcdir/$subdir/$libfile1.c
+set libsrc2 $srcdir/$subdir/$libfile2.c
+set binfile $objdir/$subdir/$testfile
+set lib_sl1 $objdir/$subdir/$libfile1.sl
+set lib_sl2 $objdir/$subdir/$libfile2.sl
+
+set lib_opts debug
+
+if [get_compiler_info ${binfile}] {
+ return -1
+}
+
+# Some targets have leading underscores on assembly symbols.
+set additional_flags [list debug shlib=$lib_sl1 shlib_load [gdb_target_symbol_prefix_flags]]
+
+if { [gdb_compile_shlib $libsrc1 $lib_sl1 $lib_opts] != ""
+ || [gdb_compile_shlib $libsrc2 $lib_sl2 $lib_opts] != ""
+ || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags] != ""} {
+ untested "Could not compile either $libsrc1 or $srcdir/$subdir/$srcfile."
+ return -1
+}
+
+clean_restart $executable
+
+gdb_load_shlibs $lib_sl1
+gdb_load_shlibs $lib_sl2
+
+if ![runto_main] {
+ fail "Can't run to main to check for trace support"
+ return -1
+}
+
+if { ![gdb_target_supports_trace] } then {
+ unsupported "Current target does not support trace"
+ return -1;
+}
+
+if [is_amd64_regs_target] {
+ set pcreg "rip"
+} elseif [is_x86_like_target] {
+ set pcreg "eip"
+} else {
+ set pcreg "pc"
+}
+
+
+# Set tracepoint during tracing experiment.
+
+proc tracepoint_change_loc_1 { trace_type } {
+ global testfile
+ global srcfile
+ global pcreg
+ global gdb_prompt
+ global pf_prefix
+
+ set old_pf_prefix $pf_prefix
+ set pf_prefix "$pf_prefix 1 $trace_type:"
+
+ clean_restart ${testfile}
+ if ![runto_main] {
+ fail "Can't run to main"
+ set pf_prefix $old_pf_prefix
+ return -1
+ }
+ gdb_test_no_output "delete break 1"
+
+ # Set a tracepoint we'll never meet. Just to avoid the complain after
+ # type `tstart' later.
+ gdb_test "next" ".*"
+ gdb_test "trace main" "Tracepoint \[0-9\] at.* file .*$srcfile, line.*" \
+ "set tracepoint on main"
+
+ gdb_test "break marker" "Breakpoint.*at.* file .*$srcfile, line.*" \
+ "breakpoint on marker"
+
+ gdb_test_no_output "tstart"
+
+ gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \
+ "continue to marker 1"
+ # Set a tracepoint during tracing.
+ gdb_test "${trace_type} set_tracepoint" ".*" "set tracepoint on set_tracepoint"
+
+ gdb_trace_setactions "set action for tracepoint" "" \
+ "collect \$$pcreg" "^$"
+
+ # tracepoint has two locations after shlib change-loc-1 is loaded.
+ gdb_test "info trace" \
+ "Num Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+\(|fast \)tracepoint\[ \]+keep y.*\<MULTIPLE\>.*4\.1.* in func4.*4\.2.* in func4.*" \
+ "tracepoint with two locations"
+
+ setup_kfail "gdb/13392" x86_64-*-*
+ gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \
+ "continue to marker 2"
+
+ # tracepoint has three locations after shlib change-loc-2 is loaded.
+ gdb_test "info trace" \
+ "Num Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+\(|fast \)tracepoint\[ \]+keep y.*\<MULTIPLE\>.*4\.1.* in func4.*4\.2.* in func4.*4\.3.* in func4 .*" \
+ "tracepoint with three locations"
+
+ gdb_test_no_output "tstop"
+
+ setup_kfail "gdb/13392" x86_64-*-*
+ gdb_test "tfind" "Found trace frame 0, tracepoint 4.*" "tfind frame 0"
+ gdb_test "tfind" "Target failed to find requested trace frame\\..*"
+
+ set pf_prefix $old_pf_prefix
+}
+
+
+tracepoint_change_loc_1 "trace"
+
+# Re-compile test case with IPA.
+set libipa $objdir/../gdbserver/libinproctrace.so
+gdb_load_shlibs $libipa
+
+if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable \
+ [list debug nowarnings shlib=$libipa shlib=$lib_sl1 shlib_load] ] != "" } {
+ untested change-loc.exp
+ return -1
+}
+
+tracepoint_change_loc_1 "ftrace"
diff --git a/gdb/testsuite/gdb.trace/change-loc.h b/gdb/testsuite/gdb.trace/change-loc.h
new file mode 100644
index 0000000..1b0e303
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/change-loc.h
@@ -0,0 +1,42 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2011 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifdef SYMBOL_PREFIX
+#define SYMBOL(str) SYMBOL_PREFIX #str
+#else
+#define SYMBOL(str) #str
+#endif
+
+/* Called from asm. */
+static void __attribute__((used))
+func5 (void)
+{}
+
+static void
+func4 (void)
+{
+ /* `set_tracepoint' is the label where we'll set multiple tracepoints and
+ breakpoints at. The insn at the label must the large enough to
+ fit a fast tracepoint jump. */
+ asm (" .global " SYMBOL(set_tracepoint) "\n"
+ SYMBOL(set_tracepoint) ":\n"
+#if (defined __x86_64__ || defined __i386__)
+ " call " SYMBOL(func5) "\n"
+#endif
+ );
+
+}
diff --git a/gdb/testsuite/gdb.trace/trace-break.c b/gdb/testsuite/gdb.trace/trace-break.c
index fd06142..a327202 100644
--- a/gdb/testsuite/gdb.trace/trace-break.c
+++ b/gdb/testsuite/gdb.trace/trace-break.c
@@ -43,6 +43,13 @@ marker (void)
" call " SYMBOL(func) "\n"
#endif
);
+
+ asm (" .global " SYMBOL(after_set_point) "\n"
+ SYMBOL(after_set_point) ":\n"
+#if (defined __x86_64__ || defined __i386__)
+ " call " SYMBOL(func) "\n"
+#endif
+ );
}
static void
diff --git a/gdb/testsuite/gdb.trace/trace-break.exp b/gdb/testsuite/gdb.trace/trace-break.exp
index c2d7b2c..50344d2 100644
--- a/gdb/testsuite/gdb.trace/trace-break.exp
+++ b/gdb/testsuite/gdb.trace/trace-break.exp
@@ -39,6 +39,20 @@ if ![gdb_target_supports_trace] {
return -1;
}
+set fpreg "fp"
+set spreg "sp"
+set pcreg "pc"
+
+if [is_amd64_regs_target] {
+ set fpreg "rbp"
+ set spreg "rsp"
+ set pcreg "rip"
+} elseif [is_x86_like_target] {
+ set fpreg "ebp"
+ set spreg "esp"
+ set pcreg "eip"
+}
+
# Set breakpoint and tracepoint at the same address.
proc break_trace_same_addr_1 { trace_type option } {
@@ -200,6 +214,159 @@ proc break_trace_same_addr_4 { trace_type option } {
set pf_prefix $old_pf_prefix
}
+# Set two tracepoints TRACE1 and TRACE2 at two locations, and start tracing.
+# Then, set tracepoint TRACE3 at either of these two locations.
+# TRACE3_AT_FIRST_LOC is a boolean variable to decide insert TRACE3 at which
+# of two locations. Verify these tracepoints work as expected.
+
+proc break_trace_same_addr_5 { trace1 trace2 trace3 trace3_at_first_loc } {
+ global executable
+ global pf_prefix
+ global hex
+ global fpreg
+ global spreg
+ global pcreg
+
+ set old_pf_prefix $pf_prefix
+ set pf_prefix "$pf_prefix 5 $trace1 $trace2 ${trace3}@${trace3_at_first_loc}:"
+
+ # Start with a fresh gdb.
+ clean_restart ${executable}
+ if ![runto_main] {
+ fail "Can't run to main"
+ set pf_prefix $old_pf_prefix
+ return -1
+ }
+
+ gdb_test "break marker" "Breakpoint \[0-9\] at $hex: file.*"
+ gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+ gdb_test "${trace1} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*" \
+ "${trace1} set_point 1"
+ gdb_trace_setactions "set action for tracepoint 1" "" \
+ "collect \$$pcreg" "^$"
+ gdb_test "${trace2} after_set_point" \
+ "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*" \
+ "${trace2} after_set_point 1"
+
+ gdb_trace_setactions "set action for tracepoint 2" "" \
+ "collect \$$spreg" "^$"
+
+ gdb_test_no_output "tstart"
+
+ gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
+
+ if [string equal $trace3_at_first_loc "1"] {
+ gdb_test "${trace3} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*" \
+ "${trace3} set_point 2"
+ } else {
+ gdb_test "${trace3} after_set_point" \
+ "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*" \
+ "${trace2} after_set_point 2"
+ }
+ gdb_trace_setactions "set action for tracepoint 3" "" \
+ "collect \$$fpreg" "^$"
+
+ gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+ gdb_test_no_output "tstop"
+
+ gdb_test "tfind tracepoint 4" "Found trace frame \[0-9\], tracepoint .*" \
+ "tfind test frame of tracepoint 4"
+ gdb_test "tdump" \
+ "Data collected at tracepoint .*, trace frame \[0-9\]:.*\\$${pcreg} = .*" \
+ "tdump 1"
+ gdb_test "tfind 0" "Found trace frame 0, tracepoint .*" \
+ "reset to frame 0 (1)"
+ gdb_test "tfind tracepoint 5" "Found trace frame \[0-9\], tracepoint .*" \
+ "tfind test frame of tracepoint 5"
+ gdb_test "tdump" \
+ "Data collected at tracepoint .*, trace frame \[0-9\]:.*\\$${spreg} = .*" \
+ "tdump 2"
+ gdb_test "tfind 0" "Found trace frame 0, tracepoint .*" \
+ "reset to frame 0 (2)"
+ gdb_test "tfind tracepoint 6" "Found trace frame \[0-9\], tracepoint .*" \
+ "tfind test frame of tracepoint 6"
+ gdb_test "tdump" \
+ "Data collected at tracepoint .*, trace frame \[0-9\]:.*\\$${fpreg} = .*" \
+ "tdump 3"
+
+ set pf_prefix $old_pf_prefix
+}
+
+# Set two tracepoints at the same address, and enable/disable them. Verify
+# tracepoints work as expect.
+
+proc break_trace_same_addr_6 { trace1 enable1 trace2 enable2 } {
+ global executable
+ global pf_prefix
+ global hex
+ global gdb_prompt
+ global spreg
+ global pcreg
+
+ set old_pf_prefix $pf_prefix
+ set pf_prefix "$pf_prefix 6 $trace1 $enable1 $trace2 $enable2:"
+
+ # Start with a fresh gdb.
+ clean_restart ${executable}
+ if ![runto_main] {
+ fail "Can't run to main"
+ set pf_prefix $old_pf_prefix
+ return -1
+ }
+
+ gdb_test "break marker" "Breakpoint \[0-9\] at $hex: file.*"
+ gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+ gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
+
+ gdb_test "${trace1} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*" \
+ "${trace1} set_point 1"
+ gdb_trace_setactions "set action for tracepoint 1" "" \
+ "collect \$$pcreg" "^$"
+ gdb_test "${trace2} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*" \
+ "${trace2} set_point 2"
+ gdb_trace_setactions "set action for tracepoint 2" "" \
+ "collect \$$spreg" "^$"
+
+ gdb_test_no_output "$enable1 4"
+ gdb_test_no_output "$enable2 5"
+
+ gdb_test_no_output "tstart"
+ gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+ gdb_test_no_output "tstop"
+
+
+ if [string equal $enable1 "enable"] {
+ gdb_test "tfind tracepoint 4" "Found trace frame \[0-9\], tracepoint .*" \
+ "tfind test frame of tracepoint 4"
+ gdb_test "tdump" \
+ "Data collected at tracepoint .*, trace frame \[0-9\]:.*\\$${pcreg} = .*" \
+ "tdump 1"
+ gdb_test "tfind 0" "Found trace frame 0, tracepoint .*" \
+ "reset to frame 0 (1)"
+ } else {
+ gdb_test "tfind tracepoint 4" "Target failed to find requested trace frame.*" \
+ "tfind test frame of tracepoint 4"
+ }
+
+ if [string equal $enable2 "enable"] {
+ gdb_test "tfind tracepoint 5" "Found trace frame \[0-9\], tracepoint .*" \
+ "tfind test frame of tracepoint 5"
+ gdb_test "tdump" \
+ "Data collected at tracepoint .*, trace frame \[0-9\]:.*\\$${spreg} = .*" \
+ "tdump 2"
+ gdb_test "tfind 0" "Found trace frame 0, tracepoint .*" \
+ "reset to frame 0 (2)"
+ } else {
+ gdb_test "tfind tracepoint 5" "Target failed to find requested trace frame.*" \
+ "tfind test frame of tracepoint 5"
+ }
+
+ set pf_prefix $old_pf_prefix
+}
+
+
foreach break_always_inserted { "on" "off" } {
break_trace_same_addr_1 "trace" ${break_always_inserted}
break_trace_same_addr_2 "trace" "trace" ${break_always_inserted}
@@ -207,6 +374,13 @@ foreach break_always_inserted { "on" "off" } {
break_trace_same_addr_4 "trace" ${break_always_inserted}
}
+foreach at_first_loc { "1" "0" } {
+ break_trace_same_addr_5 "trace" "trace" "trace" ${at_first_loc}
+}
+
+break_trace_same_addr_6 "trace" "enable" "trace" "disable"
+break_trace_same_addr_6 "trace" "disable" "trace" "enable"
+
set libipa $objdir/../gdbserver/libinproctrace.so
gdb_load_shlibs $libipa
@@ -238,4 +412,32 @@ if { [gdb_test "info sharedlibrary" ".*libinproctrace\.so.*" "IPA loaded"] != 0
break_trace_same_addr_3 "ftrace" ${break_always_inserted}
break_trace_same_addr_4 "ftrace" ${break_always_inserted}
}
+
+ foreach trace1 { "trace" "ftrace" } {
+ foreach trace2 { "trace" "ftrace" } {
+ foreach trace3 { "trace" "ftrace" } {
+
+ if { [string equal $trace1 "trace"]
+ && [string equal $trace2 "trace"]
+ && [string equal $trace3 "trace"] } {
+ continue
+ }
+
+ foreach at_first_loc { "1" "0" } {
+ break_trace_same_addr_5 $trace1 $trace2 $trace3 $at_first_loc
+ }
+ }
+ }
+ }
+
+ foreach trace1 { "trace" "ftrace" } {
+ foreach trace2 { "trace" "ftrace" } {
+ if { [string equal $trace1 "trace"]
+ && [string equal $trace2 "trace"] } {
+ continue
+ }
+ break_trace_same_addr_6 $trace1 "enable" $trace2 "disable"
+ break_trace_same_addr_6 $trace1 "disable" $trace2 "enable"
+ }
+ }
}
--
1.7.0.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [patch 8/8] Test cases
2011-11-12 3:35 ` Yao Qi
@ 2011-11-14 13:00 ` Pedro Alves
0 siblings, 0 replies; 31+ messages in thread
From: Pedro Alves @ 2011-11-14 13:00 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On Saturday 12 November 2011 03:35:09, Yao Qi wrote:
> On 11/11/2011 02:28 AM, Pedro Alves wrote:
> > Please make sure the new test messages are unique:
> >
> > $ cat testsuite/gdb.sum | grep "PASS" | sort | uniq -c | sort -n | tail -n 15
> > 1 PASS: gdb.trace/trace-break.exp: 6 trace enable trace disable: tstart
>
> I am good at making such duplicated test messages. :) These duplicated
> messages are fixed in my test case. I put this tip to wiki, and hope it
> is useful to other people.
>
> http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique
Thanks!
> 0008-testsuite-tracepoint-change-loc.patch
> * gdb.trace/change-loc-1.c: New.
> * gdb.trace/change-loc-2.c: New.
> * gdb.trace/change-loc.c: New.
> * gdb.trace/change-loc.exp: New.
> * gdb.trace/change-loc.h: New.
> * gdb.trace/trace-break.c (marker): Define new symbol.
> * gdb.trace/trace-break.exp (break_trace_same_addr_5):
> New.
> (break_trace_same_addr_6): New.
FAOD, Ok.
--
Pedro Alves
^ permalink raw reply [flat|nested] 31+ messages in thread