RE: [PATCH V4] symlookup: improves symbol lookup when a file is specified.

["Tedeschi, Walfred" <walfred.tedeschi@intel.com>]

> -----Original Message-----
> From: Simon Marchi [mailto:simon.marchi@polymtl.ca]
> Sent: Thursday, October 19, 2017 9:07 PM
> To: Tedeschi, Walfred <walfred.tedeschi@intel.com>
> Cc: palves@redhat.com; gdb-patches@sourceware.org
> Subject: Re: [PATCH V4] symlookup: improves symbol lookup when a file is
> specified.
> 
> 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  <walfred.tedeschi@intel.com>
> >
> > 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
> > <http://www.gnu.org/licenses/>.  */
> > +
> > +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
> > <http://www.gnu.org/licenses/>.  */
> > +
> > +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
> > <http://www.gnu.org/licenses/>.  */
> > +
> > +#include <dlfcn.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +
> > +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
> > <http://www.gnu.org/licenses/>.  */
> > +
> 
> 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"
> 
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. 
How I see it: Very few sane people will add a symbols in a shared library that
will collide like the case we presented here.  If one does so how can the debugger
help?

Providing the same value as the runtime or linker does? 
This one user already knows. 
Or providing what the debug information provides as value created by the library itself.
In final end both are right. :|

But when specifying the scope if user is provided the value of the debug info 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 those 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, 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
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