Enable auto-linking on Windows

[compnerd]

Summary

The MSVC driver and clang do not link against the C++ runtime
explicitly. Instead, they rely on the auto-linking via the pragma
(through use_ansi.h) to link against the correct version of the C++
runtime. Attempt to do something similar here so that linking real C++
code on Windows does not require the user to explicitly specify
c++.lib when using libc++ as a C++ runtime on windows.

[compnerd]

Fix pragma, ensure that we do not try to recursively link.

[smeenai]

include/__config

1266

This feels more like a Windows (or perhaps COFF) thing than a Microsoft ABI thing.

1267

I guess _DLL is appropriate here. Ideally though I think adding the pragma should be keyed on exactly the same conditional that determines whether we annotate with dllexport/dllimport, and right now that's only conditional on _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS.

1269

We never create a c++d.lib, as far as I can see. It's always either c++.lib for the import library or libc++.lib for the static library.

[rnk]

include/__config

1266

I think if you're not using the MS ABI, you're probably using the GCC-style driver to compile and link, and that is what normally adds the C++ library to the link line.

1269

Yeah, I'd leave that out until we actually commit cmake or install scripts that make this library.

[smeenai]

include/__config

1266

Yeah, that's fair.

[compnerd]

include/__config

1266

Well, is it really a windows thing? What about MinGW/cygwin?

The interesting thing here is that if you use the itanium environment, -lc++ is added to the linker invocation (though it is the BFD linker rather than lld). For the MSVC environment, which assumes a MS ABI style C++ library, we do not emit the c++.lib. However, this would accomplish that.

1267

Its more complicated. This is for the *user* not the library itself. When building the shared library we need to ensure that it is not added (!defined(_LIBCPP_BUILDING_LIBRARY)). When using the headers as a user, the _DLL tells you about the dynamic/static behavior.

1269

Okay, I can drop this bit of the diff.

[compnerd]

@rnk/@smeenai don't want future proofing

[smeenai]

I'm fine with future proofing, but not if it doesn't actually work in the present :)

include/__config

1267

I know, but the dllimport annotations are also for the *user*. If you're getting the dllimport annotations, you should also be getting the pragma, and vice-versa.

[compnerd]

include/__config

1267

Right, but _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS does not imply use of a static libc++, but !defined(_DLL) does, unless Im missing something. So doesn't this do exactly what you were thinking of?

[smeenai]

I think it would make sense for this change to also have the conditional for the static C++ library selection. Basically something like

#if defined(_LIBCPP_ABI_MICROSOFT) && !defined(_LIBCPP_BUILDING_LIBRARY)
# if defined(_DLL)
#  pragma comment(lib, "c++.lib")
# else
#  pragma comment(lib, "libc++.lib")
# endif
#endif

If you wanna be super consistent with use_ansi.h, change the second condition to

# if defined(_DLL) && !defined(_STATIC_CPPLIB)

LGTM with that.

Also, the diff needs more context :)

include/__config

1267

What I meant was, this is our current logic for determining whether dllimport annotations are used: https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/include/__config;319549$616-632. In other words, you get the dllimport annotations if _LIBCPP_OBJECT_FORMAT_COFF && !_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS && !_LIBCPP_BUILDING_LIBRARY.

I agree that semantically, _DLL is the right thing to key the pragma on. However, the condition for the pragma and the condition for dllimport annotations is now inconsistent, so we can end up in situations where we get the pragma but not the dllimport annotations, or vice versa, which seems wrong.

Making the conditionals consistent is hairy, however, because _DLL won't be defined for non-MSVC drivers, and the dllimport codepath is required for all of Windows. I'm fine with this as is, and we can think more about that consistency later.

Do you think you also wanna add a pragma for the non-DLL codepath (libc++.lib)? Could do that in a follow-up as well, but it seems natural to have that as part of this change, and it would be more consistent with how use_ansi.h does it.

Looks like msvcprt also has _STATIC_CPPLIB for forcing the use of the static C++ library even when _DLL. Idk if we care about supporting that.

[compnerd]

SVN r319816

[mstorsjo]

I see that @smeenai brought up the inconsistency between _DLL and _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS already back when this was reviewed. I'm running into problems with this inconsistency in various cases.

_DLL is tied to the choice of CRT (compiling with -MD defines it, compiling with -MT doesn't define it), while we might have built and be using either a statically linked or dynamically linked libc++. And as @smeenai argued, if _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS is not defined, then all declarations are decorated with dllimport, and there's no doubt about it, we have to use the DLL form of the library.

This is noticable today when running the libc++ testsuite (which is hooked up in CI these days, so by posting a patch you can get it verified too!); currently the CI configuration builds with both shared and static enabled (which links and uses the shared library only). If disabling building the static library, however, testing fails, as the autolinking tries to pull in the static library.

(We could disable autolinking within the test suite altogether, as it explicitly links the right library anyway, but I'd rather fix the autolinking feature instead of having to disable it because it misbehaves.)

I guess it's possible to set up cases with _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS disabled but still linking against the DLL, that might work at least for some subsets of the library functionality. But if manually enabling _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS in a DLL configuration, then one can also define _LIBCPP_NO_AUTO_LINK at the same time to opt out of the autolinking.

Therefore, I would like to post a patch that fixes the autolinking mechanism to rely on _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS which is closely tied to whether dllimport is used, and to whether a shared or static library was built. (And if both were built, it defaults to the shared one.)

WDYT @smeenai @compnerd?

[smeenai]

In D40660#2686669, @mstorsjo wrote:

I see that @smeenai brought up the inconsistency between _DLL and _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS already back when this was reviewed. I'm running into problems with this inconsistency in various cases.

_DLL is tied to the choice of CRT (compiling with -MD defines it, compiling with -MT doesn't define it), while we might have built and be using either a statically linked or dynamically linked libc++. And as @smeenai argued, if _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS is not defined, then all declarations are decorated with dllimport, and there's no doubt about it, we have to use the DLL form of the library.

This is noticable today when running the libc++ testsuite (which is hooked up in CI these days, so by posting a patch you can get it verified too!); currently the CI configuration builds with both shared and static enabled (which links and uses the shared library only). If disabling building the static library, however, testing fails, as the autolinking tries to pull in the static library.

(We could disable autolinking within the test suite altogether, as it explicitly links the right library anyway, but I'd rather fix the autolinking feature instead of having to disable it because it misbehaves.)

I guess it's possible to set up cases with _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS disabled but still linking against the DLL, that might work at least for some subsets of the library functionality. But if manually enabling _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS in a DLL configuration, then one can also define _LIBCPP_NO_AUTO_LINK at the same time to opt out of the autolinking.

Therefore, I would like to post a patch that fixes the autolinking mechanism to rely on _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS which is closely tied to whether dllimport is used, and to whether a shared or static library was built. (And if both were built, it defaults to the shared one.)

WDYT @smeenai @compnerd?

It's been a long time since I've worked with this, so most of the context has been paged out, but what you're proposing SGTM.

include/__config

 # endif
 #endif // defined(_LIBCPP_HAS_NO_PRAGMA_PUSH_POP_MACRO)
 
+#if defined(_LIBCPP_ABI_MICROSOFT)
+# if defined(_DLL) && !defined(_LIBCPP_BUILDING_LIBRARY)
+#   pragma comment(lib, "c++.lib")
+# endif
+#endif // defined(_LIBCPP_ABI_MICROSOFT)
 
 #endif // __cplusplus