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

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

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
>
  

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!