From patchwork Thu Dec 11 19:50:52 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Evans X-Patchwork-Id: 4196 Received: (qmail 32494 invoked by alias); 11 Dec 2014 19:51:48 -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 32483 invoked by uid 89); 11 Dec 2014 19:51:47 -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, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, KAM_STOCKGEN, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=no version=3.3.2 X-HELO: mail-pd0-f169.google.com Received: from mail-pd0-f169.google.com (HELO mail-pd0-f169.google.com) (209.85.192.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 11 Dec 2014 19:51:46 +0000 Received: by mail-pd0-f169.google.com with SMTP id z10so5667049pdj.0 for ; Thu, 11 Dec 2014 11:51:44 -0800 (PST) X-Received: by 10.70.49.77 with SMTP id s13mr20602702pdn.24.1418327503895; Thu, 11 Dec 2014 11:51:43 -0800 (PST) Received: from sspiff.org (173-13-178-50-sfba.hfc.comcastbusiness.net. [173.13.178.50]) by mx.google.com with ESMTPSA id bq7sm2092276pdb.50.2014.12.11.11.51.41 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 11 Dec 2014 11:51:43 -0800 (PST) Received: by sspiff.org (sSMTP sendmail emulation); Thu, 11 Dec 2014 11:50:52 -0800 From: Doug Evans To: gdb-patches@sourceware.org Subject: [PATCH] Remove redundant call to lookup_static_symbol. Date: Thu, 11 Dec 2014 11:50:52 -0800 Message-ID: MIME-Version: 1.0 X-IsSubscribed: yes Hi. Anytime you can remove a symbol lookup that loops over all objfiles is A Good Thing. The call to lookup_static_symbol in valops.c:value_maybe_namespace_elt is redundant with this call in cp_lookup_nested_symbol: /* Now search all static file-level symbols. We have to do this for things like typedefs in the class. We do not try to guess any imported namespace as even the fully specified namespace search is already not C++ compliant and more assumptions could make it too magic. */ size = strlen (parent_name) + 2 + strlen (nested_name) + 1; concatenated_name = alloca (size); xsnprintf (concatenated_name, size, "%s::%s", parent_name, nested_name); sym = lookup_static_symbol (concatenated_name, VAR_DOMAIN); if (sym != NULL) return sym; Earlier in value_maybe_namespace_elt we do this: sym = cp_lookup_symbol_namespace (namespace_name, name, get_selected_block (0), VAR_DOMAIN); That call goes like: value_maybe_namespace_elt -> cp_lookup_symbol_namespace -> cp_lookup_symbol_in_namespace -> lookup_symbol_file -> cp_lookup_nested_symbol -> lookup_static_symbol The call was added in commit 41f62f3939b1c69e68ef5652feb44fef90eb85c9. https://sourceware.org/ml/gdb-patches/2010-06/msg00663.html With a part 2 here: https://sourceware.org/ml/gdb-patches/2010-06/msg00664.html At the time the call to lookup_static_symbol (spelled lookup_static_symbol_aux at the time) was needed. However, this patch, 8dea366bbed7986295681c101dcfbd35aeb6dfc4, https://sourceware.org/ml/gdb-patches/2012-11/msg00387.html augmented lookup_symbol_file to call cp_lookup_nested_symbol and introduced the redundancy. [I'm not discounting the possibility that there's some subtle edge case I'm missing, with GDB that's always a possibility. But I can't find one, and we've got to make forward progress at cleaning gdb up.] It's kinda buried, so it's totally not unexpected that this happened. Part of what I want to do is make the symbol code cleaner so that it's easier to avoid such regressions in the future. That, plus spiffing up the performance testsuite to help catch these. [Though a symbol cache will prevent such regressions, but I don't want the cache to hide problems, so we'll want testing with/without it.] Regression tested on amd64-linux. Plus, tested by - removing both calls to lookup_static_symbol (value_maybe_namespace_elt and cp_lookup_nested_symbol), - seeing the testsuite failures (e.g., derivation.exp, namespace.exp, nsalias.exp), - putting the call in value_maybe_namespace_elt back, seeing some (but not all - not unexpectedly) pass again Also tested by turning on "set debug symbol-lookup 2" (see https://sourceware.org/ml/gdb-patches/2014-12/msg00255.html), and comparing the output. 2014-12-10 Doug Evans * valops.c (value_maybe_namespace_elt): Remove redundant call to lookup_static_symbol. diff --git a/gdb/valops.c b/gdb/valops.c index 4125fc0..2e3c9a1 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -3570,15 +3570,6 @@ value_maybe_namespace_elt (const struct type *curtype, get_selected_block (0), VAR_DOMAIN); if (sym == NULL) - { - char *concatenated_name = alloca (strlen (namespace_name) + 2 - + strlen (name) + 1); - - sprintf (concatenated_name, "%s::%s", namespace_name, name); - sym = lookup_static_symbol (concatenated_name, VAR_DOMAIN); - } - - if (sym == NULL) return NULL; else if ((noside == EVAL_AVOID_SIDE_EFFECTS) && (SYMBOL_CLASS (sym) == LOC_TYPEDEF))