[PATCH] gdb: Don't call gdb_load_shlib unless GDB is running

[PATCH] gdb: Don't call gdb_load_shlib unless GDB is running

[Andrew Burgess]

The gdb_load_shlib function will, on remote targets, try to run some
GDB commands.  This obviously isn't going to work unless GDB is
running.

The gdb.trace/tspeed.exp test calls gdb_load_shlib before starting
GDB.  Don't do that.

The failure that's triggered is actually DeJaGNU complaining that the
variable $use_gdb_stub doesn't exist, this is only created when GDB is
started.  Something like this should trigger a failure:

  make check-gdb \
    RUNTESTFLAGS="--target_board=remote-gdbserver-on-localhost \
                  gdb.trace/tspeed.exp"

gdb/testsuite/ChangeLog:

	* gdb.trace/tspeed.exp: Only call gdb_load_shlib after gdb has
	started.
---
 gdb/testsuite/ChangeLog            | 5 +++++
 gdb/testsuite/gdb.trace/tspeed.exp | 4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.trace/tspeed.exp b/gdb/testsuite/gdb.trace/tspeed.exp
index ecd36d2d9bd..47a82502a00 100644
--- a/gdb/testsuite/gdb.trace/tspeed.exp
+++ b/gdb/testsuite/gdb.trace/tspeed.exp
@@ -19,7 +19,6 @@ standard_testfile
 set executable $testfile
 
 set ipalib [get_in_proc_agent]
-gdb_load_shlib $ipalib
 
 if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
 	  executable [concat {debug nowarnings c} shlib=$ipalib]] != "" } {
@@ -41,6 +40,7 @@ proc prepare_for_trace_test {} {
     global executable
 
     clean_restart $executable
+    gdb_load_shlib $ipalib
 
     runto_main
 
@@ -126,6 +126,8 @@ proc gdb_trace_collection_test {} {
 }
 
 clean_restart $executable
+gdb_load_shlib $ipalib
+
 runto_main
 
 if { ![gdb_target_supports_trace] } then {
-- 
2.14.4

[PATCH] gdb: Don't call gdb_load_shlib unless GDB is running

[Keith Seitz]

On 07/11/2018 10:06 AM, Andrew Burgess wrote:
> The gdb_load_shlib function will, on remote targets, try to run some
> GDB commands.  This obviously isn't going to work unless GDB is
> running.
> 
> The gdb.trace/tspeed.exp test calls gdb_load_shlib before starting
> GDB.  Don't do that.
> 
> The failure that's triggered is actually DeJaGNU complaining that the
> variable $use_gdb_stub doesn't exist, this is only created when GDB is
> started.  Something like this should trigger a failure:
> 
>   make check-gdb \
>     RUNTESTFLAGS="--target_board=remote-gdbserver-on-localhost \
>                   gdb.trace/tspeed.exp"

Wow, I never even noticed that -- good catch.

> diff --git a/gdb/testsuite/gdb.trace/tspeed.exp b/gdb/testsuite/gdb.trace/tspeed.exp
> index ecd36d2d9bd..47a82502a00 100644
> --- a/gdb/testsuite/gdb.trace/tspeed.exp
> +++ b/gdb/testsuite/gdb.trace/tspeed.exp
> @@ -126,6 +126,8 @@ proc gdb_trace_collection_test {} {
>  }
>  
>  clean_restart $executable
> +gdb_load_shlib $ipalib
> +
>  runto_main
>  
>  if { ![gdb_target_supports_trace] } then {
> 

While I agree with the patch, I think more c/should be done to ensure that this scenario doesn't happen again. At the very least, mention this limitation in the procedure's comments.

However, is it possible/desirable to error if gdb hasn't been spawned ('![info exists ::gdb_spawn_id]' IIUC)? That should catch any other places where this currently happens (if any) and (more important) prevent this from happening in the future. [IANAM*]

Thanks,
Keith

* I am not a maintainer

[PATCHv2] gdb: Don't call gdb_load_shlib unless GDB is running

[Andrew Burgess]

Keith,

Thanks for your feedback.

I've updated the patch to add a check inside gdb_load_shlib as your
suggested, this should catch any future miss-uses of the function.

Thanks,

Andrew


---

The gdb_load_shlib function will, on remote targets, try to run some
GDB commands.  This obviously isn't going to work unless GDB is
running.

The gdb.trace/tspeed.exp test calls gdb_load_shlib before starting
GDB.  Don't do that.

The failure that's triggered is actually DeJaGNU complaining that the
variable $use_gdb_stub doesn't exist, this is only created when GDB is
started.  Something like this should trigger a failure:

  make check-gdb \
    RUNTESTFLAGS="--target_board=remote-gdbserver-on-localhost \
                  gdb.trace/tspeed.exp"

This commit also adds a check to gdb_load_shlib that GDB is running.
The check is always performed, so this should catch cases where a GDB
developer adds a use of gdb_load_shlib but doesn't test their code
with a remote target.

gdb/testsuite/ChangeLog:

	* gdb.trace/tspeed.exp: Only call gdb_load_shlib after gdb has
	started.
	* lib/gdb.exp (gdb_load_shlib): Call perror if GDB is not running.
---
 gdb/testsuite/ChangeLog            | 6 ++++++
 gdb/testsuite/gdb.trace/tspeed.exp | 4 +++-
 gdb/testsuite/lib/gdb.exp          | 6 ++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.trace/tspeed.exp b/gdb/testsuite/gdb.trace/tspeed.exp
index ecd36d2d9bd..47a82502a00 100644
--- a/gdb/testsuite/gdb.trace/tspeed.exp
+++ b/gdb/testsuite/gdb.trace/tspeed.exp
@@ -19,7 +19,6 @@ standard_testfile
 set executable $testfile
 
 set ipalib [get_in_proc_agent]
-gdb_load_shlib $ipalib
 
 if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
 	  executable [concat {debug nowarnings c} shlib=$ipalib]] != "" } {
@@ -41,6 +40,7 @@ proc prepare_for_trace_test {} {
     global executable
 
     clean_restart $executable
+    gdb_load_shlib $ipalib
 
     runto_main
 
@@ -126,6 +126,8 @@ proc gdb_trace_collection_test {} {
 }
 
 clean_restart $executable
+gdb_load_shlib $ipalib
+
 runto_main
 
 if { ![gdb_target_supports_trace] } then {
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index aef580b04d3..3e2f755e5b7 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4483,6 +4483,12 @@ proc gdb_remote_download {dest fromfile {tofile {}}} {
 # Copy the listed library to the target.
 
 proc gdb_load_shlib { file } {
+    global gdb_spawn_id
+
+    if ![info exists gdb_spawn_id] {
+	perror "gdb_load_shlib: GDB is not running"
+    }
+
     set dest [gdb_remote_download target [shlib_target_file $file]]
 
     if {[is_remote target]} {
-- 
2.14.4

Re: [PATCHv2] gdb: Don't call gdb_load_shlib unless GDB is running

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> This commit also adds a check to gdb_load_shlib that GDB is running.
Andrew> The check is always performed, so this should catch cases where a GDB
Andrew> developer adds a use of gdb_load_shlib but doesn't test their code
Andrew> with a remote target.

Andrew> gdb/testsuite/ChangeLog:

Andrew> 	* gdb.trace/tspeed.exp: Only call gdb_load_shlib after gdb has
Andrew> 	started.
Andrew> 	* lib/gdb.exp (gdb_load_shlib): Call perror if GDB is not running.

Thank you.  This patch is OK.

Tom