[PATCH v4] gdb/c++: Detect ambiguous variables in imported namespaces

[PATCH v4] gdb/c++: Detect ambiguous variables in imported namespaces

[Bruno Larsen]

When running gdb.cp/nsusing.cc and stopping at line 17, we can ask GDB
to print x and get a compiler-dependent answer. Using gcc 12.2.1, GDB
will print M::x, and using clang 16.0.0 prints N::x. Not only is this
behavior confusing to users, it is also not consistent with compiler
behaviors, which would warn that using x is ambiguous at this point.

This commit makes GDB behavior consistent with compilers. it achieves
this by making it so instead of exiting early when finding any symbol
with the correct name, GDB continues searching through all include
directives, storing all matching symbols in a relational map betwen the
mangled name and the found symbols.

If the resulting map has more than one entry, GDB says that the
reference is ambiguous and lists all possibilities. Otherwise it returns
the block_symbol structure for the desired symbol, or an empty struct if
nothing was found.

The commit also changes gdb.cp/nsusing.exp to test the ambiguous
detection.
---
Changelog for v4:
 * Dropped first patch of the series, since it was pushed
 * added a wrapper call to cp_lookup_symbols_via_imports
    this simplifies error handling and calling it outside the recursive
function
 * removed set merging by passing a map reference
 * removed the assert_not_reached thanks to Tom's comments

Changelog for v3:
 * changed map key to use std::string, so we get an ordered output.
 * Improved documentation on some spots
 * Fix bug where gdb could return a partial list of the ambiguous symbols

Changelog for v2:
 * factored out some code to avoid unnecessary repetition.
 * made it so ambiguous variables are explicitly reported
 * fixed formatting issues.
---
 gdb/cp-namespace.c               | 97 ++++++++++++++++++++++++--------
 gdb/testsuite/gdb.cp/nsusing.exp | 13 ++++-
 2 files changed, 83 insertions(+), 27 deletions(-)

diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index eff55b96ea9..093299d606e 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -32,6 +32,7 @@
 #include "buildsym.h"
 #include "language.h"
 #include "namespace.h"
+#include <map>
 #include <string>
 
 static struct block_symbol
@@ -344,7 +345,10 @@ cp_lookup_symbol_in_namespace (const char *the_namespace, const char *name,
   return sym;
 }
 
-/* Search for NAME by applying all import statements belonging to
+/* This version of the function is internal, use the wrapper unless
+   the list of ambiguous symbols is needed.
+
+   Search for NAME by applying all import statements belonging to
    BLOCK which are applicable in SCOPE.  If DECLARATION_ONLY the
    search is restricted to using declarations.
    Example:
@@ -372,27 +376,35 @@ cp_lookup_symbol_in_namespace (const char *the_namespace, const char *name,
    SEARCH_SCOPE_FIRST is an internal implementation detail: Callers must
    pass 0 for it.  Internally we pass 1 when recursing.  */
 
-static struct block_symbol
+static void
 cp_lookup_symbol_via_imports (const char *scope,
 			      const char *name,
 			      const struct block *block,
 			      const domain_enum domain,
 			      const int search_scope_first,
 			      const int declaration_only,
-			      const int search_parents)
+			      const int search_parents,
+			      std::map<std::string,
+				       struct block_symbol>& found_symbols)
 {
   struct using_direct *current;
   struct block_symbol sym = {};
   int len;
   int directive_match;
 
+  /* All the symbols we found will be kept in this relational map between
+     the mangled name and the block_symbol found.  We do this so that GDB
+     won't incorrectly report an ambiguous symbol for finding the same
+     thing twice.  */
+
   /* First, try to find the symbol in the given namespace if requested.  */
   if (search_scope_first)
-    sym = cp_lookup_symbol_in_namespace (scope, name,
-					 block, domain, 1);
-
-  if (sym.symbol != NULL)
-    return sym;
+    {
+      sym = cp_lookup_symbol_in_namespace (scope, name,
+					   block, domain, 1);
+      if (sym.symbol != nullptr)
+	found_symbols[sym.symbol->m_name] = sym;
+    }
 
   /* Due to a GCC bug, we need to know the boundaries of the current block
      to know if a certain using directive is valid.  */
@@ -446,7 +458,7 @@ cp_lookup_symbol_via_imports (const char *scope,
 	  if (declaration_only || sym.symbol != NULL || current->declaration)
 	    {
 	      if (sym.symbol != NULL)
-		return sym;
+		found_symbols[sym.symbol->m_name] = sym;
 
 	      continue;
 	    }
@@ -467,23 +479,57 @@ cp_lookup_symbol_via_imports (const char *scope,
 	      sym = cp_lookup_symbol_in_namespace (scope,
 						   current->import_src,
 						   block, domain, 1);
+	      found_symbols[sym.symbol->m_name] = sym;
 	    }
 	  else if (current->alias == NULL)
 	    {
 	      /* If this import statement creates no alias, pass
 		 current->inner as NAMESPACE to direct the search
 		 towards the imported namespace.  */
-	      sym = cp_lookup_symbol_via_imports (current->import_src,
-						  name, block,
-						  domain, 1, 0, 0);
+	      cp_lookup_symbol_via_imports (current->import_src, name,
+					    block, domain, 1, 0, 0,
+					    found_symbols);
 	    }
 
-	  if (sym.symbol != NULL)
-	    return sym;
 	}
     }
+}
 
-  return {};
+/* Wrapper for the actual cp_lookup_symbol_via_imports.  This wrapper sets
+   search_scope_first correctly and handles errors if needed.  */
+static struct block_symbol
+cp_lookup_symbol_via_imports (const char *scope,
+			      const char *name,
+			      const struct block *block,
+			      const domain_enum domain,
+			      const int declaration_only,
+			      const int search_parents)
+{
+  std::map<std::string, struct block_symbol> found_symbols;
+
+  cp_lookup_symbol_via_imports(scope, name, block, domain, 0,
+			       declaration_only, search_parents,
+			       found_symbols);
+
+  if (found_symbols.size () > 1)
+    {
+      auto itr = found_symbols.cbegin ();
+      std::string error_str = "Reference to \"";
+      error_str += name;
+      error_str += "\" is ambiguous, possibilities are: ";
+      error_str += itr->second.symbol->print_name ();
+      for (itr++; itr != found_symbols.end (); itr++)
+	{
+	  error_str += " and ";
+	  error_str += itr->second.symbol->print_name ();
+	}
+      error (_("%s"), error_str.c_str ());
+    }
+
+  if (found_symbols.size() == 1)
+    return found_symbols.cbegin ()->second;
+  else
+    return {};
 }
 
 /* Helper function that searches an array of symbols for one named NAME.  */
@@ -503,9 +549,10 @@ search_symbol_list (const char *name, int num,
   return NULL;
 }
 
-/* Like cp_lookup_symbol_via_imports, but if BLOCK is a function, it
-   searches through the template parameters of the function and the
-   function's type.  */
+/* Search for symbols whose name match NAME in the given SCOPE.
+   if BLOCK is a function, we'll search first through the template
+   parameters and function type. Afterwards (or if BLOCK is not a function)
+   search through imported directives using cp_lookup_symbol_via_imports.  */
 
 struct block_symbol
 cp_lookup_symbol_imports_or_template (const char *scope,
@@ -514,7 +561,6 @@ cp_lookup_symbol_imports_or_template (const char *scope,
 				      const domain_enum domain)
 {
   struct symbol *function = block->function ();
-  struct block_symbol result;
 
   symbol_lookup_debug_printf
     ("cp_lookup_symbol_imports_or_template (%s, %s, %s, %s)",
@@ -583,10 +629,11 @@ cp_lookup_symbol_imports_or_template (const char *scope,
 	}
     }
 
-  result = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1);
-  symbol_lookup_debug_printf
-    ("cp_lookup_symbol_imports_or_template (...) = %s",
-     result.symbol != NULL ? host_address_to_string (result.symbol) : "NULL");
+  struct block_symbol result
+    = cp_lookup_symbol_via_imports (scope, name, block, domain, 1, 1);
+  symbol_lookup_debug_printf ("cp_lookup_symbol_imports_or_template (...) = %s\n",
+		  result.symbol != nullptr
+		  ? host_address_to_string (result.symbol) : "NULL");
   return result;
 }
 
@@ -603,8 +650,8 @@ cp_lookup_symbol_via_all_imports (const char *scope, const char *name,
 
   while (block != NULL)
     {
-      sym = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 0, 1);
-      if (sym.symbol)
+      sym = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1);
+      if (sym.symbol != nullptr)
 	return sym;
 
       block = block->superblock ();
diff --git a/gdb/testsuite/gdb.cp/nsusing.exp b/gdb/testsuite/gdb.cp/nsusing.exp
index 0d374e5d03e..2a0aa29e8cf 100644
--- a/gdb/testsuite/gdb.cp/nsusing.exp
+++ b/gdb/testsuite/gdb.cp/nsusing.exp
@@ -126,11 +126,20 @@ gdb_test_multiple "print x" "print x, before using statement" {
     -re -wrap "No symbol .x. in current context.*" {
 	pass $gdb_test_name
     }
-    -re -wrap "= 911.*" {
+    -re -wrap "Reference to .x. is ambiguous.*" {
 	# GCC doesn't properly set the decl_line for namespaces, so GDB believes
 	# that the "using namespace M" line has already passed at this point.
 	xfail $gdb_test_name
     }
 }
 gdb_test "next" ".*" "using namespace M"
-gdb_test "print x" "= 911" "print x, only using M"
+gdb_test_multiple "print x" "print x, only using M" {
+    -re -wrap "= 911.*" {
+	pass $gdb_test_name
+    }
+    -re -wrap "Reference to .x. is ambiguous.*" {
+	xfail $gdb_test_name
+    }
+}
+gdb_test "next" ".*" "using namespace N"
+gdb_test "print x"  "Reference to .x. is ambiguous.*" "print x, with M and N"
-- 
2.38.1

[PING][PATCH v4] gdb/c++: Detect ambiguous variables in imported namespaces

[Bruno Larsen]

Ping!

On 21/12/2022 17:12, Bruno Larsen wrote:
> When running gdb.cp/nsusing.cc and stopping at line 17, we can ask GDB
> to print x and get a compiler-dependent answer. Using gcc 12.2.1, GDB
> will print M::x, and using clang 16.0.0 prints N::x. Not only is this
> behavior confusing to users, it is also not consistent with compiler
> behaviors, which would warn that using x is ambiguous at this point.
>
> This commit makes GDB behavior consistent with compilers. it achieves
> this by making it so instead of exiting early when finding any symbol
> with the correct name, GDB continues searching through all include
> directives, storing all matching symbols in a relational map betwen the
> mangled name and the found symbols.
>
> If the resulting map has more than one entry, GDB says that the
> reference is ambiguous and lists all possibilities. Otherwise it returns
> the block_symbol structure for the desired symbol, or an empty struct if
> nothing was found.
>
> The commit also changes gdb.cp/nsusing.exp to test the ambiguous
> detection.
> ---
> Changelog for v4:
>   * Dropped first patch of the series, since it was pushed
>   * added a wrapper call to cp_lookup_symbols_via_imports
>      this simplifies error handling and calling it outside the recursive
> function
>   * removed set merging by passing a map reference
>   * removed the assert_not_reached thanks to Tom's comments
>
> Changelog for v3:
>   * changed map key to use std::string, so we get an ordered output.
>   * Improved documentation on some spots
>   * Fix bug where gdb could return a partial list of the ambiguous symbols
>
> Changelog for v2:
>   * factored out some code to avoid unnecessary repetition.
>   * made it so ambiguous variables are explicitly reported
>   * fixed formatting issues.
> ---
>   gdb/cp-namespace.c               | 97 ++++++++++++++++++++++++--------
>   gdb/testsuite/gdb.cp/nsusing.exp | 13 ++++-
>   2 files changed, 83 insertions(+), 27 deletions(-)
>
> diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
> index eff55b96ea9..093299d606e 100644
> --- a/gdb/cp-namespace.c
> +++ b/gdb/cp-namespace.c
> @@ -32,6 +32,7 @@
>   #include "buildsym.h"
>   #include "language.h"
>   #include "namespace.h"
> +#include <map>
>   #include <string>
>   
>   static struct block_symbol
> @@ -344,7 +345,10 @@ cp_lookup_symbol_in_namespace (const char *the_namespace, const char *name,
>     return sym;
>   }
>   
> -/* Search for NAME by applying all import statements belonging to
> +/* This version of the function is internal, use the wrapper unless
> +   the list of ambiguous symbols is needed.
> +
> +   Search for NAME by applying all import statements belonging to
>      BLOCK which are applicable in SCOPE.  If DECLARATION_ONLY the
>      search is restricted to using declarations.
>      Example:
> @@ -372,27 +376,35 @@ cp_lookup_symbol_in_namespace (const char *the_namespace, const char *name,
>      SEARCH_SCOPE_FIRST is an internal implementation detail: Callers must
>      pass 0 for it.  Internally we pass 1 when recursing.  */
>   
> -static struct block_symbol
> +static void
>   cp_lookup_symbol_via_imports (const char *scope,
>   			      const char *name,
>   			      const struct block *block,
>   			      const domain_enum domain,
>   			      const int search_scope_first,
>   			      const int declaration_only,
> -			      const int search_parents)
> +			      const int search_parents,
> +			      std::map<std::string,
> +				       struct block_symbol>& found_symbols)
>   {
>     struct using_direct *current;
>     struct block_symbol sym = {};
>     int len;
>     int directive_match;
>   
> +  /* All the symbols we found will be kept in this relational map between
> +     the mangled name and the block_symbol found.  We do this so that GDB
> +     won't incorrectly report an ambiguous symbol for finding the same
> +     thing twice.  */
> +
>     /* First, try to find the symbol in the given namespace if requested.  */
>     if (search_scope_first)
> -    sym = cp_lookup_symbol_in_namespace (scope, name,
> -					 block, domain, 1);
> -
> -  if (sym.symbol != NULL)
> -    return sym;
> +    {
> +      sym = cp_lookup_symbol_in_namespace (scope, name,
> +					   block, domain, 1);
> +      if (sym.symbol != nullptr)
> +	found_symbols[sym.symbol->m_name] = sym;
> +    }
>   
>     /* Due to a GCC bug, we need to know the boundaries of the current block
>        to know if a certain using directive is valid.  */
> @@ -446,7 +458,7 @@ cp_lookup_symbol_via_imports (const char *scope,
>   	  if (declaration_only || sym.symbol != NULL || current->declaration)
>   	    {
>   	      if (sym.symbol != NULL)
> -		return sym;
> +		found_symbols[sym.symbol->m_name] = sym;
>   
>   	      continue;
>   	    }
> @@ -467,23 +479,57 @@ cp_lookup_symbol_via_imports (const char *scope,
>   	      sym = cp_lookup_symbol_in_namespace (scope,
>   						   current->import_src,
>   						   block, domain, 1);
> +	      found_symbols[sym.symbol->m_name] = sym;
>   	    }
>   	  else if (current->alias == NULL)
>   	    {
>   	      /* If this import statement creates no alias, pass
>   		 current->inner as NAMESPACE to direct the search
>   		 towards the imported namespace.  */
> -	      sym = cp_lookup_symbol_via_imports (current->import_src,
> -						  name, block,
> -						  domain, 1, 0, 0);
> +	      cp_lookup_symbol_via_imports (current->import_src, name,
> +					    block, domain, 1, 0, 0,
> +					    found_symbols);
>   	    }
>   
> -	  if (sym.symbol != NULL)
> -	    return sym;
>   	}
>       }
> +}
>   
> -  return {};
> +/* Wrapper for the actual cp_lookup_symbol_via_imports.  This wrapper sets
> +   search_scope_first correctly and handles errors if needed.  */
> +static struct block_symbol
> +cp_lookup_symbol_via_imports (const char *scope,
> +			      const char *name,
> +			      const struct block *block,
> +			      const domain_enum domain,
> +			      const int declaration_only,
> +			      const int search_parents)
> +{
> +  std::map<std::string, struct block_symbol> found_symbols;
> +
> +  cp_lookup_symbol_via_imports(scope, name, block, domain, 0,
> +			       declaration_only, search_parents,
> +			       found_symbols);
> +
> +  if (found_symbols.size () > 1)
> +    {
> +      auto itr = found_symbols.cbegin ();
> +      std::string error_str = "Reference to \"";
> +      error_str += name;
> +      error_str += "\" is ambiguous, possibilities are: ";
> +      error_str += itr->second.symbol->print_name ();
> +      for (itr++; itr != found_symbols.end (); itr++)
> +	{
> +	  error_str += " and ";
> +	  error_str += itr->second.symbol->print_name ();
> +	}
> +      error (_("%s"), error_str.c_str ());
> +    }
> +
> +  if (found_symbols.size() == 1)
> +    return found_symbols.cbegin ()->second;
> +  else
> +    return {};
>   }
>   
>   /* Helper function that searches an array of symbols for one named NAME.  */
> @@ -503,9 +549,10 @@ search_symbol_list (const char *name, int num,
>     return NULL;
>   }
>   
> -/* Like cp_lookup_symbol_via_imports, but if BLOCK is a function, it
> -   searches through the template parameters of the function and the
> -   function's type.  */
> +/* Search for symbols whose name match NAME in the given SCOPE.
> +   if BLOCK is a function, we'll search first through the template
> +   parameters and function type. Afterwards (or if BLOCK is not a function)
> +   search through imported directives using cp_lookup_symbol_via_imports.  */
>   
>   struct block_symbol
>   cp_lookup_symbol_imports_or_template (const char *scope,
> @@ -514,7 +561,6 @@ cp_lookup_symbol_imports_or_template (const char *scope,
>   				      const domain_enum domain)
>   {
>     struct symbol *function = block->function ();
> -  struct block_symbol result;
>   
>     symbol_lookup_debug_printf
>       ("cp_lookup_symbol_imports_or_template (%s, %s, %s, %s)",
> @@ -583,10 +629,11 @@ cp_lookup_symbol_imports_or_template (const char *scope,
>   	}
>       }
>   
> -  result = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1);
> -  symbol_lookup_debug_printf
> -    ("cp_lookup_symbol_imports_or_template (...) = %s",
> -     result.symbol != NULL ? host_address_to_string (result.symbol) : "NULL");
> +  struct block_symbol result
> +    = cp_lookup_symbol_via_imports (scope, name, block, domain, 1, 1);
> +  symbol_lookup_debug_printf ("cp_lookup_symbol_imports_or_template (...) = %s\n",
> +		  result.symbol != nullptr
> +		  ? host_address_to_string (result.symbol) : "NULL");
>     return result;
>   }
>   
> @@ -603,8 +650,8 @@ cp_lookup_symbol_via_all_imports (const char *scope, const char *name,
>   
>     while (block != NULL)
>       {
> -      sym = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 0, 1);
> -      if (sym.symbol)
> +      sym = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1);
> +      if (sym.symbol != nullptr)
>   	return sym;
>   
>         block = block->superblock ();
> diff --git a/gdb/testsuite/gdb.cp/nsusing.exp b/gdb/testsuite/gdb.cp/nsusing.exp
> index 0d374e5d03e..2a0aa29e8cf 100644
> --- a/gdb/testsuite/gdb.cp/nsusing.exp
> +++ b/gdb/testsuite/gdb.cp/nsusing.exp
> @@ -126,11 +126,20 @@ gdb_test_multiple "print x" "print x, before using statement" {
>       -re -wrap "No symbol .x. in current context.*" {
>   	pass $gdb_test_name
>       }
> -    -re -wrap "= 911.*" {
> +    -re -wrap "Reference to .x. is ambiguous.*" {
>   	# GCC doesn't properly set the decl_line for namespaces, so GDB believes
>   	# that the "using namespace M" line has already passed at this point.
>   	xfail $gdb_test_name
>       }
>   }
>   gdb_test "next" ".*" "using namespace M"
> -gdb_test "print x" "= 911" "print x, only using M"
> +gdb_test_multiple "print x" "print x, only using M" {
> +    -re -wrap "= 911.*" {
> +	pass $gdb_test_name
> +    }
> +    -re -wrap "Reference to .x. is ambiguous.*" {
> +	xfail $gdb_test_name
> +    }
> +}
> +gdb_test "next" ".*" "using namespace N"
> +gdb_test "print x"  "Reference to .x. is ambiguous.*" "print x, with M and N"


-- 
Cheers,
Bruno

Re: [PATCH v4] gdb/c++: Detect ambiguous variables in imported namespaces

[Tom Tromey]

>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

Bruno> When running gdb.cp/nsusing.cc and stopping at line 17, we can ask GDB
Bruno> to print x and get a compiler-dependent answer. Using gcc 12.2.1, GDB
Bruno> will print M::x, and using clang 16.0.0 prints N::x. Not only is this
Bruno> behavior confusing to users, it is also not consistent with compiler
Bruno> behaviors, which would warn that using x is ambiguous at this point.

Thank you.  This is ok.

Tom

Re: [PATCH v4] gdb/c++: Detect ambiguous variables in imported namespaces

[Bruno Larsen]

On 05/01/2023 20:53, Tom Tromey wrote:
>>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> Bruno> When running gdb.cp/nsusing.cc and stopping at line 17, we can ask GDB
> Bruno> to print x and get a compiler-dependent answer. Using gcc 12.2.1, GDB
> Bruno> will print M::x, and using clang 16.0.0 prints N::x. Not only is this
> Bruno> behavior confusing to users, it is also not consistent with compiler
> Bruno> behaviors, which would warn that using x is ambiguous at this point.
>
> Thank you.  This is ok.
Thanks, pushed!
> Tom
>

-- 
Cheers,
Bruno