From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29304 invoked by alias); 12 Jan 2016 00:17:32 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 29294 invoked by uid 89); 12 Jan 2016 00:17:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=evans, doug, Doug, Evans X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Jan 2016 00:17:29 +0000 Received: from svr-orw-fem-02x.mgc.mentorg.com ([147.34.96.206] helo=SVR-ORW-FEM-02.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1aImeM-00062m-K7 from Don_Breazeal@mentor.com ; Mon, 11 Jan 2016 16:17:26 -0800 Received: from [172.30.7.210] (147.34.91.1) by SVR-ORW-FEM-02.mgc.mentorg.com (147.34.96.168) with Microsoft SMTP Server (TLS) id 14.3.224.2; Mon, 11 Jan 2016 16:17:25 -0800 Subject: Re: [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303 To: Doug Evans , Keith Seitz References: <047d7b6d967a74c69a0529168b2e@google.com> CC: "gdb-patches@sourceware.org ml" From: Don Breazeal Message-ID: <5694460E.3080200@codesourcery.com> Date: Tue, 12 Jan 2016 00:17:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <047d7b6d967a74c69a0529168b2e@google.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00204.txt.bz2 On 1/11/2016 2:34 PM, Doug Evans wrote: > Keith Seitz writes: > > Hi, > > > > Thank you for pointing this out and supplying a patch (and *tests*!). > > > > 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. > > > > > 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. :-) > > > > 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. > > > > [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? > > > > Keith > > > > [apologies on mailer wrapping] > > > > Author: Keith Seitz > > Date: Fri Jan 8 14:00:22 2016 -0800 > > > > Tolerate malformed input for lookup_symbol-called functions. > > > > lookup_symbol is often called with user input. Consequently, any > > function called from lookup_symbol{,_in_language} should attempt to deal > > with malformed input gracefully. After all, malformed user input is > > not a programming/API error. > > > > This patch does not attempt to find/correct all instances of this. > > It only fixes locations in the code that trigger test suite failures. > > > > gdb/ChangeLog > > > > * cp-namespace.c (cp_lookup_bare_symbol): Do not assert on > > user input. > > (cp_search_static_and_baseclasses): Return null_block_symbol for > > malformed input. > > Remove assertions. > > * cp-support.c (cp_find_first_component_aux): Do not return > > a prefix length for ':' unless the next character is also ':'. > > > > diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c > > index 72002d6..aa225fe 100644 > > --- a/gdb/cp-namespace.c > > +++ b/gdb/cp-namespace.c > > @@ -166,12 +166,6 @@ cp_lookup_bare_symbol (const struct language_defn > > *langdef, > > { > > struct block_symbol sym; > > > > - /* Note: We can't do a simple assert for ':' not being in NAME because > > - ':' may be in the args of a template spec. This isn't intended to > be > > - 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. > > This is an invariant on what name can (and cannot) be. > IOW, it is wrong to call this function with name == "foo::bar", > handling that is the caller's responsibility. > Thus I think having an assert here is ok and good > (as long as it tests for the correct thing!). > > Whether it is ok to call this with name == "c:mumble" is the issue. > [or even "c:::mumble" or whatever else a user could type that > this function's caller isn't expected to handle] > On that I'm kinda ambivalent, but I like having the assert > watch for the stated invariant. > > Thoughts? Hi Doug I don't think the change in question directly relates to the issue my original patch was intended to address (PR 18303) -- with only the other changes in Keith's patch, that problem is solved. Maybe this change could be dealt with in a separate patch? Keith? Regardless, I tried a bunch of different commands and didn't ever see a "name" containing a ':' passed to cp_lookup_bare_symbol. Is there a way to make that happen? If there is a way, it seems like this assertion: gdb_assert (cp_entire_prefix_len (name) == 0); would address the issue while still allowing "c:mumble". In some cases it would be a redundant call to cp_entire_prefix_len, but that might be better than trying to re-implement the functionality in an expression passed to gdb_assert. Thanks --Don > > > sym = lookup_symbol_in_static_block (name, block, domain); > > if (sym.symbol != NULL) > > return sym; > > @@ -246,10 +240,9 @@ cp_search_static_and_baseclasses (const char *name, > > struct block_symbol klass_sym; > > struct type *klass_type; > > > > - /* The test here uses <= instead of < because Fortran also uses this, > > - and the module.exp testcase will pass "modmany::" for NAME here. > */ > > - gdb_assert (prefix_len + 2 <= strlen (name)); > > - gdb_assert (name[prefix_len + 1] == ':'); > > + /* Check for malformed input. */ > > + if (prefix_len + 2 > strlen (name) || name[prefix_len + 1] != ':') > > + return null_block_symbol; > > > > /* Find the name of the class and the name of the method, variable, > > etc. */ > > > > 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 ':'? :-) > > [btw, I think "a::::b" is illegal (note four colons), > but I don't know that for sure] > > > case 'o': > > /* Operator names can screw up the recursion. */ > > if (operator_possible > > >