This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]