[PATCH] gdb: tests/monitor: set remotetimeout to gdb_load_timeout for remote targets

[PATCH] gdb: tests/monitor: set remotetimeout to gdb_load_timeout for remote targets

[Mike Frysinger]

From: Jie Zhang <jie.zhang@analog.com>

Rather than relying on the default remotetimeout value (which might be too
small for some slower devices), use the existing gdb_load_timeout config
option to set it.

Signed-off-by: Jie Zhang <jie.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 gdb/testsuite/config/monitor.exp |   47 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 46 insertions(+), 1 deletions(-)

diff --git a/gdb/testsuite/config/monitor.exp b/gdb/testsuite/config/monitor.exp
index be98f1a..9efaf39 100644
--- a/gdb/testsuite/config/monitor.exp
+++ b/gdb/testsuite/config/monitor.exp
@@ -138,6 +138,7 @@ proc gdb_load { arg } {
     global gdb_prompt
     global timeout
     global last_gdb_file;
+    global decimal
 
     if { $arg == "" } {
 	if [info exists last_gdb_file] {
@@ -226,6 +227,32 @@ proc gdb_load { arg } {
 		set loadtimeout 1600
 	    }
 
+	    if [is_remote target] {
+		send_gdb "show remotetimeout\n"
+		gdb_expect {
+		    -re "Timeout limit to wait for target to respond is ($decimal).*$gdb_prompt $" {
+			set oldremotetimeout $expect_out(1,string);
+		    }
+		    timeout {
+			if { $verbose > 1 } {
+			    perror "Timed out trying to get remotetimeout."
+			}
+		    }
+		}
+		send_gdb "set remotetimeout $loadtimeout\n";
+		gdb_expect {
+		    -re "$gdb_prompt $" {
+			verbose "Set remotetimeout to $loadtimeout\n"
+		    }
+		    timeout {
+			if { $verbose > 1 } {
+			    perror "Timed out trying to set remotetimeout."
+			}
+		    }
+		}
+	    }
+
+	    set load_ok 0
 	    send_gdb $command;
 	    gdb_expect $loadtimeout {
 		-re "\[Ff\]ailed.*$gdb_prompt $" {
@@ -235,7 +262,7 @@ proc gdb_load { arg } {
 		}
 		-re "$gdb_prompt $" {
 		    verbose "Loaded $farg into $GDB\n"
-		    return 0;
+		    set load_ok 1
 		}
 		timeout {
 		    if { $verbose > 1 } {
@@ -243,6 +270,24 @@ proc gdb_load { arg } {
 		    }
 		}
 	    }
+
+	    if [is_remote target] {
+		send_gdb "set remotetimeout $oldremotetimeout\n";
+		gdb_expect {
+		    -re "$gdb_prompt $" {
+			verbose "Set remotetimeout to $oldremotetimeout\n"
+		    }
+		    timeout {
+			if { $verbose > 1 } {
+			    perror "Timed out trying to set remotetimeout."
+			}
+		    }
+		}
+	    }
+
+	    if { $load_ok == 1 } {
+		return 0;
+	    }
 	}
 
 	# Make sure we don't have an open connection to the target.
-- 
1.7.5.3

Re: [PATCH] gdb: tests/monitor: set remotetimeout to gdb_load_timeout for remote targets

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 246 bytes --]

On Sunday, June 05, 2011 17:34:38 Mike Frysinger wrote:
> Rather than relying on the default remotetimeout value (which might be too
> small for some slower devices), use the existing gdb_load_timeout config
> option to set it.

any love ?
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2] gdb: tests: set remotetimeout to gdb_load_timeout for remote targets

[Mike Frysinger]

From: Jie Zhang <jie.zhang@analog.com>

Rather than relying on the default remotetimeout value (which might be
too small for some slower devices), use the existing gdb_load_timeout
config option to set it.

This adds two new helpers for getting/setting the remotetimout to keep
the new logic simple.

Signed-off-by: Jie Zhang <jie.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v2
	- turn the remotetimeout logic into get/set helpers

 gdb/testsuite/config/monitor.exp |   16 +++++++++++++++-
 gdb/testsuite/lib/gdb.exp        |   36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/gdb/testsuite/config/monitor.exp b/gdb/testsuite/config/monitor.exp
index be98f1a..ca11600 100644
--- a/gdb/testsuite/config/monitor.exp
+++ b/gdb/testsuite/config/monitor.exp
@@ -226,6 +226,12 @@ proc gdb_load { arg } {
 		set loadtimeout 1600
 	    }
 
+	    if [is_remote target] {
+		set oldremotetimeout [get_remotetimeout]
+		set_remotetimeout $loadtimeout
+	    }
+
+	    set load_ok 0
 	    send_gdb $command;
 	    gdb_expect $loadtimeout {
 		-re "\[Ff\]ailed.*$gdb_prompt $" {
@@ -235,7 +241,7 @@ proc gdb_load { arg } {
 		}
 		-re "$gdb_prompt $" {
 		    verbose "Loaded $farg into $GDB\n"
-		    return 0;
+		    set load_ok 1
 		}
 		timeout {
 		    if { $verbose > 1 } {
@@ -243,6 +249,14 @@ proc gdb_load { arg } {
 		    }
 		}
 	    }
+
+	    if [is_remote target] {
+		set_remotetimeout $oldremotetimeout
+	    }
+
+	    if { $load_ok == 1 } {
+		return 0;
+	    }
 	}
 
 	# Make sure we don't have an open connection to the target.
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 9684724..ffc80af 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3614,6 +3614,42 @@ proc get_sizeof { type default } {
     return [get_integer_valueof "sizeof (${type})" $default]
 }
 
+proc get_remotetimeout { } {
+    global gdb_prompt
+    global decimal
+
+    send_gdb "show remotetimeout\n"
+    gdb_expect {
+	-re "Timeout limit to wait for target to respond is ($decimal).*$gdb_prompt $" {
+	    return $expect_out(1,string);
+	}
+	timeout {
+	    if { $verbose > 1 } {
+		perror "Timed out trying to get remotetimeout."
+	    }
+	}
+    }
+
+    # Pick the default that gdb uses
+    return 300
+}
+
+proc set_remotetimeout { timeout } {
+    global gdb_prompt
+
+    send_gdb "set remotetimeout $timeout\n";
+    gdb_expect {
+	-re "$gdb_prompt $" {
+	    verbose "Set remotetimeout to $timeout\n"
+	}
+	timeout {
+	    if { $verbose > 1 } {
+		perror "Timed out trying to set remotetimeout."
+	    }
+	}
+    }
+}
+
 # Log gdb command line and script if requested.
 if {[info exists TRANSCRIPT]} {
   rename send_gdb real_send_gdb
-- 
1.7.5.3

Re: [PATCH v2] gdb: tests: set remotetimeout to gdb_load_timeout for remote targets

[Joel Brobecker]

> Rather than relying on the default remotetimeout value (which might be
> too small for some slower devices), use the existing gdb_load_timeout
> config option to set it.

No one seems to be comfortable looking at this, so I took a look.
I think the idea makes sense.

> +	    if [is_remote target] {
> +		set oldremotetimeout [get_remotetimeout]
> +		set_remotetimeout $loadtimeout
> +	    }

Is there a specific reason you already know about that made you
change the remotetimeout only when the target is remote? If there is,
I think it'd be nice to add a comment to that effect. If it's just
because we don't expect the timeout to have any effect in the non-
remote case, then I guess we can leave it like that. But I'm not
to certain about that. I would consider the idea of making things
simpler by just setting the timeout everytime, since this part of
the code will always result in a "load" command being used.

> +proc get_remotetimeout { } {

This function needs a comment describing what it does. Same for
set_remotetimeout.

> +    send_gdb "show remotetimeout\n"
> +    gdb_expect {

Can you use gdb_test_multiple instead?

> +    # Pick the default that gdb uses
> +    return 300

I think we should at least get a warning when that happens, because
it means that the regexp in the check before failed to match.
If we don't have some kind of visual cue, I think it'll be easy
to not notice that something changed, and the function is now
returning a default value, rather than the real timeout at that
time...

> +    send_gdb "set remotetimeout $timeout\n";
> +    gdb_expect {

Same, can we use gdb_test_multiple?

-- 
Joel

Re: [PATCH v2] gdb: tests: set remotetimeout to gdb_load_timeout for remote targets

[Jie Zhang]

On 06/29/2011 12:25 PM, Joel Brobecker wrote:
>> Rather than relying on the default remotetimeout value (which might be
>> too small for some slower devices), use the existing gdb_load_timeout
>> config option to set it.
>
> No one seems to be comfortable looking at this, so I took a look.
> I think the idea makes sense.
>
Thank you! I can only answer the following question and will leave 
others to Mike.

>> +	    if [is_remote target] {
>> +		set oldremotetimeout [get_remotetimeout]
>> +		set_remotetimeout $loadtimeout
>> +	    }
>
> Is there a specific reason you already know about that made you
> change the remotetimeout only when the target is remote? If there is,
> I think it'd be nice to add a comment to that effect. If it's just
> because we don't expect the timeout to have any effect in the non-
> remote case, then I guess we can leave it like that. But I'm not
> to certain about that. I would consider the idea of making things
> simpler by just setting the timeout everytime, since this part of
> the code will always result in a "load" command being used.
>
I think it makes no sense to change remotetimeout for non-remote case so 
I added that conditional. I would like to leave it like that since I 
think the code looks clearer in this way. If we take it away, the code 
is simpler, but someone looking at the code might wonder why set 
remotetimeout for non-remote target.


Regards,
Jie

[PATCH v3] gdb: tests: set remotetimeout to gdb_load_timeout for remote targets

[Mike Frysinger]

From: Jie Zhang <jie.zhang@analog.com>

Rather than relying on the default remotetimeout value (which might be
too small for some slower devices), use the existing gdb_load_timeout
config option to set it.

This adds two new helpers for getting/setting the remotetimout to keep
the new logic simple.

Signed-off-by: Jie Zhang <jie.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v3
	- add comments to new helper funcs
	- use gdb_test_multiple

 gdb/testsuite/config/monitor.exp |   16 +++++++++++++++-
 gdb/testsuite/lib/gdb.exp        |   27 +++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 1 deletions(-)

diff --git a/gdb/testsuite/config/monitor.exp b/gdb/testsuite/config/monitor.exp
index be98f1a..ca11600 100644
--- a/gdb/testsuite/config/monitor.exp
+++ b/gdb/testsuite/config/monitor.exp
@@ -226,6 +226,12 @@ proc gdb_load { arg } {
 		set loadtimeout 1600
 	    }
 
+	    if [is_remote target] {
+		set oldremotetimeout [get_remotetimeout]
+		set_remotetimeout $loadtimeout
+	    }
+
+	    set load_ok 0
 	    send_gdb $command;
 	    gdb_expect $loadtimeout {
 		-re "\[Ff\]ailed.*$gdb_prompt $" {
@@ -235,7 +241,7 @@ proc gdb_load { arg } {
 		}
 		-re "$gdb_prompt $" {
 		    verbose "Loaded $farg into $GDB\n"
-		    return 0;
+		    set load_ok 1
 		}
 		timeout {
 		    if { $verbose > 1 } {
@@ -243,6 +249,14 @@ proc gdb_load { arg } {
 		    }
 		}
 	    }
+
+	    if [is_remote target] {
+		set_remotetimeout $oldremotetimeout
+	    }
+
+	    if { $load_ok == 1 } {
+		return 0;
+	    }
 	}
 
 	# Make sure we don't have an open connection to the target.
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 091b271..de9b4ea 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3626,6 +3626,33 @@ proc get_sizeof { type default } {
     return [get_integer_valueof "sizeof (${type})" $default]
 }
 
+# Get the current value for remotetimeout and return it.
+proc get_remotetimeout { } {
+    global gdb_prompt
+    global decimal
+
+    gdb_test_multiple "show remotetimeout" "" {
+	-re "Timeout limit to wait for target to respond is ($decimal).*$gdb_prompt $" {
+	    return $expect_out(1,string);
+	}
+    }
+
+    # Pick the default that gdb uses
+    warning "Unable to read remotetimeout"
+    return 300
+}
+
+# Set the remotetimeout to the specified timeout.  Nothing is returned.
+proc set_remotetimeout { timeout } {
+    global gdb_prompt
+
+    gdb_test_multiple "set remotetimeout $timeout" "" {
+	-re "$gdb_prompt $" {
+	    verbose "Set remotetimeout to $timeout\n"
+	}
+    }
+}
+
 # Log gdb command line and script if requested.
 if {[info exists TRANSCRIPT]} {
   rename send_gdb real_send_gdb
-- 
1.7.6

Re: [PATCH v2] gdb: tests: set remotetimeout to gdb_load_timeout for remote targets

[Pedro Alves]

On Wednesday 29 June 2011 17:25:27, Joel Brobecker wrote:
> > +    send_gdb "show remotetimeout\n"
> > +    gdb_expect {
> 
> Can you use gdb_test_multiple instead?

UUIC, that would add something like "PASS: show remotetimeout"
to every test.  Is that wanted?  I was under the impression that
was the reason we didn't use gdb_test_multiple for infrustructure
setup within gdb.exp.

-- 
Pedro Alves

Re: [toolchain-devel] [PATCH v2] gdb: tests: set remotetimeout to gdb_load_timeout for remote targets

[Mike Frysinger]

On Thu, Jun 30, 2011 at 04:58, Pedro Alves wrote:
> On Wednesday 29 June 2011 17:25:27, Joel Brobecker wrote:
>> > +    send_gdb "show remotetimeout\n"
>> > +    gdb_expect {
>>
>> Can you use gdb_test_multiple instead?
>
> UUIC, that would add something like "PASS: show remotetimeout"
> to every test.  Is that wanted?  I was under the impression that
> was the reason we didn't use gdb_test_multiple for infrustructure
> setup within gdb.exp.

i think that's only if the expect code passed to it calls `pass`.
when i ran it, i didnt see any "PASS: show remotetimeout" ... while, i
did test with gdb-6.6, i dont think this func has changed.
...
(gdb) target remote vapier:2000
Remote debugging using vapier:2000
0x20000000 in ?? ()
(gdb) show remotetimeout
Timeout limit to wait for target to respond is 2.
(gdb) set remotetimeout 1600
...
Transfer rate: 385771 bits/sec, 5067 bytes/write.
(gdb) set remotetimeout 2
(gdb) set print sevenbit-strings
...
-mike

Re: [toolchain-devel] [PATCH v2] gdb: tests: set remotetimeout to gdb_load_timeout for remote targets

[Pedro Alves]

On Thursday 30 June 2011 14:30:18, Mike Frysinger wrote:
> On Thu, Jun 30, 2011 at 04:58, Pedro Alves wrote:
> > On Wednesday 29 June 2011 17:25:27, Joel Brobecker wrote:
> >> > +    send_gdb "show remotetimeout\n"
> >> > +    gdb_expect {
> >>
> >> Can you use gdb_test_multiple instead?
> >
> > UUIC, that would add something like "PASS: show remotetimeout"
> > to every test.  Is that wanted?  I was under the impression that
> > was the reason we didn't use gdb_test_multiple for infrustructure
> > setup within gdb.exp.
> 
> i think that's only if the expect code passed to it calls `pass`.
> when i ran it, i didnt see any "PASS: show remotetimeout" ... while, i
> did test with gdb-6.6, i dont think this func has changed.

Ah, yes, you're right.  Thanks.

> ...
> (gdb) target remote vapier:2000
> Remote debugging using vapier:2000
> 0x20000000 in ?? ()
> (gdb) show remotetimeout
> Timeout limit to wait for target to respond is 2.
> (gdb) set remotetimeout 1600
> ...
> Transfer rate: 385771 bits/sec, 5067 bytes/write.
> (gdb) set remotetimeout 2
> (gdb) set print sevenbit-strings
> ...
> -mike
> 

-- 
Pedro Alves

Re: [PATCH v3] gdb: tests: set remotetimeout to gdb_load_timeout for remote targets

[Joel Brobecker]

> v3
> 	- add comments to new helper funcs
> 	- use gdb_test_multiple
> 
>  gdb/testsuite/config/monitor.exp |   16 +++++++++++++++-
>  gdb/testsuite/lib/gdb.exp        |   27 +++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 1 deletions(-)

This version is OK.  You forgot to provide a ChangeLog and I forgot
to remind you about that....

-- 
Joel

Re: [PATCH v3] gdb: tests: set remotetimeout to gdb_load_timeout for remote targets

[Mike Frysinger]

On Thu, Jun 30, 2011 at 18:54, Joel Brobecker wrote:
>> v3
>>       - add comments to new helper funcs
>>       - use gdb_test_multiple
>>
>>  gdb/testsuite/config/monitor.exp |   16 +++++++++++++++-
>>  gdb/testsuite/lib/gdb.exp        |   27 +++++++++++++++++++++++++++
>>  2 files changed, 42 insertions(+), 1 deletions(-)
>
> This version is OK.  You forgot to provide a ChangeLog and I forgot
> to remind you about that....

committed with:
2011-06-30  Jie Zhang <jie.zhang@analog.com>
            Mike Frysinger  <vapier@gentoo.org>

        * config/monitor.exp (get_remotetimeout, set_remotetimeout): New
        helper functions for getting/setting remotetimeout variable.
        * lib/gdb.exp (gdb_load): If the target is remote, set
        $oldremotetimeout to get_remotetimeout, then call set_remotetimeout
        with $loadtimeout.  Set $load_ok to 0 before doing the load.
        Instead of returning, immediately, set $load_ok to 0.  Call
        set_remotetimeout with $oldremotetimeout, and then return if
        $load_ok is 1.
-mike