boostorg / mysql

MySQL C++ client based on Boost.Asio
https://www.boost.org/doc/libs/master/libs/mysql
Boost Software License 1.0
252 stars 32 forks source link

MSVC __cplusplus flag crash #139

Closed H1X4Dev closed 1 year ago

H1X4Dev commented 1 year ago

Hello,

When using LivePlusPlus (Hot-Reload library) we have noticed that the __cplusplus flag that is included here will make the compiler compiler create 2 different functions for consumer and producer (not sure if I worded this right).

Logic is basically:

Result: Allocator/Deallocator uses wrong function. Does it work fine without hot-reloading? Yes, because that flag is never defined in first place, so the Application proceeds to call its own thing.

image

While the above image is correct for my application, after hot-reload what will happen is that it will compile with Boost.MySQL __cplusplus flag and cause the program to use a different deallocator resulting in crash.

Before: image

After Reload: image

This function should have never been called from the Application. image

I don't know what is the appropriate fix for this issue, but in my case defining the macro will fix my issues, however note that a typical user will probably never know about it's existence and will just have undefined behavior.

friendlyanon commented 1 year ago

I don't know what is the appropriate fix for this issue

Ideally, MS would finally report the C++ version properly.

In reality: https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-170
This flag was added in Visual Studio 2017 version 15.7, for which the corresponding CMake variable MSVC_VERSION reports a value equal to _MSC_VER, which is 1914.

Drone runs with MSVC 14.1, which I assume to be Visual Studio 2017 version 15.0. I'm not sure what the support policy here is, but using __cplusplus won't work on all supported (or at least tested) platforms.

Also as a curiosity, one MSVC FE dev has mentioned Boost at being liable for not properly handling this issue: https://old.reddit.com/r/cpp/comments/bqsyss/zc_cplusplus_by_default/eo8i389/

H1X4Dev commented 1 year ago

Interestingly enough, the article states:

We’ll continue to require use of the /Zc:__cplusplus switch for all minor versions of MSVC in the 19.xx family.

I also found that there is an issue raised to make this compiler option on by default on CMake.

From seeing Qt issue it seems like they are enforcing users to have it enabled. Perhaps something like

#if defined(_MSC_VER) && _MSC_VER >= 1914 && __cplusplus < 201103L 
# error "Please define /Zc:__cplusplus flag in your application."
#endif

would be great to have to avoid surprises.

anarthal commented 1 year ago

Hi @H1X4Dev,

Thanks for reporting the issue and for the detailed information you provided. IIRC, that's defined because Asio used to base one of its BOOST_ASIO_HAS_XXX on this macro. I think this is no longer the case, and that the definition should be safe to remove. If CIs report that this is the case, I will ship the change in Boost 1.83.

Regards, Ruben.