From patchwork Tue Aug 27 09:53:57 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 34278 Received: (qmail 91684 invoked by alias); 27 Aug 2019 09:54:03 -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 91675 invoked by uid 89); 27 Aug 2019 09:54:03 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=FULL, lots, sk:andrew., sk:andrew X-HELO: mail-wm1-f65.google.com Received: from mail-wm1-f65.google.com (HELO mail-wm1-f65.google.com) (209.85.128.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 27 Aug 2019 09:54:01 +0000 Received: by mail-wm1-f65.google.com with SMTP id p74so2375695wme.4 for ; Tue, 27 Aug 2019 02:54:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=4W0im7fXu3xsm0wZQWDad80TYwtISyVbHvEd1cRUHWA=; b=gHsmvHOEnvBsf9dspdo3Oa7bgPCtXsYLYDrn4IfsDOQUcRa0ARcBQ4mDMan5gFLl5K maPRORMdwLx24rsqBzSQ2dAMmBHW5bFAVYMGHgQFH6kCI3kRx3cyQX0j8TUg1NBYSU5c aEY45L4lJjFFxwSTre9i/QG5bD5nyBG6WU3Rw0/fTFmzdvMh2JZYn7S6LHsTPlWf6kWl mkTphBJ4tKazEHEJzB5X06wEV1WjVkbjTgJLTu5Wb1mwoP5cWzppC1rFzuXb+9rTOASi hATIrypAVa/P6jOu0Qo3M4D8fEWZDK6ueobfmgB2nRu9YqxCdLITOeKhdIftm6rlrv3D mHrQ== Return-Path: Received: from localhost (host86-161-16-231.range86-161.btcentralplus.com. [86.161.16.231]) by smtp.gmail.com with ESMTPSA id z12sm12894587wrt.92.2019.08.27.02.53.58 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 27 Aug 2019 02:53:58 -0700 (PDT) Date: Tue, 27 Aug 2019 10:53:57 +0100 From: Andrew Burgess To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2 4/8] Search global block from basic_lookup_symbol_nonlocal Message-ID: <20190827095357.GO6076@embecosm.com> References: <20190801170412.5553-1-tromey@adacore.com> <20190801170412.5553-5-tromey@adacore.com> <20190821103225.GF6076@embecosm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190821103225.GF6076@embecosm.com> X-Fortune: I'm not prejudiced, I hate everyone equally. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes * Andrew Burgess [2019-08-21 11:32:25 +0100]: > * Tom Tromey [2019-08-01 11:04:08 -0600]: > > > This changes basic_lookup_symbol_nonlocal to look in the global block > > of the passed-in block. If no block was passed in, it reverts to the > > previous behavior. > > > > This change is needed to ensure that 'FILENAME'::NAME lookups work > > properly. As debugging Pedro's test case showed, this was not working > > properly in the case where multiple identical names could be found > > (the one situation where this feature is truly needed :-). > > > > This also removes some old comments from basic_lookup_symbol_nonlocal > > that no longer apply once this patch goes in. > > So I guess the tests for this are going to be in the > gdb.base/print-file-var.exp changes that are part of patch #8. It > would be great if the commit message could mention this - it just > makes life easier later on. > > I wonder if we need to update other *_lookup_symbol_nonlocal functions > in a similar way? For example can the C tests be compiled as C++, > which should cause GDB to use cp_lookup_symbol_nonlocal. > > Looking at both basic_lookup_symbol_nonlocal and > cp_lookup_symbol_nonlocal, I wonder if your fix could be moved into > lookup_global_symbol? And just have 'block_global_block (block)' > checked before the search of all global blocks? The patch below applies on top of this series and extends gdb.base/print-file-var.exp to compile as both C++ and C. The C++ will fail initially. The patch also includes a proposed fix to move the searching of the "current" global block into lookup_global_symbol, after this both the C++ and C versions of the test pass, and there are no other test regressions. This patch is going to clash with Christian Biesinger's patch to refactor lookup_global_symbol, but hopefully merging these two shouldn't be that hard. I also tweaked the test to remove some duplicate test names. What do you think? Thanks, Andrew diff --git a/gdb/symtab.c b/gdb/symtab.c index bd6fa35db6a..63a39e2996a 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -2426,22 +2426,6 @@ basic_lookup_symbol_nonlocal (const struct language_defn *langdef, if (result.symbol != NULL) return result; - /* If a block was passed in, we want to search the corresponding - global block now. This yields "more expected" behavior, and is - needed to support 'FILENAME'::VARIABLE lookups. */ - const struct block *global_block = block_global_block (block); - if (global_block != nullptr) - { - result.symbol = lookup_symbol_in_block (name, - symbol_name_match_type::FULL, - global_block, domain); - if (result.symbol != nullptr) - { - result.block = global_block; - return result; - } - } - /* If we didn't find a definition for a builtin type in the static block, search for it now. This is actually the right thing to do and can be a massive performance win. E.g., when debugging a program with lots of @@ -2662,6 +2646,22 @@ lookup_global_symbol (const char *name, if (objfile != NULL) result = solib_global_lookup (objfile, name, domain); + /* If a block was passed in, we want to search the corresponding + global block first. This yields "more expected" behavior, and is + needed to support 'FILENAME'::VARIABLE lookups. */ + const struct block *global_block = block_global_block (block); + if (global_block != nullptr) + { + result.symbol = lookup_symbol_in_block (name, + symbol_name_match_type::FULL, + global_block, domain); + if (result.symbol != nullptr) + { + result.block = global_block; + return result; + } + } + /* If that didn't work go a global search (of global blocks, heh). */ if (result.symbol == NULL) { diff --git a/gdb/testsuite/gdb.base/print-file-var-lib1.c b/gdb/testsuite/gdb.base/print-file-var-lib1.c index aec04a9b02b..d172c15bc7d 100644 --- a/gdb/testsuite/gdb.base/print-file-var-lib1.c +++ b/gdb/testsuite/gdb.base/print-file-var-lib1.c @@ -19,6 +19,8 @@ ATTRIBUTE_VISIBILITY int this_version_id = 104; +START_EXTERN_C + int get_version_1 (void) { @@ -26,3 +28,5 @@ get_version_1 (void) return this_version_id; } + +END_EXTERN_C diff --git a/gdb/testsuite/gdb.base/print-file-var-lib2.c b/gdb/testsuite/gdb.base/print-file-var-lib2.c index 4dfdfa04c99..b392aff9f3d 100644 --- a/gdb/testsuite/gdb.base/print-file-var-lib2.c +++ b/gdb/testsuite/gdb.base/print-file-var-lib2.c @@ -19,9 +19,13 @@ ATTRIBUTE_VISIBILITY int this_version_id = 203; +START_EXTERN_C + int get_version_2 (void) { printf ("get_version_2: &this_version_id=%p, this_version_id=%d\n", &this_version_id, this_version_id); return this_version_id; } + +END_EXTERN_C diff --git a/gdb/testsuite/gdb.base/print-file-var-main.c b/gdb/testsuite/gdb.base/print-file-var-main.c index 29d4fed22d1..1472bd44883 100644 --- a/gdb/testsuite/gdb.base/print-file-var-main.c +++ b/gdb/testsuite/gdb.base/print-file-var-main.c @@ -23,9 +23,13 @@ #include "print-file-var.h" +START_EXTERN_C + extern int get_version_1 (void); extern int get_version_2 (void); +END_EXTERN_C + #if VERSION_ID_MAIN ATTRIBUTE_VISIBILITY int this_version_id = 55; #endif diff --git a/gdb/testsuite/gdb.base/print-file-var.exp b/gdb/testsuite/gdb.base/print-file-var.exp index a37cca70de6..1a065cf568b 100644 --- a/gdb/testsuite/gdb.base/print-file-var.exp +++ b/gdb/testsuite/gdb.base/print-file-var.exp @@ -17,7 +17,7 @@ if {[skip_shlib_tests]} { return -1 } -proc test {hidden dlopen version_id_main} { +proc test {hidden dlopen version_id_main lang} { global srcdir subdir set main "print-file-var-main" @@ -32,7 +32,7 @@ proc test {hidden dlopen version_id_main} { set libobj1 [standard_output_file ${lib1}$suffix.so] set libobj2 [standard_output_file ${lib2}$suffix.so] - set lib_opts { debug additional_flags=-fPIC } + set lib_opts { debug additional_flags=-fPIC $lang } lappend lib_opts "additional_flags=-DHIDDEN=$hidden" if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib1}.c \ @@ -46,7 +46,7 @@ proc test {hidden dlopen version_id_main} { return -1 } - set main_opts [list debug shlib=${libobj1}] + set main_opts [list debug shlib=${libobj1} $lang] if {$dlopen} { lappend main_opts "shlib_load" \ @@ -108,12 +108,14 @@ proc test {hidden dlopen version_id_main} { # Compare the values of $sym1 and $sym2. proc compare {sym1 sym2} { - # Done this way instead of comparing the symbols with "print $sym1 - # == sym2" in GDB directly so that the values of the symbols end - # up visible in the logs, for debug purposes. - set vsym1 [get_integer_valueof $sym1 -1] - set vsym2 [get_integer_valueof $sym2 -1] - gdb_assert {$vsym1 == $vsym2} "$sym1 == $sym2" + with_test_prefix "sym1=$sym1,sym2=$sym2" { + # Done this way instead of comparing the symbols with "print $sym1 + # == sym2" in GDB directly so that the values of the symbols end + # up visible in the logs, for debug purposes. + set vsym1 [get_integer_valueof $sym1 -1] + set vsym2 [get_integer_valueof $sym2 -1] + gdb_assert {$vsym1 == $vsym2} "$sym1 == $sym2" + } } if $version_id_main { @@ -123,13 +125,14 @@ proc test {hidden dlopen version_id_main} { compare "'print-file-var-lib1.c'::this_version_id" "v1" compare "'print-file-var-lib2.c'::this_version_id" "v2" - } -foreach_with_prefix hidden {0 1} { - foreach_with_prefix dlopen {0 1} { - foreach_with_prefix version_id_main {0 1} { - test $hidden $dlopen $version_id_main +foreach_with_prefix lang { c c++ } { + foreach_with_prefix hidden {0 1} { + foreach_with_prefix dlopen {0 1} { + foreach_with_prefix version_id_main {0 1} { + test $hidden $dlopen $version_id_main $lang + } } } } diff --git a/gdb/testsuite/gdb.base/print-file-var.h b/gdb/testsuite/gdb.base/print-file-var.h index fe7a3460edb..c44e4848b4a 100644 --- a/gdb/testsuite/gdb.base/print-file-var.h +++ b/gdb/testsuite/gdb.base/print-file-var.h @@ -23,4 +23,12 @@ # define ATTRIBUTE_VISIBILITY #endif +#ifdef __cplusplus +# define START_EXTERN_C extern "C" { +# define END_EXTERN_C } +#else +# define START_EXTERN_C +# define END_EXTERN_C +#endif + #endif /* PRINT_FILE_VAR_H */