Re: [patch 6/8] gdbserver - Install tracepoint when tracing is running

Yao Qi
2011-11-12T02:40:10Z

[-- 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