From 13221aec0d87c701c463d4fa54aa70096d0f43a7 Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Tue, 22 Jun 2021 10:17:53 +0100 Subject: [PATCH] gdb: replace NULL terminated array with array_view After the previous commit, this commit updates the value_struct_elt function to take an array_view rather than a NULL terminated array of values. The requirement for a NULL terminated array of values actually stems from typecmp, so the change from an array to array_view needs to be propagated through to this function. While making this change I noticed that this fixes another bug, in value_x_binop and value_x_unop GDB creates an array of values which doesn't have a NULL at the end. An array_view of this array is passed to value_user_defined_op, which then unpacks the array_view and passed the raw array to value_struct_elt, but only if the language is not C++. As value_x_binop and value_x_unop can only request member functions with the names of C++ operators, then most of the time, assuming the inferior is not a C++ program, then GDB will not find a matching member function with the call to value_struct_elt, and so typecmp will never be called, and so, GDB will avoid undefined behaviour. However, it is worth remembering that, when GDB's language is set to "auto", the current language is selected based on the language of the current compilation unit. As C++ programs usually link against libc, which is written in C, then, if the inferior is stopped in libc GDB will set the language to C. And so, it is possible that we will end up using value_struct_elt to try and lookup, and match, a C++ operator. If this occurs then GDB will experience undefined behaviour. I have extended the test added in the previous commit to also cover this case. Finally, this commit changes the API from passing around a pointer to an array to passing around a pointer to an array_view. The reason for this is that we need to be able to distinguish between the cases where we call value_struct_elt with no arguments, i.e. we are looking up a struct member, but we either don't have the arguments we want to pass yet, or we don't expect there to be any need for GDB to use the argument types to resolve any overloading; and the second case where we call value_struct_elt looking for a function that takes no arguments, that is, the argument list is empty. NOTE: While writing this I realise that if we pass an array_view at all then it will always have at least one item in it, the `this' pointer for the object we are planning to call the method on. So we could, I guess, pass an empty array_view to indicate the case where we don't know anything about the arguments, and when the array_view is length 1 or more, it means we do have the arguments. However, though we could do this, I don't think this would be better, the length 0 vs length 1 difference seems a little too subtle, I think that there's a better solution... I think a better solution would be to wrap the array_view in a gdb::optional, this would make the whole, do we have an array view or not question explicit. I haven't done this as part of this commit as making that change is much more extensive, every user of value_struct_elt will need to be updated, and as this commit already contains a bug fix, I wanted to keep the large refactoring in a separate commit, so, check out the next commit for the use of gdb::optional. gdb/ChangeLog: PR gdb/27994 * eval.c (structop_base_operation::evaluate_funcall): Pass array_view instead of array to value_struct_elt. * valarith.c (value_user_defined_op): Likewise. * valops.c (typecmp): Change parameter type from array pointer to array_view. Update header comment, and update body accordingly. (search_struct_method): Likewise. (value_struct_elt): Likewise. * value.h (value_struct_elt): Update declaration. gdb/testsuite/ChangeLog: PR gdb/27994 * gdb.cp/method-call-in-c.cc (struct foo_type): Add operator+=, change initial value of var member variable. (main): Make use of foo_type's operator+=. * gdb.cp/method-call-in-c.exp: Test use of operator+=. --- gdb/ChangeLog | 12 +++++ gdb/eval.c | 19 ++++---- gdb/testsuite/ChangeLog | 8 ++++ gdb/testsuite/gdb.cp/method-call-in-c.cc | 10 +++- gdb/testsuite/gdb.cp/method-call-in-c.exp | 4 ++ gdb/valarith.c | 2 +- gdb/valops.c | 58 +++++++++++------------ gdb/value.h | 2 +- 8 files changed, 70 insertions(+), 45 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 04ea77f06e2..9396aadca9d 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,15 @@ +2021-06-25 Andrew Burgess + + PR gdb/27994 + * eval.c (structop_base_operation::evaluate_funcall): Pass + array_view instead of array to value_struct_elt. + * valarith.c (value_user_defined_op): Likewise. + * valops.c (typecmp): Change parameter type from array pointer to + array_view. Update header comment, and update body accordingly. + (search_struct_method): Likewise. + (value_struct_elt): Likewise. + * value.h (value_struct_elt): Update declaration. + 2021-06-25 Andrew Burgess PR gdb/27994 diff --git a/gdb/eval.c b/gdb/eval.c index ab070a3d9f6..5a72bf1becb 100644 --- a/gdb/eval.c +++ b/gdb/eval.c @@ -872,9 +872,9 @@ structop_base_operation::evaluate_funcall (struct type *expect_type, struct expression *exp, enum noside noside, const std::vector &args) { - /* Allocate space for the function call arguments. Include space for a - `this' pointer at the start, and a trailing nullptr. */ - std::vector vals (args.size () + 2); + /* Allocate space for the function call arguments, Including space for a + `this' pointer at the start. */ + std::vector vals (args.size () + 1); /* First, evaluate the structure into vals[0]. */ enum exp_opcode op = opcode (); if (op == STRUCTOP_STRUCT) @@ -920,16 +920,13 @@ structop_base_operation::evaluate_funcall } } - /* Evaluate the arguments, and add the trailing nullptr. The '+ 1' here - is to allow for the `this' pointer we placed into vals[0]. */ + /* Evaluate the arguments. The '+ 1' here is to allow for the `this' + pointer we placed into vals[0]. */ for (int i = 0; i < args.size (); ++i) vals[i + 1] = args[i]->evaluate_with_coercion (exp, noside); - vals[args.size () + 1] = nullptr; - /* The array view includes the `this' pointer, but not the trailing - nullptr. */ - gdb::array_view arg_view - = gdb::make_array_view (&vals[0], args.size () + 1); + /* The array view includes the `this' pointer. */ + gdb::array_view arg_view (vals); int static_memfuncp; value *callee; @@ -950,7 +947,7 @@ structop_base_operation::evaluate_funcall { struct value *temp = vals[0]; - callee = value_struct_elt (&temp, &vals[0], tstr, + callee = value_struct_elt (&temp, &arg_view, tstr, &static_memfuncp, op == STRUCTOP_STRUCT ? "structure" : "structure pointer"); diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 878921d0932..b155c71f7ce 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,11 @@ +2021-06-25 Andrew Burgess + + PR gdb/27994 + * gdb.cp/method-call-in-c.cc (struct foo_type): Add operator+=, + change initial value of var member variable. + (main): Make use of foo_type's operator+=. + * gdb.cp/method-call-in-c.exp: Test use of operator+=. + 2021-06-25 Andrew Burgess PR gdb/27994 diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.cc b/gdb/testsuite/gdb.cp/method-call-in-c.cc index 09e4285ed5d..95f3f3c22de 100644 --- a/gdb/testsuite/gdb.cp/method-call-in-c.cc +++ b/gdb/testsuite/gdb.cp/method-call-in-c.cc @@ -29,7 +29,13 @@ struct foo_type return var++; } - int var = 123; + foo_type &operator+= (const baz_type &rhs) + { + var += (rhs.a + rhs.b + rhs.c); + return *this; + } + + int var = 120; }; int @@ -40,5 +46,7 @@ main (void) foo_type foo; + foo += b; + return foo.func (b, f); /* Break here. */ } diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.exp b/gdb/testsuite/gdb.cp/method-call-in-c.exp index 0e6851b3478..411ba6790b1 100644 --- a/gdb/testsuite/gdb.cp/method-call-in-c.exp +++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp @@ -39,5 +39,9 @@ foreach_with_prefix lang { c++ c } { gdb_test "print foo.func (b, f)" " = ${result}" incr result + + set result [expr $result + 3] + gdb_test "print foo += b" \ + " = \\((?:struct )?foo_type &\\) @${hex}: \\\{var = ${result}\\\}" } } diff --git a/gdb/valarith.c b/gdb/valarith.c index 299a99f4703..d61ad9170f8 100644 --- a/gdb/valarith.c +++ b/gdb/valarith.c @@ -344,7 +344,7 @@ value_user_defined_op (struct value **argp, gdb::array_view args, noside); } else - result = value_struct_elt (argp, args.data (), name, static_memfuncp, + result = value_struct_elt (argp, &args, name, static_memfuncp, "structure"); return result; diff --git a/gdb/valops.c b/gdb/valops.c index 2b579304204..0af7a6c3f27 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -44,14 +44,14 @@ /* Local functions. */ -static int typecmp (int staticp, int varargs, int nargs, - struct field t1[], struct value *t2[]); +static int typecmp (bool staticp, bool varargs, int nargs, + struct field t1[], const gdb::array_view t2); static struct value *search_struct_field (const char *, struct value *, struct type *, int); static struct value *search_struct_method (const char *, struct value **, - struct value **, + gdb::array_view *, LONGEST, int *, struct type *); static int find_oload_champ_namespace (gdb::array_view args, @@ -1785,15 +1785,15 @@ value_string (const char *ptr, ssize_t len, struct type *char_type) } -/* See if we can pass arguments in T2 to a function which takes - arguments of types T1. T1 is a list of NARGS arguments, and T2 is - a NULL-terminated vector. If some arguments need coercion of some - sort, then the coerced values are written into T2. Return value is +/* See if we can pass arguments in T2 to a function which takes arguments + of types T1. T1 is a list of NARGS arguments, and T2 is an array_view + of the values we're trying to pass. If some arguments need coercion of + some sort, then the coerced values are written into T2. Return value is 0 if the arguments could be matched, or the position at which they differ if not. STATICP is nonzero if the T1 argument list came from a static - member function. T2 will still include the ``this'' pointer, but + member function. T2 must still include the ``this'' pointer, but it will be skipped. For non-static member functions, we ignore the first argument, @@ -1803,19 +1803,15 @@ value_string (const char *ptr, ssize_t len, struct type *char_type) requested operation is type secure, shouldn't we? FIXME. */ static int -typecmp (int staticp, int varargs, int nargs, - struct field t1[], struct value *t2[]) +typecmp (bool staticp, bool varargs, int nargs, + struct field t1[], gdb::array_view t2) { int i; - if (t2 == 0) - internal_error (__FILE__, __LINE__, - _("typecmp: no argument list")); - /* Skip ``this'' argument if applicable. T2 will always include THIS. */ if (staticp) - t2 ++; + t2 = t2.slice (1); for (i = 0; (i < nargs) && t1[i].type ()->code () != TYPE_CODE_VOID; @@ -1823,7 +1819,7 @@ typecmp (int staticp, int varargs, int nargs, { struct type *tt1, *tt2; - if (!t2[i]) + if (i == t2.size ()) return i + 1; tt1 = check_typedef (t1[i].type ()); @@ -1868,7 +1864,7 @@ typecmp (int staticp, int varargs, int nargs, if (t1[i].type ()->code () != value_type (t2[i])->code ()) return i + 1; } - if (varargs || t2[i] == NULL) + if (varargs || i == t2.size ()) return 0; return i + 1; } @@ -2181,16 +2177,16 @@ search_struct_field (const char *name, struct value *arg1, ARG1 by OFFSET bytes, and search in it assuming it has (class) type TYPE. - The ARGS array is a list of argument values used to help finding NAME, - though ARGS can be nullptr. If ARGS is not nullptr then the list itself - must have a NULL at the end. + The ARGS array pointer is to a list of argument values used to help + finding NAME, though ARGS can be nullptr. The contents of ARGS can be + adjusted if type coercion is required in order to find a matching NAME. If found, return value, else if name matched and args not return (value) -1, else return NULL. */ static struct value * search_struct_method (const char *name, struct value **arg1p, - struct value **args, LONGEST offset, + gdb::array_view *args, LONGEST offset, int *static_memfuncp, struct type *type) { int i; @@ -2209,10 +2205,10 @@ search_struct_method (const char *name, struct value **arg1p, name_matched = 1; check_stub_method_group (type, i); - if (j > 0 && args == 0) + if (j > 0 && args == nullptr) error (_("cannot resolve overloaded method " "`%s': no arguments supplied"), name); - else if (j == 0 && args == 0) + else if (j == 0 && args == nullptr) { v = value_fn_field (arg1p, f, j, type, offset); if (v != NULL) @@ -2221,10 +2217,11 @@ search_struct_method (const char *name, struct value **arg1p, else while (j >= 0) { + gdb_assert (args != nullptr); if (!typecmp (TYPE_FN_FIELD_STATIC_P (f, j), TYPE_FN_FIELD_TYPE (f, j)->has_varargs (), TYPE_FN_FIELD_TYPE (f, j)->num_fields (), - TYPE_FN_FIELD_ARGS (f, j), args)) + TYPE_FN_FIELD_ARGS (f, j), *args)) { if (TYPE_FN_FIELD_VIRTUAL_P (f, j)) return value_virtual_fn_field (arg1p, f, j, @@ -2313,8 +2310,7 @@ search_struct_method (const char *name, struct value **arg1p, ERR is used in the error message if *ARGP's type is wrong. C++: ARGS is a list of argument types to aid in the selection of - an appropriate method. Also, handle derived types. The array ARGS must - have a NULL at the end. + an appropriate method. Also, handle derived types. STATIC_MEMFUNCP, if non-NULL, points to a caller-supplied location where the truthvalue of whether the function that was resolved was @@ -2324,7 +2320,7 @@ search_struct_method (const char *name, struct value **arg1p, found. */ struct value * -value_struct_elt (struct value **argp, struct value **args, +value_struct_elt (struct value **argp, gdb::array_view *args, const char *name, int *static_memfuncp, const char *err) { struct type *t; @@ -2354,7 +2350,7 @@ value_struct_elt (struct value **argp, struct value **args, if (static_memfuncp) *static_memfuncp = 0; - if (!args) + if (args == nullptr) { /* if there are no arguments ...do this... */ @@ -2366,7 +2362,7 @@ value_struct_elt (struct value **argp, struct value **args, /* C++: If it was not found as a data field, then try to return it as a pointer to a method. */ - v = search_struct_method (name, argp, args, 0, + v = search_struct_method (name, argp, args, 0, static_memfuncp, t); if (v == (struct value *) - 1) @@ -2381,9 +2377,9 @@ value_struct_elt (struct value **argp, struct value **args, return v; } - v = search_struct_method (name, argp, args, 0, + v = search_struct_method (name, argp, args, 0, static_memfuncp, t); - + if (v == (struct value *) - 1) { error (_("One of the arguments you tried to pass to %s could not " diff --git a/gdb/value.h b/gdb/value.h index a691f3cf3ff..40ad28e3dbb 100644 --- a/gdb/value.h +++ b/gdb/value.h @@ -826,7 +826,7 @@ extern struct value *value_neg (struct value *arg1); extern struct value *value_complement (struct value *arg1); extern struct value *value_struct_elt (struct value **argp, - struct value **args, + gdb::array_view *args, const char *name, int *static_memfuncp, const char *err); -- 2.30.2