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

[Simon Marchi <simon.marchi@polymtl.ca>]

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"

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