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

Pedro Alves
2011-11-10T16:53:40Z

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