Fix osrm-contract, tests, on Windows

As part of graph contraction, node renumbering leads to
in-place permuting of graph state, including boolean vector elements.

std::vector<bool> returns proxy objects when referencing individual
bits. To correctly swap bool elements using MSVC, we need to explicitly
apply std::vector<bool>::swap.

Making this change fixes osrm-contract on Windows.

We also correct failing tests and other undefined behaviours
(mainly iterator access outside boundaries) highlighted by MSVC.

.travis.yml

@@ -510,6 +510,7 @@ script:
   - ./unit_tests/util-tests
   - ./unit_tests/server-tests
   - ./unit_tests/partitioner-tests
+  - ./unit_tests/customizer-tests
   - |
     if [ -z "${ENABLE_SANITIZER}" ] && [ "$TARGET_ARCH" != "i686" ]; then
       npm run nodejs-tests

CHANGELOG.md

@@ -6,6 +6,7 @@
       - CHANGED: Bundled protozero updated to v1.7.0. [#5858](https://github.com/Project-OSRM/osrm-backend/pull/5858)
     - Windows:
       - FIXED: Fix bit-shift overflow in MLD partition step. [#5878](https://github.com/Project-OSRM/osrm-backend/pull/5878)
+      - FIXED: Fix vector bool permutation in graph contraction step [#5882](https://github.com/Project-OSRM/osrm-backend/pull/5882)
 
 
 # 5.23.0

appveyor-build.bat

@@ -131,6 +131,10 @@ ECHO running extractor-tests.exe ...
 unit_tests\%Configuration%\extractor-tests.exe
 IF %ERRORLEVEL% EQU 1 GOTO ERROR
 
+ECHO running contractor-tests.exe ...
+unit_tests\%Configuration%\contractor-tests.exe
+IF %ERRORLEVEL% EQU 1 GOTO ERROR
+
 ECHO running engine-tests.exe ...
 unit_tests\%Configuration%\engine-tests.exe
 IF %ERRORLEVEL% EQU 1 GOTO ERROR
@@ -143,34 +147,41 @@ ECHO running server-tests.exe ...
 unit_tests\%Configuration%\server-tests.exe
 IF %ERRORLEVEL% EQU 1 GOTO ERROR
 
-::TODO: CH processing sometimes mysteriously hangs, need to find why and enable tests below.
+ECHO running partitioner-tests.exe ...
-::ECHO running library-tests.exe ...
+unit_tests\%Configuration%\partitioner-tests.exe
-::SET test_region=monaco
+IF %ERRORLEVEL% EQU 1 GOTO ERROR
-::SET test_region_ch=ch\monaco
+
-::SET test_region_corech=corech\monaco
+ECHO running customizer-tests.exe ...
-::SET test_region_mld=mld\monaco
+unit_tests\%Configuration%\customizer-tests.exe
-::SET test_osm=%test_region%.osm.pbf
+IF %ERRORLEVEL% EQU 1 GOTO ERROR
-::IF NOT EXIST %test_osm% powershell Invoke-WebRequest http://project-osrm.wolt.com/testing/monaco.osm.pbf -OutFile %test_osm%
+
-::ECHO running %Configuration%\osrm-extract.exe -p ../profiles/car.lua %test_osm%
+ECHO running library-tests.exe ...
-::%Configuration%\osrm-extract.exe
+SET test_region=monaco
-::%Configuration%\osrm-extract.exe -p ../profiles/car.lua %test_osm%
+SET test_region_ch=ch\monaco
-::MKDIR ch
+SET test_region_corech=corech\monaco
-::XCOPY %test_region%.osrm.* ch\
+SET test_region_mld=mld\monaco
-::XCOPY %test_region%.osrm ch\
+SET test_osm=%test_region%.osm.pbf
-::MKDIR corech
+IF NOT EXIST %test_osm% powershell Invoke-WebRequest http://project-osrm.wolt.com/testing/monaco.osm.pbf -OutFile %test_osm%
-::XCOPY %test_region%.osrm.* corech\
+ECHO running %Configuration%\osrm-extract.exe -p ../profiles/car.lua %test_osm%
-::XCOPY %test_region%.osrm corech\
+%Configuration%\osrm-extract.exe
-::MKDIR mld
+%Configuration%\osrm-extract.exe -p ../profiles/car.lua %test_osm%
-::XCOPY %test_region%.osrm.* mld\
+MKDIR ch
-::XCOPY %test_region%.osrm mld\
+XCOPY %test_region%.osrm.* ch\
-::%Configuration%\osrm-contract.exe %test_region_ch%.osrm
+XCOPY %test_region%.osrm ch\
-::%Configuration%\osrm-contract.exe --core 0.8 %test_region_corech%.osrm
+MKDIR corech
-::%Configuration%\osrm-partition.exe %test_region_mld%.osrm
+XCOPY %test_region%.osrm.* corech\
-::%Configuration%\osrm-customize.exe %test_region_mld%.osrm
+XCOPY %test_region%.osrm corech\
-::XCOPY /Y ch\*.* ..\test\data\ch\
+MKDIR mld
-::XCOPY /Y corech\*.* ..\test\data\corech\
+XCOPY %test_region%.osrm.* mld\
-::XCOPY /Y mld\*.* ..\test\data\mld\
+XCOPY %test_region%.osrm mld\
-::unit_tests\%Configuration%\library-tests.exe
+%Configuration%\osrm-contract.exe %test_region_ch%.osrm
+%Configuration%\osrm-contract.exe --core 0.8 %test_region_corech%.osrm
+%Configuration%\osrm-partition.exe %test_region_mld%.osrm
+%Configuration%\osrm-customize.exe %test_region_mld%.osrm
+XCOPY /Y ch\*.* ..\test\data\ch\
+XCOPY /Y corech\*.* ..\test\data\corech\
+XCOPY /Y mld\*.* ..\test\data\mld\
+unit_tests\%Configuration%\library-tests.exe
 
 :ERROR
 ECHO ~~~~~~~~~~~~~~~~~~~~~~ ERROR %~f0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

include/extractor/extraction_helper_functions.hpp

@@ -127,7 +127,9 @@ inline std::string canonicalizeStringList(std::string strlist, const std::string
 
     // collapse spaces; this is needed in case we expand "; X" => ";  X" above
     // but also makes sense to do irregardless of the fact - canonicalizing strings.
-    const auto spaces = [](auto lhs, auto rhs) { return ::isspace(lhs) && ::isspace(rhs); };
+    const auto spaces = [](unsigned char lhs, unsigned char rhs) {
+        return ::isspace(lhs) && ::isspace(rhs);
+    };
     auto it = std::unique(begin(strlist), end(strlist), spaces);
     strlist.erase(it, end(strlist));
 

include/extractor/raster_source.hpp

@@ -47,7 +47,7 @@ class RasterGrid
     {
         xdim = _xdim;
         ydim = _ydim;
-        _data.reserve(ydim * xdim);
+        _data.resize(ydim * xdim);
         BOOST_ASSERT(ydim * xdim <= _data.capacity());
 
         // Construct FileReader
@@ -164,6 +164,7 @@ class RasterCache
     // get reference of cache
     std::vector<RasterSource> &getLoadedSources() { return LoadedSources; }
     std::unordered_map<std::string, int> &getLoadedSourcePaths() { return LoadedSourcePaths; }
+
   private:
     // constructor
     RasterCache() = default;
@@ -173,7 +174,7 @@ class RasterCache
     // the instance
     static RasterCache *g_instance;
 };
-}
+} // namespace extractor
-}
+} // namespace osrm
 
 #endif /* RASTER_SOURCE_HPP */

include/partitioner/multi_level_partition.hpp

@@ -164,11 +164,11 @@ template <storage::Ownership Ownership> class MultiLevelPartitionImpl final
     // of cell ids efficiently.
     inline NodeID GetSentinelNode() const { return partition.size() - 1; }
 
-    void SetCellID(LevelID l, NodeID node, std::size_t cell_id)
+    void SetCellID(LevelID l, NodeID node, CellID cell_id)
     {
         auto lidx = LevelIDToIndex(l);
 
-        auto shifted_id = cell_id << level_data->lidx_to_offset[lidx];
+        auto shifted_id = static_cast<std::uint64_t>(cell_id) << level_data->lidx_to_offset[lidx];
         auto cleared_cell = partition[node] & ~level_data->lidx_to_mask[lidx];
         partition[node] = cleared_cell | shifted_id;
     }

include/server/api/base_parameters_grammar.hpp

@@ -25,18 +25,20 @@ namespace
 {
 namespace ph = boost::phoenix;
 namespace qi = boost::spirit::qi;
-}
+} // namespace
 
 template <typename T, char... Fmt> struct no_trailing_dot_policy : qi::real_policies<T>
 {
     template <typename Iterator> static bool parse_dot(Iterator &first, Iterator const &last)
     {
-        if (first == last || *first != '.')
+        auto diff = std::distance(first, last);
+        if (diff <= 0 || *first != '.')
             return false;
 
         static const constexpr char fmt[sizeof...(Fmt)] = {Fmt...};
 
-        if (first + sizeof(fmt) < last && std::equal(fmt, fmt + sizeof(fmt), first + 1u))
+        if (sizeof(fmt) < static_cast<size_t>(diff) &&
+            std::equal(fmt, fmt + sizeof(fmt), first + 1u))
             return false;
 
         ++first;
@@ -225,8 +227,8 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar<Iterator, Signature>
     qi::symbols<char, engine::Approach> approach_type;
     qi::symbols<char, engine::api::BaseParameters::SnappingType> snapping_type;
 };
-}
+} // namespace api
-}
+} // namespace server
-}
+} // namespace osrm
 
 #endif

include/util/indexed_data.hpp

@@ -36,7 +36,7 @@ template <typename BlockPolicy, storage::Ownership Ownership>
 inline void write(storage::tar::FileWriter &writer,
                   const std::string &name,
                   const detail::IndexedDataImpl<BlockPolicy, Ownership> &index_data);
-}
+} // namespace serialization
 
 template <int N, typename T = std::string> struct VariableGroupBlock
 {
@@ -346,7 +346,7 @@ template <typename GroupBlockPolicy, storage::Ownership Ownership> struct Indexe
         const GroupBlockPolicy block;
         block.ReadRefrencedBlock(blocks[block_idx], internal_idx, first, last);
 
-        return adapt(&*first, &*last);
+        return adapt(first, last);
     }
 
     friend void serialization::read<GroupBlockPolicy, Ownership>(storage::tar::FileReader &reader,
@@ -359,30 +359,35 @@ template <typename GroupBlockPolicy, storage::Ownership Ownership> struct Indexe
                                                       const IndexedDataImpl &index_data);
 
   private:
-    template <class T = ResultType>
+    template <typename Iter, typename T>
+    using IsValueIterator =
+        std::enable_if_t<std::is_same<T, typename std::iterator_traits<Iter>::value_type>::value>;
+
+    template <typename T = ResultType, typename Iter, typename = IsValueIterator<Iter, ValueType>>
     typename std::enable_if<!std::is_same<T, StringView>::value, T>::type
-    adapt(const ValueType *first, const ValueType *last) const
+    adapt(const Iter first, const Iter last) const
     {
         return ResultType(first, last);
     }
 
-    template <class T = ResultType>
+    template <typename T = ResultType, typename Iter, typename = IsValueIterator<Iter, ValueType>>
     typename std::enable_if<std::is_same<T, StringView>::value, T>::type
-    adapt(const ValueType *first, const ValueType *last) const
+    adapt(const Iter first, const Iter last) const
     {
-        return ResultType(first, std::distance(first, last));
+        auto diff = std::distance(first, last);
+        return diff == 0 ? ResultType() : ResultType(&*first, diff);
     }
 
     template <typename T> using Vector = util::ViewOrVector<T, Ownership>;
     Vector<BlockReference> blocks;
     Vector<ValueType> values;
 };
-}
+} // namespace detail
 
 template <typename GroupBlockPolicy>
 using IndexedData = detail::IndexedDataImpl<GroupBlockPolicy, storage::Ownership::Container>;
 template <typename GroupBlockPolicy>
 using IndexedDataView = detail::IndexedDataImpl<GroupBlockPolicy, storage::Ownership::View>;
-}
+} // namespace util
-}
+} // namespace osrm
 #endif // OSRM_INDEXED_DATA_HPP

include/util/permutation.hpp

@@ -10,11 +10,22 @@ namespace osrm
 namespace util
 {
 
-template <typename RandomAccesIterator, typename IndexT>
+namespace permutation_detail
-void inplacePermutation(RandomAccesIterator begin,
+{
-                        RandomAccesIterator end,
+template <typename T> static inline void swap(T &a, T &b) { std::swap(a, b); }
+
+static inline void swap(std::vector<bool>::reference a, std::vector<bool>::reference b)
+{
+    std::vector<bool>::swap(a, b);
+}
+} // namespace permutation_detail
+
+template <typename RandomAccessIterator, typename IndexT>
+void inplacePermutation(RandomAccessIterator begin,
+                        RandomAccessIterator end,
                         const std::vector<IndexT> &old_to_new)
 {
+
     std::size_t size = std::distance(begin, end);
     BOOST_ASSERT(old_to_new.size() == size);
     // we need a little bit auxililary space since we need to mark
@@ -40,10 +51,10 @@ void inplacePermutation(RandomAccesIterator begin,
         for (; new_index != index; old_index = new_index, new_index = old_to_new[new_index])
         {
             was_replaced[old_index] = true;
-            std::swap(buffer, begin[new_index]);
+            permutation_detail::swap(buffer, begin[new_index]);
         }
         was_replaced[old_index] = true;
-        std::swap(buffer, begin[index]);
+        permutation_detail::swap(buffer, begin[index]);
     }
 }
 
@@ -56,7 +67,7 @@ std::vector<IndexT> orderingToPermutation(const std::vector<IndexT> &ordering)
 
     return permutation;
 }
-}
+} // namespace util
-}
+} // namespace osrm
 
 #endif

unit_tests/common/range_tools.hpp

@@ -11,9 +11,11 @@
         const auto &rhs = {__VA_ARGS__};                                                           \
         BOOST_CHECK_EQUAL_COLLECTIONS(lhs.begin(), lhs.end(), rhs.begin(), rhs.end());             \
     } while (0)
-#define CHECK_EQUAL_COLLECTIONS(lhs, rhs)                                                          \
+#define CHECK_EQUAL_COLLECTIONS(coll_lhs, coll_rhs)                                                \
     do                                                                                             \
     {                                                                                              \
+        const auto &lhs = coll_lhs;                                                                \
+        const auto &rhs = coll_rhs;                                                                \
         BOOST_CHECK_EQUAL_COLLECTIONS(lhs.begin(), lhs.end(), rhs.begin(), rhs.end());             \
     } while (0)
 

unit_tests/customizer/cell_customization.cpp

@@ -56,7 +56,7 @@ auto makeGraph(const MultiLevelPartition &mlp, const std::vector<MockEdge> &mock
     return partitioner::MultiLevelGraph<EdgeData, osrm::storage::Ownership::Container>(
         mlp, max_id + 1, edges);
 }
-}
+} // namespace
 
 BOOST_AUTO_TEST_SUITE(cell_customization_tests)
 
@@ -154,7 +154,7 @@ BOOST_AUTO_TEST_CASE(four_levels_test)
     };
 
     auto graph = makeGraph(mlp, edges);
-    std::vector<bool> node_filter(true, graph.GetNumberOfNodes());
+    std::vector<bool> node_filter(graph.GetNumberOfNodes(), true);
 
     CellStorage storage(mlp, graph);
     auto metric = storage.MakeMetric();

unit_tests/partitioner/multi_level_partition.cpp

@@ -1,6 +1,9 @@
 #include <boost/numeric/conversion/cast.hpp>
 #include <boost/test/unit_test.hpp>
 
+#include <util/integer_range.hpp>
+#include <util/msb.hpp>
+
 #include "partitioner/multi_level_partition.hpp"
 
 #define CHECK_SIZE_RANGE(range, ref) BOOST_CHECK_EQUAL(range.second - range.first, ref)
@@ -175,4 +178,56 @@ BOOST_AUTO_TEST_CASE(mlp_sorted)
     BOOST_CHECK_EQUAL(mlp.EndChildren(4, 0), 2);
 }
 
+BOOST_AUTO_TEST_CASE(large_cell_number)
+{
+    size_t num_nodes = 256;
+    size_t num_levels = 9;
+    std::vector<std::vector<CellID>> levels(num_levels, std::vector<CellID>(num_nodes));
+    std::vector<uint32_t> levels_to_num_cells(num_levels);
+
+    std::iota(levels[0].begin(), levels[0].end(), 0);
+    levels_to_num_cells[0] = num_nodes;
+
+    for (auto l : util::irange<size_t>(1UL, num_levels))
+    {
+        std::transform(levels[l - 1].begin(), levels[l - 1].end(), levels[l].begin(), [](auto val) {
+            return val / 2;
+        });
+        levels_to_num_cells[l] = levels_to_num_cells[l - 1] / 2;
+    }
+
+    // level 1:             0  1  2  3 ... 252 253 254 255
+    // level 2:             0  0  1  1 ... 126 126 127 127
+    // level 3:             0  0  0  0 ... 63  63  63  63
+    // ...
+    // level 9:             0  0  0  0 ... 0   0   0   0
+    MultiLevelPartition mlp{levels, levels_to_num_cells};
+
+    for (const auto l : util::irange<size_t>(1UL, num_levels + 1))
+    {
+        BOOST_REQUIRE_EQUAL(mlp.GetNumberOfCells(l), num_nodes >> (l - 1));
+        for (const auto n : util::irange<size_t>(0UL, num_nodes))
+        {
+            BOOST_REQUIRE_EQUAL(mlp.GetCell(l, n), levels[l - 1][n]);
+        }
+    }
+
+    for (const auto m : util::irange<size_t>(0UL, num_nodes))
+    {
+        for (const auto n : util::irange(m + 1, num_nodes))
+        {
+            BOOST_REQUIRE_EQUAL(mlp.GetHighestDifferentLevel(m, n), 1 + util::msb(m ^ n));
+        }
+    }
+
+    for (const auto l : util::irange<size_t>(2UL, num_levels + 1))
+    {
+        for (const auto c : util::irange<size_t>(0UL, levels_to_num_cells[l - 1]))
+        {
+            BOOST_REQUIRE_EQUAL(mlp.BeginChildren(l, c), 2 * c);
+            BOOST_REQUIRE_EQUAL(mlp.EndChildren(l, c), 2 * (c + 1));
+        }
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()

unit_tests/util/indexed_data.cpp

@@ -38,8 +38,8 @@ void test_rw(const Offsets &offsets, const Data &data)
 
     for (std::size_t index = 0; index < offsets.size() - 1; ++index)
     {
-        typename IndexedData::ResultType expected_result(&data[offsets[index]],
+        typename IndexedData::ResultType expected_result(data.begin() + offsets[index],
-                                                         &data[offsets[index + 1]]);
+                                                         data.begin() + offsets[index + 1]);
         BOOST_CHECK_EQUAL(expected_result, indexed_data.at(index));
     }
 }