[PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Doug Evans]

Hi.

This patch rewrites lookup_static_symbol to use
gdbarch_iterate_over_objfiles_in_search_order.

This function is currently implemented like this:
[This is before the function is renamed by this patch series.]

struct symbol *
lookup_static_symbol_aux (const char *name, const domain_enum domain)
{
  struct objfile *objfile;
  struct symbol *sym;

  sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, domain);
  if (sym != NULL)
    return sym;

  ALL_OBJFILES (objfile)
  {
    sym = lookup_symbol_aux_quick (objfile, STATIC_BLOCK, name, domain);
    if (sym != NULL)
      return sym;
  }

  return NULL;
}

Note what we're doing here.
First we're searching over all expanded symtabs in all objfiles,
and then we search partial/gdb_index tables in all objfiles.  Eh?

Normally when looking up a symbol in a particular objfile
we first list in expanded symtabs and then look in partial/gdb_index
tables.  And *then* we try the next objfile.

I can't think of any justification for the current behaviour.

This patch changes things to be consistent,
but it is a behavioural change.
The testsuite passes, and any noticeable difference
by this change would be dependent on the order in which
symtabs got expanded.  Thus I can't think of a reason to not
apply this change.

Contrary to lookup_global_symbol where at least for linux one might
not care what order objfiles are searched, for static symbols there
is more reason to care.  However, the current API doesn't provide
for this (we don't pass in BLOCK of OBJFILE as an argument) because
this routine is used as a last resort anyways.

2014-10-26  Doug Evans  <xdje42@gmail.com>

	* symtab.c (lookup_static_symbol): Rewrite to use
	gdbarch_iterate_over_objfiles_in_search_order.
	(iterated_sym_lookup_data): Renamed from global_sym_lookup_data.
	New member block_index.
	(lookup_symbol_iterator_cb): Renamed from
	lookup_symbol_global_iterator_cb.  Use block_index from
	iterated_sym_lookup_data.
	(lookup_symbol_global): Update.
	(lookup_symbol_aux_symtabs): Delete.

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 07e9fce..54bbb67 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -80,11 +80,6 @@ struct symbol *lookup_local_symbol (const char *name,
 				    enum language language);
 
 static
-struct symbol *lookup_symbol_aux_symtabs (int block_index,
-					  const char *name,
-					  const domain_enum domain);
-
-static
 struct symbol *lookup_symbol_via_quick_fns (struct objfile *objfile,
 					    int block_index,
 					    const char *name,
@@ -1474,28 +1469,6 @@ lookup_symbol_aux (const char *name, const struct block *block,
   return lookup_static_symbol (name, domain);
 }
 
-/* See symtab.h.  */
-
-struct symbol *
-lookup_static_symbol (const char *name, const domain_enum domain)
-{
-  struct objfile *objfile;
-  struct symbol *sym;
-
-  sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, domain);
-  if (sym != NULL)
-    return sym;
-
-  ALL_OBJFILES (objfile)
-  {
-    sym = lookup_symbol_via_quick_fns (objfile, STATIC_BLOCK, name, domain);
-    if (sym != NULL)
-      return sym;
-  }
-
-  return NULL;
-}
-
 /* Check to see if the symbol is defined in BLOCK or its superiors.
    Don't search STATIC_BLOCK or GLOBAL_BLOCK.  */
 
@@ -1648,26 +1621,6 @@ lookup_symbol_in_symtabs (struct objfile *objfile, int block_index,
   return NULL;
 }
 
-/* Wrapper around lookup_symbol_in_symtabs to search all objfiles.
-   Returns the first match found.  */
-
-static struct symbol *
-lookup_symbol_aux_symtabs (int block_index, const char *name,
-			   const domain_enum domain)
-{
-  struct symbol *sym;
-  struct objfile *objfile;
-
-  ALL_OBJFILES (objfile)
-  {
-    sym = lookup_symbol_in_symtabs (objfile, block_index, name, domain);
-    if (sym)
-      return sym;
-  }
-
-  return NULL;
-}
-
 /* Wrapper around lookup_symbol_in_symtabs for search_symbols.
    Look up LINKAGE_NAME in DOMAIN in the global and static blocks of OBJFILE
    and all related objfiles.  */
@@ -1771,7 +1724,7 @@ basic_lookup_symbol_nonlocal (const char *name,
      not it would be appropriate to search the current global block
      here as well.  (That's what this code used to do before the
      is_a_field_of_this check was moved up.)  On the one hand, it's
-     redundant with the lookup_symbol_aux_symtabs search that happens
+     redundant with the lookup in all objfiles search that happens
      next.  On the other hand, if decode_line_1 is passed an argument
      like filename:var, then the user presumably wants 'var' to be
      searched for in filename.  On the third hand, there shouldn't be
@@ -1815,9 +1768,9 @@ lookup_symbol_in_static_block (const char *name,
     return NULL;
 }
 
-/* Private data to be used with lookup_symbol_global_iterator_cb.  */
+/* Private data to be used with lookup_symbol_iterator_cb.  */
 
-struct global_sym_lookup_data
+struct iterated_sym_lookup_data
 {
   /* The name of the symbol we are searching for.  */
   const char *name;
@@ -1825,29 +1778,31 @@ struct global_sym_lookup_data
   /* The domain to use for our search.  */
   domain_enum domain;
 
+  /* One of STATIC_BLOCK or GLOBAL_BLOCK.  */
+  int block_index;
+
   /* The field where the callback should store the symbol if found.
      It should be initialized to NULL before the search is started.  */
   struct symbol *result;
 };
 
 /* A callback function for gdbarch_iterate_over_objfiles_in_search_order.
-   It searches by name for a symbol in the GLOBAL_BLOCK of the given
+   It searches by name for a symbol in the given block of the given
    OBJFILE.  The arguments for the search are passed via CB_DATA,
-   which in reality is a pointer to struct global_sym_lookup_data.  */
+   which in reality is a pointer to struct iterated_sym_lookup_data.  */
 
 static int
-lookup_symbol_global_iterator_cb (struct objfile *objfile,
-				  void *cb_data)
+lookup_symbol_iterator_cb (struct objfile *objfile, void *cb_data)
 {
-  struct global_sym_lookup_data *data =
-    (struct global_sym_lookup_data *) cb_data;
+  struct iterated_sym_lookup_data *data =
+    (struct iterated_sym_lookup_data *) cb_data;
 
   gdb_assert (data->result == NULL);
 
-  data->result = lookup_symbol_in_symtabs (objfile, GLOBAL_BLOCK,
+  data->result = lookup_symbol_in_symtabs (objfile, data->block_index,
 					   data->name, data->domain);
   if (data->result == NULL)
-    data->result = lookup_symbol_via_quick_fns (objfile, GLOBAL_BLOCK,
+    data->result = lookup_symbol_via_quick_fns (objfile, data->block_index,
 						data->name, data->domain);
 
   /* If we found a match, tell the iterator to stop.  Otherwise,
@@ -1858,13 +1813,30 @@ lookup_symbol_global_iterator_cb (struct objfile *objfile,
 /* See symtab.h.  */
 
 struct symbol *
+lookup_static_symbol (const char *name, const domain_enum domain)
+{
+  struct iterated_sym_lookup_data lookup_data;
+
+  memset (&lookup_data, 0, sizeof (lookup_data));
+  lookup_data.name = name;
+  lookup_data.domain = domain;
+  lookup_data.block_index = STATIC_BLOCK;
+  gdbarch_iterate_over_objfiles_in_search_order
+    (target_gdbarch (), lookup_symbol_iterator_cb, &lookup_data, NULL);
+
+  return lookup_data.result;
+}
+
+/* See symtab.h.  */
+
+struct symbol *
 lookup_symbol_global (const char *name,
 		      const struct block *block,
 		      const domain_enum domain)
 {
   struct symbol *sym = NULL;
   struct objfile *objfile = NULL;
-  struct global_sym_lookup_data lookup_data;
+  struct iterated_sym_lookup_data lookup_data;
 
   /* Call library-specific lookup procedure.  */
   objfile = lookup_objfile_from_block (block);
@@ -1876,9 +1848,10 @@ lookup_symbol_global (const char *name,
   memset (&lookup_data, 0, sizeof (lookup_data));
   lookup_data.name = name;
   lookup_data.domain = domain;
+  lookup_data.block_index = GLOBAL_BLOCK;
   gdbarch_iterate_over_objfiles_in_search_order
     (objfile != NULL ? get_objfile_arch (objfile) : target_gdbarch (),
-     lookup_symbol_global_iterator_cb, &lookup_data, objfile);
+     lookup_symbol_iterator_cb, &lookup_data, objfile);
 
   return lookup_data.result;
 }

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Yao Qi]

Doug Evans <xdje42@gmail.com> writes:

> struct symbol *
> lookup_static_symbol_aux (const char *name, const domain_enum domain)
> {
>   struct objfile *objfile;
>   struct symbol *sym;
>
>   sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, domain);
>   if (sym != NULL)
>     return sym;
>
>   ALL_OBJFILES (objfile)
>   {
>     sym = lookup_symbol_aux_quick (objfile, STATIC_BLOCK, name, domain);
>     if (sym != NULL)
>       return sym;
>   }
>
>   return NULL;
> }
>
> Note what we're doing here.
> First we're searching over all expanded symtabs in all objfiles,
> and then we search partial/gdb_index tables in all objfiles.  Eh?
>
> Normally when looking up a symbol in a particular objfile
> we first list in expanded symtabs and then look in partial/gdb_index
> tables.  And *then* we try the next objfile.
>
> I can't think of any justification for the current behaviour.
>
> This patch changes things to be consistent,
> but it is a behavioural change.

Yes, it changes the behavior.  Here is an example in my mind, but not
sure it is correct or not, say, we have a static int foo defined in two
objfiles respectively (1.c and 2.c), foo is in the partial table of two
objfiles, but is only expanded to the full symtab of *one* objfile (2.c).

             1.c    2.c
  partial    foo    foo
  full              foo

before your patch, GDB gets foo from 2.c full table, and after it, GDB
gets foo from 1.c partial table.  Is it possible?

Regarding the change, we also need to update the comments to
iterate_over_objfiles_in_search_order in gdbarch.sh,

# Iterate over all objfiles in the order that makes the most sense
# for the architecture to make global symbol searches.
                               ^^^^^^^^^^^^^
> The testsuite passes, and any noticeable difference
> by this change would be dependent on the order in which
> symtabs got expanded.  Thus I can't think of a reason to not
> apply this change.

If read this
<https://www.sourceware.org/ml/gdb-patches/2012-05/msg00204.html>
correctly, Joel expressed the same intention there.

Since gdbarch method iterate_over_objfiles_in_search_order was added for
windows target and this patch uses it, we need to test it on windows target.
If you don't have mingw testing env, let me know, I'll see if I can do
the test.

-- 
Yao (齐尧)

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Doug Evans]

Yao Qi <yao@codesourcery.com> writes:
> Doug Evans <xdje42@gmail.com> writes:
>
>> struct symbol *
>> lookup_static_symbol_aux (const char *name, const domain_enum domain)
>> {
>>   struct objfile *objfile;
>>   struct symbol *sym;
>>
>>   sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, domain);
>>   if (sym != NULL)
>>     return sym;
>>
>>   ALL_OBJFILES (objfile)
>>   {
>>     sym = lookup_symbol_aux_quick (objfile, STATIC_BLOCK, name, domain);
>>     if (sym != NULL)
>>       return sym;
>>   }
>>
>>   return NULL;
>> }
>>
>> Note what we're doing here.
>> First we're searching over all expanded symtabs in all objfiles,
>> and then we search partial/gdb_index tables in all objfiles.  Eh?
>>
>> Normally when looking up a symbol in a particular objfile
>> we first list in expanded symtabs and then look in partial/gdb_index
>> tables.  And *then* we try the next objfile.
>>
>> I can't think of any justification for the current behaviour.
>>
>> This patch changes things to be consistent,
>> but it is a behavioural change.
>
> Yes, it changes the behavior.  Here is an example in my mind, but not
> sure it is correct or not, say, we have a static int foo defined in two
> objfiles respectively (1.c and 2.c), foo is in the partial table of two
> objfiles, but is only expanded to the full symtab of *one* objfile (2.c).
>
>              1.c    2.c
>   partial    foo    foo
>   full              foo

Also note that the context is searching across objfiles,
so 1.c and 2.c are also 1.so and 2.so.

> before your patch, GDB gets foo from 2.c full table, and after it, GDB
> gets foo from 1.c partial table.  Is it possible?

The question isn't whether it is possible,
the question is whether this is a behavioural change
on which we make some kind of guarantee.
For the task at hand, we should be making a guarantee
related to library search order before making
any kind of guarantee related to internal
implementation details (partial vs full syms).
[I'm not saying we can or should make search order
guarantees, per se.  Rather, such things should
at least be coherent.]
With this patch we now perform a proper full search
of libraries in a partcular order
(however that order is defined which is a separate discussion).
Whereas today we could find foo in the last library in the search order,
even if every library has foo, just because someone accessed
some other symbol in the last library and caused the
symtab with foo to be expanded.

> Regarding the change, we also need to update the comments to
> iterate_over_objfiles_in_search_order in gdbarch.sh,
>
> # Iterate over all objfiles in the order that makes the most sense
> # for the architecture to make global symbol searches.
>                                ^^^^^^^^^^^^^

Ah, righto.

>> The testsuite passes, and any noticeable difference
>> by this change would be dependent on the order in which
>> symtabs got expanded.  Thus I can't think of a reason to not
>> apply this change.
>
> If read this
> <https://www.sourceware.org/ml/gdb-patches/2012-05/msg00204.html>
> correctly, Joel expressed the same intention there.

Righto.

> Since gdbarch method iterate_over_objfiles_in_search_order was added for
> windows target and this patch uses it, we need to test it on windows target.
> If you don't have mingw testing env, let me know, I'll see if I can do
> the test.

I'm going to be making more symtab changes so I might as well
get mingw testing going here.
Testing in progress.

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Doug Evans]

Doug Evans <xdje42@gmail.com> writes:
> Yao Qi <yao@codesourcery.com> writes:
>> Doug Evans <xdje42@gmail.com> writes:
>>
>>> struct symbol *
>>> lookup_static_symbol_aux (const char *name, const domain_enum domain)
>>> {
>>>   struct objfile *objfile;
>>>   struct symbol *sym;
>>>
>>>   sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, domain);
>>>   if (sym != NULL)
>>>     return sym;
>>>
>>>   ALL_OBJFILES (objfile)
>>>   {
>>>     sym = lookup_symbol_aux_quick (objfile, STATIC_BLOCK, name, domain);
>>>     if (sym != NULL)
>>>       return sym;
>>>   }
>>>
>>>   return NULL;
>>> }
>>>
>>> Note what we're doing here.
>>> First we're searching over all expanded symtabs in all objfiles,
>>> and then we search partial/gdb_index tables in all objfiles.  Eh?
>>>
>>> Normally when looking up a symbol in a particular objfile
>>> we first list in expanded symtabs and then look in partial/gdb_index
>>> tables.  And *then* we try the next objfile.
>>>
>>> I can't think of any justification for the current behaviour.
>>>
>>> This patch changes things to be consistent,
>>> but it is a behavioural change.
>>
>> Yes, it changes the behavior.  Here is an example in my mind, but not
>> sure it is correct or not, say, we have a static int foo defined in two
>> objfiles respectively (1.c and 2.c), foo is in the partial table of two
>> objfiles, but is only expanded to the full symtab of *one* objfile (2.c).
>>
>>              1.c    2.c
>>   partial    foo    foo
>>   full              foo
>
> Also note that the context is searching across objfiles,
> so 1.c and 2.c are also 1.so and 2.so.
>
>> before your patch, GDB gets foo from 2.c full table, and after it, GDB
>> gets foo from 1.c partial table.  Is it possible?
>
> The question isn't whether it is possible,
> the question is whether this is a behavioural change
> on which we make some kind of guarantee.
> For the task at hand, we should be making a guarantee
> related to library search order before making
> any kind of guarantee related to internal
> implementation details (partial vs full syms).
> [I'm not saying we can or should make search order
> guarantees, per se.  Rather, such things should
> at least be coherent.]
> With this patch we now perform a proper full search
> of libraries in a partcular order
> (however that order is defined which is a separate discussion).
> Whereas today we could find foo in the last library in the search order,
> even if every library has foo, just because someone accessed
> some other symbol in the last library and caused the
> symtab with foo to be expanded.
>
>> Regarding the change, we also need to update the comments to
>> iterate_over_objfiles_in_search_order in gdbarch.sh,
>>
>> # Iterate over all objfiles in the order that makes the most sense
>> # for the architecture to make global symbol searches.
>>                                ^^^^^^^^^^^^^
>
> Ah, righto.
>
>>> The testsuite passes, and any noticeable difference
>>> by this change would be dependent on the order in which
>>> symtabs got expanded.  Thus I can't think of a reason to not
>>> apply this change.
>>
>> If read this
>> <https://www.sourceware.org/ml/gdb-patches/2012-05/msg00204.html>
>> correctly, Joel expressed the same intention there.
>
> Righto.
>
>> Since gdbarch method iterate_over_objfiles_in_search_order was added for
>> windows target and this patch uses it, we need to test it on windows target.
>> If you don't have mingw testing env, let me know, I'll see if I can do
>> the test.
>
> I'm going to be making more symtab changes so I might as well
> get mingw testing going here.
> Testing in progress.

Hi.
I was looking at the callers of lookup_static_symbol,
and I think there is more cleanup possible here,
but I'd rather not take that on at the moment.
So I'm going with a simplified version of my previous patch.

Regression tested on amd64-linux.

2014-11-07  Doug Evans  <xdje42@gmail.com>

	* symtab.c (lookup_symbol_in_all_objfiles): Delete.
	(lookup_static_symbol): Move definition to new location and rewrite.
	(lookup_symbol_in_objfile): New function.
	(lookup_symbol_global_iterator_cb): Call it.

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 2aae04c..ec1c033 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -80,11 +80,6 @@ struct symbol *lookup_local_symbol (const char *name,
 				    enum language language);
 
 static
-struct symbol *lookup_symbol_in_all_objfiles (int block_index,
-					      const char *name,
-					      const domain_enum domain);
-
-static
 struct symbol *lookup_symbol_via_quick_fns (struct objfile *objfile,
 					    int block_index,
 					    const char *name,
@@ -1474,28 +1469,6 @@ lookup_symbol_aux (const char *name, const struct block *block,
   return lookup_static_symbol (name, domain);
 }
 
-/* See symtab.h.  */
-
-struct symbol *
-lookup_static_symbol (const char *name, const domain_enum domain)
-{
-  struct objfile *objfile;
-  struct symbol *sym;
-
-  sym = lookup_symbol_in_all_objfiles (STATIC_BLOCK, name, domain);
-  if (sym != NULL)
-    return sym;
-
-  ALL_OBJFILES (objfile)
-  {
-    sym = lookup_symbol_via_quick_fns (objfile, STATIC_BLOCK, name, domain);
-    if (sym != NULL)
-      return sym;
-  }
-
-  return NULL;
-}
-
 /* Check to see if the symbol is defined in BLOCK or its superiors.
    Don't search STATIC_BLOCK or GLOBAL_BLOCK.  */
 
@@ -1650,27 +1623,6 @@ lookup_symbol_in_objfile_symtabs (struct objfile *objfile, int block_index,
   return NULL;
 }
 
-/* Wrapper around lookup_symbol_in_objfile_symtabs to search all objfiles.
-   Returns the first match found.  */
-
-static struct symbol *
-lookup_symbol_in_all_objfiles (int block_index, const char *name,
-			       const domain_enum domain)
-{
-  struct symbol *sym;
-  struct objfile *objfile;
-
-  ALL_OBJFILES (objfile)
-  {
-    sym = lookup_symbol_in_objfile_symtabs (objfile, block_index, name,
-					    domain);
-    if (sym)
-      return sym;
-  }
-
-  return NULL;
-}
-
 /* Wrapper around lookup_symbol_in_objfile_symtabs for search_symbols.
    Look up LINKAGE_NAME in DOMAIN in the global and static blocks of OBJFILE
    and all related objfiles.  */
@@ -1774,7 +1726,7 @@ basic_lookup_symbol_nonlocal (const char *name,
      not it would be appropriate to search the current global block
      here as well.  (That's what this code used to do before the
      is_a_field_of_this check was moved up.)  On the one hand, it's
-     redundant with the lookup_symbol_in_all_objfiles search that happens
+     redundant with the lookup in all objfiles search that happens
      next.  On the other hand, if decode_line_1 is passed an argument
      like filename:var, then the user presumably wants 'var' to be
      searched for in filename.  On the third hand, there shouldn't be
@@ -1818,6 +1770,46 @@ lookup_symbol_in_static_block (const char *name,
     return NULL;
 }
 
+/* Perform the standard symbol lookup of NAME in OBJFILE:
+   1) First search expanded symtabs, and if not found
+   2) Search the "quick" symtabs (partial or .gdb_index).
+   BLOCK_INDEX is one of GLOBAL_BLOCK or STATIC_BLOCK.  */
+
+static struct symbol *
+lookup_symbol_in_objfile (struct objfile *objfile, int block_index,
+			  const char *name, const domain_enum domain)
+{
+  struct symbol *result;
+
+  result = lookup_symbol_in_objfile_symtabs (objfile, block_index,
+					     name, domain);
+  if (result == NULL)
+    {
+      result = lookup_symbol_via_quick_fns (objfile, block_index,
+					    name, domain);
+    }
+
+  return result;
+}
+
+/* See symtab.h.  */
+
+struct symbol *
+lookup_static_symbol (const char *name, const domain_enum domain)
+{
+  struct objfile *objfile;
+  struct symbol *result;
+
+  ALL_OBJFILES (objfile)
+    {
+      result = lookup_symbol_in_objfile (objfile, STATIC_BLOCK, name, domain);
+      if (result != NULL)
+	return result;
+    }
+
+  return NULL;
+}
+
 /* Private data to be used with lookup_symbol_global_iterator_cb.  */
 
 struct global_sym_lookup_data
@@ -1847,11 +1839,8 @@ lookup_symbol_global_iterator_cb (struct objfile *objfile,
 
   gdb_assert (data->result == NULL);
 
-  data->result = lookup_symbol_in_objfile_symtabs (objfile, GLOBAL_BLOCK,
-						   data->name, data->domain);
-  if (data->result == NULL)
-    data->result = lookup_symbol_via_quick_fns (objfile, GLOBAL_BLOCK,
-						data->name, data->domain);
+  data->result = lookup_symbol_in_objfile (objfile, GLOBAL_BLOCK,
+					   data->name, data->domain);
 
   /* If we found a match, tell the iterator to stop.  Otherwise,
      keep going.  */

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Doug Evans]

Doug Evans <xdje42@gmail.com> writes:
> Doug Evans <xdje42@gmail.com> writes:
>> Yao Qi <yao@codesourcery.com> writes:
>>> Doug Evans <xdje42@gmail.com> writes:
>>>
>>>> struct symbol *
>>>> lookup_static_symbol_aux (const char *name, const domain_enum domain)
>>>> {
>>>>   struct objfile *objfile;
>>>>   struct symbol *sym;
>>>>
>>>>   sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, domain);
>>>>   if (sym != NULL)
>>>>     return sym;
>>>>
>>>>   ALL_OBJFILES (objfile)
>>>>   {
>>>>     sym = lookup_symbol_aux_quick (objfile, STATIC_BLOCK, name, domain);
>>>>     if (sym != NULL)
>>>>       return sym;
>>>>   }
>>>>
>>>>   return NULL;
>>>> }
>>>>
>>>> Note what we're doing here.
>>>> First we're searching over all expanded symtabs in all objfiles,
>>>> and then we search partial/gdb_index tables in all objfiles.  Eh?
>>>>
>>>> Normally when looking up a symbol in a particular objfile
>>>> we first list in expanded symtabs and then look in partial/gdb_index
>>>> tables.  And *then* we try the next objfile.
>>>>
>>>> I can't think of any justification for the current behaviour.
>>>>
>>>> This patch changes things to be consistent,
>>>> but it is a behavioural change.
>>>
>>> Yes, it changes the behavior.  Here is an example in my mind, but not
>>> sure it is correct or not, say, we have a static int foo defined in two
>>> objfiles respectively (1.c and 2.c), foo is in the partial table of two
>>> objfiles, but is only expanded to the full symtab of *one* objfile (2.c).
>>>
>>>              1.c    2.c
>>>   partial    foo    foo
>>>   full              foo
>>
>> Also note that the context is searching across objfiles,
>> so 1.c and 2.c are also 1.so and 2.so.
>>
>>> before your patch, GDB gets foo from 2.c full table, and after it, GDB
>>> gets foo from 1.c partial table.  Is it possible?
>>
>> The question isn't whether it is possible,
>> the question is whether this is a behavioural change
>> on which we make some kind of guarantee.
>> For the task at hand, we should be making a guarantee
>> related to library search order before making
>> any kind of guarantee related to internal
>> implementation details (partial vs full syms).
>> [I'm not saying we can or should make search order
>> guarantees, per se.  Rather, such things should
>> at least be coherent.]
>> With this patch we now perform a proper full search
>> of libraries in a partcular order
>> (however that order is defined which is a separate discussion).
>> Whereas today we could find foo in the last library in the search order,
>> even if every library has foo, just because someone accessed
>> some other symbol in the last library and caused the
>> symtab with foo to be expanded.
>>
>>> Regarding the change, we also need to update the comments to
>>> iterate_over_objfiles_in_search_order in gdbarch.sh,
>>>
>>> # Iterate over all objfiles in the order that makes the most sense
>>> # for the architecture to make global symbol searches.
>>>                                ^^^^^^^^^^^^^
>>
>> Ah, righto.
>>
>>>> The testsuite passes, and any noticeable difference
>>>> by this change would be dependent on the order in which
>>>> symtabs got expanded.  Thus I can't think of a reason to not
>>>> apply this change.
>>>
>>> If read this
>>> <https://www.sourceware.org/ml/gdb-patches/2012-05/msg00204.html>
>>> correctly, Joel expressed the same intention there.
>>
>> Righto.
>>
>>> Since gdbarch method iterate_over_objfiles_in_search_order was added for
>>> windows target and this patch uses it, we need to test it on windows target.
>>> If you don't have mingw testing env, let me know, I'll see if I can do
>>> the test.
>>
>> I'm going to be making more symtab changes so I might as well
>> get mingw testing going here.
>> Testing in progress.
>
> Hi.
> I was looking at the callers of lookup_static_symbol,
> and I think there is more cleanup possible here,
> but I'd rather not take that on at the moment.
> So I'm going with a simplified version of my previous patch.
>
> Regression tested on amd64-linux.
>
> 2014-11-07  Doug Evans  <xdje42@gmail.com>
>
> 	* symtab.c (lookup_symbol_in_all_objfiles): Delete.
> 	(lookup_static_symbol): Move definition to new location and rewrite.
> 	(lookup_symbol_in_objfile): New function.
> 	(lookup_symbol_global_iterator_cb): Call it.

Hi.

I filed pr 17564 to document the issue.
https://sourceware.org/bugzilla/show_bug.cgi?id=17564

Attached is a testcase.
I'll add the PR number to the above ChangeLog entry as well.

2014-11-07  Doug Evans  <xdje42@gmail.com>

	PR symtab/17564
	* gdb.base/symtab-search-order.exp: New file.
	* gdb.base/symtab-search-order.c: New file.
	* gdb.base/symtab-search-order-1.c: New file.
	* gdb.base/symtab-search-order-shlib-1.c: New file.

diff --git a/gdb/testsuite/gdb.base/symtab-search-order-1.c b/gdb/testsuite/gdb.base/symtab-search-order-1.c
new file mode 100644
index 0000000..bff9b7a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symtab-search-order-1.c
@@ -0,0 +1 @@
+static int static_global = 23;
diff --git a/gdb/testsuite/gdb.base/symtab-search-order-shlib-1.c b/gdb/testsuite/gdb.base/symtab-search-order-shlib-1.c
new file mode 100644
index 0000000..a23da5f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symtab-search-order-shlib-1.c
@@ -0,0 +1,7 @@
+static int static_global = 42;
+
+int
+shlib_1_func (void)
+{
+  return static_global;
+}
diff --git a/gdb/testsuite/gdb.base/symtab-search-order.c b/gdb/testsuite/gdb.base/symtab-search-order.c
new file mode 100644
index 0000000..71496c8
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symtab-search-order.c
@@ -0,0 +1,6 @@
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/symtab-search-order.exp b/gdb/testsuite/gdb.base/symtab-search-order.exp
new file mode 100644
index 0000000..eb39d87
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symtab-search-order.exp
@@ -0,0 +1,59 @@
+# Copyright 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+standard_testfile .c symtab-search-order-1.c symtab-search-order-shlib-1.c
+set srcfile  $srcdir/$subdir/$srcfile
+set srcfile2 $srcdir/$subdir/$srcfile2
+set lib1src  $srcdir/$subdir/$srcfile3
+set lib1     [standard_output_file symtab-search-order-1.sl]
+
+set lib_opts "debug"
+set exec_opts [list debug shlib=$lib1]
+
+if [get_compiler_info] {
+    return -1
+}
+
+if { [gdb_compile_shlib $lib1src $lib1 $lib_opts] != ""
+     || [gdb_compile [list $srcfile $srcfile2] $binfile executable \
+	     $exec_opts] != ""} {
+    untested "Could not compile $lib1, or $srcfile."
+    return -1
+}
+
+# Start with a fresh gdb.
+
+clean_restart $binfile
+gdb_load_shlibs $lib1
+
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+# PR 17564
+# Expand something in the shared library,
+# and then try to print static_global in the binary.
+# We should get the static_global in the binary.
+# Note: static_global in the binary needs to be in a file
+# other than the one with "main" because gdb will expand
+# the symtab with main when starting.
+
+gdb_test "p shlib_1_func" "= .*<shlib_1_func>"
+gdb_test "p static_global" " = 23"

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Doug Evans]

Doug Evans <xdje42@gmail.com> writes:
> [...]
> I filed pr 17564 to document the issue.
> https://sourceware.org/bugzilla/show_bug.cgi?id=17564
>
> Attached is a testcase.
> I'll add the PR number to the above ChangeLog entry as well.
>
> 2014-11-07  Doug Evans  <xdje42@gmail.com>
>
> 	PR symtab/17564
> 	* gdb.base/symtab-search-order.exp: New file.
> 	* gdb.base/symtab-search-order.c: New file.
> 	* gdb.base/symtab-search-order-1.c: New file.
> 	* gdb.base/symtab-search-order-shlib-1.c: New file.

Heh.
Testing on windows showed a bad testcase.
FAIL: gdb.base/symtab-search-order.exp: p shlib_1_func

Here's a fix.
The testcase wasn't referencing anything in the shlib
so it was being discarded from the link.

2014-11-08  Doug Evans  <xdje42@gmail.com>

	PR symtab/17564
	* gdb.base/symtab-search-order.exp: New file.
	* gdb.base/symtab-search-order.c: New file.
	* gdb.base/symtab-search-order-1.c: New file.
	* gdb.base/symtab-search-order-shlib-1.c: New file.

diff --git a/gdb/testsuite/gdb.base/symtab-search-order-1.c b/gdb/testsuite/gdb.base/symtab-search-order-1.c
new file mode 100644
index 0000000..bff9b7a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symtab-search-order-1.c
@@ -0,0 +1 @@
+static int static_global = 23;
diff --git a/gdb/testsuite/gdb.base/symtab-search-order-shlib-1.c b/gdb/testsuite/gdb.base/symtab-search-order-shlib-1.c
new file mode 100644
index 0000000..a23da5f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symtab-search-order-shlib-1.c
@@ -0,0 +1,7 @@
+static int static_global = 42;
+
+int
+shlib_1_func (void)
+{
+  return static_global;
+}
diff --git a/gdb/testsuite/gdb.base/symtab-search-order.c b/gdb/testsuite/gdb.base/symtab-search-order.c
new file mode 100644
index 0000000..ab38db6
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symtab-search-order.c
@@ -0,0 +1,11 @@
+extern int shlib_1_func (void);
+
+int
+main ()
+{
+  /* We need a reference to shlib_1_func to make sure its shlib is
+     not discarded from the link.  This happens on windows.  */
+  int x = shlib_1_func ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/symtab-search-order.exp b/gdb/testsuite/gdb.base/symtab-search-order.exp
new file mode 100644
index 0000000..eb39d87
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symtab-search-order.exp
@@ -0,0 +1,59 @@
+# Copyright 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+standard_testfile .c symtab-search-order-1.c symtab-search-order-shlib-1.c
+set srcfile  $srcdir/$subdir/$srcfile
+set srcfile2 $srcdir/$subdir/$srcfile2
+set lib1src  $srcdir/$subdir/$srcfile3
+set lib1     [standard_output_file symtab-search-order-1.sl]
+
+set lib_opts "debug"
+set exec_opts [list debug shlib=$lib1]
+
+if [get_compiler_info] {
+    return -1
+}
+
+if { [gdb_compile_shlib $lib1src $lib1 $lib_opts] != ""
+     || [gdb_compile [list $srcfile $srcfile2] $binfile executable \
+	     $exec_opts] != ""} {
+    untested "Could not compile $lib1, or $srcfile."
+    return -1
+}
+
+# Start with a fresh gdb.
+
+clean_restart $binfile
+gdb_load_shlibs $lib1
+
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+# PR 17564
+# Expand something in the shared library,
+# and then try to print static_global in the binary.
+# We should get the static_global in the binary.
+# Note: static_global in the binary needs to be in a file
+# other than the one with "main" because gdb will expand
+# the symtab with main when starting.
+
+gdb_test "p shlib_1_func" "= .*<shlib_1_func>"
+gdb_test "p static_global" " = 23"

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Doug Evans]

On Sat, Nov 8, 2014 at 11:30 AM, Doug Evans <xdje42@gmail.com> wrote:
> Doug Evans <xdje42@gmail.com> writes:
>> [...]
>> I filed pr 17564 to document the issue.
>> https://sourceware.org/bugzilla/show_bug.cgi?id=17564
>>
>> Attached is a testcase.
>> I'll add the PR number to the above ChangeLog entry as well.
>>
>> 2014-11-07  Doug Evans  <xdje42@gmail.com>
>>
>>       PR symtab/17564
>>       * gdb.base/symtab-search-order.exp: New file.
>>       * gdb.base/symtab-search-order.c: New file.
>>       * gdb.base/symtab-search-order-1.c: New file.
>>       * gdb.base/symtab-search-order-shlib-1.c: New file.
>
> Heh.
> Testing on windows showed a bad testcase.
> FAIL: gdb.base/symtab-search-order.exp: p shlib_1_func
>
> Here's a fix.
> The testcase wasn't referencing anything in the shlib
> so it was being discarded from the link.

Hi.
I've committed these patches.

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Yao Qi]

Doug Evans <xdje42@gmail.com> writes:

Hi Doug,
Sorry for the delayed response.

> 2014-11-08  Doug Evans  <xdje42@gmail.com>
>
> 	PR symtab/17564
> 	* gdb.base/symtab-search-order.exp: New file.
> 	* gdb.base/symtab-search-order.c: New file.
> 	* gdb.base/symtab-search-order-1.c: New file.
> 	* gdb.base/symtab-search-order-shlib-1.c: New file.

These new .c files need copyright headers.

-- 
Yao (齐尧)

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Doug Evans]

On Thu, Nov 13, 2014 at 4:52 AM, Yao Qi <yao@codesourcery.com> wrote:
> Doug Evans <xdje42@gmail.com> writes:
>
> Hi Doug,
> Sorry for the delayed response.
>
>> 2014-11-08  Doug Evans  <xdje42@gmail.com>
>>
>>       PR symtab/17564
>>       * gdb.base/symtab-search-order.exp: New file.
>>       * gdb.base/symtab-search-order.c: New file.
>>       * gdb.base/symtab-search-order-1.c: New file.
>>       * gdb.base/symtab-search-order-shlib-1.c: New file.
>
> These new .c files need copyright headers.

These files are so trivial, I left them out.
[The .exp file *does* have a copyright header.]

I know the contribution checklist page mentions always adding them,
even if the GNU standards only require them for legally significant
headers.
It's not clear that that is intended to apply in particular cases such as these.

[I can add them, but establishing some clarity would be nice.]

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Doug Evans]

Doug Evans <xdje42@gmail.com> writes:
> On Thu, Nov 13, 2014 at 4:52 AM, Yao Qi <yao@codesourcery.com> wrote:
>> Doug Evans <xdje42@gmail.com> writes:
>>
>> Hi Doug,
>> Sorry for the delayed response.
>>
>>> 2014-11-08  Doug Evans  <xdje42@gmail.com>
>>>
>>>       PR symtab/17564
>>>       * gdb.base/symtab-search-order.exp: New file.
>>>       * gdb.base/symtab-search-order.c: New file.
>>>       * gdb.base/symtab-search-order-1.c: New file.
>>>       * gdb.base/symtab-search-order-shlib-1.c: New file.
>>
>> These new .c files need copyright headers.
>
> These files are so trivial, I left them out.
> [The .exp file *does* have a copyright header.]
>
> I know the contribution checklist page mentions always adding them,
> even if the GNU standards only require them for legally significant
> headers.
> It's not clear that that is intended to apply in particular cases such as these.
>
> [I can add them, but establishing some clarity would be nice.]

Hi.
To keep things moving along I added the copyright headers.

However, I think we could be a bit more lax when it comes to
such trivial testcases.

GCC isn't this pedantic about it.

diff --git a/gdb/testsuite/gdb.base/symtab-search-order-1.c b/gdb/testsuite/gdb.base/symtab-search-order-1.c
index bff9b7a..317b0e7 100644
--- a/gdb/testsuite/gdb.base/symtab-search-order-1.c
+++ b/gdb/testsuite/gdb.base/symtab-search-order-1.c
@@ -1 +1,18 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
 static int static_global = 23;
diff --git a/gdb/testsuite/gdb.base/symtab-search-order-shlib-1.c b/gdb/testsuite/gdb.base/symtab-search-order-shlib-1.c
index a23da5f..537be53 100644
--- a/gdb/testsuite/gdb.base/symtab-search-order-shlib-1.c
+++ b/gdb/testsuite/gdb.base/symtab-search-order-shlib-1.c
@@ -1,3 +1,20 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
 static int static_global = 42;
 
 int
diff --git a/gdb/testsuite/gdb.base/symtab-search-order.c b/gdb/testsuite/gdb.base/symtab-search-order.c
index ab38db6..ea58957 100644
--- a/gdb/testsuite/gdb.base/symtab-search-order.c
+++ b/gdb/testsuite/gdb.base/symtab-search-order.c
@@ -1,3 +1,20 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
 extern int shlib_1_func (void);
 
 int

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Joel Brobecker]

Hi Doug,

> To keep things moving along I added the copyright headers.

Thanks for doing that.

> However, I think we could be a bit more lax when it comes to
> such trivial testcases.

The FSF allows us to be lax for trivial files (with 10 lines being
one explicit limit on triviality).  But:

The problem with being lax is that it creates a little extra work
for us at the start of each year, when we update the copyright headers.
The FSF has been asking us to look into copyright headers in the past,
and it was a lot of work, so I've added a bit of simple-minded code
to the script that updates the copyright headers in GDB to also issue
a warning if a copyright header hasn't been found. That way, we can
then investigate the few problematic files each year rather than
a massive number when the FSF notices something.

That's why I always appreciate the copyright headers being added
right from the start.

So, again, thanks a lot for adding those! :)
-- 
Joel

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Doug Evans]

On Sat, Nov 15, 2014 at 7:22 PM, Joel Brobecker <brobecker@adacore.com> wrote:
> Hi Doug,
>
>> To keep things moving along I added the copyright headers.
>
> Thanks for doing that.
>
>> However, I think we could be a bit more lax when it comes to
>> such trivial testcases.
>
> The FSF allows us to be lax for trivial files (with 10 lines being
> one explicit limit on triviality).  But:
>
> The problem with being lax is that it creates a little extra work
> for us at the start of each year, when we update the copyright headers.
> The FSF has been asking us to look into copyright headers in the past,
> and it was a lot of work, so I've added a bit of simple-minded code
> to the script that updates the copyright headers in GDB to also issue
> a warning if a copyright header hasn't been found. That way, we can
> then investigate the few problematic files each year rather than
> a massive number when the FSF notices something.
>
> That's why I always appreciate the copyright headers being added
> right from the start.
>
> So, again, thanks a lot for adding those! :)

No worries.
A few more words added to the wiki to added the needed context and
clarity would be great.
I'll do that.

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Doug Evans]

On Sun, Nov 16, 2014 at 1:53 PM, Doug Evans <xdje42@gmail.com> wrote:
> On Sat, Nov 15, 2014 at 7:22 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> Hi Doug,
>>
>>> To keep things moving along I added the copyright headers.
>>
>> Thanks for doing that.
>>
>>> However, I think we could be a bit more lax when it comes to
>>> such trivial testcases.
>>
>> The FSF allows us to be lax for trivial files (with 10 lines being
>> one explicit limit on triviality).  But:
>>
>> The problem with being lax is that it creates a little extra work
>> for us at the start of each year, when we update the copyright headers.
>> The FSF has been asking us to look into copyright headers in the past,
>> and it was a lot of work, so I've added a bit of simple-minded code
>> to the script that updates the copyright headers in GDB to also issue
>> a warning if a copyright header hasn't been found. That way, we can
>> then investigate the few problematic files each year rather than
>> a massive number when the FSF notices something.
>>
>> That's why I always appreciate the copyright headers being added
>> right from the start.
>>
>> So, again, thanks a lot for adding those! :)
>
> No worries.
> A few more words added to the wiki to added the needed context and
> clarity would be great.
> I'll do that.

Hmmm, I see that in gdb the convention is "Copyright (C)" whereas in
the gdb/testsuite the convention is "Copyright" with no "(C)".
Some tests have "(C)" but the vast majority don't.
Both are allowed, and I'm not planning on going through and changing anything.
Just thought I'd point it out.

Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine

[Joel Brobecker]

> Hmmm, I see that in gdb the convention is "Copyright (C)" whereas in
> the gdb/testsuite the convention is "Copyright" with no "(C)".
> Some tests have "(C)" but the vast majority don't.
> Both are allowed, and I'm not planning on going through and changing anything.
> Just thought I'd point it out.

Yeah :-( - I think the prefered form is with the (C), and I wish that
it was automatically added during the copyright year update. But,
of course, this is not possible without the script having knowledge
of the file type because it's different for files such as texi files,
for instance. I might write a one-off python script specifically for
that specific purpose, someday....

-- 
Joel