From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x444.google.com (mail-wr1-x444.google.com [IPv6:2a00:1450:4864:20::444]) by sourceware.org (Postfix) with ESMTPS id 0F37F385B831 for ; Mon, 23 Mar 2020 17:53:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0F37F385B831 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x444.google.com with SMTP id f3so18220485wrw.7 for ; Mon, 23 Mar 2020 10:53:23 -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=d+SMLn4VhBrJhSB1dpj/Wr+r1bYub7OOyXxHdWOEJCQ=; b=asm1kQNLTt/RCiTbKwpX8mqMyTvqUtyuDqbxGciulQD5VMXXrms4yf5T3OCde+Trhs 0qK5fBza21NFVevxd45epyOc7SUosj9MbTWMI/EYNiO+kaq/DsW6bTEaphVAxehol1o+ KTFQvGyJVE4FU9nPo1ZMxNGgLGNnVqSCBpLZfShRMOXQSeUacWvMezP/nS0RIlfB9jaA bwEtX255OchnWXTMaHUHAaDO/IqJCJzSXqym9yx2+Yrr7h1Fbc09pxr3M2y+KOZ4RDzW kFgaW7++fQAhOSr8vjtD+5364D6weTMKXHtH+FVw7LjMHh/10rhSkhPEpePmzIj+0O6V IdMA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=d+SMLn4VhBrJhSB1dpj/Wr+r1bYub7OOyXxHdWOEJCQ=; b=HibNXo3radaNv5R7lFebz+x660Xi3t87y9kJo6h33/4p2BCSRvytQuJJYfI5mj1wlj 19wz6p1cgBcQXbIAL8t8WM2ichSjqE1CH8+pqYeRjjDjN9WUHnXCpyrCvsLyyeRuT0OY 7tCB/dzfhGbTTQeshTKioR5KCPJVQKneNR6iBP8vq2B3dqBibFuncMzPwE646N4UoKsl Wmye/DvYOme+nOIx8OzG0bB1ZwqktSZ0MAPjtOZUsSrCkSGE+9UoyGJ5c6u5sNCOH/iW 706n/1KU2WS/ndCLyFwEhJTKsltPdL9ybvRkZGkWrlzHKAbrnKOmWP3R8Ecz50W6MxrX lWsw== X-Gm-Message-State: ANhLgQ1KpCWVh0grJ8Rn7BATTRhQ4OjvMzDtDGNfEusBLOQbYqzfVgbd nXvazblXKQDSBlwKe9bjA2ZnvP/t6Ws= X-Google-Smtp-Source: ADFU+vuB+y7E+DaDkmm/EU29OHjQ9YhgAb3EgM0Rrv2KlEMNfjsijZ1gx56zprgwluAUe9UNku2LTw== X-Received: by 2002:adf:ecc3:: with SMTP id s3mr28715687wro.32.1584986002116; Mon, 23 Mar 2020 10:53:22 -0700 (PDT) Received: from localhost (host86-186-80-207.range86-186.btcentralplus.com. [86.186.80.207]) by smtp.gmail.com with ESMTPSA id y80sm412374wmc.45.2020.03.23.10.53.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 23 Mar 2020 10:53:21 -0700 (PDT) Date: Mon, 23 Mar 2020 17:53:20 +0000 From: Andrew Burgess To: Bernd Edlinger Cc: Christian Biesinger , "gdb-patches@sourceware.org" Subject: Re: [PATCHv3] Fix range end handling of inlined subroutines Message-ID: <20200323175320.GR3317@embecosm.com> References: <20200311220231.GJ3317@embecosm.com> <20200317222757.GN3317@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux/4.18.19-100.fc27.x86_64 (x86_64) X-Uptime: 17:43:35 up 38 days, 5:12, X-Fortune: Do you like "TENDER VITTLES"? X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-Spam-Status: No, score=-19.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Mar 2020 17:53:24 -0000 * Bernd Edlinger [2020-03-21 21:31:07 +0100]: > On 3/19/20 2:33 AM, Bernd Edlinger wrote: > > On 3/17/20 11:27 PM, Andrew Burgess wrote: > >> > >> Are you arguing that we shouldn't use std::vector anywhere in GDB? Or > >> that this particular piece of code shouldn't use std::vector? > >> > > > > No, in general std::vector is just fine. > > I think just here it is especially important to be below O(n*log(n)), > > and don't depend on the underlying implementation. > > > >> It was my understanding that we were moving GDB away from bespoke > >> container management code unless there was a very compelling reason. > >> I think as a minimum any new code that was written not using std:: > >> data structures (or some other gdb specific type) would need a comment > >> explaining why, if nothing else to stop someone replacing the code > >> with std:: types a few months down the line. > >> > > > > Right, good point. > > I can add a comment here, so that will not happen: > > > > index 46f985a..4f0d536 100644 > > --- a/gdb/buildsym.c > > +++ b/gdb/buildsym.c > > @@ -736,6 +736,10 @@ struct blockvector * > > void > > buildsym_compunit::record_inline_range_end (CORE_ADDR end) > > { > > + /* The performance of this function is very important, > > + it shall be O(n*log(n)) therefore we do not use std::vector > > + here since some compilers, e.g. visual studio, do not > > + guarantee that for vector::push_back. */ > > if (m_inline_end_vector == nullptr) > > { > > m_inline_end_vector_length = INITIAL_LINE_VECTOR_LENGTH; > > > > > > Does that look good (also from the english)? > > > > Hi Andrew, > > how are you doing? > > are you ok? Sorry, I have not been ignoring this patch. It is top of my review stack. It is being held up for a number of reasons, first Tom found a number of issues with the original patch set, I've been trying to fix these before reviewing your work in case the fixes had an impact. I posted some patches for the easier of the issues, but I've not solved the harder bug yet. I'm pretty much decided to just go ahead and review this patch anyway. But then, I'm still struggling with the choice not to use std::vector. It's still not clear if there's actually a good reason to avoid it in this case or not, so I feel I need to review other uses of std::vector throughout GDB and see how your use compares. Do you maybe have some performance figures you could share to help make the case for using custom container type? Then the whole converting is_stmt entries to !is_stmt is still concerning me. I appreciate your expanded text in the patch description, but I need to do more testing to see the real impact of this change. Most of my clients are driving GDB through some kind of GUI, which means that break points are inevitably placed using file:line sytnax. One of the most common complaints I get is when a breakpoint is placed on one line, but GDB stops on another. Generally my users can be quite forgiving of missing stack frames when inlining is in play. What this means is an argument of if you did break here you'd have a frame missing, so I'm not going to let you break there any more isn't that compelling to me. That isn't a rejection of the patch, it's just me explaining why it's taking me some time to review. Thanks, Andrew