Re: [PATCH] [RFC] PR c++/16874: make it easier to use anonymous namespaces

[Doug Evans <xdje42 at gmail dot com>]

Patrick Palka <patrick@parcs.ath.cx> 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.