This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c
- From: Alan Hayward <Alan dot Hayward at arm dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, nd <nd at arm dot com>
- Date: Fri, 24 Mar 2017 14:49:11 +0000
- Subject: Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c
- Authentication-results: sourceware.org; auth=none
- Authentication-results: gmail.com; dkim=none (message not signed) header.d=none;gmail.com; dmarc=none action=none header.from=arm.com;
- Nodisclaimer: True
- References: <E80FFABA-2912-4223-AC55-2F4DE6055F47@arm.com> <86lgspqisk.fsf@gmail.com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
Patch update as per comments below.
Tested using make check with board files unix and native-gdbserver.
Ok to commit?
I'm happy with Yao's patch below too.
Alan.
2017-03-24 Alan Hayward <alan.hayward@arm.com>
* frame.c (get_frame_register_bytes): Unwind using value.
(put_frame_register_bytes): Likewise.
diff --git a/gdb/frame.c b/gdb/frame.c
index d98003dee7c34a63bd25356e6674721664a4b2f3..05a3be400c35ada0de48fd529e9cc83e0eaa941d 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1410,16 +1410,21 @@ get_frame_register_bytes (struct frame_info *frame, int regnum,
}
else
{
- gdb_byte buf[MAX_REGISTER_SIZE];
- enum lval_type lval;
- CORE_ADDR addr;
- int realnum;
+ struct value *value = frame_unwind_register_value (frame->next,
+ regnum);
+ gdb_assert (value != NULL);
+ *optimizedp = value_optimized_out (value);
+ *unavailablep = !value_entirely_available (value);
- frame_register (frame, regnum, optimizedp, unavailablep,
- &lval, &addr, &realnum, buf);
if (*optimizedp || *unavailablep)
- return 0;
- memcpy (myaddr, buf + offset, curr_len);
+ {
+ release_value (value);
+ value_free (value);
+ return 0;
+ }
+ memcpy (myaddr, value_contents_all (value) + offset, curr_len);
+ release_value (value);
+ value_free (value);
}
myaddr += curr_len;
@@ -1460,11 +1465,15 @@ put_frame_register_bytes (struct frame_info *frame, int regnum,
}
else
{
- gdb_byte buf[MAX_REGISTER_SIZE];
-
- deprecated_frame_register_read (frame, regnum, buf);
- memcpy (buf + offset, myaddr, curr_len);
- put_frame_register (frame, regnum, buf);
+ struct value *value = frame_unwind_register_value (frame->next,
+ regnum);
+ gdb_assert (value != NULL);
+
+ memcpy ((char *) value_contents_writeable (value) + offset, myaddr,
+ curr_len);
+ put_frame_register (frame, regnum, value_contents_raw (value));
+ release_value (value);
+ value_free (value);
}
myaddr += curr_len;
> On 1 Mar 2017, at 12:32, Yao Qi <qiyaoltc@gmail.com> wrote:
>
> Alan Hayward <Alan.Hayward@arm.com> writes:
>
>> @@ -1252,7 +1252,11 @@ frame_unwind_register_signed (struct frame_info *frame, int regnum)
>> struct gdbarch *gdbarch = frame_unwind_arch (frame);
>> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> int size = register_size (gdbarch, regnum);
>> - gdb_byte buf[MAX_REGISTER_SIZE];
>> + gdb_byte buf[sizeof (LONGEST)];
>> +
>> + if (size > (int) sizeof (LONGEST))
>> + error (_("Cannot unwind integers more than %d bytes."),
>> + (int) sizeof (LONGEST));
>>
>
> We apply the restriction of extract_signed_integer to its caller here.
> People will wonder why do we have such check until he/she digs into
> extract_signed_integer. My first reaction is to add some comments to
> explain why do we do so, but the recent gdb::function_view reminds me
> that we can do something differently (and better, IMO).
>
> Current pattern of using extract_unsigned_integer is
>
> 1) allocate an array on stack,
> 2) read data from regcache or frame into the array,
> 3) pass the array to extract_unsigned_integer
>
> we can pass a callable function_view as a content provider to
> extract_unsigned_integer, so that we don't need step 1). The code
> becomes,
>
> return extract_unsigned_integer ([&] (gdb_byte *buf, size_t size)
> {
> frame_unwind_register (frame, regnum, buf);
> }, size, byte_order);
>
> We can remove some uses of MAX_REGISTER_SIZE in this way. Do you (Alan
> and others) like the patch below?
>
>> @@ -1460,11 +1473,15 @@ put_frame_register_bytes (struct frame_info *frame, int regnum,
>> }
>> else
>> {
>> - gdb_byte buf[MAX_REGISTER_SIZE];
>> -
>> - deprecated_frame_register_read (frame, regnum, buf);
>> - memcpy (buf + offset, myaddr, curr_len);
>> - put_frame_register (frame, regnum, buf);
>> + struct value *value = frame_unwind_register_value (frame->next,
>> + regnum);
>> + gdb_assert (value != NULL);
>> +
>> + memcpy ((char *) value_contents_all (value) + offset, myaddr,
>> + curr_len);
>
> Use value_contents_writeable,
>
>> + put_frame_register (frame, regnum, value_contents_all (value));
>
> s/value_contents_all/value_contents_raw/
>
> because value_contents_all requires value is available but
> deprecated_frame_register_read doesn't.
>
>> + release_value (value);
>> + value_free (value);
>
> --
> Yao (齐尧)
> From 2a76b39ffa87ed015f3637ee4a9d083f682863a0 Mon Sep 17 00:00:00 2001
> From: Yao Qi <yao.qi@linaro.org>
> Date: Wed, 1 Mar 2017 10:43:29 +0000
> Subject: [PATCH] Use content provider in extract_unsigned_integer
>
> ---
> gdb/ada-lang.c | 9 ++++++---
> gdb/defs.h | 4 ++++
> gdb/findvar.c | 38 ++++++++++++++++++++++++++++++--------
> gdb/frame.c | 7 ++++---
> gdb/ia64-tdep.c | 19 +++++++++++++++----
> 5 files changed, 59 insertions(+), 18 deletions(-)
>
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 753409c..07ce04a 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -4555,12 +4555,15 @@ value_pointer (struct value *value, struct type *type)
> {
> struct gdbarch *gdbarch = get_type_arch (type);
> unsigned len = TYPE_LENGTH (type);
> - gdb_byte *buf = (gdb_byte *) alloca (len);
> CORE_ADDR addr;
>
> addr = value_address (value);
> - gdbarch_address_to_pointer (gdbarch, type, buf, addr);
> - addr = extract_unsigned_integer (buf, len, gdbarch_byte_order (gdbarch));
> + addr = extract_unsigned_integer ([&] (gdb_byte *buf, size_t size)
> + {
> + gdbarch_address_to_pointer (gdbarch,
> + type, buf,
> + addr);
> + }, len, gdbarch_byte_order (gdbarch));
> return addr;
> }
>
> diff --git a/gdb/defs.h b/gdb/defs.h
> index aa58605..a2f8fce 100644
> --- a/gdb/defs.h
> +++ b/gdb/defs.h
> @@ -628,6 +628,10 @@ extern LONGEST extract_signed_integer (const gdb_byte *, int,
> extern ULONGEST extract_unsigned_integer (const gdb_byte *, int,
> enum bfd_endian);
>
> +extern ULONGEST
> + extract_unsigned_integer (gdb::function_view<void (gdb_byte *, size_t size)> content_provider,
> + int len, enum bfd_endian byte_order);
> +
> extern int extract_long_unsigned_integer (const gdb_byte *, int,
> enum bfd_endian, LONGEST *);
>
> diff --git a/gdb/findvar.c b/gdb/findvar.c
> index 80c709a..6d7d0de 100644
> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -81,20 +81,15 @@ That operation is not available on integers of more than %d bytes."),
> return retval;
> }
>
> -ULONGEST
> -extract_unsigned_integer (const gdb_byte *addr, int len,
> - enum bfd_endian byte_order)
> +static ULONGEST
> +extract_unsigned_integer_1 (const gdb_byte *addr, int len,
> + enum bfd_endian byte_order)
> {
> ULONGEST retval;
> const unsigned char *p;
> const unsigned char *startaddr = addr;
> const unsigned char *endaddr = startaddr + len;
>
> - if (len > (int) sizeof (ULONGEST))
> - error (_("\
> -That operation is not available on integers of more than %d bytes."),
> - (int) sizeof (ULONGEST));
> -
> /* Start at the most significant end of the integer, and work towards
> the least significant. */
> retval = 0;
> @@ -111,6 +106,33 @@ That operation is not available on integers of more than %d bytes."),
> return retval;
> }
>
> +ULONGEST
> +extract_unsigned_integer (const gdb_byte *addr, int len,
> + enum bfd_endian byte_order)
> +{
> + if (len > (int) sizeof (ULONGEST))
> + error (_("\
> +That operation is not available on integers of more than %d bytes."),
> + (int) sizeof (ULONGEST));
> +
> + return extract_unsigned_integer_1 (addr, len, byte_order);
> +}
> +
> +ULONGEST
> +extract_unsigned_integer (gdb::function_view<void (gdb_byte *, size_t size)> content_provider,
> + int len, enum bfd_endian byte_order)
> +{
> + if (len > (int) sizeof (ULONGEST))
> + error (_("\
> +That operation is not available on integers of more than %d bytes."),
> + (int) sizeof (ULONGEST));
> +
> + gdb_byte buf[sizeof (ULONGEST)];
> +
> + content_provider (buf, len);
> + return extract_unsigned_integer_1 (buf, len, byte_order);
> +}
> +
> /* Sometimes a long long unsigned integer can be extracted as a
> LONGEST value. This is done so that we can print these values
> better. If this integer can be converted to a LONGEST, this
> diff --git a/gdb/frame.c b/gdb/frame.c
> index d98003d..57f82f1 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1270,10 +1270,11 @@ frame_unwind_register_unsigned (struct frame_info *frame, int regnum)
> struct gdbarch *gdbarch = frame_unwind_arch (frame);
> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> int size = register_size (gdbarch, regnum);
> - gdb_byte buf[MAX_REGISTER_SIZE];
>
> - frame_unwind_register (frame, regnum, buf);
> - return extract_unsigned_integer (buf, size, byte_order);
> + return extract_unsigned_integer ([&] (gdb_byte *buf, size_t size)
> + {
> + frame_unwind_register (frame, regnum, buf);
> + }, size, byte_order);
> }
>
> ULONGEST
> diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
> index 4c53bc6..2748dac 100644
> --- a/gdb/ia64-tdep.c
> +++ b/gdb/ia64-tdep.c
> @@ -1516,7 +1516,6 @@ examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc,
> else if (qp == 0 && rN == 2
> && ((rM == fp_reg && fp_reg != 0) || rM == 12))
> {
> - gdb_byte buf[MAX_REGISTER_SIZE];
> CORE_ADDR saved_sp = 0;
> /* adds r2, spilloffset, rFramePointer
> or
> @@ -1534,8 +1533,15 @@ examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc,
> {
> struct gdbarch *gdbarch = get_frame_arch (this_frame);
> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> - get_frame_register (this_frame, sp_regnum, buf);
> - saved_sp = extract_unsigned_integer (buf, 8, byte_order);
> +
> + saved_sp
> + = extract_unsigned_integer ([&] (gdb_byte * buf,
> + size_t size)
> + {
> + get_frame_register (this_frame,
> + sp_regnum,
> + buf);
> + }, 8, byte_order);
> }
> spill_addr = saved_sp
> + (rM == 12 ? 0 : mem_stack_frame_size)
> @@ -1782,7 +1788,12 @@ examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc,
> else if (cfm_reg != 0)
> {
> get_frame_register (this_frame, cfm_reg, buf);
> - cfm = extract_unsigned_integer (buf, 8, byte_order);
> + cfm = extract_unsigned_integer ([&] (gdb_byte * buf, size_t size)
> + {
> + get_frame_register (this_frame,
> + cfm_reg,
> + buf);
> + }, 8, byte_order);
> }
> cache->prev_cfm = cfm;
>
> --
> 1.9.1