Dangerous implicit conversion from `layout_right_padded` to `layout_right`

[Delgan]

Hello.

I've been bitten a few times when passing the result of submdspan() to a function that expects a standard mdspan.

By default, mdspan uses layout_right, whereas submdspan() may return a layout_right_padded when the underlying data is non-contiguous. This is expected, but the result can be implicitly converted to layout_right, which may lead to undefined behavior if left unnoticed.

#include <mdspan/mdspan.hpp>
#include <vector>

#include <gtest/gtest.h>

TEST(TestImplicitLayoutConversion, conversion_from_right_padded)
{
    std::vector<int> data(3 * 3);
    for (size_t i = 0; i < data.size(); ++i)
        data[i] = int(i);

    // This is a `layout_right` of extent 3x3.
    Kokkos::mdspan<int, Kokkos::dextents<size_t, 2>> good(data.data(), 3, 3);

    // This is a `layout_right_padded` of extent 3x2 (non contiguous).
    auto sub = Kokkos::submdspan(good, Kokkos::full_extent, std::tuple{0, 2});

    // This is a `layout_right` of extent 3x2 (wrongly assumed contiguous).
    Kokkos::mdspan<int, Kokkos::dextents<size_t, 2>> bad = sub;

    // This passes, as expected.
    ASSERT_EQ((MDSPAN_IMPL_OP(bad, 0, 1)), (MDSPAN_IMPL_OP(sub, 0, 1)));

    // This fails: `bad(1, 1)` accesses `data[2*1 + 1]` instead of `data[3*1 + 1]`.
    ASSERT_EQ((MDSPAN_IMPL_OP(bad, 1, 1)), (MDSPAN_IMPL_OP(sub, 1, 1)));
}

The above test exhibits wrong memory access due to the implicit conversion. I'd argue it shouldn't even compile at all.

[nmm0]

Thanks for the report, this does seem undesirable. At the very least this violates layout_left's preconditions, but as you said it shouldn't be implicit.

Just to double check -- are you using C++20 here? We can't do conditional explicit properly before C++20. Looking at the standard, though it does look a bit suspicious and insufficiently constrained on when it allows implicit conversions, but I have to think about this more.

[Delgan]

Thanks for the prompt assessment. Yes, I'm using C++20.

[mhoemmen]

This does look like a correct implementation of the Standard. It's also consistent with layout_left::mapping's "converting constructor" from layout_stride::mapping.

On the other hand, it's not consistent with, say, layout_left_padded::mapping's constructor from layout_left_padded::mapping. That one checks the padding value in its explicit condition.

The constructor invocation in the example violates a precondition. GCC's implementation (in trunk) actually checks this at run time and raises the assertion: https://godbolt.org/z/Tb9h5dGK1 .

If we make the constructor explicit in this case, we should also go through all the other constructors that have preconditions based on strides, like layout_left::mapping<Extents>(layout_stride::mapping<InExtents>), and make them explicit.

[crtrott]

yeah it feels weird that we didn't do that. I think we should be able to get away with fixing this via an LWG issue. Note Mark: if I am not mistaken the layout_stride to layout_left is explicit unless it's rank 0:

Remarks: The expression inside explicit is equivalent to:
!(extents_type::rank() == 0 && is_convertible_v<OtherExtents, extents_type>)

So it's not consistent with that one either.

I think the consistent constraint would be something like:

`!((extents_type::rank() <= 1) || other_static_padding_value==1) && is_convertible_v<OtherExtents, extents_type>)`

I.e. we check for the ranks where it always works, and for the trivial case where the padding value was 1 at compile time.

[mhoemmen]

@crtrott wrote:

Note Mark: if I am not mistaken the layout_stride to layout_left is explicit unless it's rank 0:

Oh, I missed that bit, thank you! That was the resolution of LWG 4272, right?

[Delgan]

Apologies for the confusion in my initial report: I mistakenly wrote layout_left when I meant layout_right (though the issue happens in both cases). I edited the post for clarification to future readers.