This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] gdb/riscv: Handle empty C++ structs during argument passing
- From: Andrew Burgess <andrew dot burgess at embecosm dot com>
- To: gdb-patches at sourceware dot org
- Cc: Jim Wilson <jimw at sifive dot com>, John Baldwin <jhb at freebsd dot org>, Palmer Dabbelt <palmer at sifive dot com>
- Date: Thu, 11 Apr 2019 23:39:49 +0100
- Subject: Re: [PATCH] gdb/riscv: Handle empty C++ structs during argument passing
- References: <20190405170423.26869-1-andrew.burgess@embecosm.com>
* 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
>