Re: [PATCHv2] gdb/python: smarter symbol lookup for gdb.lookup_static_symbol

["Christian Biesinger via gdb-patches" <gdb-patches at sourceware dot org>]

On Tue, Oct 15, 2019 at 4:15 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
>
> * Christian Biesinger <cbiesinger@google.com> [2019-10-08 11:00:56 -0500]:
>
> > On Tue, Oct 8, 2019 at 6:07 AM Andrew Burgess
> > <andrew.burgess@embecosm.com> wrote:
> > >
> > > Changes since v1:
> > >
> > >   - Minor tweak to docs to address Eli's feedback,
> > >   - Rebase to current HEAD,
> > >   - Fixed a small bug in the test script that caused some failures in
> > >     C++ part of the test - no idea how I missed this originally.
> > >
> > > Thanks,
> > > Andrew
> > >
> > > ---
> > >
> > > When using gdb.lookup_static_symbol I think that GDB should find
> > > static symbols (global symbol with static linkage) from the current
> > > object file ahead of static symbols from other object files.
> >
> > So I am not sure how I feel about this. Since this is a programmatic
> > API, I'm not sure it should be affected by the current program status
> > like this. Maybe this should be optional behavior, or maybe it should
> > be possible to pass in a block? (At the same time, it is already
> > possible to find static variables through the block, if you have one)
>
> Given that gdb.lookup_static_symbol returns a single symbol, I'm
> struggling to see how having it return something other than the one
> GDB would "naturally" see (if the user took control and just typed 'p
> some_variable') would ever be useful.
>
> I think we should either change the API to return the same thing the
> CLI would see, or have the API return a list of symbols.  Simply
> picking the "first" seems pretty arbitrary, and is (I would guess)
> going to be wrong more often than not.
>
> I don't really understand why this being a "programmatic API" makes
> any difference - the CLI is programmable (though admittedly it's much
> inferior to python scripting) - they both serve the same purpose,
> allow the user to send commands to GDB and see the program state.  The
> question I think is more, does it make sense to have the symbol return
> change based on the current program state.   Given the role of GDB I'd
> say yes.
>
> How would you feel if I changed the patch to have
> 'gdb.lookup_static_symbol' return the one symbol that is the current
> value, and 'gdb.lookup_all_static_symbols' return a list of all
> matching static symbols?

Fair enough -- you convinced me. I like this plan.

Christian

> > > This means that if we have two source files f1.c and f2.c, and both
> > > files contains 'static int foo;', then when we are stopped in f1.c a
> > > call to 'gdb.lookup_static_symbol ("foo")' will find f1.c::foo, and if
> > > we are stopped in f2.c we would find 'f2.c::foo'.
> > >
> > > Given that gdb.lookup_static_symbol always returns a single symbol,
> > > but there can be multiple static symbols with the same name GDB is
> > > always making a choice about which symbols to return.  I think that it
> > > makes sense for the choice GDB makes in this case to match what a user
> > > would get on the command line if they asked to 'print foo'.
> > >
> > > gdb/testsuite/ChangeLog:
> > >
> > >         * gdb.python/py-symbol.c: Declare and call function from new
> > >         py-symbol-2.c file.
> > >         * gdb.python/py-symbol.exp: Compile both source files, and add new
> > >         tests for gdb.lookup_static_symbol.
> > >         * gdb.python/py-symbol-2.c: New file.
> > >
> > > gdb/doc/ChangeLog:
> > >
> > >         * python.texi (Symbols In Python): Extend documentation for
> > >         gdb.lookup_static_symbol.
> > >
> > > gdb/ChangeLog:
> > >
> > >         * python/py-symbol.c (gdbpy_lookup_static_symbol): Lookup in
> > >         static block of current object file first.
> > > ---
> > >  gdb/ChangeLog                          |  5 +++++
> > >  gdb/doc/ChangeLog                      |  5 +++++
> > >  gdb/doc/python.texi                    |  9 +++++++++
> > >  gdb/python/py-symbol.c                 | 25 +++++++++++++++++++++++-
> > >  gdb/testsuite/ChangeLog                |  8 ++++++++
> > >  gdb/testsuite/gdb.python/py-symbol-2.c | 26 +++++++++++++++++++++++++
> > >  gdb/testsuite/gdb.python/py-symbol.c   |  9 +++++++++
> > >  gdb/testsuite/gdb.python/py-symbol.exp | 35 ++++++++++++++++++++++------------
> > >  8 files changed, 109 insertions(+), 13 deletions(-)
> > >  create mode 100644 gdb/testsuite/gdb.python/py-symbol-2.c
> > >
> > > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> > > index 48adad4e97b..0f12de94bba 100644
> > > --- a/gdb/doc/python.texi
> > > +++ b/gdb/doc/python.texi
> > > @@ -4869,6 +4869,15 @@
> > >  up such variables, iterate over the variables of the function's
> > >  @code{gdb.Block} and check that @code{block.addr_class} is
> > >  @code{gdb.SYMBOL_LOC_STATIC}.
> > > +
> > > +There can be multiple global symbols with static linkage with the same
> > > +name.  This function will only return the first matching symbol that
> > > +it finds.  Which symbol is found depends on where @value{GDBN} is
> > > +currently stopped, as @value{GDBN} will first search for matching
> > > +symbols in the current object file, and then search all other object
> > > +files.  If the application is not yet running then @value{GDBN} will
> > > +search all object files in the order they appear in the debug
> > > +information.
> > >  @end defun
> > >
> > >  A @code{gdb.Symbol} object has the following attributes:
> > > diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
> > > index 2b10e21d872..ae9aca6c5d0 100644
> > > --- a/gdb/python/py-symbol.c
> > > +++ b/gdb/python/py-symbol.c
> > > @@ -487,9 +487,32 @@ gdbpy_lookup_static_symbol (PyObject *self, PyObject *args, PyObject *kw)
> > >                                         &domain))
> > >      return NULL;
> > >
> > > +  /* In order to find static symbols associated with the "current" object
> > > +     file ahead of those from other object files, we first need to see if
> > > +     we can acquire a current block.  If this fails however, then we still
> > > +     want to search all static symbols, so don't throw an exception just
> > > +     yet.  */
> > > +  const struct block *block = NULL;
> > >    try
> > >      {
> > > -      symbol = lookup_static_symbol (name, (domain_enum) domain).symbol;
> > > +      struct frame_info *selected_frame
> > > +       = get_selected_frame (_("No frame selected."));
> > > +      block = get_frame_block (selected_frame, NULL);
> > > +    }
> > > +  catch (const gdb_exception &except)
> > > +    {
> > > +      /* Nothing.  */
> > > +    }
> > > +
> > > +  try
> > > +    {
> > > +      if (block != nullptr)
> > > +       symbol
> > > +         = lookup_symbol_in_static_block (name, block,
> > > +                                          (domain_enum) domain).symbol;
> > > +
> > > +      if (symbol == nullptr)
> > > +       symbol = lookup_static_symbol (name, (domain_enum) domain).symbol;
> > >      }
> > >    catch (const gdb_exception &except)
> > >      {
> > > diff --git a/gdb/testsuite/gdb.python/py-symbol-2.c b/gdb/testsuite/gdb.python/py-symbol-2.c
> > > new file mode 100644
> > > index 00000000000..12872efe4b7
> > > --- /dev/null
> > > +++ b/gdb/testsuite/gdb.python/py-symbol-2.c
> > > @@ -0,0 +1,26 @@
> > > +/* This testcase is part of GDB, the GNU debugger.
> > > +
> > > +   Copyright 2010-2019 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/>.  */
> > > +
> > > +static int rr = 99;            /* line of other rr */
> > > +
> > > +void
> > > +function_in_other_file (void)
> > > +{
> > > +  /* Nothing.  */
> > > +}
> > > +
> > > +
> > > diff --git a/gdb/testsuite/gdb.python/py-symbol.c b/gdb/testsuite/gdb.python/py-symbol.c
> > > index 06a931bf5d9..51325dcd450 100644
> > > --- a/gdb/testsuite/gdb.python/py-symbol.c
> > > +++ b/gdb/testsuite/gdb.python/py-symbol.c
> > > @@ -38,6 +38,10 @@ namespace {
> > >  };
> > >  #endif
> > >
> > > +#ifdef USE_TWO_FILES
> > > +extern void function_in_other_file (void);
> > > +#endif
> > > +
> > >  int qq = 72;                   /* line of qq */
> > >  static int rr = 42;            /* line of rr */
> > >
> > > @@ -70,5 +74,10 @@ int main (int argc, char *argv[])
> > >    sclass.seti (42);
> > >    sclass.valueofi ();
> > >  #endif
> > > +
> > > +#ifdef USE_TWO_FILES
> > > +  function_in_other_file ();
> > > +#endif
> > > +
> > >    return 0; /* Break at end.  */
> > >  }
> > > diff --git a/gdb/testsuite/gdb.python/py-symbol.exp b/gdb/testsuite/gdb.python/py-symbol.exp
> > > index 5617f127e5b..61960075565 100644
> > > --- a/gdb/testsuite/gdb.python/py-symbol.exp
> > > +++ b/gdb/testsuite/gdb.python/py-symbol.exp
> > > @@ -18,9 +18,11 @@
> > >
> > >  load_lib gdb-python.exp
> > >
> > > -standard_testfile
> > > +standard_testfile py-symbol.c py-symbol-2.c
> > >
> > > -if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
> > > +set opts { debug additional_flags=-DUSE_TWO_FILES }
> > > +if {[prepare_for_testing "failed to prepare" $testfile \
> > > +        [list $srcfile $srcfile2] $opts]} {
> > >      return -1
> > >  }
> > >
> > > @@ -48,6 +50,7 @@ gdb_test "python print (gdb.lookup_global_symbol('qq').needs_frame)" \
> > >      "False" \
> > >      "print whether qq needs a frame"
> > >
> > > +# Similarly, test looking up a static symbol before we runto_main.
> > >  set rr_line [gdb_get_line_number "line of rr"]
> > >  gdb_test "python print (gdb.lookup_global_symbol ('rr') is None)" "True" \
> > >      "lookup_global_symbol for static var"
> > > @@ -100,10 +103,23 @@ gdb_test "python print (func.print_name)" "func" "test func.print_name"
> > >  gdb_test "python print (func.linkage_name)" "func" "test func.linkage_name"
> > >  gdb_test "python print (func.addr_class == gdb.SYMBOL_LOC_BLOCK)" "True" "test func.addr_class"
> > >
> > > -gdb_breakpoint [gdb_get_line_number "Break at end."]
> > > +# Stop in a second file and ensure we find its local static symbol.
> > > +gdb_breakpoint "function_in_other_file"
> > > +gdb_continue_to_breakpoint "function_in_other_file"
> > > +gdb_test "python print (gdb.lookup_static_symbol ('rr').value ())" "99" \
> > > +    "print value of rr from other file"
> > > +
> > > +# Now continue back to the first source file.
> > > +set linenum [gdb_get_line_number "Break at end."]
> > > +gdb_breakpoint "$srcfile:$linenum"
> > >  gdb_continue_to_breakpoint "Break at end for variable a" ".*Break at end.*"
> > >  gdb_py_test_silent_cmd "python frame = gdb.selected_frame()" "Get Frame" 0
> > >
> > > +# Check that we find the static sybol local to this file over the
> > > +# static symbol from the second source file.
> > > +gdb_test "python print (gdb.lookup_static_symbol ('rr').value ())" "42" \
> > > +    "print value of rr from main file"
> > > +
> > >  # Test is_variable attribute.
> > >  gdb_py_test_silent_cmd "python a = gdb.lookup_symbol(\'a\')" "Get variable a" 0
> > >  gdb_test "python print (a\[0\].is_variable)" "True" "test a.is_variable"
> > > @@ -145,17 +161,12 @@ gdb_test "python print (t\[0\].symtab)" "${py_symbol_c}" "get symtab"
> > >
> > >  # C++ tests
> > >  # Recompile binary.
> > > -if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}-cxx" executable "debug c++"] != "" } {
> > > -    untested "failed to compile in C++ mode"
> > > +lappend opts c++
> > > +if {[prepare_for_testing "failed to prepare" "${binfile}-cxx" \
> > > +        [list $srcfile $srcfile2] $opts]} {
> > >      return -1
> > >  }
> > >
> > > -# Start with a fresh gdb.
> > > -gdb_exit
> > > -gdb_start
> > > -gdb_reinitialize_dir $srcdir/$subdir
> > > -gdb_load ${binfile}-cxx
> > > -
> > >  gdb_test "python print (gdb.lookup_global_symbol ('(anonymous namespace)::anon') is None)" \
> > >      "True" "anon is None"
> > >  gdb_test "python print (gdb.lookup_static_symbol ('(anonymous namespace)::anon').value ())" \
> > > @@ -189,7 +200,7 @@ gdb_test "python print (cplusfunc.addr_class == gdb.SYMBOL_LOC_BLOCK)" "True" "t
> > >  # Test is_valid when the objfile is unloaded.  This must be the last
> > >  # test as it unloads the object file in GDB.
> > >  # Start with a fresh gdb.
> > > -clean_restart ${testfile}
> > > +clean_restart ${binfile}
> > >  if ![runto_main] then {
> > >      fail "cannot run to main."
> > >      return 0
> > > --
> > > 2.14.5
> > >