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

[Aaron Ballman via llvm-commits]

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. However,
I'm not convinced this isn't a valid extension that prevents UB from
calling std::memcmp() with a null pointer value. 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.

> It is also possible to localize _DEBUG_POINTER_IMPL effect to algorithm only
> by having llvm-algorithm.h:
>
> #define _DEBUG_POINTER_IMPL
> #include <algorithm>
>
> and have every llvm source include llvm-algorithm.h instead of algorithm.

I think that's even more fragile than using a differently-named API.

~Aaron

>
>
> 2015-08-18 2:19 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:
>>
>> On Mon, Aug 17, 2015 at 6:49 PM, Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>> >
>> >> On 2015-Aug-17, at 15:27, Aaron Ballman <aaron at aaronballman.com> wrote:
>> >>
>> >> On Mon, Aug 17, 2015 at 6:18 PM, Yaron Keren <yaron.keren at gmail.com>
>> >> wrote:
>> >>> +aaron, reid
>> >>>
>> >>> Here is a patch disabling the iterator null pointer checking in Visual
>> >>> C++
>> >>> debug mode. This makes std::equal behave as expected instead of
>> >>> asserting on
>> >>> std::equal(nullpt,r nullptr, nullptr), saving an extra check just for
>> >>> Visual
>> >>> C++ debug mode here and there.
>> >>>
>> >>> It should not cause any practical problem debugging as if a null
>> >>> iterator is
>> >>> actually dereferenced (as opposed to compared) the debugger will stop
>> >>> on
>> >>> location.
>> >>>
>> >>> What do you think?
>> >>
>> >> Mildly terrified. :-) This also disables really useful debugging tools
>> >> in other parts of the STL that have definitely found bugs for us in
>> >> the past. A lot of <algorithm>, <istream>, <ostream>, <locale>,
>> >> <memory>, <string>, and <utility> make use of _DEBUG_POINTER, which
>> >> this patch would disable.
>> >>
>> >> I would *strongly* prefer to not disable that debugging functionality,
>> >> especially since this runtime-level debugging has found bugs our own
>> >> tests have not.
>> >>
>> >> ~Aaron
>> >
>> > This _DEBUG_POINTER macro seems to violate the standard in ways that
>> > require weird workarounds for LLVM's uses of the STL.
>>
>> In what way is it violating the standard? (I apologize if this has
>> been answered before, but I didn't see anyone citing the standard,
>> just some reasonable guesses.) If you believe there is a bug with
>> MSVC, has it been reported to Microsoft, and have they weighed in on
>> the topic?
>>
>> ~Aaron
>>
>> > It seems better to me to make MSVC's STL standards-compliant (when
>> > building LLVM) than to duplicate all the STL algorithms inside
>> > `namespace llvm`, which IMO is the best alternative.
>> >
>> > It would be awesome if there were fewer "we don't follow the standard"
>> > gotchas that most of us only find, post-commit, in MSVC buildbots.
>> >
>> >>> 2015-08-17 23:45 GMT+03:00 Duncan P. N. Exon Smith
>> >>> <dexonsmith at apple.com>:
>> >>>>
>> >>>>
>> >>>>> On 2015-Aug-17, at 13:43, Yaron Keren <yaron.keren at gmail.com> wrote:
>> >>>>>
>> >>>>> I'm trying another approach. The non-null validation goes through a
>> >>>>> macro defined to a function which possibly may be predefined to
>> >>>>> nothing.
>> >>>>> This will disable all null checking on iterators in debug mode which
>> >>>>> is
>> >>>>> relevant to additional functions beyond std::equal. In practice it
>> >>>>> should
>> >>>>> not be a problem since if a null iterator is actually dereferenced
>> >>>>> (not only
>> >>>>> compared) then the debugger stops with invalid access anyhow.
>> >>>>
>> >>>> Well, this would be ideal :).
>> >>>
>> >>>
>> >
>
>