Re: [PATCH] Add an Objfile.lookup_global_symbol function

["Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>]

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