Re: [RFA] Have block_innermost_frame start from selected frame

Re: [RFA] Have block_innermost_frame start from selected frame

[Paul Hilfinger]

OK.  The discussion seems to have converged.  Here (and in the following) are
the revised patch (this time with documentation).  I will commit after a
suitable interval.  Thanks for the comments.

Paul N. Hilfinger
(Hilfinger@adacore.com)


Previously, GDB would search for the frame containing variables in a
particular lexical block starting from the current (top) frame,
ignoring any currently selected frame.  It is not clear why this is
desirable for variables that require a frame; why would a user
deliberately select one frame and then expect to see the value of a
variable in a more recent frame?  This change causes
block_innermost_frame to start looking from the selected frame, if
there is one.

This change is perhaps unnecessarily conservative.  It uses
get_selected_frame_if_set rather than get_selected_frame in order to
avoid the side effect of calling select_frame, which would probably be
harmless.

Add a paragraph to the "Program Variables" section of the texinfo
documentation concerning the use of "::" for accessing non-static variables.

2011-12-27  Paul Hilfinger  <hilfingr@adacore.com>

  * gdb/blockframe.c (block_innermost_frame): Start search from selected frame,
    if present, or otherwise the current frame.

  * gdb/doc/gdb.texinfo (Variables): Document use of :: for non-static
    variables.
---
 gdb/ChangeLog       |    5 +++++
 gdb/blockframe.c    |    9 ++++++---
 gdb/doc/ChangeLog   |    5 +++++
 gdb/doc/gdb.texinfo |   48 +++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 40f6169..d1b2401 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2011-12-30  Paul Hilfinger  <hilfingr@adacore.com>
+
+        * blockframe.c (block_innermost_frame): Start search from
+        selected frame, if present, or otherwise the current frame.
+
 2011-12-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* gdbarch.sh (max_insn_length): Extend the comment by unit.
diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index ef53a3d..2d2b45c 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -354,8 +354,9 @@ find_pc_partial_function (CORE_ADDR pc, char **name, CORE_ADDR *address,
   return find_pc_partial_function_gnu_ifunc (pc, name, address, endaddr, NULL);
 }
 
-/* Return the innermost stack frame executing inside of BLOCK, or NULL
-   if there is no such frame.  If BLOCK is NULL, just return NULL.  */
+/* Return the innermost stack frame that is executing inside of BLOCK and is 
+ * at least as old as the selected frame. Return NULL if there is no
+ * such frame.  If BLOCK is NULL, just return NULL.  */
 
 struct frame_info *
 block_innermost_frame (const struct block *block)
@@ -370,7 +371,9 @@ block_innermost_frame (const struct block *block)
   start = BLOCK_START (block);
   end = BLOCK_END (block);
 
-  frame = get_current_frame ();
+  frame = get_selected_frame_if_set ();
+  if (frame == NULL)
+    frame = get_current_frame ();
   while (frame != NULL)
     {
       struct block *frame_block = get_frame_block (frame, NULL);
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 0ede40b..00e876f 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2011-12-30  Paul Hilfinger  <hilfingr@adacore.com>
+
+	* gdb.texinfo (Variables): Document use of :: for non-static
+	variables.
+
 2011-12-23  Kevin Pouget  <kevin.pouget@st.com>
 
 	Introduce gdb.FinishBreakpoint in Python.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 3cd3b67..cd38e3f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7304,12 +7304,13 @@ examine the variable @code{b} while your program is executing inside
 the block where @code{b} is declared.
 
 @cindex variable name conflict
-There is an exception: you can refer to a variable or function whose
+You can refer to a variable or function whose
 scope is a single source file even if the current execution point is not
 in this file.  But it is possible to have more than one such variable or
 function with the same name (in different source files).  If that
 happens, referring to that name has unpredictable effects.  If you wish,
-you can specify a static variable in a particular function or file,
+you can specify a static variable in a particular function or file by
+qualifying its name
 using the colon-colon (@code{::}) notation:
 
 @cindex colon-colon, context for variables/functions
@@ -7332,8 +7333,49 @@ to print a global value of @code{x} defined in @file{f2.c}:
 (@value{GDBP}) p 'f2.c'::x
 @end smallexample
 
+The @code{::} qualifying notation is normally used for referring to
+static variables, since you typically disambiguate uses of local variables
+in functions by selecting the appropriate frame and using the
+unqualified name of the variable.  However, you may also use this notation
+to refer to local variables in frames enclosing the selected frame:
+
+@smallexample
+void
+foo (int a)
+@{
+  if (a < 10)
+      bar (a);
+  else
+      process (a);    /* Stop here */
+@}
+
+int
+bar (int a)
+@{
+  foo (a + 5);
+@}
+@end smallexample
+
+@noindent
+If you have a breakpoint at the indicated line above
+when the program executes the call @code{bar(0)}, then when the program
+stops, the commands
+
+@smallexample
+(@value{GDBP}) p a
+(@value{GDBP}) p bar::a
+(@value{GDBP}) up 2
+(@value{GDBP}) p a
+(@value{GDBP}) p bar::a
+
+@end smallexample
+
+@noindent
+will print the values @samp{10}, @samp{5}, @samp{5}, and @samp{0} in that
+order.
+
 @cindex C@t{++} scope resolution
-This use of @samp{::} is very rarely in conflict with the very similar
+These uses of @samp{::} are very rarely in conflict with the very similar
 use of the same notation in C@t{++}.  @value{GDBN} also supports use of the C@t{++}
 scope resolution operator in @value{GDBN} expressions.
 @c FIXME: Um, so what happens in one of those rare cases where it's in
-- 
1.7.0.4

Re: [RFA] Have block_innermost_frame start from selected frame

[Eli Zaretskii]

> From: Paul Hilfinger <Hilfinger@adacore.com>
> Date: Fri, 30 Dec 2011 16:52:22 -0500 (EST)
> 
> @@ -7304,12 +7304,13 @@ examine the variable @code{b} while your program is executing inside
>  the block where @code{b} is declared.
>  
>  @cindex variable name conflict
> -There is an exception: you can refer to a variable or function whose
> +You can refer to a variable or function whose
>  scope is a single source file even if the current execution point is not
>  in this file.  But it is possible to have more than one such variable or
>  function with the same name (in different source files).  If that
>  happens, referring to that name has unpredictable effects.  If you wish,
> -you can specify a static variable in a particular function or file,
> +you can specify a static variable in a particular function or file by
> +qualifying its name
>  using the colon-colon (@code{::}) notation:
>  
>  @cindex colon-colon, context for variables/functions

This hunk seems to be limited to changes in style, rather than
content.  Can you tell why you thought these style changes are needed,
in particular the first one?

Also, I have reservations about using the term "qualifying" for this
notation.  Since we already use "scope resolution" for them in the C++
context, why not use "scope resolution" for C and other languages as
well?

> +unqualified name of the variable.  However, you may also use this notation
> +to refer to local variables in frames enclosing the selected frame:

I'm not sure "enclosing frame" is clear enough.  I think we tend to
use "outer frame" elsewhere in the manual.  Alternatively, you could
use "enclosing scope".

> +@smallexample
> +(@value{GDBP}) p a
> +(@value{GDBP}) p bar::a
> +(@value{GDBP}) up 2
> +(@value{GDBP}) p a
> +(@value{GDBP}) p bar::a
> +
> +@end smallexample

Please remove the empty line before "@end smallexample".

Anyway, this begs the question: is the scope resolution available only
for print commands, or also in other commands, like `watch'?  If the
latter can also use this, then I think we should mention that, here
and where watchpoints are described.  If the `watch' command cannot
use this, then it sounds like we are inconsistent here.

Thanks.

Re: [RFA] Have block_innermost_frame start from selected frame

[Paul Hilfinger]

> > @@ -7304,12 +7304,13 @@ examine the variable @code{b} while your program is executing inside
> >  the block where @code{b} is declared.
> >  
> >  @cindex variable name conflict
> > -There is an exception: you can refer to a variable or function whose
> > +You can refer to a variable or function whose
> >  scope is a single source file even if the current execution point is not
> >  in this file.  But it is possible to have more than one such variable or
> >  function with the same name (in different source files).  If that
> >  happens, referring to that name has unpredictable effects.  If you wish,
> > -you can specify a static variable in a particular function or file,
> > +you can specify a static variable in a particular function or file by
> > +qualifying its name
> >  using the colon-colon (@code{::}) notation:
> >  
> >  @cindex colon-colon, context for variables/functions
> 
> This hunk seems to be limited to changes in style, rather than
> content.  Can you tell why you thought these style changes are needed,
> in particular the first one?
> 
> Also, I have reservations about using the term "qualifying" for this
> notation.  Since we already use "scope resolution" for them in the C++
> context, why not use "scope resolution" for C and other languages as
> well?
> 

I'm not wed to these changes.  The first one is accidental: I had wanted to
modify the section so that first it talked about unqualified uses and then 
turned to qualified uses, not as an "exception" but rather as a separate case.
Apparently, I neglected to modify the preceding portion to specifically say it
concerned unqualified variables.

The second change was to introduce the term "qualified".  However, it also
corrects a slight problem---that comma after "file," is out of place, I think.
However, if you don't like "qualifying", I am happy to remove both changes.

> > +unqualified name of the variable.  However, you may also use this notation
> > +to refer to local variables in frames enclosing the selected frame:
> 
> I'm not sure "enclosing frame" is clear enough.  I think we tend to
> use "outer frame" elsewhere in the manual.  Alternatively, you could
> use "enclosing scope".
> 

Yeah, but I really need to say something like "frames that are at or outside
the selected frame".  Would that be understandable terminology, do you think?

I don't like "enclosing scope".  We are dealing with statically scoped
languages, where "enclosing scope" usually means "lexically enclosing
scope", and that is not what we are talking about here.

> > +@smallexample
> > +(@value{GDBP}) p a
> > +(@value{GDBP}) p bar::a
> > +(@value{GDBP}) up 2
> > +(@value{GDBP}) p a
> > +(@value{GDBP}) p bar::a
> > +
> > +@end smallexample
> 
> Please remove the empty line before "@end smallexample".

Right.

> Anyway, this begs the question: is the scope resolution available only
> for print commands, or also in other commands, like `watch'?  If the
> latter can also use this, then I think we should mention that, here
> and where watchpoints are described.  If the `watch' command cannot
> use this, then it sounds like we are inconsistent here.
> 
Well, since I don't see any circular arguments here, I wouldn't say
that any questions are being begged [sorry: obligatory pedantry
there], but you are certainly right that it does raise the question of
general applicability.  The section is about "Program variables" but
all examples use 'print'.  Furthermore, you are correct that watch
behaves incorrectly at the moment: it picks up the correct variable
value when 'watch' is issued, but then reverts to the old behavior
when deciding which variable to watch, giving nonsensical results.  I
will have to revise the patch.  Thanks for catching this.

-- 
Paul N. Hilfinger
(Hilfinger@adacore.com)

Re: [RFA] Have block_innermost_frame start from selected frame

[Paul Hilfinger]

[-- Attachment #1: Type: text/plain, Size: 2814 bytes --]

This is an update of my previous patch to address a problem that Eli
uncovered: references to BLOCK:LOCAL_VARIABLE did not work in watched
expressions.  In fact, due to a bug, a local variable referred to with
BLOCK:VAR has always been handled incorrectly in 'watch', even if it
is interpreted to refer to the innermost frame associated with BLOCK,
because no breakpoint would be set to remove the watch when the
variable's frame was deallocated.

Eli, your actual comment was a question about whether the syntax
applied to watch (given that all the examples used print).  On
consideration, I decided to keep things as they are, because if the
issue is that it is unclear where variables may be used, it is a
problem that extends to the entire chapter on "Examining Data", and
should be addressed by a rather larger editorial assault.

GDB used to search for the frame containing variables in a particular
lexical block starting from the current (top) frame, ignoring any
currently selected frame.  It is not clear why this is desirable for
variables that require a frame; why would a user deliberately select
one frame and then expect to see the value of a variable in a more
recent frame?  This change causes block_innermost_frame to start
looking from the selected frame, if there is one.

This change is perhaps unnecessarily conservative.  It uses
get_selected_frame_if_set rather than get_selected_frame in order to
avoid the side effect of calling select_frame, which would probably be
harmless.

Second, expression-parsing routines previously made the unwarranted assumption
that all block-qualified variables (written with the GDB extension
<block>::<variable>) are static.  As a result, they failed to update
innermost_block, which confused the watch commands about when variables in
watched expressions went out of scope, and also caused the wrong variables to
be watched.  This change modifies these routines to treat all local variables
the same whether or not they are block-qualified.

Finally, we add a paragraph to the "Program Variables" section of the texinfo
documentation concerning the use of "::" for accessing non-static variables.

Paul N. Hilfinger
(Hilfinger@adacore.com)


2011-12-27  Paul Hilfinger  <hilfingr@adacore.com>

  * gdb/blockframe.c (block_innermost_frame): Start search from selected frame,
    if present, or otherwise the current frame.

  * gdb/c-exp.y, gdb/m2-exp.y, gdb/objc-exp.y: Update innermost_block

  * gdb/doc/gdb.texinfo (Variables): Document use of :: for non-static
    variables.
---
 gdb/blockframe.c    |    9 ++++++---
 gdb/c-exp.y         |    7 +++++++
 gdb/doc/gdb.texinfo |   44 ++++++++++++++++++++++++++++++++++++++++++--
 gdb/m2-exp.y        |    7 +++++++
 gdb/objc-exp.y      |    7 +++++++
 5 files changed, 69 insertions(+), 5 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Have-block_innermost_frame-start-from-selected-frame.patch --]
[-- Type: text/x-patch; name="0001-Have-block_innermost_frame-start-from-selected-frame.patch", Size: 4908 bytes --]

diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index 3897366..1917b68 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -352,8 +352,9 @@ find_pc_partial_function (CORE_ADDR pc, char **name, CORE_ADDR *address,
   return find_pc_partial_function_gnu_ifunc (pc, name, address, endaddr, NULL);
 }
 
-/* Return the innermost stack frame executing inside of BLOCK, or NULL
-   if there is no such frame.  If BLOCK is NULL, just return NULL.  */
+/* Return the innermost stack frame that is executing inside of BLOCK and is
+ * at least as old as the selected frame. Return NULL if there is no
+ * such frame.  If BLOCK is NULL, just return NULL.  */
 
 struct frame_info *
 block_innermost_frame (const struct block *block)
@@ -368,7 +369,9 @@ block_innermost_frame (const struct block *block)
   start = BLOCK_START (block);
   end = BLOCK_END (block);
 
-  frame = get_current_frame ();
+  frame = get_selected_frame_if_set ();
+  if (frame == NULL)
+    frame = get_current_frame ();
   while (frame != NULL)
     {
       struct block *frame_block = get_frame_block (frame, NULL);
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index bdcae33..5b57f24 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -778,6 +778,13 @@ variable:	block COLONCOLON name
 			  if (sym == 0)
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
+			  if (symbol_read_needs_frame (sym))
+			    {
+			      if (innermost_block == 0
+				  || contained_in (block_found, 
+						   innermost_block))
+				innermost_block = block_found;
+			    }
 
 			  write_exp_elt_opcode (OP_VAR_VALUE);
 			  /* block_found is set by lookup_symbol.  */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 2f4aa4f..d1a2dde 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7306,7 +7306,7 @@ scope is a single source file even if the current execution point is not
 in this file.  But it is possible to have more than one such variable or
 function with the same name (in different source files).  If that
 happens, referring to that name has unpredictable effects.  If you wish,
-you can specify a static variable in a particular function or file,
+you can specify a static variable in a particular function or file by
 using the colon-colon (@code{::}) notation:
 
 @cindex colon-colon, context for variables/functions
@@ -7329,8 +7329,48 @@ to print a global value of @code{x} defined in @file{f2.c}:
 (@value{GDBP}) p 'f2.c'::x
 @end smallexample
 
+The @code{::} notation is normally used for referring to
+static variables, since you typically disambiguate uses of local variables
+in functions by selecting the appropriate frame and using the
+simple name of the variable.  However, you may also use this notation
+to refer to local variables in frames enclosing the selected frame:
+
+@smallexample
+void
+foo (int a)
+@{
+  if (a < 10)
+      bar (a);
+  else
+      process (a);    /* Stop here */
+@}
+
+int
+bar (int a)
+@{
+  foo (a + 5);
+@}
+@end smallexample
+
+@noindent
+If you have a breakpoint at the commented line
+when the program executes the call @code{bar(0)}, then when the program
+stops, the commands
+
+@smallexample
+(@value{GDBP}) p a
+(@value{GDBP}) p bar::a
+(@value{GDBP}) up 2
+(@value{GDBP}) p a
+(@value{GDBP}) p bar::a
+@end smallexample
+
+@noindent
+will print the values @samp{10}, @samp{5}, @samp{5}, and @samp{0} in that
+order.
+
 @cindex C@t{++} scope resolution
-This use of @samp{::} is very rarely in conflict with the very similar
+These uses of @samp{::} are very rarely in conflict with the very similar
 use of the same notation in C@t{++}.  @value{GDBN} also supports use of the C@t{++}
 scope resolution operator in @value{GDBN} expressions.
 @c FIXME: Um, so what happens in one of those rare cases where it's in
diff --git a/gdb/m2-exp.y b/gdb/m2-exp.y
index 1e3e3cb..d59678b 100644
--- a/gdb/m2-exp.y
+++ b/gdb/m2-exp.y
@@ -588,6 +588,13 @@ variable:	block COLONCOLON NAME
 			  if (sym == 0)
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
+			  if (symbol_read_needs_frame (sym))
+			    {
+			      if (innermost_block == 0
+				  || contained_in (block_found, 
+						   innermost_block))
+				innermost_block = block_found;
+			    }
 
 			  write_exp_elt_opcode (OP_VAR_VALUE);
 			  /* block_found is set by lookup_symbol.  */
diff --git a/gdb/objc-exp.y b/gdb/objc-exp.y
index 346b404..00e5e7f 100644
--- a/gdb/objc-exp.y
+++ b/gdb/objc-exp.y
@@ -648,6 +648,13 @@ variable:	block COLONCOLON name
 			  if (sym == 0)
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
+			  if (symbol_read_needs_frame (sym))
+			    {
+			      if (innermost_block == 0
+				  || contained_in (block_found, 
+						   innermost_block))
+				innermost_block = block_found;
+			    }
 
 			  write_exp_elt_opcode (OP_VAR_VALUE);
 			  /* block_found is set by lookup_symbol.  */

Re: [RFA] Have block_innermost_frame start from selected frame

[Eli Zaretskii]

> From: Paul Hilfinger <Hilfinger@adacore.com>
> CC: gdb-patches@sourceware.org
> Date: Mon,  9 Jan 2012 02:16:45 -0500 (EST)
> 
> Eli, your actual comment was a question about whether the syntax
> applied to watch (given that all the examples used print).  On
> consideration, I decided to keep things as they are

What does this mean? what exactly did you leave unchanged?

And do I need to review the doc patch again, or is it largely
unchanged?

Thanks.

Re: [RFA] Have block_innermost_frame start from selected frame

[Paul Hilfinger]

> > Eli, your actual comment was a question about whether the syntax
> > applied to watch (given that all the examples used print).  On
> > consideration, I decided to keep things as they are
> 
> What does this mean? what exactly did you leave unchanged?
> 
> And do I need to review the doc patch again, or is it largely
> unchanged?
> 

Sorry.  I meant that you should not have to review the doc/ part of the
patch, which is unchanged since the last time I submitted it.  I did not
add any text specific to watchpoints, etc., on the grounds that the
entire chapter already appears intended to describe expressions in
general, and apparently has been understood as such up until now.

Thanks.

Paul

Re: [RFA] Have block_innermost_frame start from selected frame

[Joel Brobecker]

Hi Paul,

> 2011-12-27  Paul Hilfinger  <hilfingr@adacore.com>
> 
>   * gdb/blockframe.c (block_innermost_frame): Start search from selected frame,
>     if present, or otherwise the current frame.
> 
>   * gdb/c-exp.y, gdb/m2-exp.y, gdb/objc-exp.y: Update innermost_block
> 
>   * gdb/doc/gdb.texinfo (Variables): Document use of :: for non-static
>     variables.

When you create an entry in gdb's ChangeLog file, can you remember
to remove the "gdb/" prefix? Also, the doc/ subdirectory has its own
ChangeLog file as well, so remember to remove the "gdb/doc/" prefix
in the filename.

> -/* Return the innermost stack frame executing inside of BLOCK, or NULL
> -   if there is no such frame.  If BLOCK is NULL, just return NULL.  */
> +/* Return the innermost stack frame that is executing inside of BLOCK and is
> + * at least as old as the selected frame. Return NULL if there is no
> + * such frame.  If BLOCK is NULL, just return NULL.  */

Minor issue with formatting: Can you remove the '*' at the start of
the second and third line?

> -  frame = get_current_frame ();
> +  frame = get_selected_frame_if_set ();
> +  if (frame == NULL)
> +    frame = get_current_frame ();

I really flip-flopped on this, between using get_selected_frame_if_set
or just get_selected_frame. In the end, to me, the real difference
between the two is the fact that the second could potentially *change*
the global state (selected frame, language). These could be considered
side-effects while I have a feeling that block_innermost_frame is
a function that should really be pure. So I agree with you that using
get_selected_frame_if_set was the right choice.

> +			  if (symbol_read_needs_frame (sym))
> +			    {
> +			      if (innermost_block == 0
> +				  || contained_in (block_found, 
> +						   innermost_block))
> +				innermost_block = block_found;
> +			    }

I know you copied this from elsewhere in the file, and that elsewhere
also has that extraneous trailing space (`|| contained_in (block_found, '),
but could you remove it from this new hunk?

> +@smallexample
> +void
> +foo (int a)
> +@{
> +  if (a < 10)
> +      bar (a);
> +  else
> +      process (a);    /* Stop here */
> +@}

Nit-picking: The formatting looks a little off for the if and else
blocks.  It would be nicer, I think, if we remained consistent the GDB
style...

Eli?

> +@smallexample
> +(@value{GDBP}) p a
> +(@value{GDBP}) p bar::a
> +(@value{GDBP}) up 2
> +(@value{GDBP}) p a
> +(@value{GDBP}) p bar::a
> +@end smallexample
> +
> +@noindent
> +will print the values @samp{10}, @samp{5}, @samp{5}, and @samp{0} in that
> +order.

My 2 cents: We could also embed the GDB transcript, rather than
having the input on one end, and the output on the other hand.
Not a request to change, just some thoughts...

-- 
Joel

Re: [RFA] Have block_innermost_frame start from selected frame

[Eli Zaretskii]

> Date: Tue, 10 Jan 2012 08:32:13 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sourceware.org
> 
> > +@smallexample
> > +void
> > +foo (int a)
> > +@{
> > +  if (a < 10)
> > +      bar (a);
> > +  else
> > +      process (a);    /* Stop here */
> > +@}
> 
> Nit-picking: The formatting looks a little off for the if and else
> blocks.  It would be nicer, I think, if we remained consistent the GDB
> style...
> 
> Eli?

Using the GNU style is preferable, yes.

Re: [RFA] Have block_innermost_frame start from selected frame

[Joel Brobecker]

> > 2011-12-27  Paul Hilfinger  <hilfingr@adacore.com>
> > 
> >   * gdb/blockframe.c (block_innermost_frame): Start search from selected frame,
> >     if present, or otherwise the current frame.
> > 
> >   * gdb/c-exp.y, gdb/m2-exp.y, gdb/objc-exp.y: Update innermost_block
> > 
> >   * gdb/doc/gdb.texinfo (Variables): Document use of :: for non-static
> >     variables.

I had also meant to say that the patch is pre-approved, once
the small code changes I recommended are applied! Ooops...

Thank you, Paul.
-- 
Joel

[PATCH 3/3] Add test for use of "<block>::<variable>" syntax for locals in watch.

[Hilfinger]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 919 bytes --]

I have committed this patch.

First, fix a technical problem with the function recurser.  The test sets a
watch on local_x at a point where its value is technically undefined.  The
test is written assuming that the value is not initially 2, but nothing in the
C standard guarantees that.

Second, augment the existing test for variables in recursive calls to check an
equivalent expression that explicitly sets the scope of the local variable
being tracked.

2012-01-11  Paul Hilfinger  <hilfingr@adacore.com>

	* gdb.base/watchpoint.c (recurser): Initialize local_x.
	(main): Repeat recurser call.
	* gdb.base/watchpoint.exp: Check that 'watch recurser::local_x' is
	equivalent to 'local_x'.
---
 gdb/testsuite/ChangeLog               |    7 +++++++
 gdb/testsuite/gdb.base/watchpoint.c   |    8 +++++++-
 gdb/testsuite/gdb.base/watchpoint.exp |   14 ++++++++++++++
 3 files changed, 28 insertions(+), 1 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0003-Add-test-for-use-of-block-variable-syntax-for-locals.patch --]
[-- Type: text/x-patch; name="0003-Add-test-for-use-of-block-variable-syntax-for-locals.patch", Size: 3168 bytes --]

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 8c811c9..6e18dfc 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,4 +1,11 @@
 2012-01-11  Paul Hilfinger  <hilfingr@adacore.com>
+
+	* gdb.base/watchpoint.c (recurser): Initialize local_x.
+	(main): Repeat recurser call.
+	* gdb.base/watchpoint.exp: Check that 'watch recurser::local_x' is
+	equivalent to 'local_x'.
+
+2012-01-11  Paul Hilfinger  <hilfingr@adacore.com>
     	    Joel Brobecker <brobecker@adacore.com>
 
 	* gdb.base/recpar.c, gdb.base/recpar.exp: New files.
diff --git a/gdb/testsuite/gdb.base/watchpoint.c b/gdb/testsuite/gdb.base/watchpoint.c
index 50f0a83..88c110f 100644
--- a/gdb/testsuite/gdb.base/watchpoint.c
+++ b/gdb/testsuite/gdb.base/watchpoint.c
@@ -80,7 +80,7 @@ void recurser (int  x)
 void recurser (x) int  x;
 #endif
 {
-  int  local_x;
+  int  local_x = 0;
 
   if (x > 0)
     recurser (x-1);
@@ -232,6 +232,12 @@ int main ()
   marker6 ();
   recurser (2);
 
+  /* This invocation is used for watches of a local variable with explicitly
+     specified scope when recursion happens.
+     */
+  marker6 ();
+  recurser (2);
+
   marker6 ();
 
   func3 ();
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index f321de5..1860368 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -580,6 +580,7 @@ proc test_complex_watchpoint {} {
         #
         gdb_test "tbreak recurser" ".*breakpoint.*"
         gdb_test "cont" "Continuing.*recurser.*"
+        gdb_test "next" "if \\(x > 0.*" "next past local_x initialization"
         gdb_test "watch local_x" ".*\[Ww\]atchpoint \[0-9\]*: local_x" \
                  "set local watch in recursive call"
         gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: local_x.*New value = 2.*" \
@@ -587,6 +588,19 @@ proc test_complex_watchpoint {} {
         gdb_test "cont" "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" \
                  "self-delete local watch in recursive call"
 
+        # Repeat the preceding test, but this time use "recurser::local_x" as
+        # the variable to track.
+        gdb_test "cont" "Continuing.*marker6.*"
+        gdb_test "tbreak recurser" ".*breakpoint.*"
+        gdb_test "cont" "Continuing.*recurser.*"
+        gdb_test "next" "if \\(x > 0.*" "next past local_x initialization"
+        gdb_test "watch recurser::local_x" ".*\[Ww\]atchpoint \[0-9\]*: recurser::local_x" \
+                 "set local watch in recursive call with explicit scope"
+        gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: recurser::local_x.*New value = 2.*" \
+                 "trigger local watch with explicit scope in recursive call"
+        gdb_test "cont" "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" \
+                 "self-delete local watch with explicit scope in recursive call (2)"
+
 	# Disable everything so we can finish the program at full speed
 	gdb_test_no_output "disable" "disable in test_complex_watchpoint"
 

[PATCH 1/3] Have block_innermost_frame start from selected frame and document.

[Hilfinger]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 2100 bytes --]

I have committed this patch.

GDB used to search for the frame containing variables in a particular
lexical block starting from the current (top) frame, ignoring any
currently selected frame.  It is not clear why this is desirable for
variables that require a frame; why would a user deliberately select
one frame and then expect to see the value of a variable in a more
recent frame?  This change causes block_innermost_frame to start
looking from the selected frame, if there is one.  It may be
unnecessarily conservative: we use get_selected_frame_if_set rather
than get_selected_frame in order to avoid the side effect of calling
select_frame, which would probably be harmless.

Expression-parsing routines previously made the unwarranted assumption
that all block-qualified variables (written with the GDB extension
<block>::<variable>) are static.  As a result, they failed to update
innermost_block, which confused the watch commands about when
variables in watched expressions went out of scope, and also caused
the wrong variables to be watched.  This patch also modifies these
routines to treat all local variables the same whether or not they are
block-qualified.

Finally, we add a paragraph to the "Program Variables" section of the texinfo
documentation concerning the use of "::" for accessing non-static variables.

2012-01-11  Paul Hilfinger  <hilfingr@adacore.com>

	* gdb/blockframe.c (block_innermost_frame): Start search from selected
	frame, if present, or otherwise the current frame.

	* gdb/c-exp.y (variable): Update innermost_block for
	'block COLONCOLON NAME' clause.
	* gdb/m2-exp.y (variable): Ditto.
	* gdb/objc-exp.y (variable): Ditto.

	* gdb/doc/gdb.texinfo (Variables): Document use of :: for non-static
	variables.
---
 gdb/ChangeLog       |   10 ++++++++++
 gdb/blockframe.c    |    9 ++++++---
 gdb/c-exp.y         |    7 +++++++
 gdb/doc/ChangeLog   |    5 +++++
 gdb/doc/gdb.texinfo |   45 +++++++++++++++++++++++++++++++++++++++++++--
 gdb/m2-exp.y        |    7 +++++++
 gdb/objc-exp.y      |    7 +++++++
 7 files changed, 85 insertions(+), 5 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Have-block_innermost_frame-start-from-selected-frame.patch --]
[-- Type: text/x-patch; name="0001-Have-block_innermost_frame-start-from-selected-frame.patch", Size: 5810 bytes --]

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1ac2510..29f2d1e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2012-01-11  Paul Hilfinger  <hilfingr@adacore.com>
+
+	* blockframe.c (block_innermost_frame): Start search from selected
+	frame, if present, or otherwise the current frame.
+
+	* c-exp.y (variable): Update innermost_block for
+	'block COLONCOLON NAME' clause.
+	* m2-exp.y (variable): Ditto.
+	* objc-exp.y (variable): Ditto.
+
 2012-01-10  Tom Tromey  <tromey@redhat.com>
 
 	PR python/13199:
diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index 3897366..cb61cf3 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -352,8 +352,9 @@ find_pc_partial_function (CORE_ADDR pc, char **name, CORE_ADDR *address,
   return find_pc_partial_function_gnu_ifunc (pc, name, address, endaddr, NULL);
 }
 
-/* Return the innermost stack frame executing inside of BLOCK, or NULL
-   if there is no such frame.  If BLOCK is NULL, just return NULL.  */
+/* Return the innermost stack frame that is executing inside of BLOCK and is
+   at least as old as the selected frame. Return NULL if there is no
+   such frame.  If BLOCK is NULL, just return NULL.  */
 
 struct frame_info *
 block_innermost_frame (const struct block *block)
@@ -368,7 +369,9 @@ block_innermost_frame (const struct block *block)
   start = BLOCK_START (block);
   end = BLOCK_END (block);
 
-  frame = get_current_frame ();
+  frame = get_selected_frame_if_set ();
+  if (frame == NULL)
+    frame = get_current_frame ();
   while (frame != NULL)
     {
       struct block *frame_block = get_frame_block (frame, NULL);
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 24a4761..bf4f4bc 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -778,6 +778,13 @@ variable:	block COLONCOLON name
 			  if (sym == 0)
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
+			  if (symbol_read_needs_frame (sym))
+			    {
+			      if (innermost_block == 0
+				  || contained_in (block_found,
+						   innermost_block))
+				innermost_block = block_found;
+			    }
 
 			  write_exp_elt_opcode (OP_VAR_VALUE);
 			  /* block_found is set by lookup_symbol.  */
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 5a9d2ff..2914190 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2012-01-11  Paul Hilfinger  <hilfingr@adacore.com>
+
+	* gdb.texinfo (Variables): Document use of :: for non-static
+	variables.
+
 2012-01-05  Joel Brobecker  <brobecker@adacore.com>
 
 	* gdbint.texinfo (Start of New Year Procedure): Update
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 2f4aa4f..4a8ff7b 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7306,7 +7306,7 @@ scope is a single source file even if the current execution point is not
 in this file.  But it is possible to have more than one such variable or
 function with the same name (in different source files).  If that
 happens, referring to that name has unpredictable effects.  If you wish,
-you can specify a static variable in a particular function or file,
+you can specify a static variable in a particular function or file by
 using the colon-colon (@code{::}) notation:
 
 @cindex colon-colon, context for variables/functions
@@ -7329,8 +7329,49 @@ to print a global value of @code{x} defined in @file{f2.c}:
 (@value{GDBP}) p 'f2.c'::x
 @end smallexample
 
+The @code{::} notation is normally used for referring to
+static variables, since you typically disambiguate uses of local variables
+in functions by selecting the appropriate frame and using the
+simple name of the variable.  However, you may also use this notation
+to refer to local variables in frames enclosing the selected frame:
+
+@smallexample
+void
+foo (int a)
+@{
+  if (a < 10)
+    bar (a);
+  else
+    process (a);    /* Stop here */
+@}
+
+int
+bar (int a)
+@{
+  foo (a + 5);
+@}
+@end smallexample
+
+@noindent
+For example, if there is a breakpoint at the commented line,
+here is what you might see
+when the program stops after executing the call @code{bar(0)}:
+
+@smallexample
+(@value{GDBP}) p a
+$1 = 10
+(@value{GDBP}) p bar::a
+$2 = 5
+(@value{GDBP}) up 2
+#2  0x080483d0 in foo (a=5) at foobar.c:12
+(@value{GDBP}) p a
+$3 = 5
+(@value{GDBP}) p bar::a
+$4 = 0
+@end smallexample
+
 @cindex C@t{++} scope resolution
-This use of @samp{::} is very rarely in conflict with the very similar
+These uses of @samp{::} are very rarely in conflict with the very similar
 use of the same notation in C@t{++}.  @value{GDBN} also supports use of the C@t{++}
 scope resolution operator in @value{GDBN} expressions.
 @c FIXME: Um, so what happens in one of those rare cases where it's in
diff --git a/gdb/m2-exp.y b/gdb/m2-exp.y
index 1e3e3cb..ef9ec8e 100644
--- a/gdb/m2-exp.y
+++ b/gdb/m2-exp.y
@@ -588,6 +588,13 @@ variable:	block COLONCOLON NAME
 			  if (sym == 0)
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
+			  if (symbol_read_needs_frame (sym))
+			    {
+			      if (innermost_block == 0
+				  || contained_in (block_found,
+						   innermost_block))
+				innermost_block = block_found;
+			    }
 
 			  write_exp_elt_opcode (OP_VAR_VALUE);
 			  /* block_found is set by lookup_symbol.  */
diff --git a/gdb/objc-exp.y b/gdb/objc-exp.y
index 346b404..b43ba66 100644
--- a/gdb/objc-exp.y
+++ b/gdb/objc-exp.y
@@ -648,6 +648,13 @@ variable:	block COLONCOLON name
 			  if (sym == 0)
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
+			  if (symbol_read_needs_frame (sym))
+			    {
+			      if (innermost_block == 0
+				  || contained_in (block_found,
+						   innermost_block))
+				innermost_block = block_found;
+			    }
 
 			  write_exp_elt_opcode (OP_VAR_VALUE);
 			  /* block_found is set by lookup_symbol.  */

[PATCH 2/3] Add testcase for locals identified with FUNCTION::VAR syntax.

[Hilfinger]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 802 bytes --]

I have committed this patch.

This test stops inside a recursive function after a few levels of recursion,
goes up some frames, and then accesses a local variable with 'print foo::val'
rather than the usual 'print val' to see if the former pays attention to the
selected frame.

2012-01-11  Paul Hilfinger  <hilfingr@adacore.com>
    	    Joel Brobecker <brobecker@adacore.com>

	* gdb.base/recpar.c, gdb.base/recpar.exp: New files.
---
 gdb/testsuite/ChangeLog           |    5 ++++
 gdb/testsuite/gdb.base/recpar.c   |   42 +++++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/recpar.exp |   42 +++++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/recpar.c
 create mode 100644 gdb/testsuite/gdb.base/recpar.exp


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-testcase-for-locals-identified-with-FUNCTION-VAR.patch --]
[-- Type: text/x-patch; name="0002-Add-testcase-for-locals-identified-with-FUNCTION-VAR.patch", Size: 3388 bytes --]

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index c79071c..8c811c9 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2012-01-11  Paul Hilfinger  <hilfingr@adacore.com>
+    	    Joel Brobecker <brobecker@adacore.com>
+
+	* gdb.base/recpar.c, gdb.base/recpar.exp: New files.
+
 2012-01-05  Pedro Alves  <alves.ped@gmail.com>
 
 	* gdb.mi/mi-stepn.c, gdb.mi/mi-stepn.exp: New files.
diff --git a/gdb/testsuite/gdb.base/recpar.c b/gdb/testsuite/gdb.base/recpar.c
new file mode 100644
index 0000000..e6c3e23
--- /dev/null
+++ b/gdb/testsuite/gdb.base/recpar.c
@@ -0,0 +1,42 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+foo (int n)
+{
+  int val = n;
+
+  {
+    char val = n ? 'y' : 'n'; /* Hides upper-level `val'.  */
+
+    if (val == 'y') /* BREAK */
+      return n + foo (n - 1);
+  }
+
+  return 0;
+}
+
+int
+main (void)
+{
+  int res = foo (5);
+
+  if (res != 15) /* Dummy use of variable res.  */
+    return 1;
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/recpar.exp b/gdb/testsuite/gdb.base/recpar.exp
new file mode 100644
index 0000000..6dd466b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/recpar.exp
@@ -0,0 +1,42 @@
+# Copyright (C) 2012 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile recpar
+set srcfile ${testfile}.c
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
+    return -1
+}
+
+if ![runto_main] {
+    untested ${testfile}.exp
+    return -1
+}
+
+gdb_test "break $srcfile:[gdb_get_line_number BREAK $srcfile] if n == 3" \
+         "Breakpoint $decimal at $hex: file .*recpar\\.c, line $decimal\\."
+
+gdb_test "continue" \
+         "Breakpoint .* foo \\(n=3\\) at .*$srcfile:$decimal.*"
+
+gdb_test "backtrace" \
+         "#0 +foo \\(n=3\\).*\r\n#1.* foo \\(n=4\\).*\r\n#2.* foo \\(n=5\\).*#3.* main \\(\\).*"
+
+gdb_test "frame 2" \
+         "#2 .* foo \\(n=5\\) .*"
+
+# In the currently selected frame, n=5, and thus foo::val should be 5
+# as well.
+gdb_test "print foo::val" \
+         " = 5"

Re: [RFA] Have block_innermost_frame start from selected frame

[Jan Kratochvil]

On Thu, 29 Dec 2011 20:01:34 +0100, Paul Hilfinger wrote:
> I understand the argument here, but I'm not sure I can agree.  The
> ambiguity you speak of already occurs with high frequency, after all,
> since when I say
>   
>     print x
> 
> there may be many local x's lying around,

Yes, some warning/menu-select in such case was one of the ways considered to
implement Tom's ambiguous-linespec patch (which I did not implement myself in
the end at all, sure kudos to Tom).


> that warnings would not be considered helpful.

I really do not mind, not more mails are needed, it is true if
warning/menu/whatever should be printed in this case you are right there are
more such places where it should also happen.  Just this is a GDB behavior
change so I thought it may be even more appropriate in such case.  Never mind.


> > /* Return the innermost stack frame executing inside of BLOCK, or NULL
> >    if there is no such frame.  If BLOCK is NULL, just return NULL.  */
> >
> > struct frame_info *
> > block_innermost_frame (const struct block *block)
> 
> Good point.  In fact, do you think we should change the function name?
> The frame is no longer "innermost", after all.

It is still innermost-to-the-selected-frame.  There isn't going to be a second
function implementing the original innermost-to-the-current-frame behavior.
I do not see a need for name change in this case.


Thanks,
Jan

Re: [RFA] Have block_innermost_frame start from selected frame

[Paul Hilfinger]

> In such case the doc should be updated, particularly that it has became now
> related to the currently selected frame.

Will do.

> It may be all even more tricky than it was before.  What about using query()
> if such reference is ambiguous?
> 
> It may not be so easy determining the ambiguity.  Something like checking
> symbol_read_needs_frame() and then also checking if there exist >= 2 different
> frames containing the block.

I understand the argument here, but I'm not sure I can agree.  The
ambiguity you speak of already occurs with high frequency, after all,
since when I say
  
    print x

there may be many local x's lying around, both recursive instances of the same
definition or instances of unrelated definitions.  Programmers are
expected to understand this and already have a mechanism for specifying
which they mean (up/frame/down).  It is a very common situation
(especially for someone like me who is always playing around with
compilers in which a significant percentage of calls are recursive).  I
am inclined to think, therefore, that warnings would not be considered helpful.

> And the comment of this function is no longer valid then:
>
> /* Return the innermost stack frame executing inside of BLOCK, or NULL
>    if there is no such frame.  If BLOCK is NULL, just return NULL.  */
>
> struct frame_info *
> block_innermost_frame (const struct block *block)

Good point.  In fact, do you think we should change the function name?
The frame is no longer "innermost", after all.

Paul Hilfinger

Re: [RFA] Have block_innermost_frame start from selected frame

[Joel Brobecker]

> I find that GDB should ask or at least warn more in general.

OK, I see why you are suggesting queries and menus, now.

I think that generally speaking, we're trying to be less verbose,
to make sure that any important message does not get drowned as
you explain.  Similarly, I would like us to limit the number of
queries and menus to the minimum as well. That's why the
multiple-symbols setting is set to "all" rather than "ask", for
instance. I realize it's a matter of opinion, and people can
easily disagree. No problem.

But I think we can easily do without the query here. As I said,
the old behavior can be reproduced. The new behavior is more versatile.
And, to me at least, and the few at AdaCore who discussed this,
we all concluded that the new behavior made more sense than the old.

> > It's the only way to get the value of "var" in our testcase, and you
> > cannot currently do it with the old behavior.
>
> You can already do so many things with GDB, just people do not do even
> 5% of them because it is all too magic to learn.

You are probably right, but following your logic, we should dumb GDB
down. Many times, that's a good thing, I agree. But I don't think
that's always the case. And I do think that it goes against general
usability in our situation.

-- 
Joel

Re: [RFA] Have block_innermost_frame start from selected frame

[Jan Kratochvil]

On Wed, 28 Dec 2011 16:30:08 +0100, Joel Brobecker wrote:
> What do you mean by ambiguous? Is it the case described here, where
> we have multiple frames for the given FUNCTION?

Yes.  Depending on which recursive function frame you select the returned
value is different.


> I don't think we should query().

In fact we should menu-select them but that is offtopic now.


> Users often respond negatively to query, particularly in a case like this
> where we can make the behavior unambiguous and easily describable.

I find that GDB should ask or at least warn more in general.  Like that if you
debug -O2 code it should warn you on first `step' that it will not work much.
If you debug program/function without -g it should warn you (not just "no
debugging symbols found" lost in the screens of "Reading symbols from "
messages).  It would prevent false accuses of GDB due to bugs elsewhere.

GIT gives nice messages suggesting what to do in any case of a problem.


> It's the only way to get the value of "var" in our testcase, and you cannot
> currently do it with the old behavior.

You can already do so many things with GDB, just people do not do even 5% of
them because it is all too magic to learn.


> I think simple is good enough, in this particular case.

I am not going to try to block it but it is still the current GDB style closed
to hackers.


Thanks,
Jan

Re: [RFA] Have block_innermost_frame start from selected frame

[Joel Brobecker]

> refrenced doc:
> 	If you wish, you can specify a static variable in a particular
> 	function or file, using the colon-colon (`::') notation:
> 		FILE::VARIABLE
> 		FUNCTION::VARIABLE
> 
> In such case the doc should be updated, particularly that it has became now
> related to the currently selected frame.

Good idea. (and thanks for catching the need to update the
funtion description comment).

> > This change causes block_innermost_frame to start looking from the selected
> > frame, if there is one.
> 
> It may be all even more tricky than it was before.  What about using
> query() if such reference is ambiguous?

What do you mean by ambiguous? Is it the case described here, where
we have multiple frames for the given FUNCTION?

I don't think we should query(). Users often respond negatively
to query, particularly in a case like this where we can make
the behavior unambiguous and easily describable.

I think one reason why I like Paul's choice is the fact that his
suggestion opens the expression to something that is more useful
than the original behavior. With the new behavior, you can get
the old behavior by simply switching back to frame 0, and then
querying FUNCTION::VARIABLE. But the new behavior introduces
the possibility of getting the value of a FUNCTION::VARIABLE inside
another frame by simply selecting that frame. It's the only way
to get the value of "var" in our testcase, and you cannot currently
do it with the old behavior.

> It may not be so easy determining the ambiguity.  Something like
> checking symbol_read_needs_frame() and then also checking if there
> exist >= 2 different frames containing the block.

I think simple is good enough, in this particular case.

-- 
Joel

Re: [RFA] Have block_innermost_frame start from selected frame

[Jan Kratochvil]

On Tue, 27 Dec 2011 20:58:09 +0100, Paul Hilfinger wrote:
> --- a/gdb/blockframe.c
> +++ b/gdb/blockframe.c
> @@ -370,7 +370,9 @@ block_innermost_frame (const struct block *block)
>    start = BLOCK_START (block);
>    end = BLOCK_END (block);
>  
> -  frame = get_current_frame ();
> +  frame = get_selected_frame_if_set ();
> +  if (frame == NULL)
> +    frame = get_current_frame ();

And the comment of this function is no longer valid then:

/* Return the innermost stack frame executing inside of BLOCK, or NULL
   if there is no such frame.  If BLOCK is NULL, just return NULL.  */

struct frame_info *
block_innermost_frame (const struct block *block)


Regards,
Jan

Re: [RFA] Have block_innermost_frame start from selected frame

[Jan Kratochvil]

On Tue, 27 Dec 2011 20:58:09 +0100, Paul Hilfinger wrote:
> The GDB documentation suggests that the notation FOO::x is intended for static
> variables, but in fact it also "works" for local variables as well.

refrenced doc:
	If you wish, you can specify a static variable in a particular
	function or file, using the colon-colon (`::') notation:
		FILE::VARIABLE
		FUNCTION::VARIABLE

In such case the doc should be updated, particularly that it has became now
related to the currently selected frame.


> This change causes block_innermost_frame to start looking from the selected
> frame, if there is one.

It may be all even more tricky than it was before.  What about using query()
if such reference is ambiguous?

It may not be so easy determining the ambiguity.  Something like checking
symbol_read_needs_frame() and then also checking if there exist >= 2 different
frames containing the block.


Thanks,
Jan

[RFA] Have block_innermost_frame start from selected frame

[Paul Hilfinger]

The GDB documentation suggests that the notation FOO::x is intended for static
variables, but in fact it also "works" for local variables as well.  For
various reasons, we tend to do this quite a bit in Ada, but the issue
raised here comes up in C as well (the message following this one contains a
proposed test case in C).  The change proposed here causes no regressions, but
the obvious question is why things currently work as they do (FOO::x ignores
the selected frame).  Is this because (as we hope) nobody else uses FOO:: for
referencing local variables?

Previously, GDB would search for the frame containing variables in a
particular lexical block starting from the current (top) frame,
ignoring any currently selected frame.  It is not clear why this is
desirable for variables that require a frame; why would a user
deliberately select one frame and then expect to see the value of a
variable in a more recent frame?  This change causes
block_innermost_frame to start looking from the selected frame, if
there is one.

This change is perhaps unnecessarily conservative.  It uses
get_selected_frame_if_set rather than get_selected_frame in order to
avoid the side effect of calling select_frame, which would probably be
harmless.

Paul N. Hilfinger


2011-12-27  Paul Hilfinger  <hilfingr@adacore.com>

* gdb/blockframe.c (block_innermost_frame): Start search from selected from,
    if present, or otherwise the current frame.
---
 gdb/ChangeLog    |    5 +++++
 gdb/blockframe.c |    4 +++-
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 87031a6..39bc63a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2011-12-27  Paul Hilfinger  <hilfingr@adacore.com>
+
+        * blockframe.c (block_innermost_frame): Start search from 
+        selected from,if present, or otherwise the current frame.
+
 2011-12-27  Joel Brobecker  <brobecker@adacore.com>
 
 	* ada-lang.c (should_use_wild_match): New function.
diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index ef53a3d..92271ff 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -370,7 +370,9 @@ block_innermost_frame (const struct block *block)
   start = BLOCK_START (block);
   end = BLOCK_END (block);
 
-  frame = get_current_frame ();
+  frame = get_selected_frame_if_set ();
+  if (frame == NULL)
+    frame = get_current_frame ();
   while (frame != NULL)
     {
       struct block *frame_block = get_frame_block (frame, NULL);
-- 
1.7.0.4