Add check so end iterator is not dereferenced

[jwlawson]

The current behaviour of get_node in the virtual pointer mapper will
use lower_bound to get the buffer which is not less than the input
pointer. If the pointer mapper has only allocated one buffer, and we are
looking up a pointer offset into that buffer then the call to
lower_bound will return an iterator pointing past the end of the map.
This means we cannot dereference this iterator, as it does not point
into the map, but we still want to decrement this iterator to get the
buffer that the virtual pointer is referencing.

By adding a check for whether the iterator is the end we can short
circuit the iterator dereference, and so avoid this problem without
otherwise changing the behaviour of the pointer mapper.

[Ruyk]

Thanks for the fix, I am surprised we haven't seen this issue before. Could you add a test for this case to ensure we don't break it?

[jwlawson]

The current tests exhibit this behaviour, but silently pass until you turn on debug iterators (eg. using -D_GLIBCXX_DEBUG).

I'm not sure what a more explicit test would look like.

[DuncanMcBain]

@jwlawson is correct - the first time we tried the SDK on Windows after the virtual pointer was added, it failed because Windows debug builds use checked iterators by default.

Unfortunately the _GLIBCXX_DEBUG isn't totally safe - you can't pass containers between TUs that have DEBUG and not DEBUG, so I'm not sure you could reliably create a test that links against a non-_GLIBCXX_DEBUG ComputeCpp. It might work if you do target_definitions() on the tests... The test "offset" fails when built in debug mode. If it definitely doesn't share containers with the library, it should work!

[DuncanMcBain]

@Ruyk how about a compromise: I can push a separate MR that sets -D_GLIBCXX_DEBUG=1 for some of the tests for the virtual pointer, and we merge this as a fix? (It might in general be difficult to "force" the error, since the PointerMapper stuff is fairly opaque, so we can't really do a check inside the test). If nothing else, given that lower_bound() can return end(), we need to check for that before dereferencing. I'd like to get this merged.