[PATCH] gdb: fix regression in evaluate_funcall for non C++ like cases

[PATCH] gdb: fix regression in evaluate_funcall for non C++ like cases

[Andrew Burgess]

This regression, as it is exposed by the test added in this commit,
first became noticable with this commit:

  commit d182f2797922a305fbd1ef6a483cc39a56b43e02
  Date:   Mon Mar 8 07:27:57 2021 -0700

      Convert c-exp.y to use operations

But, this commit only added converted the C expression parser to make
use of code that was added in this commit:

  commit a00b7254fb614af557de7ae7cc0eb39a0ce0e408
  Date:   Mon Mar 8 07:27:57 2021 -0700

      Implement function call operations

And it was this second commit that actually introduced the bugs (there
are two).

In structop_base_operation::evaluate_funcall we build up an argument
list in the vector vals.  Later in this function the argument list
might be passed to value_struct_elt.

Prior to commit a00b7254fb614 the vals vector (or argvec as it used to
be called) stored the value for the function callee in the argvec at
index 0.  After commit a00b7254fb614 the callee value is now held in a
separate variable.

What this means is that previous, when we called value_struct_elt we
would pass the address of argvec[1] as this was the first argument.
But now we should be passing the address of vals[0].  Unfortunately,
we are still passing vals[1], effectively skipping the first
argument.

The second issue is that, prior to commit a00b7254fb614, the argvec
array was NULL terminated.  This is required as value_struct_elt
calls search_struct_method, which calls typecmp, and typecmp requires
that the array have a NULL at the end.

After commit a00b7254fb614 this NULL has been lost, and we are
therefore violating the API requirements of typecmp.

This commit fixes both of these regressions.  I also extended the
header comments on search_struct_method and value_struct_elt to make
it clearer that the array required a NULL marker at the end.

gdb/ChangeLog:

	PR gdb/27994
	* eval.c (structop_base_operation::evaluate_funcall): Add a
	nullptr to the end of the args array, which should not be included
	in the argument array_view.  Pass all the arguments through to
	value_struct_elt.
	* valops.c (search_struct_method): Update header comment.
	(value_struct_elt): Likewise.

gdb/testsuite/ChangeLog:

	PR gdb/27994
	* gdb.cp/method-call-in-c.cc: New file.
	* gdb.cp/method-call-in-c.exp: New file.
---
 gdb/ChangeLog                             | 10 ++++++
 gdb/eval.c                                | 15 ++++++--
 gdb/testsuite/ChangeLog                   |  6 ++++
 gdb/testsuite/gdb.cp/method-call-in-c.cc  | 44 +++++++++++++++++++++++
 gdb/testsuite/gdb.cp/method-call-in-c.exp | 44 +++++++++++++++++++++++
 gdb/valops.c                              |  7 +++-
 6 files changed, 122 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.cc
 create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.exp

diff --git a/gdb/eval.c b/gdb/eval.c
index 659493c8237..ab070a3d9f6 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -872,7 +872,9 @@ structop_base_operation::evaluate_funcall
      (struct type *expect_type, struct expression *exp, enum noside noside,
       const std::vector<operation_up> &args)
 {
-  std::vector<value *> vals (args.size () + 1);
+  /* Allocate space for the function call arguments.  Include space for a
+     `this' pointer at the start, and a trailing nullptr.  */
+  std::vector<value *> vals (args.size () + 2);
   /* First, evaluate the structure into vals[0].  */
   enum exp_opcode op = opcode ();
   if (op == STRUCTOP_STRUCT)
@@ -918,9 +920,16 @@ 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].  */
   for (int i = 0; i < args.size (); ++i)
     vals[i + 1] = args[i]->evaluate_with_coercion (exp, noside);
-  gdb::array_view<value *> arg_view = vals;
+  vals[args.size () + 1] = nullptr;
+
+  /* The array view includes the `this' pointer, but not the trailing
+     nullptr.  */
+  gdb::array_view<value *> arg_view
+    = gdb::make_array_view (&vals[0], args.size () + 1);
 
   int static_memfuncp;
   value *callee;
@@ -941,7 +950,7 @@ structop_base_operation::evaluate_funcall
     {
       struct value *temp = vals[0];
 
-      callee = value_struct_elt (&temp, &vals[1], tstr,
+      callee = value_struct_elt (&temp, &vals[0], tstr,
 				 &static_memfuncp,
 				 op == STRUCTOP_STRUCT
 				 ? "structure" : "structure pointer");
diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.cc b/gdb/testsuite/gdb.cp/method-call-in-c.cc
new file mode 100644
index 00000000000..b84a6890cdf
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.cc
@@ -0,0 +1,44 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+struct baz_type
+{
+  int a = 0;
+  int b = 1;
+  int c = 2;
+};
+
+struct foo_type
+{
+  int func (baz_type b, float f)
+  {
+    return var++;
+  }
+
+  int var = 0;
+};
+
+int
+main (void)
+{
+  baz_type b = {};
+  float f = 1.0;
+
+  foo_type foo;
+
+  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
new file mode 100644
index 00000000000..e0dd8e94ae7
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp
@@ -0,0 +1,44 @@
+# Copyright 2021 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Ensure that calling a member function works correctly even when the
+# language is forced to 'C' (this should be fine, so long at no
+# overload resolution is required), or when overload-resolution is
+# off.
+
+standard_testfile .cc
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+if ![runto_main] then {
+    return 0
+}
+
+gdb_breakpoint [gdb_get_line_number "Break here"]
+gdb_continue_to_breakpoint "Break here"
+
+set result 0
+foreach_with_prefix lang { c++ c } {
+    foreach_with_prefix overload_resolution { on off } {
+	gdb_test_no_output "set overload-resolution ${overload_resolution}"
+	gdb_test "set language ${lang}" ".*"
+
+	gdb_test "print foo.func (b, f)" " = ${result}" \
+	    "call foo.func with language auto;c++, overload-resolution on"
+	incr result
+    }
+}
diff --git a/gdb/valops.c b/gdb/valops.c
index 8694c124b52..2b579304204 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -2181,6 +2181,10 @@ 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.
+
    If found, return value, else if name matched and args not return
    (value) -1, else return NULL.  */
 
@@ -2309,7 +2313,8 @@ 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.
+   an appropriate method.  Also, handle derived types.  The array ARGS must
+   have a NULL at the end.
 
    STATIC_MEMFUNCP, if non-NULL, points to a caller-supplied location
    where the truthvalue of whether the function that was resolved was
-- 
2.25.4

Re: [PATCH] gdb: fix regression in evaluate_funcall for non C++ like cases

[Simon Marchi]

On 2021-06-21 7:39 p.m., Andrew Burgess wrote:
> This regression, as it is exposed by the test added in this commit,
> first became noticable with this commit:
> 
>   commit d182f2797922a305fbd1ef6a483cc39a56b43e02
>   Date:   Mon Mar 8 07:27:57 2021 -0700
> 
>       Convert c-exp.y to use operations
> 
> But, this commit only added converted the C expression parser to make
> use of code that was added in this commit:
> 
>   commit a00b7254fb614af557de7ae7cc0eb39a0ce0e408
>   Date:   Mon Mar 8 07:27:57 2021 -0700
> 
>       Implement function call operations
> 
> And it was this second commit that actually introduced the bugs (there
> are two).
> 
> In structop_base_operation::evaluate_funcall we build up an argument
> list in the vector vals.  Later in this function the argument list
> might be passed to value_struct_elt.
> 
> Prior to commit a00b7254fb614 the vals vector (or argvec as it used to
> be called) stored the value for the function callee in the argvec at

Do you mean the "this" value?  It's not clear which value "value" refers
to.

> index 0.  After commit a00b7254fb614 the callee value is now held in a
> separate variable.
> 
> What this means is that previous, when we called value_struct_elt we
> would pass the address of argvec[1] as this was the first argument.
> But now we should be passing the address of vals[0].  Unfortunately,
> we are still passing vals[1], effectively skipping the first
> argument.
> 
> The second issue is that, prior to commit a00b7254fb614, the argvec
> array was NULL terminated.  This is required as value_struct_elt
> calls search_struct_method, which calls typecmp, and typecmp requires
> that the array have a NULL at the end.
> 
> After commit a00b7254fb614 this NULL has been lost, and we are
> therefore violating the API requirements of typecmp.
> 
> This commit fixes both of these regressions.  I also extended the
> header comments on search_struct_method and value_struct_elt to make
> it clearer that the array required a NULL marker at the end.

I'm a little troubled by what you said in the bug, that the language is
set to C, because we are stopped in libc.  I understand why it's set to
C.  But the fact that calling a method works anyway _and_ the behavior
is different than what you'd get if you were stopped anywhere else in
GDB and the language was C++ (w.r.t overload resolution), that's a bit
scary.  It means my "print current_inferior_..." command could have
different results depending on where I'm stopped.

I don't have any idea for a better design, I just never realized this
before.

In the test, I'd suggest starting the expect value at something else
than 0, since 0 is a bit more likely to happen "by chance".  Maybe start
at a completely arbitrary value like 123.  But otherwise, the patch
LGTM, thanks.

Simon

[PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases

[Andrew Burgess]

Changes since v1:

  - Updated the commit message for patch #1 after Simon's query,
    hopefully this should make it clearer what I'm talking about.

  - Passing a NULL terminated array around sucks now we have
    array_view, patch #2 changes the API to use array_view, which
    exposed another bug in this area (which is fixed in this patch).

  - After this discussion in the bug (gdb/27994), patch #3 updates the
    API even further, we now make use of gdb::optional instead of
    passing a pointer to an array_view.

---

Andrew Burgess (3):
  gdb: fix regression in evaluate_funcall for non C++ like cases
  gdb: replace NULL terminated array with array_view
  gdb: use gdb::optional instead of passing a pointer to gdb::array_view

 gdb/ChangeLog                             | 51 ++++++++++++++++++++
 gdb/ada-lang.c                            |  6 +--
 gdb/eval.c                                | 19 ++++++--
 gdb/f-lang.c                              |  2 +-
 gdb/guile/scm-value.c                     |  2 +-
 gdb/m2-lang.c                             |  4 +-
 gdb/opencl-lang.c                         |  2 +-
 gdb/python/py-value.c                     |  2 +-
 gdb/rust-lang.c                           | 18 +++----
 gdb/testsuite/ChangeLog                   | 14 ++++++
 gdb/testsuite/gdb.cp/method-call-in-c.cc  | 52 +++++++++++++++++++++
 gdb/testsuite/gdb.cp/method-call-in-c.exp | 48 +++++++++++++++++++
 gdb/valarith.c                            |  2 +-
 gdb/valops.c                              | 57 ++++++++++++-----------
 gdb/value.h                               |  2 +-
 15 files changed, 229 insertions(+), 52 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.cc
 create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.exp

-- 
2.25.4

[PATCHv2 1/3] gdb: fix regression in evaluate_funcall for non C++ like cases

[Andrew Burgess]

This regression, as it is exposed by the test added in this commit,
first became noticable with this commit:

  commit d182f2797922a305fbd1ef6a483cc39a56b43e02
  Date:   Mon Mar 8 07:27:57 2021 -0700

      Convert c-exp.y to use operations

But, this commit only added converted the C expression parser to make
use of code that was added in this commit:

  commit a00b7254fb614af557de7ae7cc0eb39a0ce0e408
  Date:   Mon Mar 8 07:27:57 2021 -0700

      Implement function call operations

And it was this second commit that actually introduced the bugs (there
are two).

In structop_base_operation::evaluate_funcall we build up an argument
list in the vector vals.  Later in this function the argument list
might be passed to value_struct_elt.

Prior to commit a00b7254fb614 the vals vector (or argvec as it used to
be called) stored the value for the function callee in the argvec at
index 0.  This 'callee' value is what ends up being passed to
evaluate_subexp_do_call, and represents the function to be called, the
value contents are the address of the function, and the value type is
the function signature.  The remaining items held in the argvec were
the values to pass to the function.  For a non-static member function
the `this' pointer would be at index 1 in the array.

After commit a00b7254fb614 this callee value is now held in a separate
variable, not the vals array.  So, for non-static member functions,
the `this' pointer is now at index 0, with any other arguments after
that.

What this means is that previous, when we called value_struct_elt we
would pass the address of argvec[1] as this was the first argument.
But now we should be passing the address of vals[0].  Unfortunately,
we are still passing vals[1], effectively skipping the first
argument.

The second issue is that, prior to commit a00b7254fb614, the argvec
array was NULL terminated.  This is required as value_struct_elt
calls search_struct_method, which calls typecmp, and typecmp requires
that the array have a NULL at the end.

After commit a00b7254fb614 this NULL has been lost, and we are
therefore violating the API requirements of typecmp.

This commit fixes both of these regressions.  I also extended the
header comments on search_struct_method and value_struct_elt to make
it clearer that the array required a NULL marker at the end.

gdb/ChangeLog:

	PR gdb/27994
	* eval.c (structop_base_operation::evaluate_funcall): Add a
	nullptr to the end of the args array, which should not be included
	in the argument array_view.  Pass all the arguments through to
	value_struct_elt.
	* valops.c (search_struct_method): Update header comment.
	(value_struct_elt): Likewise.

gdb/testsuite/ChangeLog:

	PR gdb/27994
	* gdb.cp/method-call-in-c.cc: New file.
	* gdb.cp/method-call-in-c.exp: New file.
---
 gdb/ChangeLog                             | 10 ++++++
 gdb/eval.c                                | 15 ++++++--
 gdb/testsuite/ChangeLog                   |  6 ++++
 gdb/testsuite/gdb.cp/method-call-in-c.cc  | 44 +++++++++++++++++++++++
 gdb/testsuite/gdb.cp/method-call-in-c.exp | 44 +++++++++++++++++++++++
 gdb/valops.c                              |  7 +++-
 6 files changed, 122 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.cc
 create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.exp

diff --git a/gdb/eval.c b/gdb/eval.c
index 659493c8237..ab070a3d9f6 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -872,7 +872,9 @@ structop_base_operation::evaluate_funcall
      (struct type *expect_type, struct expression *exp, enum noside noside,
       const std::vector<operation_up> &args)
 {
-  std::vector<value *> vals (args.size () + 1);
+  /* Allocate space for the function call arguments.  Include space for a
+     `this' pointer at the start, and a trailing nullptr.  */
+  std::vector<value *> vals (args.size () + 2);
   /* First, evaluate the structure into vals[0].  */
   enum exp_opcode op = opcode ();
   if (op == STRUCTOP_STRUCT)
@@ -918,9 +920,16 @@ 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].  */
   for (int i = 0; i < args.size (); ++i)
     vals[i + 1] = args[i]->evaluate_with_coercion (exp, noside);
-  gdb::array_view<value *> arg_view = vals;
+  vals[args.size () + 1] = nullptr;
+
+  /* The array view includes the `this' pointer, but not the trailing
+     nullptr.  */
+  gdb::array_view<value *> arg_view
+    = gdb::make_array_view (&vals[0], args.size () + 1);
 
   int static_memfuncp;
   value *callee;
@@ -941,7 +950,7 @@ structop_base_operation::evaluate_funcall
     {
       struct value *temp = vals[0];
 
-      callee = value_struct_elt (&temp, &vals[1], tstr,
+      callee = value_struct_elt (&temp, &vals[0], tstr,
 				 &static_memfuncp,
 				 op == STRUCTOP_STRUCT
 				 ? "structure" : "structure pointer");
diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.cc b/gdb/testsuite/gdb.cp/method-call-in-c.cc
new file mode 100644
index 00000000000..09e4285ed5d
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.cc
@@ -0,0 +1,44 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+struct baz_type
+{
+  int a = 0;
+  int b = 1;
+  int c = 2;
+};
+
+struct foo_type
+{
+  int func (baz_type b, float f)
+  {
+    return var++;
+  }
+
+  int var = 123;
+};
+
+int
+main (void)
+{
+  baz_type b = {};
+  float f = 1.0;
+
+  foo_type foo;
+
+  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
new file mode 100644
index 00000000000..eb48979e465
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp
@@ -0,0 +1,44 @@
+# Copyright 2021 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Ensure that calling a member function works correctly even when the
+# language is forced to 'C' (this should be fine, so long at no
+# overload resolution is required), or when overload-resolution is
+# off.
+
+standard_testfile .cc
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+if ![runto_main] then {
+    return 0
+}
+
+gdb_breakpoint [gdb_get_line_number "Break here"]
+gdb_continue_to_breakpoint "Break here"
+
+set result 123
+foreach_with_prefix lang { c++ c } {
+    foreach_with_prefix overload_resolution { on off } {
+	gdb_test_no_output "set overload-resolution ${overload_resolution}"
+	gdb_test "set language ${lang}" ".*"
+
+	gdb_test "print foo.func (b, f)" " = ${result}" \
+	    "call foo.func with language auto;c++, overload-resolution on"
+	incr result
+    }
+}
diff --git a/gdb/valops.c b/gdb/valops.c
index 8694c124b52..2b579304204 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -2181,6 +2181,10 @@ 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.
+
    If found, return value, else if name matched and args not return
    (value) -1, else return NULL.  */
 
@@ -2309,7 +2313,8 @@ 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.
+   an appropriate method.  Also, handle derived types.  The array ARGS must
+   have a NULL at the end.
 
    STATIC_MEMFUNCP, if non-NULL, points to a caller-supplied location
    where the truthvalue of whether the function that was resolved was
-- 
2.25.4

[PATCHv2 2/3] gdb: replace NULL terminated array with array_view

[Andrew Burgess]

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                                |  2 +-
 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, 63 insertions(+), 35 deletions(-)

diff --git a/gdb/eval.c b/gdb/eval.c
index ab070a3d9f6..6bc4132288b 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -950,7 +950,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/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 eb48979e465..066e3648382 100644
--- a/gdb/testsuite/gdb.cp/method-call-in-c.exp
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp
@@ -40,5 +40,9 @@ foreach_with_prefix lang { c++ c } {
 	gdb_test "print foo.func (b, f)" " = ${result}" \
 	    "call foo.func with language auto;c++, overload-resolution on"
 	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<value *> 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<value *> 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<value *> *,
 					   LONGEST, int *, struct type *);
 
 static int find_oload_champ_namespace (gdb::array_view<value *> args,
@@ -1785,15 +1785,15 @@ value_string (const char *ptr, ssize_t len, struct type *char_type)
 }
 
 \f
-/* 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<value *> 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<value *> *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<value *> *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 <value *> *args,
 				       const char *name, int *static_memfuncp,
 				       const char *err);
 
-- 
2.25.4

[PATCHv2 3/3] gdb: use gdb::optional instead of passing a pointer to gdb::array_view

[Andrew Burgess]

Following on from the previous commit, this commit changes the API of
value_struct_elt to take gdb::optional<gdb::array_view<value *>>
instead of a pointer to the gdb::array_view.

This makes the optional nature of the array_view parameter explicit.

This commit is purely a refactoring commit, there should be no user
visible change after this commit.

I have deliberately kept this refactor separate from the previous two
commits as this is a more extensive change, and I'm not 100% sure that
using gdb::optional for the parameter type, instead of a pointer, is
going to be to everyone's taste.  If there's push back on this patch
then this one can be dropped from the series.

gdb/ChangeLog:

	* ada-lang.c (desc_bounds): Use '{}' instead of NULL to indicate
	an empty gdb::optional when calling value_struct_elt.
	(desc_data): Likewise.
	(desc_one_bound): Likewise.
	* eval.c (structop_base_operation::evaluate_funcall): Pass
	gdb::array_view, not a gdb::array_view* to value_struct_elt.
	(eval_op_structop_struct): Use '{}' instead of NULL to indicate
	an empty gdb::optional when calling value_struct_elt.
	(eval_op_structop_ptr): Likewise.
	* f-lang.c (fortran_structop_operation::evaluate): Likewise.
	* guile/scm-value.c (gdbscm_value_field): Likewise.
	* m2-lang.c (eval_op_m2_high): Likewise.
	(eval_op_m2_subscript): Likewise.
	* opencl-lang.c (opencl_structop_operation::evaluate): Likewise.
	* python/py-value.c (valpy_getitem): Likewise.
	* rust-lang.c (rust_val_print_str): Likewise.
	(rust_range): Likewise.
	(rust_subscript): Likewise.
	(eval_op_rust_structop): Likewise.
	(rust_aggregate_operation::evaluate): Likewise.
	* valarith.c (value_user_defined_op): Likewise.
	* valops.c (search_struct_method): Change parameter type, update
	function body accordingly, and update header comment.
	(value_struct_elt): Change parameter type, update function body
	accordingly.
	* value.h (value_struct_elt): Update declaration.
---
 gdb/ChangeLog         | 29 +++++++++++++++++++++++++++++
 gdb/ada-lang.c        |  6 +++---
 gdb/eval.c            |  6 +++---
 gdb/f-lang.c          |  2 +-
 gdb/guile/scm-value.c |  2 +-
 gdb/m2-lang.c         |  4 ++--
 gdb/opencl-lang.c     |  2 +-
 gdb/python/py-value.c |  2 +-
 gdb/rust-lang.c       | 18 +++++++++---------
 gdb/valarith.c        |  2 +-
 gdb/valops.c          | 24 +++++++++++++-----------
 gdb/value.h           |  2 +-
 12 files changed, 65 insertions(+), 34 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 6ed6b65e705..dd9e1354617 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -1484,7 +1484,7 @@ desc_bounds (struct value *arr)
 
   else if (is_thick_pntr (type))
     {
-      struct value *p_bounds = value_struct_elt (&arr, NULL, "P_BOUNDS", NULL,
+      struct value *p_bounds = value_struct_elt (&arr, {}, "P_BOUNDS", NULL,
 					       _("Bad GNAT array descriptor"));
       struct type *p_bounds_type = value_type (p_bounds);
 
@@ -1566,7 +1566,7 @@ desc_data (struct value *arr)
   if (is_thin_pntr (type))
     return thin_data_pntr (arr);
   else if (is_thick_pntr (type))
-    return value_struct_elt (&arr, NULL, "P_ARRAY", NULL,
+    return value_struct_elt (&arr, {}, "P_ARRAY", NULL,
 			     _("Bad GNAT array descriptor"));
   else
     return NULL;
@@ -1606,7 +1606,7 @@ desc_one_bound (struct value *bounds, int i, int which)
   char bound_name[20];
   xsnprintf (bound_name, sizeof (bound_name), "%cB%d",
 	     which ? 'U' : 'L', i - 1);
-  return value_struct_elt (&bounds, NULL, bound_name, NULL,
+  return value_struct_elt (&bounds, {}, bound_name, NULL,
 			   _("Bad GNAT array descriptor bounds"));
 }
 
diff --git a/gdb/eval.c b/gdb/eval.c
index 6bc4132288b..ca13e4cd188 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -950,7 +950,7 @@ structop_base_operation::evaluate_funcall
     {
       struct value *temp = vals[0];
 
-      callee = value_struct_elt (&temp, &arg_view, tstr,
+      callee = value_struct_elt (&temp, arg_view, tstr,
 				 &static_memfuncp,
 				 op == STRUCTOP_STRUCT
 				 ? "structure" : "structure pointer");
@@ -1135,7 +1135,7 @@ eval_op_structop_struct (struct type *expect_type, struct expression *exp,
 			 enum noside noside,
 			 struct value *arg1, const char *string)
 {
-  struct value *arg3 = value_struct_elt (&arg1, NULL, string,
+  struct value *arg3 = value_struct_elt (&arg1, {}, string,
 					 NULL, "structure");
   if (noside == EVAL_AVOID_SIDE_EFFECTS)
     arg3 = value_zero (value_type (arg3), VALUE_LVAL (arg3));
@@ -1191,7 +1191,7 @@ eval_op_structop_ptr (struct type *expect_type, struct expression *exp,
       }
   }
 
-  struct value *arg3 = value_struct_elt (&arg1, NULL, string,
+  struct value *arg3 = value_struct_elt (&arg1, {}, string,
 					 NULL, "structure pointer");
   if (noside == EVAL_AVOID_SIDE_EFFECTS)
     arg3 = value_zero (value_type (arg3), VALUE_LVAL (arg3));
diff --git a/gdb/f-lang.c b/gdb/f-lang.c
index 5731c400305..16ec9e04044 100644
--- a/gdb/f-lang.c
+++ b/gdb/f-lang.c
@@ -1511,7 +1511,7 @@ fortran_structop_operation::evaluate (struct type *expect_type,
 	arg1 = std::get<0> (m_storage)->evaluate (nullptr, exp, EVAL_NORMAL);
     }
 
-  value *elt = value_struct_elt (&arg1, NULL, str, NULL, "structure");
+  value *elt = value_struct_elt (&arg1, {}, str, NULL, "structure");
 
   if (noside == EVAL_AVOID_SIDE_EFFECTS)
     {
diff --git a/gdb/guile/scm-value.c b/gdb/guile/scm-value.c
index 24bb5547957..d4d76df0411 100644
--- a/gdb/guile/scm-value.c
+++ b/gdb/guile/scm-value.c
@@ -694,7 +694,7 @@ gdbscm_value_field (SCM self, SCM field_scm)
 
       struct value *tmp = v_smob->value;
 
-      struct value *res_val = value_struct_elt (&tmp, NULL, field.get (), NULL,
+      struct value *res_val = value_struct_elt (&tmp, {}, field.get (), NULL,
 						"struct/class/union");
 
       return vlscm_scm_from_value (res_val);
diff --git a/gdb/m2-lang.c b/gdb/m2-lang.c
index be1a8ed1437..911d67d8672 100644
--- a/gdb/m2-lang.c
+++ b/gdb/m2-lang.c
@@ -50,7 +50,7 @@ eval_op_m2_high (struct type *expect_type, struct expression *exp,
 
 	  type = type->field (1).type ();
 	  /* i18n: Do not translate the "_m2_high" part!  */
-	  arg1 = value_struct_elt (&temp, NULL, "_m2_high", NULL,
+	  arg1 = value_struct_elt (&temp, {}, "_m2_high", NULL,
 				   _("unbounded structure "
 				     "missing _m2_high field"));
 
@@ -83,7 +83,7 @@ eval_op_m2_subscript (struct type *expect_type, struct expression *exp,
 	error (_("internal error: unbounded "
 		 "array structure is unknown"));
       /* i18n: Do not translate the "_m2_contents" part!  */
-      arg1 = value_struct_elt (&temp, NULL, "_m2_contents", NULL,
+      arg1 = value_struct_elt (&temp, {}, "_m2_contents", NULL,
 			       _("unbounded structure "
 				 "missing _m2_contents field"));
 	  
diff --git a/gdb/opencl-lang.c b/gdb/opencl-lang.c
index 1d6117a9285..136969ead76 100644
--- a/gdb/opencl-lang.c
+++ b/gdb/opencl-lang.c
@@ -708,7 +708,7 @@ opencl_structop_operation::evaluate (struct type *expect_type,
 				 noside);
   else
     {
-      struct value *v = value_struct_elt (&arg1, NULL,
+      struct value *v = value_struct_elt (&arg1, {},
 					  std::get<1> (m_storage).c_str (),
 					  NULL, "structure");
 
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index 43da8f0fbc4..8df8a15f8d6 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -992,7 +992,7 @@ valpy_getitem (PyObject *self, PyObject *key)
       scoped_value_mark free_values;
 
       if (field)
-	res_val = value_struct_elt (&tmp, NULL, field.get (), NULL,
+	res_val = value_struct_elt (&tmp, {}, field.get (), NULL,
 				    "struct/class/union");
       else if (bitpos >= 0)
 	res_val = value_struct_elt_bitpos (&tmp, bitpos, field_type,
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 3b15bb22a27..60ea89b1394 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -302,9 +302,9 @@ static void
 rust_val_print_str (struct ui_file *stream, struct value *val,
 		    const struct value_print_options *options)
 {
-  struct value *base = value_struct_elt (&val, NULL, "data_ptr", NULL,
+  struct value *base = value_struct_elt (&val, {}, "data_ptr", NULL,
 					 "slice");
-  struct value *len = value_struct_elt (&val, NULL, "length", NULL, "slice");
+  struct value *len = value_struct_elt (&val, {}, "length", NULL, "slice");
 
   val_print_string (TYPE_TARGET_TYPE (value_type (base)), "UTF-8",
 		    value_as_address (base), value_as_long (len), stream,
@@ -1030,7 +1030,7 @@ rust_range (struct type *expect_type, struct expression *exp,
 
   if (low != NULL)
     {
-      struct value *start = value_struct_elt (&result, NULL, "start", NULL,
+      struct value *start = value_struct_elt (&result, {}, "start", NULL,
 					      "range");
 
       value_assign (start, low);
@@ -1038,7 +1038,7 @@ rust_range (struct type *expect_type, struct expression *exp,
 
   if (high != NULL)
     {
-      struct value *end = value_struct_elt (&result, NULL, "end", NULL,
+      struct value *end = value_struct_elt (&result, {}, "end", NULL,
 					    "range");
 
       value_assign (end, high);
@@ -1176,8 +1176,8 @@ rust_subscript (struct type *expect_type, struct expression *exp,
 	{
 	  struct value *len;
 
-	  base = value_struct_elt (&lhs, NULL, "data_ptr", NULL, "slice");
-	  len = value_struct_elt (&lhs, NULL, "length", NULL, "slice");
+	  base = value_struct_elt (&lhs, {}, "data_ptr", NULL, "slice");
+	  len = value_struct_elt (&lhs, {}, "length", NULL, "slice");
 	  low_bound = 0;
 	  high_bound = value_as_long (len);
 	}
@@ -1400,7 +1400,7 @@ eval_op_rust_structop (struct type *expect_type, struct expression *exp,
 
       try
 	{
-	  result = value_struct_elt (&lhs, NULL, field_name,
+	  result = value_struct_elt (&lhs, {}, field_name,
 				     NULL, "structure");
 	}
       catch (const gdb_exception_error &except)
@@ -1411,7 +1411,7 @@ eval_op_rust_structop (struct type *expect_type, struct expression *exp,
 	}
     }
   else
-    result = value_struct_elt (&lhs, NULL, field_name, NULL, "structure");
+    result = value_struct_elt (&lhs, {}, field_name, NULL, "structure");
   if (noside == EVAL_AVOID_SIDE_EFFECTS)
     result = value_zero (value_type (result), VALUE_LVAL (result));
   return result;
@@ -1457,7 +1457,7 @@ rust_aggregate_operation::evaluate (struct type *expect_type,
       if (noside == EVAL_NORMAL)
 	{
 	  const char *fieldname = item.first.c_str ();
-	  value *field = value_struct_elt (&result, nullptr, fieldname,
+	  value *field = value_struct_elt (&result, {}, fieldname,
 					   nullptr, "structure");
 	  value_assign (field, val);
 	}
diff --git a/gdb/valarith.c b/gdb/valarith.c
index d61ad9170f8..9ebad648b27 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -344,7 +344,7 @@ value_user_defined_op (struct value **argp, gdb::array_view<value *> args,
 					  noside);
     }
   else
-    result = value_struct_elt (argp, &args, 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 0af7a6c3f27..a1c2bbf23fa 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -51,7 +51,7 @@ static struct value *search_struct_field (const char *, struct value *,
 					  struct type *, int);
 
 static struct value *search_struct_method (const char *, struct value **,
-					   gdb::array_view<value *> *,
+					   gdb::optional<gdb::array_view<value *>>,
 					   LONGEST, int *, struct type *);
 
 static int find_oload_champ_namespace (gdb::array_view<value *> args,
@@ -2177,17 +2177,18 @@ 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 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.
+   The ARGS array is to an optional list of argument values used to help
+   finding NAME.  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,
-		      gdb::array_view<value *> *args, LONGEST offset,
-		      int *static_memfuncp, struct type *type)
+		      gdb::optional<gdb::array_view<value *>> args,
+		      LONGEST offset, int *static_memfuncp,
+		      struct type *type)
 {
   int i;
   struct value *v;
@@ -2205,10 +2206,10 @@ search_struct_method (const char *name, struct value **arg1p,
 
 	  name_matched = 1;
 	  check_stub_method_group (type, i);
-	  if (j > 0 && args == nullptr)
+	  if (j > 0 && !args.has_value ())
 	    error (_("cannot resolve overloaded method "
 		     "`%s': no arguments supplied"), name);
-	  else if (j == 0 && args == nullptr)
+	  else if (j == 0 && !args.has_value ())
 	    {
 	      v = value_fn_field (arg1p, f, j, type, offset);
 	      if (v != NULL)
@@ -2217,7 +2218,7 @@ search_struct_method (const char *name, struct value **arg1p,
 	  else
 	    while (j >= 0)
 	      {
-		gdb_assert (args != nullptr);
+		gdb_assert (args.has_value ());
 		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 (),
@@ -2320,7 +2321,8 @@ search_struct_method (const char *name, struct value **arg1p,
    found.  */
 
 struct value *
-value_struct_elt (struct value **argp, gdb::array_view<value *> *args,
+value_struct_elt (struct value **argp,
+		  gdb::optional<gdb::array_view<value *>> args,
 		  const char *name, int *static_memfuncp, const char *err)
 {
   struct type *t;
@@ -2350,7 +2352,7 @@ value_struct_elt (struct value **argp, gdb::array_view<value *> *args,
   if (static_memfuncp)
     *static_memfuncp = 0;
 
-  if (args == nullptr)
+  if (!args.has_value ())
     {
       /* if there are no arguments ...do this...  */
 
diff --git a/gdb/value.h b/gdb/value.h
index 40ad28e3dbb..379cddafbe7 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,
-				       gdb::array_view <value *> *args,
+				       gdb::optional<gdb::array_view <value *>> args,
 				       const char *name, int *static_memfuncp,
 				       const char *err);
 
-- 
2.25.4

Re: [PATCHv2 1/3] gdb: fix regression in evaluate_funcall for non C++ like cases

[Simon Marchi]

> diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.exp b/gdb/testsuite/gdb.cp/method-call-in-c.exp
> new file mode 100644
> index 00000000000..eb48979e465
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp
> @@ -0,0 +1,44 @@
> +# Copyright 2021 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Ensure that calling a member function works correctly even when the
> +# language is forced to 'C' (this should be fine, so long at no
> +# overload resolution is required), or when overload-resolution is
> +# off.
> +
> +standard_testfile .cc
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    return 0
> +}
> +
> +gdb_breakpoint [gdb_get_line_number "Break here"]
> +gdb_continue_to_breakpoint "Break here"
> +
> +set result 123
> +foreach_with_prefix lang { c++ c } {
> +    foreach_with_prefix overload_resolution { on off } {
> +	gdb_test_no_output "set overload-resolution ${overload_resolution}"
> +	gdb_test "set language ${lang}" ".*"

You can simply omit the ".*".

> +
> +	gdb_test "print foo.func (b, f)" " = ${result}" \
> +	    "call foo.func with language auto;c++, overload-resolution on"

The test name looks wrong.

Otherwise, LGTM.

Simon

Re: [PATCHv2 2/3] gdb: replace NULL terminated array with array_view

[Simon Marchi]

On 2021-06-23 8:26 a.m., Andrew Burgess wrote:
> 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++.

I remember seeing this while attempting to convert to gdb::array_view
and thinking it looked fishy.

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

Agreed, by the principle of least astonishment, it would not be a good
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.

Agreed, using gdb::optional communicates the intent.

> 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                                |  2 +-
>  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, 63 insertions(+), 35 deletions(-)
> 
> diff --git a/gdb/eval.c b/gdb/eval.c
> index ab070a3d9f6..6bc4132288b 100644
> --- a/gdb/eval.c
> +++ b/gdb/eval.c
> @@ -950,7 +950,7 @@ structop_base_operation::evaluate_funcall

At the beginning of this function:

  /* Allocate space for the function call arguments.  Include space for a
     `this' pointer at the start, and a trailing nullptr.  */
  std::vector<value *> vals (args.size () + 2);

Does this need to be turned back to `+ 1`?  And this removed:

  vals[args.size () + 1] = nullptr;

And if so, this:

  gdb::array_view<value *> arg_view
    = gdb::make_array_view (&vals[0], args.size () + 1);

Could be simplified to:

  gdb::array_view<value *> arg_view (vals);

Since we now want the array_view to refer to the whole vector.

The comments that refer to the "trailing nullptr" should also be
adjusted.

> diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.exp b/gdb/testsuite/gdb.cp/method-call-in-c.exp
> index eb48979e465..066e3648382 100644
> --- a/gdb/testsuite/gdb.cp/method-call-in-c.exp
> +++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp
> @@ -40,5 +40,9 @@ foreach_with_prefix lang { c++ c } {
>  	gdb_test "print foo.func (b, f)" " = ${result}" \
>  	    "call foo.func with language auto;c++, overload-resolution on"
>  	incr result
> +
> +	set result [expr $result + 3]
> +	gdb_test "print foo += b" \
> +	    " = \\((?:struct )?foo_type &\\) @${hex}: \\\{var = ${result}\\\}"
>      }

Since there are code paths that deal with the member function being
static, I would suggest also testing:

 - call foo.a_static_method(<args>)
 - call foo_type::a_static_method(<args>)

I would think that testing the two methods, because I would guess that
in the first case, a `this` pointer exists, but is then ignored when
calling a_static_method.  But in the second case, there was never a
`this` pointer to begin with.  So the behavior may be different.

Simon

Re: [PATCHv2 3/3] gdb: use gdb::optional instead of passing a pointer to gdb::array_view

[Simon Marchi]

On 2021-06-23 8:26 a.m., Andrew Burgess wrote:
> Following on from the previous commit, this commit changes the API of
> value_struct_elt to take gdb::optional<gdb::array_view<value *>>
> instead of a pointer to the gdb::array_view.
> 
> This makes the optional nature of the array_view parameter explicit.
> 
> This commit is purely a refactoring commit, there should be no user
> visible change after this commit.
> 
> I have deliberately kept this refactor separate from the previous two
> commits as this is a more extensive change, and I'm not 100% sure that
> using gdb::optional for the parameter type, instead of a pointer, is
> going to be to everyone's taste.  If there's push back on this patch
> then this one can be dropped from the series.

I think it's better.

> @@ -2177,17 +2177,18 @@ 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 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.
> +   The ARGS array is to an optional list of argument values used to help

I find the wording a bit strange.  This would be more straightforward:

  ARGS is an optional array of argument values used to...

Otherwise, LGTM, thanks!

Simon

[PATCHv3 0/4] gdb: fix regression in evaluate_funcall for non C++ like cases

[Andrew Burgess]

Changes since v2:

  - Have made all of the improvements Simon suggested, it's all minor
    stuff, so there's no significant differences since v2 in patches 1
    to 3.

  - The new tests requested by Simon in patch 2 are added in the new
    patch 4.

  - Patch 4 is new, when added the extra tests Simon requested, I
    found another bug, which is fixed in this new patch.

---

Andrew Burgess (4):
  gdb: fix regression in evaluate_funcall for non C++ like cases
  gdb: replace NULL terminated array with array_view
  gdb: use gdb::optional instead of passing a pointer to gdb::array_view
  gdb: fix invalid arg coercion when calling static member functions

 gdb/ChangeLog                             | 56 +++++++++++++++++++++
 gdb/ada-lang.c                            |  6 +--
 gdb/eval.c                                | 14 ++++--
 gdb/f-lang.c                              |  2 +-
 gdb/guile/scm-value.c                     |  2 +-
 gdb/infcall.c                             |  4 +-
 gdb/m2-lang.c                             |  4 +-
 gdb/opencl-lang.c                         |  2 +-
 gdb/python/py-value.c                     |  2 +-
 gdb/rust-lang.c                           | 18 +++----
 gdb/testsuite/ChangeLog                   | 23 +++++++++
 gdb/testsuite/gdb.cp/method-call-in-c.cc  | 61 +++++++++++++++++++++++
 gdb/testsuite/gdb.cp/method-call-in-c.exp | 50 +++++++++++++++++++
 gdb/valarith.c                            |  2 +-
 gdb/valops.c                              | 57 +++++++++++----------
 gdb/value.h                               |  2 +-
 16 files changed, 252 insertions(+), 53 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.cc
 create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.exp

-- 
2.25.4

[PATCHv3 1/4] gdb: fix regression in evaluate_funcall for non C++ like cases

[Andrew Burgess]

This regression, as it is exposed by the test added in this commit,
first became noticable with this commit:

  commit d182f2797922a305fbd1ef6a483cc39a56b43e02
  Date:   Mon Mar 8 07:27:57 2021 -0700

      Convert c-exp.y to use operations

But, this commit only added converted the C expression parser to make
use of code that was added in this commit:

  commit a00b7254fb614af557de7ae7cc0eb39a0ce0e408
  Date:   Mon Mar 8 07:27:57 2021 -0700

      Implement function call operations

And it was this second commit that actually introduced the bugs (there
are two).

In structop_base_operation::evaluate_funcall we build up an argument
list in the vector vals.  Later in this function the argument list
might be passed to value_struct_elt.

Prior to commit a00b7254fb614 the vals vector (or argvec as it used to
be called) stored the value for the function callee in the argvec at
index 0.  This 'callee' value is what ends up being passed to
evaluate_subexp_do_call, and represents the function to be called, the
value contents are the address of the function, and the value type is
the function signature.  The remaining items held in the argvec were
the values to pass to the function.  For a non-static member function
the `this' pointer would be at index 1 in the array.

After commit a00b7254fb614 this callee value is now held in a separate
variable, not the vals array.  So, for non-static member functions,
the `this' pointer is now at index 0, with any other arguments after
that.

What this means is that previous, when we called value_struct_elt we
would pass the address of argvec[1] as this was the first argument.
But now we should be passing the address of vals[0].  Unfortunately,
we are still passing vals[1], effectively skipping the first
argument.

The second issue is that, prior to commit a00b7254fb614, the argvec
array was NULL terminated.  This is required as value_struct_elt
calls search_struct_method, which calls typecmp, and typecmp requires
that the array have a NULL at the end.

After commit a00b7254fb614 this NULL has been lost, and we are
therefore violating the API requirements of typecmp.

This commit fixes both of these regressions.  I also extended the
header comments on search_struct_method and value_struct_elt to make
it clearer that the array required a NULL marker at the end.

You will notice in the test attached to this commit that I test
calling a non-static member function, but not calling a static member
function.  The reason for this is that calling static member functions
is currently broken due to a different bug.  That will be fixed in a
later patch in this series, at which time I'll add a test for calling
a static member function.

gdb/ChangeLog:

	PR gdb/27994
	* eval.c (structop_base_operation::evaluate_funcall): Add a
	nullptr to the end of the args array, which should not be included
	in the argument array_view.  Pass all the arguments through to
	value_struct_elt.
	* valops.c (search_struct_method): Update header comment.
	(value_struct_elt): Likewise.

gdb/testsuite/ChangeLog:

	PR gdb/27994
	* gdb.cp/method-call-in-c.cc: New file.
	* gdb.cp/method-call-in-c.exp: New file.
---
 gdb/ChangeLog                             | 10 ++++++
 gdb/eval.c                                | 15 ++++++--
 gdb/testsuite/ChangeLog                   |  6 ++++
 gdb/testsuite/gdb.cp/method-call-in-c.cc  | 44 +++++++++++++++++++++++
 gdb/testsuite/gdb.cp/method-call-in-c.exp | 43 ++++++++++++++++++++++
 gdb/valops.c                              |  7 +++-
 6 files changed, 121 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.cc
 create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.exp

diff --git a/gdb/eval.c b/gdb/eval.c
index 659493c8237..ab070a3d9f6 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -872,7 +872,9 @@ structop_base_operation::evaluate_funcall
      (struct type *expect_type, struct expression *exp, enum noside noside,
       const std::vector<operation_up> &args)
 {
-  std::vector<value *> vals (args.size () + 1);
+  /* Allocate space for the function call arguments.  Include space for a
+     `this' pointer at the start, and a trailing nullptr.  */
+  std::vector<value *> vals (args.size () + 2);
   /* First, evaluate the structure into vals[0].  */
   enum exp_opcode op = opcode ();
   if (op == STRUCTOP_STRUCT)
@@ -918,9 +920,16 @@ 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].  */
   for (int i = 0; i < args.size (); ++i)
     vals[i + 1] = args[i]->evaluate_with_coercion (exp, noside);
-  gdb::array_view<value *> arg_view = vals;
+  vals[args.size () + 1] = nullptr;
+
+  /* The array view includes the `this' pointer, but not the trailing
+     nullptr.  */
+  gdb::array_view<value *> arg_view
+    = gdb::make_array_view (&vals[0], args.size () + 1);
 
   int static_memfuncp;
   value *callee;
@@ -941,7 +950,7 @@ structop_base_operation::evaluate_funcall
     {
       struct value *temp = vals[0];
 
-      callee = value_struct_elt (&temp, &vals[1], tstr,
+      callee = value_struct_elt (&temp, &vals[0], tstr,
 				 &static_memfuncp,
 				 op == STRUCTOP_STRUCT
 				 ? "structure" : "structure pointer");
diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.cc b/gdb/testsuite/gdb.cp/method-call-in-c.cc
new file mode 100644
index 00000000000..09e4285ed5d
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.cc
@@ -0,0 +1,44 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+struct baz_type
+{
+  int a = 0;
+  int b = 1;
+  int c = 2;
+};
+
+struct foo_type
+{
+  int func (baz_type b, float f)
+  {
+    return var++;
+  }
+
+  int var = 123;
+};
+
+int
+main (void)
+{
+  baz_type b = {};
+  float f = 1.0;
+
+  foo_type foo;
+
+  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
new file mode 100644
index 00000000000..0e6851b3478
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp
@@ -0,0 +1,43 @@
+# Copyright 2021 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Ensure that calling a member function works correctly even when the
+# language is forced to 'C' (this should be fine, so long at no
+# overload resolution is required), or when overload-resolution is
+# off.
+
+standard_testfile .cc
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+if ![runto_main] then {
+    return 0
+}
+
+gdb_breakpoint [gdb_get_line_number "Break here"]
+gdb_continue_to_breakpoint "Break here"
+
+set result 123
+foreach_with_prefix lang { c++ c } {
+    foreach_with_prefix overload_resolution { on off } {
+	gdb_test_no_output "set overload-resolution ${overload_resolution}"
+	gdb_test "set language ${lang}"
+
+	gdb_test "print foo.func (b, f)" " = ${result}"
+	incr result
+    }
+}
diff --git a/gdb/valops.c b/gdb/valops.c
index 8694c124b52..2b579304204 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -2181,6 +2181,10 @@ 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.
+
    If found, return value, else if name matched and args not return
    (value) -1, else return NULL.  */
 
@@ -2309,7 +2313,8 @@ 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.
+   an appropriate method.  Also, handle derived types.  The array ARGS must
+   have a NULL at the end.
 
    STATIC_MEMFUNCP, if non-NULL, points to a caller-supplied location
    where the truthvalue of whether the function that was resolved was
-- 
2.25.4

[PATCHv3 2/4] gdb: replace NULL terminated array with array_view

[Andrew Burgess]

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/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<operation_up> &args)
 {
-  /* Allocate space for the function call arguments.  Include space for a
-     `this' pointer at the start, and a trailing nullptr.  */
-  std::vector<value *> vals (args.size () + 2);
+  /* Allocate space for the function call arguments, Including space for a
+     `this' pointer at the start.  */
+  std::vector<value *> 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<value *> arg_view
-    = gdb::make_array_view (&vals[0], args.size () + 1);
+  /* The array view includes the `this' pointer.  */
+  gdb::array_view<value *> 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/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<value *> 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<value *> 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<value *> *,
 					   LONGEST, int *, struct type *);
 
 static int find_oload_champ_namespace (gdb::array_view<value *> args,
@@ -1785,15 +1785,15 @@ value_string (const char *ptr, ssize_t len, struct type *char_type)
 }
 
 \f
-/* 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<value *> 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<value *> *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<value *> *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 <value *> *args,
 				       const char *name, int *static_memfuncp,
 				       const char *err);
 
-- 
2.25.4

[PATCHv3 3/4] gdb: use gdb::optional instead of passing a pointer to gdb::array_view

[Andrew Burgess]

Following on from the previous commit, this commit changes the API of
value_struct_elt to take gdb::optional<gdb::array_view<value *>>
instead of a pointer to the gdb::array_view.

This makes the optional nature of the array_view parameter explicit.

This commit is purely a refactoring commit, there should be no user
visible change after this commit.

I have deliberately kept this refactor separate from the previous two
commits as this is a more extensive change, and I'm not 100% sure that
using gdb::optional for the parameter type, instead of a pointer, is
going to be to everyone's taste.  If there's push back on this patch
then this one can be dropped from the series.

gdb/ChangeLog:

	* ada-lang.c (desc_bounds): Use '{}' instead of NULL to indicate
	an empty gdb::optional when calling value_struct_elt.
	(desc_data): Likewise.
	(desc_one_bound): Likewise.
	* eval.c (structop_base_operation::evaluate_funcall): Pass
	gdb::array_view, not a gdb::array_view* to value_struct_elt.
	(eval_op_structop_struct): Use '{}' instead of NULL to indicate
	an empty gdb::optional when calling value_struct_elt.
	(eval_op_structop_ptr): Likewise.
	* f-lang.c (fortran_structop_operation::evaluate): Likewise.
	* guile/scm-value.c (gdbscm_value_field): Likewise.
	* m2-lang.c (eval_op_m2_high): Likewise.
	(eval_op_m2_subscript): Likewise.
	* opencl-lang.c (opencl_structop_operation::evaluate): Likewise.
	* python/py-value.c (valpy_getitem): Likewise.
	* rust-lang.c (rust_val_print_str): Likewise.
	(rust_range): Likewise.
	(rust_subscript): Likewise.
	(eval_op_rust_structop): Likewise.
	(rust_aggregate_operation::evaluate): Likewise.
	* valarith.c (value_user_defined_op): Likewise.
	* valops.c (search_struct_method): Change parameter type, update
	function body accordingly, and update header comment.
	(value_struct_elt): Change parameter type, update function body
	accordingly.
	* value.h (value_struct_elt): Update declaration.
---
 gdb/ChangeLog         | 29 +++++++++++++++++++++++++++++
 gdb/ada-lang.c        |  6 +++---
 gdb/eval.c            |  6 +++---
 gdb/f-lang.c          |  2 +-
 gdb/guile/scm-value.c |  2 +-
 gdb/m2-lang.c         |  4 ++--
 gdb/opencl-lang.c     |  2 +-
 gdb/python/py-value.c |  2 +-
 gdb/rust-lang.c       | 18 +++++++++---------
 gdb/valarith.c        |  2 +-
 gdb/valops.c          | 24 +++++++++++++-----------
 gdb/value.h           |  2 +-
 12 files changed, 65 insertions(+), 34 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 6ed6b65e705..dd9e1354617 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -1484,7 +1484,7 @@ desc_bounds (struct value *arr)
 
   else if (is_thick_pntr (type))
     {
-      struct value *p_bounds = value_struct_elt (&arr, NULL, "P_BOUNDS", NULL,
+      struct value *p_bounds = value_struct_elt (&arr, {}, "P_BOUNDS", NULL,
 					       _("Bad GNAT array descriptor"));
       struct type *p_bounds_type = value_type (p_bounds);
 
@@ -1566,7 +1566,7 @@ desc_data (struct value *arr)
   if (is_thin_pntr (type))
     return thin_data_pntr (arr);
   else if (is_thick_pntr (type))
-    return value_struct_elt (&arr, NULL, "P_ARRAY", NULL,
+    return value_struct_elt (&arr, {}, "P_ARRAY", NULL,
 			     _("Bad GNAT array descriptor"));
   else
     return NULL;
@@ -1606,7 +1606,7 @@ desc_one_bound (struct value *bounds, int i, int which)
   char bound_name[20];
   xsnprintf (bound_name, sizeof (bound_name), "%cB%d",
 	     which ? 'U' : 'L', i - 1);
-  return value_struct_elt (&bounds, NULL, bound_name, NULL,
+  return value_struct_elt (&bounds, {}, bound_name, NULL,
 			   _("Bad GNAT array descriptor bounds"));
 }
 
diff --git a/gdb/eval.c b/gdb/eval.c
index 5a72bf1becb..5c348c34e66 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -947,7 +947,7 @@ structop_base_operation::evaluate_funcall
     {
       struct value *temp = vals[0];
 
-      callee = value_struct_elt (&temp, &arg_view, tstr,
+      callee = value_struct_elt (&temp, arg_view, tstr,
 				 &static_memfuncp,
 				 op == STRUCTOP_STRUCT
 				 ? "structure" : "structure pointer");
@@ -1132,7 +1132,7 @@ eval_op_structop_struct (struct type *expect_type, struct expression *exp,
 			 enum noside noside,
 			 struct value *arg1, const char *string)
 {
-  struct value *arg3 = value_struct_elt (&arg1, NULL, string,
+  struct value *arg3 = value_struct_elt (&arg1, {}, string,
 					 NULL, "structure");
   if (noside == EVAL_AVOID_SIDE_EFFECTS)
     arg3 = value_zero (value_type (arg3), VALUE_LVAL (arg3));
@@ -1188,7 +1188,7 @@ eval_op_structop_ptr (struct type *expect_type, struct expression *exp,
       }
   }
 
-  struct value *arg3 = value_struct_elt (&arg1, NULL, string,
+  struct value *arg3 = value_struct_elt (&arg1, {}, string,
 					 NULL, "structure pointer");
   if (noside == EVAL_AVOID_SIDE_EFFECTS)
     arg3 = value_zero (value_type (arg3), VALUE_LVAL (arg3));
diff --git a/gdb/f-lang.c b/gdb/f-lang.c
index 5731c400305..16ec9e04044 100644
--- a/gdb/f-lang.c
+++ b/gdb/f-lang.c
@@ -1511,7 +1511,7 @@ fortran_structop_operation::evaluate (struct type *expect_type,
 	arg1 = std::get<0> (m_storage)->evaluate (nullptr, exp, EVAL_NORMAL);
     }
 
-  value *elt = value_struct_elt (&arg1, NULL, str, NULL, "structure");
+  value *elt = value_struct_elt (&arg1, {}, str, NULL, "structure");
 
   if (noside == EVAL_AVOID_SIDE_EFFECTS)
     {
diff --git a/gdb/guile/scm-value.c b/gdb/guile/scm-value.c
index 24bb5547957..d4d76df0411 100644
--- a/gdb/guile/scm-value.c
+++ b/gdb/guile/scm-value.c
@@ -694,7 +694,7 @@ gdbscm_value_field (SCM self, SCM field_scm)
 
       struct value *tmp = v_smob->value;
 
-      struct value *res_val = value_struct_elt (&tmp, NULL, field.get (), NULL,
+      struct value *res_val = value_struct_elt (&tmp, {}, field.get (), NULL,
 						"struct/class/union");
 
       return vlscm_scm_from_value (res_val);
diff --git a/gdb/m2-lang.c b/gdb/m2-lang.c
index be1a8ed1437..911d67d8672 100644
--- a/gdb/m2-lang.c
+++ b/gdb/m2-lang.c
@@ -50,7 +50,7 @@ eval_op_m2_high (struct type *expect_type, struct expression *exp,
 
 	  type = type->field (1).type ();
 	  /* i18n: Do not translate the "_m2_high" part!  */
-	  arg1 = value_struct_elt (&temp, NULL, "_m2_high", NULL,
+	  arg1 = value_struct_elt (&temp, {}, "_m2_high", NULL,
 				   _("unbounded structure "
 				     "missing _m2_high field"));
 
@@ -83,7 +83,7 @@ eval_op_m2_subscript (struct type *expect_type, struct expression *exp,
 	error (_("internal error: unbounded "
 		 "array structure is unknown"));
       /* i18n: Do not translate the "_m2_contents" part!  */
-      arg1 = value_struct_elt (&temp, NULL, "_m2_contents", NULL,
+      arg1 = value_struct_elt (&temp, {}, "_m2_contents", NULL,
 			       _("unbounded structure "
 				 "missing _m2_contents field"));
 	  
diff --git a/gdb/opencl-lang.c b/gdb/opencl-lang.c
index 1d6117a9285..136969ead76 100644
--- a/gdb/opencl-lang.c
+++ b/gdb/opencl-lang.c
@@ -708,7 +708,7 @@ opencl_structop_operation::evaluate (struct type *expect_type,
 				 noside);
   else
     {
-      struct value *v = value_struct_elt (&arg1, NULL,
+      struct value *v = value_struct_elt (&arg1, {},
 					  std::get<1> (m_storage).c_str (),
 					  NULL, "structure");
 
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index 43da8f0fbc4..8df8a15f8d6 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -992,7 +992,7 @@ valpy_getitem (PyObject *self, PyObject *key)
       scoped_value_mark free_values;
 
       if (field)
-	res_val = value_struct_elt (&tmp, NULL, field.get (), NULL,
+	res_val = value_struct_elt (&tmp, {}, field.get (), NULL,
 				    "struct/class/union");
       else if (bitpos >= 0)
 	res_val = value_struct_elt_bitpos (&tmp, bitpos, field_type,
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 3b15bb22a27..60ea89b1394 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -302,9 +302,9 @@ static void
 rust_val_print_str (struct ui_file *stream, struct value *val,
 		    const struct value_print_options *options)
 {
-  struct value *base = value_struct_elt (&val, NULL, "data_ptr", NULL,
+  struct value *base = value_struct_elt (&val, {}, "data_ptr", NULL,
 					 "slice");
-  struct value *len = value_struct_elt (&val, NULL, "length", NULL, "slice");
+  struct value *len = value_struct_elt (&val, {}, "length", NULL, "slice");
 
   val_print_string (TYPE_TARGET_TYPE (value_type (base)), "UTF-8",
 		    value_as_address (base), value_as_long (len), stream,
@@ -1030,7 +1030,7 @@ rust_range (struct type *expect_type, struct expression *exp,
 
   if (low != NULL)
     {
-      struct value *start = value_struct_elt (&result, NULL, "start", NULL,
+      struct value *start = value_struct_elt (&result, {}, "start", NULL,
 					      "range");
 
       value_assign (start, low);
@@ -1038,7 +1038,7 @@ rust_range (struct type *expect_type, struct expression *exp,
 
   if (high != NULL)
     {
-      struct value *end = value_struct_elt (&result, NULL, "end", NULL,
+      struct value *end = value_struct_elt (&result, {}, "end", NULL,
 					    "range");
 
       value_assign (end, high);
@@ -1176,8 +1176,8 @@ rust_subscript (struct type *expect_type, struct expression *exp,
 	{
 	  struct value *len;
 
-	  base = value_struct_elt (&lhs, NULL, "data_ptr", NULL, "slice");
-	  len = value_struct_elt (&lhs, NULL, "length", NULL, "slice");
+	  base = value_struct_elt (&lhs, {}, "data_ptr", NULL, "slice");
+	  len = value_struct_elt (&lhs, {}, "length", NULL, "slice");
 	  low_bound = 0;
 	  high_bound = value_as_long (len);
 	}
@@ -1400,7 +1400,7 @@ eval_op_rust_structop (struct type *expect_type, struct expression *exp,
 
       try
 	{
-	  result = value_struct_elt (&lhs, NULL, field_name,
+	  result = value_struct_elt (&lhs, {}, field_name,
 				     NULL, "structure");
 	}
       catch (const gdb_exception_error &except)
@@ -1411,7 +1411,7 @@ eval_op_rust_structop (struct type *expect_type, struct expression *exp,
 	}
     }
   else
-    result = value_struct_elt (&lhs, NULL, field_name, NULL, "structure");
+    result = value_struct_elt (&lhs, {}, field_name, NULL, "structure");
   if (noside == EVAL_AVOID_SIDE_EFFECTS)
     result = value_zero (value_type (result), VALUE_LVAL (result));
   return result;
@@ -1457,7 +1457,7 @@ rust_aggregate_operation::evaluate (struct type *expect_type,
       if (noside == EVAL_NORMAL)
 	{
 	  const char *fieldname = item.first.c_str ();
-	  value *field = value_struct_elt (&result, nullptr, fieldname,
+	  value *field = value_struct_elt (&result, {}, fieldname,
 					   nullptr, "structure");
 	  value_assign (field, val);
 	}
diff --git a/gdb/valarith.c b/gdb/valarith.c
index d61ad9170f8..9ebad648b27 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -344,7 +344,7 @@ value_user_defined_op (struct value **argp, gdb::array_view<value *> args,
 					  noside);
     }
   else
-    result = value_struct_elt (argp, &args, 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 0af7a6c3f27..bd547923496 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -51,7 +51,7 @@ static struct value *search_struct_field (const char *, struct value *,
 					  struct type *, int);
 
 static struct value *search_struct_method (const char *, struct value **,
-					   gdb::array_view<value *> *,
+					   gdb::optional<gdb::array_view<value *>>,
 					   LONGEST, int *, struct type *);
 
 static int find_oload_champ_namespace (gdb::array_view<value *> args,
@@ -2177,17 +2177,18 @@ 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 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.
+   ARGS is an optional array of argument values used to help finding NAME.
+   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,
-		      gdb::array_view<value *> *args, LONGEST offset,
-		      int *static_memfuncp, struct type *type)
+		      gdb::optional<gdb::array_view<value *>> args,
+		      LONGEST offset, int *static_memfuncp,
+		      struct type *type)
 {
   int i;
   struct value *v;
@@ -2205,10 +2206,10 @@ search_struct_method (const char *name, struct value **arg1p,
 
 	  name_matched = 1;
 	  check_stub_method_group (type, i);
-	  if (j > 0 && args == nullptr)
+	  if (j > 0 && !args.has_value ())
 	    error (_("cannot resolve overloaded method "
 		     "`%s': no arguments supplied"), name);
-	  else if (j == 0 && args == nullptr)
+	  else if (j == 0 && !args.has_value ())
 	    {
 	      v = value_fn_field (arg1p, f, j, type, offset);
 	      if (v != NULL)
@@ -2217,7 +2218,7 @@ search_struct_method (const char *name, struct value **arg1p,
 	  else
 	    while (j >= 0)
 	      {
-		gdb_assert (args != nullptr);
+		gdb_assert (args.has_value ());
 		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 (),
@@ -2320,7 +2321,8 @@ search_struct_method (const char *name, struct value **arg1p,
    found.  */
 
 struct value *
-value_struct_elt (struct value **argp, gdb::array_view<value *> *args,
+value_struct_elt (struct value **argp,
+		  gdb::optional<gdb::array_view<value *>> args,
 		  const char *name, int *static_memfuncp, const char *err)
 {
   struct type *t;
@@ -2350,7 +2352,7 @@ value_struct_elt (struct value **argp, gdb::array_view<value *> *args,
   if (static_memfuncp)
     *static_memfuncp = 0;
 
-  if (args == nullptr)
+  if (!args.has_value ())
     {
       /* if there are no arguments ...do this...  */
 
diff --git a/gdb/value.h b/gdb/value.h
index 40ad28e3dbb..379cddafbe7 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,
-				       gdb::array_view <value *> *args,
+				       gdb::optional<gdb::array_view <value *>> args,
 				       const char *name, int *static_memfuncp,
 				       const char *err);
 
-- 
2.25.4

[PATCHv3 4/4] gdb: fix invalid arg coercion when calling static member functions

[Andrew Burgess]

In this commit:

  commit 7022349d5c86bae74b49225515f42d2e221bd368
  Date:   Mon Sep 4 20:21:13 2017 +0100

      Stop assuming no-debug-info functions return int

A new if case was added to call_function_by_hand_dummy to decide if a
function should be considered prototyped or not.  Previously the code
was structured like this:

  if (COND_1)
    ACTION_1
  else if (COND_2)
    ACTION_2
  else
    ACTION_3

With the new block the code now looks like this:

  if (COND_1)
    ACTION_1
  if (NEW_COND)
    NEW_ACTION
  else if (COND_2)
    ACTION_2
  else
    ACTION_3

Notice the new block was added as and 'if' not 'else if'.  I'm running
into a case where GDB executes ACTION_1 and then ACTION_2.  Prior to
the above commit GDB would only have executed ACTION_1.

The actions in the code in question are trying to figure out if a
function should be considered prototyped or not.  When a function is
not prototyped some arguments will be coerced, e.g. floats to doubles.

The COND_1 / ACTION_1 are a very broad, any member function should be
considered prototyped, however, after the above patch GDB is now
executing the later ACTION_2 which checks to see if the function's
type has the 'prototyped' flag set - this is not the case for the
member functions I'm testing, and so GDB treats the function as
unprototyped and casts the float argument to a double.

I believe that adding the new check as 'if' rather than 'else if' was
a mistake, and so in this commit I add in the missing 'else'.

gdb/ChangeLog:

	* infcall.c (call_function_by_hand_dummy): Add missing 'else' when
	setting prototyped flag.

gdb/testsuite/ChangeLog:

	* gdb.cp/method-call-in-c.cc (struct foo_type): Add static member
	function static_method.
	(global_var): New global.
	(main): Use new static_method to ensure it is compiled in.
	* gdb.cp/method-call-in-c.exp: Test calls to static member
	function.
---
 gdb/ChangeLog                             | 5 +++++
 gdb/infcall.c                             | 4 ++--
 gdb/testsuite/ChangeLog                   | 9 +++++++++
 gdb/testsuite/gdb.cp/method-call-in-c.cc  | 9 +++++++++
 gdb/testsuite/gdb.cp/method-call-in-c.exp | 3 +++
 5 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/gdb/infcall.c b/gdb/infcall.c
index ca3347fbb9d..40298fb1318 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -1026,8 +1026,8 @@ call_function_by_hand_dummy (struct value *function,
 	 prototyped.  Can we respect TYPE_VARARGS?  Probably not.  */
       if (ftype->code () == TYPE_CODE_METHOD)
 	prototyped = 1;
-      if (TYPE_TARGET_TYPE (ftype) == NULL && ftype->num_fields () == 0
-	  && default_return_type != NULL)
+      else if (TYPE_TARGET_TYPE (ftype) == NULL && ftype->num_fields () == 0
+	       && default_return_type != NULL)
 	{
 	  /* Calling a no-debug function with the return type
 	     explicitly cast.  Assume the function is prototyped,
diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.cc b/gdb/testsuite/gdb.cp/method-call-in-c.cc
index 95f3f3c22de..846a9111ef9 100644
--- a/gdb/testsuite/gdb.cp/method-call-in-c.cc
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.cc
@@ -35,9 +35,16 @@ struct foo_type
     return *this;
   }
 
+  static int static_method (float f, baz_type b)
+  {
+    return b.a + b.b + b.c + (int) f;
+  }
+
   int var = 120;
 };
 
+volatile int global_var;
+
 int
 main (void)
 {
@@ -48,5 +55,7 @@ main (void)
 
   foo += b;
 
+  global_var = foo.static_method (f, 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 411ba6790b1..5debc0e9a7a 100644
--- a/gdb/testsuite/gdb.cp/method-call-in-c.exp
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp
@@ -43,5 +43,8 @@ foreach_with_prefix lang { c++ c } {
 	set result [expr $result + 3]
 	gdb_test "print foo += b" \
 	    " = \\((?:struct )?foo_type &\\) @${hex}: \\\{var = ${result}\\\}"
+
+	gdb_test "print foo.static_method (f, b)" " = 4"
+	gdb_test "print foo_type::static_method (f, b)" " = 4"
     }
 }
-- 
2.25.4

Re: [PATCHv3 4/4] gdb: fix invalid arg coercion when calling static member functions

[Simon Marchi]

On 2021-06-23 6:44 p.m., Andrew Burgess wrote:
> In this commit:
> 
>   commit 7022349d5c86bae74b49225515f42d2e221bd368
>   Date:   Mon Sep 4 20:21:13 2017 +0100
> 
>       Stop assuming no-debug-info functions return int
> 
> A new if case was added to call_function_by_hand_dummy to decide if a
> function should be considered prototyped or not.  Previously the code
> was structured like this:
> 
>   if (COND_1)
>     ACTION_1
>   else if (COND_2)
>     ACTION_2
>   else
>     ACTION_3
> 
> With the new block the code now looks like this:
> 
>   if (COND_1)
>     ACTION_1
>   if (NEW_COND)
>     NEW_ACTION
>   else if (COND_2)
>     ACTION_2
>   else
>     ACTION_3
> 
> Notice the new block was added as and 'if' not 'else if'.  I'm running
> into a case where GDB executes ACTION_1 and then ACTION_2.  Prior to
> the above commit GDB would only have executed ACTION_1.
> 
> The actions in the code in question are trying to figure out if a
> function should be considered prototyped or not.  When a function is
> not prototyped some arguments will be coerced, e.g. floats to doubles.
> 
> The COND_1 / ACTION_1 are a very broad, any member function should be
> considered prototyped, however, after the above patch GDB is now
> executing the later ACTION_2 which checks to see if the function's
> type has the 'prototyped' flag set - this is not the case for the
> member functions I'm testing, and so GDB treats the function as
> unprototyped and casts the float argument to a double.
> 
> I believe that adding the new check as 'if' rather than 'else if' was
> a mistake, and so in this commit I add in the missing 'else'.

Wow, good investigation work, thanks for fixing this.  It all LGTM.

Simon

Re: [PATCHv3 4/4] gdb: fix invalid arg coercion when calling static member functions

[Andrew Burgess]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-06-25 10:53:16 -0400]:

> On 2021-06-23 6:44 p.m., Andrew Burgess wrote:
> > In this commit:
> > 
> >   commit 7022349d5c86bae74b49225515f42d2e221bd368
> >   Date:   Mon Sep 4 20:21:13 2017 +0100
> > 
> >       Stop assuming no-debug-info functions return int
> > 
> > A new if case was added to call_function_by_hand_dummy to decide if a
> > function should be considered prototyped or not.  Previously the code
> > was structured like this:
> > 
> >   if (COND_1)
> >     ACTION_1
> >   else if (COND_2)
> >     ACTION_2
> >   else
> >     ACTION_3
> > 
> > With the new block the code now looks like this:
> > 
> >   if (COND_1)
> >     ACTION_1
> >   if (NEW_COND)
> >     NEW_ACTION
> >   else if (COND_2)
> >     ACTION_2
> >   else
> >     ACTION_3
> > 
> > Notice the new block was added as and 'if' not 'else if'.  I'm running
> > into a case where GDB executes ACTION_1 and then ACTION_2.  Prior to
> > the above commit GDB would only have executed ACTION_1.
> > 
> > The actions in the code in question are trying to figure out if a
> > function should be considered prototyped or not.  When a function is
> > not prototyped some arguments will be coerced, e.g. floats to doubles.
> > 
> > The COND_1 / ACTION_1 are a very broad, any member function should be
> > considered prototyped, however, after the above patch GDB is now
> > executing the later ACTION_2 which checks to see if the function's
> > type has the 'prototyped' flag set - this is not the case for the
> > member functions I'm testing, and so GDB treats the function as
> > unprototyped and casts the float argument to a double.
> > 
> > I believe that adding the new check as 'if' rather than 'else if' was
> > a mistake, and so in this commit I add in the missing 'else'.
> 
> Wow, good investigation work, thanks for fixing this.  It all LGTM.

I've pushed this series.

Thanks for all the feedback,

Andrew

[PATCHv2 2/3] gdb: replace NULL terminated array with array_view

[Andrew Burgess]

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                                |  2 +-
 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, 63 insertions(+), 35 deletions(-)

diff --git a/gdb/eval.c b/gdb/eval.c
index ab070a3d9f6..6bc4132288b 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -950,7 +950,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/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 eb48979e465..066e3648382 100644
--- a/gdb/testsuite/gdb.cp/method-call-in-c.exp
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp
@@ -40,5 +40,9 @@ foreach_with_prefix lang { c++ c } {
 	gdb_test "print foo.func (b, f)" " = ${result}" \
 	    "call foo.func with language auto;c++, overload-resolution on"
 	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<value *> 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<value *> 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<value *> *,
 					   LONGEST, int *, struct type *);
 
 static int find_oload_champ_namespace (gdb::array_view<value *> args,
@@ -1785,15 +1785,15 @@ value_string (const char *ptr, ssize_t len, struct type *char_type)
 }
 
 \f
-/* 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<value *> 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<value *> *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<value *> *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 <value *> *args,
 				       const char *name, int *static_memfuncp,
 				       const char *err);
 
-- 
2.25.4