Re: [RFA 3/3] gdb_breakpoint cleanup: Add support for drpintf, trace, ftrace

[Pedro Alves <palves at redhat dot com>]

On 04/30/2013 07:00 PM, Keith Seitz wrote:
> On 04/29/2013 11:17 AM, Pedro Alves wrote:
>> Sorry to be picky, but this "e.g." confuses me.  What other examples
>> could there be?  Maybe you meant "i.e." ?  (Though that doesn't
>> seem to add anything over what the option name itself tells us?)
> 
> You're right. I shouldn't make the same mistake that is too often made: our OWN developers aren't stupid; when a fast tracepoint is mentioned, they (should) know that the procedure is talking about the gdb command "ftrace".

Yeah.

The way I've always looked at "temporary", the existing "e.g." it has
in its description made some sense, since we have more than one breakpoint
type that supports being set as temporary.  E.g., "thbreak", "tcatch".
We don't have temporary watchpoints, though we could (or could add
support in the CLI for setting all kinds of different breakpoints
as temporary. with an option, e.g., "watch -temporary", instead
of adding more commands).  I now understand you're looking at "temporary"
as a type, not a flag.

> Ok, as a pretty old-time Tcl developer, I would offer the following alternative: Introduce options and optional arguments. In Tcl, this is almost trivial to do.
> 
> Currently, gdb_breakpoint just searches for well-known flags. With a small change, we could allow usage like:
> 
> # Set the default type (breakpoint), using a default test name
> gdb_breakpoint "main"
> 
> # Set a temporary breakpoint, using a default test name
> gdb_breakpoint "main" -type temporary

I think it'd be nicer, as in, less cognitive load, that either
"temporary" be considered a separate flag "-temporary"; or, if it
is to be treated as a type, then that it be renamed "-type tbreak".
WDYT?

> # Set a pending tracepoint with a given test name
> gdb_breakpoint "main" -pending -type trace \
>     -message "set pending breakpoint"
> 
> and so on. We've been using this style of API in Insight/libgui for a very long time.
> 
> What do you think?

I like that.

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 827295b..5d1b808 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -339,10 +339,13 @@ proc gdb_start_cmd {args} {
>  #
>  # Supported options are:
>  # allow-pending -- permit a pending breakpoint to be set
> -# temporary -- set a temporary breakpoint, e.g., "tbreak"
> +# temporary -- set a temporary breakpoint
>  # message -- print PASS messages*
>  # no-message -- do not print FAIL messages*
>  # pending -- set a pending breakpoint (FAIL if it is not pending)
> +# trace -- set a tracepoint
> +# ftrace -- set a fast tracepoint
> +# dprintf -- set a dynamic printf
>  #
>  # The result is 1 for success, 0 for failure.
>  #
> @@ -365,12 +368,26 @@ proc gdb_breakpoint { function args } {
>  	set pending_response "y"
>      }
>  

> -    if {[lsearch -exact $args temporary] != -1} {
> -	set break_command "tbreak"
> -	set break_message "Temporary breakpoint"
...
> +    } elseif {[lsearch -exact $args "temporary"] != -1} {
> +	set break_command "break"
> +	set break_message "Breakpoint"
> +	set type "breakpoint"

This lost the "t".  Should be "tbreak".

Otherwise OK.

Thanks,
-- 
Pedro Alves