This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[PING] Re: [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303
- From: Don Breazeal <donb at codesourcery dot com>
- To: Doug Evans <dje at google dot com>
- Cc: Keith Seitz <keiths at redhat dot com>, "gdb-patches at sourceware dot org ml" <gdb-patches at sourceware dot org>
- Date: Mon, 25 Jan 2016 09:26:46 -0800
- Subject: [PING] Re: [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303
- Authentication-results: sourceware.org; auth=none
- References: <047d7b6d967a74c69a0529168b2e at google dot com> <56954538 dot 1030007 at redhat dot com>
On 1/12/2016 10:26 AM, Keith Seitz wrote:
> On 01/11/2016 02:34 PM, Doug Evans wrote:
>> > - 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.
> [snip]
>> On that I'm kinda ambivalent, but I like having the assert
>> watch for the stated invariant.
>>
>> Thoughts?
>
> I missed that comment. [Well, I didn't even look at it. I'm so used to
> seeing no/minimal comments for symbol searching functions that I seldom
> even look for them. My bad.]
>
> That seems like a reasonable assertion, then, as long as it really does
> test what it is supposed to. How about:
>
> if (strchr (name, '<') == NULL && strchr (name, '(') == NULL)
> gdb_assert (strstr (name, "::") == NULL);
>
> Or something like that?
>
>> > 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 ':'? :-)
>>
>
> I don't think that matters at all. It isn't the scope operator in C++
> unless it is *two* colons. Not just a single colon. [Note that I believe
> we are going to have to deal with the general single-colon issue when
> running this code with abitags, but that's a patch for some other time.
> Or maybe this patch already mitigates that to a degree. I haven't
> checked into it at all.]
>
> Keith
>
Hi Doug, any thoughts on earlier responses from Keith and me to your
comments on this issue?
Thanks
--Don