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

[Doug Evans <xdje42 at gmail dot com>]

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"