diff mbox

[PR,c++/17132] Add new argument NOSIDE to find_overload_match

Message ID CAGyQ6gwjpbYWoT7xki0tNQF7vq9mA_d+zMrnCDxXGUbU6gn1=A@mail.gmail.com
State New, archived
Headers

Commit Message

Siva Chandra Reddy July 9, 2014, 6:52 p.m. UTC 
  The bug with examples where it manifests is detailed on the bug page.
To summarize here:

When expressions involving calls to virtual methods are evaluated
under EVAL_AVOID_SIDE_EFFECTS, the intermediate objects could be
zeroed values. In which case, when find_overload_match tries to
(indirectly) lookup the method for the most derived type of the object
via its vptr, GDB trips with "Cannot access memory at address 0x0."
This patch adds an additional argument NOSIDE to find_overload_match
which serves as a flag to indicate whether method resolution should be
performed by looking up the most derived object via the vptr. When
NOSIDE is EVAL_AVOID_SIDE_EFFECTS, the method for the compile time
type (instead of the most derived type) of the object is returned by
find_overload_match. This will not affect the functionality as all
that is important when evaluating under EVAL_AVOID_SIDE_EFFECTS is the
return type of the method.

Regression tested testsuite/gdb.cp. But, am I simplifying the problem somewhere?

ChangeLog
2014-07-09  Siva Chandra Reddy  <sivachandra@google.com>

    gdb/

            * eval.c: Update all calls to find_overload_match.
            * valarith.c: Update calls to find_overload_match.
            (value_user_defined_cpp_op, value_user_defined_op): New
            argument NOSIDE. Update all callers.
            * valops.c (find_overload_match): New argument NOSIDE.
            * value.h (find_overload_match): Update signature.

    gdb/testsuite

            * gdb.cp/pr17132.cc: New file.
            * gdb.cp/pr17132.exp: New file.

Comments

Siva Chandra Reddy July 16, 2014, 1:34 p.m. UTC | #1 
Ping.

On Wed, Jul 9, 2014 at 11:52 AM, Siva Chandra <sivachandra@google.com> wrote:
> The bug with examples where it manifests is detailed on the bug page.
> To summarize here:
>
> When expressions involving calls to virtual methods are evaluated
> under EVAL_AVOID_SIDE_EFFECTS, the intermediate objects could be
> zeroed values. In which case, when find_overload_match tries to
> (indirectly) lookup the method for the most derived type of the object
> via its vptr, GDB trips with "Cannot access memory at address 0x0."
> This patch adds an additional argument NOSIDE to find_overload_match
> which serves as a flag to indicate whether method resolution should be
> performed by looking up the most derived object via the vptr. When
> NOSIDE is EVAL_AVOID_SIDE_EFFECTS, the method for the compile time
> type (instead of the most derived type) of the object is returned by
> find_overload_match. This will not affect the functionality as all
> that is important when evaluating under EVAL_AVOID_SIDE_EFFECTS is the
> return type of the method.
>
> Regression tested testsuite/gdb.cp. But, am I simplifying the problem somewhere?
>
> ChangeLog
> 2014-07-09  Siva Chandra Reddy  <sivachandra@google.com>
>
>     gdb/
>
>             * eval.c: Update all calls to find_overload_match.
>             * valarith.c: Update calls to find_overload_match.
>             (value_user_defined_cpp_op, value_user_defined_op): New
>             argument NOSIDE. Update all callers.
>             * valops.c (find_overload_match): New argument NOSIDE.
>             * value.h (find_overload_match): Update signature.
>
>     gdb/testsuite
>
>             * gdb.cp/pr17132.cc: New file.
>             * gdb.cp/pr17132.exp: New file.
Siva Chandra Reddy Aug. 1, 2014, 2:38 p.m. UTC | #2 
Ping.

On Wed, Jul 16, 2014 at 6:34 AM, Siva Chandra <sivachandra@google.com> wrote:
> Ping.
>
> On Wed, Jul 9, 2014 at 11:52 AM, Siva Chandra <sivachandra@google.com> wrote:
>> The bug with examples where it manifests is detailed on the bug page.
>> To summarize here:
>>
>> When expressions involving calls to virtual methods are evaluated
>> under EVAL_AVOID_SIDE_EFFECTS, the intermediate objects could be
>> zeroed values. In which case, when find_overload_match tries to
>> (indirectly) lookup the method for the most derived type of the object
>> via its vptr, GDB trips with "Cannot access memory at address 0x0."
>> This patch adds an additional argument NOSIDE to find_overload_match
>> which serves as a flag to indicate whether method resolution should be
>> performed by looking up the most derived object via the vptr. When
>> NOSIDE is EVAL_AVOID_SIDE_EFFECTS, the method for the compile time
>> type (instead of the most derived type) of the object is returned by
>> find_overload_match. This will not affect the functionality as all
>> that is important when evaluating under EVAL_AVOID_SIDE_EFFECTS is the
>> return type of the method.
>>
>> Regression tested testsuite/gdb.cp. But, am I simplifying the problem somewhere?
>>
>> ChangeLog
>> 2014-07-09  Siva Chandra Reddy  <sivachandra@google.com>
>>
>>     gdb/
>>
>>             * eval.c: Update all calls to find_overload_match.
>>             * valarith.c: Update calls to find_overload_match.
>>             (value_user_defined_cpp_op, value_user_defined_op): New
>>             argument NOSIDE. Update all callers.
>>             * valops.c (find_overload_match): New argument NOSIDE.
>>             * value.h (find_overload_match): Update signature.
>>
>>     gdb/testsuite
>>
>>             * gdb.cp/pr17132.cc: New file.
>>             * gdb.cp/pr17132.exp: New file.
Doug Evans Aug. 5, 2014, 4:41 p.m. UTC | #3 
Siva Chandra writes:
 > Ping.
 > 
 > On Wed, Jul 16, 2014 at 6:34 AM, Siva Chandra <sivachandra@google.com> wrote:
 > > Ping.
 > >
 > > On Wed, Jul 9, 2014 at 11:52 AM, Siva Chandra <sivachandra@google.com> wrote:
 > >> The bug with examples where it manifests is detailed on the bug page.
 > >> To summarize here:
 > >>
 > >> When expressions involving calls to virtual methods are evaluated
 > >> under EVAL_AVOID_SIDE_EFFECTS, the intermediate objects could be
 > >> zeroed values. In which case, when find_overload_match tries to
 > >> (indirectly) lookup the method for the most derived type of the object
 > >> via its vptr, GDB trips with "Cannot access memory at address 0x0."
 > >> This patch adds an additional argument NOSIDE to find_overload_match
 > >> which serves as a flag to indicate whether method resolution should be
 > >> performed by looking up the most derived object via the vptr. When
 > >> NOSIDE is EVAL_AVOID_SIDE_EFFECTS, the method for the compile time
 > >> type (instead of the most derived type) of the object is returned by
 > >> find_overload_match. This will not affect the functionality as all
 > >> that is important when evaluating under EVAL_AVOID_SIDE_EFFECTS is the
 > >> return type of the method.
 > >>
 > >> Regression tested testsuite/gdb.cp. But, am I simplifying the problem somewhere?
 > >>
 > >> ChangeLog
 > >> 2014-07-09  Siva Chandra Reddy  <sivachandra@google.com>
 > >>
 > >>     gdb/
 > >>
 > >>             * eval.c: Update all calls to find_overload_match.
 > >>             * valarith.c: Update calls to find_overload_match.
 > >>             (value_user_defined_cpp_op, value_user_defined_op): New
 > >>             argument NOSIDE. Update all callers.
 > >>             * valops.c (find_overload_match): New argument NOSIDE.
 > >>             * value.h (find_overload_match): Update signature.
 > >>
 > >>     gdb/testsuite
 > >>
 > >>             * gdb.cp/pr17132.cc: New file.
 > >>             * gdb.cp/pr17132.exp: New file.

Hi.

I'm not confident I know c++ well enough to say for certain that there
isn't a problem somewhere, but I'm ok with the patch, modulo a few
comment changes.

@@ -2767,9 +2771,20 @@ find_overload_match (struct value **args, int nargs,
     {
       if (src_method_oload_champ >= 0)
 	{
-	  if (TYPE_FN_FIELD_VIRTUAL_P (fns_ptr, method_oload_champ))
-	    *valp = value_virtual_fn_field (&temp, fns_ptr, method_oload_champ,
-					    basetype, boffset);
+	  /* If the best matching method is virtual, then picking the correct
+	     implementation of the method will require reading the vtable of
+	     the object via the vtable pointer.  Hence, do not do this if
+	     NOSIDE is EVAL_AVOID_SIDE_EFFECTS.  It will not lead to any
+	     errors because of this as all that is of interest when evaluating
+	     expressions under EVAL_AVOID_SIDE_EFFECTS is the return type of
+	     the method.  */
+	  if (TYPE_FN_FIELD_VIRTUAL_P (fns_ptr, method_oload_champ)
+	      && noside != EVAL_AVOID_SIDE_EFFECTS)
+	    {
+	      *valp = value_virtual_fn_field (&temp, fns_ptr,
+					      method_oload_champ, basetype,
+					      boffset);
+	    }
 	  else
 	    *valp = value_fn_field (&temp, fns_ptr, method_oload_champ,
 				    basetype, boffset);

It doesn't feel right to encode in find_overload_match any assumptions
about why it might be called with EVAL_AVOID_SIDE_EFFECTS.  At least
not explicitly.  Instead it should say what it does when passed
EVAL_AVOID_SIDE_EFFECTS and let the caller decide.

Thus I would change this comment:

+   If NOSIDE is EVAL_AVOID_SIDE_EFFECTS, then OBJP's memory will not be
+   read while picking the best overload match.

to this (or something like it):

+   If NOSIDE is EVAL_AVOID_SIDE_EFFECTS, then OBJP's memory cannot be
+   read while picking the best overload match (it may be all zeroes and thus
+   not have a vtable pointer), in which case skip virtual function lookup.
+   This is ok as typically EVAL_AVOID_SIDE_EFFECTS is only used to determine
+   the result type.

and then I'd be ok with just removing this comment:

+	  /* If the best matching method is virtual, then picking the correct
+	     implementation of the method will require reading the vtable of
+	     the object via the vtable pointer.  Hence, do not do this if
+	     NOSIDE is EVAL_AVOID_SIDE_EFFECTS.  It will not lead to any
+	     errors because of this as all that is of interest when evaluating
+	     expressions under EVAL_AVOID_SIDE_EFFECTS is the return type of
+	     the method.  */
diff mbox

Patch

diff --git a/gdb/eval.c b/gdb/eval.c
index d374b6a..036af1a 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -1601,7 +1601,7 @@  evaluate_subexp_standard (struct type *expect_type,
                                NON_METHOD, /* not method */
                                NULL, NULL, /* pass NULL symbol since
 					      symbol is unknown */
-                               NULL, &symp, NULL, 0);
+                               NULL, &symp, NULL, 0, noside);
 
           /* Now fix the expression being evaluated.  */
           exp->elts[save_pos1 + 2].symbol = symp;
@@ -1637,7 +1637,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	                                  METHOD, /* method */
 					  &arg2,  /* the object */
 					  NULL, &valp, NULL,
-					  &static_memfuncp, 0);
+					  &static_memfuncp, 0, noside);
 
 	      if (op == OP_SCOPE && !static_memfuncp)
 		{
@@ -1708,7 +1708,7 @@  evaluate_subexp_standard (struct type *expect_type,
 					  NULL,        /* no need for name */
 	                                  NON_METHOD,  /* not method */
 	                                  NULL, function, /* the function */
-					  NULL, &symp, NULL, no_adl);
+					  NULL, &symp, NULL, no_adl, noside);
 
 	      if (op == OP_VAR_VALUE)
 		{
diff --git a/gdb/testsuite/gdb.cp/pr17132.cc b/gdb/testsuite/gdb.cp/pr17132.cc
new file mode 100644
index 0000000..42d99af
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/pr17132.cc
@@ -0,0 +1,61 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+
+/* One could use unique_ptr instead, but that requires a GCC which can
+   support "-std=c++11".  */
+
+template <typename T>
+class smart_ptr
+{
+public:
+  smart_ptr (T *obj) : _obj (obj) { }
+  ~smart_ptr () { delete _obj; }
+
+  T *operator-> ();
+
+private:
+  T *_obj;
+};
+
+template <typename T>
+T *
+smart_ptr<T>::operator-> ()
+{
+  return _obj;
+}
+
+class A
+{
+public:
+  virtual int func ();
+};
+
+int
+A::func ()
+{
+  return 3;
+}
+
+int
+main ()
+{
+  A *a_ptr = 0;
+  smart_ptr<A> a (new A);
+
+  return a->func();  /* Break here  */
+}
diff --git a/gdb/testsuite/gdb.cp/pr17132.exp b/gdb/testsuite/gdb.cp/pr17132.exp
new file mode 100644
index 0000000..26a02a2
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/pr17132.exp
@@ -0,0 +1,40 @@ 
+# Copyright 2014 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/>.
+
+# This file is part of the gdb testsuite
+
+if {[skip_cplus_tests]} { continue }
+
+standard_testfile .cc
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    return -1
+}
+
+gdb_breakpoint [gdb_get_line_number "Break here"]
+gdb_continue_to_breakpoint "Break here"
+
+gdb_test "ptype a_ptr->func ()" ".* = int" "ptype a_ptr->func ()"
+gdb_test "ptype a->func ()" ".* = int" "ptype a->func ()"
+gdb_test "p sizeof (a_ptr->func()) == sizeof (int)" ".* = true" \
+  "p sizeof (a_ptr->func())"
+gdb_test "p sizeof (a->func()) == sizeof (int)" ".* = true" \
+  "p sizeof (a->func())"
+gdb_test "p 1 && a->func()" ".* = true" "p 1 && a->func()"
+gdb_test "p 0 || a->func()" ".* = true" "p 0 || a->func()"
diff --git a/gdb/valarith.c b/gdb/valarith.c
index 4da41cb..512600f 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -287,7 +287,7 @@  unop_user_defined_p (enum exp_opcode op, struct value *arg1)
 
 static struct value *
 value_user_defined_cpp_op (struct value **args, int nargs, char *operator,
-                           int *static_memfuncp)
+                           int *static_memfuncp, enum noside noside)
 {
 
   struct symbol *symp = NULL;
@@ -296,7 +296,7 @@  value_user_defined_cpp_op (struct value **args, int nargs, char *operator,
   find_overload_match (args, nargs, operator, BOTH /* could be method */,
                        &args[0] /* objp */,
                        NULL /* pass NULL symbol since symbol is unknown */,
-                       &valp, &symp, static_memfuncp, 0);
+                       &valp, &symp, static_memfuncp, 0, noside);
 
   if (valp)
     return valp;
@@ -318,12 +318,15 @@  value_user_defined_cpp_op (struct value **args, int nargs, char *operator,
 
 static struct value *
 value_user_defined_op (struct value **argp, struct value **args, char *name,
-                       int *static_memfuncp, int nargs)
+                       int *static_memfuncp, int nargs, enum noside noside)
 {
   struct value *result = NULL;
 
   if (current_language->la_language == language_cplus)
-    result = value_user_defined_cpp_op (args, nargs, name, static_memfuncp);
+    {
+      result = value_user_defined_cpp_op (args, nargs, name, static_memfuncp,
+					  noside);
+    }
   else
     result = value_struct_elt (argp, args, name, static_memfuncp,
                                "structure");
@@ -472,7 +475,7 @@  value_x_binop (struct value *arg1, struct value *arg2, enum exp_opcode op,
     }
 
   argvec[0] = value_user_defined_op (&arg1, argvec + 1, tstr,
-                                     &static_memfuncp, 2);
+                                     &static_memfuncp, 2, noside);
 
   if (argvec[0])
     {
@@ -583,7 +586,7 @@  value_x_unop (struct value *arg1, enum exp_opcode op, enum noside noside)
     }
 
   argvec[0] = value_user_defined_op (&arg1, argvec + 1, tstr,
-                                     &static_memfuncp, nargs);
+                                     &static_memfuncp, nargs, noside);
 
   if (argvec[0])
     {
diff --git a/gdb/valops.c b/gdb/valops.c
index 4164d63..09cde5e 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -2452,6 +2452,9 @@  value_find_oload_method_list (struct value **argp, const char *method,
    ADL overload candidates when performing overload resolution for a fully
    qualified name.
 
+   If NOSIDE is EVAL_AVOID_SIDE_EFFECTS, then OBJP's memory will not be
+   read while picking the best overload match.
+
    Note: This function does *not* check the value of
    overload_resolution.  Caller must check it to see whether overload
    resolution is permitted.  */
@@ -2461,7 +2464,8 @@  find_overload_match (struct value **args, int nargs,
 		     const char *name, enum oload_search_type method,
 		     struct value **objp, struct symbol *fsym,
 		     struct value **valp, struct symbol **symp, 
-		     int *staticp, const int no_adl)
+		     int *staticp, const int no_adl,
+		     const enum noside noside)
 {
   struct value *obj = (objp ? *objp : NULL);
   struct type *obj_type = obj ? value_type (obj) : NULL;
@@ -2767,9 +2771,20 @@  find_overload_match (struct value **args, int nargs,
     {
       if (src_method_oload_champ >= 0)
 	{
-	  if (TYPE_FN_FIELD_VIRTUAL_P (fns_ptr, method_oload_champ))
-	    *valp = value_virtual_fn_field (&temp, fns_ptr, method_oload_champ,
-					    basetype, boffset);
+	  /* If the best matching method is virtual, then picking the correct
+	     implementation of the method will require reading the vtable of
+	     the object via the vtable pointer.  Hence, do not do this if
+	     NOSIDE is EVAL_AVOID_SIDE_EFFECTS.  It will not lead to any
+	     errors because of this as all that is of interest when evaluating
+	     expressions under EVAL_AVOID_SIDE_EFFECTS is the return type of
+	     the method.  */
+	  if (TYPE_FN_FIELD_VIRTUAL_P (fns_ptr, method_oload_champ)
+	      && noside != EVAL_AVOID_SIDE_EFFECTS)
+	    {
+	      *valp = value_virtual_fn_field (&temp, fns_ptr,
+					      method_oload_champ, basetype,
+					      boffset);
+	    }
 	  else
 	    *valp = value_fn_field (&temp, fns_ptr, method_oload_champ,
 				    basetype, boffset);
diff --git a/gdb/value.h b/gdb/value.h
index c2e9faf..56feb89 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -694,7 +694,8 @@  extern int find_overload_match (struct value **args, int nargs,
 				enum oload_search_type method,
 				struct value **objp, struct symbol *fsym,
 				struct value **valp, struct symbol **symp,
-				int *staticp, const int no_adl);
+				int *staticp, const int no_adl,
+				enum noside noside);
 
 extern struct value *value_field (struct value *arg1, int fieldno);