[PATCH] Fix a bug in tstatus.exp matching tstatus output

[PATCH] Fix a bug in tstatus.exp matching tstatus output

[Yao Qi]

When running gdb.trace/tstatus.exp, we got the following output,

(gdb) tstatus
Trace stopped by a tstop command (because I can).
Collected 1 trace frames.
Trace buffer has 5242418 bytes of 5242880 bytes free (0% full).
Trace will stop if GDB disconnects.
Trace user is me me me.
Trace notes: different note.
Not looking at any trace frame.
Trace started at 572.802356 secs, stopped 0.085292 secs later.

apparently the stop reason is printed, but the test summary is

  (gdb) PASS: gdb.trace/tstatus.exp: tstatus does not report trace stop reason

after examining tstatus.exp, I find the "\\\" is missed for parentheses in
"(because I can)".  This patch adds "\\\" for them, and the test summary
becomes:

  (gdb) PASS: gdb.trace/tstatus.exp: tstatus reports trace stop reason

In gdb testsuite, both "\\\(" and "\\(" are used.  After reading
section "4.3. Backslashes" in book "Exploring Expect", I think they
are the same on effect.

gdb/testsuite:

2013-03-04  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/tstatus.exp (run_trace_experiment): Add "\\\" to
	parentheses.
---
 gdb/testsuite/gdb.trace/tstatus.exp |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/tstatus.exp b/gdb/testsuite/gdb.trace/tstatus.exp
index db620d0..654b64e 100644
--- a/gdb/testsuite/gdb.trace/tstatus.exp
+++ b/gdb/testsuite/gdb.trace/tstatus.exp
@@ -99,7 +99,7 @@ proc run_trace_experiment {} {
     gdb_test_no_output "tstop because I can" "trace stopped with note"
 
     gdb_test_multiple "tstatus" "check on trace status after stop" {
-	-re "Trace stopped by a tstop command (because I can)\..*Trace will stop if GDB disconnects\.\[\r\n\]+Trace user is me me me\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
+	-re "Trace stopped by a tstop command \\\(because I can\\\)\..*Trace will stop if GDB disconnects\.\[\r\n\]+Trace user is me me me\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
 	    pass "tstatus reports trace stop reason"
 	}
 	-re "Trace stopped by a tstop command\..*\r\n$gdb_prompt $" {
-- 
1.7.7.6

Re: [PATCH] Fix a bug in tstatus.exp matching tstatus output

[Joel Brobecker]

> In gdb testsuite, both "\\\(" and "\\(" are used.  After reading
> section "4.3. Backslashes" in book "Exploring Expect", I think they
> are the same on effect.

I don't understand why the third backslash would be needed, though.
The first two is the backslash escaping the '(' character in the
regular expression. The third one escapes the '(' as the TCL string
level, but I don't think it is a special character (like '[' is,
for instance), is it?

-- 
Joel

Re: [PATCH] Fix a bug in tstatus.exp matching tstatus output

[Pedro Alves]

On 03/04/2013 03:35 PM, Yao Qi wrote:
>   (gdb) PASS: gdb.trace/tstatus.exp: tstatus does not report trace stop reason
...
>   (gdb) PASS: gdb.trace/tstatus.exp: tstatus reports trace stop reason

I think the former (and any other similar case) should be
UNSUPPORTED rather than a PASS.  That'd make it much easier
to spot these issues, along with regressions and progressions.

-- 
Pedro Alves

Re: [PATCH] Fix a bug in tstatus.exp matching tstatus output

[Tom Tromey]

Joel> I don't understand why the third backslash would be needed, though.
Joel> The first two is the backslash escaping the '(' character in the
Joel> regular expression. The third one escapes the '(' as the TCL string
Joel> level, but I don't think it is a special character (like '[' is,
Joel> for instance), is it?

You are correct.  The third backslash is not needed for parens.

Tom

Re: [PATCH] Fix a bug in tstatus.exp matching tstatus output

[Yao Qi]

On 03/05/2013 12:22 AM, Joel Brobecker wrote:
>> In gdb testsuite, both "\\\(" and "\\(" are used.  After reading
>> section "4.3. Backslashes" in book "Exploring Expect", I think they
>> are the same on effect.
> 
> I don't understand why the third backslash would be needed, though.
> The first two is the backslash escaping the '(' character in the
> regular expression. The third one escapes the '(' as the TCL string
> level, but I don't think it is a special character (like '[' is,
> for instance), is it?
> 

Right, of course we can use two backslashes here.

Backslash translation is done by Tcl and pattern matcher.  In Tcl,
"Sequences that have no special translation are replaced by the 
character without the backslash" [1], and in pattern matcher, use 
"backslashes to force the following character into its literal 
equivalent." [2]

For "\\(", Tcl will translate it to "\(" and pattern matcher will treat 
it as literal "(".
For "\\\(", Tcl will tranlate it "\(" as well, because it translates 
"\\" to "\" and "\(" to "(".

Here is the updated patch using two backslashes.

-- 
Yao (齐尧)


gdb/testsuite:

2013-03-05  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/tstatus.exp (run_trace_experiment): Escape parentheses
	by "\\".
---
 gdb/testsuite/gdb.trace/tstatus.exp |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/tstatus.exp b/gdb/testsuite/gdb.trace/tstatus.exp
index 483bb20..3271ace 100644
--- a/gdb/testsuite/gdb.trace/tstatus.exp
+++ b/gdb/testsuite/gdb.trace/tstatus.exp
@@ -99,7 +99,7 @@ proc run_trace_experiment {} {
     gdb_test_no_output "tstop because I can" "trace stopped with note"
 
     gdb_test_multiple "tstatus" "check on trace status after stop" {
-	-re "Trace stopped by a tstop command (because I can)\..*Trace will stop if GDB disconnects\.\[\r\n\]+Trace user is me me me\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
+	-re "Trace stopped by a tstop command \\(because I can\\)\..*Trace will stop if GDB disconnects\.\[\r\n\]+Trace user is me me me\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
 	    pass "tstatus reports trace stop reason"
 	}
 	-re "Trace stopped by a tstop command\..*\r\n$gdb_prompt $" {
-- 
1.7.7.6

Re: [PATCH] Fix a bug in tstatus.exp matching tstatus output

[Yao Qi]

On 03/05/2013 12:32 AM, Pedro Alves wrote:
> On 03/04/2013 03:35 PM, Yao Qi wrote:
>>    (gdb) PASS: gdb.trace/tstatus.exp: tstatus does not report trace stop reason
> ...
>>    (gdb) PASS: gdb.trace/tstatus.exp: tstatus reports trace stop reason
>
> I think the former (and any other similar case) should be
> UNSUPPORTED rather than a PASS.  That'd make it much easier
> to spot these issues, along with regressions and progressions.
>
Several lines above, the comments say:

# Now play with tstatus a bit.
# Since note support is optional, we need to match both with and without
# cases.

so all tests below here match two cases (with and without notes).  I am 
not sure we can change it to UNSUPPORTED, but it is not the purpose or 
motivation of this patch.  It can be addressed separately.

-- 
Yao (齐尧)

tstatus.exp: use UNSUPPORTED for optional features that are not supported (was: Re: [PATCH] Fix a bug in tstatus.exp matching tstatus output)

[Pedro Alves]

On 03/05/2013 02:38 AM, Yao Qi wrote:
> On 03/05/2013 12:32 AM, Pedro Alves wrote:
>> On 03/04/2013 03:35 PM, Yao Qi wrote:
>>>    (gdb) PASS: gdb.trace/tstatus.exp: tstatus does not report trace stop reason
>> ...
>>>    (gdb) PASS: gdb.trace/tstatus.exp: tstatus reports trace stop reason
>>
>> I think the former (and any other similar case) should be
>> UNSUPPORTED rather than a PASS.  That'd make it much easier
>> to spot these issues, along with regressions and progressions.
>>
> Several lines above, the comments say:
> 
> # Now play with tstatus a bit.
> # Since note support is optional, we need to match both with and without
> # cases.
> 
> so all tests below here match two cases (with and without notes).  
> I am not sure we can change it to UNSUPPORTED, but 

I think we can.  More so since the "not supported" paths
actually explicitly check for output you'd get if the feature
wasn't supported, so real unexpected failures should be caught
as FAILs.

So now e.g., we wanted to check if tstatus reports the
trace stop reason, and if the target does, we get

 PASS: tstatus reports trace stop reason

if the target reports what we'd expect if trace stop reason
isn't supported, we get:

 UNSUPPORTED: tstatus reports trace stop reason

and if the target reports something else unexpected, we
get:

 FAIL: tstatus reports trace stop reason

That has the advantage that the test string is always
the same and only the test results change
(PASS/FAIL/UNSUPPORTED), which makes it easier for
testers see regressions, compared to:

 -PASS: gdb.trace/tstatus.exp: tstatus reports trace stop reason
 +PASS: gdb.trace/tstatus.exp: tstatus does not report trace stop reason

which clearly easily goes by unnoticed, as evidenced by
the existing problem your patch addresses.

> it is not the purpose or motivation of this patch.  It can be
> addressed separately.

Yes, of course.  Here's a patch to address it.

This is what I get with current tree (_without_ your patch):

Running target native-gdbserver
Running ../../../src/gdb/testsuite/gdb.trace/tstatus.exp ...
PASS: gdb.trace/tstatus.exp: IPA loaded
PASS: gdb.trace/tstatus.exp: tracepoint at gdb_c_test
PASS: gdb.trace/tstatus.exp: collect at set_point: define actions
PASS: gdb.trace/tstatus.exp: 4-byte fast tracepoint could not be set
PASS: gdb.trace/tstatus.exp: advance to trace begin
PASS: gdb.trace/tstatus.exp: start trace experiment
PASS: gdb.trace/tstatus.exp: advance through tracing
PASS: gdb.trace/tstatus.exp: tstatus reports trace note
PASS: gdb.trace/tstatus.exp: change tracing note
PASS: gdb.trace/tstatus.exp: tstatus reports different trace note
PASS: gdb.trace/tstatus.exp: change tracing user
PASS: gdb.trace/tstatus.exp: tstatus reports trace user
PASS: gdb.trace/tstatus.exp: trace stopped with note
UNSUPPORTED: gdb.trace/tstatus.exp: tstatus reports trace stop reason
PASS: gdb.trace/tstatus.exp: info trace reports tracepoint hit count and traceframe usage

gdb/testsuite/
2013-03-05  Pedro Alves  <palves@redhat.com>

	* gdb.trace/tstatus.exp (run_trace_experiment): When the target
	doesn't support the tested optional feature, call "unsupported"
	with the same test message as the "pass" case, instead of calling
	"pass" with a different message.  Use the same text for the "fail"
	cases too.

---

 gdb/testsuite/gdb.trace/tstatus.exp |   36 ++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/tstatus.exp b/gdb/testsuite/gdb.trace/tstatus.exp
index 8a0bbdf..d95a1c5 100644
--- a/gdb/testsuite/gdb.trace/tstatus.exp
+++ b/gdb/testsuite/gdb.trace/tstatus.exp
@@ -73,57 +73,59 @@ proc run_trace_experiment {} {
     # Since note support is optional, we need to match both with and without
     # cases.
 
+    set test "tstatus reports trace note"
     gdb_test_multiple "tstatus" "check on trace status" {
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Trace notes: my tracing note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
-	    pass "tstatus reports trace note"
+	    pass $test
 	}
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Not looking at any trace frame.*\r\n$gdb_prompt $" {
-	    pass "tstatus does not report any trace note"
+	    unsupported $test
 	}
     }
 
     gdb_test "set trace-notes different note" "" "change tracing note"
 
-    gdb_test_multiple "tstatus" "check on trace status with diff note" {
+    set test "tstatus reports different trace note"
+    gdb_test_multiple "tstatus" $test {
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
-	    pass "tstatus reports different trace note"
+	    pass $test
 	}
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Not looking at any trace frame.*\r\n$gdb_prompt $" {
-	    pass "tstatus does not report any different trace note"
+	    unsupported $test
 	}
     }
 
     gdb_test "set trace-user me me me" "" "change tracing user"
 
-    gdb_test_multiple "tstatus" "check on trace status with diff note" {
+    set test "tstatus reports trace user"
+    gdb_test_multiple "tstatus" $test {
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Trace user is me me me\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
-	    pass "tstatus reports trace user"
+	    pass $test
 	}
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Not looking at any trace frame.*\r\n$gdb_prompt $" {
-	    pass "tstatus does not report trace user"
+	    unsupported $test
 	}
     }
 
     gdb_test_no_output "tstop because I can" "trace stopped with note"
 
-    gdb_test_multiple "tstatus" "check on trace status after stop" {
+    set test "tstatus reports trace stop reason"
+    gdb_test_multiple "tstatus" $test {
 	-re "Trace stopped by a tstop command (because I can)\..*Trace will stop if GDB disconnects\.\[\r\n\]+Trace user is me me me\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
-	    pass "tstatus reports trace stop reason"
+	    pass $test
 	}
 	-re "Trace stopped by a tstop command\..*\r\n$gdb_prompt $" {
-	    pass "tstatus does not report trace stop reason"
+	    unsupported $test
 	}
     }
 
-    # Hit count and traceframe usage of tracepoint is optional, so
-    # pass it either way.
-
-    gdb_test_multiple "info trace" "show tracepoint state" {
+    set test "info trace reports tracepoint hit count and traceframe usage"
+    gdb_test_multiple "info trace" $test {
 	-re "actions\.c:\[0-9\]+\[\r\n\]+\[\t ]+tracepoint already hit 1 time\[\r\n\]+\[\t ]+trace buffer usage ${decimal} bytes\.\[\r\n\]+\[\t ]+collect parm.*\r\n$gdb_prompt $" {
-	    pass "info trace reports tracepoint hit count and traceframe usage"
+	    pass $test
 	}
 	-re "actions\.c:\[0-9\]+\[\r\n\]+\[\t ]+collect parm.*\r\n$gdb_prompt $" {
-	    pass "info trace does not report tracepoint hit count and traceframe usage"
+	    unsupported $test
 	}
     }
 }

Re: [PATCH] Fix a bug in tstatus.exp matching tstatus output

[Pedro Alves]

On 03/05/2013 02:23 AM, Yao Qi wrote:

> Here is the updated patch using two backslashes.

I guess that leaves rubber-stamping it.  :-)

On 03/05/2013 02:23 AM, Yao Qi wrote:
> gdb/testsuite:
> 
> 2013-03-05  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.trace/tstatus.exp (run_trace_experiment): Escape parentheses
> 	by "\\".

OK.

-- 
Pedro Alves

Re: tstatus.exp: use UNSUPPORTED for optional features that are not supported

[Yao Qi]

Pedro,
The patch looks pretty good to me.

On 03/06/2013 03:02 AM, Pedro Alves wrote:
> +    set test "tstatus reports trace note"
>       gdb_test_multiple "tstatus" "check on trace status" {
                                     ^^^^^^^^^^^^^^^^^^^^^^ use $test here.

-- 
Yao (齐尧)

Re: tstatus.exp: use UNSUPPORTED for optional features that are not supported

[Pedro Alves]

On 03/06/2013 01:13 AM, Yao Qi wrote:
> Pedro,
> The patch looks pretty good to me.
> 
> On 03/06/2013 03:02 AM, Pedro Alves wrote:
>> +    set test "tstatus reports trace note"
>>       gdb_test_multiple "tstatus" "check on trace status" {
>                                     ^^^^^^^^^^^^^^^^^^^^^^ use $test here.

Thanks.

Here's what I checked in.

-- >8 --
Subject: tstatus.exp: use UNSUPPORTED for optional features that are not supported

The current tstatus.exp tests shows PASSes if either the target
support or not the optional tstatus bits:

 PASS: gdb.trace/tstatus.exp: tstatus does not report trace stop reason
 PASS: gdb.trace/tstatus.exp: tstatus reports trace stop reason

The former (and any other similar case) should be UNSUPPORTED rather
than PASS.  That'd make it much easier to spot actually problems with
the test (e.g., the one Yao's previous patch addressed), along with
regressions and progressions.

The "not supported" paths in tstatus.exp explicitly check for output
you'd get if the feature wasn't supported, so real unexpected failures
will still be caught as FAILs.

So now e.g., where we wanted to check if tstatus reports the trace
stop reason, and if the target does support it, we get

 PASS: tstatus reports trace stop reason

if the target actually reports what we'd expect if the trace stop
reason isn't supported, we get:

 UNSUPPORTED: tstatus reports trace stop reason

and if the target reports something else unexpected, we get:

 FAIL: tstatus reports trace stop reason

That has the added bonus that the test string is always the same and
only the test results change (PASS/FAIL/UNSUPPORTED), which makes it
easier for testers see regressions, compared to the previous:

 -PASS: gdb.trace/tstatus.exp: tstatus reports trace stop reason
 +PASS: gdb.trace/tstatus.exp: tstatus does not report trace stop reason

which clearly easily goes by unnoticed, as evidenced by the existing
problem Yao's previous patch addressed.

Tested on x86_64 Fedora 17.

gdb/testsuite/
2013-03-06  Pedro Alves  <palves@redhat.com>

	* gdb.trace/tstatus.exp (run_trace_experiment): When the target
	doesn't support the tested optional feature, call "unsupported"
	with the same test message as the "pass" case, instead of calling
	"pass" with a different message.  Use the same text for the "fail"
	cases too.
---
 gdb/testsuite/gdb.trace/tstatus.exp |   43 +++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/tstatus.exp b/gdb/testsuite/gdb.trace/tstatus.exp
index 46faaf8..743db91 100644
--- a/gdb/testsuite/gdb.trace/tstatus.exp
+++ b/gdb/testsuite/gdb.trace/tstatus.exp
@@ -51,60 +51,63 @@ proc run_trace_experiment {} {
 	"advance through tracing"
      # Now play with tstatus a bit.
-    # Since note support is optional, we need to match both with and without
-    # cases.
 -    gdb_test_multiple "tstatus" "check on trace status" {
+    # Since support for notes, user, stop reason, etc. is optional, we
+    # need to match both with and without cases.
+
+    set test "tstatus reports trace note"
+    gdb_test_multiple "tstatus" $test {
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Trace notes: my tracing note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
-	    pass "tstatus reports trace note"
+	    pass $test
 	}
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Not looking at any trace frame.*\r\n$gdb_prompt $" {
-	    pass "tstatus does not report any trace note"
+	    unsupported $test
 	}
     }
      gdb_test "set trace-notes different note" "" "change tracing note"
 -    gdb_test_multiple "tstatus" "check on trace status with diff note" {
+    set test "tstatus reports different trace note"
+    gdb_test_multiple "tstatus" $test {
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
-	    pass "tstatus reports different trace note"
+	    pass $test
 	}
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Not looking at any trace frame.*\r\n$gdb_prompt $" {
-	    pass "tstatus does not report any different trace note"
+	    unsupported $test
 	}
     }
      gdb_test "set trace-user me me me" "" "change tracing user"
 -    gdb_test_multiple "tstatus" "check on trace status with diff note" {
+    set test "tstatus reports trace user"
+    gdb_test_multiple "tstatus" $test {
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Trace user is me me me\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
-	    pass "tstatus reports trace user"
+	    pass $test
 	}
 	-re "Trace is running.*Trace will stop if GDB disconnects\.\[\r\n\]+Not looking at any trace frame.*\r\n$gdb_prompt $" {
-	    pass "tstatus does not report trace user"
+	    unsupported $test
 	}
     }
      gdb_test_no_output "tstop because I can" "trace stopped with note"
 -    gdb_test_multiple "tstatus" "check on trace status after stop" {
+    set test "tstatus reports trace stop reason"
+    gdb_test_multiple "tstatus" $test {
 	-re "Trace stopped by a tstop command \\(because I can\\)\..*Trace will stop if GDB disconnects\.\[\r\n\]+Trace user is me me me\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\..*\r\n$gdb_prompt $" {
-	    pass "tstatus reports trace stop reason"
+	    pass $test
 	}
 	-re "Trace stopped by a tstop command\..*\r\n$gdb_prompt $" {
-	    pass "tstatus does not report trace stop reason"
+	    unsupported $test
 	}
     }
 -    # Hit count and traceframe usage of tracepoint is optional, so
-    # pass it either way.
-
-    gdb_test_multiple "info trace" "show tracepoint state" {
+    set test "info trace reports tracepoint hit count and traceframe usage"
+    gdb_test_multiple "info trace" $test {
 	-re "actions\.c:\[0-9\]+\[\r\n\]+\[\t ]+tracepoint already hit 1 time\[\r\n\]+\[\t ]+trace buffer usage ${decimal} bytes\.\[\r\n\]+\[\t ]+collect parm.*\r\n$gdb_prompt $" {
-	    pass "info trace reports tracepoint hit count and traceframe usage"
+	    pass $test
 	}
 	-re "actions\.c:\[0-9\]+\[\r\n\]+\[\t ]+collect parm.*\r\n$gdb_prompt $" {
-	    pass "info trace does not report tracepoint hit count and traceframe usage"
+	    unsupported $test
 	}
     }
 }