From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 75861 invoked by alias); 19 Oct 2017 19:07:25 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 74527 invoked by uid 89); 19 Oct 2017 19:07:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=Independent X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 19 Oct 2017 19:07:22 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v9JJ7FnT023900 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 19 Oct 2017 15:07:20 -0400 Received: by simark.ca (Postfix, from userid 112) id 8868A1E566; Thu, 19 Oct 2017 15:07:15 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 44CE21E541; Thu, 19 Oct 2017 15:06:53 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 19 Oct 2017 19:07:00 -0000 From: Simon Marchi To: Walfred Tedeschi Cc: palves@redhat.com, gdb-patches@sourceware.org Subject: Re: [PATCH V4] symlookup: improves symbol lookup when a file is specified. In-Reply-To: <1508317280-31265-1-git-send-email-walfred.tedeschi@intel.com> References: <1508317280-31265-1-git-send-email-walfred.tedeschi@intel.com> Message-ID: <327caaf3429595c07a29d455ea3ed6a0@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 19 Oct 2017 19:07:15 +0000 X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00642.txt.bz2 Hi, Just a nit, git shows this when applying your patch: Applying: symlookup: improves symbol lookup when a file is specified. .git/rebase-apply/patch:157: new blank line at EOF. + .git/rebase-apply/patch:253: new blank line at EOF. + warning: 2 lines add whitespace errors. If there are extra blank lines at the end of the new files, you can remove them. On 2017-10-18 05:01, Walfred Tedeschi wrote: > The provided patch adds a scope lookup with higher priority in the case > a file is provided for the evaluation. > > Usual symbol lookup from GDB follows linker logic. This is what is > desired most of the time. In the usage case presented a full qualified > scope is provided, so the lookup has to follow this priority. Failing > in > finding the symbol at this scope usual path is followed. > > As use and test case it is presented two shared objects having a global > variable with the same name but comming from different source files. > Without the patch evaluating those variables providing the file scope > returns the wrong value; It returns the value seen in the first loaded > shared object. Using the patch the value defined in the file scope is > the one returned. Without using this patch the result of the > evaluations > are: > > print 'print-file-var-lib1.c'::this_version_id > $1 = 104 > > and > > print 'print-file-var-lib2.c'::this_version_id > $2 = 104 # This value is wrong we have set 203 in the code > > > With the patch applied we get: > > print 'print-file-var-lib1.c'::this_version_id > $1 = 104 > > and > > print 'print-file-var-lib2.c'::this_version_id > $2 = 203 # Now value is correct > > > > One of the already existing test cases in print-file-var.exp starts to > fail after aplying this patch. In fact the value that the linker > sees is different then the one debugger can read from the shared > object. > In this sense it looks like to me that the test has to be changed. > The fail is in the call: > print 'print-file-var-lib2.c'::this_version_id == v2 > > in this case v1 and v2 are the same and equal to the this_version_id > defined in print-file-var-lib1.c. Test is changed to assure that > when the scope operator is used the value seen in print-file-var-lib2.c > is the one GDB evaluates, i.e. 203. > > * Reading again Simon's comments I have decided to take a quick look > on > the failing test. I have fixed it according to the logic of the > current > patch. > > 2017-10-18 Walfred Tedeschi > > gdb/ChangeLog: > > * symtab.c (lookup_global_symbol): Add new lookup to ensure > priority on given block. > > gdb/testsuite/ChangeLog: > > * gdb.base/print-file-var-dlopen-main.c: New file. > * gdb.base/print-file-var-dlopen.exp: New test based on > print-file-var.exp. > * gdb.base/print-file-var-dlopen-lib1.c: New file. > * gdb.base/print-file-var-dlopen-lib2.c: New file. > * gdb.base/print-file-var.exp: Modify expected result for > evaluating 'print-file-var-lib2.c'::this_version_id. > > --- > gdb/symtab.c | 4 + > .../gdb.base/print-file-var-dlopen-lib1.c | 25 ++++++ > .../gdb.base/print-file-var-dlopen-lib2.c | 25 ++++++ > .../gdb.base/print-file-var-dlopen-main.c | 61 > +++++++++++++++ > gdb/testsuite/gdb.base/print-file-var-dlopen.exp | 90 > ++++++++++++++++++++++ > gdb/testsuite/gdb.base/print-file-var.exp | 4 +- > 6 files changed, 208 insertions(+), 1 deletion(-) > create mode 100644 gdb/testsuite/gdb.base/print-file-var-dlopen-lib1.c > create mode 100644 gdb/testsuite/gdb.base/print-file-var-dlopen-lib2.c > create mode 100644 gdb/testsuite/gdb.base/print-file-var-dlopen-main.c > create mode 100644 gdb/testsuite/gdb.base/print-file-var-dlopen.exp > > diff --git a/gdb/symtab.c b/gdb/symtab.c > index 16a6b2e..a2c307f 100644 > --- a/gdb/symtab.c > +++ b/gdb/symtab.c > @@ -2589,6 +2589,10 @@ lookup_global_symbol (const char *name, > if (objfile != NULL) > result = solib_global_lookup (objfile, name, domain); > > + /* We still need to look on the global scope of current object file. > */ > + if (result.symbol == NULL && objfile != NULL) > + result = lookup_symbol_in_objfile (objfile, GLOBAL_BLOCK, name, > domain); > + > /* If that didn't work go a global search (of global blocks, heh). > */ > if (result.symbol == NULL) > { > diff --git a/gdb/testsuite/gdb.base/print-file-var-dlopen-lib1.c > b/gdb/testsuite/gdb.base/print-file-var-dlopen-lib1.c > new file mode 100644 > index 0000000..09ec947 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/print-file-var-dlopen-lib1.c > @@ -0,0 +1,25 @@ > +/* This testcase is part of GDB, the GNU debugger. > + Copyright 2012-2017 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 > . */ > + > +int this_version_id = 104; > + > +int > +get_version (void) > +{ > + static int test; > + test = this_version_id; > + return test; > +} > diff --git a/gdb/testsuite/gdb.base/print-file-var-dlopen-lib2.c > b/gdb/testsuite/gdb.base/print-file-var-dlopen-lib2.c > new file mode 100644 > index 0000000..b097cd2 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/print-file-var-dlopen-lib2.c > @@ -0,0 +1,25 @@ > +/* This testcase is part of GDB, the GNU debugger. > + Copyright 2012-2017 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 > . */ > + > +int this_version_id = 203; > + > +int > +get_version (void) > +{ > + static int test; > + test = this_version_id; > + return test; > +} > diff --git a/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c > b/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c > new file mode 100644 > index 0000000..954a64e > --- /dev/null > +++ b/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c > @@ -0,0 +1,61 @@ > +/* This testcase is part of GDB, the GNU debugger. > + Copyright 2017 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 > . */ > + > +#include > +#include > +#include > + > +int > +dummy (void) > +{ > + return 1; > +} > + > +int > +main (void) > +{ > + int (*get_version1) (void); > + int (*get_version2) (void); > + int v1, v2; > + > + void *lib1 = dlopen ("print-file-var-dlopen-lib1.so", RTLD_LAZY); > + void *lib2 = dlopen ("print-file-var-dlopen-lib2.so", RTLD_LAZY); > + > + if (lib1 == NULL || lib2 == NULL) > + return 1; > + > + *(int **) (&get_version1) = dlsym (lib1, "get_version"); > + *(int **) (&get_version2) = dlsym (lib2, "get_version"); > + > + if (get_version1 != NULL > + && get_version2 != NULL) > + { > + v1 = get_version1(); > + v2 = get_version2(); > + } > + > + > + if (v1 != 104 || v2 != 203) > + return 1; > + > + dummy (); /* STOP */ > + > + dlclose (lib1); > + dlclose (lib2); > + > + return 0; > +} > + > diff --git a/gdb/testsuite/gdb.base/print-file-var-dlopen.exp > b/gdb/testsuite/gdb.base/print-file-var-dlopen.exp > new file mode 100644 > index 0000000..9c3c0e6 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/print-file-var-dlopen.exp > @@ -0,0 +1,90 @@ > +# Copyright 2017 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 > . */ > + Could you add here a one sentence description of what this test file is about? When looking at test files, it is nice to have a quick summary to know what it is testing. > +if {[skip_shlib_tests]} { > + return 0 > +} > + > +set executable print-file-var-dlopen-main > + > +set lib1 "print-file-var-dlopen-lib1" > +set lib2 "print-file-var-dlopen-lib2" > +set libsrc1 $srcdir/$subdir/${lib1}.c > +set libsrc2 $srcdir/$subdir/${lib2}.c > +set libobj1 [standard_output_file ${lib1}.so] > +set libobj2 [standard_output_file ${lib2}.so] > +set lib_dlopen1 [shlib_target_file ${libobj1}] > +set lib_dlopen2 [shlib_target_file ${libobj2}] > + > +set srcfile $srcdir/$subdir/$executable.c > +set binfile [standard_output_file $executable] > +set shlibdir [standard_output_file {}] shlibdir seems unused. > + > +set lib_opts debug > +set exec_opts [list debug shlib_load > additional_flags=-DSHLIB_NAME=\"${lib_dlopen1}\" \ > +additional_flags=-DSHLIB_NAME2=\"${lib_dlopen2}\"] The flags definied SHLIB_NAME and SHLIB_NAME2 are not useful, I think. > + > +if { [gdb_compile_shlib $libsrc1 $libobj1 $lib_opts] != "" > + || [gdb_compile_shlib $libsrc2 $libobj2 $lib_opts] != "" > + || [gdb_compile $srcfile $binfile executable $exec_opts] != ""} { > + untested "failed to compile" > + return -1 > +} > + > +clean_restart $executable > + > +if ![runto_main] { > + untested "could not run to main" > + return -1 > +} > + > +# Create to shared libraries having the symbol "this_version_id" > defined in both "to" -> "two" > +# libraries global scope. Main program will dllopen one by one and > evaluate the "dllopen" -> "dlopen" > +# symbol "this_version_id" via a function provided by the library > assigning the value of > +# "this_version_id" to V1 and V2. > +# Using the scope to perform the evaluations should return the value > +# defined in the c file. > + > +# To avoid adding target-specific code in this testcase, the program > +# sets two local variable named 'v1' and 'v2' with the value of > +# our global variables. This allows us to compare the value that > +# GDB returns for each query against the actual value seen by > +# the program itself. > + > +# Get past the initialization of variables 'v1' and 'v2'. > + > +set bp_location \ > + [gdb_get_line_number "STOP" "${executable}.c"] > +gdb_test "break $executable.c:$bp_location" \ > + "Breakpoint \[0-9\]+ at 0x\[0-9a-fA-F\]+: .*" \ > + "breapoint past v1 & v2 initialization" > + > +gdb_continue_to_breakpoint "continue to the STOP" > + > +# Now check the value of this_version_id in both print-file-var-lib1.c > +# and print-file-var-lib2.c. The filenames mentioned here are wrong. It's probably simpler to say "... in both libraries". > + > +gdb_test "print 'print-file-var-dlopen-lib1.c'::this_version_id == v1" > \ > + " = 1" > + > +gdb_test "print 'print-file-var-dlopen-lib2.c'::this_version_id == v2" > \ > + " = 1" > + > +gdb_test "print 'print-file-var-dlopen-lib2.c'::get_version::test == > v2" \ > + " = 1" > + > +gdb_test "print 'print-file-var-dlopen-lib1.c'::get_version::test == > v1" \ > + " = 1" > + > diff --git a/gdb/testsuite/gdb.base/print-file-var.exp > b/gdb/testsuite/gdb.base/print-file-var.exp > index 223a67d..ae5071a 100644 > --- a/gdb/testsuite/gdb.base/print-file-var.exp > +++ b/gdb/testsuite/gdb.base/print-file-var.exp > @@ -88,5 +88,7 @@ gdb_test "continue" \ > gdb_test "print 'print-file-var-lib1.c'::this_version_id == v1" \ > " = 1" > > -gdb_test "print 'print-file-var-lib2.c'::this_version_id == v2" \ > +# Independent of the linker value seen in the second library should > +# be 203. > +gdb_test "print 'print-file-var-lib2.c'::this_version_id == 203" \ > " = 1" I am not convinced about changing this test. Normally, we want the debugger, when evaluating expressions to act as close as possible to the target program. If the value of this_version_id read in lib2 was 104, then that's probably what print 'print-file-var-lib2.c'::this_version_id should show as well, don't you think? This test is checking that this is the case. Simon