From patchwork Fri Nov 7 16:28:07 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Evans X-Patchwork-Id: 3612 Received: (qmail 28459 invoked by alias); 7 Nov 2014 16:29:03 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 28449 invoked by uid 89); 7 Nov 2014 16:29:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f43.google.com Received: from mail-pa0-f43.google.com (HELO mail-pa0-f43.google.com) (209.85.220.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 07 Nov 2014 16:29:00 +0000 Received: by mail-pa0-f43.google.com with SMTP id eu11so3817747pac.30 for ; Fri, 07 Nov 2014 08:28:59 -0800 (PST) X-Received: by 10.66.65.137 with SMTP id x9mr13448109pas.0.1415377738767; Fri, 07 Nov 2014 08:28:58 -0800 (PST) Received: from seba.sebabeach.org.gmail.com (173-13-178-53-sfba.hfc.comcastbusiness.net. [173.13.178.53]) by mx.google.com with ESMTPSA id 16sm9194794pdj.42.2014.11.07.08.28.57 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 07 Nov 2014 08:28:58 -0800 (PST) From: Doug Evans To: Yao Qi Cc: Subject: Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine References: <87d29cvqfu.fsf@codesourcery.com> Date: Fri, 07 Nov 2014 08:28:07 -0800 In-Reply-To: (Doug Evans's message of "Fri, 07 Nov 2014 01:07:19 -0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes Doug Evans writes: > Doug Evans writes: >> Yao Qi writes: >>> Doug Evans 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 >>> >>> 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 > > * 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 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 . + +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" "= .*" +gdb_test "p static_global" " = 23"