[RFC] Support temporary breakpoints in runto_main.

[RFC] Support temporary breakpoints in runto_main.

[Doug Evans]

Hi.

If I imagine I'm new to gdb and see the name runto_main I can well
imagine not expecting the implementation of that command to leave the
breakpoint behind.
Leaving the breakpoint behind is not something I would expect from that name.

Thus to me I think the default for runto_main should be to use
a temporary breakpoint.  However, there are several tests that
have been written that assume it does.

And, there is one test that assumed it didn't: wp-replication.exp.
But that I fixed differently:
http://sourceware.org/ml/gdb-patches/2013-05/msg00797.html

This patch adds the ability to use temporary breakpoints,
and makes the default "permanent".  Thought it would be easy
to switch the default once all the various tests are updated.

This is only RFC as I'm happy to check it in, but it's not something
that "fixes" a bug or is currently useful.

2013-05-21  Doug Evans  <dje@google.com>

	* lib/gdb.exp (gdb_breakpoint): New option "permanent".
	(runto): Ditto.
	(runto_main): New argument "args".

Index: testsuite/lib/gdb.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/lib/gdb.exp,v
retrieving revision 1.231
diff -u -p -r1.231 gdb.exp
--- testsuite/lib/gdb.exp	6 May 2013 22:11:15 -0000	1.231
+++ testsuite/lib/gdb.exp	22 May 2013 00:44:01 -0000
@@ -336,14 +336,19 @@ proc gdb_start_cmd {args} {
 
 # Set a breakpoint at FUNCTION.  If there is an additional argument it is
 # a list of options; the supported options are allow-pending, temporary,
-# message, no-message, and passfail.
+# permanent, message, no-message, and passfail.
 # The result is 1 for success, 0 for failure.
 #
+# By default a permanent breakpoint is created.
+#
 # Note: The handling of message vs no-message is messed up, but it's based
 # on historical usage.  By default this function does not print passes,
 # only fails.
 # no-message: turns off printing of fails (and passes, but they're already off)
 # message: turns on printing of passes (and fails, but they're already on)
+#
+# If both temporary/permanent or message/no-message are specified,
+# the last one wins.
 
 proc gdb_breakpoint { function args } {
     global gdb_prompt
@@ -354,18 +359,20 @@ proc gdb_breakpoint { function args } {
 	set pending_response y
     }
 
-    set break_command "break"
-    set break_message "Breakpoint"
-    if {[lsearch -exact $args temporary] != -1} {
+    set temporary_loc [lsearch -exact $args temporary]
+    set permanent_loc [lsearch -exact $args permanent]
+    if { $temporary_loc > $permanent_loc } {
 	set break_command "tbreak"
 	set break_message "Temporary breakpoint"
+    } else {
+	set break_command "break"
+	set break_message "Breakpoint"
     }
 
     set print_pass 0
     set print_fail 1
     set no_message_loc [lsearch -exact $args no-message]
     set message_loc [lsearch -exact $args message]
-    # The last one to appear in args wins.
     if { $no_message_loc > $message_loc } {
 	set print_fail 0
     } elseif { $message_loc > $no_message_loc } {
@@ -430,6 +437,9 @@ proc gdb_breakpoint { function args } {
 # just compare to "function" because it might be a fully qualified,
 # single quoted C++ function specifier.
 #
+# By default a permanent breakpoint is used.
+# Override this by passing "temporary" in args.
+#
 # If there are additional arguments, pass them to gdb_breakpoint.
 # We recognize no-message/message ourselves.
 # The default is no-message.
@@ -444,8 +454,16 @@ proc runto { function args } {
 
     delete_breakpoints
 
-    # Default to "no-message".
-    set args "no-message $args"
+    # Default to "no-message" and "permanent".
+    set args "no-message permanent $args"
+
+    set temporary_loc [lsearch -exact $args temporary]
+    set permanent_loc [lsearch -exact $args permanent]
+    if { $temporary_loc > $permanent_loc } {
+	set break_message "Temporary breakpoint"
+    } else {
+	set break_message "Breakpoint"
+    }
 
     set print_pass 0
     set print_fail 1
@@ -474,13 +492,13 @@ proc runto { function args } {
     # the "at foo.c:36" output we get with -g.
     # the "in func" output we get without -g.
     gdb_expect 30 {
-	-re "Break.* at .*:$decimal.*$gdb_prompt $" {
+	-re "$break_message \[0-9\]*, .* at .*:$decimal.*$gdb_prompt $" {
 	    if { $print_pass } {
 		pass $test_name
 	    }
 	    return 1
 	}
-	-re "Breakpoint \[0-9\]*, \[0-9xa-f\]* in .*$gdb_prompt $" { 
+	-re "$break_message \[0-9\]*, \[0-9xa-f\]* in .*$gdb_prompt $" { 
 	    if { $print_pass } {
 		pass $test_name
 	    }
@@ -525,12 +543,13 @@ proc runto { function args } {
 }
 
 # Ask gdb to run until we hit a breakpoint at main.
+# args is the same as for runto.
 #
 # N.B. This function deletes all existing breakpoints.
 # If you don't want that, use gdb_start_cmd.
 
-proc runto_main { } {
-    return [runto main no-message]
+proc runto_main { args } {
+    return [runto main $args]
 }
 
 ### Continue, and expect to hit a breakpoint.

Re: [RFC] Support temporary breakpoints in runto_main.

[Joel Brobecker]

> If I imagine I'm new to gdb and see the name runto_main I can well
> imagine not expecting the implementation of that command to leave the
> breakpoint behind.  Leaving the breakpoint behind is not something I
> would expect from that name.

FWIW, the AdaCore's gdb-testsuite framework defines a "runto" method,
and it does set a temporary breakpoint by default (with the option
of making it permanent). Same reasoning as yours went into the "design"
of this method's interface.

Not sure if it's worth spending time changing this, but I would
welcome the change...

-- 
Joel

Re: [RFC] Support temporary breakpoints in runto_main.

[Doug Evans]

Doug Evans writes:
 > Hi.
 > 
 > If I imagine I'm new to gdb and see the name runto_main I can well
 > imagine not expecting the implementation of that command to leave the
 > breakpoint behind.
 > Leaving the breakpoint behind is not something I would expect from that name.
 > 
 > Thus to me I think the default for runto_main should be to use
 > a temporary breakpoint.  However, there are several tests that
 > have been written that assume it does.
 > 
 > And, there is one test that assumed it didn't: wp-replication.exp.
 > But that I fixed differently:
 > http://sourceware.org/ml/gdb-patches/2013-05/msg00797.html
 > 
 > This patch adds the ability to use temporary breakpoints,
 > and makes the default "permanent".  Thought it would be easy
 > to switch the default once all the various tests are updated.
 > 
 > This is only RFC as I'm happy to check it in, but it's not something
 > that "fixes" a bug or is currently useful.
 > 
 > 2013-05-21  Doug Evans  <dje@google.com>
 > 
 > 	* lib/gdb.exp (gdb_breakpoint): New option "permanent".
 > 	(runto): Ditto.
 > 	(runto_main): New argument "args".

Blech.  Gotta love tcl varargs.
Here's a revised patch.

2013-05-21  Doug Evans  <dje@google.com>

	* lib/gdb.exp (gdb_breakpoint): New option "permanent".
	(runto): Ditto.
	(runto_main): New argument "args".

Index: testsuite/lib/gdb.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/lib/gdb.exp,v
retrieving revision 1.231
diff -u -p -r1.231 gdb.exp
--- testsuite/lib/gdb.exp	6 May 2013 22:11:15 -0000	1.231
+++ testsuite/lib/gdb.exp	22 May 2013 07:18:52 -0000
@@ -336,14 +336,19 @@ proc gdb_start_cmd {args} {
 
 # Set a breakpoint at FUNCTION.  If there is an additional argument it is
 # a list of options; the supported options are allow-pending, temporary,
-# message, no-message, and passfail.
+# permanent, message, no-message, and passfail.
 # The result is 1 for success, 0 for failure.
 #
+# By default a permanent breakpoint is created.
+#
 # Note: The handling of message vs no-message is messed up, but it's based
 # on historical usage.  By default this function does not print passes,
 # only fails.
 # no-message: turns off printing of fails (and passes, but they're already off)
 # message: turns on printing of passes (and fails, but they're already on)
+#
+# If both temporary/permanent or message/no-message are specified,
+# the last one wins.
 
 proc gdb_breakpoint { function args } {
     global gdb_prompt
@@ -354,18 +359,20 @@ proc gdb_breakpoint { function args } {
 	set pending_response y
     }
 
-    set break_command "break"
-    set break_message "Breakpoint"
-    if {[lsearch -exact $args temporary] != -1} {
+    set temporary_loc [lsearch -exact $args temporary]
+    set permanent_loc [lsearch -exact $args permanent]
+    if { $temporary_loc > $permanent_loc } {
 	set break_command "tbreak"
 	set break_message "Temporary breakpoint"
+    } else {
+	set break_command "break"
+	set break_message "Breakpoint"
     }
 
     set print_pass 0
     set print_fail 1
     set no_message_loc [lsearch -exact $args no-message]
     set message_loc [lsearch -exact $args message]
-    # The last one to appear in args wins.
     if { $no_message_loc > $message_loc } {
 	set print_fail 0
     } elseif { $message_loc > $no_message_loc } {
@@ -430,6 +437,9 @@ proc gdb_breakpoint { function args } {
 # just compare to "function" because it might be a fully qualified,
 # single quoted C++ function specifier.
 #
+# By default a permanent breakpoint is used.
+# Override this by passing "temporary" in args.
+#
 # If there are additional arguments, pass them to gdb_breakpoint.
 # We recognize no-message/message ourselves.
 # The default is no-message.
@@ -444,8 +454,16 @@ proc runto { function args } {
 
     delete_breakpoints
 
-    # Default to "no-message".
-    set args "no-message $args"
+    # Default to "no-message" and "permanent".
+    set args "no-message permanent $args"
+
+    set temporary_loc [lsearch -exact $args temporary]
+    set permanent_loc [lsearch -exact $args permanent]
+    if { $temporary_loc > $permanent_loc } {
+	set break_message "Temporary breakpoint"
+    } else {
+	set break_message "Breakpoint"
+    }
 
     set print_pass 0
     set print_fail 1
@@ -474,13 +492,13 @@ proc runto { function args } {
     # the "at foo.c:36" output we get with -g.
     # the "in func" output we get without -g.
     gdb_expect 30 {
-	-re "Break.* at .*:$decimal.*$gdb_prompt $" {
+	-re "$break_message \[0-9\]*, .* at .*:$decimal.*$gdb_prompt $" {
 	    if { $print_pass } {
 		pass $test_name
 	    }
 	    return 1
 	}
-	-re "Breakpoint \[0-9\]*, \[0-9xa-f\]* in .*$gdb_prompt $" { 
+	-re "$break_message \[0-9\]*, \[0-9xa-f\]* in .*$gdb_prompt $" { 
 	    if { $print_pass } {
 		pass $test_name
 	    }
@@ -525,12 +543,13 @@ proc runto { function args } {
 }
 
 # Ask gdb to run until we hit a breakpoint at main.
+# args is the same as for runto.
 #
 # N.B. This function deletes all existing breakpoints.
 # If you don't want that, use gdb_start_cmd.
 
-proc runto_main { } {
-    return [runto main no-message]
+proc runto_main { args } {
+    return [eval runto main $args]
 }
 
 ### Continue, and expect to hit a breakpoint.

Re: [RFC] Support temporary breakpoints in runto_main.

[Pedro Alves]

On 05/22/2013 08:34 AM, Doug Evans wrote:

> 2013-05-21  Doug Evans  <dje@google.com>
> 
> 	* lib/gdb.exp (gdb_breakpoint): New option "permanent".
> 	(runto): Ditto.
> 	(runto_main): New argument "args".

This is fine with me, though it's a bit unfortunate that "permanent breakpoint"
has some other meaning in GDB.  It means a breakpoint already planted in the
target without GDB involvement, one that GDB can't insert or remove.
I can't imagine many options left, so I'd suggest "temporary" vs
"no-temporary"/"non-temporary"...

-- 
Pedro Alves