This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 11/15/2013 09:48 AM, Tom Tromey wrote:
"Keith" == Keith Seitz <keiths@redhat.com> writes:
Keith> +      /* cp_lookup_nested_symbol might have accidentally found a constructor
Keith> +	 named COPY when we really wanted a base class of the same name.
Keith> +	 Double-check this case by looking for a base class.  */
Keith> +      {
Keith> +	struct type *base_type = find_type_baseclass_by_name (type, copy);
Keith> +
Keith> +	if (base_type != NULL)
Keith> +	  {
Keith> +	    yylval.tsym.type = base_type;
Keith> +	    return TYPENAME;
Keith> +	  }

I wonder what happens here if you actually do want the constructor.
Like "print B::B".  Will it still find the baseclass?

cp_lookup_nested_symbol will find the constructor's symbol, and this whole block is bypassed.

Keith> +/* Search through the base classes of PARENT_TYPE for a base class
Keith> +   named NAME and return its type.  If not found, return NULL.  */
Keith> +
Keith> +struct type *
Keith> +find_type_baseclass_by_name (struct type *parent_type, const char *name)
Keith> +{
Keith> +  int i;
Keith> +
Keith> +  for (i = 0; i < TYPE_N_BASECLASSES (parent_type); ++i)

Probably this function requires check_typedef sprinkled liberally
around.

Keith> +    {
Keith> +      struct type *type = TYPE_BASECLASS (parent_type, i);
Keith> +      const char *base_name = TYPE_BASECLASS_NAME (parent_type, i);
Keith> +
Keith> +      if (base_name == NULL)
Keith> +	continue;
Keith> +
Keith> +      if (streq (base_name, name))
Keith> +	return type;

It is possible to have ambiguous base classes?
And if so, where is that diagnosed?

Yes, it is possible, but we're not interested in ascertaining that here. As far as the parser is concerned, we simply need to know whether the string represents a type name.

The case of ambiguous base classes is handled by value_cast_pointers which calls search_struct_field, which notices the ambiguity and errors.

This probably isn't ideal should some other developer use this function later for something other than parsing, but it is currently sufficient.

I've added tests for ambiguous bases.

Another horrible thought is what does "this->typedef_name::field" mean?
Anything?  What is typedef_name is a typedef for one of the base classes?

If I'm reading the standard correctly, this is not possible (at least not in the way I think you mean). 3.4.5 Class Member Access #4 says:

"If the id-expression in a class member access is a qualified-id of the form:

  class-name-or-namespace-name::...

the class-name-or-namespace-name following the . or -> operator is first looked up in the class of the object expression and the name, if found, is used. Otherwise it is looked up in the context of the entire postfix-expression." -- n3290 draft

So for example (according to my reading),

class A {};
typedef A AA;
class B : public AA {};

B::A::whatever <- valid
B::AA::whatever <- not valid - "B" does not define anything called "AA".

Now if the user adds a typedef into B for AA, then the above expression would be valid.

I've added tests to make sure that gdb enforces this behavior (which it already does).

Keith> +gdb_test "print D::i" "Cannot reference non-static field \"i\""

Keith> +gdb_test "print D::i" "= 4"

Duplicated test names.
I think there are more than one.
It's fine to just wrap the sequences in with_test_prefix though.

Fixed.

Keith

ChangeLog
2013-11-21  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
	current instance, if any.

testsuite/ChangeLog
2013-11-21  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 5d4cd81..03af9e7 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -2960,13 +2960,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..a23dec1 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -703,6 +703,34 @@ 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;
+
+  CHECK_TYPEDEF (parent_type);
+  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 4fc57ec..8e7b16f 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..6be0ddf
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/impl-this.cc
@@ -0,0 +1,135 @@
+/* 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;
+  T common;
+  B () : i (2), common (200) {}
+};
+
+typedef B<int> Bint;
+
+class C : public virtual A<int>
+{
+public:
+  int i;
+  int c;
+  int common;
+  C () : i (3), c (30), common (300) {}
+};
+
+class BB : public A<int>
+{
+public:
+  int i;
+  BB () : i (20) {}
+};
+
+class CC : public A<int>
+{
+public:
+  int i;
+  CC () : i (30) {}
+};
+
+class Ambig : public BB, public CC
+{
+public:
+  int i;
+  Ambig () : i (1000) {}
+};
+
+class D : public Bint, public C
+{
+public:
+  int i;
+  int x;
+  Ambig am;
+  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);
+    SUM (Bint::i);
+    //SUM (D::Bint::i);
+    //SUM (D::Bint::A<int>::i);
+    SUM (Bint::A<int>::i);
+    // ambiguous: SUM (common);
+    SUM (B<int>::common);
+    SUM (C::common);
+    SUM (am.i);
+    // ambiguous: SUM (am.A<int>::i);
+
+    return sum;
+  }
+};
+
+int
+main (void)
+{
+  Bint b;
+  D d;
+
+  return d.f () + b.i;
+}
diff --git a/gdb/testsuite/gdb.cp/impl-this.exp b/gdb/testsuite/gdb.cp/impl-this.exp
new file mode 100644
index 0000000..ce7c780
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/impl-this.exp
@@ -0,0 +1,130 @@
+# 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.
+with_test_prefix "before run" {
+    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.
+with_test_prefix "at D::f (valid expressions)" {
+    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::Bint::i" \
+	"No type \"Bint\" within class or namespace \"D\"."
+    gdb_test "print Bint::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 D::Bint::A<int>::i" \
+	"No type \"Bint\" within class or namespace \"D\"."
+    gdb_test "print Bint::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
+with_test_prefix "at D::f (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::Bint::c" \
+	"No type \"Bint\" within class or namespace \"D\"."
+
+    gdb_test "print D::Bint::A<int>::c" \
+	"No type \"Bint\" within class or namespace \"D\"."
+    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 Bint::c" "There is no field named c"
+    gdb_test "print Bint::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 D::Bint::x" \
+	"No type \"Bint\" within class or namespace \"D\"."
+    gdb_test "print D::Bint::A<int>::x" \
+	"No type \"Bint\" within class or namespace \"D\"."
+    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 Bint::x" "There is no field named x"
+    gdb_test "print Bint::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"
+}
+
+# Test some ambiguous names
+with_test_prefix "at D::f (ambiguous names)" {
+    gdb_test "print B<int>::common" " = 200"
+    gdb_test "print Bint::common" " = 200"
+    gdb_test "print C::common" " = 300"
+    gdb_test "print am.i" " = 1000"
+    gdb_test "print am.A<int>::i" \
+	"base class 'A<int>' is ambiguous in type 'Ambig'"
+    gdb_test "print am.BB::A<int>::i" \
+	"base class 'A<int>' is ambiguous in type 'Ambig'"
+    gdb_test "print am.CC::A<int>::i" \
+	"base class 'A<int>' is ambiguous in type 'Ambig'"
+}

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]