New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Presence of REQUIRE changes value of its argument in MSVC 15.5 x64 Release build #1130

Closed
ComicSansMS opened this Issue Dec 12, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@ComicSansMS

ComicSansMS commented Dec 12, 2017

Consider the following code:

#define CATCH_CONFIG_MAIN
#include <catch.hpp>        // Catch 2.0.1

#include <functional>

using Func = std::function<void()>;

void initial_func() {}

Func g_func;

Func getFunc()
{
    g_func = initial_func;
    return g_func;
}

TEST_CASE("Blub")
{
    auto const handler = getFunc().target<decltype(&initial_func)>();
    CHECK(*handler == initial_func);
    REQUIRE(handler);
}

When building this on MSVC 15.5 in Debug mode this runs fine. But in Release mode the handler changes its value in the presence of the REQUIRE to a value close to nullptr (0x7 to be exact). Commenting out the REQUIRE makes the check above it pass again.

The problem is reproducible with both x86 and x64 builds (although in x86 it only seems to occur if the REQUIRE happens before the CHECK).

This looks awfully strange to me, but I am currently unable to reproduce the problem without Catch. I am pretty sure that it's a compiler bug, but I would like you to confirm that it's not a problem with Catch first.
If you could help me come up with a minimal reproducer that does not involve Catch, that would also be greatly appreciated.

@riksteri

This comment has been minimized.

riksteri commented Dec 15, 2017

I am able to reproduce this, and it also occurs when I compile on Windows with clang++ 5.0.0. With clang, the REQUIRE(handler); is not required, CHECK(*handler == initial_func); alone is enough to cause this. This does not happen with clang if optimisations are disabled with -O0, levels 1-3 make this occur.

It looks like the memory handler is pointing at gets overwritten. Stepping through the expanded code* with a debugger shows that an AssertionHandler object gets written to where handler points at. The same actually occurs if you omit the CHECK and only use REQUIRE(handler);, but in that case it does not matter because the value pointed at by handler is not used for anything.

It's not difficult to reproduce the overwriting itself in a general case, but in my minimalising attempts it stops occurring as soon as the value of *handler matters - I can only detect the memory changing with a debugger in situations where handler is not dereferenced. In the Catch reproduction, *handler clearly is used but gets trampled anyways: Catch::Decomposer() <= *handler == initial_func

I have been able to reproduce this without the full catch.hpp header, by copying only the relevant bits of Catch, I'll see if I can reduce it further to be a more general example.

*The two macros get expanded to this (in current master branch, details have changed since last release but bits that matter here are the same, and the issue occurs in both):

auto const handler = getFunc().target<decltype(&initial_func)>();
do 
{
	Catch::AssertionHandler catchAssertionHandler(
		"REQUIRE",
		::Catch::SourceLineInfo("X:\\path\\to\\source\\file.cpp", static_cast<std::size_t>(24)),
		"handler",
		Catch::ResultDisposition::Normal); 
	try
	{
		catchAssertionHandler.handleExpr(Catch::Decomposer() <= handler); 
	} 
	catch(...)
	{
		catchAssertionHandler.handleUnexpectedInflightException(); 
	}
	catchAssertionHandler.complete();
} while((void)0, false && static_cast<bool>(!!(handler)));
do
{
	Catch::AssertionHandler catchAssertionHandler(
		"CHECKAA",
		::Catch::SourceLineInfo("X:\\path\\to\\source\\file.cpp", static_cast<std::size_t>(25)),
		"*handler == initial_func",
		Catch::ResultDisposition::ContinueOnFailure);
	try
	{
		catchAssertionHandler.handleExpr(Catch::Decomposer() <= *handler == initial_func);
	}
	catch(...)
	{
		catchAssertionHandler.handleUnexpectedInflightException();
	}
	catchAssertionHandler.complete();
} while((void)0, false && static_cast<bool>(!!(*handler == initial_func)));
@riksteri

This comment has been minimized.

riksteri commented Dec 16, 2017

This is the smallest I've been able to get to. I copied bits of code from Catch to get there. At this point the phenomenon does not occur with clang anymore, but does with MSVC. When iterating this, MSVC occasionally spat out an internal compiler error when trying to build the code.

#include <functional>
#include <iostream>

using Func = std::function<void()>;

void initial_func() {}

Func g_func;

Func getFunc()
{
	g_func = initial_func;
	return g_func;
}

template<typename LhsT>
class ExprLhs {
	LhsT m_lhs;
public:
	explicit ExprLhs(LhsT lhs) : m_lhs(lhs) {}

	template<typename RhsT>
	auto operator == (RhsT const& rhs) -> bool {
		return m_lhs == rhs;
	}
};

template<typename T>
auto UseTheThing(T const& lhs) -> ExprLhs<T const&> {
	return ExprLhs<T const&>{ lhs };
}

int main()
{
	auto const handler = getFunc().target<decltype(&initial_func)>();

	{
		int trampler[22] = {};

		std::cout << "Don't optimize it away: " << trampler[0] << "\n";

		UseTheThing(handler);
	}

	auto result = ExprLhs<decltype(*handler)>(*handler) == initial_func;
	if (!result)
	{
		std::cerr << "ERROR" << "\n";
	}
	else
	{
		std::cout << "All OK!" << "\n";
	}

	return 0;
}
@philsquared

This comment has been minimized.

Collaborator

philsquared commented Jan 2, 2018

Thanks for the additional repro steps @riksteri. That helps a lot, although I'm still not quite sure what to do with this right now.

@ComicSansMS

This comment has been minimized.

ComicSansMS commented Jan 14, 2018

I filed a bug report with Visual Studio.

If they can confirm that it's a problem of the compiler, it might be okay to just close the bug here (although it would be nice if we could find a workaround that will still allow Catch to work correctly in that case).

Big thanks @riksteri for your help with the reproducer!

@ComicSansMS

This comment has been minimized.

ComicSansMS commented Feb 11, 2018

Okay, this was my fault.

The return value returned by std::function::target() points to an internal structure of the enclosing std::function, so when that enclosing object goes out of scope, the value becomes invalid. The obvious solution is to either keep the std::function itself alive until after the check, or to store the dereferenced value instead, which is the C function pointer that I was interested in in the first place.
Thanks for your help!

@philsquared

This comment has been minimized.

Collaborator

philsquared commented Feb 11, 2018

Thanks for reporting back, @ComicSansMS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment