[PATCH] Fix variable objects for references to pointers

[PATCH] Fix variable objects for references to pointers

[Nick Roberts]

This is a RFA for the patch below to fix variable objects for references to
pointers as previously discussed.  I've moved the changes into get_type_deref,
as suggested by Vladimir, so that they appear in one rather than four places.

Currently get_type_deref dereferences once for a reference or a pointer.  All
this patch does is dereference twice in the case of a references to a pointer.


-- 
Nick                                           http://www.inet.net.nz/~nickrob


2006-12-14  Nick Roberts  <nickrob@snap.net.nz>

	* varobj.c (get_type_deref): Fix variable objects for references to
	pointers.


*** varobj.c	09 Dec 2006 10:59:12 +1300	1.65
--- varobj.c	14 Dec 2006 17:51:46 +1300	
*************** get_type (struct varobj *var)
*** 1535,1541 ****
    return type;
  }
  
! /* This returns the type of the variable, dereferencing pointers, too. */
  static struct type *
  get_type_deref (struct varobj *var)
  {
--- 1535,1542 ----
    return type;
  }
  
! /* This returns the type of the variable, dereferencing references, pointers
!    and references to pointers, too. */
  static struct type *
  get_type_deref (struct varobj *var)
  {
*************** get_type_deref (struct varobj *var)
*** 1543,1551 ****
  
    type = get_type (var);
  
!   if (type != NULL && (TYPE_CODE (type) == TYPE_CODE_PTR
! 		       || TYPE_CODE (type) == TYPE_CODE_REF))
!     type = get_target_type (type);
  
    return type;
  }
--- 1544,1556 ----
  
    type = get_type (var);
  
!   if (type)
!     {
!       if (TYPE_CODE (type) == TYPE_CODE_REF)
! 	type = get_target_type (type);
!       if (TYPE_CODE (type) == TYPE_CODE_PTR)
! 	type = get_target_type (type);
!     }
  
    return type;
  }
*************** c_number_of_children (struct varobj *var
*** 1848,1856 ****
        break;
  
      case TYPE_CODE_PTR:
!       /* This is where things get compilcated. All pointers have one child.
           Except, of course, for struct and union ptr, which we automagically
!          dereference for the user and function ptrs, which have no children.
           We also don't dereference void* as we don't know what to show.
           We can show char* so we allow it to be dereferenced.  If you decide
           to test for it, please mind that a little magic is necessary to
--- 1853,1861 ----
        break;
  
      case TYPE_CODE_PTR:
!       /* This is where things get complicated. All pointers have one child.
           Except, of course, for struct and union ptr, which we automagically
!          dereference for the user, and function ptrs, which have no children.
           We also don't dereference void* as we don't know what to show.
           We can show char* so we allow it to be dereferenced.  If you decide
           to test for it, please mind that a little magic is necessary to

Re: [PATCH] Fix variable objects for references to pointers

[Vladimir Prus]

Nick Roberts wrote:

> This is a RFA for the patch below to fix variable objects for references
> to pointers as previously discussed.  I've moved the changes into
> get_type_deref, as suggested by Vladimir, so that they appear in one
> rather than four places.
> 
> Currently get_type_deref dereferences once for a reference or a pointer. 
> All this patch does is dereference twice in the case of a references to a
> pointer.

Is there are testcase for this?

- Volodya

Re: [PATCH] Fix variable objects for references to pointers

[Nick Roberts]

 > Is there are testcase for this?

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2006-12-14  Nick Roberts  <nickrob@snap.net.nz>

	* gdb.mi/mi-var-cp.exp: New test for references to pointers.

	* gdb.mi/mi-var-cp.cc: Remove unnecessary string quotes.
	 (reference_to_pointer): New procedure for above test.


*** mi-var-cp.exp	09 Dec 2006 10:52:03 +1300	1.2
--- mi-var-cp.exp	14 Dec 2006 22:45:31 +1300	
*************** mi_gdb_load ${binfile}
*** 43,54 ****
  
  mi_runto reference_update_tests
  
! mi_create_varobj "RX" "rx" "create varobj for rx"
  
  set x_assignment [gdb_get_line_number "x = 567;"]
! mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment-1] \
      "step to x assignment"
! mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment] \
      "step to x assignment"
  
  mi_varobj_update RX {RX} "update RX (1)"
--- 43,54 ----
  
  mi_runto reference_update_tests
  
! mi_create_varobj RX rx "create varobj for rx"
  
  set x_assignment [gdb_get_line_number "x = 567;"]
! mi_next_to reference_update_tests {} ".*${srcfile}" [expr $x_assignment-1] \
      "step to x assignment"
! mi_next_to reference_update_tests {} ".*${srcfile}" [expr $x_assignment] \
      "step to x assignment"
  
  mi_varobj_update RX {RX} "update RX (1)"
*************** mi_varobj_update RX {RX} "update RX (1)"
*** 56,91 ****
  mi_check_varobj_value RX 167 "check RX: expect 167"
  
  # Execute the first 'x = 567' line.
! mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment+1] \
      "step to x assignment"
  
  mi_varobj_update RX {RX} "update RX (2)"
  mi_check_varobj_value RX 567 "check RX: expect 567"
  
  # Execute the second 'x = 567' line.
! mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment+2] \
      "step to x assignment"
  
  mi_varobj_update RX {} "update RX (3)"
  
  mi_runto base_in_reference_test
  
! mi_create_varobj "S2" "s2" "create varobj for s2"
  
! mi_list_varobj_children "S2" {{"S2.S" "S" "1" "S"}} "list children of s2"
  
! mi_list_varobj_children "S2.S" {{"S2.S.public" "public" "2"}} \
      "list children of s2.s"
  
! mi_list_varobj_children "S2.S.public"\
  {
!     {"S2.S.public.i" "i" "0" "int"}
!     {"S2.S.public.j" "j" "0" "int"}
  } "list children of s2.s.public"
  
! mi_check_varobj_value "S2.S.public.i" "67" "check S2.S.public.i"
! mi_check_varobj_value "S2.S.public.j" "89" "check S2.S.public.j"
  
  
  mi_gdb_exit
  return 0
--- 56,113 ----
  mi_check_varobj_value RX 167 "check RX: expect 167"
  
  # Execute the first 'x = 567' line.
! mi_next_to reference_update_tests {} ".*${srcfile}" [expr $x_assignment+1] \
      "step to x assignment"
  
  mi_varobj_update RX {RX} "update RX (2)"
  mi_check_varobj_value RX 567 "check RX: expect 567"
  
  # Execute the second 'x = 567' line.
! mi_next_to reference_update_tests {} ".*${srcfile}" [expr $x_assignment+2] \
      "step to x assignment"
  
  mi_varobj_update RX {} "update RX (3)"
  
  mi_runto base_in_reference_test
  
! mi_create_varobj S2 s2 "create varobj for s2"
  
! mi_list_varobj_children S2 {{S2.S S 1 S}} "list children of s2"
  
! mi_list_varobj_children S2.S {{S2.S.public public 2}} \
      "list children of s2.s"
  
! mi_list_varobj_children S2.S.public\
  {
!     {S2.S.public.i i 0 int}
!     {S2.S.public.j j 0 int}
  } "list children of s2.s.public"
  
! mi_check_varobj_value S2.S.public.i 67 "check S2.S.public.i"
! mi_check_varobj_value S2.S.public.j 89 "check S2.S.public.j"
  
+ mi_runto reference_to_pointer
+ 
+ set end_of_proc [gdb_get_line_number "return 99;"]
+ send_gdb "-exec-next 4\n"
+ gdb_expect {
+     -re "\\^running\r\n${mi_gdb_prompt}\\*stopped,reason=\"end-stepping-range\",thread-id=\"\[01\]\",frame=\{addr=\"$hex\",func=\"reference_to_pointer\",args=\\\[\\\],file=\".*${srcfile}\",fullname=\"${fullname_syntax}${srcfile}\",line=\"$end_of_proc\"\}\r\n$mi_gdb_prompt$" {
+ 	pass "4xnext to return 99"
+     }
+     timeout { fail "4xnext in reference_to_pointer (timeout)" }
+ }
+ 
+ mi_create_varobj RPTR rptr_s "create varobj for rptr_s"
+ 
+ mi_list_varobj_children RPTR {{RPTR.public public 2}} \
+     "list public child of RPTR"
+ 
+ mi_list_varobj_children  RPTR.public \
+     {{RPTR.public.i i 0 int}
+      {RPTR.public.j j 0 int}} "list children of reference to pointer"
+ 
+ mi_check_varobj_value RPTR.public.i 67 "check i member"
+ mi_check_varobj_value RPTR.public.j 89 "check j member"
  
  mi_gdb_exit
  return 0

Re: [PATCH] Fix variable objects for references to pointers

[Vladimir Prus]

Nick Roberts wrote:
 
> 2006-12-14  Nick Roberts  <nickrob@snap.net.nz>
> 
> * gdb.mi/mi-var-cp.exp: New test for references to pointers.
> 
> * gdb.mi/mi-var-cp.cc: Remove unnecessary string quotes.

This comment likely applies to .exp file, not .cc.  Besides, 
it would be better not to mix style changes with essential changes,
to simplify review.

Ah, and I've posted a patch to move most of .exp into .cc file,
so I guess we've mid-air collision.

> (reference_to_pointer): New procedure for above test.

Am I missing something, or this change is not included in
your mail? Ah, I think it's the CVS problem, whereas added
files are not included in diff.


> + set end_of_proc [gdb_get_line_number "return 99;"]
> + send_gdb "-exec-next 4\n"
> + gdb_expect {
> +     -re
> "\\^running\r\n${mi_gdb_prompt}\\*stopped,reason=\"end-stepping-range\",thread-id=\"\[01\]\",frame=\{addr=\"$hex\",func=\"reference_to_pointer\",args=\\\[\\\],file=\".*${srcfile}\",fullname=\"${fullname_syntax}${srcfile}\",line=\"$end_of_proc\"\}\r\n$mi_gdb_prompt$"
> { +       pass "4xnext to return 99" +     }
> +     timeout { fail "4xnext in reference_to_pointer (timeout)" }
> + }

FWIW, once my "Simplified MI tests" patch is checked in, this block
can be just removed.

> +
> + mi_create_varobj RPTR rptr_s "create varobj for rptr_s"
> +
> + mi_list_varobj_children RPTR {{RPTR.public public 2}} \
> +     "list public child of RPTR"
> +
> + mi_list_varobj_children  RPTR.public \
> +     {{RPTR.public.i i 0 int}
> +      {RPTR.public.j j 0 int}} "list children of reference to pointer"
> +
> + mi_check_varobj_value RPTR.public.i 67 "check i member"
> + mi_check_varobj_value RPTR.public.j 89 "check j member"

Looks good. I've poked at this manually and it seems to work like
it should.

Now I guess you need to post a patch including the C++ file changes,
now that it's in CVS, and we need to find somebody who can actually
approve the patch.

Thanks,
Volodya

Re: [PATCH] Fix variable objects for references to pointers

[Nick Roberts]

 > > 2006-12-14  Nick Roberts  <nickrob@snap.net.nz>
 > > 
 > > * gdb.mi/mi-var-cp.exp: New test for references to pointers.
 > > 
 > > * gdb.mi/mi-var-cp.cc: Remove unnecessary string quotes.
 > 
 > This comment likely applies to .exp file, not .cc.  

Yes.

 >                                                     Besides, 
 > it would be better not to mix style changes with essential changes,
 > to simplify review.

I was trying to be consistent.  Previously some arguments were in string
quotes and some weren't.

 > Ah, and I've posted a patch to move most of .exp into .cc file,
 > so I guess we've mid-air collision.
 > 
 > > (reference_to_pointer): New procedure for above test.
 > 
 > Am I missing something, or this change is not included in
 > your mail? Ah, I think it's the CVS problem, whereas added
 > files are not included in diff.

Sorry, I forgot include it.  Its attached below

 > > + set end_of_proc [gdb_get_line_number "return 99;"]
 > > + send_gdb "-exec-next 4\n"
 > > + gdb_expect {
 > > +     -re
 > > "\\^running\r\n${mi_gdb_prompt}\\*stopped,reason=\"end-stepping-range\",thread-id=\"\[01\]\",frame=\{addr=\"$hex\",func=\"reference_to_pointer\",args=\\\[\\\],file=\".*${srcfile}\",fullname=\"${fullname_syntax}${srcfile}\",line=\"$end_of_proc\"\}\r\n$mi_gdb_prompt$"
 > > { +       pass "4xnext to return 99" +     }
 > > +     timeout { fail "4xnext in reference_to_pointer (timeout)" }
 > > + }
 > 
 > FWIW, once my "Simplified MI tests" patch is checked in, this block
 > can be just removed.

Sure.

 > > +
 > > + mi_create_varobj RPTR rptr_s "create varobj for rptr_s"
 > > +
 > > + mi_list_varobj_children RPTR {{RPTR.public public 2}} \
 > > +     "list public child of RPTR"
 > > +
 > > + mi_list_varobj_children  RPTR.public \
 > > +     {{RPTR.public.i i 0 int}
 > > +      {RPTR.public.j j 0 int}} "list children of reference to pointer"
 > > +
 > > + mi_check_varobj_value RPTR.public.i 67 "check i member"
 > > + mi_check_varobj_value RPTR.public.j 89 "check j member"
 > 
 > Looks good. I've poked at this manually and it seems to work like
 > it should.
 > 
 > Now I guess you need to post a patch including the C++ file changes,
 > now that it's in CVS, and we need to find somebody who can actually
 > approve the patch.

I'm pursuing this issue.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


*** mi-var-cp.cc	09 Dec 2006 10:52:03 +1300	1.2
--- mi-var-cp.cc	14 Dec 2006 21:13:21 +1300	
*************** void base_in_reference_test_main ()
*** 39,48 ****
--- 39,57 ----
    base_in_reference_test (s);
  }
  
+ int reference_to_pointer ()
+ {
+   S s, *ptr_s, *& rptr_s = ptr_s;
+   s.i = 67;
+   s.j = 89;
+   ptr_s = &s;
+   return 99;
+ }
  
  int main ()
  {
    reference_update_tests ();
    base_in_reference_test_main ();
+   reference_to_pointer ();
    return 0;
  }

Re: [PATCH] Fix variable objects for references to pointers

[Daniel Jacobowitz]

On Thu, Dec 14, 2006 at 05:53:21PM +1300, Nick Roberts wrote:
> 2006-12-14  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* varobj.c (get_type_deref): Fix variable objects for references to
> 	pointers.

This is OK.  I'll review the testcase next.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Fix variable objects for references to pointers

[Daniel Jacobowitz]

On Thu, Dec 14, 2006 at 10:48:34PM +1300, Nick Roberts wrote:
> + mi_runto reference_to_pointer
> + 
> + set end_of_proc [gdb_get_line_number "return 99;"]
> + send_gdb "-exec-next 4\n"
> + gdb_expect {
> +     -re "\\^running\r\n${mi_gdb_prompt}\\*stopped,reason=\"end-stepping-range\",thread-id=\"\[01\]\",frame=\{addr=\"$hex\",func=\"reference_to_pointer\",args=\\\[\\\],file=\".*${srcfile}\",fullname=\"${fullname_syntax}${srcfile}\",line=\"$end_of_proc\"\}\r\n$mi_gdb_prompt$" {
> + 	pass "4xnext to return 99"
> +     }
> +     timeout { fail "4xnext in reference_to_pointer (timeout)" }
> + }

Using send_gdb / gdb_expect this way is a problem.  If anything goes
wrong, then the test will sit until it times out.  There's only
one pattern, so you could use mi_gdb_test.  "-exec-next 4"
isn't great either; compiler changes or test changes can easily
make you end up somewhere unexpected.

This line isn't testing anything you're interested in, right?
Then you can just put a breakpoint in the right place and run
to that breakpoint.  I'd recommend mi_continue_to.  I don't
see a convenient helper in mi-support.exp to set the breakpoint, but
there's an example in mi-var-cmd.exp.

(Hardcoding the breakpoint number that way isn't great either
but I'll clean that up another day.)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Fix variable objects for references to pointers

[Nick Roberts]

 > > + set end_of_proc [gdb_get_line_number "return 99;"]
 > > + send_gdb "-exec-next 4\n"
 > > + gdb_expect {
 > > +     -re "\\^running\r\n${mi_gdb_prompt}\\*stopped,reason=\"end-stepping-range\",thread-id=\"\[01\]\",frame=\{addr=\"$hex\",func=\"reference_to_pointer\",args=\\\[\\\],file=\".*${srcfile}\",fullname=\"${fullname_syntax}${srcfile}\",line=\"$end_of_proc\"\}\r\n$mi_gdb_prompt$" {
 > > + 	pass "4xnext to return 99"
 > > +     }
 > > +     timeout { fail "4xnext in reference_to_pointer (timeout)" }
 > > + }
 > 
 > Using send_gdb / gdb_expect this way is a problem.  If anything goes
 > wrong, then the test will sit until it times out.  There's only
 > one pattern, so you could use mi_gdb_test.  "-exec-next 4"
 > isn't great either; compiler changes or test changes can easily
 > make you end up somewhere unexpected.
 > 
 > This line isn't testing anything you're interested in, right?
 > Then you can just put a breakpoint in the right place and run
 > to that breakpoint.  I'd recommend mi_continue_to.  I don't
 > see a convenient helper in mi-support.exp to set the breakpoint, but
 > there's an example in mi-var-cmd.exp.

Something like below?

set end_of_proc [gdb_get_line_number "return 99;"]
send_gdb "-break-insert $end_of_proc\n"
mi_continue_to ".*" reference_to_pointer  "" ".*${srcfile}" $end_of_proc \
    "continue to return 99"


Other tests seem to use mi_gdb_test for -break-insert but I'm not interested in
the result and mi_continue presumably sifts through the output until it reaches
something it recognises.  I'm also not that interested in the breakpoint
number so I've used a wildcard for future-proofing.


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] Fix variable objects for references to pointers

[Vladimir Prus]

Nick Roberts wrote:

>  > This line isn't testing anything you're interested in, right?
>  > Then you can just put a breakpoint in the right place and run
>  > to that breakpoint.  I'd recommend mi_continue_to.  I don't
>  > see a convenient helper in mi-support.exp to set the breakpoint, but
>  > there's an example in mi-var-cmd.exp.
> 
> Something like below?
> 
> set end_of_proc [gdb_get_line_number "return 99;"]
> send_gdb "-break-insert $end_of_proc\n"
> mi_continue_to ".*" reference_to_pointer  "" ".*${srcfile}" $end_of_proc \
>     "continue to return 99"
> 
> 
> Other tests seem to use mi_gdb_test for -break-insert but I'm not
> interested in the result and mi_continue presumably sifts through the
> output until it reaches
> something it recognises.  I'm also not that interested in the breakpoint
> number so I've used a wildcard for future-proofing.

I don't think there's need to polish this fragment. The "Simplified MI testing"
patch of mine is almost approved, and introduces automatic stepping to the right
line in source.

Ah, and I think I'd need to modify that patch so that if you stop on
the wrong line we error out, and don't wait for timeout -- I think the 
current version of my patch has exactly this problem that Dan pointed out
earlier in this thread.

- Volodya

Re: [PATCH] Fix variable objects for references to pointers

[Daniel Jacobowitz]

On Thu, Jan 04, 2007 at 10:41:56PM +1300, Nick Roberts wrote:
> Something like below?
> 
> set end_of_proc [gdb_get_line_number "return 99;"]
> send_gdb "-break-insert $end_of_proc\n"
> mi_continue_to ".*" reference_to_pointer  "" ".*${srcfile}" $end_of_proc \
>     "continue to return 99"
> 
> 
> Other tests seem to use mi_gdb_test for -break-insert but I'm not interested in
> the result and mi_continue presumably sifts through the output until it reaches
> something it recognises.  I'm also not that interested in the breakpoint
> number so I've used a wildcard for future-proofing.

Let's hold off on this for a little bit and use Vlad's new stuff.

The above is better, except you really should use mi_gdb_test even if
you don't care about the result.  It's important that every time you
send a command to GDB that produces output, you also consume that
output.  Otherwise, you can get this behavior:

-> -break-insert $end_of_proc
-> -exec-continue
<- ^done
<- (gdb)
  Oops, that looks like the default case, something must have gone
  wrong with -exec-continue.  FAIL.
<- ^running
<- ^stopped
<- (gdb)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Fix variable objects for references to pointers

[Nick Roberts]

 > Let's hold off on this for a little bit and use Vlad's new stuff.

Here are the tests using the new stuff.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2007-01-05  Nick Roberts  <nickrob@snap.net.nz>

	* gdb.mi/mi-var-cp.exp: New test for references to pointers.

	* gdb.mi/mi-var-cp.cc: (reference_to_pointer): New procedure for above
	 test.


*** mi-var-cp.exp	05 Jan 2007 10:59:45 +1300	1.3
--- mi-var-cp.exp	05 Jan 2007 11:00:07 +1300	
*************** mi_prepare_inline_tests $srcfile
*** 43,48 ****
--- 43,49 ----
  
  mi_run_inline_test reference_update
  mi_run_inline_test base_in_reference
+ mi_run_inline_test reference_to_pointer
  
  mi_gdb_exit
  return 0


*** mi-var-cp.cc	05 Jan 2007 10:58:59 +1300	1.3
--- mi-var-cp.cc	05 Jan 2007 11:15:21 +1300	
*************** void base_in_reference_test_main ()
*** 70,79 ****
--- 70,103 ----
    base_in_reference_test (s);
  }
  
+ int reference_to_pointer ()
+ {
+   S s, *ptr_s, *& rptr_s = ptr_s;
+   s.i = 67;
+   s.j = 89;
+   ptr_s = &s;
+   /*: BEGIN: reference_to_pointer :*/
+   return 99;
+   /*: 
+     mi_create_varobj RPTR rptr_s "create varobj for rptr_s"
+ 
+     mi_list_varobj_children RPTR {{RPTR.public public 2}}	\
+     "list public child of RPTR"
+ 
+     mi_list_varobj_children  RPTR.public	\
+     {{RPTR.public.i i 0 int}
+     {RPTR.public.j j 0 int}} "list children of reference to pointer"
+ 
+     mi_check_varobj_value RPTR.public.i 67 "check i member"
+     mi_check_varobj_value RPTR.public.j 89 "check j member"
+   :*/
+   /*: END: reference_to_pointer :*/
+ }
  
  int main ()
  {
    reference_update_tests ();
    base_in_reference_test_main ();
+   reference_to_pointer ();
    return 0;
  }

Re: [PATCH] Fix variable objects for references to pointers

[Daniel Jacobowitz]

On Fri, Jan 05, 2007 at 11:30:30AM +1300, Nick Roberts wrote:
>  > Let's hold off on this for a little bit and use Vlad's new stuff.
> 
> Here are the tests using the new stuff.
> 
> -- 
> Nick                                           http://www.inet.net.nz/~nickrob
> 
> 
> 2007-01-05  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* gdb.mi/mi-var-cp.exp: New test for references to pointers.
> 
> 	* gdb.mi/mi-var-cp.cc: (reference_to_pointer): New procedure for above
> 	 test.

Thanks.  These are OK.

-- 
Daniel Jacobowitz
CodeSourcery