[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 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.

- 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
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140916/56eb59b1/attachment.html>