Re: [PATCH 2/2] Reset trace local state when opening trace file

[Pedro Alves <palves at redhat dot com>]

On 06/06/2013 10:17 AM, Yao Qi wrote:
> On 06/05/2013 07:04 PM, Pedro Alves wrote:
>> This is really doing what the existing:
>>
>> /* This function handles the details of what to do about an ongoing
>>     tracing run if the user has asked to detach or otherwise disconnect
>>     from the target.  */
>>
>> void
>> disconnect_tracing (void)
>> {
>>    /* Also we want to be out of tfind mode, otherwise things can get
>>       confusing upon reconnection.  Just use these calls instead of
>>       full tfind_1 behavior because we're in the middle of detaching,
>>       and there's no point to updating current stack frame etc.  */
>>    set_current_traceframe (-1);
>>    set_tracepoint_num (-1);
>>    set_traceframe_context (NULL);
>> }
>>
>> is supposed to do, though disconnect_tracing seems to miss doing a
>> couple things the new function does.  I think they should be merged.
> 
> Yes, think a little more, I was even wondering disconnect_tracing may
> be removed completely.  Since we call trace_reset_local_state in
> remote_close to clear tracing state, we don't need to call function
> disconnect_tracing.

Yeah, I had thought about it too.  We can't.

> As we can see, disconnect_tracing is called from three places,
> detach_command, disconnect_command, and force_quit, and remote_close is
> called afterwards.  We can remove disconnect_tracing and, in effect,
> postpone the clearing local tracing-related state.
> 
> With this change, GDB may remove some breakpoints before clear local
> state.  An extreme case is that a breakpoint is set at address FOO, and
> action in a tracepoint collects the memory at FOO.  In tfind mode, when
> detach to the remote target, remove breakpoints,  GDB probably read
> from traceframe instead of the live inferior.  This is OK, but GDB
> can't write because writing to traceframes is forbidden.  It's unsafe
> to remove disconnect_tracing.

Exactly.  There's also things like prepare_for_detach that better be
done on the live target.

> The patch below is to call trace_reset_local_state in disconnect_tracing.

Thanks.

> 2013-06-06  Yao Qi  <yao@codesourcery.com>
>
> 	* tracepoint.c (start_tracing): Move code to ...
> 	(trace_reset_local_state): ... here.  New.
> 	(disconnect_tracing): Don't call set_current_traceframe,
> 	set_tracepoint_num, and set_traceframe_context. Call
> 	trace_reset_local_state instead.
> 	(tfile_close): Call trace_reset_local_state.
> 	* ctf.c (ctf_close): Likewise.
> 	* remote.c (remote_close): Likewise.
> 	* tracepoint.h (trace_reset_local_state): Declare.

OK.

-- 
Pedro Alves