From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by sourceware.org (Postfix) with ESMTPS id 04B14389852F for ; Fri, 23 Apr 2021 06:41:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 04B14389852F Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id 368D62400FF for ; Fri, 23 Apr 2021 08:41:21 +0200 (CEST) Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4FRPns2t4vz9rxG; Fri, 23 Apr 2021 08:41:21 +0200 (CEST) Subject: Re: [PATCH v3] gdb: Do autoload before notifying Python side in new_objfile event To: Simon Marchi , 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> <2e5a3c5f-361d-08b6-3224-660d6abb6687@polymtl.ca> From: Michael Weghorn Message-ID: <0c435306-3368-d707-fffe-f7e03a93fb26@posteo.de> Date: Fri, 23 Apr 2021 06:41:16 +0000 MIME-Version: 1.0 In-Reply-To: <2e5a3c5f-361d-08b6-3224-660d6abb6687@polymtl.ca> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, 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: Fri, 23 Apr 2021 06:41:26 -0000 On 22/04/2021 18.06, Simon Marchi via Gdb-patches wrote: > 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. Great, thanks! > >>> 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. What do you think about this? /* Observer has been inserted at the end of the vector, so it will be after any potential dependencies attached earlier. If the observer has a token, it means that other observers can specify a dependency on it, so sorting is necessary to ensure those will be after the newly inserted observer afterwards. */ if (t != nullptr) { sort_elements (); } Michael