This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]