RFA: fix PR python/13351

RFA: fix PR python/13351

[Tom Tromey]

This fixes PR python/13351.

The bug is that gdb.lookup_symbol required a current frame, but this
restriction doesn't really make sense.  It is ok to look up global
symbols without a current frame.

This patch just lifts the restriction.

This requires a doc review.

Built and regtested on x86-64 Fedora 15.

Tom

2012-01-31  Tom Tromey  <tromey@redhat.com>

	PR python/13351:
	* python/py-symbol.c (gdbpy_lookup_symbol): Work even when there
	is no frame.

2012-01-31  Tom Tromey  <tromey@redhat.com>

	* gdb.texinfo (Symbols In Python): lookup_symbol works even
	without a current frame.

2012-01-31  Tom Tromey  <tromey@redhat.com>

	* gdb.python/py-symbol.exp: Add test for frame-less
	lookup_symbol.

From 29e3b00fe4b2593af94ed37f3f6c29d2a218ecbd Mon Sep 17 00:00:00 2001
From: Tom Tromey <tromey@redhat.com>
Date: Tue, 31 Jan 2012 13:53:35 -0700
Subject: [PATCH 1/3] fix for PR 13351 - let lookup_symbol work without a
 frame

---
 gdb/ChangeLog                          |    6 ++++++
 gdb/doc/ChangeLog                      |    5 +++++
 gdb/doc/gdb.texinfo                    |    3 ++-
 gdb/python/py-symbol.c                 |    5 +++--
 gdb/testsuite/ChangeLog                |    5 +++++
 gdb/testsuite/gdb.python/py-symbol.exp |    3 +++
 6 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 5738d14..86fa88f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -23924,7 +23924,8 @@ arguments.
 optional @var{block} argument restricts the search to symbols visible
 in that @var{block}.  The @var{block} argument must be a
 @code{gdb.Block} object.  If omitted, the block for the current frame
-is used.  The optional @var{domain} argument restricts
+is used; if there is no current frame, then file-scoped and global
+symbols are searched.  The optional @var{domain} argument restricts
 the search to the domain type.  The @var{domain} argument must be a
 domain constant defined in the @code{gdb} module and described later
 in this chapter.
diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
index 9d32a71..f93a35a 100644
--- a/gdb/python/py-symbol.c
+++ b/gdb/python/py-symbol.c
@@ -292,8 +292,9 @@ gdbpy_lookup_symbol (PyObject *self, PyObject *args, PyObject *kw)
 
       TRY_CATCH (except, RETURN_MASK_ALL)
 	{
-	  selected_frame = get_selected_frame (_("No frame selected."));
-	  block = get_frame_block (selected_frame, NULL);
+	  selected_frame = get_selected_frame_if_set ();
+	  if (selected_frame)
+	    block = get_frame_block (selected_frame, NULL);
 	}
       GDB_PY_HANDLE_EXCEPTION (except);
     }
diff --git a/gdb/testsuite/gdb.python/py-symbol.exp b/gdb/testsuite/gdb.python/py-symbol.exp
index b0e73c3..91bfd51 100644
--- a/gdb/testsuite/gdb.python/py-symbol.exp
+++ b/gdb/testsuite/gdb.python/py-symbol.exp
@@ -42,6 +42,9 @@ gdb_py_test_silent_cmd "python main_func = gdb.lookup_global_symbol(\"main\")" "
 gdb_test "python print main_func.is_function" "True" "Test main_func.is_function"
 gdb_test "python print gdb.lookup_global_symbol(\"junk\")" "None" "Test lookup_global_symbol(\"junk\")"
 
+gdb_test "python print gdb.lookup_symbol('main')\[0\].is_function" "True" \
+    "Lookup main using lookup_symbol"
+
 if ![runto_main] then {
     fail "Can't run to main"
     return 0
-- 
1.7.6.5

Re: RFA: fix PR python/13351

[Doug Evans]

On Tue, Jan 31, 2012 at 1:59 PM, Tom Tromey <tromey@redhat.com> wrote:
> This fixes PR python/13351.
>
> The bug is that gdb.lookup_symbol required a current frame, but this
> restriction doesn't really make sense.  It is ok to look up global
> symbols without a current frame.
>
> This patch just lifts the restriction.
>
> This requires a doc review.
>
> Built and regtested on x86-64 Fedora 15.
>
> Tom
>
> 2012-01-31  Tom Tromey  <tromey@redhat.com>
>
>        PR python/13351:
>        * python/py-symbol.c (gdbpy_lookup_symbol): Work even when there
>        is no frame.

I think a bit of discussion is needed. [Sorry!]

There is also lookup_global_symbol.
Are we sure we want lookup_symbol to work when there is no frame?
[it doesn't feel very clean to me, but maybe I just need to look at it
differently]

Re: RFA: fix PR python/13351

[Pedro Alves]

On 01/31/2012 09:59 PM, Tom Tromey wrote:
> --- a/gdb/python/py-symbol.c
> +++ b/gdb/python/py-symbol.c
> @@ -292,8 +292,9 @@ gdbpy_lookup_symbol (PyObject *self, PyObject *args, PyObject *kw)
>  
>        TRY_CATCH (except, RETURN_MASK_ALL)
>  	{
> -	  selected_frame = get_selected_frame (_("No frame selected."));
> -	  block = get_frame_block (selected_frame, NULL);
> +	  selected_frame = get_selected_frame_if_set ();
> +	  if (selected_frame)
> +	    block = get_frame_block (selected_frame, NULL);
>  	}

I'm getting a bit nervous about get_selected_frame_if_set usages getting spread
around.  (It feels a bit like we're going back in time, to a time before the
always-a-frame work.)  The selected frame may not be set, while we still may have
a current frame, so get_selected_frame would lazily select it, and so the lookup
would still start on a frame block.  If you don't want an error, you can pass NULL
to get_selected_frame.

-- 
Pedro Alves

Re: RFA: fix PR python/13351

[Phil Muldoon]

On 01/31/2012 11:13 PM, Doug Evans wrote:
> On Tue, Jan 31, 2012 at 1:59 PM, Tom Tromey<tromey@redhat.com>  wrote:
>> This fixes PR python/13351.
>>
>> The bug is that gdb.lookup_symbol required a current frame, but this
>> restriction doesn't really make sense.  It is ok to look up global
>> symbols without a current frame.
>>
>> This patch just lifts the restriction.
>>
>> This requires a doc review.
>>
>> Built and regtested on x86-64 Fedora 15.
>>
>> Tom
>>
>> 2012-01-31  Tom Tromey<tromey@redhat.com>
>>
>>         PR python/13351:
>>         * python/py-symbol.c (gdbpy_lookup_symbol): Work even when there
>>         is no frame.
> I think a bit of discussion is needed. [Sorry!]
>
> There is also lookup_global_symbol.
> Are we sure we want lookup_symbol to work when there is no frame?
> [it doesn't feel very clean to me, but maybe I just need to look at it
> differently]

I agree that two APIs for the same functionality are a pain.  However we 
have to keep them for backwards compatibility.
However, can we not deprecated and even gut lookup_global_symbol to call 
lookup_symbol with a frame?

Cheers,

Phil

Re: RFA: fix PR python/13351

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> I think a bit of discussion is needed. [Sorry!]

No problem.

Doug> Are we sure we want lookup_symbol to work when there is no frame?
Doug> [it doesn't feel very clean to me, but maybe I just need to look at it
Doug> differently]

lookup_symbol already works without a frame, if you pass in a block.

To me it seemed reasonable to extend this to search the global blocks.
After all, even with a block argument it might return a symbol from a
global block -- it is just that there is no way to request this from the
start.

That is, I didn't see a particular compelling reason for the current
restriction.  What is the point of this error?

What seems not-clean to you?

Actually I wonder whether this patch goes far enough.  I think perhaps
it should be possible to write lookup_symbol(block = None) to get the
no-frame behavior; and also lookup_symbol(block = some_frame) to take
the block from an explicit frame.

Tom

Re: RFA: fix PR python/13351

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> I agree that two APIs for the same functionality are a pain.  However
Phil> we have to keep them for backwards compatibility.
Phil> However, can we not deprecated and even gut lookup_global_symbol to
Phil> call lookup_symbol with a frame?

I think the two aren't really equivalent; lookup_symbol_in_language
calls demangle_for_lookup, but lookup_symbol_global does not.

Tom

Re: RFA: fix PR python/13351

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I'm getting a bit nervous about get_selected_frame_if_set usages
Pedro> getting spread around.  (It feels a bit like we're going back in
Pedro> time, to a time before the always-a-frame work.)  The selected
Pedro> frame may not be set, while we still may have a current frame, so
Pedro> get_selected_frame would lazily select it, and so the lookup
Pedro> would still start on a frame block.  If you don't want an error,
Pedro> you can pass NULL to get_selected_frame.

Ok, I can make that change.

I don't understand why get_selected_frame is the preferred API.  To me
it seems clearly worse: passing in an error message is ugly, and it
isn't really possible to distinguish "no frames" from "an exception was
thrown for some other reason".  Neither of these problems would affect
get_selected_frame_if_set.

Perhaps I can do the detection by checking has_stack_frames?  Is that safe?

Tom

Re: RFA: fix PR python/13351

[Pedro Alves]

On 02/01/2012 04:04 PM, Tom Tromey wrote:

> I don't understand why get_selected_frame is the preferred API.  To me
> it seems clearly worse: passing in an error message is ugly, and it
> isn't really possible to distinguish "no frames" from "an exception was
> thrown for some other reason".  Neither of these problems would affect
> get_selected_frame_if_set.
> 
> Perhaps I can do the detection by checking has_stack_frames?  Is that safe?

Yes.  Though you could just call get_selected_block instead (which does the
has_stack_frames check).

       TRY_CATCH (except, RETURN_MASK_ALL)
 	{
-	  selected_frame = get_selected_frame (_("No frame selected."));
-	  block = get_frame_block (selected_frame, NULL);
+	  block = get_selected_block (NULL);
 	}
       GDB_PY_HANDLE_EXCEPTION (except);

-- 
Pedro Alves

Re: RFA: fix PR python/13351

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Yes.  Though you could just call get_selected_block instead
Pedro> (which does the has_stack_frames check).

Thanks, I didn't know about that one.

Tom

Re: RFA: fix PR python/13351

[Doug Evans]

On Wed, Feb 1, 2012 at 8:01 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
> lookup_symbol already works without a frame, if you pass in a block.
>
> To me it seemed reasonable to extend this to search the global blocks.
> After all, even with a block argument it might return a symbol from a
> global block -- it is just that there is no way to request this from the
> start.
>
> That is, I didn't see a particular compelling reason for the current
> restriction.  What is the point of this error?
>
> What seems not-clean to you?

I'm ok with it now.

> Actually I wonder whether this patch goes far enough.  I think perhaps
> it should be possible to write lookup_symbol(block = None) to get the
> no-frame behavior; and also lookup_symbol(block = some_frame) to take
> the block from an explicit frame.

Ok.
[please document that no-frame includes the case where the thread is running]

Re: RFA: fix PR python/13351

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> This requires a doc review.

Ping.

Tom

Re: RFA: fix PR python/13351

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Date: Tue, 07 Feb 2012 11:04:49 -0700
> 
> >>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:
> 
> Tom> This requires a doc review.
> 
> Ping.

OK, and sorry for the delay.