Re: [RFC-v2] amd64-windows: Fix funcall with by-pointer arguments

[Joel Brobecker <brobecker at adacore dot com>]

Hi Pierre,

On Sat, Sep 14, 2013 at 12:10:59AM +0200, Pierre Muller wrote:
>  As explained in the other thread,
> I merged Joel's patch in this thread with my patch
> in http://www.sourceware.org/ml/gdb-patches/2013-09/msg00145.html
> 
> I attached the difference between two runs of:
> make check RUNTESTFKLAGS="gdb.base/*.exp"
> with current CVS GDB and with the patch below applied
> (plus a few other submitted patches concerning
> testsuite runs for *-*-ming* builds).
> 
> The number of failure in gdb.base
> drops from 326 to 243 FAILs.

Thanks for doing that. I haven't completely finished reviewing
the patch yet, but I wanted to send a message so that you and
everyone else knows that I am on it. The patch looked good, minus
some trivial formatting issues which I will fix, but I noticed
a problem with function returning arrays. I am not sure where
it is coming from, whether from me incorrectly applying your
change, or missing something else, or whether it's a regression.

I ran out of time for today, but will try to pursue this further
tomorrow.

Just one comment below:

> 2013-09-12  Joel Brobecker  <brobecker@adacore.com>
> 	    Pierre Muller  <muller@sourceware.org>
> 
> 	* amd64-windows-tdep.c: #include "value.h" and "inferior.h".
> 	(amd64_windows_classify): Delete.
> 	(amd64_windows_passed_by_xmm_register)
> 	(amd64_windows_return_in_xmm0_register)
> 	(amd64_windows_passed_by_integer_register)
> 	(amd64_windows_passed_by_pointer)
> 	(amd64_windows_adjust_args_passed_by_pointer)
> 	(amd64_windows_store_arg_in_reg, amd64_windows_push_arguments)
> 	(amd64_windows_push_dummy_call): New functions.
> 	(amd64_return_value): Use new functions above. Add "debug infrum"
> 	information.
> 	(amd64_windows_init_abi): Remove setting of
> 	tdep->call_dummy_num_integer_regs, tdep->call_dummy_integer_regs,
> 	tdep->classify, tdep->memory_args_by_pointer and
> 	tdep->integer_param_regs_saved_in_caller_frame.
> 	Add call to set_gdbarch_push_dummy_call.
>  /* The registers used to pass integer arguments during a function call.  */
>  static int amd64_windows_dummy_call_integer_regs[] =
>  {
>    AMD64_RCX_REGNUM,          /* %rcx */
>    AMD64_RDX_REGNUM,          /* %rdx */
> -  8,                         /* %r8 */
> -  9                          /* %r9 */
> +  AMD64_R8_REGNUM,           /* %r8 */
> +  AMD64_R9_REGNUM            /* %r9 */

This change can be checked in independently of this patch series,
I think.


>  };
>  
> -/* Implement the "classify" method in the gdbarch_tdep structure
> -   for amd64-windows.  */
> +/* Return nonzero if an argument of type TYPE should be passed
> +   via one of the XMM registers.  */
>  
> -static void
> -amd64_windows_classify (struct type *type, enum amd64_reg_class class[2])
> +static int
> +amd64_windows_passed_by_xmm_register (struct type *type)
>  {
>    switch (TYPE_CODE (type))
>      {
> -      case TYPE_CODE_ARRAY:
> -	/* Arrays are always passed by memory.	*/
> -	class[0] = class[1] = AMD64_MEMORY;
> -	break;
> +      case TYPE_CODE_TYPEDEF:
> +	if (TYPE_TARGET_TYPE (type))
> +	  return amd64_windows_passed_by_xmm_register (
> +		   TYPE_TARGET_TYPE (type));
> +	else
> +	  return 0;
> +      case TYPE_CODE_STRUCT:
> +      case TYPE_CODE_UNION:
> +	if (TYPE_NFIELDS (type) == 1)
> +	  return amd64_windows_passed_by_xmm_register (
> +		    TYPE_FIELD_TYPE (type, 0));
> +	else
> +	  return 0;
> +      case TYPE_CODE_FLT:
> +      case TYPE_CODE_DECFLOAT:
> +	return (TYPE_LENGTH (type) == 4 || TYPE_LENGTH (type) == 8);
> +      default:
> +	return 0;
> +    }
> +}
> +
> +/* Return nonzero if an return value of type TYPE should be passed
> +   in XMM0 register.  */
> +
> +static int
> +amd64_windows_return_in_xmm0_register (struct type *type)
> +{
> +  if (amd64_windows_passed_by_xmm_register (type))
> +    return 1;
> +  else
> +    {
> +      /* __m128, __m128d and __m128i types are returned in $xmm0 also.
> +	 But they are not stored in XMM registers as arguments,
> +	 at least not in GCC 4.7.6 x86_64-w64-mingw32 target.
> +	 Note: The TYPE_VECTOR check should prevent other arrays from
> +	 being treated as special xmm types.
> +         Note: This all works for the "unique" __fastcall calling
> convention
> +	 as defined by Microsoft.  The newer __vectorcall convention
> +	 does support passing __m128X type parameters in xmm registers
> +	 using /Gv option of Microsoft compilers.  */
> +      struct type *ltype = check_typedef (type);
> +      if (ltype && TYPE_CODE (ltype) == TYPE_CODE_ARRAY
> +	  && (TYPE_CODE (TYPE_TARGET_TYPE (ltype)) == TYPE_CODE_FLT
> +	      || TYPE_CODE (TYPE_TARGET_TYPE (ltype)) == TYPE_CODE_INT)
> +	  && TYPE_VECTOR (ltype) && TYPE_LENGTH (ltype) == 16)
> +	return 1;
> +      else
> +	return 0;
> +    }
> +}
> +
> +/* Return nonzero if an argument of type TYPE should be passed
> +   via one of the integer registers.  */
>  
> +static int
> +amd64_windows_passed_by_integer_register (struct type *type)
> +{
> +  switch (TYPE_CODE (type))
> +    {
> +      case TYPE_CODE_TYPEDEF:
> +	if (TYPE_TARGET_TYPE (type))
> +	  return amd64_windows_passed_by_integer_register (
> +		   TYPE_TARGET_TYPE (type));
> +	else
> +	  return 0;
>        case TYPE_CODE_STRUCT:
>        case TYPE_CODE_UNION:
> -        /* Struct/Union types whose size is 1, 2, 4, or 8 bytes
> -	   are passed as if they were integers of the same size.
> -	   Types of different sizes are passed by memory.  */
> -	if (TYPE_LENGTH (type) == 1
> -	    || TYPE_LENGTH (type) == 2
> -	    || TYPE_LENGTH (type) == 4
> -	    || TYPE_LENGTH (type) == 8)
> +	if (TYPE_NFIELDS (type) == 1)
>  	  {
> -	    class[0] = AMD64_INTEGER;
> -	    class[1] = AMD64_NO_CLASS;
> +	    /* If only one field, exclude float type accepted as xmm reg.
> */
> +	    if (amd64_windows_passed_by_xmm_register (
> +		  TYPE_FIELD_TYPE (type, 0)))
> +	      return 0;
> +	    else
> +	      return amd64_windows_passed_by_integer_register (
> +		    TYPE_FIELD_TYPE (type, 0));
>  	  }
> -	else
> -	  class[0] = class[1] = AMD64_MEMORY;
> -	break;
> +      /* Struct/Union with several fields: Pass Through.  */
> +      case TYPE_CODE_INT:
> +      case TYPE_CODE_ENUM:
> +      case TYPE_CODE_BOOL:
> +      case TYPE_CODE_RANGE:
> +      case TYPE_CODE_CHAR:
> +      case TYPE_CODE_PTR:
> +      case TYPE_CODE_REF:
> +      case TYPE_CODE_COMPLEX:
> +	return (TYPE_LENGTH (type) == 1
> +		|| TYPE_LENGTH (type) == 2
> +		|| TYPE_LENGTH (type) == 4
> +		|| TYPE_LENGTH (type) == 8);
>  
>        default:
> -	/* For all the other types, the conventions are the same as
> -	   with the System V ABI.  */
> -	amd64_classify (type, class);
> +	return 0;
>      }
>  }
>  
> +/* Return non-zero iff an argument of the given TYPE should be passed
> +   by pointer.  */
> +
> +static int
> +amd64_windows_passed_by_pointer (struct type *type)
> +{
> +  if (amd64_windows_passed_by_integer_register (type))
> +    return 0;
> +
> +  if (amd64_windows_passed_by_xmm_register (type))
> +    return 0;
> +
> +  return 1;
> +}
> +
> +/* For each argument that should be passed by pointer, reserve some
> +   stack space, store a copy of the argument on the stack, and replace
> +   the argument by its address.  Return the new Stack Pointer value.
> +
> +   NARGS is the number of arguments. ARGS is the array containing
> +   the value of each argument.  SP is value of the Stack Pointer.  */
> +
> +static CORE_ADDR
> +amd64_windows_adjust_args_passed_by_pointer (struct gdbarch *gdbarch,
> +					     struct value **args,
> +					     int nargs, CORE_ADDR sp)
> +{
> +  int i;
> +
> +  for (i = 0; i < nargs; i++)
> +    if (amd64_windows_passed_by_pointer (value_type (args[i])))
> +      {
> +	struct type *type = value_type (args[i]);
> +	const gdb_byte *valbuf = value_contents (args[i]);
> +	const int len = TYPE_LENGTH (type);
> +
> +	/* Store a copy of that argument on the stack, aligned to
> +	   a 16 bytes boundary, and then use the copy's address as
> +	   the argument.  */
> +
> +	sp -= len;
> +	sp &= ~0xf;
> +	write_memory (sp, valbuf, len);
> +
> +	args[i] =
> +	  value_addr (value_from_contents_and_address (type, valbuf, sp));
> +	if (debug_infrun)
> +	  printf_filtered (_("Parameter #%d, length %d  copied on stack "
> +			     "at address %s, and passed as pointer\n"),
> +			   i, len, paddress (gdbarch, sp));
> +      }
> +
> +  return sp;
> +}
> +
> +/* Store the value of ARG in register REGNO (right-justified).
> +   REGCACHE is the register cache.  */
> +
> +static void
> +amd64_windows_store_arg_in_reg (struct gdbarch *gdbarch,
> +				struct regcache *regcache,
> +				struct value *arg, int regno, int i)
> +{
> +  struct type *type = value_type (arg);
> +  const gdb_byte *valbuf = value_contents (arg);
> +  gdb_byte buf[8];
> +
> +  gdb_assert (TYPE_LENGTH (type) <= 8);
> +  memset (buf, 0, sizeof buf);
> +  memcpy (buf, valbuf, min (TYPE_LENGTH (type), 8));
> +  regcache_cooked_write (regcache, regno, buf);
> +  if (debug_infrun)
> +    printf_filtered (_("Parameter #%d, length %d copied to register %s\n"),
> +		     i, TYPE_LENGTH (type), i386_pseudo_register_name(
> +					      gdbarch, regno));
> +}
> +
> +/* Push the arguments for an inferior function call, and return
> +   the updated value of the SP (Stack Pointer).
> +
> +   All arguments are identical to the arguments used in
> +   amd64_windows_push_dummy_call.  */
> +
> +static CORE_ADDR
> +amd64_windows_push_arguments (struct gdbarch *gdbarch,
> +			      struct regcache *regcache, int nargs,
> +			      struct value **args, CORE_ADDR sp,
> +			      int struct_return)
> +{
> +  int reg_idx = 0;
> +  int i;
> +  struct value **stack_args = alloca (nargs * sizeof (struct value *));
> +  int num_stack_args = 0;
> +  int num_elements = 0;
> +  int element = 0;
> +
> +  /* First, handle the arguments passed by pointer.
> +
> +     These arguments are replaced by pointers to a copy we are making
> +     in inferior memory.  So use a copy of the ARGS table, to avoid
> +     modifying the original one.  */
> +  {
> +    struct value **args1 = alloca (nargs * sizeof (struct value *));
> +
> +    memcpy (args1, args, nargs * sizeof (struct value *));
> +    sp = amd64_windows_adjust_args_passed_by_pointer (gdbarch, args1,
> +						      nargs, sp);
> +    args = args1;
> +  }
> +
> +  /* Reserve a register for the "hidden" argument.  */
> +  if (struct_return)
> +    reg_idx++;
> +
> +  for (i = 0; i < nargs; i++)
> +    {
> +      struct type *type = value_type (args[i]);
> +      int len = TYPE_LENGTH (type);
> +      int on_stack_p = 1;
> +
> +      if (reg_idx < ARRAY_SIZE (amd64_windows_dummy_call_integer_regs))
> +	{
> +	  if (amd64_windows_passed_by_integer_register (type))
> +	    {
> +	      amd64_windows_store_arg_in_reg
> +		(gdbarch, regcache, args[i],
> +		 amd64_windows_dummy_call_integer_regs[reg_idx], i);
> +	      on_stack_p = 0;
> +	      reg_idx++;
> +	    }
> +	  else if (amd64_windows_passed_by_xmm_register (type))
> +	    {
> +	      amd64_windows_store_arg_in_reg
> +	        (gdbarch, regcache, args[i], AMD64_XMM0_REGNUM + reg_idx,
> i);
> +	      /* In case of varargs, these parameters must also be
> +		 passed via the integer registers.  */
> +	      amd64_windows_store_arg_in_reg
> +		(gdbarch, regcache, args[i],
> +		 amd64_windows_dummy_call_integer_regs[reg_idx], i);
> +	      on_stack_p = 0;
> +	      reg_idx++;
> +	    }
> +	}
> +
> +      if (on_stack_p)
> +	{
> +	  num_elements += ((len + 7) / 8);
> +	  stack_args[num_stack_args++] = args[i];
> +	}
> +    }
> +
> +  /* Allocate space for the arguments on the stack, keeping it
> +     aligned on a 16 byte boundary.  */
> +  sp -= num_elements * 8;
> +  sp &= ~0xf;
> +
> +  /* Write out the arguments to the stack.  */
> +  for (i = 0; i < num_stack_args; i++)
> +    {
> +      struct type *type = value_type (stack_args[i]);
> +      const gdb_byte *valbuf = value_contents (stack_args[i]);
> +
> +      write_memory (sp + element * 8, valbuf, TYPE_LENGTH (type));
> +      if (debug_infrun)
> +	printf_filtered (_("Parameter #%d, length %d copied on stack"
> +			   " at address %s\n"),
> +			   i, TYPE_LENGTH (type),
> +			   paddress (gdbarch, sp + element * 8));
> +      element += ((TYPE_LENGTH (type) + 7) / 8);
> +    }
> +
> +  return sp;
> +}
> +
> +/* Implement the "push_dummy_call" gdbarch method.  */
> +
> +static CORE_ADDR
> +amd64_windows_push_dummy_call
> +  (struct gdbarch *gdbarch, struct value *function,
> +   struct regcache *regcache, CORE_ADDR bp_addr,
> +   int nargs, struct value **args,
> +   CORE_ADDR sp, int struct_return, CORE_ADDR struct_addr)
> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  gdb_byte buf[8];
> +
> +  /* Pass arguments.  */
> +  sp = amd64_windows_push_arguments (gdbarch, regcache, nargs, args, sp,
> +				     struct_return);
> +
> +  /* Pass "hidden" argument".  */
> +  if (struct_return)
> +    {
> +      /* The "hidden" argument is passed throught the first argument
> +         register.  */
> +      const int arg_regnum = amd64_windows_dummy_call_integer_regs[0];
> +
> +      store_unsigned_integer (buf, 8, byte_order, struct_addr);
> +      regcache_cooked_write (regcache, arg_regnum, buf);
> +    }
> +
> +  /* Reserve some memory on the stack for the integer-parameter
> +     registers, as required by the ABI.  */
> +  sp -= ARRAY_SIZE (amd64_windows_dummy_call_integer_regs) * 8;
> +
> +  /* Store return address.  */
> +  sp -= 8;
> +  store_unsigned_integer (buf, 8, byte_order, bp_addr);
> +  write_memory (sp, buf, 8);
> +
> +  /* Update the stack pointer...  */
> +  store_unsigned_integer (buf, 8, byte_order, sp);
> +  regcache_cooked_write (regcache, AMD64_RSP_REGNUM, buf);
> +
> +  /* ...and fake a frame pointer.  */
> +  regcache_cooked_write (regcache, AMD64_RBP_REGNUM, buf);
> +
> +  return sp + 16;
> +}
> +
>  /* Implement the "return_value" gdbarch method for amd64-windows.  */
>  
>  static enum return_value_convention
> @@ -90,22 +379,10 @@ amd64_windows_return_value (struct gdbar
>  
>    /* See if our value is returned through a register.  If it is, then
>       store the associated register number in REGNUM.  */
> -  switch (TYPE_CODE (type))
> -    {
> -      case TYPE_CODE_FLT:
> -      case TYPE_CODE_DECFLOAT:
> -        /* __m128, __m128i, __m128d, floats, and doubles are returned
> -           via XMM0.  */
> -        if (len == 4 || len == 8 || len == 16)
> -          regnum = AMD64_XMM0_REGNUM;
> -        break;
> -      default:
> -        /* All other values that are 1, 2, 4 or 8 bytes long are returned
> -           via RAX.  */
> -        if (len == 1 || len == 2 || len == 4 || len == 8)
> -          regnum = AMD64_RAX_REGNUM;
> -        break;
> -    }
> +  if (amd64_windows_return_in_xmm0_register (type))
> +    regnum = AMD64_XMM0_REGNUM;
> +  else if (amd64_windows_passed_by_integer_register (type))
> +    regnum = AMD64_RAX_REGNUM;
>  
>    if (regnum < 0)
>      {
> @@ -117,8 +394,10 @@ amd64_windows_return_value (struct gdbar
>  	  regcache_raw_read_unsigned (regcache, AMD64_RAX_REGNUM, &addr);
>  	  read_memory (addr, readbuf, TYPE_LENGTH (type));
>  	}
> +      if (debug_infrun)
> +	printf_filtered (_("Return value as memory address in RAX\n"));
>        return RETURN_VALUE_ABI_RETURNS_ADDRESS;
> -    }
> +     }
>    else
>      {
>        /* Extract the return value from the register where it was stored.
> */
> @@ -126,6 +405,9 @@ amd64_windows_return_value (struct gdbar
>  	regcache_raw_read_part (regcache, regnum, 0, len, readbuf);
>        if (writebuf)
>  	regcache_raw_write_part (regcache, regnum, 0, len, writebuf);
> +      if (debug_infrun)
> +	printf_filtered (_("Return value in register %s\n"),
> +			 gdbarch_register_name (gdbarch, regnum));
>        return RETURN_VALUE_REGISTER_CONVENTION;
>      }
>  }
> @@ -976,12 +1258,7 @@ amd64_windows_init_abi (struct gdbarch_i
>    set_gdbarch_long_bit (gdbarch, 32);
>  
>    /* Function calls.  */
> -  tdep->call_dummy_num_integer_regs =
> -    ARRAY_SIZE (amd64_windows_dummy_call_integer_regs);
> -  tdep->call_dummy_integer_regs = amd64_windows_dummy_call_integer_regs;
> -  tdep->classify = amd64_windows_classify;
> -  tdep->memory_args_by_pointer = 1;
> -  tdep->integer_param_regs_saved_in_caller_frame = 1;
> +  set_gdbarch_push_dummy_call (gdbarch, amd64_windows_push_dummy_call);
>    set_gdbarch_return_value (gdbarch, amd64_windows_return_value);
>    set_gdbarch_skip_main_prologue (gdbarch, amd64_skip_main_prologue);
>    set_gdbarch_skip_trampoline_code (gdbarch,



-- 
Joel