diff mbox

save breakpoints does not save signal catchpoints correctly

Message ID 20141003203219.GA30562@host2.jankratochvil.net
State New
Headers show

Commit Message

Jan Kratochvil Oct. 3, 2014, 8:32 p.m. UTC
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
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.

Comments

Yao Qi Oct. 9, 2014, 1:37 p.m. UTC | #1
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.
Jan Kratochvil Oct. 9, 2014, 5:54 p.m. UTC | #2
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
Yao Qi Oct. 10, 2014, 12:59 a.m. UTC | #3
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.
diff mbox

Patch

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'"