Bug when using reinterpret_cast between two similar classes?

Question

I have an array wrapper that I use as a matrix/vector class, and a struct of two floats to represent points.
I don't want to re-define all arithmetic operators again for points, when I already have them available for vectors, so I want to add implicit conversions between them. I use reinterpret_cast, as shown in the snippet below.

template <class T, size_t N>
struct Array {
    T data[N];
    constexpr T &operator[](size_t index) { return data[index]; }
    constexpr const T &operator[](size_t index) const { return data[index]; }
};

template <class T, size_t R, size_t C>
using TMatrix = Array<Array<T, C>, R>;

template <class T, size_t R>
using TColVector = TMatrix<T, R, 1>;

struct Point {
    float x;
    float y;

    constexpr Point(float x, float y) : x{x}, y{y} {}
    constexpr Point(const TColVector<float, 2> &vec) : x{vec[0]}, y{vec[1]} {}

    TColVector<float, 2> &vec() {
        static_assert(sizeof(*this) == sizeof(TColVector<float, 2>));
        return *reinterpret_cast<TColVector<float, 2> *>(this);
    }
    operator TColVector<float, 2> &() { return vec(); }
};

When using the implicit conversion from Point to TColVector<float, 2>, I get incorrect results. Even stranger: the results are correct as long as I print the intermediate results, but incorrect when I comment out the print statements. And it seems to be always correct on gcc 7.3.0 for x86, and sometimes incorrect on gcc 8.3.0 for ARMv7.

This is the function that gave a correct result with the print statements, and an incorrect result when I commented out the print statements:

static float distanceSquared(Point a, Point b) {
    using namespace std;
    // cout << "a = " << a << ", b = " << b << endl;
    auto diff = a.vec() - b.vec(); // Array<T, N> operator-(const Array<T, N> &lhs, const Array<T, N> &rhs)
    // cout << "diff = " << Point(diff) << endl;
    auto result = normsq(diff); // auto normsq(const TColVector<T, C> &colvector) -> decltype(colvector[0] * colvector[0])
    // cout << "normsq(diff) = " << result << endl;
    return result;
}

Am I doing something wrong here?

The solution seems to be this (even though it doesn't work as an lvalue):

TColVector<float, 2> vec() const { return {x, y}; }

I tried to isolate the problem from the rest of my project, but I haven't been able to reproduce it in isolation, so I would like to know if I have to keep on looking for other problems, even though it seems alright for now.

Here's the entire code on GitHub (it doesn't seem to demonstrate the problem in isolation): https://github.com/tttapa/random/blob/master/SO-reinterpret_cast.cpp

Michael Kenzel · Accepted Answer · 2019-05-12T01:37:28.940

3

Your code has undefined behavior. You can't just reinterpret_cast a Point* to an Array<Array<float, 2>, 1>*. The result of this cast alone is potentially unspecified (the reinterpret_cast here invokes [expr.reinterpret.cast]/7 a pointer conversion to void* [expr.static.cast]/4 [conv.ptr]/2 (still fine) followed by a conversion from void* to Array<Array<float, 2>, 1>*, which may be unspecified if the alignment of Point turns out not to be at least as strict as that of Array<Array<float, 2>, 1> [expr.static.cast]/13). Even if the cast itself happens to work out, you're not allowed to dereference the resulting pointer and access the object the resulting lvalue refers to. Doing so would violate the strict aliasing rule [basic.lval]/11 (see, e.g., here and here for more). Your two types may end up having the same memory layout in practice, but they are not pointer-interconvertible [basic.compound]/4. Printing the intermediate results most likely keeps the compiler from performing optimizations based on the undefined behavior there, that's why the problem doesn't manifest then…

You'll have to think of some other solution I'm afraid, e.g, just implementing the necessary operators for Point. Or just return an Array<Array<float, 2>, 1> initialized from your x and y. If only used in expressions, that Array<Array<float, 2>, 1> should typically end up being optimized away anyways (since the relevant parts are all templates here, their definition will be known and the compiler should inline all this stuff). Or make your Point be a column vector

struct Point : TColVector<float, 2> {};

and define some accessor functions to get x(point) and y(point)…

edited May 12 '19 at 01:37

answered May 12 '19 at 00:03

Michael Kenzel

15,508
2
30
39

@LightnessRacesinOrbit you're right, the cast itself is already potentially unspecified if the alignment of `Point` is not at least as strict as that of the other type. In practice, that's unlikely not to be the case, but yes, technically, it might be, so I updated my answer… – Michael Kenzel May 12 '19 at 01:33
expr.static.cast/13 is about roundtrip through `void*`, not an unrelated pointer type. – Lightness Races in Orbit May 12 '19 at 14:36
@LightnessRacesinOrbit Yes, but [expr.reinterpret.cast]/7 defines the result of `reinterpret_cast` of (potentially unrelated) object pointer types to be that of a sequence of two `static_cast`, one to `void*` and one from `void*` to the target type!? – Michael Kenzel May 12 '19 at 15:11
Oh yeah so it does :D – Lightness Races in Orbit May 12 '19 at 15:18
I wonder when C++ abandoned the Spirit of C principle (quoting the authors of C89) "Don't prevent the programmer from doing what needs to be done"? – supercat May 12 '19 at 22:05
@supercat the rules are exactly the same in C and have been since forever… – Michael Kenzel May 12 '19 at 22:20
The language described by K&R has no such restrictions, and compilers have since forever have included options to support an extension waiving such rules. I see nor reason to believe that the authors of the Standard intended that the rules be an obstacle to programmers doing what needs to be done. – supercat May 13 '19 at 06:07
@supercat well, all I can say is that the rules have been like that since ANSI C was first standardized… – Michael Kenzel May 13 '19 at 09:58
@MichaelKenzel: Neither C89, nor any standard since, has made any real effort to fully define a language sufficient to do everything that people had been doing with C, but merely to specify a core language that implementations intended for various purposes would extend as needed to accomplish those purposes. In cases where K&R2 specified a behavior that would be less than ideal for some purposes, the authors of the Standard wanted to allow compilers targeting such purposes to substitute a more useful behavior, but that doesn't mean they didn't intend that commonplace compilers ignore K&R2. – supercat May 13 '19 at 17:09

Bug when using reinterpret_cast between two similar classes?

1 Answers1