[PATCH v2 0/2] Improve handling of using directives

[PATCH v2 0/2] Improve handling of using directives

[Bruno Larsen]

This series tries to fix two of the main issues I saw when trying to get
gdb.cp/nsusing.exp working with clang. The first issue was that GDB
wouldn't care about when the 'using' directive happened in the code,
even if we were stopped before it, it was considered valid. The second
was that GDB would find the first reasonable variable in the imported
declarations and leave early, not caring about whether that variable was
ambiguous or not. Each of my patches fixes one of those issues.

Changelog for v2:
 * factored out some code to avoid unnecessary repetition.
 * made it so ambiguous variables are explicitly reported
 * fixed formatting issues.

Bruno Larsen (2):
  gdb/c++: validate 'using' directives based on the current line
  gdb/c++: Detect ambiguous variables in imported namespaces

 gdb/cp-namespace.c               | 82 ++++++++++++++++++++++++--------
 gdb/dwarf2/read.c                | 25 +++++++++-
 gdb/namespace.c                  | 25 ++++++++++
 gdb/namespace.h                  | 16 ++++++-
 gdb/testsuite/gdb.cp/nsusing.cc  |  3 +-
 gdb/testsuite/gdb.cp/nsusing.exp | 25 ++++++++--
 6 files changed, 149 insertions(+), 27 deletions(-)

-- 
2.38.1

[PATCH v2 1/2] gdb/c++: validate 'using' directives based on the current line

[Bruno Larsen]

When asking GDB to print a variable from an imported namespace, we only
want to see variables imported in lines that the inferior has already
gone through, as is being tested last in gdb.cp/nsusing.exp. However
with the proposed change to gdb.cp/nsusing.exp, we get the following
failures:

(gdb) PASS: gdb.cp/nsusing.exp: continue to breakpoint: marker10 stop
print x
$9 = 911
(gdb) FAIL: gdb.cp/nsusing.exp: print x, before using statement
next
15        y += x;
(gdb) PASS: gdb.cp/nsusing.exp: using namespace M
print x
$10 = 911
(gdb) PASS: gdb.cp/nsusing.exp: print x, only using M

Showing that the feature wasn't functioning properly, it just so
happened that gcc ordered the namespaces in a convenient way.
This happens because GDB doesn't take into account the line where the
"using namespace" directive is written. So long as it shows up in the
current scope, we assume it is valid.

To fix this, add a new member to struct using_direct, that stores the
line where the directive was written, and a new function that informs if
the using directive is valid already.

Unfortunately, due to a GCC bug, the failure still shows up. Compilers
that set the declaration line of the using directive correctly (such as
Clang) do not show such a bug, so the test includes an XFAIL for gcc
code.

Finally, because the final test of gdb.cp/nsusing.exp has turned into
multiple that all would need XFAILs for older GCCs (<= 4.3), and that
GCC is very old, if it is detected, the test just exits early.
---
 gdb/cp-namespace.c               | 15 ++++++++++++---
 gdb/dwarf2/read.c                | 25 ++++++++++++++++++++++++-
 gdb/namespace.c                  | 25 +++++++++++++++++++++++++
 gdb/namespace.h                  | 16 +++++++++++++++-
 gdb/testsuite/gdb.cp/nsusing.cc  |  3 ++-
 gdb/testsuite/gdb.cp/nsusing.exp | 16 +++++++++++++---
 6 files changed, 91 insertions(+), 9 deletions(-)

diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index 634dab6ada0..6ecb29fb1ac 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -93,10 +93,12 @@ cp_scan_for_anonymous_namespaces (struct buildsym_compunit *compunit,
 	      /* We've found a component of the name that's an
 		 anonymous namespace.  So add symbols in it to the
 		 namespace given by the previous component if there is
-		 one, or to the global namespace if there isn't.  */
+		 one, or to the global namespace if there isn't.
+		 The declared line of this using directive can be set
+		 to 0, this way it is always considered valid.  */
 	      std::vector<const char *> excludes;
 	      add_using_directive (compunit->get_local_using_directives (),
-				   dest, src, NULL, NULL, excludes,
+				   dest, src, NULL, NULL, excludes, 0,
 				   1, &objfile->objfile_obstack);
 	    }
 	  /* The "+ 2" is for the "::".  */
@@ -392,16 +394,23 @@ cp_lookup_symbol_via_imports (const char *scope,
   if (sym.symbol != NULL)
     return 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.  */
+  symtab_and_line boundary_sal = find_pc_line (block->end () - 1, 0);
+
   /* Go through the using directives.  If any of them add new names to
      the namespace we're searching in, see if we can find a match by
      applying them.  */
-
   for (current = block_using (block);
        current != NULL;
        current = current->next)
     {
       const char **excludep;
 
+      /* If the using directive was below the place we are stopped at,
+	 do not use this directive.  */
+      if (!current->valid_line (boundary_sal.line))
+	continue;
       len = strlen (current->import_dest);
       directive_match = (search_parents
 			 ? (startswith (scope, current->import_dest)
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 60e120a9d76..68e3149a4bb 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -9299,6 +9299,26 @@ using_directives (struct dwarf2_cu *cu)
     return cu->get_builder ()->get_local_using_directives ();
 }
 
+/* Read the DW_ATTR_decl_line attribute for the given DIE in the
+   given CU.  If the format is not recognized or the attribute is
+   not present, set it to 0.  */
+
+static unsigned int
+read_decl_line (struct die_info *die, struct dwarf2_cu *cu)
+{
+
+  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);
+  if (decl_line == nullptr)
+    return 0;
+  if (decl_line->form_is_constant ())
+    return decl_line->constant_value (0);
+  else if (decl_line->form_is_unsigned ())
+    return decl_line->as_unsigned ();
+
+  complaint (_("Declared line for using directive is of incorrect format"));
+  return 0;
+}
+
 /* Read the import statement specified by the given die and record it.  */
 
 static void
@@ -9441,6 +9461,7 @@ read_import_statement (struct die_info *die, struct dwarf2_cu *cu)
 		       import_alias,
 		       imported_declaration,
 		       excludes,
+		       read_decl_line (die, cu),
 		       0,
 		       &objfile->objfile_obstack);
 }
@@ -16078,7 +16099,9 @@ read_namespace (struct die_info *die, struct dwarf2_cu *cu)
 	  std::vector<const char *> excludes;
 	  add_using_directive (using_directives (cu),
 			       previous_prefix, type->name (), NULL,
-			       NULL, excludes, 0, &objfile->objfile_obstack);
+			       NULL, excludes,
+			       read_decl_line (die, cu),
+			       0, &objfile->objfile_obstack);
 	}
     }
 
diff --git a/gdb/namespace.c b/gdb/namespace.c
index 0c39c921a3e..b2cca5a1da4 100644
--- a/gdb/namespace.c
+++ b/gdb/namespace.c
@@ -18,6 +18,7 @@
 
 #include "defs.h"
 #include "namespace.h"
+#include "frame.h"
 
 /* Add a using directive to USING_DIRECTIVES.  If the using directive
    in question has already been added, don't add it twice.
@@ -40,6 +41,7 @@ add_using_directive (struct using_direct **using_directives,
 		     const char *alias,
 		     const char *declaration,
 		     const std::vector<const char *> &excludes,
+		     unsigned int decl_line,
 		     int copy_names,
 		     struct obstack *obstack)
 {
@@ -76,6 +78,9 @@ add_using_directive (struct using_direct **using_directives,
       if (ix < excludes.size () || current->excludes[ix] != NULL)
 	continue;
 
+      if (decl_line != current->decl_line)
+	continue;
+
       /* Parameters exactly match CURRENT.  */
       return;
     }
@@ -111,6 +116,26 @@ add_using_directive (struct using_direct **using_directives,
 	    excludes.size () * sizeof (*newobj->excludes));
   newobj->excludes[excludes.size ()] = NULL;
 
+  newobj->decl_line = decl_line;
+
   newobj->next = *using_directives;
   *using_directives = newobj;
 }
+
+/* See namespace.h.  */
+
+bool
+using_direct::valid_line (unsigned int boundary) const
+{
+  try
+    {
+      CORE_ADDR curr_pc = get_frame_pc (get_selected_frame (nullptr));
+      symtab_and_line curr_sal = find_pc_line (curr_pc, 0);
+      return (decl_line <= curr_sal.line)
+	     || (decl_line >= boundary);
+    }
+  catch (const gdb_exception &ex)
+    {
+      return true;
+    }
+}
diff --git a/gdb/namespace.h b/gdb/namespace.h
index dc052a44e42..b46806684c8 100644
--- a/gdb/namespace.h
+++ b/gdb/namespace.h
@@ -30,7 +30,8 @@
    string representing the alias.  Otherwise, ALIAS is NULL.
    DECLARATION is the name of the imported declaration, if this import
    statement represents one.  Otherwise DECLARATION is NULL and this
-   import statement represents a namespace.
+   import statement represents a namespace.  DECL_LINE is the line
+   where the using directive is written in the source code.
 
    C++:      using namespace A;
    Fortran:  use A
@@ -96,6 +97,11 @@ struct using_direct
 
   struct using_direct *next;
 
+  /* The line where the using directive was declared on the source file.
+     This is used to check if the using directive is already active at the
+     point where the inferior is stopped.  */
+  unsigned int decl_line;
+
   /* Used during import search to temporarily mark this node as
      searched.  */
   int searched;
@@ -103,6 +109,13 @@ struct using_direct
   /* USING_DIRECT has variable allocation size according to the number of
      EXCLUDES entries, the last entry is NULL.  */
   const char *excludes[1];
+
+  /* Returns true if the using_direcive USING_DIR is valid in CURR_LINE.
+     Because current GCC (at least version 12.2) sets the decl_line as
+     the last line in the current block, we need to take this into
+     consideration when checking the validity, by comparing it to
+     BOUNDARY, the last line of the current block.  */
+  bool valid_line (unsigned int boundary) const;
 };
 
 extern void add_using_directive (struct using_direct **using_directives,
@@ -111,6 +124,7 @@ extern void add_using_directive (struct using_direct **using_directives,
 				 const char *alias,
 				 const char *declaration,
 				 const std::vector<const char *> &excludes,
+				 const unsigned int decl_line,
 				 int copy_names,
 				 struct obstack *obstack);
 
diff --git a/gdb/testsuite/gdb.cp/nsusing.cc b/gdb/testsuite/gdb.cp/nsusing.cc
index fa5c9d01f59..dcf0ba99e22 100644
--- a/gdb/testsuite/gdb.cp/nsusing.cc
+++ b/gdb/testsuite/gdb.cp/nsusing.cc
@@ -10,8 +10,9 @@ namespace N
 
 int marker10 ()
 {
+  int y = 1; // marker10 stop
   using namespace M;
-  int y = x + 1; // marker10 stop
+  y += x;
   using namespace N;
   return y;
 }
diff --git a/gdb/testsuite/gdb.cp/nsusing.exp b/gdb/testsuite/gdb.cp/nsusing.exp
index 2835207a21e..b79f3d26084 100644
--- a/gdb/testsuite/gdb.cp/nsusing.exp
+++ b/gdb/testsuite/gdb.cp/nsusing.exp
@@ -120,8 +120,18 @@ gdb_continue_to_breakpoint "marker10 stop"
 
 if { [test_compiler_info {gcc-[0-3]-*}] ||
      [test_compiler_info {gcc-4-[0-3]-*}]} {
-    setup_xfail *-*-*
+    return
 }
 
-# Assert that M::x is printed and not N::x
-gdb_test "print x" "= 911" "print x (from M::x)"
+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.*" {
+	# 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"
-- 
2.38.1

[PATCH v2 2/2] 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 map containing zero or one entries.

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

diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index 6ecb29fb1ac..df177b20d92 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
@@ -372,7 +373,7 @@ 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 std::map<const char *, struct block_symbol>
 cp_lookup_symbol_via_imports (const char *scope,
 			      const char *name,
 			      const struct block *block,
@@ -386,13 +387,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<const char *,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 +454,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 +475,43 @@ 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<const char *,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 {};
+  if (found_symbols.size () > 1)
+    {
+      auto itr = found_symbols.begin();
+      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.  */
@@ -514,7 +542,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 +623,19 @@ cp_lookup_symbol_imports_or_template (const char *scope,
 	}
     }
 
-  result = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1);
+  std::map<const char *,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 +647,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<const char *,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

Re: [PATCH v2 1/2] gdb/c++: validate 'using' directives based on the current line

[Lancelot SIX]

Hi Bruno,

I have included comments inlined in the patch.

On Wed, Nov 16, 2022 at 03:13:36PM +0100, Bruno Larsen via Gdb-patches wrote:
> When asking GDB to print a variable from an imported namespace, we only
> want to see variables imported in lines that the inferior has already
> gone through, as is being tested last in gdb.cp/nsusing.exp. However
> with the proposed change to gdb.cp/nsusing.exp, we get the following
> failures:
> 
> (gdb) PASS: gdb.cp/nsusing.exp: continue to breakpoint: marker10 stop
> print x
> $9 = 911
> (gdb) FAIL: gdb.cp/nsusing.exp: print x, before using statement
> next
> 15        y += x;
> (gdb) PASS: gdb.cp/nsusing.exp: using namespace M
> print x
> $10 = 911
> (gdb) PASS: gdb.cp/nsusing.exp: print x, only using M
> 
> Showing that the feature wasn't functioning properly, it just so
> happened that gcc ordered the namespaces in a convenient way.
> This happens because GDB doesn't take into account the line where the
> "using namespace" directive is written. So long as it shows up in the
> current scope, we assume it is valid.
> 
> To fix this, add a new member to struct using_direct, that stores the
> line where the directive was written, and a new function that informs if
> the using directive is valid already.
> 
> Unfortunately, due to a GCC bug, the failure still shows up. Compilers
> that set the declaration line of the using directive correctly (such as
> Clang) do not show such a bug, so the test includes an XFAIL for gcc
> code.
> 
> Finally, because the final test of gdb.cp/nsusing.exp has turned into
> multiple that all would need XFAILs for older GCCs (<= 4.3), and that
> GCC is very old, if it is detected, the test just exits early.
> ---
>  gdb/cp-namespace.c               | 15 ++++++++++++---
>  gdb/dwarf2/read.c                | 25 ++++++++++++++++++++++++-
>  gdb/namespace.c                  | 25 +++++++++++++++++++++++++
>  gdb/namespace.h                  | 16 +++++++++++++++-
>  gdb/testsuite/gdb.cp/nsusing.cc  |  3 ++-
>  gdb/testsuite/gdb.cp/nsusing.exp | 16 +++++++++++++---
>  6 files changed, 91 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
> index 634dab6ada0..6ecb29fb1ac 100644
> --- a/gdb/cp-namespace.c
> +++ b/gdb/cp-namespace.c
> @@ -93,10 +93,12 @@ cp_scan_for_anonymous_namespaces (struct buildsym_compunit *compunit,
>  	      /* We've found a component of the name that's an
>  		 anonymous namespace.  So add symbols in it to the
>  		 namespace given by the previous component if there is
> -		 one, or to the global namespace if there isn't.  */
> +		 one, or to the global namespace if there isn't.
> +		 The declared line of this using directive can be set
> +		 to 0, this way it is always considered valid.  */
>  	      std::vector<const char *> excludes;
>  	      add_using_directive (compunit->get_local_using_directives (),
> -				   dest, src, NULL, NULL, excludes,
> +				   dest, src, NULL, NULL, excludes, 0,
>  				   1, &objfile->objfile_obstack);
>  	    }
>  	  /* The "+ 2" is for the "::".  */
> @@ -392,16 +394,23 @@ cp_lookup_symbol_via_imports (const char *scope,
>    if (sym.symbol != NULL)
>      return 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.  */
> +  symtab_and_line boundary_sal = find_pc_line (block->end () - 1, 0);
> +
>    /* Go through the using directives.  If any of them add new names to
>       the namespace we're searching in, see if we can find a match by
>       applying them.  */
> -
>    for (current = block_using (block);
>         current != NULL;
>         current = current->next)
>      {
>        const char **excludep;
>  
> +      /* If the using directive was below the place we are stopped at,
> +	 do not use this directive.  */
> +      if (!current->valid_line (boundary_sal.line))
> +	continue;
>        len = strlen (current->import_dest);
>        directive_match = (search_parents
>  			 ? (startswith (scope, current->import_dest)
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 60e120a9d76..68e3149a4bb 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -9299,6 +9299,26 @@ using_directives (struct dwarf2_cu *cu)
>      return cu->get_builder ()->get_local_using_directives ();
>  }
>  
> +/* Read the DW_ATTR_decl_line attribute for the given DIE in the
> +   given CU.  If the format is not recognized or the attribute is
> +   not present, set it to 0.  */
> +
> +static unsigned int
> +read_decl_line (struct die_info *die, struct dwarf2_cu *cu)
> +{
> +
> +  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);
> +  if (decl_line == nullptr)
> +    return 0;
> +  if (decl_line->form_is_constant ())
> +    return decl_line->constant_value (0);

This is probably me being pedantic here, but constant_value return a
LONGEST (i.e. long on x86_64) while read_decl_line returns an unsigned
int.

I really do not expect any realistic scenario where a line number goes
above UINT_MAX, but I can easily imagine a buggy producer giving a
negative value which would end up trash after the cast to unsigned int.
Should we check that "0 <= decl_line->constant_value (0) <= UINT_MAX" ?

> +  else if (decl_line->form_is_unsigned ())

I do not see when this case should be possible.  The DW_AT_decl_line
attribute is of class "constant" (so one of DW_FORM_data[1,2,4,8,16],
DW_FORM_[s,u]data] or DW_FORM_implicit_const.  The only case not covered
by form_is_constant is DW_FORM_data16 and it is not covered by
form_is_unsigned either.  I do believe that this is more a problem in
attribute::form_is_constant / attribute::constant_value.  Of course, the
problem is that attribute::constant_value signature does not allow to
return a 128bits value, but this is a question out of scope of this
patch.

Calling form_is_unsigned can return true if the form is DW_FORM_ref_addr
or DW_FORM_sec_offset which would not make much sense in my opinion.

I think I would remove this "else if" block completely as getting there
would imply invalid DWARF.  In such situation, I think returning 0 would
the right thing to do.

Best,
Lancelot.

> +    return decl_line->as_unsigned ();
> +
> +  complaint (_("Declared line for using directive is of incorrect format"));
> +  return 0;
> +}
> +
>  /* Read the import statement specified by the given die and record it.  */
>  
>  static void
> @@ -9441,6 +9461,7 @@ read_import_statement (struct die_info *die, struct dwarf2_cu *cu)
>  		       import_alias,
>  		       imported_declaration,
>  		       excludes,
> +		       read_decl_line (die, cu),
>  		       0,
>  		       &objfile->objfile_obstack);
>  }
> @@ -16078,7 +16099,9 @@ read_namespace (struct die_info *die, struct dwarf2_cu *cu)
>  	  std::vector<const char *> excludes;
>  	  add_using_directive (using_directives (cu),
>  			       previous_prefix, type->name (), NULL,
> -			       NULL, excludes, 0, &objfile->objfile_obstack);
> +			       NULL, excludes,
> +			       read_decl_line (die, cu),
> +			       0, &objfile->objfile_obstack);
>  	}
>      }
>  
> diff --git a/gdb/namespace.c b/gdb/namespace.c
> index 0c39c921a3e..b2cca5a1da4 100644
> --- a/gdb/namespace.c
> +++ b/gdb/namespace.c
> @@ -18,6 +18,7 @@
>  
>  #include "defs.h"
>  #include "namespace.h"
> +#include "frame.h"
>  
>  /* Add a using directive to USING_DIRECTIVES.  If the using directive
>     in question has already been added, don't add it twice.
> @@ -40,6 +41,7 @@ add_using_directive (struct using_direct **using_directives,
>  		     const char *alias,
>  		     const char *declaration,
>  		     const std::vector<const char *> &excludes,
> +		     unsigned int decl_line,
>  		     int copy_names,
>  		     struct obstack *obstack)
>  {
> @@ -76,6 +78,9 @@ add_using_directive (struct using_direct **using_directives,
>        if (ix < excludes.size () || current->excludes[ix] != NULL)
>  	continue;
>  
> +      if (decl_line != current->decl_line)
> +	continue;
> +
>        /* Parameters exactly match CURRENT.  */
>        return;
>      }
> @@ -111,6 +116,26 @@ add_using_directive (struct using_direct **using_directives,
>  	    excludes.size () * sizeof (*newobj->excludes));
>    newobj->excludes[excludes.size ()] = NULL;
>  
> +  newobj->decl_line = decl_line;
> +
>    newobj->next = *using_directives;
>    *using_directives = newobj;
>  }
> +
> +/* See namespace.h.  */
> +
> +bool
> +using_direct::valid_line (unsigned int boundary) const
> +{
> +  try
> +    {
> +      CORE_ADDR curr_pc = get_frame_pc (get_selected_frame (nullptr));
> +      symtab_and_line curr_sal = find_pc_line (curr_pc, 0);
> +      return (decl_line <= curr_sal.line)
> +	     || (decl_line >= boundary);
> +    }
> +  catch (const gdb_exception &ex)
> +    {
> +      return true;
> +    }
> +}
> diff --git a/gdb/namespace.h b/gdb/namespace.h
> index dc052a44e42..b46806684c8 100644
> --- a/gdb/namespace.h
> +++ b/gdb/namespace.h
> @@ -30,7 +30,8 @@
>     string representing the alias.  Otherwise, ALIAS is NULL.
>     DECLARATION is the name of the imported declaration, if this import
>     statement represents one.  Otherwise DECLARATION is NULL and this
> -   import statement represents a namespace.
> +   import statement represents a namespace.  DECL_LINE is the line
> +   where the using directive is written in the source code.
>  
>     C++:      using namespace A;
>     Fortran:  use A
> @@ -96,6 +97,11 @@ struct using_direct
>  
>    struct using_direct *next;
>  
> +  /* The line where the using directive was declared on the source file.
> +     This is used to check if the using directive is already active at the
> +     point where the inferior is stopped.  */
> +  unsigned int decl_line;
> +
>    /* Used during import search to temporarily mark this node as
>       searched.  */
>    int searched;
> @@ -103,6 +109,13 @@ struct using_direct
>    /* USING_DIRECT has variable allocation size according to the number of
>       EXCLUDES entries, the last entry is NULL.  */
>    const char *excludes[1];
> +
> +  /* Returns true if the using_direcive USING_DIR is valid in CURR_LINE.
> +     Because current GCC (at least version 12.2) sets the decl_line as
> +     the last line in the current block, we need to take this into
> +     consideration when checking the validity, by comparing it to
> +     BOUNDARY, the last line of the current block.  */
> +  bool valid_line (unsigned int boundary) const;
>  };
>  
>  extern void add_using_directive (struct using_direct **using_directives,
> @@ -111,6 +124,7 @@ extern void add_using_directive (struct using_direct **using_directives,
>  				 const char *alias,
>  				 const char *declaration,
>  				 const std::vector<const char *> &excludes,
> +				 const unsigned int decl_line,
>  				 int copy_names,
>  				 struct obstack *obstack);
>  
> diff --git a/gdb/testsuite/gdb.cp/nsusing.cc b/gdb/testsuite/gdb.cp/nsusing.cc
> index fa5c9d01f59..dcf0ba99e22 100644
> --- a/gdb/testsuite/gdb.cp/nsusing.cc
> +++ b/gdb/testsuite/gdb.cp/nsusing.cc
> @@ -10,8 +10,9 @@ namespace N
>  
>  int marker10 ()
>  {
> +  int y = 1; // marker10 stop
>    using namespace M;
> -  int y = x + 1; // marker10 stop
> +  y += x;
>    using namespace N;
>    return y;
>  }
> diff --git a/gdb/testsuite/gdb.cp/nsusing.exp b/gdb/testsuite/gdb.cp/nsusing.exp
> index 2835207a21e..b79f3d26084 100644
> --- a/gdb/testsuite/gdb.cp/nsusing.exp
> +++ b/gdb/testsuite/gdb.cp/nsusing.exp
> @@ -120,8 +120,18 @@ gdb_continue_to_breakpoint "marker10 stop"
>  
>  if { [test_compiler_info {gcc-[0-3]-*}] ||
>       [test_compiler_info {gcc-4-[0-3]-*}]} {
> -    setup_xfail *-*-*
> +    return
>  }
>  
> -# Assert that M::x is printed and not N::x
> -gdb_test "print x" "= 911" "print x (from M::x)"
> +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.*" {
> +	# 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"
> -- 
> 2.38.1
> 

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

[Lancelot SIX]

Hi Bruno,

Thanks for working on this.  I have included remarks inlined in the path.

On Wed, Nov 16, 2022 at 03:13:37PM +0100, Bruno Larsen via Gdb-patches 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 map containing zero or one entries.
> 
> The commit also changes gdb.cp/nsusing.exp to test the ambiguous
> detection.
> ---
>  gdb/cp-namespace.c               | 69 +++++++++++++++++++++++---------
>  gdb/testsuite/gdb.cp/nsusing.exp | 13 +++++-
>  2 files changed, 61 insertions(+), 21 deletions(-)
> 
> diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
> index 6ecb29fb1ac..df177b20d92 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
> @@ -372,7 +373,7 @@ 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.  */

Should the function's description be updated to reflect the new return
type? i.e. describe a case where a map of more than 1 element is
returned.

>  
> -static struct block_symbol
> +static std::map<const char *, struct block_symbol>
>  cp_lookup_symbol_via_imports (const char *scope,
>  			      const char *name,
>  			      const struct block *block,
> @@ -386,13 +387,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<const char *,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 +454,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;

Looks like you have changed the indentation here with two more spaces.

>  
>  	      continue;
>  	    }
> @@ -467,23 +475,43 @@ 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<const char *,struct block_symbol> sym_map =
> +		    cp_lookup_symbol_via_imports (current->import_src,
>  						  name, block,
>  						  domain, 1, 0, 0);

I think the usual coding style is to have the "=" after the line break.
This would become

    std::map<const char *,struct block_symbol> sym_map
      = cp_lookup_symbol_via_imports (…);

> +	      found_symbols.merge(sym_map);
>  	    }
>  
> -	  if (sym.symbol != NULL)
> -	    return sym;
>  	}
>      }
>  
> -  return {};
> +  if (found_symbols.size () > 1)
> +    {
> +      auto itr = found_symbols.begin();

Could it be `cbegin ()` ? (and you forgot a space before the parens).

> +      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 ());

I do not know if this something we care about, but your error message
will list the options in an unspecified order.  Iterating over the map
will follow the order of the keys, which are the addresses of the
mangled names of the symbols.

My instinct would prefer to list the options in lexicographical order
(of the demangled name).  This is probably not really important but it
could be annoying when writing test checking for the output message.

> +    }
> +  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.  */
> @@ -514,7 +542,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 +623,19 @@ cp_lookup_symbol_imports_or_template (const char *scope,
>  	}
>      }
>  
> -  result = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1);
> +  std::map<const char *,struct block_symbol> result =
> +      cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1);

Same remark for the "=" before the line break.

>    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");

If result is a std::map<const char *, T>, result[0] will look for the
key (const char *) 0, which is most probably in the map.  It therefore
creates a new instance of T using the default ctor, insert it in the map
for key 0 and returns a reference to it.

I think yoy want something like result.begin ().second.symbol or
something along those lines like you do…

>      }
> -  return result;
> +  if (result.empty ())
> +    return {};
> +  else
> +    return result.begin ()->second;

… here!

This however raises the question: if cp_lookup_symbol_via_imports can
return multiple block_symbol, shouldn't
cp_lookup_symbol_imports_or_template do the same when it basically
forwards what cp_lookup_symbol_via_imports returns?

I am not really familiar with this part of the code, so I do not know if
this can happen (but I guess it could).

I would say that:
- If the map can only contain one element and you only introduce it to
  hold the result of cp_lookup_symbol_via_imports, then you should
  assert that its size is 1 here (and maybe have a comment explaining
  why it must be a map of size 1).
- If it can be anything else, I guess you would need to return the
  entire map, or have a comment explaining why any member could do.

As the function's comment says "Like cp_lookup_symbol_via_imports, but
[…]" I am tempted to think that it should also return a map.

I hope those help.
All the best,
Lancelot.

>  }
>  
>  /* Search for NAME by applying relevant import statements belonging to BLOCK
> @@ -616,13 +647,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<const char *,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
> 

Re: [PATCH v2 1/2] gdb/c++: validate 'using' directives based on the current line

[Bruno Larsen]

On 16/11/2022 17:14, Lancelot SIX wrote:
> Hi Bruno,
>
> I have included comments inlined in the patch.
>
> On Wed, Nov 16, 2022 at 03:13:36PM +0100, Bruno Larsen via Gdb-patches wrote:
>> When asking GDB to print a variable from an imported namespace, we only
>> want to see variables imported in lines that the inferior has already
>> gone through, as is being tested last in gdb.cp/nsusing.exp. However
>> with the proposed change to gdb.cp/nsusing.exp, we get the following
>> failures:
>>
>> (gdb) PASS: gdb.cp/nsusing.exp: continue to breakpoint: marker10 stop
>> print x
>> $9 = 911
>> (gdb) FAIL: gdb.cp/nsusing.exp: print x, before using statement
>> next
>> 15        y += x;
>> (gdb) PASS: gdb.cp/nsusing.exp: using namespace M
>> print x
>> $10 = 911
>> (gdb) PASS: gdb.cp/nsusing.exp: print x, only using M
>>
>> Showing that the feature wasn't functioning properly, it just so
>> happened that gcc ordered the namespaces in a convenient way.
>> This happens because GDB doesn't take into account the line where the
>> "using namespace" directive is written. So long as it shows up in the
>> current scope, we assume it is valid.
>>
>> To fix this, add a new member to struct using_direct, that stores the
>> line where the directive was written, and a new function that informs if
>> the using directive is valid already.
>>
>> Unfortunately, due to a GCC bug, the failure still shows up. Compilers
>> that set the declaration line of the using directive correctly (such as
>> Clang) do not show such a bug, so the test includes an XFAIL for gcc
>> code.
>>
>> Finally, because the final test of gdb.cp/nsusing.exp has turned into
>> multiple that all would need XFAILs for older GCCs (<= 4.3), and that
>> GCC is very old, if it is detected, the test just exits early.
>> ---
>>   gdb/cp-namespace.c               | 15 ++++++++++++---
>>   gdb/dwarf2/read.c                | 25 ++++++++++++++++++++++++-
>>   gdb/namespace.c                  | 25 +++++++++++++++++++++++++
>>   gdb/namespace.h                  | 16 +++++++++++++++-
>>   gdb/testsuite/gdb.cp/nsusing.cc  |  3 ++-
>>   gdb/testsuite/gdb.cp/nsusing.exp | 16 +++++++++++++---
>>   6 files changed, 91 insertions(+), 9 deletions(-)
>>
>> diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
>> index 634dab6ada0..6ecb29fb1ac 100644
>> --- a/gdb/cp-namespace.c
>> +++ b/gdb/cp-namespace.c
>> @@ -93,10 +93,12 @@ cp_scan_for_anonymous_namespaces (struct buildsym_compunit *compunit,
>>   	      /* We've found a component of the name that's an
>>   		 anonymous namespace.  So add symbols in it to the
>>   		 namespace given by the previous component if there is
>> -		 one, or to the global namespace if there isn't.  */
>> +		 one, or to the global namespace if there isn't.
>> +		 The declared line of this using directive can be set
>> +		 to 0, this way it is always considered valid.  */
>>   	      std::vector<const char *> excludes;
>>   	      add_using_directive (compunit->get_local_using_directives (),
>> -				   dest, src, NULL, NULL, excludes,
>> +				   dest, src, NULL, NULL, excludes, 0,
>>   				   1, &objfile->objfile_obstack);
>>   	    }
>>   	  /* The "+ 2" is for the "::".  */
>> @@ -392,16 +394,23 @@ cp_lookup_symbol_via_imports (const char *scope,
>>     if (sym.symbol != NULL)
>>       return 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.  */
>> +  symtab_and_line boundary_sal = find_pc_line (block->end () - 1, 0);
>> +
>>     /* Go through the using directives.  If any of them add new names to
>>        the namespace we're searching in, see if we can find a match by
>>        applying them.  */
>> -
>>     for (current = block_using (block);
>>          current != NULL;
>>          current = current->next)
>>       {
>>         const char **excludep;
>>   
>> +      /* If the using directive was below the place we are stopped at,
>> +	 do not use this directive.  */
>> +      if (!current->valid_line (boundary_sal.line))
>> +	continue;
>>         len = strlen (current->import_dest);
>>         directive_match = (search_parents
>>   			 ? (startswith (scope, current->import_dest)
>> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
>> index 60e120a9d76..68e3149a4bb 100644
>> --- a/gdb/dwarf2/read.c
>> +++ b/gdb/dwarf2/read.c
>> @@ -9299,6 +9299,26 @@ using_directives (struct dwarf2_cu *cu)
>>       return cu->get_builder ()->get_local_using_directives ();
>>   }
>>   
>> +/* Read the DW_ATTR_decl_line attribute for the given DIE in the
>> +   given CU.  If the format is not recognized or the attribute is
>> +   not present, set it to 0.  */
>> +
>> +static unsigned int
>> +read_decl_line (struct die_info *die, struct dwarf2_cu *cu)
>> +{
>> +
>> +  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);
>> +  if (decl_line == nullptr)
>> +    return 0;
>> +  if (decl_line->form_is_constant ())
>> +    return decl_line->constant_value (0);
> This is probably me being pedantic here, but constant_value return a
> LONGEST (i.e. long on x86_64) while read_decl_line returns an unsigned
> int.
>
> I really do not expect any realistic scenario where a line number goes
> above UINT_MAX, but I can easily imagine a buggy producer giving a
> negative value which would end up trash after the cast to unsigned int.
> Should we check that "0 <= decl_line->constant_value (0) <= UINT_MAX" ?
Sure, I could add a check and complaint here.
>
>> +  else if (decl_line->form_is_unsigned ())
> I do not see when this case should be possible.  The DW_AT_decl_line
> attribute is of class "constant" (so one of DW_FORM_data[1,2,4,8,16],
> DW_FORM_[s,u]data] or DW_FORM_implicit_const.  The only case not covered
> by form_is_constant is DW_FORM_data16 and it is not covered by
> form_is_unsigned either.  I do believe that this is more a problem in
> attribute::form_is_constant / attribute::constant_value.  Of course, the
> problem is that attribute::constant_value signature does not allow to
> return a 128bits value, but this is a question out of scope of this
> patch.
>
> Calling form_is_unsigned can return true if the form is DW_FORM_ref_addr
> or DW_FORM_sec_offset which would not make much sense in my opinion.
>
> I think I would remove this "else if" block completely as getting there
> would imply invalid DWARF.  In such situation, I think returning 0 would
> the right thing to do.

Well, now I'm very confused... When I was just starting out, I needed 
this to make the patch work with clang, but now it doesn't seem 
necessary anymore. Thanks for double checking it!

-- 
Cheers,
Bruno

>
> Best,
> Lancelot.
>
>> +    return decl_line->as_unsigned ();
>> +
>> +  complaint (_("Declared line for using directive is of incorrect format"));
>> +  return 0;
>> +}
>> +
>>   /* Read the import statement specified by the given die and record it.  */
>>   
>>   static void
>> @@ -9441,6 +9461,7 @@ read_import_statement (struct die_info *die, struct dwarf2_cu *cu)
>>   		       import_alias,
>>   		       imported_declaration,
>>   		       excludes,
>> +		       read_decl_line (die, cu),
>>   		       0,
>>   		       &objfile->objfile_obstack);
>>   }
>> @@ -16078,7 +16099,9 @@ read_namespace (struct die_info *die, struct dwarf2_cu *cu)
>>   	  std::vector<const char *> excludes;
>>   	  add_using_directive (using_directives (cu),
>>   			       previous_prefix, type->name (), NULL,
>> -			       NULL, excludes, 0, &objfile->objfile_obstack);
>> +			       NULL, excludes,
>> +			       read_decl_line (die, cu),
>> +			       0, &objfile->objfile_obstack);
>>   	}
>>       }
>>   
>> diff --git a/gdb/namespace.c b/gdb/namespace.c
>> index 0c39c921a3e..b2cca5a1da4 100644
>> --- a/gdb/namespace.c
>> +++ b/gdb/namespace.c
>> @@ -18,6 +18,7 @@
>>   
>>   #include "defs.h"
>>   #include "namespace.h"
>> +#include "frame.h"
>>   
>>   /* Add a using directive to USING_DIRECTIVES.  If the using directive
>>      in question has already been added, don't add it twice.
>> @@ -40,6 +41,7 @@ add_using_directive (struct using_direct **using_directives,
>>   		     const char *alias,
>>   		     const char *declaration,
>>   		     const std::vector<const char *> &excludes,
>> +		     unsigned int decl_line,
>>   		     int copy_names,
>>   		     struct obstack *obstack)
>>   {
>> @@ -76,6 +78,9 @@ add_using_directive (struct using_direct **using_directives,
>>         if (ix < excludes.size () || current->excludes[ix] != NULL)
>>   	continue;
>>   
>> +      if (decl_line != current->decl_line)
>> +	continue;
>> +
>>         /* Parameters exactly match CURRENT.  */
>>         return;
>>       }
>> @@ -111,6 +116,26 @@ add_using_directive (struct using_direct **using_directives,
>>   	    excludes.size () * sizeof (*newobj->excludes));
>>     newobj->excludes[excludes.size ()] = NULL;
>>   
>> +  newobj->decl_line = decl_line;
>> +
>>     newobj->next = *using_directives;
>>     *using_directives = newobj;
>>   }
>> +
>> +/* See namespace.h.  */
>> +
>> +bool
>> +using_direct::valid_line (unsigned int boundary) const
>> +{
>> +  try
>> +    {
>> +      CORE_ADDR curr_pc = get_frame_pc (get_selected_frame (nullptr));
>> +      symtab_and_line curr_sal = find_pc_line (curr_pc, 0);
>> +      return (decl_line <= curr_sal.line)
>> +	     || (decl_line >= boundary);
>> +    }
>> +  catch (const gdb_exception &ex)
>> +    {
>> +      return true;
>> +    }
>> +}
>> diff --git a/gdb/namespace.h b/gdb/namespace.h
>> index dc052a44e42..b46806684c8 100644
>> --- a/gdb/namespace.h
>> +++ b/gdb/namespace.h
>> @@ -30,7 +30,8 @@
>>      string representing the alias.  Otherwise, ALIAS is NULL.
>>      DECLARATION is the name of the imported declaration, if this import
>>      statement represents one.  Otherwise DECLARATION is NULL and this
>> -   import statement represents a namespace.
>> +   import statement represents a namespace.  DECL_LINE is the line
>> +   where the using directive is written in the source code.
>>   
>>      C++:      using namespace A;
>>      Fortran:  use A
>> @@ -96,6 +97,11 @@ struct using_direct
>>   
>>     struct using_direct *next;
>>   
>> +  /* The line where the using directive was declared on the source file.
>> +     This is used to check if the using directive is already active at the
>> +     point where the inferior is stopped.  */
>> +  unsigned int decl_line;
>> +
>>     /* Used during import search to temporarily mark this node as
>>        searched.  */
>>     int searched;
>> @@ -103,6 +109,13 @@ struct using_direct
>>     /* USING_DIRECT has variable allocation size according to the number of
>>        EXCLUDES entries, the last entry is NULL.  */
>>     const char *excludes[1];
>> +
>> +  /* Returns true if the using_direcive USING_DIR is valid in CURR_LINE.
>> +     Because current GCC (at least version 12.2) sets the decl_line as
>> +     the last line in the current block, we need to take this into
>> +     consideration when checking the validity, by comparing it to
>> +     BOUNDARY, the last line of the current block.  */
>> +  bool valid_line (unsigned int boundary) const;
>>   };
>>   
>>   extern void add_using_directive (struct using_direct **using_directives,
>> @@ -111,6 +124,7 @@ extern void add_using_directive (struct using_direct **using_directives,
>>   				 const char *alias,
>>   				 const char *declaration,
>>   				 const std::vector<const char *> &excludes,
>> +				 const unsigned int decl_line,
>>   				 int copy_names,
>>   				 struct obstack *obstack);
>>   
>> diff --git a/gdb/testsuite/gdb.cp/nsusing.cc b/gdb/testsuite/gdb.cp/nsusing.cc
>> index fa5c9d01f59..dcf0ba99e22 100644
>> --- a/gdb/testsuite/gdb.cp/nsusing.cc
>> +++ b/gdb/testsuite/gdb.cp/nsusing.cc
>> @@ -10,8 +10,9 @@ namespace N
>>   
>>   int marker10 ()
>>   {
>> +  int y = 1; // marker10 stop
>>     using namespace M;
>> -  int y = x + 1; // marker10 stop
>> +  y += x;
>>     using namespace N;
>>     return y;
>>   }
>> diff --git a/gdb/testsuite/gdb.cp/nsusing.exp b/gdb/testsuite/gdb.cp/nsusing.exp
>> index 2835207a21e..b79f3d26084 100644
>> --- a/gdb/testsuite/gdb.cp/nsusing.exp
>> +++ b/gdb/testsuite/gdb.cp/nsusing.exp
>> @@ -120,8 +120,18 @@ gdb_continue_to_breakpoint "marker10 stop"
>>   
>>   if { [test_compiler_info {gcc-[0-3]-*}] ||
>>        [test_compiler_info {gcc-4-[0-3]-*}]} {
>> -    setup_xfail *-*-*
>> +    return
>>   }
>>   
>> -# Assert that M::x is printed and not N::x
>> -gdb_test "print x" "= 911" "print x (from M::x)"
>> +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.*" {
>> +	# 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"
>> -- 
>> 2.38.1
>>

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

[Bruno Larsen]

On 16/11/2022 18:49, Lancelot SIX wrote:
> Hi Bruno,
>
> Thanks for working on this.  I have included remarks inlined in the path.
>
> On Wed, Nov 16, 2022 at 03:13:37PM +0100, Bruno Larsen via Gdb-patches 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 map containing zero or one entries.
>>
>> The commit also changes gdb.cp/nsusing.exp to test the ambiguous
>> detection.
>> ---
>>   gdb/cp-namespace.c               | 69 +++++++++++++++++++++++---------
>>   gdb/testsuite/gdb.cp/nsusing.exp | 13 +++++-
>>   2 files changed, 61 insertions(+), 21 deletions(-)
>>
>> diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
>> index 6ecb29fb1ac..df177b20d92 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
>> @@ -372,7 +373,7 @@ 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.  */
> Should the function's description be updated to reflect the new return
> type? i.e. describe a case where a map of more than 1 element is
> returned.
Yes, thanks!
>
>>   
>> -static struct block_symbol
>> +static std::map<const char *, struct block_symbol>
>>   cp_lookup_symbol_via_imports (const char *scope,
>>   			      const char *name,
>>   			      const struct block *block,
>> @@ -386,13 +387,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<const char *,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 +454,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;
> Looks like you have changed the indentation here with two more spaces.
>
>>   
>>   	      continue;
>>   	    }
>> @@ -467,23 +475,43 @@ 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<const char *,struct block_symbol> sym_map =
>> +		    cp_lookup_symbol_via_imports (current->import_src,
>>   						  name, block,
>>   						  domain, 1, 0, 0);
> I think the usual coding style is to have the "=" after the line break.
> This would become
>
>      std::map<const char *,struct block_symbol> sym_map
>        = cp_lookup_symbol_via_imports (…);
>
>> +	      found_symbols.merge(sym_map);
>>   	    }
>>   
>> -	  if (sym.symbol != NULL)
>> -	    return sym;
>>   	}
>>       }
>>   
>> -  return {};
>> +  if (found_symbols.size () > 1)
>> +    {
>> +      auto itr = found_symbols.begin();
> Could it be `cbegin ()` ? (and you forgot a space before the parens).
>
>> +      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 ());
> I do not know if this something we care about, but your error message
> will list the options in an unspecified order.  Iterating over the map
> will follow the order of the keys, which are the addresses of the
> mangled names of the symbols.
>
> My instinct would prefer to list the options in lexicographical order
> (of the demangled name).  This is probably not really important but it
> could be annoying when writing test checking for the output message.

Would you be OK with lexicological ordering of mangled names? That could 
be easily done by changing to std::map<std::string, T> and we keep all 
of the rest of the code.

>
>> +    }
>> +  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.  */
>> @@ -514,7 +542,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 +623,19 @@ cp_lookup_symbol_imports_or_template (const char *scope,
>>   	}
>>       }
>>   
>> -  result = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1);
>> +  std::map<const char *,struct block_symbol> result =
>> +      cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1);
> Same remark for the "=" before the line break.
>
>>     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");
> If result is a std::map<const char *, T>, result[0] will look for the
> key (const char *) 0, which is most probably in the map.  It therefore
> creates a new instance of T using the default ctor, insert it in the map
> for key 0 and returns a reference to it.
>
> I think yoy want something like result.begin ().second.symbol or
> something along those lines like you do…
Right, yes, I encountered this testing locally, but forgot to change 
this part. oops
>
>>       }
>> -  return result;
>> +  if (result.empty ())
>> +    return {};
>> +  else
>> +    return result.begin ()->second;
> … here!
>
> This however raises the question: if cp_lookup_symbol_via_imports can
> return multiple block_symbol, shouldn't
> cp_lookup_symbol_imports_or_template do the same when it basically
> forwards what cp_lookup_symbol_via_imports returns?

Well, you just found a different flaw in my logic... My plan is that the 
function will only return multiple values when being called recursively, 
and the top-level call would error out in case it found multiple 
symbols. This makes it so asserting that the size is 1 or reacting to 
different sizes redundant outside cp_lookup_symbol_via_imports.

However, I forgot to add the check for the function being top level 
(seeing if search_scope_first is 1), which could lead us to incomplete 
lists. I'll fix this and document this behavior better for v3

>
> I am not really familiar with this part of the code, so I do not know if
> this can happen (but I guess it could).
>
> I would say that:
> - If the map can only contain one element and you only introduce it to
>    hold the result of cp_lookup_symbol_via_imports, then you should
>    assert that its size is 1 here (and maybe have a comment explaining
>    why it must be a map of size 1).
As I mentioned above, before returning from the top-level call, 
cp_lookup_symbol_via_imports will already have errored out if there is 
more than one element. If you think it is important, I can document it. 
It felt self-explanatory to me, but I might just be in too deep at this 
point
> - If it can be anything else, I guess you would need to return the
>    entire map, or have a comment explaining why any member could do.
>
> As the function's comment says "Like cp_lookup_symbol_via_imports, but
> […]" I am tempted to think that it should also return a map.

I am thinking that maybe that comment should be rewritten. The reason 
for the call may be similar, but the internal workings look pretty 
different (mainly because it doesn't implement anything from 
cp_lookup_symbol_via_imports, just calls that function). I'm thinking 
something along the lines of:

search for symbols whose names match NAME. if BLOCK is a function, 
search through the template parameters and function type. Then (or if it 
is not a function) call cp_lookup_symbol_via imports to search for other 
symbols in the given SCOPE.

Or something of the sorts. What do you think?

-- 
Cheers,
Bruno

>
> I hope those help.
> All the best,
> Lancelot.
>
>>   }
>>   
>>   /* Search for NAME by applying relevant import statements belonging to BLOCK
>> @@ -616,13 +647,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<const char *,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
>>

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

[Lancelot SIX]

> > > +      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 ());
> > I do not know if this something we care about, but your error message
> > will list the options in an unspecified order.  Iterating over the map
> > will follow the order of the keys, which are the addresses of the
> > mangled names of the symbols.
> > 
> > My instinct would prefer to list the options in lexicographical order
> > (of the demangled name).  This is probably not really important but it
> > could be annoying when writing test checking for the output message.
> 
> Would you be OK with lexicological ordering of mangled names? That could be
> easily done by changing to std::map<std::string, T> and we keep all of the
> rest of the code.
> 

Yes, that sounds OK to me.  This is what I had in mind ;)

> > >       }
> > > -  return result;
> > > +  if (result.empty ())
> > > +    return {};
> > > +  else
> > > +    return result.begin ()->second;
> > … here!
> > 
> > This however raises the question: if cp_lookup_symbol_via_imports can
> > return multiple block_symbol, shouldn't
> > cp_lookup_symbol_imports_or_template do the same when it basically
> > forwards what cp_lookup_symbol_via_imports returns?
> 
> Well, you just found a different flaw in my logic... My plan is that the
> function will only return multiple values when being called recursively, and
> the top-level call would error out in case it found multiple symbols. This
> makes it so asserting that the size is 1 or reacting to different sizes
> redundant outside cp_lookup_symbol_via_imports.
> 
> However, I forgot to add the check for the function being top level (seeing
> if search_scope_first is 1), which could lead us to incomplete lists. I'll
> fix this and document this behavior better for v3
> 
> > 
> > I am not really familiar with this part of the code, so I do not know if
> > this can happen (but I guess it could).
> > 
> > I would say that:
> > - If the map can only contain one element and you only introduce it to
> >    hold the result of cp_lookup_symbol_via_imports, then you should
> >    assert that its size is 1 here (and maybe have a comment explaining
> >    why it must be a map of size 1).
> As I mentioned above, before returning from the top-level call,
> cp_lookup_symbol_via_imports will already have errored out if there is more
> than one element. If you think it is important, I can document it. It felt
> self-explanatory to me, but I might just be in too deep at this point
> > - If it can be anything else, I guess you would need to return the
> >    entire map, or have a comment explaining why any member could do.
> > 
> > As the function's comment says "Like cp_lookup_symbol_via_imports, but
> > […]" I am tempted to think that it should also return a map.
> 
> I am thinking that maybe that comment should be rewritten. The reason for
> the call may be similar, but the internal workings look pretty different
> (mainly because it doesn't implement anything from
> cp_lookup_symbol_via_imports, just calls that function). I'm thinking
> something along the lines of:
> 
> search for symbols whose names match NAME. if BLOCK is a function, search
> through the template parameters and function type. Then (or if it is not a
> function) call cp_lookup_symbol_via imports to search for other symbols in
> the given SCOPE.
> 
> Or something of the sorts. What do you think?

Yeah, rephrasing the comment could help.

Best,
Lancelot.