Re: [PATCH] Add an Objfile.lookup_global_symbol function

Christian Biesinger via gdb-patches
2019-07-26T01:38:29Z

On Mon, Jul 22, 2019 at 6:30 PM Simon Marchi <simark@simark.ca> wrote:
> What did you end up deciding, to let Python code search for static symbols?

I decided to follow your suggestion and add a lookup_static_symbol
function (to Objfile and gdb). I haven't had time to implement it yet
but hopefully I can do that in the next couple of days. Would you like
me to do Objfile.lookup_static_symbol as part of this patch or
together with gdb.lookup_static_symbol?

> I just have a few nits on the patch below (and some questions about the doc,
> which is why I added Eli in CC).

Will send a new patch momentarily.

> On 2019-07-22 4:51 p.m., Christian Biesinger via gdb-patches wrote:
> > Per the discussion in the thread about my gdb.lookup_global_symbol
> > patch, I've changed this patch to only look in GLOBAL_SCOPE.
> >
> > This is essentially the inverse of Symbol.objfile. This allows
> > handling different symbols with the same name (but from different
> > objfiles) and can also be faster if the objfile is known.
> >
> > gdb/ChangeLog:
> >
> > 2019-06-25 Christian Biesinger <cbiesinger@google.com>
> >
> > * NEWS: Mention new function Objfile.lookup_global_symbol.
> > * python/py-objfile.c (objfpy_lookup_global_symbol): New function
> > Objfile.lookup_global_symbol.
> >
> > gdb/doc/ChangeLog:
> >
> > 2019-06-25 Christian Biesinger <cbiesinger@google.com>
> >
> > * python.texi (Objfiles In Python): Document new function
> > Objfile.lookup_global_symbol.
> >
> > gdb/testsuite/ChangeLog:
> >
> > 2019-06-25 Christian Biesinger <cbiesinger@google.com>
> >
> > * gdb.python/py-objfile.c: Add global and static vars.
> > * gdb.python/py-objfile.exp: Test new function Objfile.
> > lookup_global_symbol.
> > ---
> > gdb/NEWS | 3 ++
> > gdb/doc/python.texi | 12 ++++++++
> > gdb/python/py-objfile.c | 40 ++++++++++++++++++++++++-
> > gdb/testsuite/gdb.python/py-objfile.c | 3 ++
> > gdb/testsuite/gdb.python/py-objfile.exp | 7 +++++
> > 5 files changed, 64 insertions(+), 1 deletion(-)
> >
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index cc1d58520d..c57cb4576e 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -36,6 +36,9 @@
> > ** gdb.Type has a new property 'objfile' which returns the objfile the
> > type was defined in.
> >
> > + ** gdb.Objfile has a new method 'lookup_global_symbol' to lookup a symbol
> > + from this objfile only.
> > +
> > * New commands
> >
> > | [COMMAND] | SHELL_COMMAND
> > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> > index 034623513b..80da607824 100644
> > --- a/gdb/doc/python.texi
> > +++ b/gdb/doc/python.texi
> > @@ -4441,6 +4441,18 @@ searches then this function can be used to add a debug info file
> > from a different place.
> > @end defun
> >
> > +@defun Objfile.lookup_global_symbol (name @r{[}, domain@r{]})
> > +Searches for a global symbol named @var{name} in this objfile. Optionally, the
> > +search scope can be restricted with the @var{domain} argument.
> > +The @var{domain} argument must be a domain constant defined in the @code{gdb}
> > +module and described in @ref{Symbols In Python}. This function is similar to
> > +@code{gdb.lookup_global_symbol}, except that the search is limited to this
> > +objfile.
> > +
> > +The result is a @code{gdb.Symbol} object or @code{None} if the symbol
> > +is not found.
> > +@end defun
> > +
> > @node Frames In Python
> > @subsubsection Accessing inferior stack frames from Python
> >
> > diff --git a/gdb/python/py-objfile.c b/gdb/python/py-objfile.c
> > index 199c567a04..a9a6ae6725 100644
> > --- a/gdb/python/py-objfile.c
> > +++ b/gdb/python/py-objfile.c
> > @@ -406,7 +406,7 @@ objfpy_is_valid (PyObject *self, PyObject *args)
> > Py_RETURN_TRUE;
> > }
> >
> > -/* Implementation of gdb.Objfile.add_separate_debug_file (self) -> Boolean. */
> > +/* Implementation of gdb.Objfile.add_separate_debug_file (self, string) -> None. */
>
> This appears to be an unrelated (but valid) change. Can you please make a separate patch
> and push it as obvious? Just mind the 80 columns. If the function returns None, I think
> we could just omit the return type and say:
>
> /* Implementation of gdb.Objfile.add_separate_debug_file (self, string). */
>
> ... and it will fit.
>
> So just push a patch with this (including the ChangeLog entry), and post it to the list
> with the "PATCH obvious" prefix, or something like that, to let people know.

OK, done. (Oops, I see you already answered my question here that I
asked you again on IRC)

> > static PyObject *
> > objfpy_add_separate_debug_file (PyObject *self, PyObject *args, PyObject *kw)
> > @@ -434,6 +434,39 @@ objfpy_add_separate_debug_file (PyObject *self, PyObject *args, PyObject *kw)
> > Py_RETURN_NONE;
> > }
> >
> > +/* Implementation of gdb.Objfile.lookup_global_symbol (self, string [, domain]) -> gdb.Symbol. */
>
> > +
> > +static PyObject *
> > +objfpy_lookup_global_symbol (PyObject *self, PyObject *args, PyObject *kw)
> > +{
> > + static const char *keywords[] = { "name", "domain", NULL };
> > + objfile_object *obj = (objfile_object *) self;
> > + const char *symbol_name;
> > + int domain = VAR_DOMAIN;
> > +
> > + OBJFPY_REQUIRE_VALID (obj);
> > +
> > + if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|i", keywords, &symbol_name,
> > + &domain))
> > + return nullptr;
> > +
> > + try
> > + {
> > + struct symbol *sym = lookup_global_symbol_from_objfile
> > + (obj->objfile, symbol_name, (domain_enum) domain).symbol;
> > + if (sym == nullptr)
> > + Py_RETURN_NONE;
> > +
> > + return symbol_to_symbol_object (sym);
> > + }
> > + catch (const gdb_exception &except)
> > + {
> > + GDB_PY_HANDLE_EXCEPTION (except);
> > + }
> > +
> > + Py_RETURN_NONE;
> > +}
> > +
> > /* Implement repr() for gdb.Objfile. */
> >
> > static PyObject *
> > @@ -652,6 +685,11 @@ Return true if this object file is valid, false if not." },
> > "add_separate_debug_file (file_name).\n\
> > Add FILE_NAME to the list of files containing debug info for the objfile." },
> >
> > + { "lookup_global_symbol", (PyCFunction) objfpy_lookup_global_symbol,
> > + METH_VARARGS | METH_KEYWORDS,
> > + "lookup_global_symbol (name [, domain]).\n\
> > +Looks up a global symbol in this objfile and returns it." },
> > +
>
> To be consistent with the rest of the Python function help strings, I think it should
> be
>
> "Look up a global symbol in this objfile and return it."
>
> i.e. not at the third person of the singular. Eli, could you tell which form is preferred?
> The change to python.texi might also need to be changed to that form as well.
>
> > { NULL }
> > };
> >
> > diff --git a/gdb/testsuite/gdb.python/py-objfile.c b/gdb/testsuite/gdb.python/py-objfile.c
> > index ac41491234..6d751bddae 100644
> > --- a/gdb/testsuite/gdb.python/py-objfile.c
> > +++ b/gdb/testsuite/gdb.python/py-objfile.c
> > @@ -15,6 +15,9 @@
> > 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 global_var = 42;
> > +static int static_var = 50;
> > +
> > int
> > main ()
> > {
> > diff --git a/gdb/testsuite/gdb.python/py-objfile.exp b/gdb/testsuite/gdb.python/py-objfile.exp
> > index b5e0c5e760..2e8358bd76 100644
> > --- a/gdb/testsuite/gdb.python/py-objfile.exp
> > +++ b/gdb/testsuite/gdb.python/py-objfile.exp
> > @@ -55,6 +55,13 @@ gdb_test "python print (gdb.lookup_objfile (\"${testfile}\").filename)" \
> > gdb_test "python print (gdb.lookup_objfile (\"junk\"))" \
> > "Objfile not found\\.\r\n${python_error_text}"
> >
> > +gdb_test "python print (gdb.lookup_objfile (\"${testfile}\").lookup_global_symbol (\"global_var\").name)" \
> > + "global_var" "lookup_global_symbol finds a valid symbol"
> > +gdb_test "python print gdb.lookup_objfile (\"${testfile}\").lookup_global_symbol (\"static_var\") is None" \
> > + "True" "lookup_global_symbol does not find static symbols"
>
> Please add parentheses to this print so it works with Python 3.
>
> > +gdb_test "python print (gdb.lookup_objfile (\"${testfile}\").lookup_global_symbol (\"stdout\"))" \
> > + "None" "lookup_global_symbol doesn't find symbol in other objfile"
> > +
> > set binfile_build_id [get_build_id $binfile]
> > if [string compare $binfile_build_id ""] {
> > verbose -log "binfile_build_id = $binfile_build_id"
> >
>
> Simon