From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 75243 invoked by alias); 23 Sep 2019 14:17:01 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 75235 invoked by uid 89); 23 Sep 2019 14:17:01 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-12.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=scary, truly, pascal, Pascal X-HELO: mail-wr1-f67.google.com Received: from mail-wr1-f67.google.com (HELO mail-wr1-f67.google.com) (209.85.221.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 23 Sep 2019 14:16:59 +0000 Received: by mail-wr1-f67.google.com with SMTP id v8so14187464wrt.2 for ; Mon, 23 Sep 2019 07:16:59 -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=apOjQIRuXDyQEJjk3VJRyzyok50ANdczvVBoQvA/TXc=; b=EC3v/2NS/3/rFiJeDJ8xw+dmc+NeRcIiYxsoCWXsWHzof3j9Vd3nOXxawDb042VhVk yGZK6iWOQ7omGF5VzB+nfk2kiNiKAjTq4j5mk3zQfgCMXa7HTSy51voEFHLofnME0VZL epl7ixJbJGZnFAI3UUO7rSIDjhXoBMTlVsJb8MuVRBiWK1w7s0Oitq4nDuCrTmzHhWSl zqLHO/doRuzxWLPADN981CFeTE07QMqsxF6Hdlt/Nh9rnyN0QmSpxPnfOY6dno4HV0Of 2d/P1KiMRq9pRpLuB2hfThMph0N4/FWOeo7BejVkpxZL6lrlMBsvhwi6tYEkn0PB5NMn /KHg== Return-Path: Received: from localhost (host86-161-16-231.range86-161.btcentralplus.com. [86.161.16.231]) by smtp.gmail.com with ESMTPSA id y3sm11257380wrw.83.2019.09.23.07.16.56 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 23 Sep 2019 07:16:56 -0700 (PDT) Date: Mon, 23 Sep 2019 14:17:00 -0000 From: Andrew Burgess To: Christian Biesinger Cc: Tom Tromey , gdb-patches Subject: Re: [PATCH v2 2/8] Search global block from basic_lookup_symbol_nonlocal Message-ID: <20190923141655.GH4962@embecosm.com> References: <20190920192017.15293-1-tromey@adacore.com> <20190920192017.15293-3-tromey@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Fortune: There's just something I don't like about Virginia User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00445.txt.bz2 * Christian Biesinger [2019-09-21 13:31:30 +0900]: > On Sat, Sep 21, 2019 at 4:20 AM Tom Tromey wrote: > > From: Andrew Burgess > > > > This changes lookup_global_symbol 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 further extends the situations where lookup_global_symbols checks > a local scope first (currently only objfile) but lookup_static_symbol > doesn't. [ Note, in the below I talk about "my patch", I really mean mine and Tom's as my suggested rework was based on his original patch. ] I didn't fully understand this for two reasons, first, you say "further extends", however, both lookup_global_symbol and lookup_static_symbol pre-patch, both just call lookup_global_or_static_symbol. And I couldn't see any code in lookup_global_or_static_symbol that was conditional on global or static lookup. So my question is where is the existing situation in which lookup_global_symbol searches a local scope first? I ask only so I can try to understand your question and my change in relation to the existing code. Second, I wanted to better understand what you mean by "local scope". My understanding of lookup_global_symbol is that it searches every global block from every object file in some particular order. The change here is simply that it makes more sense to search the global scope relating to the current compilation unit before searching other global scopes. I can fully understand that this can be referred to as a "local scope", I just wanted to make sure we're all agreed on what's happening here. > Is that really correct? I think it makes sense. If we have multiple global symbols with the same name, we should find the one corresponding to the current scope I think. That feels like what I'd expect to happen. And especially as when we do a FILE::VAR lookup we end up in lookup_global_symbol with the block corresponding to FILE. If we then ignore that block and search all global scopes in the predefined order then we will always find the same global symbol, which is certainly wrong. > I would think that > lookup_static_symbol is even more likely to need that check, since > static symbols are probably (?) more likely to share the same name. Is > my interpretation wrong? No, I think you are very right.... But... There are currently 5 uses of lookup_static_symbol that I could find, 2 of these are in: d-namespace.c:find_symbol_in_baseclass cp-namespace.c:cp_lookup_nested_symbol_1 These are interesting because almost immediately before calling lookup_static_symbol we call lookup_symbol_in_static_block, which I think is solving the same problem as this proposed patch. Then we have a call to lookup_static_symbol in: symtab.c:lookup_symbol_aux This calls lookup_static_symbol as the last fallback action after calling the language specific hook la_lookup_symbol_nonlocal. I've only audited some languages, but for those I've looked at they all call lookup_symbol_in_static_block as one of their early actions: symtab.c:basic_lookup_symbol_nonlocal (for C, Pascal) cp-namespace.c:cp_lookup_bare_symbol (for C++, Fortran) cp-namespace.c:cp_lookup_bare_symbol (for Fortran) d-namespace.c:d_lookup_symbol (for D) rust-lang.c:rust_lookup_symbol_nonlocal (for Rust) That leaves two uses of lookup_static_symbol, these are: python/py-symbol.c:gdbpy_lookup_static_symbol d-namespace.c:d_lookup_nested_symbol In these cases there is no call to lookup_symbol_in_static_block. I would need to investigate more to see if these are working as expected or not. I suspect the python use case might not always do what a user expects, as it searches static symbols in a predefined order, if we have multiple with the same name we would always find the same one first, but we'd probably expect to find one from the current object file before one from a different object file. As for the D use case, I don't know D, so don't feel qualified to judge. I think my conclusion is that you're right, but, refactoring this code to have lookup_static_symbol call lookup_symbol_in_static_block (or equivalent in all cases) seems a pretty scary change. I'd ideally like to see that refactoring separated from this patch series. My vote would be to merge this, and then, possibly we can look at reworking symbol lookup inline with your suggestion. What do you think? Thanks, Andrew