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

Wed Sep 17 16:02:49 PDT 2014

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) 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").

>
>
> - 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/8b9582c4/attachment.html>