[PATCH 0/2] Fix mi-exec-run.exp with native-extended-gdbserver

[PATCH 0/2] Fix mi-exec-run.exp with native-extended-gdbserver

[Andrew Burgess]

While testing another patch I hit some unstable test failures in
mi-exec-run.exp, when using native-extended-gdbserver board.

These two patches fix the two separate issues that were impacting this
test.

Now I see no failures for mi-exec-run.exp using the unix board, or the
native-extended-gdbserver board.

---

Andrew Burgess (2):
  gdb: fix failures in gdb.mi/mi-exec-run.exp with
    native-extended-gdbserver
  gdb/testsuite: fix native-extended-gdbserver failure in
    mi-exec-run.exp

 .../boards/native-extended-gdbserver.exp      |  2 +-
 gdb/testsuite/gdb.mi/mi-break-qualified.exp   |  2 +-
 gdb/testsuite/gdb.mi/mi-exec-run.exp          | 91 ++++++++++++++-----
 gdb/testsuite/lib/mi-support.exp              | 24 +++--
 4 files changed, 86 insertions(+), 33 deletions(-)

-- 
2.25.4

[PATCH 1/2] gdb: fix failures in gdb.mi/mi-exec-run.exp with native-extended-gdbserver

[Andrew Burgess]

When running the gdb.mi/mi-exec-run.exp test using the
native-extended-gdbserver I see failures like this:

  FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=main: mi=main: force-fail=1: run failure detected
  FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=main: mi=separate: force-fail=1: run failure detected
  FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=1: run failure detected

There's a race condition here, so you might see a slightly different
set of failures, but I always see some from the 'run failure detected'
test.

NOTE: I also see an additional test failure:

 FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=0: breakpoint hit reported on console (timeout)

but that is a completely different issue, and is not being addressed
in this commit.

The problem for the 'run failure detected' test is that we end up
in gdb_expect looking for output from two spawn-ids, one from
gdbserver, and one from gdb.  We're looking for one output pattern
from each spawn-id, and for the test to pass we need to see both
patterns.

Now, if gdb exits then this is a test failure (this would indicate gdb
crashing, which is bad), so we have the eof pattern associated with
the gdb spawn-id.

However, in this particular test we expect gdbserver to fail to
execute the binary (the test binary is set non-executable), and so we
get an error message from gdbserver (which matches the pattern), and
then gdbserver exits, this is expected.

In our gdb_expect call we don't have an eof pattern for gdbserver, and
so, when gdbserver exits, expect sees eof and drops out of the
gdb_expect call.

If both the gdbserver AND gdb patterns are seen before the gdbserver
eof triggers then everything is fine.

But, if expect sees gdbserver's eof before seeing the output from gdb
then we will exit the gdb_expect call, and (as we have not seen both
patterns) we will call 'fail'.

I initially tried to fix this by adding a new eof pattern for the
gdbserver spawn-id that called exp_continue, however, this causes
expect to throw an error, it is not happy to have '-i' options that
make use of a closed spawn-id.

So, the solution I propose here is to wrap the gdb_expect call in a
loop.

I then dynamically build the set of patterns that will be passed to
the gdb_expect call, excluding the gdbserver related patterns if the
gdbserver has already exited.

Now, the first time around the loop, as gdb and gdbserver are alive, I
add all the patterns and enter gdb_expect just like before.

When gdbserver exits we drop out of gdb_expect and go around the loop
again.  This time we only add the gdb related patterns, and reenter
the gdb_expect call.  Once the gdb related pattern is seen we drop out
of the gdb_expect call and exit the loop.

I now see no failures relating to 'run failure detected'.
---
 gdb/testsuite/gdb.mi/mi-exec-run.exp | 89 +++++++++++++++++++++-------
 1 file changed, 67 insertions(+), 22 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-exec-run.exp b/gdb/testsuite/gdb.mi/mi-exec-run.exp
index f397159078f..d736e19090a 100644
--- a/gdb/testsuite/gdb.mi/mi-exec-run.exp
+++ b/gdb/testsuite/gdb.mi/mi-exec-run.exp
@@ -90,35 +90,80 @@ proc test {inftty_mode mi_mode force_fail} {
 	set saw_perm_error 0
 	set saw_mi_error 0
 	set already_failed 0
+	set inferior_exited false
+
 	set test "run failure detected"
 	send_gdb "-exec-run --start\n"
 
-	gdb_expect {
-	    -i "$inferior_spawn_id"
-	    -re ".*Cannot exec.*Permission denied" {
-		set saw_perm_error 1
-		verbose -log "saw perm error"
-		if {!$saw_mi_error} {
-		    exp_continue
+	# The following call to gdb_expect is embedded within this
+	# loop in order to allow testing with the
+	# native-extended-gdbserver board.  Please test with this
+	# board if making any changes to the following loop.
+	while { ! [expr ($saw_perm_error && $saw_mi_error) \
+		       || $already_failed] } {
+
+	    # Place a list of expect pattern/command blocks into CODE,
+	    # then pass these to a gdb_expect call at the end of this
+	    # loop.
+	    set code {}
+
+	    # If the inferior has not exited then we add two patterns,
+	    # the first looks for the expected output from the
+	    # inferior, the second detects eof from the inferior.  If
+	    # the inferior is gdbserver, then after complaining about
+	    # permission denied, gdbserver will exit (which triggers
+	    # the eof pattern).
+	    #
+	    # When we see the eof pattern we will exit the gdb_expect
+	    # call (at the end of this loop), then next time around
+	    # these patterns (relating to the now closed inferior)
+	    # will not be included.
+	    if { ! $inferior_exited } {
+		append code {
+		    -i "$inferior_spawn_id"
+		    -re ".*Cannot exec.*Permission denied" {
+			set saw_perm_error 1
+			verbose -log "saw perm error"
+			if {!$saw_mi_error} {
+			    exp_continue
+			}
+		    }
+
+		    -i "$inferior_spawn_id"
+		    eof {
+			set inferior_exited true
+		    }
 		}
 	    }
-	    -i "$gdb_spawn_id"
-	    -re "\\^error,msg=\"(During startup program exited with code 127|Running .* on the remote target failed)" {
-		set saw_mi_error 1
-		verbose -log "saw mi error"
-		if {!$saw_perm_error} {
-		    exp_continue
+
+	    # These patterns all relate to the main gdb spawn-id, if
+	    # gdb exits then this is an error, so we don't need the
+	    # same "has spawn-id exited?" protection that we do for
+	    # the inferior spawn-id above.
+	    append code {
+		-i "$gdb_spawn_id"
+		-re "\\^error,msg=\"(During startup program exited with code 127|Running .* on the remote target failed)" {
+		    set saw_mi_error 1
+		    verbose -log "saw mi error"
+		    if {!$saw_perm_error} {
+			exp_continue
+		    }		}
+
+		-i "$gdb_main_spawn_id"
+		eof {
+		    set already_failed 1
+		    fail "$test (eof)"
+		}
+
+		timeout {
+		    set already_failed 1
+		    fail "$test (timeout)"
 		}
 	    }
-	    timeout {
-		set already_failed 1
-		fail "$test (timeout)"
-	    }
-	    -i "$gdb_main_spawn_id"
-	    eof {
-		set already_failed 1
-		fail "$test (eof)"
-	    }
+
+	    # Now we can call expect with the set of pattern/command blocks
+	    # we collected above.
+	    gdb_expect $code
 	}
 
 	if {$saw_perm_error && $saw_mi_error} {
-- 
2.25.4

[PATCH 2/2] gdb/testsuite: fix native-extended-gdbserver failure in mi-exec-run.exp

[Andrew Burgess]

When running with the native-extended-gdbserver board, I currently see
one failure in gdb.mi/mi-exec-run.exp:

  FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=0: breakpoint hit reported on console (timeout)

In this test run the MI interface should be started in a separate tty,
which means we should have a CLI tty and an MI tty, however, in this
case, this is not happening, and instead GDB is just started in MI
mode, and there is not CLI terminal.

The particular test above tries to switch to the CLI terminal and look
for some expected output, however, as there is no CLI terminal the
output never arrives, and the test fails.

It turns out that this is not a GDB problem at all, rather, this is an
issue with argument passing within the test script.

When mi_gdb_start is called in mi-exec-run.exp, we pass a list of
flags, however, by the time we get to default_mi_gdb_start the list of
flags has become a list containing a list of flags, and so, in
default_mi_gdb_start, when we iterate over the flags looking for first
"separate-mi-tty" and then "separate-inferior-tty" neither of these
conditions match, as what we actually have is a single string
"separate-mi-tty separate-inferior-tty".

To fix this I've changed mi_gdb_start and default_mi_gdb_start so the
argument to these functions is no longer called 'args' (which has the
special behaviour of capturing all remaining arguments into a list),
and instead call the argument 'flags' - which is now specified to be a
list of strings.

[ NOTE: Passing a single string in place of a list of strings is fine,
  so existing uses of mi_gdb_start will not be impacted. ]

Then I updated boards/native-extended-gdbserver.exp to use 'eval' so
that the '$args' is passed correctly in this case.

While auditing the uses of mi_gdb_start, I spotted that we passed an
empty string as an argument in mi-break-qualified.exp, I removed this
as it was not needed, and this makes the use consistent with similar
cases in the testsuite.

Now that mi_gdb_start takes a list, we no longer need to use eval in
mi-exec-run.exp when calling mi_gdb_start, which I think is nicer.

With this change in place I see no failures in gdb.mi/mi-exec-run.exp
when using the native-extended-gdbserver board.
---
 .../boards/native-extended-gdbserver.exp      |  2 +-
 gdb/testsuite/gdb.mi/mi-break-qualified.exp   |  2 +-
 gdb/testsuite/gdb.mi/mi-exec-run.exp          |  2 +-
 gdb/testsuite/lib/mi-support.exp              | 24 ++++++++++++-------
 4 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
index b925c364993..67acc6fea7c 100644
--- a/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -58,7 +58,7 @@ proc mi_gdb_start { args } {
     global gdbserver_reconnect_p
 
     # Spawn GDB.
-    set res [extended_gdbserver_mi_gdb_start $args]
+    set res [eval extended_gdbserver_mi_gdb_start $args]
     if { $res } {
 	return $res
     }
diff --git a/gdb/testsuite/gdb.mi/mi-break-qualified.exp b/gdb/testsuite/gdb.mi/mi-break-qualified.exp
index ff37950fd76..ca6df0782df 100644
--- a/gdb/testsuite/gdb.mi/mi-break-qualified.exp
+++ b/gdb/testsuite/gdb.mi/mi-break-qualified.exp
@@ -95,7 +95,7 @@ proc test_break_qualified {} {
 
 mi_gdb_exit
 
-if [mi_gdb_start ""] {
+if [mi_gdb_start] {
     return
 }
 
diff --git a/gdb/testsuite/gdb.mi/mi-exec-run.exp b/gdb/testsuite/gdb.mi/mi-exec-run.exp
index d736e19090a..4b5e71311fa 100644
--- a/gdb/testsuite/gdb.mi/mi-exec-run.exp
+++ b/gdb/testsuite/gdb.mi/mi-exec-run.exp
@@ -60,7 +60,7 @@ proc test {inftty_mode mi_mode force_fail} {
 	lappend start_ops "separate-mi-tty"
     }
 
-    if [eval mi_gdb_start $start_ops] {
+    if [mi_gdb_start $start_ops] {
 	return
     }
 
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index a231b8311b9..e578a7e6f9b 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -131,7 +131,13 @@ proc mi_create_inferior_pty {} {
     }
 }
 
-proc mi_gdb_start_separate_mi_tty { args } {
+#
+# Like default_mi_gdb_start below, but the MI is created as a separate
+# ui in a new tty.  The global MI_SPAWN_ID is updated to point at the
+# new tty created for the MI interface.  The global GDB_MAIN_SPAWN_ID
+# is updated to the current value of the global GDB_SPAWN_ID.
+#
+proc mi_gdb_start_separate_mi_tty { { flags {} } } {
     global gdb_prompt mi_gdb_prompt
     global timeout
     global gdb_spawn_id gdb_main_spawn_id mi_spawn_id
@@ -139,8 +145,8 @@ proc mi_gdb_start_separate_mi_tty { args } {
 
     set separate_inferior_pty 0
 
-    foreach arg $args {
-	if {$arg == "separate-inferior-tty"} {
+    foreach flag $flags {
+	if {$flag == "separate-inferior-tty"} {
 	    set separate_inferior_pty 1
 	}
     }
@@ -183,6 +189,8 @@ proc mi_gdb_start_separate_mi_tty { args } {
 #
 # default_mi_gdb_start [FLAGS] -- start gdb running, default procedure
 #
+# FLAGS is a list of flags, each flag is a string.
+#
 # If "separate-inferior-tty" is specified, the inferior works with
 # it's own PTY.
 #
@@ -193,7 +201,7 @@ proc mi_gdb_start_separate_mi_tty { args } {
 # tests on different hosts all using the same server, things can
 # get really slow.  Give gdb at least 3 minutes to start up.
 #
-proc default_mi_gdb_start { args } {
+proc default_mi_gdb_start { { flags {} } } {
     global use_gdb_stub
     global GDB
     global INTERNAL_GDBFLAGS GDBFLAGS
@@ -218,16 +226,16 @@ proc default_mi_gdb_start { args } {
 
     set separate_inferior_pty 0
 
-    foreach arg $args {
-	if {$arg == "separate-mi-tty"} {
+    foreach flag $flags {
+	if {$flag == "separate-mi-tty"} {
 	    set separate_mi_pty 1
-	} elseif {$arg == "separate-inferior-tty"} {
+	} elseif {$flag == "separate-inferior-tty"} {
 	    set separate_inferior_pty 1
 	}
     }
 
     if {$separate_mi_pty} {
-	return [eval mi_gdb_start_separate_mi_tty $args]
+	return [mi_gdb_start_separate_mi_tty $flags]
     }
 
     set inferior_pty no-tty
-- 
2.25.4

Re: [PATCH 1/2] gdb: fix failures in gdb.mi/mi-exec-run.exp with native-extended-gdbserver

[Pedro Alves]

On 2022-04-27 19:17, Andrew Burgess via Gdb-patches wrote:
> When running the gdb.mi/mi-exec-run.exp test using the
> native-extended-gdbserver I see failures like this:
> 
>   FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=main: mi=main: force-fail=1: run failure detected
>   FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=main: mi=separate: force-fail=1: run failure detected
>   FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=1: run failure detected
> 
> There's a race condition here, so you might see a slightly different
> set of failures, but I always see some from the 'run failure detected'
> test.
> 
> NOTE: I also see an additional test failure:
> 
>  FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=0: breakpoint hit reported on console (timeout)
> 
> but that is a completely different issue, and is not being addressed
> in this commit.
> 
> The problem for the 'run failure detected' test is that we end up
> in gdb_expect looking for output from two spawn-ids, one from
> gdbserver, and one from gdb.  We're looking for one output pattern
> from each spawn-id, and for the test to pass we need to see both
> patterns.
> 
> Now, if gdb exits then this is a test failure (this would indicate gdb
> crashing, which is bad), so we have the eof pattern associated with
> the gdb spawn-id.
> 
> However, in this particular test we expect gdbserver to fail to
> execute the binary (the test binary is set non-executable), and so we
> get an error message from gdbserver (which matches the pattern), and
> then gdbserver exits, this is expected.
> 
> In our gdb_expect call we don't have an eof pattern for gdbserver, and
> so, when gdbserver exits, expect sees eof and drops out of the
> gdb_expect call.
> 
> If both the gdbserver AND gdb patterns are seen before the gdbserver
> eof triggers then everything is fine.
> 
> But, if expect sees gdbserver's eof before seeing the output from gdb
> then we will exit the gdb_expect call, and (as we have not seen both
> patterns) we will call 'fail'.
> 
> I initially tried to fix this by adding a new eof pattern for the
> gdbserver spawn-id that called exp_continue, however, this causes
> expect to throw an error, it is not happy to have '-i' options that
> make use of a closed spawn-id.
> 

I suspect this will look simpler if you use an indirect spawd id.  Did you try that?

From the expect man page:

  "The -i flag may also name a global variable in which case the variable is read for a
   list of spawn ids. The variable is reread whenever it changes. This provides a way of
   changing the I/O source while the command is in execution. Spawn ids provided this way
   are called "indirect" spawn ids."

Some other testcases use this technique to handle scenarios similar to what you
describe, of not wanting to expect output out of a spawn_id anymore.
E.g., gdb.base/interrupt.exp has:

        set spawn_list "$inferior_spawn_id"

        gdb_test_multiple "" $msg {
            -i spawn_list -re "end of file" {
                set saw_end_of_file 1
                verbose -log "saw \"end of file\""
                if {!$saw_inferior_exit} {
                    # When $inferior_spawn_id != $gdb_spawn_id, such
                    # as when testing with gdbserver, we may see the
                    # eof (the process exit, not the string just
                    # matched) for $inferior_spawn_id before the
                    # expected gdb output.  Clear this so we no longer
                    # expect anything out of $inferior_spawn_id.
                    set spawn_list ""
                    exp_continue
                }
            }
       [...]

[PATCHv2 0/2] Fix mi-exec-run.exp with native-extended-gdbserver

[Andrew Burgess]

In v2:

  - Rewrote patch #1 in line with Pedro's suggestion - much smaller
    change now.

  - Rebased onto current upstream/master.

---

Andrew Burgess (2):
  gdb: fix failures in gdb.mi/mi-exec-run.exp with
    native-extended-gdbserver
  gdb/testsuite: fix native-extended-gdbserver failure in
    mi-exec-run.exp

 .../boards/native-extended-gdbserver.exp      |  2 +-
 gdb/testsuite/gdb.mi/mi-break-qualified.exp   |  2 +-
 gdb/testsuite/gdb.mi/mi-exec-run.exp          | 21 ++++++++++++++--
 gdb/testsuite/lib/mi-support.exp              | 24 ++++++++++++-------
 4 files changed, 37 insertions(+), 12 deletions(-)

-- 
2.25.4

[PATCHv2 1/2] gdb: fix failures in gdb.mi/mi-exec-run.exp with native-extended-gdbserver

[Andrew Burgess]

When running the gdb.mi/mi-exec-run.exp test using the
native-extended-gdbserver I see failures like this:

  FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=main: mi=main: force-fail=1: run failure detected
  FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=main: mi=separate: force-fail=1: run failure detected
  FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=1: run failure detected

There's a race condition here, so you might see a slightly different
set of failures, but I always see some from the 'run failure detected'
test.

NOTE: I also see an additional test failure:

 FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=0: breakpoint hit reported on console (timeout)

but that is a completely different issue, and is not being addressed
in this commit.

The problem for the 'run failure detected' test is that we end up
in gdb_expect looking for output from two spawn-ids, one from
gdbserver, and one from gdb.  We're looking for one output pattern
from each spawn-id, and for the test to pass we need to see both
patterns.

Now, if gdb exits then this is a test failure (this would indicate gdb
crashing, which is bad), so we have an eof pattern associated with
the gdb spawn-id.

However, in this particular test we expect gdbserver to fail to
execute the binary (the test binary is set non-executable), and so we
get an error message from gdbserver (which matches the pattern), and
then gdbserver exits, this is expected.

The problem is that after spotting the pattern from gdbserver, we
often see the eof from gdbserver before we see the pattern from gdb.
If this happens then we drop out of the gdb_expect without ever seeing
the pattern from gdb, and fail the test.

In this commit, I place the spawn-id of gdbserver into a global
variable, and then use this global variable as the -i option within
the gdb_expect.

Now, once we have seen the expected pattern on the gdbserver spawn-id,
the global variable is cleared.  After this the gdb_expect no longer
checks the gdbserver spawn-id for additional output, and so never sees
the eof event.  This leaves the gdb_expect running, which allows the
pattern from gdb to be seen, and for the test to pass.

I now see no failures relating to 'run failure detected'.
---
 gdb/testsuite/gdb.mi/mi-exec-run.exp | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.mi/mi-exec-run.exp b/gdb/testsuite/gdb.mi/mi-exec-run.exp
index f397159078f..ffbe6bcd36e 100644
--- a/gdb/testsuite/gdb.mi/mi-exec-run.exp
+++ b/gdb/testsuite/gdb.mi/mi-exec-run.exp
@@ -93,10 +93,27 @@ proc test {inftty_mode mi_mode force_fail} {
 	set test "run failure detected"
 	send_gdb "-exec-run --start\n"
 
+	# Redirect through SPAWN_LIST global.  If the
+	# inferior_spawn_id is not the same as gdb_spawn_id, e.g. when
+	# testing with gdbserver, the gdbserver can exit after
+	# emitting it's error message.
+	#
+	# If inferior_spawn_id exits then we may see the eof from that
+	# spawn-id before we see the pattern from the gdb_spawn_id,
+	# which will kick us out of the gdb_expect, and cause us to
+	# fail the test.
+	#
+	# Instead we clean SPAWN_LIST once we've seen the expected
+	# pattern from that spawn-id, and after that we no longer care
+	# when gdbserver exits.
+	global spawn_list
+	set spawn_list "$inferior_spawn_id"
+
 	gdb_expect {
-	    -i "$inferior_spawn_id"
+	    -i spawn_list
 	    -re ".*Cannot exec.*Permission denied" {
 		set saw_perm_error 1
+		set spawn_list ""
 		verbose -log "saw perm error"
 		if {!$saw_mi_error} {
 		    exp_continue
-- 
2.25.4

[PATCHv2 2/2] gdb/testsuite: fix native-extended-gdbserver failure in mi-exec-run.exp

[Andrew Burgess]

When running with the native-extended-gdbserver board, I currently see
one failure in gdb.mi/mi-exec-run.exp:

  FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=0: breakpoint hit reported on console (timeout)

In this test run the MI interface should be started in a separate tty,
which means we should have a CLI tty and an MI tty, however, in this
case, this is not happening, and instead GDB is just started in MI
mode, and there is not CLI terminal.

The particular test above tries to switch to the CLI terminal and look
for some expected output, however, as there is no CLI terminal the
output never arrives, and the test fails.

It turns out that this is not a GDB problem at all, rather, this is an
issue with argument passing within the test script.

When mi_gdb_start is called in mi-exec-run.exp, we pass a list of
flags, however, by the time we get to default_mi_gdb_start the list of
flags has become a list containing a list of flags, and so, in
default_mi_gdb_start, when we iterate over the flags looking for first
"separate-mi-tty" and then "separate-inferior-tty" neither of these
conditions match, as what we actually have is a single string
"separate-mi-tty separate-inferior-tty".

To fix this I've changed mi_gdb_start and default_mi_gdb_start so the
argument to these functions is no longer called 'args' (which has the
special behaviour of capturing all remaining arguments into a list),
and instead call the argument 'flags' - which is now specified to be a
list of strings.

[ NOTE: Passing a single string in place of a list of strings is fine,
  so existing uses of mi_gdb_start will not be impacted. ]

Then I updated boards/native-extended-gdbserver.exp to use 'eval' so
that the '$args' is passed correctly in this case.

While auditing the uses of mi_gdb_start, I spotted that we passed an
empty string as an argument in mi-break-qualified.exp, I removed this
as it was not needed, and this makes the use consistent with similar
cases in the testsuite.

Now that mi_gdb_start takes a list, we no longer need to use eval in
mi-exec-run.exp when calling mi_gdb_start, which I think is nicer.

With this change in place I see no failures in gdb.mi/mi-exec-run.exp
when using the native-extended-gdbserver board.
---
 .../boards/native-extended-gdbserver.exp      |  2 +-
 gdb/testsuite/gdb.mi/mi-break-qualified.exp   |  2 +-
 gdb/testsuite/gdb.mi/mi-exec-run.exp          |  2 +-
 gdb/testsuite/lib/mi-support.exp              | 24 ++++++++++++-------
 4 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
index b925c364993..67acc6fea7c 100644
--- a/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -58,7 +58,7 @@ proc mi_gdb_start { args } {
     global gdbserver_reconnect_p
 
     # Spawn GDB.
-    set res [extended_gdbserver_mi_gdb_start $args]
+    set res [eval extended_gdbserver_mi_gdb_start $args]
     if { $res } {
 	return $res
     }
diff --git a/gdb/testsuite/gdb.mi/mi-break-qualified.exp b/gdb/testsuite/gdb.mi/mi-break-qualified.exp
index ff37950fd76..ca6df0782df 100644
--- a/gdb/testsuite/gdb.mi/mi-break-qualified.exp
+++ b/gdb/testsuite/gdb.mi/mi-break-qualified.exp
@@ -95,7 +95,7 @@ proc test_break_qualified {} {
 
 mi_gdb_exit
 
-if [mi_gdb_start ""] {
+if [mi_gdb_start] {
     return
 }
 
diff --git a/gdb/testsuite/gdb.mi/mi-exec-run.exp b/gdb/testsuite/gdb.mi/mi-exec-run.exp
index ffbe6bcd36e..f8e6550850f 100644
--- a/gdb/testsuite/gdb.mi/mi-exec-run.exp
+++ b/gdb/testsuite/gdb.mi/mi-exec-run.exp
@@ -60,7 +60,7 @@ proc test {inftty_mode mi_mode force_fail} {
 	lappend start_ops "separate-mi-tty"
     }
 
-    if [eval mi_gdb_start $start_ops] {
+    if [mi_gdb_start $start_ops] {
 	return
     }
 
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index a231b8311b9..e578a7e6f9b 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -131,7 +131,13 @@ proc mi_create_inferior_pty {} {
     }
 }
 
-proc mi_gdb_start_separate_mi_tty { args } {
+#
+# Like default_mi_gdb_start below, but the MI is created as a separate
+# ui in a new tty.  The global MI_SPAWN_ID is updated to point at the
+# new tty created for the MI interface.  The global GDB_MAIN_SPAWN_ID
+# is updated to the current value of the global GDB_SPAWN_ID.
+#
+proc mi_gdb_start_separate_mi_tty { { flags {} } } {
     global gdb_prompt mi_gdb_prompt
     global timeout
     global gdb_spawn_id gdb_main_spawn_id mi_spawn_id
@@ -139,8 +145,8 @@ proc mi_gdb_start_separate_mi_tty { args } {
 
     set separate_inferior_pty 0
 
-    foreach arg $args {
-	if {$arg == "separate-inferior-tty"} {
+    foreach flag $flags {
+	if {$flag == "separate-inferior-tty"} {
 	    set separate_inferior_pty 1
 	}
     }
@@ -183,6 +189,8 @@ proc mi_gdb_start_separate_mi_tty { args } {
 #
 # default_mi_gdb_start [FLAGS] -- start gdb running, default procedure
 #
+# FLAGS is a list of flags, each flag is a string.
+#
 # If "separate-inferior-tty" is specified, the inferior works with
 # it's own PTY.
 #
@@ -193,7 +201,7 @@ proc mi_gdb_start_separate_mi_tty { args } {
 # tests on different hosts all using the same server, things can
 # get really slow.  Give gdb at least 3 minutes to start up.
 #
-proc default_mi_gdb_start { args } {
+proc default_mi_gdb_start { { flags {} } } {
     global use_gdb_stub
     global GDB
     global INTERNAL_GDBFLAGS GDBFLAGS
@@ -218,16 +226,16 @@ proc default_mi_gdb_start { args } {
 
     set separate_inferior_pty 0
 
-    foreach arg $args {
-	if {$arg == "separate-mi-tty"} {
+    foreach flag $flags {
+	if {$flag == "separate-mi-tty"} {
 	    set separate_mi_pty 1
-	} elseif {$arg == "separate-inferior-tty"} {
+	} elseif {$flag == "separate-inferior-tty"} {
 	    set separate_inferior_pty 1
 	}
     }
 
     if {$separate_mi_pty} {
-	return [eval mi_gdb_start_separate_mi_tty $args]
+	return [mi_gdb_start_separate_mi_tty $flags]
     }
 
     set inferior_pty no-tty
-- 
2.25.4

Re: [PATCHv2 1/2] gdb: fix failures in gdb.mi/mi-exec-run.exp with native-extended-gdbserver

[Pedro Alves]

On 2022-04-28 12:27, Andrew Burgess via Gdb-patches wrote:
> When running the gdb.mi/mi-exec-run.exp test using the
> native-extended-gdbserver I see failures like this:
> 
>   FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=main: mi=main: force-fail=1: run failure detected
>   FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=main: mi=separate: force-fail=1: run failure detected
>   FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=1: run failure detected
> 
> There's a race condition here, so you might see a slightly different
> set of failures, but I always see some from the 'run failure detected'
> test.
> 
> NOTE: I also see an additional test failure:
> 
>  FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=0: breakpoint hit reported on console (timeout)
> 
> but that is a completely different issue, and is not being addressed
> in this commit.
> 
> The problem for the 'run failure detected' test is that we end up
> in gdb_expect looking for output from two spawn-ids, one from
> gdbserver, and one from gdb.  We're looking for one output pattern
> from each spawn-id, and for the test to pass we need to see both
> patterns.
> 
> Now, if gdb exits then this is a test failure (this would indicate gdb
> crashing, which is bad), so we have an eof pattern associated with
> the gdb spawn-id.
> 
> However, in this particular test we expect gdbserver to fail to
> execute the binary (the test binary is set non-executable), and so we
> get an error message from gdbserver (which matches the pattern), and
> then gdbserver exits, this is expected.
> 
> The problem is that after spotting the pattern from gdbserver, we
> often see the eof from gdbserver before we see the pattern from gdb.
> If this happens then we drop out of the gdb_expect without ever seeing
> the pattern from gdb, and fail the test.
> 
> In this commit, I place the spawn-id of gdbserver into a global
> variable, and then use this global variable as the -i option within
> the gdb_expect.
> 
> Now, once we have seen the expected pattern on the gdbserver spawn-id,
> the global variable is cleared.  After this the gdb_expect no longer
> checks the gdbserver spawn-id for additional output, and so never sees
> the eof event.  This leaves the gdb_expect running, which allows the
> pattern from gdb to be seen, and for the test to pass.
> 
> I now see no failures relating to 'run failure detected'.
> ---
>  gdb/testsuite/gdb.mi/mi-exec-run.exp | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)

OK.

Re: [PATCHv2 2/2] gdb/testsuite: fix native-extended-gdbserver failure in mi-exec-run.exp

[Pedro Alves]

On 2022-04-28 12:27, Andrew Burgess via Gdb-patches wrote:
> When running with the native-extended-gdbserver board, I currently see
> one failure in gdb.mi/mi-exec-run.exp:
> 
>   FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=0: breakpoint hit reported on console (timeout)
> 
> In this test run the MI interface should be started in a separate tty,
> which means we should have a CLI tty and an MI tty, however, in this
> case, this is not happening, and instead GDB is just started in MI
> mode, and there is not CLI terminal.
> 
> The particular test above tries to switch to the CLI terminal and look
> for some expected output, however, as there is no CLI terminal the
> output never arrives, and the test fails.
> 
> It turns out that this is not a GDB problem at all, rather, this is an
> issue with argument passing within the test script.
> 
> When mi_gdb_start is called in mi-exec-run.exp, we pass a list of
> flags, however, by the time we get to default_mi_gdb_start the list of
> flags has become a list containing a list of flags, and so, in
> default_mi_gdb_start, when we iterate over the flags looking for first
> "separate-mi-tty" and then "separate-inferior-tty" neither of these
> conditions match, as what we actually have is a single string
> "separate-mi-tty separate-inferior-tty".

So that actual bug is in native-extended-gdbserver.exp:mi_gdb_start,
where we pass down $args without using eval or "{*}$args".

Like, this fixes it for me:

diff --git c/gdb/testsuite/boards/native-extended-gdbserver.exp w/gdb/testsuite/boards/native-extended-gdbserver.exp
index b925c364993..67acc6fea7c 100644
--- c/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ w/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -58,7 +58,7 @@ proc mi_gdb_start { args } {
     global gdbserver_reconnect_p
 
     # Spawn GDB.
-    set res [extended_gdbserver_mi_gdb_start $args]
+    set res [eval extended_gdbserver_mi_gdb_start $args]
     if { $res } {
        return $res
     }


Or this:

diff --git c/gdb/testsuite/boards/native-extended-gdbserver.exp w/gdb/testsuite/boards/native-extended-gdbserver.exp
index b925c364993..2a79429f4eb 100644
--- c/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ w/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -58,7 +58,7 @@ proc mi_gdb_start { args } {
     global gdbserver_reconnect_p
 
     # Spawn GDB.
-    set res [extended_gdbserver_mi_gdb_start $args]
+    set res [extended_gdbserver_mi_gdb_start {*}$args]
     if { $res } {
        return $res
     }
 
The rest of the fix is a "nice to have".  I'm OK with it, but it could have
been a separate cleanup patch, since the change to
native-extended-gdbserver.exp:mi_gdb_start is the same regardless.

> 
> To fix this I've changed mi_gdb_start and default_mi_gdb_start so the
> argument to these functions is no longer called 'args' (which has the
> special behaviour of capturing all remaining arguments into a list),
> and instead call the argument 'flags' - which is now specified to be a
> list of strings.
> 
> [ NOTE: Passing a single string in place of a list of strings is fine,
>   so existing uses of mi_gdb_start will not be impacted. ]
> 
> Then I updated boards/native-extended-gdbserver.exp to use 'eval' so
> that the '$args' is passed correctly in this case.
> 

Right, that was the bug, AFAICS.

> While auditing the uses of mi_gdb_start, I spotted that we passed an
> empty string as an argument in mi-break-qualified.exp, I removed this
> as it was not needed, and this makes the use consistent with similar
> cases in the testsuite.
> 
> Now that mi_gdb_start takes a list, we no longer need to use eval in
> mi-exec-run.exp when calling mi_gdb_start, which I think is nicer.
> 
> With this change in place I see no failures in gdb.mi/mi-exec-run.exp
> when using the native-extended-gdbserver board.

This is fine.

[PATCHv3 0/3] gdb/testsuite: fix mi-exec-run.exp

[Andrew Burgess]

Pedro,

Thanks for your feedback.  I've split the patch into three parts.  I'm
happy to drop the API change (patch #2) if you feel it is not a good
idea.

How's this?

Thanks,
Andrew

---

Andrew Burgess (3):
  gdb/testsuite: fix mi-exec-run.exp with native-extended-gdbserver
    board
  gdb/testsuite: change mi_gdb_start to take a list of flags
  gdb/testsuite: small cleanup in mi-break-qualified.exp

 .../boards/native-extended-gdbserver.exp      |  2 +-
 gdb/testsuite/gdb.mi/mi-break-qualified.exp   |  2 +-
 gdb/testsuite/gdb.mi/mi-exec-run.exp          |  2 +-
 gdb/testsuite/lib/mi-support.exp              | 24 ++++++++++++-------
 4 files changed, 19 insertions(+), 11 deletions(-)

-- 
2.25.4

[PATCHv3 1/3] gdb/testsuite: fix mi-exec-run.exp with native-extended-gdbserver board

[Andrew Burgess]

When running with the native-extended-gdbserver board, I currently see
one failure in gdb.mi/mi-exec-run.exp:

    FAIL: gdb.mi/mi-exec-run.exp: inferior-tty=separate: mi=separate: force-fail=0: breakpoint hit reported on console (timeout)

In this test the MI interface should be started in a separate tty,
which means we should have a CLI tty and an MI tty, however, this is
not happening.  Instead GDB is just started in MI mode and there is no
CLI tty.

The test script tries to switch between the CLI an MI terminals and
look for some expected output on each, however, as there is no CLI
terminal the expected output never arrives, and the test times out.

It turns out that this is not a GDB problem, rather, this is an issue
with argument passing within the test script.

The proc default_mi_gdb_start expects to take a set of flags (strings)
as arguments, each of flag is expected to be a separate argument.  The
default_mi_gdb_start proc collects all its arguments into a list using
the special 'args' parameter name, and then iterates over this list to
see which flags were passed.

In mi_gdb_start, which forwards to default_mi_gdb_start, the arguments
are also gathered into the 'args' parameter list, but are then
expanded back to be separate arguments using the eval trick, i.e.:

  proc mi_gdb_start { args } {
    return [eval default_mi_gdb_start $args]
  }

This ensures that when we arrive in default_mi_gdb_start each flag is
a separate argument, rather than appearing as a single list containing
all arguments.

When using the native-extended-gdbserver board however, the file
boards/native-extended-gdbserver.exp is loaded, and this file replaces
the default mi_gdb_start with its own version.

This new mi_gdb_start also gathers the arguments into an 'args' list,
but forgets to expand the arguments out using the eval trick.

As a result, when using the native-extended-gdbserver board, by the
time we get to default_mi_gdb_start, we end up with the args list
containing a single item, which is a list containing all the arguments
the user passed.

What this means is that if the user passes two arguments, then, in
default_mi_gdb_start, instead of seeing two separate arguments, we see
a single argument made by concatenating the two arguments together.

The only place this is a problem is in the test mi-exec-run.exp,
which (as far as I can see) is the only test where we might try to
pass both arguments at the same time.  Currently we think we passed
both arguments to mi_gdb_start, but mi_gdb_start behaves as if no
arguments were passed.

This commit fixes the problem by making use of the eval trick within
the native-extended-gdbserver version of mi_gdb_start.  After this,
the FAIL listed at the top of this message is resolved.
---
 gdb/testsuite/boards/native-extended-gdbserver.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
index b925c364993..67acc6fea7c 100644
--- a/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -58,7 +58,7 @@ proc mi_gdb_start { args } {
     global gdbserver_reconnect_p
 
     # Spawn GDB.
-    set res [extended_gdbserver_mi_gdb_start $args]
+    set res [eval extended_gdbserver_mi_gdb_start $args]
     if { $res } {
 	return $res
     }
-- 
2.25.4

[PATCHv3 2/3] gdb/testsuite: change mi_gdb_start to take a list of flags

[Andrew Burgess]

After this previous commit I was thinking about the API of
mi_gdb_start.  I felt that the idea of passing flags as separate
arguments and using 'args' to gather these into a list, though clever,
was not an intuitive API.

In this commit I modify mi_gdb_start so that it expects a single
argument, which should be a list of flags.  Thus, where we previously
would have said:

  mi_gdb_start separate-mi-tty separate-inferior-tty

We would now say:

  mi_gdb_start { separate-mi-tty separate-inferior-tty }

However, it turns out we never actually call mi_gdb_start passing two
arguments in this way at all.  We do in some places do this:

  mi_gdb_start separate-inferior-tty

But that's fine, a single string like this works equally well as a
single item list, so this will not need updating.

There is also one place where we do this:

  eval mi_gdb_start $start_ops

where $start_ops is a list that might contains 0, 1, or 2 items.  The
eval here is used to expand the $start_ops list so mi_gdb_start sees
the list contents as separate arguments.  In this case we just need to
drop the use of eval.

I think that the new API is more intuitive, but others might
disagree, in which case I can drop this change.

There should be no change in what is tested after this commit.
---
 gdb/testsuite/gdb.mi/mi-exec-run.exp |  2 +-
 gdb/testsuite/lib/mi-support.exp     | 24 ++++++++++++++++--------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-exec-run.exp b/gdb/testsuite/gdb.mi/mi-exec-run.exp
index ffbe6bcd36e..f8e6550850f 100644
--- a/gdb/testsuite/gdb.mi/mi-exec-run.exp
+++ b/gdb/testsuite/gdb.mi/mi-exec-run.exp
@@ -60,7 +60,7 @@ proc test {inftty_mode mi_mode force_fail} {
 	lappend start_ops "separate-mi-tty"
     }
 
-    if [eval mi_gdb_start $start_ops] {
+    if [mi_gdb_start $start_ops] {
 	return
     }
 
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index a231b8311b9..e578a7e6f9b 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -131,7 +131,13 @@ proc mi_create_inferior_pty {} {
     }
 }
 
-proc mi_gdb_start_separate_mi_tty { args } {
+#
+# Like default_mi_gdb_start below, but the MI is created as a separate
+# ui in a new tty.  The global MI_SPAWN_ID is updated to point at the
+# new tty created for the MI interface.  The global GDB_MAIN_SPAWN_ID
+# is updated to the current value of the global GDB_SPAWN_ID.
+#
+proc mi_gdb_start_separate_mi_tty { { flags {} } } {
     global gdb_prompt mi_gdb_prompt
     global timeout
     global gdb_spawn_id gdb_main_spawn_id mi_spawn_id
@@ -139,8 +145,8 @@ proc mi_gdb_start_separate_mi_tty { args } {
 
     set separate_inferior_pty 0
 
-    foreach arg $args {
-	if {$arg == "separate-inferior-tty"} {
+    foreach flag $flags {
+	if {$flag == "separate-inferior-tty"} {
 	    set separate_inferior_pty 1
 	}
     }
@@ -183,6 +189,8 @@ proc mi_gdb_start_separate_mi_tty { args } {
 #
 # default_mi_gdb_start [FLAGS] -- start gdb running, default procedure
 #
+# FLAGS is a list of flags, each flag is a string.
+#
 # If "separate-inferior-tty" is specified, the inferior works with
 # it's own PTY.
 #
@@ -193,7 +201,7 @@ proc mi_gdb_start_separate_mi_tty { args } {
 # tests on different hosts all using the same server, things can
 # get really slow.  Give gdb at least 3 minutes to start up.
 #
-proc default_mi_gdb_start { args } {
+proc default_mi_gdb_start { { flags {} } } {
     global use_gdb_stub
     global GDB
     global INTERNAL_GDBFLAGS GDBFLAGS
@@ -218,16 +226,16 @@ proc default_mi_gdb_start { args } {
 
     set separate_inferior_pty 0
 
-    foreach arg $args {
-	if {$arg == "separate-mi-tty"} {
+    foreach flag $flags {
+	if {$flag == "separate-mi-tty"} {
 	    set separate_mi_pty 1
-	} elseif {$arg == "separate-inferior-tty"} {
+	} elseif {$flag == "separate-inferior-tty"} {
 	    set separate_inferior_pty 1
 	}
     }
 
     if {$separate_mi_pty} {
-	return [eval mi_gdb_start_separate_mi_tty $args]
+	return [mi_gdb_start_separate_mi_tty $flags]
     }
 
     set inferior_pty no-tty
-- 
2.25.4

[PATCHv3 3/3] gdb/testsuite: small cleanup in mi-break-qualified.exp

[Andrew Burgess]

It is not necessary to pass an empty string to mi_gdb_start, passing
the empty string is equivalent to passing no arguments, which is what
we do everywhere else (that we don't need to specify an actual
argument).

The only place we use 'mi_gdb_start ""' is in
gdb.mi/mi-break-qualified.exp, so in this commit I just replace that
with a call to 'mi_gdb_start' - just for consistency.

There should be no change in what is tested after this commit.
---
 gdb/testsuite/gdb.mi/mi-break-qualified.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.mi/mi-break-qualified.exp b/gdb/testsuite/gdb.mi/mi-break-qualified.exp
index ff37950fd76..ca6df0782df 100644
--- a/gdb/testsuite/gdb.mi/mi-break-qualified.exp
+++ b/gdb/testsuite/gdb.mi/mi-break-qualified.exp
@@ -95,7 +95,7 @@ proc test_break_qualified {} {
 
 mi_gdb_exit
 
-if [mi_gdb_start ""] {
+if [mi_gdb_start] {
     return
 }
 
-- 
2.25.4

Re: [PATCHv3 0/3] gdb/testsuite: fix mi-exec-run.exp

[Pedro Alves]

On 2022-04-29 19:15, Andrew Burgess via Gdb-patches wrote:
> Pedro,
> 
> Thanks for your feedback.  I've split the patch into three parts.  I'm
> happy to drop the API change (patch #2) if you feel it is not a good
> idea.
> 
> How's this?

Thanks, LGTM.  I don't have strong opinions on patch #2.  It's fine with me.

Re: [PATCHv3 0/3] gdb/testsuite: fix mi-exec-run.exp

[Andrew Burgess]

Pedro Alves <pedro@palves.net> writes:

> On 2022-04-29 19:15, Andrew Burgess via Gdb-patches wrote:
>> Pedro,
>> 
>> Thanks for your feedback.  I've split the patch into three parts.  I'm
>> happy to drop the API change (patch #2) if you feel it is not a good
>> idea.
>> 
>> How's this?
>
> Thanks, LGTM.  I don't have strong opinions on patch #2.  It's fine with me.

Thanks, I pushed this series.

Andrew