[patch] MI: Error out on -var-create invalid FRAME-ADDR

[patch] MI: Error out on -var-create invalid FRAME-ADDR

[Jan Kratochvil]

Hi,

-var-create could create VAROBJ with uninitialized thread_id leading to
unexpected processing.

The frame parameter of -var-create is FRAME-ADDR, one could expect it is
FRAME-ID.  It is even commented in the source:
	    /* FIXME: cagney/2002-11-23: This code should be doing a
	       lookup using the frame ID and not just the frame's
	       ``address''.  This, of course, means an interface
	       change.  However, with out that interface change ISAs,
	       such as the ia64 with its two stacks, won't work.
	       Similar goes for the case where there is a frameless
	       function.  */
	    fi = find_frame_addr_in_frame_chain (frame);

At least error out when the frame is not found AND it harms.

Therefore the original command Dodji bugreporting it:
	-var-create var2 0 variable
should have been instead:
	-var-create --thread 1 --frame 0 var2 * variable

Regression tested on {x86_64,i686}-fedora11-linux-gnu.

find_frame_addr_in_frame_chain already has an explicit check for 0 so 0
currently can never be a valid frame address on any arch:
  if (frame_addr == (CORE_ADDR) 0)
    return NULL;


Thanks,
Jan


gdb/
2009-08-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* varobj.c (varobj_create): Error if the specified frame was not found
	and it is needed.

gdb/testsuite/
2009-08-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.mi/mi2-var-cmd.exp (create variable with invalid FRAME-ADDR): New.

--- a/gdb/testsuite/gdb.mi/mi2-var-cmd.exp
+++ b/gdb/testsuite/gdb.mi/mi2-var-cmd.exp
@@ -126,6 +126,11 @@ mi_gdb_test "-var-create int * int" \
 	"&\"Attempt to use a type name as an expression.\\\\n\".*\\^error,msg=\"mi_cmd_var_create: unable to create variable object\"" \
 	"create int"
 
+# The number 0 must be an invalid frame address and linteger a local variable.
+mi_gdb_test "-var-create invalidframe 0 linteger" \
+	"&\"Failed to find the specified frame.\\\\n\".*\\^error,msg=\"mi_cmd_var_create: unable to create variable object\"" \
+	"create variable with invalid FRAME-ADDR"
+
 
 #####             #####
 #                     #
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -565,8 +567,19 @@ varobj_create (char *objname,
          we must select the appropriate frame before parsing
          the expression, otherwise the value will not be current.
          Since select_frame is so benign, just call it for all cases. */
-      if (innermost_block && fi != NULL)
+      if (innermost_block)
 	{
+	  /* User could specify explicit FRAME-ADDR which was not found but
+	     EXPRESSION is frame specific and we would not be able to evaluate
+	     it correctly next time.  With VALID_BLOCK set we must also set
+	     FRAME and THREAD_ID.  */
+	  if (fi == NULL)
+	    {
+	      fprintf_unfiltered (gdb_stderr, "Failed to find the specified"
+				  " frame.\n");
+	      return NULL;
+	    }
+
 	  var->root->frame = get_frame_id (fi);
 	  var->root->thread_id = pid_to_thread_id (inferior_ptid);
 	  old_fi = get_selected_frame (NULL);

Re: [patch] MI: Error out on -var-create invalid FRAME-ADDR

[Vladimir Prus]

Jan Kratochvil wrote:

> -      if (innermost_block && fi != NULL)
> +      if (innermost_block)
> {
> +         /* User could specify explicit FRAME-ADDR which was not found but
> +            EXPRESSION is frame specific and we would not be able to evaluate
> +            it correctly next time.  With VALID_BLOCK set we must also set
> +            FRAME and THREAD_ID.  */
> +         if (fi == NULL)
> +           {
> +             fprintf_unfiltered (gdb_stderr, "Failed to find the specified"
> +                                 " frame.\n");
> +             return NULL;
> +           }

Why is this fprintf + NULL return, as opposed to error?

Seems OK otherwise.

- Volodya

Re: [patch] MI: Error out on -var-create invalid FRAME-ADDR

[Jan Kratochvil]

On Mon, 10 Aug 2009 18:54:52 +0200, Vladimir Prus wrote:
> Jan Kratochvil wrote:
> 
> > -      if (innermost_block && fi != NULL)
> > +      if (innermost_block)
> > {
> > +         /* User could specify explicit FRAME-ADDR which was not found but
> > +            EXPRESSION is frame specific and we would not be able to evaluate
> > +            it correctly next time.  With VALID_BLOCK set we must also set
> > +            FRAME and THREAD_ID.  */
> > +         if (fi == NULL)
> > +           {
> > +             fprintf_unfiltered (gdb_stderr, "Failed to find the specified"
> > +                                 " frame.\n");
> > +             return NULL;
> > +           }
> 
> Why is this fprintf + NULL return, as opposed to error?

I had there error before but changed it to match the code several lines above:

      /* Don't allow variables to be created for types. */
      if (var->root->exp->elts[0].opcode == OP_TYPE)
	{
	  do_cleanups (old_chain);
	  fprintf_unfiltered (gdb_stderr, "Attempt to use a type name"
			      " as an expression.\n");
	  return NULL;
	}
+
mi_gdb_test "-var-create int * int" \
	"&\"Attempt to use a type name as an expression.\\\\n\".*\\^error,msg=\"mi_cmd_var_create: unable to create variable object\"" \
	"create int"

Expecting this former case cannot be changed due to MI2 compatibility but I am
free to change the new error "Failed to find the specified frame." to use
error().  Check it in with or without error()?


Thanks,
Jan

Re: [patch] MI: Error out on -var-create invalid FRAME-ADDR

[Vladimir Prus]

On Monday 10 August 2009 Jan Kratochvil wrote:

> On Mon, 10 Aug 2009 18:54:52 +0200, Vladimir Prus wrote:
> > Jan Kratochvil wrote:
> > 
> > > -      if (innermost_block && fi != NULL)
> > > +      if (innermost_block)
> > > {
> > > +         /* User could specify explicit FRAME-ADDR which was not found but
> > > +            EXPRESSION is frame specific and we would not be able to evaluate
> > > +            it correctly next time.  With VALID_BLOCK set we must also set
> > > +            FRAME and THREAD_ID.  */
> > > +         if (fi == NULL)
> > > +           {
> > > +             fprintf_unfiltered (gdb_stderr, "Failed to find the specified"
> > > +                                 " frame.\n");
> > > +             return NULL;
> > > +           }
> > 
> > Why is this fprintf + NULL return, as opposed to error?
> 
> I had there error before but changed it to match the code several lines above:
> 
>       /* Don't allow variables to be created for types. */
>       if (var->root->exp->elts[0].opcode == OP_TYPE)
> 	{
> 	  do_cleanups (old_chain);
> 	  fprintf_unfiltered (gdb_stderr, "Attempt to use a type name"
> 			      " as an expression.\n");
> 	  return NULL;
> 	}
> +
> mi_gdb_test "-var-create int * int" \
> 	"&\"Attempt to use a type name as an expression.\\\\n\".*\\^error,msg=\"mi_cmd_var_create: unable to create variable object\"" \
> 	"create int"
> 
> Expecting this former case cannot be changed due to MI2 compatibility but I am
> free to change the new error "Failed to find the specified frame." to use
> error().  Check it in with or without error()?

I'd prefer error for all new code. Thanks!

- Volodya

Re: [patch] MI: Error out on -var-create invalid FRAME-ADDR

[Jan Kratochvil]

On Thu, 13 Aug 2009 07:42:54 +0200, Vladimir Prus wrote:
> I'd prefer error for all new code. Thanks!

Attached, OK to check-in?

Regression-tested on {x86_64,i686}-fedora11-linux-gnu.


Thanks,
Jan


gdb/
2009-08-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* varobj.c (varobj_create): Error if the specified frame was not found
	and it is needed.

gdb/testsuite/
2009-08-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.mi/mi2-var-cmd.exp (create variable with invalid FRAME-ADDR): New.

--- ./gdb/varobj.c	14 Aug 2009 00:32:32 -0000	1.146
+++ ./gdb/varobj.c	25 Aug 2009 14:25:48 -0000
@@ -565,8 +565,15 @@ varobj_create (char *objname,
          we must select the appropriate frame before parsing
          the expression, otherwise the value will not be current.
          Since select_frame is so benign, just call it for all cases. */
-      if (innermost_block && fi != NULL)
+      if (innermost_block)
 	{
+	  /* User could specify explicit FRAME-ADDR which was not found but
+	     EXPRESSION is frame specific and we would not be able to evaluate
+	     it correctly next time.  With VALID_BLOCK set we must also set
+	     FRAME and THREAD_ID.  */
+	  if (fi == NULL)
+	    error (_("Failed to find the specified frame"));
+
 	  var->root->frame = get_frame_id (fi);
 	  var->root->thread_id = pid_to_thread_id (inferior_ptid);
 	  old_fi = get_selected_frame (NULL);
--- ./gdb/testsuite/gdb.mi/mi2-var-cmd.exp	7 Jul 2009 09:25:38 -0000	1.23
+++ ./gdb/testsuite/gdb.mi/mi2-var-cmd.exp	25 Aug 2009 14:25:50 -0000
@@ -126,6 +126,11 @@ mi_gdb_test "-var-create int * int" \
 	"&\"Attempt to use a type name as an expression.\\\\n\".*\\^error,msg=\"mi_cmd_var_create: unable to create variable object\"" \
 	"create int"
 
+# The number 0 must be an invalid frame address and linteger a local variable.
+mi_gdb_test "-var-create invalidframe 0 linteger" \
+	"\\^error,msg=\"Failed to find the specified frame\"" \
+	"create variable with invalid FRAME-ADDR"
+
 
 #####             #####
 #                     #

Re: [patch] MI: Error out on -var-create invalid FRAME-ADDR

[Vladimir Prus]

On Thursday 27 August 2009 Jan Kratochvil wrote:

> On Thu, 13 Aug 2009 07:42:54 +0200, Vladimir Prus wrote:
> > I'd prefer error for all new code. Thanks!
> 
> Attached, OK to check-in?
> 
> Regression-tested on {x86_64,i686}-fedora11-linux-gnu.

This looks fine, thanks you!

- Volodya

Re: [patch] MI: Error out on -var-create invalid FRAME-ADDR

[Jan Kratochvil]

On Thu, 27 Aug 2009 07:25:59 +0200, Vladimir Prus wrote:
> This looks fine, thanks you!

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2009-08/msg00133.html


Thanks,
Jan