From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21749 invoked by alias); 7 Dec 2014 23:33:06 -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 21737 invoked by uid 89); 7 Dec 2014 23:33:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f45.google.com Received: from mail-pa0-f45.google.com (HELO mail-pa0-f45.google.com) (209.85.220.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sun, 07 Dec 2014 23:33:03 +0000 Received: by mail-pa0-f45.google.com with SMTP id lj1so4058499pab.32 for ; Sun, 07 Dec 2014 15:33:01 -0800 (PST) X-Received: by 10.68.65.79 with SMTP id v15mr56239279pbs.56.1417995181693; Sun, 07 Dec 2014 15:33:01 -0800 (PST) Received: from seba.sebabeach.org.gmail.com (173-13-178-50-sfba.hfc.comcastbusiness.net. [173.13.178.50]) by mx.google.com with ESMTPSA id qc8sm34709103pdb.70.2014.12.07.15.33.00 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 07 Dec 2014 15:33:00 -0800 (PST) From: Doug Evans To: Patrick Palka Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] [RFC] PR c++/16874: make it easier to use anonymous namespaces References: <1417404096-16170-1-git-send-email-patrick@parcs.ath.cx> Date: Sun, 07 Dec 2014 23:33:00 -0000 In-Reply-To: <1417404096-16170-1-git-send-email-patrick@parcs.ath.cx> (Patrick Palka's message of "Sun, 30 Nov 2014 22:21:36 -0500") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-12/txt/msg00171.txt.bz2 Patrick Palka writes: > Symbols referring to names defined inside a top-level C++ anonymous > namespace have an "(anonymous namespace)::" prefix attached to them. > Referring to such symbols in commands such as "print" and "break" is > cumbersome because the prefix is tricky to type and symbols with this > prefix do not TAB-complete properly. > > To make it easier to refer to such symbols, this patch allows the user > to omit "(anonymous namespace)::" prefix from these symbols. In other > words, this patch allows the symbol "(anonymous namespace)::FOO" to be > referred to as "FOO". Likewise "(anonymous namespace)::FOO::BAR" ==> > "FOO::BAR". But not "FOO::(anonymous namespace)::BAZ" ==> "FOO::BAZ" > because the anonymous namespace in question is not the top-level one. > It also doesn't handle "(anonymous namespace)::(anonymous namespace)::FOO" > ==> "FOO". > > This patch is implemented in three hunks. The cp-namespace.c hunk > handles the elision of the anon prefix during symbol lookup in the > expression context (.e.g. "print foo"). The linespec.c hunk handles the > elision of the anon prefix during symbol lookup in the breakpoint > context (e.g. "break foo"). And finally the symtab.c hunk handles the > elision of the anon prefix during symbol completion. This patch does > not yet have a test case, but nonetheless I would very much appreciate > comments on the approach that this patch takes in address the mentioned > PR. > > I chose this approach because symbols defined in the root anonymous > namespace are very akin to static symbols so it is intuitive and > convenient to treat them as such, that is, to pretend that their > (anonymous namespace):: prefix doesn't even exist (unless the prefix was > explicitly given by the user). > > gdb/ChangeLog: > > * cp-support.h (cp_in_root_anonymous_namespace_p): New function. > * cp-namespace.c (cp_lookup_symbol_nonlocal): Try looking for > the symbol within the root anonymous namespace. > * linespec.c (find_linespec_symbols): Likewise. > * symtab.c (completion_list_add_name): Ignore the root anonymous > namespace prefix when looking for matching symbols. Hi. Some comments inline. [But first, thanks for looking at this!] > diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c > index fcfd17b..149c3c1 100644 > --- a/gdb/cp-namespace.c > +++ b/gdb/cp-namespace.c > @@ -233,8 +233,27 @@ cp_lookup_symbol_nonlocal (const char *name, > if (sym != NULL) > return sym; > > - return cp_lookup_symbol_namespace (scope, name, > - block, domain); > + sym = cp_lookup_symbol_namespace (scope, name, block, domain); > + if (sym != NULL) > + return sym; > + > + /* If we can't find the symbol then try searching for it inside the > + root anonymous namespace, i.e. the symbol > + (anonymous namespace)::NAME. */ > + > + if (!cp_in_root_anonymous_namespace_p (name)) > + { > + char *anon_name = alloca (strlen (CP_ANONYMOUS_NAMESPACE_STR "::") > + + strlen (name) + 1); > + strcpy (anon_name, CP_ANONYMOUS_NAMESPACE_STR "::"); > + strcat (anon_name, name); > + > + sym = cp_lookup_symbol_nonlocal (anon_name, block, domain); > + if (sym != NULL) > + return sym; > + } > + > + return NULL; > } > > /* Look up NAME in the C++ namespace NAMESPACE. Other arguments are One of the more important issues to keep in mind here is performance. [It's not the only issue of course. Usefulness is also important. But performance is not unimportant: users are tired of gdb's slowness.] In this case, don't underestimate the importance of handling the null case efficiently (imagine looking up a mispelled symbol in a large program that uses a large number of shared libraries). The above code will cause that failing search to be done twice (setting aside the existing inefficiences where the same symbol is already looked up multiple times). So I think the above second attempt to look up the symbol needs to be done smarter. E.g., maybe just call lookup_static_symbol and/or lookup_global_symbol. [Dig deeper into all that's done from cp_lookup_symbol_nonlocal on down. There is some cleanup to be done here btw. I'm working on some of it, just a heads up.] However, I'm also wondering why the above patch is needed. Maybe it is, but there is existing code to handle looking up symbols in anonymous namespaces via usings. E.g., try this: bash$ make check RUNTESTFLAGS=anon-ns.exp [ignore the output] bash$ make run GDBFLAGS=testsuite/gdb.cp/anon-ns (gdb) start (gdb) step (gdb) p doit1(void) $1 = {void (void)} 0x400580 <(anonymous namespace)::doit1()> (gdb) Note that doit1 was found in anonymous namespace even though I didn't specify it. [btw, if you play around with this, there are bugs in this area, e.g., 17686] There is the issue that the above example has stepped into the context where doit1 is defined, and thus full debug info for this compilation unit has been read in. If one repeats the above example thusly: bash$ make run GDBFLAGS=testsuite/gdb.cp/anon-ns (gdb) start (gdb) (gdb) p doit1(void) A syntax error in expression, near `)'. Note that it didn't work this time. [That's not the error I'd expect, but it falls out from failed symbol lookup.] One can make the case that this *should* work, however there are tests to verify things like this do not work: symbols in anonymous namespaces are not intended to be found unless one is in the appropriate file already. E.g., testsuite/gdb.cp/namespace.exp: gdb_test "print XOtherFile" "No symbol \"XOtherFile\" in current context." > diff --git a/gdb/cp-support.h b/gdb/cp-support.h > index c0ae35b..10be09f 100644 > --- a/gdb/cp-support.h > +++ b/gdb/cp-support.h > @@ -143,6 +143,16 @@ struct using_direct > }; > > > +/* Test whether SYMBOL correponds to a name inside the root anonymous > + namespace. */ > + > +static inline int Nit: We generally don't specify inline anywhere, and just let the compiler do it. > +cp_in_root_anonymous_namespace_p (const char *symbol) > +{ > + return strncmp (symbol, CP_ANONYMOUS_NAMESPACE_STR "::", > + CP_ANONYMOUS_NAMESPACE_LEN + 2) == 0; > +} > + > /* Functions from cp-support.c. */ > > extern char *cp_canonicalize_string (const char *string); > diff --git a/gdb/linespec.c b/gdb/linespec.c > index 82384ca..8a6c7f7 100644 > --- a/gdb/linespec.c > +++ b/gdb/linespec.c > @@ -3134,7 +3134,25 @@ find_linespec_symbols (struct linespec_state *state, > find_function_symbols (state, file_symtabs, lookup_name, > symbols, minsyms); > > - /* If we were unable to locate a symbol of the same name, try dividing > + /* For convenience, if LOOKUP_NAME was not found then look for > + (anonymous namespace)::LOOKUP_NAME (C++ only). */ > + > + if (state->language->la_language == language_cplus > + && VEC_empty (symbolp, *symbols) > + && VEC_empty (bound_minimal_symbol_d, *minsyms) > + && !cp_in_root_anonymous_namespace_p (name)) Adding language specific hacks to non-language specific files is something we prefer to avoid. Generally, we prefer adding the appropriate "method" to the language ops struct: struct language_defn. [It's not an absolute rule of course.] I can imagine gdb's existing support for having usings of (anonymous namespace) doesn't work for linespecs, but I wonder if it should, and thus would be a preferable way to do instead of the above. It might even fix more problems in this area, dunno. > + { > + char *new_lookup_name = alloca (strlen (CP_ANONYMOUS_NAMESPACE_STR "::") > + + strlen (lookup_name) + 1); > + > + strcpy (new_lookup_name, CP_ANONYMOUS_NAMESPACE_STR "::"); > + strcat (new_lookup_name, name); > + > + find_function_symbols (state, file_symtabs, new_lookup_name, > + symbols, minsyms); > + } > + > + /* If we were still unable to locate a corresponding symbol, try dividing > the name into class and method names and searching the class and its > baseclasses. */ > if (VEC_empty (symbolp, *symbols) > diff --git a/gdb/symtab.c b/gdb/symtab.c > index 345c20d..30f0dbe 100644 > --- a/gdb/symtab.c > +++ b/gdb/symtab.c > @@ -4164,6 +4164,16 @@ completion_list_add_name (const char *symname, > const char *sym_text, int sym_text_len, > const char *text, const char *word) > { > + /* Skip over the redundant "(anonymous namespace)::" prefix on symbols within > + the root anonymous namespace so that if the user is completing the name > + FOO then we want to match it with the symbol (anonymous namespace)::FOOBAR > + and to output FOOBAR in the completion list (C++ only). */ > + > + if (current_language->la_language == language_cplus > + && cp_in_root_anonymous_namespace_p (symname) > + && !cp_in_root_anonymous_namespace_p (sym_text)) > + symname += strlen (CP_ANONYMOUS_NAMESPACE_STR "::"); > + > /* Clip symbols that cannot match. */ > if (!compare_symbol_name (symname, sym_text, sym_text_len)) > return; I need to play with this part some more, but don't have any more time at the moment. Hopefully the above is enough to advance the discussion until then.