From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 63655 invoked by alias); 28 Mar 2018 03:34:38 -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 63643 invoked by uid 89); 28 Mar 2018 03:34:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=UD:ca, Either, H*r:10.0.0, judge 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; Wed, 28 Mar 2018 03:34:36 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 3C6881E4B2; Tue, 27 Mar 2018 23:34:34 -0400 (EDT) Subject: Re: [RFA 1/2] Make line tables independent of progspace To: Tom Tromey Cc: gdb-patches@sourceware.org, macro@mips.com References: <20180321171809.13115-1-tom@tromey.com> <20180321171809.13115-2-tom@tromey.com> <87zi2uw3uc.fsf@tromey.com> <87vadiw24a.fsf@tromey.com> From: Simon Marchi Message-ID: Date: Wed, 28 Mar 2018 03:34:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <87vadiw24a.fsf@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-03/txt/msg00579.txt.bz2 On 2018-03-27 12:53 AM, Tom Tromey wrote: > Tom> I've run the new patch through the buildbot. I'll resubmit it soon, I > Tom> just have to update the ChangeLog and rewrite the commit message. > > Here it is. Let me reiterate that this probably requires careful review > as the buildbot may not test some of these paths. Hi again, I took another look. For the old debug info formats, it's really hard to tell if the code is right, since we don't have automated tests or even a manual way to test, for most of them. But I don't think that should hinder improvements to the formats that are actually used today. I think the changes you did are a good "best effort". If we judge that the old formats are not useful anymore and have most likely bit-rotten with time, we should think about removing them eventually. Anyway, I just have some small comments here, otherwise the patch LGTM. Maybe it would be a good idea to get the opinion of Maciej (in CC) for the addr_bits_remove part? > @@ -3209,13 +3219,17 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent) > > /* Is this file's first line closer than the first lines of other files? > If so, record this file, and its first line, as best alternate. */ > - if (item->pc > pc && (!alt || item->pc < alt->pc)) > - alt = item; > + if (item->address (iter_s) > pc > + && (!alt || (item->address (iter_s) < alt->address (alt_symtab)))) > + { > + alt = item; > + alt_symtab = iter_s; > + } > > - auto pc_compare = [](const CORE_ADDR & pc, > - const struct linetable_entry & lhs)->bool > + auto pc_compare = [=](const CORE_ADDR & pc, > + const struct linetable_entry & lhs)->bool > { > - return pc < lhs.pc; > + return pc < lhs.address (iter_s); > }; Since we know this will be called many times and address() is substantially more costly than just reading a CORE_ADDR field, maybe it would be good to save it to a variable before and use that in the lambda. I was also thinking the same thing for the various other calls to address() in this function, since sometimes the same value is recomputed multiple times. For example, "item->address (iter_s)" in if (best && item < last && (item->address (iter_s) > best->address (best_symtab)) && (best_end == 0 || best_end > item->address (iter_s))) best_end = item->address (iter_s); > @@ -1227,8 +1228,45 @@ struct rust_vtable_symbol : public symbol > > struct linetable_entry > { > - int line; > - CORE_ADDR pc; > + /* Set the members of this object. */ > + void set (int line_, CORE_ADDR pc) > + { > + m_line = line_; > + m_pc = pc; > + } > + > + /* Set the members of this object from another linetable_entry. */ > + void set (const linetable_entry &other) > + { > + m_line = other.m_line; > + m_pc = other.m_pc; > + } This really looks like an assignment operator (the default one would be enough). Is there a reason to have this set overload? Either way works, I'm just curious. > @@ -499,19 +505,20 @@ arrange_linetable (struct linetable *oldLineTb) > { > /* If the function was compiled with XLC, we may have to add an > extra line to cover the function prologue. */ > - jj = fentry[ii].line; > + jj = fentry[ii].line (); > if (jj + 1 < oldLineTb->nitems > - && oldLineTb->item[jj].pc != oldLineTb->item[jj + 1].pc) > + && (oldLineTb->item[jj].raw_address () > + != oldLineTb->item[jj + 1].raw_address ())) > { > - newLineTb->item[newline] = oldLineTb->item[jj]; > - newLineTb->item[newline].line = oldLineTb->item[jj + 1].line; > + newLineTb->item[newline].set (oldLineTb->item[jj].raw_address (), > + oldLineTb->item[jj + 1].line ()); The address and the line are inverted here. Thanks, Simon