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

[Pedro Alves <pedro at codesourcery dot com>]

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