Re: [PATCH v3] gdb: Do autoload before notifying Python side in new_objfile event

[Simon Marchi <simon.marchi@polymtl.ca>]

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