Crash when creating mocks from helper functions (or copying them)

[fschuh]

When creating a mock from a helper function, it will lead to a segmentation fault when compiling in Debug. It works in Release, and I think it could be related to the compiler inlining the function, which doesn't happen in Debug.

I was able to reproduce a crash in Release when copying a mock object, which could be related (though the crash happens in a different spot).

These test cases (all self descriptive) reproduce the issue:

    class ITestAction
    {
    public:
        virtual int DoStuff(int n1, int n2) = 0;
        virtual ~ITestAction() = 0 {};
    };

    Mock<ITestAction> CreateMock()
    {
        Mock<ITestAction> mock;
        When(Method(mock, DoStuff)).AlwaysReturn(10);
        return mock;
    }

    TEST_CASE("Using a helper function to create the mock (crashes in Debug)")
    {
        Mock<ITestAction> mock = CreateMock();

        mock.get().DoStuff(1, 2); // crash happens here

        Verify(Method(mock, DoStuff).Using(1, 2)).Exactly(1);
    }

    TEST_CASE("Copying the mock (crashes in Debug and Release)")
    {
        Mock<ITestAction> mock;
        When(Method(mock, DoStuff)).AlwaysReturn(10);

        Mock<ITestAction> mockCopy = mock;

        mockCopy.get().DoStuff(1, 2);

        Verify(Method(mockCopy, DoStuff).Using(1, 2)).Exactly(1);

       // crash happens at the end of the test, on the destructor
    }

Currently the only safe way to create a mock from a helper function is to pass the mock by reference and modify it inside the function.

FakeIt version 2.2.0 + Catch2, tested on MSVC and Clang compilers on Windows 10.

[malcolmdavey]

Would guess that copy may not be supported but move more likely to work.

What C++ language are you using. It may be that the first one (CreateMock()) may work on C++17/20 due to NRVO. Having a look at some of the implementation, seems like copy construction should currently be deleted at the moment (maybe move should work?)

I'm sure with enough work this could be supported. PR's are welcome.

[fschuh]

I tried C++ 14, 17, and 20, all with the same results.

Good point on NRVO, that explains why it doesn't crash in Release - I ran some tests, and the copy is not happening in Release for the CreateMock() test. In Debug, however, NRVO is not happening and the object is being copied, thus leading to the crash.

So currently the workaround is to really avoid copies. It would be nice if copies were supported, as this is one more non-documented gotcha we need to be aware of when writing cleaner tests or refactoring them.

[malcolmdavey]

Yeah. That would need some work. (Debug was crashing for me too)

Quickest change would be to disable copy/move for now I guess.

Implementation of each object is unfortunately complex, so we need to make sure all the copying happens properly (and or moves)

[malcolmdavey]

Also thinking the difference between debug and release, is that debug is often better at catching problems.

Or it could be differences in NRVO.

Also note the VS 2022 may have differences - and we may be able to control it better (not that this should necessarily be a fix though)

Optional copy/move elision for return of named variable (for simple cases, before VS 17.4)

Occurs under /O2 unless the function has multiple returned symbols with overlapping lifetimes or the type’s copy or move constructor has default arguments.

https://devblogs.microsoft.com/cppblog/improving-copy-and-move-elision/

[FranckRJ]

I'll have a quick look to check if it can be easy to support copies, but I think it won't be easy and I'll just disable copies for mocks. At least they should be able to be moved I guess.

[FranckRJ]

Fixed by #334, now mocks are movable (they can't be copyable as explained in the PR). It will be released in the next version of FakeIt.