Re: [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303

Don Breazeal
2016-01-11T19:21:36Z

On 1/8/2016 3:03 PM, Keith Seitz wrote:
> Hi,
>
> Thank you for pointing this out and supplying a patch (and *tests*!).

Thanks for the review.

>
> 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.

My thinking was that if there were changes to the linespec grammar it
would be legit to have to change the parser.

>
>> 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. :-)

...that covers more cases.

>
> 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.

I wrote up another simple test that tries to use the "non-existent
windows file with a logical drive in its name" when accessing a
variable, and your solution fixes that case while mine doesn't.

Yours:
(gdb) print 'C:/does/not/exist.cc'::var
No symbol "C:/does/not/exist.cc" in current context.

Mine:
(gdb) print 'C:/does/not/exist.cc'::var
../../binutils-gdb/gdb/cp-namespace.c:252: internal-error:
cp_search_static_and_baseclasses: Assertion `name[prefix_len + 1] ==
':'' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.

The handling of the bad linespec by both our patches is identical from
the user perspective.

Seems like no further discussion is needed; your approach is the one to use.

>
> [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?

FWIW your changes make sense to me. Do you want to proceed with your
patch, and I'll revise mine so it only includes the tests?

Thanks!
--Don

Test use of filename with Windows-style logical drive as the
scope of a variable.
---
gdb/testsuite/gdb.cp/file-err.cc | 35 +++++++++++++++++++++++++++++
gdb/testsuite/gdb.cp/file-err.exp | 47
+++++++++++++++++++++++++++++++++++++++
2 files changed, 82 insertions(+)

diff --git a/gdb/testsuite/gdb.cp/file-err.cc
b/gdb/testsuite/gdb.cp/file-err.cc
new file mode 100644
index 0000000..19c92d8
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/file-err.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.cp/file-err.exp
b/gdb/testsuite/gdb.cp/file-err.exp
new file mode 100644
index 0000000..9d93578
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/file-err.exp
@@ -0,0 +1,47 @@
+# 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
+}
+
+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"
+
+# Try to access a variable using scope that is a non-existent filename
+# with a Windows-style logical drive in the name.
+set nonexistent_file C:/does/not/exist.cc
+gdb_test "print '$nonexistent_file'::var" \
+ ".*No symbol \"$nonexistent_file\" in current context.*" \
+ "print var from \"$nonexistent_file\""
--
1.8.1.1