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

[Yaron Keren via llvm-commits]

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/18fe75ca/attachment.html>
-------------- next part --------------
Index: cmake/modules/HandleLLVMOptions.cmake
===================================================================
--- cmake/modules/HandleLLVMOptions.cmake	(revision 245678)
+++ cmake/modules/HandleLLVMOptions.cmake	(working copy)
@@ -247,6 +247,12 @@
 endif()
 
 if( MSVC )
+  if( CMAKE_CXX_COMPILER_VERSION VERSION_LESS 19.0 )
+    # For MSVC 2013, disable iterator null pointer checking in debug mode,
+    # especially so std::equal(nullptr, nullptr, nullptr) will not assert.
+    add_llvm_definitions("-D_DEBUG_POINTER_IMPL=")
+  endif()
+  
   include(ChooseMSVCCRT)
 
   if( NOT (${CMAKE_VERSION} VERSION_LESS 2.8.11) )
Index: include/llvm/ADT/ArrayRef.h
===================================================================
--- include/llvm/ADT/ArrayRef.h	(revision 245678)
+++ include/llvm/ADT/ArrayRef.h	(working copy)
@@ -156,8 +156,6 @@
     bool equals(ArrayRef RHS) const {
       if (Length != RHS.Length)
         return false;
-      if (Length == 0)
-        return true;
       return std::equal(begin(), end(), RHS.begin());
     }
 
Index: lib/CodeGen/SelectionDAG/SelectionDAG.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/SelectionDAG.cpp	(revision 245678)
+++ lib/CodeGen/SelectionDAG/SelectionDAG.cpp	(working copy)
@@ -5676,7 +5676,7 @@
          "Update with wrong number of operands");
 
   // If no operands changed just return the input node.
-  if (Ops.empty() || std::equal(Ops.begin(), Ops.end(), N->op_begin()))
+  if (std::equal(Ops.begin(), Ops.end(), N->op_begin()))
     return N;
 
   // See if the modified node already exists.