Re: [RFA] Fix c++/14819 (implicit this)

[Keith Seitz <keiths at redhat dot com>]

On 11/06/2013 09:42 AM, Tom Tromey wrote:
> > > > > > "Keith" == Keith Seitz <keiths@redhat.com> writes:
> Keith> +		  tmp = lookup_pointer_type (TYPE_TARGET_TYPE (type));
> Keith> +		  result = value_from_pointer (tmp,
> Keith> +					       value_as_long (v) + mem_offset);
> Keith> +		  return value_ind (result);
>
> Here I suspect it would be better to avoid the value_ind for some values
> of "noside".  Otherwise I think we could end up reading memory
> needlessly (by trying to compute the correct enclosing type).

I'm not entirely sure it is possible to actually get EVAL_SKIP here 
(EVAL_AVOID_SIDE_EFFECTS is handled already), but I've changed it to 
"bail" if we see anything other than EVAL_NORMAL.

I've also discovered a few other bugs related to this and added tests 
for those. This is concerning finding base classes which are templates.

As I'm sure you knwo, the code currently finds baseclasses via 
lookup_symbol returning a symobl for the constructor. This is what the 
workarounds in classify_name (and now classify_inner_name) are all 
about. These are necessary evils, unfortunately.

Since template constructors look like "CLASS<template, parameter, 
list>::CLASS (args)", lookup_symbol does not find the constructor.

So I've added a new function, find_type_baseclass_by_name which searches 
the base classes of the given type for a base class of the given name. 
Calling this in classify_inner_name in the appropriate places "makes 
everything work." (TM)

How does this look?

Keith

ChangeLog
2013-11-13  Keith Seitz  <keiths@redhat.com>

	PR c++/14819
	* c-exp.y (classify_inner_name): If no matching symbol was
	found, try looking up the token as a base class.
	Likewise if a constructor was found.
	* cp-namespace.c (find_type_baseclass_by_name): New function.
	* cp-support.h (find_type_baseclass_by_name): Declare.
	* valops.c (value_struct_elt_for_reference): If we get
	a non-static field, try to get a value based on the
	an implicit this pointer.

testsuite/ChangeLog
2013-11-13  Keith Seitz  <keiths@redhat.com>

	PR c++/14819
	* gdb.cp/impl-this.cc: New file.
	* gdb.cp/impl-this.exp: New file.


diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 77713dd..3ab8e32 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -2948,13 +2948,39 @@ classify_inner_name (const struct block *block, struct type *context)
 
   copy = copy_name (yylval.ssym.stoken);
   yylval.ssym.sym = cp_lookup_nested_symbol (type, copy, block);
+
+  /* If no symbol was found, search for a matching base class named
+     COPY.  This will allow users to enter qualified names of class members
+     relative to the `this' pointer.  */
   if (yylval.ssym.sym == NULL)
-    return ERROR;
+    {
+      struct type *base_type = find_type_baseclass_by_name (type, copy);
+
+      if (base_type != NULL)
+	{
+	  yylval.tsym.type = base_type;
+	  return TYPENAME;
+	}
+
+      return ERROR;
+    }
 
   switch (SYMBOL_CLASS (yylval.ssym.sym))
     {
     case LOC_BLOCK:
     case LOC_LABEL:
+      /* cp_lookup_nested_symbol might have accidentally found a constructor
+	 named COPY when we really wanted a base class of the same name.
+	 Double-check this case by looking for a base class.  */
+      {
+	struct type *base_type = find_type_baseclass_by_name (type, copy);
+
+	if (base_type != NULL)
+	  {
+	    yylval.tsym.type = base_type;
+	    return TYPENAME;
+	  }
+      }
       return ERROR;
 
     case LOC_TYPEDEF:
diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index 36134c0..adecf21 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -703,6 +703,33 @@ lookup_symbol_file (const char *name,
   return sym;
 }
 
+/* Search through the base classes of PARENT_TYPE for a base class
+   named NAME and return its type.  If not found, return NULL.  */
+
+struct type *
+find_type_baseclass_by_name (struct type *parent_type, const char *name)
+{
+  int i;
+
+  for (i = 0; i < TYPE_N_BASECLASSES (parent_type); ++i)
+    {
+      struct type *type = TYPE_BASECLASS (parent_type, i);
+      const char *base_name = TYPE_BASECLASS_NAME (parent_type, i);
+
+      if (base_name == NULL)
+	continue;
+
+      if (streq (base_name, name))
+	return type;
+
+      type = find_type_baseclass_by_name (type, name);
+      if (type != NULL)
+	return type;
+    }
+
+  return NULL;
+}
+
 /* Search through the base classes of PARENT_TYPE for a symbol named
    NAME in block BLOCK.  */
 
diff --git a/gdb/cp-support.h b/gdb/cp-support.h
index 0f2cebb..4358b23 100644
--- a/gdb/cp-support.h
+++ b/gdb/cp-support.h
@@ -220,6 +220,11 @@ extern struct symbol *cp_lookup_nested_symbol (struct type *parent_type,
 
 struct type *cp_lookup_transparent_type (const char *name);
 
+/* See description in cp-namespace.c.  */
+
+struct type *find_type_baseclass_by_name (struct type *parent_type,
+					  const char *name);
+
 /* Functions from cp-name-parser.y.  */
 
 extern struct demangle_parse_info *cp_demangled_name_to_comp
diff --git a/gdb/valops.c b/gdb/valops.c
index 8bff686..780d6be 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -3128,10 +3128,35 @@ value_struct_elt_for_reference (struct type *domain, int offset,
 	    return value_from_longest
 	      (lookup_memberptr_type (TYPE_FIELD_TYPE (t, i), domain),
 	       offset + (LONGEST) (TYPE_FIELD_BITPOS (t, i) >> 3));
-	  else if (noside == EVAL_AVOID_SIDE_EFFECTS)
+	  else if (noside != EVAL_NORMAL)
 	    return allocate_value (TYPE_FIELD_TYPE (t, i));
 	  else
-	    error (_("Cannot reference non-static field \"%s\""), name);
+	    {
+	      /* Try to evaluate NAME as a qualified name with implicit
+		 this pointer.  In this case, attempt to return the
+		 equivalent to `this->*(&TYPE::NAME)'.  */
+	      v = value_of_this_silent (current_language);
+	      if (v != NULL)
+		{
+		  struct value *ptr;
+		  long mem_offset;
+		  struct type *type, *tmp;
+
+		  ptr = value_aggregate_elt (domain, name, NULL, 1, noside);
+		  type = check_typedef (value_type (ptr));
+		  gdb_assert (type != NULL
+			      && TYPE_CODE (type) == TYPE_CODE_MEMBERPTR);
+		  tmp = lookup_pointer_type (TYPE_DOMAIN_TYPE (type));
+		  v = value_cast_pointers (tmp, v, 1);
+		  mem_offset = value_as_long (ptr);
+		  tmp = lookup_pointer_type (TYPE_TARGET_TYPE (type));
+		  result = value_from_pointer (tmp,
+					       value_as_long (v) + mem_offset);
+		  return value_ind (result);
+		}
+
+	      error (_("Cannot reference non-static field \"%s\""), name);
+	    }
 	}
     }
 
diff --git a/gdb/testsuite/gdb.cp/impl-this.cc b/gdb/testsuite/gdb.cp/impl-this.cc
new file mode 100644
index 0000000..abe6ffd
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/impl-this.cc
@@ -0,0 +1,99 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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/>.  */
+
+#ifdef DEBUG
+#include <stdio.h>
+#endif
+
+template <typename T>
+class A
+{
+public:
+  T i;
+  T z;
+  A () : i (1), z (10) {}
+};
+
+template <typename T>
+class B : public virtual A<T>
+{
+public:
+  T i;
+  B () : i (2) {}
+};
+
+class C : public virtual A<int>
+{
+public:
+  int i;
+  int c;
+  C () : i (3), c (30) {}
+};
+
+class D : public B<int>, public C
+{
+public:
+  int i;
+  int x;
+  D () : i (4), x (40) {}
+
+#ifdef DEBUG
+#define SUM(X)					\
+  do						\
+    {						\
+      sum += (X);				\
+      printf ("" #X " = %d\n", (X));		\
+    }						\
+  while (0)
+#else
+#define SUM(X) sum += (X)
+#endif
+
+int
+f (void)
+  {
+    int sum = 0;
+
+    SUM (i);
+    SUM (D::i);
+    SUM (D::B<int>::i);
+    SUM (B<int>::i);
+    SUM (D::C::i);
+    SUM (C::i);
+    SUM (D::B<int>::A<int>::i);
+    SUM (B<int>::A<int>::i);
+    SUM (A<int>::i);
+    SUM (D::C::A<int>::i);
+    SUM (C::A<int>::i);
+    SUM (D::x);
+    SUM (x);
+    SUM (D::C::c);
+    SUM (C::c);
+    SUM (c);
+    SUM (D::A<int>::i);
+
+    return sum;
+  }
+};
+
+int
+main (void)
+{
+  D d;
+
+  return d.f ();
+}
diff --git a/gdb/testsuite/gdb.cp/impl-this.exp b/gdb/testsuite/gdb.cp/impl-this.exp
new file mode 100644
index 0000000..7005d4c
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/impl-this.exp
@@ -0,0 +1,91 @@
+# Copyright 2013 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
+
+# Test expressions which assume an implicit "this" with a qualified
+# name.
+
+if {[skip_cplus_tests]} { continue }
+
+standard_testfile .cc
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}]} {
+    return -1
+}
+
+# First test expressions when there is no context.
+gdb_test "print i" "No symbol \"i\" in current context."
+gdb_test "print D::i" "Cannot reference non-static field \"i\""
+gdb_test "print D::B<int>::i" "Cannot reference non-static field \"i\""
+gdb_test "print B<int>::i" "Cannot reference non-static field \"i\""
+gdb_test "print D::C::i" "Cannot reference non-static field \"i\""
+gdb_test "print C::i" "Cannot reference non-static field \"i\""
+gdb_test "print D::B<int>::A<int>::i" "Cannot reference non-static field \"i\""
+gdb_test "print B<int>::A<int>::i" "Cannot reference non-static field \"i\""
+gdb_test "print A<int>::i" "Cannot reference non-static field \"i\""
+gdb_test "print D::C::A<int>::i" "Cannot reference non-static field \"i\""
+gdb_test "print C::A<int>::i" "Cannot reference non-static field \"i\""
+gdb_test "print D::x" "Cannot reference non-static field \"x\""
+gdb_test "print x" "No symbol \"x\" in current context."
+gdb_test "print D::C::c" "Cannot reference non-static field \"c\""
+gdb_test "print C::c" "Cannot reference non-static field \"c\""
+gdb_test "print c" "No symbol \"c\" in current context."
+gdb_test "print D::A<int>::i" "Cannot reference non-static field \"i\""
+
+# Run to D::f.
+if {![runto_main]} {
+    perror "couldn't run to main"
+    continue
+}
+
+gdb_breakpoint "D::f"
+gdb_continue_to_breakpoint "continue to D::f"
+
+# Now test valid expressions in the class hierarchy for D.
+gdb_test "print i" "= 4"
+gdb_test "print D::i" "= 4"
+gdb_test "print D::B<int>::i" "= 2"
+gdb_test "print B<int>::i" "= 2"
+gdb_test "print D::C::i" "= 3"
+gdb_test "print C::i" "= 3"
+gdb_test "print D::B<int>::A<int>::i" "= 1"
+gdb_test "print B<int>::A<int>::i" "= 1"
+gdb_test "print A<int>::i" "= 1"
+gdb_test "print D::C::A<int>::i" "= 1"
+gdb_test "print C::A<int>::i" "= 1"
+gdb_test "print D::x" "= 40"
+gdb_test "print x" "= 40"
+gdb_test "print D::C::c" "= 30"
+gdb_test "print C::c" "= 30"
+gdb_test "print c" "= 30"
+gdb_test "print D::A<int>::i" "= 1"
+
+# Test some invalid expressions
+gdb_test "print D::B<int>::c" "There is no field named c"
+gdb_test "print D::B<int>::A<int>::c" "There is no field named c"
+gdb_test "print D::C::A<int>::c" "There is no field named c"
+gdb_test "print B<int>::c" "There is no field named c"
+gdb_test "print B<int>::A<int>::c" "There is no field named c"
+gdb_test "print C::A<int>::c" "There is no field named c"
+gdb_test "print D::B<int>::x" "There is no field named x"
+gdb_test "print D::B<int>::A<int>::x" "There is no field named x"
+gdb_test "print B<int>::x" "There is no field named x"
+gdb_test "print B<int>::A<int>::x" "There is no field named x"
+gdb_test "print D::C::x" "There is no field named x"
+gdb_test "print C::x" "There is no field named x"
+gdb_test "print D::C::A<int>::x" "There is no field named x"
+gdb_test "print C::A<int>::x" "There is no field named x"
+