Re: [PATCH v6 3/3] gdb: extend the [[N]]::foo syntax for files

[Andrew Burgess <aburgess@redhat.com>]

Guinevere Larsen <guinevere@redhat.com> writes:

> This commit implements the missing support for [[N]]::'file.c'::var
> syntax that was skipped on the previous commit.
>
> This is done by adding a new value to the global parser_state, so that
> the classify_name function can restrict its search for file names to the
> specified linker namespace.  It had to be done this way because if the
> logic was contained on the newly added "block: block COLONCOLON
> FILENAME" rule, we would not have the name to rerun the search.

Did you consider adding the filename to the <whatever> type such that it
was available within the rule to allow for the filename to be re-looked
up?

I'd be interested to know if this was tried, why this was worse than
pushing parser state back to the lexer, which I always thought was not a
great design.

>                                                                  Because
> of that, the new rule only exists to make sure this is only used in the
> intended syntax.  This commit also adds a link to the solib_ops relevant
> to the expression being parsed, as a way to identify if linker
> namespaces are supported; this could be achieved by guarding the search
> functions instead, but I think that this is more reliable.
>
> The new rule uses "block" rather than a linker namespace specific token,
> because if we allowed that token to also reduce into block, the new rule
> would never be triggered as the simple reduction would be preferred. If
> we didn't allow the linker namespace to reduce into block, the rest of
> the syntaxes for [[N]]::foo would be more involved, so the option in
> this commit seemed like the simplest solution.
>
> The rule "block: block COLONCOLON name" also had to be updated because,
> if a user tried to search for a file not present in the linker
> namespace, GDB would miscategorize it to 'name' instead of 'filename',
> and if no function of the same name was found, the error would only say
> "No function \"foo\" (...)" which could confuse a user. The new error
> says "Nothing named \"foo\" (...)" instead since it is impossible for us
> to disambiguate between filename or function name, and both would be
> valid in this code path.
>
> The parser_state changes were not used for the previous commit because
> the lookup_symbol calls would lead to an even more intrusive change for
> no real gain, since the search would be rerun any way. There is an
> argument to be made that the results could be different if only
> namespace had a type and another had a variable, but a situation where
> that significantly changes the result is quite unlikely, I think.
> ---
>  gdb/c-exp.y                               | 18 +++++++++++--
>  gdb/linespec.c                            |  4 +--
>  gdb/parse.c                               |  3 ++-
>  gdb/parser-defs.h                         | 32 +++++++++++++++++++++--
>  gdb/rust-parse.c                          |  3 ++-
>  gdb/symtab.c                              | 26 +++++++++++++-----
>  gdb/symtab.h                              |  7 ++++-
>  gdb/testsuite/gdb.base/dlmopen-ns-ids.exp | 12 +++++++++
>  8 files changed, 90 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/c-exp.y b/gdb/c-exp.y
> index cb70bf3c863..f59aca327ed 100644
> --- a/gdb/c-exp.y
> +++ b/gdb/c-exp.y
> @@ -1071,6 +1071,7 @@ block	:	BLOCKNAME
>  			{
>  			    $$.search_namespace = true;
>  			    $$.namespace_val = $2.val;
> +			    pstate->set_linker_namespace ($2.val);
>  			}
>  	;
>  
> @@ -1087,12 +1088,24 @@ block	:	block COLONCOLON name
>  						 SEARCH_FUNCTION_DOMAIN,
>  						 nullptr);
>  
> -			  if (tem.symbol == nullptr)
> +			  if (tem.symbol == nullptr && !$$.search_namespace)
>  			    error (_("No function \"%s\" in specified context."),
>  				   copy.c_str ());
> +			  else if (tem.symbol == nullptr && $$.search_namespace)

My yacc foo is weak, but isn't '$$' the rule's output?  Which is only
being set below.  Shouldn't this be $1 maybe?  If there's not some
reason why search_namespace is being set to the correct value (that I'm
missing) then this would seem to indicate that there's a gap in the
testing here.

> +			    /* COPY can be a function or a file.  There is no way
> +			       to identify which the user intended, so emit a
> +			       generic warning instead.  */
> +			    error (_("Nothing named \"%s\" in specified context."),
> +				   copy.c_str ());
>  			  $$.b_val = tem.symbol->value_block ();
>  			  $$.search_namespace = false;
>  			}
> +	|	block COLONCOLON FILENAME
> +			{
> +			    if (!$1.search_namespace)
> +				error (_("Filename must be the first part of the expression"));
> +			    $$ = $3;
> +			}
>  	;
>  
>  variable:	name_not_typename ENTRY
> @@ -3173,7 +3186,8 @@ classify_name (struct parser_state *par_state, const struct block *block,
>  	  || is_quoted_name)
>  	{
>  	  /* See if it's a file name. */
> -	  if (auto symtab = lookup_symtab (current_program_space, copy.c_str ());
> +	  if (auto symtab = lookup_symtab (current_program_space, copy.c_str (),
> +					   par_state->get_linker_namespace ());
>  	      symtab != nullptr)
>  	    {
>  	      yylval.bval.b_val
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 08f7fdd0daa..ec90ad4826a 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -3633,11 +3633,11 @@ collect_symtabs_from_filename (const char *file,
>  	  if (pspace->executing_startup)
>  	    continue;
>  
> -	  iterate_over_symtabs (pspace, file, collector);
> +	  iterate_over_symtabs (pspace, file, -1, collector);
>  	}
>      }
>    else
> -    iterate_over_symtabs (search_pspace, file, collector);
> +    iterate_over_symtabs (search_pspace, file, -1, collector);
>  
>    /* It is tempting to use the unordered_dense 'extract' method here,
>       and remove the separate vector -- but it's unclear if ordering
> diff --git a/gdb/parse.c b/gdb/parse.c
> index d76768f41d7..7358c369d52 100644
> --- a/gdb/parse.c
> +++ b/gdb/parse.c
> @@ -420,7 +420,8 @@ parse_exp_in_context (const char **stringptr, CORE_ADDR pc,
>  
>    parser_state ps (lang, get_current_arch (), expression_context_block,
>  		   expression_context_pc, flags, *stringptr,
> -		   completer != nullptr, tracker);
> +		   completer != nullptr, tracker,
> +		   current_program_space->solib_ops ());
>  
>    scoped_restore_current_language lang_saver (lang->la_language);
>  
> diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
> index ff4ab6a09de..c0be1083f66 100644
> --- a/gdb/parser-defs.h
> +++ b/gdb/parser-defs.h
> @@ -26,6 +26,7 @@
>  #include "expression.h"
>  #include "symtab.h"
>  #include "expop.h"
> +#include "solib.h"
>  
>  struct block;
>  struct language_defn;
> @@ -147,7 +148,8 @@ struct parser_state : public expr_builder
>  		parser_flags flags,
>  		const char *input,
>  		bool completion,
> -		innermost_block_tracker *tracker)
> +		innermost_block_tracker *tracker,
> +		const solib_ops *ops)
>      : expr_builder (lang, gdbarch),
>        expression_context_block (context_block),
>        expression_context_pc (context_pc),
> @@ -157,7 +159,8 @@ struct parser_state : public expr_builder
>        comma_terminates ((flags & PARSER_COMMA_TERMINATES) != 0),
>        parse_completion (completion),
>        void_context_p ((flags & PARSER_VOID_CONTEXT) != 0),
> -      debug ((flags & PARSER_DEBUG) != 0)
> +      debug ((flags & PARSER_DEBUG) != 0),
> +      m_solib_ops (ops)
>    {
>    }
>  
> @@ -263,6 +266,21 @@ struct parser_state : public expr_builder
>      push (expr::make_operation<T> (std::move (lhs), std::move (rhs)));
>    }
>  
> +  void set_linker_namespace (LONGEST ns_id)
> +  {
> +    if (m_solib_ops == nullptr)
> +      error (_("Linker namespaces require an active inferior"));
> +    if (m_solib_ops->supports_namespaces ())
> +      linker_namespace = ns_id;
> +    else
> +      error (_("Linker namespaces are not supported"));
> +  }
> +
> +  LONGEST get_linker_namespace ()

This can be const.

> +  {
> +    return linker_namespace;
> +  }
> +
>    /* Function called from the various parsers' yyerror functions to throw
>       an error.  The error will include a message identifying the location
>       of the error within the current expression.  */
> @@ -323,6 +341,16 @@ struct parser_state : public expr_builder
>  
>    /* Stack of operations.  */
>    std::vector<expr::operation_up> m_operations;
> +
> +  /* If the expression is being restricted to a specific namespace, this is
> +     where that information is stored for the block lookup.  It should be
> +     accessed through setter/getters to ensure that the linker namespace is
> +     only set when the gdbarch supports it.  */
> +  LONGEST linker_namespace = -1;

This should be renamed m_linker_namespace as it is private.

Also, I couldn't find anything that would reset the linker namepsace
back to -1 when parsing a complex expression.  For example, something
like:

  print [[1]]::'foo.c'::var_a + 'bar.c'::var_b

After parsing the [[1]] linker_namespace will be set to '1', but is it
ever reset?  Will we not end up parsing the "'bar.c'::var_b" as if the
lookup had to be in namespace 1?

Thanks,
Andrew

> +
> +  /* Used to figure out if an inferior is capable of handling linker
> +     namespaces at all.  */
> +  const solib_ops *m_solib_ops;
>  };
>  
>  /* A string token, either a char-string or bit-string.  Char-strings are
> diff --git a/gdb/rust-parse.c b/gdb/rust-parse.c
> index b740d5a7b16..27f03b23e6a 100644
> --- a/gdb/rust-parse.c
> +++ b/gdb/rust-parse.c
> @@ -2315,7 +2315,8 @@ rust_lex_tests (void)
>  {
>    /* Set up dummy "parser", so that rust_type works.  */
>    parser_state ps (language_def (language_rust), current_inferior ()->arch (),
> -		   nullptr, 0, 0, nullptr, 0, nullptr);
> +		   nullptr, 0, 0, nullptr, 0, nullptr,
> +		   current_program_space->solib_ops ());
>    rust_parser parser (&ps);
>  
>    rust_lex_test_one (&parser, "", 0);
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index d237a5cc5d2..bbaf94e51d0 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -618,6 +618,7 @@ compare_filenames_for_search (const char *filename, const char *search_name)
>  
>  void
>  iterate_over_symtabs (program_space *pspace, const char *name,
> +		      LONGEST linker_ns,
>  		      gdb::function_view<bool (symtab *)> callback)
>  {
>    gdb::unique_xmalloc_ptr<char> real_path;
> @@ -630,20 +631,33 @@ iterate_over_symtabs (program_space *pspace, const char *name,
>        gdb_assert (IS_ABSOLUTE_PATH (real_path.get ()));
>      }
>  
> -  for (objfile &objfile : pspace->objfiles ())
> -    if (objfile.map_symtabs_matching_filename (name, real_path.get (),
> -					       callback))
> +  std::vector<objfile *> objfiles_to_search;
> +  if (linker_ns >= 0)
> +    {
> +      gdb_assert (pspace->solib_ops ()->supports_namespaces ());
> +      objfiles_to_search
> +	= get_objfiles_in_linker_namespace (linker_ns, pspace);
> +    }
> +  else
> +    {
> +      for (objfile &objf : pspace->objfiles ())
> +	objfiles_to_search.push_back (&objf);
> +    }
> +
> +  for (objfile *objfile : objfiles_to_search)
> +    if (objfile->map_symtabs_matching_filename (name, real_path.get (),
> +						callback))
>        return;
>  }
>  
>  /* See symtab.h.  */
>  
>  symtab *
> -lookup_symtab (program_space *pspace, const char *name)
> +lookup_symtab (program_space *pspace, const char *name, LONGEST linker_ns)
>  {
>    struct symtab *result = NULL;
>  
> -  iterate_over_symtabs (pspace, name, [&] (symtab *symtab)
> +  iterate_over_symtabs (pspace, name, linker_ns, [&] (symtab *symtab)
>      {
>        result = symtab;
>        return true;
> @@ -6158,7 +6172,7 @@ collect_file_symbol_completion_matches (completion_tracker &tracker,
>  
>    /* Go through symtabs for SRCFILE and check the externs and statics
>       for symbols which match.  */
> -  iterate_over_symtabs (current_program_space, srcfile, [&] (symtab *s)
> +  iterate_over_symtabs (current_program_space, srcfile, -1, [&] (symtab *s)
>      {
>        add_symtab_completions (s->compunit (),
>  			      tracker, mode, lookup_name,
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index fe2304acb4f..bda6e82da02 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2079,7 +2079,8 @@ const char *multiple_symbols_select_mode (void);
>  
>  /* Lookup a symbol table in PSPACE by source file name.  */
>  
> -extern symtab *lookup_symtab (program_space *pspace, const char *name);
> +extern symtab *lookup_symtab (program_space *pspace, const char *name,
> +			      LONGEST linker_ns = -1);
>  
>  /* An object of this type is passed as the 'is_a_field_of_this'
>     argument to lookup_symbol and lookup_symbol_in_language.  */
> @@ -2805,10 +2806,14 @@ bool compare_glob_filenames_for_search (const char *filename,
>     psymtabs.  *If* there is no '/' in the name, a match after a '/' in the
>     symtab filename will also work.
>  
> +   If LINKER_NS is 0 or greater, only the objfiles in the provided linker
> +   namespace will be iterated over.
> +
>     Call CALLBACK with each symtab that is found.  If CALLBACK returns
>     true, the search stops.  */
>  
>  void iterate_over_symtabs (program_space *pspace, const char *name,
> +			   LONGEST linker_ns,
>  			   gdb::function_view<bool (symtab *)> callback);
>  
>  std::vector<const linetable_entry *> find_linetable_entries_for_symtab_line
> diff --git a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
> index fec7adb5317..a1e18fc39d3 100644
> --- a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
> +++ b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
> @@ -325,9 +325,15 @@ proc_with_prefix test_print_namespace_symbol {} {
>      clean_restart
>      gdb_load $::binfile
>  
> +    set ns0 [ns_id_for_command 0]
>      set ns1 [ns_id_for_command 1]
>      set ns2 [ns_id_for_command 2]
>  
> +    # Test printing variables before starting the inferior
> +    gdb_test "print ${ns1}::gdb_dlmopen_glob" \
> +	"Linker namespaces require an active inferior" \
> +	"Before starting inferior"
> +
>      if { ![runto_main] } {
>  	return
>      }
> @@ -365,6 +371,12 @@ proc_with_prefix test_print_namespace_symbol {} {
>      # returning a random one, we just say we didn't find one.
>      gdb_test "print gdb_dlmopen_glob" \
>  	"No symbol .gdb_dlmopen_glob. in the current linker namespace."
> +
> +    # Minimal testing for finding files in namespaces.
> +    gdb_test "print ${ns1}::'${::srcfile_lib}'::gdb_dlmopen_glob" \
> +	".* = 2"
> +    gdb_test "print ${ns0}::'${::srcfile_lib}'::gdb_dlmopen_glob" \
> +	"Nothing named .${::srcfile_lib}. in specified context."
>  }
>  
>  test_info_shared
> -- 
> 2.51.0