[RFC][gdb/testsuite] Remove breakpoint command in gdb.base/ui-redirect.exp

[RFC][gdb/testsuite] Remove breakpoint command in gdb.base/ui-redirect.exp

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

Hi,

I noticed a bit in gdb.base/ui-redirect.exp that sets a breakpoint
command on main, but then due to runto_main, the break point is deleted:
...
(gdb) break main^M
Breakpoint 1 at 0x4004d7: file
/data/gdb_versions/devel/src/gdb/testsuite/gdb.base/start.c, line 34.^M
(gdb) commands^M
Type commands for breakpoint(s) 1, one per line.^M
End with a line saying just "end".^M
>PASS: gdb.base/ui-redirect.exp: commands
print 1^M
>PASS: gdb.base/ui-redirect.exp: print 1
end^M
(gdb) PASS: gdb.base/ui-redirect.exp: end
delete breakpoints^M
Delete all breakpoints? (y or n) y^M
...

In the original commit ( submission here
https://sourceware.org/ml/gdb-patches/2010-09/msg00120.html ) there was
no runto_main, and the breakpoint command was not triggered either.

Is this some artefact, and can it be removed, or is it actually testing
something related to redirection?

Thanks,
- Tom

[-- Attachment #2: tmp.patch --]
[-- Type: text/x-patch, Size: 678 bytes --]

diff --git a/gdb/testsuite/gdb.base/ui-redirect.exp b/gdb/testsuite/gdb.base/ui-redirect.exp
index 4507ac51a2..e48ca72e91 100644
--- a/gdb/testsuite/gdb.base/ui-redirect.exp
+++ b/gdb/testsuite/gdb.base/ui-redirect.exp
@@ -23,23 +23,6 @@ if { [prepare_for_testing "failed to prepare" ui-redirect start.c] } {
     return -1
 }
 
-gdb_breakpoint main
-
-set test "commands"
-gdb_test_multiple $test $test {
-    -re "End with a line saying just \"end\"\\.\r\n>$" {
-	pass $test
-    }
-}
-
-set test "print 1"
-gdb_test_multiple $test $test {
-    -re "\r\n>$" {
-	pass $test
-    }
-}
-gdb_test_no_output "end"
-
 if ![runto_main] {
     fail "can't run to main"
     return -1

Re: [RFC][gdb/testsuite] Remove breakpoint command in gdb.base/ui-redirect.exp

[Andrew Burgess]

* Tom de Vries <tdevries@suse.de> [2019-09-03 19:43:44 +0200]:

> Hi,
> 
> I noticed a bit in gdb.base/ui-redirect.exp that sets a breakpoint
> command on main, but then due to runto_main, the break point is deleted:
> ...
> (gdb) break main^M
> Breakpoint 1 at 0x4004d7: file
> /data/gdb_versions/devel/src/gdb/testsuite/gdb.base/start.c, line 34.^M
> (gdb) commands^M
> Type commands for breakpoint(s) 1, one per line.^M
> End with a line saying just "end".^M
> >PASS: gdb.base/ui-redirect.exp: commands
> print 1^M
> >PASS: gdb.base/ui-redirect.exp: print 1
> end^M
> (gdb) PASS: gdb.base/ui-redirect.exp: end
> delete breakpoints^M
> Delete all breakpoints? (y or n) y^M
> ...
> 
> In the original commit ( submission here
> https://sourceware.org/ml/gdb-patches/2010-09/msg00120.html ) there was
> no runto_main, and the breakpoint command was not triggered either.
> 
> Is this some artefact, and can it be removed, or is it actually testing
> something related to redirection?

I can't comment on the original patch intention, but given this part
of the tests never really did anything useful, and the "commands"
mechanism is tested in gdb.base/commands.exp, I'd be OK with what you
propose below.

I'd suggest give it a few days for anyone else to chip in, then just
commit this as an obvious cleanup.

Thanks,
Andrew

> 
> Thanks,
> - Tom

> diff --git a/gdb/testsuite/gdb.base/ui-redirect.exp b/gdb/testsuite/gdb.base/ui-redirect.exp
> index 4507ac51a2..e48ca72e91 100644
> --- a/gdb/testsuite/gdb.base/ui-redirect.exp
> +++ b/gdb/testsuite/gdb.base/ui-redirect.exp
> @@ -23,23 +23,6 @@ if { [prepare_for_testing "failed to prepare" ui-redirect start.c] } {
>      return -1
>  }
>  
> -gdb_breakpoint main
> -
> -set test "commands"
> -gdb_test_multiple $test $test {
> -    -re "End with a line saying just \"end\"\\.\r\n>$" {
> -	pass $test
> -    }
> -}
> -
> -set test "print 1"
> -gdb_test_multiple $test $test {
> -    -re "\r\n>$" {
> -	pass $test
> -    }
> -}
> -gdb_test_no_output "end"
> -
>  if ![runto_main] {
>      fail "can't run to main"
>      return -1

Re: [RFC][gdb/testsuite] Remove breakpoint command in gdb.base/ui-redirect.exp

[Jan Kratochvil]

On Tue, 03 Sep 2019 19:43:44 +0200, Tom de Vries wrote:
> I noticed a bit in gdb.base/ui-redirect.exp that sets a breakpoint
> command on main, but then due to runto_main, the break point is deleted:
...
> In the original commit ( submission here
> https://sourceware.org/ml/gdb-patches/2010-09/msg00120.html ) there was
> no runto_main, and the breakpoint command was not triggered either.
> 
> Is this some artefact, and can it be removed, or is it actually testing
> something related to redirection?

It looks to me the patch which added the "runto_main" part:

commit ca1285d17534cff3041c07ac7841288e1b3ba19c
Author: Alan Hayward <alan.hayward@arm.com>
Date:   Fri May 17 14:15:01 2019 +0100
    Add debug redirect option

disabled the regression testing.

"runto_main" should be removed otherwise the testcase does not test anything
(or it just tests less).


Jan

Re: [RFC][gdb/testsuite] Remove breakpoint command in gdb.base/ui-redirect.exp

[Jan Kratochvil]

On Wed, 04 Sep 2019 12:59:52 +0200, Andrew Burgess wrote:
> I can't comment on the original patch intention, but given this part
> of the tests never really did anything useful, and the "commands"
> mechanism is tested in gdb.base/commands.exp, I'd be OK with what you
> propose below.

The "commands" mechanism is not tested there for "save breakpoints" with
various types of output redirection.


Jan

[PATCH][gdb/testsuite] Restore breakpoint command in ui-redirect.exp

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]

[ was: Re: [RFC][gdb/testsuite] Remove breakpoint command in
gdb.base/ui-redirect.exp ]

On 04-09-19 13:48, Jan Kratochvil wrote:
> On Tue, 03 Sep 2019 19:43:44 +0200, Tom de Vries wrote:
>> I noticed a bit in gdb.base/ui-redirect.exp that sets a breakpoint
>> command on main, but then due to runto_main, the break point is deleted:
> ...
>> In the original commit ( submission here
>> https://sourceware.org/ml/gdb-patches/2010-09/msg00120.html ) there was
>> no runto_main, and the breakpoint command was not triggered either.
>>
>> Is this some artefact, and can it be removed, or is it actually testing
>> something related to redirection?
> 
> It looks to me the patch which added the "runto_main" part:
> 
> commit ca1285d17534cff3041c07ac7841288e1b3ba19c
> Author: Alan Hayward <alan.hayward@arm.com>
> Date:   Fri May 17 14:15:01 2019 +0100
>     Add debug redirect option
> 
> disabled the regression testing.
> 
> "runto_main" should be removed otherwise the testcase does not test anything
> (or it just tests less).

Attached patch fixes the problem by replacing the initial
"gdb_breakpoint main" with the runto_main, such that the breakpoint
command is preserved throughout.

OK for trunk?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Restore-breakpoint-command-in-ui-redirect.exp.patch --]
[-- Type: text/x-patch, Size: 4977 bytes --]

[gdb/testsuite] Restore breakpoint command in ui-redirect.exp

In gdb.base/ui-redirect.exp, the "save breakpoint" command is used to write
the current breakpoints to a file, but the actual output is not verified.

Consequently, the test has regressed in that the "print 1" command associated
with a breakpoint on main is removed by a subsequent runto_main, which first
deletes all breakpoints:
...
(gdb) break main
Breakpoint 1 at 0x4004d7: file start.c, line 34.
(gdb) commands
Type commands for breakpoint(s) 1, one per line.
End with a line saying just "end".
> PASS: gdb.base/ui-redirect.exp: commands
print 1
> PASS: gdb.base/ui-redirect.exp: print 1
end
(gdb) PASS: gdb.base/ui-redirect.exp: end
delete breakpoints
Delete all breakpoints? (y or n) y
...
and consequently the "save breakpoint" output is missing the breakpoint
command for main:
...
 break main
-  commands
-    print 1
-  end
 break foo
 break bar
...

Fix this by replacing "gdb_breakpoint main" with runto_main, and verifying the
"save breakpoints" output.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2019-09-05  Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (cmp_file_string): New proc.
	* gdb.base/ui-redirect.exp: Replace "gdb_breakpoint main" with
	runto_main.  Verify save breakpoints output.

---
 gdb/testsuite/gdb.base/ui-redirect.exp | 33 +++++++++++++++++++++++++--------
 gdb/testsuite/lib/gdb.exp              | 27 +++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.base/ui-redirect.exp b/gdb/testsuite/gdb.base/ui-redirect.exp
index 4507ac51a2..3da30cd7f7 100644
--- a/gdb/testsuite/gdb.base/ui-redirect.exp
+++ b/gdb/testsuite/gdb.base/ui-redirect.exp
@@ -23,7 +23,10 @@ if { [prepare_for_testing "failed to prepare" ui-redirect start.c] } {
     return -1
 }
 
-gdb_breakpoint main
+if ![runto_main] {
+    fail "can't run to main"
+    return -1
+}
 
 set test "commands"
 gdb_test_multiple $test $test {
@@ -40,18 +43,29 @@ gdb_test_multiple $test $test {
 }
 gdb_test_no_output "end"
 
-if ![runto_main] {
-    fail "can't run to main"
-    return -1
-}
 gdb_breakpoint "foo"
 gdb_breakpoint "bar"
 
+set cmds \
+    [list \
+	 "break main" \
+	 "  commands" \
+	 "    print 1" \
+	 "  end" \
+	 "break foo" \
+	 "break bar"]
+set cmds [join $cmds "\n"]
+set cmds "$cmds\n"
+set outdir [standard_output_file {}]
+set cmds_file "$outdir/cmds.txt"
+
 with_test_prefix "logging" {
     gdb_test_no_output "set logging file /dev/null"
     gdb_test "set logging on" \
     "Copying output to /dev/null.*Copying debug output to /dev/null\\."
-    gdb_test "save breakpoints /dev/null" "Saved to file '/dev/null'\\."
+    gdb_test "save breakpoints $cmds_file" "Saved to file '$cmds_file'\\." \
+	"save breakpoints cmds.txt"
+    cmp_file_string "$cmds_file" "$cmds" "cmds.txt"
     gdb_test "set logging off" "Done logging to /dev/null\\."
     gdb_test "help" "List of classes of commands:.*"
 }
@@ -60,7 +74,8 @@ with_test_prefix "redirect" {
     gdb_test "set logging redirect on"
     gdb_test "set logging on" \
     "Redirecting output to /dev/null.*Copying debug output to /dev/null\\."
-    gdb_test_no_output "save breakpoints /dev/null"
+    gdb_test_no_output "save breakpoints $cmds_file" "save breakpoints cmds.txt"
+    cmp_file_string "$cmds_file" "$cmds" "cmds.txt"
     gdb_test "set logging off" "Done logging to /dev/null\\."
     gdb_test "help" "List of classes of commands:.*"
 }
@@ -71,7 +86,9 @@ with_test_prefix "redirect while already logging" {
     "Copying output to /dev/null.*Copying debug output to /dev/null\\."
     gdb_test "set logging redirect on" \
     ".*warning: Currently logging .*Turn the logging off and on to make the new setting effective.*"
-    gdb_test "save breakpoints /dev/null" "Saved to file '/dev/null'\\."
+    gdb_test "save breakpoints $cmds_file" "Saved to file '$cmds_file'\\." \
+	"save breakpoints cmds.txt"
+    cmp_file_string "$cmds_file" "$cmds" "cmds.txt"
     gdb_test "set logging off" "Done logging to /dev/null\\."
     gdb_test "help" "List of classes of commands:.*"
     gdb_test_no_output "set logging redirect off"
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 76805fb5ec..6ceec00e37 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -6668,5 +6668,32 @@ proc gdb_write_cmd_file { cmdline } {
     catch "close $cmd_file"
 }
 
+# Compare contents of FILE to string STR. Pass with MSG if equal, otherwise
+# fail with MSG.
+
+proc cmp_file_string { file str msg } {
+    if { ![file exists $file]} {
+	fail "$msg"
+	return
+    }
+
+    set caught_error [catch {
+	set fp [open "$file" r]
+	set file_contents [read $fp]
+	close $fp
+    } error_message]
+    if { $caught_error } then {
+	error "$error_message"
+	fail "$msg"
+	return
+    }
+
+    if { $file_contents == $str } {
+	pass "$msg"
+    } else {
+	fail "$msg"
+    }
+}
+
 # Always load compatibility stuff.
 load_lib future.exp

Re: [PATCH][gdb/testsuite] Restore breakpoint command in ui-redirect.exp

[Jan Kratochvil]

On Thu, 05 Sep 2019 09:34:57 +0200, Tom de Vries wrote:
> Attached patch fixes the problem by replacing the initial
> "gdb_breakpoint main" with the runto_main, such that the breakpoint
> command is preserved throughout.

Yes, the test is now much better than even was the initial one by me.


> OK for trunk?

I cannot approve GDB patches but LGTM.


Thanks,
Jan

Re: [PATCH][gdb/testsuite] Restore breakpoint command in ui-redirect.exp

[Tom de Vries]

On 05-09-19 14:03, Jan Kratochvil wrote:
> On Thu, 05 Sep 2019 09:34:57 +0200, Tom de Vries wrote:
>> Attached patch fixes the problem by replacing the initial
>> "gdb_breakpoint main" with the runto_main, such that the breakpoint
>> command is preserved throughout.
> 
> Yes, the test is now much better than even was the initial one by me.
> 
> 
>> OK for trunk?
> 
> I cannot approve GDB patches but LGTM.
> 

Thanks. I would at this point commit the patch under the obvious rule,
if it didn't introduce a new proc cmp_file_string in lib/gdb.exp.

Thanks,
- Tom

Re: [PATCH][gdb/testsuite] Restore breakpoint command in ui-redirect.exp

[Pedro Alves]

On 9/5/19 8:34 AM, Tom de Vries wrote:

> +set cmds \
> +    [list \
> +	 "break main" \
> +	 "  commands" \
> +	 "    print 1" \
> +	 "  end" \
> +	 "break foo" \
> +	 "break bar"]
> +set cmds [join $cmds "\n"]

Use multi_line_input ?

> +# Compare contents of FILE to string STR. Pass with MSG if equal, otherwise
> +# fail with MSG.

Double space after period.

Thanks,
Pedro Alves

Re: [PATCH][gdb/testsuite] Restore breakpoint command in ui-redirect.exp

[Tom de Vries]

On 05-09-19 14:19, Pedro Alves wrote:
> On 9/5/19 8:34 AM, Tom de Vries wrote:
> 
>> +set cmds \
>> +    [list \
>> +	 "break main" \
>> +	 "  commands" \
>> +	 "    print 1" \
>> +	 "  end" \
>> +	 "break foo" \
>> +	 "break bar"]
>> +set cmds [join $cmds "\n"]
> 
> Use multi_line_input ?
> 
>> +# Compare contents of FILE to string STR. Pass with MSG if equal, otherwise
>> +# fail with MSG.
> 
> Double space after period.
> 

Thanks for the review.

Committed with these two review remarks fixed.

Thanks,
- Tom