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

[Michael Weghorn <m.weghorn@posteo.de>]

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