From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21978 invoked by alias); 20 Oct 2017 07:45:59 -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 21862 invoked by uid 89); 20 Oct 2017 07:45:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=Deutschland, deutschland, Fred, Munich X-HELO: mga02.intel.com Received: from mga02.intel.com (HELO mga02.intel.com) (134.134.136.20) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 20 Oct 2017 07:45:35 +0000 Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Oct 2017 00:45:30 -0700 X-ExtLoop1: 1 Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by fmsmga001.fm.intel.com with ESMTP; 20 Oct 2017 00:45:12 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.248]) by IRSMSX106.ger.corp.intel.com ([169.254.8.36]) with mapi id 14.03.0319.002; Fri, 20 Oct 2017 08:45:10 +0100 From: "Tedeschi, Walfred" To: Simon Marchi CC: "palves@redhat.com" , "gdb-patches@sourceware.org" Subject: RE: [PATCH V4] symlookup: improves symbol lookup when a file is specified. Date: Fri, 20 Oct 2017 07:45:00 -0000 Message-ID: References: <1508317280-31265-1-git-send-email-walfred.tedeschi@intel.com> <327caaf3429595c07a29d455ea3ed6a0@polymtl.ca> In-Reply-To: <327caaf3429595c07a29d455ea3ed6a0@polymtl.ca> dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00657.txt.bz2 > -----Original Message----- > From: Simon Marchi [mailto:simon.marchi@polymtl.ca] > Sent: Thursday, October 19, 2017 9:07 PM > To: Tedeschi, Walfred > Cc: palves@redhat.com; gdb-patches@sourceware.org > Subject: Re: [PATCH V4] symlookup: improves symbol lookup when a file is > specified. >=20 > Hi, >=20 > Just a nit, git shows this when applying your patch: >=20 > 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. >=20 > If there are extra blank lines at the end of the new files, you can remove > them. >=20 > 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 =3D 104 > > > > and > > > > print 'print-file-var-lib2.c'::this_version_id > > $2 =3D 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 =3D 104 > > > > and > > > > print 'print-file-var-lib2.c'::this_version_id > > $2 =3D 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 =3D=3D 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 !=3D NULL) > > result =3D solib_global_lookup (objfile, name, domain); > > > > + /* We still need to look on the global scope of current object file. > > */ > > + if (result.symbol =3D=3D NULL && objfile !=3D NULL) > > + result =3D 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 =3D=3D 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 =3D 104; > > + > > +int > > +get_version (void) > > +{ > > + static int test; > > + test =3D 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 =3D 203; > > + > > +int > > +get_version (void) > > +{ > > + static int test; > > + test =3D 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 =3D dlopen ("print-file-var-dlopen-lib1.so", RTLD_LAZY); > > + void *lib2 =3D dlopen ("print-file-var-dlopen-lib2.so", RTLD_LAZY); > > + > > + if (lib1 =3D=3D NULL || lib2 =3D=3D NULL) > > + return 1; > > + > > + *(int **) (&get_version1) =3D dlsym (lib1, "get_version"); *(int **) > > + (&get_version2) =3D dlsym (lib2, "get_version"); > > + > > + if (get_version1 !=3D NULL > > + && get_version2 !=3D NULL) > > + { > > + v1 =3D get_version1(); > > + v2 =3D get_version2(); > > + } > > + > > + > > + if (v1 !=3D 104 || v2 !=3D 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 > > . */ > > + >=20 > Could you add here a one sentence description of what this test file is a= bout? > When looking at test files, it is nice to have a quick summary to know wh= at it > is testing. >=20 > > +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 > > +{}] >=20 > shlibdir seems unused. >=20 > > + > > +set lib_opts debug > > +set exec_opts [list debug shlib_load > > additional_flags=3D-DSHLIB_NAME=3D\"${lib_dlopen1}\" \ > > +additional_flags=3D-DSHLIB_NAME2=3D\"${lib_dlopen2}\"] >=20 > The flags definied SHLIB_NAME and SHLIB_NAME2 are not useful, I think. >=20 > > + > > +if { [gdb_compile_shlib $libsrc1 $libobj1 $lib_opts] !=3D "" > > + || [gdb_compile_shlib $libsrc2 $libobj2 $lib_opts] !=3D "" > > + || [gdb_compile $srcfile $binfile executable $exec_opts] !=3D ""}= { > > + 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 >=20 > "to" -> "two" >=20 > > +# libraries global scope. Main program will dllopen one by one and > > evaluate the >=20 > "dllopen" -> "dlopen" >=20 > > +# 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. >=20 > The filenames mentioned here are wrong. It's probably simpler to say "..= . in > both libraries". >=20 > > + > > +gdb_test "print 'print-file-var-dlopen-lib1.c'::this_version_id =3D=3D= v1" > > \ > > + " =3D 1" > > + > > +gdb_test "print 'print-file-var-dlopen-lib2.c'::this_version_id =3D=3D= v2" > > \ > > + " =3D 1" > > + > > +gdb_test "print 'print-file-var-dlopen-lib2.c'::get_version::test =3D= =3D > > v2" \ > > + " =3D 1" > > + > > +gdb_test "print 'print-file-var-dlopen-lib1.c'::get_version::test =3D= =3D > > v1" \ > > + " =3D 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 =3D=3D v1" \ > > " =3D 1" > > > > -gdb_test "print 'print-file-var-lib2.c'::this_version_id =3D=3D 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 =3D=3D 203" \ > > " =3D 1" >=20 Hi Simon, Thanks for your review! For all the comment above I agree, Thanks again! For the one below there are different point of views.=20 How I see it: Very few sane people will add a symbols in a shared library t= hat will collide like the case we presented here. If one does so how can the d= ebugger help? Providing the same value as the runtime or linker does?=20 This one user already knows.=20 Or providing what the debug information provides as value created by the li= brary itself. In final end both are right. :| But when specifying the scope if user is provided the value of the debug in= fo it should be easier to spot that there is something weird going on in the code. That was my rationale. Perhaps, the debugger could here even provide a new command to analyze thos= e colliding symbols. /Fred > 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, th= en > 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. >=20 > Simon Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928