Re: [PATCH PR c++/17132] Add new argument NOSIDE to find_overload_match

[Doug Evans <dje at google dot com>]

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