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

[Aaron Ballman via llvm-commits]

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

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