From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128910 invoked by alias); 15 Oct 2019 14:15: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 128861 invoked by uid 89); 15 Oct 2019 14:15:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,KAM_STOCKGEN,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=Changes X-HELO: mail-wm1-f42.google.com Received: from mail-wm1-f42.google.com (HELO mail-wm1-f42.google.com) (209.85.128.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 15 Oct 2019 14:15:20 +0000 Received: by mail-wm1-f42.google.com with SMTP id v17so20447306wml.4 for ; Tue, 15 Oct 2019 07:15:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Ls+x5wvSmlyCiCG6X8X46N0RD4WOEZ+4HKpoLRzHFzs=; b=GrEYCLnUar6sj1aDKZ3bwJSWZn+CP20QUqT6nUSZ7/U8zgGGQKr92jkQVgUrcxsZ9m zjlZzjm8WuEdUzZ21Hw/zAu4bypd0JdUT2Nstb2gfGjntus1pOSvRLz7K4SzmvvyXZgH bY4NQbU5AJ0xmDdtbTsiAQXe28D98I5QezRYJ6+8hI0YLdQXbT9Bd2//fnAPKTSaFI7d 64Z6CHLT1Z17qeT7zUoctnVwaIUPNYNvQpJI4KTghzLfTKfO/V6U1XUReSEfhZDk5Lah tq3+Wu6UFa5s5+L4j54lXkmYsOMMiv/Wi/sPrF4CwZ6ExHnS5MNMHZlojKWfBLUzjD5r FHyQ== Return-Path: Received: from localhost (host86-128-12-122.range86-128.btcentralplus.com. [86.128.12.122]) by smtp.gmail.com with ESMTPSA id z9sm21613273wrl.35.2019.10.15.07.15.16 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 15 Oct 2019 07:15:16 -0700 (PDT) Date: Tue, 15 Oct 2019 14:15:00 -0000 From: Andrew Burgess To: Christian Biesinger Cc: gdb-patches Subject: Re: [PATCHv2] gdb/python: smarter symbol lookup for gdb.lookup_static_symbol Message-ID: <20191015141515.GW4962@embecosm.com> References: <83blvborrm.fsf@gnu.org> <20191008110737.29788-1-andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Fortune: If you put it off long enough, it might go away. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-10/txt/msg00420.txt.bz2 * 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? Thanks, Andrew > > > 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 > >