Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols.

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

On Wed, Sep 2, 2015 at 11:51 PM, Yao Qi <qiyaoltc@gmail.com> wrote:

> I didn't follow your previous tracepoint patches, so I don't understand
> what problem are you trying to fix in this patch.  Nowadays, GDB sends
> fast tracepoint to GDBserver, if GDBserver can't install it (jump pad is
> too far from tracepoint), GDBserver will return error, and GDB knows
> about it.  Do you want to do the check in GDB side rather than GDBserver
> side?

Hi Yao,

from what I see, if GDBserver cannot install a fast tracepoint, it will
indeed return an error.  However, GDB does not react particularly well
to such errors: if you look at remote_download_tracepoint, any type of
error returned from the QTDP packet will result in:

  putpkt (buf);
  remote_get_noisy_reply (&target_buf, &target_buf_size);
  if (strcmp (target_buf, "OK"))
    error (_("Target does not support tracepoints."));

All checking whether a specific tracepoint is valid currently takes
place earlier, on the GDB side, using a gdbarch callback:

  if (b->type == bp_fast_tracepoint)
    {
      /* Only test for support at download time; we may not know
         target capabilities at definition time.  */
      if (remote_supports_fast_tracepoints ())
        {
          if (gdbarch_fast_tracepoint_valid_at (loc->gdbarch, tpaddr,
                                                NULL))
            xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":F%x",
                       gdb_insn_length (loc->gdbarch, tpaddr));
          else
            /* If it passed validation at definition but fails now,
               something is very wrong.  */
            internal_error (__FILE__, __LINE__,
                            _("Fast tracepoint not "
                              "valid during download"));
        }
      else
        /* Fast tracepoints are functionally identical to regular
           tracepoints, so don't take lack of support as a reason to
           give up on the trace run.  */
        warning (_("Target does not support fast tracepoints, "
                   "downloading %d as regular tracepoint"), b->number);
    }

If the target does not support fast tracepoints at all, they are
converted into regular tracepoints, which seems reasonable.

However, if just a single tracepoint is invalid, we simply get an
internal error here, which is the problem Wei-cheng is trying to fix.
[ The same gdbarch_fast_tracepoint_valid_at call is done at the time
the tracepoint was installed originally.  However, if at that time
the tracepoint was pending, and its location has now been resolved,
that check was not re-done. ]

I see two issues here:

- I agree with you that there's duplicated checks here.  The gdbarch
  callback gdbarch_fast_tracepoint_valid_at is apparently supposed to
  duplicate the logic done in gdbserver.  In any case, any tracepoint
  considered "valid" by callback is expected to succeed on the target.

- The ICE can be triggered by normal user action (if the result of
  gdbarch_fast_tracepoint_valid_at changes if locations are recomputed,
  see above).

What I had asked Wei-cheng to implement is to fix these two issues:
properly duplicate the validity check for PowerPC, and re-validate
every time locations are resolved.

Maybe it would indeed be better to move the validity-checking logic
completely to the target side, i.e. always attempt to download the
tracepoint, and react more intelligently if that fails (e.g. downgrade
to a regular tracepoint?).  I'm not sure if that is doable with the
current remote protocol definition.  Thoughts?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com