[PATCH v2 1/5] libstdc++: Check prerequisites of layout_*::operator().

[Luc Grosheintz]

On 7/3/25 12:45, Jonathan Wakely wrote:
> On Thu, 3 Jul 2025 at 11:12, Tomasz Kaminski <tkaminsk@redhat.com> wrote:
>> On Thu, Jul 3, 2025 at 12:08 PM Luc Grosheintz <luc.grosheintz@gmail.com> wrote:
>>> The reasoning for this approach was:
>>>
>>>     1. The mapping::operator() and mdspan::operator[] have the same
>>>     precondition; and mdspan::operator[] calls mapping::operator().
> 
> Yes, although a user-defined mapping might not bother to check
> preconditions. So in order for us to implement the required check in
> operator[] we really need to check it there.
> 
> We could check it in *both* places, and assume that the compiler will
> see that the second check is entirely redundant.
> 
> We could also check in mapping::operator() and then in
> mdspan::operator[] do something like:
> 
> if constexpr (!__is_std_mapping<mapping_type>)
>    __glibcxx_assert(...);
> 
> So if we know there's a check in mapping::operator() then don't bother
> to check in operator[] as well.

This is an excellent description of how it's currently implemented. We
only skip the check in mdspan::operator[] if we know that the equivalent
is performed in mapping::operator().

>>>     2. The place I chose to check the precondition is where we already
>>>     have both the index and the extent in L1 and almost certainly in a
>>>     register. The hope was that together with branch prediction, this
>>>     will be a reasonably cheap place to put the check.
>>>
>>>     3. The layouts are highly valuable on their own. I've implemented
>>>     that piece of logic numerous times in different contexts; and it's
>>>     wonderful that soon we can convert `i, j, k` to a linear index easily
>>>     using the standard library.
>>>     Therefore, I didn't want to skip them in mapping::operator() because
>>>     they're a guard against out of bounds accesses, e.g. in a user-defined
>>>     dynamically allocated, owning, multi-dimensional array.
>>
>> I think such types would have their own bounds checks, contracts, preconditions.
> 
> Not if it's just something wrapping a unique_ptr<T[]>, for example.

Very valid points. I'd like to add that out of bounds accesses when
iterating over multi-dimensional arrays is a reasonably frequent bug
while developing scientific codes, e.g. write (i, i) instead of (i, j)
or get an N and M mixed up. If unchecked it manifests itself in cryptic
ways; if checked it's usually trivial to fix.

To me checking in mapping::operator() seems like a good idea, I'm sure
it would have caught bugs in code I wrote.

>>>
>>> There's a few paths forwards:
>>>
>>>     1. Remove the check from mapping::operator() and unconditionally
>>>     check in mdspan::operator[].
>>
>> I would go for option 1.
> 
> My original preference was option 2, but I've convinced myself that we
> need the checks in operator[]. Rather than repeat them in both places,
> I think I'm OK with option 1 too.

I'll downgrade them to _GLIBCXX_ASSERT_DEBUG and make the check in
mdspan::operator[] unconditional.

> 
>>>     2. Leave it as is and return when we do optimization or hardening.
>>>
>>>     3. Start measuring to figure out the cost of these checks; and then
>>>     decide.
>>>
>>> I'm open to all three.
>