[PATCH 0/2] Two minor partial symtab cleanups

[PATCH 0/2] Two minor partial symtab cleanups

[Tom Tromey]

Here are two minor partial symtab cleanups, also coming from the
larger quick symbol function cleanup series that I'm working on.

Regression tested on x86-64 Fedora 32.

Let me know what you think.

Tom

[PATCH 1/2] Allow expand_symtabs_matching to examine imported psymtabs

[Tom Tromey]

Currently the psymtab variant of expand_symtabs_matching has this
check:

      /* We skip shared psymtabs because file-matching doesn't apply
	 to them; but we search them later in the loop.  */
      if (ps->user != NULL)
	continue;

In a larger series I'm working on, it's convenient to remove this
check.  And, I noticed that a similar check is not done for
expand_symtabs_with_fullname.  So, it made sense to me to remove the
check here as well.

gdb/ChangeLog
2021-03-24  Tom Tromey  <tom@tromey.com>

	* psymtab.c (psymbol_functions::expand_symtabs_matching): Remove
	"user" check.
---
 gdb/ChangeLog | 5 +++++
 gdb/psymtab.c | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index 597817269c1..5a64166d983 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1313,11 +1313,6 @@ psymbol_functions::expand_symtabs_matching
       if (ps->readin_p (objfile))
 	continue;
 
-      /* We skip shared psymtabs because file-matching doesn't apply
-	 to them; but we search them later in the loop.  */
-      if (ps->user != NULL)
-	continue;
-
       if (file_matcher)
 	{
 	  bool match;
-- 
2.26.2

[PATCH 2/2] Simplify psymbol_functions::expand_symtabs_matching

[Tom Tromey]

I noticed that psymbol_functions::expand_symtabs_matching calls
make_ignore_params once per psymtab that is matched.  This seems
possibly expensive, so this patch hoists the call out of the loop.

gdb/ChangeLog
2021-03-24  Tom Tromey  <tom@tromey.com>

	* psymtab.c (psymbol_functions::expand_symtabs_matching): Only
	call make_ignore_params once.
---
 gdb/ChangeLog | 5 +++++
 gdb/psymtab.c | 6 +++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index 5a64166d983..1ea7376a8c8 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1306,6 +1306,10 @@ psymbol_functions::expand_symtabs_matching
   for (partial_symtab *ps : require_partial_symbols (objfile))
     ps->searched_flag = PST_NOT_SEARCHED;
 
+  gdb::optional<lookup_name_info> psym_lookup_name;
+  if (lookup_name != nullptr)
+    psym_lookup_name = lookup_name->make_ignore_params ();
+
   for (partial_symtab *ps : m_partial_symtabs->range ())
     {
       QUIT;
@@ -1335,7 +1339,7 @@ psymbol_functions::expand_symtabs_matching
 
       if ((symbol_matcher == NULL && lookup_name == NULL)
 	  || recursively_search_psymtabs (ps, objfile, domain,
-					  lookup_name->make_ignore_params (),
+					  *psym_lookup_name,
 					  symbol_matcher))
 	{
 	  struct compunit_symtab *symtab =
-- 
2.26.2

Re: [PATCH 1/2] Allow expand_symtabs_matching to examine imported psymtabs

[Simon Marchi]

On 2021-03-24 4:15 p.m., Tom Tromey wrote:
> Currently the psymtab variant of expand_symtabs_matching has this
> check:
> 
>       /* We skip shared psymtabs because file-matching doesn't apply
> 	 to them; but we search them later in the loop.  */
>       if (ps->user != NULL)
> 	continue;
> 
> In a larger series I'm working on, it's convenient to remove this
> check.  And, I noticed that a similar check is not done for
> expand_symtabs_with_fullname.  So, it made sense to me to remove the
> check here as well.
> 
> gdb/ChangeLog
> 2021-03-24  Tom Tromey  <tom@tromey.com>
> 
> 	* psymtab.c (psymbol_functions::expand_symtabs_matching): Remove
> 	"user" check.
> ---
>  gdb/ChangeLog | 5 +++++
>  gdb/psymtab.c | 5 -----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/psymtab.c b/gdb/psymtab.c
> index 597817269c1..5a64166d983 100644
> --- a/gdb/psymtab.c
> +++ b/gdb/psymtab.c
> @@ -1313,11 +1313,6 @@ psymbol_functions::expand_symtabs_matching
>        if (ps->readin_p (objfile))
>  	continue;
>  
> -      /* We skip shared psymtabs because file-matching doesn't apply
> -	 to them; but we search them later in the loop.  */
> -      if (ps->user != NULL)
> -	continue;
> -
>        if (file_matcher)
>  	{
>  	  bool match;
> 

I'm not against this change, but I don't understand it.  It seems
logical to me to skip shared psymtabs, because we'll reach them through
some other psymtabs that include them.  So, both versions seem correct,
but maybe knowing why it's convenient to you would help.

Simon

Re: [PATCH 2/2] Simplify psymbol_functions::expand_symtabs_matching

[Simon Marchi]

On 2021-03-24 4:15 p.m., Tom Tromey wrote:
> I noticed that psymbol_functions::expand_symtabs_matching calls
> make_ignore_params once per psymtab that is matched.  This seems
> possibly expensive, so this patch hoists the call out of the loop.
> 
> gdb/ChangeLog
> 2021-03-24  Tom Tromey  <tom@tromey.com>
> 
> 	* psymtab.c (psymbol_functions::expand_symtabs_matching): Only
> 	call make_ignore_params once.
> ---
>  gdb/ChangeLog | 5 +++++
>  gdb/psymtab.c | 6 +++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/psymtab.c b/gdb/psymtab.c
> index 5a64166d983..1ea7376a8c8 100644
> --- a/gdb/psymtab.c
> +++ b/gdb/psymtab.c
> @@ -1306,6 +1306,10 @@ psymbol_functions::expand_symtabs_matching
>    for (partial_symtab *ps : require_partial_symbols (objfile))
>      ps->searched_flag = PST_NOT_SEARCHED;
>  
> +  gdb::optional<lookup_name_info> psym_lookup_name;
> +  if (lookup_name != nullptr)
> +    psym_lookup_name = lookup_name->make_ignore_params ();
> +
>    for (partial_symtab *ps : m_partial_symtabs->range ())
>      {
>        QUIT;
> @@ -1335,7 +1339,7 @@ psymbol_functions::expand_symtabs_matching
>  
>        if ((symbol_matcher == NULL && lookup_name == NULL)
>  	  || recursively_search_psymtabs (ps, objfile, domain,
> -					  lookup_name->make_ignore_params (),
> +					  *psym_lookup_name,
>  					  symbol_matcher))
>  	{
>  	  struct compunit_symtab *symtab =
> 

This LGTM, thanks.

Simon

Re: [PATCH 1/2] Allow expand_symtabs_matching to examine imported psymtabs

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

>> -      /* We skip shared psymtabs because file-matching doesn't apply
>> -	 to them; but we search them later in the loop.  */
>> -      if (ps->user != NULL)
>> -	continue;
>> -

Simon> I'm not against this change, but I don't understand it.  It seems
Simon> logical to me to skip shared psymtabs, because we'll reach them through
Simon> some other psymtabs that include them.  So, both versions seem correct,
Simon> but maybe knowing why it's convenient to you would help.

If the shared psymtab has a file name, then that file name will not be
checked by the file matcher.  This doesn't matter directly now because
file matching is often done via a different method, that does check
these.  However, with my proposed series to remove the redundant
methods, this issue is exposed.

Tom

Re: [PATCH 1/2] Allow expand_symtabs_matching to examine imported psymtabs

[Simon Marchi]

On 2021-03-26 2:17 p.m., Tom Tromey wrote:>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
>>> -      /* We skip shared psymtabs because file-matching doesn't apply
>>> -	 to them; but we search them later in the loop.  */
>>> -      if (ps->user != NULL)
>>> -	continue;
>>> -
> 
> Simon> I'm not against this change, but I don't understand it.  It seems
> Simon> logical to me to skip shared psymtabs, because we'll reach them through
> Simon> some other psymtabs that include them.  So, both versions seem correct,
> Simon> but maybe knowing why it's convenient to you would help.
> 
> If the shared psymtab has a file name, then that file name will not be
> checked by the file matcher.  This doesn't matter directly now because
> file matching is often done via a different method, that does check
> these.  However, with my proposed series to remove the redundant
> methods, this issue is exposed.

Ok, thanks.

Simon