From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5436 invoked by alias); 15 Mar 2011 18:48:27 -0000 Received: (qmail 5422 invoked by uid 22791); 15 Mar 2011 18:48:25 -0000 X-SWARE-Spam-Status: No, hits=-5.0 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_BJ,TW_CP,TW_GP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 15 Mar 2011 18:48:13 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p2FImCT8024410 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 15 Mar 2011 14:48:12 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p2FIm9Xs018048 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 15 Mar 2011 14:48:10 -0400 Message-ID: <4D7FB469.9080703@redhat.com> Date: Tue, 15 Mar 2011 19:03:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101209 Fedora/3.1.7-0.35.b3pre.fc13 Lightning/1.0b3pre Thunderbird/3.1.7 MIME-Version: 1.0 To: Jan Kratochvil CC: gdb-patches@sourceware.org Subject: Re: [patch 0/3] Re: [RFA] c++/11734 revisited (and c++/12273) References: <20110227211637.GA18378@host1.dyn.jankratochvil.net> <4D6D6C74.8080304@redhat.com> <20110313222824.GA24322@host1.jankratochvil.net> In-Reply-To: <20110313222824.GA24322@host1.jankratochvil.net> Content-Type: multipart/mixed; boundary="------------020408030306010303000606" X-IsSubscribed: yes 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 X-SW-Source: 2011-03/txt/msg00808.txt.bz2 This is a multi-part message in MIME format. --------------020408030306010303000606 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2669 Hi, Jan, thank you for taking the time to look at this. Believe me when I say, I feel for ya! On 03/13/2011 03:28 PM, Jan Kratochvil wrote: > Although I wrote `isalnum' it is a wrong character class, for example > isalnum ('_') == 0 but `_' is a valid identifier character. I have a cleanup that I've been working on which changed all this, but you've figured out before I have submitted my cleanup. I have done exactly as you suggest. > All these cases should be written like p[6] both for the IMO better > readability and for making it less fragile against bugs like `(*p + 6)'. Done. > There may be also "task" catching but that is used by Ada and it already > worked before without such exception so it is probably OK. I looked in the manual and the various gdb command help strings, but I could not find this. Maybe I didn't check Ada-specific commands? In any case, this whole linespec thing seems overtly fragile to begin with. It is a very poor abstraction for what is essentially a language-dependent task (of identifying names). But it is trivial enough to add to this function, for whatever that's worth. > It seems to me here could be sufficient instead of is_overloaded just: > if (*p == '(') > > Or do you have a countercase where it would not work? Nope. That was one of the cleanups that I'd arrived at last week, too. > *(p + 5) == '\'' > -> > strchr (get_gdb_completer_quote_characters (), p[5]) != NULL Done. > Underrun of the strings you reported as present even in pre-physname GDB so > I have just filed it as: > decode_linespec_1 vagrind errors on "" > http://sourceware.org/bugzilla/show_bug.cgi?id=12535 Indeed! I've changed this to check that ptr > start (where start is ptr at entry to the function). >> + if (current_language->la_language == language_cplus >> + || current_language->la_language == language_java) >> + p = keep_name_info (p); > > Wrong indentation. Fixed. > It seems to me here could be sufficient instead of is_overloaded just: > if (strchr (copy, '(') != NULL) > > Or do you have a countercase where it would not work? No countercase. I agree that strchr should be sufficient here when comparing the variable copy instead of real_saved_arg. Thank you for pointing that out -- I missed that. > It is approved with these changes if you agree with them. I do not expect > anyone else is going to futher review it. I'll await one final "OK" from you before committing, just in case you seen anything additional on which you'd like to comment. Thanks again for looking at this. FWIW, I've attached only the linespec patch which includes the requested changes. Keith --------------020408030306010303000606 Content-Type: text/plain; name="linespec.c.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="linespec.c.patch" Content-length: 11072 Index: linespec.c =================================================================== RCS file: /cvs/src/src/gdb/linespec.c,v retrieving revision 1.113 diff -u -p -r1.113 linespec.c --- linespec.c 4 Mar 2011 20:07:22 -0000 1.113 +++ linespec.c 15 Mar 2011 18:40:18 -0000 @@ -41,6 +41,7 @@ #include "mi/mi-cmds.h" #include "target.h" #include "arch-utils.h" +#include /* We share this one with symtab.c, but it is not exported widely. */ @@ -213,6 +214,19 @@ find_methods (struct type *t, char *name int i1 = 0; int ibase; char *class_name = type_name_no_tag (t); + struct cleanup *cleanup; + char *canon; + + /* NAME is typed by the user: it needs to be canonicalized before + passing to lookup_symbol. */ + canon = cp_canonicalize_string (name); + if (canon != NULL) + { + name = canon; + cleanup = make_cleanup (xfree, name); + } + else + cleanup = make_cleanup (null_cleanup, NULL); /* Ignore this class if it doesn't have a name. This is ugly, but unless we figure out how to get the physname without the name of @@ -275,6 +289,7 @@ find_methods (struct type *t, char *name i1 += find_methods (TYPE_BASECLASS (t, ibase), name, language, sym_arr + i1); + do_cleanups (cleanup); return i1; } @@ -663,6 +678,68 @@ find_method_overload_end (char *p) return p; } + +/* Does P point to a sequence of characters which implies the end + of a name? Terminals include "if" and "thread" clauses. */ + +static int +name_end (char *p) +{ + while (isspace (*p)) + ++p; + if (*p == 'i' && p[1] == 'f' + && (isspace (p[2]) || p[2] == '\0' || p[2] == '(')) + return 1; + + if (strncmp (p, "thread", 6) == 0 + && (isspace (p[6]) || p[6] == '\0')) + return 1; + + return 0; +} + +/* Keep important information used when looking up a name. This includes + template parameters, overload information, and important keywords. */ + +static char * +keep_name_info (char *ptr) +{ + char *p = ptr; + char *start = ptr; + + /* Keep any template parameters. */ + if (name_end (ptr)) + goto done; + + while (isspace (*p)) + ++p; + if (*p == '<') + ptr = p = find_template_name_end (ptr); + + if (name_end (ptr)) + goto done; + + /* Keep method overload information. */ + if (*p == '(') + ptr = p = find_method_overload_end (p); + + if (name_end (ptr)) + goto done; + + /* Keep important keywords. */ + while (isspace (*p)) + ++p; + if (strncmp (p, "const", 5) == 0 + && (isspace (p[5]) || p[5] == '\0' + || strchr (get_gdb_completer_quote_characters (), p[5]) != NULL)) + ptr = p = p + 5; + + done: + while (ptr > start && isspace (*(ptr - 1))) + --ptr; + return ptr; +} + /* The parser of linespec itself. */ @@ -871,17 +948,8 @@ decode_line_1 (char **argptr, int funfir p = skip_quoted (*argptr); } - /* Keep any template parameters. */ - if (*p == '<') - p = find_template_name_end (p); - - /* Keep method overload information. */ - if (*p == '(') - p = find_method_overload_end (p); - - /* Make sure we keep important kewords like "const". */ - if (strncmp (p, " const", 6) == 0) - p += 6; + /* Keep any important naming information. */ + p = keep_name_info (p); copy = (char *) alloca (p - *argptr + 1); memcpy (copy, *argptr, p - *argptr); @@ -1057,6 +1125,10 @@ locate_first_half (char **argptr, int *i error (_("malformed template specification in command")); p = temp_end; } + + if (p[0] == '(') + p = find_method_overload_end (p); + /* Check for a colon and a plus or minus and a [ (which indicates an Objective-C method). */ if (is_objc_method_format (p)) @@ -1224,7 +1296,7 @@ decode_objc (char **argptr, int funfirst static struct symtabs_and_lines decode_compound (char **argptr, int funfirstline, char ***canonical, - char *saved_arg, char *p, int *not_found_ptr) + char *the_real_saved_arg, char *p, int *not_found_ptr) { struct symtabs_and_lines values; char *p2; @@ -1235,6 +1307,23 @@ decode_compound (char **argptr, int funf struct symbol *sym_class; struct type *t; char *saved_java_argptr = NULL; + char *saved_arg; + + /* If the user specified any completer quote characters in the input, + strip them. They are superfluous. */ + saved_arg = alloca (strlen (the_real_saved_arg) + 1); + { + char *dst = saved_arg; + char *src = the_real_saved_arg; + char *quotes = get_gdb_completer_quote_characters (); + while (*src != '\0') + { + if (strchr (quotes, *src) == NULL) + *dst++ = *src; + ++src; + } + *dst = '\0'; + } /* First check for "global" namespace specification, of the form "::foo". If found, skip over the colons and jump to normal @@ -1251,8 +1340,10 @@ decode_compound (char **argptr, int funf find_method. 2) AAA::inA isn't the name of a class. In that case, either the - user made a typo or AAA::inA is the name of a namespace. - Either way, we just look up AAA::inA::fun with lookup_symbol. + user made a typo, AAA::inA is the name of a namespace, or it is + the name of a minimal symbol. + We just look up AAA::inA::fun with lookup_symbol. If that fails, + try lookup_minimal_symbol. Thus, our first task is to find everything before the last set of double-colons and figure out if it's the name of a class. So we @@ -1273,6 +1364,8 @@ decode_compound (char **argptr, int funf while (1) { + static char *break_characters = " \t("; + /* Move pointer up to next possible class/namespace token. */ p = p2 + 1; /* Restart with old value +1. */ @@ -1283,8 +1376,9 @@ decode_compound (char **argptr, int funf /* PASS2: p2->"::fun", p->":fun" */ /* Move pointer ahead to next double-colon. */ - while (*p && (p[0] != ' ') && (p[0] != '\t') && (p[0] != '\'') - && (*p != '(')) + while (*p + && strchr (break_characters, *p) == NULL + && strchr (get_gdb_completer_quote_characters (), *p) == NULL) { if (current_language->la_language == language_cplus) p += cp_validate_operator (p); @@ -1308,9 +1402,12 @@ decode_compound (char **argptr, int funf else if ((p[0] == ':') && (p[1] == ':')) break; /* Found double-colon. */ else - /* PASS2: We'll keep getting here, until p->"", at which point - we exit this loop. */ - p++; + { + /* PASS2: We'll keep getting here, until P points to one of the + break characters, at which point we exit this loop. */ + if (*p && strchr (break_characters, *p) == NULL) + p++; + } } if (*p != ':') @@ -1319,7 +1416,7 @@ decode_compound (char **argptr, int funf unsuccessfully all the components of the string, and p->""(PASS2). */ - /* We get here if p points to ' ', '\t', '\'', "::" or ""(i.e + /* We get here if p points to one of the break characters or "" (i.e., string ended). */ /* Save restart for next time around. */ p2 = p; @@ -1379,18 +1476,8 @@ decode_compound (char **argptr, int funf p += cp_validate_operator (p - 8) - 8; } - /* Keep any template parameters. */ - if (*p == '<') - p = find_template_name_end (p); - - /* Keep method overload information. */ - a = strchr (p, '('); - if (a != NULL) - p = find_method_overload_end (a); - - /* Make sure we keep important kewords like "const". */ - if (strncmp (p, " const", 6) == 0) - p += 6; + /* Keep any important naming information. */ + p = keep_name_info (p); /* Java may append typenames, so assume that if there is anything else left in *argptr, it must be a typename. */ @@ -1470,6 +1557,10 @@ decode_compound (char **argptr, int funf /* We couldn't find a class, so we're in case 2 above. We check the entire name as a symbol instead. */ + if (current_language->la_language == language_cplus + || current_language->la_language == language_java) + p = keep_name_info (p); + copy = (char *) alloca (p - saved_arg2 + 1); memcpy (copy, saved_arg2, p - saved_arg2); /* Note: if is_quoted should be true, we snuff out quote here @@ -1479,15 +1570,24 @@ decode_compound (char **argptr, int funf *argptr = (*p == '\'') ? p + 1 : p; /* Look up entire name. */ - sym = lookup_symbol (copy, 0, VAR_DOMAIN, 0); + sym = lookup_symbol (copy, get_selected_block (0), VAR_DOMAIN, 0); if (sym) return symbol_found (funfirstline, canonical, copy, sym, NULL); + else + { + struct minimal_symbol *msym; + + /* Couldn't find any interpretation as classes/namespaces. As a last + resort, try the minimal symbol tables. */ + msym = lookup_minimal_symbol (copy, NULL, NULL); + if (msym != NULL) + return minsym_found (funfirstline, msym); + } - /* Couldn't find any interpretation as classes/namespaces, so give - up. The quotes are important if copy is empty. */ + /* Couldn't find a minimal symbol, either, so give up. */ if (not_found_ptr) *not_found_ptr = 1; - cplusplus_error (saved_arg, + cplusplus_error (the_real_saved_arg, "Can't find member of namespace, " "class, struct, or union named \"%s\"\n", copy); @@ -1526,7 +1626,7 @@ lookup_prefix_sym (char **argptr, char * /* At this point p1->"::inA::fun", p->"inA::fun" copy->"AAA", argptr->"inA::fun". */ - sym = lookup_symbol (copy, 0, STRUCT_DOMAIN, 0); + sym = lookup_symbol (copy, get_selected_block (0), STRUCT_DOMAIN, 0); if (sym == NULL) { /* Typedefs are in VAR_DOMAIN so the above symbol lookup will @@ -1594,20 +1694,32 @@ find_method (int funfirstline, char ***c /* If we were given a specific overload instance, use that (or error if no matches were found). Otherwise ask the user which one to use. */ - if (strchr (saved_arg, '(') != NULL) + if (strchr (copy, '(')) { int i; - char *name = saved_arg; - char *canon = cp_canonicalize_string (name); + char *name; + char *canon; struct cleanup *cleanup; + /* Construct the proper search name based on SYM_CLASS and COPY. + SAVED_ARG may contain a valid name, but that name might not be + what is actually stored in the symbol table. For example, + if SAVED_ARG (and SYM_CLASS) were found via an import + ("using namespace" in C++), then the physname of + SYM_CLASS ("A::myclass") may not be the same as SAVED_ARG + ("myclass"). */ + name = xmalloc (strlen (SYMBOL_NATURAL_NAME (sym_class)) + + 2 /* "::" */ + strlen (copy) + 1); + strcpy (name, SYMBOL_NATURAL_NAME (sym_class)); + strcat (name, "::"); + strcat (name, copy); + canon = cp_canonicalize_string (name); if (canon != NULL) { + xfree (name); name = canon; - cleanup = make_cleanup (xfree, canon); } - else - cleanup = make_cleanup (null_cleanup, NULL); + cleanup = make_cleanup (xfree, name); for (i = 0; i < i1; ++i) { --------------020408030306010303000606--