RE: [PATCH v2 2/2] btrace: allow recording to be started for running threads

Metzger, Markus T
2017-01-26T14:54:16Z

Hello Pedro,

> >>> +/* Validate that we can read PTID's registers. */
> >>> +
> >>> +static void
> >>> +validate_registers_access_ptid (ptid_t ptid)
> >>> +{
> >>> + struct cleanup *cleanup = save_inferior_ptid ();
> >>> +
> >>> + inferior_ptid = ptid;
> >>> + validate_registers_access ();
> >>> + do_cleanups (cleanup);
> >>> +}
> >>
> >> Do we need this, we we have the new function added
> >> by patch #1 ?
> >
> > This is a safeguard. I don't expect that we will ever throw, here.
>
> Aren't the checks done inside the validate_registers_access function
> exactly the same as done in can_access_registers_ptid?
>
> And if that doesn't throw, and the thread is running, you have
> register accesses right after:
>
> /* Make sure we can read TP's registers. */
> validate_registers_access_ptid (tp->ptid);
>
> regcache = get_thread_regcache (tp->ptid);
> pc = regcache_read_pc (regcache);
>
>
> I think I must be missing something.

btrace_add_pc is called in two different flows:
- when enabling tracing
- when decoding trace

The first checks if registers can be accessed and silently omits the
call if they can't.
The second calls btrace_add_pc without checking that registers can
be accessed. I expected it to fail already before calling btrace_add_pc
and I added this just as a safeguard.

When I actually try to read the trace while the thread is running it fails
in perf_event_read_bts for BTS - again while trying to read the PC.
For PT, it fails here.

It isn't really safe to read the trace buffer while the thread is running
and generating more trace data. I would therefore add the check to
btrace_fetch and remove it here.

We thus allow tracing to be started and stopped while the thread is
running but we require the thread to be stopped before reading
the trace. OK?

> >>> +# We need to enable non-stop mode for the remote case.
> >>> +gdb_test_no_output "set non-stop on"
> >>
> >> This is too late with --target_board=native-extended-gdbserver.
> >> Use instead:
> >>
> >> save_vars { GDBFLAGS } {
> >> append GDBFLAGS " -ex \"set non-stop on\""
> >> clean_restart $testfile
> >> }
> >
> > This test seems to run fine with --target_board=native-extended-gdbserver.
>
> With that board, gdb connects to gdbserver as soon as it starts.
>
> In the posted version of the patch, "set non-stop on" was being issued
> after starting gdb. The result being, "set non-stop on" was being issued
> after the initial connection.
>
> But "set non-stop on" won't really work correctly after initial connection
> It'd be possible to make work, but, currently it doesn't.
> The issue is that GDB only tells the server to switch to the non-stop
> variant of the protocol on the initial connection setup. After the initial
> connection, the server continue in all-stop mode, while gdb is in non-stop
> mode. This results in errors like:
>
> Unexpected vCont reply in non-stop mode:
> T05swbreak:;06:f0d8ffffff7f0000;07:10d8ffffff7f0000;10:08f9ddf7ff7f0000;thread:
> p4c0d.4c0d;core:2;
>
> when you try to run something.
>
> So I'm puzzled. If the test was still passing, but gdb.log showed errors
> like the above, it suggests that the test may need to have its regexps
> tightened a bit.

The test silently stopped when runto_main failed. With another patch that is
currently in review, this will cause an UNTESTED.

Thanks,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928