[PATCH] Fix mi "-var-create" regression

[PATCH] Fix mi "-var-create" regression

[Luis Gustavo]

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

Hi,

After the commit that fixed #13393 
(http://sourceware.org/bugzilla/show_bug.cgi?id=13393), certain calls to 
-var-create with "set print object on" started failing with a 
dereferencing error, like so:

(gdb)
-var-create - * $sp
^error,msg="Attempt to dereference a generic pointer."
(gdb)

This happens because in value.c:value_actual_type we have this check:

       if (TYPE_CODE (result) == TYPE_CODE_PTR
           || TYPE_CODE (result) == TYPE_CODE_REF)
         {
           struct type *real_type;

           real_type = value_rtti_indirect_type (value, NULL, NULL, NULL);
           if (real_type)
             {
               if (real_type_found)
                 *real_type_found = 1;
               result = real_type;
             }
         }

If we reach this point with result being a pointer of reference, we 
execute the if block. This will eventually lead to a call to 
valops.c:get_value_at, where we throw an error in case the target type 
is void (pointers usually have such a target type)

Anton does mention a particular problem with indirect pointers in #13393 
comment #8, but it is not clear if a bug ticket has been filed for that.

The following patch adds a check for target type void, and skips the 
problematic if block accordingly. This fixes the problem and does not 
seem to cause any regressions.

With "set print object off", everything works as it should.

Ok?

[-- Attachment #2: rtti-print-object.diff --]
[-- Type: text/x-patch, Size: 951 bytes --]

2012-10-10  Luis Machado  <lgustavo@codesourcery.com>

	* value.c (value_actual_type): Check for TYPE_CODE_VOID
	target types.

Index: gdb/gdb/value.c
===================================================================
--- gdb.orig/gdb/value.c	2012-10-10 16:38:21.872234906 -0300
+++ gdb/gdb/value.c	2012-10-10 16:42:49.560222099 -0300
@@ -850,8 +850,13 @@ value_actual_type (struct value *value,
   result = value_type (value);
   if (opts.objectprint)
     {
-      if (TYPE_CODE (result) == TYPE_CODE_PTR
+      /* If result's target type is TYPE_CODE_VOID, do not try fetching its rtti
+	 type.  GDB will try to dereference the void pointer and will throw an
+	 error when trying to do so.  */
+      if ((TYPE_CODE (result) == TYPE_CODE_PTR
 	  || TYPE_CODE (result) == TYPE_CODE_REF)
+	  && ((TYPE_TARGET_TYPE (result) != NULL)
+	      && TYPE_CODE (TYPE_TARGET_TYPE (result)) != TYPE_CODE_VOID))
         {
           struct type *real_type;
 

Re: [PATCH] Fix mi "-var-create" regression

[Sergio Durigan Junior]

On Wednesday, October 10 2012, Luis Gustavo wrote:

> Hi,

Hey Luis :-)

> 2012-10-10  Luis Machado  <lgustavo@codesourcery.com>
>
> 	* value.c (value_actual_type): Check for TYPE_CODE_VOID
> 	target types.
>
> Index: gdb/gdb/value.c
> ===================================================================
> --- gdb.orig/gdb/value.c	2012-10-10 16:38:21.872234906 -0300
> +++ gdb/gdb/value.c	2012-10-10 16:42:49.560222099 -0300
> @@ -850,8 +850,13 @@ value_actual_type (struct value *value,
>    result = value_type (value);
>    if (opts.objectprint)
>      {
> -      if (TYPE_CODE (result) == TYPE_CODE_PTR
> +      /* If result's target type is TYPE_CODE_VOID, do not try fetching its rtti
> +	 type.  GDB will try to dereference the void pointer and will throw an
> +	 error when trying to do so.  */
> +      if ((TYPE_CODE (result) == TYPE_CODE_PTR
>  	  || TYPE_CODE (result) == TYPE_CODE_REF)
> +	  && ((TYPE_TARGET_TYPE (result) != NULL)
> +	      && TYPE_CODE (TYPE_TARGET_TYPE (result)) != TYPE_CODE_VOID))
>          {
>            struct type *real_type;
>  

As far as I remember this code (thanks for the explanation BTW), the
patch looks fine by me (not a maintainer).  Just two things I noticed:

1) I believe you could remove some of the parentheses in the `if'
above.  Something like:

  if ((TYPE_CODE (result) == TYPE_CODE_PTR
       || TYPE_CODE (result) == TYPE_CODE_REF)
      && TYPE_TARGET_TYPE (result) != NULL
      && TYPE_CODE (TYPE_TARGET_TYPE (result)) != TYPE_CODE_VOID)

would have the same effect, right?

2) Would it be possible to provide a testcase for this issue?  Not sure
if it's really needed, but I guess it won't hurt :-).  Of course, if
some maintainer thinks it's useless, then please disconsider the idea
right away.

Thanks a lot,

-- 
Sergio

Re: [PATCH] Fix mi "-var-create" regression

[Luis Machado]

On 10/11/2012 06:53 PM, Sergio Durigan Junior wrote:
> On Wednesday, October 10 2012, Luis Gustavo wrote:
>
>> Hi,
>
> Hey Luis :-)

Hey!

> 1) I believe you could remove some of the parentheses in the `if'
> above.  Something like:
>
>    if ((TYPE_CODE (result) == TYPE_CODE_PTR
>         || TYPE_CODE (result) == TYPE_CODE_REF)
>        &&  TYPE_TARGET_TYPE (result) != NULL
>        &&  TYPE_CODE (TYPE_TARGET_TYPE (result)) != TYPE_CODE_VOID)
>
> would have the same effect, right?

Ah, of course. Thanks for noticing that.

>
> 2) Would it be possible to provide a testcase for this issue?  Not sure
> if it's really needed, but I guess it won't hurt :-).  Of course, if
> some maintainer thinks it's useless, then please disconsider the idea
> right away.

I thought about this for a while, and the best testing mechanism seems 
to be just turning the "print object" option on by default and running 
the entire testsuite, otherwise this test will be very specific to the 
-var-create mechanism.

value_actual_type is called from 3 different places IIRC.

If we're thinking about a regression coverage test, then i agree that a 
test is appropriate in this case, so we can be sure it will never happen 
again.

Luis

Re: [PATCH] Fix mi "-var-create" regression

[Tom Tromey]

>>>>> "Luis" == Luis Machado <lgustavo@codesourcery.com> writes:

Luis> I thought about this for a while, and the best testing mechanism seems
Luis> to be just turning the "print object" option on by default and running
Luis> the entire testsuite, otherwise this test will be very specific to the
Luis> -var-create mechanism.

I think you'd see a lot of failures doing this.

FWIW I think 'set print object on' should be the default.
It is a FAQ for C++ programmers.
However, I think this would require a lot of annoying test suite
hacking.

Luis> If we're thinking about a regression coverage test, then i agree that
Luis> a test is appropriate in this case, so we can be sure it will never
Luis> happen again.

I think a regression test is both necessary and sufficient.

Tom

RE: [PATCH] Fix mi "-var-create" regression

[Marc Khouzam]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Luis Gustavo
> Sent: Wednesday, October 10, 2012 4:05 PM
> To: gdb-patches@sourceware.org
> Subject: [PATCH] Fix mi "-var-create" regression
> 
> Hi,
> 
> After the commit that fixed #13393 
> (http://sourceware.org/bugzilla/show_bug.cgi?id=13393), 
> certain calls to 
> -var-create with "set print object on" started failing with a 
> dereferencing error, like so:
> 
> (gdb)
> -var-create - * $sp
> ^error,msg="Attempt to dereference a generic pointer."
> (gdb)

Thanks for proposing this fix.  I'm also seeing this error.
Eclipse sets "print object on" by default so I'm seeing it often.
When fixed, can the final patch also go into the 7_5 branch?

Thanks!

Marc

Re: [PATCH] Fix mi "-var-create" regression

[Joel Brobecker]

Hi Luis,

> 2012-10-10  Luis Machado  <lgustavo@codesourcery.com>
> 
> 	* value.c (value_actual_type): Check for TYPE_CODE_VOID
> 	target types.

I don't feel entirely comfortable approving this patch, not because
I think it's iffy, but just because I don't have a good knowledge
of what is going on for you (RTTI is C++, and I don't follow C++
support). If no one else has the time to officially review your patch
within the next week or so, I will take a deeper look. It does look
reasonable.

I did notice a couple of things (besides the unnecessary parens):

> -      if (TYPE_CODE (result) == TYPE_CODE_PTR
> +      /* If result's target type is TYPE_CODE_VOID, do not try fetching its rtti
> +	 type.  GDB will try to dereference the void pointer and will throw an
> +	 error when trying to do so.  */

Can you reformat the comment to fit within 70 characters. That's the
soft line limit...

> +      if ((TYPE_CODE (result) == TYPE_CODE_PTR
>  	  || TYPE_CODE (result) == TYPE_CODE_REF)
> +	  && ((TYPE_TARGET_TYPE (result) != NULL)
> +	      && TYPE_CODE (TYPE_TARGET_TYPE (result)) != TYPE_CODE_VOID))

I don't think that you need to test for the TYPE_TARGET_TYPE, since
you already know that it's either a PTR or REF type, which always
have a TARGET_TYPE.

-- 
Joel

Re: [PATCH] Fix mi "-var-create" regression

[Luis Machado]

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

On 10/14/2012 02:18 PM, Joel Brobecker wrote:
> I don't feel entirely comfortable approving this patch, not because
> I think it's iffy, but just because I don't have a good knowledge
> of what is going on for you (RTTI is C++, and I don't follow C++
> support). If no one else has the time to officially review your patch
> within the next week or so, I will take a deeper look. It does look
> reasonable.
>

The biggest problem caused by this regression is the breakage of 
information sent to IDEs based on MI output, as Marc pointed out.

Eclipse, for example, will attempt to list registers and gdb will end up 
throwing these errors, causing eclipse to get all confused when trying 
to display the register values.

This is more about a regression of a generic gdb feature than a fix to a 
C++ feature.

> I did notice a couple of things (besides the unnecessary parens):
>
>> -      if (TYPE_CODE (result) == TYPE_CODE_PTR
>> +      /* If result's target type is TYPE_CODE_VOID, do not try fetching its rtti
>> +	 type.  GDB will try to dereference the void pointer and will throw an
>> +	 error when trying to do so.  */
>
> Can you reformat the comment to fit within 70 characters. That's the
> soft line limit...
>

Fixed.

>> +      if ((TYPE_CODE (result) == TYPE_CODE_PTR
>>   	  || TYPE_CODE (result) == TYPE_CODE_REF)
>> +	&&  ((TYPE_TARGET_TYPE (result) != NULL)
>> +	&&  TYPE_CODE (TYPE_TARGET_TYPE (result)) != TYPE_CODE_VOID))
>
> I don't think that you need to test for the TYPE_TARGET_TYPE, since
> you already know that it's either a PTR or REF type, which always
> have a TARGET_TYPE.
>

Well spotted. I fixed this now.

Thanks,
Luis

[-- Attachment #2: rtti-print-object.diff --]
[-- Type: text/x-patch, Size: 905 bytes --]

2012-10-15  Luis Machado  <lgustavo@codesourcery.com>

	* value.c (value_actual_type): Check for TYPE_CODE_VOID
	target types.

Index: gdb/gdb/value.c
===================================================================
--- gdb.orig/gdb/value.c	2012-10-10 16:54:36.048188274 -0300
+++ gdb/gdb/value.c	2012-10-14 21:41:22.997797277 -0300
@@ -850,8 +850,13 @@ value_actual_type (struct value *value,
   result = value_type (value);
   if (opts.objectprint)
     {
-      if (TYPE_CODE (result) == TYPE_CODE_PTR
+      /* If result's target type is TYPE_CODE_VOID, do not try
+	 fetching its rtti type.  GDB will try to dereference
+	 the void pointer and will throw an error when trying to
+	 do so.  */
+      if ((TYPE_CODE (result) == TYPE_CODE_PTR
 	  || TYPE_CODE (result) == TYPE_CODE_REF)
+	  && TYPE_CODE (TYPE_TARGET_TYPE (result)) != TYPE_CODE_VOID)
         {
           struct type *real_type;
 

Re: [PATCH] Fix mi "-var-create" regression

[Tom Tromey]

>>>>> "Luis" == Luis Machado <lgustavo@codesourcery.com> writes:

Luis> +	  && TYPE_CODE (TYPE_TARGET_TYPE (result)) != TYPE_CODE_VOID)

On further reflection, I have a question and a comment.

First, perhaps instead of excluding void* here, we should only allow
pointers to structs.  That is the only case that has RTTI anyhow.  What
do you think?

Also, I think this needs a check_typedef around TYPE_TARGET_TYPE.

Tom

Re: [PATCH] Fix mi "-var-create" regression

[Luis Machado]

Hi Tom,

On 10/17/2012 10:13 PM, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado<lgustavo@codesourcery.com>  writes:
>
> Luis>  +	&&  TYPE_CODE (TYPE_TARGET_TYPE (result)) != TYPE_CODE_VOID)
>
> On further reflection, I have a question and a comment.
>
> First, perhaps instead of excluding void* here, we should only allow
> pointers to structs.  That is the only case that has RTTI anyhow.  What
> do you think?

That sounds reasonable to me.
>
> Also, I think this needs a check_typedef around TYPE_TARGET_TYPE.
>

Probably. If the target type is deep into a typedef pile, we won't get 
this right.

I'll have it fixed and will provide that testcase.

Thanks!
Luis

Re: [PATCH] Fix mi "-var-create" regression

[Luis Machado]

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

Hi Tom,

On 10/17/2012 10:13 PM, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado<lgustavo@codesourcery.com>  writes:
>
> Luis>  +	&&  TYPE_CODE (TYPE_TARGET_TYPE (result)) != TYPE_CODE_VOID)
>
> On further reflection, I have a question and a comment.
>
> First, perhaps instead of excluding void* here, we should only allow
> pointers to structs.  That is the only case that has RTTI anyhow.  What
> do you think?
>
> Also, I think this needs a check_typedef around TYPE_TARGET_TYPE.
>
> Tom

Thanks for the comments. I've addressed them in this new version, now 
with a testcase.

Hopefully the testcase is well-formed.

Thanks,
Luis

[-- Attachment #2: rtti-print-object.diff --]
[-- Type: text/x-patch, Size: 3891 bytes --]

2012-10-15  Luis Machado  <lgustavo@codesourcery.com>

	* value.c (value_actual_type): Check for TYPE_CODE_STRUCT
	target types.

	testsuite/
	* mi-var-create-rtti.c: New file.
	* mi-var-create-rtti.exp: New file.

Index: gdb/gdb/value.c
===================================================================
--- gdb.orig/gdb/value.c	2012-10-28 20:22:13.209235579 -0200
+++ gdb/gdb/value.c	2012-10-28 20:27:07.857218717 -0200
@@ -850,8 +850,12 @@ value_actual_type (struct value *value,
   result = value_type (value);
   if (opts.objectprint)
     {
-      if (TYPE_CODE (result) == TYPE_CODE_PTR
+      /* If result's target type is TYPE_CODE_STRUCT, proceed to
+	 fetch its rtti type.  */
+      if ((TYPE_CODE (result) == TYPE_CODE_PTR
 	  || TYPE_CODE (result) == TYPE_CODE_REF)
+	  && TYPE_CODE (check_typedef (TYPE_TARGET_TYPE (result)))
+	     == TYPE_CODE_STRUCT)
         {
           struct type *real_type;
 
Index: gdb/gdb/testsuite/gdb.mi/mi-var-create-rtti.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb/gdb/testsuite/gdb.mi/mi-var-create-rtti.c	2012-10-28 19:27:03.241424995 -0200
@@ -0,0 +1,24 @@
+/* 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
+main (void)
+{
+  int i = 0;
+
+  return i; /* next-line */
+}
Index: gdb/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp	2012-10-28 20:21:34.737237782 -0200
@@ -0,0 +1,52 @@
+# 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/>.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+standard_testfile .c
+set opts {debug}
+
+if [build_executable $testfile.exp $testfile $srcfile $opts] {
+    return -1;
+}
+
+mi_delete_breakpoints
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_load ${binfile}
+
+if ![mi_run_to_main] {
+    untested "could not run to main"
+    return -1
+}
+
+# Test creating a register-based variable.  We pick
+# register SP since it is a pointer to data.  This checks
+# for a regression when creating MI variables from pointers
+# with "print object" enabled.
+
+# Enable "print object"
+mi_gdb_test "-gdb-set print object on" ".*"
+
+# Test creating a variable for $sp
+mi_gdb_test "-var-create sp1 * \$sp" \
+	    "\\^done,name=\"sp1\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
+	    "-var-create sp1 * \$sp"
+gdb_exit

Re: [PATCH] Fix mi "-var-create" regression

[Tom Tromey]

>>>>> "Luis" == Luis Machado <lgustavo@codesourcery.com> writes:

Luis> 2012-10-15  Luis Machado  <lgustavo@codesourcery.com>
Luis> 	* value.c (value_actual_type): Check for TYPE_CODE_STRUCT
Luis> 	target types.

Luis> 	testsuite/
Luis> 	* mi-var-create-rtti.c: New file.
Luis> 	* mi-var-create-rtti.exp: New file.

This looks good to me.  One tiny nit:

Luis> Index: gdb/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
Luis> +    return -1;

No need for a ";" here.

Ok with this change.

thanks,
Tom

Re: [PATCH] Fix mi "-var-create" regression

[Luis Machado]

On 10/30/2012 06:36 PM, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado<lgustavo@codesourcery.com>  writes:
>
> Luis>  2012-10-15  Luis Machado<lgustavo@codesourcery.com>
> Luis>  	* value.c (value_actual_type): Check for TYPE_CODE_STRUCT
> Luis>  	target types.
>
> Luis>  	testsuite/
> Luis>  	* mi-var-create-rtti.c: New file.
> Luis>  	* mi-var-create-rtti.exp: New file.
>
> This looks good to me.  One tiny nit:
>
> Luis>  Index: gdb/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> Luis>  +    return -1;
>
> No need for a ";" here.
>
> Ok with this change.
>
> thanks,
> Tom

Thanks. I've checked this in now.

Luis

RE: [PATCH] Fix mi "-var-create" regression

[Marc Khouzam]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Luis Machado
> Sent: Wednesday, November 14, 2012 2:22 PM
> To: Tom Tromey
> Cc: Joel Brobecker; gdb-patches@sourceware.org
> Subject: Re: [PATCH] Fix mi "-var-create" regression
> 
> On 10/30/2012 06:36 PM, Tom Tromey wrote:
> >>>>>> "Luis" == Luis Machado<lgustavo@codesourcery.com>  writes:
> >
> > Luis>  2012-10-15  Luis Machado<lgustavo@codesourcery.com>
> > Luis>  	* value.c (value_actual_type): Check for 
> TYPE_CODE_STRUCT
> > Luis>  	target types.
> >
> > Luis>  	testsuite/
> > Luis>  	* mi-var-create-rtti.c: New file.
> > Luis>  	* mi-var-create-rtti.exp: New file.
> >
> > This looks good to me.  One tiny nit:
> >
> > Luis>  Index: gdb/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> > Luis>  +    return -1;
> >
> > No need for a ";" here.
> >
> > Ok with this change.
> >
> > thanks,
> > Tom
> 
> Thanks. I've checked this in now.

Nice!
Is it possible to put this in the 7_5 branch?

Thanks

Marc

Re: [PATCH] Fix mi "-var-create" regression

[Joel Brobecker]

> Nice!
> Is it possible to put this in the 7_5 branch?

I haven't looked at the patch itself, but at this point, we really
want any of them to be extra safe. If the approver thinks so, then
it can go in.

-- 
Joel

Re: [PATCH] Fix mi "-var-create" regression

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I haven't looked at the patch itself, but at this point, we really
Joel> want any of them to be extra safe. If the approver thinks so, then
Joel> it can go in.

I think it would be fine.

Tom

RE: [PATCH] Fix mi "-var-create" regression

[Marc Khouzam]

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com] 
> Sent: Thursday, November 15, 2012 1:47 PM
> To: Joel Brobecker
> Cc: Marc Khouzam; 'lgustavo@codesourcery.com'; 
> 'gdb-patches@sourceware.org'
> Subject: Re: [PATCH] Fix mi "-var-create" regression
> 
> >>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> Joel> I haven't looked at the patch itself, but at this 
> point, we really
> Joel> want any of them to be extra safe. If the approver 
> thinks so, then
> Joel> it can go in.
> 
> I think it would be fine.

Thanks!

So how does it work?  Luis, are you ok to check it in to 7_5
yourself or should I do it (am I even allowed to do that?)

Re: [PATCH] Fix mi "-var-create" regression

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> So how does it work?  Luis, are you ok to check it in to 7_5
Marc> yourself or should I do it (am I even allowed to do that?)

It is fine if you do it.

Tom

Re: [PATCH] Fix mi "-var-create" regression

[Luis Machado]

On 11/15/2012 06:54 PM, Marc Khouzam wrote:
>> -----Original Message-----
>> From: Tom Tromey [mailto:tromey@redhat.com]
>> Sent: Thursday, November 15, 2012 1:47 PM
>> To: Joel Brobecker
>> Cc: Marc Khouzam; 'lgustavo@codesourcery.com';
>> 'gdb-patches@sourceware.org'
>> Subject: Re: [PATCH] Fix mi "-var-create" regression
>>
>>>>>>> "Joel" == Joel Brobecker<brobecker@adacore.com>  writes:
>>
>> Joel>  I haven't looked at the patch itself, but at this
>> point, we really
>> Joel>  want any of them to be extra safe. If the approver
>> thinks so, then
>> Joel>  it can go in.
>>
>> I think it would be fine.
>
> Thanks!
>
> So how does it work?  Luis, are you ok to check it in to 7_5
> yourself or should I do it (am I even allowed to do that?)

Either option is fine with me. If you want, i can check it in.

Regards,
Luis

RE: [PATCH] Fix mi "-var-create" regression

[Marc Khouzam]

I think it would be faster if you took care of it.

Much appreciated!

(sorry for the top-post)


Best regards,

Marc


----- Original Message -----
From: Luis Machado <lgustavo@codesourcery.com>
To: Marc Khouzam <marc.khouzam@ericsson.com>
Cc: 'Tom Tromey' <tromey@redhat.com>, 'Joel Brobecker' <brobecker@adacore.com>, "'lgustavo@codesourcery.com'" <lgustavo@codesourcery.com>, "'gdb-patches@sourceware.org'" <gdb-patches@sourceware.org>
Sent: 15-11-2012 18:28
Subject: Re: [PATCH] Fix mi "-var-create" regression



On 11/15/2012 06:54 PM, Marc Khouzam wrote:
>> -----Original Message-----
>> From: Tom Tromey [mailto:tromey@redhat.com]
>> Sent: Thursday, November 15, 2012 1:47 PM
>> To: Joel Brobecker
>> Cc: Marc Khouzam; 'lgustavo@codesourcery.com';
>> 'gdb-patches@sourceware.org'
>> Subject: Re: [PATCH] Fix mi "-var-create" regression
>>
>>>>>>> "Joel" == Joel Brobecker<brobecker@adacore.com>  writes:
>>
>> Joel>  I haven't looked at the patch itself, but at this
>> point, we really
>> Joel>  want any of them to be extra safe. If the approver
>> thinks so, then
>> Joel>  it can go in.
>>
>> I think it would be fine.
>
> Thanks!
>
> So how does it work?  Luis, are you ok to check it in to 7_5
> yourself or should I do it (am I even allowed to do that?)

Either option is fine with me. If you want, i can check it in.

Regards,
Luis

Re: [PATCH] Fix mi "-var-create" regression

[Luis Machado]

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

On 11/15/2012 10:50 PM, Marc Khouzam wrote:
> I think it would be faster if you took care of it.
>
> Much appreciated!
>
> (sorry for the top-post)
>
>
> Best regards,
>
> Marc
>
>
> ----- Original Message -----
> From: Luis Machado<lgustavo@codesourcery.com>
> To: Marc Khouzam<marc.khouzam@ericsson.com>
> Cc: 'Tom Tromey'<tromey@redhat.com>, 'Joel Brobecker'<brobecker@adacore.com>, "'lgustavo@codesourcery.com'"<lgustavo@codesourcery.com>, "'gdb-patches@sourceware.org'"<gdb-patches@sourceware.org>
> Sent: 15-11-2012 18:28
> Subject: Re: [PATCH] Fix mi "-var-create" regression
>
>
>
> On 11/15/2012 06:54 PM, Marc Khouzam wrote:
>>> -----Original Message-----
>>> From: Tom Tromey [mailto:tromey@redhat.com]
>>> Sent: Thursday, November 15, 2012 1:47 PM
>>> To: Joel Brobecker
>>> Cc: Marc Khouzam; 'lgustavo@codesourcery.com';
>>> 'gdb-patches@sourceware.org'
>>> Subject: Re: [PATCH] Fix mi "-var-create" regression
>>>
>>>>>>>> "Joel" == Joel Brobecker<brobecker@adacore.com>   writes:
>>>
>>> Joel>   I haven't looked at the patch itself, but at this
>>> point, we really
>>> Joel>   want any of them to be extra safe. If the approver
>>> thinks so, then
>>> Joel>   it can go in.
>>>
>>> I think it would be fine.
>>
>> Thanks!
>>
>> So how does it work?  Luis, are you ok to check it in to 7_5
>> yourself or should I do it (am I even allowed to do that?)
>
> Either option is fine with me. If you want, i can check it in.
>
> Regards,
> Luis

Done.

Attached is what i checked in.

Regards,
Luis

[-- Attachment #2: rtti-print-object.diff --]
[-- Type: text/x-patch, Size: 3914 bytes --]

2012-10-15  Luis Machado  <lgustavo@codesourcery.com>

	gdb/
	* value.c (value_actual_type): Check for TYPE_CODE_STRUCT
	target types.

	gdb/testsuite/
	* gdb.mi/mi-var-create-rtti.c: New file.
	* gdb.mi/mi-var-create-rtti.exp: New file.

Index: gdb/gdb/value.c
===================================================================
--- gdb.orig/gdb/value.c	2012-10-28 20:22:13.209235579 -0200
+++ gdb/gdb/value.c	2012-10-28 20:27:07.857218717 -0200
@@ -850,8 +850,12 @@ value_actual_type (struct value *value,
   result = value_type (value);
   if (opts.objectprint)
     {
-      if (TYPE_CODE (result) == TYPE_CODE_PTR
+      /* If result's target type is TYPE_CODE_STRUCT, proceed to
+	 fetch its rtti type.  */
+      if ((TYPE_CODE (result) == TYPE_CODE_PTR
 	  || TYPE_CODE (result) == TYPE_CODE_REF)
+	  && TYPE_CODE (check_typedef (TYPE_TARGET_TYPE (result)))
+	     == TYPE_CODE_STRUCT)
         {
           struct type *real_type;
 
Index: gdb/gdb/testsuite/gdb.mi/mi-var-create-rtti.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb/gdb/testsuite/gdb.mi/mi-var-create-rtti.c	2012-10-28 19:27:03.241424995 -0200
@@ -0,0 +1,24 @@
+/* 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
+main (void)
+{
+  int i = 0;
+
+  return i; /* next-line */
+}
Index: gdb/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp	2012-10-28 20:21:34.737237782 -0200
@@ -0,0 +1,52 @@
+# 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/>.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+standard_testfile .c
+set opts {debug}
+
+if [build_executable $testfile.exp $testfile $srcfile $opts] {
+    return -1
+}
+
+mi_delete_breakpoints
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_load ${binfile}
+
+if ![mi_run_to_main] {
+    untested "could not run to main"
+    return -1
+}
+
+# Test creating a register-based variable.  We pick
+# register SP since it is a pointer to data.  This checks
+# for a regression when creating MI variables from pointers
+# with "print object" enabled.
+
+# Enable "print object"
+mi_gdb_test "-gdb-set print object on" ".*"
+
+# Test creating a variable for $sp
+mi_gdb_test "-var-create sp1 * \$sp" \
+	    "\\^done,name=\"sp1\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
+	    "-var-create sp1 * \$sp"
+gdb_exit

Re: [PATCH] Fix mi "-var-create" regression

[Andreas Schwab]

Luis Machado <lgustavo@codesourcery.com> writes:

> +# Test creating a variable for $sp
> +mi_gdb_test "-var-create sp1 * \$sp" \
> +	    "\\^done,name=\"sp1\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
> +	    "-var-create sp1 * \$sp"
> +gdb_exit

Expecting: ^(-var-create sp1 \* \$sp[
]+)?(\^done,name="sp1",numchild="0",value="0x[0-9A-Fa-f]+",type="void \*",has_more="0"[
]+[(]gdb[)] 
[ ]*)
-var-create sp1 * $sp
^done,name="sp1",numchild="0",value="70368744167200",type="uint64_t",has_more="0"
(gdb) 
FAIL: gdb.mi/mi-var-create-rtti.exp: -var-create sp1 * $sp

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Fix mi "-var-create" regression

[Joel Brobecker]

> Expecting: ^(-var-create sp1 \* \$sp[
> ]+)?(\^done,name="sp1",numchild="0",value="0x[0-9A-Fa-f]+",type="void \*",has_more="0"[
> ]+[(]gdb[)] 
> [ ]*)
> -var-create sp1 * $sp
> ^done,name="sp1",numchild="0",value="70368744167200",type="uint64_t",has_more="0"
> (gdb) 
> FAIL: gdb.mi/mi-var-create-rtti.exp: -var-create sp1 * $sp

Ah yes, the type of $sp isn't guaranteed to be a "void *".
Seems obvious in retrospect, but easy to overlook. And easy to fix,
depending on what the testcase was really trying to do.

-- 
Joel

Re: [PATCH] Fix mi "-var-create" regression

[Luis Machado]

On 11/29/2012 07:07 PM, Joel Brobecker wrote:
>> Expecting: ^(-var-create sp1 \* \$sp[
>> ]+)?(\^done,name="sp1",numchild="0",value="0x[0-9A-Fa-f]+",type="void \*",has_more="0"[
>> ]+[(]gdb[)]
>> [ ]*)
>> -var-create sp1 * $sp
>> ^done,name="sp1",numchild="0",value="70368744167200",type="uint64_t",has_more="0"
>> (gdb)
>> FAIL: gdb.mi/mi-var-create-rtti.exp: -var-create sp1 * $sp
>
> Ah yes, the type of $sp isn't guaranteed to be a "void *".
> Seems obvious in retrospect, but easy to overlook. And easy to fix,
> depending on what the testcase was really trying to do.
>

True. Is this m68k?

I'll have to find a register that is of type (void *) for all the 
architectures.

Luis

Re: [PATCH] Fix mi "-var-create" regression

[Joel Brobecker]

> True. Is this m68k?

Did Andreas ever answer that question?

> I'll have to find a register that is of type (void *) for all the
> architectures.

And do you absolutely need the register to be of type (void *)
for the test to exercise the original problem? I don't know if
we can find any that's guaranteed to be of any type. So you might
have to use different register names depending on the target.

-- 
Joel

Re: [PATCH] Fix mi "-var-create" regression

[Luis Machado]

On 12/07/2012 12:48 AM, Joel Brobecker wrote:
>> True. Is this m68k?
>
> Did Andreas ever answer that question?

Not yet.

>
>> I'll have to find a register that is of type (void *) for all the
>> architectures.
>
> And do you absolutely need the register to be of type (void *)
> for the test to exercise the original problem? I don't know if
> we can find any that's guaranteed to be of any type. So you might
> have to use different register names depending on the target.
>

Yeah. I may need to do something different here. I didn't forget about 
this one though.

Luis

Re: [PATCH] Fix mi "-var-create" regression

[Luis Machado]

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

Hi,

On 12/07/2012 12:51 AM, Luis Machado wrote:
> On 12/07/2012 12:48 AM, Joel Brobecker wrote:
>> And do you absolutely need the register to be of type (void *)
>> for the test to exercise the original problem? I don't know if
>> we can find any that's guaranteed to be of any type. So you might
>> have to use different register names depending on the target.
>>
>
> Yeah. I may need to do something different here. I didn't forget about
> this one though.
>
> Luis

Casting seems to be the most sensible solution to this problem.

This patch addresses the problem Andreas saw by creating a variable of 
type (void *). This way we are guaranteed to have the type we need to 
reproduce the problem.

I thought about replacing SP with PC, but SP is probably available for 
all targets.

What do you think?

Luis

[-- Attachment #2: var_create_rtti.diff --]
[-- Type: text/x-patch, Size: 985 bytes --]

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 4620c2e..9ffd991 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2012-12-26  Luis Machado  <lgustavo@codesourcery.com>
+
+	* gdb.mi/mi-var-create-rtti.exp: Create a variable of
+	type void *.
+
 2012-12-25  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* gdb.mi/mi-fullname-deleted.exp: New file.
diff --git a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
index 16d8551..d6697de 100644
--- a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
@@ -46,7 +46,7 @@ if ![mi_run_to_main] {
 mi_gdb_test "-gdb-set print object on" ".*"
 
 # Test creating a variable for $sp
-mi_gdb_test "-var-create sp1 * \$sp" \
+mi_gdb_test "-var-create sp1 * ((void*)\$sp)" \
 	    "\\^done,name=\"sp1\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
 	    "-var-create sp1 * \$sp"
 gdb_exit

Re: [PATCH] Fix mi "-var-create" regression

[Joel Brobecker]

> This patch addresses the problem Andreas saw by creating a variable
> of type (void *). This way we are guaranteed to have the type we
> need to reproduce the problem.

I don't see a problem with that. But can you add a comment explaining
why you are casting the register to void *?

Thanks!

> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 4620c2e..9ffd991 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2012-12-26  Luis Machado  <lgustavo@codesourcery.com>
> +
> +	* gdb.mi/mi-var-create-rtti.exp: Create a variable of
> +	type void *.
> +
>  2012-12-25  Jan Kratochvil  <jan.kratochvil@redhat.com>
>  
>  	* gdb.mi/mi-fullname-deleted.exp: New file.
> diff --git a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> index 16d8551..d6697de 100644
> --- a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> +++ b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> @@ -46,7 +46,7 @@ if ![mi_run_to_main] {
>  mi_gdb_test "-gdb-set print object on" ".*"
>  
>  # Test creating a variable for $sp
> -mi_gdb_test "-var-create sp1 * \$sp" \
> +mi_gdb_test "-var-create sp1 * ((void*)\$sp)" \
>  	    "\\^done,name=\"sp1\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
>  	    "-var-create sp1 * \$sp"
>  gdb_exit


-- 
Joel

Re: [PATCH] Fix mi "-var-create" regression

[Luis Machado]

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

On 01/03/2013 01:46 AM, Joel Brobecker wrote:
>> This patch addresses the problem Andreas saw by creating a variable
>> of type (void *). This way we are guaranteed to have the type we
>> need to reproduce the problem.
>
> I don't see a problem with that. But can you add a comment explaining
> why you are casting the register to void *?

Yes, of course.

How does this comment look?

Luis

[-- Attachment #2: var_create_rtti.diff --]
[-- Type: text/x-patch, Size: 1103 bytes --]

diff --git a/ChangeLog b/ChangeLog
index a5a47ca..aa180a6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2013-01-03  Luis Machado  <lgustavo@codesourcery.com>
+
+	* gdb.mi/mi-var-create-rtti.exp: Create a variable of
+	type void *.
+
 2012-12-20  Jan-Benedict Glaw  <jbglaw@lug-owl.de>
 
 	* Makefile.def (install-target-libgo): Depend on
diff --git a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
index da3cf1b..f991951 100644
--- a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
@@ -45,8 +45,10 @@ if ![mi_run_to_main] {
 # Enable "print object"
 mi_gdb_test "-gdb-set print object on" ".*"
 
-# Test creating a variable for $sp
-mi_gdb_test "-var-create sp1 * \$sp" \
+# Test creating a variable for $sp.
+# We use a explicit cast to (void *) as that is the
+# type that caused the bug this testcase is testing for.
+mi_gdb_test "-var-create sp1 * ((void*)\$sp)" \
 	    "\\^done,name=\"sp1\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
 	    "-var-create sp1 * \$sp"
 gdb_exit

Re: [PATCH] Fix mi "-var-create" regression

[Joel Brobecker]

> How does this comment look?

Looks good to me. Thank you!

-- 
Joel