From 96acdaf0d59185d9ae1233748d3fa27f5ab89a01 Mon Sep 17 00:00:00 2001 From: Michael Bell Date: Sat, 14 Nov 2020 18:02:56 +0000 Subject: [PATCH] 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 returns proxy objects when referencing individual bits. To correctly swap bool elements using MSVC, we need to explicitly apply std::vector::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 | 1 + CHANGELOG.md | 1 + appveyor-build.bat | 67 +++++++++++-------- .../extractor/extraction_helper_functions.hpp | 4 +- include/extractor/raster_source.hpp | 7 +- include/partitioner/multi_level_partition.hpp | 4 +- .../server/api/base_parameters_grammar.hpp | 14 ++-- include/util/indexed_data.hpp | 25 ++++--- include/util/permutation.hpp | 25 +++++-- unit_tests/common/range_tools.hpp | 4 +- unit_tests/customizer/cell_customization.cpp | 4 +- .../partitioner/multi_level_partition.cpp | 55 +++++++++++++++ unit_tests/util/indexed_data.cpp | 4 +- 13 files changed, 153 insertions(+), 62 deletions(-) mode change 100755 => 100644 include/extractor/raster_source.hpp diff --git a/.travis.yml b/.travis.yml index 8a62697ae71..e1b1339e431 100644 --- a/.travis.yml +++ b/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 130c4215336..8003a438c91 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/appveyor-build.bat b/appveyor-build.bat index c24f92382be..37cfd8f07f1 100644 --- a/appveyor-build.bat +++ b/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 library-tests.exe ... -::SET test_region=monaco -::SET test_region_ch=ch\monaco -::SET test_region_corech=corech\monaco -::SET test_region_mld=mld\monaco -::SET test_osm=%test_region%.osm.pbf -::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% -::%Configuration%\osrm-extract.exe -::%Configuration%\osrm-extract.exe -p ../profiles/car.lua %test_osm% -::MKDIR ch -::XCOPY %test_region%.osrm.* ch\ -::XCOPY %test_region%.osrm ch\ -::MKDIR corech -::XCOPY %test_region%.osrm.* corech\ -::XCOPY %test_region%.osrm corech\ -::MKDIR mld -::XCOPY %test_region%.osrm.* mld\ -::XCOPY %test_region%.osrm mld\ -::%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 +ECHO running partitioner-tests.exe ... +unit_tests\%Configuration%\partitioner-tests.exe +IF %ERRORLEVEL% EQU 1 GOTO ERROR + +ECHO running customizer-tests.exe ... +unit_tests\%Configuration%\customizer-tests.exe +IF %ERRORLEVEL% EQU 1 GOTO ERROR + +ECHO running library-tests.exe ... +SET test_region=monaco +SET test_region_ch=ch\monaco +SET test_region_corech=corech\monaco +SET test_region_mld=mld\monaco +SET test_osm=%test_region%.osm.pbf +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% +%Configuration%\osrm-extract.exe +%Configuration%\osrm-extract.exe -p ../profiles/car.lua %test_osm% +MKDIR ch +XCOPY %test_region%.osrm.* ch\ +XCOPY %test_region%.osrm ch\ +MKDIR corech +XCOPY %test_region%.osrm.* corech\ +XCOPY %test_region%.osrm corech\ +MKDIR mld +XCOPY %test_region%.osrm.* mld\ +XCOPY %test_region%.osrm mld\ +%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 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/include/extractor/extraction_helper_functions.hpp b/include/extractor/extraction_helper_functions.hpp index af6ec86de7b..5fb790c9b26 100644 --- a/include/extractor/extraction_helper_functions.hpp +++ b/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)); diff --git a/include/extractor/raster_source.hpp b/include/extractor/raster_source.hpp old mode 100755 new mode 100644 index eebe6297993..45e495f221a --- a/include/extractor/raster_source.hpp +++ b/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 &getLoadedSources() { return LoadedSources; } std::unordered_map &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 */ diff --git a/include/partitioner/multi_level_partition.hpp b/include/partitioner/multi_level_partition.hpp index 8dffe4d04fd..0b61f21d6f2 100644 --- a/include/partitioner/multi_level_partition.hpp +++ b/include/partitioner/multi_level_partition.hpp @@ -164,11 +164,11 @@ template 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(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; } diff --git a/include/server/api/base_parameters_grammar.hpp b/include/server/api/base_parameters_grammar.hpp index 31bc4a71752..7d24f909b7b 100644 --- a/include/server/api/base_parameters_grammar.hpp +++ b/include/server/api/base_parameters_grammar.hpp @@ -25,18 +25,20 @@ namespace { namespace ph = boost::phoenix; namespace qi = boost::spirit::qi; -} +} // namespace template struct no_trailing_dot_policy : qi::real_policies { template 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(diff) && + std::equal(fmt, fmt + sizeof(fmt), first + 1u)) return false; ++first; @@ -225,8 +227,8 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar qi::symbols approach_type; qi::symbols snapping_type; }; -} -} -} +} // namespace api +} // namespace server +} // namespace osrm #endif diff --git a/include/util/indexed_data.hpp b/include/util/indexed_data.hpp index 080311040c1..8df3c350a3d 100644 --- a/include/util/indexed_data.hpp +++ b/include/util/indexed_data.hpp @@ -36,7 +36,7 @@ template inline void write(storage::tar::FileWriter &writer, const std::string &name, const detail::IndexedDataImpl &index_data); -} +} // namespace serialization template struct VariableGroupBlock { @@ -346,7 +346,7 @@ template 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(storage::tar::FileReader &reader, @@ -359,30 +359,35 @@ template struct Indexe const IndexedDataImpl &index_data); private: - template + template + using IsValueIterator = + std::enable_if_t::value_type>::value>; + + template > typename std::enable_if::value, T>::type - adapt(const ValueType *first, const ValueType *last) const + adapt(const Iter first, const Iter last) const { return ResultType(first, last); } - template + template > typename std::enable_if::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 using Vector = util::ViewOrVector; Vector blocks; Vector values; }; -} +} // namespace detail template using IndexedData = detail::IndexedDataImpl; template using IndexedDataView = detail::IndexedDataImpl; -} -} +} // namespace util +} // namespace osrm #endif // OSRM_INDEXED_DATA_HPP diff --git a/include/util/permutation.hpp b/include/util/permutation.hpp index f558b131276..907efc4da02 100644 --- a/include/util/permutation.hpp +++ b/include/util/permutation.hpp @@ -10,11 +10,22 @@ namespace osrm namespace util { -template -void inplacePermutation(RandomAccesIterator begin, - RandomAccesIterator end, +namespace permutation_detail +{ +template static inline void swap(T &a, T &b) { std::swap(a, b); } + +static inline void swap(std::vector::reference a, std::vector::reference b) +{ + std::vector::swap(a, b); +} +} // namespace permutation_detail + +template +void inplacePermutation(RandomAccessIterator begin, + RandomAccessIterator end, const std::vector &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 orderingToPermutation(const std::vector &ordering) return permutation; } -} -} +} // namespace util +} // namespace osrm #endif diff --git a/unit_tests/common/range_tools.hpp b/unit_tests/common/range_tools.hpp index 904ded9c1b9..8626a97103e 100644 --- a/unit_tests/common/range_tools.hpp +++ b/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) diff --git a/unit_tests/customizer/cell_customization.cpp b/unit_tests/customizer/cell_customization.cpp index d5a3f9eb801..58a46828135 100644 --- a/unit_tests/customizer/cell_customization.cpp +++ b/unit_tests/customizer/cell_customization.cpp @@ -56,7 +56,7 @@ auto makeGraph(const MultiLevelPartition &mlp, const std::vector &mock return partitioner::MultiLevelGraph( 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 node_filter(true, graph.GetNumberOfNodes()); + std::vector node_filter(graph.GetNumberOfNodes(), true); CellStorage storage(mlp, graph); auto metric = storage.MakeMetric(); diff --git a/unit_tests/partitioner/multi_level_partition.cpp b/unit_tests/partitioner/multi_level_partition.cpp index e2807a37fdf..d25664525e9 100644 --- a/unit_tests/partitioner/multi_level_partition.cpp +++ b/unit_tests/partitioner/multi_level_partition.cpp @@ -1,6 +1,9 @@ #include #include +#include +#include + #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> levels(num_levels, std::vector(num_nodes)); + std::vector 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(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(1UL, num_levels + 1)) + { + BOOST_REQUIRE_EQUAL(mlp.GetNumberOfCells(l), num_nodes >> (l - 1)); + for (const auto n : util::irange(0UL, num_nodes)) + { + BOOST_REQUIRE_EQUAL(mlp.GetCell(l, n), levels[l - 1][n]); + } + } + + for (const auto m : util::irange(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(2UL, num_levels + 1)) + { + for (const auto c : util::irange(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() diff --git a/unit_tests/util/indexed_data.cpp b/unit_tests/util/indexed_data.cpp index a9001fcb70f..6d9eecbe910 100644 --- a/unit_tests/util/indexed_data.cpp +++ b/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]], - &data[offsets[index + 1]]); + typename IndexedData::ResultType expected_result(data.begin() + offsets[index], + data.begin() + offsets[index + 1]); BOOST_CHECK_EQUAL(expected_result, indexed_data.at(index)); } }