Fix type casts losing typedefs and reimplement "whatis" typedef stripping

  On 11/18/2017 10:58 PM, Simon Marchi wrote:
> On 2017-11-18 03:57 PM, Yao Qi wrote:
>> On Mon, Aug 21, 2017 at 11:38 AM, Pedro Alves <palves@redhat.com> wrote:
>>
>> Hi Pedro,
>> The new tests in gdb.base/whatis-ptype-typedefs.exp fail on 32-bit target.
>>
>> https://gdb-build.sergiodj.net/builders/Ubuntu-AArch32-m32/builds/1175/steps/test%20gdb/logs/stdio
>> https://gdb-build.sergiodj.net/builders/Fedora-i686/builds/6867/steps/test%20gdb/logs/stdio
>> https://gdb-build.sergiodj.net/builders/Fedora-x86_64-m32/builds/6849/steps/test%20gdb/logs/stdio
>>
>> Can you take a look?
>>
> 
> I took a quick look.  The issue (at least one of them) boils down to the fact
> that on 64 bits, you can't do this:
> 
> (gdb) p (float_typedef) v_uchar_array_t_struct_typedef
> Invalid cast.
> 
> but on 32 bits you can:
> 
> (gdb) p (float_typedef) v_uchar_array_t_struct_typedef
> $1 = 1.16251721e-41
> 
> The expression basically tries to cast an array (which decays to a pointer) to
> a float.  The cast works on 32 bits (doesn't give Invalid cast) because a float
> and a pointer are of the same size, and the execution enters this if branch:
> 
> https://github.com/bminor/binutils-gdb/blob/master/gdb/valops.c#L554
> 
> On 64 bits, they are not the same size, so it ends up in the invalid cast
> branch.
> 
> I don't really know what to do from there.  Should we leave the behavior as-is
> and update the test, or prevent this kind of cast (the compiler doesn't accept
> that anyway, and I don't see any real use case to this).  This function (value_cast)
> is a bit convoluted, I'm always afraid to touch it...

I'm not 100% sure either.  value_cast is documented as:
~~~
   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.  */
~~~

and I've found this useful on several occasions (though for me it's
usually more around converting an object to some structure).

The point of the test was to cover as many of code paths in
value_cast as possible, as a sort of documentation of the current
behavior:


    # The main idea here is testing all the different paths in the
    # value casting code in GDB (value_cast), making sure typedefs are
    # preserved.
...
		# We try all combinations, even those that don't
		# parse, or are invalid, to catch the case of a
		# regression making them inadvertently valid.  For
		# example, these convertions are invalid:
...

So in that spirit, I propose starting my making the testcase adjust
itself, like below, and also test floats of different sizes, leaving
changing GDB's behavior for a separate consideration/change (using
the fixed/extended test as baseline).

This passes on x86 both 64-bit and 32-bit.

From 1a02eedaadcb7b62a5990f5838952b0600d4a8cc Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 20 Nov 2017 16:39:58 +0000
Subject: [PATCH] fix

---
 gdb/testsuite/gdb.base/whatis-ptype-typedefs.c   | 10 +++++++
 gdb/testsuite/gdb.base/whatis-ptype-typedefs.exp | 37 ++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 3 deletions(-)
  

On 2017-11-20 11:42 AM, Pedro Alves wrote:
> On 11/18/2017 10:58 PM, Simon Marchi wrote:
>> On 2017-11-18 03:57 PM, Yao Qi wrote:
>>> On Mon, Aug 21, 2017 at 11:38 AM, Pedro Alves <palves@redhat.com> wrote:
>>>
>>> Hi Pedro,
>>> The new tests in gdb.base/whatis-ptype-typedefs.exp fail on 32-bit target.
>>>
>>> https://gdb-build.sergiodj.net/builders/Ubuntu-AArch32-m32/builds/1175/steps/test%20gdb/logs/stdio
>>> https://gdb-build.sergiodj.net/builders/Fedora-i686/builds/6867/steps/test%20gdb/logs/stdio
>>> https://gdb-build.sergiodj.net/builders/Fedora-x86_64-m32/builds/6849/steps/test%20gdb/logs/stdio
>>>
>>> Can you take a look?
>>>
>>
>> I took a quick look.  The issue (at least one of them) boils down to the fact
>> that on 64 bits, you can't do this:
>>
>> (gdb) p (float_typedef) v_uchar_array_t_struct_typedef
>> Invalid cast.
>>
>> but on 32 bits you can:
>>
>> (gdb) p (float_typedef) v_uchar_array_t_struct_typedef
>> $1 = 1.16251721e-41
>>
>> The expression basically tries to cast an array (which decays to a pointer) to
>> a float.  The cast works on 32 bits (doesn't give Invalid cast) because a float
>> and a pointer are of the same size, and the execution enters this if branch:
>>
>> https://github.com/bminor/binutils-gdb/blob/master/gdb/valops.c#L554
>>
>> On 64 bits, they are not the same size, so it ends up in the invalid cast
>> branch.
>>
>> I don't really know what to do from there.  Should we leave the behavior as-is
>> and update the test, or prevent this kind of cast (the compiler doesn't accept
>> that anyway, and I don't see any real use case to this).  This function (value_cast)
>> is a bit convoluted, I'm always afraid to touch it...
> 
> I'm not 100% sure either.  value_cast is documented as:
> ~~~
>    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.  */
> ~~~
> 
> and I've found this useful on several occasions (though for me it's
> usually more around converting an object to some structure).
> 
> The point of the test was to cover as many of code paths in
> value_cast as possible, as a sort of documentation of the current
> behavior:
> 
> 
>     # The main idea here is testing all the different paths in the
>     # value casting code in GDB (value_cast), making sure typedefs are
>     # preserved.
> ...
> 		# We try all combinations, even those that don't
> 		# parse, or are invalid, to catch the case of a
> 		# regression making them inadvertently valid.  For
> 		# example, these convertions are invalid:
> ...
> 
> So in that spirit, I propose starting my making the testcase adjust
> itself, like below, and also test floats of different sizes, leaving
> changing GDB's behavior for a separate consideration/change (using
> the fixed/extended test as baseline).

That's fine with me, it's strange at first that it works on an architecture
but not the other, but it makes sense when you know why.

> 
> This passes on x86 both 64-bit and 32-bit.
> 
> From 1a02eedaadcb7b62a5990f5838952b0600d4a8cc Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 20 Nov 2017 16:39:58 +0000
> Subject: [PATCH] fix
> 
> ---
>  gdb/testsuite/gdb.base/whatis-ptype-typedefs.c   | 10 +++++++
>  gdb/testsuite/gdb.base/whatis-ptype-typedefs.exp | 37 ++++++++++++++++++++++--
>  2 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/whatis-ptype-typedefs.c b/gdb/testsuite/gdb.base/whatis-ptype-typedefs.c
> index 5711a96..35c7279 100644
> --- a/gdb/testsuite/gdb.base/whatis-ptype-typedefs.c
> +++ b/gdb/testsuite/gdb.base/whatis-ptype-typedefs.c
> @@ -56,6 +56,16 @@ DEF (int);
>  typedef float float_typedef;
>  DEF (float);
>  
> +/* Double floats.  */
> +
> +typedef double double_typedef;
> +DEF (double);
> +
> +/* Long doubles.  */
> +
> +typedef long double long_double_typedef;
> +DEF (long_double);
> +
>  /* Enums.  */
>  
>  typedef enum colors {red, green, blue} colors_typedef;
> diff --git a/gdb/testsuite/gdb.base/whatis-ptype-typedefs.exp b/gdb/testsuite/gdb.base/whatis-ptype-typedefs.exp
> index d333d81..c8fa2bd 100644
> --- a/gdb/testsuite/gdb.base/whatis-ptype-typedefs.exp
> +++ b/gdb/testsuite/gdb.base/whatis-ptype-typedefs.exp
> @@ -92,6 +92,16 @@ set table {
>      {"v_float_typedef"    "float_typedef"    "float"}
>      {"v_float_typedef2"   "float_typedef2"   "float"}
>  
> +    {"double_typedef"     "double"           "double"}
> +    {"double_typedef2"    "double_typedef"   "double"}
> +    {"v_double_typedef"   "double_typedef"   "double"}
> +    {"v_double_typedef2"  "double_typedef2"  "double"}
> +
> +    {"long_double_typedef"    "long double"           "long double"}
> +    {"long_double_typedef2"   "long_double_typedef"   "long double"}
> +    {"v_long_double_typedef"  "long_double_typedef"   "long double"}
> +    {"v_long_double_typedef2" "long_double_typedef2"  "long double"}
> +
>      {"colors_typedef"     "(enum )?colors"   "enum colors( : unsigned int)? {red, green, blue}"}
>      {"colors_typedef2"    "colors_typedef"   "enum colors( : unsigned int)? {red, green, blue}"}
>      {"v_colors_typedef"   "colors_typedef"   "enum colors( : unsigned int)? {red, green, blue}"}
> @@ -199,6 +209,20 @@ proc run_tests {lang} {
>  	}
>      }
>  
> +    # If floats and pointers have he same on this architecture, then

This sentence weird.

> +    # casting from array/function to float works, because
> +    # arrays/functions first decay to pointers, and then GDB's cast is
> +    # more general than a C cast and accepts any two types of the same
> +    # length.
> +    set float_ptr_same_size \
> +	[get_integer_valueof "sizeof (float) == sizeof (void *)" -1]
> +
> +    set double_ptr_same_size \
> +	[get_integer_valueof "sizeof (double) == sizeof (void *)" -1]
> +
> +    set long_double_ptr_same_size \
> +	[get_integer_valueof "sizeof (long double) == sizeof (void *)" -1]
> +
>      # Test converting/casting all variables in the first column of the
>      # table to all types (found in the first column of the table).
>      # The aggregates are all defined to be the same size so that
> @@ -230,7 +254,7 @@ proc run_tests {lang} {
>  		# regression making them inadvertently valid.  For
>  		# example, these convertions are invalid:

Not in this patch, but "convertions".

>  		#
> -		#  float <-> array
> +		#  float <-> array   [iff sizeof pointer != sizeof float]
>  		#  array -> function (not function pointer)
>  		#  array -> member_ptr
>  		#
> @@ -247,8 +271,15 @@ proc run_tests {lang} {
>  		    gdb_test "whatis ($to) $from" "syntax error.*" "whatis ($to) $from (syntax)"
>  		    gdb_test "ptype ($to) $from" "syntax error.*" "ptype ($to) $from (syntax)"
>  		} elseif {([string match "*float*" $from] && [string match "*array*" $to])
> -			  || ([string match "float*" $to] && [string match "*array*" $from])
> -			  || ([string match "float*" $to] && [string match "*method" $from])
> +			  || (!$float_ptr_same_size
> +			      && ([string match "float*" $to] && [string match "*array*" $from]
> +				  || [string match "float*" $to] && [string match "*method" $from]))
> +			  || (!$double_ptr_same_size
> +			      && ([string match "double*" $to] && [string match "*array*" $from]
> +				  || [string match "double*" $to] && [string match "*method" $from]))
> +			  || (!$long_double_ptr_same_size
> +			      && ([string match "long_double*" $to] && [string match "*array*" $from]
> +				  || [string match "long_double*" $to] && [string match "*method" $from]))
>  			  || ([string match "*ftype" $to] && [string match "*array*" $from])
>  			  || ([string match "*ftype2" $to] && [string match "*array*" $from])
>  			  || ([string match "*ftype" $to] && [string match "*method" $from])
> 

Otherwise, LGTM.  I haven't tested it, but I trust that you did :)

Simon
  

On 17-11-20 16:42:48, Pedro Alves wrote:
> 
> So in that spirit, I propose starting my making the testcase adjust
> itself, like below, and also test floats of different sizes, leaving
> changing GDB's behavior for a separate consideration/change (using
> the fixed/extended test as baseline).
> 

I agree.  Some tests fail on 32-bit at the point when they were added,
so tests should match gdb's behaviour.  As usual, we can gdb behaviour
after we are sure the behaviour is incorrect.

Beside Simon's review, patch is good to me.
  

On Nov 20 2017, Pedro Alves <palves@redhat.com> wrote:

> +    # If floats and pointers have he same on this architecture, then

the same size

Andreas.