Check for _MSC_VER before define _LIBCPP_MSVCRT
ClosedPublic

Authored by bruno on Fri, Jun 23, 5:33 PM.

Details

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?

Diff Detail

Repository
rL LLVM
bruno created this revision.Fri, Jun 23, 5:33 PM
smeenai added subscribers: majnemer, compnerd.

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 added inline comments.Sat, Jun 24, 4:10 PM
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 added inline comments.Sat, Jun 24, 4:32 PM
include/__config
234–235 ↗(On Diff #103807)

clang/c2 is dead.

bcraig added inline comments.Sat, Jun 24, 7:06 PM
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 marked an inline comment as done.Mon, Jun 26, 9:28 AM
bruno added inline comments.
include/__config
234–235 ↗(On Diff #103807)

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

bruno updated this revision to Diff 103975.Mon, Jun 26, 9:29 AM

Update patch after reviewer suggestions!

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

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?

_LIBCPP_MS_CRT seems fine too.

bruno added a comment.Fri, Jul 7, 2:16 PM

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?

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 accepted this revision.Thu, Jul 13, 8:30 AM

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.

This revision is now accepted and ready to land.Thu, Jul 13, 8:30 AM
This revision was automatically updated to reflect the committed changes.
bruno added a comment.Mon, Jul 17, 3:03 PM

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.