Re: [PATCH] Improve MI inferior output check / mi-console.exp

[Luis Machado <lgustavo at codesourcery dot com>]

On 11/01/2013 10:06 AM, Pedro Alves wrote:
> On 10/30/2013 12:30 PM, Luis Machado wrote:
>
> > The following patch accomplishes that and tweaks things a little further.
> >
> > First, i removed the misleading test description. It does not seem to be
> > aligned with what the test does nowadays. It is not restricted to
> > remote/sim targets anymore.
> >
> > I've put back the mi-support.exp bit since that is required for remote
> > targets, otherwise we will try to read from the spawn id.
> >
> > I'm explicitly checking for "remote" and "extended-remote" gdb_protocol
> > values here since neither "isremote" nor "use_gdb_stub" seem to have a
> > consistent meaning.
>
> > Extended-remote should be both "use_gdb_stub" and  "isremote", but it isn't.
>
> No it shouldn't.  ;-)
>
> > Hopefully this version also addresses the gdb-simulator vs rsp-simulator
> > confusion.
>
> I don't think it does completely yet?  A check for the
> sim is missing.  We'd need:
>
>      [target_info protocol] == "sim"
>
> (see mi_gdb_target_load)

Done.

> And, try:
>
> $ grep noinferiorio /usr/share/dejagnu/* -rn
>
> Where we find lots of random non-remote boards that
> set gdb,noinferiorio 1.
>
> And the patch makes all such boards fall to the
>
> > +	    } else {
> >   		global mi_inferior_spawn_id
>
> bit.  Shouldn't the patch be leaving the
>
> 	    if { ![target_info exists gdb,noinferiorio] } {
>
> check as the outermost?

I agree, that's more appropriate. I pondered about reversing the double 
negation (not noinferiorio?), but i've left it there.

> > +# The program's real output string.
> > +set program_output "Hello \\\"!\r\n"
> > +
> > +# Prepare the pattern for the PTY output of a native target.
> > +set output [string map {"\r\n" "\[\r\n\]+"} $program_output]
> > +set output [string map {"\\" "\\\\"} $output]
> > +
> > +# Prepare the pattern for the semihosted output.
> > +set semihosted_output [semihosted_string $program_output]
> > +
> > +# Combine both outputs in a single pattern.
> > +set both_patterns "($semihosted_output|$output)"
>
> BTW, I'd find that naming the variables like:
>
> # The program's real output string.
> set program_output "Hello \\\"!\r\n"
>
> # Prepare the pattern for the PTY output of a native target.
> set native_output [string map {"\r\n" "\[\r\n\]+"} $program_output]
> set native_output [string map {"\\" "\\\\"} $native_output]
>
> # Prepare the pattern for the semihosted output.
> set semihosted_output [semihosted_string $program_output]
>
> # Combine both outputs in a single pattern.
> set output "($semihosted_output|$native_output)"
>
> to be a bit clearer.

Changed as well.

How does the following patch look?

Thanks,
Luis

2013-11-04  Luis Machado  <lgustavo@codesourcery.com>

	* lib/mi-support.exp (mi_gdb_test): Expect different formats
	of inferior output for remote and native sessions.
	* gdb.mi/mi-console.exp: Remove obsolete comment.
	Check for semihosted inferior output pattern.
	(semihosted_string): New function.

diff --git a/gdb/testsuite/gdb.mi/mi-console.exp b/gdb/testsuite/gdb.mi/mi-console.exp
index b7643e7..47b0677 100644
--- a/gdb/testsuite/gdb.mi/mi-console.exp
+++ b/gdb/testsuite/gdb.mi/mi-console.exp
@@ -14,14 +14,38 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 # Test inferior console output, with MI.
-#
-# This test only works when talking to a target that routes its output
-# through GDB.  Check that we're either talking to a simulator or a
-# remote target.
 
 load_lib mi-support.exp
 set MIFLAGS "-i=mi"
 
+#
+# Given STRING, return the semihosted version of that string.
+#
+proc semihosted_string { string } {
+    set semihosted_list {}
+    set leading_markers "@\""
+    set trailing_markers "\"\r\n"
+
+    if {$string != "" } {
+	set split_string [split $string ""]
+
+	foreach char $split_string {
+	    # Escape special characters.
+	    if {$char == "\\"} {
+		set char "\\\\\\\\"
+	    } elseif {$char == "\r"} {
+		  set char "\\\\r"
+	    } elseif {$char == "\n"} {
+		  set char "\\\\n"
+	    } elseif {$char == "\""} {
+		  set char "\\\\\""
+	    }
+	    lappend semihosted_list $leading_markers $char $trailing_markers
+	}
+    }
+  return [join $semihosted_list ""]
+}
+
 gdb_exit
 if [mi_gdb_start separate-inferior-tty] {
     continue
@@ -36,11 +60,34 @@ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {deb
 
 mi_run_to_main
 
+# The output we get from the target depends on how it is hosted.  If
+# we are semihosted (e.g., the sim or a remote target that supports
+# the File I/O remote protocol extension), we see the target I/O
+# encapsulated in MI target output stream records.  If debugging with
+# a native target, the inferior's I/O streams are connected directly
+# to a PTY we create for the inferior (notice separate-inferior-tty
+# above), and we just see the program's output unadorned.  If
+# debugging with a remote target that doesn't support semihosting,
+# we'll see nothing.
+
+# The program's real output string.
+set program_output "Hello \\\"!\r\n"
+
+# Prepare the pattern for the PTY output of a native target.
+set native_output [string map {"\r\n" "\[\r\n\]+"} $program_output]
+set native_output [string map {"\\" "\\\\"} $native_output]
+
+# Prepare the pattern for the semihosted output.
+set semihosted_output [semihosted_string $program_output]
+
+# Combine both outputs in a single pattern.
+set output "($semihosted_output|$native_output)"
+
 # Next over the hello() call which will produce lots of output
 mi_gdb_test "220-exec-next" \
 	    "220\\^running(\r\n\\*running,thread-id=\"all\")?" \
 	    "Testing console output" \
-	    "Hello \\\\\"!\[\r\n\]+"
+	    $output
 
 mi_expect_stop "end-stepping-range" "main" "" ".*mi-console.c" "14" "" \
     "finished step over hello"
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index 3d55609..f38158e 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -765,20 +765,35 @@ proc mi_gdb_test { args } {
     if { $result == 0 } {
 	if [ info exists ipattern ] {
 	    if { ![target_info exists gdb,noinferiorio] } {
-		global mi_inferior_spawn_id
-		expect {
-		    -i $mi_inferior_spawn_id -re "$ipattern" {
-			pass "$message inferior output"
+		if { [target_info gdb_protocol] == "remote"
+		    || [target_info gdb_protocol] == "extended-remote"
+		    || [target_info gdb_protocol] == "sim"} {
+
+		    gdb_expect {
+			-re "$ipattern" {
+			    pass "$message inferior output"
+			}
+			timeout {
+			    fail "$message inferior output (timeout)"
+			    set result 1
+			}
 		    }
-		    timeout {
-			fail "$message inferior output (timeout)"
-			set result 1
+		} else {
+		    global mi_inferior_spawn_id
+		    expect {
+			-i $mi_inferior_spawn_id -re "$ipattern" {
+			    pass "$message inferior output"
+			}
+			timeout {
+			    fail "$message inferior output (timeout)"
+			    set result 1
+			}
 		    }
 		}
 	    } else {
 		unsupported "$message inferior output"
 	    }
-        }
+	}
     }
 
     return $result