Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303
@ 2016-01-08 18:44 Don Breazeal
  2016-01-08 23:03 ` Keith Seitz
  0 siblings, 1 reply; 6+ messages in thread
From: Don Breazeal @ 2016-01-08 18:44 UTC (permalink / raw)
  To: gdb-patches

This patch fixes an assertion failure that results from a problem
with handling a single colon in a linespec.  On a Windows host, using
the break command (or the MI request -break-insert) with the
"filename:linenumber" style linespec will cause an assertion failure if
the file doesn't exist, or isn't listed in the program's symbols, and
the filename contains a Windows logical volume, e.g. "C:".

One of the new tests in this patch demonstrated the failure like this:

Running ../../../binutils-gdb/gdb/testsuite/gdb.linespec/ls-errs-cp.exp ...
FAIL: gdb.linespec/ls-errs-cp.exp: break "spaces: and :colons.cc":3 (GDB internal error)
FAIL: gdb.linespec/ls-errs-cp.exp: break C:/nonexist-with-windrive.cc:3 (GDB internal error)

The assertion failure looked like this:

../../binutils-gdb/gdb/cp-namespace.c:252: internal-error: cp_search_static_and_baseclasses: Assertion `name[prefix_len + 1] == ':'' failed.

During GDB's parsing of the linespec, when the filename was not found in
the program's symbols, GDB tried to determine if the filename string
could be some other valid identifier.  Eventually it would get to a point
where it concluded that the Windows logical volume, or whatever was to the
left of the colon, was a C++ namespace.  When it found only one colon, it
would assert.

GDB's linespec grammar allows for several different linespecs that contain
':'.  The only one that has a number after the colon is filename:linenum.

With this patch, if we get a "file not found" error when looking the
filename up in the program's symbols, we look ahead in the linespec
being parsed to see if there is a number following the colon.  If there
is, we are guaranteed that the string preceding the colon is a filename.
In this case we can just throw an error and quit parsing the linespec,
since it is invalid.

The result of this is that GDB will set a pending breakpoint (as directed)
instead of terminating with an assertion failure.

There is another possible solution.  After no symbols are found for the
file and we determine that it is a filename:linenum style linespec, we
could just consume the filename token and parse the rest of the
linespec.  That works as well, but there is no advantage to it.

I created two new tests for this.  One is just gdb.linespec/ls-errs.exp
copied and converted to use C++ instead of C, and to add the Windows
logical drive case.  The other is an MI test to provide a regression
test for the specific case reported in PR 18303.

Tested on a Windows 7 x86_64 system with a PowerPC board running Linux,
and on an x86_64 system with both native and remote.

OK?
Thanks,
--Don

gdb/ChangeLog:
        PR breakpoints/18303
	* linespec.c (linespec_lexer_peek_second_token): New function.
	(parse_linespec): Call linespec_lexer_peek_second_token and
	throw error if sourcer file not found.

gdb/testsuite/ChangeLog:
	* gdb/testsuite/gdb.linespec/ls-errs-cp.cc: New test program.
	* gdb/testsuite/gdb.linespec/ls-errs-cp.exp: New test.
	* gdb/testsuite/gdb.mi/linespec-err-cp.cc: New test program.
	* gdb/testsuite/gdb.mi/linespec-err-cp.exp: New test.

---
 gdb/linespec.c                            |   31 ++++
 gdb/testsuite/gdb.linespec/ls-errs-cp.cc  |   36 +++++
 gdb/testsuite/gdb.linespec/ls-errs-cp.exp |  240 +++++++++++++++++++++++++++++
 gdb/testsuite/gdb.mi/linespec-err-cp.cc   |   35 ++++
 gdb/testsuite/gdb.mi/linespec-err-cp.exp  |   57 +++++++
 5 files changed, 399 insertions(+), 0 deletions(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 50951bd..668a85c 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -801,6 +801,26 @@ linespec_lexer_peek_token (linespec_parser *parser)
   return next;
 }
 
+/* Return the token-after-next without consuming either the current
+   token or the next token.  */
+
+static linespec_token
+linespec_lexer_peek_second_token (linespec_parser *parser)
+{
+  linespec_token next;
+  const char *saved_stream = PARSER_STREAM (parser);
+  linespec_token saved_token = parser->lexer.current;
+
+  /* Get the next token.  */
+  next = linespec_lexer_consume_token (parser);
+
+  /* Get the token after that.  */
+  next = linespec_lexer_consume_token (parser);
+  PARSER_STREAM (parser) = saved_stream;
+  parser->lexer.current = saved_token;
+  return next;
+}
+
 /* Helper functions.  */
 
 /* Add SAL to SALS.  */
@@ -2306,6 +2326,17 @@ parse_linespec (linespec_parser *parser, const char *arg)
 	}
       else
 	{
+	  /* Check to see if we have the case filename:linenum
+	     where the file was not found in the program symbols.
+	     Peek at the token after the ':'.  If it is a line
+	     number then we have that case, so propagate the error.  */
+	  token = linespec_lexer_peek_second_token (parser);
+	  if (token.type == LSTOKEN_NUMBER)
+	    {
+	      throw_error (file_exception.error,
+			   "%s", file_exception.message);
+	    }
+
 	  /* No symtabs found -- discard user_filename.  */
 	  xfree (user_filename);
 
diff --git a/gdb/testsuite/gdb.linespec/ls-errs-cp.cc b/gdb/testsuite/gdb.linespec/ls-errs-cp.cc
new file mode 100644
index 0000000..a3a43db
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/ls-errs-cp.cc
@@ -0,0 +1,36 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012-2016 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/>.  */
+
+int
+myfunction (int aa)
+{
+  int i;
+
+  i = aa + 42;
+  return i;    /* set breakpoint here */
+}
+
+int
+main (void)
+{
+  int a;
+
+  a = myfunction (a);
+
+ here:
+  return a;
+}
diff --git a/gdb/testsuite/gdb.linespec/ls-errs-cp.exp b/gdb/testsuite/gdb.linespec/ls-errs-cp.exp
new file mode 100644
index 0000000..be1c3ba
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/ls-errs-cp.exp
@@ -0,0 +1,240 @@
+# Copyright 2012-2016 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 linespec errors with C++.
+# Derived from gdb.linespec/ls-errs.exp.
+
+if { [skip_cplus_tests] } { continue }
+
+standard_testfile .cc
+set exefile $testfile
+
+if {[prepare_for_testing $testfile $exefile $srcfile {debug c++}]} {
+    return -1
+}
+
+# Turn off the pending breakpoint queries.
+gdb_test_no_output "set breakpoint pending off"
+
+# Turn off completion limiting
+gdb_test_no_output "set max-completions unlimited"
+
+if ![runto_main] {
+    fail "Can't run to main"
+    return 0
+}
+
+# Run to a location in the file.
+set bp_location [gdb_get_line_number "set breakpoint here"]
+
+gdb_test "break $srcfile:$bp_location" \
+    "Breakpoint.*at.* file .*$srcfile, line $bp_location\\." \
+    "breakpoint line number in file"
+
+gdb_continue_to_breakpoint "$bp_location"
+
+# We intentionally do not use gdb_breakpoint for these tests.
+
+# Break at 'linespec' and expect the message in ::error_messages indexed by
+# msg_id with the associated args.
+proc test_break {linespec msg_id args} {
+    global error_messages
+
+    gdb_test "break $linespec" [string_to_regexp \
+				[eval format \$error_messages($msg_id) $args]]
+}
+
+# Common error message format strings.
+array set error_messages {
+    invalid_file "No source file named %s."
+    invalid_function "Function \"%s\" not defined."
+    invalid_var_or_func "Undefined convenience variable or function \"%s\" not defined."
+    invalid_function_f "Function \"%s\" not defined in \"%s\"."
+    invalid_var_or_func_f \
+	"Undefined convenience variable or function \"%s\" not defined in \"%s\"."
+    invalid_label "No label \"%s\" defined in function \"%s\"."
+    invalid_parm "invalid linespec argument, \"%s\""
+    invalid_offset "No line %d in the current file."
+    invalid_offset_f "No line %d in file \"%s\"."
+    malformed_line_offset "malformed line offset: \"%s\""
+    source_incomplete \
+	"Source filename requires function, label, or line offset."
+    unexpected "malformed linespec error: unexpected %s"
+    unexpected_opt "malformed linespec error: unexpected %s, \"%s\""
+    unmatched_quote "unmatched quote"
+    garbage "Garbage '%s' at end of command"
+}
+
+# Some commonly used whitespace tests around ':'.
+set spaces [list ":" ": " " :" " : " "\t:  " "  :\t" "\t:\t" " \t:\t " \
+		"\t  \t:\t  \t  \t"]
+
+# A list of invalid offsets.
+set invalid_offsets [list -100 +500 1000]
+
+# Try some simple, invalid linespecs involving spaces.
+foreach x $spaces {
+    test_break $x unexpected "colon"
+}
+
+# Test invalid filespecs starting with offset.  This is done
+# first so that default offsets are tested.
+foreach x $invalid_offsets {
+    set offset $x
+
+    # Relative offsets are relative to line 16.  Adjust
+    # expected offset from error message accordingly.
+    if {[string index $x 0] == "+" ||
+	[string index $x 0] == "-"} {
+	incr offset 24
+    }
+    test_break $x invalid_offset $offset
+    test_break "-line $x" invalid_offset $offset
+}
+
+# Test offsets with trailing tokens w/ and w/o spaces.
+foreach x $spaces {
+    test_break "3$x" unexpected "colon"
+    test_break "+10$x" unexpected "colon"
+    test_break "-10$x" unexpected "colon"
+}
+
+foreach x {1 +1 +100 -10} {
+    test_break "3 $x" unexpected_opt "number" $x
+    test_break "-line 3 $x" garbage $x
+    test_break "+10 $x" unexpected_opt "number" $x
+    test_break "-line +10 $x" garbage $x
+    test_break "-10 $x" unexpected_opt "number" $x
+    test_break "-line -10 $x" garbage $x
+}
+
+foreach x {3 +10 -10} {
+    test_break "$x foo" unexpected_opt "string" "foo"
+    test_break "-line $x foo" garbage "foo"
+}
+
+# Test invalid linespecs starting with filename.
+set invalid_files [list "this_file_doesn't_exist.cc" \
+		   "this file has spaces.cc" \
+	           "\"file::colons.cc\"" \
+	           "'file::colons.cc'" \
+	           "\"this \"file\" has quotes.cc\"" \
+	           "'this \"file\" has quotes.cc'" \
+	           "'this 'file' has quotes.cc'" \
+	           "\"this 'file' has quotes.cc\"" \
+	           "\"spaces: and :colons.cc\"" \
+	           "'more: :spaces: :and  colons::.cc'" \
+		   "C:/nonexist-with-windrive.cc"]
+
+foreach x $invalid_files {
+    # Remove any quoting from FILENAME for the error message.
+    test_break "$x:3" invalid_file [string trim $x \"']
+}
+foreach x [list "this_file_doesn't_exist.cc" \
+	       "file::colons.cc" \
+	       "'file::colons.cc'"] {
+    test_break "-source $x -line 3" \
+	invalid_file [string trim $x \"']
+}
+
+# Test that option lexing stops at whitespace boundaries
+test_break "-source this file has spaces.cc -line 3" \
+    invalid_file "this"
+
+test_break "-function function whitespace" \
+    invalid_function "function"
+
+test_break "-source $srcfile -function function whitespace" \
+    invalid_function_f "function" $srcfile
+
+test_break "-function main -label label whitespace" \
+    invalid_label "label" "main"
+
+# Test unmatched quotes.
+foreach x {"\"src-file.cc'" "'src-file.cc"} {
+    test_break "$x:3" unmatched_quote
+}
+
+test_break $srcfile invalid_function $srcfile
+foreach x {"foo" " foo" " foo "} {
+    # Trim any leading/trailing whitespace for error messages.
+    test_break "$srcfile:$x" invalid_function_f [string trim $x] $srcfile
+    test_break "-source $srcfile -function $x" \
+	invalid_function_f [string trim $x] $srcfile
+    test_break "$srcfile:main:$x" invalid_label [string trim $x] "main"
+    test_break "-source $srcfile -function main -label $x" \
+	invalid_label [string trim $x] "main"
+}
+
+foreach x $spaces {
+    test_break "$srcfile$x" unexpected "end of input"
+    test_break "$srcfile:main$x" unexpected "end of input"
+}
+
+test_break "${srcfile}::" invalid_function "${srcfile}::"
+test_break "$srcfile:3 1" unexpected_opt "number" "1"
+test_break "-source $srcfile -line 3 1" garbage "1"
+test_break "$srcfile:3 +100" unexpected_opt "number" "+100"
+test_break "-source $srcfile -line 3 +100" garbage "+100"
+test_break "$srcfile:3 -100" unexpected_opt "number" "-100"
+test_break "$srcfile:3 foo" unexpected_opt "string" "foo"
+test_break "-source $srcfile -line 3 foo" garbage "foo"
+
+foreach x $invalid_offsets {
+    test_break "$srcfile:$x" invalid_offset_f $x $srcfile
+    test_break "\"$srcfile:$x\"" invalid_offset_f $x $srcfile
+    test_break "'$srcfile:$x'" invalid_offset_f $x $srcfile
+    test_break "-source $srcfile -line $x" invalid_offset_f $x $srcfile
+}
+test_break "-source $srcfile -line -x" malformed_line_offset "-x"
+
+# Test invalid filespecs starting with function.
+foreach x {"foobar" "foo::bar" "foo.bar" "foo ." "foo bar" "foo 1" \
+	       "foo 0" "foo +10" "foo -10" "foo +100" "foo -100"} {
+    test_break $x invalid_function $x
+    test_break "-function \"$x\"" invalid_function $x
+}
+
+foreach x $spaces {
+    test_break "main${x}there" invalid_label "there" "main"
+    if {[test_compiler_info {clang-*-*}]} {  setup_xfail clang/14500 *-*-* }
+    test_break "main:here${x}" unexpected "end of input"
+}
+
+foreach x {"3" "+100" "-100" "foo"} {
+    test_break "main 3" invalid_function "main 3"
+    test_break "-function \"main $x\"" invalid_function "main $x"
+    test_break "main:here $x" invalid_label "here $x" "main"
+    test_break "-function main -label \"here $x\"" \
+	invalid_label "here $x" "main"
+}
+
+foreach x {"if" "task" "thread"} {
+    test_break $x invalid_function $x
+}
+
+test_break "'main.cc'flubber" unexpected_opt "string" "flubber"
+test_break "'main.cc',21" invalid_function "main.cc"
+test_break "'main.cc' " invalid_function "main.cc"
+test_break "'main.cc'3" unexpected_opt "number" "3"
+test_break "'main.cc'+3" unexpected_opt "number" "+3"
+
+# Test undefined convenience variables.
+set x {$zippo}
+test_break $x invalid_var_or_func $x
+test_break "$srcfile:$x" invalid_var_or_func_f $x $srcfile
+
+# Explicit linespec-specific tests
+test_break "-source $srcfile" source_incomplete
diff --git a/gdb/testsuite/gdb.mi/linespec-err-cp.cc b/gdb/testsuite/gdb.mi/linespec-err-cp.cc
new file mode 100644
index 0000000..19c92d8
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/linespec-err-cp.cc
@@ -0,0 +1,35 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 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/>.  */
+
+int
+myfunction (int aa)
+{
+  int i;
+
+  i = aa + 42;
+  return i;    /* set breakpoint here */
+}
+
+int
+main (void)
+{
+  int a;
+
+  a = myfunction (a);
+
+  return a;
+}
diff --git a/gdb/testsuite/gdb.mi/linespec-err-cp.exp b/gdb/testsuite/gdb.mi/linespec-err-cp.exp
new file mode 100644
index 0000000..1a8682e
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/linespec-err-cp.exp
@@ -0,0 +1,57 @@
+# Copyright 2016 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/>.
+
+# Regression test for PR breakpoints/18303.  Tests that the correct
+# errors is generated when setting a breakpoint in a non-existent
+# file with a Windows-style logical drive names and C++.
+
+if { [skip_cplus_tests] } { continue }
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+standard_testfile .cc
+set exefile $testfile
+
+if {[prepare_for_testing $testfile $exefile $srcfile {debug c++}]} {
+    return -1
+}
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+# Turn off the pending breakpoint queries.
+mi_gdb_test "-interpreter-exec console \"set breakpoint pending off\"" \
+  {=cmd-param-changed,param=\"breakpoint pending\",.*\^done} \
+  "-interpreter-exec console \"set breakpoint pending off\""
+
+mi_run_to_main
+
+# Run to a location in the file.
+set bp_location [gdb_get_line_number "set breakpoint here"]
+
+mi_gdb_test "-break-insert ${srcfile}:${bp_location}" \
+    {\^done,bkpt=.number="2",type="breakpoint".*\}} "set breakpoint"
+
+mi_execute_to "exec-continue" "breakpoint-hit" "myfunction" ".*" ".*" "24" \
+    { "" "disp=\"keep\"" } "breakpoint hit"
+
+# Set a breakpoint in a C++ source file whose name contains a
+# Windows-style logical drive.
+mi_gdb_test \
+    "-break-insert -f \"c:/uu.cpp:13\"" \
+    ".*No source file named c:/uu.cpp.*"
-- 
1.7.0.4


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303
@ 2016-01-11 22:34 Doug Evans
  2016-01-12  0:17 ` Don Breazeal
  2016-01-12 18:26 ` Keith Seitz
  0 siblings, 2 replies; 6+ messages in thread
From: Doug Evans @ 2016-01-11 22:34 UTC (permalink / raw)
  To: Keith Seitz; +Cc: donb, gdb-patches@sourceware.org ml

Keith Seitz writes:
  > Hi,
  >
  > Thank you for pointing this out and supplying a patch (and *tests*!).
  >
  > On 01/08/2016 10:44 AM, Don Breazeal wrote:
  >
  > > During GDB's parsing of the linespec, when the filename was not found  
in
  > > the program's symbols, GDB tried to determine if the filename string
  > > could be some other valid identifier.  Eventually it would get to a  
point
  > > where it concluded that the Windows logical volume, or whatever was to  
the
  > > left of the colon, was a C++ namespace.  When it found only one colon,  
it
  > > would assert.
  >
  > I have to admit, when I first read this, my reaction was that this is a
  > symbol lookup error. After investigating it a bit, I think it is. [More
  > rationale below.]
  >
  > > GDB's linespec grammar allows for several different linespecs that  
contain
  > > ':'.  The only one that has a number after the colon is  
filename:linenum.
  >
  > True, but for how long? I don't know. The parser actually does/could (or
  > for some brief time *did*) support offsets just about anywhere, e.g.,
  > foo.c:function:label:3. That support was removed and legacy (buggy)
  > behavior of ignoring the offset was maintained in the parser/sal
  > converter. There is no reason we couldn't support it, though.
  >
  > > There is another possible solution.  After no symbols are found for the
  > > file and we determine that it is a filename:linenum style linespec, we
  > > could just consume the filename token and parse the rest of the
  > > linespec.  That works as well, but there is no advantage to it.
  >
  > And, of course, I came up with an entirely different solution. :-)
  >
  > Essentially what is happening is that the linespec parser is calling
  > lookup_symbol on the user input (e.g., "spaces: and :colons.cc"). That
  > is causing several problems, all which assume the input is well-formed.
  > As you've discovered, that is a pretty poor assumption. Malformed (user)
  > input should not cause an assertion failure IMO.
  >
  > > I created two new tests for this.  One is just gdb.linespec/ls-errs.exp
  > > copied and converted to use C++ instead of C, and to add the Windows
  > > logical drive case.  The other is an MI test to provide a regression
  > > test for the specific case reported in PR 18303.
  >
  > EXCELLENT! Thank you so much for doing this. These tests were
  > outrageously useful while attempting to understand the problem.
  >
  > With that said, I offer *for discussion* this alternative solution,
  > which removes the assumption that input to lookup_symbol is/must be
  > well-formed.
  >
  > [There is one additional related/necessary fixlet snuck in here... The
  > C++ name parser always assumed that ':' was followed by another ':'. Bad
  > assumption. So it would return an incorrect result in the malformed  
case.]
  >
  > WDYT?
  >
  > Keith
  >
  > [apologies on mailer wrapping]
  >
  > Author: Keith Seitz <keiths@redhat.com>
  > Date:   Fri Jan 8 14:00:22 2016 -0800
  >
  > Tolerate malformed input for lookup_symbol-called functions.
  >
  > lookup_symbol is often called with user input. Consequently, any
  > function called from lookup_symbol{,_in_language} should attempt to deal
  > with malformed input gracefully. After all, malformed user input is
  > not a programming/API error.
  >
  > This patch does not attempt to find/correct all instances of this.
  > It only fixes locations in the code that trigger test suite failures.
  >
  >     gdb/ChangeLog
  >
  >     	* cp-namespace.c (cp_lookup_bare_symbol): Do not assert on
  >     	user input.
  >     	(cp_search_static_and_baseclasses): Return null_block_symbol for
  >     	malformed input.
  >     	Remove assertions.
  >     	* cp-support.c (cp_find_first_component_aux): Do not return
  >     	a prefix length for ':' unless the next character is also ':'.
  >
  > diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
  > index 72002d6..aa225fe 100644
  > --- a/gdb/cp-namespace.c
  > +++ b/gdb/cp-namespace.c
  > @@ -166,12 +166,6 @@ cp_lookup_bare_symbol (const struct language_defn
  > *langdef,
  >  {
  >    struct block_symbol sym;
  >
  > -  /* Note: We can't do a simple assert for ':' not being in NAME because
  > -     ':' may be in the args of a template spec.  This isn't intended to  
be
  > -     a complete test, just cheap and documentary.  */
  > -  if (strchr (name, '<') == NULL && strchr (name, '(') == NULL)
  > -    gdb_assert (strchr (name, ':') == NULL);
  > -

Heya.

The assert is intended to catch (some) violations of this
(from the function comment):

    NAME is guaranteed to not have any scope (no "::") in its name, though
    if for example NAME is a template spec then "::" may appear in the
    argument list.

This is an invariant on what name can (and cannot) be.
IOW, it is wrong to call this function with name == "foo::bar",
handling that is the caller's responsibility.
Thus I think having an assert here is ok and good
(as long as it tests for the correct thing!).

Whether it is ok to call this with name == "c:mumble" is the issue.
[or even "c:::mumble" or whatever else a user could type that
this function's caller isn't expected to handle]
On that I'm kinda ambivalent, but I like having the assert
watch for the stated invariant.

Thoughts?

  >    sym = lookup_symbol_in_static_block (name, block, domain);
  >    if (sym.symbol != NULL)
  >      return sym;
  > @@ -246,10 +240,9 @@ cp_search_static_and_baseclasses (const char *name,
  >    struct block_symbol klass_sym;
  >    struct type *klass_type;
  >
  > -  /* The test here uses <= instead of < because Fortran also uses this,
  > -     and the module.exp testcase will pass "modmany::" for NAME here.   
*/
  > -  gdb_assert (prefix_len + 2 <= strlen (name));
  > -  gdb_assert (name[prefix_len + 1] == ':');
  > +  /* Check for malformed input.  */
  > +  if (prefix_len + 2 > strlen (name) || name[prefix_len + 1] != ':')
  > +    return null_block_symbol;
  >
  >    /* Find the name of the class and the name of the method, variable,
  > etc.  */
  >
  > diff --git a/gdb/cp-support.c b/gdb/cp-support.c
  > index df127c4..a71c6ad 100644
  > --- a/gdb/cp-support.c
  > +++ b/gdb/cp-support.c
  > @@ -1037,8 +1037,13 @@ cp_find_first_component_aux (const char *name,
  > int permissive)
  >  	      return strlen (name);
  >  	    }
  >  	case '\0':
  > -	case ':':
  >  	  return index;
  > +	case ':':
  > +	  /* ':' marks a component iff the next character is also a ':'.
  > +	     Otherwise it is probably malformed input.  */
  > +	  if (name[index + 1] == ':')
  > +	    return index;
  > +	  break;

What if name[index+2] is also ':'? :-)

[btw, I think "a::::b" is illegal (note four colons),
but I don't know that for sure]

  >  	case 'o':
  >  	  /* Operator names can screw up the recursion.  */
  >  	  if (operator_possible
  >

-- 
/dje


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-12 18:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-08 18:44 [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303 Don Breazeal
2016-01-08 23:03 ` Keith Seitz
2016-01-11 19:21   ` Don Breazeal
2016-01-11 22:34 Doug Evans
2016-01-12  0:17 ` Don Breazeal
2016-01-12 18:26 ` Keith Seitz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox