This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] Introduce remote_target_is_gdbserver
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Thu, 11 Sep 2014 10:47:52 -0400
- Subject: Re: [PATCH v2] Introduce remote_target_is_gdbserver
- Authentication-results: sourceware.org; auth=none
- References: <1409948495-13599-1-git-send-email-simon dot marchi at ericsson dot com> <87k35h3asl dot fsf at redhat dot com>
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.