[patch] save breakpoints does not save signal catchpoints correctly

[patch] save breakpoints does not save signal catchpoints correctly

[Jan Kratochvil]

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

Hi,

Miroslav Franc submitted a fix
	https://bugzilla.redhat.com/show_bug.cgi?id=1146170#c0

so posting it with a testcase.

No regressions on {x86_64,x86_64-m32,i686}-fedora21pre-linux-gnu.


Jan

[-- Attachment #2: franc1.patch --]
[-- Type: text/plain, Size: 2181 bytes --]

gdb/
2014-10-03  Miroslav Franc  <mfranc@redhat.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix "save breakpoints" for "catch" command.
	* break-catch-sig.c (signal_catchpoint_print_recreate): Add trailing
	newline.

gdb/testsuite/
2014-10-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix "save breakpoints" for "catch" command.
	* gdb.base/catch-signal.exp: Add gdb_breakpoint "main".
	Remove -nonewline.  Match also the added "main" line.

diff --git a/gdb/break-catch-sig.c b/gdb/break-catch-sig.c
index c41bf33..579fb78 100644
--- a/gdb/break-catch-sig.c
+++ b/gdb/break-catch-sig.c
@@ -346,6 +346,7 @@ signal_catchpoint_print_recreate (struct breakpoint *b, struct ui_file *fp)
     }
   else if (c->catch_all)
     fprintf_unfiltered (fp, " all");
+  fputc_unfiltered ('\n', fp);
 }
 
 /* Implement the "explains_signal" breakpoint_ops method for signal
diff --git a/gdb/testsuite/gdb.base/catch-signal.exp b/gdb/testsuite/gdb.base/catch-signal.exp
index 22caf40..7002f84 100644
--- a/gdb/testsuite/gdb.base/catch-signal.exp
+++ b/gdb/testsuite/gdb.base/catch-signal.exp
@@ -117,6 +117,7 @@ foreach {arg desc} {"" "standard signals" \
 	"set catchpoint '$arg' for printing"
     gdb_test "info break" "$decimal.*catchpoint.*signal.*$desc.*" \
 	"info break for '$arg'"
+    gdb_breakpoint "main"
     gdb_test "save breakpoints [standard_output_file bps.$i]" \
 	"Saved to file .*bps.$i.*" \
 	"save breakpoints for '$arg'"
@@ -124,15 +125,17 @@ foreach {arg desc} {"" "standard signals" \
     set filename [remote_upload host [standard_output_file bps.$i] \
 		      [standard_output_file bps-local.$i]]
     set fd [open $filename]
-    set contents [read -nonewline $fd]
+    set contents [read $fd]
     close $fd
 
+    set nl "\r?\n"
     if {$arg == ""} {
-	set pattern "catch signal"
+	set pattern "catch signal$nl"
     } else {
-	set pattern "catch signal $arg"
+	set pattern "catch signal $arg$nl"
     }
-    if {[string match $pattern $contents]} {
+    set pattern "${pattern}break main$nl"
+    if {[regexp "$pattern" $contents]} {
 	pass "results of save breakpoints for '$arg'"
     } else {
 	fail "results of save breakpoints for '$arg'"

Re: [patch] save breakpoints does not save signal catchpoints correctly

[Yao Qi]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

The fix looks right to me.  Just one comment on the test case...

> -    set contents [read -nonewline $fd]
> +    set contents [read $fd]
>      close $fd
>  

IWBN to split the contents into a list and match each element in the
list one by one, so that ...

> +    set nl "\r?\n"
>      if {$arg == ""} {
> -	set pattern "catch signal"
> +	set pattern "catch signal$nl"
>      } else {
> -	set pattern "catch signal $arg"
> +	set pattern "catch signal $arg$nl"
>      }
> -    if {[string match $pattern $contents]} {
> +    set pattern "${pattern}break main$nl"
> +    if {[regexp "$pattern" $contents]} {

... the pattern can be simplified and we can still use "string match".

On the other hand, in this way, the test can be easily extended in the
future, for example, saving more breakpoints in the file.

-- 
Yao (齐尧)

Re: [patch] save breakpoints does not save signal catchpoints correctly

[Jan Kratochvil]

On Thu, 09 Oct 2014 15:37:24 +0200, Yao Qi wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> The fix looks right to me.  Just one comment on the test case...
> 
> > -    set contents [read -nonewline $fd]
> > +    set contents [read $fd]
> >      close $fd
> >  
> 
> IWBN to split the contents into a list and match each element in the
> list one by one, so that ...
> 
> > +    set nl "\r?\n"
> >      if {$arg == ""} {
> > -	set pattern "catch signal"
> > +	set pattern "catch signal$nl"
> >      } else {
> > -	set pattern "catch signal $arg"
> > +	set pattern "catch signal $arg$nl"
> >      }
> > -    if {[string match $pattern $contents]} {
> > +    set pattern "${pattern}break main$nl"
> > +    if {[regexp "$pattern" $contents]} {
> 
> ... the pattern can be simplified and we can still use "string match".
> 
> On the other hand, in this way, the test can be easily extended in the
> future, for example, saving more breakpoints in the file.

Sorry but be more specific how you wanted to simplify the code.
Any idea I have is 10x more complicated.

With the list I have Googled
	What is the right way of comparing two lists in TCL?
	https://stackoverflow.com/questions/5195153/what-is-the-right-way-of-comparing-two-lists-in-tcl

and those solutions seem too complicated to me.

gdb_expect_list or gdb_test_sequence are not usable here as this is not an
output from inferior but just contents of a file.


Thanks,
Jan

Re: [patch] save breakpoints does not save signal catchpoints correctly

[Yao Qi]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> Sorry but be more specific how you wanted to simplify the code.
> Any idea I have is 10x more complicated.
>
> With the list I have Googled
> 	What is the right way of comparing two lists in TCL?
> 	https://stackoverflow.com/questions/5195153/what-is-the-right-way-of-comparing-two-lists-in-tcl
>
> and those solutions seem too complicated to me.

I didn't mean to compare two lists.  We only have one list here, and we
can check each element in it, for example,

     set fd [open $filename]
-    set contents [read -nonewline $fd]
+    set file_data [read $fd]
+    set data [split $file_data "\n"]
+    set contents [lindex $data 0]
+
     close $fd

contents is the first line of the file, and the pattern matching code
can be unchanged.  After that, we can check the second line of the file,
which should be "break main".  The test becomes,

    set fd [open $filename]
    set file_data [read $fd]
    set data [split $file_data "\n"]
    close $fd

    if {$arg == ""} {
	set pattern "catch signal"
    } else {
	set pattern "catch signal $arg"
    }
    # Check the first line.
    gdb_assert {[string match $pattern [lindex $data 0]]} \
	"1st line of save breakpoints for '$arg'"
    # Check the second line.
    gdb_assert {[string match "break main" [lindex $data 1]]} \
	"2nd line of save breakpoints for '$arg'"

This will be more clear and easier to extend when there are more lines in
the file.

-- 
Yao (齐尧)

[patchv2] save breakpoints does not save signal catchpoints correctly

[Jan Kratochvil]

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

On Fri, 10 Oct 2014 02:59:01 +0200, Yao Qi wrote:
> I didn't mean to compare two lists.  We only have one list here, and we
> can check each element in it, for example,

Done.


Thanks,
Jan

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

gdb/
2014-10-03  Miroslav Franc  <mfranc@redhat.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix "save breakpoints" for "catch" command.
	* break-catch-sig.c (signal_catchpoint_print_recreate): Add trailing
	newline.

gdb/testsuite/
2014-10-12  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Yao Qi  <yao@codesourcery.com>

	Fix "save breakpoints" for "catch" command.
	* gdb.base/catch-signal.exp: Add gdb_breakpoint "main".
	Remove -nonewline.  Match also the added "main" line.

diff --git a/gdb/break-catch-sig.c b/gdb/break-catch-sig.c
index c41bf33..579fb78 100644
--- a/gdb/break-catch-sig.c
+++ b/gdb/break-catch-sig.c
@@ -346,6 +346,7 @@ signal_catchpoint_print_recreate (struct breakpoint *b, struct ui_file *fp)
     }
   else if (c->catch_all)
     fprintf_unfiltered (fp, " all");
+  fputc_unfiltered ('\n', fp);
 }
 
 /* Implement the "explains_signal" breakpoint_ops method for signal
diff --git a/gdb/testsuite/gdb.base/catch-signal.exp b/gdb/testsuite/gdb.base/catch-signal.exp
index 22caf40..616b2a8 100644
--- a/gdb/testsuite/gdb.base/catch-signal.exp
+++ b/gdb/testsuite/gdb.base/catch-signal.exp
@@ -117,6 +117,7 @@ foreach {arg desc} {"" "standard signals" \
 	"set catchpoint '$arg' for printing"
     gdb_test "info break" "$decimal.*catchpoint.*signal.*$desc.*" \
 	"info break for '$arg'"
+    gdb_breakpoint "main"
     gdb_test "save breakpoints [standard_output_file bps.$i]" \
 	"Saved to file .*bps.$i.*" \
 	"save breakpoints for '$arg'"
@@ -124,7 +125,8 @@ foreach {arg desc} {"" "standard signals" \
     set filename [remote_upload host [standard_output_file bps.$i] \
 		      [standard_output_file bps-local.$i]]
     set fd [open $filename]
-    set contents [read -nonewline $fd]
+    set file_data [read $fd]
+    set data [split $file_data "\n"]
     close $fd
 
     if {$arg == ""} {
@@ -132,11 +134,17 @@ foreach {arg desc} {"" "standard signals" \
     } else {
 	set pattern "catch signal $arg"
     }
-    if {[string match $pattern $contents]} {
-	pass "results of save breakpoints for '$arg'"
-    } else {
-	fail "results of save breakpoints for '$arg'"
-    }
+    gdb_assert {[expr [llength $data] == 3]} \
+	"Number of lines of save breakpoints for '$arg'"
+    # Check the first line.
+    gdb_assert {[string match $pattern [lindex $data 0]]} \
+	"1st line of save breakpoints for '$arg'"
+    # Check the second line.
+    gdb_assert {[string match "break main" [lindex $data 1]]} \
+        "2nd line of save breakpoints for '$arg'"
+    # Check the trailing newline.
+    gdb_assert {[string match "" [lindex $data 2]]} \
+        "Trailing newline of save breakpoints for '$arg'"
 
     incr i
 }

Re: [patchv2] save breakpoints does not save signal catchpoints correctly

[Yao Qi]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> gdb/
> 2014-10-03  Miroslav Franc  <mfranc@redhat.com>
> 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
>
> 	Fix "save breakpoints" for "catch" command.
> 	* break-catch-sig.c (signal_catchpoint_print_recreate): Add trailing
> 	newline.
>
> gdb/testsuite/
> 2014-10-12  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Yao Qi  <yao@codesourcery.com>
>
> 	Fix "save breakpoints" for "catch" command.
> 	* gdb.base/catch-signal.exp: Add gdb_breakpoint "main".
> 	Remove -nonewline.  Match also the added "main" line.

Looks good to me.

-- 
Yao (齐尧)

[commit] [patchv2] save breakpoints does not save signal catchpoints correctly

[Jan Kratochvil]

On Mon, 13 Oct 2014 11:16:38 +0200, Yao Qi wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> > gdb/
> > 2014-10-03  Miroslav Franc  <mfranc@redhat.com>
> > 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
> >
> > 	Fix "save breakpoints" for "catch" command.
> > 	* break-catch-sig.c (signal_catchpoint_print_recreate): Add trailing
> > 	newline.
> >
> > gdb/testsuite/
> > 2014-10-12  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 	    Yao Qi  <yao@codesourcery.com>
> >
> > 	Fix "save breakpoints" for "catch" command.
> > 	* gdb.base/catch-signal.exp: Add gdb_breakpoint "main".
> > 	Remove -nonewline.  Match also the added "main" line.
> 
> Looks good to me.

Checked in:
	c780cc2f5062451a568458b6ef9b8aef7cc1dd8a


Thanks,
Jan