This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 4/5] download and install tracepoint via agent
On 03/07/2012 03:31 PM, Yao Qi wrote:
> This is the real stuff. GDBserver sends tracepoint information to agent
> complying a protocol I described here,
>
> In-Process Agent Protocol (v1)
> http://sourceware.org/ml/gdb/2012-02/msg00070.html
>
> Note that, the protocol for fast tracepoint is updated a little, including,
> - Command header is changed from "TF" to "FastTrace:",
> - The reply of "FastTrace:" command contains a flag for OK or Error.
>
> GDBserver sends fast tracepoint command along with all information of a
> fast tracepoint to agent, and agent installs it. I implemented
> "FastTrace:" in another debugging agent (we call it dagent) instead of
> gdbserver/libinproctrace.so. Regression tested on
> x86_64-linux/native-gdbserver with dagent.
>
> This patch series doesn't include the patch of IPA protocol part. I am
> sure there should be some changes in protocol during the review, and
> I'll post it when these patches are reviewed.
As I said before, I'm not convinced the extra trouble of having yet
another protocol is really a good idea, so I'll just not comment on it much,
except to note that without a protocol version in the stream somewhere,
it seems you're likely to have trouble latter on when we want to add
extra optional features.
> gdb:
>
> * common/agent.c (agent_run_command): Add one more parameter `len'.
> Update callers.
> * common/agent.h: Update declaration.
> * gdb/linux-nat.c:
Looks stale.
> +static char *
> +m_tracepoint_action_send (struct tracepoint_action *action, char *buffer)
> +{
> + struct collect_memory_action *maction
> + = (struct collect_memory_action *) action;
> +
> + memcpy (buffer, (void *) &maction->addr, 8);
> + buffer += 8;
> + memcpy (buffer, (void *) &maction->len, 8);
> + buffer += 8;
> + memcpy (buffer, (void *) &maction->basereg, 4);
> + buffer += 4;
Unnecessary casts. Could have used COPY_TP_TO_BUF here, possibly renamed
COPY_FIELD_TO_BUF.
> +/* Copy agent expression AEXPR to P. If AEXPR is NULL, copy 0 to P. */
> +
> +static char *
> +agent_expr_send ( struct agent_expr *aexpr, char *p)
Spurious space after (.
Return not documented.
(As a general note, usually, I prefer putting the
destination on the left, in accordance with C library
functions (e.g., memcpy), and, making the source const, so just by
looking at the prototype one can clearly see which is the destination.
static char *agent_expr_send (char *p, const struct agent_expr *aexpr);
)
> @@ -3113,15 +3195,27 @@ cmd_qtstart (char *packet)
>
> if (tpoint->type == fast_tracepoint)
> {
> - download_tracepoint_1 (tpoint);
> -
> if (prev_ftpoint != NULL
> && prev_ftpoint->address == tpoint->address)
> - clone_fast_tracepoint (tpoint, prev_ftpoint);
> + {
> + download_tracepoint_1 (tpoint);
This call seems reachable with the new agent. Is that okay?
> + clone_fast_tracepoint (tpoint, prev_ftpoint);
> + }
> else
> {
> - if (install_fast_tracepoint (tpoint, packet) == 0)
> - prev_ftpoint = tpoint;
> + if (use_agent
> + && agent_capability_check (AGENT_CAPA_FAST_TRACE))
> + /* Download and install fast tracepoint by agent. */
> + {
The comment should be within the scope, after the brace.
> + if (tracepoint_send_agent (tpoint) == 0)
> + prev_ftpoint = tpoint;
> + }
> + else
> + {
> + download_tracepoint_1 (tpoint);
> + if (install_fast_tracepoint (tpoint, packet) == 0)
> + prev_ftpoint = tpoint;
> + }
> }
> }
> else
>
> +#define COPY_TP_TO_BUF(TPOINT,BUF,FIELD,LENGTH) \
> + memcpy (BUF, (void *) &TPOINT->FIELD, LENGTH); \
> + BUF += LENGTH;
- always wrap multiple statements in do {} while (0)
- you can get rid of the LENGTH parameter using sizeof.
- unnecessary cast.
- wrap argument uses in ()'s.
- renaming so it's usable in the case I pointed out above.
- putting destination in the left.
#define COPY_FIELD_TO_BUF(BUF, OBJ, FIELD) \
do { \
memcpy (BUF, &(OBJ)->FIELD, sizeof ((OBJ)->FIELD)); \
BUF += sizeof ((OBJ)->FIELD); \
} while (0)
Otherwise okay.
--
Pedro Alves