MSVC interpreting move-only struct argument as a pointer

Question

I have a simple one-member struct with deleted copy construction/assignment, and default move construction/assignment. I'm trying to pass one of these structs to a function by value and return the member - pretty straightforward.

struct NoCopy {
    explicit NoCopy(int x) : x{x} {}

    NoCopy(const NoCopy&) = delete;
    NoCopy& operator=(const NoCopy&) = delete;

    NoCopy(NoCopy&&) = default;
    NoCopy& operator=(NoCopy&&) = default;

    int x;
};

// noinline to ensure the crash is reproducible in release
// not required to reproduce the problem code
__declspec(noinline) int problem_function(NoCopy x) {
    return x.x;
}

int main() {
    return problem_function(NoCopy{ 1 });
}

The problem is that when compiled with MSVC, this function crashes.

Looking at the disassembly, it appears that when the copy constructor is deleted, MSVC attempts to interpret x as if it were a NoCopy* and the subsequent member read causes a segmentation fault.

Here's a godbolt example, with gcc and clang for reference: https://godbolt.org/z/jG7kIw

Note that both gcc and clang behave as expected. Also note that this happens in both optimised and unoptimised builds, and appears to affect both MSVC 2015 and 2017.

For reference, I'm compiling on my machine with Visual Studio Professional 2015 (14.0.25431.01 Update 3) - and I'm predominantly testing x64 builds. My platform toolset for the crash repro is set to v140.

So my question is: are there any reasonable explanations for this, or am I looking at a compiler bug.

edit: I've submitted a bug report over here

edit #2: If like me, you're stuck with a similar problem and are unable to update VS easily - it appears that defining move constructor/assignment operator manually instead of using = default causes MSVC to spit out correct code at the call site and avoids the crash. here's a new godbolt

for this reason, things like std::unique_ptr don't seem to be affected. struct size also seems to be a factor.

Stock

Using this example I don't get a crash. Can you give a fully compileable example that crashes? – Vincevincelette

My compiler won't compile the code when I try to use problem_function. It complains that the code is calling the deleted constructor of NoCopy. – Methacrylate

I'm confused. Doesn't passing by value do a copy by design? Is it ok for the code to compile to begin with? – Denial

@Denial It depends. In C++17, if you use a temporary as the function argument then no (thanks guaranteed copy elision). Pre C++17, then yes. – Vincevincelette

@Denial You can also pass by value with move semantics. – Proffer

How can you pass a NoCopy instance by value if you don't allow copies nor moves? If you pass a variable by value you should provide at least one of these or you should pass it by reference. – Coagulate

@Lorand Move constructor and move assignment operators are = default. – Proffer

@Vincevincelette The reason that example doesn't crash is because you're compiling with GCC, which I mentioned works as intended. I mentioned in the title that the issue was with MSVC. I'm updating the snippet in the post to include a compiling invocation of problem_function – Stock

@Lorand With C++17 guarnateed copy elision problem_function(NoCopy{some_value}); is legal as no copy or move actually happens. x gets directly initialized. – Vincevincelette

@PatrickMonaghan I only used gcc there to give you a link. I have MSVS installed on my machine and using the code in the link I don't get a crash. – Vincevincelette

@Vincevincelette Damn that copy elision, i always forget about it. – Coagulate

That code builds fine on my ms141 compiler. Which compiler version are you using? – Glaswegian

This is probably a compiler bug. I get x was nullptr which doesn't make sense. Note that if you change the value you pass from 0 to something else, it's clear that the implementation is trying to cast that value to a NoCopy*. – Proffer

@Vincevincelette it may be worth noting that I'm compiling for x64. I've also noticed that my updated snippet doesn't demonstrate the crash - but in both cases produces code that attempts to use the passed argument incorrectly. I'll update the sample again once I have a sample that demonstrates the problem in both debug and release – Stock

I switched to vs140 and with std::cout << problem_function(NoCopy{0}); it still works fine. – Glaswegian

@PatrickMonaghan Switching to x64 doesn't change anything for me. I'm using 15.8.7 compiling against /std:c++latest – Vincevincelette

I can reproduce the behavior with the code as it's provided. I'm building x64 debug with VS2015 14.0.25431.01 update 3 with toolkit v140 (default for VS2015). It looks to me like a compiler bug with how mandatory copy elision is implemented. The value given to construct NoCopy is treated as an address cast to NoCopy*. – Proffer

Actually, it seems like the value of the x member is interpreted as a pointer to the NoCopy. This example produces the same error. – Proffer

I've updated the post again with as many details as I could - and I've updated the godbolt as well, just in case. – Stock

it is worth mentioning that the godbolt sample does demonstrate the problem I'm having. MSVC is definitely dereferencing things it shouldn't be, where GCC and Clang do not. – Stock

Thank you for updating the question code with more details. This seems like a legit bug in MSVC's CL.EXE compiler/linker. – Methacrylate

Sounds like the sort of thing that should be submitted as VS feedback, although since this is in vs2015 they may well not care. – Dore

Answer 1

I can't see how this is anything other than an egregious compiler bug. The code is valid.

It does perhaps seem strange that something so fundamental was broken in two MSVS versions already, but if I had to guess it'd be due to relatively new C++17 copy elision support. (Of course I am using the term "support" somewhat loosely in this instance.)

(OP's VS bug raised online here.)

Gelya

I already had pretty strong suspicions that this wasn't in fact correct, but wasn't entirely sure that I wasn't just missing something basic. thanks for the confirmation; I've submitted a bug report – Stock

@PatrickMonaghan Nice one. You could add a link to that report to my answer too. – Gelya

That was the plan but I'm having a bit of trouble finding a link for it. Will link it back as soon as I figure out MS' developer community :T – Stock

FWIW I can't see that you've posted it there but, then again, I don't really know what I'm doing either. – Gelya

I'm starting to suspect that it didn't actually submit - even though I definitely clicked a button that said submit, and even though I was definitely thanked for my feedback. If I open the report dialog again I see a notice telling me to "upgrade visual studio to submit a report" - which is not actually possible at the moment for reasons. just going to keep trying I guess – Stock

figured it out. the report is over at https://developercommunity.visualstudio.com/content/problem/373332/incorrect-code-generation-for-move-only-types-pass.html – Stock

and yes I agree - I've wasted a tonne of time on this and I'm not exactly pleased about it – Stock

The problem seems to be, that the MSVC compiler subtracts 40 from the stack pointer and writes later at offset 48 into the stack. This destroys the data in the stack. – Aerify

@CAF: Outstanding – Gelya