symlookup: improves symbol lookup when a file is specified.

  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
  

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
  

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
  

On 09/06/2017 09: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.

I'm not sure I understand the description above, so I'd like to try this
locally to try to understand it better, but the test fails for me:

 (gdb) break print-file-var-dlopen-main.c:51
 Breakpoint 2 at 0x400751: file /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c, line 51.
 (gdb) PASS: gdb.base/print-file-var-dlopen.exp: breapoint past v1 & v2 initialization
 continue
 Continuing.
 [Inferior 1 (process 11534) exited with code 01]
 (gdb) FAIL: gdb.base/print-file-var-dlopen.exp: continue to STOP marker (the program exited)

The program exits because dlopen failed.  Is there a reason the test
is trying to use LD_LIBRARY_PATH instead of compiling the program
with the shared library file name defined as an absolute path via
additional_flags=-DXXXX like seemingly every other dlopen test does?

> +
> +  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;
> +}
> +

I tried to fix the above to get it running for me, but then noticed other
things clearly bad with the test.  This certainly can't have passed as
is for you.

For example, the test reuses these source files from the
print-file-var.exp test:

 +set lib1 "print-file-var-lib1"
 +set lib2 "print-file-var-lib2"

But then looks for "get_version" symbols in them, but there's
no such symbols...:

 +  *(int **) (&get_version1) = dlsym (lib1, "get_version");
 +  *(int **) (&get_version2) = dlsym (lib2, "get_version");

While here:

> +
> +gdb_test "print 'print-file-var-dlopen-lib1.c'::this_version_id == v1" \
> +         " = 1"

it looks like the test actually wanted to use some different
source files, but there's no print-file-var-dlopen-lib1.c included
in the patch.

Walfred, please double check what you intend to submit.

Pedro Alves
  

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Monday, October 9, 2017 2:42 PM
To: Tedeschi, Walfred <walfred.tedeschi@intel.com>; qiyaoltc@gmail.com
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] symlookup: improves symbol lookup when a file is specified.

On 09/06/2017 09: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.

I'm not sure I understand the description above, so I'd like to try this locally to try to understand it better, but the test fails for me:

 (gdb) break print-file-var-dlopen-main.c:51  Breakpoint 2 at 0x400751: file /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c, line 51.
 (gdb) PASS: gdb.base/print-file-var-dlopen.exp: breapoint past v1 & v2 initialization  continue  Continuing.
 [Inferior 1 (process 11534) exited with code 01]
 (gdb) FAIL: gdb.base/print-file-var-dlopen.exp: continue to STOP marker (the program exited)

The program exits because dlopen failed.  Is there a reason the test is trying to use LD_LIBRARY_PATH instead of compiling the program with the shared library file name defined as an absolute path via additional_flags=-DXXXX like seemingly every other dlopen test does?

> +
> +  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;
> +}
> +

I tried to fix the above to get it running for me, but then noticed other things clearly bad with the test.  This certainly can't have passed as is for you.

For example, the test reuses these source files from the print-file-var.exp test:

 +set lib1 "print-file-var-lib1"
 +set lib2 "print-file-var-lib2"

But then looks for "get_version" symbols in them, but there's no such symbols...:

 +  *(int **) (&get_version1) = dlsym (lib1, "get_version");  +  *(int **) (&get_version2) = dlsym (lib2, "get_version");

While here:

> +
> +gdb_test "print 'print-file-var-dlopen-lib1.c'::this_version_id == v1" \
> +         " = 1"

it looks like the test actually wanted to use some different source files, but there's no print-file-var-dlopen-lib1.c included in the patch.

Walfred, please double check what you intend to submit.

Pedro Alves


Pedro,

Thanks a lot for your review and sorry.

I should have started patching the test that was already existing in GDB and then moved to a new one.
I believe that a fixup was missed in the process. I will prepare the patch again!


Regards,
/Fred
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