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 (齐尧)