From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3288 invoked by alias); 20 Nov 2019 00:03:05 -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 3278 invoked by uid 89); 20 Nov 2019 00:03:05 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-32.3 required=5.0 tests=AWL,BAYES_00,ENV_AND_HDR_SPF_MATCH,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,HTML_MESSAGE,KAM_STOCKGEN,RCVD_IN_DNSWL_NONE,SPF_PASS,USER_IN_DEF_SPF_WL autolearn=ham version=3.3.1 spammy=gdb's, gdbs, H*c:alternative 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; Wed, 20 Nov 2019 00:03:01 +0000 Received: by mail-wr1-f67.google.com with SMTP id q15so13198869wrw.7 for ; Tue, 19 Nov 2019 16:03:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=uOIEVe/Goaf3hzVCQ/w/eBLxSPBcEvmJ4ytF3fe0+x8=; b=JF2vchLEIfQeuQntN3+qorIqon7IohCRaDZL/k9wwtNjZVuOLL5zLgmpf4tCbeg2QH /dlaQEgsqNAbb+VaYTu1TJM/P9F/KvkPcyuGR69qH0q1Lk5C4AhB4tJPTTFW78pKwa5t sa0J/6j8rRHi9QM0zcNMmSwVLLBtOKC07LxIvfnmMXExaEBG7CbQ3hzeGKVGWwSCFUuS niShEOtIMbJVZBMwOOz2n/3W5pj5QQ5m0aI4Bs4kofhlKe3kLf8RXDE2tkL1dVz9aoJ7 v1ou8aGbmZjRLCAqPRQQTUiJg+YYhj21Ag+SxVnqXH1dEj0AcvxxWDEB/OkmfIo8Ku68 1f7Q== MIME-Version: 1.0 References: <20191107040541.208168-1-tamur@google.com> <8348cd40-ec26-731b-2b41-76ffc1f1886c@simark.ca> In-Reply-To: <8348cd40-ec26-731b-2b41-76ffc1f1886c@simark.ca> From: "Ali Tamur via gdb-patches" Reply-To: Ali Tamur Date: Wed, 20 Nov 2019 00:03:00 -0000 Message-ID: Subject: Re: [PATCH] Fix infinite recursion bug at get_msymbol_address. To: Simon Marchi Cc: gdb-patches@sourceware.org, Tom Tromey , Eric Christopher Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-11/txt/msg00614.txt.bz2 Hi Simon, We have some hooks into gdb's "new objfile" event, where we optionally load related debug files from somewhere else. The failing test involves these hooks and reducing it further is proving difficult. I put a breakpoint just before entering the loop: CORE_ADDR get_msymbol_address (struct objfile *objf, const struct minimal_symbol *minsym) { gdb_assert (minsym->maybe_copied); gdb_assert ((objf->flags & OBJF_MAINLINE) == 0); const char *linkage_name = MSYMBOL_LINKAGE_NAME (minsym); for (objfile *objfile : current_program_space->objfiles ()) { if ((objfile->flags & OBJF_MAINLINE) != 0) { bound_minimal_symbol found = lookup_minimal_symbol_linkage (linkage_name, objfile);
if (found.minsym != nullptr) return BMSYMBOL_VALUE_ADDRESS (found); } } return (minsym->value.address + ANOFFSET (objf->section_offsets, minsym->section)); } (top-gdb) c (top-gdb) p objf $12 = (objfile *) 0x1a5dae0 (top-gdb) p minsym $13 = (const minimal_symbol *) 0x7ffff70fff88 (top-gdb) p objf->flags $14 = {m_enum_value = (OBJF_REORDERED | OBJF_USERLOADED | OBJF_PSYMTABS_READ)} (top-gdb) p objfile $15 = (objfile *) 0x1928000 (top-gdb) p objfile->flags $16 = {m_enum_value = (OBJF_REORDERED | OBJF_USERLOADED | OBJF_PSYMTABS_READ | OBJF_MAINLINE)} (top-gdb) p found $17 = {minsym = 0x7ffff70fff88, objfile = 0x1a5dae0} So, objfile and objf are different objects. We call lookup_minimal_symbol_linkage(linkage_name, objfile), it iterates through objfile->separate_debug_objfiles() and returns the original objf and minsym. The stack trace is like this: #0 get_msymbol_address (objf=0x1a5dae0, minsym=0x7ffff70fff88) at gdb/symtab.c:6306 #1 0x0000000000c98b1e in lookup_minimal_symbol_by_pc_name (pc=0x10d120, name=0x1c86990 "main(int, char**)", objf=0x1a5dae0) at gdb/minsyms.c:613 #2 0x0000000000e090d8 in fixup_section (ginfo=0x1d42af0, addr=0x10d120, objfile=0x1a5dae0) at gdb/symtab.c:1649 #3 0x0000000000e09577 in fixup_symbol_section (sym=0x1d42af0, objfile=0x1a5dae0) at gdb/symtab.c:1759 #4 0x0000000000e16401 in lookup_symbol_via_quick_fns (objfile=0x1a5dae0, block_index=GLOBAL_BLOCK, name=0x1f995b0 "main", domain=VAR_DOMAIN) at gdb/symtab.c:2408 #5 0x0000000000e0ab30 in lookup_symbol_in_objfile (objfile=0x1a5dae0, block_index=GLOBAL_BLOCK, name=0x1f995b0 "main", domain=VAR_DOMAIN) at gdb/symtab.c:2530 I don't know whether what we are doing breaks too many assumptions: * already have a debug file and load an equivalent one again? * have two debug files that define the same symbol? * something else? but my patch with a simple guard against infinite recursion seems to work well enough. Thanks, Ali On Mon, Nov 18, 2019 at 9:15 PM Simon Marchi wrote: > On 2019-11-06 11:05 p.m., Ali Tamur via gdb-patches wrote: > > The patch 4b610737f0 seems to have introduced the possibility of infinite > > recursion. I have encountered the problem while debugging a failing > in-house > > test. I am sorry, it is fairly difficult to reduce the test case (and I > don't > > understand most of what is going on) but the stack trace shows a call to > > objfpy_add_separate_debug_file, which eventually causes > > lookup_minimal_symbol_by_pc_name to be invoked, which calls > get_msymbol_address. > > Somehow lookup_minimal_symbol_linkage finds the same symbol and the > function > > calls itself with the same parameters. I don't know whether this should > be > > classified as 'it should never happen', but this simple patch makes the > test > > pass and should be harmless, I think. > > > > gdb/ChangeLog > > > > * symtab.c (get_msymbol_address): Guard against infinite recursion. > > --- > > gdb/symtab.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/gdb/symtab.c b/gdb/symtab.c > > index 2c934b9c22..b231cc6e84 100644 > > --- a/gdb/symtab.c > > +++ b/gdb/symtab.c > > @@ -6328,7 +6328,7 @@ get_msymbol_address (struct objfile *objf, const > struct minimal_symbol *minsym) > > { > > bound_minimal_symbol found > > = lookup_minimal_symbol_linkage (linkage_name, objfile); > > - if (found.minsym != nullptr) > > + if (found.minsym != nullptr && found.minsym != minsym) > > return BMSYMBOL_VALUE_ADDRESS (found); > > } > > } > > Hi Ali, > > At the top of the function, we assert that the objfile associated to the > minsym > is not the main one: > > gdb_assert ((objf->flags & OBJF_MAINLINE) == 0); > > We then iterate on all objfiles, looking only for main objfiles (presumably > there's only one). We look up a minsym with the same name as our original > minsym in > the main objfile. > > How could our original minsym (which is not supposed to come from the main > objfile, > I believe) could be the same one as the minsym found when looking up in > the main > objfile? > > If this is indeed not supposed to happen, I think a fix like this would > just paper over the real problem. Or maybe you have a legitimate use case > that > wasn't expected. You've talked about objfpy_add_separate_debug_file, so > clearly your > use case involves separate debug files. It's possible that we are passing > the objfile > representing the separate debug file of the main objfile or something like > that. > > I'd like if we could get a reproducer before we commit a fix we are not > sure to > understand. Can you give the backtrace, maybe we'll get some idea? > > Also, could you provide as much info about your use case as possible to > try to guide > us in finding the root issue? > > Simon >