[PATCH] This phabricator revision is the merge of 4 patches that aim to provide resolving of AT_abstract_origin and AT_specification attributes.

[David Blaikie]

On Wed, Sep 17, 2014 at 4:02 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:

>
> On Tue, Sep 16, 2014 at 10:52 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
>
>>
>>
>> On Mon, Sep 15, 2014 at 11:22 PM, Frédéric Riss <friss at apple.com> wrote:
>>
>>>
>>> On 16 Sep 2014, at 02:33, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>
>>>
>>> On Mon, Sep 8, 2014 at 11:28 AM, Frédéric Riss <friss at apple.com> wrote:
>>>
>>>>
>>>> On 08 Sep 2014, at 19:45, Alexey Samsonov <vonosmas at gmail.com> wrote:
>>>>
>>>>
>>>> On Mon, Sep 8, 2014 at 2:39 AM, Frédéric Riss <friss at apple.com> wrote:
>>>>
>>>>> >
>>>>> > ================
>>>>> > Comment at: lib/DebugInfo/DWARFContext.cpp:401
>>>>> > @@ +400,3 @@
>>>>> > +                  const std::unique_ptr<UnitType> &RHS) const {
>>>>> > +    return LHS->getNextUnitOffset() <= RHS->getNextUnitOffset();
>>>>> > +  }
>>>>> > ----------------
>>>>> > I have a sneaking suspicion that <= ordering would violate the
>>>>> requirements of lower_bound...
>>>>> >
>>>>> > I /suspect/ the right answer is:
>>>>> >
>>>>> > Off < RHS->getOffset()
>>>>> >
>>>>> > LHS->getNextUnitOffset() <= Off
>>>>> >
>>>>> > LHS->getNextUnitOffset() <= RHS->getOffset()
>>>>> >
>>>>> > That should be a proper "less than" for a half open range [offset,
>>>>> next unit offset), I think... maybe?
>>>>>
>>>>> I think you’re generally right. But all the documentation I can find
>>>>> about lower_bound tells me that only the comp(Unit, Offset) will be used by
>>>>> lower_bound. The other 2 functions make it adhere to the Compare concept,
>>>>> but I find it strange to implement unused functions just for that. Would it
>>>>> be legal to just pass a function accepting a Unit and an Offset?
>>>>>
>>>>> BTW, is there an authoritative freely available source regarding these
>>>>> sort of STL usage details?
>>>>>
>>>>> >
>>>>> > On 05 Sep 2014, at 22:46, Alexey Samsonov <vonosmas at gmail.com>
>>>>> wrote:
>>>>> >
>>>>> > IIRC, originally getCompileUnitForOffset() was supposed to work only
>>>>> for the exact Offsets (which are returned by DWARFUnit::getOffset()). Now
>>>>> that you're changing it to work for arbitrary offsets, I think it makes
>>>>> more sense to rewrite it as follows:
>>>>>
>>>>> Then the comment of the function in the header was wrong.
>>>>>
>>>>
>>>> Yes, aparently it is wrong. Otherwise function implementation is plain
>>>> wrong.
>>>>
>>>>
>>>>>
>>>>> > if (CUs.empty()) return nullptr;
>>>>> > auto It = std::upper_bound(CUs.begin(), CUs.end(), Offset,
>>>>> OffsetComparator());
>>>>> > if (It == CUs.begin()) return nullptr;
>>>>> > --It;
>>>>> > return (It->getOffset <= Offset && Offset < It->getNextUnitOffset())
>>>>> ? It->get() : nullptr;
>>>>> >
>>>>> > and leave the comparator as is. I would rather not play with
>>>>> non-strict comparisons.
>>>>>
>>>>> I could even keep using lower_bound without modifying the comparator,
>>>>> and when it returns CUs.end(), check --CUs.end() for the given offset. It
>>>>> would be a bit more efficient than the above I believe.
>>>>>
>>>>
>>>> I'm not sure I follow. std::lower_bound() returns an element which is
>>>> greater than or equal than the provided value. If you want to find the CU
>>>> with the largest offset, which is still less than the offset
>>>> you provide, you should always decrement the std::lower_bound(). But
>>>> than there is the special case for equal offset... that's why you should
>>>> better use upper_bound, which returns the element
>>>> strictly greater than the provided value, and decrement the result
>>>> unconditionally.
>>>>
>>>>
>>>> You’re right of course. There is another solution I believe: use only
>>>> strict comparisons against getNextUnitOffset in the comparator and use
>>>> upper_bound as search function. This way, the returned unit is there first
>>>> one where getNextUnitOffset is strictly greater than the searched offset
>>>> which is exactly what we are looking for, isn’t it? David, what do you
>>>> prefer?
>>>>
>>>
>>> I've lost track of which parts of this comparator we are reviewing
>>> where, but anyway...
>>>
>>> this discussion is about how to use lower_bound or upper_bound?
>>>
>>>   1, 2, 3, 4[, end]
>>>
>>> the lower_bound and upper_bound of:
>>>
>>> 3 is 3, 4
>>> 5 is end, end
>>> 1 is 1, 2
>>>
>>> etc...
>>>
>>> So why can't we just use lower_bound and check != end. If the range is
>>> contiguous (which it is, right?) then that should suffice. Any non-end
>>> result would be the element we're looking for.
>>>
>>> (assuming the comparison provided is "CU->getNextOffset() <= Offset")
>>>
>>> Or am I missing something?
>>>
>>>
>>> I didn’t update this patch, I was waiting for approval on D5262 which is
>>> the OffsetComparator subpart of this, to rebase it. But now I am confused…
>>> lower_bound with <= comparison was my first proposal here, but you were
>>> bothered by the non-strict comparison.
>>>
>>
>> Looking back over my comments, my source of confusion, as you say, came
>> from the non-strictness (in the casual sense of the term, not necessarily
>> the "strict/strict-weak ordering" sense which the standard uses - I
>> actually don't understand enough about comparators to know when something
>> is or is not a strict-weak ordering off-hand (probably one of those "if it
>> feels right" for me things)).
>>
>> Specifically it was the oddity of having all three operations using <=
>> which didn't make sense. Indeed the suggested three versions I wrote
>> includes what I'm now suggesting/considering (lower_bound + NextUnitOffset
>> <= Offset), but without the other 2 so there's less to get confused
>> by/worry about.
>>
>>
>>> Now that we’ve gone through this a few times, I find the upper_bound
>>> (with Offset < NextUnitOffset) version more logical,
>>>
>>
>> I'd like to understand why you find it more logical - perhaps I'm missing
>> something in my understanding.
>>
>> I find the upper_bound version less clear because we provide a comparator
>> that, in some sense, says that the unit that contains the Offset is
>> actually "less than" the CU we're looking for... (at least that's how I
>> think about it) and deliberately take the unit one beyond that element.
>> Usually I would think of the result of upper_bound as "the thing greater
>> than what I'm looking for" - I'd use it if I want to visit "everything
>> after <blah>" but in this case it's actually the thing we're looking for -
>> we've just muddled with the comparison.
>>
>> Whereas using lower_bound with NextUnitOffset <= Offset seems to make
>> sense to me - I'm searching for the element with "Offset" within its range.
>> Any unit with a NextUnitOffset <= Offset is "less" than the unit I'm
>> looking for.
>>
>> I see Alexey was where this got into a discussion of upper_bound,
>> strictness, etc.
>>
>> Alexey - perhaps you can describe your preference?
>>
>> As mentioned previously, you're more of an owner of this code than I am -
>> so if you have a strong preference to some particular formulation, go with
>> it. I'm just curious to understand.
>>
>
> Ah, explaining preferences is hard. As Frederic mentioned, both ways are
> legit, depending on the mental model you choose. Parsing the code suggested
> is now somewhat straightforward to me.
> I guess I just learned to ignore the names of std::lower_bound /
> std::upper_bound (they are nonsensical anyway)
>

I usually try to make some sense of them - though this discussion's clearly
kicked that sense around a bit :)

I still think of lower/upper as forming the classic C++ half open range
[lower_bound, upper_bound) given an ordered sequence and a less than
comparison to some 'thing'.


> and instead substitute them with semantics. (upper_bound with a custom
> comparator,
> is not "find a thing greater", it's just "find the leftmost element for
> which X holds").
>

*nod*

Anyway - all yours :)

 - Dave


>
>
>>
>>
>> - David
>>
>>
>>> but I think it is totally equivalent to lower_bound with (NextUnitOffset
>>> <= Offset).
>>>
>>> Fred
>>>
>>>
>>>
>>>>
>>>> Fred
>>>>
>>>>
>>>> So should I use David’s variation, remove the seemingly unused
>>>>> comparison functions, don’t touch the comparator object and rewrite the
>>>>> function? What do you prefer?
>>>>>
>>>>> Fred
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Alexey Samsonov
>>>> vonosmas at gmail.com
>>>>
>>>>
>>>
>>
>
>
> --
> Alexey Samsonov
> vonosmas at gmail.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140917/261295d0/attachment.html>