From: Alan Hayward <Alan.Hayward@arm.com>
To: Pedro Alves <palves@redhat.com>
Cc: "Maciej W. Rozycki" <macro@imgtec.com>,
Yao Qi <qiyaoltc@gmail.com>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
nd <nd@arm.com>
Subject: Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)
Date: Mon, 12 Jun 2017 09:09:00 -0000 [thread overview]
Message-ID: <F1DDCCAB-AF1B-49C0-80C9-5186650F46E2@arm.com> (raw)
In-Reply-To: <eedca2bb-3197-70e4-da20-ffefe2b38588@redhat.com>
> On 9 Jun 2017, at 15:29, Pedro Alves <palves@redhat.com> wrote:
>
> On 06/09/2017 02:23 PM, Maciej W. Rozycki wrote:
>
>> Hardcoding things, and especially with literals, causes a maintenance
>> pain when things change. Bad code elsewhere is not an excuse; besides,
>> the other places where `8' is hardcoded are not (buffer) limits, but just
>> handle specific register sizes, which are not going to change, and which
>> are a completely different matter.
>
> Perhaps you won't be surprised to learn that I subscribe to the same general
> principles. However, I believe that this isn't, in fact, a completely
> different matter, because the buffer in question is used to hold the
> contents of a structure's address, to either put in a general register,
> or in a stack slot, and those (addresses and general registers), unlike FPRs,
> aren't expected to grow for a given architecture.
>
>> So if you find `alloca' unacceptable,
>> then the limit has to be a macro, and an assertion check is also due (as
>> already proposed), preferably using `sizeof', so that a future change does
>> not break it by accident.
>
> I think a patch like the below is likely to clarify things.
> (Builds, but is otherwise untested).
>
I’d be happy with this patch. I like how the define refers just to the usage
of the register.
With the two minor changes below, it passes all my tests.
> From 5bdf1c939d72bf2718a542164d99bb396f0526e3 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 9 Jun 2017 15:10:50 +0100
> Subject: [PATCH] mips
>
> ---
> gdb/mips-tdep.c | 38 ++++++++++++++++++++++----------------
> 1 file changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 82f91ba..7844c73 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -271,6 +271,9 @@ mips_isa_regsize (struct gdbarch *gdbarch)
> / gdbarch_bfd_arch_info (gdbarch)->bits_per_byte);
> }
>
> +/* Max saved register size. */
> +#define MAX_MIPS_ABI_REGSIZE 8
> +
> /* Return the currently configured (or set) saved register size. */
>
> unsigned int
> @@ -4476,7 +4479,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> int stack_offset = 0;
> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> CORE_ADDR func_addr = find_function_addr (function, NULL);
> - int regsize = mips_abi_regsize (gdbarch);
> + int abi_regsize = mips_abi_regsize (gdbarch);
>
> /* For shared libraries, "t9" needs to point at the function
> address. */
> @@ -4499,7 +4502,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> than necessary for EABI, because the first few arguments are
> passed in registers, but that's OK. */
> for (argnum = 0; argnum < nargs; argnum++)
> - len += align_up (TYPE_LENGTH (value_type (args[argnum])), regsize);
> + len += align_up (TYPE_LENGTH (value_type (args[argnum])), abi_regsize);
> sp -= align_up (len, 16);
>
> if (mips_debug)
> @@ -4528,7 +4531,9 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> for (argnum = 0; argnum < nargs; argnum++)
> {
> const gdb_byte *val;
> - gdb_byte valbuf[MAX_REGISTER_SIZE];
> + /* This holds the address of structures that are passed by
> + reference. */
> + gdb_byte ref_valbuf[MAX_MIPS_ABI_REGISTER_SIZE];
Name of the define is MAX_MIPS_ABI_REGSIZE, not MAX_MIPS_ABI_REGISTER_SIZE.
> struct value *arg = args[argnum];
> struct type *arg_type = check_typedef (value_type (arg));
> int len = TYPE_LENGTH (arg_type);
> @@ -4541,13 +4546,14 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>
> /* The EABI passes structures that do not fit in a register by
> reference. */
> - if (len > regsize
> + if (len > abi_regsize
> && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION))
> {
> - store_unsigned_integer (valbuf, regsize, byte_order,
> + gdb_assert (ARRAY_SIZE (ref_valbuf) >= abi_regsize);
> + store_unsigned_integer (ref_valbuf, abi_regsize, byte_order,
> value_address (arg));
> typecode = TYPE_CODE_PTR;
> - len = regsize;
> + len = abi_regsize;
> val = valbuf;
This line needs changing from valbuf to ref_valbuf.
> if (mips_debug)
> fprintf_unfiltered (gdb_stdlog, " push");
> @@ -4560,7 +4566,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> up before the check to see if there are any FP registers
> left. Non MIPS_EABI targets also pass the FP in the integer
> registers so also round up normal registers. */
> - if (regsize < 8 && fp_register_arg_p (gdbarch, typecode, arg_type))
> + if (abi_regsize < 8 && fp_register_arg_p (gdbarch, typecode, arg_type))
> {
> if ((float_argreg & 1))
> float_argreg++;
> @@ -4626,12 +4632,12 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> /* Copy the argument to general registers or the stack in
> register-sized pieces. Large arguments are split between
> registers and stack. */
> - /* Note: structs whose size is not a multiple of regsize
> + /* Note: structs whose size is not a multiple of abi_regsize
> are treated specially: Irix cc passes
> them in registers where gcc sometimes puts them on the
> stack. For maximum compatibility, we will put them in
> both places. */
> - int odd_sized_struct = (len > regsize && len % regsize != 0);
> + int odd_sized_struct = (len > abi_regsize && len % abi_regsize != 0);
>
> /* Note: Floating-point values that didn't fit into an FP
> register are only written to memory. */
> @@ -4639,7 +4645,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> {
> /* Remember if the argument was written to the stack. */
> int stack_used_p = 0;
> - int partial_len = (len < regsize ? len : regsize);
> + int partial_len = (len < abi_regsize ? len : abi_regsize);
>
> if (mips_debug)
> fprintf_unfiltered (gdb_stdlog, " -- partial=%d",
> @@ -4657,15 +4663,15 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> stack_used_p = 1;
> if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> {
> - if (regsize == 8
> + if (abi_regsize == 8
> && (typecode == TYPE_CODE_INT
> || typecode == TYPE_CODE_PTR
> || typecode == TYPE_CODE_FLT) && len <= 4)
> - longword_offset = regsize - len;
> + longword_offset = abi_regsize - len;
> else if ((typecode == TYPE_CODE_STRUCT
> || typecode == TYPE_CODE_UNION)
> - && TYPE_LENGTH (arg_type) < regsize)
> - longword_offset = regsize - len;
> + && TYPE_LENGTH (arg_type) < abi_regsize)
> + longword_offset = abi_regsize - len;
> }
>
> if (mips_debug)
> @@ -4706,7 +4712,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> if (mips_debug)
> fprintf_filtered (gdb_stdlog, " - reg=%d val=%s",
> argreg,
> - phex (regval, regsize));
> + phex (regval, abi_regsize));
> regcache_cooked_write_signed (regcache, argreg, regval);
> argreg++;
> }
> @@ -4721,7 +4727,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> only needs to be adjusted when it has been used. */
>
> if (stack_used_p)
> - stack_offset += align_up (partial_len, regsize);
> + stack_offset += align_up (partial_len, abi_regsize);
> }
> }
> if (mips_debug)
> --
> 2.5.5
>
>
next prev parent reply other threads:[~2017-06-12 9:09 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-04 10:12 [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE Alan Hayward
2017-04-04 17:19 ` John Baldwin
2017-04-05 10:27 ` Alan Hayward
2017-04-11 15:37 ` Yao Qi
2017-05-05 8:03 ` [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (1/4) Alan Hayward
2017-05-05 21:44 ` Yao Qi
2017-05-05 8:04 ` [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4) Alan Hayward
2017-06-07 8:31 ` Alan Hayward
2017-06-08 20:27 ` Maciej W. Rozycki
2017-06-09 10:31 ` Alan Hayward
2017-06-09 11:03 ` Pedro Alves
2017-06-09 11:48 ` Maciej W. Rozycki
2017-06-09 12:05 ` Pedro Alves
2017-06-09 13:23 ` Maciej W. Rozycki
2017-06-09 14:29 ` Pedro Alves
2017-06-12 9:09 ` Alan Hayward [this message]
2017-06-12 18:11 ` Pedro Alves
2017-06-12 14:29 ` Maciej W. Rozycki
2017-05-05 8:04 ` [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4) Alan Hayward
2017-05-05 19:51 ` John Baldwin
2017-05-12 8:53 ` Yao Qi
2017-05-16 11:16 ` Alan Hayward
2017-05-22 12:07 ` Yao Qi
2017-05-22 16:05 ` Alan Hayward
2017-05-22 17:15 ` Pedro Alves
2017-05-23 17:49 ` Alan Hayward
2017-05-23 18:30 ` Pedro Alves
2017-05-24 9:08 ` Alan Hayward
2017-05-24 9:20 ` Pedro Alves
2017-05-24 10:20 ` Alan Hayward
2017-05-24 11:07 ` Pedro Alves
2017-05-24 19:45 ` Alan Hayward
2017-05-25 10:46 ` Pedro Alves
2017-05-25 11:43 ` Yao Qi
2017-05-25 11:48 ` Pedro Alves
2017-05-26 8:54 ` Alan Hayward
2017-05-26 10:26 ` Pedro Alves
2017-05-26 15:30 ` Alan Hayward
2017-05-26 15:49 ` Pedro Alves
2017-05-26 16:18 ` Alan Hayward
2017-05-26 16:00 ` John Baldwin
2017-05-24 9:29 ` Pedro Alves
2017-05-05 8:04 ` [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (3/4) Alan Hayward
2017-05-05 21:54 ` Yao Qi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=F1DDCCAB-AF1B-49C0-80C9-5186650F46E2@arm.com \
--to=alan.hayward@arm.com \
--cc=gdb-patches@sourceware.org \
--cc=macro@imgtec.com \
--cc=nd@arm.com \
--cc=palves@redhat.com \
--cc=qiyaoltc@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).