Re: [PATCH] Fix type casts losing typedefs and reimplement "whatis" typedef stripping

[Simon Marchi <simon.marchi@ericsson.com>]

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