[PATCH 1/4] gdb: fix length of array view returned by some value_contents functions

[PATCH 1/4] gdb: fix length of array view returned by some value_contents functions

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

In commit 50888e42dcd3 ("gdb: change functions returning value contents
to use gdb::array_view"), I believe I made a mistake with the length of
the array views returned by some functions.  All functions return a view
of `TYPE_LENGTH (value_type (type))` length.  This is not correct when
the value's enclosing type is larger than the value's type.  In that
case, the value's contents buffer is of the size of the enclosing type,
and the value's actual contents is a slice of that (as returned by
value_contents).  So, functions value_contents_all_raw,
value_contents_for_printing and value_contents_for_printing_const are
not correct.  Since they are meant to return the value's contents buffer
as a whole, they should have the size of the enclosing type.

There is nothing that uses the returned array view size at the moment,
so this didn't cause a problem.  But it became apparent when trying to
adjust some callers.

Change-Id: Ib4e8837e1069111d2b2784d3253d5f3002419e68
---
 gdb/value.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/value.c b/gdb/value.c
index 998bec321a2..8669ad8fe70 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1164,7 +1164,7 @@ value_contents_all_raw (struct value *value)
 {
   allocate_value_contents (value);
 
-  ULONGEST length = TYPE_LENGTH (value_type (value));
+  ULONGEST length = TYPE_LENGTH (value_enclosing_type (value));
   return gdb::make_array_view (value->contents.get (), length);
 }
 
@@ -1249,7 +1249,7 @@ value_contents_for_printing (struct value *value)
   if (value->lazy)
     value_fetch_lazy (value);
 
-  ULONGEST length = TYPE_LENGTH (value_type (value));
+  ULONGEST length = TYPE_LENGTH (value_enclosing_type (value));
   return gdb::make_array_view (value->contents.get (), length);
 }
 
@@ -1258,7 +1258,7 @@ value_contents_for_printing_const (const struct value *value)
 {
   gdb_assert (!value->lazy);
 
-  ULONGEST length = TYPE_LENGTH (value_type (value));
+  ULONGEST length = TYPE_LENGTH (value_enclosing_type (value));
   return gdb::make_array_view (value->contents.get (), length);
 }
 
-- 
2.33.0

[PATCH 2/4] gdbsupport: add array_view copy function

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

An assertion was recently added to array_view::operator[] to ensure we
don't do out of bounds accesses.  However, when the array_view is copied
to or from using memcpy, it bypasses this safety.

To address this, add a `copy` free function that copies data from an
array view to another.  It ensures that the destination and source array
views have the same size and element size, which prevents any kind of
overflow in the source and in the destination.

I have chosen to allow the array views to have different types, because
I am thinking we might want to copy arrays of bfd_byte into arrays of
gdb_byte, for example.

Change a few randomly selected spots to use the new function, to show
how it can be used.

Change-Id: Ibeaca04e0028410fd44ce82f72e60058d6230a03
---
 gdb/ada-lang.c          | 17 +++++---------
 gdb/dwarf2/expr.c       |  4 ++--
 gdb/frame.c             |  4 ++--
 gdb/valarith.c          | 49 +++++++++++++++++++++++------------------
 gdb/valops.c            | 44 ++++++++++++++++++++----------------
 gdb/value.c             | 22 +++++++++---------
 gdbsupport/array-view.h | 15 +++++++++++++
 7 files changed, 88 insertions(+), 67 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index d964dae42cf..9458f7a9cf7 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2586,9 +2586,7 @@ ada_value_assign (struct value *toval, struct value *fromval)
       write_memory_with_notification (to_addr, buffer, len);
 
       val = value_copy (toval);
-      memcpy (value_contents_raw (val).data (),
-	      value_contents (fromval).data (),
-	      TYPE_LENGTH (type));
+      copy (value_contents_raw (val), value_contents (fromval));
       deprecated_set_value_type (val, type);
 
       return val;
@@ -4184,9 +4182,7 @@ ada_convert_actual (struct value *actual, struct type *formal_type0)
 
 	      actual_type = ada_check_typedef (value_type (actual));
 	      val = allocate_value (actual_type);
-	      memcpy ((char *) value_contents_raw (val).data (),
-		      (char *) value_contents (actual).data (),
-		      TYPE_LENGTH (actual_type));
+	      copy (value_contents_raw (val), value_contents (actual));
 	      actual = ensure_lval (val);
 	    }
 	  result = value_addr (actual);
@@ -8898,7 +8894,6 @@ ada_promote_array_of_integrals (struct type *type, struct value *val)
 {
   struct type *elt_type = TYPE_TARGET_TYPE (type);
   LONGEST lo, hi;
-  struct value *res;
   LONGEST i;
 
   /* Verify that both val and type are arrays of scalars, and
@@ -8914,16 +8909,16 @@ ada_promote_array_of_integrals (struct type *type, struct value *val)
   if (!get_array_bounds (type, &lo, &hi))
     error (_("unable to determine array bounds"));
 
-  res = allocate_value (type);
+  value *res = allocate_value (type);
+  gdb::array_view<gdb_byte> res_contents = value_contents_writeable (res);
 
   /* Promote each array element.  */
   for (i = 0; i < hi - lo + 1; i++)
     {
       struct value *elt = value_cast (elt_type, value_subscript (val, lo + i));
+      int elt_len = TYPE_LENGTH (elt_type);
 
-      memcpy ((value_contents_writeable (res).data ()
-	       + (i * TYPE_LENGTH (elt_type))),
-	      value_contents_all (elt).data (), TYPE_LENGTH (elt_type));
+      copy (res_contents.slice (elt_len * i, elt_len), value_contents_all (elt));
     }
 
   return res;
diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
index 652161955d5..e00a620406f 100644
--- a/gdb/dwarf2/expr.c
+++ b/gdb/dwarf2/expr.c
@@ -1037,8 +1037,8 @@ dwarf_expr_context::fetch_result (struct type *type, struct type *subobj_type,
 	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG)
 	      subobj_offset += n - max;
 
-	    memcpy (value_contents_raw (retval).data (),
-		    value_contents_all (val).data () + subobj_offset, len);
+	    copy (value_contents_raw (retval),
+		  value_contents_all (val).slice (subobj_offset, len));
 	  }
 	  break;
 
diff --git a/gdb/frame.c b/gdb/frame.c
index 2a899fc494f..70b1247d15e 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1362,9 +1362,9 @@ read_frame_register_unsigned (frame_info *frame, int regnum,
     {
       struct gdbarch *gdbarch = get_frame_arch (frame);
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      int size = register_size (gdbarch, VALUE_REGNUM (regval));
 
-      *val = extract_unsigned_integer (value_contents (regval).data (), size,
+      gdb::array_view<const gdb_byte> contents = value_contents (regval);
+      *val = extract_unsigned_integer (contents.data (), contents.size (),
 				       byte_order);
       return true;
     }
diff --git a/gdb/valarith.c b/gdb/valarith.c
index 140ef448137..ebf394df437 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -1531,7 +1531,7 @@ value_vector_widen (struct value *scalar_value, struct type *vector_type)
 {
   /* Widen the scalar to a vector.  */
   struct type *eltype, *scalar_type;
-  struct value *val, *elval;
+  struct value *elval;
   LONGEST low_bound, high_bound;
   int i;
 
@@ -1554,11 +1554,14 @@ value_vector_widen (struct value *scalar_value, struct type *vector_type)
       && !value_equal (elval, scalar_value))
     error (_("conversion of scalar to vector involves truncation"));
 
-  val = allocate_value (vector_type);
+  value *val = allocate_value (vector_type);
+  gdb::array_view<gdb_byte> val_contents = value_contents_writeable (val);
+  int elt_len = TYPE_LENGTH (eltype);
+
   for (i = 0; i < high_bound - low_bound + 1; i++)
     /* Duplicate the contents of elval into the destination vector.  */
-    memcpy (value_contents_writeable (val).data () + (i * TYPE_LENGTH (eltype)),
-	    value_contents_all (elval).data (), TYPE_LENGTH (eltype));
+    copy (val_contents.slice (i * elt_len, elt_len),
+	  value_contents_all (elval));
 
   return val;
 }
@@ -1569,7 +1572,6 @@ value_vector_widen (struct value *scalar_value, struct type *vector_type)
 static struct value *
 vector_binop (struct value *val1, struct value *val2, enum exp_opcode op)
 {
-  struct value *val, *tmp, *mark;
   struct type *type1, *type2, *eltype1, *eltype2;
   int t1_is_vec, t2_is_vec, elsize, i;
   LONGEST low_bound1, high_bound1, low_bound2, high_bound2;
@@ -1599,15 +1601,15 @@ vector_binop (struct value *val1, struct value *val2, enum exp_opcode op)
       || low_bound1 != low_bound2 || high_bound1 != high_bound2)
     error (_("Cannot perform operation on vectors with different types"));
 
-  val = allocate_value (type1);
-  mark = value_mark ();
+  value *val = allocate_value (type1);
+  gdb::array_view<gdb_byte> val_contents = value_contents_writeable (val);
+  value *mark = value_mark ();
   for (i = 0; i < high_bound1 - low_bound1 + 1; i++)
     {
-      tmp = value_binop (value_subscript (val1, i),
-			 value_subscript (val2, i), op);
-      memcpy (value_contents_writeable (val).data () + i * elsize,
-	      value_contents_all (tmp).data (),
-	      elsize);
+      value *tmp = value_binop (value_subscript (val1, i),
+				value_subscript (val2, i), op);
+      copy (val_contents.slice (i * elsize, elsize),
+	    value_contents_all (tmp));
      }
   value_free_to_mark (mark);
 
@@ -1888,7 +1890,7 @@ value_neg (struct value *arg1)
     return value_binop (value_zero (type, not_lval), arg1, BINOP_SUB);
   else if (type->code () == TYPE_CODE_ARRAY && type->is_vector ())
     {
-      struct value *tmp, *val = allocate_value (type);
+      struct value *val = allocate_value (type);
       struct type *eltype = check_typedef (TYPE_TARGET_TYPE (type));
       int i;
       LONGEST low_bound, high_bound;
@@ -1896,12 +1898,14 @@ value_neg (struct value *arg1)
       if (!get_array_bounds (type, &low_bound, &high_bound))
 	error (_("Could not determine the vector bounds"));
 
+      gdb::array_view<gdb_byte> val_contents = value_contents_writeable (val);
+      int elt_len = TYPE_LENGTH (eltype);
+
       for (i = 0; i < high_bound - low_bound + 1; i++)
 	{
-	  tmp = value_neg (value_subscript (arg1, i));
-	  memcpy ((value_contents_writeable (val).data ()
-		   + i * TYPE_LENGTH (eltype)),
-		  value_contents_all (tmp).data (), TYPE_LENGTH (eltype));
+	  value *tmp = value_neg (value_subscript (arg1, i));
+	  copy (val_contents.slice (i * elt_len, elt_len),
+		value_contents_all (tmp));
 	}
       return val;
     }
@@ -1931,7 +1935,6 @@ value_complement (struct value *arg1)
     val = value_from_longest (type, ~value_as_long (arg1));
   else if (type->code () == TYPE_CODE_ARRAY && type->is_vector ())
     {
-      struct value *tmp;
       struct type *eltype = check_typedef (TYPE_TARGET_TYPE (type));
       int i;
       LONGEST low_bound, high_bound;
@@ -1940,12 +1943,14 @@ value_complement (struct value *arg1)
 	error (_("Could not determine the vector bounds"));
 
       val = allocate_value (type);
+      gdb::array_view<gdb_byte> val_contents = value_contents_writeable (val);
+      int elt_len = TYPE_LENGTH (eltype);
+
       for (i = 0; i < high_bound - low_bound + 1; i++)
 	{
-	  tmp = value_complement (value_subscript (arg1, i));
-	  memcpy ((value_contents_writeable (val).data ()
-		   + i * TYPE_LENGTH (eltype)),
-		  value_contents_all (tmp).data (), TYPE_LENGTH (eltype));
+	  value *tmp = value_complement (value_subscript (arg1, i));
+	  copy (val_contents.slice (i * elt_len, elt_len),
+		value_contents_all (tmp));
 	}
     }
   else if (type->code () == TYPE_CODE_COMPLEX)
diff --git a/gdb/valops.c b/gdb/valops.c
index 9787cdbb513..106e12d3234 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -586,9 +586,12 @@ value_cast (struct type *type, struct value *arg2)
 	 sees a cast as a simple reinterpretation of the pointer's
 	 bits.  */
       if (code2 == TYPE_CODE_PTR)
-	longest = extract_unsigned_integer
-		    (value_contents (arg2).data (), TYPE_LENGTH (type2),
-		     type_byte_order (type2));
+	{
+	  gdb::array_view<const gdb_byte> contents = value_contents (arg2);
+	  longest = extract_unsigned_integer (contents.data (),
+					      contents.size (),
+					      type_byte_order (type2));
+	}
       else
 	longest = value_as_long (arg2);
       return value_from_longest (to_type, convert_to_boolean ?
@@ -954,18 +957,19 @@ value_one (struct type *type)
       struct type *eltype = check_typedef (TYPE_TARGET_TYPE (type1));
       int i;
       LONGEST low_bound, high_bound;
-      struct value *tmp;
 
       if (!get_array_bounds (type1, &low_bound, &high_bound))
 	error (_("Could not determine the vector bounds"));
 
       val = allocate_value (type);
+      gdb::array_view<gdb_byte> val_contents = value_contents_writeable (val);
+      int elt_len = TYPE_LENGTH (eltype);
+
       for (i = 0; i < high_bound - low_bound + 1; i++)
 	{
-	  tmp = value_one (eltype);
-	  memcpy ((value_contents_writeable (val).data ()
-		   + i * TYPE_LENGTH (eltype)),
-		  value_contents_all (tmp).data (), TYPE_LENGTH (eltype));
+	  value *tmp = value_one (eltype);
+	  copy (val_contents.slice (i * elt_len, elt_len),
+		value_contents_all (tmp));
 	}
     }
   else
@@ -1342,8 +1346,7 @@ value_assign (struct value *toval, struct value *fromval)
      implies the returned value is not lazy, even if TOVAL was.  */
   val = value_copy (toval);
   set_value_lazy (val, 0);
-  memcpy (value_contents_raw (val).data (), value_contents (fromval).data (),
-	  TYPE_LENGTH (type));
+  copy (value_contents_raw (val), value_contents (fromval));
 
   /* We copy over the enclosing type and pointed-to offset from FROMVAL
      in the case of pointer types.  For object types, the enclosing type
@@ -4030,10 +4033,13 @@ value_literal_complex (struct value *arg1,
   arg1 = value_cast (real_type, arg1);
   arg2 = value_cast (real_type, arg2);
 
-  memcpy (value_contents_raw (val).data (),
-	  value_contents (arg1).data (), TYPE_LENGTH (real_type));
-  memcpy (value_contents_raw (val).data () + TYPE_LENGTH (real_type),
-	  value_contents (arg2).data (), TYPE_LENGTH (real_type));
+  int len = TYPE_LENGTH (real_type);
+
+  copy (value_contents_raw (val).slice (0, len),
+	value_contents (arg1));
+  copy (value_contents_raw (val).slice (len, len),
+	value_contents (arg2));
+
   return val;
 }
 
@@ -4074,12 +4080,12 @@ cast_into_complex (struct type *type, struct value *val)
       struct type *val_real_type = TYPE_TARGET_TYPE (value_type (val));
       struct value *re_val = allocate_value (val_real_type);
       struct value *im_val = allocate_value (val_real_type);
+      int len = TYPE_LENGTH (val_real_type);
 
-      memcpy (value_contents_raw (re_val).data (),
-	      value_contents (val).data (), TYPE_LENGTH (val_real_type));
-      memcpy (value_contents_raw (im_val).data (),
-	      value_contents (val).data () + TYPE_LENGTH (val_real_type),
-	      TYPE_LENGTH (val_real_type));
+      copy (value_contents_raw (re_val),
+	    value_contents (val).slice (0, len));
+      copy (value_contents_raw (im_val),
+	    value_contents (val).slice (len, len));
 
       return value_literal_complex (re_val, im_val, type);
     }
diff --git a/gdb/value.c b/gdb/value.c
index 8669ad8fe70..7c86e7e13ca 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1343,9 +1343,13 @@ value_contents_copy_raw (struct value *dst, LONGEST dst_offset,
 					     TARGET_CHAR_BIT * length));
 
   /* Copy the data.  */
-  memcpy (value_contents_all_raw (dst).data () + dst_offset * unit_size,
-	  value_contents_all_raw (src).data () + src_offset * unit_size,
-	  length * unit_size);
+  gdb::array_view<gdb_byte> dst_contents
+    = value_contents_all_raw (dst).slice (dst_offset * unit_size,
+					  length * unit_size);
+  gdb::array_view<const gdb_byte> src_contents
+    = value_contents_all_raw (src).slice (src_offset * unit_size,
+					  length * unit_size);
+  copy (dst_contents, src_contents);
 
   /* Copy the meta-data, adjusted.  */
   src_bit_offset = src_offset * unit_size * HOST_CHAR_BIT;
@@ -1720,13 +1724,11 @@ value_copy (struct value *arg)
   val->stack = arg->stack;
   val->is_zero = arg->is_zero;
   val->initialized = arg->initialized;
+
   if (!value_lazy (val))
-    {
-      memcpy (value_contents_all_raw (val).data (),
-	      value_contents_all_raw (arg).data (),
-	      TYPE_LENGTH (value_enclosing_type (arg)));
+    copy (value_contents_all_raw (val),
+	  value_contents_all_raw (arg));
 
-    }
   val->unavailable = arg->unavailable;
   val->optimized_out = arg->optimized_out;
   val->parent = arg->parent;
@@ -1771,9 +1773,7 @@ value_non_lval (struct value *arg)
       struct type *enc_type = value_enclosing_type (arg);
       struct value *val = allocate_value (enc_type);
 
-      memcpy (value_contents_all_raw (val).data (),
-	      value_contents_all (arg).data (),
-	      TYPE_LENGTH (enc_type));
+      copy (value_contents_all_raw (val), value_contents_all (arg));
       val->type = arg->type;
       set_value_embedded_offset (val, value_embedded_offset (arg));
       set_value_pointed_to_offset (val, value_pointed_to_offset (arg));
diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
index ab1d032c60e..5b2aee7ad25 100644
--- a/gdbsupport/array-view.h
+++ b/gdbsupport/array-view.h
@@ -205,6 +205,21 @@ class array_view
   size_type m_size;
 };
 
+/* Copy the contents referenced by the array view SRC to the array view DEST.
+
+   The two array views must have the same length and element size.
+
+   The copy is done using memcpy, so both element types must be trivially
+   copyable (this is checked by poison.h).  */
+
+template <typename T, typename U>
+void copy (gdb::array_view<T> dest, gdb::array_view<U> src)
+{
+  gdb_static_assert (sizeof (T) == sizeof (U));
+  gdb_assert (dest.size () == src.size ());
+  memcpy (dest.data (), src.data (), src.size () * sizeof (T));
+}
+
 /* Compare LHS and RHS for (deep) equality.  That is, whether LHS and
    RHS have the same sizes, and whether each pair of elements of LHS
    and RHS at the same position compares equal.  */
-- 
2.33.0

[PATCH 3/4] gdb: make extract_integer take an array_view

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

I think it would make sense for extract_integer, extract_signed_integer
and extract_unsigned_integer to take an array_view.  This way, when we
extract an integer, we can validate that we don't overflow the buffer
passed by the caller (e.g. ask to extract a 4-byte integer but pass a
2-byte buffer).

 - Change extract_integer to take an array_view
 - Add overloads of extract_signed_integer and extract_unsigned_integer
   that take array_views.  Keep the existing versions so we don't
   need to change all callers, but make them call the array_view
   versions.

This shortens some places like:

  result = extract_unsigned_integer (value_contents (result_val).data (),
				     TYPE_LENGTH (value_type (result_val)),
				     byte_order);

into

  result = extract_unsigned_integer (value_contents (result_val), byte_order);

value_contents returns an array view that is of length
`TYPE_LENGTH (value_type (result_val))` already, so the length is
implicitly communicated through the array view.

Change-Id: Ic1c1f98c88d5c17a8486393af316f982604d6c95
---
 gdb/defs.h                          | 23 ++++++++++++++++---
 gdb/dwarf2/expr.c                   |  4 +---
 gdb/findvar.c                       | 35 ++++++++++++++---------------
 gdb/regcache.c                      | 21 +++++++----------
 gdb/unittests/gmp-utils-selftests.c |  2 +-
 5 files changed, 47 insertions(+), 38 deletions(-)

diff --git a/gdb/defs.h b/gdb/defs.h
index f7e09eca9db..3b6a0e63905 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -63,6 +63,7 @@
 
 #include "gdbsupport/host-defs.h"
 #include "gdbsupport/enum-flags.h"
+#include "gdbsupport/array-view.h"
 
 /* Scope types enumerator.  List the types of scopes the compiler will
    accept.  */
@@ -500,20 +501,36 @@ enum symbol_needs_kind
 /* In findvar.c.  */
 
 template<typename T, typename = RequireLongest<T>>
-T extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order);
+T extract_integer (gdb::array_view<const gdb_byte>, enum bfd_endian byte_order);
+
+static inline LONGEST
+extract_signed_integer (gdb::array_view<const gdb_byte> buf,
+			enum bfd_endian byte_order)
+{
+  return extract_integer<LONGEST> (buf, byte_order);
+}
 
 static inline LONGEST
 extract_signed_integer (const gdb_byte *addr, int len,
 			enum bfd_endian byte_order)
 {
-  return extract_integer<LONGEST> (addr, len, byte_order);
+  return extract_signed_integer (gdb::array_view<const gdb_byte> (addr, len),
+				 byte_order);
+}
+
+static inline ULONGEST
+extract_unsigned_integer (gdb::array_view<const gdb_byte> buf,
+			  enum bfd_endian byte_order)
+{
+  return extract_integer<ULONGEST> (buf, byte_order);
 }
 
 static inline ULONGEST
 extract_unsigned_integer (const gdb_byte *addr, int len,
 			  enum bfd_endian byte_order)
 {
-  return extract_integer<ULONGEST> (addr, len, byte_order);
+  return extract_unsigned_integer (gdb::array_view<const gdb_byte> (addr, len),
+				   byte_order);
 }
 
 extern int extract_long_unsigned_integer (const gdb_byte *, int,
diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
index e00a620406f..e308a50d5f5 100644
--- a/gdb/dwarf2/expr.c
+++ b/gdb/dwarf2/expr.c
@@ -1157,9 +1157,7 @@ dwarf_expr_context::fetch_address (int n)
   ULONGEST result;
 
   dwarf_require_integral (value_type (result_val));
-  result = extract_unsigned_integer (value_contents (result_val).data (),
-				     TYPE_LENGTH (value_type (result_val)),
-				     byte_order);
+  result = extract_unsigned_integer (value_contents (result_val), byte_order);
 
   /* For most architectures, calling extract_unsigned_integer() alone
      is sufficient for extracting an address.  However, some
diff --git a/gdb/findvar.c b/gdb/findvar.c
index d2b77133982..12134f88c0a 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -48,14 +48,11 @@ you lose
 
 template<typename T, typename>
 T
-extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order)
+extract_integer (gdb::array_view<const gdb_byte> buf, enum bfd_endian byte_order)
 {
   typename std::make_unsigned<T>::type retval = 0;
-  const unsigned char *p;
-  const unsigned char *startaddr = addr;
-  const unsigned char *endaddr = startaddr + len;
 
-  if (len > (int) sizeof (T))
+  if (buf.size () > (int) sizeof (T))
     error (_("\
 That operation is not available on integers of more than %d bytes."),
 	   (int) sizeof (T));
@@ -64,36 +61,38 @@ That operation is not available on integers of more than %d bytes."),
      the least significant.  */
   if (byte_order == BFD_ENDIAN_BIG)
     {
-      p = startaddr;
+      size_t i = 0;
+
       if (std::is_signed<T>::value)
 	{
 	  /* Do the sign extension once at the start.  */
-	  retval = ((LONGEST) * p ^ 0x80) - 0x80;
-	  ++p;
+	  retval = ((LONGEST) buf[i] ^ 0x80) - 0x80;
+	  ++i;
 	}
-      for (; p < endaddr; ++p)
-	retval = (retval << 8) | *p;
+      for (; i < buf.size (); ++i)
+	retval = (retval << 8) | buf[i];
     }
   else
     {
-      p = endaddr - 1;
+      ssize_t i = buf.size () - 1;
+
       if (std::is_signed<T>::value)
 	{
 	  /* Do the sign extension once at the start.  */
-	  retval = ((LONGEST) * p ^ 0x80) - 0x80;
-	  --p;
+	  retval = ((LONGEST) buf[i] ^ 0x80) - 0x80;
+	  --i;
 	}
-      for (; p >= startaddr; --p)
-	retval = (retval << 8) | *p;
+      for (; i >= 0; --i)
+	retval = (retval << 8) | buf[i];
     }
   return retval;
 }
 
 /* Explicit instantiations.  */
-template LONGEST extract_integer<LONGEST> (const gdb_byte *addr, int len,
+template LONGEST extract_integer<LONGEST> (gdb::array_view<const gdb_byte> buf,
 					   enum bfd_endian byte_order);
-template ULONGEST extract_integer<ULONGEST> (const gdb_byte *addr, int len,
-					     enum bfd_endian byte_order);
+template ULONGEST extract_integer<ULONGEST>
+  (gdb::array_view<const gdb_byte> buf, enum bfd_endian 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
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 8457284c12a..b3ecba2fbe6 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -620,15 +620,12 @@ template<typename T, typename>
 enum register_status
 readable_regcache::raw_read (int regnum, T *val)
 {
-  gdb_byte *buf;
-  enum register_status status;
-
   assert_regnum (regnum);
-  buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]);
-  status = raw_read (regnum, buf);
+  size_t len = m_descr->sizeof_register[regnum];
+  gdb_byte *buf = (gdb_byte *) alloca (len);
+  register_status status = raw_read (regnum, buf);
   if (status == REG_VALID)
-    *val = extract_integer<T> (buf,
-			       m_descr->sizeof_register[regnum],
+    *val = extract_integer<T> ({buf, len},
 			       gdbarch_byte_order (m_descr->gdbarch));
   else
     *val = 0;
@@ -772,14 +769,12 @@ template<typename T, typename>
 enum register_status
 readable_regcache::cooked_read (int regnum, T *val)
 {
-  enum register_status status;
-  gdb_byte *buf;
-
   gdb_assert (regnum >= 0 && regnum < m_descr->nr_cooked_registers);
-  buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]);
-  status = cooked_read (regnum, buf);
+  size_t len = m_descr->sizeof_register[regnum];
+  gdb_byte *buf = (gdb_byte *) alloca (len);
+  register_status status = cooked_read (regnum, buf);
   if (status == REG_VALID)
-    *val = extract_integer<T> (buf, m_descr->sizeof_register[regnum],
+    *val = extract_integer<T> ({buf, len},
 			       gdbarch_byte_order (m_descr->gdbarch));
   else
     *val = 0;
diff --git a/gdb/unittests/gmp-utils-selftests.c b/gdb/unittests/gmp-utils-selftests.c
index f40666e4cd5..e48bb39166e 100644
--- a/gdb/unittests/gmp-utils-selftests.c
+++ b/gdb/unittests/gmp-utils-selftests.c
@@ -299,7 +299,7 @@ write_and_extract (T val, size_t buf_len, enum bfd_endian byte_order)
   gdb_byte *buf = (gdb_byte *) alloca (buf_len);
   v.write ({buf, buf_len}, byte_order, !std::is_signed<T>::value);
 
-  return extract_integer<T> (buf, buf_len, byte_order);
+  return extract_integer<T> ({buf, buf_len}, byte_order);
 }
 
 /* Test the gdb_mpz::write method over a reasonable range of values.
-- 
2.33.0

[PATCH 4/4] gdb: trivial changes to use array_view

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

Change a few relatively obvious spots using value contents to propagate
the use array_view a bit more.

Change-Id: I5338a60986f06d5969fec803d04f8423c9288a15
---
 gdb/frame.c     |  2 +-
 gdb/rust-lang.c | 10 ++++------
 gdb/valarith.c  |  9 +++------
 gdb/valops.c    | 28 ++++++++++------------------
 gdb/value.c     |  2 +-
 5 files changed, 19 insertions(+), 32 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 70b1247d15e..268397259de 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1261,7 +1261,7 @@ frame_unwind_register_value (frame_info *next_frame, int regnum)
 	  else
 	    {
 	      int i;
-	      const gdb_byte *buf = value_contents (value).data ();
+	      gdb::array_view<const gdb_byte> buf = value_contents (value);
 
 	      fprintf_unfiltered (&debug_file, " bytes=");
 	      fprintf_unfiltered (&debug_file, "[");
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index e5a404187be..9ab8fbe30c0 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -1317,9 +1317,8 @@ eval_op_rust_struct_anon (struct type *expect_type, struct expression *exp,
 
       if (rust_enum_p (type))
 	{
-	  gdb::array_view<const gdb_byte> view (value_contents (lhs).data (),
-						TYPE_LENGTH (type));
-	  type = resolve_dynamic_type (type, view, value_address (lhs));
+	  type = resolve_dynamic_type (type, value_contents (lhs),
+				       value_address (lhs));
 
 	  if (rust_empty_enum_p (type))
 	    error (_("Cannot access field %d of empty enum %s"),
@@ -1380,9 +1379,8 @@ eval_op_rust_structop (struct type *expect_type, struct expression *exp,
   struct type *type = value_type (lhs);
   if (type->code () == TYPE_CODE_STRUCT && rust_enum_p (type))
     {
-      gdb::array_view<const gdb_byte> view (value_contents (lhs).data (),
-					    TYPE_LENGTH (type));
-      type = resolve_dynamic_type (type, view, value_address (lhs));
+      type = resolve_dynamic_type (type, value_contents (lhs),
+				   value_address (lhs));
 
       if (rust_empty_enum_p (type))
 	error (_("Cannot access field %s of empty enum %s"),
diff --git a/gdb/valarith.c b/gdb/valarith.c
index ebf394df437..ac995a5cbb0 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -927,12 +927,10 @@ fixed_point_binop (struct value *arg1, struct value *arg2, enum exp_opcode op)
 	  type2 = type1;
 	}
 
-      v1.read_fixed_point (gdb::make_array_view (value_contents (arg1).data (),
-						 TYPE_LENGTH (type1)),
+      v1.read_fixed_point (value_contents (arg1),
 			   type_byte_order (type1), type1->is_unsigned (),
 			   type1->fixed_point_scaling_factor ());
-      v2.read_fixed_point (gdb::make_array_view (value_contents (arg2).data (),
-						 TYPE_LENGTH (type2)),
+      v2.read_fixed_point (value_contents (arg2),
 			   type_byte_order (type2), type2->is_unsigned (),
 			   type2->fixed_point_scaling_factor ());
     }
@@ -942,8 +940,7 @@ fixed_point_binop (struct value *arg1, struct value *arg2, enum exp_opcode op)
       value *fp_val = allocate_value (type1);
 
       fp.write_fixed_point
-	(gdb::make_array_view (value_contents_raw (fp_val).data (),
-			       TYPE_LENGTH (type1)),
+	(value_contents_raw (fp_val),
 	 type_byte_order (type1),
 	 type1->is_unsigned (),
 	 type1->fixed_point_scaling_factor ());
diff --git a/gdb/valops.c b/gdb/valops.c
index 106e12d3234..86a1da11ec3 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -351,9 +351,8 @@ value_to_gdb_mpq (struct value *value)
 		  || is_fixed_point_type (type));
 
       gdb_mpz vz;
-      vz.read (gdb::make_array_view (value_contents (value).data (),
-				     TYPE_LENGTH (type)),
-	       type_byte_order (type), type->is_unsigned ());
+      vz.read (value_contents (value), type_byte_order (type),
+	       type->is_unsigned ());
       mpq_set_z (result.val, vz.val);
 
       if (is_fixed_point_type (type))
@@ -392,8 +391,7 @@ value_cast_to_fixed_point (struct type *to_type, struct value *from_val)
   /* Finally, create the result value, and pack the unscaled value
      in it.  */
   struct value *result = allocate_value (to_type);
-  unscaled.write (gdb::make_array_view (value_contents_raw (result).data (),
-					TYPE_LENGTH (to_type)),
+  unscaled.write (value_contents_raw (result),
 		  type_byte_order (to_type),
 		  to_type->is_unsigned ());
 
@@ -554,11 +552,10 @@ value_cast (struct type *type, struct value *arg2)
 	{
 	  gdb_mpq fp_val;
 
-	  fp_val.read_fixed_point
-	    (gdb::make_array_view (value_contents (arg2).data (),
-				   TYPE_LENGTH (type2)),
-	     type_byte_order (type2), type2->is_unsigned (),
-	     type2->fixed_point_scaling_factor ());
+	  fp_val.read_fixed_point (value_contents (arg2),
+				   type_byte_order (type2),
+				   type2->is_unsigned (),
+				   type2->fixed_point_scaling_factor ());
 
 	  struct value *v = allocate_value (to_type);
 	  target_float_from_host_double (value_contents_raw (v).data (),
@@ -1259,14 +1256,9 @@ value_assign (struct value *toval, struct value *fromval)
 					   value_contents (fromval).data ());
 	      }
 	    else
-	      {
-		gdb::array_view<const gdb_byte> contents
-		  = gdb::make_array_view (value_contents (fromval).data (),
-					  TYPE_LENGTH (type));
-		put_frame_register_bytes (frame, value_reg,
-					  value_offset (toval),
-					  contents);
-	      }
+	      put_frame_register_bytes (frame, value_reg,
+					value_offset (toval),
+					value_contents (fromval));
 	  }
 
 	gdb::observers::register_changed.notify (frame, value_reg);
diff --git a/gdb/value.c b/gdb/value.c
index 7c86e7e13ca..88080a01df8 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -4010,7 +4010,7 @@ value_fetch_lazy_register (struct value *val)
       else
 	{
 	  int i;
-	  const gdb_byte *buf = value_contents (new_val).data ();
+	  gdb::array_view<const gdb_byte> buf = value_contents (new_val);
 
 	  if (VALUE_LVAL (new_val) == lval_register)
 	    fprintf_unfiltered (&debug_file, " register=%d",
-- 
2.33.0

Re: [PATCH 2/4] gdbsupport: add array_view copy function

[Simon Marchi]

On 2021-11-08 4:06 p.m., Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> An assertion was recently added to array_view::operator[] to ensure we
> don't do out of bounds accesses.  However, when the array_view is copied
> to or from using memcpy, it bypasses this safety.
> 
> To address this, add a `copy` free function that copies data from an
> array view to another.  It ensures that the destination and source array
> views have the same size and element size, which prevents any kind of
> overflow in the source and in the destination.
> 
> I have chosen to allow the array views to have different types, because
> I am thinking we might want to copy arrays of bfd_byte into arrays of
> gdb_byte, for example.
> 
> Change a few randomly selected spots to use the new function, to show
> how it can be used.
> 
> Change-Id: Ibeaca04e0028410fd44ce82f72e60058d6230a03

Hi Tom,

This patch of mine causes the following test failure:

Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.ada/packed_array_assign.exp ...
FAIL: gdb.ada/packed_array_assign.exp: print pra(1) := pr (GDB internal error)

I couldn't figure out if this is a mistake on my part, or if my assert points out an
existing bug.  Could you give it a quick look?

The problem is that the values we are copying from and to don't have the same length:

(top-gdb) p toval.type.length
$1 = 5
(top-gdb) p fromval.type.length
$2 = 8

... and that trips on the new assert.

The two values appear to have the same type, so I don't understand where the different
lengths come from.

Simon

Re: [PATCH 1/4] gdb: fix length of array view returned by some value_contents functions

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> From: Simon Marchi <simon.marchi@polymtl.ca>
Simon> In commit 50888e42dcd3 ("gdb: change functions returning value contents
Simon> to use gdb::array_view"), I believe I made a mistake with the length of
Simon> the array views returned by some functions.  All functions return a view
Simon> of `TYPE_LENGTH (value_type (type))` length.  This is not correct when
Simon> the value's enclosing type is larger than the value's type.  In that
Simon> case, the value's contents buffer is of the size of the enclosing type,
Simon> and the value's actual contents is a slice of that (as returned by
Simon> value_contents).  So, functions value_contents_all_raw,
Simon> value_contents_for_printing and value_contents_for_printing_const are
Simon> not correct.  Since they are meant to return the value's contents buffer
Simon> as a whole, they should have the size of the enclosing type.

This looks good to me.

Tom

Re: [PATCH 2/4] gdbsupport: add array_view copy function

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> An assertion was recently added to array_view::operator[] to ensure we
Simon> don't do out of bounds accesses.  However, when the array_view is copied
Simon> to or from using memcpy, it bypasses this safety.

Simon> To address this, add a `copy` free function that copies data from an
Simon> array view to another.  It ensures that the destination and source array
Simon> views have the same size and element size, which prevents any kind of
Simon> overflow in the source and in the destination.

In C++20 I think we could use std::copy by having the array view
iterators implement the contiguous_iterator concept.  So, maybe a
comment to this effect would be good.

Though maybe in C++20 we would want to switch from array_view to
std::span.

Tom

Re: [PATCH 2/4] gdbsupport: add array_view copy function

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> This patch of mine causes the following test failure:

Simon> Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.ada/packed_array_assign.exp ...
Simon> FAIL: gdb.ada/packed_array_assign.exp: print pra(1) := pr (GDB internal error)

Simon> I couldn't figure out if this is a mistake on my part, or if my assert points out an
Simon> existing bug.  Could you give it a quick look?

I'll try to look at this soon.

Tom

Re: [PATCH 1/4] gdb: fix length of array view returned by some value_contents functions

[Simon Marchi]

On 2021-11-16 3:41 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> From: Simon Marchi <simon.marchi@polymtl.ca>
> Simon> In commit 50888e42dcd3 ("gdb: change functions returning value contents
> Simon> to use gdb::array_view"), I believe I made a mistake with the length of
> Simon> the array views returned by some functions.  All functions return a view
> Simon> of `TYPE_LENGTH (value_type (type))` length.  This is not correct when
> Simon> the value's enclosing type is larger than the value's type.  In that
> Simon> case, the value's contents buffer is of the size of the enclosing type,
> Simon> and the value's actual contents is a slice of that (as returned by
> Simon> value_contents).  So, functions value_contents_all_raw,
> Simon> value_contents_for_printing and value_contents_for_printing_const are
> Simon> not correct.  Since they are meant to return the value's contents buffer
> Simon> as a whole, they should have the size of the enclosing type.
> 
> This looks good to me.
> 
> Tom
> 

Thanks, I pushed that one.

Simon

Re: [PATCH 2/4] gdbsupport: add array_view copy function

[Simon Marchi]

On 2021-11-16 3:53 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Simon> An assertion was recently added to array_view::operator[] to ensure we
> Simon> don't do out of bounds accesses.  However, when the array_view is copied
> Simon> to or from using memcpy, it bypasses this safety.
>
> Simon> To address this, add a `copy` free function that copies data from an
> Simon> array view to another.  It ensures that the destination and source array
> Simon> views have the same size and element size, which prevents any kind of
> Simon> overflow in the source and in the destination.
>
> In C++20 I think we could use std::copy by having the array view
> iterators implement the contiguous_iterator concept.  So, maybe a
> comment to this effect would be good.

Do you mean we could use std::copy to implement this copy function,
instead of using memcpy, and that would deal with the details of copying
in the most efficient manner?

If I read cppreference correctly [1], std::copy is available before
C++20, it's just that in C++20 it has been made constexpr.  So I could
maybe give it a try, instead of using memcpy.

[1] https://en.cppreference.com/w/cpp/algorithm/copy

> Though maybe in C++20 we would want to switch from array_view to
> std::span.

Ack.

Simon

Re: [PATCH 2/4] gdbsupport: add array_view copy function

[Tom Tromey]

>> In C++20 I think we could use std::copy by having the array view
>> iterators implement the contiguous_iterator concept.  So, maybe a
>> comment to this effect would be good.

Simon> Do you mean we could use std::copy to implement this copy function,
Simon> instead of using memcpy, and that would deal with the details of copying
Simon> in the most efficient manner?

Yeah, but IIUC it will only use memcpy or whatever if it sees the
contiguous_iterator concept (and if the content type is trivially
copyable).  I'm not sure if there's a way to get this behavior before
C++20 though.

Tom

Re: [PATCH 2/4] gdbsupport: add array_view copy function

[Simon Marchi]

On 2021-11-16 7:20 p.m., Tom Tromey wrote:
>>> In C++20 I think we could use std::copy by having the array view
>>> iterators implement the contiguous_iterator concept.  So, maybe a
>>> comment to this effect would be good.
>
> Simon> Do you mean we could use std::copy to implement this copy function,
> Simon> instead of using memcpy, and that would deal with the details of copying
> Simon> in the most efficient manner?
>
> Yeah, but IIUC it will only use memcpy or whatever if it sees the
> contiguous_iterator concept (and if the content type is trivially
> copyable).  I'm not sure if there's a way to get this behavior before
> C++20 though.
>
> Tom
>

I did some tests (with g++ 11, if that matters), compiled GDB with
-std=c++11, and it seems like std::copy does the right thing.  When
copying a trivial type (array_view<int>), it ends up using
this __builtin_memmove:

    https://gitlab.com/gnutools/gcc/-/blob/6f4ac4f81f89caac7e74127ed2e6db6bbb3d7426/libstdc++-v3/include/bits/stl_algobase.h#L414-434

When copying a type that has a user-defined copy assignment operator, it
ends up using this manual loop:

    https://gitlab.com/gnutools/gcc/-/blob/6f4ac4f81f89caac7e74127ed2e6db6bbb3d7426/libstdc++-v3/include/bits/stl_algobase.h#L394-412

That seems to be exactly what we want.

Also, reading about std::copy made me realize that we would have to make
sure we handle overlapping ranges correctly.  Using std::copy or
std::copy_backwards (depending on whether the destination comes before
the source) handles that.

I will post an updated series.

Simon