[PATCH v2] Introduce remote_target_is_gdbserver

[Simon Marchi]

On 14-09-05 07:30 PM, Sergio Durigan Junior wrote:
> On Friday, September 05 2014, Simon Marchi wrote:
> 
>> Oops, I had some unstaged changes when I sent this patch, so here is
>> an updated one with those changes. Sorry for the noise.
>>
>> This patch introduces a function in gdbserver-support.exp to find out
>> whether the current target is GDBserver.
>>
>> The code was inspired from gdb.trace/qtor.exp, so it replaces the code
>> there by a call to the new function.
> 
> Hi Simon,
> 
> Thanks for the patch.  A few comments.
> 
>> gdb/testsuite/ChangeLog:
>>
>> 	* lib/gdbserver-support.exp (remote_target_is_gdbserver): New
>> 	fonction.
> 
> Typo: fonction.
> 
> You also forgot to mention the change to gdb.trace/qtro.exp.
> 
>> ---
>>  gdb/testsuite/gdb.trace/qtro.exp        | 14 +-------------
>>  gdb/testsuite/lib/gdbserver-support.exp | 23 +++++++++++++++++++++++
>>  2 files changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.trace/qtro.exp b/gdb/testsuite/gdb.trace/qtro.exp
>> index 22b5051..95b6b85 100644
>> --- a/gdb/testsuite/gdb.trace/qtro.exp
>> +++ b/gdb/testsuite/gdb.trace/qtro.exp
>> @@ -98,19 +98,7 @@ if { $traceframe_info_supported == -1 } {
>>  }
>>  
>>  # Check whether we're testing with our own GDBserver.
>> -set is_gdbserver -1
>> -set test "probe for GDBserver"
>> -gdb_test_multiple "monitor help" $test {
>> -    -re "The following monitor commands are supported.*debug-hw-points.*remote-debug.*GDBserver.*$gdb_prompt $" {
>> -	set is_gdbserver 1
>> -	pass $test
>> -    }
>> -    -re "$gdb_prompt $" {
>> -	set is_gdbserver 0
>> -	pass $test
>> -    }
>> -}
>> -if { $is_gdbserver == -1 } {
>> +if ![remote_target_is_gdbserver] {
> 
> I know it's just a matter of style, but I'd prefer if you kept the
> surrounding brackets in the condition:
> 
>   if { ![remote_target_is_gdbserver] } {
>   ...
> 
>>      return -1
>>  }
>>  
>> diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
>> index 8c91e28..300c3db 100644
>> --- a/gdb/testsuite/lib/gdbserver-support.exp
>> +++ b/gdb/testsuite/lib/gdbserver-support.exp
>> @@ -419,3 +419,26 @@ proc mi_gdbserver_start_multi { } {
>>  
>>      return [mi_gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
>>  }
>> +
>> +# Return true if the current remote target is an instance of gdbserver.
>> +
>> +proc remote_target_is_gdbserver { } {
>> +    global gdb_prompt
>> +
>> +    set is_gdbserver 0
>> +    set test "Probing for GDBserver"
>> +
>> +    gdb_test_multiple "monitor help" $test {
>> +	-re "The following monitor commands are supported.*Quit GDBserver.*$gdb_prompt $" {
>> +		pass $test
>> +		set is_gdbserver 1
>> +	}
>> +	-re "$gdb_prompt $" {
>> +		pass $test
>> +	}
>> +	default {
>> +		pass $test
>> +	}
> 
> Do we really need these "pass"?  I'd rather we don't put it, and by
> looking at lib/gdb.exp I see many tests also don't use it.

I thought so, but apparently no. I thought that each gdb_test_multiple had to be matched with one pass or fail.

>> +    }
>> +    return $is_gdbserver
>> +}
> 
> Otherwise, looks good to me (this is not an approval).
> 
> Cheers,

Thanks,

Sending v2 now.