From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from barracuda.ebox.ca (barracuda.ebox.ca [96.127.255.19]) by sourceware.org (Postfix) with ESMTPS id 01C97385841D for ; Wed, 1 Dec 2021 16:49:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 01C97385841D X-ASG-Debug-ID: 1638377346-0c856e2e463ea630001-fS2M51 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id GJ0mvMLa0yhZjBt0 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 01 Dec 2021 11:49:06 -0500 (EST) X-Barracuda-Envelope-From: simon.marchi@polymtl.ca X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from simark.localdomain (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) by smtp.ebox.ca (Postfix) with ESMTP id A075C441B21; Wed, 1 Dec 2021 11:49:06 -0500 (EST) From: Simon Marchi X-Barracuda-RBL-IP: 192.222.157.6 X-Barracuda-Effective-Source-IP: 192-222-157-6.qc.cable.ebox.net[192.222.157.6] X-Barracuda-Apparent-Source-IP: 192.222.157.6 To: gdb-patches@sourceware.org Subject: [PATCH v3 1/3] gdbsupport: add array_view copy function Date: Wed, 1 Dec 2021 11:49:03 -0500 X-ASG-Orig-Subj: [PATCH v3 1/3] gdbsupport: add array_view copy function Message-Id: <20211201164905.702081-1-simon.marchi@polymtl.ca> X-Mailer: git-send-email 2.33.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Barracuda-Connect: smtp.ebox.ca[96.127.255.82] X-Barracuda-Start-Time: 1638377346 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at ebox.ca X-Barracuda-Scan-Msg-Size: 19474 X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.00 X-Barracuda-Spam-Status: No, SCORE=0.00 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests= X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.94332 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- X-Spam-Status: No, score=-16.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_QUARANTINE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_SOFTFAIL, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Dec 2021 16:49:11 -0000 New in v3: - Fix stale comment - Change order of parameters - Don't copy if src == dest 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 and std::copy_backward assigns each element in a loop. To properly support overlapping ranges, we must use std::copy or std::copy_backward, depending on whether the destination is before the source or vice-versa. std::copy and std::copy_backward don't support copying exactly overlapping ranges (where the source range is equal to the destination range). But in this case, no copy is needed anyway, so we do nothing. The order of parameters of the new copy function is based on std::copy and std::copy_backward, where the source comes before the destination. 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/unittests/array-view-selftests.c | 104 +++++++++++++++++++++++++++ gdb/valarith.c | 49 +++++++------ gdb/valops.c | 35 ++++----- gdb/value.c | 22 +++--- gdbsupport/array-view.h | 15 ++++ 7 files changed, 184 insertions(+), 62 deletions(-) diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index b84e10fd029f..c6cefe9d8e89 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 (fromval), value_contents_raw (val)); 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 (actual), value_contents_raw (val)); 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 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 (value_contents_all (elt), res_contents.slice (elt_len * i, elt_len)); } return res; diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c index 652161955d52..592dbe19d562 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_all (val).slice (subobj_offset, len), + value_contents_raw (retval)); } break; diff --git a/gdb/unittests/array-view-selftests.c b/gdb/unittests/array-view-selftests.c index 9df48db39129..998bd3a57812 100644 --- a/gdb/unittests/array-view-selftests.c +++ b/gdb/unittests/array-view-selftests.c @@ -546,6 +546,108 @@ run_tests () } } +template +void +run_copy_test () +{ + /* Test non-overlapping copy. */ + { + const std::vector src_v = {1, 2, 3, 4}; + std::vector dest_v (4, -1); + + SELF_CHECK (dest_v != src_v); + copy (gdb::array_view (src_v), gdb::array_view (dest_v)); + SELF_CHECK (dest_v == src_v); + } + + /* Test overlapping copy, where the source is before the destination. */ + { + std::vector vec = {1, 2, 3, 4, 5, 6, 7, 8}; + gdb::array_view v = vec; + + copy (v.slice (1, 4), + v.slice (2, 4)); + + std::vector 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 vec = {1, 2, 3, 4, 5, 6, 7, 8}; + gdb::array_view v = vec; + + copy (v.slice (2, 4), + v.slice (1, 4)); + + std::vector 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 vec = {1, 2, 3, 4, 5, 6, 7, 8}; + gdb::array_view v = vec; + + copy (v.slice (2, 4), + v.slice (2, 4)); + + std::vector 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 and + array_view. */ + 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 (); + + /* Test with a non-trivial type. */ + foo::n_assign_op_called = 0; + run_copy_test (); + + /* 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 == 12); +} + } /* 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 140ef448137e..496570762799 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 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 (value_contents_all (elval), + val_contents.slice (i * elt_len, elt_len)); 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 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 (value_contents_all (tmp), + val_contents.slice (i * elsize, elsize)); } 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 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 (value_contents_all (tmp), + val_contents.slice (i * elt_len, elt_len)); } 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 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 (value_contents_all (tmp), + val_contents.slice (i * elt_len, elt_len)); } } else if (type->code () == TYPE_CODE_COMPLEX) diff --git a/gdb/valops.c b/gdb/valops.c index c552e828a947..ca71c128de9c 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -954,18 +954,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 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 (value_contents_all (tmp), + val_contents.slice (i * elt_len, elt_len)); } } else @@ -1342,8 +1343,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 (fromval), value_contents_raw (val)); /* We copy over the enclosing type and pointed-to offset from FROMVAL in the case of pointer types. For object types, the enclosing type @@ -4058,10 +4058,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 (arg1), + value_contents_raw (val).slice (0, len)); + copy (value_contents (arg2), + value_contents_raw (val).slice (len, len)); + return val; } @@ -4102,12 +4105,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 (val).slice (0, len), + value_contents_raw (re_val)); + copy (value_contents (val).slice (len, len), + value_contents_raw (im_val)); return value_literal_complex (re_val, im_val, type); } diff --git a/gdb/value.c b/gdb/value.c index 7649b029f91b..4e66329e82c5 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 dst_contents + = value_contents_all_raw (dst).slice (dst_offset * unit_size, + length * unit_size); + gdb::array_view src_contents + = value_contents_all_raw (src).slice (src_offset * unit_size, + length * unit_size); + copy (src_contents, dst_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 (arg), + value_contents_all_raw (val)); - } 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 (arg), value_contents_all_raw (val)); 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 edf66559e2d9..2590bc96061c 100644 --- a/gdbsupport/array-view.h +++ b/gdbsupport/array-view.h @@ -19,6 +19,7 @@ #define COMMON_ARRAY_VIEW_H #include "traits.h" +#include #include /* An array_view is an abstraction that provides a non-owning view @@ -206,6 +207,20 @@ 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. */ + +template +void copy (gdb::array_view src, gdb::array_view dest) +{ + gdb_assert (dest.size () == src.size ()); + 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 ()); +} + /* 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.1