From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 4F5A83950839 for ; Thu, 22 Apr 2021 16:06:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4F5A83950839 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 13MG6TW5012256 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 22 Apr 2021 12:06:34 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 13MG6TW5012256 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 61AA61E54D; Thu, 22 Apr 2021 12:06:28 -0400 (EDT) Subject: Re: [PATCH v3] gdb: Do autoload before notifying Python side in new_objfile event To: Michael Weghorn , gdb-patches@sourceware.org References: <20210210154053.82927-1-m.weghorn@posteo.de> <20210326082934.2522904-1-m.weghorn@posteo.de> <6574953b-c91a-3764-6b9f-b9e0923fd1ac@polymtl.ca> <8e33b994-44fd-e58a-1309-1c0fb954a900@posteo.de> From: Simon Marchi Message-ID: <2e5a3c5f-361d-08b6-3224-660d6abb6687@polymtl.ca> Date: Thu, 22 Apr 2021 12:06:28 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <8e33b994-44fd-e58a-1309-1c0fb954a900@posteo.de> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 22 Apr 2021 16:06:29 +0000 X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, 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: Thu, 22 Apr 2021 16:06:44 -0000 On 2021-04-22 11:46 a.m., Michael Weghorn wrote: > > On 20/04/2021 23.57, Simon Marchi wrote: >> Huh, I started working on this because I didn't see your message and >> didn't realize you had implemented it already. Here's my WIP branch for >> reference: >> >> https://github.com/simark/binutils-gdb/tree/observer-dep >> >> I made some random improvements to observable.h in the form of >> preparatory patches. They could be merged later, or you could rebase >> your patch on top if you think it would help you. > > Thanks, I've rebased on top of commit f7716715f1d2a71a5c95b7ac0e0323b742692e6d > ("gdbsupport: add observer_debug_printf, OBSERVER_SCOPED_DEBUG_ENTER_EXIT") > from that WIP branch and dropped those parts from my patch that were already > included in commits on that branch up to that point. > > I've left out the last two patches from that branch which were doing > similar things to my patch, but it didn't look straightforward to me > how to reasonably "merge" the two versions. > I've taken some inspiration for naming (which was nicer on your branch) and split > my patch into two separate ones similar to how it is done on your branch, > though (one patch to add the functionality to order observers + corresponding > unit tests; one patch to make use of that and add the testcase > for the testsuite). > > I was uncertain of whether or not to include your commits from the WIP > branch in the patch series to send to the mailing list, and haven't done > so yet in v4 (but can of course do so in the next version if it makes > sense) that I just sent: > > https://sourceware.org/pipermail/gdb-patches/2021-April/178069.html Thanks! Then I will try to post my patches soon and get them merged. >> I left a few comments below. But in general this looks great, pretty >> much what I had in mind when I suggested this idea. >> >> Random question for you: while doing this work, I stumbled upon this >> line in the detach method: >> >> m_observers.erase (iter, m_observers.end ()); >> >> Does that look right to you? Doesn't this delete more observers than >> the one we want to detach, if there are more observers after it? > > As I understand it, that should be OK, since 'std::remove_if' is called > first, which - as I understand it - should make sure that all > not-to-be-removed observers will be before 'iter' afterwards. > http://www.cplusplus.com/reference/algorithm/remove_if/?kw=remove_if > says: > "Transforms the range [first,last) into a range with all the elements for which > pred returns true removed, and returns an iterator to the new end of that > range. > The function cannot alter the properties of the object containing the range of > elements (i.e., it cannot alter the size of an array or a container): > The removal is done by replacing the elements for which pred returns true > by the next element for which it does not, and signaling the new size of > the shortened range by returning an iterator to the element that should be > considered its new past-the-end element." Ah, right, I forgot how that worked, thanks. >> I think we need to call sort_elements even if key == nullptr, because >> the two observers can be added in any order. If observer A depends on >> B, A won't have a key (it doesn't need to). If B if registered, then A, >> you want to sort at that time. So I would just sort all the time on >> attach. > > I think that case should be fine without reordering, because the observer > is initially always added at the end of the vector (using 'emplace_back'), > i.e. after all potential dependencies that have been inserted earlier), > i.e. A already comes after B if it is registered later. > > The idea was to avoid reordering the vector if it's not necessary, but > I don't know whether that makes any difference performance-wise in practice > and can drop that if you think it makes sense. > (I've left it in v4 to hear about your opinion first.) Ok, that makes sense. A comment to explain that would be good though, since it's not obvious and can seem wrong for others too. Simon