This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PING] Re: [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303
- From: Doug Evans <dje at google dot com>
- To: Don Breazeal <donb at codesourcery dot com>
- Cc: Keith Seitz <keiths at redhat dot com>, "gdb-patches at sourceware dot org ml" <gdb-patches at sourceware dot org>
- Date: Tue, 26 Jan 2016 08:55:47 -0800
- Subject: Re: [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> <56A65AD6 dot 8060001 at codesourcery dot com>
On Mon, Jan 25, 2016 at 9:26 AM, Don Breazeal <donb@codesourcery.com> wrote:
> 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
Sorry, nothing more to add.
Keith's suggestion is fine by me.