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

Message ID 20210621233949.2266811-1-andrew.burgess@embecosm.com
State Superseded
Headers show
Series
  • gdb: fix regression in evaluate_funcall for non C++ like cases
Related show

Commit Message

Andrew Burgess June 21, 2021, 11:39 p.m.
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

-- 
2.25.4

Comments

Simon Marchi via Gdb-patches June 22, 2021, 1:47 p.m. | #1
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
Andrew Burgess June 23, 2021, 12:26 p.m. | #2
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

Patch

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