Check for _MSC_VER before define _LIBCPP_MSVCRT

[bruno]

Summary

_LIBCPP_MSVCRT is defined because _WIN32 is defined and MINGW32 is not.

Some non-windows targets using MS extensions define _WIN32 for compatibility with Windows but do not have MSVC compatibility. This patch is an attempt to do not have _LIBCPP_MSVCRT defined for such targets, allowing libcxx to build. This patch seems the natural way to go for me, but others more experienced here might have additional suggestions?

[smeenai]

This looks sensible to me. I don't know if there are any scenarios in which you'd be using the Microsoft CRT without having _MSC_VER defined, however. Added some people who may have a better idea of that (@compnerd, @majnemer and @rnk)

include/__config

234–235 ↗

(On Diff #103807)

You can combine this into just

#  if defined(_MSC_VER) && !defined(__MINGW32__)

I don't know if __MINGW32__ and _MSC_VER will ever be compiled simultaneously. (clang never defines _MSC_VER for its MinGW triples, for example.)

[compnerd]

include/__config

234–235 ↗

(On Diff #103807)

What if MinGW is built with clang/c2 and MSVC extensions? I think that the two could be defined together. What about cygwin and clang/c2? I guess we can ignore that since cygwin is not under active development.

I think this really goes back to my idea for an additional flag to indicate the C library in use. We can interpret it from the canonicalized triple that LLVM/clang use.

[majnemer]

include/__config

234–235 ↗

(On Diff #103807)

clang/c2 is dead.

[bcraig]

include/__config

234–235 ↗

(On Diff #103807)

At some point, I would like to see (or will need to introduce) a flag for which Windows C library is in use (so I'm agreeing with / echoing @compnerd). What all options are there right now? There's the Visual Studio C-runtime (multiple versions), there's msvcrt (used by the OS and mingw), there's the ancient crtdll that we shouldn't ever support, and there's the kernel C runtime (I'm probably the only person that cares about that).

I will note that I don't like the name of the macro here. _LIBCPP_MSVCRT implies that msvcrt.dll is being used, when it isn't. I don't think that this patch needs to fix that naming though.

[bruno]

include/__config

234–235 ↗

(On Diff #103807)

Any suggestion on a new name instead of _LIBCPP_MSVCRT for a future patch?

[bruno]

Update patch after reviewer suggestions!

[bcraig]

LGTM, but you should probably get approval from somebody a bit more senior with the project.

include/__config

234–235 ↗

(On Diff #103807)

_LIBCPP_MS_UCRT? (UCRT for universal C-runtime). That's more accurate than MSVCRT, thought it still isn't entirely accurate, as the UCRT is only a subset of the full CRT.

Other brainstormed names...
_LIBCPP_MS_VISUAL_STUDIO_CRT
_LIBCPP_MSVS_CRT
_LIBCPP_MSVC_CRT
_LIBCPP_THE_REAL_MSVC_CRT

[compnerd]

Thinking more about this, on Windows, is there a strong reason to default to a different libc by default on Windows? @bruno would reusing -ffreestanding work for you here? Or is there something else that we can identify about the target environment that can indicate that the MS CRT is unavailable? I think that what is weird to me about this is that this is not about compatibility with Visual Studio but about the underlying libc. It feels like it would be similar in spirit to say that libc++ defaults to libSystem as the underlying libc on Linux.

@bcraig thoughts on _LIBCPP_MS_CRT as the alternate name?

[bcraig]

_LIBCPP_MS_CRT seems fine too.

[bruno]

Thinking more about this, on Windows, is there a strong reason to default to a different libc by default on Windows?

I'm mostly concerned with -fms-extensions + darwin, which doesn't apply to this scenario, maybe someone else knows a better answer here.

@bruno would reusing -ffreestanding work for you here?

I think forcing the use of -ffreestanding with Apple platforms just for this is too restrictive.

Or is there something else that we can identify about the target environment that can indicate that the MS CRT is unavailable? I think that what is weird to me about this is that this is not about compatibility with Visual Studio but about the underlying libc. It feels like it would be similar in spirit to say that libc++ defaults to libSystem as the underlying libc on Linux.

Right, makes total sense. I'm assuming that -fms-extensions for other targets (Linux) will also rely in not using _LIBCPP_MSVCRT, however #ifdefing for other platforms here doens't seem to add much because they usually do not set _MSC_VER anyways. Additional ideas?

[compnerd]

In D34588#802533, @bruno wrote:

Thinking more about this, on Windows, is there a strong reason to default to a different libc by default on Windows?

I'm mostly concerned with -fms-extensions + darwin, which doesn't apply to this scenario, maybe someone else knows a better answer here.

Could something like _WIN32 && _MSC_VER make this better? That would allow us to differentiate between the various environments. It's kinda icky, but, does make sense in a round about way.

@bruno would reusing -ffreestanding work for you here?

I think forcing the use of -ffreestanding with Apple platforms just for this is too restrictive.

Okay, was worth the shot anyways.

Or is there something else that we can identify about the target environment that can indicate that the MS CRT is unavailable? I think that what is weird to me about this is that this is not about compatibility with Visual Studio but about the underlying libc. It feels like it would be similar in spirit to say that libc++ defaults to libSystem as the underlying libc on Linux.

Right, makes total sense. I'm assuming that -fms-extensions for other targets (Linux) will also rely in not using _LIBCPP_MSVCRT, however #ifdefing for other platforms here doens't seem to add much because they usually do not set _MSC_VER anyways. Additional ideas?

Another bad idea: -U_MSC_VER. The undefine should come after the preprocessor is initialized and should have the same effect as not having it defined.

[rnk]

Some non-windows targets using MS extensions define _WIN32 for compatibility with Windows but do not have MSVC compatibility.

So, these hypothetical targets have the Win32 API available, but they do not use the MSVC CRT, correct? They could use musl, or some custom C runtime, but they don't want libc++ to aim for MSVCRT compatibility.

---

In any case, checking _MSC_VER is the best thing I can think of for guessing if the MSVC CRT is going to be present, so this looks good.

[bruno]

Could something like _WIN32 && _MSC_VER make this better? That would allow us to differentiate between the various environments. It's kinda icky, but, does make sense in a round about way.

The check in the patch was already under a #if defined(_WIN32), having to repeat that again seemed not worth the effort!

Another bad idea: -U_MSC_VER. The undefine should come after the preprocessor is initialized and should have the same effect as not having it defined.

That's a bit non canonical :-)

In D34588#808076, @rnk wrote:

Some non-windows targets using MS extensions define _WIN32 for compatibility with Windows but do not have MSVC compatibility.

So, these hypothetical targets have the Win32 API available, but they do not use the MSVC CRT, correct?

Right.

Thanks for the review! Committed r308225.

libcxx/trunk/include/__config

 
 #if defined(_WIN32)
 #  define _LIBCPP_WIN32API
 #  define _LIBCPP_LITTLE_ENDIAN 1
 #  define _LIBCPP_BIG_ENDIAN    0
 #  define _LIBCPP_SHORT_WCHAR   1
 // Both MinGW and native MSVC provide a "MSVC"-like enviroment
 #  define _LIBCPP_MSVCRT_LIKE
-// If mingw not explicitly detected, assume using MS C runtime only.
-#  ifndef __MINGW32__
+// If mingw not explicitly detected, assume using MS C runtime only if
+// a MS compatibility version is specified.
+#  if defined(_MSC_VER) && !defined(__MINGW32__)
 #    define _LIBCPP_MSVCRT // Using Microsoft's C Runtime library
 #  endif
 #  if (defined(_M_AMD64) || defined(__x86_64__)) || (defined(_M_ARM) || defined(__arm__))
 #    define _LIBCPP_HAS_BITSCAN64
 #  endif
 # if defined(_LIBCPP_MSVCRT)
 #   define _LIBCPP_HAS_QUICK_EXIT
 # endif