Re: [RFC] Improve windows amd64 call command

[Joel Brobecker <brobecker at adacore dot com>]

> mingw64 GDB does a poor job when it comes to 
> handling call command.
> 
>   This is because it uses the SYS V ABI,
> while windows OS uses a separate ABI called Microsoft x64 calling convention
> in

Hi Pierre. I think you patch is the same as the first one I initially
proposed, and got rejected by Mark saying that we should really have our
own separate code for Windows as the two ABIs are really completely
different. Since then, I've re-implemented the calling convention
following that advice. The patch is posted at:
http://www.sourceware.org/ml/gdb-patches/2013-01/msg00332.html

I cannot believe it was 8 months ago! I was waiting on Mark's input,
and then meant to just go ahead and finish it up, and since then,
life has taken over.

Can you try my patch and see if it works as well for you? (assuming
it still applies - otherwise, I will rebase for you tomorrow).
If not, we could work together to finalize it and get it in for
everyone to enjoy.

> 
> http://en.wikipedia.org/wiki/X86_calling_conventions#x86-64_calling_conventi
> ons
> 
>   This patch removes all failures in gdb.base/call*.exp
> when using x86_6'-w64-mingw32-gcc version 4.7.3,
> but some of those results might be GCC version dependent...
> It would also be nice to  know what happens if a Microsoft compiler
> is used...
> 
>   The only regression I found in gdb.base is in  varargs.exp:
> < FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3,
> fc4)
> < FAIL: gdb.base/varargs.exp: print find_max_double_real(4, dc1, dc2, dc3,
> dc4)
> < FAIL: gdb.base/varargs.exp: print find_max_long_double_real(4, ldc1, ldc2,
> ldc3, ldc4)
> ---
> > FAIL: gdb.base/varargs.exp: print find_max_double(5,1.0,17.0,2.0,3.0,4.0)
> It's isn't a true regression, as there is only one failure instead of three,
> but the one failure seemed to pass without the change...
> I didn't investigate that yet.
> 
>  The overall change for gdb.base testsuite runs is 253 failures instead of
> 327
> without the patch.
> 
>  Comments welcome,
> 
> 
> Pierre Muller
> GDB pascal language maintainer
> 
> 2013-09-03  Pierre Muller  <muller@sourceware.org>
> 
> 	* i386-tdep.h (struct gdbarch_tdep): Add new fields,
> 	neede to handle amd64 windows specfic calling conventions.
> 	* amd64-tdep.c (amd64_dummy_call_fp_regs): New array.
> 	(amd64_return_value): Add debug infrun information.
> 	(amd64_push_arguments): handle new fields of gdbarch_tdep
> 	structure.
> 	(amd64_push_dummy_call): Likewise.
> 	(amd64_init_abi): Initialize added fields.
> 	* amd64-windows-tdep.c (amd64_windows_dummy_call_fp_regs): New
> array.
> 	(amd64_windows_classify): Try to comply with Windows ABI.
> 	(amd64_windows_return_value): Do not use $xmm0 return register
> 	for "long double" return type.
> 	(amd64_windows_init_abi): Initialize new fields of gdbarch_tdep.
> 	
> 
> Index: src/gdb/i386-tdep.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-tdep.h,v
> retrieving revision 1.82
> diff -u -p -r1.82 i386-tdep.h
> --- src/gdb/i386-tdep.h	1 Jan 2013 06:32:45 -0000	1.82
> +++ src/gdb/i386-tdep.h	4 Sep 2013 21:21:34 -0000
> @@ -80,6 +80,10 @@ struct gdbarch_tdep
>       are passed through the stack on x86.  */
>    int call_dummy_num_integer_regs;
>    int *call_dummy_integer_regs;
> +  /* Used on amd64 only.  The floating point registers used to pass
> +     reals when making function calls.  */
> +  int call_dummy_num_fp_regs;
> +  int *call_dummy_fp_regs;
>  
>    /* Used on amd64 only.  Classify TYPE according to calling conventions,
>       and store the result in CLASS.  */
> @@ -104,6 +108,14 @@ struct gdbarch_tdep
>       above).  */
>    int integer_param_regs_saved_in_caller_frame;
>  
> +  /* Used on amd64 only.
> +
> +     If non-zero, integer and floating point registers are coupled:
> +     Either ECX or XMM0 are used as first register argument,
> +     but second will be EDX or XMM1, never ECX nor XMM0, this
> +     coupled behavior is used in Microsoft x64 ABI.  */
> +  int integer_and_fp_regs_coupled;
> +
>    /* Floating-point registers.  */
>    struct regset *fpregset;
>    size_t sizeof_fpregset;
> Index: src/gdb/amd64-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/amd64-tdep.c,v
> retrieving revision 1.117
> diff -u -p -r1.117 amd64-tdep.c
> --- src/gdb/amd64-tdep.c	1 Jan 2013 06:32:37 -0000	1.117
> +++ src/gdb/amd64-tdep.c	4 Sep 2013 21:21:36 -0000
> @@ -103,6 +103,16 @@ static int amd64_dummy_call_integer_regs
>    9				/* %r9 */
>  };
>  
> +/* The registers used to pass floating point arguments in function call.
> */
> +static int amd64_dummy_call_fp_regs[] =
> +{
> +  /* %xmm0 ... %xmm7 */
> +  AMD64_XMM0_REGNUM + 0, AMD64_XMM1_REGNUM,
> +  AMD64_XMM0_REGNUM + 2, AMD64_XMM0_REGNUM + 3,
> +  AMD64_XMM0_REGNUM + 4, AMD64_XMM0_REGNUM + 5,
> +  AMD64_XMM0_REGNUM + 6, AMD64_XMM0_REGNUM + 7,
> +};
> +
>  /* DWARF Register Number Mapping as defined in the System V psABI,
>     section 3.6.  */
>  
> @@ -650,6 +660,8 @@ amd64_return_value (struct gdbarch *gdba
>  	  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;
>      }
>  
> @@ -674,6 +686,8 @@ amd64_return_value (struct gdbarch *gdba
>  	  regcache_raw_write_unsigned (regcache, AMD64_FTAG_REGNUM, 0xfff);
>  	}
>  
> +      if (debug_infrun)
> +	printf_filtered (_("Complex X87 return value in ST0/ST1\n"));
>        return RETURN_VALUE_REGISTER_CONVENTION;
>      }
>  
> @@ -739,6 +753,9 @@ amd64_return_value (struct gdbarch *gdba
>        if (writebuf)
>  	regcache_raw_write_part (regcache, regnum, offset, min (len, 8),
>  				 writebuf + i * 8);
> +      if (debug_infrun)
> +	printf_filtered (_("Return value in register %s\n"),
> +			 gdbarch_register_name (gdbarch, regnum));
>      }
>  
>    return RETURN_VALUE_REGISTER_CONVENTION;
> @@ -753,15 +770,10 @@ amd64_push_arguments (struct regcache *r
>    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>    int *integer_regs = tdep->call_dummy_integer_regs;
>    int num_integer_regs = tdep->call_dummy_num_integer_regs;
> +  int *sse_regs = tdep->call_dummy_fp_regs;
> +  int num_sse_regs = tdep->call_dummy_num_fp_regs;
> +  int regs_coupled = tdep->integer_and_fp_regs_coupled;
>  
> -  static int sse_regnum[] =
> -  {
> -    /* %xmm0 ... %xmm7 */
> -    AMD64_XMM0_REGNUM + 0, AMD64_XMM1_REGNUM,
> -    AMD64_XMM0_REGNUM + 2, AMD64_XMM0_REGNUM + 3,
> -    AMD64_XMM0_REGNUM + 4, AMD64_XMM0_REGNUM + 5,
> -    AMD64_XMM0_REGNUM + 6, AMD64_XMM0_REGNUM + 7,
> -  };
>    struct value **stack_args = alloca (nargs * sizeof (struct value *));
>    /* An array that mirrors the stack_args array.  For all arguments
>       that are passed by MEMORY, if that argument's address also needs
> @@ -769,8 +781,10 @@ amd64_push_arguments (struct regcache *r
>       that register number (or a negative value otherwise).  */
>    int *arg_addr_regno = alloca (nargs * sizeof (int));
>    int num_stack_args = 0;
> -  int num_elements = 0;
> -  int element = 0;
> +  int total_64bit_elements = 0;
> +  int stack_64bit_elements = 0;
> +  int indirect_element = 0;
> +  int addr_element = 0;
>    int integer_reg = 0;
>    int sse_reg = 0;
>    int i;
> @@ -798,19 +812,27 @@ amd64_push_arguments (struct regcache *r
>        for (j = 0; j < 2; j++)
>  	{
>  	  if (class[j] == AMD64_INTEGER)
> -	    needed_integer_regs++;
> +	    {
> +	      needed_integer_regs++;
> +	      if (regs_coupled)
> +		needed_sse_regs++;
> +	    }
>  	  else if (class[j] == AMD64_SSE)
> -	    needed_sse_regs++;
> +	    {
> +	      needed_sse_regs++;
> +	      if (regs_coupled)
> +		needed_integer_regs++;
> +	    }
>  	}
>  
>        /* Check whether enough registers are available, and if the
>           argument should be passed in registers at all.  */
>        if (integer_reg + needed_integer_regs > num_integer_regs
> -	  || sse_reg + needed_sse_regs > ARRAY_SIZE (sse_regnum)
> +	  || sse_reg + needed_sse_regs > num_sse_regs
>  	  || (needed_integer_regs == 0 && needed_sse_regs == 0))
>  	{
>  	  /* The argument will be passed on the stack.  */
> -	  num_elements += ((len + 7) / 8);
> +	  total_64bit_elements += ((len + 7) / 8);
>  	  stack_args[num_stack_args] = args[i];
>            /* If this is an AMD64_MEMORY argument whose address must also
>               be passed in one of the integer registers, reserve that
> @@ -818,11 +840,29 @@ amd64_push_arguments (struct regcache *r
>               we can store the argument address as soon as we know it.  */
>            if (class[0] == AMD64_MEMORY
>                && tdep->memory_args_by_pointer
> -              && integer_reg < tdep->call_dummy_num_integer_regs)
> -            arg_addr_regno[num_stack_args] =
> -              tdep->call_dummy_integer_regs[integer_reg++];
> +              && integer_reg < num_integer_regs)
> +            {
> +	      arg_addr_regno[num_stack_args] =
> +		tdep->call_dummy_integer_regs[integer_reg++];
> +	      if (debug_infrun)
> +		printf_filtered (_("Arg. %d (len=%d on stack),"
> +				   " addr. in register %s\n"),
> +		  i, len, gdbarch_register_name (gdbarch,
> +			    arg_addr_regno[num_stack_args]));
> +	      if (regs_coupled)
> +		sse_reg++;
> +	    }
>            else
> -            arg_addr_regno[num_stack_args] = -1;
> +            {
> +	      arg_addr_regno[num_stack_args] = -1;
> +	      if (tdep->memory_args_by_pointer
> +		  && len != 1 && len != 2 && len != 4 && len != 8)
> +		total_64bit_elements += 1;
> +	      stack_64bit_elements += 1;
> +	      if (debug_infrun)
> +		printf_filtered (_("Arg. %d (len=%d on stack)\n"),
> +		  i, len);
> +	    }
>            num_stack_args++;
>  	}
>        else
> @@ -842,15 +882,28 @@ amd64_push_arguments (struct regcache *r
>  		{
>  		case AMD64_INTEGER:
>  		  regnum = integer_regs[integer_reg++];
> +		  if (debug_infrun)
> +		    printf_filtered (_("Arg. %d in register %s\n"),
> +		      i, gdbarch_register_name (gdbarch, regnum));
> +		  if (regs_coupled)
> +		    sse_reg++;
>  		  break;
>  
>  		case AMD64_SSE:
> -		  regnum = sse_regnum[sse_reg++];
> +		  regnum = sse_regs[sse_reg++];
> +		  if (debug_infrun)
> +		    printf_filtered (_("Arg. %d in register %s\n"),
> +		      i, gdbarch_register_name (gdbarch, regnum));
> +		  if (regs_coupled)
> +		    integer_reg++;
>  		  break;
>  
>  		case AMD64_SSEUP:
>  		  gdb_assert (sse_reg > 0);
> -		  regnum = sse_regnum[sse_reg - 1];
> +		  if (debug_infrun)
> +		    printf_filtered (_("Arg. %d in register %s at offset
> 8\n"),
> +		      i, gdbarch_register_name (gdbarch, regnum));
> +		  regnum = sse_regs[sse_reg - 1];
>  		  offset = 8;
>  		  break;
>  
> @@ -867,7 +920,7 @@ amd64_push_arguments (struct regcache *r
>      }
>  
>    /* Allocate space for the arguments on the stack.  */
> -  sp -= num_elements * 8;
> +  sp -= total_64bit_elements * 8;
>  
>    /* The psABI says that "The end of the input argument area shall be
>       aligned on a 16 byte boundary."  */
> @@ -878,9 +931,20 @@ amd64_push_arguments (struct regcache *r
>      {
>        struct type *type = value_type (stack_args[i]);
>        const gdb_byte *valbuf = value_contents (stack_args[i]);
> -      CORE_ADDR arg_addr = sp + element * 8;
> +      int len = TYPE_LENGTH (type);
> +      CORE_ADDR arg_addr;
> +      int indirect = (arg_addr_regno[i] > 0)
> +		     || (tdep->memory_args_by_pointer
> +			 && len != 1 && len != 2 && len != 4 && len != 8);
> +      if (indirect)
> +	arg_addr = sp + (stack_64bit_elements + indirect_element) * 8;
> +      else
> +	arg_addr = sp + addr_element * 8;
>  
> -      write_memory (arg_addr, valbuf, TYPE_LENGTH (type));
> +      write_memory (arg_addr, valbuf, len);
> +      if (debug_infrun)
> +	printf_unfiltered (_("Stack arg. %d (len=%d) at addr. %s\n"),
> +			   i, len, paddress (gdbarch, arg_addr));
>        if (arg_addr_regno[i] >= 0)
>          {
>            /* We also need to store the address of that argument in
> @@ -890,8 +954,34 @@ amd64_push_arguments (struct regcache *r
>  
>            store_unsigned_integer (buf, 8, byte_order, arg_addr);
>            regcache_cooked_write (regcache, arg_addr_regno[i], buf);
> +	  if (debug_infrun)
> +	    printf_unfiltered (_("Stack arg. %d addr. stored in reg. %s\n"),
> +			       i, gdbarch_register_name (gdbarch,
> +				    arg_addr_regno[i]));
>          }
> -      element += ((TYPE_LENGTH (type) + 7) / 8);
> +      else if (indirect)
> +	{
> +          /* We also need to store the address of that argument in
> +             the given register.  */
> +          gdb_byte buf[8];
> +          enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +	  CORE_ADDR arg_addrp = sp + addr_element * 8;
> +
> +          store_unsigned_integer (buf, 8, byte_order, arg_addr);
> +	  write_memory (arg_addrp, buf, 8);
> +
> +	  if (debug_infrun)
> +	    printf_unfiltered (_("Stack arg. %d addr. stored at addr.
> %s\n"),
> +			       i, paddress (gdbarch, arg_addrp));
> +	}
> +      if (indirect)
> +	{
> +	  indirect_element += ((len + 7) / 8);
> +	  if (arg_addr_regno[i] < 0)
> +	    addr_element += 1;
> +	}
> +      else
> +	addr_element += ((len + 7) / 8);
>      }
>  
>    /* The psABI says that "For calls that may call functions that use
> @@ -899,7 +989,7 @@ amd64_push_arguments (struct regcache *r
>       containing ellipsis (...) in the declaration) %al is used as
>       hidden argument to specify the number of SSE registers used.  */
>    regcache_raw_write_unsigned (regcache, AMD64_RAX_REGNUM, sse_reg);
> -  return sp; 
> +  return sp;
>  }
>  
>  static CORE_ADDR
> @@ -913,7 +1003,11 @@ amd64_push_dummy_call (struct gdbarch *g
>    gdb_byte buf[8];
>  
>    /* Pass arguments.  */
> +  if (debug_infrun)
> +    printf_unfiltered (_("Start sp=%s\n"), paddress (gdbarch, sp));
>    sp = amd64_push_arguments (regcache, nargs, args, sp, struct_return);
> +  if (debug_infrun)
> +    printf_unfiltered (_("sp after push args=%s\n"), paddress (gdbarch,
> sp));
>  
>    /* Pass "hidden" argument".  */
>    if (struct_return)
> @@ -929,19 +1023,33 @@ amd64_push_dummy_call (struct gdbarch *g
>    /* Reserve some memory on the stack for the integer-parameter registers,
>       if required by the ABI.  */
>    if (tdep->integer_param_regs_saved_in_caller_frame)
> -    sp -= tdep->call_dummy_num_integer_regs * 8;
> +    {
> +      sp -= tdep->call_dummy_num_integer_regs * 8;
> +      if (debug_infrun)
> +	printf_unfiltered (_("sp after integer reg. saved area=%s\n"),
> +			   paddress (gdbarch, sp));
> +
> +    }
>  
>    /* Store return address.  */
>    sp -= 8;
>    store_unsigned_integer (buf, 8, byte_order, bp_addr);
>    write_memory (sp, buf, 8);
> +  if (debug_infrun)
> +    printf_unfiltered (_("Return addr. %s stored at %s\n"),
> +		       paddress (gdbarch, bp_addr), paddress (gdbarch, sp));
> +
>  
>    /* Finally, update the stack pointer...  */
>    store_unsigned_integer (buf, 8, byte_order, sp);
>    regcache_cooked_write (regcache, AMD64_RSP_REGNUM, buf);
> +  if (debug_infrun)
> +    printf_unfiltered (_("Setting RSP to %s\n"), paddress (gdbarch, sp));
>  
>    /* ...and fake a frame pointer.  */
>    regcache_cooked_write (regcache, AMD64_RBP_REGNUM, buf);
> +  if (debug_infrun)
> +    printf_unfiltered (_("Setting RBP to %s\n"), paddress (gdbarch, sp));
>  
>    return sp + 16;
>  }
> @@ -2924,7 +3032,11 @@ amd64_init_abi (struct gdbarch_info info
>    tdep->call_dummy_num_integer_regs =
>      ARRAY_SIZE (amd64_dummy_call_integer_regs);
>    tdep->call_dummy_integer_regs = amd64_dummy_call_integer_regs;
> +  tdep->call_dummy_num_fp_regs =
> +    ARRAY_SIZE (amd64_dummy_call_fp_regs);
> +  tdep->call_dummy_fp_regs = amd64_dummy_call_fp_regs;
>    tdep->classify = amd64_classify;
> +  tdep->integer_and_fp_regs_coupled = 0;
>  
>    set_gdbarch_convert_register_p (gdbarch, i387_convert_register_p);
>    set_gdbarch_register_to_value (gdbarch, i387_register_to_value);
> Index: src/gdb/amd64-windows-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/amd64-windows-tdep.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 amd64-windows-tdep.c
> --- src/gdb/amd64-windows-tdep.c	2 Sep 2013 09:28:02 -0000	1.17
> +++ src/gdb/amd64-windows-tdep.c	4 Sep 2013 21:21:36 -0000
> @@ -31,6 +31,7 @@
>  #include "coff/i386.h"
>  #include "coff/pe.h"
>  #include "libcoff.h"
> +#include "buildsym.h"
>  
>  /* The registers used to pass integer arguments during a function call.  */
>  static int amd64_windows_dummy_call_integer_regs[] =
> @@ -41,6 +42,14 @@ static int amd64_windows_dummy_call_inte
>    9                          /* %r9 */
>  };
>  
> +/* The registers used to pass floating point arguments in function call.
> */
> +static int amd64_windows_dummy_call_fp_regs[] =
> +{
> +  /* %xmm0 ... %xmm7 */
> +  AMD64_XMM0_REGNUM + 0, AMD64_XMM1_REGNUM,
> +  AMD64_XMM0_REGNUM + 2, AMD64_XMM0_REGNUM + 3,
> +};
> +
>  /* Implement the "classify" method in the gdbarch_tdep structure
>     for amd64-windows.  */
>  
> @@ -64,13 +73,45 @@ amd64_windows_classify (struct type *typ
>  	    || TYPE_LENGTH (type) == 4
>  	    || TYPE_LENGTH (type) == 8)
>  	  {
> -	    class[0] = AMD64_INTEGER;
> -	    class[1] = AMD64_NO_CLASS;
> +	    if (TYPE_NFIELDS (type) == 1)
> +	      {
> +		struct type *subtype = check_typedef (
> +					 TYPE_FIELD_TYPE (type, 0));
> +
> +		amd64_windows_classify (subtype, class);
> +	      }
> +	    else
> +	      {
> +		class[0] = AMD64_INTEGER;
> +		class[1] = AMD64_NO_CLASS;
> +	      }
>  	  }
>  	else
>  	  class[0] = class[1] = AMD64_MEMORY;
>  	break;
>  
> +      /* Pass all complex types in memory.  */
> +      case TYPE_CODE_COMPLEX:
> +	  if (TYPE_LENGTH (type) == 8)
> +	    {
> +	      class[0] = AMD64_INTEGER;
> +	      class[1] = AMD64_NO_CLASS;
> +	    }
> +	  else
> +	    class[0] = class[1] = AMD64_MEMORY;
> +	break;
> +
> +      case TYPE_CODE_FLT:
> +	/* For "long double" type,
> +           GNU C compiler still uses x87 coprocessor.
> +	   Parameters are passed by memory address.  */
> +	if ((TYPE_CODE (type) == TYPE_CODE_FLT)
> +	    && (processing_gcc_compilation != 0)
> +	    && ((TYPE_LENGTH(type) == 12) || (TYPE_LENGTH(type) == 16)))
> +	  {
> +	    class[0] = class[1] = AMD64_MEMORY;
> +	    break;
> +	  }
>        default:
>  	/* For all the other types, the conventions are the same as
>  	   with the System V ABI.  */
> @@ -94,6 +135,11 @@ amd64_windows_return_value (struct gdbar
>      {
>        case TYPE_CODE_FLT:
>        case TYPE_CODE_DECFLOAT:
> +	/* GNU C compiler still uses coprocessor by default.  */
> +	if ((TYPE_CODE (type) == TYPE_CODE_FLT)
> +	    && (processing_gcc_compilation != 0)
> +	    && ((len == 12) || (len == 16)))
> +	  break;
>          /* __m128, __m128i, __m128d, floats, and doubles are returned
>             via XMM0.  */
>          if (len == 4 || len == 8 || len == 16)
> @@ -979,9 +1025,13 @@ amd64_windows_init_abi (struct gdbarch_i
>    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->call_dummy_num_fp_regs =
> +    ARRAY_SIZE (amd64_windows_dummy_call_fp_regs);
> +  tdep->call_dummy_fp_regs = amd64_windows_dummy_call_fp_regs;
>    tdep->classify = amd64_windows_classify;
>    tdep->memory_args_by_pointer = 1;
>    tdep->integer_param_regs_saved_in_caller_frame = 1;
> +  tdep->integer_and_fp_regs_coupled = 1;
>    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