Re: [PATCH 3/3] MI notification on trace stop: triggered by remote

[Pedro Alves <palves at redhat dot com>]

On 01/08/2013 01:07 PM, Yao Qi wrote:
> On 01/08/2013 03:21 AM, Pedro Alves wrote:
>> I don't think trace_stop_notes is correct here.  The user
>> could have changed GDB's notes since they were set it
>> in the target, or the target could have decided to report
>> different notes, right?  I think ts->notes instead gives you
>> the notes as reported by the target, so that'd be better.
>>
> 
> When user changes 'trace_stop_notes', the stop notes in remote target
> should be sync'ed, because target_set_trace_notes is called when 
> executing command 'set trace-stop-notes'.  

Yes, but that's racy.  There's always a window between "user
sets notes" and "target reports trace stopped", where the
target may have stopped before gdb set the notes on the target.

> If target reports different 
> notes, it is not correct to use trace_stop_notes, as you said.
> 
> The correct stop notes got from the remote target should be in 
> ts->stop_desc instead of ts->notes.  Further reading gives me more 
> thoughts, that, trace stop notes make sense when trace is stopped by 
> command 'tstop', IIUC.  In this case, trace stop is triggered by 
> remote, we shouldn't use trace stop notes at all.  In the new version 
> of patch, I call 'observer_notify_trace_stopped (NULL)'.  WDYT?

Dunno.

> 
>> BTW, isn't the frontend always going to fetch the trace status
>> afterwards anyway?  IOW is putting the notes on the MI
>> notification actually useful?  Or rather, why are notes in
>> the notification while other status bits are not?
>>
> 
> The reason I put trace notes and stop notes into trace started/stopped 
> notification is that notes are sort of parameters of commands 'tstart' 
> and 'tstop'.

On the CLI, as convenience, but not on MI.  Dunno about that parallel.
You've made the RSP report the whole status, but MI report only a bit
of the status.  It feels like a double standard to me.  The MI
notification doesn't even include the _reason_ for the stop, therefore
the frontend will inevitably fetch the trace status afterwards.  The
notes seems to me not to be the most interesting.  So it make much
more sense to me to either not include anything in the MI notification,
or include the whole status.

> 
> GDB emits many qtstatus packets to get trace status in sync with the 
> remote target.  As GDB has async remote notification on trace, the 
> trace status in GDB side can be updated by notification, and save some 
> qtstatus packets to query.  I'll post a follow-up patch to reduce the 
> number of qtstatus packets.

Ack.  You'll want some mechanism of GDB knowing it'll expect trace
notifications.  A qSupported feature perhaps.

-- 
Pedro Alves