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

[PATCH v3 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 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.

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               | 99 ++++++++++++++++++++++++--------
 gdb/dwarf2/read.c                | 30 +++++++++-
 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, 167 insertions(+), 31 deletions(-)

-- 
2.38.1

[PATCH v3 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                | 30 +++++++++++++++++++++++++++++-
 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, 96 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..a19e97e873d 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -9299,6 +9299,31 @@ 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 ())
+    {
+      LONGEST val = decl_line->constant_value (0);
+      if (0 <= val && val <= UINT_MAX)
+	return (unsigned int) val;
+
+      complaint (_("Declared line for using directive is too large"));
+      return 0;
+    }
+
+  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 +9466,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 +16104,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 v3 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               | 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

Re: [PATCH v3 0/2] Improve handling of using directives

[Bruno Larsen]

Ping!


-- 
Cheers,
Bruno

On 22/11/2022 12:33, Bruno Larsen wrote:
> 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 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.
>
> 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               | 99 ++++++++++++++++++++++++--------
>   gdb/dwarf2/read.c                | 30 +++++++++-
>   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, 167 insertions(+), 31 deletions(-)
>

[PINGv2][PATCH v3 0/2] Improve handling of using directives

[Bruno Larsen]

Ping!
On 07/12/2022 14:53, Bruno Larsen wrote:
> Ping!
>
>
> -- 
> Cheers,
> Bruno
>
> On 22/11/2022 12:33, Bruno Larsen wrote:
>> 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 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.
>>
>> 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               | 99 ++++++++++++++++++++++++--------
>>   gdb/dwarf2/read.c                | 30 +++++++++-
>>   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, 167 insertions(+), 31 deletions(-)


-- 
Cheers,
Bruno

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

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

On Tue, Nov 22, 2022 at 12:52 PM Bruno Larsen via Gdb-patches <
gdb-patches@sourceware.org> wrote:

>
>
> 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.
>

I suggest using shorter sentences in general and I think the last sentence
needs
to be split in two. I had to read it 3 times to fully understand it.

>
> --
> 2.38.1
>
> I can confirm the patch applies cleanly on top of master and doesn't seem
to add new
failures to make check runs.

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

[Bruno Larsen]

On 15/12/2022 14:26, Alexandra Petlanova Hajkova via Gdb-patches wrote:
> On Tue, Nov 22, 2022 at 12:52 PM Bruno Larsen via Gdb-patches <
> gdb-patches@sourceware.org> wrote:
>
>>
>> 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.
>>
> I suggest using shorter sentences in general and I think the last sentence
> needs
> to be split in two. I had to read it 3 times to fully understand it.

Hi Sasha,

Thanks for the review! I'll try to keep my sentences short in the 
future. What do you think about changing the last paragraph to:

Finally, more tests were added to the end of gdb.cp/nsusing.exp. 
However, GCC version 4.3 or older already had troubles with the original 
test. If we detect that one of those old GCCs are used the test will 
just exit early instead of add many XFAILs.

-- 
Cheers,
Bruno

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

[Tom Tromey]

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

Bruno> +/* Read the DW_ATTR_decl_line attribute for the given DIE in the
Bruno> +   given CU.  If the format is not recognized or the attribute is
Bruno> +   not present, set it to 0.  */
Bruno> +
Bruno> +static unsigned int
Bruno> +read_decl_line (struct die_info *die, struct dwarf2_cu *cu)
Bruno> +{
Bruno> +
Bruno> +  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);

There's a spurious blank line here.
Ok with this fixed.

Tom

Re: [PATCH v3 2/2] 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.

Thanks for the patch.

Bruno> +      error (_("%s"), error_str.c_str ());
Bruno> +    }
Bruno> +  else
Bruno> +    return found_symbols;
Bruno> +
Bruno> +  /* This is needed to silence a -Werror=return-type warning, because
Bruno> +     the above if case doesn't have a return statement.  */
Bruno> +  gdb_assert_not_reached ();
Bruno>  }

This is surprising, because error is marked as noreturn.
Anyway perhaps a better workaround would be to just remove the 'else'.

Bruno> +  /* Despite getting a map, it should have at most one element, otherwise
Bruno> +     cp_lookup_symbol_via_import will have already reported the ambiguity.  */
Bruno> +  std::map<std::string, struct block_symbol> result
Bruno> +    = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1);

I think it would be better to have some kind of wrapper, where
cp_lookup_symbol_via_imports takes a set as a parameter (to avoid having
to merge sets), and then the wrapper extracts the result.

Maybe the error handling could be stuck in the wrapper as well.  I
didn't look into that.

Tom