[2/2] 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.
---
 gdb/amd64-windows-tdep.c |  4 ++--
 gdb/frame.c              | 22 ++++++++++--------
 gdb/frame.h              | 10 ++++-----
 gdb/mips-tdep.c          | 48 ++++++++++++++++++++++------------------
 4 files changed, 47 insertions(+), 37 deletions(-)

Changes from RFCv4 patch 2:

- Allocated gdb::byte_vectors instead of making array_views in mips-tdep.c,
  as suggested by Baris. This required changing raw_buffer in
  mips_print_fp_register (the caller of mips_read_fp_register_single and
  mips_read_fp_register_double) to a gdb::byte_vector as well, and also
  changing the rare_buffer arguments to array views.

- Don't use std::optional for the buffer argument in frame_register_unwind,
  as suggested by Simon.

- Changed the gdb_assert in frame_register_unwind to require the buffer
  argument to have the same size as the register.
  

On 2025-01-10 11:44, Thiago Jung Bauermann wrote:
> 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.
> ---
>  gdb/amd64-windows-tdep.c |  4 ++--
>  gdb/frame.c              | 22 ++++++++++--------
>  gdb/frame.h              | 10 ++++-----
>  gdb/mips-tdep.c          | 48 ++++++++++++++++++++++------------------
>  4 files changed, 47 insertions(+), 37 deletions(-)
> 
> Changes from RFCv4 patch 2:
> 
> - Allocated gdb::byte_vectors instead of making array_views in mips-tdep.c,
>   as suggested by Baris. This required changing raw_buffer in
>   mips_print_fp_register (the caller of mips_read_fp_register_single and
>   mips_read_fp_register_double) to a gdb::byte_vector as well, and also
>   changing the rare_buffer arguments to array views.
> 
> - Don't use std::optional for the buffer argument in frame_register_unwind,
>   as suggested by Simon.
> 
> - Changed the gdb_assert in frame_register_unwind to require the buffer
>   argument to have the same size as the register.
> 
> 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..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.

> @@ -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.

If you agree, then this LGTM with those fixed.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon
  

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);
  

On 1/10/25 23:28, Thiago Jung Bauermann wrote:
> +      gdb_assert (buffer.size () >= value->type ()->length ());
> +

This causes a regression on s390x-linux for test-case gdb.base/return.exp:
...
(gdb) PASS: gdb.base/return.exp: continue to return of -5
return 5^M
Make func2 return now? (y or n) y^M
/home/vries/gdb/src/gdb/frame.c:1207: internal-error: 
frame_register_unwind: Assertion `buffer.size () >= value->type 
()->length ()' failed.^M
A problem internal to GDB has been detected,^M
further debugging may prove unreliable.^M
----- Backtrace -----^M
FAIL: gdb.base/return.exp: return value 5 (GDB internal error)
...

Before the commit, the test-case produces a fail, but doesn't assert:
...
(gdb) PASS: gdb.base/return.exp: continue to return of -5
return 5^M
Make func2 return now? (y or n) y^M
value has been optimized out^M
(gdb) FAIL: gdb.base/return.exp: return value 5
...

Concretely, we're trying to read machine register r11, which according 
to the CFI is saved in dwarf register 16:
...
   DW_CFA_register: r11 in r16 (f0)
...

Dwarf register 16 corresponds to f0 / v0 according to the ABI, and since 
v0 is available, v0 is picked by s390_dwarf_reg_to_regnum.

The assert then fails because the buffer that should hold the value of 8 
byte register r11:
...
(gdb) p buffer.size ()
$1 = 8
...
is smaller than the size of register v0:
...
(gdb) p value->type ()->length ()
$2 = 16
...

Removing the assert reverts back to previous behaviour.

Properly fixing this requires us to only look at the part that is 
relevant, copying the value from there, and checking for optimized out 
and unavailable only there.

This worked for s390x-linux:
...
diff --git a/gdb/frame.c b/gdb/frame.c
index 10a32dcd896..02583857019 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1193,8 +1193,14 @@ frame_register_unwind (const frame_info_ptr 
&next_frame, int regnum,

    gdb_assert (value != NULL);

-  *optimizedp = value->optimized_out ();
-  *unavailablep = !value->entirely_available ();
+  if (value->lazy ())
+    value->fetch_lazy ();
+
+  *optimizedp
+    = value->bits_any_optimized_out (value->offset () * 8,
+				     buffer.size () * 8);
+  *unavailablep
+    = !value->bytes_available (value->offset (), buffer.size ());
    *lvalp = value->lval ();
    *addrp = value->address ();
    if (*lvalp == lval_register)
@@ -1204,13 +1210,17 @@ frame_register_unwind (const frame_info_ptr 
&next_frame, int regnum,

    if (!buffer.empty ())
      {
-      gdb_assert (buffer.size () >= value->type ()->length ());
+      gdb_assert (buffer.size ()
+		  <= value->type ()->length () - value->offset ());

        if (!*optimizedp && !*unavailablep)
-	memcpy (buffer.data (), value->contents_all ().data (),
-		value->type ()->length ());
+	{
+	  auto value_part
+	    = value->contents_all ().slice (value->offset (), buffer.size ());
+	  memcpy (buffer.data (), value_part.data (), buffer.size ());
+	}
        else
-	memset (buffer.data (), 0, value->type ()->length ());
+	memset (buffer.data (), 0, buffer.size ());
      }

    /* Dispose of the new value.  This prevents watchpoints from
...

Thanks,
- Tom
  

Hello Tom,

Tom de Vries <tdevries@suse.de> writes:

> On 1/10/25 23:28, Thiago Jung Bauermann wrote:
>> +      gdb_assert (buffer.size () >= value->type ()->length ());
>> +
>
> This causes a regression on s390x-linux for test-case gdb.base/return.exp:
> ...
> (gdb) PASS: gdb.base/return.exp: continue to return of -5
> return 5^M
> Make func2 return now? (y or n) y^M
> /home/vries/gdb/src/gdb/frame.c:1207: internal-error: frame_register_unwind: Assertion
> `buffer.size () >= value->type ()->length ()' failed.^M
> A problem internal to GDB has been detected,^M
> further debugging may prove unreliable.^M
> ----- Backtrace -----^M
> FAIL: gdb.base/return.exp: return value 5 (GDB internal error)
> ...
>
> Before the commit, the test-case produces a fail, but doesn't assert:
> ...
> (gdb) PASS: gdb.base/return.exp: continue to return of -5
> return 5^M
> Make func2 return now? (y or n) y^M
> value has been optimized out^M
> (gdb) FAIL: gdb.base/return.exp: return value 5
> ...
>
> Concretely, we're trying to read machine register r11, which according to the CFI is saved
> in dwarf register 16:
> ...
>   DW_CFA_register: r11 in r16 (f0)
> ...
>
> Dwarf register 16 corresponds to f0 / v0 according to the ABI, and since v0 is available,
> v0 is picked by s390_dwarf_reg_to_regnum.
>
> The assert then fails because the buffer that should hold the value of 8 byte register
> r11:
> ...
> (gdb) p buffer.size ()
> $1 = 8
> ...
> is smaller than the size of register v0:
> ...
> (gdb) p value->type ()->length ()
> $2 = 16
> ...
>
> Removing the assert reverts back to previous behaviour.

Thank you for debugging the issue! So memcpy was overflowing the
buffer. Nice to see the assert in action. :)

> Properly fixing this requires us to only look at the part that is relevant, copying the
> value from there, and checking for optimized out and unavailable only there.
>
> This worked for s390x-linux:
> ...
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 10a32dcd896..02583857019 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1193,8 +1193,14 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
> regnum,
>
>    gdb_assert (value != NULL);
>
> -  *optimizedp = value->optimized_out ();
> -  *unavailablep = !value->entirely_available ();
> +  if (value->lazy ())
> +    value->fetch_lazy ();
> +
> +  *optimizedp
> +    = value->bits_any_optimized_out (value->offset () * 8,
> +				     buffer.size () * 8);
> +  *unavailablep
> +    = !value->bytes_available (value->offset (), buffer.size ());
>    *lvalp = value->lval ();
>    *addrp = value->address ();
>    if (*lvalp == lval_register)
> @@ -1204,13 +1210,17 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
> regnum,
>
>    if (!buffer.empty ())
>      {
> -      gdb_assert (buffer.size () >= value->type ()->length ());
> +      gdb_assert (buffer.size ()
> +		  <= value->type ()->length () - value->offset ());

It should be '>=' above.

>        if (!*optimizedp && !*unavailablep)
> -	memcpy (buffer.data (), value->contents_all ().data (),
> -		value->type ()->length ());
> +	{
> +	  auto value_part
> +	    = value->contents_all ().slice (value->offset (), buffer.size ());
> +	  memcpy (buffer.data (), value_part.data (), buffer.size ());
> +	}
>        else
> -	memset (buffer.data (), 0, value->type ()->length ());
> +	memset (buffer.data (), 0, buffer.size ());
>      }
>
>    /* Dispose of the new value.  This prevents watchpoints from
> ...

Thank you for the patch! With the fix in the assert comparison:

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

--
Thiago
  

On 1/11/25 16:46, Thiago Jung Bauermann wrote:
> 
> Hello Tom,
> 
> Tom de Vries <tdevries@suse.de> writes:
> 
>> On 1/10/25 23:28, Thiago Jung Bauermann wrote:
>>> +      gdb_assert (buffer.size () >= value->type ()->length ());
>>> +
>>
>> This causes a regression on s390x-linux for test-case gdb.base/return.exp:
>> ...
>> (gdb) PASS: gdb.base/return.exp: continue to return of -5
>> return 5^M
>> Make func2 return now? (y or n) y^M
>> /home/vries/gdb/src/gdb/frame.c:1207: internal-error: frame_register_unwind: Assertion
>> `buffer.size () >= value->type ()->length ()' failed.^M
>> A problem internal to GDB has been detected,^M
>> further debugging may prove unreliable.^M
>> ----- Backtrace -----^M
>> FAIL: gdb.base/return.exp: return value 5 (GDB internal error)
>> ...
>>
>> Before the commit, the test-case produces a fail, but doesn't assert:
>> ...
>> (gdb) PASS: gdb.base/return.exp: continue to return of -5
>> return 5^M
>> Make func2 return now? (y or n) y^M
>> value has been optimized out^M
>> (gdb) FAIL: gdb.base/return.exp: return value 5
>> ...
>>
>> Concretely, we're trying to read machine register r11, which according to the CFI is saved
>> in dwarf register 16:
>> ...
>>    DW_CFA_register: r11 in r16 (f0)
>> ...
>>
>> Dwarf register 16 corresponds to f0 / v0 according to the ABI, and since v0 is available,
>> v0 is picked by s390_dwarf_reg_to_regnum.
>>
>> The assert then fails because the buffer that should hold the value of 8 byte register
>> r11:
>> ...
>> (gdb) p buffer.size ()
>> $1 = 8
>> ...
>> is smaller than the size of register v0:
>> ...
>> (gdb) p value->type ()->length ()
>> $2 = 16
>> ...
>>
>> Removing the assert reverts back to previous behaviour.
> 
> Thank you for debugging the issue! So memcpy was overflowing the
> buffer. Nice to see the assert in action. :)
> 

Hi Thiago,

thanks for the quick review.

Not the memcpy, but the memset, because the optimized out branch was 
activated, but buffer overflow indeed.

>> Properly fixing this requires us to only look at the part that is relevant, copying the
>> value from there, and checking for optimized out and unavailable only there.
>>
>> This worked for s390x-linux:
>> ...
>> diff --git a/gdb/frame.c b/gdb/frame.c
>> index 10a32dcd896..02583857019 100644
>> --- a/gdb/frame.c
>> +++ b/gdb/frame.c
>> @@ -1193,8 +1193,14 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
>> regnum,
>>
>>     gdb_assert (value != NULL);
>>
>> -  *optimizedp = value->optimized_out ();
>> -  *unavailablep = !value->entirely_available ();
>> +  if (value->lazy ())
>> +    value->fetch_lazy ();
>> +
>> +  *optimizedp
>> +    = value->bits_any_optimized_out (value->offset () * 8,
>> +				     buffer.size () * 8);
>> +  *unavailablep
>> +    = !value->bytes_available (value->offset (), buffer.size ());
>>     *lvalp = value->lval ();
>>     *addrp = value->address ();
>>     if (*lvalp == lval_register)
>> @@ -1204,13 +1210,17 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
>> regnum,
>>
>>     if (!buffer.empty ())
>>       {
>> -      gdb_assert (buffer.size () >= value->type ()->length ());
>> +      gdb_assert (buffer.size ()
>> +		  <= value->type ()->length () - value->offset ());
> 
> It should be '>=' above.
> 

That doesn't work unfortunately:
- buffer.size () is 8
- value->type ()->length () is 16
- value->offset () == 0

So we'd have gdb_assert (8 >= 16).

FWIW, I've tried out a less intrusive fix which also works:
...
diff --git a/gdb/frame.c b/gdb/frame.c
index 10a32dcd896..96e0752888f 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1191,6 +1191,19 @@ frame_register_unwind (const frame_info_ptr 
&next_frame, int regnum,

    value = frame_unwind_register_value (next_frame, regnum);

+  frame_info_ptr this_frame = get_prev_frame (next_frame);
+  struct gdbarch *gdbarch = frame_unwind_arch (this_frame);
+  size_t reg_size = register_size (gdbarch, regnum);
+
+  if (value->type ()->length () > reg_size)
+    {
+      struct value *part_val
+	= value::allocate_register (this_frame, regnum);
+      value->contents_copy (part_val, 0, value->offset (), reg_size);
+      release_value (value);
+      value = part_val;
+    }
+
    gdb_assert (value != NULL);

    *optimizedp = value->optimized_out ();
...
but I have doubts whether reg_size is correct in case gdbarch changes 
between frames and register sizes are different.

Thanks,
- Tom

>>         if (!*optimizedp && !*unavailablep)
>> -	memcpy (buffer.data (), value->contents_all ().data (),
>> -		value->type ()->length ());
>> +	{
>> +	  auto value_part
>> +	    = value->contents_all ().slice (value->offset (), buffer.size ());
>> +	  memcpy (buffer.data (), value_part.data (), buffer.size ());
>> +	}
>>         else
>> -	memset (buffer.data (), 0, value->type ()->length ());
>> +	memset (buffer.data (), 0, buffer.size ());
>>       }
>>
>>     /* Dispose of the new value.  This prevents watchpoints from
>> ...
> 
> Thank you for the patch! With the fix in the assert comparison:
> 
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> 
> --
> Thiago
  

Tom de Vries <tdevries@suse.de> writes:

> On 1/11/25 16:46, Thiago Jung Bauermann wrote:
>> Hello Tom,
>> Tom de Vries <tdevries@suse.de> writes:
>>
>>> On 1/10/25 23:28, Thiago Jung Bauermann wrote:
>> Thank you for debugging the issue! So memcpy was overflowing the
>> buffer. Nice to see the assert in action. :)
>
> Hi Thiago,
>
> thanks for the quick review.

Too quick, as it turns out. Looking at it again, that fix relies on
buffer.size () reflecting the correct size of the register being
unwound. Simon found a counterexample in i386_unwind_pc, which is used
for both i386 and x86_64 and thus passes an 8-byte buffer:

https://inbox.sourceware.org/gdb-patches/321e71e0-43de-4604-bb7e-34f6f64b83bf@simark.ca/

> Not the memcpy, but the memset, because the optimized out branch was activated, but buffer
> overflow indeed.
>
>>> Properly fixing this requires us to only look at the part that is relevant, copying the
>>> value from there, and checking for optimized out and unavailable only there.
>>>
>>> This worked for s390x-linux:
>>> ...
>>> diff --git a/gdb/frame.c b/gdb/frame.c
>>> index 10a32dcd896..02583857019 100644
>>> --- a/gdb/frame.c
>>> +++ b/gdb/frame.c
>>> @@ -1193,8 +1193,14 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
>>> regnum,
>>>
>>>     gdb_assert (value != NULL);
>>>
>>> -  *optimizedp = value->optimized_out ();
>>> -  *unavailablep = !value->entirely_available ();
>>> +  if (value->lazy ())
>>> +    value->fetch_lazy ();
>>> +
>>> +  *optimizedp
>>> +    = value->bits_any_optimized_out (value->offset () * 8,
>>> +				     buffer.size () * 8);
>>> +  *unavailablep
>>> +    = !value->bytes_available (value->offset (), buffer.size ());
>>>     *lvalp = value->lval ();
>>>     *addrp = value->address ();
>>>     if (*lvalp == lval_register)
>>> @@ -1204,13 +1210,17 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
>>> regnum,
>>>
>>>     if (!buffer.empty ())
>>>       {
>>> -      gdb_assert (buffer.size () >= value->type ()->length ());
>>> +      gdb_assert (buffer.size ()
>>> +		  <= value->type ()->length () - value->offset ());
>> It should be '>=' above.
>>
>
> That doesn't work unfortunately:
> - buffer.size () is 8
> - value->type ()->length () is 16
> - value->offset () == 0
>
> So we'd have gdb_assert (8 >= 16).

Ah, I didn't know offset was 0. We do need an assert that the buffer is
big enough though. :/

> FWIW, I've tried out a less intrusive fix which also works:
> ...
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 10a32dcd896..96e0752888f 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1191,6 +1191,19 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
> regnum,
>
>    value = frame_unwind_register_value (next_frame, regnum);

Your fix below is interesting, but I'm starting to think that the real
bug is that frame_unwind_register_value returns a value with a type that
isn't regnum's type. Even if the value was found in another register,
that shouldn't concern the caller. WDYT?

Also, should I revert the patch while we don't find a solution?

> +  frame_info_ptr this_frame = get_prev_frame (next_frame);
> +  struct gdbarch *gdbarch = frame_unwind_arch (this_frame);

Shouldn't this be either:

  struct gdbarch *gdbarch = frame_unwind_arch (next_frame);

or:

  struct gdbarch *gdbarch = get_frame_arch (this_frame);

? I still get confused about this/next in frame unwinding...

> +  size_t reg_size = register_size (gdbarch, regnum);
> +
> +  if (value->type ()->length () > reg_size)
> +    {
> +      struct value *part_val
> +	= value::allocate_register (this_frame, regnum);

Shouldn't next_frame be used here? (Same confused face as above.)

> +      value->contents_copy (part_val, 0, value->offset (), reg_size);
> +      release_value (value);
> +      value = part_val;
> +    }
> +
>    gdb_assert (value != NULL);
>
>    *optimizedp = value->optimized_out ();
> ...
> but I have doubts whether reg_size is correct in case gdbarch changes between frames and
> register sizes are different.

AFAIU GDB should know the gdbarch of each frame in the stack, otherwise
things can get weird. Also, is there another option? As I mentioned
before, the buffer size can't be used unless we audit callers to make
sure they pass a buffer with the same size as the register being unwound
(as Simon mentioned in the email I linked above).

--
Thiago
  

On 1/12/25 01:32, Thiago Jung Bauermann wrote:
> 
> Tom de Vries <tdevries@suse.de> writes:
> 
>> On 1/11/25 16:46, Thiago Jung Bauermann wrote:
>>> Hello Tom,
>>> Tom de Vries <tdevries@suse.de> writes:
>>>
>>>> On 1/10/25 23:28, Thiago Jung Bauermann wrote:
>>> Thank you for debugging the issue! So memcpy was overflowing the
>>> buffer. Nice to see the assert in action. :)
>>
>> Hi Thiago,
>>
>> thanks for the quick review.
> 
> Too quick, as it turns out. Looking at it again, that fix relies on
> buffer.size () reflecting the correct size of the register being
> unwound. Simon found a counterexample in i386_unwind_pc, which is used
> for both i386 and x86_64 and thus passes an 8-byte buffer:
> 
> https://inbox.sourceware.org/gdb-patches/321e71e0-43de-4604-bb7e-34f6f64b83bf@simark.ca/
> 

Ack, I've taken this into account in a patch I've just submitted ( 
https://sourceware.org/pipermail/gdb-patches/2025-January/214695.html ).

>> Not the memcpy, but the memset, because the optimized out branch was activated, but buffer
>> overflow indeed.
>>

I seem to have confused myself here.  I thought I saw the test-case 
failing-but-not-asserting at 7fcdec025c0^, but I no longer see that. 
Instead I see it pass at 7fcdec025c0^, so it must have been an artifact 
of somehow not building clean or at the wrong commit.

In other words, indeed the memcpy triggers a buffer overflow.

My apologies for providing misleading information.

>>>> Properly fixing this requires us to only look at the part that is relevant, copying the
>>>> value from there, and checking for optimized out and unavailable only there.
>>>>
>>>> This worked for s390x-linux:
>>>> ...
>>>> diff --git a/gdb/frame.c b/gdb/frame.c
>>>> index 10a32dcd896..02583857019 100644
>>>> --- a/gdb/frame.c
>>>> +++ b/gdb/frame.c
>>>> @@ -1193,8 +1193,14 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
>>>> regnum,
>>>>
>>>>      gdb_assert (value != NULL);
>>>>
>>>> -  *optimizedp = value->optimized_out ();
>>>> -  *unavailablep = !value->entirely_available ();
>>>> +  if (value->lazy ())
>>>> +    value->fetch_lazy ();
>>>> +
>>>> +  *optimizedp
>>>> +    = value->bits_any_optimized_out (value->offset () * 8,
>>>> +				     buffer.size () * 8);
>>>> +  *unavailablep
>>>> +    = !value->bytes_available (value->offset (), buffer.size ());
>>>>      *lvalp = value->lval ();
>>>>      *addrp = value->address ();
>>>>      if (*lvalp == lval_register)
>>>> @@ -1204,13 +1210,17 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
>>>> regnum,
>>>>
>>>>      if (!buffer.empty ())
>>>>        {
>>>> -      gdb_assert (buffer.size () >= value->type ()->length ());
>>>> +      gdb_assert (buffer.size ()
>>>> +		  <= value->type ()->length () - value->offset ());
>>> It should be '>=' above.
>>>
>>
>> That doesn't work unfortunately:
>> - buffer.size () is 8
>> - value->type ()->length () is 16
>> - value->offset () == 0
>>
>> So we'd have gdb_assert (8 >= 16).
> 
> Ah, I didn't know offset was 0. We do need an assert that the buffer is
> big enough though. :/
> 

The patch I've submitted no longer has such an assert, instead it 
guarantees no buffer overflow through use of std::min.

>> FWIW, I've tried out a less intrusive fix which also works:
>> ...
>> diff --git a/gdb/frame.c b/gdb/frame.c
>> index 10a32dcd896..96e0752888f 100644
>> --- a/gdb/frame.c
>> +++ b/gdb/frame.c
>> @@ -1191,6 +1191,19 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
>> regnum,
>>
>>     value = frame_unwind_register_value (next_frame, regnum);
> 
> Your fix below is interesting, but I'm starting to think that the real
> bug is that frame_unwind_register_value returns a value with a type that
> isn't regnum's type. Even if the value was found in another register,
> that shouldn't concern the caller. WDYT?
> 

I'm not sure.  In this case the type seems less relevant because it's 
not returned from frame_register_unwind.  What is returned is 
value->regnum (), so this size discrepancy could propagate.

Things would be greatly simplified if frame_unwind_register_value 
returned the value of f0 instead of v0 in this case.

But perhaps that is a special case, and we're better off covering the 
generic case here in frame_register_unwind and elsewhere.

FWIW, I'm working on other patches addressing the exact same problem in 
other places.

> Also, should I revert the patch while we don't find a solution?
> 

If my proposed patch is acceptable, then there's no need, otherwise we 
might need to revisit that.

>> +  frame_info_ptr this_frame = get_prev_frame (next_frame);
>> +  struct gdbarch *gdbarch = frame_unwind_arch (this_frame);
> 
> Shouldn't this be either:
> 
>    struct gdbarch *gdbarch = frame_unwind_arch (next_frame);
> 
> or:
> 
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
> 
> ? I still get confused about this/next in frame unwinding...
> 

Yeah, I was and very much still am confused about this.

In my proposed patch, that's no longer relevant, I've pretty much 
reverted to my original approach.

>> +  size_t reg_size = register_size (gdbarch, regnum);
>> +
>> +  if (value->type ()->length () > reg_size)
>> +    {
>> +      struct value *part_val
>> +	= value::allocate_register (this_frame, regnum);
> 
> Shouldn't next_frame be used here? (Same confused face as above.)
> 
>> +      value->contents_copy (part_val, 0, value->offset (), reg_size);
>> +      release_value (value);
>> +      value = part_val;
>> +    }
>> +
>>     gdb_assert (value != NULL);
>>
>>     *optimizedp = value->optimized_out ();
>> ...
>> but I have doubts whether reg_size is correct in case gdbarch changes between frames and
>> register sizes are different.
> 
> AFAIU GDB should know the gdbarch of each frame in the stack, otherwise
> things can get weird. Also, is there another option? As I mentioned
> before, the buffer size can't be used unless we audit callers to make
> sure they pass a buffer with the same size as the register being unwound
> (as Simon mentioned in the email I linked above).

I'm not sure I understand your question, but my concern is that I'm not 
picking the right frame.

Picking the wrong frame will only matter in case architecture changes 
between frames, OTOH detecting that we've picked the wrong frame means 
exercising that scenario, and I don't know how to.

Thanks,
- Tom