This is an archive of the discontinued LLVM Phabricator instance.

Paths

Table of Contentst

-
include/
1/6
__config

Differential D34588

Check for _MSC_VER before define _LIBCPP_MSVCRT
ClosedPublic

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

Download Raw Diff

Details

Reviewers

EricWF
mclow.lists
dexonsmith
bcraig
smeenai
compnerd
majnemer
rnk

Commits

rGe59dd00038a3: Check for _MSC_VER before defining _LIBCPP_MSVCRT
rCXX308225: Check for _MSC_VER before defining _LIBCPP_MSVCRT
rL308225: Check for _MSC_VER before defining _LIBCPP_MSVCRT

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

Event Timeline

bruno created this revision.Jun 23 2017, 5:33 PM

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
232–235	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.)

chapuni added a subscriber: chapuni.Jun 23 2017, 8:26 PM

compnerd added inline comments.Jun 24 2017, 4:10 PM

include/__config
232–235	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.Jun 24 2017, 4:32 PM

include/__config
232–235	clang/c2 is dead.

bcraig added inline comments.Jun 24 2017, 7:06 PM

include/__config
232–235	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.Jun 26 2017, 9:28 AM

bruno added inline comments.

include/__config
232–235	Any suggestion on a new name instead of `_LIBCPP_MSVCRT` for a future patch?

Update patch after reviewer suggestions!

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

include/__config
232–235	_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.

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?

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.

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.Jul 13 2017, 8:30 AM

Closed by commit rL308225: Check for _MSC_VER before defining _LIBCPP_MSVCRT (authored by bruno). · Explain WhyJul 17 2017, 2:53 PM

This revision was automatically updated to reflect the committed changes.

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.

Revision Contents

Path

Size

include/

__config

5 lines

Diff 103975

include/__config

	Show First 20 Lines • Show All 223 Lines • ▼ Show 20 Lines

	#if defined(_WIN32)			#if defined(_WIN32)
	# define _LIBCPP_WIN32API			# define _LIBCPP_WIN32API
	# define _LIBCPP_LITTLE_ENDIAN 1			# define _LIBCPP_LITTLE_ENDIAN 1
	# define _LIBCPP_BIG_ENDIAN 0			# define _LIBCPP_BIG_ENDIAN 0
	# define _LIBCPP_SHORT_WCHAR 1			# define _LIBCPP_SHORT_WCHAR 1
	// Both MinGW and native MSVC provide a "MSVC"-like enviroment			// Both MinGW and native MSVC provide a "MSVC"-like enviroment
	# define _LIBCPP_MSVCRT_LIKE			# define _LIBCPP_MSVCRT_LIKE
	// If mingw not explicitly detected, assume using MS C runtime only.			// If mingw not explicitly detected, assume using MS C runtime only if
	# ifndef __MINGW32__			// a MS compatibility version is specified.
				# if defined(_MSC_VER) && !defined(__MINGW32__)
	# define _LIBCPP_MSVCRT // Using Microsoft's C Runtime library			# define _LIBCPP_MSVCRT // Using Microsoft's C Runtime library
				smeenaiUnsubmitted Done Reply Inline Actions 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.) smeenai: You can combine this into just ``` # if defined(_MSC_VER) && !defined(__MINGW32__) ``` I…
				compnerdUnsubmitted Not Done Reply Inline Actions 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. compnerd: What if MinGW is built with clang/c2 and MSVC extensions? I think that the two could be…
				majnemerUnsubmitted Not Done Reply Inline Actions clang/c2 is dead. majnemer: clang/c2 is dead.
				bcraigUnsubmitted Not Done Reply Inline Actions 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. bcraig: At some point, I would like to see (or will need to introduce) a flag for which Windows C…
				brunoAuthorUnsubmitted Not Done Reply Inline Actions Any suggestion on a new name instead of `_LIBCPP_MSVCRT` for a future patch? bruno: Any suggestion on a new name instead of `_LIBCPP_MSVCRT` for a future patch?
				bcraigUnsubmitted Not Done Reply Inline Actions _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 bcraig: _LIBCPP_MS_UCRT? (UCRT for universal C-runtime). That's more accurate than MSVCRT, thought it…
	# endif			# endif
	# if (defined(_M_AMD64) \|\| defined(__x86_64__)) \|\| (defined(_M_ARM) \|\| defined(__arm__))			# if (defined(_M_AMD64) \|\| defined(__x86_64__)) \|\| (defined(_M_ARM) \|\| defined(__arm__))
	# define _LIBCPP_HAS_BITSCAN64			# define _LIBCPP_HAS_BITSCAN64
	# endif			# endif
	# if defined(_LIBCPP_MSVCRT)			# if defined(_LIBCPP_MSVCRT)
	# define _LIBCPP_HAS_QUICK_EXIT			# define _LIBCPP_HAS_QUICK_EXIT
	# endif			# endif

	▲ Show 20 Lines • Show All 1,007 Lines • Show Last 20 Lines