[PATCH v2] Introduce remote_target_is_gdbserver

[PATCH v2] Introduce remote_target_is_gdbserver

[Simon Marchi]

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.

gdb/testsuite/ChangeLog:

	* lib/gdbserver-support.exp (remote_target_is_gdbserver): New
	fonction.
---
 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] {
     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
+	}
+    }
+    return $is_gdbserver
+}
-- 
2.1.0

Re: [PATCH v2] Introduce remote_target_is_gdbserver

[Sergio Durigan Junior]

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.

> +    }
> +    return $is_gdbserver
> +}

Otherwise, looks good to me (this is not an approval).

Cheers,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [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.

Re: [PATCH v2] Introduce remote_target_is_gdbserver

[Pedro Alves]

On 09/11/2014 03:47 PM, Simon Marchi wrote:
> On 14-09-05 07:30 PM, Sergio Durigan Junior wrote:


>>>  # 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
>>> -    }
>>> -}


>>> +# 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.

FWIW, yeah, that was the original rationale behind the "pass"es
in the original code.  The test was written in the form of
"Probing for GDBserver", and the idea is that if we find that we're
not running GDBserver but something else, the _probing_ itself was
still a success.

Thanks,
Pedro Alves