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

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

Any feedback?

-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Tedeschi, Walfred
Sent: Wednesday, September 20, 2017 4:20 PM
To: palves@redhat.com; qiyaoltc@gmail.com
Cc: gdb-patches@sourceware.org
Subject: [ping][PATCH] symlookup: improves symbol lookup when a file is specified.

Any feedback?


On 09/06/2017 10:46 AM, Walfred Tedeschi wrote:
> The provided patch adds a global lookup with higher priority for the 
> provided file.
>
> Usual symbol lookup from GDB follows linker logic.  This is what is 
> desired most of the time.  In the usage case a file is provided as 
> scope, so the lookup has to follow this priority. Failing in finding 
> the symbol at this scope usual path is followed.
>
> As 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 accessing them via the file scope returns the value 
> seen in the first loaded shared object. Using the patch the value 
> defined in the file scope is the one returned.
>
> One of the already existing test cases in print-file-var.exp starts to 
> fail after using 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 there also in both libraries the symbol this_version_id.
> During load of the libraries linker resolves the symbol as the first 
> one loaded and independent of the scope symbol will have the same 
> value, as defined in the first library loaded.
> However, when defining the scope the value should represent the value 
> in that context. Diferent evaluations of the same symbols might also 
> better spot the issue in users code.
>
> - I haven't changed the test because I wanted to hear the community 
> thought on the subject.
>
>
>
> 2017-07-13  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/symtab.c                                       |   4 +
>   .../gdb.base/print-file-var-dlopen-main.c          |  62 +++++++++++++
>   gdb/testsuite/gdb.base/print-file-var-dlopen.exp   | 101 +++++++++++++++++++++
>   3 files changed, 167 insertions(+)
>   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 8492315..920461f 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -2590,6 +2590,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-main.c 
> b/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
> new file mode 100644
> index 0000000..98cfd97
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
> @@ -0,0 +1,62 @@
> +/* 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;
> +
> +  dummy ();
> +  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();
> +    }
> +
> +  dummy (); /* STOP  */
> +  dlclose (lib1);
> +  dlclose (lib2);
> +  if (v1 != 104)
> +    return 1;
> +  /* The value returned by get_version_2 depends on the target 
> + system.  */  if (v2 != 104 || v2 != 203)
> +    return 2;
> +
> +  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..87f89f2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/print-file-var-dlopen.exp
> @@ -0,0 +1,101 @@
> +# 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/>.  
> +*/
> +
> +if {[skip_shlib_tests]} {
> +    return -1
> +}
> +
> +set executable print-file-var-dlopen-main
> +
> +set lib1 "print-file-var-lib1"
> +set lib2 "print-file-var-lib2"
> +
> +set libobj1 [standard_output_file ${lib1}.so] set libobj2 
> +[standard_output_file ${lib2}.so]
> +
> +set lib_opts { debug additional_flags=-fPIC additional_flags=-shared}
> +
> +if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib1}.c \
> +                        ${libobj1} \
> +                        ${lib_opts} ] != "" } {
> +    return -1
> +}
> +if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib2}.c \
> +                        ${libobj2} \
> +                        ${lib_opts} ] != "" } {
> +    return -1
> +}
> +if { [gdb_compile "${srcdir}/${subdir}/${executable}.c" \
> +                  [standard_output_file ${executable}] \
> +                  executable \
> +                  [list debug shlib=-ldl]]
> +     != ""} {
> +    return -1
> +}
> +
> +clean_restart $executable
> +
> +set outdir [file dirname $libobj1]
> +
> +gdb_test_no_output "set env LD_LIBRARY_PATH=${outdir}/:"
> +
> +if ![runto_main] {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +# Try printing "this_version_num" qualified with the name of the file 
> +# where the variables are defined.  There are two global variables # 
> +with that name, and some systems such as GNU/Linux merge them # into 
> +one single entity, while some other systems such as Windows # keep 
> +them separate.  In the first situation, we have to verify # that GDB 
> +does not randomly select the wrong instance, even when # a specific 
> +filename is used to qualified the lookup.  And in the # second case, 
> +we have to verify that GDB does select the instance # defined in the 
> +given filename.
> +#
> +# 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_test "continue" \
> +         "Breakpoint \[0-9\]+, main \\(\\) at.*" \
> +         "continue to STOP marker"
> +
> +# Now check the value of this_version_id in both 
> +print-file-var-lib1.c # and print-file-var-lib2.c.
> +
> +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" \
> +         " = 100"
> +

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

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