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

[Bruno Larsen <blarsen@redhat.com>]

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 map containing zero or one entries.

The commit also changes gdb.cp/nsusing.exp to test the ambiguous
detection.
---
 gdb/cp-namespace.c               | 86 +++++++++++++++++++++++---------
 gdb/testsuite/gdb.cp/nsusing.exp | 13 ++++-
 2 files changed, 74 insertions(+), 25 deletions(-)

diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index 6ecb29fb1ac..49c6ed8a54c 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
@@ -370,9 +371,13 @@ cp_lookup_symbol_in_namespace (const char *the_namespace, const char *name,
    only the import of Y is considered.
 
    SEARCH_SCOPE_FIRST is an internal implementation detail: Callers must
-   pass 0 for it.  Internally we pass 1 when recursing.  */
+   pass 0 for it.  Internally we pass 1 when recursing.
 
-static struct block_symbol
+   When recursing, the function may return a map with multiple elements,
+   corresponding to the symbols that were found in the lower scopes.  The
+   top level call will never return a map with more than one element.  */
+
+static std::map<std::string, struct block_symbol>
 cp_lookup_symbol_via_imports (const char *scope,
 			      const char *name,
 			      const struct block *block,
@@ -386,13 +391,20 @@ cp_lookup_symbol_via_imports (const char *scope,
   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.  */
+  std::map<std::string, struct block_symbol> found_symbols;
+
   /* 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,45 @@ 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,
+	      std::map<std::string, struct block_symbol> sym_map
+		  = cp_lookup_symbol_via_imports (current->import_src,
 						  name, block,
 						  domain, 1, 0, 0);
+	      found_symbols.merge(sym_map);
 	    }
 
-	  if (sym.symbol != NULL)
-	    return sym;
 	}
     }
 
-  return {};
+  /* We only want to trigger this error on the top level call, otherwise
+     the error may only have a partial list of the possible symbols.  */
+  if (search_scope_first == 0 && 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 ());
+    }
+  else
+    return found_symbols;
+
+  /* This is needed to silence a -Werror=return-type warning, because
+     the above if case doesn't have a return statement.  */
+  gdb_assert_not_reached ();
 }
 
 /* Helper function that searches an array of symbols for one named NAME.  */
@@ -503,9 +537,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 +549,6 @@ cp_lookup_symbol_imports_or_template (const char *scope,
 				      const domain_enum domain)
 {
   struct symbol *function = block->function ();
-  struct block_symbol result;
 
   if (symbol_lookup_debug)
     {
@@ -596,15 +630,21 @@ cp_lookup_symbol_imports_or_template (const char *scope,
 	}
     }
 
-  result = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1);
+  /* Despite getting a map, it should have at most one element, otherwise
+     cp_lookup_symbol_via_import will have already reported the ambiguity.  */
+  std::map<std::string, struct block_symbol> result
+    = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1);
   if (symbol_lookup_debug)
     {
       gdb_printf (gdb_stdlog,
 		  "cp_lookup_symbol_imports_or_template (...) = %s\n",
-		  result.symbol != NULL
-		  ? host_address_to_string (result.symbol) : "NULL");
+		  result.size() == 1
+		  ? host_address_to_string (result[0].symbol) : "NULL");
     }
-  return result;
+  if (result.empty ())
+    return {};
+  else
+    return result.begin ()->second;
 }
 
 /* Search for NAME by applying relevant import statements belonging to BLOCK
@@ -616,13 +656,13 @@ cp_lookup_symbol_via_all_imports (const char *scope, const char *name,
 				  const struct block *block,
 				  const domain_enum domain)
 {
-  struct block_symbol sym;
+  std::map<std::string, struct block_symbol> sym;
 
   while (block != NULL)
     {
       sym = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 0, 1);
-      if (sym.symbol)
-	return sym;
+      if (sym.size() == 1)
+	return sym.begin ()->second;
 
       block = block->superblock ();
     }
diff --git a/gdb/testsuite/gdb.cp/nsusing.exp b/gdb/testsuite/gdb.cp/nsusing.exp
index b79f3d26084..4ef0f48c507 100644
--- a/gdb/testsuite/gdb.cp/nsusing.exp
+++ b/gdb/testsuite/gdb.cp/nsusing.exp
@@ -127,11 +127,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