Fix several "set remote foo-packet on/off" commands.

  Hi Yao,

Thanks for the review.

On 04/01/2014 09:53 AM, Yao Qi wrote:
> On 04/01/2014 07:10 AM, Pedro Alves wrote:
>> +/* Returns true if the multi-process extensions are in effect.  */
>> +static int
>> +remote_multi_process_p (struct remote_state *rs)
>> +{
>> +  return packet_support (PACKET_multiprocess_feature) == PACKET_ENABLE;
>> +}
> 
> Argument 'rs' is not needed.  We can remove it.

Yeah.  (Though note the parameter was already there, I just moved the
function further below at the same time I reimplemented it.)

> However, your patch moves more gdb remote target states from
> 'struct remote_state' to a global state.  I feel that it has
> conflict with the multi-target project.  

As part of that project, we'll definitely need to move the
whole remote_protocol_packets global array to remote_state.
This global array already exists.  I've just moved a few more manually
managed flags into this array.  Note this is listed as a TODO in
the MultiTarget project's wiki page.
Therefore, I'm not really adding any conflict, as that work will
already have to be done.  When that is done, we'll likely end up
with passing a 'struct remote_state *' pointer to the new packet_support
function.  And with that in mind, it just looks like unnecessary
churn to remove the parameter from remote_multi_process_p now (and
update all callers), only to add it back again.  Do you agree?

> Not sure it is part
> of your plan to convert the global state to per-target state.

I don't plan to work on it myself in the near future, but it's
definitely the way to go.

>> +
>> +	# Force-disable the InstallInTrace RSP feature.
>> +	gdb_test_no_output "set remote install-in-trace-packet off"
>> +
>> +	# Set a tracepoint while a trace experiment is ongoing.
>> +	set test "set tracepoint on set_tracepoint"
>> +	gdb_test_multiple "${trace_type} set_tracepoint" $test {
>> +	    -re "Target returns error code .* too far .*$gdb_prompt $" {
>> +		if [string equal $trace_type "ftrace"] {
>> +		    # The target was unable to install the fast tracepoint
>> +		    # (e.g., jump pad too far from tracepoint).
>> +		    pass "$test (too far)"
> 
> A nit here, a fast tracepoint is not installed due to "jump pad too far"
> instead of "set remote install-in-trace-packet off".  Do we want to emit
> a PASS here?

Oh, hmm.  Copy-paste from the other function in the file, and I
just didn't think about that part much.  It actually makes no sense
to even expect that the target sends back any error code.  We've just
disabled installing tracepoints while a trace experiment is running,
so we shouldn't see any response from the target at all.

> IMO, UNSUPPORTED is better.

Agreed, though note you made this a PASS in tracepoint_change_loc_1.  ;-)

>  Secondly, if "jump pad too far"
> causes fast tracepoint not installed, we should return here to skip the
> rest of the test, because the test below can't tell why a fast
> tracepoint is not installed, is it caused by
> "set remote install-in-trace-packet off" or "jump pad too far".

Even better is to not expect any error at all, and then make sure
that no traceframe was collected.

Here's what I'm folding into v2 of the patch.

---
 gdb/remote.c                           |  1 +
 gdb/testsuite/gdb.trace/change-loc.exp | 35 +++++++++++++++++-----------------
 2 files changed, 18 insertions(+), 18 deletions(-)
  

On 04/01/2014 08:06 PM, Pedro Alves wrote:
> As part of that project, we'll definitely need to move the
> whole remote_protocol_packets global array to remote_state.
> This global array already exists.  I've just moved a few more manually
> managed flags into this array.  Note this is listed as a TODO in
> the MultiTarget project's wiki page.
> Therefore, I'm not really adding any conflict, as that work will
> already have to be done.  When that is done, we'll likely end up
> with passing a 'struct remote_state *' pointer to the new packet_support
> function.  And with that in mind, it just looks like unnecessary
> churn to remove the parameter from remote_multi_process_p now (and
> update all callers), only to add it back again.  Do you agree?
> 

Yes, that is fine to keep unused parameter 'rs' remote_multi_process_p.

>> > Not sure it is part
>> > of your plan to convert the global state to per-target state.
> I don't plan to work on it myself in the near future, but it's
> definitely the way to go.
> 

OK, got it.

>>> >> +
>>> >> +	# Force-disable the InstallInTrace RSP feature.
>>> >> +	gdb_test_no_output "set remote install-in-trace-packet off"
>>> >> +
>>> >> +	# Set a tracepoint while a trace experiment is ongoing.
>>> >> +	set test "set tracepoint on set_tracepoint"
>>> >> +	gdb_test_multiple "${trace_type} set_tracepoint" $test {
>>> >> +	    -re "Target returns error code .* too far .*$gdb_prompt $" {
>>> >> +		if [string equal $trace_type "ftrace"] {
>>> >> +		    # The target was unable to install the fast tracepoint
>>> >> +		    # (e.g., jump pad too far from tracepoint).
>>> >> +		    pass "$test (too far)"
>> > 
>> > A nit here, a fast tracepoint is not installed due to "jump pad too far"
>> > instead of "set remote install-in-trace-packet off".  Do we want to emit
>> > a PASS here?
> Oh, hmm.  Copy-paste from the other function in the file, and I
> just didn't think about that part much.  It actually makes no sense
> to even expect that the target sends back any error code.  We've just
> disabled installing tracepoints while a trace experiment is running,
> so we shouldn't see any response from the target at all.
> 
>> > IMO, UNSUPPORTED is better.
> Agreed, though note you made this a PASS in tracepoint_change_loc_1.  ;-)
> 

Sorry about that :)

patch v2 looks good to me.
  

On 04/01/2014 01:46 PM, Yao Qi wrote:

> patch v2 looks good to me.

Thanks Yao.  I've pushed this in.