Catch2: Corrupt test expressions with MSVC + rvalues

[ccorn]

There is a bug that manifests as failing CHECK(lhs == rhs) where (lhs == rhs) by itself would yield true.
To reproduce, this seems to need the combination of all the following:

1. Catch2, presumably its expression deconstruction,
2. MSVC as of VS2017 in C++17 mode,
3. rvalue-returning reinterpret_cast

In practice I use such constructs on containers with boost::units::quantity elements to get a view of the container with physical units of the elements stripped or changed.

Tested with VS2017, Catch2 v2.7.2 installed via vcpkg.

I have made a demo comprising the attached files:

• CMakeLists.txt:
  For use with CMake and CTest
• test2_catch2.cc.txt:
  Test code, remove the .txt extension before use
• test2_catch2.log:
  CMake build and CTest output. The first line is the command issued in Git Bash.
  $CMAKE_TOOLCHAIN_FILE is the path to vcpkgs scripts/buildsystems/vcpkg.cmake.

I have tried to minimize the demo, except that I added code to also demonstrate versions that do not trigger the bug.

• Needs MSVC: GCC9 works
• Needs Catch2 expression deconstruction: Parenthesizing the test or applying unary + to the rvalue works.
• Needs rvalues to trigger the bug: const lvalues work
• Needs a sufficiently opaque rvalue: a mere std::move is not enough

You might ask: Is Catch2 or MSVC to blame for this?
Well, MSVC seems to have a part in this, but Catch2's expression deconstruction is involved as well.
It makes expressions return different results when inside a CHECK macro. A test framework should not do that.
If you want to support VS2017, you should look into this.

[grisumbras]

The way you use reinterpret_cast results in undefined behaviour. That might be the source of this issue.

[horenmar]

Yeah, as far as I can see, what you are doing there is UB which means that whatever the compiler does, it is within its rights to do it...

[ccorn]

You might want to look up §6.9.2 Compound types:

(4) Two objects a and b are pointer-interconvertible if:

• [...]
• (4.3) one is a standard-layout class object and the other is the first non-static data member of that object, [...]

If two objects are pointer-interconvertible, then they have the same address, and it is possible to obtain a pointer to one from a pointer to the other via a reinterpret_cast.

Ergo: no undefined behavior in this demo.

[grisumbras]

You don't do conversions between pointers. That clause is pretty much unrelated to this code.

[ccorn]

You don't do conversions between pointers

Quoting §8.2.10 Reinterpret cast:

(11) A glvalue expression of type T1 can be cast to the type “reference to T2” if an expression of type “pointer to T1” can be explicitly converted to the type “pointer to T2” using a reinterpret_cast. The result refers to the same object as the source glvalue, but with the specified type. [Note: That is, for lvalues, a reference cast reinterpret_cast<T&>(x) has the same effect as the conversion *reinterpret_cast<T*>(&x) with the built-in & and * operators (and similarly for reinterpret_cast<T&&>(x)). -- end note] No temporary is created, no copy is made, and constructors or conversion functions are not called.

And, to preempt another round of objections: xvalues are both glvalues and rvalues (§6.10).

[grisumbras]

Hmm... Yeah, you are right. Just to be sure, you use reinterpret_cast there to confuse the compiler?

[ccorn]

Just to be sure, you use reinterpret_cast there to confuse the compiler?

It seems that the bug is not triggered if the compiler can track the rvalue, as the std::move example with unwrap2 shows. Therefore the reinterpret_cast. For what concerns Catch2, the issue might simply be something like missing function/operator overloads for rvalues, unrelated to the reinterpret_cast tricks in the demo.

[horenmar]

Well played, I'll admit I didn't know of .11.

[sfranzen]

I tried the example with the current master branch and it will not compile with VS2017 and /WX (treat warnings as errors):

warning C4172: returning address of local variable or temporary

which is emitted for the unwrap function. It is not emitted with VS2019 and all tests pass. I think the VS2017 compiler is wrong, but returning the address of a temporary is UB.

[ccorn]

It is not emitted with VS2019 and all tests pass.

That is good news.

I think the VS2017 compiler is wrong, but returning the address of a temporary is UB.

That's passing the address of an xvalue through a function, to be used within the remainder of the containing full-expression. The semantics are perfectly well defined as demonstrated above. Yes, reinterpret_cast is an indicator for UB, but not a sure one. Returning the address of a temporary is, in fact, very well-defined behavior: The result is usable until the end of the full-expression, in some cases longer. The same compiler warning would have to be issued for std::move and std::forward, and that may be why VS2019 got rid of that warning or improved its checking.

[ccorn]

Returning the address of a temporary is, in fact, very well-defined behavior: The result is usable until the end of the full-expression, in some cases longer.

Edit: If the temporary has been passed in by reference, that is (as is the case here). Stating this here to prevent readers from getting the wrong idea that somehow local variables get their lifetimes extended with a return by reference, which is certainly not the case.

[sfranzen]

Yes, but I meant to say that I don't see where an address is being taken, which is why I think VS2017 is somehow generating code that does, and this leads to non-compliant (and undefined) behaviour. As you quoted earlier, reinterpreting some glvalue x of type T1 as a reference to T2 is legal if reinterpret_cast<T2*>(&x) is legal. Since x in the unwrap function is an lvalue of type wrap<float> and you can reinterpret a wrap<float>* as a float*, it should be legal to reinterpret_cast<float&&>(x), and this should result in all your example equalities being true. That is the way I interpret it and I think you've stumbled on a compiler error that has been fixed in VS2019.

[sfranzen]

On another note, I found that writing the unwrap operation as std::move(reinterpret_cast<T&>(x)) also compiles and passes the test with VS2017.

[ccorn]

On another note, I found that writing the unwrap operation as std::move(reinterpret_cast<T&>(x)) also compiles and passes the test with VS2017.

This is a very helpful observation. Thanks!

[sfranzen]

Also found this bug report against VS2017, which confirms that this was fixed in VS2019.

[horenmar]

So, catching up:

• This was caused by a codegen bug in VS2017
• It is fixed in VS2019
• It can be worked around by modifying the user code slightly, but it is unknown whether it can be mitigated in Catch2

Based on this, I think I am going to close this issue as 3rd party bug.

[JoeyGrajciar]

Closing based on previous comment.