[PATCH] Fix DWARFContext::getCompileUnitForOffset().

[David Blaikie]

================
Comment at: lib/DebugInfo/DWARFUnit.h:66
@@ -65,3 +65,3 @@
   UnitType *getUnitForOffset(uint32_t Offset) const {
-    auto *CU = std::lower_bound(this->begin(), this->end(), Offset,
+    auto *CU = std::upper_bound(this->begin(), this->end(), Offset,
                                 UnitOffsetComparator());
----------------
friss wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > friss wrote:
> > > > samsonov wrote:
> > > > > friss wrote:
> > > > > > samsonov wrote:
> > > > > > > You still need to check that Offset you're looking for is inside the CU.
> > > > > > I might be wrong, but as I answered last time you told me that, I think this is not true. The admissible offsets go from 0 to lastUnit->getNextUnitOffset() exclusive. There is no hole in the section.
> > > > > Sorry, I've received both answers at the same time (probably, your first comment was saved as draft but not submitted).
> > > > > 
> > > > > You are right - there are no holes. Please add a comment about it and feel free to submit this.
> > > > Thanks! And sorry, I should have checked that the old review reply got sent out.
> > > > David are you also OK with this? I gathered that maybe you wanted to look more into the implications of this.
> > > Thanks for looping back around/checking.
> > > 
> > > I'm still unclear on a few things here:
> > > 
> > > Do we need all 3 comparison operations? (if we only need one, I won't have to think so hard about the others - if we only need one, which I think we do (specifically the "(unique_ptr<Unit>, unsigned offset)")) - if we don't, please just delete the two unneeded ones in a preliminary cleanup patch to simplify this one.
> > > 
> > > getNextUnitOffset < Offset seems... OK, thinking about it I see that it makes sense. I'm not sure if a comment would be helpful to explain why (I can't think of a simple comment that really helps).
> > > 
> > > Looks fine to me with the optional preliminary cleanup mentioned above.
> > > 
> > > (we should be able to use code coverage to see that the other two comparison functions are never executed... someone maintains a code coverage website for LLVM but I forget the URL)
> > (further comment - if those other two comparisons are required, I think they may be incorrect, specifically I think the RHS should be getOffset in (1) and (3), not "getNextUnitOffset") - I can explain more if it turns out these are going to remain in the code, if they're being deleted then there's no need to discuss them)
> I don't think we need the 3, but it depends on the point of view, and I can't seem to find a definitive reference on the subject.
> 
> The documentation I found states that the functor needs to be convertible to a conversion between the value and the collection contents (the direction of the comparison varies between lower_/upper_bound). I tested with libcxx by only providing a simple function, and it works.
> I also read that the parameter should adhere to the Compare concept. This means among other things that comp(a,a) must return false. With a simple function comparing an offset and a Unit, comp(a,a) is simply illegal.
> 
> I'm quite sure that no implementation would use anything else that a simple comparison function, but this doesn't mean it is right.
Since we're already relying on heterogenous comparisons here, I think it's safe to remove the other two.

As for language lawyering - it looks like the changes proposed here: http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#270 were applied to C++11, where the final wording says:

"The elements e of [first,last) shall be partitioned with respect to the expression ... comp(e, value)."

http://reviews.llvm.org/D5262