Re: [PATCH 4/7] py-symbol: Require a frame for lookup_symbol only when necessary

[Phil Muldoon <pmuldoon@redhat.com>]

On 04/02/16 17:29, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
>
> gdbpy_lookup_symbol requires a frame prior to doing the symbol lookup but
> a frame isn't necessary for many symbol types.  Calling
> symbol_read_needs_frame will tell us if it's necessary but we need to
> have already looked up the symbol to use it.  This patch puts the
> lookup first and then only resolves the frame if one is required.
>
> This allows us to lookup static symbols directly from python rather
> than using gdb.eval_and_parse.

Thanks. I've rewritten this patch email reply from the garbled so hopefully you
can make sense of it! ;)

All of these patches need documentation in the user manual, and also new tests
for functionality.

> ---
>  gdb/python/py-symbol.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
> index cbbc9e2..c7f0ff8 100644
> --- a/gdb/python/py-symbol.c
> +++ b/gdb/python/py-symbol.c
> @@ -382,14 +382,28 @@ gdbpy_lookup_symbol (PyObject *self, PyObject *args, PyObject *kw)
>  
>    if (block_obj)
>      block = block_object_to_block (block_obj);
> -  else
> +
> +  TRY
> +    {
> +      symbol = lookup_symbol (name, block, (domain_enum) domain,
> +			      &is_a_field_of_this).symbol;
> +    }
> +  CATCH (except, RETURN_MASK_ALL)
> +    {
> +      GDB_PY_HANDLE_EXCEPTION (except);
> +    }
> +  END_CATCH
> +
> +  if (symbol && !block)
>      {
>        struct frame_info *selected_frame;
>  
>        TRY
>  	{
> -	  selected_frame = get_selected_frame (_("No frame selected."));
> -	  block = get_frame_block (selected_frame, NULL);
> +	  if (symbol_read_needs_frame(symbol)) {

Space after the function name and before the (. This and others.
> +	    selected_frame = get_selected_frame (_("No frame selected."));
> +	    block = get_frame_block (selected_frame, NULL);
> +	  }
>  	}
>        CATCH (except, RETURN_MASK_ALL)
>  	{
> @@ -398,17 +412,6 @@ gdbpy_lookup_symbol (PyObject *self, PyObject *args, PyObject *kw)
>        END_CATCH
>      }
>  
> -  TRY
> -    {
> -      symbol = lookup_symbol (name, block, (domain_enum) domain,
> -			      &is_a_field_of_this).symbol;
> -    }
> -  CATCH (except, RETURN_MASK_ALL)
> -    {
> -      GDB_PY_HANDLE_EXCEPTION (except);
> -    }
> -  END_CATCH
> -
>    ret_tuple = PyTuple_New (2);
>    if (!ret_tuple)
>      return NULL;

My only concern here is (and the context is not meaningful enough to
see), is that you've removed an "else" above which is an unconditional
branch, and replaced it with an IF instead. What happens when both IFs
can fail?

Cheers

Phil