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

[Aaron Ballman via llvm-commits]

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