From patchwork Thu Jun 25 19:41:28 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Keith Seitz X-Patchwork-Id: 7351 Received: (qmail 87163 invoked by alias); 25 Jun 2015 19:41:33 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 87147 invoked by uid 89); 25 Jun 2015 19:41:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, KAM_STOCKGEN, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 25 Jun 2015 19:41:30 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id A2DDC3A4438 for ; Thu, 25 Jun 2015 19:41:29 +0000 (UTC) Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t5PJfTcC010910 for ; Thu, 25 Jun 2015 15:41:29 -0400 From: Keith Seitz To: gdb-patches@sourceware.org Subject: [PATCH] PR 16253 revisited Date: Thu, 25 Jun 2015 12:41:28 -0700 Message-Id: <1435261288-27308-1-git-send-email-keiths@redhat.com> X-IsSubscribed: yes This is a request for formal review of an earlier proposed workaround for c++/16253. A complete description of the proposal is below. Changes since proposal (with Doug's assistance -- THANKS DOUG!): - Add exact/best domain matching concept to block_lookup_symbol. - Add comment to block_lookup_symbol explaining why c++/16253 is not likely to affect blocks defined in functions. - Update tests to coverage test block_lookup_symbol. --- Last year a patch was submitted/approved/commited to eliminate symbol_matches_domain which was causing this problem. It was later reverted because it introduced a (severe) performance regression. Recap: (gdb) list 1 enum e {A,B,C} e; 2 int main (void) { return 0; } 3 (gdb) p e Attempt to use a type name as an expression The parser attempts to find a symbol named "e" of VAR_DOMAIN. This gets passed down through lookup_symbol and (eventually) into block_lookup_symbol_primary, which iterates over the block's dictionary of symbols: for (sym = dict_iter_name_first (block->dict, name, &dict_iter); sym != NULL; sym = dict_iter_name_next (name, &dict_iter)) { if (symbol_matches_domain (SYMBOL_LANGUAGE (sym), SYMBOL_DOMAIN (sym), domain)) return sym; } The problem here is that we have a symbol named "e" in both STRUCT_DOMAIN and VAR_DOMAIN, and for languages like C++, Java, and Ada, where a tag name may be used as an implicit typedef of the type, symbol_matches_domain ignores the difference between VAR_DOMAIN and STRUCT_DOMAIN. As it happens, the STRUCT_DOMAIN symbol is found first, considered a match, and that symbol is returned to the parser, eliciting the (now dreaded) error message. Since this bug exists specifically because we have both STRUCT and VAR_DOMAIN symbols in a given block/CU, this patch rather simply/naively changes block_lookup_symbol_primary so that it continues to search for an exact domain match on the symbol if symbol_matches_domain returns a symbol which does not exactly match the requested domain. This "fixes" the immediate problem, but admittedly might uncover other, related bugs. [Paranoia?] However, it causes no regressions (functional or performance) in the test suite. A similar change has been made to block_lookup_symbol for other cases in which this bug might appear. The tests from the previous submission have been resurrected and updated. However since we can still be given a matching symbol with a different domain than requested, we cannot say that a symbol "was not found." The error messages today will still be the (dreaded) "Attempt to use a type name..." ChangeLog PR 16253 * block.c (block_lookup_symbol): For non-function blocks, continue to search for a symbol with an exact domain match Otherwise, return any previously found "best domain" symbol. (block_lookup_symbol_primary): Likewise. testsuite/ChangeLog PR 16253 * gdb.cp/var-tag-2.cc: New file. * gdb.cp/var-tag-3.cc: New file. * gdb.cp/var-tag-4.cc: New file. * gdb.cp/var-tag.cc: New file. * gdb.cp/var-tag.exp: New file. --- gdb/ChangeLog | 9 ++++ gdb/block.c | 31 ++++++++--- gdb/testsuite/ChangeLog | 10 ++++ gdb/testsuite/gdb.cp/var-tag-2.cc | 22 ++++++++ gdb/testsuite/gdb.cp/var-tag-3.cc | 22 ++++++++ gdb/testsuite/gdb.cp/var-tag-4.cc | 22 ++++++++ gdb/testsuite/gdb.cp/var-tag.cc | 44 ++++++++++++++++ gdb/testsuite/gdb.cp/var-tag.exp | 105 ++++++++++++++++++++++++++++++++++++++ 8 files changed, 259 insertions(+), 6 deletions(-) create mode 100644 gdb/testsuite/gdb.cp/var-tag-2.cc create mode 100644 gdb/testsuite/gdb.cp/var-tag-3.cc create mode 100644 gdb/testsuite/gdb.cp/var-tag-4.cc create mode 100644 gdb/testsuite/gdb.cp/var-tag.cc create mode 100644 gdb/testsuite/gdb.cp/var-tag.exp diff --git a/gdb/ChangeLog b/gdb/ChangeLog index a52624b..0291355 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,12 @@ +2015-06-25 Keith Seitz + Doug Evans + + PR 16253 + * block.c (block_lookup_symbol): For non-function blocks, + continue to search for a symbol with an exact domain match + Otherwise, return any previously found "best domain" symbol. + (block_lookup_symbol_primary): Likewise. + 2015-06-24 Keith Seitz * build-id.c (build_id_to_debug_bfd): Add cleanup to free diff --git a/gdb/block.c b/gdb/block.c index 79a8f19..f7621aa 100644 --- a/gdb/block.c +++ b/gdb/block.c @@ -739,13 +739,21 @@ block_lookup_symbol (const struct block *block, const char *name, if (!BLOCK_FUNCTION (block)) { + struct symbol *other = NULL; + ALL_BLOCK_SYMBOLS_WITH_NAME (block, name, iter, sym) { + if (SYMBOL_DOMAIN (sym) == domain) + return sym; + /* This is a bit of a hack, but symbol_matches_domain might ignore + STRUCT vs VAR domain symbols. So if a matching symbol is found, + make sure there is no "better" matching symbol, i.e., one with + exactly the same domain. PR 16253. */ if (symbol_matches_domain (SYMBOL_LANGUAGE (sym), SYMBOL_DOMAIN (sym), domain)) - return sym; + other = sym; } - return NULL; + return other; } else { @@ -753,7 +761,10 @@ block_lookup_symbol (const struct block *block, const char *name, list; this loop makes sure to take anything else other than parameter symbols first; it only uses parameter symbols as a last resort. Note that this only takes up extra computation - time on a match. */ + time on a match. + It's hard to define types in the parameter list (at least in + C/C++) so we don't do the same PR 16253 hack here that is done + for the !BLOCK_FUNCTION case. */ struct symbol *sym_found = NULL; @@ -779,23 +790,31 @@ struct symbol * block_lookup_symbol_primary (const struct block *block, const char *name, const domain_enum domain) { - struct symbol *sym; + struct symbol *sym, *other; struct dict_iterator dict_iter; /* Verify BLOCK is STATIC_BLOCK or GLOBAL_BLOCK. */ gdb_assert (BLOCK_SUPERBLOCK (block) == NULL || BLOCK_SUPERBLOCK (BLOCK_SUPERBLOCK (block)) == NULL); + other = NULL; for (sym = dict_iter_name_first (block->dict, name, &dict_iter); sym != NULL; sym = dict_iter_name_next (name, &dict_iter)) { + if (SYMBOL_DOMAIN (sym) == domain) + return sym; + + /* This is a bit of a hack, but symbol_matches_domain might ignore + STRUCT vs VAR domain symbols. So if a matching symbol is found, + make sure there is no "better" matching symbol, i.e., one with + exactly the same domain. PR 16253. */ if (symbol_matches_domain (SYMBOL_LANGUAGE (sym), SYMBOL_DOMAIN (sym), domain)) - return sym; + other = sym; } - return NULL; + return other; } /* See block.h. */ diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index c78dd29..d60139b 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,13 @@ +2015-06-25 Keith Seitz + Doug Evans + + PR 16253 + * gdb.cp/var-tag-2.cc: New file. + * gdb.cp/var-tag-3.cc: New file. + * gdb.cp/var-tag-4.cc: New file. + * gdb.cp/var-tag.cc: New file. + * gdb.cp/var-tag.exp: New file. + 2015-06-24 Peter Bergner * gdb.arch/powerpc-power.exp : Fixup test results. diff --git a/gdb/testsuite/gdb.cp/var-tag-2.cc b/gdb/testsuite/gdb.cp/var-tag-2.cc new file mode 100644 index 0000000..7733473 --- /dev/null +++ b/gdb/testsuite/gdb.cp/var-tag-2.cc @@ -0,0 +1,22 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2015 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 . */ + +/* This object is in a separate file so that its debug info is not + expanded at startup. Once debug info is expanded we are no longer + exercising block_lookup_symbol, and instead are exercising + block_lookup_symbol_primary. */ +enum E2 {a2, b2, c2} E2; diff --git a/gdb/testsuite/gdb.cp/var-tag-3.cc b/gdb/testsuite/gdb.cp/var-tag-3.cc new file mode 100644 index 0000000..7f2133f --- /dev/null +++ b/gdb/testsuite/gdb.cp/var-tag-3.cc @@ -0,0 +1,22 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2015 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 . */ + +/* This object is in a separate file so that its debug info is not + expanded at startup. Once debug info is expanded we are no longer + exercising block_lookup_symbol, and instead are exercising + block_lookup_symbol_primary. */ +struct S2 {} S2; diff --git a/gdb/testsuite/gdb.cp/var-tag-4.cc b/gdb/testsuite/gdb.cp/var-tag-4.cc new file mode 100644 index 0000000..162541c --- /dev/null +++ b/gdb/testsuite/gdb.cp/var-tag-4.cc @@ -0,0 +1,22 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2015 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 . */ + +/* This object is in a separate file so that its debug info is not + expanded at startup. Once debug info is expanded we are no longer + exercising block_lookup_symbol, and instead are exercising + block_lookup_symbol_primary. */ +union U2 {int a; char b;} U2; diff --git a/gdb/testsuite/gdb.cp/var-tag.cc b/gdb/testsuite/gdb.cp/var-tag.cc new file mode 100644 index 0000000..93b9caf --- /dev/null +++ b/gdb/testsuite/gdb.cp/var-tag.cc @@ -0,0 +1,44 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2014 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +int global = 3; + +class C { +public: + struct C1 {} C1; + enum E1 {a1, b1, c1} E1; + union U1 {int a1; char b1;} U1; + + C () : E1 (b1) {} + void global (void) const {} + int f (void) const { global (); return 0; } +} C; + +struct S {} S; +enum E {a, b, c} E; +union U {int a; char b;} U; + +class CC {} cc; +struct SS {} ss; +enum EE {ea, eb, ec} ee; +union UU {int aa; char bb;} uu; + +int +main (void) +{ + return C.f (); +} diff --git a/gdb/testsuite/gdb.cp/var-tag.exp b/gdb/testsuite/gdb.cp/var-tag.exp new file mode 100644 index 0000000..30aab99 --- /dev/null +++ b/gdb/testsuite/gdb.cp/var-tag.exp @@ -0,0 +1,105 @@ +# Copyright 2014, 2015 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 . + +# This file is part of the gdb testsuite + +# Test expressions in which variable names shadow tag names. + +if {[skip_cplus_tests]} { continue } + +standard_testfile var-tag.cc var-tag-2.cc var-tag-3.cc var-tag-4.cc + +if {[prepare_for_testing $testfile.exp $testfile \ + [list $srcfile $srcfile2 $srcfile3 $srcfile4] {debug c++}]} { + return -1 +} + +proc do_global_tests {lang} { + set invalid_print "Attempt to use a type name as an expression" + set ptypefmt "type = (class|enum|union|struct) %s {.*}" + + with_test_prefix $lang { + gdb_test_no_output "set language $lang" + gdb_test "ptype C" "type = class C {.*}" + gdb_test "print E" "= a" + gdb_test "ptype E" "type = enum E {.*}" + gdb_test "print S" "= {}" + gdb_test "ptype S" "type = struct S {.*}" + gdb_test "print U" "= {.*}" + gdb_test "ptype U" "type = union U {.*}" + gdb_test "print cc" "= {.*}" + gdb_test "ptype cc" "type = class CC {.*}" + gdb_test "print CC" [format $invalid_print "CC"] + gdb_test "ptype CC" [format $ptypefmt "CC"] + gdb_test "print ss" "= {}" + gdb_test "ptype ss" "type = struct SS {.*}" + gdb_test "print SS" [format $invalid_print "SS"] + gdb_test "ptype SS" [format $ptypefmt "SS"] + gdb_test "print ee" "= .*" + gdb_test "ptype ee" "type = enum EE {.*}" + gdb_test "print EE" [format $invalid_print "EE"] + gdb_test "ptype EE" [format $ptypefmt "EE"] + gdb_test "print uu" "= {.*}" + gdb_test "ptype uu" "type = union UU {.*}" + gdb_test "print UU" [format $invalid_print "UU"] + gdb_test "ptype UU" [format $ptypefmt "UU"] + + # These tests exercise lookup of symbols using the "quick fns" API. + # Each of them is in a separate CU as once its CU is expanded, + # we're no longer using the quick fns API. + gdb_test "print E2" "= a2" + gdb_test "ptype E2" "type = enum E2 {.*}" + gdb_test "print S2" "= {}" + gdb_test "ptype S2" "type = struct S2 {.*}" + gdb_test "print U2" "= {.*}" + gdb_test "ptype U2" "type = union U2 {.*}" + } +} + +# First test expressions when there is no context. +with_test_prefix "before start" { + do_global_tests c++ + do_global_tests c +} + +# Run to main and test again. +if {![runto_main]} { + perror "couldn't run to main" + continue +} + +with_test_prefix "in main" { + do_global_tests c++ + do_global_tests c +} + +# Run to C::f and test again. +gdb_breakpoint "C::f" +gdb_continue_to_breakpoint "continue to C::f" +with_test_prefix "in C::f" { + do_global_tests c++ + do_global_tests c +} + +# Another hard-to-guess-the-users-intent bug... +# It would be really nice if we could query the user! +with_test_prefix "global collision" { + gdb_test_no_output "set language c++" + setup_kfail "c++/16463" "*-*-*" + gdb_test "print global" "= 3" + + # ... with a simple workaround: + gdb_test "print ::global" "= 3" +}