Re: [PATCH v6 7/9] Explicit locations: add UI features for CLI

[Doug Evans <xdje42 at gmail dot com>]

Keith Seitz <keiths@redhat.com> writes:
> Differences in this revision:
>
> 1. Add comma-handling in explicit_location_lex_one. Require whitespace
>    in options to be quoted.
> 2. Add tests for above.
> 3. if {0} completer tests that cause internal-error.
> 4. Change explicit_location_completer to use complete_on_enum.
> 5. Updated memory managemenet in string_to_explicit_location.
> 6. Add tests for explicit ranges.
> 7. My regexp fix for the capture_command_output has not been approved
>    upstream. It is included here to prevent regressions.
>
> --
>
> This patch exposes explicit locations to the CLI user.  This enables
> users to "explicitly" specify attributes of the breakpoint location
> to avoid any ambiguity that might otherwise exist with linespecs.
>
> The general syntax of explicit locations is:
> -source SOURCE_FILENAME -line {+-}LINE -function FUNCTION_NAME
> -label LABEL_NAME
>
> Option names may be abbreviated, e.g., "-s SOURCE_FILENAME -li 3" and users
> may use the completer with either options or values.
>
> gdb/ChangeLog:
>
> 	* completer.c: Include location.h.
> 	(enum match_type): New enum.
> 	(location_completer): Rename to ...
> 	(linespec_completer): ... this.
> 	(collect_explicit_location_matches, backup_text_ptr)
> 	(explicit_location_completer): New functions.
> 	(location_completer): "New" function; handle linespec
> 	and explicit location completions.
> 	(complete_line_internal): Remove all location completer-specific
> 	handling.
> 	* linespec.c (linespec_lexer_lex_keyword, is_ada_operator)
> 	(find_toplevel_char): Export.
> 	(linespec_parse_line_offset): Export.
> 	Issue error if STRING is not numerical.
> 	(gdb_get_linespec_parser_quote_characters): New function.
> 	* linespec.h (linespec_parse_line_offset): Declare.
> 	(get_gdb_linespec_parser_quote_characters): Declare.
> 	(is_ada_operator): Declare.
> 	(find_toplevel_char): Declare.
> 	(linespec_lexer_lex_keyword): Declare.
> 	* location.c (explicit_to_event_location): New function.
> 	(explicit_location_lex_one): New function.
> 	(string_to_explicit_location): New function.
> 	(string_to_event_location): Handle explicit locations.
> 	* location.h (explicit_to_event_location): Declare.
> 	(string_to_explicit_location): Declare.
>
> gdb/testsuite/ChangeLog:
>
> 	* gdb.linespec/3explicit.c: New file.
> 	* gdb.linespec/cpexplicit.cc: New file.
> 	* gdb.linespec/cpexplicit.exp: New file.
> 	* gdb.linespec/explicit.c: New file.
> 	* gdb.linespec/explicit.exp: New file.
> 	* gdb.linespec/explicit2.c: New file.
> 	* gdb.linespec/ls-errs.exp: Add explicit location tests.
> 	* lib/gdb.exp (capture_command_output): Regexp-escape `command'
> 	before using in the matching pattern.
> 	Clarify that `prefix' is a regular expression.

Hi.
Two nits.  Ok with those changes.
[no need to resubmit for review]

> ...
> diff --git a/gdb/location.c b/gdb/location.c
> index 7882b2d..1d35b6b 100644
> --- a/gdb/location.c
> +++ b/gdb/location.c
> @@ -442,6 +442,196 @@ event_location_to_string (struct event_location *location)
>    return EL_STRING (location);
>  }
>  
> +/* A lexer for explicit locations.  This function will advance INP
> +   past any strings that it lexes.  Returns a malloc'd copy of the
> +   lexed string or NULL if no lexing was done.  */
> +
> +static char *
> +explicit_location_lex_one (const char **inp,
> +			   const struct language_defn *language)
> +{
> +  const char *start = *inp;
> +
> +  if (*start == '\0')
> +    return NULL;
> +
> +  /* If quoted, skip to the ending quote.  */
> +  if (strchr (get_gdb_linespec_parser_quote_characters (), *start))
> +    {
> +      char quote_char = *start;
> +
> +      /* If the input is not an Ada operator, skip to the matching
> +	 closing quote and return the string.  */
> +      if (!(language->la_language == language_ada
> +	    && quote_char == '\"' && is_ada_operator (start)))
> +	{
> +	  const char *end = find_toplevel_char (start + 1, quote_char);
> +
> +	  if (end == NULL)
> +	    error (_("Unmatched quote, %s."), start);
> +	  *inp = end + 1;
> +	  return savestring (start + 1, *inp - start - 2);
> +	}
> +    }
> +
> +  /* If the input starts with '-' or '+', the string ends with the next
> +     whitespace or comma.  */
> +  if (*start == '-' || *start == '+')
> +    {
> +      while (*inp[0] != '\0' && *inp[0] != ',' && !isspace (*inp[0]))
> +	++(*inp);
> +    }
> +  else
> +    {
> +      /* Handle numbers first, stopping at the next whitespace or ','.  */
> +      while (isdigit (*inp[0]))
> +	++(*inp);
> +      if (*inp[0] == '\0' || isspace (*inp[0]) || *inp[0] == ',')
> +	return savestring (start, *inp - start);
> +
> +      /* Otherwise stop at the next occurrence of "SPACE -", '\0',
> +	 keyword, or ','.  */

This comment needs updating (right?).

> +      *inp = start;
> +      while ((*inp)[0]
> +	     && (*inp)[0] != ','
> +	     && !(isspace ((*inp)[0])
> +		  || linespec_lexer_lex_keyword (&(*inp)[1])))
> +	{
> +	  /* Special case: C++ operator,.  */
> +	  if (language->la_language == language_cplus
> +	      && strncmp (*inp, "operator", 8)
> +	      && (*inp)[9] == ',')
> +	    (*inp) += 9;
> +	  ++(*inp);
> +	}
> ...
> diff --git a/gdb/testsuite/gdb.linespec/explicit.exp b/gdb/testsuite/gdb.linespec/explicit.exp
> new file mode 100644
> index 0000000..414ec1c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.linespec/explicit.exp
> @@ -0,0 +1,410 @@
> +# Copyright 2012-2015 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Tests for explicit locations
> +
> +standard_testfile explicit.c explicit2.c 3explicit.c
> +set exefile $testfile
> +
> +if {[prepare_for_testing $testfile $exefile \
> +	 [list $srcfile $srcfile2 $srcfile3] {debug nowarnings}]} {
> +    return -1
> +}
> +
> +# Wrap the entire test in a namespace to avoid contaminating other tests.
> +namespace eval $testfile {
> +
> +    # Test the given (explicit) LINESPEC which should cause gdb to break
> +    # at LOCATION.
> +    proc test_breakpoint {linespec location} {
> +
> +	set testname "set breakpoint at \"$linespec\""
> +	# Delete all breakpoints, set a new breakpoint at LINESPEC,
> +	# and attempt to run to it.
> +	delete_breakpoints
> +	if {[gdb_breakpoint $linespec]} {
> +	    pass $testname
> +	    send_log "\nexpecting locpattern \"$location\"\n"
> +	    gdb_continue_to_breakpoint $linespec $location
> +	} else {
> +	    fail $testname
> +	}
> +    }
> +
> +    # Add the given LINESPEC to the array named in THEARRAY.  GDB is expected
> +    # to stop at LOCATION.
> +    proc add {thearray linespec location} {
> +	upvar $thearray ar
> +
> +	lappend ar(linespecs) $linespec
> +	lappend ar(locations) $location
> +    }
> +
> +    # A list of all explicit linespec arguments.
> +    variable all_arguments
> +    set all_arguments {"source" "function" "label" "line"}
> +
> +    # Some locations used in this test
> +    variable lineno
> +    variable location
> +    set lineno(normal) [gdb_get_line_number "myfunction location" $srcfile]
> +    set lineno(top) [gdb_get_line_number "top location" $srcfile]
> +    foreach v [array names lineno] {
> +	set location($v) ".*[string_to_regexp "$srcfile:$lineno($v)"].*"
> +    }
> +
> +    # A list of explicit locations and the corresponding location.
> +    variable linespecs
> +    set linespecs(linespecs) {}
> +    set linespecs(location) {}
> +
> +    add linespecs "-source $srcfile -function myfunction" $location(normal)
> +    add linespecs "-source $srcfile -function myfunction -label top" \
> +	$location(top)
> +
> +    # This isn't implemented yet; -line is silently ignored.
> +    add linespecs "-source $srcfile -function myfunction -label top -line 3" \
> +	$location(top)
> +    add linespecs "-source $srcfile -line $lineno(top)" $location(top)
> +    add linespecs "-function myfunction" $location(normal)
> +    add linespecs "-function myfunction -label top" $location(top)
> +
> +    # These are also not yet supported; -line is silently ignored.
> +    add linespecs "-function myfunction -line 3" $location(normal)
> +    add linespecs "-function myfunction -label top -line 3" $location(top)
> +    add linespecs "-line 3" $location(normal)
> +
> +    # Test that static tracepoints on marker ID are not interpreted
> +    # as an erroneous explicit option.
> +    gdb_test "strace -m gdbfoobarbaz" "You can't do that.*"
> +
> +    # Fire up gdb.
> +    if {![runto_main]} {
> +	return -1
> +    }
> +
> +    # Turn off queries
> +    gdb_test_no_output "set confirm off"
> +
> +    # Simple error tests (many more are tested in ls-err.exp)
> +    foreach arg $all_arguments {
> +	# Test missing argument
> +	gdb_test "break -$arg" \
> +	    [string_to_regexp "missing argument for \"-$arg\""]
> +
> +	# Test abbreviations
> +	set short [string range $arg 0 3]
> +	gdb_test "break -$short" \
> +	    [string_to_regexp "missing argument for \"-$short\""]
> +    }
> +
> +    # Test invalid arguments
> +    foreach arg {"-foo" "-foo bar" "-function myfunction -foo" \
> +		     "-function -myfunction -foo bar"} {
> +	gdb_test "break $arg" \
> +	    [string_to_regexp "invalid explicit location argument, \"-foo\""]
> +    }
> +
> +    # Test explicit locations, with and without conditions.
> +    # For these tests, it is easiest to turn of pending breakpoint.
> +    gdb_test_no_output "set breakpoint pending off" \
> +	"turn off pending breakpoints"
> +
> +    foreach linespec $linespecs(linespecs) loc_pattern $linespecs(locations) {
> +
> +	# Test the linespec
> +	test_breakpoint $linespec $loc_pattern
> +
> +	# Test with a valid condition
> +	delete_breakpoints
> +	set tst "set breakpoint at \"$linespec\" with valid condition"
> +	if {[gdb_breakpoint "$linespec if arg == 0"]} {
> +	    pass $tst
> +
> +	    gdb_test "info break" ".*stop only if arg == 0.*" \
> +		"info break of conditional breakpoint at \"$linespec\""
> +	} else {
> +	    fail $tst
> +	}
> +
> +	# Test with invalid condition
> +	gdb_test "break $linespec if foofoofoo == 1" \
> +	    ".*No symbol \"foofoofoo\" in current context.*" \
> +	    "set breakpoint at \"$linespec\" with invalid condition"
> +
> +	# Test with thread
> +	delete_breakpoints
> +	gdb_test "break $linespec thread 123" "Unknown thread 123."
> +    }
> +
> +    # Test the explicit location completer
> +    foreach abbrev {"fun" "so" "lab" "li"}  full {"function" "source" "label" "line"} {
> +	set tst "complete 'break -$abbrev'"
> +	send_gdb "break -${abbrev}\t"
> +	gdb_test_multiple "" $tst {
> +	    "break -$full " {
> +		send_gdb "\n"
> +		gdb_test_multiple "" $tst {
> +		    -re "missing argument for \"-$full\".*$gdb_prompt " {
> +			pass $tst
> +		    }
> +		}
> +	    }
> +	}
> +	set tst "complete -$full with no value"
> +	send_gdb "break -$full \t"
> +	gdb_test_multiple "" $tst {
> +	    -re ".*break -$full " {
> +		send_gdb "\n"
> +		gdb_test_multiple "" $tst {
> +		    -re ".*Source filename requires function, label, or line offset\..*$gdb_prompt " {
> +			if {[string equal $full "source"]} {
> +			    pass $tst
> +			} else {
> +			    faill $tst
> +			}
> +		    }
> +		    -re "missing argument for \"-$full\".*$gdb_prompt " {
> +			pass $tst
> +		    }
> +		}
> +	    }
> +	}
> +    }
> +
> +    set tst "complete unique function name"
> +    send_gdb "break -function mai\t"
> +    gdb_test_multiple "" $tst {
> +	"break -function mai\\\x07n" {
> +	    send_gdb "\n"
> +	    gdb_test "" ".*Breakpoint \[0-9\]+.*" $tst
> +	    gdb_test_no_output "delete \$bpnum" "delete $tst breakpoint"
> +	}
> +    }
> +
> +    set tst "complete non-unique function name"
> +    send_gdb "break -function myfunc\t"
> +    gdb_test_multiple "" $tst {
> +	"break -function myfunc\\\x07tion" {
> +	    send_gdb "\t\t"
> +	    gdb_test_multiple "" $tst {
> +		-re "\\\x07\r\nmyfunction\[ \t\]+myfunction2\[ \t\]+myfunction3\[ \t\]+myfunction4\[ \t\]+\r\n$gdb_prompt " {
> +		    gdb_test "2" ".*Breakpoint \[0-9\]+.*" $tst
> +		    gdb_test_no_output "delete \$bpnum" "delete $tst breakpoint"
> +		}
> +	    }
> +	}
> +    }
> +
> +    set tst "complete non-existant function name"
> +    send_gdb "break -function foo\t"
> +    gdb_test_multiple "" $tst {
> +	"break -function foo\\\x07" {
> +	    send_gdb "\t\t"
> +	    gdb_test_multiple "" $tst {
> +		-re "\\\x07\\\x07" {
> +		    send_gdb "\n"
> +		    gdb_test "" {Function "foo" not defined.} $tst
> +		}
> +	    }
> +	}
> +    }
> +
> +    set tst "complete unique file name"
> +    send_gdb "break -source 3ex\t"
> +    gdb_test_multiple "" $tst {
> +	"break -source 3explicit.c " {
> +	    send_gdb "\n"
> +	    gdb_test "" \
> +		{Source filename requires function, label, or line offset.} $tst
> +	}
> +    }
> +
> +    set tst "complete non-unique file name"
> +    send_gdb "break -source exp\t"
> +    gdb_test_multiple "" $tst {
> +	"break -source exp\\\x07licit" {
> +	    send_gdb "\t\t"
> +	    gdb_test_multiple "" $tst {
> +		-re "\\\x07\r\nexplicit.c\[ \t\]+explicit2.c\[ \t\]+\r\n$gdb_prompt" {
> +		    send_gdb "\n"
> +		    gdb_test "" \
> +			{Source filename requires function, label, or line offset.} \
> +			$tst
> +		}
> +	    }
> +	}
> +
> +	"break -source exp\\\x07l" {
> +	    # This pattern may occur when glibc debuginfo is installed.
> +	    send_gdb "\t\t"
> +	    gdb_test_multiple "" $tst {
> +		-re "\\\x07\r\nexplicit.c\[ \t\]+explicit2.c\[ \t\]+expl.*\r\n$gdb_prompt" {
> +		    send_gdb "\n"
> +		    gdb_test "" \
> +			{Source filename requires function, label, or line offset.} \
> +			$tst
> +		}
> +	    }
> +	}
> +    }
> +
> +    set tst "complete non-existant file name"
> +    send_gdb "break -source foo\t"
> +    gdb_test_multiple "" $tst {
> +	"break -source foo" {
> +	    send_gdb "\t\t"
> +	    gdb_test_multiple "" $tst {
> +		"\\\x07\\\x07" {
> +		    send_gdb "\n"
> +		    gdb_test "" \
> +			{Source filename requires function, label, or line offset.} \
> +			$tst
> +		}
> +	    }
> +	}
> +    }
> +
> +    # These tests are disabled pending resolution of gdb/17960.
> +    # They cannot be XFAILed because they cause an internal-error.
> +    if {0} {
> +    set tst "complete filename and unique function name"
> +    send_gdb "break -source explicit.c -function ma\t"
> +    gdb_test_multiple "" $tst {
> +	"break -source explicit.c -function main " {
> +	    send_gdb "\n"
> +	    gdb_test "" ".*Breakpoint .*" $tst
> +	    gdb_test_no_output "delete \$bpnum" "delete $tst breakpoint"
> +	}
> +    }
> +
> +    set tst "complete filename and non-unique function name"
> +    send_gdb "break -so 3explicit.c -func myfunc\t"
> +    gdb_test_multiple "" $tst {
> +	"break -so 3explicit.c -func myfunc\\\x07tion" {
> +	    send_gdb "\t\t"
> +	    gdb_test_multiple "" $tst {
> +		-re "\\\x07\r\nmyfunction3\[ \t\]+myfunction4\[ \t\]+\r\n$gdb_prompt " {
> +		    gdb_test "3" ".*Breakpoint \[0-9\]+.*" $tst
> +		    gdb_test_no_output "delete \$bpnum" "delete $tst breakpoint"
> +		}
> +	    }
> +	}
> +    }
> +
> +    set tst "complete filename and non-existant function name"
> +    send_gdb "break -sou 3explicit.c -fun foo\t"
> +    gdb_test_multiple "" $tst {
> +	"break -sou 3explicit.c -fun foo\\\x07" {
> +	    send_gdb "\t\t"
> +	    gdb_test_multiple "" $tst {
> +		"\\\x07\\\x07" {
> +		    send_gdb "\n"
> +		    gdb_test "" \
> +			{Function "foo" not defined in "3explicit.c".} $tst
> +		}
> +	    }
> +	}
> +    }
> +
> +    set tst "complete filename and function reversed"
> +    send_gdb "break -func myfunction4 -source 3ex\t"
> +    gdb_test_multiple "" $tst {
> +	"break -func myfunction4 -source 3explicit.c " {
> +	    send_gdb "\n"
> +	    gdb_test "" "Breakpoint \[0-9\]+.*" $tst
> +	    gdb_test_no_output "delete \$bpnum" "delete $tst breakpoint"
> +	}
> +    }
> +    }

I have checked in the temp fix for 17960 so the "if {0}" can be removed.

Thanks!