Skip to content

Commit 9957225

Browse files
crtrotttru
crtrott
authored and
tru
committed
[libc++][mdspan] Fix uglification, categorize asserts and move tests
Fixes uglification in mdspan deduction guides, which CI did not test for until recently. The CI modification and mdspan testing overlapped, so mdspan landed with green CI, and the CI modification landed too. Make most assertions in mdspan and its helper classes trigger during a hardened build in order to catch out of bounds access errors. Also moves all mdspan assertions tests from libcxx/test/std to libcxx/test/libcxx. Differential Revision: https://reviews.llvm.org/156181
1 parent ab863b3 commit 9957225

22 files changed

+126
-66
lines changed

‎libcxx/include/__mdspan/extents.h

Lines changed: 38 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -171,11 +171,14 @@ struct __maybe_static_array {
171171
_TStatic __static_val = _StaticValues::__get(__i);
172172
if (__static_val == _DynTag) {
173173
__dyn_vals_[_DynamicIdxMap::__get(__i)] = __values[__i];
174-
}
175-
// Precondition check
176-
else
177-
_LIBCPP_ASSERT_UNCATEGORIZED(__values[__i] == static_cast<_TDynamic>(__static_val),
178-
"extents construction: mismatch of provided arguments with static extents.");
174+
} else
175+
// Not catching this could lead to out of bounds errors later
176+
// e.g. using my_mdspan_t = mdspan<int, extents<int, 10>>; my_mdspan_t = m(new int[5], 5);
177+
// Right-hand-side construction looks ok with allocation and size matching,
178+
// but since (potentially elsewhere defined) my_mdspan_t has static size m now thinks its range is 10 not 5
179+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
180+
__values[__i] == static_cast<_TDynamic>(__static_val),
181+
"extents construction: mismatch of provided arguments with static extents.");
179182
}
180183
}
181184

@@ -187,11 +190,14 @@ struct __maybe_static_array {
187190
_TStatic __static_val = _StaticValues::__get(__i);
188191
if (__static_val == _DynTag) {
189192
__dyn_vals_[_DynamicIdxMap::__get(__i)] = static_cast<_TDynamic>(__vals[__i]);
190-
}
191-
// Precondition check
192-
else
193-
_LIBCPP_ASSERT_UNCATEGORIZED(static_cast<_TDynamic>(__vals[__i]) == static_cast<_TDynamic>(__static_val),
194-
"extents construction: mismatch of provided arguments with static extents.");
193+
} else
194+
// Not catching this could lead to out of bounds errors later
195+
// e.g. using my_mdspan_t = mdspan<int, extents<int, 10>>; my_mdspan_t = m(new int[N], span<int,1>(&N));
196+
// Right-hand-side construction looks ok with allocation and size matching,
197+
// but since (potentially elsewhere defined) my_mdspan_t has static size m now thinks its range is 10 not N
198+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
199+
static_cast<_TDynamic>(__vals[__i]) == static_cast<_TDynamic>(__static_val),
200+
"extents construction: mismatch of provided arguments with static extents.");
195201
}
196202
}
197203

@@ -310,8 +316,10 @@ class extents {
310316
(sizeof...(_OtherIndexTypes) == __rank_ || sizeof...(_OtherIndexTypes) == __rank_dynamic_))
311317
_LIBCPP_HIDE_FROM_ABI constexpr explicit extents(_OtherIndexTypes... __dynvals) noexcept
312318
: __vals_(static_cast<index_type>(__dynvals)...) {
313-
_LIBCPP_ASSERT_UNCATEGORIZED(__mdspan_detail::__are_representable_as<index_type>(__dynvals...),
314-
"extents ctor: arguments must be representable as index_type and nonnegative");
319+
// Not catching this could lead to out of bounds errors later
320+
// e.g. mdspan m(ptr, dextents<char, 1>(200u)); leads to an extent of -56 on m
321+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__mdspan_detail::__are_representable_as<index_type>(__dynvals...),
322+
"extents ctor: arguments must be representable as index_type and nonnegative");
315323
}
316324

317325
template <class _OtherIndexType, size_t _Size>
@@ -321,8 +329,10 @@ class extents {
321329
explicit(_Size != __rank_dynamic_)
322330
_LIBCPP_HIDE_FROM_ABI constexpr extents(const array<_OtherIndexType, _Size>& __exts) noexcept
323331
: __vals_(span(__exts)) {
324-
_LIBCPP_ASSERT_UNCATEGORIZED(__mdspan_detail::__are_representable_as<index_type>(span(__exts)),
325-
"extents ctor: arguments must be representable as index_type and nonnegative");
332+
// Not catching this could lead to out of bounds errors later
333+
// e.g. mdspan m(ptr, dextents<char, 1>(array<unsigned,1>(200))); leads to an extent of -56 on m
334+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__mdspan_detail::__are_representable_as<index_type>(span(__exts)),
335+
"extents ctor: arguments must be representable as index_type and nonnegative");
326336
}
327337

328338
template <class _OtherIndexType, size_t _Size>
@@ -332,8 +342,11 @@ class extents {
332342
explicit(_Size != __rank_dynamic_)
333343
_LIBCPP_HIDE_FROM_ABI constexpr extents(const span<_OtherIndexType, _Size>& __exts) noexcept
334344
: __vals_(__exts) {
335-
_LIBCPP_ASSERT_UNCATEGORIZED(__mdspan_detail::__are_representable_as<index_type>(__exts),
336-
"extents ctor: arguments must be representable as index_type and nonnegative");
345+
// Not catching this could lead to out of bounds errors later
346+
// e.g. array a{200u}; mdspan<int, dextents<char,1>> m(ptr, extents(span<unsigned,1>(a))); leads to an extent of -56
347+
// on m
348+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__mdspan_detail::__are_representable_as<index_type>(__exts),
349+
"extents ctor: arguments must be representable as index_type and nonnegative");
337350
}
338351

339352
private:
@@ -382,10 +395,16 @@ class extents {
382395
for (size_t __r = 0; __r < rank(); __r++) {
383396
if constexpr (static_cast<make_unsigned_t<index_type>>(numeric_limits<index_type>::max()) <
384397
static_cast<make_unsigned_t<_OtherIndexType>>(numeric_limits<_OtherIndexType>::max())) {
385-
_LIBCPP_ASSERT_UNCATEGORIZED(__mdspan_detail::__is_representable_as<index_type>(__other.extent(__r)),
386-
"extents ctor: arguments must be representable as index_type and nonnegative");
398+
// Not catching this could lead to out of bounds errors later
399+
// e.g. dextents<char,1>> e(dextents<unsigned,1>(200)) leads to an extent of -56 on e
400+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
401+
__mdspan_detail::__is_representable_as<index_type>(__other.extent(__r)),
402+
"extents ctor: arguments must be representable as index_type and nonnegative");
387403
}
388-
_LIBCPP_ASSERT_UNCATEGORIZED(
404+
// Not catching this could lead to out of bounds errors later
405+
// e.g. mdspan<int, extents<int, 10>> m = mdspan<int, dextents<int, 1>>(new int[5], 5);
406+
// Right-hand-side construction was ok, but m now thinks its range is 10 not 5
407+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
389408
(_Values::__static_value(__r) == dynamic_extent) ||
390409
(static_cast<index_type>(__other.extent(__r)) == static_cast<index_type>(_Values::__static_value(__r))),
391410
"extents construction: mismatch of provided arguments with static extents.");

‎libcxx/include/__mdspan/layout_left.h

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,9 @@ class layout_left::mapping {
7575
_LIBCPP_HIDE_FROM_ABI constexpr mapping() noexcept = default;
7676
_LIBCPP_HIDE_FROM_ABI constexpr mapping(const mapping&) noexcept = default;
7777
_LIBCPP_HIDE_FROM_ABI constexpr mapping(const extents_type& __ext) noexcept : __extents_(__ext) {
78-
_LIBCPP_ASSERT_UNCATEGORIZED(
78+
// not catching this could lead to out-of-bounds access later when used inside mdspan
79+
// mapping<dextents<char, 2>> map(dextents<char, 2>(40,40)); map(10, 3) == -126
80+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
7981
__required_span_size_is_representable(__ext),
8082
"layout_left::mapping extents ctor: product of extents must be representable as index_type.");
8183
}
@@ -85,7 +87,9 @@ class layout_left::mapping {
8587
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_OtherExtents, extents_type>)
8688
mapping(const mapping<_OtherExtents>& __other) noexcept
8789
: __extents_(__other.extents()) {
88-
_LIBCPP_ASSERT_UNCATEGORIZED(
90+
// not catching this could lead to out-of-bounds access later when used inside mdspan
91+
// mapping<dextents<char, 2>> map(mapping<dextents<int, 2>>(dextents<int, 2>(40,40))); map(10, 3) == -126
92+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
8993
__mdspan_detail::__is_representable_as<index_type>(__other.required_span_size()),
9094
"layout_left::mapping converting ctor: other.required_span_size() must be representable as index_type.");
9195
}
@@ -95,7 +99,13 @@ class layout_left::mapping {
9599
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_OtherExtents, extents_type>)
96100
mapping(const layout_right::mapping<_OtherExtents>& __other) noexcept
97101
: __extents_(__other.extents()) {
98-
_LIBCPP_ASSERT_UNCATEGORIZED(
102+
// not catching this could lead to out-of-bounds access later when used inside mdspan
103+
// Note: since this is constraint to rank 1, extents itself would catch the invalid conversion first
104+
// and thus this assertion should never be triggered, but keeping it here for consistency
105+
// layout_left::mapping<dextents<char, 1>> map(
106+
// layout_right::mapping<dextents<unsigned, 1>>(dextents<unsigned, 1>(200))); map.extents().extent(0) ==
107+
// -56
108+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
99109
__mdspan_detail::__is_representable_as<index_type>(__other.required_span_size()),
100110
"layout_left::mapping converting ctor: other.required_span_size() must be representable as index_type.");
101111
}
@@ -123,8 +133,12 @@ class layout_left::mapping {
123133
requires((sizeof...(_Indices) == extents_type::rank()) && (is_convertible_v<_Indices, index_type> && ...) &&
124134
(is_nothrow_constructible_v<index_type, _Indices> && ...))
125135
_LIBCPP_HIDE_FROM_ABI constexpr index_type operator()(_Indices... __idx) const noexcept {
126-
_LIBCPP_ASSERT_UNCATEGORIZED(__mdspan_detail::__is_multidimensional_index_in(__extents_, __idx...),
127-
"layout_left::mapping: out of bounds indexing");
136+
// Mappings are generally meant to be used for accessing allocations and are meant to guarantee to never
137+
// return a value exceeding required_span_size(), which is used to know how large an allocation one needs
138+
// Thus, this is a canonical point in multi-dimensional data structures to make invalid element access checks
139+
// However, mdspan does check this on its own, so for now we avoid double checking in hardened mode
140+
_LIBCPP_ASSERT(__mdspan_detail::__is_multidimensional_index_in(__extents_, __idx...),
141+
"layout_left::mapping: out of bounds indexing");
128142
array<index_type, extents_type::rank()> __idx_a{static_cast<index_type>(__idx)...};
129143
return [&]<size_t... _Pos>(index_sequence<_Pos...>) {
130144
index_type __res = 0;
@@ -145,7 +159,10 @@ class layout_left::mapping {
145159
_LIBCPP_HIDE_FROM_ABI constexpr index_type stride(rank_type __r) const noexcept
146160
requires(extents_type::rank() > 0)
147161
{
148-
_LIBCPP_ASSERT_UNCATEGORIZED(__r < extents_type::rank(), "layout_left::mapping::stride(): invalid rank index");
162+
// While it would be caught by extents itself too, using a too large __r
163+
// is functionally an out of bounds access on the stored information needed to compute strides
164+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
165+
__r < extents_type::rank(), "layout_left::mapping::stride(): invalid rank index");
149166
index_type __s = 1;
150167
for (rank_type __i = extents_type::rank() - 1; __i > __r; __i--)
151168
__s *= __extents_.extent(__i);

‎libcxx/include/__mdspan/layout_right.h

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,9 @@ class layout_right::mapping {
7474
_LIBCPP_HIDE_FROM_ABI constexpr mapping() noexcept = default;
7575
_LIBCPP_HIDE_FROM_ABI constexpr mapping(const mapping&) noexcept = default;
7676
_LIBCPP_HIDE_FROM_ABI constexpr mapping(const extents_type& __ext) noexcept : __extents_(__ext) {
77-
_LIBCPP_ASSERT_UNCATEGORIZED(
77+
// not catching this could lead to out-of-bounds access later when used inside mdspan
78+
// mapping<dextents<char, 2>> map(dextents<char, 2>(40,40)); map(3, 10) == -126
79+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
7880
__required_span_size_is_representable(__ext),
7981
"layout_right::mapping extents ctor: product of extents must be representable as index_type.");
8082
}
@@ -84,7 +86,9 @@ class layout_right::mapping {
8486
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_OtherExtents, extents_type>)
8587
mapping(const mapping<_OtherExtents>& __other) noexcept
8688
: __extents_(__other.extents()) {
87-
_LIBCPP_ASSERT_UNCATEGORIZED(
89+
// not catching this could lead to out-of-bounds access later when used inside mdspan
90+
// mapping<dextents<char, 2>> map(mapping<dextents<int, 2>>(dextents<int, 2>(40,40))); map(3, 10) == -126
91+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
8892
__mdspan_detail::__is_representable_as<index_type>(__other.required_span_size()),
8993
"layout_right::mapping converting ctor: other.required_span_size() must be representable as index_type.");
9094
}
@@ -94,7 +98,13 @@ class layout_right::mapping {
9498
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_OtherExtents, extents_type>)
9599
mapping(const layout_left::mapping<_OtherExtents>& __other) noexcept
96100
: __extents_(__other.extents()) {
97-
_LIBCPP_ASSERT_UNCATEGORIZED(
101+
// not catching this could lead to out-of-bounds access later when used inside mdspan
102+
// Note: since this is constraint to rank 1, extents itself would catch the invalid conversion first
103+
// and thus this assertion should never be triggered, but keeping it here for consistency
104+
// layout_right::mapping<dextents<char, 1>> map(
105+
// layout_left::mapping<dextents<unsigned, 1>>(dextents<unsigned, 1>(200))); map.extents().extent(0) ==
106+
// -56
107+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
98108
__mdspan_detail::__is_representable_as<index_type>(__other.required_span_size()),
99109
"layout_right::mapping converting ctor: other.required_span_size() must be representable as index_type.");
100110
}
@@ -122,8 +132,12 @@ class layout_right::mapping {
122132
requires((sizeof...(_Indices) == extents_type::rank()) && (is_convertible_v<_Indices, index_type> && ...) &&
123133
(is_nothrow_constructible_v<index_type, _Indices> && ...))
124134
_LIBCPP_HIDE_FROM_ABI constexpr index_type operator()(_Indices... __idx) const noexcept {
125-
_LIBCPP_ASSERT_UNCATEGORIZED(__mdspan_detail::__is_multidimensional_index_in(__extents_, __idx...),
126-
"layout_right::mapping: out of bounds indexing");
135+
// Mappings are generally meant to be used for accessing allocations and are meant to guarantee to never
136+
// return a value exceeding required_span_size(), which is used to know how large an allocation one needs
137+
// Thus, this is a canonical point in multi-dimensional data structures to make invalid element access checks
138+
// However, mdspan does check this on its own, so for now we avoid double checking in hardened mode
139+
_LIBCPP_ASSERT(__mdspan_detail::__is_multidimensional_index_in(__extents_, __idx...),
140+
"layout_right::mapping: out of bounds indexing");
127141
return [&]<size_t... _Pos>(index_sequence<_Pos...>) {
128142
index_type __res = 0;
129143
((__res = static_cast<index_type>(__idx) + __extents_.extent(_Pos) * __res), ...);
@@ -142,7 +156,10 @@ class layout_right::mapping {
142156
_LIBCPP_HIDE_FROM_ABI constexpr index_type stride(rank_type __r) const noexcept
143157
requires(extents_type::rank() > 0)
144158
{
145-
_LIBCPP_ASSERT_UNCATEGORIZED(__r < extents_type::rank(), "layout_right::mapping::stride(): invalid rank index");
159+
// While it would be caught by extents itself too, using a too large __r
160+
// is functionally an out of bounds access on the stored information needed to compute strides
161+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
162+
__r < extents_type::rank(), "layout_right::mapping::stride(): invalid rank index");
146163
index_type __s = 1;
147164
for (rank_type __i = extents_type::rank() - 1; __i > __r; __i--)
148165
__s *= __extents_.extent(__i);

‎libcxx/include/__mdspan/mdspan.h

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,10 @@ class mdspan {
166166
// always construct its extents() only from the dynamic extents, instead of from the other extents.
167167
if constexpr (rank() > 0) {
168168
for (size_t __r = 0; __r < rank(); __r++) {
169-
_LIBCPP_ASSERT_UNCATEGORIZED(
169+
// Not catching this could lead to out of bounds errors later
170+
// e.g. mdspan<int, dextents<char,1>, non_checking_layout> m =
171+
// mdspan<int, dextents<unsigned, 1>, non_checking_layout>(ptr, 200); leads to an extent of -56 on m
172+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
170173
(static_extent(__r) == dynamic_extent) ||
171174
(static_cast<index_type>(__other.extent(__r)) == static_cast<index_type>(static_extent(__r))),
172175
"mdspan: conversion mismatch of source dynamic extents with static extents");
@@ -185,8 +188,10 @@ class mdspan {
185188
(is_nothrow_constructible_v<index_type, _OtherIndexTypes> && ...) &&
186189
(sizeof...(_OtherIndexTypes) == rank()))
187190
_LIBCPP_HIDE_FROM_ABI constexpr reference operator[](_OtherIndexTypes... __indices) const {
188-
_LIBCPP_ASSERT_UNCATEGORIZED(__mdspan_detail::__is_multidimensional_index_in(extents(), __indices...),
189-
"mdspan: operator[] out of bounds access");
191+
// Note the standard layouts would also check this, but user provided ones may not, so we
192+
// check the precondition here
193+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__mdspan_detail::__is_multidimensional_index_in(extents(), __indices...),
194+
"mdspan: operator[] out of bounds access");
190195
return __acc_.access(__ptr_, __map_(static_cast<index_type>(std::move(__indices))...));
191196
}
192197

@@ -209,6 +214,8 @@ class mdspan {
209214
}
210215

211216
_LIBCPP_HIDE_FROM_ABI constexpr size_type size() const noexcept {
217+
// Could leave this as only checked in debug mode: semantically size() is never
218+
// guaranteed to be related to any accessible range
212219
_LIBCPP_ASSERT_UNCATEGORIZED(
213220
false == ([&]<size_t... _Idxs>(index_sequence<_Idxs...>) {
214221
size_type __prod = 1;
@@ -260,13 +267,13 @@ template <class _ElementType, class... _OtherIndexTypes>
260267
explicit mdspan(_ElementType*, _OtherIndexTypes...)
261268
-> mdspan<_ElementType, dextents<size_t, sizeof...(_OtherIndexTypes)>>;
262269

263-
template <class Pointer>
264-
requires(is_pointer_v<remove_reference_t<Pointer>>)
265-
mdspan(Pointer&&) -> mdspan<remove_pointer_t<remove_reference_t<Pointer>>, extents<size_t>>;
270+
template <class _Pointer>
271+
requires(is_pointer_v<remove_reference_t<_Pointer>>)
272+
mdspan(_Pointer&&) -> mdspan<remove_pointer_t<remove_reference_t<_Pointer>>, extents<size_t>>;
266273

267-
template <class CArray>
268-
requires(is_array_v<CArray> && (rank_v<CArray> == 1))
269-
mdspan(CArray&) -> mdspan<remove_all_extents_t<CArray>, extents<size_t, extent_v<CArray, 0>>>;
274+
template <class _CArray>
275+
requires(is_array_v<_CArray> && (rank_v<_CArray> == 1))
276+
mdspan(_CArray&) -> mdspan<remove_all_extents_t<_CArray>, extents<size_t, extent_v<_CArray, 0>>>;
270277

271278
template <class _ElementType, class _OtherIndexType, size_t _Size>
272279
mdspan(_ElementType*, const array<_OtherIndexType, _Size>&) -> mdspan<_ElementType, dextents<size_t, _Size>>;
@@ -281,16 +288,16 @@ template <class _ElementType, class _OtherIndexType, size_t... _ExtentsPack>
281288
mdspan(_ElementType*, const extents<_OtherIndexType, _ExtentsPack...>&)
282289
-> mdspan<_ElementType, extents<_OtherIndexType, _ExtentsPack...>>;
283290

284-
template <class _ElementType, class MappingType>
285-
mdspan(_ElementType*, const MappingType&)
286-
-> mdspan<_ElementType, typename MappingType::extents_type, typename MappingType::layout_type>;
291+
template <class _ElementType, class _MappingType>
292+
mdspan(_ElementType*, const _MappingType&)
293+
-> mdspan<_ElementType, typename _MappingType::extents_type, typename _MappingType::layout_type>;
287294

288-
template <class MappingType, class AccessorType>
289-
mdspan(const typename AccessorType::data_handle_type, const MappingType&, const AccessorType&)
290-
-> mdspan<typename AccessorType::element_type,
291-
typename MappingType::extents_type,
292-
typename MappingType::layout_type,
293-
AccessorType>;
295+
template <class _MappingType, class _AccessorType>
296+
mdspan(const typename _AccessorType::data_handle_type, const _MappingType&, const _AccessorType&)
297+
-> mdspan<typename _AccessorType::element_type,
298+
typename _MappingType::extents_type,
299+
typename _MappingType::layout_type,
300+
_AccessorType>;
294301

295302
#endif // _LIBCPP_STD_VER >= 23
296303

‎libcxx/test/std/containers/views/mdspan/extents/assert.conversion.pass.cpp renamed to ‎libcxx/test/libcxx/containers/views/mdspan/extents/assert.conversion.pass.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
//===----------------------------------------------------------------------===//
77
// REQUIRES: has-unix-headers
88
// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
9-
// UNSUPPORTED: !libcpp-has-debug-mode
9+
// UNSUPPORTED: !libcpp-has-hardened-mode && !libcpp-has-debug-mode
1010
// XFAIL: availability-verbose_abort-missing
1111

1212
// <mdspan>

0 commit comments

Comments
 (0)