From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101396 invoked by alias); 16 Oct 2019 21:53:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 101388 invoked by uid 89); 16 Oct 2019 21:53:40 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-29.4 required=5.0 tests=AWL,BAYES_00,ENV_AND_HDR_SPF_MATCH,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,KAM_STOCKGEN,RCVD_IN_DNSWL_NONE,SPF_PASS,USER_IN_DEF_SPF_WL autolearn=ham version=3.3.1 spammy=role, HX-HELO:sk:mail-ot X-HELO: mail-ot1-f51.google.com Received: from mail-ot1-f51.google.com (HELO mail-ot1-f51.google.com) (209.85.210.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 16 Oct 2019 21:53:37 +0000 Received: by mail-ot1-f51.google.com with SMTP id m19so111055otp.1 for ; Wed, 16 Oct 2019 14:53:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2ylzbTaTcx0jjfef8pojII6jiIsSiwYjT9jMv5NaT4s=; b=lNHzZF2YKYdSkwkAESkUmbgnQXDRSExEhqtQRa3G//a99ELwa/3LlYkVCStqdNEVnP Nc+OFwqORJP7Tr6yyXSxRCCco9mL5mnISO6M6f/gCv/N06YOxL4XE3PlmxWmH9/JXPmt xQBBxIg6x0az75h8MmFZVybK8qOGyrBEBIOXVBiSaGyBWqgdQUNlMrCJN84sRmdA7yGA 7RmemKPXmRWFuRTzmWJIF4KfEpV9AI7sAO+gHNQKGej2wFIVZ29/uK9A7MDgNwC3kl6L mKwzJ6CYF3Q+WG1qkk/hqCsKBkEecgpLN6gmdPTYmY4OxHoa1aKDSmgxsrv6QyKGdy+E c6bA== MIME-Version: 1.0 References: <83blvborrm.fsf@gnu.org> <20191008110737.29788-1-andrew.burgess@embecosm.com> <20191015141515.GW4962@embecosm.com> In-Reply-To: <20191015141515.GW4962@embecosm.com> From: "Christian Biesinger via gdb-patches" Reply-To: Christian Biesinger Date: Wed, 16 Oct 2019 21:53:00 -0000 Message-ID: Subject: Re: [PATCHv2] gdb/python: smarter symbol lookup for gdb.lookup_static_symbol To: Andrew Burgess Cc: gdb-patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-10/txt/msg00519.txt.bz2 On Tue, Oct 15, 2019 at 4:15 PM Andrew Burgess wrote: > > * Christian Biesinger [2019-10-08 11:00:56 -0500]: > > > On Tue, Oct 8, 2019 at 6:07 AM Andrew Burgess > > 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 . */ > > > + > > > +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 > > >