[llvm] r243996 - Avoid passing nullptr to std::equal.

Fri Aug 21 10:19:29 PDT 2015

LGTM too if you also remove the warning from the CodingStandards :).

> On 2015-Aug-21, at 09:10, Aaron Ballman <aaron at aaronballman.com> wrote:
> 
> LGTM! Thank you!
> 
> ~Aaron
> 
> On Fri, Aug 21, 2015 at 5:29 AM, Yaron Keren <yaron.keren at gmail.com> wrote:
>> Is that OK?
>> 
>> 
>> 2015-08-20 22:01 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:
>>> 
>>> On Thu, Aug 20, 2015 at 2:28 PM, Duncan P. N. Exon Smith
>>> <dexonsmith at apple.com> wrote:
>>>> 
>>>>> On 2015-Aug-20, at 10:53, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>>> 
>>>>> On Thu, Aug 20, 2015 at 1:49 PM, Yaron Keren <yaron.keren at gmail.com>
>>>>> wrote:
>>>>>> So, there are three options
>>>>>> 
>>>>>> 1) Go _DEBUG_POINTER_IMPL route for VC2013 only. This will also
>>>>>> disable the
>>>>>> null checks in the other headers where they are beneficial and may
>>>>>> miss real
>>>>>> bugs.
>>>>> 
>>>>> If we limit it to just MSVC 2013, I think this is a reasonable route
>>>>> to go because we can still get reasonable debug testing from MSVC 2015
>>>>> users who build in debug mode. Then, when we drop support for 2013,
>>>>> the check can go away.
>>>>> 
>>>>> ~Aaron
>>>>> 
>>>> 
>>>> If it works, I like this best, but didn't you say that MSVC 2013 calls
>>>> std::memcmp(nullptr) from std::equal()?  IIUC, then the interesting uses
>>>> of std::equal() would invoke UB when compiled with MSVC 2013.  Whether
>>>> or
>>>> not it's a bug in MSVC, we shouldn't introduce UB :(.
>>>> 
>>>> Or did I miss a beat somewhere?
>>> 
>>> Only some of the overloads call std::memcmp(); if we are not using
>>> std::equal() over container elements that are of type char, signed
>>> char, or unsigned char, we should be fine. It seems those are the only
>>> overloads for _Equal() (which is used to implement std::equal()) that
>>> invoke std::memcmp().
>>> 
>>>> 
>>>>>> 2) Replace std::equal with llvm::isEqual or stl::equal or ...
>>>>>> 
>>>>>> 3) Solve the llvm::equal ambiguity using namespace trick while making
>>>>>> peace
>>>>>> with the llvm::equal functor.
>>>> 
>>>> If #1 doesn't work, this is the option I like for the short-term, and
>>>> we can remove the workaround (go back to std::equal()) when we drop
>>>> support for MSVC 2013.
>>> 
>>> I would be fine with this as well if #1 cannot work out for some reason.
>>> 
>>> ~Aaron
>>> 
>>>> 
>>>>>> 2015-08-18 16:13 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:
>>>>>>> 
>>>>>>> On Mon, Aug 17, 2015 at 8:28 PM, Duncan P. N. Exon Smith
>>>>>>> <dexonsmith at apple.com> wrote:
>>>>>>>> 
>>>>>>>>> On 2015-Aug-17, at 16:38, Aaron Ballman <aaron at aaronballman.com>
>>>>>>>>> wrote:
>>>>>>>>> 
>>>>>>>>> On Mon, Aug 17, 2015 at 7:23 PM, Yaron Keren
>>>>>>>>> <yaron.keren at gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>> An  issue that was patched several times in LLVM is
>>>>>>>>>> std::equal(nullptr,
>>>>>>>>>> nullptr, nullptr) where VC asserts on the third nullptr. Earlier
>>>>>>>>>> in
>>>>>>>>>> the
>>>>>>>>>> thread Marshal wrote this should return true and not assert.
>>>>>>>>> 
>>>>>>>>> Yes, I've fixed some myself, IIRC. Marshall's logic makes sense to
>>>>>>>>> me,
>>>>>>>>> and from my reading of the standard, it also seems to fit.
>>>>>>>> 
>>>>>>>> 25.2.11 is pretty clear:
>>>>>>>> 
>>>>>>>>> Returns: true if for every iterator i in the range [first1,last1)
>>>>>>>>> the
>>>>>>>>> following corresponding condi- tions hold: *i == *(first2 + (i -
>>>>>>>>> first1)),
>>>>>>>>> pred(*i, *(first2 + (i - first1))) != false. Otherwise, returns
>>>>>>>>> false.
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Unless you're suggesting that `[nullptr, nullptr)` is an invalid
>>>>>>>> iterator range, or that `nullptr` is an invalid iterator?  But
>>>>>>>> `[nullptr, nullptr)` supports all the necessary operations for an
>>>>>>>> empty iterator range, so how could that be?  (Maybe I'm guessing
>>>>>>>> wrong and this is a straw man... in which case, how could this be
>>>>>>>> a conforming extension?)
>>>>>>> 
>>>>>>> I took longer to read more of the standard (specifically, the parts
>>>>>>> about library requirements in [res.on.arguments] and [iterators]),
>>>>>>> and
>>>>>>> I don't think this is a conforming extension. Then I fired up MSVC
>>>>>>> 2015 and did some experiments, and realized the behavior is different
>>>>>>> in 2015, so I'm not even certain the behavior was intentional in the
>>>>>>> first place.
>>>>>>> 
>>>>>>> In MSVC 2013, equal would check the range and then check the last
>>>>>>> pointer for null, and this check would trigger the assertion.
>>>>>>> In MSVC 2015, equal would check the range, and then check the last
>>>>>>> pointer for null only if the range is not empty, and so no assertion
>>>>>>> is triggered.
>>>>>>> 
>>>>>>> (There is still a degenerate case bug where std::equal(nullptr,
>>>>>>> nullptr, nullptr) refuses to compile that I've reported.)
>>>>>>> 
>>>>>>>>> However,
>>>>>>>>> I'm not convinced this isn't a valid extension that prevents UB
>>>>>>>>> from
>>>>>>>>> calling std::memcmp() with a null pointer value.
>>>>>>>> 
>>>>>>>> `std::equal` isn't defined in terms of `std::memcmp()`, it's defined
>>>>>>>> as above.  How could it be valid to return anything but `true`?  The
>>>>>>>> wording is clear.
>>>>>>>> 
>>>>>>>> But if MSVC is calling `std::memcmp()` directly then we probably
>>>>>>>> don't have a choice but to reimplement `std::equal()` ourselves,
>>>>>>>> correctly :(.
>>>>>>> 
>>>>>>> I think that's true in MSVC 2013, unfortunately. However, I also
>>>>>>> think
>>>>>>> that speaks to a hackish solution limited to just MSVC 2013 being
>>>>>>> somewhat more acceptable because we will be getting rid of 2013
>>>>>>> sometime in the future (next few years).
>>>>>>> 
>>>>>>> ~Aaron
>>>>>>> 
>>>>>>>> 
>>>>>>>>> I think LWG would
>>>>>>>>> need to weigh in on the matter, but I don't know of any issues that
>>>>>>>>> are open on this topic that we could point to for a definitive
>>>>>>>>> answer.
>>>>>>>>> Perhaps Marshall is aware of something, however.
>>>>>>>> 
>>>>>>>> 
>>>>>> 
>>>>>> 
>>>> 
>> 
>> 