This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH v2] symlookup: improves symbol lookup when a file is specified.
- From: "Tedeschi, Walfred" <walfred dot tedeschi at intel dot com>
- To: Simon Marchi <simon dot marchi at ericsson dot com>, "palves at redhat dot com" <palves at redhat dot com>, "simon dot marchi at polymtl dot ca" <simon dot marchi at polymtl dot ca>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Tue, 17 Oct 2017 07:57:03 +0000
- Subject: RE: [PATCH v2] symlookup: improves symbol lookup when a file is specified.
- Authentication-results: sourceware.org; auth=none
- Dlp-product: dlpe-windows
- Dlp-reaction: no-action
- Dlp-version: 11.0.0.116
- References: <1507727728-30540-1-git-send-email-walfred.tedeschi@intel.com> <88efd84d-5449-f2d3-3757-c16c6f201506@ericsson.com>
Simon,
Thanks for your review!
Some comments below...
> From: Simon Marchi [mailto:simon.marchi@ericsson.com]
> Sent: Monday, October 16, 2017 7:06 PM
> To: Tedeschi, Walfred <walfred.tedeschi@intel.com>; palves@redhat.com;
> simon.marchi@polymtl.ca
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2] symlookup: improves symbol lookup when a file is
> specified.
>
> On 2017-10-11 09:15 AM, Walfred Tedeschi wrote:
> > The provided patch adds a scope lookup with higher priority in the
> > case a a file is provided for the evaluation.
Looks like we have also here a double "a"
> >
> > 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 provided, so the lookup has to follow this
> > priority. Failing in
>
> "provided provided"
Ok!
>
> > 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.
> >
> > 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
> >
> > During load of the libraries linker resolves the symbol as the first
> > one loaded and independent of the scope the 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 can't really comment on how right the fix is, but I can see that the current
> behavior is clearly wrong:
>
I will add that in the commit message, I think it might make it clearer.
> (gdb) p 'print-file-var-dlopen-lib1.c'::this_version_id
> $1 = 104
> (gdb) p 'print-file-var-dlopen-lib2.c'::this_version_id
> $2 = 104
>
> And that your patch fixes it:
>
> (gdb) p 'print-file-var-dlopen-lib1.c'::this_version_id
> $1 = 104
> (gdb) p 'print-file-var-dlopen-lib2.c'::this_version_id
> $2 = 203
>
> > 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.base/print-file-var-dlopen-lib1.c: New file.
> > * gdb.base/print-file-var-dlopen-lib2.c: New file.
> >
> >
> > ---
> > 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 | 113
> +++++++++++++++++++++
> > 5 files changed, 228 insertions(+)
> > 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.
>
> Are the years right here (the 2012)?
I have improved a test case from 2012 to do this one.
>
> > +
> > + 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..1c331ba
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/print-file-var-dlopen.exp
> > @@ -0,0 +1,113 @@
> > +# 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 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
> > +{}]
> > +
> > +set lib_opts debug
> > +set exec_opts [list debug shlib_load
> > +additional_flags=-DSHLIB_NAME=\"${lib_dlopen1}\"
> > +additional_flags=-DSHLIB_NAME2=\"${lib_dlopen2}\"]
> > +
> > +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
> > +}
> > +
> > +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
> > +
> > +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.
>
> I don't really understand what you mean by "GNU/Linux merge them into
> one single entity". There are still two distinct variables, no?
This comment comes from the other test, we do not need it anymore.
>
> > +gdb_test "continue" \
> > + "Breakpoint \[0-9\]+, main \\(\\) at.*" \
> > + "continue to STOP marker"
>
> You could use gdb_continue_to_breakpoint.
Thank you!
>
> 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