[RFA] 12843

[RFA] 12843

[Keith Seitz]

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

Hi,

A long time ago, some linespec hacking that I did broke the ability to 
use drive letters on Windows. This turned out especially bad since 
linespec canonicalization could unexpectedly (to the user) include them. 
As a result, gdb was/is unable to re-set some breakpoints on Windows hosts.

This is a pretty simple patch to workaround this problem. It is not 
perfect, but implementing something "proper" would be very involved, and 
I am not quite sure I want to commit to hacking more of linespec.c 
without rewriting the whole thing (which would certainly interfer with 
Tom's ambiguous linespec work).

So I'm not sure whether this should be an RFC or and RFA, but I've 
decided the later.

This patch checks if ':' is followed by '/' and/or '\\' (depending on 
definition of IS_DIR_SEPARATOR). If it is, we keep looking for the next 
terminal in the string. This is problematic for filenames with ":\\" or 
"://" in them, but locate_first_half has gotten those wrong for a long, 
long time (in fact, any filename at all with a colon(s) in it).

In any case, this patch will allow Windows hosts to function properly 
again, and causes no regressions on x86_64-linux and i686-pc-cygwin32.

Keith

ChangeLog
2011-08-25  Keith Seitz  <keiths@redhat.com>

	PR gdb/12843
	* linespec.c (locate_first_half): Do not stop on a colon
	if the next character is a directory separator character.

testsuite/ChangeLog
2011-08-25  Keith Seitz  <keiths@redhat.com>

	PR gdb/12843
	* gdb.base/ls-driveletter.exp: New file.

[-- Attachment #2: 12843.patch --]
[-- Type: text/plain, Size: 2399 bytes --]

diff --git a/gdb/linespec.c b/gdb/linespec.c
index b96c79f..b35ac6a 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.  */
 
@@ -1216,10 +1217,16 @@ locate_first_half (char **argptr, int *is_quote_enclosed)
 	}
       /* Check for the end of the first half of the linespec.  End of
          line, a tab, a colon or a space.  But if enclosed in double
-	 quotes we do not break on enclosed spaces.  */
+	 quotes we do not break on enclosed spaces.
+
+	 Colons are especially problematic, since we don't really want to stop
+	 if the colon is part of a filename.  The simple heuristic used here
+	 is to keep going if the colon is followed by a directory separator.
+	 This will capture drive letters on Windows, but it will (still) fail
+	 for any filename containing ":", "::", ":/", or ":\\".  */
       if (!*p
 	  || p[0] == '\t'
-	  || (p[0] == ':')
+	  || (p[0] == ':' && !IS_DIR_SEPARATOR (p[1]))
 	  || ((p[0] == ' ') && !*is_quote_enclosed))
 	break;
       if (p[0] == '.' && strchr (p, ':') == NULL)
diff --git a/gdb/testsuite/gdb.base/ls-driveletter.exp b/gdb/testsuite/gdb.base/ls-driveletter.exp
new file mode 100644
index 0000000..66efd1f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/ls-driveletter.exp
@@ -0,0 +1,26 @@
+# 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/>.
+
+# Test linespecs containing drive letters.
+# PR gdb/12843
+
+# We don't actually need our own test case to test this, so grab
+# another one.
+
+if {[prepare_for_testing ls-driveletter.exp memattr memattr.c {debug}]} {
+  return -1
+}
+
+gdb_test "list c:/foo/bar/baz.c:1" "No source file named c:/foo/bar/baz.c."

Re: [RFA] 12843

[Tom Tromey]

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

Keith> 	PR gdb/12843
Keith> 	* linespec.c (locate_first_half): Do not stop on a colon
Keith> 	if the next character is a directory separator character.

Ok.

I suspect we should tighten this further so that only drive letters work
and not oddball stuff like "break file:/whatever.c:73".  What do you
think?

Tom

Re: [RFA] 12843

[Keith Seitz]

On 08/26/2011 11:22 AM, Tom Tromey wrote:

> I suspect we should tighten this further so that only drive letters work
> and not oddball stuff like "break file:/whatever.c:73".  What do you
> think?

I can play with that a little further. I'll also take the time to add a 
bunch of tests to highlight some of the problems. There is an open PR 
about this, to. In general the use of ':' or "::" in filenames is really 
bad for linespecs.

Keith

Re: [RFA] 12843

[Tom Tromey]

Keith> I can play with that a little further. I'll also take the time to add
Keith> a bunch of tests to highlight some of the problems. There is an open
Keith> PR about this, to. In general the use of ':' or "::" in filenames is
Keith> really bad for linespecs.

My view is that, aside from the drive-letter case, we should require
funny file names to be quoted.  That is:

Valid:    break 'file with spaces.c':73
Invalid:  break file with spaces.c:73

Valid:    break 'file:with:colons.c':73
Invalid:  break file:with:colons.c:73

Valid either way:
          break c:/file.c:73
          break 'c:/file.c':73

I am not sure how to handle file names with quotes; IIUC typical
escaping syntax won't work because it is already used in DOS-style file
names.  This matters since I think MI clients already have to play funny
games here :-(

(I'd like -break-insert to avoid linespecs completely, which would be a
big improvement IMO, but of course we still have to worry about
compatibility.)


Furthermore I think that quoted text should always be a token: we should
not try to extend the token boundaries or break the token up.  That is:

Valid:    break 'file.c':function
Invalid:  break 'file.c:function'
Invalid:  break 'file'.c:function


I think adopting these rules will make some of my ambiguous linespec
changes simpler.  Also I think they are reasonably faithful to user
expectation; not sure about compatibility, especially for the first rule
-- also note that quoting isn't actually documented :-(

Also, on irc Keith pointed out PR 12706, which this would fix.

Tom

Re: [RFA] 12843

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Fri, 26 Aug 2011 13:07:34 -0600
> 
> My view is that, aside from the drive-letter case, we should require
> funny file names to be quoted.

Agreed.

> I am not sure how to handle file names with quotes; IIUC typical
> escaping syntax won't work because it is already used in DOS-style file
> names.  This matters since I think MI clients already have to play funny
> games here :-(

Can we standardize on quoting with ".." instead?  If so, DOS file
names will not be a problem, because DOS/Windows filesystems don't
allow the `"' character in file names.  Therefore, \" can be safely
interpreted as a literal double quote character.

If we must use '..' style quoting, then how about doubling the ' to
express a literal quote character?

> Furthermore I think that quoted text should always be a token: we should
> not try to extend the token boundaries or break the token up.  That is:
> 
> Valid:    break 'file.c':function
> Invalid:  break 'file.c:function'
> Invalid:  break 'file'.c:function

What about these:

 break 'file with spaces.c:function:with:colons'
 break 'file with spaces.c':'function:with:colons'

?  Do you propose just the latter to be valid?

Re: [RFA] 12843

[asmwarrior]

On 2011-8-27 16:44, Eli Zaretskii wrote:
> What about these:
>
>   break 'file with spaces.c:function:with:colons'
>   break 'file with spaces.c':'function:with:colons'
I would expect only the second one is valid.

> This patch checks if ':' is followed by '/' and/or '\\' (depending on definition of IS_DIR_SEPARATOR). If it is, we keep looking for the next terminal in the string. This is problematic for filenames with ":\\" or "://" in them, but locate_first_half has gotten those wrong for a long, long time (in fact, any filename at all with a colon(s) in it).

Let's revert back to the old and wrong way. ^_^, the patch should works 
fine under Windows, because no extra colons is allowed in file names there.

asmwarrior
ollydbg from codeblocks' forum

Re: [RFA] 12843

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Tom> I am not sure how to handle file names with quotes; IIUC typical
Tom> escaping syntax won't work because it is already used in DOS-style file
Tom> names.  This matters since I think MI clients already have to play funny
Tom> games here :-(

Eli> Can we standardize on quoting with ".." instead?

I don't think so, because that is already accepted by linespec, so
compatibility problems apply.

Eli> If we must use '..' style quoting, then how about doubling the ' to
Eli> express a literal quote character?

It would work for me.

Tom> Furthermore I think that quoted text should always be a token: we should
Tom> not try to extend the token boundaries or break the token up.  That is:
Tom> Valid:    break 'file.c':function
Tom> Invalid:  break 'file.c:function'
Tom> Invalid:  break 'file'.c:function

Eli> What about these:

Eli>  break 'file with spaces.c:function:with:colons'
Eli>  break 'file with spaces.c':'function:with:colons'

Eli> ?  Do you propose just the latter to be valid?

Yes, just the latter.

The former would attempt to find a function with the quoted name,
following the "quoted text is a single token" rule.

Tom

Re: [RFA] 12843

[André Pönitz]

On Friday 26 August 2011 21:07:34 ext Tom Tromey wrote:
> Keith> I can play with that a little further. I'll also take the time to add
> Keith> a bunch of tests to highlight some of the problems. There is an open
> Keith> PR about this, to. In general the use of ':' or "::" in filenames is
> Keith> really bad for linespecs.
> 
> My view is that, aside from the drive-letter case, we should require
> funny file names to be quoted.  That is:
> 
> Valid:    break 'file with spaces.c':73
> Invalid:  break file with spaces.c:73
> 
> Valid:    break 'file:with:colons.c':73
> Invalid:  break file:with:colons.c:73
> 
> Valid either way:
>           break c:/file.c:73
>           break 'c:/file.c':73
> 
> I am not sure how to handle file names with quotes; IIUC typical
> escaping syntax won't work because it is already used in DOS-style file
> names.  This matters since I think MI clients already have to play funny
> games here :-(

Indeed. Something like

 25-break-insert -f "\"some thing.cpp\":794"

tends to work. I found it non-obvious in the beginning...

> (I'd like -break-insert to avoid linespecs completely, which would be a
> big improvement IMO, ...

From an MI client's point of view, passing all location information as 
a single argument is neither wanted nor needed.

 25-break-insert --file "some thing.cpp" --line 794

or even require everything to be quoted as in

 25-break-insert  --file "some thing.cpp" --line "794"

would be easier to handle than what's there now.

> ... but of course we still have to worry about compatibility.)

Just using new flags for the parameters should do the trick in this case.

In general, in the past, differences between different versions of gdb have
been large enough to require modifications in consumer code anyway (both
to take advantage of new features, and to handle incompatibilities in input
syntax), so requiring 100% "feature" compatibility _just for the sake of it_ is
unlikely to be helpful. At least I would prefer a syntax with simple, uniform
quoting rules over ad-hoc solutions tied to a specific OS or file system.

> [...]
> I think adopting these rules will make some of my ambiguous linespec
> changes simpler. 

I wonder whether it would be possible to just leave the current syntax
unchanged, and introduce a new, better behaved syntax and use some
global setting for toggling between them. So everybody concerned about
compatibility would not notice a change, and the others would put 

  'set breakpoint syntax 2011'  [or whatever]

in their .gdbinit and could use the new way.

Andre'

Re: [RFA] 12843

[Pedro Alves]

On Monday 29 August 2011 08:20:40, André Pönitz wrote:
> Indeed. Something like
> 
>  25-break-insert -f "\"some thing.cpp\":794"
> 
> tends to work. I found it non-obvious in the beginning...
> 
> > (I'd like -break-insert to avoid linespecs completely, which would be a
> > big improvement IMO, ...
> 
> From an MI client's point of view, passing all location information as 
> a single argument is neither wanted nor needed.
> 
>  25-break-insert --file "some thing.cpp" --line 794
> 
> or even require everything to be quoted as in
> 
>  25-break-insert  --file "some thing.cpp" --line "794"
> 
> would be easier to handle than what's there now.
> 
> > ... but of course we still have to worry about compatibility.)
> 
> Just using new flags for the parameters should do the trick in this case.

Yeah, though I expect frontends to support letting the user specify
manually where to insert the breakpoint (say, with a popup dialog
where you write foo.exe:bar or something more complicated, thus we'd
always still need some form of linespec support in MI.

-- 
Pedro Alves

Re: [RFA] 12843

[Matt Rice]

On Mon, Aug 29, 2011 at 3:12 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Monday 29 August 2011 08:20:40, André Pönitz wrote:
>> Indeed. Something like
>>
>>  25-break-insert -f "\"some thing.cpp\":794"
>>
>> tends to work. I found it non-obvious in the beginning...
>>
>> > (I'd like -break-insert to avoid linespecs completely, which would be a
>> > big improvement IMO, ...
>>
>> From an MI client's point of view, passing all location information as
>> a single argument is neither wanted nor needed.
>>
>>  25-break-insert --file "some thing.cpp" --line 794
>>
>> or even require everything to be quoted as in
>>
>>  25-break-insert  --file "some thing.cpp" --line "794"
>>
>> would be easier to handle than what's there now.
>>
>> > ... but of course we still have to worry about compatibility.)
>>
>> Just using new flags for the parameters should do the trick in this case.
>
> Yeah, though I expect frontends to support letting the user specify
> manually where to insert the breakpoint (say, with a popup dialog
> where you write foo.exe:bar or something more complicated, thus we'd
> always still need some form of linespec support in MI.

there's always interpreter-exec console seems a decent way of handling
user input *shrug*...

that said, at some point I thought about (instead of the objfile
'break foo.so:func' syntax)...
that it might be worthwhile to consider linespec parsing and the
evaluation separate,

thus linespec decomposes into 'objfile=foo.so symbol=func', that means
i suppose any commands (not sure there are any non-python/non-user
defined ones, i at least have a python one) linespec commands would
need quoting,

but `operator=' seemed to throw a wrench into that, though keys
unknown to linespec could be considered however i suppose.
then making linespec accept the decomposed or traditional linespec

25-break-insert  "file=\"some thing.cpp\" line=794"
or 25-break-insert -f "\"some thing.cpp\":794"

*shrug*

Re: [RFA] 12843

[André Pönitz]

On Monday 29 August 2011 12:12:53 ext Pedro Alves wrote:
> > Just using new flags for the parameters should do the trick in this case.
> 
> Yeah, though I expect frontends to support letting the user specify
> manually where to insert the breakpoint (say, with a popup dialog
> where you write foo.exe:bar or something more complicated, 

It's the IDE's job to capture the user's intentions, i.e. either let the
user provide explicit information about the kind of the breakpoint 
(file and line, function, 'on throw' etc) directly in the dialog, or do 
the kind of linespec parsing gdb currently does (which feels pretty
unnatural for a "normal" GUI user who typically does not even want
to know what kind of debugging "backend" is used.

But ... it looks like we look at different "frontends" here.

There is a whole spectrum of "debugger frontends" from, let's say 
gdb/TUI, ddd etc on one end, which are essentially passing through
user input more or less 1:1, requiring knowledge of exact gdb input
syntax, and "IDEs" that essentially do the job of translating high
level user intentions to "something that makes the backend do the
right thing". A "normal" IDE user does typically not think "Hey, looks 
like I am on line 123 in a file called 'foo.cpp', and I know that I have
a gdb backend, with version x.y or better, let's do -break-insert
-f "\"foo.cpp\":123" here." He just presses <F9> or clicks.

Setting a breakpoint by file name and line number covers easily 90%
of all breakpoints ever set through an IDE. Unfortunately, that case 
is not as robust with gdb as one would wish, and the "mangling them
into a single string, hoping that gdb can demangle and guess the
original meaning." is part of the problem. Using individual parameters
for specifying different 'aspects' would be more robust.

I am really interested in the IDE case which I believe to be one intented
use case for the MI protocol, and would like to ask to consider choosing 
a robust protocol that makes general automated interaction with gdb simple.

Andre'

Re: [RFA] 12843

[Pedro Alves]

On Monday 29 August 2011 13:17:02, André Pönitz wrote:
> On Monday 29 August 2011 12:12:53 ext Pedro Alves wrote:
> > > Just using new flags for the parameters should do the trick in this case.
> > 
> > Yeah, though I expect frontends to support letting the user specify
> > manually where to insert the breakpoint (say, with a popup dialog
> > where you write foo.exe:bar or something more complicated, 
> 
> It's the IDE's job to capture the user's intentions, i.e. either let the
> user provide explicit information about the kind of the breakpoint 
> (file and line, function, 'on throw' etc) directly in the dialog, or do 
> the kind of linespec parsing gdb currently does (which feels pretty
> unnatural for a "normal" GUI user who typically does not even want
> to know what kind of debugging "backend" is used.

I'd say the latter is arguable.  Note I did not suggest that new switches
like --file and --line would be bad, I even agreed!
As long as the old form works, looks like everyone will be happy?

Thanks,
-- 
Pedro Alves

Re: [RFA] 12843

[Jan Kratochvil]

On Mon, 29 Aug 2011 14:17:02 +0200, André Pönitz wrote:
> I am really interested in the IDE case which I believe to be one intented
> use case for the MI protocol, and would like to ask to consider choosing 
> a robust protocol that makes general automated interaction with gdb simple.

When you are already considering changes on the front end side has been
discussed some replacement by D-Bus, ORBit2 or similar RPC protocol?
It would make all the parsing and escaping not any issue.  I spend these days
on some entryval extensions of MI protocol so I have these ideas.


Thanks,
Jan

Re: [RFA] 12843

[André Pönitz]

On Monday 29 August 2011 21:21:12 ext Jan Kratochvil wrote:
> On Mon, 29 Aug 2011 14:17:02 +0200, André Pönitz wrote:
> > I am really interested in the IDE case which I believe to be one intented
> > use case for the MI protocol, and would like to ask to consider choosing 
> > a robust protocol that makes general automated interaction with gdb simple.
> 
> When you are already considering changes on the front end side has been
> discussed some replacement by D-Bus, ORBit2 or similar RPC protocol?

Please, pretty please, don't go down that road.

Consider using something that's available _painlessly_ cross-platform,
possibly avoid additional dependencies just for transfering a few
bytes. A text protocol with well-defined escape rules is completely 
fine here, and not _that_ difficult to define and implement.

It's already today quite some effort to get a properly Python-enabled
gdb up and running and distributed on e.g. Windows. There wasn't
even an "official" build for quite some time...

> It would make all the parsing and escaping not any issue.

It looks like the "parsing and escaping problems" come from the
attempt to stay compatible with the currently existing linespec 
syntax. There seemed to be consensus that a second set of
parameters with well-defined syntax would solve that problem.

> I spend these days on some entryval extensions of MI protocol 
> so I have these ideas.

I've been adding extra "MI style" output fields for all kind of data,
including structured and image data, for two years now, and  
escaping is _really_ not a problem. At worst, and if it needs to be
fast, one can base64- or hex-encode on one side and decode on
the other. It's a _M_achine _I_nterface after all.

Andre'

Re: [RFA] 12843

[Jan Kratochvil]

On Tue, 30 Aug 2011 10:21:15 +0200, André Pönitz wrote:
> On Monday 29 August 2011 21:21:12 ext Jan Kratochvil wrote:
> > When you are already considering changes on the front end side has been
> > discussed some replacement by D-Bus, ORBit2 or similar RPC protocol?
> 
> Please, pretty please, don't go down that road.

OK, if there is no interest from the front end side sure it makes no sense.


> It's already today quite some effort to get a properly Python-enabled
> gdb up and running and distributed on e.g. Windows. There wasn't
> even an "official" build for quite some time...

There could be bundled some of all those marshalling libraries (like readline
and discussed expat etc.) but OK.


Thanks,
Jan

Re: [RFA] 12843

[Tom Tromey]

Jan> When you are already considering changes on the front end side has been
Jan> discussed some replacement by D-Bus, ORBit2 or similar RPC protocol?

André> Please, pretty please, don't go down that road.

I think this is easily done already without much help from us -- Python
has a variety of RPC approaches out of the box, you could write up
XMLRPC to our API pretty easily.

André> I've been adding extra "MI style" output fields for all kind of data,
André> including structured and image data, for two years now, and  
André> escaping is _really_ not a problem.

Do you generate MI-compliant output from Python?  I'm curious.

Tom

Re: [RFA] 12843

[André Pönitz]

On Tuesday 30 August 2011 17:02:33 ext Tom Tromey wrote:
> Jan> When you are already considering changes on the front end side has been
> Jan> discussed some replacement by D-Bus, ORBit2 or similar RPC protocol?
> 
> André> Please, pretty please, don't go down that road.
> 
> I think this is easily done already without much help from us -- Python
> has a variety of RPC approaches out of the box, you could write up
> XMLRPC to our API pretty easily.
> 
> André> I've been adding extra "MI style" output fields for all kind of data,
> André> including structured and image data, for two years now, and  
> André> escaping is _really_ not a problem.
> 
> Do you generate MI-compliant output from Python?  I'm curious.

Sort of. Originally it was fully compliant, then extra fields had been
added and additional commas became "legal" (to save a few cycles
for the check whether they are necessary). But it's still pretty MI-ish.

As short example:

  void test()
  {
    std::vector<int> l;
    l.push_back(1);
    l.push_back(2);
    l.push_back(3);
    dummyStatement(&l);  // <== Break here
  }

produces in the "expanded" state where all children are visible

  "data=[{iname=\"local.l\",name=\"l\",addr=\"0xbfffeea8\",numchild=\"3\",
  childtype=\"int\",childnumchild=\"0\",addrbase=\"0x80a99f0\",addrstep=\"0x4\",
  children=[{value=\"1\",},{value=\"2\",},{value=\"3\",},],
  type=\"std::vector<int, std::allocator<int> >\",value=\"<3 items>\",},,]\n"

That will be displayed as something like

	Name	Value	Type 
	l	<3 items>	vector<int>
	[0]	1	int
	[1]	2	int
	[2]	3	int

The childtype/childnumchild/addrbase/addrstep is mostly there to 
reduce the overhead of repetitive date in the childitems, iname is
similar to the current -var-info-path-expression.

Andre'

Re: [RFA] 12843

[Tom Tromey]

>>>>> "André" == André Pönitz <andre.poenitz@nokia.com> writes:

Tom> Do you generate MI-compliant output from Python?  I'm curious.

André> Sort of. Originally it was fully compliant, then extra fields had been
André> added and additional commas became "legal" (to save a few cycles
André> for the check whether they are necessary). But it's still pretty MI-ish.

I ask because I occasionally wonder whether it would be useful to add
support for converting (a subset of) Python objects to MI.

Phil might need it for breakpoint_ops; but I think we were thinking in
terms of a one-off, where a generic facility might be better.

Tom

Re: [RFA] 12843

[André Pönitz]

On Tuesday 30 August 2011 19:20:58 ext Tom Tromey wrote:
> >>>>> "André" == André Pönitz <andre.poenitz@nokia.com> writes:
> 
> Tom> Do you generate MI-compliant output from Python?  I'm curious.
> 
> André> Sort of. Originally it was fully compliant, then extra fields had been
> André> added and additional commas became "legal" (to save a few cycles
> André> for the check whether they are necessary). But it's still pretty MI-ish.
> 
> I ask because I occasionally wonder whether it would be useful to add
> support for converting (a subset of) Python objects to MI.
> 
> Phil might need it for breakpoint_ops; but I think we were thinking in
> terms of a one-off, where a generic facility might be better.

Having a generic way to serialize gdb's python objects to MI might
be convenient. On the other hand that's generally not too difficult 
to do "manually" for some specific case. 

It might be also interesting from a performance point of view,
especially if this would be usable for lots of items. 

I am not sure how much of a difference this would make, though, 
but allowing the "sloppy" commas I mentioned certainly made a
difference, so I guess there's some general potential to gain a
few cycles by doing the formatting in C and not in Python.

Andre'

Re: [RFA] 12843

[Tom Tromey]

Tom> (I'd like -break-insert to avoid linespecs completely, which would be a
Tom> big improvement IMO, ...

André> From an MI client's point of view, passing all location information as 
André> a single argument is neither wanted nor needed.

Yeah.  I am not sure why it works this way, as it seems to be against
the whole idea of MI.

André> or even require everything to be quoted as in
André>  25-break-insert  --file "some thing.cpp" --line "794"

There's no need; MI already defines an input quoting syntax that is
sufficient.

Tom> ... but of course we still have to worry about compatibility.)

André> Just using new flags for the parameters should do the trick in this case.

André> In general, in the past, differences between different versions
André> of gdb have been large enough to require modifications in
André> consumer code anyway (both to take advantage of new features, and
André> to handle incompatibilities in input syntax), so requiring 100%
André> "feature" compatibility _just for the sake of it_ is unlikely to
André> be helpful. At least I would prefer a syntax with simple, uniform
André> quoting rules over ad-hoc solutions tied to a specific OS or file
André> system.

For MI, I don't think there is any issue.  We can just add options to
-break-insert.  (I don't know of anybody specifically working on this,
but I will at least put it in bugzilla.)

My view is that it is best to be compatible with existing "reasonable"
practice when possible, where "reasonable" means something akin to "has
actually happened".

In this case I think we can simplify linespec lexing without unduly
breaking things.

Tom> I think adopting these rules will make some of my ambiguous linespec
Tom> changes simpler. 

André> I wonder whether it would be possible to just leave the current syntax
André> unchanged, and introduce a new, better behaved syntax and use some
André> global setting for toggling between them. So everybody concerned about
André> compatibility would not notice a change, and the others would put 
André>   'set breakpoint syntax 2011'  [or whatever]
André> in their .gdbinit and could use the new way.

My first reaction is against this, but I don't have a particularly good
explanation for that.  I will think about it.

Tom

Re: [RFA] 12843

[Keith Seitz]

On 08/29/2011 12:46 PM, Tom Tromey wrote:

> My first reaction is against this, but I don't have a particularly good
> explanation for that.  I will think about it.

Is there actually any reason to require a flag? Couldn't we simply allow 
either a flag-based location _or_ a linespec, but not both, i.e.,

-insert-break -file foo.c -function my_function

OR

-insert-break foo.c:my_function

but not

-insert-break -file foo.c my_function

This would allow backward compatibility and new/better functionality.

I wouldn't be surprised if IDEs adopted the flag-based version very quickly.

Keith

Re: [RFA] 12843

[Daniel Jacobowitz]

On Mon, Aug 29, 2011 at 5:12 PM, Keith Seitz <keiths@redhat.com> wrote:
> On 08/29/2011 12:46 PM, Tom Tromey wrote:
>
>> My first reaction is against this, but I don't have a particularly good
>> explanation for that.  I will think about it.
>
> Is there actually any reason to require a flag? Couldn't we simply allow
> either a flag-based location _or_ a linespec, but not both, i.e.,
>
> -insert-break -file foo.c -function my_function
>
> OR
>
> -insert-break foo.c:my_function
>
> but not
>
> -insert-break -file foo.c my_function
>
> This would allow backward compatibility and new/better functionality.
>
> I wouldn't be surprised if IDEs adopted the flag-based version very quickly.

Heck, sometimes I'd use this from the command line if I could.

-- 
Thanks,
Daniel

Re: [RFA] 12843

[Tom Tromey]

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

Tom> My first reaction is against this, but I don't have a particularly good
Tom> explanation for that.  I will think about it.

Keith> Is there actually any reason to require a flag? Couldn't we simply
Keith> allow either a flag-based location _or_ a linespec, but not both,

I think there are 2 ideas here.

For MI we can just add options, as you say.  This causes no problems for
compatibility; it may be a bit of a pain to implement, but nothing
crazily difficult, just some more virtual methods in breakpoint.

I thought Andre's proposal about "set breakpoint syntax 2011" was
specifically for the CLI, where it would switch from linespec to
something new.  That's what I was responding to.

Keith> I wouldn't be surprised if IDEs adopted the flag-based version
Keith> very quickly.

Yeah.  I filed a bug for this:
http://sourceware.org/bugzilla/show_bug.cgi?id=13139

Tom

Re: [RFA] 12843

[Tom Tromey]

Tom> I thought Andre's proposal about "set breakpoint syntax 2011" was
Tom> specifically for the CLI, where it would switch from linespec to
Tom> something new.  That's what I was responding to.

It occurred to me today that we could probably handle this via a new
option to "break" et al.

For the SystemTap probes rebase, after ambiguous linespecs are done, I'm
planning to add a "-p" option to mean "parse as a SystemTap probe":

    break -p provider:name

I think this doesn't clash with any historical use.

So, we could look for a flag first thing and switch to "split syntax":

    break -s sourcefile -f function -l line

I don't immediately see any problems with this.

Tom

Re: [RFA] 12843

[Keith Seitz]

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

On 08/26/2011 11:22 AM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz<keiths@redhat.com>  writes:
>
> Keith>  	PR gdb/12843
> Keith>  	* linespec.c (locate_first_half): Do not stop on a colon
> Keith>  	if the next character is a directory separator character.
>
> Ok.
>
> I suspect we should tighten this further so that only drive letters work
> and not oddball stuff like "break file:/whatever.c:73".  What do you
> think?

I think we can safely do that -- especially since '/' is verboten on 
unix. So in that case, "file:/whatever.c" parses to the file 
"whatever.c" in the directory "file:", and we want to keep the whole 
thing together anyway.

Mind you, this doesn't solve the general case of colons in filenames, 
which is still broken.

Note: I've renamed the test to linespecs.exp, hoping that this file will 
grow into a full-fledged specification for future linespec work. Which I 
hope to do sometime soon. :-)

How about this? [Tested on x86_64-linux and i686-pc-cygwin (with much 
pain).]

Keith

ChangeLog
2011-08-30  Keith Seitz  <keiths@redhat.com>

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

testsuite/ChangeLog
2011-08-30  Keith Seitz  <keiths@redhat.com>

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

[-- Attachment #2: 12843-2.patch --]
[-- Type: text/plain, Size: 2680 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..bd07779
--- /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 memattr memattr.c {debug}]} {
+  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] 12843

[Jan Kratochvil]

On Wed, 31 Aug 2011 20:17:05 +0200, Keith Seitz wrote:
> -	  || p[0] == '\t'
> -	  || (p[0] == ':')
> +	  || p[0] == '\t' || p[0] == ':'

This is code cleanup outside of the scope of this patch (but OK).


> +if {[prepare_for_testing linespecs.exp memattr memattr.c {debug}]} {
  +if [prepare_for_testing linespecs.exp linespecs memattr.c] {

The testcase linespecs.exp should have binary linespecs, no matter from which
sources it was created.  (I really do not mind {debug} or not.)


Thanks,
Jan

Re: [RFA] 12843

[Tom Tromey]

Keith> Note: I've renamed the test to linespecs.exp, hoping that this file
Keith> will grow into a full-fledged specification for future linespec
Keith> work. Which I hope to do sometime soon. :-)

On my branch I made a gdb.linespec directory and put some stuff there.

Tom

Re: [RFA] 12843

[asmwarrior]

On 2011-9-1 2:17, Keith Seitz wrote:
>
> How about this? [Tested on x86_64-linux and i686-pc-cygwin (with much
> pain).]
>
> Keith
>
> ChangeLog
> 2011-08-30 Keith Seitz <keiths@redhat.com>
>
> PR gdb/12843
> * linespec.c (locate_first_half): Keep ':' if it looks
> like it could be part of a drive letter or filename.
>
> testsuite/ChangeLog
> 2011-08-30 Keith Seitz <keiths@redhat.com>
>
> PR gdb/12843
> * gdb.base/linespecs.exp: New file.

I test this patch on MinGW32 + Windows XP, and it works fine.
and This patch is much better than your previous patch.

asmwarrior
ollydbg from codeblocks' forum