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

[Pedro Alves <palves at redhat dot com>]

On 01/25/2017 03:52 PM, Metzger, Markus T wrote:
> Hello Pedro,
> 
> Thanks for your review.
> 
> 
>>> +/* 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.

> 
> 
>>> +	  && can_access_registers_ptid (tp->ptid))
>>
>> If you have this check, why do you need to the TRY/CATCH ?
>>
>> Or even, given the validate_registers_access_ptid check
>> above, why is this check necessary?
>>
>>> +	btrace_add_pc (tp);
>>> +    }
>>> +  CATCH (exception, RETURN_MASK_ALL)
>>
>> Adding a RETURN_MASK_ALL always raises alarm bells,
>> because this swallows a user-typed ctrl-c, which
>> is probably wrong.
>>
>>> +    {
>>> +      btrace_disable (tp);
>>> +    }
>>> +  END_CATCH
> 
> The TRY/CATCH is to clean things up in case of errors or ctrl-c.  The caller
> will clean things up for previously enabled threads but expects each
> btrace_enable to either succeed or fail and throw.
> 
> I see that I forgot to rethrow, though.  This is not intended to swallow
> the error - only to disable tracing again in case of errors.

OK, with the rethrow it'd make a lot more sense.

> 
> The can_access_registers_ptid check is to avoid the error and thus allow
> "record btrace" for running (or exited) threads where the btrace_add_pc
> call can be omitted.
> 
> 
>>> +# 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.

Thanks,
Pedro Alves