[PATCH 2/2] GDB: Use gdb::array_view for buffers used in register reading and unwinding
Thiago Jung Bauermann
thiago.bauermann@linaro.org
Fri Jan 10 22:28:00 GMT 2025
Simon Marchi <simark@simark.ca> writes:
> On 2025-01-10 11:44, Thiago Jung Bauermann wrote:
>> diff --git a/gdb/frame.c b/gdb/frame.c
>> index ba4a07179f64..b64f9067ab9e 100644
>> --- a/gdb/frame.c
>> +++ b/gdb/frame.c
>> @@ -1112,7 +1112,7 @@ frame_save_as_regcache (const frame_info_ptr &this_frame)
>> {
>> auto cooked_read = [this_frame] (int regnum, gdb::array_view<gdb_byte> buf)
>> {
>> - if (!deprecated_frame_register_read (this_frame, regnum, buf.data ()))
>> + if (!deprecated_frame_register_read (this_frame, regnum, buf))
>> return REG_UNAVAILABLE;
>> else
>> return REG_VALID;
>> @@ -1177,7 +1177,8 @@ void
>> frame_register_unwind (const frame_info_ptr &next_frame, int regnum,
>> int *optimizedp, int *unavailablep,
>> enum lval_type *lvalp, CORE_ADDR *addrp,
>> - int *realnump, gdb_byte *bufferp)
>> + int *realnump,
>> + gdb::array_view<gdb_byte> buffer)
>> {
>> struct value *value;
>>
>
> There are some comments referring to `bufferp` that would need to be
> updated. IMO the line:
>
> /* gdb_assert (bufferp != NULL); */
>
> ... can just be removed.
Ah, indeed. I don't know how I missed them. Fixed.
>> @@ -1202,13 +1203,15 @@ frame_register_unwind (const frame_info_ptr &next_frame, int regnum,
>> else
>> *realnump = -1;
>>
>> - if (bufferp)
>> + if (!buffer.empty ())
>> {
>> + gdb_assert (buffer.size () == value->type ()->length ());
>
> Regarding this, I think I'm going to backtrack. I started to survey the
> callers (and the callers of callers) of this function, and found one
> case where this doesn't hold, that looks legitimate:
>
> - frame_register_unwind, called by
> - frame_unwind_register, called by
> - i386_unwind_pc
>
> i386_unwind_pc has an 8 bytes static array and asks to unwind the PC
> register. This function is used for both amd64 (8 bytes PC) and i386 (4
> bytes PC). This function would now have to create a view of the right
> size, something like:
>
> static CORE_ADDR
> i386_unwind_pc (struct gdbarch *gdbarch, const frame_info_ptr &next_frame)
> {
> gdb_byte buf[8];
> auto view = gdb::make_array_view
> (buf, builtin_type (gdbarch)->builtin_func_ptr->length ());
>
> frame_unwind_register (next_frame, gdbarch_pc_regnum (gdbarch), view);
> return extract_typed_address (view.data (),
> builtin_type (gdbarch)->builtin_func_ptr);
> }
>
> Without doing changes like this all over the place, things will break.
> I'm not saying it can't be done, but it would require a lot of effort
> and careful review (and testing when possible).
>
> So, I think it would be safer for now to revert to what you had before,
> assert that the buffer is large enough for the value.
That's true. Thank you for digging into it. Changed the assert back to
what it was.
> If you agree, then this LGTM with those fixed.
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
Thanks! committed with the fixes above as commit 7fcdec025c05.
For completeness, I'm pasting the patch I committed below.
--
Thiago
>From 7fcdec025c0510741f3d7efc53e623e03c29e7d3 Mon Sep 17 00:00:00 2001
From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Subject: [PATCH] GDB: Use gdb::array_view for buffers used in register reading
and unwinding
This allows checking the size of the given buffer. Changes
frame_register_unwind (), frame_unwind_register (), get_frame_register ()
and deprecated_frame_register_read ().
As pointed out by Baris, in the case of MIPS target code this is best
done by changing a couple of alloca-based buffers in
mips_read_fp_register_single and mips_print_fp_register to
gdb::byte_vector instances.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
---
gdb/amd64-windows-tdep.c | 4 ++--
gdb/frame.c | 25 ++++++++++++---------
gdb/frame.h | 10 ++++-----
gdb/mips-tdep.c | 48 ++++++++++++++++++++++------------------
4 files changed, 48 insertions(+), 39 deletions(-)
diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 9e255bb2d433..ca7b7d96bc22 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -801,7 +801,7 @@ amd64_windows_frame_decode_insns (const frame_info_ptr &this_frame,
std::array<gdb_byte, 8> buf;
int frreg = amd64_windows_w2gdb_regnum[frame_reg];
- get_frame_register (this_frame, frreg, buf.data ());
+ get_frame_register (this_frame, frreg, buf);
save_addr = extract_unsigned_integer (buf, byte_order);
frame_debug_printf (" frame_reg=%s, val=%s",
@@ -1097,7 +1097,7 @@ amd64_windows_frame_cache (const frame_info_ptr &this_frame, void **this_cache)
/* Get current PC and SP. */
pc = get_frame_pc (this_frame);
- get_frame_register (this_frame, AMD64_RSP_REGNUM, buf.data ());
+ get_frame_register (this_frame, AMD64_RSP_REGNUM, buf);
cache->sp = extract_unsigned_integer (buf, byte_order);
cache->pc = pc;
diff --git a/gdb/frame.c b/gdb/frame.c
index ba4a07179f64..10a32dcd8966 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1112,7 +1112,7 @@ frame_save_as_regcache (const frame_info_ptr &this_frame)
{
auto cooked_read = [this_frame] (int regnum, gdb::array_view<gdb_byte> buf)
{
- if (!deprecated_frame_register_read (this_frame, regnum, buf.data ()))
+ if (!deprecated_frame_register_read (this_frame, regnum, buf))
return REG_UNAVAILABLE;
else
return REG_VALID;
@@ -1177,17 +1177,17 @@ void
frame_register_unwind (const frame_info_ptr &next_frame, int regnum,
int *optimizedp, int *unavailablep,
enum lval_type *lvalp, CORE_ADDR *addrp,
- int *realnump, gdb_byte *bufferp)
+ int *realnump,
+ gdb::array_view<gdb_byte> buffer)
{
struct value *value;
- /* Require all but BUFFERP to be valid. A NULL BUFFERP indicates
+ /* Require all but BUFFER to be valid. An empty BUFFER indicates
that the value proper does not need to be fetched. */
gdb_assert (optimizedp != NULL);
gdb_assert (lvalp != NULL);
gdb_assert (addrp != NULL);
gdb_assert (realnump != NULL);
- /* gdb_assert (bufferp != NULL); */
value = frame_unwind_register_value (next_frame, regnum);
@@ -1202,13 +1202,15 @@ frame_register_unwind (const frame_info_ptr &next_frame, int regnum,
else
*realnump = -1;
- if (bufferp)
+ if (!buffer.empty ())
{
+ gdb_assert (buffer.size () >= value->type ()->length ());
+
if (!*optimizedp && !*unavailablep)
- memcpy (bufferp, value->contents_all ().data (),
+ memcpy (buffer.data (), value->contents_all ().data (),
value->type ()->length ());
else
- memset (bufferp, 0, value->type ()->length ());
+ memset (buffer.data (), 0, value->type ()->length ());
}
/* Dispose of the new value. This prevents watchpoints from
@@ -1217,7 +1219,8 @@ frame_register_unwind (const frame_info_ptr &next_frame, int regnum,
}
void
-frame_unwind_register (const frame_info_ptr &next_frame, int regnum, gdb_byte *buf)
+frame_unwind_register (const frame_info_ptr &next_frame, int regnum,
+ gdb::array_view<gdb_byte> buf)
{
int optimized;
int unavailable;
@@ -1238,7 +1241,7 @@ frame_unwind_register (const frame_info_ptr &next_frame, int regnum, gdb_byte *b
void
get_frame_register (const frame_info_ptr &frame,
- int regnum, gdb_byte *buf)
+ int regnum, gdb::array_view<gdb_byte> buf)
{
frame_unwind_register (frame_info_ptr (frame->next), regnum, buf);
}
@@ -1482,7 +1485,7 @@ put_frame_register (const frame_info_ptr &next_frame, int regnum,
bool
deprecated_frame_register_read (const frame_info_ptr &frame, int regnum,
- gdb_byte *myaddr)
+ gdb::array_view<gdb_byte> myaddr)
{
int optimized;
int unavailable;
@@ -1541,7 +1544,7 @@ get_frame_register_bytes (const frame_info_ptr &next_frame, int regnum,
int realnum;
frame_register_unwind (next_frame, regnum, optimizedp, unavailablep,
- &lval, &addr, &realnum, buffer.data ());
+ &lval, &addr, &realnum, buffer);
if (*optimizedp || *unavailablep)
return false;
}
diff --git a/gdb/frame.h b/gdb/frame.h
index b265c9bc5bb3..e207c714c560 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -685,14 +685,14 @@ const char *unwind_stop_reason_to_string (enum unwind_stop_reason);
const char *frame_stop_reason_string (const frame_info_ptr &);
/* Unwind the stack frame so that the value of REGNUM, in the previous
- (up, older) frame is returned. If VALUEP is nullptr, don't
+ (up, older) frame is returned. If VALUE is zero-sized, don't
fetch/compute the value. Instead just return the location of the
value. */
extern void frame_register_unwind (const frame_info_ptr &frame, int regnum,
int *optimizedp, int *unavailablep,
enum lval_type *lvalp,
CORE_ADDR *addrp, int *realnump,
- gdb_byte *valuep = nullptr);
+ gdb::array_view<gdb_byte> value = {});
/* Fetch a register from this, or unwind a register from the next
frame. Note that the get_frame methods are wrappers to
@@ -701,9 +701,9 @@ extern void frame_register_unwind (const frame_info_ptr &frame, int regnum,
do return a lazy value. */
extern void frame_unwind_register (const frame_info_ptr &next_frame,
- int regnum, gdb_byte *buf);
+ int regnum, gdb::array_view<gdb_byte> buf);
extern void get_frame_register (const frame_info_ptr &frame,
- int regnum, gdb_byte *buf);
+ int regnum, gdb::array_view<gdb_byte> buf);
struct value *frame_unwind_register_value (const frame_info_ptr &next_frame,
int regnum);
@@ -889,7 +889,7 @@ extern void print_frame_info (const frame_print_options &fp_opts,
extern frame_info_ptr block_innermost_frame (const struct block *);
extern bool deprecated_frame_register_read (const frame_info_ptr &frame, int regnum,
- gdb_byte *buf);
+ gdb::array_view<gdb_byte> buf);
/* From stack.c. */
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index c00efbd02ad0..d217f23631f8 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -951,14 +951,17 @@ mips_register_to_value (const frame_info_ptr &frame, int regnum,
if (mips_convert_register_float_case_p (gdbarch, regnum, type))
{
- get_frame_register (frame, regnum + 0, to + 4);
- get_frame_register (frame, regnum + 1, to + 0);
+ gdb::array_view<gdb_byte> first_half = gdb::make_array_view (to, 4);
+ gdb::array_view<gdb_byte> second_half = gdb::make_array_view (to + 4, 4);
- if (!get_frame_register_bytes (next_frame, regnum + 0, 0, { to + 4, 4 },
+ get_frame_register (frame, regnum + 0, second_half);
+ get_frame_register (frame, regnum + 1, first_half);
+
+ if (!get_frame_register_bytes (next_frame, regnum + 0, 0, second_half,
optimizedp, unavailablep))
return 0;
- if (!get_frame_register_bytes (next_frame, regnum + 1, 0, { to + 0, 4 },
+ if (!get_frame_register_bytes (next_frame, regnum + 1, 0, first_half,
optimizedp, unavailablep))
return 0;
*optimizedp = *unavailablep = 0;
@@ -6252,11 +6255,11 @@ mips_o64_return_value (struct gdbarch *gdbarch, struct value *function,
static void
mips_read_fp_register_single (const frame_info_ptr &frame, int regno,
- gdb_byte *rare_buffer)
+ gdb::array_view<gdb_byte> rare_buffer)
{
struct gdbarch *gdbarch = get_frame_arch (frame);
int raw_size = register_size (gdbarch, regno);
- gdb_byte *raw_buffer = (gdb_byte *) alloca (raw_size);
+ gdb::byte_vector raw_buffer (raw_size);
if (!deprecated_frame_register_read (frame, regno, raw_buffer))
error (_("can't read register %d (%s)"),
@@ -6272,11 +6275,11 @@ mips_read_fp_register_single (const frame_info_ptr &frame, int regno,
else
offset = 0;
- memcpy (rare_buffer, raw_buffer + offset, 4);
+ memcpy (rare_buffer.data (), raw_buffer.data () + offset, 4);
}
else
{
- memcpy (rare_buffer, raw_buffer, 4);
+ memcpy (rare_buffer.data (), raw_buffer.data (), 4);
}
}
@@ -6286,7 +6289,7 @@ mips_read_fp_register_single (const frame_info_ptr &frame, int regno,
static void
mips_read_fp_register_double (const frame_info_ptr &frame, int regno,
- gdb_byte *rare_buffer)
+ gdb::array_view<gdb_byte> rare_buffer)
{
struct gdbarch *gdbarch = get_frame_arch (frame);
int raw_size = register_size (gdbarch, regno);
@@ -6311,13 +6314,14 @@ mips_read_fp_register_double (const frame_info_ptr &frame, int regno,
each register. */
if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
{
- mips_read_fp_register_single (frame, regno, rare_buffer + 4);
+ mips_read_fp_register_single (frame, regno, rare_buffer.slice (4));
mips_read_fp_register_single (frame, regno + 1, rare_buffer);
}
else
{
mips_read_fp_register_single (frame, regno, rare_buffer);
- mips_read_fp_register_single (frame, regno + 1, rare_buffer + 4);
+ mips_read_fp_register_single (frame, regno + 1,
+ rare_buffer.slice (4));
}
}
}
@@ -6327,15 +6331,13 @@ mips_print_fp_register (struct ui_file *file, const frame_info_ptr &frame,
int regnum)
{ /* Do values for FP (float) regs. */
struct gdbarch *gdbarch = get_frame_arch (frame);
- gdb_byte *raw_buffer;
std::string flt_str, dbl_str;
const struct type *flt_type = builtin_type (gdbarch)->builtin_float;
const struct type *dbl_type = builtin_type (gdbarch)->builtin_double;
- raw_buffer
- = ((gdb_byte *)
- alloca (2 * register_size (gdbarch, mips_regnum (gdbarch)->fp0)));
+ gdb::byte_vector raw_buffer (2 * register_size (gdbarch,
+ mips_regnum (gdbarch)->fp0));
gdb_printf (file, "%s:", gdbarch_register_name (gdbarch, regnum));
gdb_printf (file, "%*s",
@@ -6349,10 +6351,11 @@ mips_print_fp_register (struct ui_file *file, const frame_info_ptr &frame,
/* 4-byte registers: Print hex and floating. Also print even
numbered registers as doubles. */
mips_read_fp_register_single (frame, regnum, raw_buffer);
- flt_str = target_float_to_string (raw_buffer, flt_type, "%-17.9g");
+ flt_str = target_float_to_string (raw_buffer.data (), flt_type,
+ "%-17.9g");
get_formatted_print_options (&opts, 'x');
- print_scalar_formatted (raw_buffer,
+ print_scalar_formatted (raw_buffer.data (),
builtin_type (gdbarch)->builtin_uint32,
&opts, 'w', file);
@@ -6361,7 +6364,8 @@ mips_print_fp_register (struct ui_file *file, const frame_info_ptr &frame,
if ((regnum - gdbarch_num_regs (gdbarch)) % 2 == 0)
{
mips_read_fp_register_double (frame, regnum, raw_buffer);
- dbl_str = target_float_to_string (raw_buffer, dbl_type, "%-24.17g");
+ dbl_str = target_float_to_string (raw_buffer.data (), dbl_type,
+ "%-24.17g");
gdb_printf (file, " dbl: %s", dbl_str.c_str ());
}
@@ -6372,13 +6376,15 @@ mips_print_fp_register (struct ui_file *file, const frame_info_ptr &frame,
/* Eight byte registers: print each one as hex, float and double. */
mips_read_fp_register_single (frame, regnum, raw_buffer);
- flt_str = target_float_to_string (raw_buffer, flt_type, "%-17.9g");
+ flt_str = target_float_to_string (raw_buffer.data (), flt_type,
+ "%-17.9g");
mips_read_fp_register_double (frame, regnum, raw_buffer);
- dbl_str = target_float_to_string (raw_buffer, dbl_type, "%-24.17g");
+ dbl_str = target_float_to_string (raw_buffer.data (), dbl_type,
+ "%-24.17g");
get_formatted_print_options (&opts, 'x');
- print_scalar_formatted (raw_buffer,
+ print_scalar_formatted (raw_buffer.data (),
builtin_type (gdbarch)->builtin_uint64,
&opts, 'g', file);
More information about the Gdb-patches
mailing list