[v2,1/3] gdbsupport: add array_view copy function

Message ID 20211118212951.2547899-2-simon.marchi@efficios.com
State New
Headers show
Series
  • Add array_view copy function and more
Related show

Commit Message

Mike Frysinger via Gdb-patches Nov. 18, 2021, 9:29 p.m.
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 that safety.

To address this, add a `copy` free function that copies data from an
array view to another, ensuring that the destination and source array
views have the same size.  When copying to or from parts of an
array_view, we are expected to use gdb::array_view::slice, which does
its own bounds check.  With all that, any copy operation that goes out
of bounds should be caught by an assertion at runtime.

copy is implemented using std::copy (and std::copy_backward), which, at
least on libstdc++, appears to pick memmove when copying trivial data.
So in the end there shouldn't be much difference vs using a bare memcpy,
as we do right now.  When copying non-trivial data, std::copy simply
assigns each element in a loop, as expected.

To properly support overlapping ranges, we must use std::copy_backward
instead of std::copy if the destination array overlaps and starts after
the source array.

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

Add a test for the new function, testing both with arrays of a trivial
type (int) and of a non-trivial type (foo).  Test non-overlapping
ranges as well as three kinds of overlapping ranges: source before dest,
dest before source, and dest == source.

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

-- 
2.33.1

Comments

Pedro Alves Nov. 19, 2021, 6:11 p.m. | #1
Hi!

On 2021-11-18 21:29, Simon Marchi via Gdb-patches wrote:
>  

> +/* 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).  */


This sentence about memcpy is stale.

> +

> +template <typename T, typename U>

> +void copy (gdb::array_view<T> dest, gdb::array_view<U> src)

> +{

> +  gdb_assert (dest.size () == src.size ());

> +  if (dest.data () <= src.data ())

> +    std::copy(src.begin (), src.end (), dest.begin ());


Missing space before parens.

> +  else

> +    std::copy_backward (src.begin (), src.end (), dest.end ());

> +}


Three points below.

#1 - overlap triggering undefined behavior

If SRC and DEST are exactly the same, as in:

  /* Test overlapping copy, where the source is the same as the destination.  */
  {
    std::vector<T> vec = {1, 2, 3, 4, 5, 6, 7, 8};
    gdb::array_view<T> v = vec;

    copy (v.slice (2, 4),
	  v.slice (2, 4));

    std::vector<T> expected = {1, 2, 3, 4, 5, 6, 7, 8};
    SELF_CHECK (vec == expected);
  }

Isn't copy invoking undefined behavior?  In this case, we end up
in std::copy, which states, in 25.3.1 in the C++11 standard:

 "Requires: result shall not be in the range [first,last)."

In the case in question, result == first, so it is in range.


#2 - argument order

You don't mention this explicitly in the commit log, so I'll raise it:

It's sad that memcpy and std::copy have source/dest positions reversed.

Off-hand, I'd think it is less confusing if a function named "copy", same
name as the C++ standard function, follows the convention of std::copy.

#3 - slicing

The function as is allows slicing.  I.e., this compiles and passes runtime testing:

  {
    struct A { int i; };
    struct B : A { int j; };

    A a[2];
    B b[2];

    gdb::array_view<A> v1 = a;
    gdb::array_view<B> v2 = b;

    copy (v1, v2);
  }

Do we want that?  I'd think not off-hand.

Pedro Alves
Mike Frysinger via Gdb-patches Nov. 22, 2021, 1:32 a.m. | #2
On 2021-11-19 13:11, Pedro Alves wrote:
> Hi!

> 

> On 2021-11-18 21:29, Simon Marchi via Gdb-patches wrote:

>>  

>> +/* 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).  */

> 

> This sentence about memcpy is stale.


Fixed.

>> +

>> +template <typename T, typename U>

>> +void copy (gdb::array_view<T> dest, gdb::array_view<U> src)

>> +{

>> +  gdb_assert (dest.size () == src.size ());

>> +  if (dest.data () <= src.data ())

>> +    std::copy(src.begin (), src.end (), dest.begin ());

> 

> Missing space before parens.


Fixed.

>> +  else

>> +    std::copy_backward (src.begin (), src.end (), dest.end ());

>> +}

> 

> Three points below.

> 

> #1 - overlap triggering undefined behavior

> 

> If SRC and DEST are exactly the same, as in:

> 

>   /* Test overlapping copy, where the source is the same as the destination.  */

>   {

>     std::vector<T> vec = {1, 2, 3, 4, 5, 6, 7, 8};

>     gdb::array_view<T> v = vec;

> 

>     copy (v.slice (2, 4),

> 	  v.slice (2, 4));

> 

>     std::vector<T> expected = {1, 2, 3, 4, 5, 6, 7, 8};

>     SELF_CHECK (vec == expected);

>   }

> 

> Isn't copy invoking undefined behavior?  In this case, we end up

> in std::copy, which states, in 25.3.1 in the C++11 standard:

> 

>  "Requires: result shall not be in the range [first,last)."

> 

> In the case in question, result == first, so it is in range.


Oh, right.  It seem like we can't use copy nor copy_backward if the
ranges are exactly overlapping.  Anyway, there's not much use to do the
copy if the source == the destination.  So I'll change it to this, would
that work?

  if (dest.data () < src.data ())
    std::copy (src.begin (), src.end (), dest.begin ());
  else if (dest.data () > src.data ())
    std::copy_backward (src.begin (), src.end (), dest.end ());

> #2 - argument order

> 

> You don't mention this explicitly in the commit log, so I'll raise it:

> 

> It's sad that memcpy and std::copy have source/dest positions reversed.

> 

> Off-hand, I'd think it is less confusing if a function named "copy", same

> name as the C++ standard function, follows the convention of std::copy.


Since I was mostly replacing some memcpys, I indeed mimicked memcpy.
But I'll change it to (src, dest).

> #3 - slicing

> 

> The function as is allows slicing.  I.e., this compiles and passes runtime testing:

> 

>   {

>     struct A { int i; };

>     struct B : A { int j; };

> 

>     A a[2];

>     B b[2];

> 

>     gdb::array_view<A> v1 = a;

>     gdb::array_view<B> v2 = b;

> 

>     copy (v1, v2);

>   }

> 

> Do we want that?  I'd think not off-hand.


I tried it, and we fall in the implementation of std::copy that does a
for loop over all items and justs uses the assignment operator (in other
words, the implementation for non-memcpyable types).  So I think the
behavior will be correct.

Simon

Patch

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index b84e10fd029..fa30dc9404d 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/unittests/array-view-selftests.c b/gdb/unittests/array-view-selftests.c
index 9df48db3912..a67be0534ac 100644
--- a/gdb/unittests/array-view-selftests.c
+++ b/gdb/unittests/array-view-selftests.c
@@ -546,6 +546,108 @@  run_tests ()
   }
 }
 
+template <typename T>
+void
+run_copy_test ()
+{
+  /* Test non-overlapping copy.  */
+  {
+    const std::vector<T> src_v = {1, 2, 3, 4};
+    std::vector<T> dest_v (4, -1);
+
+    SELF_CHECK (dest_v != src_v);
+    copy (gdb::array_view<T> (dest_v), gdb::array_view<const T> (src_v));
+    SELF_CHECK (dest_v == src_v);
+  }
+
+  /* Test overlapping copy, where the source is before the destination.  */
+  {
+    std::vector<T> vec = {1, 2, 3, 4, 5, 6, 7, 8};
+    gdb::array_view<T> v = vec;
+
+    copy (v.slice (2, 4),
+	  v.slice (1, 4));
+
+    std::vector<T> expected = {1, 2, 2, 3, 4, 5, 7, 8};
+    SELF_CHECK (vec == expected);
+  }
+
+  /* Test overlapping copy, where the source is after the destination.  */
+  {
+    std::vector<T> vec = {1, 2, 3, 4, 5, 6, 7, 8};
+    gdb::array_view<T> v = vec;
+
+    copy (v.slice (1, 4),
+	  v.slice (2, 4));
+
+    std::vector<T> expected = {1, 3, 4, 5, 6, 6, 7, 8};
+    SELF_CHECK (vec == expected);
+  }
+
+  /* Test overlapping copy, where the source is the same as the destination.  */
+  {
+    std::vector<T> vec = {1, 2, 3, 4, 5, 6, 7, 8};
+    gdb::array_view<T> v = vec;
+
+    copy (v.slice (2, 4),
+	  v.slice (2, 4));
+
+    std::vector<T> expected = {1, 2, 3, 4, 5, 6, 7, 8};
+    SELF_CHECK (vec == expected);
+  }
+}
+
+/* Class with a non-trivial copy assignment operator, used to test the
+   array_view copy function.  */
+struct foo
+{
+  /* Can be implicitly constructed from an int, such that we can use the same
+     templated test function to test against array_view<int> and
+     array_view<foo>.  */
+  foo (int n)
+    : n (n)
+  {}
+
+  /* Needed to avoid -Wdeprecated-copy-with-user-provided-copy error with
+     Clang.  */
+  foo (const foo &other) = default;
+
+  void operator= (const foo &other)
+  {
+    this->n = other.n;
+    this->n_assign_op_called++;
+  }
+
+  bool operator==(const foo &other) const
+  {
+    return this->n == other.n;
+  }
+
+  int n;
+
+  /* Number of times the assignment operator has been called.  */
+  static int n_assign_op_called;
+};
+
+int foo::n_assign_op_called = 0;
+
+/* Test the array_view copy free function.  */
+
+static void
+run_copy_tests ()
+{
+  /* Test with a trivial type.  */
+  run_copy_test<int> ();
+
+  /* Test with a non-trivial type.  */
+  foo::n_assign_op_called = 0;
+  run_copy_test<foo> ();
+
+  /* Make sure that for the non-trivial type foo, the assignment operator was
+     called an amount of times that makes sense.  */
+  SELF_CHECK (foo::n_assign_op_called == 16);
+}
+
 } /* namespace array_view_tests */
 } /* namespace selftests */
 
@@ -555,4 +657,6 @@  _initialize_array_view_selftests ()
 {
   selftests::register_test ("array_view",
 			    selftests::array_view_tests::run_tests);
+  selftests::register_test ("array_view-copy",
+			    selftests::array_view_tests::run_copy_tests);
 }
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 7649b029f91..51c9e02cc27 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1344,9 +1344,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;
@@ -1721,13 +1725,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;
@@ -1772,9 +1774,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 edf66559e2d..8dd53b0b232 100644
--- a/gdbsupport/array-view.h
+++ b/gdbsupport/array-view.h
@@ -19,6 +19,7 @@ 
 #define COMMON_ARRAY_VIEW_H
 
 #include "traits.h"
+#include <algorithm>
 #include <type_traits>
 
 /* An array_view is an abstraction that provides a non-owning view
@@ -206,6 +207,23 @@  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_assert (dest.size () == src.size ());
+  if (dest.data () <= src.data ())
+    std::copy(src.begin (), src.end (), dest.begin ());
+  else
+    std::copy_backward (src.begin (), src.end (), dest.end ());
+}
+
 /* 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.  */