[PATCH 0/4] gdb/testsuite: remove can_spawn_for_attach_1

[PATCH 0/4] gdb/testsuite: remove can_spawn_for_attach_1

[Andrew Burgess]

A recent patch got me wondering why we have can_spawn_for_attach_1 and
can_spawn_for_attach.  I think we probably shouldn't.  This series
moves the functionality from can_spawn_for_attach into gdb_do_cache
(lib/cache.exp), removing the need for two can_spawn_for_attach*
procs.

---

Andrew Burgess (4):
  gdb/testsuite: remove trailing \r from rust_llvm_version result
  gdb/testsuite: improve with_override
  gdb/testsuite: restructure gdb_data_cache (lib/cache.exp)
  gdb/testsuite: track if a caching proc calls gdb_exit or not

 gdb/testsuite/gdb.testsuite/with-override.exp |  44 ++++++
 gdb/testsuite/lib/cache.exp                   |  94 +++++++++---
 gdb/testsuite/lib/gdb.exp                     | 143 +++++++++---------
 gdb/testsuite/lib/rust-support.exp            |   3 +-
 4 files changed, 198 insertions(+), 86 deletions(-)


base-commit: 40acbd34527648e0c375b965b16ab5b7f2ecae6c
-- 
2.25.4

[PATCH 1/4] gdb/testsuite: remove trailing \r from rust_llvm_version result

[Andrew Burgess]

I noticed that the value returned by rust_llvm_version had a trailing
carriage return.  I don't think this is causing any problems right
now, but looking at the code I don't think this was the desired
behaviour.

The current code runs 'rustc --version --verbose', splits the output
at each '\n' and then loops over every line looking for the line that
contains the LLVM version.

There are two problems here.  First, at the end of each captured line
we have '\r\n', so when we split the lines on '\n', each of the lines
will still end with a '\r' character.

Second, though we loop over the lines, when we try to compare the line
contents we actually compare the unsplit full output.  Luckily this
still finds the match, but this renders the loop over lines redundant.

This commit makes two fixes:

 1. I use regsub to convert all '\r\n' sequences to '\n'; now when we
    split on '\n' the lines will not end in '\r'.

 2. Within the loop over lines block I now check the line contents
    rather than the unsplit full output; now we capture a value
    without a trailing '\r'.

There's only one test (gdb.rust/simple.exp) that uses
rust_llvm_version, and it doesn't care if there's a trailing '\r' or
not, so this change should make no difference there.
---
 gdb/testsuite/lib/rust-support.exp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/rust-support.exp b/gdb/testsuite/lib/rust-support.exp
index 6b3da2a69e4..971a4a6c298 100644
--- a/gdb/testsuite/lib/rust-support.exp
+++ b/gdb/testsuite/lib/rust-support.exp
@@ -86,8 +86,9 @@ gdb_caching_proc rust_llvm_version {} {
 	verbose "could not find rustc"
     } else {
 	set output [lindex [remote_exec host "$rustc --version --verbose"] 1]
+	set output [regsub -all "\r\n" $output "\n"]
 	foreach line [split $output \n] {
-	    if {[regexp "LLVM version: (.+)\$" $output ignore version]} {
+	    if {[regexp "LLVM version: (.+)\$" $line ignore version]} {
 		return $version
 	    }
 	}
-- 
2.25.4

Re: [PATCH 1/4] gdb/testsuite: remove trailing \r from rust_llvm_version result

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

I saw your note about ignoring this series but...

Andrew> This commit makes two fixes:

Andrew>  1. I use regsub to convert all '\r\n' sequences to '\n'; now when we
Andrew>     split on '\n' the lines will not end in '\r'.

Andrew>  2. Within the loop over lines block I now check the line contents
Andrew>     rather than the unsplit full output; now we capture a value
Andrew>     without a trailing '\r'.

... I think this is worthwhile on its own.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 1/4] gdb/testsuite: remove trailing \r from rust_llvm_version result

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> I saw your note about ignoring this series but...
>
> Andrew> This commit makes two fixes:
>
> Andrew>  1. I use regsub to convert all '\r\n' sequences to '\n'; now when we
> Andrew>     split on '\n' the lines will not end in '\r'.
>
> Andrew>  2. Within the loop over lines block I now check the line contents
> Andrew>     rather than the unsplit full output; now we capture a value
> Andrew>     without a trailing '\r'.
>
> ... I think this is worthwhile on its own.
>
> Approved-By: Tom Tromey <tom@tromey.com>

I've gone ahead and pushed just this patch.

Thanks,
Andrew

[PATCH 2/4] gdb/testsuite: improve with_override

[Andrew Burgess]

I wanted to use 'with_override' to override a proc, but within the
overridden proc I wanted to call the original function.  I could just
write my own version of 'with_override' that does what I want...

... or I could extend the existing 'with_override' to include this new
functionality, which is what I've done in this commit.

You can now do this:

  with_override some_proc new_proc save_name {
    ... body ....
  }

Now, while BODY is executing calls to 'some_proc' will actually result
in calling 'new_proc'.  However, calling 'save_name' will call the
original definition of 'some_proc'.

If 'save_name' already exists when 'with_override' is called then the
original value of 'save_name' will be backed up and then restored once
the with_override has completed.

If you don't need the new functionality then the old behaviour still
works just fine, i.e.:

  with_override some_proc new_proc {
    ... body ...
  }

My use of this new functionality will appear in a later commit, but
for now I've added some unit-tests for the new functionality.
---
 gdb/testsuite/gdb.testsuite/with-override.exp | 44 ++++++++++++++
 gdb/testsuite/lib/gdb.exp                     | 60 +++++++++++++++++--
 2 files changed, 99 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.testsuite/with-override.exp b/gdb/testsuite/gdb.testsuite/with-override.exp
index a0a49625372..12467061ba7 100644
--- a/gdb/testsuite/gdb.testsuite/with-override.exp
+++ b/gdb/testsuite/gdb.testsuite/with-override.exp
@@ -26,10 +26,18 @@ proc foo2 {} {
     return 2
 }
 
+# Ensure that 'old_foo' doesn't exist.
+if { [info procs old_foo] != "" } {
+    rename old_foo ""
+}
+
 with_test_prefix no-args {
 
     with_test_prefix before {
 	gdb_assert { [foo] == 0 }
+	gdb_assert { [foo1] == 1 }
+	gdb_assert { [foo2] == 2 }
+	gdb_assert { [info procs old_foo] == "" } "old_foo does not exist"
     }
 
     with_override foo foo1 {
@@ -44,8 +52,30 @@ with_test_prefix no-args {
 	}
     }
 
+    with_override foo foo1 old_foo {
+	with_test_prefix old_foo {
+	    with_test_prefix before {
+		gdb_assert { [old_foo] == 0 }
+		gdb_assert { [foo] == 1 }
+	    }
+
+	    with_override foo foo2 old_foo {
+		gdb_assert { [old_foo] == 1 }
+		gdb_assert { [foo] == 2 }
+	    }
+
+	    with_test_prefix after {
+		gdb_assert { [old_foo] == 0 }
+		gdb_assert { [foo] == 1 }
+	    }
+	}
+    }
+
     with_test_prefix after {
 	gdb_assert { [foo] == 0 }
+	gdb_assert { [foo1] == 1 }
+	gdb_assert { [foo2] == 2 }
+	gdb_assert { [info procs old_foo] == "" } "old_foo does not exist"
     }
 }
 
@@ -63,6 +93,7 @@ with_test_prefix default-arg {
 	gdb_assert { [foo] == 1 }
 	gdb_assert { [foo 0] == 1 }
 	gdb_assert { [foo 1] == 2 }
+	gdb_assert { [info procs old_foo] == "" } "old_foo does not exist"
     }
 
     with_override foo foo_plus_1 {
@@ -73,9 +104,22 @@ with_test_prefix default-arg {
 	}
     }
 
+    with_override foo foo_plus_1 old_foo {
+	with_test_prefix old_foo {
+	    gdb_assert { [foo] == 2 }
+	    gdb_assert { [foo 0] == 2 }
+	    gdb_assert { [foo 1] == 3 }
+
+	    gdb_assert { [old_foo] == 1 }
+	    gdb_assert { [old_foo 0] == 1 }
+	    gdb_assert { [old_foo 1] == 2 }
+	}
+    }
+
     with_test_prefix after {
 	gdb_assert { [foo] == 1 }
 	gdb_assert { [foo 0] == 1 }
 	gdb_assert { [foo 1] == 2 }
+	gdb_assert { [info procs old_foo] == "" } "old_foo does not exist"
     }
 }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index cdc3721a1cd..8235d4f28eb 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9726,10 +9726,18 @@ proc info_args_with_defaults { name } {
     return $args
 }
 
+# Use as either:
+#
+#   with_override NAME OVERRIDE BODY
+#   with_override NAME OVERRIDE SAVE_NAME BODY
+#
 # Override proc NAME to proc OVERRIDE for the duration of the execution of
-# BODY.
+# a BODY.
+#
+# If the SAVE_NAME form is used then NAME will be available as
+# SAVE_NAME for the duration of BODY.
 
-proc with_override { name override body } {
+proc with_override { name override args } {
     # Implementation note: It's possible to implement the override using
     # rename, like this:
     #   rename $name save_$name
@@ -9742,11 +9750,43 @@ proc with_override { name override body } {
     # - the override is no longer available under its original name during
     #   the override
     # So, we use this more elaborate but cleaner mechanism.
+    #
+    # When the SAVE_NAME argument is provided to with_override then we
+    # do use rename, but we first backup any existing proc called
+    # SAVE_NAME, delete the existing SAVE_NAME, and only then do the
+    # rename.
+
+    if { [llength $args] == 1 } {
+	set save_name ""
+	set body [lindex $args 0]
+    } elseif { [llength $args] == 2 } {
+	set save_name [lindex $args 0]
+	set body [lindex $args 1]
+    } else {
+	perror "invalid argument count to with_override: [llength $args]"
+	return
+    }
+
+    # If the user wants to save the original proc, but the name they'd
+    # like to save into already exists then capture details of the
+    # thing we're about to overwrite.
+    if { $save_name != "" && [info procs $save_name] != "" } {
+	set save_name_args [info_args_with_defaults $save_name]
+	set save_name_body [info body $save_name]
+	rename $save_name ""
+	set save_name_existed true
+    } else {
+	set save_name_existed false
+    }
 
     # Save the old proc, if it exists.
     if { [info procs $name] != "" } {
-	set old_args [info_args_with_defaults $name]
-	set old_body [info body $name]
+	if { $save_name != "" } {
+	    rename $name $save_name
+	} else {
+	    set old_args [info_args_with_defaults $name]
+	    set old_body [info body $name]
+	}
 	set existed true
     } else {
 	set existed false
@@ -9762,11 +9802,21 @@ proc with_override { name override body } {
 
     # Restore old proc if it existed on entry, else delete it.
     if { $existed } {
-	eval proc $name {$old_args} {$old_body}
+	if { $save_name != "" } {
+	    rename $name ""
+	    rename $save_name $name
+	} else {
+	    eval proc $name {$old_args} {$old_body}
+	}
     } else {
 	rename $name ""
     }
 
+    # Restore the proc we saved over, if necessary.
+    if { $save_name_existed } {
+	eval proc $save_name {$save_name_args} {$save_name_body}
+    }
+
     # Return as appropriate.
     if { $code == 1 } {
         global errorInfo errorCode
-- 
2.25.4

[PATCH 3/4] gdb/testsuite: restructure gdb_data_cache (lib/cache.exp)

[Andrew Burgess]

In a later commit I want to add more information to
gdb_data_cache (see lib/cache.exp).  Specifically I want to track if
the underlying function of a caching proc calls gdb_exit or not.

Currently gdb_data_cache is an associative array, the keys of which
are the name of the caching proc.

In this commit I add ',value' suffix to the gdb_data_cache keys.  In
later commits I'll add additional entries with different suffixes.

There should be no noticable changes after this commit, this is just a
restructuring.
---
 gdb/testsuite/lib/cache.exp | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
index 80667349f52..e7b9114058b 100644
--- a/gdb/testsuite/lib/cache.exp
+++ b/gdb/testsuite/lib/cache.exp
@@ -70,8 +70,8 @@ proc gdb_do_cache {name args} {
     set cache_name [file join [target_info name] $name {*}$args]
 
     set is_cached 0
-    if {[info exists gdb_data_cache($cache_name)]} {
-	set cached $gdb_data_cache($cache_name)
+    if {[info exists gdb_data_cache(${cache_name},value)]} {
+	set cached $gdb_data_cache(${cache_name},value)
 	verbose "$name: returning '$cached' from cache" 2
 	if { $cache_verify == 0 } {
 	    return $cached
@@ -83,9 +83,9 @@ proc gdb_do_cache {name args} {
 	set cache_filename [make_gdb_parallel_path cache $cache_name]
 	if {[file exists $cache_filename]} {
 	    set fd [open $cache_filename]
-	    set gdb_data_cache($cache_name) [read -nonewline $fd]
+	    set gdb_data_cache(${cache_name},value) [read -nonewline $fd]
 	    close $fd
-	    set cached $gdb_data_cache($cache_name)
+	    set cached $gdb_data_cache(${cache_name},value)
 	    verbose "$name: returning '$cached' from file cache" 2
 	    if { $cache_verify == 0 } {
 		return $cached
@@ -95,9 +95,9 @@ proc gdb_do_cache {name args} {
     }
 
     set real_name gdb_real__$name
-    set gdb_data_cache($cache_name) [gdb_do_cache_wrap $real_name {*}$args]
+    set gdb_data_cache(${cache_name},value) [gdb_do_cache_wrap $real_name {*}$args]
     if { $cache_verify == 1 && $is_cached == 1 } {
-	set computed $gdb_data_cache($cache_name)
+	set computed $gdb_data_cache(${cache_name},value)
 	if { $cached != $computed } {
 	    error [join [list "Inconsistent results for $cache_name:"
 			 "cached: $cached vs. computed: $computed"]]
@@ -105,15 +105,15 @@ proc gdb_do_cache {name args} {
     }
 
     if {[info exists GDB_PARALLEL]} {
-	verbose "$name: returning '$gdb_data_cache($cache_name)' and writing file" 2
+	verbose "$name: returning '$gdb_data_cache(${cache_name},value)' and writing file" 2
 	file mkdir [file dirname $cache_filename]
 	# Make sure to write the results file atomically.
 	set fd [open $cache_filename.[pid] w]
-	puts $fd $gdb_data_cache($cache_name)
+	puts $fd $gdb_data_cache(${cache_name},value)
 	close $fd
 	file rename -force -- $cache_filename.[pid] $cache_filename
     }
-    return $gdb_data_cache($cache_name)
+    return $gdb_data_cache(${cache_name},value)
 }
 
 # Define a new proc named NAME, with optional args ARGS.  BODY is the body of
-- 
2.25.4

[PATCH 4/4] gdb/testsuite: track if a caching proc calls gdb_exit or not

[Andrew Burgess]

After a recent patch review I asked myself why can_spawn_for_attach
exists.  This proc currently does some checks, and then calls
can_spawn_for_attach_1 which is an actual caching proc.

The answer is that can_spawn_for_attach exists in order to call
gdb_exit the first time can_spawn_for_attach is called within any test
script.

The reason this is useful is that can_spawn_for_attach_1 calls
gdb_exit.  If imagine the user calling can_spawn_for_attach_1 directly
then a problem might exist.  Imagine a test written like this:

  gdb_start

  if { [can_spawn_for_attach_1] } {
    ... do stuff that assumes GDB is running ...
  }

If this test is NOT the first test run, and if an earlier test calls
can_spawn_for_attach_1, then when the above test is run the
can_spawn_for_attach_1 call will return the cached value and gdb_exit
will not be called.

But, if the above test IS the first test run then
can_spawn_for_attach_1 will not returned the cached value, but will
instead compute the cached value, a process that ends up calling
gdb_exit.  When the body of the if is executed GDB would no longer be
running and the test would fail!

So can_spawn_for_attach was added which ensures that we _always_ call
gdb_exit the first time can_spawn_for_attach is called within a single
test script, this ensures that in the above case, even if the above is
not the first test run, gdb_exit will still be called.  This avoids
some hidden bugs in the testsuite.

However, what I observe is that can_spawn_for_attach is not the only
caching proc that calls gdb_exit.  Why does can_spawn_for_attach get
special treatment when surely the same issue exists for any other
caching proc that calls gdb_exit?

I think a better solution is to move the logic from
can_spawn_for_attach into cache.exp and generalise it so that it
applies to all caching procs.

This commit does this by:

 1. When the underlying caching proc is executed we wrap gdb_exit.
    This wrapper sets a global to true if gdb_exit is called.  The
    value of this global is stored in gdb_data_cache (using a ',exit'
    suffix), and also written to the cache file if appropriate.

 2. When a cached value is returned from gdb_do_cache, if the
    underlying proc would have called gdb_exit, and if this is the
    first use of the caching proc in this test script, then we call
    gdb_exit.

When storing the ',exit' value into the on-disk cache file, the flag
value is stored on a second line.  Currently every cached value only
occupies a single line, and a check is added to ensure this remains
true in the future.

One issue did come up in testing, a FAIL in gdb.base/break-interp.exp,
this was caused by can_spawn_for_attach_1 calling gdb_start without
first calling gdb_exit.  Under the old way of doing things
can_spawn_for_attach would call gdb_exit _before_ possibly calling the
actual caching proc.  Under the new scheme gdb_exit is called _after_
calling the actual caching proc.  What was happening was that
break-interp.exp would leave GDB running then call
can_spawn_for_attach, when the test in can_spawn_for_attach_1 tried to
attach to the inferior, state left in the running GDB would cause some
unexpected behaviour.  Fixed by having can_spawn_for_attach_1 call
gdb_exit before calling gdb_start, this ensures we have a fresh GDB.

With this done can_spawn_for_attach_1 can be renamed to
can_spawn_for_attach, and the existing can_spawn_for_attach can be
deleted.
---
 gdb/testsuite/lib/cache.exp | 86 +++++++++++++++++++++++++++++++------
 gdb/testsuite/lib/gdb.exp   | 83 +++++++++--------------------------
 2 files changed, 93 insertions(+), 76 deletions(-)

diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
index e7b9114058b..fef065ec8b0 100644
--- a/gdb/testsuite/lib/cache.exp
+++ b/gdb/testsuite/lib/cache.exp
@@ -46,6 +46,40 @@ proc gdb_do_cache_wrap {real_name args} {
     return $result
 }
 
+# Global written to by wrap_gdb_exit.  Set to true if wrap_gdb_exit is
+# called.
+
+set gdb_exit_called false
+
+# Wrapper around gdb_exit.  Use with_override to replace gdb_exit with
+# wrap_gdb_exit, the original gdb_exit is renamed to orig_gdb_exit.
+
+proc wrap_gdb_exit {} {
+    set ::gdb_exit_called true
+    orig_gdb_exit
+}
+
+# If DO_EXIT is false then this proc does nothing.  If DO_EXIT is true
+# then call gdb_exit the first time this proc is called for each
+# unique value of NAME within a single test.  Every subsequent time
+# this proc is called within a single test (for a given value of
+# NAME), don't call gdb_exit.
+
+proc gdb_cache_maybe_gdb_exit { name do_exit } {
+    if { !$do_exit } {
+	return
+    }
+
+    # To track if this proc has been called for NAME we create a
+    # global variable.  In gdb_cleanup_globals (see gdb.exp) this
+    # global will be deleted when the test has finished.
+    set global_name __${name}__cached_gdb_exit_called
+    if { ![info exists ::${global_name}] } {
+	gdb_exit
+	set ::${global_name} true
+    }
+}
+
 # A helper for gdb_caching_proc that handles the caching.
 
 proc gdb_do_cache {name args} {
@@ -71,10 +105,12 @@ proc gdb_do_cache {name args} {
 
     set is_cached 0
     if {[info exists gdb_data_cache(${cache_name},value)]} {
-	set cached $gdb_data_cache(${cache_name},value)
-	verbose "$name: returning '$cached' from cache" 2
+	set cached_value $gdb_data_cache(${cache_name},value)
+	set cached_exit $gdb_data_cache(${cache_name},exit)
+	verbose "$name: returning '$cached_value' from cache" 2
 	if { $cache_verify == 0 } {
-	    return $cached
+	    gdb_cache_maybe_gdb_exit $name $cached_exit
+	    return $cached_value
 	}
 	set is_cached 1
     }
@@ -83,24 +119,46 @@ proc gdb_do_cache {name args} {
 	set cache_filename [make_gdb_parallel_path cache $cache_name]
 	if {[file exists $cache_filename]} {
 	    set fd [open $cache_filename]
-	    set gdb_data_cache(${cache_name},value) [read -nonewline $fd]
+	    set content [split [read -nonewline $fd] \n]
 	    close $fd
-	    set cached $gdb_data_cache(${cache_name},value)
-	    verbose "$name: returning '$cached' from file cache" 2
+	    set gdb_data_cache(${cache_name},value) [lindex $content 0]
+	    set gdb_data_cache(${cache_name},exit) [lindex $content 1]
+	    set cached_value $gdb_data_cache(${cache_name},value)
+	    set cached_exit $gdb_data_cache(${cache_name},exit)
+	    verbose "$name: returning '$cached_value' from file cache" 2
 	    if { $cache_verify == 0 } {
-		return $cached
+		gdb_cache_maybe_gdb_exit $name $cached_exit
+		return $cached_value
 	    }
 	    set is_cached 1
 	}
     }
 
-    set real_name gdb_real__$name
-    set gdb_data_cache(${cache_name},value) [gdb_do_cache_wrap $real_name {*}$args]
+    set ::gdb_exit_called false
+    with_override gdb_exit wrap_gdb_exit orig_gdb_exit {
+	set real_name gdb_real__$name
+	set gdb_data_cache(${cache_name},value) [gdb_do_cache_wrap $real_name {*}$args]
+    }
+    set gdb_data_cache(${cache_name},exit) $::gdb_exit_called
+
+    # If a value being stored in the cache contains a newline then
+    # when we try to read the value back from an on-disk cache file
+    # we'll interpret the second line of the value as the ',exit' value.
+    if { [regexp "\[\r\n\]" $gdb_data_cache(${cache_name},value)] } {
+	set computed_value $gdb_data_cache(${cache_name},value)
+	error "Newline found in value for $cache_name: $computed_value"
+    }
+
     if { $cache_verify == 1 && $is_cached == 1 } {
-	set computed $gdb_data_cache(${cache_name},value)
-	if { $cached != $computed } {
-	    error [join [list "Inconsistent results for $cache_name:"
-			 "cached: $cached vs. computed: $computed"]]
+	set computed_value $gdb_data_cache(${cache_name},value)
+	set computed_exit $gdb_data_cache(${cache_name},exit)
+	if { $cached_value != $computed_value } {
+	    error [join [list "Inconsistent value results for $cache_name:"
+			 "cached: $cached_value vs. computed: $computed_value"]]
+	}
+	if { $cached_exit != $computed_exit } {
+	    error [join [list "Inconsistent exit results for $cache_name:"
+			 "cached: $cached_exit vs. computed: $computed_exit"]]
 	}
     }
 
@@ -110,9 +168,11 @@ proc gdb_do_cache {name args} {
 	# Make sure to write the results file atomically.
 	set fd [open $cache_filename.[pid] w]
 	puts $fd $gdb_data_cache(${cache_name},value)
+	puts $fd $gdb_data_cache(${cache_name},exit)
 	close $fd
 	file rename -force -- $cache_filename.[pid] $cache_filename
     }
+    gdb_cache_maybe_gdb_exit $name $gdb_data_cache(${cache_name},exit)
     return $gdb_data_cache(${cache_name},value)
 }
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 8235d4f28eb..d29fd740f91 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -6186,14 +6186,23 @@ proc gdb_exit { } {
     catch default_gdb_exit
 }
 
-# Helper function for can_spawn_for_attach.  Try to spawn and attach, and
-# return 0 only if we cannot attach because it's unsupported.
-
-gdb_caching_proc can_spawn_for_attach_1 {} {
-    # For the benefit of gdb-caching-proc-consistency.exp, which
-    # calls can_spawn_for_attach_1 directly.  Keep in sync with
-    # can_spawn_for_attach.
-    if { [is_remote target] || [target_info exists use_gdb_stub] } {
+# Return true if we can spawn a program on the target and attach to
+# it.
+
+gdb_caching_proc can_spawn_for_attach {} {
+    # We use exp_pid to get the inferior's pid, assuming that gives
+    # back the pid of the program.  On remote boards, that would give
+    # us instead the PID of e.g., the ssh client, etc.
+    if {[is_remote target]} {
+	verbose -log "can't spawn for attach (target is remote)"
+	return 0
+    }
+
+    # The "attach" command doesn't make sense when the target is
+    # stub-like, where GDB finds the program already started on
+    # initial connection.
+    if {[target_info exists use_gdb_stub]} {
+	verbose -log "can't spawn for attach (target is stub)"
 	return 0
     }
 
@@ -6218,6 +6227,9 @@ gdb_caching_proc can_spawn_for_attach_1 {} {
     set test_spawn_id [spawn_wait_for_attach_1 $obj]
     remote_file build delete $obj
 
+    # In case GDB is already running.
+    gdb_exit
+    
     gdb_start
 
     set test_pid [spawn_id_get_pid $test_spawn_id]
@@ -6239,61 +6251,6 @@ gdb_caching_proc can_spawn_for_attach_1 {} {
     return $res
 }
 
-# Return true if we can spawn a program on the target and attach to
-# it.  Calls gdb_exit for the first call in a test-case.
-
-proc can_spawn_for_attach { } {
-    # We use exp_pid to get the inferior's pid, assuming that gives
-    # back the pid of the program.  On remote boards, that would give
-    # us instead the PID of e.g., the ssh client, etc.
-    if {[is_remote target]} {
-	verbose -log "can't spawn for attach (target is remote)"
-	return 0
-    }
-
-    # The "attach" command doesn't make sense when the target is
-    # stub-like, where GDB finds the program already started on
-    # initial connection.
-    if {[target_info exists use_gdb_stub]} {
-	verbose -log "can't spawn for attach (target is stub)"
-	return 0
-    }
-
-    # The normal sequence to use for a runtime test like
-    # can_spawn_for_attach_1 is:
-    # - gdb_exit (don't use a running gdb, we don't know what state it is in),
-    # - gdb_start (start a new gdb), and
-    # - gdb_exit (cleanup).
-    #
-    # By making can_spawn_for_attach_1 a gdb_caching_proc, we make it
-    # unpredictable which test-case will call it first, and consequently a
-    # test-case may pass in say a full test run, but fail when run
-    # individually, due to a can_spawn_for_attach call in a location where a
-    # gdb_exit (as can_spawn_for_attach_1 does) breaks things.
-    # To avoid this, we move the initial gdb_exit out of
-    # can_spawn_for_attach_1, guaranteeing that we end up in the same state
-    # regardless of whether can_spawn_for_attach_1 is called.  However, that
-    # is only necessary for the first call in a test-case, so cache the result
-    # in a global (which should be reset after each test-case) to keep track
-    # of that.
-    #
-    # In summary, we distinguish between three cases:
-    # - first call in first test-case.  Executes can_spawn_for_attach_1.
-    #   Calls gdb_exit, gdb_start, gdb_exit.
-    # - first call in following test-cases.  Uses cached result of
-    #   can_spawn_for_attach_1.  Calls gdb_exit.
-    # - rest.  Use cached result in cache_can_spawn_for_attach_1. Calls no
-    #   gdb_start or gdb_exit.
-    global cache_can_spawn_for_attach_1
-    if { [info exists cache_can_spawn_for_attach_1] } {
-	return $cache_can_spawn_for_attach_1
-    }
-    gdb_exit
-
-    set cache_can_spawn_for_attach_1 [can_spawn_for_attach_1]
-    return $cache_can_spawn_for_attach_1
-}
-
 # Centralize the failure checking of "attach" command.
 # Return 0 if attach failed, otherwise return 1.
 
-- 
2.25.4

Re: [PATCH 0/4] gdb/testsuite: remove can_spawn_for_attach_1

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> A recent patch got me wondering why we have can_spawn_for_attach_1 and
> can_spawn_for_attach.  I think we probably shouldn't.  This series
> moves the functionality from can_spawn_for_attach into gdb_do_cache
> (lib/cache.exp), removing the need for two can_spawn_for_attach*
> procs.

There's a problem with patch #2 of this series that I don't currently
have a solution too.  Please ignore this for now.

Thanks,
Andrew

[PATCHv2 0/2] gdb/testsuite: remove can_spawn_for_attach_1

[Andrew Burgess]

A recent patch got me wondering why we have can_spawn_for_attach_1 and
can_spawn_for_attach.  I think we probably shouldn't.  This series
moves the functionality from can_spawn_for_attach into gdb_do_cache
(lib/cache.exp), removing the need for two can_spawn_for_attach*
procs.

Changes from V1:

  The old #1 patch was approved and has been merged.
  
  The Linaro CI revealed an issue with patch #2 in the old v1 series, so
  I've dropped this patch and reworked a later patch to remove the need
  for this change.
  
  Patch #3 from the old series is the new #1, and is unchanged.
  
  Patch #4 from the old series is the new #2, and is slightly changed
  from the v1 series.

---

Andrew Burgess (2):
  gdb/testsuite: restructure gdb_data_cache (lib/cache.exp)
  gdb/testsuite: track if a caching proc calls gdb_exit or not

 gdb/testsuite/lib/cache.exp | 130 +++++++++++++++++++++++++++++++-----
 gdb/testsuite/lib/gdb.exp   |  83 ++++++-----------------
 2 files changed, 134 insertions(+), 79 deletions(-)


base-commit: 2db414c36b4f030782c2c8a24c916c3033261af0
-- 
2.25.4

[PATCHv2 1/2] gdb/testsuite: restructure gdb_data_cache (lib/cache.exp)

[Andrew Burgess]

In a later commit I want to add more information to
gdb_data_cache (see lib/cache.exp).  Specifically I want to track if
the underlying function of a caching proc calls gdb_exit or not.

Currently gdb_data_cache is an associative array, the keys of which
are the name of the caching proc.

In this commit I add ',value' suffix to the gdb_data_cache keys.  In
later commits I'll add additional entries with different suffixes.

There should be no noticable changes after this commit, this is just a
restructuring.
---
 gdb/testsuite/lib/cache.exp | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
index 80667349f52..e7b9114058b 100644
--- a/gdb/testsuite/lib/cache.exp
+++ b/gdb/testsuite/lib/cache.exp
@@ -70,8 +70,8 @@ proc gdb_do_cache {name args} {
     set cache_name [file join [target_info name] $name {*}$args]
 
     set is_cached 0
-    if {[info exists gdb_data_cache($cache_name)]} {
-	set cached $gdb_data_cache($cache_name)
+    if {[info exists gdb_data_cache(${cache_name},value)]} {
+	set cached $gdb_data_cache(${cache_name},value)
 	verbose "$name: returning '$cached' from cache" 2
 	if { $cache_verify == 0 } {
 	    return $cached
@@ -83,9 +83,9 @@ proc gdb_do_cache {name args} {
 	set cache_filename [make_gdb_parallel_path cache $cache_name]
 	if {[file exists $cache_filename]} {
 	    set fd [open $cache_filename]
-	    set gdb_data_cache($cache_name) [read -nonewline $fd]
+	    set gdb_data_cache(${cache_name},value) [read -nonewline $fd]
 	    close $fd
-	    set cached $gdb_data_cache($cache_name)
+	    set cached $gdb_data_cache(${cache_name},value)
 	    verbose "$name: returning '$cached' from file cache" 2
 	    if { $cache_verify == 0 } {
 		return $cached
@@ -95,9 +95,9 @@ proc gdb_do_cache {name args} {
     }
 
     set real_name gdb_real__$name
-    set gdb_data_cache($cache_name) [gdb_do_cache_wrap $real_name {*}$args]
+    set gdb_data_cache(${cache_name},value) [gdb_do_cache_wrap $real_name {*}$args]
     if { $cache_verify == 1 && $is_cached == 1 } {
-	set computed $gdb_data_cache($cache_name)
+	set computed $gdb_data_cache(${cache_name},value)
 	if { $cached != $computed } {
 	    error [join [list "Inconsistent results for $cache_name:"
 			 "cached: $cached vs. computed: $computed"]]
@@ -105,15 +105,15 @@ proc gdb_do_cache {name args} {
     }
 
     if {[info exists GDB_PARALLEL]} {
-	verbose "$name: returning '$gdb_data_cache($cache_name)' and writing file" 2
+	verbose "$name: returning '$gdb_data_cache(${cache_name},value)' and writing file" 2
 	file mkdir [file dirname $cache_filename]
 	# Make sure to write the results file atomically.
 	set fd [open $cache_filename.[pid] w]
-	puts $fd $gdb_data_cache($cache_name)
+	puts $fd $gdb_data_cache(${cache_name},value)
 	close $fd
 	file rename -force -- $cache_filename.[pid] $cache_filename
     }
-    return $gdb_data_cache($cache_name)
+    return $gdb_data_cache(${cache_name},value)
 }
 
 # Define a new proc named NAME, with optional args ARGS.  BODY is the body of
-- 
2.25.4

[PATCHv2 2/2] gdb/testsuite: track if a caching proc calls gdb_exit or not

[Andrew Burgess]

After a recent patch review I asked myself why can_spawn_for_attach
exists.  This proc currently does some checks, and then calls
can_spawn_for_attach_1 which is an actual caching proc.

The answer is that can_spawn_for_attach exists in order to call
gdb_exit the first time can_spawn_for_attach is called within any test
script.

The reason this is useful is that can_spawn_for_attach_1 calls
gdb_exit.  If imagine the user calling can_spawn_for_attach_1 directly
then a problem might exist.  Imagine a test written like this:

  gdb_start

  if { [can_spawn_for_attach_1] } {
    ... do stuff that assumes GDB is running ...
  }

If this test is NOT the first test run, and if an earlier test calls
can_spawn_for_attach_1, then when the above test is run the
can_spawn_for_attach_1 call will return the cached value and gdb_exit
will not be called.

But, if the above test IS the first test run then
can_spawn_for_attach_1 will not returned the cached value, but will
instead compute the cached value, a process that ends up calling
gdb_exit.  When the body of the if is executed GDB would no longer be
running and the test would fail!

So can_spawn_for_attach was added which ensures that we _always_ call
gdb_exit the first time can_spawn_for_attach is called within a single
test script, this ensures that in the above case, even if the above is
not the first test run, gdb_exit will still be called.  This avoids
some hidden bugs in the testsuite.

However, what I observe is that can_spawn_for_attach is not the only
caching proc that calls gdb_exit.  Why does can_spawn_for_attach get
special treatment when surely the same issue exists for any other
caching proc that calls gdb_exit?

I think a better solution is to move the logic from
can_spawn_for_attach into cache.exp and generalise it so that it
applies to all caching procs.

This commit does this by:

 1. When the underlying caching proc is executed we track calls to
    gdb_exit.  If a caching proc calls gdb_exit then this information
    is stored in gdb_data_cache (using a ',exit' suffix), and also
    written to the cache file if appropriate.

 2. When a cached value is returned from gdb_do_cache, if the
    underlying proc would have called gdb_exit, and if this is the
    first use of the caching proc in this test script, then we call
    gdb_exit.

When storing the ',exit' value into the on-disk cache file, the flag
value is stored on a second line.  Currently every cached value only
occupies a single line, and a check is added to ensure this remains
true in the future.

To track calls to gdb_exit I eventually settled on using TCL's trace
mechanism.  We already make use of this in lib/gdb.exp so I figure
this is OK to use.  This should be fine, so long as non of the caching
procs use 'with_override' to replace gdb_exit, or do any other proc
replacement to change gdb_exit, however, I think that is pretty
unlikely.

One issue did come up in testing, a FAIL in gdb.base/break-interp.exp,
prior to this commit can_spawn_for_attach would call gdb_exit before
calling the underlying caching proc.  After this call we call gdb_exit
after calling the caching proc.

The underlying caching proc relies on gdb_exit having been called.  To
resolve this issue I just added a call to gdb_exit into
can_spawn_for_attach.

With this done can_spawn_for_attach_1 can be renamed to
can_spawn_for_attach, and the existing can_spawn_for_attach can be
deleted.
---
 gdb/testsuite/lib/cache.exp | 120 ++++++++++++++++++++++++++++++++----
 gdb/testsuite/lib/gdb.exp   |  83 ++++++-------------------
 2 files changed, 129 insertions(+), 74 deletions(-)

diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
index e7b9114058b..e5d134aa628 100644
--- a/gdb/testsuite/lib/cache.exp
+++ b/gdb/testsuite/lib/cache.exp
@@ -46,6 +46,40 @@ proc gdb_do_cache_wrap {real_name args} {
     return $result
 }
 
+# Global written to by gdb_exit_called proc.  Is set to true to
+# indicate that a caching proc has called gdb_exit.
+
+set gdb_exit_called false
+
+# This proc is called via TCL's trace mechanism whenever gdb_exit is
+# called during the execution of a caching proc.  This sets the global
+# flag to indicate that gdb_exit has been called.
+
+proc gdb_exit_called { args } {
+    set ::gdb_exit_called true
+}
+
+# If DO_EXIT is false then this proc does nothing.  If DO_EXIT is true
+# then call gdb_exit the first time this proc is called for each
+# unique value of NAME within a single test.  Every subsequent time
+# this proc is called within a single test (for a given value of
+# NAME), don't call gdb_exit.
+
+proc gdb_cache_maybe_gdb_exit { name do_exit } {
+    if { !$do_exit } {
+	return
+    }
+
+    # To track if this proc has been called for NAME we create a
+    # global variable.  In gdb_cleanup_globals (see gdb.exp) this
+    # global will be deleted when the test has finished.
+    set global_name __${name}__cached_gdb_exit_called
+    if { ![info exists ::${global_name}] } {
+	gdb_exit
+	set ::${global_name} true
+    }
+}
+
 # A helper for gdb_caching_proc that handles the caching.
 
 proc gdb_do_cache {name args} {
@@ -71,10 +105,12 @@ proc gdb_do_cache {name args} {
 
     set is_cached 0
     if {[info exists gdb_data_cache(${cache_name},value)]} {
-	set cached $gdb_data_cache(${cache_name},value)
-	verbose "$name: returning '$cached' from cache" 2
+	set cached_value $gdb_data_cache(${cache_name},value)
+	set cached_exit $gdb_data_cache(${cache_name},exit)
+	verbose "$name: returning '$cached_value' from cache" 2
 	if { $cache_verify == 0 } {
-	    return $cached
+	    gdb_cache_maybe_gdb_exit $name $cached_exit
+	    return $cached_value
 	}
 	set is_cached 1
     }
@@ -83,24 +119,84 @@ proc gdb_do_cache {name args} {
 	set cache_filename [make_gdb_parallel_path cache $cache_name]
 	if {[file exists $cache_filename]} {
 	    set fd [open $cache_filename]
-	    set gdb_data_cache(${cache_name},value) [read -nonewline $fd]
+	    set content [split [read -nonewline $fd] \n]
 	    close $fd
-	    set cached $gdb_data_cache(${cache_name},value)
-	    verbose "$name: returning '$cached' from file cache" 2
+	    set gdb_data_cache(${cache_name},value) [lindex $content 0]
+	    set gdb_data_cache(${cache_name},exit) [lindex $content 1]
+	    set cached_value $gdb_data_cache(${cache_name},value)
+	    set cached_exit $gdb_data_cache(${cache_name},exit)
+	    verbose "$name: returning '$cached_value' from file cache" 2
 	    if { $cache_verify == 0 } {
-		return $cached
+		gdb_cache_maybe_gdb_exit $name $cached_exit
+		return $cached_value
 	    }
 	    set is_cached 1
 	}
     }
 
+    # Add a trace hook to gdb_exit.  In the case of recursive calls to
+    # gdb_do_cache we only want to install the trace hook once, so we
+    # set a global to indicate that the trace is in place.
+    #
+    # We also set a local flag to indicate that this is the scope in
+    # which the debug trace needs to be removed.
+    if { ![info exists ::gdb_exit_trace_in_place] } {
+	trace add execution gdb_exit enter gdb_exit_called
+	set ::gdb_exit_trace_in_place true
+	set gdb_exit_trace_created true
+    } else {
+	set gdb_exit_trace_created false
+    }
+
+    # As above, we need to consider recursive calls into gdb_do_cache.
+    # Store the old value of gdb_exit_called global and then set the
+    # flag to false.  Initially gdb_exit_called is always false, but
+    # for recursive calls to gdb_do_cache we can't know the state of
+    # gdb_exit_called.
+    #
+    # Before starting a recursive gdb_do_cache call we need
+    # gdb_exit_called to be false, that way the inner call can know if
+    # it invoked gdb_exit or not.
+    #
+    # Once the recursive call completes, if it did call gdb_exit then
+    # the outer, parent call to gdb_do_cache should also be considered
+    # as having called gdb_exit.
+    set old_gdb_exit_called $::gdb_exit_called
+    set ::gdb_exit_called false
+
     set real_name gdb_real__$name
     set gdb_data_cache(${cache_name},value) [gdb_do_cache_wrap $real_name {*}$args]
+    set gdb_data_cache(${cache_name},exit) $::gdb_exit_called
+
+    # See comment above where OLD_GDB_EXIT_CALLED is set.
+    if { $old_gdb_exit_called } {
+	set ::gdb_exit_called true
+    }
+
+    # See comment above where GDB_EXIT_TRACE_CREATED is set.
+    if { $gdb_exit_trace_created } {
+	trace remove execution gdb_exit enter gdb_exit_called
+	unset ::gdb_exit_trace_in_place
+    }
+
+    # If a value being stored in the cache contains a newline then
+    # when we try to read the value back from an on-disk cache file
+    # we'll interpret the second line of the value as the ',exit' value.
+    if { [regexp "\[\r\n\]" $gdb_data_cache(${cache_name},value)] } {
+	set computed_value $gdb_data_cache(${cache_name},value)
+	error "Newline found in value for $cache_name: $computed_value"
+    }
+
     if { $cache_verify == 1 && $is_cached == 1 } {
-	set computed $gdb_data_cache(${cache_name},value)
-	if { $cached != $computed } {
-	    error [join [list "Inconsistent results for $cache_name:"
-			 "cached: $cached vs. computed: $computed"]]
+	set computed_value $gdb_data_cache(${cache_name},value)
+	set computed_exit $gdb_data_cache(${cache_name},exit)
+	if { $cached_value != $computed_value } {
+	    error [join [list "Inconsistent value results for $cache_name:"
+			 "cached: $cached_value vs. computed: $computed_value"]]
+	}
+	if { $cached_exit != $computed_exit } {
+	    error [join [list "Inconsistent exit results for $cache_name:"
+			 "cached: $cached_exit vs. computed: $computed_exit"]]
 	}
     }
 
@@ -110,9 +206,11 @@ proc gdb_do_cache {name args} {
 	# Make sure to write the results file atomically.
 	set fd [open $cache_filename.[pid] w]
 	puts $fd $gdb_data_cache(${cache_name},value)
+	puts $fd $gdb_data_cache(${cache_name},exit)
 	close $fd
 	file rename -force -- $cache_filename.[pid] $cache_filename
     }
+    gdb_cache_maybe_gdb_exit $name $gdb_data_cache(${cache_name},exit)
     return $gdb_data_cache(${cache_name},value)
 }
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 5aeaa8d09b4..e439d6dd9c0 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -6199,14 +6199,23 @@ proc gdb_exit { } {
     catch default_gdb_exit
 }
 
-# Helper function for can_spawn_for_attach.  Try to spawn and attach, and
-# return 0 only if we cannot attach because it's unsupported.
-
-gdb_caching_proc can_spawn_for_attach_1 {} {
-    # For the benefit of gdb-caching-proc-consistency.exp, which
-    # calls can_spawn_for_attach_1 directly.  Keep in sync with
-    # can_spawn_for_attach.
-    if { [is_remote target] || [target_info exists use_gdb_stub] } {
+# Return true if we can spawn a program on the target and attach to
+# it.
+
+gdb_caching_proc can_spawn_for_attach {} {
+    # We use exp_pid to get the inferior's pid, assuming that gives
+    # back the pid of the program.  On remote boards, that would give
+    # us instead the PID of e.g., the ssh client, etc.
+    if {[is_remote target]} {
+	verbose -log "can't spawn for attach (target is remote)"
+	return 0
+    }
+
+    # The "attach" command doesn't make sense when the target is
+    # stub-like, where GDB finds the program already started on
+    # initial connection.
+    if {[target_info exists use_gdb_stub]} {
+	verbose -log "can't spawn for attach (target is stub)"
 	return 0
     }
 
@@ -6231,6 +6240,9 @@ gdb_caching_proc can_spawn_for_attach_1 {} {
     set test_spawn_id [spawn_wait_for_attach_1 $obj]
     remote_file build delete $obj
 
+    # In case GDB is already running.
+    gdb_exit
+
     gdb_start
 
     set test_pid [spawn_id_get_pid $test_spawn_id]
@@ -6252,61 +6264,6 @@ gdb_caching_proc can_spawn_for_attach_1 {} {
     return $res
 }
 
-# Return true if we can spawn a program on the target and attach to
-# it.  Calls gdb_exit for the first call in a test-case.
-
-proc can_spawn_for_attach { } {
-    # We use exp_pid to get the inferior's pid, assuming that gives
-    # back the pid of the program.  On remote boards, that would give
-    # us instead the PID of e.g., the ssh client, etc.
-    if {[is_remote target]} {
-	verbose -log "can't spawn for attach (target is remote)"
-	return 0
-    }
-
-    # The "attach" command doesn't make sense when the target is
-    # stub-like, where GDB finds the program already started on
-    # initial connection.
-    if {[target_info exists use_gdb_stub]} {
-	verbose -log "can't spawn for attach (target is stub)"
-	return 0
-    }
-
-    # The normal sequence to use for a runtime test like
-    # can_spawn_for_attach_1 is:
-    # - gdb_exit (don't use a running gdb, we don't know what state it is in),
-    # - gdb_start (start a new gdb), and
-    # - gdb_exit (cleanup).
-    #
-    # By making can_spawn_for_attach_1 a gdb_caching_proc, we make it
-    # unpredictable which test-case will call it first, and consequently a
-    # test-case may pass in say a full test run, but fail when run
-    # individually, due to a can_spawn_for_attach call in a location where a
-    # gdb_exit (as can_spawn_for_attach_1 does) breaks things.
-    # To avoid this, we move the initial gdb_exit out of
-    # can_spawn_for_attach_1, guaranteeing that we end up in the same state
-    # regardless of whether can_spawn_for_attach_1 is called.  However, that
-    # is only necessary for the first call in a test-case, so cache the result
-    # in a global (which should be reset after each test-case) to keep track
-    # of that.
-    #
-    # In summary, we distinguish between three cases:
-    # - first call in first test-case.  Executes can_spawn_for_attach_1.
-    #   Calls gdb_exit, gdb_start, gdb_exit.
-    # - first call in following test-cases.  Uses cached result of
-    #   can_spawn_for_attach_1.  Calls gdb_exit.
-    # - rest.  Use cached result in cache_can_spawn_for_attach_1. Calls no
-    #   gdb_start or gdb_exit.
-    global cache_can_spawn_for_attach_1
-    if { [info exists cache_can_spawn_for_attach_1] } {
-	return $cache_can_spawn_for_attach_1
-    }
-    gdb_exit
-
-    set cache_can_spawn_for_attach_1 [can_spawn_for_attach_1]
-    return $cache_can_spawn_for_attach_1
-}
-
 # Centralize the failure checking of "attach" command.
 # Return 0 if attach failed, otherwise return 1.
 
-- 
2.25.4