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

[Aaron Ballman via llvm-commits]

On Mon, Aug 17, 2015 at 7:06 PM, Justin Bogner <mail at justinbogner.com> wrote:
> Aaron Ballman via llvm-commits <llvm-commits at lists.llvm.org> writes:
>> 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.
>
> What kinds of tools are we talking about here? The incorrect checking in
> std::equals case causes us to fail only at runtime, in debug mode, and
> on MSVC, which means it's pretty likely for developers to commit code
> that breaks it, and workarounds to implement llvm:: versions of standard
> functions to work around broken environments have a fairly high cost in
> terms of maintenance and cognitive load. Also, as evidenced by this
> thread, developer time.

I have fixed several bugs over the years because Microsoft's STL has
pointed out areas where we've mucked up use of the STL (most often
it's been with invalidated iterators). I've yet to see one of these
specific instances also be caught by our test suites. Turning off
debugging features that catch real issues means we will waste
considerable developer time in the future.

> Are the checks we're gaining by having this macro defined worth the pain
> we have to endure to get them?

IMO, yes.

FWIW, a quick look at the implementation of std::equal() in MSVC's STL
shows that at least in some circumstance, it calls std::memcmp(),
where passing a nullptr is definitely UB, even when the size is
specified as 0. Are we sure this is a noncomforming extension (with
some citations or an official word from LWG), or is std::memcmp() is
not a valid implementation for std::equal() with no predicate?

~Aaron

>
>> 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
>>
>>>
>>>
>>> 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 :).
>>>
>>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits