[RFA/ping] 12843

[RFA/ping] 12843

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

Hi,

A loooong time ago, we were discussing this bug, which essentially makes 
it impossible to (re)set a breakpoint on a linespec containing a 
filename with a Windows-style drive letter. This is especially bad since 
canonicalization may add this information.

Here's a link to the original discussion:
http://sourceware.org/ml/gdb-patches/2011-08/msg00473.html

I believe that only one correction was asked (by Jan) to fix-up an exec 
file name in the test suite, and I have fixed this.

I think this bug needs to be fixed for 7.4, so I humbly resubmit.

Tested against HEAD on x86_64-linux and i686-pc-cygwin (well, at least 
as best I could: there are a ton of problems with testing on cygwin -- a 
subject for some other time).

Although we may want to move linespecs.exp under gdb.linespec and rename 
it (to accommodate Tom's pending ambiguous linespec patch)?

Keith

ChangeLog
2011-11-10  Keith Seitz  <keiths@redhat.com>

	PR gdb/12843
	* linespec.c (locate_first_half): Keep ':' if it looks
	like it could be part of a Windows path starting with
	a drive letter.

testsuite/ChangeLog
2011-11-10  Keith Seitz  <keiths@redhat.com>

	PR gdb/12843
	* gdb.base/linespecs.exp: New file.

[-- Attachment #2: 12843-ping.patch --]
[-- Type: text/x-patch, Size: 2674 bytes --]

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 37ec368..721bf12 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -43,6 +43,7 @@
 #include "arch-utils.h"
 #include <ctype.h>
 #include "cli/cli-utils.h"
+#include "filenames.h"
 
 /* Prototypes for local functions.  */
 
@@ -1194,6 +1195,16 @@ locate_first_half (char **argptr, int *is_quote_enclosed)
 	  ++p;
 	}
     }
+
+
+  /* Check for a drive letter in the filename.  This is done on all hosts
+     to capture cross-compilation environments.  On Unixen, directory
+     separators are illegal in filenames, so if the user enters "e:/foo.c",
+     he is referring to a directory named "e:" and a source file named
+     "foo.c", and we still want to keep these two pieces together.  */
+  if (isalpha (p[0]) && p[1] == ':' && IS_DIR_SEPARATOR (p[2]))
+    p += 3;
+
   for (; *p; p++)
     {
       if (p[0] == '<')
@@ -1218,8 +1229,7 @@ locate_first_half (char **argptr, int *is_quote_enclosed)
          line, a tab, a colon or a space.  But if enclosed in double
 	 quotes we do not break on enclosed spaces.  */
       if (!*p
-	  || p[0] == '\t'
-	  || (p[0] == ':')
+	  || p[0] == '\t' || p[0] == ':'
 	  || ((p[0] == ' ') && !*is_quote_enclosed))
 	break;
       if (p[0] == '.' && strchr (p, ':') == NULL)
diff --git a/gdb/testsuite/gdb.base/linespecs.exp b/gdb/testsuite/gdb.base/linespecs.exp
new file mode 100644
index 0000000..559cdcc
--- /dev/null
+++ b/gdb/testsuite/gdb.base/linespecs.exp
@@ -0,0 +1,29 @@
+# Copyright 2011 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/>.
+
+# Linespec tests
+
+# We don't currently need our own test case for testing, so grab
+# another one.
+
+if {[prepare_for_testing linespecs.exp linespecs memattr.c]} {
+  return -1
+}
+
+# PR gdb/12843
+gdb_test "list c:/foo/bar/baz.c:1" "No source file named c:/foo/bar/baz.c."
+gdb_test "list c:/foo/bar/baz.c" "Function \"c:/foo/bar/baz.c\" not defined."
+gdb_test "list fooc:/foo/bar/baz.c:1" "No source file named fooc."
+gdb_test "list fooc:/foo/bar/baz.c" "No source file named fooc."

Re: [RFA/ping] 12843

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> Although we may want to move linespecs.exp under gdb.linespec and
Keith> rename it (to accommodate Tom's pending ambiguous linespec patch)?

Don't worry about it.
I will update my patch to do that.

Keith> 	PR gdb/12843
Keith> 	* linespec.c (locate_first_half): Keep ':' if it looks
Keith> 	like it could be part of a Windows path starting with
Keith> 	a drive letter.

Ok.

Tom

Re: [RFA/ping] 12843

[Joel Brobecker]

> -	  || p[0] == '\t'
> -	  || (p[0] == ':')
> +	  || p[0] == '\t' || p[0] == ':'

I wish reformatting changes like this were made separately.
Usually, they can even be checked in under the obvious rule...
In this case, not so sure - I like the previous formatting better,
because each branch of the multiple-or was on its own line.
But don't change it on my account, it's not terribly important.

-- 
Joel

Re: [RFA/ping] 12843

[Keith Seitz]

On 11/10/2011 10:12 AM, Joel Brobecker wrote:
>> -	  || p[0] == '\t'
>> -	  || (p[0] == ':')
>> +	  || p[0] == '\t' || p[0] == ':'
>
> I wish reformatting changes like this were made separately.
> Usually, they can even be checked in under the obvious rule...
> In this case, not so sure - I like the previous formatting better,
> because each branch of the multiple-or was on its own line.
> But don't change it on my account, it's not terribly important.
>

Agh! Jan noticed this, too, and I totally forgot about it. My guess is 
that I actually *did* have a change there at one time, but "reverted" it 
(to this reformatted entry).

I'll wipe that out.  I am also going to try to run the MinGW test suite 
on gdb before committing. That should be fun.

Keith

Re: [RFA/ping] 12843

[Keith Seitz]

On 11/10/2011 10:36 AM, Keith Seitz wrote:
> I am also going to try to run the MinGW test suite
> on gdb before committing. That should be fun.

Yup. "Fun." It looks like I still have quite a bit of mucking around 
until I can get this to work, but I'll keep at it here and there.

Nonetheless, I've committed all this patch. Thanks to Tom, Jan, and Joel 
for taking a peek.

Keith