From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18381 invoked by alias); 12 Jul 2018 17:12:25 -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 18339 invoked by uid 89); 12 Jul 2018 17:12:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_STOCKGEN,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 12 Jul 2018 17:12:22 +0000 Received: by simark.ca (Postfix, from userid 112) id 35BEE1EF29; Thu, 12 Jul 2018 13:12:20 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=simark.ca; s=mail; t=1531415540; bh=+f86lD5mSHPSTv6Y9F0iQ0SX9MdEnJKGU25HDxedlhc=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=VJ2oicpdQTPCUoZ5j2tay0xVWGga1N7lhfthb4jRV0IMsOFJBTELxkveE2ikqIYoO pxhq4J018XRIv70ral2e/QtwoFQzuYEODQcWjUqRYG+aBYr+PRbRaVDpMSp8MYTFEY TARcT4XGsF2Hjoa6gI6duPND+Vzm3k/UcbUjMGxQ= Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id A55951E48F; Thu, 12 Jul 2018 13:12:15 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=simark.ca; s=mail; t=1531415535; bh=+f86lD5mSHPSTv6Y9F0iQ0SX9MdEnJKGU25HDxedlhc=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=W3xV6wnaqEpdEkfrO0vCXBlBCgWfjhuSorG7CzbUQekjv67R388fGnDz3E5WwcXjB 8X5HV1crxHK5ZvfZEyXxPmslINx3kjs+4tdDu0Pa6oddrIJdoS8qHDDZV1PgQSFFP9 QtcdINDak/hRaHxx+DHT7FxJRSjZETfzDw4wpOAw= MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 12 Jul 2018 17:12:00 -0000 From: Simon Marchi To: Keith Seitz Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2] Really fix gdb/23010: SYMBOL_LANGUAGE != DICT_LANGUAGE assertion In-Reply-To: <97d83efa-56b9-0e1a-a6d5-1ee2115c8a99@redhat.com> References: <20180627190341.24442-1-keiths@redhat.com> <33b99113-c705-a029-47df-3e6c0fc49139@simark.ca> <97d83efa-56b9-0e1a-a6d5-1ee2115c8a99@redhat.com> Message-ID: X-Sender: simark@simark.ca User-Agent: Roundcube Webmail/1.3.6 X-SW-Source: 2018-07/txt/msg00344.txt.bz2 On 2018-07-12 11:47, Keith Seitz wrote: >> I have a question about the assertion vs complaint. Is it logically >> impossible (putting aside other GDB bugs) for this assertion to fail? >> Or is it possible now to feed bad debug info to GDB that will trigger >> this new assert? If it's the former, then no problem, if it's the >> later >> then an assertion is not appropriate. > > IMO the former. The code is currently structured so that a dictionary > can *only* have symbols of one language inside it. While it may be > possible to trigger this with malicious DWARF hacking, I cannot really > say for certain that it isn't possible in valid DWARF. [Sorry, that's > more of a politician's answer to your question.] Ok. I'm asking because ideally, we should react to bad input (including DWARF) by providing a useful error message rather than asserting. I'm fine with leaving it like this. That's not in the scope of your patch, and your patch does not make the situation worse (it only points out the problem earlier). >>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c >>> index 4ad0527406..ba70315957 100644 >>> --- a/gdb/dwarf2read.c >>> +++ b/gdb/dwarf2read.c >>> @@ -9701,6 +9701,24 @@ compute_delayed_physnames (struct dwarf2_cu >>> *cu) >>> cu->method_list.clear (); >>> } >>> >>> +/* A wrapper for add_symbol_to_list to ensure that SYMBOL's language >>> is >>> + the same as all other symbols in LISTHEAD. If a new symbol is >>> added >>> + with a different language, this function asserts. */ >>> + >>> +static inline void >>> +dw2_add_symbol_to_list (struct symbol *symbol, struct pending >>> **listhead) >>> +{ >>> + /* Only assert if LISTHEAD already contains symbols of a different >>> + language (dict_create_hashed/insert_symbol_hashed requires that >>> all >>> + symbols in this list are of the same language). */ >>> + gdb_assert ((*listhead) == NULL >>> + || (*listhead)->nsyms == 0 >>> + || (SYMBOL_LANGUAGE ((*listhead)->symbol[0]) >>> + == SYMBOL_LANGUAGE (symbol))); >> >> I wonder if it's actually possible to have (*listhead)->nsyms == 0, >> since as >> soon as a "struct pending" is allocated, there is at least one sym put >> into >> it (in add_symbol_to_list). > > Ha! Yes, that's true. I didn't even look. I just erred on the > defensive side. I'll remove that. > >> It's probably a bit hard to tell for older debug formats, but is it >> only for >> DWARF that this condition must hold? Did you consider putting this >> assertion >> directly in add_symbol_to_list? I'm not saying it's the right thing >> to do, I'm >> just asking. > > The assertion holds for any debug reader that uses hashed > dictionaries, and that list isn't a straightforward list to compose. > That's probably why I originally isolated to dwarf2read.c. Or I just > plain didn't think about (interfering with) other debuginfo readers. > > Any symbol reader using add_symbol_to_list could create hashed > dictionaries, but AFAICT there is no requirement that it does. > > Out of curiosity, I moved (and adjusted) the assertion to > add_symbol_to_list and ran some tests. As expected, DWARF tests showed > no difference. STABS testing, though, triggers the assertion. > add_symbol_to_list is often called before any dictionary is created. Ok, thanks. The patch looks good to me, but I'd like if somebody with more experience in this area gave a second opinion. Simon