[PATCH] Add block['var'] accessor

[PATCH] Add block['var'] accessor

[Christian Biesinger via gdb-patches]

Currently we support iteration on blocks; this patch extends that to make
subscript access work as well.

gdb/ChangeLog:

2019-08-01  Christian Biesinger  <cbiesinger@google.com>

	* NEWS: Mention dictionary access on blocks.
	* python/py-block.c (blpy_getitem): New function.
  (block_object_as_mapping): New struct.
  (block_object_type): Use new struct for tp_as_mapping field.

gdb/doc/ChangeLog:

2019-08-01  Christian Biesinger  <cbiesinger@google.com>

	* python.texi (Blocks In Python): Document dictionary access on blocks.

gdb/testsuite/ChangeLog:

2019-08-01  Christian Biesinger  <cbiesinger@google.com>

	* gdb.python/py-block.exp: Test dictionary access on blocks.
---
 gdb/NEWS                              |  3 +++
 gdb/doc/python.texi                   |  7 +++++-
 gdb/python/py-block.c                 | 32 ++++++++++++++++++++++++++-
 gdb/testsuite/gdb.python/py-block.exp |  5 +++++
 4 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 3c3351e2c4..2db86c2ae7 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -47,6 +47,9 @@
   ** gdb.Objfile has new methods 'lookup_global_symbol' and
      'lookup_static_symbol' to lookup a symbol from this objfile only.
 
+  ** gdb.Block now supports the dictionary syntax for accessing symbols in
+     this block (e.g. block['local_variable']).
+
 * New commands
 
 | [COMMAND] | SHELL_COMMAND
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 3fdccd5e43..832283dede 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -4722,7 +4722,12 @@ A @code{gdb.Block} is iterable.  The iterator returns the symbols
 should not assume that a specific block object will always contain a
 given symbol, since changes in @value{GDBN} features and
 infrastructure may cause symbols move across blocks in a symbol
-table.
+table.  You can also use Python's @dfn{dictionary syntax} to access
+variables in this block, e.g.:
+
+@smallexample
+symbol = some_block['variable']  # symbol is of type gdb.Symbol
+@end smallexample
 
 The following block-related functions are available in the @code{gdb}
 module:
diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
index 90140ebc34..94d2454533 100644
--- a/gdb/python/py-block.c
+++ b/gdb/python/py-block.c
@@ -224,6 +224,30 @@ blpy_is_static (PyObject *self, void *closure)
   Py_RETURN_FALSE;
 }
 
+/* Given a string, returns the gdb.Symbol representing that symbol in this
+   block.  If such a symbol does not exist, returns NULL with a Python
+   exception.  */
+
+static PyObject *
+blpy_getitem (PyObject *self, PyObject *key)
+{
+  const struct block *block;
+
+  BLPY_REQUIRE_VALID (self, block);
+
+  gdb::unique_xmalloc_ptr<char> name = python_string_to_host_string (key);
+  if (name == nullptr)
+    return nullptr;
+
+  struct symbol *sym = block_lookup_symbol (
+    block, name.get(), symbol_name_match_type::FULL, VAR_DOMAIN);
+  if (!sym) {
+    PyErr_SetObject (PyExc_KeyError, key);
+    return nullptr;
+  }
+  return symbol_to_symbol_object (sym);
+}
+
 static void
 blpy_dealloc (PyObject *obj)
 {
@@ -440,6 +464,12 @@ static gdb_PyGetSetDef block_object_getset[] = {
   { NULL }  /* Sentinel */
 };
 
+static PyMappingMethods block_object_as_mapping = {
+  NULL,
+  blpy_getitem,
+  NULL
+};
+
 PyTypeObject block_object_type = {
   PyVarObject_HEAD_INIT (NULL, 0)
   "gdb.Block",			  /*tp_name*/
@@ -453,7 +483,7 @@ PyTypeObject block_object_type = {
   0,				  /*tp_repr*/
   0,				  /*tp_as_number*/
   0,				  /*tp_as_sequence*/
-  0,				  /*tp_as_mapping*/
+  &block_object_as_mapping,	  /*tp_as_mapping*/
   0,				  /*tp_hash */
   0,				  /*tp_call*/
   0,				  /*tp_str*/
diff --git a/gdb/testsuite/gdb.python/py-block.exp b/gdb/testsuite/gdb.python/py-block.exp
index 20d3968c04..6be1abe7ae 100644
--- a/gdb/testsuite/gdb.python/py-block.exp
+++ b/gdb/testsuite/gdb.python/py-block.exp
@@ -43,6 +43,11 @@ gdb_test "python print (block)" "<gdb.Block object at $hex>" "check block not No
 gdb_test "python print (block.function)" "None" "first anonymous block"
 gdb_test "python print (block.start)" "${decimal}" "check start not None"
 gdb_test "python print (block.end)" "${decimal}" "check end not None"
+gdb_test "python print (block\['f'\].name == 'f')" "True" "check variable access"
+gdb_test "python print (block\['nonexistent'\])" ".*KeyError: 'nonexistent'.*" \
+         "check nonexistent variable"
+gdb_test "python print (block\[42\])" ".*TypeError: Expected a string.*" \
+         "check non-string key"
 
 # Test global/static blocks
 gdb_py_test_silent_cmd "python frame = gdb.selected_frame()" "Get Frame" 0
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH] Add block['var'] accessor

[Eli Zaretskii]

> Date: Thu,  1 Aug 2019 17:34:20 -0500
> From: "Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>
> Cc: Christian Biesinger <cbiesinger@google.com>
> 
> Currently we support iteration on blocks; this patch extends that to make
> subscript access work as well.
> 
> gdb/ChangeLog:
> 
> 2019-08-01  Christian Biesinger  <cbiesinger@google.com>
> 
> 	* NEWS: Mention dictionary access on blocks.
> 	* python/py-block.c (blpy_getitem): New function.
>   (block_object_as_mapping): New struct.
>   (block_object_type): Use new struct for tp_as_mapping field.
> 
> gdb/doc/ChangeLog:
> 
> 2019-08-01  Christian Biesinger  <cbiesinger@google.com>
> 
> 	* python.texi (Blocks In Python): Document dictionary access on blocks.
> 
> gdb/testsuite/ChangeLog:
> 
> 2019-08-01  Christian Biesinger  <cbiesinger@google.com>
> 
> 	* gdb.python/py-block.exp: Test dictionary access on blocks.

OK for the documentation parts, thanks.

Re: [PATCH] Add block['var'] accessor

[Tom Tromey]

>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

Christian> 2019-08-01  Christian Biesinger  <cbiesinger@google.com>

Christian> 	* NEWS: Mention dictionary access on blocks.
Christian> 	* python/py-block.c (blpy_getitem): New function.
Christian>   (block_object_as_mapping): New struct.
Christian>   (block_object_type): Use new struct for tp_as_mapping field.

Thank you for the patch.

Christian> +  struct symbol *sym = block_lookup_symbol (
Christian> +    block, name.get(), symbol_name_match_type::FULL, VAR_DOMAIN);

What about looking up things in other domains?  I wonder if it's
possible to, say, have a local type that can be found by iteration over
the block but not by dictionary access.  If so, that seems confusing.

Christian> +  if (!sym) {
Christian> +    PyErr_SetObject (PyExc_KeyError, key);
Christian> +    return nullptr;
Christian> +  }

The formatting here is incorrect.  The call to block_lookup_symbol looks
weird (gdb doesn't generally use trailing parens like that), and the
brace after the "if" is misplaced.

Tom

[PATCH v2] Add block['var'] accessor

[Christian Biesinger via gdb-patches]

Sorry about the coding style. Both issues should be fixed now.

Currently we support iteration on blocks; this patch extends that to make
subscript access work as well.

gdb/ChangeLog:

2019-08-01  Christian Biesinger  <cbiesinger@google.com>

	* NEWS: Mention dictionary access on blocks.
	* python/py-block.c (blpy_getitem): New function.
	(block_object_as_mapping): New struct.
	(block_object_type): Use new struct for tp_as_mapping field.

gdb/doc/ChangeLog:

2019-08-01  Christian Biesinger  <cbiesinger@google.com>

	* python.texi (Blocks In Python): Document dictionary access on blocks.

gdb/testsuite/ChangeLog:

2019-08-01  Christian Biesinger  <cbiesinger@google.com>

	* gdb.python/py-block.exp: Test dictionary access on blocks.
---
 gdb/NEWS                              |  3 ++
 gdb/doc/python.texi                   |  7 ++++-
 gdb/python/py-block.c                 | 45 ++++++++++++++++++++++++++-
 gdb/testsuite/gdb.python/py-block.exp |  5 +++
 4 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 3c3351e2c4..2db86c2ae7 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -47,6 +47,9 @@
   ** gdb.Objfile has new methods 'lookup_global_symbol' and
      'lookup_static_symbol' to lookup a symbol from this objfile only.
 
+  ** gdb.Block now supports the dictionary syntax for accessing symbols in
+     this block (e.g. block['local_variable']).
+
 * New commands
 
 | [COMMAND] | SHELL_COMMAND
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 3fdccd5e43..832283dede 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -4722,7 +4722,12 @@ A @code{gdb.Block} is iterable.  The iterator returns the symbols
 should not assume that a specific block object will always contain a
 given symbol, since changes in @value{GDBN} features and
 infrastructure may cause symbols move across blocks in a symbol
-table.
+table.  You can also use Python's @dfn{dictionary syntax} to access
+variables in this block, e.g.:
+
+@smallexample
+symbol = some_block['variable']  # symbol is of type gdb.Symbol
+@end smallexample
 
 The following block-related functions are available in the @code{gdb}
 module:
diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
index 90140ebc34..4dc47ff169 100644
--- a/gdb/python/py-block.c
+++ b/gdb/python/py-block.c
@@ -224,6 +224,43 @@ blpy_is_static (PyObject *self, void *closure)
   Py_RETURN_FALSE;
 }
 
+/* Given a string, returns the gdb.Symbol representing that symbol in this
+   block.  If such a symbol does not exist, returns NULL with a Python
+   exception.  */
+
+static PyObject *
+blpy_getitem (PyObject *self, PyObject *key)
+{
+  const struct block *block;
+
+  BLPY_REQUIRE_VALID (self, block);
+
+  gdb::unique_xmalloc_ptr<char> name = python_string_to_host_string (key);
+  if (name == nullptr)
+    return nullptr;
+
+  lookup_name_info lookup_name (name.get(), symbol_name_match_type::FULL);
+
+  /* We use ALL_BLOCK_SYMBOLS_WITH_NAME instead of block_lookup_symbol so
+     that we can look up symbols irrespective of the domain, matching the
+     iterator. It would be confusing if the iterator returns symbols you
+     can't find via getitem.  */
+  struct block_iterator iter;
+  struct symbol *sym = nullptr;
+  ALL_BLOCK_SYMBOLS_WITH_NAME (block, lookup_name, iter, sym)
+    {
+      /* Just stop at the first match */
+      break;
+    }
+
+  if (sym == nullptr)
+    {
+      PyErr_SetObject (PyExc_KeyError, key);
+      return nullptr;
+    }
+  return symbol_to_symbol_object (sym);
+}
+
 static void
 blpy_dealloc (PyObject *obj)
 {
@@ -440,6 +477,12 @@ static gdb_PyGetSetDef block_object_getset[] = {
   { NULL }  /* Sentinel */
 };
 
+static PyMappingMethods block_object_as_mapping = {
+  NULL,
+  blpy_getitem,
+  NULL
+};
+
 PyTypeObject block_object_type = {
   PyVarObject_HEAD_INIT (NULL, 0)
   "gdb.Block",			  /*tp_name*/
@@ -453,7 +496,7 @@ PyTypeObject block_object_type = {
   0,				  /*tp_repr*/
   0,				  /*tp_as_number*/
   0,				  /*tp_as_sequence*/
-  0,				  /*tp_as_mapping*/
+  &block_object_as_mapping,	  /*tp_as_mapping*/
   0,				  /*tp_hash */
   0,				  /*tp_call*/
   0,				  /*tp_str*/
diff --git a/gdb/testsuite/gdb.python/py-block.exp b/gdb/testsuite/gdb.python/py-block.exp
index 20d3968c04..6be1abe7ae 100644
--- a/gdb/testsuite/gdb.python/py-block.exp
+++ b/gdb/testsuite/gdb.python/py-block.exp
@@ -43,6 +43,11 @@ gdb_test "python print (block)" "<gdb.Block object at $hex>" "check block not No
 gdb_test "python print (block.function)" "None" "first anonymous block"
 gdb_test "python print (block.start)" "${decimal}" "check start not None"
 gdb_test "python print (block.end)" "${decimal}" "check end not None"
+gdb_test "python print (block\['f'\].name == 'f')" "True" "check variable access"
+gdb_test "python print (block\['nonexistent'\])" ".*KeyError: 'nonexistent'.*" \
+         "check nonexistent variable"
+gdb_test "python print (block\[42\])" ".*TypeError: Expected a string.*" \
+         "check non-string key"
 
 # Test global/static blocks
 gdb_py_test_silent_cmd "python frame = gdb.selected_frame()" "Get Frame" 0
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH v2] Add block['var'] accessor

[Tom Tromey]

>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

Christian> Sorry about the coding style. Both issues should be fixed now.
Christian> Currently we support iteration on blocks; this patch extends that to make
Christian> subscript access work as well.

Thank you for the patch.
This is ok.

Tom

Re: [PATCH v2] Add block['var'] accessor

[Christian Biesinger via gdb-patches]

On Mon, Aug 5, 2019 at 12:33 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:
>
> Christian> Sorry about the coding style. Both issues should be fixed now.
> Christian> Currently we support iteration on blocks; this patch extends that to make
> Christian> subscript access work as well.
>
> Thank you for the patch.
> This is ok.

Thanks, pushed.

Christian

Re: [PATCH] Add block['var'] accessor

[Simon Marchi]

On 2019-08-02 9:36 a.m., Tom Tromey wrote:
> Christian> +  struct symbol *sym = block_lookup_symbol (
> Christian> +    block, name.get(), symbol_name_match_type::FULL, VAR_DOMAIN);
> 
> What about looking up things in other domains?  I wonder if it's
> possible to, say, have a local type that can be found by iteration over
> the block but not by dictionary access.  If so, that seems confusing.

Just wondering, was this commented responded to?

Simon

Re: [PATCH] Add block['var'] accessor

[Christian Biesinger via gdb-patches]

On Mon, Aug 5, 2019 at 1:17 PM Simon Marchi <simark@simark.ca> wrote:
>
> On 2019-08-02 9:36 a.m., Tom Tromey wrote:
> > Christian> +  struct symbol *sym = block_lookup_symbol (
> > Christian> +    block, name.get(), symbol_name_match_type::FULL, VAR_DOMAIN);
> >
> > What about looking up things in other domains?  I wonder if it's
> > possible to, say, have a local type that can be found by iteration over
> > the block but not by dictionary access.  If so, that seems confusing.
>
> Just wondering, was this commented responded to?

Oh sorry, in the most recent version of the patch I fixed it by using
ALL_BLOCK_SYMBOLS_WITH_NAME, which should match the behavior of the
iterator afaict.

Christian

Re: [PATCH] Add block['var'] accessor

[Simon Marchi]

On 2019-08-05 14:19, Christian Biesinger wrote:
> On Mon, Aug 5, 2019 at 1:17 PM Simon Marchi <simark@simark.ca> wrote:
>> 
>> On 2019-08-02 9:36 a.m., Tom Tromey wrote:
>> > Christian> +  struct symbol *sym = block_lookup_symbol (
>> > Christian> +    block, name.get(), symbol_name_match_type::FULL, VAR_DOMAIN);
>> >
>> > What about looking up things in other domains?  I wonder if it's
>> > possible to, say, have a local type that can be found by iteration over
>> > the block but not by dictionary access.  If so, that seems confusing.
>> 
>> Just wondering, was this commented responded to?
> 
> Oh sorry, in the most recent version of the patch I fixed it by using
> ALL_BLOCK_SYMBOLS_WITH_NAME, which should match the behavior of the
> iterator afaict.

Ok, thanks!

Note that it helps others (including the reviewer) if you acknowledge 
the comments, either with Ok / Done or "I'd rather not do that because 
X".

Simon

Re: [PATCH] Add block['var'] accessor

[Christian Biesinger via gdb-patches]

On Mon, Aug 5, 2019 at 1:28 PM Simon Marchi <simark@simark.ca> wrote:
>
> On 2019-08-05 14:19, Christian Biesinger wrote:
> > On Mon, Aug 5, 2019 at 1:17 PM Simon Marchi <simark@simark.ca> wrote:
> >>
> >> On 2019-08-02 9:36 a.m., Tom Tromey wrote:
> >> > Christian> +  struct symbol *sym = block_lookup_symbol (
> >> > Christian> +    block, name.get(), symbol_name_match_type::FULL, VAR_DOMAIN);
> >> >
> >> > What about looking up things in other domains?  I wonder if it's
> >> > possible to, say, have a local type that can be found by iteration over
> >> > the block but not by dictionary access.  If so, that seems confusing.
> >>
> >> Just wondering, was this commented responded to?
> >
> > Oh sorry, in the most recent version of the patch I fixed it by using
> > ALL_BLOCK_SYMBOLS_WITH_NAME, which should match the behavior of the
> > iterator afaict.
>
> Ok, thanks!
>
> Note that it helps others (including the reviewer) if you acknowledge
> the comments, either with Ok / Done or "I'd rather not do that because
> X".

I did say at the top "Both issues should be fixed now." -- are you
suggesting it's better to individually acknowledge them? I'd be happy
to do that if y'all prefer.

Christian