[PR/24474] Make gdb.lookup_static_symbol also check the STATIC_SCOPE

  gdb/ChangeLog:

2019-06-04  Christian Biesinger  <cbiesinger@google.com>

	PR/24474: Make gdb.lookup_global_symbol also check the STATIC_SCOPE
	because from a C/C++ developer's perspective, these are also globals.
	* NEWS: Mention this change.
	* python/py-symbol.c (gdbpy_lookup_global_symbol): Call
	lookup_static_symbol if lookup_global_symbol returns NULL.

gdb/testsuite/ChangeLog:

2019-06-04  Christian Biesinger  <cbiesinger@google.com>

	* gdb.python/py-symbol.c: Add a static variable and one in an anonymous
	namespace.
	* gdb.python/py-symbol.exp: Check that lookup_global_symbol finds those
	variables.


---
 gdb/NEWS                               |  2 ++
 gdb/python/py-symbol.c                 | 11 +++++++++++
 gdb/testsuite/gdb.python/py-symbol.c   |  5 +++++
 gdb/testsuite/gdb.python/py-symbol.exp | 13 +++++++++++++
 4 files changed, 31 insertions(+)
  

ping?

On Tue, Jun 4, 2019 at 8:24 PM Christian Biesinger
<cbiesinger@google.com> wrote:
>
> gdb/ChangeLog:
>
> 2019-06-04  Christian Biesinger  <cbiesinger@google.com>
>
>         PR/24474: Make gdb.lookup_global_symbol also check the STATIC_SCOPE
>         because from a C/C++ developer's perspective, these are also globals.
>         * NEWS: Mention this change.
>         * python/py-symbol.c (gdbpy_lookup_global_symbol): Call
>         lookup_static_symbol if lookup_global_symbol returns NULL.
>
> gdb/testsuite/ChangeLog:
>
> 2019-06-04  Christian Biesinger  <cbiesinger@google.com>
>
>         * gdb.python/py-symbol.c: Add a static variable and one in an anonymous
>         namespace.
>         * gdb.python/py-symbol.exp: Check that lookup_global_symbol finds those
>         variables.
>
>
> ---
>  gdb/NEWS                               |  2 ++
>  gdb/python/py-symbol.c                 | 11 +++++++++++
>  gdb/testsuite/gdb.python/py-symbol.c   |  5 +++++
>  gdb/testsuite/gdb.python/py-symbol.exp | 13 +++++++++++++
>  4 files changed, 31 insertions(+)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index ded1fce406..bfd8d0a191 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -30,6 +30,8 @@
>    ** gdb.Type has a new property 'objfile' which returns the objfile the
>       type was defined in.
>
> +  ** gdb.lookup_global_symbol will now also find symbols with static linkage.
> +
>  * New built-in convenience variables $_shell_exitcode and $_shell_exitsignal
>    provide the exitcode or exit status of the shell commands launched by
>    GDB commands such as "shell", "pipe" and "make".
> diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
> index 8605ae71a2..5636ef9013 100644
> --- a/gdb/python/py-symbol.c
> +++ b/gdb/python/py-symbol.c
> @@ -456,6 +456,17 @@ gdbpy_lookup_global_symbol (PyObject *self, PyObject *args, PyObject *kw)
>        GDB_PY_HANDLE_EXCEPTION (except);
>      }
>
> +  if (symbol == NULL) {
> +    try
> +      {
> +        symbol = lookup_static_symbol (name, (domain_enum) domain).symbol;
> +      }
> +    catch (const gdb_exception &except)
> +      {
> +        GDB_PY_HANDLE_EXCEPTION (except);
> +      }
> +  }
> +
>    if (symbol)
>      {
>        sym_obj = symbol_to_symbol_object (symbol);
> diff --git a/gdb/testsuite/gdb.python/py-symbol.c b/gdb/testsuite/gdb.python/py-symbol.c
> index f77c8c8585..06a931bf5d 100644
> --- a/gdb/testsuite/gdb.python/py-symbol.c
> +++ b/gdb/testsuite/gdb.python/py-symbol.c
> @@ -32,9 +32,14 @@ class SimpleClass
>      return i; /* Break in class. */
>    }
>  };
> +
> +namespace {
> +  int anon = 10;
> +};
>  #endif
>
>  int qq = 72;                   /* line of qq */
> +static int rr = 42;            /* line of rr */
>
>  int func (int arg)
>  {
> diff --git a/gdb/testsuite/gdb.python/py-symbol.exp b/gdb/testsuite/gdb.python/py-symbol.exp
> index 5b8a2be7c4..722fcda0f0 100644
> --- a/gdb/testsuite/gdb.python/py-symbol.exp
> +++ b/gdb/testsuite/gdb.python/py-symbol.exp
> @@ -48,6 +48,16 @@ gdb_test "python print (gdb.lookup_global_symbol('qq').needs_frame)" \
>      "False" \
>      "print whether qq needs a frame"
>
> +set rr_line [gdb_get_line_number "line of rr"]
> +gdb_test "python print (gdb.lookup_global_symbol ('rr').line)" "$rr_line" \
> +    "print line number of rr"
> +
> +gdb_test "python print (gdb.lookup_global_symbol ('rr').value ())" "42" \
> +    "print value of rr"
> +
> +gdb_test "python print (gdb.lookup_global_symbol ('rr').needs_frame)" \
> +    "False" \
> +    "print whether rr needs a frame"
>
>  if ![runto_main] then {
>      fail "can't run to main"
> @@ -137,6 +147,9 @@ gdb_start
>  gdb_reinitialize_dir $srcdir/$subdir
>  gdb_load ${binfile}-cxx
>
> +gdb_test "python print (gdb.lookup_global_symbol ('(anonymous namespace)::anon').value ())" "10" \
> +    "print value of rr"
> +
>  if ![runto_main] then {
>      fail "can't run to main"
>      return 0
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
  

ping?

On Tue, Jun 4, 2019 at 8:24 PM Christian Biesinger
<cbiesinger@google.com> wrote:
>
> gdb/ChangeLog:
>
> 2019-06-04  Christian Biesinger  <cbiesinger@google.com>
>
>         PR/24474: Make gdb.lookup_global_symbol also check the STATIC_SCOPE
>         because from a C/C++ developer's perspective, these are also globals.
>         * NEWS: Mention this change.
>         * python/py-symbol.c (gdbpy_lookup_global_symbol): Call
>         lookup_static_symbol if lookup_global_symbol returns NULL.
>
> gdb/testsuite/ChangeLog:
>
> 2019-06-04  Christian Biesinger  <cbiesinger@google.com>
>
>         * gdb.python/py-symbol.c: Add a static variable and one in an anonymous
>         namespace.
>         * gdb.python/py-symbol.exp: Check that lookup_global_symbol finds those
>         variables.
>
>
> ---
>  gdb/NEWS                               |  2 ++
>  gdb/python/py-symbol.c                 | 11 +++++++++++
>  gdb/testsuite/gdb.python/py-symbol.c   |  5 +++++
>  gdb/testsuite/gdb.python/py-symbol.exp | 13 +++++++++++++
>  4 files changed, 31 insertions(+)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index ded1fce406..bfd8d0a191 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -30,6 +30,8 @@
>    ** gdb.Type has a new property 'objfile' which returns the objfile the
>       type was defined in.
>
> +  ** gdb.lookup_global_symbol will now also find symbols with static linkage.
> +
>  * New built-in convenience variables $_shell_exitcode and $_shell_exitsignal
>    provide the exitcode or exit status of the shell commands launched by
>    GDB commands such as "shell", "pipe" and "make".
> diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
> index 8605ae71a2..5636ef9013 100644
> --- a/gdb/python/py-symbol.c
> +++ b/gdb/python/py-symbol.c
> @@ -456,6 +456,17 @@ gdbpy_lookup_global_symbol (PyObject *self, PyObject *args, PyObject *kw)
>        GDB_PY_HANDLE_EXCEPTION (except);
>      }
>
> +  if (symbol == NULL) {
> +    try
> +      {
> +        symbol = lookup_static_symbol (name, (domain_enum) domain).symbol;
> +      }
> +    catch (const gdb_exception &except)
> +      {
> +        GDB_PY_HANDLE_EXCEPTION (except);
> +      }
> +  }
> +
>    if (symbol)
>      {
>        sym_obj = symbol_to_symbol_object (symbol);
> diff --git a/gdb/testsuite/gdb.python/py-symbol.c b/gdb/testsuite/gdb.python/py-symbol.c
> index f77c8c8585..06a931bf5d 100644
> --- a/gdb/testsuite/gdb.python/py-symbol.c
> +++ b/gdb/testsuite/gdb.python/py-symbol.c
> @@ -32,9 +32,14 @@ class SimpleClass
>      return i; /* Break in class. */
>    }
>  };
> +
> +namespace {
> +  int anon = 10;
> +};
>  #endif
>
>  int qq = 72;                   /* line of qq */
> +static int rr = 42;            /* line of rr */
>
>  int func (int arg)
>  {
> diff --git a/gdb/testsuite/gdb.python/py-symbol.exp b/gdb/testsuite/gdb.python/py-symbol.exp
> index 5b8a2be7c4..722fcda0f0 100644
> --- a/gdb/testsuite/gdb.python/py-symbol.exp
> +++ b/gdb/testsuite/gdb.python/py-symbol.exp
> @@ -48,6 +48,16 @@ gdb_test "python print (gdb.lookup_global_symbol('qq').needs_frame)" \
>      "False" \
>      "print whether qq needs a frame"
>
> +set rr_line [gdb_get_line_number "line of rr"]
> +gdb_test "python print (gdb.lookup_global_symbol ('rr').line)" "$rr_line" \
> +    "print line number of rr"
> +
> +gdb_test "python print (gdb.lookup_global_symbol ('rr').value ())" "42" \
> +    "print value of rr"
> +
> +gdb_test "python print (gdb.lookup_global_symbol ('rr').needs_frame)" \
> +    "False" \
> +    "print whether rr needs a frame"
>
>  if ![runto_main] then {
>      fail "can't run to main"
> @@ -137,6 +147,9 @@ gdb_start
>  gdb_reinitialize_dir $srcdir/$subdir
>  gdb_load ${binfile}-cxx
>
> +gdb_test "python print (gdb.lookup_global_symbol ('(anonymous namespace)::anon').value ())" "10" \
> +    "print value of rr"
> +
>  if ![runto_main] then {
>      fail "can't run to main"
>      return 0
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
  

weekly ping

On Mon, Jul 8, 2019 at 5:04 PM Christian Biesinger
<cbiesinger@google.com> wrote:
>
> ping?
>
> On Tue, Jun 4, 2019 at 8:24 PM Christian Biesinger
> <cbiesinger@google.com> wrote:
> >
> > gdb/ChangeLog:
> >
> > 2019-06-04  Christian Biesinger  <cbiesinger@google.com>
> >
> >         PR/24474: Make gdb.lookup_global_symbol also check the STATIC_SCOPE
> >         because from a C/C++ developer's perspective, these are also globals.
> >         * NEWS: Mention this change.
> >         * python/py-symbol.c (gdbpy_lookup_global_symbol): Call
> >         lookup_static_symbol if lookup_global_symbol returns NULL.
> >
> > gdb/testsuite/ChangeLog:
> >
> > 2019-06-04  Christian Biesinger  <cbiesinger@google.com>
> >
> >         * gdb.python/py-symbol.c: Add a static variable and one in an anonymous
> >         namespace.
> >         * gdb.python/py-symbol.exp: Check that lookup_global_symbol finds those
> >         variables.
> >
> >
> > ---
> >  gdb/NEWS                               |  2 ++
> >  gdb/python/py-symbol.c                 | 11 +++++++++++
> >  gdb/testsuite/gdb.python/py-symbol.c   |  5 +++++
> >  gdb/testsuite/gdb.python/py-symbol.exp | 13 +++++++++++++
> >  4 files changed, 31 insertions(+)
> >
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index ded1fce406..bfd8d0a191 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -30,6 +30,8 @@
> >    ** gdb.Type has a new property 'objfile' which returns the objfile the
> >       type was defined in.
> >
> > +  ** gdb.lookup_global_symbol will now also find symbols with static linkage.
> > +
> >  * New built-in convenience variables $_shell_exitcode and $_shell_exitsignal
> >    provide the exitcode or exit status of the shell commands launched by
> >    GDB commands such as "shell", "pipe" and "make".
> > diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
> > index 8605ae71a2..5636ef9013 100644
> > --- a/gdb/python/py-symbol.c
> > +++ b/gdb/python/py-symbol.c
> > @@ -456,6 +456,17 @@ gdbpy_lookup_global_symbol (PyObject *self, PyObject *args, PyObject *kw)
> >        GDB_PY_HANDLE_EXCEPTION (except);
> >      }
> >
> > +  if (symbol == NULL) {
> > +    try
> > +      {
> > +        symbol = lookup_static_symbol (name, (domain_enum) domain).symbol;
> > +      }
> > +    catch (const gdb_exception &except)
> > +      {
> > +        GDB_PY_HANDLE_EXCEPTION (except);
> > +      }
> > +  }
> > +
> >    if (symbol)
> >      {
> >        sym_obj = symbol_to_symbol_object (symbol);
> > diff --git a/gdb/testsuite/gdb.python/py-symbol.c b/gdb/testsuite/gdb.python/py-symbol.c
> > index f77c8c8585..06a931bf5d 100644
> > --- a/gdb/testsuite/gdb.python/py-symbol.c
> > +++ b/gdb/testsuite/gdb.python/py-symbol.c
> > @@ -32,9 +32,14 @@ class SimpleClass
> >      return i; /* Break in class. */
> >    }
> >  };
> > +
> > +namespace {
> > +  int anon = 10;
> > +};
> >  #endif
> >
> >  int qq = 72;                   /* line of qq */
> > +static int rr = 42;            /* line of rr */
> >
> >  int func (int arg)
> >  {
> > diff --git a/gdb/testsuite/gdb.python/py-symbol.exp b/gdb/testsuite/gdb.python/py-symbol.exp
> > index 5b8a2be7c4..722fcda0f0 100644
> > --- a/gdb/testsuite/gdb.python/py-symbol.exp
> > +++ b/gdb/testsuite/gdb.python/py-symbol.exp
> > @@ -48,6 +48,16 @@ gdb_test "python print (gdb.lookup_global_symbol('qq').needs_frame)" \
> >      "False" \
> >      "print whether qq needs a frame"
> >
> > +set rr_line [gdb_get_line_number "line of rr"]
> > +gdb_test "python print (gdb.lookup_global_symbol ('rr').line)" "$rr_line" \
> > +    "print line number of rr"
> > +
> > +gdb_test "python print (gdb.lookup_global_symbol ('rr').value ())" "42" \
> > +    "print value of rr"
> > +
> > +gdb_test "python print (gdb.lookup_global_symbol ('rr').needs_frame)" \
> > +    "False" \
> > +    "print whether rr needs a frame"
> >
> >  if ![runto_main] then {
> >      fail "can't run to main"
> > @@ -137,6 +147,9 @@ gdb_start
> >  gdb_reinitialize_dir $srcdir/$subdir
> >  gdb_load ${binfile}-cxx
> >
> > +gdb_test "python print (gdb.lookup_global_symbol ('(anonymous namespace)::anon').value ())" "10" \
> > +    "print value of rr"
> > +
> >  if ![runto_main] then {
> >      fail "can't run to main"
> >      return 0
> > --
> > 2.22.0.rc1.311.g5d7573a151-goog
> >
  

On 2019-06-04 9:24 p.m., Christian Biesinger via gdb-patches wrote:
> gdb/ChangeLog:
> 
> 2019-06-04  Christian Biesinger  <cbiesinger@google.com>
> 
> 	PR/24474: Make gdb.lookup_global_symbol also check the STATIC_SCOPE
> 	because from a C/C++ developer's perspective, these are also globals.
> 	* NEWS: Mention this change.
> 	* python/py-symbol.c (gdbpy_lookup_global_symbol): Call
> 	lookup_static_symbol if lookup_global_symbol returns NULL.
> 
> gdb/testsuite/ChangeLog:
> 
> 2019-06-04  Christian Biesinger  <cbiesinger@google.com>
> 
> 	* gdb.python/py-symbol.c: Add a static variable and one in an anonymous
> 	namespace.
> 	* gdb.python/py-symbol.exp: Check that lookup_global_symbol finds those
> 	variables.

Hi Christian,

Sorry for the delay, I'll try to help you with this.

I am not sure this is the right thing to do, or at least some things need to be
clarified.  Global symbols are visible through the entire program (there can only
be one of that name), whereas there can be many static symbols with the same name.
What should the user expect if there are multiple static symbols with the same name?
I suppose we'll return the first one that matches, without any particular guarantee
about which one.

It is possible to get to a particular static symbol using gdb.lookup_symbol, provided
you pass a block that is under the right static block, and that no other symbol shadows
this one from the point of view of the block.  But IIRC, it can prove difficult to get
access the static block of a particular compilation unit (if you don't yet have a reference
to a frame block under that static block).

<thought drift>
I have been thinking for a while that we are missing the concept of compilation units in
our Python API, that would be the bridge between objfiles and global/static blocks.

- From an objfile, you could list all included compilation units or search through them
- From the compilation unit, you could obtain its global/static block.

From there, you could use gdb.lookup_symbol, passing the static block of the compilation unit.

However, in my (naive) attempt at adding compunits to the Python API [1], which consisted of
wrapping compunit_symtab objects in Python, the problem I faced is that compunit_symtab objects
are not created until full symbols are read.  So unless you used -readnow, you would not
see all the compilation units you would expect (because they are lazily created).
</thought drift>

Would it be an option to add a gdb.lookup_static_symbol function, that would only look through
the static blocks?  Its behavior could be that if you don't pass a block, it searches through
all the static blocks until it finds a matching symbol (just like your patch does with
gdb.lookup_global_symbol if no symbol is found).  And if you pass a block, it restricts the
search to the static block linked to that block, ensuring you find the static symbol you want.

Simon

[1] https://github.com/simark/binutils-gdb/commits/py-compunit
  

On Mon, Jul 15, 2019 at 8:28 PM Simon Marchi <simark@simark.ca> wrote:
> Would it be an option to add a gdb.lookup_static_symbol function, that would only look through
> the static blocks?  Its behavior could be that if you don't pass a block, it searches through
> all the static blocks until it finds a matching symbol (just like your patch does with
> gdb.lookup_global_symbol if no symbol is found).  And if you pass a block, it restricts the
> search to the static block linked to that block, ensuring you find the static symbol you want.

Thanks for your response! I have started implementing this and
concluded that I would prefer not to add a block argument with this
behavior to lookup_static_symbol:
- If I add it with the behavior you suggest, this will be very
confusing to use because it won't find function-local static variables
(they are not part of the static block)
- It does not add new functionality. You can already access static
symbols if you have a block: [sym for sym in block if sym.addr_class
== gdb.SYMBOL_LOC_STATIC]. And you can already do that in a function's
static block too, using block.static_block.
- I'd be happy to add a patch that adds makes block['foo'] work, in
addition to the currently-existing iteration

Conversely, lookup_static_symbol without a block does add new functionality.

I will send a new patch with this in a moment.

Christian
  

On 2019-07-26 18:04, Christian Biesinger wrote:
> Thanks for your response! I have started implementing this and
> concluded that I would prefer not to add a block argument with this
> behavior to lookup_static_symbol:
> - If I add it with the behavior you suggest, this will be very
> confusing to use because it won't find function-local static variables
> (they are not part of the static block)

Agreed, it would be a bit confusing to pass a block to a 'lookup' 
function, and have the search actually done in another block.

> - It does not add new functionality. You can already access static
> symbols if you have a block: [sym for sym in block if sym.addr_class
> == gdb.SYMBOL_LOC_STATIC]. And you can already do that in a function's
> static block too, using block.static_block.

I agree.

> - I'd be happy to add a patch that adds makes block['foo'] work, in
> addition to the currently-existing iteration

That is a separate issue, but yeah, if blocks can be seen as containers 
of symbols, and symbol names are guaranteed to be unique within a block, 
then it sounds like it would be handy.

> 
> Conversely, lookup_static_symbol without a block does add new 
> functionality.

Yes, and it's always possible to add parameters after if needed since 
it's Python.

> I will send a new patch with this in a moment.

Thanks,

Simon
  

On Mon, Jul 15, 2019 at 10:28 PM Simon Marchi <simark@simark.ca> wrote:
> I am not sure this is the right thing to do, or at least some things need to be
> clarified.  Global symbols are visible through the entire program (there can only
> be one of that name), whereas there can be many static symbols with the same name.
> What should the user expect if there are multiple static symbols with the same name?
> I suppose we'll return the first one that matches, without any particular guarantee
> about which one.

By the way, to follow up on that comment-- the symbol cache (or, for
that matter, symtab.c:lookup_static_symbol) does not take that into
account at all, it will only ever have one cache entry for one static
symbol name. In fact it does not even differentiate by objfile, unlike
for global symbols (??). Should maybe be fixed someday...

Christian