Visual Studio 14 Debug assertion failed

[ruslo]

I've tried to run unit-test with Visual Studio 14 2015 generator and got runtime error:

Expression: vector iterator not dereferencable

Last backtrace entry comes from:

json/src/json.hpp

Line 5119 in 0a81353

return *m_it.array_iterator;

which came from:

json/test/unit.cpp

Line 2879 in 0a81353

CHECK(it2 == jarray.end());

[nlohmann]

Hi @ruslo,

unfortunately, I have no experience with MSVC. There have been numerous attempts to have the code work with MSVC before, but it is breaking again and again (see https://ci.appveyor.com/project/nlohmann/json). It would be great if you could explain to me what is wrong in the code or better propose a fix.

Thanks,
Niels

[ruslo]

The last time I've tried to figure out what is happening I've noticed a hairy chain of CHECK macro expanding. It seems that while doing expand there is an access to the iterator which is equal to end() and hence is not dereferencable.

[dariomt]

I've been taking a look at this issue.
It seems that:

1. CHECK macro is very smart and will detect (lhs op rhs), i.e. the lhs, the rhs and the operator in the expression
2. to provide error messages if will "stringify" lhs and rhs trying to call the streaming operator
3. the streaming operator of the json iterators stream their value as seen in

friend std::ostream& operator<<(std::ostream& o, const iterator_wrapper_internal& w)
{
    return o << w.value();
}

4. in the problematic case, both lhs and rhs are end iterators, so trying to dereference them is undefined behavior which is caught by MSVC in Debug mode

To me the problem is 3)
Why would an iterator be Streamable?
I can't see a use case for that!
Iterators are not Streamable, the values they point to could be, but never the iterator.

My proposed resolution is to remove that streaming operator for iterators (which doesn't make sense).

With that, the code compiles and I can see other tests failing for other reasons, which I'll try to investigate.

[dariomt]

Doh! I was not running the tests in the right directory.

Now, after removing operator<< for iterators, I see only two tests failing in Debug mode with VS2015:

$ time _build/Debug/json_unit.exe "*"
{
    "happy": true,
    "pi": 3.141
}
{
  "happy": true,
  "pi": 3.141
}
"foo"
1
true
"foo"
1
true

json_unit.exe is a Catch v1.2.1 host application.
Run with -? for options

-------------------------------------------------------------------------------
concepts
  class iterator
  CopyAssignable
-------------------------------------------------------------------------------
E:\GitProjects\json\test\unit.cpp(9285)
...............................................................................

E:\GitProjects\json\test\unit.cpp(9387): FAILED:
  CHECK( std::is_nothrow_copy_assignable<json::iterator>::value )
with expansion:
  false

E:\GitProjects\json\test\unit.cpp(9388): FAILED:
  CHECK( std::is_nothrow_copy_assignable<json::const_iterator>::value )
with expansion:
  false

===============================================================================
test cases:      29 |      28 passed | 1 failed
assertions: 3341887 | 3341885 passed | 2 failed


real    13m34.997s
user    0m0.000s
sys     0m0.015s

If I understand the message, it seems the json iterators are expected to have nothrow copy and assignment, but for some reason those are not nothrow.
I'm not sure if this is a MSVC bug or something else. I'll try to investigate.

[dariomt]

Pull Request ready!

[dariomt]

All the test pass in Release mode.
That's a surprise! I'm not sure why those two tests pass in Release but fail in Debug...

[gregmarr]

All the test pass in Release mode.
That's a surprise! I'm not sure why those two tests pass in Release but fail in Debug...

The debug builds in Visual Studio have a lot of extra iterator debugging code. The release builds just silently ignore various types of iterator errors.

[dariomt]

I'm talking about CHECK( std::is_nothrow_copy_assignable<json::iterator>::value )
I didn't expect that the nothrow properties of iterators would change between Debug and Release.
It might be that the extra debugging checks of the iterators affect this.

[gregmarr]

json::iterator contains, by default, std::map::iterator and std::vector::iterator. There's probably something in one of those that would allow a copy construction to throw. I think that at least in the past the iterator registered itself with the container, which allowed the container to mark the iterators as invalid when the container was destroyed. This would probably require storing a vector of iterator pointers.

[dariomt]

OK, it seems that json::iterator holds an ArrayType::iterator, which in the test is a std::vector::iterator. In Debug mode the STL iterators are complex enough to not be std::is_nothrow_copy_assignable

I'd suggest removing that check if MSVC in Debug

[gregmarr]

That makes sense to me.

[dariomt]

Another PR ready!

[dariomt]

Hi!
I don't see anyone assigned to this issue.
Any chance to get PR #131 merged?

[dariomt]

In fact I also have the related PR #130 pending review.

[nlohmann]

I merged #130 and #131 - is the issue still present?

[ruslo]

Yes, version from master branch works for me. Though I see that some tests just disabled :) In my opinion not a best way to solve the problems. Anyway seems that it can be closed now.