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

Fri Aug 21 11:09:02 PDT 2015

r245711, thanks!


2015-08-21 20:19 GMT+03:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:

> 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.
> >>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>>>
> >>>>
> >>
> >>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150821/403eadc6/attachment.html>