Skip to content

Commit 96acdaf

Browse files
mjjbellmjjbell
committed
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.
1 parent 98fd175 commit 96acdaf

File tree

13 files changed

+153
-62
lines changed

13 files changed

+153
-62
lines changed

‎.travis.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,7 @@ script:
510510
- ./unit_tests/util-tests
511511
- ./unit_tests/server-tests
512512
- ./unit_tests/partitioner-tests
513+
- ./unit_tests/customizer-tests
513514
- |
514515
if [ -z "${ENABLE_SANITIZER}" ] && [ "$TARGET_ARCH" != "i686" ]; then
515516
npm run nodejs-tests

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
- CHANGED: Bundled protozero updated to v1.7.0. [#5858](https://github.com/Project-OSRM/osrm-backend/pull/5858)
77
- Windows:
88
- FIXED: Fix bit-shift overflow in MLD partition step. [#5878](https://github.com/Project-OSRM/osrm-backend/pull/5878)
9+
- FIXED: Fix vector bool permutation in graph contraction step [#5882](https://github.com/Project-OSRM/osrm-backend/pull/5882)
910

1011

1112
# 5.23.0

‎appveyor-build.bat

Lines changed: 39 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,10 @@ ECHO running extractor-tests.exe ...
131131
unit_tests\%Configuration%\extractor-tests.exe
132132
IF %ERRORLEVEL% EQU 1 GOTO ERROR
133133

134+
ECHO running contractor-tests.exe ...
135+
unit_tests\%Configuration%\contractor-tests.exe
136+
IF %ERRORLEVEL% EQU 1 GOTO ERROR
137+
134138
ECHO running engine-tests.exe ...
135139
unit_tests\%Configuration%\engine-tests.exe
136140
IF %ERRORLEVEL% EQU 1 GOTO ERROR
@@ -143,34 +147,41 @@ ECHO running server-tests.exe ...
143147
unit_tests\%Configuration%\server-tests.exe
144148
IF %ERRORLEVEL% EQU 1 GOTO ERROR
145149

146-
::TODO: CH processing sometimes mysteriously hangs, need to find why and enable tests below.
147-
::ECHO running library-tests.exe ...
148-
::SET test_region=monaco
149-
::SET test_region_ch=ch\monaco
150-
::SET test_region_corech=corech\monaco
151-
::SET test_region_mld=mld\monaco
152-
::SET test_osm=%test_region%.osm.pbf
153-
::IF NOT EXIST %test_osm% powershell Invoke-WebRequest http://project-osrm.wolt.com/testing/monaco.osm.pbf -OutFile %test_osm%
154-
::ECHO running %Configuration%\osrm-extract.exe -p ../profiles/car.lua %test_osm%
155-
::%Configuration%\osrm-extract.exe
156-
::%Configuration%\osrm-extract.exe -p ../profiles/car.lua %test_osm%
157-
::MKDIR ch
158-
::XCOPY %test_region%.osrm.* ch\
159-
::XCOPY %test_region%.osrm ch\
160-
::MKDIR corech
161-
::XCOPY %test_region%.osrm.* corech\
162-
::XCOPY %test_region%.osrm corech\
163-
::MKDIR mld
164-
::XCOPY %test_region%.osrm.* mld\
165-
::XCOPY %test_region%.osrm mld\
166-
::%Configuration%\osrm-contract.exe %test_region_ch%.osrm
167-
::%Configuration%\osrm-contract.exe --core 0.8 %test_region_corech%.osrm
168-
::%Configuration%\osrm-partition.exe %test_region_mld%.osrm
169-
::%Configuration%\osrm-customize.exe %test_region_mld%.osrm
170-
::XCOPY /Y ch\*.* ..\test\data\ch\
171-
::XCOPY /Y corech\*.* ..\test\data\corech\
172-
::XCOPY /Y mld\*.* ..\test\data\mld\
173-
::unit_tests\%Configuration%\library-tests.exe
150+
ECHO running partitioner-tests.exe ...
151+
unit_tests\%Configuration%\partitioner-tests.exe
152+
IF %ERRORLEVEL% EQU 1 GOTO ERROR
153+
154+
ECHO running customizer-tests.exe ...
155+
unit_tests\%Configuration%\customizer-tests.exe
156+
IF %ERRORLEVEL% EQU 1 GOTO ERROR
157+
158+
ECHO running library-tests.exe ...
159+
SET test_region=monaco
160+
SET test_region_ch=ch\monaco
161+
SET test_region_corech=corech\monaco
162+
SET test_region_mld=mld\monaco
163+
SET test_osm=%test_region%.osm.pbf
164+
IF NOT EXIST %test_osm% powershell Invoke-WebRequest http://project-osrm.wolt.com/testing/monaco.osm.pbf -OutFile %test_osm%
165+
ECHO running %Configuration%\osrm-extract.exe -p ../profiles/car.lua %test_osm%
166+
%Configuration%\osrm-extract.exe
167+
%Configuration%\osrm-extract.exe -p ../profiles/car.lua %test_osm%
168+
MKDIR ch
169+
XCOPY %test_region%.osrm.* ch\
170+
XCOPY %test_region%.osrm ch\
171+
MKDIR corech
172+
XCOPY %test_region%.osrm.* corech\
173+
XCOPY %test_region%.osrm corech\
174+
MKDIR mld
175+
XCOPY %test_region%.osrm.* mld\
176+
XCOPY %test_region%.osrm mld\
177+
%Configuration%\osrm-contract.exe %test_region_ch%.osrm
178+
%Configuration%\osrm-contract.exe --core 0.8 %test_region_corech%.osrm
179+
%Configuration%\osrm-partition.exe %test_region_mld%.osrm
180+
%Configuration%\osrm-customize.exe %test_region_mld%.osrm
181+
XCOPY /Y ch\*.* ..\test\data\ch\
182+
XCOPY /Y corech\*.* ..\test\data\corech\
183+
XCOPY /Y mld\*.* ..\test\data\mld\
184+
unit_tests\%Configuration%\library-tests.exe
174185

175186
:ERROR
176187
ECHO ~~~~~~~~~~~~~~~~~~~~~~ ERROR %~f0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

‎include/extractor/extraction_helper_functions.hpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,9 @@ inline std::string canonicalizeStringList(std::string strlist, const std::string
127127

128128
// collapse spaces; this is needed in case we expand "; X" => "; X" above
129129
// but also makes sense to do irregardless of the fact - canonicalizing strings.
130-
const auto spaces = [](auto lhs, auto rhs) { return ::isspace(lhs) && ::isspace(rhs); };
130+
const auto spaces = [](unsigned char lhs, unsigned char rhs) {
131+
return ::isspace(lhs) && ::isspace(rhs);
132+
};
131133
auto it = std::unique(begin(strlist), end(strlist), spaces);
132134
strlist.erase(it, end(strlist));
133135

‎include/extractor/raster_source.hpp

100755100644
Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ class RasterGrid
4747
{
4848
xdim = _xdim;
4949
ydim = _ydim;
50-
_data.reserve(ydim * xdim);
50+
_data.resize(ydim * xdim);
5151
BOOST_ASSERT(ydim * xdim <= _data.capacity());
5252

5353
// Construct FileReader
@@ -164,6 +164,7 @@ class RasterCache
164164
// get reference of cache
165165
std::vector<RasterSource> &getLoadedSources() { return LoadedSources; }
166166
std::unordered_map<std::string, int> &getLoadedSourcePaths() { return LoadedSourcePaths; }
167+
167168
private:
168169
// constructor
169170
RasterCache() = default;
@@ -173,7 +174,7 @@ class RasterCache
173174
// the instance
174175
static RasterCache *g_instance;
175176
};
176-
}
177-
}
177+
} // namespace extractor
178+
} // namespace osrm
178179

179180
#endif /* RASTER_SOURCE_HPP */

‎include/partitioner/multi_level_partition.hpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,11 +164,11 @@ template <storage::Ownership Ownership> class MultiLevelPartitionImpl final
164164
// of cell ids efficiently.
165165
inline NodeID GetSentinelNode() const { return partition.size() - 1; }
166166

167-
void SetCellID(LevelID l, NodeID node, std::size_t cell_id)
167+
void SetCellID(LevelID l, NodeID node, CellID cell_id)
168168
{
169169
auto lidx = LevelIDToIndex(l);
170170

171-
auto shifted_id = cell_id << level_data->lidx_to_offset[lidx];
171+
auto shifted_id = static_cast<std::uint64_t>(cell_id) << level_data->lidx_to_offset[lidx];
172172
auto cleared_cell = partition[node] & ~level_data->lidx_to_mask[lidx];
173173
partition[node] = cleared_cell | shifted_id;
174174
}

‎include/server/api/base_parameters_grammar.hpp

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,18 +25,20 @@ namespace
2525
{
2626
namespace ph = boost::phoenix;
2727
namespace qi = boost::spirit::qi;
28-
}
28+
} // namespace
2929

3030
template <typename T, char... Fmt> struct no_trailing_dot_policy : qi::real_policies<T>
3131
{
3232
template <typename Iterator> static bool parse_dot(Iterator &first, Iterator const &last)
3333
{
34-
if (first == last || *first != '.')
34+
auto diff = std::distance(first, last);
35+
if (diff <= 0 || *first != '.')
3536
return false;
3637

3738
static const constexpr char fmt[sizeof...(Fmt)] = {Fmt...};
3839

39-
if (first + sizeof(fmt) < last && std::equal(fmt, fmt + sizeof(fmt), first + 1u))
40+
if (sizeof(fmt) < static_cast<size_t>(diff) &&
41+
std::equal(fmt, fmt + sizeof(fmt), first + 1u))
4042
return false;
4143

4244
++first;
@@ -225,8 +227,8 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar<Iterator, Signature>
225227
qi::symbols<char, engine::Approach> approach_type;
226228
qi::symbols<char, engine::api::BaseParameters::SnappingType> snapping_type;
227229
};
228-
}
229-
}
230-
}
230+
} // namespace api
231+
} // namespace server
232+
} // namespace osrm
231233

232234
#endif

‎include/util/indexed_data.hpp

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ template <typename BlockPolicy, storage::Ownership Ownership>
3636
inline void write(storage::tar::FileWriter &writer,
3737
const std::string &name,
3838
const detail::IndexedDataImpl<BlockPolicy, Ownership> &index_data);
39-
}
39+
} // namespace serialization
4040

4141
template <int N, typename T = std::string> struct VariableGroupBlock
4242
{
@@ -346,7 +346,7 @@ template <typename GroupBlockPolicy, storage::Ownership Ownership> struct Indexe
346346
const GroupBlockPolicy block;
347347
block.ReadRefrencedBlock(blocks[block_idx], internal_idx, first, last);
348348

349-
return adapt(&*first, &*last);
349+
return adapt(first, last);
350350
}
351351

352352
friend void serialization::read<GroupBlockPolicy, Ownership>(storage::tar::FileReader &reader,
@@ -359,30 +359,35 @@ template <typename GroupBlockPolicy, storage::Ownership Ownership> struct Indexe
359359
const IndexedDataImpl &index_data);
360360

361361
private:
362-
template <class T = ResultType>
362+
template <typename Iter, typename T>
363+
using IsValueIterator =
364+
std::enable_if_t<std::is_same<T, typename std::iterator_traits<Iter>::value_type>::value>;
365+
366+
template <typename T = ResultType, typename Iter, typename = IsValueIterator<Iter, ValueType>>
363367
typename std::enable_if<!std::is_same<T, StringView>::value, T>::type
364-
adapt(const ValueType *first, const ValueType *last) const
368+
adapt(const Iter first, const Iter last) const
365369
{
366370
return ResultType(first, last);
367371
}
368372

369-
template <class T = ResultType>
373+
template <typename T = ResultType, typename Iter, typename = IsValueIterator<Iter, ValueType>>
370374
typename std::enable_if<std::is_same<T, StringView>::value, T>::type
371-
adapt(const ValueType *first, const ValueType *last) const
375+
adapt(const Iter first, const Iter last) const
372376
{
373-
return ResultType(first, std::distance(first, last));
377+
auto diff = std::distance(first, last);
378+
return diff == 0 ? ResultType() : ResultType(&*first, diff);
374379
}
375380

376381
template <typename T> using Vector = util::ViewOrVector<T, Ownership>;
377382
Vector<BlockReference> blocks;
378383
Vector<ValueType> values;
379384
};
380-
}
385+
} // namespace detail
381386

382387
template <typename GroupBlockPolicy>
383388
using IndexedData = detail::IndexedDataImpl<GroupBlockPolicy, storage::Ownership::Container>;
384389
template <typename GroupBlockPolicy>
385390
using IndexedDataView = detail::IndexedDataImpl<GroupBlockPolicy, storage::Ownership::View>;
386-
}
387-
}
391+
} // namespace util
392+
} // namespace osrm
388393
#endif // OSRM_INDEXED_DATA_HPP

‎include/util/permutation.hpp

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,22 @@ namespace osrm
1010
namespace util
1111
{
1212

13-
template <typename RandomAccesIterator, typename IndexT>
14-
void inplacePermutation(RandomAccesIterator begin,
15-
RandomAccesIterator end,
13+
namespace permutation_detail
14+
{
15+
template <typename T> static inline void swap(T &a, T &b) { std::swap(a, b); }
16+
17+
static inline void swap(std::vector<bool>::reference a, std::vector<bool>::reference b)
18+
{
19+
std::vector<bool>::swap(a, b);
20+
}
21+
} // namespace permutation_detail
22+
23+
template <typename RandomAccessIterator, typename IndexT>
24+
void inplacePermutation(RandomAccessIterator begin,
25+
RandomAccessIterator end,
1626
const std::vector<IndexT> &old_to_new)
1727
{
28+
1829
std::size_t size = std::distance(begin, end);
1930
BOOST_ASSERT(old_to_new.size() == size);
2031
// we need a little bit auxililary space since we need to mark
@@ -40,10 +51,10 @@ void inplacePermutation(RandomAccesIterator begin,
4051
for (; new_index != index; old_index = new_index, new_index = old_to_new[new_index])
4152
{
4253
was_replaced[old_index] = true;
43-
std::swap(buffer, begin[new_index]);
54+
permutation_detail::swap(buffer, begin[new_index]);
4455
}
4556
was_replaced[old_index] = true;
46-
std::swap(buffer, begin[index]);
57+
permutation_detail::swap(buffer, begin[index]);
4758
}
4859
}
4960

@@ -56,7 +67,7 @@ std::vector<IndexT> orderingToPermutation(const std::vector<IndexT> &ordering)
5667

5768
return permutation;
5869
}
59-
}
60-
}
70+
} // namespace util
71+
} // namespace osrm
6172

6273
#endif

‎unit_tests/common/range_tools.hpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,11 @@
1111
const auto &rhs = {__VA_ARGS__}; \
1212
BOOST_CHECK_EQUAL_COLLECTIONS(lhs.begin(), lhs.end(), rhs.begin(), rhs.end()); \
1313
} while (0)
14-
#define CHECK_EQUAL_COLLECTIONS(lhs, rhs) \
14+
#define CHECK_EQUAL_COLLECTIONS(coll_lhs, coll_rhs) \
1515
do \
1616
{ \
17+
const auto &lhs = coll_lhs; \
18+
const auto &rhs = coll_rhs; \
1719
BOOST_CHECK_EQUAL_COLLECTIONS(lhs.begin(), lhs.end(), rhs.begin(), rhs.end()); \
1820
} while (0)
1921

0 commit comments

Comments
 (0)