gdb/riscv: Handle empty C++ structs during argument passing

  This commit resolves a large number of failures in the test script
gdb.base/infcall-nested-structs.exp which were caused by GDB (for
RISC-V) incorrectly handling empty C++ structures when preparing
arguments for a dummy call, or collecting a return value.

The issue is further complicated in that there was a bug in GCC, such
that in some cases GCC would generate incorrect code when passing a
small structure that contained empty sub-structures.  This was fixed
in GCC trunk on 5-March-2019, so in order to see the best results with
this patch you'll need a recent version of GCC.

Anything that used to work should continue to work after this patch,
regardless of GCC version being used.

The fix in this commit is that GDB now pays more attention to the
offset of fields within a structure when preparing arguments as in C++
an empty structure has a non-zero size, this is an example:

  struct s1 { struct s2 { } empty; int f; };

We previously assumed that 'f' was at offset 0 inside type 's1',
however this is not the case in C++ as 's2' has size 1, and with
alignment 'f' is likely at some even bigger offset inside 's1'.

gdb/ChangeLog:

	* riscv-tdep.c (riscv_call_arg_complex_float): Fix offset of first
	component to 0.
	(riscv_struct_info::riscv_struct_info): Initialise m_offsets
	member.
	(riscv_struct_info::analyse): New implementation using new
	analyse_inner member function.
	(riscv_struct_info::field_offset): New member function.
	(riscv_struct_info::m_offsets): New member variable.
	(riscv_struct_info::analyse_inner): New private member function,
	takes the old implementation of riscv_struct_info::analyse but
	extended to track field offsets.
	(riscv_call_arg_struct): Update the struct folding special cases
	to handle cases where empty C++ structs, which are non-zero
	length, are found.
	(riscv_arg_location): Initialise the length of each location, a
	non-zero length now indicates the location is in use.
	(riscv_push_dummy_call): Allow for the first location having a
	non-zero offset when setting up arguments.
	(riscv_return_value): Likewise, but for return values.
---
 gdb/ChangeLog    |  22 ++++++++
 gdb/riscv-tdep.c | 153 +++++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 132 insertions(+), 43 deletions(-)
  

* Andrew Burgess <andrew.burgess@embecosm.com> [2019-04-05 18:04:23 +0100]:

> This commit resolves a large number of failures in the test script
> gdb.base/infcall-nested-structs.exp which were caused by GDB (for
> RISC-V) incorrectly handling empty C++ structures when preparing
> arguments for a dummy call, or collecting a return value.
> 
> The issue is further complicated in that there was a bug in GCC, such
> that in some cases GCC would generate incorrect code when passing a
> small structure that contained empty sub-structures.  This was fixed
> in GCC trunk on 5-March-2019, so in order to see the best results with
> this patch you'll need a recent version of GCC.
> 
> Anything that used to work should continue to work after this patch,
> regardless of GCC version being used.
> 
> The fix in this commit is that GDB now pays more attention to the
> offset of fields within a structure when preparing arguments as in C++
> an empty structure has a non-zero size, this is an example:
> 
>   struct s1 { struct s2 { } empty; int f; };
> 
> We previously assumed that 'f' was at offset 0 inside type 's1',
> however this is not the case in C++ as 's2' has size 1, and with
> alignment 'f' is likely at some even bigger offset inside 's1'.
> 
> gdb/ChangeLog:
> 
> 	* riscv-tdep.c (riscv_call_arg_complex_float): Fix offset of first
> 	component to 0.
> 	(riscv_struct_info::riscv_struct_info): Initialise m_offsets
> 	member.
> 	(riscv_struct_info::analyse): New implementation using new
> 	analyse_inner member function.
> 	(riscv_struct_info::field_offset): New member function.
> 	(riscv_struct_info::m_offsets): New member variable.
> 	(riscv_struct_info::analyse_inner): New private member function,
> 	takes the old implementation of riscv_struct_info::analyse but
> 	extended to track field offsets.
> 	(riscv_call_arg_struct): Update the struct folding special cases
> 	to handle cases where empty C++ structs, which are non-zero
> 	length, are found.
> 	(riscv_arg_location): Initialise the length of each location, a
> 	non-zero length now indicates the location is in use.
> 	(riscv_push_dummy_call): Allow for the first location having a
> 	non-zero offset when setting up arguments.
> 	(riscv_return_value): Likewise, but for return values.

I've now pushed this patch.

Thanks,
Andrew



> ---
>  gdb/ChangeLog    |  22 ++++++++
>  gdb/riscv-tdep.c | 153 +++++++++++++++++++++++++++++++++++++++----------------
>  2 files changed, 132 insertions(+), 43 deletions(-)
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index ff5f36e7621..e933e34590a 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -1994,7 +1994,7 @@ riscv_call_arg_complex_float (struct riscv_arg_info *ainfo,
>        int len = ainfo->length / 2;
>  
>        result = riscv_assign_reg_location (&ainfo->argloc[0],
> -					  &cinfo->float_regs, len, len);
> +					  &cinfo->float_regs, len, 0);
>        gdb_assert (result);
>  
>        result = riscv_assign_reg_location (&ainfo->argloc[1],
> @@ -2015,14 +2015,18 @@ class riscv_struct_info
>  public:
>    riscv_struct_info ()
>      : m_number_of_fields (0),
> -      m_types { nullptr, nullptr }
> +      m_types { nullptr, nullptr },
> +      m_offsets { 0, 0 }
>    {
>      /* Nothing.  */
>    }
>  
>    /* Analyse TYPE descending into nested structures, count the number of
>       scalar fields and record the types of the first two fields found.  */
> -  void analyse (struct type *type);
> +  void analyse (struct type *type)
> +  {
> +    analyse_inner (type, 0);
> +  }
>  
>    /* The number of scalar fields found in the analysed type.  This is
>       currently only accurate if the value returned is 0, 1, or 2 as the
> @@ -2042,6 +2046,16 @@ public:
>      return m_types[index];
>    }
>  
> +  /* Return the offset of scalar field INDEX within the analysed type. Will
> +     return 0 if there is no field at that index.  Only INDEX values 0 and
> +     1 can be requested as the RiscV ABI only has special cases for
> +     structures with 1 or 2 fields.  */
> +  int field_offset (int index) const
> +  {
> +    gdb_assert (index < (sizeof (m_offsets) / sizeof (m_offsets[0])));
> +    return m_offsets[index];
> +  }
> +
>  private:
>    /* The number of scalar fields found within the structure after recursing
>       into nested structures.  */
> @@ -2050,13 +2064,20 @@ private:
>    /* The types of the first two scalar fields found within the structure
>       after recursing into nested structures.  */
>    struct type *m_types[2];
> +
> +  /* The offsets of the first two scalar fields found within the structure
> +     after recursing into nested structures.  */
> +  int m_offsets[2];
> +
> +  /* Recursive core for ANALYSE, the OFFSET parameter tracks the byte
> +     offset from the start of the top level structure being analysed.  */
> +  void analyse_inner (struct type *type, int offset);
>  };
>  
> -/* Analyse TYPE descending into nested structures, count the number of
> -   scalar fields and record the types of the first two fields found.  */
> +/* See description in class declaration.  */
>  
>  void
> -riscv_struct_info::analyse (struct type *type)
> +riscv_struct_info::analyse_inner (struct type *type, int offset)
>  {
>    unsigned int count = TYPE_NFIELDS (type);
>    unsigned int i;
> @@ -2068,11 +2089,13 @@ riscv_struct_info::analyse (struct type *type)
>  
>        struct type *field_type = TYPE_FIELD_TYPE (type, i);
>        field_type = check_typedef (field_type);
> +      int field_offset
> +	= offset + TYPE_FIELD_BITPOS (type, i) / TARGET_CHAR_BIT;
>  
>        switch (TYPE_CODE (field_type))
>  	{
>  	case TYPE_CODE_STRUCT:
> -	  analyse (field_type);
> +	  analyse_inner (field_type, field_offset);
>  	  break;
>  
>  	default:
> @@ -2082,7 +2105,10 @@ riscv_struct_info::analyse (struct type *type)
>  	     structure we can special case, and pass the structure in
>  	     memory.  */
>  	  if (m_number_of_fields < 2)
> -	    m_types[m_number_of_fields] = field_type;
> +	    {
> +	      m_types[m_number_of_fields] = field_type;
> +	      m_offsets[m_number_of_fields] = field_offset;
> +	    }
>  	  m_number_of_fields++;
>  	  break;
>  	}
> @@ -2115,17 +2141,54 @@ riscv_call_arg_struct (struct riscv_arg_info *ainfo,
>        if (sinfo.number_of_fields () == 1
>  	  && TYPE_CODE (sinfo.field_type (0)) == TYPE_CODE_COMPLEX)
>  	{
> -	  gdb_assert (TYPE_LENGTH (ainfo->type)
> -		      == TYPE_LENGTH (sinfo.field_type (0)));
> -	  return riscv_call_arg_complex_float (ainfo, cinfo);
> +	  /* The following is similar to RISCV_CALL_ARG_COMPLEX_FLOAT,
> +	     except we use the type of the complex field instead of the
> +	     type from AINFO, and the first location might be at a non-zero
> +	     offset.  */
> +	  if (TYPE_LENGTH (sinfo.field_type (0)) <= (2 * cinfo->flen)
> +	      && riscv_arg_regs_available (&cinfo->float_regs) >= 2
> +	      && !ainfo->is_unnamed)
> +	    {
> +	      bool result;
> +	      int len = TYPE_LENGTH (sinfo.field_type (0)) / 2;
> +	      int offset = sinfo.field_offset (0);
> +
> +	      result = riscv_assign_reg_location (&ainfo->argloc[0],
> +						  &cinfo->float_regs, len,
> +						  offset);
> +	      gdb_assert (result);
> +
> +	      result = riscv_assign_reg_location (&ainfo->argloc[1],
> +						  &cinfo->float_regs, len,
> +						  (offset + len));
> +	      gdb_assert (result);
> +	    }
> +	  else
> +	    riscv_call_arg_scalar_int (ainfo, cinfo);
> +	  return;
>  	}
>  
>        if (sinfo.number_of_fields () == 1
>  	  && TYPE_CODE (sinfo.field_type (0)) == TYPE_CODE_FLT)
>  	{
> -	  gdb_assert (TYPE_LENGTH (ainfo->type)
> -		      == TYPE_LENGTH (sinfo.field_type (0)));
> -	  return riscv_call_arg_scalar_float (ainfo, cinfo);
> +	  /* The following is similar to RISCV_CALL_ARG_SCALAR_FLOAT,
> +	     except we use the type of the first scalar field instead of
> +	     the type from AINFO.  Also the location might be at a non-zero
> +	     offset.  */
> +	  if (TYPE_LENGTH (sinfo.field_type (0)) > cinfo->flen
> +	      || ainfo->is_unnamed)
> +	    riscv_call_arg_scalar_int (ainfo, cinfo);
> +	  else
> +	    {
> +	      int offset = sinfo.field_offset (0);
> +	      int len = TYPE_LENGTH (sinfo.field_type (0));
> +
> +	      if (!riscv_assign_reg_location (&ainfo->argloc[0],
> +					      &cinfo->float_regs,
> +					      len, offset))
> +		riscv_call_arg_scalar_int (ainfo, cinfo);
> +	    }
> +	  return;
>  	}
>  
>        if (sinfo.number_of_fields () == 2
> @@ -2135,17 +2198,14 @@ riscv_call_arg_struct (struct riscv_arg_info *ainfo,
>  	  && TYPE_LENGTH (sinfo.field_type (1)) <= cinfo->flen
>  	  && riscv_arg_regs_available (&cinfo->float_regs) >= 2)
>  	{
> -	  int len0, len1, offset;
> -
> -	  gdb_assert (TYPE_LENGTH (ainfo->type) <= (2 * cinfo->flen));
> -
> -	  len0 = TYPE_LENGTH (sinfo.field_type (0));
> +	  int len0 = TYPE_LENGTH (sinfo.field_type (0));
> +	  int offset = sinfo.field_offset (0);
>  	  if (!riscv_assign_reg_location (&ainfo->argloc[0],
> -					  &cinfo->float_regs, len0, 0))
> +					  &cinfo->float_regs, len0, offset))
>  	    error (_("failed during argument setup"));
>  
> -	  len1 = TYPE_LENGTH (sinfo.field_type (1));
> -	  offset = align_up (len0, riscv_type_alignment (sinfo.field_type (1)));
> +	  int len1 = TYPE_LENGTH (sinfo.field_type (1));
> +	  offset = sinfo.field_offset (1);
>  	  gdb_assert (len1 <= (TYPE_LENGTH (ainfo->type)
>  			       - TYPE_LENGTH (sinfo.field_type (0))));
>  
> @@ -2163,15 +2223,14 @@ riscv_call_arg_struct (struct riscv_arg_info *ainfo,
>  	      && is_integral_type (sinfo.field_type (1))
>  	      && TYPE_LENGTH (sinfo.field_type (1)) <= cinfo->xlen))
>  	{
> -	  int len0, len1, offset;
> -
> -	  len0 = TYPE_LENGTH (sinfo.field_type (0));
> +	  int  len0 = TYPE_LENGTH (sinfo.field_type (0));
> +	  int offset = sinfo.field_offset (0);
>  	  if (!riscv_assign_reg_location (&ainfo->argloc[0],
> -					  &cinfo->float_regs, len0, 0))
> +					  &cinfo->float_regs, len0, offset))
>  	    error (_("failed during argument setup"));
>  
> -	  len1 = TYPE_LENGTH (sinfo.field_type (1));
> -	  offset = align_up (len0, riscv_type_alignment (sinfo.field_type (1)));
> +	  int len1 = TYPE_LENGTH (sinfo.field_type (1));
> +	  offset = sinfo.field_offset (1);
>  	  gdb_assert (len1 <= cinfo->xlen);
>  	  if (!riscv_assign_reg_location (&ainfo->argloc[1],
>  					  &cinfo->int_regs, len1, offset))
> @@ -2186,19 +2245,18 @@ riscv_call_arg_struct (struct riscv_arg_info *ainfo,
>  	      && TYPE_CODE (sinfo.field_type (1)) == TYPE_CODE_FLT
>  	      && TYPE_LENGTH (sinfo.field_type (1)) <= cinfo->flen))
>  	{
> -	  int len0, len1, offset;
> -
> -	  len0 = TYPE_LENGTH (sinfo.field_type (0));
> -	  len1 = TYPE_LENGTH (sinfo.field_type (1));
> -	  offset = align_up (len0, riscv_type_alignment (sinfo.field_type (1)));
> +	  int len0 = TYPE_LENGTH (sinfo.field_type (0));
> +	  int len1 = TYPE_LENGTH (sinfo.field_type (1));
>  
>  	  gdb_assert (len0 <= cinfo->xlen);
>  	  gdb_assert (len1 <= cinfo->flen);
>  
> +	  int offset = sinfo.field_offset (0);
>  	  if (!riscv_assign_reg_location (&ainfo->argloc[0],
> -					  &cinfo->int_regs, len0, 0))
> +					  &cinfo->int_regs, len0, offset))
>  	    error (_("failed during argument setup"));
>  
> +	  offset = sinfo.field_offset (1);
>  	  if (!riscv_assign_reg_location (&ainfo->argloc[1],
>  					  &cinfo->float_regs,
>  					  len1, offset))
> @@ -2234,6 +2292,8 @@ riscv_arg_location (struct gdbarch *gdbarch,
>    ainfo->align = riscv_type_alignment (ainfo->type);
>    ainfo->is_unnamed = is_unnamed;
>    ainfo->contents = nullptr;
> +  ainfo->argloc[0].c_length = 0;
> +  ainfo->argloc[1].c_length = 0;
>  
>    switch (TYPE_CODE (ainfo->type))
>      {
> @@ -2475,10 +2535,11 @@ riscv_push_dummy_call (struct gdbarch *gdbarch,
>  	      memset (tmp, -1, sizeof (tmp));
>  	    else
>  	      memset (tmp, 0, sizeof (tmp));
> -	    memcpy (tmp, info->contents, info->argloc[0].c_length);
> +	    memcpy (tmp, (info->contents + info->argloc[0].c_offset),
> +		    info->argloc[0].c_length);
>  	    regcache->cooked_write (info->argloc[0].loc_data.regno, tmp);
>  	    second_arg_length =
> -	      ((info->argloc[0].c_length < info->length)
> +	      (((info->argloc[0].c_length + info->argloc[0].c_offset) < info->length)
>  	       ? info->argloc[1].c_length : 0);
>  	    second_arg_data = info->contents + info->argloc[1].c_offset;
>  	  }
> @@ -2630,18 +2691,24 @@ riscv_return_value (struct gdbarch  *gdbarch,
>  			  <= register_size (gdbarch, regnum));
>  
>  	      if (readbuf)
> -		regcache->cooked_read_part (regnum, 0,
> -					    info.argloc[0].c_length,
> -					    readbuf);
> +		{
> +		  gdb_byte *ptr = readbuf + info.argloc[0].c_offset;
> +		  regcache->cooked_read_part (regnum, 0,
> +					      info.argloc[0].c_length,
> +					      ptr);
> +		}
>  
>  	      if (writebuf)
> -		regcache->cooked_write_part (regnum, 0,
> -					     info.argloc[0].c_length,
> -					     writebuf);
> +		{
> +		  const gdb_byte *ptr = writebuf + info.argloc[0].c_offset;
> +		  regcache->cooked_write_part (regnum, 0,
> +					       info.argloc[0].c_length,
> +					       ptr);
> +		}
>  
>  	      /* A return value in register can have a second part in a
>  		 second register.  */
> -	      if (info.argloc[0].c_length < info.length)
> +	      if (info.argloc[1].c_length > 0)
>  		{
>  		  switch (info.argloc[1].loc_type)
>  		    {
> -- 
> 2.14.5
>