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

Joel Brobecker
2013-09-16T20:57:10Z

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