diff mbox

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

Message ID 1454606973-31017-5-git-send-email-jeffm@suse.com
State New, archived
Headers

Commit Message

Jeff Mahoney Feb. 4, 2016, 5:29 p.m. UTC 
  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.
---
 gdb/python/py-symbol.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

Comments

Phil Muldoon Feb. 5, 2016, 12:17 p.m. UTC | #1 
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.

Please add documentation and further tests for new 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)) {

Formatting nit, space after function name and before (.

> +        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 >      }

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
Jeff Mahoney Feb. 5, 2016, 4:07 p.m. UTC | #2 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/5/16 7:17 AM, Phil Muldoon wrote:
> 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.
> 
> Please add documentation and further tests for new 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)) {
> 
> Formatting nit, space after function name and before (.
> 
>> +        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 >      }
> 
> 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?

This patch is broken.  It works for my need to look up static symbols
but it fails the py-symbol testsuite.  We look up the block and never
use it, so it's no wonder it's failing.

- -Jeff


- -- 
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.19 (Darwin)
Comment: GPGTools - http://gpgtools.org

iQIcBAEBAgAGBQJWtMjbAAoJEB57S2MheeWyJhcQALzkE3DaXUyTHxaSovnUbe6+
bFleikXDjbL4UKYwT/0iJfVoaH8weoWMiFfrYVXJgCqXIUEX9mtUPfMXq5NHyJ71
0FgQa9pRVCK2sS/4jpPjMwL40AU4DI4XYOHbSowZfm9kn28yY+uP6UkbiWsv5dDn
egNkSzjyrX4MzMHSzZIDqNBGqPdp03nLBKyrjl8iLtgUK6HZwBponk2Y50l8d+gy
M6FP4xXMUqKgW0JBW+P1V/S6YowW+RCaHgBrmwISe9QU79M3s3Jpi+VD+WxEO8HY
rUrlOXzJhMm/IcF9r4ROmYGXFdQdOjlMyTQtOkyCe8+VYlwOsc107GzipNnQ2mRf
Rd2+2UZNs+430pcpivq+h+wDlcM5Cu6QWDFa9/TIHw97n776YeQ6bZHcSCXFOvd2
FrWsOqQoM2zNTKTjuxCJyr2lfgHxFTiSggVrX1Glmw2sVdwks5DvoGXNa8lnq1fp
PGII0edxMPg1neOhZZ24j/k2dB1dZmAOMuzUg6OggSiVSj31R+ZmarE/xRHfXqPk
kTfuJHX45dpBTZQ+X6VgWkYzG6OHhRV2ilWHky7PETM3LTpTVFNGqOxEEYZach0/
C5SV6Zykp+wjkZTVLL/q3YgPRHJj+SUKRRBEj7qfG4PcyeI0ybsDVfQ8ZEh6JANI
WWRyy3/zyUbONSV77I/R
=eJFa
-----END PGP SIGNATURE-----
Phil Muldoon Feb. 5, 2016, 4:35 p.m. UTC | #3 
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
diff mbox

Patch

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)) {
+	    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;