Views can be initialized from rvalues

[Hartmut Kaiser]

We ran into a subtle issue that caused us some head scratching. Here is a short snippet demonstrating the problem:

#include <blaze/Math.h>
#include <iostream>

blaze::DynamicMatrix<double> get_matrix()
{
    return blaze::DynamicMatrix<double>{4, 5, 42.0};
}

int main()
{
    auto rows = blaze::rows(get_matrix(), {1, 3});
    auto columns = blaze::columns(rows, {1, 3});

    std::cout << columns << '\n';

    return 0;
}

If you run this code with all optimizations enabled it will simply crash. The reason for this (unexpected) behavior is that all views in blaze can be initialized from rvalues (here the object returned from get_matrix()). Since blaze views internally store a reference to the argument they were created from, the temporary object (as returned from get_matrix()) will have gone out of scope at the point of access during the output statement, causing the reference to have become invalid.

I would have expected for the code to either fail compiling or to run without crashing.

[Klaus Iglberger]

Hi Hartmut!

Thank a lot for pointing out this issue. The problem is easy to reproduce and we agree that in this particular situation the code should either fail to compile or run without crashing. However, the design decision was deliberate since it is necessary to create views by means of rvalues. The following code snippets give two examples:

blaze::DynamicMatrix<double> A( 4, 5, 42.0 );

auto submatrix = blaze::columns( blaze::rows( A, { 1, 3 } ), { 1, 3 } );  // Selecting a submatrix from matrix A; the call to 'rows()' results in an rvalue

blaze::DynamicMatrix<double> A( 4, 5, 20.0 );
blaze::DynamicMatrix<double> B( 4, 5, 22.0 );

auto rows = blaze::rows( A + B, { 1, 3 } );  // Selecting two rows from a matrix addition; 'A+B' results in an rvalue

However, in contrast to your example, these two examples work flawlessly. The difference is that in both cases the given operands are expression objects, not concrete matrix types. In case of expressions, Blaze will store copies of the operands instead of references.

Being confronted with your example we agree that we have to try to ameliorate the situation. Either it is possible to adapt the code such that it is possible to distinguish between the "good" rvalues and the "bad" rvalues or we will extend the documentation of views to clearly warn about this potential misuse.

Best regards,

Klaus!

[Klaus Iglberger]

Hi Hartmut!

Thanks again for creating this issue. After some days of testing and deliberation we have come to conclude that although the current behavior can lead to lifetime issues we will not change it. We believe that there are cases where this behavior is desirable and perhaps even required. A simple example we encountered during our testing is the use of declval() to evaluate the return type of the rows() function:

using MatrixType = blaze::DynamicMatrix<double>;

using ResultType = decltype( blaze::rows<0>( std::declval<MatrixType>() ) );

Since std::declval() returns an rvalue reference, this would not work anymore if rvalues would lead to a compilation error.

This behavior, which is identical to the way views are handled in the standard (see for instance std::string_view), has already been documented in both the tutorial and the wiki:

... Views represents parts of a vector or matrix, [...]. As such, views act as a reference to specific elements of a vector or matrix. This reference is valid and can be used in every way as any other vector or matrix can be used as long as the referenced vector or matrix is not resized or entirely destroyed. ...

In order to further improve the situation, we have extended the documentation in two ways: First, we have added a paragraph explaining compound views:

... It is also possible to create nested views (compound views), such as for instance bands of submatrices or row selections on column selections. A compound view also acts as reference to specific elements of the underlying vector or matrix and is valid as long as the underlying, referenced vector or matrix is not resized or entirely destroyed. ...

Second, we have added warnings to the documentation of all views. For instance, the following warning has been added to the documentation of subvectors:

Warning: It is the programmer's responsibility to ensure the subvector does not outlive the viewed vector:

// Creating a subvector on a temporary vector; results in a dangling reference!
auto sv = subvector<1UL,3UL>( DynamicVector<int>{ 1, 2, 3, 4, 5 } );

We hope that with these additions the reference character of views is better highlighted and bugs can be avoided. Thanks again for pointing out this issue,

Best regards,

Klaus!

[Hartmut Kaiser]

Dear Klaus,

thanks for your time you took to provide this long explanation. I think I fully understand the issue. However, I still would like to ask you to reconsider this decision. I strongly believe that allowing for views to be initialized from rvalues is a mistake. The problems introduced will lead to bugs that are difficult to isolate.

Allowing to initialize views from non-const and const references should be sufficient for all intents and purposes (for the cases where the argument is not another view - those you can take by value).

See for instance how std::reference_wrapper handles things: std::ref() requires a non-cost reference (ref(T&) {...}, with ref(T&&) = delete), while std::cref() requires a const reference (cref(T const&) {...}, with cref(T const&&) = delete), which prohibits initialization from rvalues in any case.

---

For your specific comment related to std::declval<>():

using MatrixType = blaze::DynamicMatrix<double>; using ResultType = decltype( blaze::rows<0>( std::declval<MatrixType>() ) );

Couldn't you instead write:

using MatrixType = blaze::DynamicMatrix<double>;
using ResultType = decltype( blaze::rows<0>( std::declval<MatrixType&>() ) );

if you need something else than an rvalue?

Thanks again for all your work on Blaze!

Regards Hartmut