AArch64: Prevent infinite recursion when checking AAPCS types

  gdb.dwarf2/dw2-cp-infcall-ref-static.exp includes a struct which
contains itself.  Add support to aapcs_is_vfp_call_or_return_candidate to
prevent infinite recursion.

The simple solution is to pass a counter through the functions and abort once
the max limit is reached.  However, this does not catch the case where a
struct only contains itself and no other members.  That can be solved by
marking a type as invalid before checking all of it's children.

Add a count for the call or ret candidate to main_type, restricted to 3
bits - the value 5 (or above) is used as invalid.

This code is testsed by both gdb.base/infcall-nested-structs.exp
and gdb.dwarf2/dw2-cp-infcall-ref-static.exp.

gdb/ChangeLog:

2018-12-12  Alan Hayward  <alan.hayward@arm.com>

	* aarch64-tdep.c (HA_MAX_NUM_FLDS): Remove define.
	(aapcs_is_vfp_call_or_return_candidate_1): Remember type.
	(aapcs_is_vfp_call_or_return_candidate): Likewise.
	* gdbtypes.h (AAPCS_VFP_CALL_OR_RET_CAND_MAX): Add define.
	(AAPCS_VFP_CALL_OR_RET_CAND_INVALID): Likewise.
	(TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.
	(TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.
	(struct main_type): Add aapcs_candidate_count.
---
 gdb/aarch64-tdep.c | 87 ++++++++++++++++++++++++++--------------------
 gdb/gdbtypes.h     | 23 ++++++++++++
 2 files changed, 72 insertions(+), 38 deletions(-)
  

Ping.

> On 12 Dec 2018, at 09:59, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> gdb.dwarf2/dw2-cp-infcall-ref-static.exp includes a struct which
> contains itself.  Add support to aapcs_is_vfp_call_or_return_candidate to
> prevent infinite recursion.
> 
> The simple solution is to pass a counter through the functions and abort once
> the max limit is reached.  However, this does not catch the case where a
> struct only contains itself and no other members.  That can be solved by
> marking a type as invalid before checking all of it's children.
> 
> Add a count for the call or ret candidate to main_type, restricted to 3
> bits - the value 5 (or above) is used as invalid.
> 
> This code is testsed by both gdb.base/infcall-nested-structs.exp
> and gdb.dwarf2/dw2-cp-infcall-ref-static.exp.
> 
> gdb/ChangeLog:
> 
> 2018-12-12  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* aarch64-tdep.c (HA_MAX_NUM_FLDS): Remove define.
> 	(aapcs_is_vfp_call_or_return_candidate_1): Remember type.
> 	(aapcs_is_vfp_call_or_return_candidate): Likewise.
> 	* gdbtypes.h (AAPCS_VFP_CALL_OR_RET_CAND_MAX): Add define.
> 	(AAPCS_VFP_CALL_OR_RET_CAND_INVALID): Likewise.
> 	(TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.
> 	(TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.
> 	(struct main_type): Add aapcs_candidate_count.
> ---
> gdb/aarch64-tdep.c | 87 ++++++++++++++++++++++++++--------------------
> gdb/gdbtypes.h     | 23 ++++++++++++
> 2 files changed, 72 insertions(+), 38 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index ae56c9ca34..3fc58308d6 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -64,10 +64,6 @@
> #define bit(obj,st) (((obj) >> (st)) & 1)
> #define bits(obj,st,fn) (((obj) >> (st)) & submask ((fn) - (st)))
> 
> -/* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
> -   four members.  */
> -#define HA_MAX_NUM_FLDS		4
> -
> /* All possible aarch64 target descriptors.  */
> struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];
> 
> @@ -1146,46 +1142,58 @@ aarch64_type_align (struct type *t)
> 
> /* Worker function for aapcs_is_vfp_call_or_return_candidate.
> 
> -   Return the number of register required, or -1 on failure.
> +   Set TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE to the number of registers required,
> +   or AAPCS_VFP_CALL_OR_RET_CAND_INVALID on failure.
> 
>    When encountering a base element, if FUNDAMENTAL_TYPE is not set then set it
>    to the element, else fail if the type of this element does not match the
>    existing value.  */
> 
> -static int
> +static void
> aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
> 					 struct type **fundamental_type)
> {
>   if (type == nullptr)
> -    return -1;
> +    return;
> +
> +  /* Check if we have already evaluated this TYPE.  */
> +  if (TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type)
> +      >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)
> +    return;
> +
> +  /* Set TYPE to invalid to prevent infinite recursion.  */
> +  TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type,
> +					    AAPCS_VFP_CALL_OR_RET_CAND_INVALID);
> 
>   switch (TYPE_CODE (type))
>     {
>     case TYPE_CODE_FLT:
>       if (TYPE_LENGTH (type) > 16)
> -	return -1;
> +	return;
> 
>       if (*fundamental_type == nullptr)
> 	*fundamental_type = type;
>       else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)
> 	       || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))
> -	return -1;
> +	return;
> 
> -      return 1;
> +      TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);
> +      return;
> 
>     case TYPE_CODE_COMPLEX:
>       {
> 	struct type *target_type = check_typedef (TYPE_TARGET_TYPE (type));
> 	if (TYPE_LENGTH (target_type) > 16)
> -	  return -1;
> +	  return;
> 
> 	if (*fundamental_type == nullptr)
> 	  *fundamental_type = target_type;
> 	else if (TYPE_LENGTH (target_type) != TYPE_LENGTH (*fundamental_type)
> 		 || TYPE_CODE (target_type) != TYPE_CODE (*fundamental_type))
> -	  return -1;
> +	  return;
> 
> -	return 2;
> +	TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 2);
> +	return;
>       }
> 
>     case TYPE_CODE_ARRAY:
> @@ -1193,28 +1201,33 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
> 	if (TYPE_VECTOR (type))
> 	  {
> 	    if (TYPE_LENGTH (type) != 8 && TYPE_LENGTH (type) != 16)
> -	      return -1;
> +	      return;
> 
> 	    if (*fundamental_type == nullptr)
> 	      *fundamental_type = type;
> 	    else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)
> 		     || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))
> -	      return -1;
> +	      return;
> 
> -	    return 1;
> +	    TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);
> 	  }
> 	else
> 	  {
> +	    /* Calculate if type of an element in the array is valid.  */
> 	    struct type *target_type = TYPE_TARGET_TYPE (type);
> -	    int count = aapcs_is_vfp_call_or_return_candidate_1
> -			  (target_type, fundamental_type);
> +	    aapcs_is_vfp_call_or_return_candidate_1 (target_type,
> +						     fundamental_type);
> 
> -	    if (count == -1)
> -	      return count;
> +	    int count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (target_type);
> +	    if (count >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)
> +	      return;
> +
> +	    /* Expand to cover the whole array.  The set handles overflow.  */
> 
> 	    count *= (TYPE_LENGTH (type) / TYPE_LENGTH (target_type));
> -	      return count;
> +	    TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);
> 	  }
> +	return;
>       }
> 
>     case TYPE_CODE_STRUCT:
> @@ -1226,20 +1239,23 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
> 	  {
> 	    struct type *member = check_typedef (TYPE_FIELD_TYPE (type, i));
> 
> -	    int sub_count = aapcs_is_vfp_call_or_return_candidate_1
> -			      (member, fundamental_type);
> -	    if (sub_count == -1)
> -	      return -1;
> +	    aapcs_is_vfp_call_or_return_candidate_1 (member, fundamental_type);
> +
> +	    int sub_count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (member);
> +
> +	    if (sub_count + count > AAPCS_VFP_CALL_OR_RET_CAND_MAX)
> +	      return;
> +
> 	    count += sub_count;
> 	  }
> -	return count;
> +
> +	TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);
> +	return;
>       }
> 
>     default:
> -      break;
> +      return;
>     }
> -
> -  return -1;
> }
> 
> /* Return true if an argument, whose type is described by TYPE, can be passed or
> @@ -1269,16 +1285,11 @@ aapcs_is_vfp_call_or_return_candidate (struct type *type, int *count,
> 
>   *fundamental_type = nullptr;
> 
> -  int ag_count = aapcs_is_vfp_call_or_return_candidate_1 (type,
> -							  fundamental_type);
> +  aapcs_is_vfp_call_or_return_candidate_1 (type, fundamental_type);
> 
> -  if (ag_count > 0 && ag_count <= HA_MAX_NUM_FLDS)
> -    {
> -      *count = ag_count;
> -      return true;
> -    }
> -  else
> -    return false;
> +  *count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type);
> +
> +  return (*count < AAPCS_VFP_CALL_OR_RET_CAND_INVALID);
> }
> 
> /* AArch64 function call information structure.  */
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index f0adec7a20..600112abf5 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -391,6 +391,23 @@ DEF_ENUM_FLAGS_TYPE (enum type_instance_flag_value, type_instance_flags);
> #define TYPE_ADDRESS_CLASS_ALL(t) (TYPE_INSTANCE_FLAGS(t) \
> 				   & TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)
> 
> +
> +/* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call
> +   parameter or return parameter, as per the AAPCS64 5.4.2.C.  Value can be:
> +     0: the type has not been checked.
> +     1 to AAPCS_VFP_CALL_OR_RET_CAND_MAX: Valid, and has that many members.
> +     AAPCS_VFP_CALL_OR_RET_CAND_MAX: Invalid.  */
> +
> +#define AAPCS_VFP_CALL_OR_RET_CAND_MAX 4
> +#define AAPCS_VFP_CALL_OR_RET_CAND_INVALID (AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1)
> +
> +#define TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype) \
> +  (TYPE_MAIN_TYPE (thistype)->aapcs_candidate_count)
> +
> +#define TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype, val) \
> +  TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (thistype) = \
> +    (std::min (val, AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1))
> +
> /* * Information needed for a discriminated union.  A discriminated
>    union is handled somewhat differently from an ordinary union.
> 
> @@ -716,6 +733,12 @@ struct main_type
> 
>   ENUM_BITFIELD(type_specific_kind) type_specific_field : 3;
> 
> +  /* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call
> +     parameter or return parameter, as per the AAPCS64 5.4.2.C.  Required to
> +     prevent recursive checking.  */
> +
> +  unsigned int aapcs_candidate_count : 3;
> +
>   /* * Number of fields described for this type.  This field appears
>      at this location because it packs nicely here.  */
> 
> -- 
> 2.17.2 (Apple Git-113)
>
  

On 12/12/2018 09:59 AM, Alan Hayward wrote:
> gdb.dwarf2/dw2-cp-infcall-ref-static.exp includes a struct which
> contains itself.  Add support to aapcs_is_vfp_call_or_return_candidate to
> prevent infinite recursion.

But the contained struct is a static field, no?  How does that affect
the calling convention (or whatever is being computed) ?

Without looking at any of this in detail, I'd off hand think that
this code should be skipping/ignoring static fields?

Thanks,
Pedro Alves

> 
> The simple solution is to pass a counter through the functions and abort once
> the max limit is reached.  However, this does not catch the case where a
> struct only contains itself and no other members.  That can be solved by
> marking a type as invalid before checking all of it's children.
> 
> Add a count for the call or ret candidate to main_type, restricted to 3
> bits - the value 5 (or above) is used as invalid.
> 
> This code is testsed by both gdb.base/infcall-nested-structs.exp
> and gdb.dwarf2/dw2-cp-infcall-ref-static.exp.
> 
> gdb/ChangeLog:
> 
> 2018-12-12  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* aarch64-tdep.c (HA_MAX_NUM_FLDS): Remove define.
> 	(aapcs_is_vfp_call_or_return_candidate_1): Remember type.
> 	(aapcs_is_vfp_call_or_return_candidate): Likewise.
> 	* gdbtypes.h (AAPCS_VFP_CALL_OR_RET_CAND_MAX): Add define.
> 	(AAPCS_VFP_CALL_OR_RET_CAND_INVALID): Likewise.
> 	(TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.
> 	(TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.
> 	(struct main_type): Add aapcs_candidate_count.
> ---
>  gdb/aarch64-tdep.c | 87 ++++++++++++++++++++++++++--------------------
>  gdb/gdbtypes.h     | 23 ++++++++++++
>  2 files changed, 72 insertions(+), 38 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index ae56c9ca34..3fc58308d6 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -64,10 +64,6 @@
>  #define bit(obj,st) (((obj) >> (st)) & 1)
>  #define bits(obj,st,fn) (((obj) >> (st)) & submask ((fn) - (st)))
>  
> -/* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
> -   four members.  */
> -#define HA_MAX_NUM_FLDS		4
> -
>  /* All possible aarch64 target descriptors.  */
>  struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];
>  
> @@ -1146,46 +1142,58 @@ aarch64_type_align (struct type *t)
>  
>  /* Worker function for aapcs_is_vfp_call_or_return_candidate.
>  
> -   Return the number of register required, or -1 on failure.
> +   Set TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE to the number of registers required,
> +   or AAPCS_VFP_CALL_OR_RET_CAND_INVALID on failure.
>  
>     When encountering a base element, if FUNDAMENTAL_TYPE is not set then set it
>     to the element, else fail if the type of this element does not match the
>     existing value.  */
>  
> -static int
> +static void
>  aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
>  					 struct type **fundamental_type)
>  {
>    if (type == nullptr)
> -    return -1;
> +    return;
> +
> +  /* Check if we have already evaluated this TYPE.  */
> +  if (TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type)
> +      >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)
> +    return;
> +
> +  /* Set TYPE to invalid to prevent infinite recursion.  */
> +  TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type,
> +					    AAPCS_VFP_CALL_OR_RET_CAND_INVALID);
>  
>    switch (TYPE_CODE (type))
>      {
>      case TYPE_CODE_FLT:
>        if (TYPE_LENGTH (type) > 16)
> -	return -1;
> +	return;
>  
>        if (*fundamental_type == nullptr)
>  	*fundamental_type = type;
>        else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)
>  	       || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))
> -	return -1;
> +	return;
>  
> -      return 1;
> +      TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);
> +      return;
>  
>      case TYPE_CODE_COMPLEX:
>        {
>  	struct type *target_type = check_typedef (TYPE_TARGET_TYPE (type));
>  	if (TYPE_LENGTH (target_type) > 16)
> -	  return -1;
> +	  return;
>  
>  	if (*fundamental_type == nullptr)
>  	  *fundamental_type = target_type;
>  	else if (TYPE_LENGTH (target_type) != TYPE_LENGTH (*fundamental_type)
>  		 || TYPE_CODE (target_type) != TYPE_CODE (*fundamental_type))
> -	  return -1;
> +	  return;
>  
> -	return 2;
> +	TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 2);
> +	return;
>        }
>  
>      case TYPE_CODE_ARRAY:
> @@ -1193,28 +1201,33 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
>  	if (TYPE_VECTOR (type))
>  	  {
>  	    if (TYPE_LENGTH (type) != 8 && TYPE_LENGTH (type) != 16)
> -	      return -1;
> +	      return;
>  
>  	    if (*fundamental_type == nullptr)
>  	      *fundamental_type = type;
>  	    else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)
>  		     || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))
> -	      return -1;
> +	      return;
>  
> -	    return 1;
> +	    TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);
>  	  }
>  	else
>  	  {
> +	    /* Calculate if type of an element in the array is valid.  */
>  	    struct type *target_type = TYPE_TARGET_TYPE (type);
> -	    int count = aapcs_is_vfp_call_or_return_candidate_1
> -			  (target_type, fundamental_type);
> +	    aapcs_is_vfp_call_or_return_candidate_1 (target_type,
> +						     fundamental_type);
>  
> -	    if (count == -1)
> -	      return count;
> +	    int count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (target_type);
> +	    if (count >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)
> +	      return;
> +
> +	    /* Expand to cover the whole array.  The set handles overflow.  */
>  
>  	    count *= (TYPE_LENGTH (type) / TYPE_LENGTH (target_type));
> -	      return count;
> +	    TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);
>  	  }
> +	return;
>        }
>  
>      case TYPE_CODE_STRUCT:
> @@ -1226,20 +1239,23 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
>  	  {
>  	    struct type *member = check_typedef (TYPE_FIELD_TYPE (type, i));
>  
> -	    int sub_count = aapcs_is_vfp_call_or_return_candidate_1
> -			      (member, fundamental_type);
> -	    if (sub_count == -1)
> -	      return -1;
> +	    aapcs_is_vfp_call_or_return_candidate_1 (member, fundamental_type);
> +
> +	    int sub_count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (member);
> +
> +	    if (sub_count + count > AAPCS_VFP_CALL_OR_RET_CAND_MAX)
> +	      return;
> +
>  	    count += sub_count;
>  	  }
> -	return count;
> +
> +	TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);
> +	return;
>        }
>  
>      default:
> -      break;
> +      return;
>      }
> -
> -  return -1;
>  }
>  
>  /* Return true if an argument, whose type is described by TYPE, can be passed or
> @@ -1269,16 +1285,11 @@ aapcs_is_vfp_call_or_return_candidate (struct type *type, int *count,
>  
>    *fundamental_type = nullptr;
>  
> -  int ag_count = aapcs_is_vfp_call_or_return_candidate_1 (type,
> -							  fundamental_type);
> +  aapcs_is_vfp_call_or_return_candidate_1 (type, fundamental_type);
>  
> -  if (ag_count > 0 && ag_count <= HA_MAX_NUM_FLDS)
> -    {
> -      *count = ag_count;
> -      return true;
> -    }
> -  else
> -    return false;
> +  *count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type);
> +
> +  return (*count < AAPCS_VFP_CALL_OR_RET_CAND_INVALID);
>  }
>  
>  /* AArch64 function call information structure.  */
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index f0adec7a20..600112abf5 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -391,6 +391,23 @@ DEF_ENUM_FLAGS_TYPE (enum type_instance_flag_value, type_instance_flags);
>  #define TYPE_ADDRESS_CLASS_ALL(t) (TYPE_INSTANCE_FLAGS(t) \
>  				   & TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)
>  
> +
> +/* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call
> +   parameter or return parameter, as per the AAPCS64 5.4.2.C.  Value can be:
> +     0: the type has not been checked.
> +     1 to AAPCS_VFP_CALL_OR_RET_CAND_MAX: Valid, and has that many members.
> +     AAPCS_VFP_CALL_OR_RET_CAND_MAX: Invalid.  */
> +
> +#define AAPCS_VFP_CALL_OR_RET_CAND_MAX 4
> +#define AAPCS_VFP_CALL_OR_RET_CAND_INVALID (AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1)
> +
> +#define TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype) \
> +  (TYPE_MAIN_TYPE (thistype)->aapcs_candidate_count)
> +
> +#define TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype, val) \
> +  TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (thistype) = \
> +    (std::min (val, AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1))
> +
>  /* * Information needed for a discriminated union.  A discriminated
>     union is handled somewhat differently from an ordinary union.
>  
> @@ -716,6 +733,12 @@ struct main_type
>  
>    ENUM_BITFIELD(type_specific_kind) type_specific_field : 3;
>  
> +  /* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call
> +     parameter or return parameter, as per the AAPCS64 5.4.2.C.  Required to
> +     prevent recursive checking.  */
> +
> +  unsigned int aapcs_candidate_count : 3;
> +
>    /* * Number of fields described for this type.  This field appears
>       at this location because it packs nicely here.  */
>  
>
  

> On 9 Jan 2019, at 19:01, Pedro Alves <palves@redhat.com> wrote:

> 

> On 12/12/2018 09:59 AM, Alan Hayward wrote:

>> gdb.dwarf2/dw2-cp-infcall-ref-static.exp includes a struct which

>> contains itself.  Add support to aapcs_is_vfp_call_or_return_candidate to

>> prevent infinite recursion.

> 

> But the contained struct is a static field, no?  How does that affect

> the calling convention (or whatever is being computed) ?

> 

> Without looking at any of this in detail, I'd off hand think that

> this code should be skipping/ignoring static fields?

> 

> Thanks,

> Pedro Alves

> 


Hmmm... yes, good spot. Tried this in GCC, and the static members are not
passed as args. GCC doesn’t have any explicit code to cover this in the
backend - I suspect that by the time it gets there the static members are
effectivley already in a special global var.

I’ll write up some new test cases to cover the cases with normal structs with
static members.
And then I’ll see if I generate some tests with non-static infinite structs.
Maybe it’ll squash the need for this patch.


Thanks,
Alan.


>> 

>> The simple solution is to pass a counter through the functions and abort once

>> the max limit is reached.  However, this does not catch the case where a

>> struct only contains itself and no other members.  That can be solved by

>> marking a type as invalid before checking all of it's children.

>> 

>> Add a count for the call or ret candidate to main_type, restricted to 3

>> bits - the value 5 (or above) is used as invalid.

>> 

>> This code is testsed by both gdb.base/infcall-nested-structs.exp

>> and gdb.dwarf2/dw2-cp-infcall-ref-static.exp.

>> 

>> gdb/ChangeLog:

>> 

>> 2018-12-12  Alan Hayward  <alan.hayward@arm.com>

>> 

>> 	* aarch64-tdep.c (HA_MAX_NUM_FLDS): Remove define.

>> 	(aapcs_is_vfp_call_or_return_candidate_1): Remember type.

>> 	(aapcs_is_vfp_call_or_return_candidate): Likewise.

>> 	* gdbtypes.h (AAPCS_VFP_CALL_OR_RET_CAND_MAX): Add define.

>> 	(AAPCS_VFP_CALL_OR_RET_CAND_INVALID): Likewise.

>> 	(TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.

>> 	(TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.

>> 	(struct main_type): Add aapcs_candidate_count.

>> ---

>> gdb/aarch64-tdep.c | 87 ++++++++++++++++++++++++++--------------------

>> gdb/gdbtypes.h     | 23 ++++++++++++

>> 2 files changed, 72 insertions(+), 38 deletions(-)

>> 

>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c

>> index ae56c9ca34..3fc58308d6 100644

>> --- a/gdb/aarch64-tdep.c

>> +++ b/gdb/aarch64-tdep.c

>> @@ -64,10 +64,6 @@

>> #define bit(obj,st) (((obj) >> (st)) & 1)

>> #define bits(obj,st,fn) (((obj) >> (st)) & submask ((fn) - (st)))

>> 

>> -/* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most

>> -   four members.  */

>> -#define HA_MAX_NUM_FLDS		4

>> -

>> /* All possible aarch64 target descriptors.  */

>> struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];

>> 

>> @@ -1146,46 +1142,58 @@ aarch64_type_align (struct type *t)

>> 

>> /* Worker function for aapcs_is_vfp_call_or_return_candidate.

>> 

>> -   Return the number of register required, or -1 on failure.

>> +   Set TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE to the number of registers required,

>> +   or AAPCS_VFP_CALL_OR_RET_CAND_INVALID on failure.

>> 

>>    When encountering a base element, if FUNDAMENTAL_TYPE is not set then set it

>>    to the element, else fail if the type of this element does not match the

>>    existing value.  */

>> 

>> -static int

>> +static void

>> aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,

>> 					 struct type **fundamental_type)

>> {

>>   if (type == nullptr)

>> -    return -1;

>> +    return;

>> +

>> +  /* Check if we have already evaluated this TYPE.  */

>> +  if (TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type)

>> +      >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)

>> +    return;

>> +

>> +  /* Set TYPE to invalid to prevent infinite recursion.  */

>> +  TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type,

>> +					    AAPCS_VFP_CALL_OR_RET_CAND_INVALID);

>> 

>>   switch (TYPE_CODE (type))

>>     {

>>     case TYPE_CODE_FLT:

>>       if (TYPE_LENGTH (type) > 16)

>> -	return -1;

>> +	return;

>> 

>>       if (*fundamental_type == nullptr)

>> 	*fundamental_type = type;

>>       else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)

>> 	       || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))

>> -	return -1;

>> +	return;

>> 

>> -      return 1;

>> +      TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);

>> +      return;

>> 

>>     case TYPE_CODE_COMPLEX:

>>       {

>> 	struct type *target_type = check_typedef (TYPE_TARGET_TYPE (type));

>> 	if (TYPE_LENGTH (target_type) > 16)

>> -	  return -1;

>> +	  return;

>> 

>> 	if (*fundamental_type == nullptr)

>> 	  *fundamental_type = target_type;

>> 	else if (TYPE_LENGTH (target_type) != TYPE_LENGTH (*fundamental_type)

>> 		 || TYPE_CODE (target_type) != TYPE_CODE (*fundamental_type))

>> -	  return -1;

>> +	  return;

>> 

>> -	return 2;

>> +	TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 2);

>> +	return;

>>       }

>> 

>>     case TYPE_CODE_ARRAY:

>> @@ -1193,28 +1201,33 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,

>> 	if (TYPE_VECTOR (type))

>> 	  {

>> 	    if (TYPE_LENGTH (type) != 8 && TYPE_LENGTH (type) != 16)

>> -	      return -1;

>> +	      return;

>> 

>> 	    if (*fundamental_type == nullptr)

>> 	      *fundamental_type = type;

>> 	    else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)

>> 		     || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))

>> -	      return -1;

>> +	      return;

>> 

>> -	    return 1;

>> +	    TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);

>> 	  }

>> 	else

>> 	  {

>> +	    /* Calculate if type of an element in the array is valid.  */

>> 	    struct type *target_type = TYPE_TARGET_TYPE (type);

>> -	    int count = aapcs_is_vfp_call_or_return_candidate_1

>> -			  (target_type, fundamental_type);

>> +	    aapcs_is_vfp_call_or_return_candidate_1 (target_type,

>> +						     fundamental_type);

>> 

>> -	    if (count == -1)

>> -	      return count;

>> +	    int count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (target_type);

>> +	    if (count >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)

>> +	      return;

>> +

>> +	    /* Expand to cover the whole array.  The set handles overflow.  */

>> 

>> 	    count *= (TYPE_LENGTH (type) / TYPE_LENGTH (target_type));

>> -	      return count;

>> +	    TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);

>> 	  }

>> +	return;

>>       }

>> 

>>     case TYPE_CODE_STRUCT:

>> @@ -1226,20 +1239,23 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,

>> 	  {

>> 	    struct type *member = check_typedef (TYPE_FIELD_TYPE (type, i));

>> 

>> -	    int sub_count = aapcs_is_vfp_call_or_return_candidate_1

>> -			      (member, fundamental_type);

>> -	    if (sub_count == -1)

>> -	      return -1;

>> +	    aapcs_is_vfp_call_or_return_candidate_1 (member, fundamental_type);

>> +

>> +	    int sub_count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (member);

>> +

>> +	    if (sub_count + count > AAPCS_VFP_CALL_OR_RET_CAND_MAX)

>> +	      return;

>> +

>> 	    count += sub_count;

>> 	  }

>> -	return count;

>> +

>> +	TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);

>> +	return;

>>       }

>> 

>>     default:

>> -      break;

>> +      return;

>>     }

>> -

>> -  return -1;

>> }

>> 

>> /* Return true if an argument, whose type is described by TYPE, can be passed or

>> @@ -1269,16 +1285,11 @@ aapcs_is_vfp_call_or_return_candidate (struct type *type, int *count,

>> 

>>   *fundamental_type = nullptr;

>> 

>> -  int ag_count = aapcs_is_vfp_call_or_return_candidate_1 (type,

>> -							  fundamental_type);

>> +  aapcs_is_vfp_call_or_return_candidate_1 (type, fundamental_type);

>> 

>> -  if (ag_count > 0 && ag_count <= HA_MAX_NUM_FLDS)

>> -    {

>> -      *count = ag_count;

>> -      return true;

>> -    }

>> -  else

>> -    return false;

>> +  *count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type);

>> +

>> +  return (*count < AAPCS_VFP_CALL_OR_RET_CAND_INVALID);

>> }

>> 

>> /* AArch64 function call information structure.  */

>> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h

>> index f0adec7a20..600112abf5 100644

>> --- a/gdb/gdbtypes.h

>> +++ b/gdb/gdbtypes.h

>> @@ -391,6 +391,23 @@ DEF_ENUM_FLAGS_TYPE (enum type_instance_flag_value, type_instance_flags);

>> #define TYPE_ADDRESS_CLASS_ALL(t) (TYPE_INSTANCE_FLAGS(t) \

>> 				   & TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)

>> 

>> +

>> +/* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call

>> +   parameter or return parameter, as per the AAPCS64 5.4.2.C.  Value can be:

>> +     0: the type has not been checked.

>> +     1 to AAPCS_VFP_CALL_OR_RET_CAND_MAX: Valid, and has that many members.

>> +     AAPCS_VFP_CALL_OR_RET_CAND_MAX: Invalid.  */

>> +

>> +#define AAPCS_VFP_CALL_OR_RET_CAND_MAX 4

>> +#define AAPCS_VFP_CALL_OR_RET_CAND_INVALID (AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1)

>> +

>> +#define TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype) \

>> +  (TYPE_MAIN_TYPE (thistype)->aapcs_candidate_count)

>> +

>> +#define TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype, val) \

>> +  TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (thistype) = \

>> +    (std::min (val, AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1))

>> +

>> /* * Information needed for a discriminated union.  A discriminated

>>    union is handled somewhat differently from an ordinary union.

>> 

>> @@ -716,6 +733,12 @@ struct main_type

>> 

>>   ENUM_BITFIELD(type_specific_kind) type_specific_field : 3;

>> 

>> +  /* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call

>> +     parameter or return parameter, as per the AAPCS64 5.4.2.C.  Required to

>> +     prevent recursive checking.  */

>> +

>> +  unsigned int aapcs_candidate_count : 3;

>> +

>>   /* * Number of fields described for this type.  This field appears

>>      at this location because it packs nicely here.  */

>> 

>>
  

On 01/10/2019 01:44 PM, Alan Hayward wrote:
> 
> 
>> On 9 Jan 2019, at 19:01, Pedro Alves <palves@redhat.com> wrote:
>>
>> On 12/12/2018 09:59 AM, Alan Hayward wrote:
>>> gdb.dwarf2/dw2-cp-infcall-ref-static.exp includes a struct which
>>> contains itself.  Add support to aapcs_is_vfp_call_or_return_candidate to
>>> prevent infinite recursion.
>>
>> But the contained struct is a static field, no?  How does that affect
>> the calling convention (or whatever is being computed) ?
>>
>> Without looking at any of this in detail, I'd off hand think that
>> this code should be skipping/ignoring static fields?
>>
>>
> 
> Hmmm... yes, good spot. Tried this in GCC, and the static members are not
> passed as args. 

Right, C++ static data members aren't passed anywhere, they're just globals
with fixed addresses.  You can think of:

 struct A
 {
   static A a;
 };

Basically the same as:

 struct A
 {
 };
 A a;

The difference is mainly that in the former, the global is
called 'A::a', while in the latter it's called 'a'.  Not that
much different from, say:

 struct A
 {
 };
 namespace NS
 {
   A a;
 };

Thanks,
Pedro Alves

> GCC doesn’t have any explicit code to cover this in the
> backend - I suspect that by the time it gets there the static members are
> effectivley already in a special global var.


> 
> I’ll write up some new test cases to cover the cases with normal structs with
> static members.
> And then I’ll see if I generate some tests with non-static infinite structs.
> Maybe it’ll squash the need for this patch.
> 
> 
> Thanks,
> Alan.
> 
> 
>>>
>>> The simple solution is to pass a counter through the functions and abort once
>>> the max limit is reached.  However, this does not catch the case where a
>>> struct only contains itself and no other members.  That can be solved by
>>> marking a type as invalid before checking all of it's children.
>>>
>>> Add a count for the call or ret candidate to main_type, restricted to 3
>>> bits - the value 5 (or above) is used as invalid.
>>>
>>> This code is testsed by both gdb.base/infcall-nested-structs.exp
>>> and gdb.dwarf2/dw2-cp-infcall-ref-static.exp.
>>>
>>> gdb/ChangeLog:
>>>
>>> 2018-12-12  Alan Hayward  <alan.hayward@arm.com>
>>>
>>> 	* aarch64-tdep.c (HA_MAX_NUM_FLDS): Remove define.
>>> 	(aapcs_is_vfp_call_or_return_candidate_1): Remember type.
>>> 	(aapcs_is_vfp_call_or_return_candidate): Likewise.
>>> 	* gdbtypes.h (AAPCS_VFP_CALL_OR_RET_CAND_MAX): Add define.
>>> 	(AAPCS_VFP_CALL_OR_RET_CAND_INVALID): Likewise.
>>> 	(TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.
>>> 	(TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.
>>> 	(struct main_type): Add aapcs_candidate_count.
>>> ---
>>> gdb/aarch64-tdep.c | 87 ++++++++++++++++++++++++++--------------------
>>> gdb/gdbtypes.h     | 23 ++++++++++++
>>> 2 files changed, 72 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>> index ae56c9ca34..3fc58308d6 100644
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -64,10 +64,6 @@
>>> #define bit(obj,st) (((obj) >> (st)) & 1)
>>> #define bits(obj,st,fn) (((obj) >> (st)) & submask ((fn) - (st)))
>>>
>>> -/* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
>>> -   four members.  */
>>> -#define HA_MAX_NUM_FLDS		4
>>> -
>>> /* All possible aarch64 target descriptors.  */
>>> struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];
>>>
>>> @@ -1146,46 +1142,58 @@ aarch64_type_align (struct type *t)
>>>
>>> /* Worker function for aapcs_is_vfp_call_or_return_candidate.
>>>
>>> -   Return the number of register required, or -1 on failure.
>>> +   Set TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE to the number of registers required,
>>> +   or AAPCS_VFP_CALL_OR_RET_CAND_INVALID on failure.
>>>
>>>    When encountering a base element, if FUNDAMENTAL_TYPE is not set then set it
>>>    to the element, else fail if the type of this element does not match the
>>>    existing value.  */
>>>
>>> -static int
>>> +static void
>>> aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
>>> 					 struct type **fundamental_type)
>>> {
>>>   if (type == nullptr)
>>> -    return -1;
>>> +    return;
>>> +
>>> +  /* Check if we have already evaluated this TYPE.  */
>>> +  if (TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type)
>>> +      >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)
>>> +    return;
>>> +
>>> +  /* Set TYPE to invalid to prevent infinite recursion.  */
>>> +  TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type,
>>> +					    AAPCS_VFP_CALL_OR_RET_CAND_INVALID);
>>>
>>>   switch (TYPE_CODE (type))
>>>     {
>>>     case TYPE_CODE_FLT:
>>>       if (TYPE_LENGTH (type) > 16)
>>> -	return -1;
>>> +	return;
>>>
>>>       if (*fundamental_type == nullptr)
>>> 	*fundamental_type = type;
>>>       else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)
>>> 	       || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))
>>> -	return -1;
>>> +	return;
>>>
>>> -      return 1;
>>> +      TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);
>>> +      return;
>>>
>>>     case TYPE_CODE_COMPLEX:
>>>       {
>>> 	struct type *target_type = check_typedef (TYPE_TARGET_TYPE (type));
>>> 	if (TYPE_LENGTH (target_type) > 16)
>>> -	  return -1;
>>> +	  return;
>>>
>>> 	if (*fundamental_type == nullptr)
>>> 	  *fundamental_type = target_type;
>>> 	else if (TYPE_LENGTH (target_type) != TYPE_LENGTH (*fundamental_type)
>>> 		 || TYPE_CODE (target_type) != TYPE_CODE (*fundamental_type))
>>> -	  return -1;
>>> +	  return;
>>>
>>> -	return 2;
>>> +	TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 2);
>>> +	return;
>>>       }
>>>
>>>     case TYPE_CODE_ARRAY:
>>> @@ -1193,28 +1201,33 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
>>> 	if (TYPE_VECTOR (type))
>>> 	  {
>>> 	    if (TYPE_LENGTH (type) != 8 && TYPE_LENGTH (type) != 16)
>>> -	      return -1;
>>> +	      return;
>>>
>>> 	    if (*fundamental_type == nullptr)
>>> 	      *fundamental_type = type;
>>> 	    else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)
>>> 		     || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))
>>> -	      return -1;
>>> +	      return;
>>>
>>> -	    return 1;
>>> +	    TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);
>>> 	  }
>>> 	else
>>> 	  {
>>> +	    /* Calculate if type of an element in the array is valid.  */
>>> 	    struct type *target_type = TYPE_TARGET_TYPE (type);
>>> -	    int count = aapcs_is_vfp_call_or_return_candidate_1
>>> -			  (target_type, fundamental_type);
>>> +	    aapcs_is_vfp_call_or_return_candidate_1 (target_type,
>>> +						     fundamental_type);
>>>
>>> -	    if (count == -1)
>>> -	      return count;
>>> +	    int count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (target_type);
>>> +	    if (count >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)
>>> +	      return;
>>> +
>>> +	    /* Expand to cover the whole array.  The set handles overflow.  */
>>>
>>> 	    count *= (TYPE_LENGTH (type) / TYPE_LENGTH (target_type));
>>> -	      return count;
>>> +	    TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);
>>> 	  }
>>> +	return;
>>>       }
>>>
>>>     case TYPE_CODE_STRUCT:
>>> @@ -1226,20 +1239,23 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
>>> 	  {
>>> 	    struct type *member = check_typedef (TYPE_FIELD_TYPE (type, i));
>>>
>>> -	    int sub_count = aapcs_is_vfp_call_or_return_candidate_1
>>> -			      (member, fundamental_type);
>>> -	    if (sub_count == -1)
>>> -	      return -1;
>>> +	    aapcs_is_vfp_call_or_return_candidate_1 (member, fundamental_type);
>>> +
>>> +	    int sub_count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (member);
>>> +
>>> +	    if (sub_count + count > AAPCS_VFP_CALL_OR_RET_CAND_MAX)
>>> +	      return;
>>> +
>>> 	    count += sub_count;
>>> 	  }
>>> -	return count;
>>> +
>>> +	TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);
>>> +	return;
>>>       }
>>>
>>>     default:
>>> -      break;
>>> +      return;
>>>     }
>>> -
>>> -  return -1;
>>> }
>>>
>>> /* Return true if an argument, whose type is described by TYPE, can be passed or
>>> @@ -1269,16 +1285,11 @@ aapcs_is_vfp_call_or_return_candidate (struct type *type, int *count,
>>>
>>>   *fundamental_type = nullptr;
>>>
>>> -  int ag_count = aapcs_is_vfp_call_or_return_candidate_1 (type,
>>> -							  fundamental_type);
>>> +  aapcs_is_vfp_call_or_return_candidate_1 (type, fundamental_type);
>>>
>>> -  if (ag_count > 0 && ag_count <= HA_MAX_NUM_FLDS)
>>> -    {
>>> -      *count = ag_count;
>>> -      return true;
>>> -    }
>>> -  else
>>> -    return false;
>>> +  *count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type);
>>> +
>>> +  return (*count < AAPCS_VFP_CALL_OR_RET_CAND_INVALID);
>>> }
>>>
>>> /* AArch64 function call information structure.  */
>>> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
>>> index f0adec7a20..600112abf5 100644
>>> --- a/gdb/gdbtypes.h
>>> +++ b/gdb/gdbtypes.h
>>> @@ -391,6 +391,23 @@ DEF_ENUM_FLAGS_TYPE (enum type_instance_flag_value, type_instance_flags);
>>> #define TYPE_ADDRESS_CLASS_ALL(t) (TYPE_INSTANCE_FLAGS(t) \
>>> 				   & TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)
>>>
>>> +
>>> +/* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call
>>> +   parameter or return parameter, as per the AAPCS64 5.4.2.C.  Value can be:
>>> +     0: the type has not been checked.
>>> +     1 to AAPCS_VFP_CALL_OR_RET_CAND_MAX: Valid, and has that many members.
>>> +     AAPCS_VFP_CALL_OR_RET_CAND_MAX: Invalid.  */
>>> +
>>> +#define AAPCS_VFP_CALL_OR_RET_CAND_MAX 4
>>> +#define AAPCS_VFP_CALL_OR_RET_CAND_INVALID (AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1)
>>> +
>>> +#define TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype) \
>>> +  (TYPE_MAIN_TYPE (thistype)->aapcs_candidate_count)
>>> +
>>> +#define TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype, val) \
>>> +  TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (thistype) = \
>>> +    (std::min (val, AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1))
>>> +
>>> /* * Information needed for a discriminated union.  A discriminated
>>>    union is handled somewhat differently from an ordinary union.
>>>
>>> @@ -716,6 +733,12 @@ struct main_type
>>>
>>>   ENUM_BITFIELD(type_specific_kind) type_specific_field : 3;
>>>
>>> +  /* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call
>>> +     parameter or return parameter, as per the AAPCS64 5.4.2.C.  Required to
>>> +     prevent recursive checking.  */
>>> +
>>> +  unsigned int aapcs_candidate_count : 3;
>>> +
>>>   /* * Number of fields described for this type.  This field appears
>>>      at this location because it packs nicely here.  */
>>>
>>>
>
  

> On 10 Jan 2019, at 16:07, Pedro Alves <palves@redhat.com> wrote:

> 

> On 01/10/2019 01:44 PM, Alan Hayward wrote:

>> 

>> 

>>> On 9 Jan 2019, at 19:01, Pedro Alves <palves@redhat.com> wrote:

>>> 

>>> On 12/12/2018 09:59 AM, Alan Hayward wrote:

>>>> gdb.dwarf2/dw2-cp-infcall-ref-static.exp includes a struct which

>>>> contains itself.  Add support to aapcs_is_vfp_call_or_return_candidate to

>>>> prevent infinite recursion.

>>> 

>>> But the contained struct is a static field, no?  How does that affect

>>> the calling convention (or whatever is being computed) ?

>>> 

>>> Without looking at any of this in detail, I'd off hand think that

>>> this code should be skipping/ignoring static fields?

>>> 

>>> 

>> 

>> Hmmm... yes, good spot. Tried this in GCC, and the static members are not

>> passed as args. 

> 

> Right, C++ static data members aren't passed anywhere, they're just globals

> with fixed addresses.  You can think of:

> 

> struct A

> {

>   static A a;

> };

> 

> Basically the same as:

> 

> struct A

> {

> };

> A a;

> 

> The difference is mainly that in the former, the global is

> called 'A::a', while in the latter it's called 'a'.  Not that

> much different from, say:

> 

> struct A

> {

> };

> namespace NS

> {

>   A a;

> };

> 


Just an fyi,
By creating a c++ test case, I’ve also found some issues with empty structs
inside structs (c++ classes have a minimum size of 0, causing g++/clang to
reject passing it in registers because it’s not aligned properly).

Also, looks like it gdb_asserts when running the test on x86 too.

This patch will still be needed for infinite recursive structs, but I’ll throw
it in as part of a new patch series.


Alan.


> Thanks,

> Pedro Alves

> 

>> GCC doesn’t have any explicit code to cover this in the

>> backend - I suspect that by the time it gets there the static members are

>> effectivley already in a special global var.

> 

> 

>> 

>> I’ll write up some new test cases to cover the cases with normal structs with

>> static members.

>> And then I’ll see if I generate some tests with non-static infinite structs.

>> Maybe it’ll squash the need for this patch.

>> 

>> 

>> Thanks,

>> Alan.

>> 

>> 

>>>> 

>>>> The simple solution is to pass a counter through the functions and abort once

>>>> the max limit is reached.  However, this does not catch the case where a

>>>> struct only contains itself and no other members.  That can be solved by

>>>> marking a type as invalid before checking all of it's children.

>>>> 

>>>> Add a count for the call or ret candidate to main_type, restricted to 3

>>>> bits - the value 5 (or above) is used as invalid.

>>>> 

>>>> This code is testsed by both gdb.base/infcall-nested-structs.exp

>>>> and gdb.dwarf2/dw2-cp-infcall-ref-static.exp.

>>>> 

>>>> gdb/ChangeLog:

>>>> 

>>>> 2018-12-12  Alan Hayward  <alan.hayward@arm.com>

>>>> 

>>>> 	* aarch64-tdep.c (HA_MAX_NUM_FLDS): Remove define.

>>>> 	(aapcs_is_vfp_call_or_return_candidate_1): Remember type.

>>>> 	(aapcs_is_vfp_call_or_return_candidate): Likewise.

>>>> 	* gdbtypes.h (AAPCS_VFP_CALL_OR_RET_CAND_MAX): Add define.

>>>> 	(AAPCS_VFP_CALL_OR_RET_CAND_INVALID): Likewise.

>>>> 	(TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.

>>>> 	(TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.

>>>> 	(struct main_type): Add aapcs_candidate_count.

>>>> ---

>>>> gdb/aarch64-tdep.c | 87 ++++++++++++++++++++++++++--------------------

>>>> gdb/gdbtypes.h     | 23 ++++++++++++

>>>> 2 files changed, 72 insertions(+), 38 deletions(-)

>>>> 

>>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c

>>>> index ae56c9ca34..3fc58308d6 100644

>>>> --- a/gdb/aarch64-tdep.c

>>>> +++ b/gdb/aarch64-tdep.c

>>>> @@ -64,10 +64,6 @@

>>>> #define bit(obj,st) (((obj) >> (st)) & 1)

>>>> #define bits(obj,st,fn) (((obj) >> (st)) & submask ((fn) - (st)))

>>>> 

>>>> -/* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most

>>>> -   four members.  */

>>>> -#define HA_MAX_NUM_FLDS		4

>>>> -

>>>> /* All possible aarch64 target descriptors.  */

>>>> struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];

>>>> 

>>>> @@ -1146,46 +1142,58 @@ aarch64_type_align (struct type *t)

>>>> 

>>>> /* Worker function for aapcs_is_vfp_call_or_return_candidate.

>>>> 

>>>> -   Return the number of register required, or -1 on failure.

>>>> +   Set TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE to the number of registers required,

>>>> +   or AAPCS_VFP_CALL_OR_RET_CAND_INVALID on failure.

>>>> 

>>>>   When encountering a base element, if FUNDAMENTAL_TYPE is not set then set it

>>>>   to the element, else fail if the type of this element does not match the

>>>>   existing value.  */

>>>> 

>>>> -static int

>>>> +static void

>>>> aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,

>>>> 					 struct type **fundamental_type)

>>>> {

>>>>  if (type == nullptr)

>>>> -    return -1;

>>>> +    return;

>>>> +

>>>> +  /* Check if we have already evaluated this TYPE.  */

>>>> +  if (TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type)

>>>> +      >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)

>>>> +    return;

>>>> +

>>>> +  /* Set TYPE to invalid to prevent infinite recursion.  */

>>>> +  TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type,

>>>> +					    AAPCS_VFP_CALL_OR_RET_CAND_INVALID);

>>>> 

>>>>  switch (TYPE_CODE (type))

>>>>    {

>>>>    case TYPE_CODE_FLT:

>>>>      if (TYPE_LENGTH (type) > 16)

>>>> -	return -1;

>>>> +	return;

>>>> 

>>>>      if (*fundamental_type == nullptr)

>>>> 	*fundamental_type = type;

>>>>      else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)

>>>> 	       || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))

>>>> -	return -1;

>>>> +	return;

>>>> 

>>>> -      return 1;

>>>> +      TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);

>>>> +      return;

>>>> 

>>>>    case TYPE_CODE_COMPLEX:

>>>>      {

>>>> 	struct type *target_type = check_typedef (TYPE_TARGET_TYPE (type));

>>>> 	if (TYPE_LENGTH (target_type) > 16)

>>>> -	  return -1;

>>>> +	  return;

>>>> 

>>>> 	if (*fundamental_type == nullptr)

>>>> 	  *fundamental_type = target_type;

>>>> 	else if (TYPE_LENGTH (target_type) != TYPE_LENGTH (*fundamental_type)

>>>> 		 || TYPE_CODE (target_type) != TYPE_CODE (*fundamental_type))

>>>> -	  return -1;

>>>> +	  return;

>>>> 

>>>> -	return 2;

>>>> +	TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 2);

>>>> +	return;

>>>>      }

>>>> 

>>>>    case TYPE_CODE_ARRAY:

>>>> @@ -1193,28 +1201,33 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,

>>>> 	if (TYPE_VECTOR (type))

>>>> 	  {

>>>> 	    if (TYPE_LENGTH (type) != 8 && TYPE_LENGTH (type) != 16)

>>>> -	      return -1;

>>>> +	      return;

>>>> 

>>>> 	    if (*fundamental_type == nullptr)

>>>> 	      *fundamental_type = type;

>>>> 	    else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)

>>>> 		     || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))

>>>> -	      return -1;

>>>> +	      return;

>>>> 

>>>> -	    return 1;

>>>> +	    TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);

>>>> 	  }

>>>> 	else

>>>> 	  {

>>>> +	    /* Calculate if type of an element in the array is valid.  */

>>>> 	    struct type *target_type = TYPE_TARGET_TYPE (type);

>>>> -	    int count = aapcs_is_vfp_call_or_return_candidate_1

>>>> -			  (target_type, fundamental_type);

>>>> +	    aapcs_is_vfp_call_or_return_candidate_1 (target_type,

>>>> +						     fundamental_type);

>>>> 

>>>> -	    if (count == -1)

>>>> -	      return count;

>>>> +	    int count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (target_type);

>>>> +	    if (count >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)

>>>> +	      return;

>>>> +

>>>> +	    /* Expand to cover the whole array.  The set handles overflow.  */

>>>> 

>>>> 	    count *= (TYPE_LENGTH (type) / TYPE_LENGTH (target_type));

>>>> -	      return count;

>>>> +	    TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);

>>>> 	  }

>>>> +	return;

>>>>      }

>>>> 

>>>>    case TYPE_CODE_STRUCT:

>>>> @@ -1226,20 +1239,23 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,

>>>> 	  {

>>>> 	    struct type *member = check_typedef (TYPE_FIELD_TYPE (type, i));

>>>> 

>>>> -	    int sub_count = aapcs_is_vfp_call_or_return_candidate_1

>>>> -			      (member, fundamental_type);

>>>> -	    if (sub_count == -1)

>>>> -	      return -1;

>>>> +	    aapcs_is_vfp_call_or_return_candidate_1 (member, fundamental_type);

>>>> +

>>>> +	    int sub_count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (member);

>>>> +

>>>> +	    if (sub_count + count > AAPCS_VFP_CALL_OR_RET_CAND_MAX)

>>>> +	      return;

>>>> +

>>>> 	    count += sub_count;

>>>> 	  }

>>>> -	return count;

>>>> +

>>>> +	TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);

>>>> +	return;

>>>>      }

>>>> 

>>>>    default:

>>>> -      break;

>>>> +      return;

>>>>    }

>>>> -

>>>> -  return -1;

>>>> }

>>>> 

>>>> /* Return true if an argument, whose type is described by TYPE, can be passed or

>>>> @@ -1269,16 +1285,11 @@ aapcs_is_vfp_call_or_return_candidate (struct type *type, int *count,

>>>> 

>>>>  *fundamental_type = nullptr;

>>>> 

>>>> -  int ag_count = aapcs_is_vfp_call_or_return_candidate_1 (type,

>>>> -							 fundamental_type);

>>>> +  aapcs_is_vfp_call_or_return_candidate_1 (type, fundamental_type);

>>>> 

>>>> -  if (ag_count > 0 && ag_count <= HA_MAX_NUM_FLDS)

>>>> -    {

>>>> -      *count = ag_count;

>>>> -      return true;

>>>> -    }

>>>> -  else

>>>> -    return false;

>>>> +  *count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type);

>>>> +

>>>> +  return (*count < AAPCS_VFP_CALL_OR_RET_CAND_INVALID);

>>>> }

>>>> 

>>>> /* AArch64 function call information structure.  */

>>>> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h

>>>> index f0adec7a20..600112abf5 100644

>>>> --- a/gdb/gdbtypes.h

>>>> +++ b/gdb/gdbtypes.h

>>>> @@ -391,6 +391,23 @@ DEF_ENUM_FLAGS_TYPE (enum type_instance_flag_value, type_instance_flags);

>>>> #define TYPE_ADDRESS_CLASS_ALL(t) (TYPE_INSTANCE_FLAGS(t) \

>>>> 				   & TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)

>>>> 

>>>> +

>>>> +/* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call

>>>> +   parameter or return parameter, as per the AAPCS64 5.4.2.C. Value can be:

>>>> +     0: the type has not been checked.

>>>> +     1 to AAPCS_VFP_CALL_OR_RET_CAND_MAX: Valid, and has that many members.

>>>> +     AAPCS_VFP_CALL_OR_RET_CAND_MAX: Invalid.  */

>>>> +

>>>> +#define AAPCS_VFP_CALL_OR_RET_CAND_MAX 4

>>>> +#define AAPCS_VFP_CALL_OR_RET_CAND_INVALID (AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1)

>>>> +

>>>> +#define TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype) \

>>>> +  (TYPE_MAIN_TYPE (thistype)->aapcs_candidate_count)

>>>> +

>>>> +#define TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype, val) \

>>>> +  TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (thistype) = \

>>>> +    (std::min (val, AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1))

>>>> +

>>>> /* * Information needed for a discriminated union.  A discriminated

>>>>   union is handled somewhat differently from an ordinary union.

>>>> 

>>>> @@ -716,6 +733,12 @@ struct main_type

>>>> 

>>>>  ENUM_BITFIELD(type_specific_kind) type_specific_field : 3;

>>>> 

>>>> +  /* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call

>>>> +     parameter or return parameter, as per the AAPCS64 5.4.2.C. Required to

>>>> +     prevent recursive checking.  */

>>>> +

>>>> +  unsigned int aapcs_candidate_count : 3;

>>>> +

>>>>  /* * Number of fields described for this type.  This field appears

>>>>     at this location because it packs nicely here.  */