Re: [RFC - Python scripting] New methods Symtab.global_block and Symtab.static_block (docs included)

[Phil Muldoon <pmuldoon at redhat dot com>]

On 04/17/2012 08:25 AM, Siva Chandra wrote:
> The attached patch is a result of discussions here:
>     http://sourceware.org/ml/gdb-patches/2012-04/msg00226.html
> and here:
>     http://sourceware.org/ml/gdb-patches/2012-04/msg00302.html

Thanks, a few comments.

We already have gdb.Block.is_static and gdb.Block.is_global.  I don't
have an objection to this patch per-se, but wouldn't the user acquire
the global/static blocks similarly by iterating the blocks in the
Python code and doing the above tests?
 
> Code:
> 2012-04-17  Siva Chandra Reddy  <sivachandra@google.com>
> 
>         Add two new methods global_block and static_block to gdb.Symtab
>         objects.

I don't think GNU Style ChangeLogs allow for a summary line.  I don't
have an issue with it, but, check with maintainers.  Just a nit.

>         * NEWS (Python scripting): Add entry about the new methods.
>         * python/py-symtab.c (stpy_global_block): New function which
>         implements the gdb.Symtab.global_block() method.
>         (stpy_static_block): New function which implements the
>         gdb.Symtab.static_block() method.
>         (symtab_object_methods): Add entries for the two new methods.
> 
> Testsuite:
> 2012-04-17  Siva Chandra Reddy  <sivachandra@google.com>
> 
>         * py-symbol.exp: Add tests to test the new methods
>         gdb.Symtab.global_block() and gdb.Symtab.static_block().
>         * py-symbol.c: Add new struct to help test
>         gdb.Symtab.static_block().

gdb.python/ prefix to filenames here.
 
> Docs:
> 2012-04-17  Siva Chandra Reddy  <sivachandra@google.com>
> 
>         * gdb.texinfo (Symbol Tables In Python): Add documentation about
>         the new methods global_block and static_block on gdb.Symtab
>         objects.
> 
> Thanks,
> Siva Chandra


> +@defun Symtab.global_block ()
> +Return the global block of the underlying symbol table.  Note that,
> +though highly unlikely, the returned @code{gdb.Block} objects are not
> +guaranteed to be identical across different @value{GDBN} releases.
> +@end defun

In what sense not identical?  I know Doug brought this up, and it is
fair point.  But if I was a Python scripting author it would make me nervous
and distrustful of this API.  

It might also break our API promise, depending on how one looks at it.
What might change? I think mitigation here could be achieved through a
more thorough explanation?  Doug would have to expand on what he thinks
might change.  As it is, this patch makes me a little nervous of
the whole block API in general.

> +@defun Symtab.static_block ()
> +Return the static block of the underlying symbol table.  Note that,
> +though highly unlikely, the returned @code{gdb.Block} objects are not
> +guaranteed to be identical across different @value{GDBN} releases.
> +@end defun

Ditto above.
 
> +static PyObject *
> +stpy_global_block (PyObject *self, PyObject *args)
> +{
> +  struct symtab *symtab = NULL;
> > +  struct block *block = NULL;
> +
> +  STPY_REQUIRE_VALID (self, symtab);
> +
> +  block = symtab->blockvector->block[GLOBAL_BLOCK];
> +  return block_to_block_object (block, symtab->objfile);

I don't think so, but can block ever be NULL here?  Looking at the
code, I'm not even sure if it would matter as set_block just stores a
reference to the passed block.  However, blpy_get_superblock has a
NULL block check, so it might be worthwhile checking it out.

> +  block = symtab->blockvector->block[STATIC_BLOCK];
> +  return block_to_block_object (block, symtab->objfile);

Same as above.

> Index: testsuite/gdb.python/py-symbol.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.python/py-symbol.c,v
>
> retrieving revision 1.5
> diff -u -p -r1.5 py-symbol.c
 
> Index: testsuite/gdb.python/py-symtab.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.python/py-symtab.exp,v
> retrieving revision 1.8
> diff -u -p -r1.8 py-symtab.exp

I don't want you to fix this as it is unrelated to your patch, but
.exp files should have their own test inferior.  Something I noted so
I remember to fix it!

Cheers,

Phil