Re: [patch] fix for c++/2416

Re: [patch] fix for c++/2416

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> If you'll delete the extra blank line, the patch and testcases are OK.

Removed blank line, both code and tests committed. Thanks for reviewing the
patch.

The PR 2416 should probably be updated too... I haven't done that, should I?

Oh... and how do I change gnats password?


BTW, thanks for fixing the date on my previous ChangeLog commit, I had
blindly 
copied my prepared change log without changing the date.

Re: [patch] fix for c++/2416

[Daniel Jacobowitz]

On Tue, Apr 01, 2008 at 12:26:16PM -0400, Aleksandar Ristovski wrote:
> Daniel Jacobowitz wrote:
> > If you'll delete the extra blank line, the patch and testcases are OK.
> 
> Removed blank line, both code and tests committed. Thanks for reviewing the
> patch.
> 
> The PR 2416 should probably be updated too... I haven't done that, should I?

Yes please.

> Oh... and how do I change gnats password?

You can't, but if you really want to for some reason, ask overseers@.
Don't send a new password there, it's publically archived.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] fix for c++/2416

[Daniel Jacobowitz]

On Tue, Apr 01, 2008 at 11:33:32AM -0400, Aleksandar Ristovski wrote:
> I believe we should.
> 
> Here is a dummy c++ program where this is done explicitly:
> 
> #include <iostream>
> using namespace std;
> int main ()
> {
>    int a = 42;
>    int &refa = (int &)a; // cast not needed, yet compiler doesn't complain.
>    cout << refa << endl;
>    return 0;
> }

I had to ask two C++ experts what this meant and let them argue about
it for a little while... it's even legal if a is a double.  This is
*reinterpret_cast<int *>(&a), an lvalue of type int.  That's not what
GDB will do with it, but it's close enough for now.

If you'll delete the extra blank line, the patch and testcases are OK.
Thanks for your patience.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] fix for c++/2416

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Thu, Feb 28, 2008 at 11:45:07PM -0500, Aleksandar Ristovski wrote:
>> Allright, then how about this, yet newer and yet more revisited diff? I 
>> removed changes to eval.c and let it simply call value_cast as it used
to. 
>> Now value_cast knows how to handle references.
> 
> We were converging on a fix and then there's a whole different patch
> to look at... sorry I couldn't make time until now.
> 
>> @@ -257,6 +290,7 @@ value_cast_pointers (struct type *type, 
>>    return arg2;
>>  }
>>  
>> +
>>  /* Cast value ARG2 to type TYPE and return as a value.
>>     More general than a C cast: accepts any two types of the same length,
>>     and if ARG2 is an lvalue it can be cast into anything at all.  */
> 
> Please drop the whitespace change.
> 
>> @@ -275,6 +309,26 @@ value_cast (struct type *type, struct va
>>    if (value_type (arg2) == type)
>>      return arg2;
>>  
>> +  code1 = TYPE_CODE (check_typedef (type));
>> +
>> +  /* Check if we are casting struct reference to struct reference.  */
>> +  if (code1 == TYPE_CODE_REF)
>> +    {
>> +      /* We dereference type; then we recurse and finally
>> +         we generate value of the given reference. Nothing wrong with 
>> +	 that.  */
>> +      struct type *t1 = check_typedef (type);
>> +      struct type *dereftype = check_typedef (TYPE_TARGET_TYPE (t1));
>> +      struct value *val =  value_cast (dereftype, arg2);
>> +      return value_ref (val); 
>> +    }
> 
> This allows things like "(int&) int_var", by automatically creating
> references.  Should we really do that?
> 

I believe we should.

Here is a dummy c++ program where this is done explicitly:

#include <iostream>
using namespace std;
int main ()
{
   int a = 42;
   int &refa = (int &)a; // cast not needed, yet compiler doesn't complain.
   cout << refa << endl;
   return 0;
}

Re: [patch] fix for c++/2416

[Daniel Jacobowitz]

On Thu, Feb 28, 2008 at 11:45:07PM -0500, Aleksandar Ristovski wrote:
> Allright, then how about this, yet newer and yet more revisited diff? I 
> removed changes to eval.c and let it simply call value_cast as it used to. 
> Now value_cast knows how to handle references.

We were converging on a fix and then there's a whole different patch
to look at... sorry I couldn't make time until now.

> @@ -257,6 +290,7 @@ value_cast_pointers (struct type *type, 
>    return arg2;
>  }
>  
> +
>  /* Cast value ARG2 to type TYPE and return as a value.
>     More general than a C cast: accepts any two types of the same length,
>     and if ARG2 is an lvalue it can be cast into anything at all.  */

Please drop the whitespace change.

> @@ -275,6 +309,26 @@ value_cast (struct type *type, struct va
>    if (value_type (arg2) == type)
>      return arg2;
>  
> +  code1 = TYPE_CODE (check_typedef (type));
> +
> +  /* Check if we are casting struct reference to struct reference.  */
> +  if (code1 == TYPE_CODE_REF)
> +    {
> +      /* We dereference type; then we recurse and finally
> +         we generate value of the given reference. Nothing wrong with 
> +	 that.  */
> +      struct type *t1 = check_typedef (type);
> +      struct type *dereftype = check_typedef (TYPE_TARGET_TYPE (t1));
> +      struct value *val =  value_cast (dereftype, arg2);
> +      return value_ref (val); 
> +    }

This allows things like "(int&) int_var", by automatically creating
references.  Should we really do that?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] fix for c++/2416

[Aleksandar Ristovski]

Aleksandar Ristovski wrote:
> Daniel Jacobowitz wrote:
>> On Thu, Feb 28, 2008 at 01:53:13PM -0500, Aleksandar Ristovski wrote:
>>> See revisited diff (attached). Also, please find attached the 
>>> testcase diff, I added check for reference casting. Do I need to 
>>> provide change log for tests too?
>>
>> The testsuite changes look fine but do need a changelog; that
>> goes in testsuite/ChangeLog.
> 
> 2008-02-28  Aleksandar Ristovski  <aristovski@qnx.com>
> 
>     * gdb.cp/casts.cc: Add class reference variables.
>     * gdb.cp/casts.exp: New test cases for up/down casting references.
> 

Is the testsuite diff (including changelog) ok to commit? The added tests
will 
add 2 more FAIL-s and 2 more PASS-es to the tests, which should turn into 4
more 
PASS-es, pending the fix to PR2416.

> 
>>
>> There's nothing wrong with casting a reference to a non-reference;
>> that coerces the reference.  Also you have to beware typedefs.
>>
>>       enum type_code code1, code2;
>>       code1 = TYPE_CODE (check_typedef (value_type (arg1)));
>>       code2 = TYPE_CODE (check_typedef (value_type (arg2)));
>>
>>       if (code1 == TYPE_CODE_REF && code2 == TYPE_CODE_REF)
>>         arg1 = value_cast_pointers (type, arg1);
>>       else if (code1 == TYPE_CODE_REF)
>>         error (_("Attempt to cast to reference type from non-reference 
>> type."));
>>       else
>>         arg1 = value_cast (type, arg1);
> 
> Allright, then how about this, yet newer and yet more revisited diff? I 
> removed changes to eval.c and let it simply call value_cast as it used 
> to. Now value_cast knows how to handle references.
> 
> 
> 2008-02-28  Aleksandar Ristovski <aristovski@qnx.com>
> 
>     * valops.c (value_cast_structs): New function. Cast related
>     STRUCT types up/down and return cast value. The body of this
>     function comes mostly from value_cast_pointers.
>     (value_cast_pointers): Code for actual cast STRUCT-STRUCT moved
>     to value_cast_structs. Now value_cast_pointers needs only create
>     appropriate reference after using value_cast_structs for actual
>     casting.
>     (value_cast): Handle references.
>     
> 

I haven't received any feedback on this.

Re: [patch] fix for c++/2416

[Aleksandar Ristovski]

Any thoughts on this?

Your feedback would be greatly appreciated.

Thanks,

Aleksandar Ristovski
QNX Software Systems


Aleksandar Ristovski wrote:
> Daniel Jacobowitz wrote:
>> On Thu, Feb 28, 2008 at 01:53:13PM -0500, Aleksandar Ristovski wrote:
>>> See revisited diff (attached). Also, please find attached the 
>>> testcase diff, I added check for reference casting. Do I need to 
>>> provide change log for tests too?
>>
>> The testsuite changes look fine but do need a changelog; that
>> goes in testsuite/ChangeLog.
> 
> 2008-02-28  Aleksandar Ristovski  <aristovski@qnx.com>
> 
>     * gdb.cp/casts.cc: Add class reference variables.
>     * gdb.cp/casts.exp: New test cases for up/down casting references.
> 
> 
>>
>> There's nothing wrong with casting a reference to a non-reference;
>> that coerces the reference.  Also you have to beware typedefs.
>>
>>       enum type_code code1, code2;
>>       code1 = TYPE_CODE (check_typedef (value_type (arg1)));
>>       code2 = TYPE_CODE (check_typedef (value_type (arg2)));
>>
>>       if (code1 == TYPE_CODE_REF && code2 == TYPE_CODE_REF)
>>         arg1 = value_cast_pointers (type, arg1);
>>       else if (code1 == TYPE_CODE_REF)
>>         error (_("Attempt to cast to reference type from non-reference 
>> type."));
>>       else
>>         arg1 = value_cast (type, arg1);
> 
> Allright, then how about this, yet newer and yet more revisited diff? I 
> removed changes to eval.c and let it simply call value_cast as it used 
> to. Now value_cast knows how to handle references.
> 
> 
> 2008-02-28  Aleksandar Ristovski <aristovski@qnx.com>
> 
>     * valops.c (value_cast_structs): New function. Cast related
>     STRUCT types up/down and return cast value. The body of this
>     function comes mostly from value_cast_pointers.
>     (value_cast_pointers): Code for actual cast STRUCT-STRUCT moved
>     to value_cast_structs. Now value_cast_pointers needs only create
>     appropriate reference after using value_cast_structs for actual
>     casting.
>     (value_cast): Handle references.
>     
> 

Re: [patch] fix for c++/2416

[Aleksandar Ristovski]

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

Daniel Jacobowitz wrote:
> On Thu, Feb 28, 2008 at 01:53:13PM -0500, Aleksandar Ristovski wrote:
>> See revisited diff (attached). Also, please find attached the testcase 
>> diff, I added check for reference casting. Do I need to provide change log 
>> for tests too?
> 
> The testsuite changes look fine but do need a changelog; that
> goes in testsuite/ChangeLog.

2008-02-28  Aleksandar Ristovski  <aristovski@qnx.com>

	* gdb.cp/casts.cc: Add class reference variables.
	* gdb.cp/casts.exp: New test cases for up/down casting references.


> 
> There's nothing wrong with casting a reference to a non-reference;
> that coerces the reference.  Also you have to beware typedefs.
> 
> 	  enum type_code code1, code2;
> 	  code1 = TYPE_CODE (check_typedef (value_type (arg1)));
> 	  code2 = TYPE_CODE (check_typedef (value_type (arg2)));
> 
> 	  if (code1 == TYPE_CODE_REF && code2 == TYPE_CODE_REF)
> 	    arg1 = value_cast_pointers (type, arg1);
> 	  else if (code1 == TYPE_CODE_REF)
> 	    error (_("Attempt to cast to reference type from non-reference type."));
> 	  else
> 	    arg1 = value_cast (type, arg1);

Allright, then how about this, yet newer and yet more revisited diff? I removed 
changes to eval.c and let it simply call value_cast as it used to. Now 
value_cast knows how to handle references.


2008-02-28  Aleksandar Ristovski <aristovski@qnx.com>

	* valops.c (value_cast_structs): New function. Cast related
	STRUCT types up/down and return cast value. The body of this
	function comes mostly from value_cast_pointers.
	(value_cast_pointers): Code for actual cast STRUCT-STRUCT moved
	to value_cast_structs. Now value_cast_pointers needs only create
	appropriate reference after using value_cast_structs for actual
	casting.
	(value_cast): Handle references.
	


[-- Attachment #2: valops.c.diff --]
[-- Type: text/plain, Size: 6951 bytes --]

Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.183
diff -u -p -r1.183 valops.c
--- gdb/valops.c	4 Feb 2008 00:23:04 -0000	1.183
+++ gdb/valops.c	29 Feb 2008 04:30:05 -0000
@@ -192,6 +192,62 @@ allocate_space_in_inferior (int len)
   return value_as_long (value_allocate_space_in_inferior (len));
 }
 
+/* Cast struct value VAL to type TYPE and return as a value.
+   Both type and val must be of TYPE_CODE_STRUCT or TYPE_CODE_UNION
+   for this to work. Typedef to one of the codes is permitted.  */
+
+static struct value *
+value_cast_structs (struct type *type, struct value *v2)
+{
+  struct type *t1;
+  struct type *t2;
+  struct value *v;
+
+  gdb_assert (type != NULL && v2 != NULL);
+
+  t1 = check_typedef (type);
+  t2 = check_typedef (value_type (v2));
+
+  /* Check preconditions.  */
+  gdb_assert ((TYPE_CODE (t1) == TYPE_CODE_STRUCT
+	       || TYPE_CODE (t1) == TYPE_CODE_UNION)
+	      && !!"Precondition is that type is of STRUCT or UNION kind.");
+  gdb_assert ((TYPE_CODE (t2) == TYPE_CODE_STRUCT
+	       || TYPE_CODE (t2) == TYPE_CODE_UNION)
+	      && !!"Precondition is that value is of STRUCT or UNION kind");
+
+  /* Upcasting: look in the type of the source to see if it contains the
+     type of the target as a superclass.  If so, we'll need to
+     offset the pointer rather than just change its type.  */
+  if (TYPE_NAME (t1) != NULL)
+    {
+      v = search_struct_field (type_name_no_tag (t1),
+			       v2, 0, t2, 1);
+      if (v)
+	return v;
+    }
+
+  /* Downcasting: look in the type of the target to see if it contains the
+     type of the source as a superclass.  If so, we'll need to
+     offset the pointer rather than just change its type.
+     FIXME: This fails silently with virtual inheritance.  */
+  if (TYPE_NAME (t2) != NULL)
+    {
+      v = search_struct_field (type_name_no_tag (t2),
+			       value_zero (t1, not_lval), 0, t1, 1);
+      if (v)
+	{
+	  /* Downcasting is possible (t1 is superclass of v2).  */
+	  CORE_ADDR addr2 = VALUE_ADDRESS (v2);
+	  addr2 -= (VALUE_ADDRESS (v)
+		    + value_offset (v)
+		    + value_embedded_offset (v));
+	  return value_at (type, addr2);
+	}
+    }
+  return v2;
+}
+
 /* Cast one pointer or reference type to another.  Both TYPE and
    the type of ARG2 should be pointer types, or else both should be
    reference types.  Returns the new pointer or reference.  */
@@ -199,6 +255,7 @@ allocate_space_in_inferior (int len)
 struct value *
 value_cast_pointers (struct type *type, struct value *arg2)
 {
+  struct type *type1 = check_typedef (type);
   struct type *type2 = check_typedef (value_type (arg2));
   struct type *t1 = check_typedef (TYPE_TARGET_TYPE (type));
   struct type *t2 = check_typedef (TYPE_TARGET_TYPE (type2));
@@ -207,47 +264,23 @@ value_cast_pointers (struct type *type, 
       && TYPE_CODE (t2) == TYPE_CODE_STRUCT
       && !value_logical_not (arg2))
     {
-      struct value *v;
+      struct value *v2;
 
-      /* Look in the type of the source to see if it contains the
-	 type of the target as a superclass.  If so, we'll need to
-	 offset the pointer rather than just change its type.  */
-      if (TYPE_NAME (t1) != NULL)
+      if (TYPE_CODE (type2) == TYPE_CODE_REF)
+	v2 = coerce_ref (arg2);
+      else
+	v2 = value_ind (arg2);
+      gdb_assert (TYPE_CODE (value_type (v2)) == TYPE_CODE_STRUCT
+		  && !!"Why did coercion fail?");
+      v2 = value_cast_structs (t1, v2);
+      /* At this point we have what we can have, un-dereference if needed.  */
+      if (v2)
 	{
-	  struct value *v2;
-
-	  if (TYPE_CODE (type2) == TYPE_CODE_REF)
-	    v2 = coerce_ref (arg2);
-	  else
-	    v2 = value_ind (arg2);
-	  v = search_struct_field (type_name_no_tag (t1),
-				   v2, 0, t2, 1);
-	  if (v)
-	    {
-	      v = value_addr (v);
-	      deprecated_set_value_type (v, type);
-	      return v;
-	    }
-	}
-
-      /* Look in the type of the target to see if it contains the
-	 type of the source as a superclass.  If so, we'll need to
-	 offset the pointer rather than just change its type.
-	 FIXME: This fails silently with virtual inheritance.  */
-      if (TYPE_NAME (t2) != NULL)
-	{
-	  v = search_struct_field (type_name_no_tag (t2),
-				   value_zero (t1, not_lval), 0, t1, 1);
-	  if (v)
-	    {
-	      CORE_ADDR addr2 = value_as_address (arg2);
-	      addr2 -= (VALUE_ADDRESS (v)
-			+ value_offset (v)
-			+ value_embedded_offset (v));
-	      return value_from_pointer (type, addr2);
-	    }
+	  struct value *v = value_addr (v2);
+	  deprecated_set_value_type (v, type);
+	  return v;
 	}
-    }
+   }
 
   /* No superclass found, just change the pointer type.  */
   arg2 = value_copy (arg2);
@@ -257,6 +290,7 @@ value_cast_pointers (struct type *type, 
   return arg2;
 }
 
+
 /* Cast value ARG2 to type TYPE and return as a value.
    More general than a C cast: accepts any two types of the same length,
    and if ARG2 is an lvalue it can be cast into anything at all.  */
@@ -275,6 +309,26 @@ value_cast (struct type *type, struct va
   if (value_type (arg2) == type)
     return arg2;
 
+  code1 = TYPE_CODE (check_typedef (type));
+
+  /* Check if we are casting struct reference to struct reference.  */
+  if (code1 == TYPE_CODE_REF)
+    {
+      /* We dereference type; then we recurse and finally
+         we generate value of the given reference. Nothing wrong with 
+	 that.  */
+      struct type *t1 = check_typedef (type);
+      struct type *dereftype = check_typedef (TYPE_TARGET_TYPE (t1));
+      struct value *val =  value_cast (dereftype, arg2);
+      return value_ref (val); 
+    }
+
+  code2 = TYPE_CODE (check_typedef (value_type (arg2)));
+
+  if (code2 == TYPE_CODE_REF)
+    /* We deref the value and then do the cast.  */
+    return value_cast (type, coerce_ref (arg2)); 
+
   CHECK_TYPEDEF (type);
   code1 = TYPE_CODE (type);
   arg2 = coerce_ref (arg2);
@@ -342,21 +396,10 @@ value_cast (struct type *type, struct va
 	    || code2 == TYPE_CODE_DECFLOAT || code2 == TYPE_CODE_ENUM
 	    || code2 == TYPE_CODE_RANGE);
 
-  if (code1 == TYPE_CODE_STRUCT
-      && code2 == TYPE_CODE_STRUCT
+  if ((code1 == TYPE_CODE_STRUCT || code1 == TYPE_CODE_UNION)
+      && (code2 == TYPE_CODE_STRUCT || code2 == TYPE_CODE_UNION)
       && TYPE_NAME (type) != 0)
-    {
-      /* Look in the type of the source to see if it contains the
-         type of the target as a superclass.  If so, we'll need to
-         offset the object in addition to changing its type.  */
-      struct value *v = search_struct_field (type_name_no_tag (type),
-					     arg2, 0, type2, 1);
-      if (v)
-	{
-	  deprecated_set_value_type (v, type);
-	  return v;
-	}
-    }
+    return value_cast_structs (type, arg2);
   if (code1 == TYPE_CODE_FLT && scalar)
     return value_from_double (type, value_as_double (arg2));
   else if (code1 == TYPE_CODE_DECFLOAT && scalar)

Re: [patch] fix for c++/2416

[Michael Snyder]

On Thu, 2008-02-28 at 13:53 -0500, Aleksandar Ristovski wrote:

> See revisited diff (attached). Also, please find attached the testcase diff, I 
> added check for reference casting. Do I need to provide change log for tests too?

Yes please.  

Thanks for your perseverance on this.  Good job.
(this is not formal approval, I haven't had a 
chance to review the new patch yet).

Re: [patch] fix for c++/2416

[Daniel Jacobowitz]

On Thu, Feb 28, 2008 at 01:53:13PM -0500, Aleksandar Ristovski wrote:
> See revisited diff (attached). Also, please find attached the testcase 
> diff, I added check for reference casting. Do I need to provide change log 
> for tests too?

The testsuite changes look fine but do need a changelog; that
goes in testsuite/ChangeLog.

> NOTE: value_as_address does *not* behave as you described, if passed in a 
> REF to STRUCT, it will coerce_ref, that is ok, but then it will pass 
> coerced value to unpack_long which doesn't know what to do with a STRUCT 
> (which is why I had that confusing code you and Michael complained about).

That's what I was trying to describe, sorry if it was unclear.
There's value_addr, which takes the address of its argument - give
it a struct or a struct reference, get back a struct pointer.  Then
there's value_as_address, which tries to interpret its argument as
an address.  Give it a pointer, get back the pointer as a CORE_ADDR.
Giving it a struct doesn't do anything useful.

> @@ -1985,8 +1985,18 @@ evaluate_subexp_standard (struct type *e
>        arg1 = evaluate_subexp (type, exp, pos, noside);
>        if (noside == EVAL_SKIP)
>  	goto nosideret;
> -      if (type != value_type (arg1))
> -	arg1 = value_cast (type, arg1);
> +      if (type != value_type (arg1)) 
> +	{
> +	  if (TYPE_CODE (value_type (arg1)) == TYPE_CODE_REF
> +	      && TYPE_CODE (type) == TYPE_CODE_REF)
> +	    arg1 = value_cast_pointers (type, arg1);
> +	  else if (TYPE_CODE (value_type (arg1)) != TYPE_CODE_REF
> +		   && TYPE_CODE (type) != TYPE_CODE_REF)
> +	    arg1 = value_cast (type, arg1);
> +	  else /* We can not do much here.  */
> +	    error (_("Attempt to cast to reference type from non-reference "\
> +		     "type or vice versa."));
> +	}
>        return arg1;
>  
>      case UNOP_MEMVAL:

There's nothing wrong with casting a reference to a non-reference;
that coerces the reference.  Also you have to beware typedefs.

	  enum type_code code1, code2;
	  code1 = TYPE_CODE (check_typedef (value_type (arg1)));
	  code2 = TYPE_CODE (check_typedef (value_type (arg2)));

	  if (code1 == TYPE_CODE_REF && code2 == TYPE_CODE_REF)
	    arg1 = value_cast_pointers (type, arg1);
	  else if (code1 == TYPE_CODE_REF)
	    error (_("Attempt to cast to reference type from non-reference type."));
	  else
	    arg1 = value_cast (type, arg1);

> Index: gdb/valops.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/valops.c,v
> retrieving revision 1.183
> diff -u -p -r1.183 valops.c
> --- gdb/valops.c	4 Feb 2008 00:23:04 -0000	1.183
> +++ gdb/valops.c	28 Feb 2008 18:47:50 -0000
> @@ -199,6 +199,7 @@ allocate_space_in_inferior (int len)
>  struct value *
>  value_cast_pointers (struct type *type, struct value *arg2)
>  {
> +  struct type *type1 = check_typedef (type);
>    struct type *type2 = check_typedef (value_type (arg2));
>    struct type *t1 = check_typedef (TYPE_TARGET_TYPE (type));
>    struct type *t2 = check_typedef (TYPE_TARGET_TYPE (type2));

Probably t1 should be set from type1.

>  				   value_zero (t1, not_lval), 0, t1, 1);
>  	  if (v)
>  	    {
> -	      CORE_ADDR addr2 = value_as_address (arg2);
> +	      /* Downcasting is possible (t1 is superclass of v2).  */
> +	      CORE_ADDR addr2 = VALUE_ADDRESS (v2);

Yep, that makes sense.  This was the real bug.

>  	      addr2 -= (VALUE_ADDRESS (v)
>  			+ value_offset (v)
>  			+ value_embedded_offset (v));
> -	      return value_from_pointer (type, addr2);
> +	      return value_from_pointer (type1, addr2);

Why type1?  It's nice to preserve the requested type on results,
when we can.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] fix for c++/2416

[Aleksandar Ristovski]

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

Daniel Jacobowitz wrote:
> On Wed, Feb 27, 2008 at 04:08:08PM -0500, Aleksandar Ristovski wrote:
>> Another thing that confuses me is such a special treatment for references 
>> to something... in my mind, (talking about C++) we should be able to 
>> internally treat them as pointers to that something.

I take this back, it looks like we do treat them the way I thought it should.

> 
> value_as_address on a reference is supposed to referenced value as an
> address, not the pointer value stored in the reference.  It's probably
> the caller of value_as_address which is mistaken.
> 
This was the real clue! Thank you for the feedback! Indeed, it appears calling 
value_as_address is not needed. Once we dereference PTR/REF, we get value 
representing "the real thing". From there we can fetch address for calculating 
the offset, and then all works fine.

See revisited diff (attached). Also, please find attached the testcase diff, I 
added check for reference casting. Do I need to provide change log for tests too?

NOTE: value_as_address does *not* behave as you described, if passed in a REF to 
STRUCT, it will coerce_ref, that is ok, but then it will pass coerced value to 
unpack_long which doesn't know what to do with a STRUCT (which is why I had that 
confusing code you and Michael complained about).


2008-02-28  Aleksandar Ristovski <aristovski@qnx.com>

	* eval.c (evaluate_subexp_standard): UNOP_CAST use
	value_cast_pointers when casting reference to reference. Print
	error when reference/non-reference mix.
	* valops.c (value_cast_pointers): Use coerced value to determine
	address when downcasting.


[-- Attachment #2: casting.diff --]
[-- Type: text/plain, Size: 3512 bytes --]

Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.80
diff -u -p -r1.80 eval.c
--- gdb/eval.c	4 Feb 2008 00:23:04 -0000	1.80
+++ gdb/eval.c	28 Feb 2008 18:47:40 -0000
@@ -1985,8 +1985,18 @@ evaluate_subexp_standard (struct type *e
       arg1 = evaluate_subexp (type, exp, pos, noside);
       if (noside == EVAL_SKIP)
 	goto nosideret;
-      if (type != value_type (arg1))
-	arg1 = value_cast (type, arg1);
+      if (type != value_type (arg1)) 
+	{
+	  if (TYPE_CODE (value_type (arg1)) == TYPE_CODE_REF
+	      && TYPE_CODE (type) == TYPE_CODE_REF)
+	    arg1 = value_cast_pointers (type, arg1);
+	  else if (TYPE_CODE (value_type (arg1)) != TYPE_CODE_REF
+		   && TYPE_CODE (type) != TYPE_CODE_REF)
+	    arg1 = value_cast (type, arg1);
+	  else /* We can not do much here.  */
+	    error (_("Attempt to cast to reference type from non-reference "\
+		     "type or vice versa."));
+	}
       return arg1;
 
     case UNOP_MEMVAL:
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.183
diff -u -p -r1.183 valops.c
--- gdb/valops.c	4 Feb 2008 00:23:04 -0000	1.183
+++ gdb/valops.c	28 Feb 2008 18:47:50 -0000
@@ -199,6 +199,7 @@ allocate_space_in_inferior (int len)
 struct value *
 value_cast_pointers (struct type *type, struct value *arg2)
 {
+  struct type *type1 = check_typedef (type);
   struct type *type2 = check_typedef (value_type (arg2));
   struct type *t1 = check_typedef (TYPE_TARGET_TYPE (type));
   struct type *t2 = check_typedef (TYPE_TARGET_TYPE (type2));
@@ -208,18 +209,20 @@ value_cast_pointers (struct type *type, 
       && !value_logical_not (arg2))
     {
       struct value *v;
+      struct value *v2; /* Real thing, dereferenced PTR/REF.  */
 
-      /* Look in the type of the source to see if it contains the
+      if (TYPE_CODE (type2) == TYPE_CODE_REF)
+	v2 = coerce_ref (arg2);
+      else
+	v2 = value_ind (arg2);
+      gdb_assert (TYPE_CODE (value_type (v2)) == TYPE_CODE_STRUCT
+		  && !!"Why did coercion fail?");
+
+      /* Upcasting: look in the type of the source to see if it contains the
 	 type of the target as a superclass.  If so, we'll need to
 	 offset the pointer rather than just change its type.  */
       if (TYPE_NAME (t1) != NULL)
 	{
-	  struct value *v2;
-
-	  if (TYPE_CODE (type2) == TYPE_CODE_REF)
-	    v2 = coerce_ref (arg2);
-	  else
-	    v2 = value_ind (arg2);
 	  v = search_struct_field (type_name_no_tag (t1),
 				   v2, 0, t2, 1);
 	  if (v)
@@ -230,7 +233,7 @@ value_cast_pointers (struct type *type, 
 	    }
 	}
 
-      /* Look in the type of the target to see if it contains the
+      /* Downcasting: look in the type of the target to see if it contains the
 	 type of the source as a superclass.  If so, we'll need to
 	 offset the pointer rather than just change its type.
 	 FIXME: This fails silently with virtual inheritance.  */
@@ -240,11 +243,12 @@ value_cast_pointers (struct type *type, 
 				   value_zero (t1, not_lval), 0, t1, 1);
 	  if (v)
 	    {
-	      CORE_ADDR addr2 = value_as_address (arg2);
+	      /* Downcasting is possible (t1 is superclass of v2).  */
+	      CORE_ADDR addr2 = VALUE_ADDRESS (v2);
 	      addr2 -= (VALUE_ADDRESS (v)
 			+ value_offset (v)
 			+ value_embedded_offset (v));
-	      return value_from_pointer (type, addr2);
+	      return value_from_pointer (type1, addr2);
 	    }
 	}
     }

[-- Attachment #3: casting.tests.diff --]
[-- Type: text/plain, Size: 1692 bytes --]

Index: gdb/testsuite/gdb.cp/casts.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/casts.cc,v
retrieving revision 1.1
diff -u -p -r1.1 casts.cc
--- gdb/testsuite/gdb.cp/casts.cc	23 Aug 2003 03:55:59 -0000	1.1
+++ gdb/testsuite/gdb.cp/casts.cc	28 Feb 2008 18:48:29 -0000
@@ -15,6 +15,8 @@ main (int argc, char **argv)
 {
   A *a = new B(42, 1729);
   B *b = (B *) a;
+  A &ar = *b;
+  B &br = (B&)ar;
 
   return 0;  /* breakpoint spot: casts.exp: 1 */
 }
Index: gdb/testsuite/gdb.cp/casts.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/casts.exp,v
retrieving revision 1.6
diff -u -p -r1.6 casts.exp
--- gdb/testsuite/gdb.cp/casts.exp	1 Jan 2008 22:53:19 -0000	1.6
+++ gdb/testsuite/gdb.cp/casts.exp	28 Feb 2008 18:48:29 -0000
@@ -80,3 +80,20 @@ gdb_test "print * (B *) a" ".* = {<A> = 
 # the dereference.
 gdb_test "print * b" ".* = {<A> = {a = 42}, b = 1729}" \
     "let compiler cast base class pointer to derived class pointer"
+
+# Check upcasting (it is trivial but still). 
+gdb_test "print * (A *) b" ".* = {a = 42}" \
+    "cast derived class pointer to base class pointer"
+
+# Casting References.
+# Check upcasting. 
+gdb_test "print (A &) br" ".* = .A &.* {a = 42}" \
+    "cast derived class reference to base class reference"
+
+# Check downcasting.
+gdb_test "print (B &) ar" ".* = .B.* {<A> = {a = 42}, b = 1729}" \
+    "cast base class reference to derived class reference"
+
+# Check compiler casting
+gdb_test "print br" ".* = .B.* {<A> = {a = 42}, b = 1729}" \
+    "let compiler cast base class reference to derived class reference"

Re: [patch] fix for c++/2416

[Daniel Jacobowitz]

On Wed, Feb 27, 2008 at 04:08:08PM -0500, Aleksandar Ristovski wrote:
> I haven't made any changes to coerce_array itself, only made sure we do 
> not call it if it's not an array. But I find its implementation somewhat 
> confusing: it looks like it can be called with all kinds of TYPE_CODE for 
> the arg... if that is correct than the name is misfortunate... and if not, 
> then we should determine why is it being called with TYPE_CODE_REF (my 
> patch eliminates one place where this is being done).

This arises from the C language standard coercions, more or less.
It is supposed to be called with references, functions, pointers, et
cetera.  If you give it an array, it gives you back a pointer to the
first element; the same if you do it with a reference to an array.

> Another thing that confuses me is such a special treatment for references 
> to something... in my mind, (talking about C++) we should be able to 
> internally treat them as pointers to that something.

No, we try to treat them as the thing referenced-to, not the pointer
doing the referencing.  This is to be consistent with the languages
we're evaluating.  Expression evaluation and printing goes wrong if
you don't.

> For example, in the fix I provided if you comment out
>
>   //if (TYPE_CODE (value_type (val)) == TYPE_CODE_ARRAY)
>
> in value.c:1042 (after applying my patch) it will call, as without the 
> patch, coerce_array; then if you follow to coerce_ref you will see that 
> after coerce_ref, address is messed up causing printing garbage (no 
> surprise since the address is wrong).

value_as_address on a reference is supposed to referenced value as an
address, not the pointer value stored in the reference.  It's probably
the caller of value_as_address which is mistaken.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] fix for c++/2416

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> 
> For expressions, casting one reference to another is OK; casting a
> reference to a non-reference is also fine.  Only non-reference to
> reference is trouble.
> 
>>> The new error is incorrect, which does suggest some missing tests.
>>> You can cast from a reference type; value_cast follows references,
>>> so an int is just like an int &.
>>>
>>> Like Michael, I don't understand the value.c changes.  Could you
>>> explain them?
>>>
>> I replied to that.
> 
> What about the changes to coerce_array?
> 

I haven't made any changes to coerce_array itself, only made sure we do not call 
it if it's not an array. But I find its implementation somewhat confusing: it 
looks like it can be called with all kinds of TYPE_CODE for the arg... if that 
is correct than the name is misfortunate... and if not, then we should determine 
why is it being called with TYPE_CODE_REF (my patch eliminates one place where 
this is being done).

Another thing that confuses me is such a special treatment for references to 
something... in my mind, (talking about C++) we should be able to internally 
treat them as pointers to that something. For example, in the fix I provided if 
you comment out

   //if (TYPE_CODE (value_type (val)) == TYPE_CODE_ARRAY)

in value.c:1042 (after applying my patch) it will call, as without the patch, 
coerce_array; then if you follow to coerce_ref you will see that after 
coerce_ref, address is messed up causing printing garbage (no surprise since the 
address is wrong).


Unfortunately I do not have time at the moment to get any deeper than this, but 
I will reiterate (and please feel free to enlighten me with the reasons) that I 
do not see reasons for treating TYPE_CODE_REF in a different way than TYPE_CODE_PTR.

Re: [patch] fix for c++/2416

[Daniel Jacobowitz]

On Wed, Feb 27, 2008 at 02:48:20PM -0500, Aleksandar Ristovski wrote:
> Not sure what you mean by "C++ does not permit this"...
>
> This is valid:
>
> void testCast1 (IP::base & arg) {
>   IP::derived &ader = (IP::derived &)arg;
>   cout << "Test the casting\n";
>   ader.foo();
> }

Humph.  You're right, of course.  When I added that assertion I
must not have thought about this case.  I think you've put the fix
in the right place; the reference behavior is a bit startling, so
we would like to know if it comes up anywhere else in GDB besides from
an expression.

For expressions, casting one reference to another is OK; casting a
reference to a non-reference is also fine.  Only non-reference to
reference is trouble.

>> The new error is incorrect, which does suggest some missing tests.
>> You can cast from a reference type; value_cast follows references,
>> so an int is just like an int &.
>>
>> Like Michael, I don't understand the value.c changes.  Could you
>> explain them?
>>
> I replied to that.

What about the changes to coerce_array?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] fix for c++/2416

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Wed, Feb 27, 2008 at 01:03:26PM -0500, Aleksandar Ristovski wrote:
>> Hello,
>>
>> As described in the bug report 2416, the problem is with casting to a 
>> reference. The attached patch should fix this.
> 
>   /* You can't cast to a reference type.  See value_cast_pointers
>      instead.  */
>   gdb_assert (code1 != TYPE_CODE_REF);
> 
> Is casting to a reference type useful, or should we have issued an
> error instead?  C++ does not permit this.
Not sure what you mean by "C++ does not permit this"...

This is valid:

void testCast1 (IP::base & arg) {
   IP::derived &ader = (IP::derived &)arg;
   cout << "Test the casting\n";
   ader.foo();
}

> 
> The new error is incorrect, which does suggest some missing tests.
> You can cast from a reference type; value_cast follows references,
> so an int is just like an int &.
> 
> Like Michael, I don't understand the value.c changes.  Could you
> explain them?
> 
I replied to that.

Re: [patch] fix for c++/2416

[Daniel Jacobowitz]

On Wed, Feb 27, 2008 at 01:03:26PM -0500, Aleksandar Ristovski wrote:
> Hello,
>
> As described in the bug report 2416, the problem is with casting to a 
> reference. The attached patch should fix this.

  /* You can't cast to a reference type.  See value_cast_pointers
     instead.  */
  gdb_assert (code1 != TYPE_CODE_REF);

Is casting to a reference type useful, or should we have issued an
error instead?  C++ does not permit this.

The new error is incorrect, which does suggest some missing tests.
You can cast from a reference type; value_cast follows references,
so an int is just like an int &.

Like Michael, I don't understand the value.c changes.  Could you
explain them?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] fix for c++/2416

[Aleksandar Ristovski]

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

Michael Snyder wrote:
> On Wed, 2008-02-27 at 13:03 -0500, Aleksandar Ristovski wrote:
>> Hello,
>>
>> As described in the bug report 2416, the problem is with casting to a reference. 
>> The attached patch should fix this.
>>
>> Testing on head, no change in pass rate encountered (probably suggesting there 
>> is no test case for this - sample code in the bug report should be a good 
>> starting point).
> 
> I don't understand the change in unpack_long.  
> You want to treat structs and unions as longs?

Hmm... I thought I put it there to handle cases when we end up with these type 
codes; in that case, I thought, value contains the address and we intepret it as 
a long...

However, now I am having trouble reproducing this case, so maybe I had "covered" 
this scenario while still searching for a more general solution... in any case, 
tests give the same results without unpack_long change and the new diff is attached.

Thanks,

Aleksandar


2008-02-27  Aleksandar Ristovski <aristovski@qnx.com>

	* eval.c (evaluate_subexp_standard): UNOP_CAST use
	value_cast_pointers when casting reference to reference. Print
	error when reference/non-reference mix.
	* value.c (value_as_address): Call coerce_array only on arrays.



[-- Attachment #2: casting.diff --]
[-- Type: text/plain, Size: 1698 bytes --]

Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.80
diff -u -p -r1.80 eval.c
--- gdb/eval.c	4 Feb 2008 00:23:04 -0000	1.80
+++ gdb/eval.c	27 Feb 2008 19:02:41 -0000
@@ -1985,8 +1985,18 @@ evaluate_subexp_standard (struct type *e
       arg1 = evaluate_subexp (type, exp, pos, noside);
       if (noside == EVAL_SKIP)
 	goto nosideret;
-      if (type != value_type (arg1))
-	arg1 = value_cast (type, arg1);
+      if (type != value_type (arg1)) 
+	{
+	  if (TYPE_CODE (value_type (arg1)) == TYPE_CODE_REF
+	      && TYPE_CODE (type) == TYPE_CODE_REF)
+	    arg1 = value_cast_pointers (type, arg1);
+	  else if (TYPE_CODE (value_type (arg1)) != TYPE_CODE_REF
+		   && TYPE_CODE (type) != TYPE_CODE_REF)
+	    arg1 = value_cast (type, arg1);
+	  else /* We can not do much here.  */
+	    error (_("Attempt to cast to reference type from non-reference "\
+		     "type or vice versa."));
+	}
       return arg1;
 
     case UNOP_MEMVAL:
Index: gdb/value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.57
diff -u -p -r1.57 value.c
--- gdb/value.c	18 Jan 2008 17:07:40 -0000	1.57
+++ gdb/value.c	27 Feb 2008 19:02:44 -0000
@@ -1039,7 +1039,8 @@ value_as_address (struct value *val)
       || TYPE_CODE (value_type (val)) == TYPE_CODE_METHOD)
     return VALUE_ADDRESS (val);
 
-  val = coerce_array (val);
+  if (TYPE_CODE (value_type (val)) == TYPE_CODE_ARRAY)
+    val = coerce_array (val);
 
   /* Some architectures (e.g. Harvard), map instruction and data
      addresses onto a single large unified address space.  For

Re: [patch] fix for c++/2416

[Michael Snyder]

On Wed, 2008-02-27 at 13:03 -0500, Aleksandar Ristovski wrote:
> Hello,
> 
> As described in the bug report 2416, the problem is with casting to a reference. 
> The attached patch should fix this.
> 
> Testing on head, no change in pass rate encountered (probably suggesting there 
> is no test case for this - sample code in the bug report should be a good 
> starting point).

I don't understand the change in unpack_long.  
You want to treat structs and unions as longs?

Is this related to the reference-to-reference change?

> 2008-02-27  Aleksandar Ristovski <aristovski@qnx.com>
> 
> 	* eval.c (evaluate_subexp_standard): UNOP_CAST use
> 	value_cast_pointers when casting reference to reference. Print
> 	error when reference/non-reference mix.
> 	* value.c (value_as_address): Call coerce_array only on arrays.
> 	(unpack_long): Cover C++ cases TYPE_CODE_STRUCT and UNION.
> 
> 
> plain text document attachment (casting.diff)
> Index: gdb/eval.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/eval.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 eval.c
> --- gdb/eval.c	4 Feb 2008 00:23:04 -0000	1.80
> +++ gdb/eval.c	27 Feb 2008 17:01:03 -0000
> @@ -1985,8 +1985,18 @@ evaluate_subexp_standard (struct type *e
>        arg1 = evaluate_subexp (type, exp, pos, noside);
>        if (noside == EVAL_SKIP)
>  	goto nosideret;
> -      if (type != value_type (arg1))
> -	arg1 = value_cast (type, arg1);
> +      if (type != value_type (arg1)) 
> +	{
> +	  if (TYPE_CODE (value_type (arg1)) == TYPE_CODE_REF
> +	      && TYPE_CODE (type) == TYPE_CODE_REF)
> +	    arg1 = value_cast_pointers (type, arg1);
> +	  else if (TYPE_CODE (value_type (arg1)) != TYPE_CODE_REF
> +		   && TYPE_CODE (type) != TYPE_CODE_REF)
> +	    arg1 = value_cast (type, arg1);
> +	  else /* We can not do much here.  */
> +	    error (_("Attempt to cast to reference type from non-reference "\
> +		     "type or vice versa."));
> +	}
>        return arg1;
>  
>      case UNOP_MEMVAL:
> Index: gdb/value.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/value.c,v
> retrieving revision 1.57
> diff -u -p -r1.57 value.c
> --- gdb/value.c	18 Jan 2008 17:07:40 -0000	1.57
> +++ gdb/value.c	27 Feb 2008 17:01:03 -0000
> @@ -1039,7 +1039,8 @@ value_as_address (struct value *val)
>        || TYPE_CODE (value_type (val)) == TYPE_CODE_METHOD)
>      return VALUE_ADDRESS (val);
>  
> -  val = coerce_array (val);
> +  if (TYPE_CODE (value_type (val)) == TYPE_CODE_ARRAY)
> +    val = coerce_array (val);
>  
>    /* Some architectures (e.g. Harvard), map instruction and data
>       addresses onto a single large unified address space.  For
> @@ -1120,6 +1121,8 @@ unpack_long (struct type *type, const gd
>      case TYPE_CODE_CHAR:
>      case TYPE_CODE_RANGE:
>      case TYPE_CODE_MEMBERPTR:
> +    case TYPE_CODE_STRUCT:
> +    case TYPE_CODE_UNION:
>        if (nosign)
>  	return extract_unsigned_integer (valaddr, len);
>        else

[patch] fix for c++/2416

[Aleksandar Ristovski]

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

Hello,

As described in the bug report 2416, the problem is with casting to a reference. 
The attached patch should fix this.

Testing on head, no change in pass rate encountered (probably suggesting there 
is no test case for this - sample code in the bug report should be a good 
starting point).

Thanks,

Aleksandar Ristovski
QNX Software Systems

2008-02-27  Aleksandar Ristovski <aristovski@qnx.com>

	* eval.c (evaluate_subexp_standard): UNOP_CAST use
	value_cast_pointers when casting reference to reference. Print
	error when reference/non-reference mix.
	* value.c (value_as_address): Call coerce_array only on arrays.
	(unpack_long): Cover C++ cases TYPE_CODE_STRUCT and UNION.



[-- Attachment #2: casting.diff --]
[-- Type: text/plain, Size: 1981 bytes --]

Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.80
diff -u -p -r1.80 eval.c
--- gdb/eval.c	4 Feb 2008 00:23:04 -0000	1.80
+++ gdb/eval.c	27 Feb 2008 17:01:03 -0000
@@ -1985,8 +1985,18 @@ evaluate_subexp_standard (struct type *e
       arg1 = evaluate_subexp (type, exp, pos, noside);
       if (noside == EVAL_SKIP)
 	goto nosideret;
-      if (type != value_type (arg1))
-	arg1 = value_cast (type, arg1);
+      if (type != value_type (arg1)) 
+	{
+	  if (TYPE_CODE (value_type (arg1)) == TYPE_CODE_REF
+	      && TYPE_CODE (type) == TYPE_CODE_REF)
+	    arg1 = value_cast_pointers (type, arg1);
+	  else if (TYPE_CODE (value_type (arg1)) != TYPE_CODE_REF
+		   && TYPE_CODE (type) != TYPE_CODE_REF)
+	    arg1 = value_cast (type, arg1);
+	  else /* We can not do much here.  */
+	    error (_("Attempt to cast to reference type from non-reference "\
+		     "type or vice versa."));
+	}
       return arg1;
 
     case UNOP_MEMVAL:
Index: gdb/value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.57
diff -u -p -r1.57 value.c
--- gdb/value.c	18 Jan 2008 17:07:40 -0000	1.57
+++ gdb/value.c	27 Feb 2008 17:01:03 -0000
@@ -1039,7 +1039,8 @@ value_as_address (struct value *val)
       || TYPE_CODE (value_type (val)) == TYPE_CODE_METHOD)
     return VALUE_ADDRESS (val);
 
-  val = coerce_array (val);
+  if (TYPE_CODE (value_type (val)) == TYPE_CODE_ARRAY)
+    val = coerce_array (val);
 
   /* Some architectures (e.g. Harvard), map instruction and data
      addresses onto a single large unified address space.  For
@@ -1120,6 +1121,8 @@ unpack_long (struct type *type, const gd
     case TYPE_CODE_CHAR:
     case TYPE_CODE_RANGE:
     case TYPE_CODE_MEMBERPTR:
+    case TYPE_CODE_STRUCT:
+    case TYPE_CODE_UNION:
       if (nosign)
 	return extract_unsigned_integer (valaddr, len);
       else