* [RFA] Fix c++/14819 (implicit this) @ 2013-11-06 5:24 Keith Seitz 2013-11-06 13:50 ` Jan Kratochvil 2013-11-06 17:44 ` Tom Tromey 0 siblings, 2 replies; 8+ messages in thread From: Keith Seitz @ 2013-11-06 5:24 UTC (permalink / raw) To: gdb-patches@sourceware.org ml [-- Attachment #1: Type: text/plain, Size: 1218 bytes --] Hi, This is an attempt to address c++/14819. The simplest reproducer is: class C { public: int x; C():x(42) {} int f() { return C::x; } } c; int main() { c.f(); } (gdb) print C::x A syntax error in expression, near `x'. This is happening because all qualified names like this go through value_aggregate_elt, which dispatches structs and classes to value_struct_elt_for_reference. That function does not allow references to non-static member data. The solution I've implemented (as explained in the comments) is to treat a qualified name on non-static member with an implicit `this'. So the qualified name is looked up in the current this pointer (if any), and a value is computed as if the user had typed, "this->*(&QUALIFIED_NAME)". No regressions on native x86_64-linux or native-gdbserver. Comments, questions, concerns? Keith ChangeLog 2013-11-05 Keith Seitz <keiths@redhat.com> PR c++/14819 * 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-05 Keith Seitz <keiths@redhat.com> PR c++/14819 * gdb.cp/impl-this.cc: New file. * gdb.cp/impl-this.exp: New file. [-- Attachment #2: implicit-this.patch --] [-- Type: text/x-patch, Size: 5234 bytes --] diff --git a/gdb/valops.c b/gdb/valops.c index 8bff686..2f3ead8 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -3131,7 +3131,32 @@ value_struct_elt_for_reference (struct type *domain, int offset, else if (noside == EVAL_AVOID_SIDE_EFFECTS) 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 (t, 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..5b52f24 --- /dev/null +++ b/gdb/testsuite/gdb.cp/impl-this.cc @@ -0,0 +1,52 @@ +/* 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/>. */ + +class A +{ +public: + int a; + int x, y, z; + A (void) : a (11), x (14), y (15), z (16) {} +}; + +class B : public A +{ +public: + int a, b; + int x, y; + int solo; + B (void) : a (21), b (22), x (24), y (25), solo (100) {} +}; + +class C : public B +{ +public: + int a, b, c; + int x; + C (void) : a (31), b (32), c (33), x (34) {} + int f (void) const + { + return (C::a + B::a + C::a - C::b - B::b + C::c + - C::x - B::x - C::x + B::y + A::y - A::z); /* break here */ + } +} c; + +int +main (void) +{ + return c.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..679b8ce --- /dev/null +++ b/gdb/testsuite/gdb.cp/impl-this.exp @@ -0,0 +1,67 @@ +# 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 +} + +if {![runto_main]} { + perror "couldn't run to main" + continue +} + +gdb_test "print A::a" "Cannot reference non-static field \"a\"" +gdb_test "print A::b" "There is no field named b" + +gdb_breakpoint [gdb_get_line_number "break here"] +gdb_continue_to_breakpoint "run to C::f" + +gdb_test "print A::a" "= 11" +gdb_test "print A::b" "There is no field named b" +gdb_test "print A::c" "There is no field named c" +gdb_test "print A::x" "= 14" +gdb_test "print A::y" "= 15" +gdb_test "print A::z" "= 16" + +gdb_test "print B::a" "= 21" +gdb_test "print B::b" "= 22" +gdb_test "print B::c" "There is no field named c" +gdb_test "print B::x" "= 24" +gdb_test "print B::y" "= 25" +gdb_test "print B::z" "= 16"; # B inherits from A! +gdb_test "print B::solo" "= 100" + +gdb_test "print C::a" "= 31" +gdb_test "print C::b" "= 32" +gdb_test "print C::c" "= 33" +gdb_test "print C::x" "= 34" +gdb_test "print C::y" "= 25"; # C inherits from B! +gdb_test "print C::z" "= 16"; # C inherits from B and B from A! + +gdb_test "print a" "= 31" +gdb_test "print b" "= 32" +gdb_test "print c" "= 33" +gdb_test "print x" "= 34" +gdb_test "print y" "= 25"; # C inherits from B! +gdb_test "print z" "= 16"; # C inherits from B and B from A! ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix c++/14819 (implicit this) 2013-11-06 5:24 [RFA] Fix c++/14819 (implicit this) Keith Seitz @ 2013-11-06 13:50 ` Jan Kratochvil 2013-11-06 17:44 ` Tom Tromey 1 sibling, 0 replies; 8+ messages in thread From: Jan Kratochvil @ 2013-11-06 13:50 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches@sourceware.org ml Hi Keith, while the solution seems pretty tricky (some more comments what each line does would be helpful) it does not work for 'p B::i' and 'p C::i'. But these could be fixed I guess, I did not try to do so. ------------------------------------------------------------------------------ class A { public: int i; A(int i_):i(i_) {} }; class B:public A { public: B():A(1) {} }; class C:public A { public: C():A(2) {} }; class D:public B,public C { public: int f() { return B::i + C::i; // return i; // error: reference to ‘i’ is ambiguous // return A::i; // error: ‘A’ is an ambiguous base of ‘D’ } }; D d; int main() { return d.f(); } ------------------------------------------------------------------------------ (gdb) p B::i base class 'A' is ambiguous in type 'D' - this is bug (gdb) p C::i base class 'A' is ambiguous in type 'D' - this is bug (gdb) p A::i base class 'A' is ambiguous in type 'D' - this is correct (gdb) p i $1 = 2 - this seems already filed as: ambiguous using reference is not error http://sourceware.org/bugzilla/show_bug.cgi?id=11540 ------------------------------------------------------------------------------ On Tue, 05 Nov 2013 22:08:58 +0100, Keith Seitz wrote: > --- a/gdb/valops.c > +++ b/gdb/valops.c > @@ -3131,7 +3131,32 @@ value_struct_elt_for_reference (struct type *domain, int offset, > else if (noside == EVAL_AVOID_SIDE_EFFECTS) > 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; LONGEST, it is returned by value_as_long. > + struct type *type, *tmp; > + > + ptr = value_aggregate_elt (t, name, NULL, 1, noside); I cannot reproduce it but I find it a bit fragile, value_aggregate_elt can return NULL (although it probably would not get here in such case). At least gdb_assert would be good. > + 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); > + } > } > } Thanks, Jan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix c++/14819 (implicit this) 2013-11-06 5:24 [RFA] Fix c++/14819 (implicit this) Keith Seitz 2013-11-06 13:50 ` Jan Kratochvil @ 2013-11-06 17:44 ` Tom Tromey 2013-11-14 1:01 ` Keith Seitz 1 sibling, 1 reply; 8+ messages in thread From: Tom Tromey @ 2013-11-06 17:44 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches@sourceware.org ml >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: Keith> + /* Try to evaluate NAME as a qualified name with implicit Keith> + this pointer. In this case, attempt to return the Keith> + equivalent to `this->*(&TYPE::NAME)'. */ Nice approach, I totally didn't think of it. I think it would be good to have a test case involving a virtual base class, preferably one where the virtual base ends up in two different places in different classes (and then test both cases). 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). Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix c++/14819 (implicit this) 2013-11-06 17:44 ` Tom Tromey @ 2013-11-14 1:01 ` Keith Seitz 2013-11-15 17:57 ` Tom Tromey 0 siblings, 1 reply; 8+ messages in thread From: Keith Seitz @ 2013-11-14 1:01 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches@sourceware.org ml [-- Attachment #1: Type: text/plain, Size: 2134 bytes --] 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. [-- Attachment #2: implicit-this-2.patch --] [-- Type: text/x-patch, Size: 10448 bytes --] 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" + ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix c++/14819 (implicit this) 2013-11-14 1:01 ` Keith Seitz @ 2013-11-15 17:57 ` Tom Tromey 2013-11-21 19:25 ` Keith Seitz 0 siblings, 1 reply; 8+ messages in thread From: Tom Tromey @ 2013-11-15 17:57 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches@sourceware.org ml >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: Keith> I'm not entirely sure it is possible to actually get EVAL_SKIP Keith> here (EVAL_AVOID_SIDE_EFFECTS is handled already), but I've Keith> changed it to "bail" if we see anything other than EVAL_NORMAL. Yeah. Ideally I think we wouldn't let EVAL_* escape eval.c. Not your problem though. Keith> So I've added a new function, find_type_baseclass_by_name which Keith> searches the base classes of the given type for a base class of the Keith> given name. Calling this in classify_inner_name in the appropriate Keith> places "makes everything work." (TM) This seems like a fine idea to me. 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? 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? 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? 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. Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix c++/14819 (implicit this) 2013-11-15 17:57 ` Tom Tromey @ 2013-11-21 19:25 ` Keith Seitz 2013-11-21 21:12 ` Tom Tromey 0 siblings, 1 reply; 8+ messages in thread From: Keith Seitz @ 2013-11-21 19:25 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches@sourceware.org ml [-- Attachment #1: Type: text/plain, Size: 4091 bytes --] 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. [-- Attachment #2: implicit-this-2.patch --] [-- Type: text/x-patch, Size: 12828 bytes --] 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'" +} ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix c++/14819 (implicit this) 2013-11-21 19:25 ` Keith Seitz @ 2013-11-21 21:12 ` Tom Tromey 2013-11-26 1:25 ` Keith Seitz 0 siblings, 1 reply; 8+ messages in thread From: Tom Tromey @ 2013-11-21 21:12 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches@sourceware.org ml >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: Keith> 2013-11-21 Keith Seitz <keiths@redhat.com> Keith> PR c++/14819 Keith> * c-exp.y (classify_inner_name): If no matching symbol was Keith> found, try looking up the token as a base class. Keith> Likewise if a constructor was found. Keith> * cp-namespace.c (find_type_baseclass_by_name): New function. Keith> * cp-support.h (find_type_baseclass_by_name): Declare. Keith> * valops.c (value_struct_elt_for_reference): If we get Keith> a non-static field, try to get a value based on the Keith> current instance, if any. Keith> testsuite/ChangeLog Keith> 2013-11-21 Keith Seitz <keiths@redhat.com> Keith> PR c++/14819 Keith> * gdb.cp/impl-this.cc: New file. Keith> * gdb.cp/impl-this.exp: New file. Thanks, Keith. I appreciate your research into my questions. Keith> +struct type * Keith> +find_type_baseclass_by_name (struct type *parent_type, const char *name) Keith> +{ Keith> + int i; Keith> + Keith> + CHECK_TYPEDEF (parent_type); Keith> + for (i = 0; i < TYPE_N_BASECLASSES (parent_type); ++i) Keith> + { Keith> + struct type *type = TYPE_BASECLASS (parent_type, i); I think a check_typedef call is needed here. Ok with that change. Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix c++/14819 (implicit this) 2013-11-21 21:12 ` Tom Tromey @ 2013-11-26 1:25 ` Keith Seitz 0 siblings, 0 replies; 8+ messages in thread From: Keith Seitz @ 2013-11-26 1:25 UTC (permalink / raw) To: gdb-patches@sourceware.org ml [-- Attachment #1: Type: text/plain, Size: 1222 bytes --] On 11/21/2013 01:06 PM, Tom Tromey wrote: > Keith> +struct type * > Keith> +find_type_baseclass_by_name (struct type *parent_type, const char *name) > Keith> +{ > Keith> + int i; > Keith> + > Keith> + CHECK_TYPEDEF (parent_type); > Keith> + for (i = 0; i < TYPE_N_BASECLASSES (parent_type); ++i) > Keith> + { > Keith> + struct type *type = TYPE_BASECLASS (parent_type, i); > > I think a check_typedef call is needed here. > > Ok with that change. I've added that (it shouldn't hurt) and committed the patch (which I am also attaching for reference). Thank you for the review. Keith ChangeLog 2013-11-25 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-25 Keith Seitz <keiths@redhat.com> PR c++/14819 * gdb.cp/impl-this.cc: New file. * gdb.cp/impl-this.exp: New file. [-- Attachment #2: implicit-this-final.patch --] [-- Type: text/x-patch, Size: 12844 bytes --] 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..d0520bd 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 = check_typedef (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,1 +1,131 @@ +# 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'" +} ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-11-25 21:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-11-06 5:24 [RFA] Fix c++/14819 (implicit this) Keith Seitz 2013-11-06 13:50 ` Jan Kratochvil 2013-11-06 17:44 ` Tom Tromey 2013-11-14 1:01 ` Keith Seitz 2013-11-15 17:57 ` Tom Tromey 2013-11-21 19:25 ` Keith Seitz 2013-11-21 21:12 ` Tom Tromey 2013-11-26 1:25 ` Keith Seitz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).