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

[Lancelot SIX <lsix@lancelotsix.com>]

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
>