gdb passes and returns incorrect values when dealing with small array on Sparc
Commit Message
From: Vladimir Mezentsev <vladimir.mezentsev@oracle.com>
gdb has a special type (TYPE_CODE_ARRAY) to support the gcc extension
(https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html).
TYPE_CODE_ARRAY is handled incorrectly for both (32- and 64-bit) modes on Sparc machines.
Tested on sparc64-linux-gnu and sparc-solaris (32- and 64-bit mode).
No regressions.
2017-03-27 Vladimir Mezentsev <vladimir.mezentsev@oracle.com>
* gdb/sparc-tdep.c (sparc_structure_return_p): New function.
* gdb/sparc-tdep.c (sparc_arg_on_registers_p): Likewise.
* gdb/sparc-tdep.c (sparc32_store_arguments): Use the new functions.
* gdb/sparc64-tdep.c: (sparc64_16_byte_align_p): Handle TYPE_CODE_ARRAY
* gdb/sparc64-tdep.c: (sparc64_store_floating_fields): Likewise
* gdb/sparc64-tdep.c: (sparc64_extract_floating_fields): Likewise
---
gdb/sparc-tdep.c | 68 +++++++++++++++++++++++++++++++++++++++-------------
gdb/sparc64-tdep.c | 42 ++++++++++++++++++++++++++++++-
2 files changed, 91 insertions(+), 19 deletions(-)
Comments
Hi Vladimir,
Sorry for the delay. I think everyone was hoping that someone
that actually groks SPARC stuff would take a look at this.
I don't think we have any maintainer currently actively caring for
SPARC in particular. Maybe you guys (Oracle folks) could help
review each others' patches? I think that'd help get these
patches in quicker.
I'll comment below on formatting / convention / procedural
stuff, and assume the patch's actual substance is correct.
On 03/29/2017 05:17 PM, vladimir.mezentsev@oracle.com wrote:
> From: Vladimir Mezentsev <vladimir.mezentsev@oracle.com>
>
> gdb has a special type (TYPE_CODE_ARRAY) to support the gcc extension
> (https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html).
> TYPE_CODE_ARRAY is handled incorrectly for both (32- and 64-bit) modes on Sparc machines.
>
> Tested on sparc64-linux-gnu and sparc-solaris (32- and 64-bit mode).
> No regressions.
Note there should be no leading-tab indentation in the body of the
commit log. Please make sure this is reindented on the left.
>
> 2017-03-27 Vladimir Mezentsev <vladimir.mezentsev@oracle.com>
Two spaces before and after your name.
>
> * gdb/sparc-tdep.c (sparc_structure_return_p): New function.
> * gdb/sparc-tdep.c (sparc_arg_on_registers_p): Likewise.
> * gdb/sparc-tdep.c (sparc32_store_arguments): Use the new functions.
> * gdb/sparc64-tdep.c: (sparc64_16_byte_align_p): Handle TYPE_CODE_ARRAY
> * gdb/sparc64-tdep.c: (sparc64_store_floating_fields): Likewise
> * gdb/sparc64-tdep.c: (sparc64_extract_floating_fields): Likewise
Entries are relative to the closest ChangeLog file, which is gdb/ChangeLog
in this case. File names should be not repeated, and there should be a period
after "Likewise" (though you merge those entries). There should be no ":" between
the file name and function name. Here's how I'd fix it:
gdb/ChangeLog:
2017-03-27 Vladimir Mezentsev <vladimir.mezentsev@oracle.com>
* sparc-tdep.c (sparc_structure_return_p)
(sparc_arg_on_registers_p): New functions.
(sparc32_store_arguments): Use them.
* sparc64-tdep.c (sparc64_16_byte_align_p)
(sparc64_store_floating_fields, sparc64_extract_floating_fields):
Handle TYPE_CODE_ARRAY.
You can find these details and more at
<https://sourceware.org/gdb/wiki/ContributionChecklist>.
> ---
> gdb/sparc-tdep.c | 68 +++++++++++++++++++++++++++++++++++++++-------------
> gdb/sparc64-tdep.c | 42 ++++++++++++++++++++++++++++++-
> 2 files changed, 91 insertions(+), 19 deletions(-)
>
> diff --git a/gdb/sparc-tdep.c b/gdb/sparc-tdep.c
> index d346aec..07808f3 100644
> --- a/gdb/sparc-tdep.c
> +++ b/gdb/sparc-tdep.c
> @@ -297,6 +297,39 @@ sparc_structure_or_union_p (const struct type *type)
> return 0;
> }
>
> +static int
> +sparc_structure_return_p (const struct type *type)
All functions should have an intro comment.
While at it, please let's use bool/true/false in new code.
> +{
> + if ((TYPE_CODE (type) == TYPE_CODE_ARRAY) && (TYPE_LENGTH (type) <= 8))
GDB/GNU's coding standard avoids redundant parenthesis:
if (TYPE_CODE (type) == TYPE_CODE_ARRAY && TYPE_LENGTH (type) <= 8)
etc. throughout the patch.
> + {
> + struct type *t = check_typedef (TYPE_TARGET_TYPE (type));
> +
> + if (sparc_floating_p (t) && (TYPE_LENGTH (t) == 8))
... here ...
> + return 1;
> + return 0;
> + }
> + if (sparc_floating_p (type) && (TYPE_LENGTH (type) == 16))
... here ...
> + return 1;
> + return sparc_structure_or_union_p (type);
> +}
> +
> +static int
> +sparc_arg_on_registers_p (const struct type *type)
> +{
> + if ((TYPE_CODE (type) == TYPE_CODE_ARRAY) && (TYPE_LENGTH (type) <= 8))
... here ...
> + {
> + struct type *t = check_typedef (TYPE_TARGET_TYPE (type));
> +
> + if (sparc_floating_p (t) && (TYPE_LENGTH (t) == 8))
... here ...
> + return 0;
> + return 1;
> + }
> + if (sparc_structure_or_union_p (type) || sparc_complex_floating_p (type)
> + || (sparc_floating_p (type) && (TYPE_LENGTH (type) == 16)))
... here ...
> + return 0;
> + return 1;
> +}
> +
> /* Register information. */
> #define SPARC32_FPU_REGISTERS \
> "f0", "f1", "f2", "f3", "f4", "f5", "f6", "f7", \
> @@ -569,9 +602,7 @@ sparc32_store_arguments (struct regcache *regcache, int nargs,
> struct type *type = value_type (args[i]);
> int len = TYPE_LENGTH (type);
>
> - if (sparc_structure_or_union_p (type)
> - || (sparc_floating_p (type) && len == 16)
> - || sparc_complex_floating_p (type))
> + if (!sparc_arg_on_registers_p (type))
> {
> /* Structure, Union and Quad-Precision Arguments. */
> sp -= len;
> @@ -593,11 +624,8 @@ sparc32_store_arguments (struct regcache *regcache, int nargs,
> else
> {
> /* Integral and pointer arguments. */
> - gdb_assert (sparc_integral_or_pointer_p (type));
> -
> - if (len < 4)
> - args[i] = value_cast (builtin_type (gdbarch)->builtin_int32,
> - args[i]);
> + gdb_assert (sparc_integral_or_pointer_p (type) ||
|| goes on the next line.
> + ((TYPE_CODE (type) == TYPE_CODE_ARRAY) && (len <= 8)));
Redundant parens.
> num_elements += ((len + 3) / 4);
> }
> }
> @@ -619,6 +647,15 @@ sparc32_store_arguments (struct regcache *regcache, int nargs,
> const bfd_byte *valbuf = value_contents (args[i]);
> struct type *type = value_type (args[i]);
> int len = TYPE_LENGTH (type);
> + gdb_byte buf[4];
> +
> + if (len < 4)
> + {
> + memset (buf, 0, 4 - len);
> + memcpy (buf + 4 - len, valbuf, len);
> + valbuf = buf;
> + len = 4;
> + }
>
> gdb_assert (len == 4 || len == 8);
>
> @@ -1344,10 +1381,10 @@ sparc32_extract_return_value (struct type *type, struct regcache *regcache,
> int len = TYPE_LENGTH (type);
> gdb_byte buf[32];
>
> - gdb_assert (!sparc_structure_or_union_p (type));
> - gdb_assert (!(sparc_floating_p (type) && len == 16));
> + gdb_assert (!sparc_structure_return_p (type));
>
> - if (sparc_floating_p (type) || sparc_complex_floating_p (type))
> + if (sparc_floating_p (type) || sparc_complex_floating_p (type) ||
> + (TYPE_CODE (type) == TYPE_CODE_ARRAY))
Redundant parens. "||" goes on next line. When we have several conditions
that don't fit on one line, I think it reads better to then put each
on its own line:
if (sparc_floating_p (type)
|| sparc_complex_floating_p (type)
|| TYPE_CODE (type) == TYPE_CODE_ARRAY)
> {
> /* Floating return values. */
> regcache_cooked_read (regcache, SPARC_F0_REGNUM, buf);
> @@ -1396,11 +1433,9 @@ sparc32_store_return_value (struct type *type, struct regcache *regcache,
> const gdb_byte *valbuf)
> {
> int len = TYPE_LENGTH (type);
> - gdb_byte buf[8];
> + gdb_byte buf[32];
Is it obvious for someone looking at this function what this
"32" is and whether it is big enough?
>
> - gdb_assert (!sparc_structure_or_union_p (type));
> - gdb_assert (!(sparc_floating_p (type) && len == 16));
> - gdb_assert (len <= 8);
> + gdb_assert (!sparc_structure_return_p (type));
>
> if (sparc_floating_p (type) || sparc_complex_floating_p (type))
> {
> @@ -1456,8 +1491,7 @@ sparc32_return_value (struct gdbarch *gdbarch, struct value *function,
> guarantees that we can always find the return value, not just
> before the function returns. */
>
> - if (sparc_structure_or_union_p (type)
> - || (sparc_floating_p (type) && TYPE_LENGTH (type) == 16))
> + if (sparc_structure_return_p (type))
> {
> ULONGEST sp;
> CORE_ADDR addr;
> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
> index bf0da18..0b1a0ee 100644
> --- a/gdb/sparc64-tdep.c
> +++ b/gdb/sparc64-tdep.c
> @@ -669,6 +669,12 @@ static const struct frame_base sparc64_frame_base =
> static int
> sparc64_16_byte_align_p (struct type *type)
> {
> + if ((TYPE_CODE (type) == TYPE_CODE_ARRAY)) {
Redundant parens.
{ does on the next line.
> + struct type *t = check_typedef (TYPE_TARGET_TYPE (type));
> +
> + if (sparc64_floating_p (t))
> + return 1;
> + }
> if (sparc64_floating_p (type) && TYPE_LENGTH (type) == 16)
> return 1;
>
> @@ -703,7 +709,23 @@ sparc64_store_floating_fields (struct regcache *regcache, struct type *type,
>
> gdb_assert (element < 16);
>
> - if (sparc64_floating_p (type)
> + if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
> + {
> + gdb_byte buf[8];
> + int regnum = SPARC_F0_REGNUM + element * 2 + bitpos / 32;
> +
> + valbuf += (bitpos / 8);
Redundant parens.
> + if (len < 8)
> + {
> + memset (buf, 0, 8 - len);
> + memcpy (buf + 8 - len, valbuf, len);
> + valbuf = buf;
> + len = 8;
> + }
> + for (int n = 0; n < ((len + 3) / 4); n++)
Ditto.
> + regcache_cooked_write (regcache, regnum + n, valbuf + n * 4);
> + }
> + else if (sparc64_floating_p (type)
> || (sparc64_complex_floating_p (type) && len <= 16))
Ditto.
> {
> int regnum;
> @@ -776,7 +798,23 @@ sparc64_extract_floating_fields (struct regcache *regcache, struct type *type,
> {
> struct gdbarch *gdbarch = get_regcache_arch (regcache);
>
> - if (sparc64_floating_p (type))
> + if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
> + {
> + int len = TYPE_LENGTH (type);
> + int regnum = SPARC_F0_REGNUM + (bitpos / 32);
Spurious space after " = ". Redundant parens.
> +
> + valbuf += (bitpos / 8);
> + if (len < 4)
> + {
> + gdb_byte buf[4];
> + regcache_cooked_read (regcache, regnum, buf);
> + memcpy (valbuf, buf + 4 - len, len);
> + }
> + else
> + for (int i = 0; i < ((len + 3) / 4); i++)
> + regcache_cooked_read (regcache, regnum + i, valbuf + i * 4);
Redundant parens multiple times.
> + }
> + else if (sparc64_floating_p (type))
> {
> int len = TYPE_LENGTH (type);
> int regnum;
>
Please resubmit a v2 patch with these issues addressed.
Thanks,
Pedro Alves
Hi Pedro.
Sorry for the delay. I think everyone was hoping that someone
that actually groks SPARC stuff would take a look at this.
I don't think we have any maintainer currently actively caring for
SPARC in particular. Maybe you guys (Oracle folks) could help
review each others' patches? I think that'd help get these
patches in quicker.
We (the Oracle toolchain team) actually review the GDB patches
internally from a functional perspective before they are sent upstream.
We can make it more explicit in our future submissions with a note in
the patch submission.
Hi Jose,
On 04/18/2017 10:28 PM, Jose E. Marchesi wrote:
>
> Hi Pedro.
>
> Sorry for the delay. I think everyone was hoping that someone
> that actually groks SPARC stuff would take a look at this.
> I don't think we have any maintainer currently actively caring for
> SPARC in particular. Maybe you guys (Oracle folks) could help
> review each others' patches? I think that'd help get these
> patches in quicker.
>
> We (the Oracle toolchain team) actually review the GDB patches
> internally from a functional perspective before they are sent upstream.
>
> We can make it more explicit in our future submissions with a note in
> the patch submission.
Internal review is of course fine and appreciated, but I still think
it better to do at least some of it in the open. With a bit of visible
involvement, it'll be natural for one (or more) of you guys to
end up being gdb/SPARC maintainer. (As you know, maintainership
status is individual, not through affiliation.)
Thanks,
Pedro Alves
> We (the Oracle toolchain team) actually review the GDB patches
> internally from a functional perspective before they are sent upstream.
>
> We can make it more explicit in our future submissions with a note in
> the patch submission.
Internal review is of course fine and appreciated, but I still think
it better to do at least some of it in the open.
Yeah, I agree: the more done in the open the better.
@@ -297,6 +297,39 @@ sparc_structure_or_union_p (const struct type *type)
return 0;
}
+static int
+sparc_structure_return_p (const struct type *type)
+{
+ if ((TYPE_CODE (type) == TYPE_CODE_ARRAY) && (TYPE_LENGTH (type) <= 8))
+ {
+ struct type *t = check_typedef (TYPE_TARGET_TYPE (type));
+
+ if (sparc_floating_p (t) && (TYPE_LENGTH (t) == 8))
+ return 1;
+ return 0;
+ }
+ if (sparc_floating_p (type) && (TYPE_LENGTH (type) == 16))
+ return 1;
+ return sparc_structure_or_union_p (type);
+}
+
+static int
+sparc_arg_on_registers_p (const struct type *type)
+{
+ if ((TYPE_CODE (type) == TYPE_CODE_ARRAY) && (TYPE_LENGTH (type) <= 8))
+ {
+ struct type *t = check_typedef (TYPE_TARGET_TYPE (type));
+
+ if (sparc_floating_p (t) && (TYPE_LENGTH (t) == 8))
+ return 0;
+ return 1;
+ }
+ if (sparc_structure_or_union_p (type) || sparc_complex_floating_p (type)
+ || (sparc_floating_p (type) && (TYPE_LENGTH (type) == 16)))
+ return 0;
+ return 1;
+}
+
/* Register information. */
#define SPARC32_FPU_REGISTERS \
"f0", "f1", "f2", "f3", "f4", "f5", "f6", "f7", \
@@ -569,9 +602,7 @@ sparc32_store_arguments (struct regcache *regcache, int nargs,
struct type *type = value_type (args[i]);
int len = TYPE_LENGTH (type);
- if (sparc_structure_or_union_p (type)
- || (sparc_floating_p (type) && len == 16)
- || sparc_complex_floating_p (type))
+ if (!sparc_arg_on_registers_p (type))
{
/* Structure, Union and Quad-Precision Arguments. */
sp -= len;
@@ -593,11 +624,8 @@ sparc32_store_arguments (struct regcache *regcache, int nargs,
else
{
/* Integral and pointer arguments. */
- gdb_assert (sparc_integral_or_pointer_p (type));
-
- if (len < 4)
- args[i] = value_cast (builtin_type (gdbarch)->builtin_int32,
- args[i]);
+ gdb_assert (sparc_integral_or_pointer_p (type) ||
+ ((TYPE_CODE (type) == TYPE_CODE_ARRAY) && (len <= 8)));
num_elements += ((len + 3) / 4);
}
}
@@ -619,6 +647,15 @@ sparc32_store_arguments (struct regcache *regcache, int nargs,
const bfd_byte *valbuf = value_contents (args[i]);
struct type *type = value_type (args[i]);
int len = TYPE_LENGTH (type);
+ gdb_byte buf[4];
+
+ if (len < 4)
+ {
+ memset (buf, 0, 4 - len);
+ memcpy (buf + 4 - len, valbuf, len);
+ valbuf = buf;
+ len = 4;
+ }
gdb_assert (len == 4 || len == 8);
@@ -1344,10 +1381,10 @@ sparc32_extract_return_value (struct type *type, struct regcache *regcache,
int len = TYPE_LENGTH (type);
gdb_byte buf[32];
- gdb_assert (!sparc_structure_or_union_p (type));
- gdb_assert (!(sparc_floating_p (type) && len == 16));
+ gdb_assert (!sparc_structure_return_p (type));
- if (sparc_floating_p (type) || sparc_complex_floating_p (type))
+ if (sparc_floating_p (type) || sparc_complex_floating_p (type) ||
+ (TYPE_CODE (type) == TYPE_CODE_ARRAY))
{
/* Floating return values. */
regcache_cooked_read (regcache, SPARC_F0_REGNUM, buf);
@@ -1396,11 +1433,9 @@ sparc32_store_return_value (struct type *type, struct regcache *regcache,
const gdb_byte *valbuf)
{
int len = TYPE_LENGTH (type);
- gdb_byte buf[8];
+ gdb_byte buf[32];
- gdb_assert (!sparc_structure_or_union_p (type));
- gdb_assert (!(sparc_floating_p (type) && len == 16));
- gdb_assert (len <= 8);
+ gdb_assert (!sparc_structure_return_p (type));
if (sparc_floating_p (type) || sparc_complex_floating_p (type))
{
@@ -1456,8 +1491,7 @@ sparc32_return_value (struct gdbarch *gdbarch, struct value *function,
guarantees that we can always find the return value, not just
before the function returns. */
- if (sparc_structure_or_union_p (type)
- || (sparc_floating_p (type) && TYPE_LENGTH (type) == 16))
+ if (sparc_structure_return_p (type))
{
ULONGEST sp;
CORE_ADDR addr;
@@ -669,6 +669,12 @@ static const struct frame_base sparc64_frame_base =
static int
sparc64_16_byte_align_p (struct type *type)
{
+ if ((TYPE_CODE (type) == TYPE_CODE_ARRAY)) {
+ struct type *t = check_typedef (TYPE_TARGET_TYPE (type));
+
+ if (sparc64_floating_p (t))
+ return 1;
+ }
if (sparc64_floating_p (type) && TYPE_LENGTH (type) == 16)
return 1;
@@ -703,7 +709,23 @@ sparc64_store_floating_fields (struct regcache *regcache, struct type *type,
gdb_assert (element < 16);
- if (sparc64_floating_p (type)
+ if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
+ {
+ gdb_byte buf[8];
+ int regnum = SPARC_F0_REGNUM + element * 2 + bitpos / 32;
+
+ valbuf += (bitpos / 8);
+ if (len < 8)
+ {
+ memset (buf, 0, 8 - len);
+ memcpy (buf + 8 - len, valbuf, len);
+ valbuf = buf;
+ len = 8;
+ }
+ for (int n = 0; n < ((len + 3) / 4); n++)
+ regcache_cooked_write (regcache, regnum + n, valbuf + n * 4);
+ }
+ else if (sparc64_floating_p (type)
|| (sparc64_complex_floating_p (type) && len <= 16))
{
int regnum;
@@ -776,7 +798,23 @@ sparc64_extract_floating_fields (struct regcache *regcache, struct type *type,
{
struct gdbarch *gdbarch = get_regcache_arch (regcache);
- if (sparc64_floating_p (type))
+ if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
+ {
+ int len = TYPE_LENGTH (type);
+ int regnum = SPARC_F0_REGNUM + (bitpos / 32);
+
+ valbuf += (bitpos / 8);
+ if (len < 4)
+ {
+ gdb_byte buf[4];
+ regcache_cooked_read (regcache, regnum, buf);
+ memcpy (valbuf, buf + 4 - len, len);
+ }
+ else
+ for (int i = 0; i < ((len + 3) / 4); i++)
+ regcache_cooked_read (regcache, regnum + i, valbuf + i * 4);
+ }
+ else if (sparc64_floating_p (type))
{
int len = TYPE_LENGTH (type);
int regnum;