[PATCH 0/2] Improve handling of using directives

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

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               | 50 +++++++++++++++++++++++++++-----
 gdb/dwarf2/read.c                | 30 ++++++++++++++++++-
 gdb/namespace.c                  | 26 +++++++++++++++++
 gdb/namespace.h                  | 14 ++++++++-
 gdb/testsuite/gdb.cp/nsusing.cc  |  3 +-
 gdb/testsuite/gdb.cp/nsusing.exp | 27 +++++++++++++++--
 6 files changed, 137 insertions(+), 13 deletions(-)

-- 
2.37.3

[PATCH 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                  | 26 ++++++++++++++++++++++++++
 gdb/namespace.h                  | 14 +++++++++++++-
 gdb/testsuite/gdb.cp/nsusing.cc  |  3 ++-
 gdb/testsuite/gdb.cp/nsusing.exp | 16 +++++++++++++---
 6 files changed, 95 insertions(+), 9 deletions(-)

diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index 634dab6ada0..c1b6978b7c8 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 (!valid_line (current, 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 071d0c48e99..7755f87f5bb 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -9435,12 +9435,28 @@ read_import_statement (struct die_info *die, struct dwarf2_cu *cu)
 	process_die (child_die, cu);
       }
 
+  /* When was the using directive was declared.
+     If this was not supplied, set it to 0 so the directive is always valid.
+     Since the type changes from DWARF 4 to DWARF 5, we must check
+     the type of the attribute.  */
+  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);
+  unsigned int line;
+  if (decl_line == nullptr)
+    line = 0;
+  else if (decl_line->form_is_constant ())
+    line = decl_line->constant_value (0);
+  else if (decl_line->form_is_unsigned ())
+    line = decl_line->as_unsigned ();
+  else
+    gdb_assert_not_reached ();
+
   add_using_directive (using_directives (cu),
 		       import_prefix,
 		       canonical_name,
 		       import_alias,
 		       imported_declaration,
 		       excludes,
+		       line,
 		       0,
 		       &objfile->objfile_obstack);
 }
@@ -16064,9 +16080,21 @@ read_namespace (struct die_info *die, struct dwarf2_cu *cu)
 	  const char *previous_prefix = determine_prefix (die, cu);
 
 	  std::vector<const char *> excludes;
+	  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);
+	  unsigned int line;
+	  if (decl_line == nullptr)
+	    line = 0;
+	  else if (decl_line->form_is_constant ())
+	    line = decl_line->constant_value (0);
+	  else if (decl_line->form_is_unsigned ())
+	    line = decl_line->as_unsigned ();
+	  else
+	    gdb_assert_not_reached ();
 	  add_using_directive (using_directives (cu),
 			       previous_prefix, type->name (), NULL,
-			       NULL, excludes, 0, &objfile->objfile_obstack);
+			       NULL, excludes,
+			       line,
+			       0, &objfile->objfile_obstack);
 	}
     }
 
diff --git a/gdb/namespace.c b/gdb/namespace.c
index 0c39c921a3e..d467b0c80bb 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,27 @@ 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
+valid_line (struct using_direct *using_dir,
+	    unsigned int boundary)
+{
+  try
+    {
+      CORE_ADDR curr_pc = get_frame_pc (get_selected_frame (nullptr));
+      symtab_and_line curr_sal = find_pc_line (curr_pc, 0);
+      return using_dir->decl_line <= curr_sal.line
+	     || using_dir->decl_line >= boundary;
+    }
+  catch (const gdb_exception &ex)
+    {
+      return true;
+    }
+}
diff --git a/gdb/namespace.h b/gdb/namespace.h
index dc052a44e42..ed97f9cf804 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,8 @@ struct using_direct
 
   struct using_direct *next;
 
+  unsigned int decl_line;
+
   /* Used during import search to temporarily mark this node as
      searched.  */
   int searched;
@@ -111,7 +114,16 @@ 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);
 
+/* 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.  */
+extern bool valid_line (struct using_direct *using_dir,
+			unsigned int boundary);
+
 #endif /* NAMESPACE_H */
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..bce6e30adc1 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
+    }
+    # 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.
+    -re -wrap "= 911.*" {
+	xfail $gdb_test_name
+    }
+}
+gdb_test "next" ".*" "using namespace M"
+gdb_test "print x" "= 911" "print x, only using M"
-- 
2.37.3

Re: [PATCH 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> When asking GDB to print a variable from an imported namespace, we only
Bruno> want to see variables imported in lines that the inferior has already
Bruno> gone through, as is being tested last in gdb.cp/nsusing.exp. However
Bruno> with the proposed change to gdb.cp/nsusing.exp, we get the following
Bruno> failures:

Thanks for the patch.  I have a few minor comments.

Bruno> +      /* If the using directive was below the place we are stopped at,
Bruno> +	 do not use this directive.  */
Bruno> +      if (!valid_line (current, boundary_sal.line))
Bruno> +	continue;

'valid_line' seems like a kind of generic name for this.
Maybe it should be a method on 'using_direct'?

Bruno> +  /* When was the using directive was declared.
Bruno> +     If this was not supplied, set it to 0 so the directive is always valid.
Bruno> +     Since the type changes from DWARF 4 to DWARF 5, we must check
Bruno> +     the type of the attribute.  */
Bruno> +  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);
Bruno> +  unsigned int line;
Bruno> +  if (decl_line == nullptr)
Bruno> +    line = 0;
Bruno> +  else if (decl_line->form_is_constant ())
Bruno> +    line = decl_line->constant_value (0);
Bruno> +  else if (decl_line->form_is_unsigned ())
Bruno> +    line = decl_line->as_unsigned ();
Bruno> +  else
Bruno> +    gdb_assert_not_reached ();

Asserting here seems wrong, because it means gdb will crash in response
to invalid DWARF.  It's better to emit a complaint and fall back to some
default, or ignore the directive.

Bruno>  	  std::vector<const char *> excludes;
Bruno> +	  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);
Bruno> +	  unsigned int line;
Bruno> +	  if (decl_line == nullptr)
Bruno> +	    line = 0;
Bruno> +	  else if (decl_line->form_is_constant ())
Bruno> +	    line = decl_line->constant_value (0);
Bruno> +	  else if (decl_line->form_is_unsigned ())
Bruno> +	    line = decl_line->as_unsigned ();
Bruno> +	  else
Bruno> +	    gdb_assert_not_reached ();

Here too.
If there's a lot of duplicated code maybe it should be factored into a
new function (I didn't look).

Bruno> +bool
Bruno> +valid_line (struct using_direct *using_dir,

Seems like this could be const; and if it's a method it could be
const-qualified.

Bruno> +      return using_dir->decl_line <= curr_sal.line
Bruno> +	     || using_dir->decl_line >= boundary;

GNU style would parenthesize this.

Bruno>    struct using_direct *next;
 
Bruno> +  unsigned int decl_line;

Should have a comment.

Tom

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

[Andrew Burgess]

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

> 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                  | 26 ++++++++++++++++++++++++++
>  gdb/namespace.h                  | 14 +++++++++++++-
>  gdb/testsuite/gdb.cp/nsusing.cc  |  3 ++-
>  gdb/testsuite/gdb.cp/nsusing.exp | 16 +++++++++++++---
>  6 files changed, 95 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
> index 634dab6ada0..c1b6978b7c8 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 (!valid_line (current, 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 071d0c48e99..7755f87f5bb 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -9435,12 +9435,28 @@ read_import_statement (struct die_info *die, struct dwarf2_cu *cu)
>  	process_die (child_die, cu);
>        }
>  
> +  /* When was the using directive was declared.
> +     If this was not supplied, set it to 0 so the directive is always valid.
> +     Since the type changes from DWARF 4 to DWARF 5, we must check
> +     the type of the attribute.  */
> +  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);
> +  unsigned int line;
> +  if (decl_line == nullptr)
> +    line = 0;
> +  else if (decl_line->form_is_constant ())
> +    line = decl_line->constant_value (0);
> +  else if (decl_line->form_is_unsigned ())
> +    line = decl_line->as_unsigned ();
> +  else
> +    gdb_assert_not_reached ();
> +
>    add_using_directive (using_directives (cu),
>  		       import_prefix,
>  		       canonical_name,
>  		       import_alias,
>  		       imported_declaration,
>  		       excludes,
> +		       line,
>  		       0,
>  		       &objfile->objfile_obstack);
>  }
> @@ -16064,9 +16080,21 @@ read_namespace (struct die_info *die, struct dwarf2_cu *cu)
>  	  const char *previous_prefix = determine_prefix (die, cu);
>  
>  	  std::vector<const char *> excludes;
> +	  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);
> +	  unsigned int line;
> +	  if (decl_line == nullptr)
> +	    line = 0;
> +	  else if (decl_line->form_is_constant ())
> +	    line = decl_line->constant_value (0);
> +	  else if (decl_line->form_is_unsigned ())
> +	    line = decl_line->as_unsigned ();
> +	  else
> +	    gdb_assert_not_reached ();
>  	  add_using_directive (using_directives (cu),
>  			       previous_prefix, type->name (), NULL,
> -			       NULL, excludes, 0, &objfile->objfile_obstack);
> +			       NULL, excludes,
> +			       line,
> +			       0, &objfile->objfile_obstack);
>  	}
>      }
>  
> diff --git a/gdb/namespace.c b/gdb/namespace.c
> index 0c39c921a3e..d467b0c80bb 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,27 @@ 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
> +valid_line (struct using_direct *using_dir,
> +	    unsigned int boundary)
> +{
> +  try
> +    {
> +      CORE_ADDR curr_pc = get_frame_pc (get_selected_frame (nullptr));
> +      symtab_and_line curr_sal = find_pc_line (curr_pc, 0);
> +      return using_dir->decl_line <= curr_sal.line
> +	     || using_dir->decl_line >= boundary;
> +    }
> +  catch (const gdb_exception &ex)
> +    {
> +      return true;
> +    }
> +}
> diff --git a/gdb/namespace.h b/gdb/namespace.h
> index dc052a44e42..ed97f9cf804 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,8 @@ struct using_direct
>  
>    struct using_direct *next;
>  
> +  unsigned int decl_line;
> +
>    /* Used during import search to temporarily mark this node as
>       searched.  */
>    int searched;
> @@ -111,7 +114,16 @@ 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);
>  
> +/* 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.  */
> +extern bool valid_line (struct using_direct *using_dir,
> +			unsigned int boundary);
> +
>  #endif /* NAMESPACE_H */
> 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..bce6e30adc1 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
> +    }
> +    # 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.

I'm pretty sure that comments at this scope will mess up the
gdb_test_multiple.  I don't recall the details, but it's something to do
with the way we expand the blocks to build the final gdb_expect call.

I think you need to move the comment to...


> +    -re -wrap "= 911.*" {

... here.

Thanks,
Andrew

> +	xfail $gdb_test_name
> +    }
> +}
> +gdb_test "next" ".*" "using namespace M"
> +gdb_test "print x" "= 911" "print x, only using M"
> -- 
> 2.37.3

[PATCH 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 it so instead of exiting early when finding any symbol
with the correct name, GDB continues searching through include
directives until it finds another symbol with the same name but a
different mangled name - returning an ambiguous variable - or goes
through all imported namespaces and returns zero or one matching symbols.

The commit also changes gdb.cp/nsusing.exp to test the ambiguous
detection.
---
 gdb/cp-namespace.c               | 35 ++++++++++++++++++++++++++++----
 gdb/testsuite/gdb.cp/nsusing.exp | 19 +++++++++++++----
 2 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index c1b6978b7c8..7a07a8dbe75 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -383,6 +383,8 @@ cp_lookup_symbol_via_imports (const char *scope,
 {
   struct using_direct *current;
   struct block_symbol sym = {};
+  struct block_symbol saved_sym = {};
+  const char* sym_name = nullptr;
   int len;
   int directive_match;
 
@@ -392,7 +394,11 @@ cp_lookup_symbol_via_imports (const char *scope,
 					 block, domain, 1);
 
   if (sym.symbol != NULL)
-    return sym;
+    {
+      saved_sym = sym;
+      sym_name = saved_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 +452,16 @@ cp_lookup_symbol_via_imports (const char *scope,
 	  if (declaration_only || sym.symbol != NULL || current->declaration)
 	    {
 	      if (sym.symbol != NULL)
-		return sym;
+		{
+		  if(sym_name == nullptr)
+		    {
+		      saved_sym = sym;
+		      sym_name = saved_sym.symbol->m_name;
+		      sym = {};
+		    }
+		  else if (strcmp(sym_name, sym.symbol->m_name) != 0)
+		    error (_("reference to \"%s\" is ambiguous"), name);
+		}
 
 	      continue;
 	    }
@@ -479,11 +494,23 @@ cp_lookup_symbol_via_imports (const char *scope,
 	    }
 
 	  if (sym.symbol != NULL)
-	    return sym;
+	    {
+	      if(sym_name == nullptr)
+		{
+		  saved_sym = sym;
+		  sym_name = saved_sym.symbol->m_name;
+		  sym = {};
+		}
+	      else if (strcmp(sym_name, sym.symbol->m_name) != 0)
+		error (_("reference to \"%s\" is ambiguous"), name);
+	    }
 	}
     }
 
-  return {};
+  if (sym_name != nullptr)
+    return saved_sym;
+  else
+    return {};
 }
 
 /* Helper function that searches an array of symbols for one named NAME.  */
diff --git a/gdb/testsuite/gdb.cp/nsusing.exp b/gdb/testsuite/gdb.cp/nsusing.exp
index bce6e30adc1..363ae862953 100644
--- a/gdb/testsuite/gdb.cp/nsusing.exp
+++ b/gdb/testsuite/gdb.cp/nsusing.exp
@@ -123,15 +123,26 @@ if { [test_compiler_info {gcc-[0-3]-*}] ||
     return
 }
 
+    # GCC doesn't properly set the decl_line for namespaces, so GDB believes
+    # that both using directives are valid as long as we are in this scope.
+exp_internal 1
 gdb_test_multiple "print x" "print x, before using statement" {
     -re -wrap "No symbol .x. in current context.*" {
 	pass $gdb_test_name
     }
-    # 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.
-    -re -wrap "= 911.*" {
+    -re -wrap "reference to .x. is ambiguous.*" {
 	xfail $gdb_test_name
     }
 }
+exp_internal 0
 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.37.3

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

[Tom Tromey]

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

Bruno> This commit makes it so instead of exiting early when finding any symbol
Bruno> with the correct name, GDB continues searching through include
Bruno> directives until it finds another symbol with the same name but a
Bruno> different mangled name - returning an ambiguous variable - or goes
Bruno> through all imported namespaces and returns zero or one matching symbols.

Thanks.  This idea makes sense to me.

Bruno> +  const char* sym_name = nullptr;

Wrong "*" placement.

Bruno> +		  if(sym_name == nullptr)

Missing space.

Bruno> +		    {
Bruno> +		      saved_sym = sym;
Bruno> +		      sym_name = saved_sym.symbol->m_name;
Bruno> +		      sym = {};
Bruno> +		    }
Bruno> +		  else if (strcmp(sym_name, sym.symbol->m_name) != 0)

Here too.  There is at least one more of these as well.

Bruno> +		    error (_("reference to \"%s\" is ambiguous"), name);

It would be more friendly to users if it printed the full names of the
ambiguous symbols... is that possible?

Tom

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

[Bruno Larsen]

On 09/11/2022 18:11, Tom Tromey wrote:
>>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> Bruno> This commit makes it so instead of exiting early when finding any symbol
> Bruno> with the correct name, GDB continues searching through include
> Bruno> directives until it finds another symbol with the same name but a
> Bruno> different mangled name - returning an ambiguous variable - or goes
> Bruno> through all imported namespaces and returns zero or one matching symbols.
>
> Thanks.  This idea makes sense to me.
>
> Bruno> +  const char* sym_name = nullptr;
>
> Wrong "*" placement.
>
> Bruno> +		  if(sym_name == nullptr)
>
> Missing space.
>
> Bruno> +		    {
> Bruno> +		      saved_sym = sym;
> Bruno> +		      sym_name = saved_sym.symbol->m_name;
> Bruno> +		      sym = {};
> Bruno> +		    }
> Bruno> +		  else if (strcmp(sym_name, sym.symbol->m_name) != 0)
>
> Here too.  There is at least one more of these as well.
>
> Bruno> +		    error (_("reference to \"%s\" is ambiguous"), name);
>
> It would be more friendly to users if it printed the full names of the
> ambiguous symbols... is that possible?

Do you mean a message like the one below?

reference to "x" is ambiguous, possiblities are M::x and N::x

-- 
Cheers,
Bruno

>
> Tom
>

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

[Tom Tromey]

>>>>> "Bruno" == Bruno Larsen <blarsen@redhat.com> writes:

Bruno> +		    error (_("reference to \"%s\" is ambiguous"), name);

>> It would be more friendly to users if it printed the full names of the
>> ambiguous symbols... is that possible?

Bruno> Do you mean a message like the one below?

Bruno> reference to "x" is ambiguous, possiblities are M::x and N::x

Yes.

Tom

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

[Andrew Burgess]

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

> 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 it so instead of exiting early when finding any symbol
> with the correct name, GDB continues searching through include
> directives until it finds another symbol with the same name but a
> different mangled name - returning an ambiguous variable - or goes
> through all imported namespaces and returns zero or one matching symbols.
>
> The commit also changes gdb.cp/nsusing.exp to test the ambiguous
> detection.
> ---
>  gdb/cp-namespace.c               | 35 ++++++++++++++++++++++++++++----
>  gdb/testsuite/gdb.cp/nsusing.exp | 19 +++++++++++++----
>  2 files changed, 46 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
> index c1b6978b7c8..7a07a8dbe75 100644
> --- a/gdb/cp-namespace.c
> +++ b/gdb/cp-namespace.c
> @@ -383,6 +383,8 @@ cp_lookup_symbol_via_imports (const char *scope,
>  {
>    struct using_direct *current;
>    struct block_symbol sym = {};
> +  struct block_symbol saved_sym = {};
> +  const char* sym_name = nullptr;
>    int len;
>    int directive_match;
>  
> @@ -392,7 +394,11 @@ cp_lookup_symbol_via_imports (const char *scope,
>  					 block, domain, 1);
>  
>    if (sym.symbol != NULL)
> -    return sym;
> +    {
> +      saved_sym = sym;
> +      sym_name = saved_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 +452,16 @@ cp_lookup_symbol_via_imports (const char *scope,
>  	  if (declaration_only || sym.symbol != NULL || current->declaration)
>  	    {
>  	      if (sym.symbol != NULL)
> -		return sym;
> +		{
> +		  if(sym_name == nullptr)
> +		    {
> +		      saved_sym = sym;
> +		      sym_name = saved_sym.symbol->m_name;
> +		      sym = {};
> +		    }
> +		  else if (strcmp(sym_name, sym.symbol->m_name) != 0)
> +		    error (_("reference to \"%s\" is ambiguous"), name);
> +		}
>  
>  	      continue;
>  	    }
> @@ -479,11 +494,23 @@ cp_lookup_symbol_via_imports (const char *scope,
>  	    }
>  
>  	  if (sym.symbol != NULL)
> -	    return sym;
> +	    {
> +	      if(sym_name == nullptr)
> +		{
> +		  saved_sym = sym;
> +		  sym_name = saved_sym.symbol->m_name;
> +		  sym = {};
> +		}
> +	      else if (strcmp(sym_name, sym.symbol->m_name) != 0)
> +		error (_("reference to \"%s\" is ambiguous"), name);
> +	    }
>  	}
>      }
>  
> -  return {};
> +  if (sym_name != nullptr)
> +    return saved_sym;
> +  else
> +    return {};
>  }
>  
>  /* Helper function that searches an array of symbols for one named NAME.  */
> diff --git a/gdb/testsuite/gdb.cp/nsusing.exp b/gdb/testsuite/gdb.cp/nsusing.exp
> index bce6e30adc1..363ae862953 100644
> --- a/gdb/testsuite/gdb.cp/nsusing.exp
> +++ b/gdb/testsuite/gdb.cp/nsusing.exp
> @@ -123,15 +123,26 @@ if { [test_compiler_info {gcc-[0-3]-*}] ||
>      return
>  }
>  
> +    # GCC doesn't properly set the decl_line for namespaces, so GDB believes
> +    # that both using directives are valid as long as we are in this scope.
> +exp_internal 1

I suspect the exp_internal was added for debugging?  Should this, and
the 'exp_internal 0' line below be removed?

Thanks,
Andrew


>  gdb_test_multiple "print x" "print x, before using statement" {
>      -re -wrap "No symbol .x. in current context.*" {
>  	pass $gdb_test_name
>      }
> -    # 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.
> -    -re -wrap "= 911.*" {
> +    -re -wrap "reference to .x. is ambiguous.*" {
>  	xfail $gdb_test_name
>      }
>  }
> +exp_internal 0
>  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.37.3